Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 12/14] Platform/ARM: Add CadenceQspiNorFlashDeviceLib for NorFlashDxe

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 9235 bytes --]

Hi Sahil,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 09/05/2024 07:25 am, Sahil wrote:
> Hi Pierre, Thanks for reviewing the patchset. Please find my comment 
> inline below.
>
> On Thu, 2 May 2024 at 18:47, PierreGondois via groups.io 
> <http://groups.io> <pierre.gondois=arm.com@groups.io> wrote:
> >
> > Hello Sahil,
> >
> > On 4/23/24 07:56, Sahil Kaushal via groups.io <http://groups.io> wrote:
> > > From: sahil <sahil@arm.com>
> > >
> > > In N1Sdp platform, the SoC is connected to IOFPGA which has a
> > > Cadence Quad SPI (QSPI) controller. This QSPI controller manages
> > > the flash chip device via QSPI bus.
> > >
> > > This patch adds CadenceQspiNorFlashDeviceLib which is used to
> > > manage and access the above configuration.
> > >
> > > Signed-off-by: sahil <sahil@arm.com>
> > > ---
> > > 
> Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.inf 
> |   32 +
> > > 
> Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.h 
>   |   44 +
> > > 
> Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.c 
>   | 1011 ++++++++++++++++++++
> > >   3 files changed, 1087 insertions(+)
> > >
> >
> > [snip]
> >
> > > +
> > > +/**
> > > +  Converts milliseconds into number of ticks of the performance 
> counter.
> > > +
> > > +  @param[in] Milliseconds  Milliseconds to convert into ticks.
> > > +
> > > +  @retval Milliseconds expressed as number of ticks.
> > > +
> > > +**/
> > > +STATIC
> > > +UINT64
> > > +MilliSecondsToTicks (
> > > +  IN UINTN  Milliseconds
> > > +  )
> > > +{
> > > +  CONST UINT64  NanoSecondsPerTick = GetTimeInNanoSecond (1);
> > > +
> > > +  return (Milliseconds * 1000000) / NanoSecondsPerTick;
> >
> > Should use DivU64x64Remainder() here:
> > {
> >    UINT64  NanoSecondsPerTick;
> >    UINT64  NanoSeconds;
> >
> >    NanoSecondsPerTick = GetTimeInNanoSecond (1);
> >    NanoSeconds = MultU64x32 (Milliseconds, 1000000);
> >
> >    return DivU64x64Remainder (NanoSeconds, NanoSecondsPerTick, NULL);
> > }
> >
> > > +}
> > > +
> > > +/**
> > > +  Poll Status register for NOR flash erase/write completion.
> > > +
> > > +  @param[in]      Instance           NOR flash Instance.
> > > +
> > > +  @retval         EFI_SUCCESS        Request is executed 
> successfully.
> > > +  @retval         EFI_TIMEOUT        Operation timed out.
> > > +  @retval         EFI_DEVICE_ERROR   Controller operartion failed.
> >
> > operartion -> typo
> > (same at another place I think)
> >
> > [snip]
> >
> > > +
> > > +/**
> > > +  Read from nor flash.
> > > +
> > > +  @param[in]     Instance               NOR flash Instance of 
> variable store region.
> > > +  @param[in]     Lba                    The starting logical 
> block index to read from.
> > > +  @param[in]     Offset                 Offset into the block at 
> which to begin reading.
> > > +  @param[in]     BufferSizeInBytes      The number of bytes to read.
> > > +  @param[out]    Buffer                 The pointer to a 
> caller-allocated buffer that
> > > +                                        should copied with read data.
> > > +
> > > +  @retval        EFI_SUCCESS            The read is completed.
> > > +  @retval        EFI_INVALID_PARAMETER  Invalid parameters passed.
> > > +**/
> > > +EFI_STATUS
> > > +NorFlashRead (
> > > +  IN NOR_FLASH_INSTANCE  *Instance,
> > > +  IN EFI_LBA             Lba,
> > > +  IN UINTN               Offset,
> > > +  IN UINTN               BufferSizeInBytes,
> > > +  OUT VOID               *Buffer
> > > +  )
> > > +{
> > > +  UINTN  StartAddress;
> > > +
> > > +  // The buffer must be valid
> > > +  if (Buffer == NULL) {
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  // Return if we do not have any byte to read
> > > +  if (BufferSizeInBytes == 0) {
> > > +    return EFI_SUCCESS;
> > > +  }
> > > +
> > > +  if (((Lba * Instance->Media.BlockSize) + Offset + 
> BufferSizeInBytes) >
> > > +      Instance->Size)
> > > +  {
> > > +    DEBUG ((
> > > +      DEBUG_ERROR,
> > > +      "NorFlashRead: ERROR - Read will exceed device size.\n"
> > > +      ));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  // Get the address to start reading from
> > > +  StartAddress = GET_NOR_BLOCK_ADDRESS (
> > > +                   Instance->RegionBaseAddress,
> > > +                   Lba,
> > > +                   Instance->Media.BlockSize
> > > +                   );
> > > +
> > > +  // Readout the data
> > > +  CopyMem (Buffer, (UINTN *)(StartAddress + Offset), 
> BufferSizeInBytes);
> >
> > The original code at:
> >  Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
> >
> > implements and uses AlignedCopyMem()/NorFlashWriteBuffer() which seems
> > to be more efficient.
> > Just to be sure I understand correctly, is the maximal read/write size
> > of 4 bytes ? Meaning that these functions are not needed ?
> >
> > ---
> >
> > NorFlashWriteBuffer() is not implemented here IIUC won't be 
> implemtned as not
> > needed. Maybe in an additional patch, the function could be removed 
> from the
> > library interface at:
> >    Platform/ARM/Include/Library/NorFlashDeviceLib.h
> > and made static in:
> >  Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
> >
> CopyMem() and AlignedCopyMem() have nearly identical implementations, 
> therefore I think we can
> continue using CopyMem() here.
>
> For NorFlashWriteBuffer(), in the P30 spec, it looks like buffered 
> programming is one of the features
> of the IP whereas there is no such feature in cadence IP. So, I think 
> there is no need for
> NorFlashWriteBuffer() for Cadence controller Library.
>
> I will push another patch in the next patchset to remove 
>  NorFlashWriteBuffer() from the
> NorFlashDeviceLib.h header file.
[SAMI] Also make NorFlashWriteBuffer() a STATIC function in 
P30NorFlashDeviceLib.c.
>
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +/**
> > > +  Write a full or portion of a block.
> > > +
> > > +  @param[in]       Instance              NOR flash Instance of 
> variable store region.
> > > +  @param[in]       Lba                   The starting logical 
> block index to write to.
> > > +  @param[in]       Offset                Offset into the block at 
> which to begin writing.
> > > +  @param[in, out]  NumBytes              The total size of the 
> buffer.
> > > +  @param[in]       Buffer                The pointer to a 
> caller-allocated buffer that
> > > +                                         contains the source for 
> the write.
> > > +
> > > +  @retval          EFI_SUCCESS           The write is completed.
> > > +  @retval          EFI_OUT_OF_RESOURCES  Invalid Buffer passed.
> > > +  @retval          EFI_BAD_BUFFER_SIZE   Buffer size not enough.
> > > +  @retval          EFI_DEVICE_ERROR      The device reported an 
> error.
> > > +**/
> > > +EFI_STATUS
> > > +NorFlashWriteSingleBlock (
> > > +  IN        NOR_FLASH_INSTANCE  *Instance,
> > > +  IN        EFI_LBA             Lba,
> > > +  IN        UINTN               Offset,
> > > +  IN OUT    UINTN               *NumBytes,
> > > +  IN        UINT8               *Buffer
> > > +  )
> > > +{
> > > +  EFI_STATUS  Status;
> > > +  UINT32      Tmp;
> > > +  UINT32      TmpBuf;
> > > +  UINT32      WordToWrite;
> > > +  UINT32      Mask;
> > > +  BOOLEAN     DoErase;
> > > +  UINTN       BytesToWrite;
> > > +  UINTN       CurOffset;
> > > +  UINTN       WordAddr;
> > > +  UINTN       BlockSize;
> > > +  UINTN       BlockAddress;
> > > +  UINTN       PrevBlockAddress;
> > > +
> > > +  if (Buffer == NULL) {
> > > +    DEBUG ((
> > > +      DEBUG_ERROR,
> > > +      "NorFlashWriteSingleBlock: ERROR - Buffer is invalid\n"
> > > +      ));
> > > +    return EFI_OUT_OF_RESOURCES;
> >
> > EFI_INVALID_PARAMETER instead I think
> >
> > > +  }
> > > +
> > > +  PrevBlockAddress = 0;
> > > +
> > > +  DEBUG ((
> > > +    DEBUG_INFO,
> > > +    "NorFlashWriteSingleBlock(Parameters: Lba=%ld, Offset=0x%x, "
> > > +    "*NumBytes=0x%x, Buffer @ 0x%08x)\n",
> > > +    Lba,
> > > +    Offset,
> > > +    *NumBytes,
> > > +    Buffer
> > > +    ));
> > > +
> > > +  // Localise the block size to avoid de-referencing pointers all 
> the time
> >
> > Localise -> Locate
> >
> > > +  BlockSize = Instance->Media.BlockSize;
> > > +
> >
> > [snip]
> >
> >
> > 
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118971): https://edk2.groups.io/g/devel/message/118971
Mute This Topic: https://groups.io/mt/105690947/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 16646 bytes --]