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

["PierreGondois" <pierre.gondois@arm.com>]

Hello Sahil,

On 4/23/24 07:56, Sahil Kaushal via 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

> +
> +  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 (#118509): https://edk2.groups.io/g/devel/message/118509
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]
-=-=-=-=-=-=-=-=-=-=-=-