From: "sahil" <sahil@arm.com>
To: devel@edk2.groups.io, pierre.gondois@arm.com
Cc: sahil.kaushal@arm.com, Ard Biesheuvel <ardb+tianocore@kernel.org>,
Leif Lindholm <quic_llindhol@quicinc.com>,
Sami Mujawar <sami.mujawar@arm.com>,
"levi.yun" <yeoreum.yun@arm.com>
Subject: Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 12/14] Platform/ARM: Add CadenceQspiNorFlashDeviceLib for NorFlashDxe
Date: Thu, 9 May 2024 11:55:13 +0530 [thread overview]
Message-ID: <CAHDJ2V6RFrmfzJsYU6S6QNgjiJFQkt+bU1oUs3CfeF+=pZHLsg@mail.gmail.com> (raw)
In-Reply-To: <8f292b6a-4c18-48c5-8e20-5eb9c86538e0@arm.com>
[-- Attachment #1: Type: text/plain, Size: 7944 bytes --]
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 <pierre.gondois=
arm.com@groups.io> wrote:
>
> 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
>
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.
> > +
> > + 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 (#118730): https://edk2.groups.io/g/devel/message/118730
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: 10999 bytes --]
next prev parent reply other threads:[~2024-05-09 6:25 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-23 5:56 [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 00/14] Split NorFlashDxe driver and add CadenceQspiNorFlashDeviceLib library Sahil Kaushal
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 01/14] Platform/ARM/NorFlashDxe: Move DiskIo related functions out of NorFlash.c Sahil Kaushal
2024-04-24 9:49 ` levi.yun
2024-05-16 15:17 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 02/14] Platform/ARM/NorFlashDxe: Move NorFlashVirtualNotifyEvent Sahil Kaushal
2024-05-16 15:17 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 03/14] Platform/ARM/NorFlashDxe: Add NorFlashCommon.h header file Sahil Kaushal
2024-04-24 9:49 ` levi.yun
2024-05-16 15:17 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 04/14] Platform/ARM/NorFlashDxe: Move flash specific functions to NorFlash.c Sahil Kaushal
2024-04-24 9:49 ` levi.yun
2024-05-16 15:17 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 05/14] Platform/ARM: Create NorFlashDeviceLib library interface for flash specific functions Sahil Kaushal
2024-04-24 9:50 ` levi.yun
2024-05-16 15:18 ` Sami Mujawar
2024-05-21 8:37 ` sahil
2024-05-21 14:05 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 06/14] Platform/ARM: Add P30NorFlashDeviceLib Library Sahil Kaushal
2024-04-24 9:49 ` levi.yun
2024-05-16 15:18 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 07/14] Platform/ARM/NorFlashDxe: Switch from NorFlash.c to NorFlashDeviceLib Sahil Kaushal
2024-04-24 9:50 ` levi.yun
2024-05-16 15:18 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 08/14] Platform/ARM: Add HostRegisterBaseAddress variable Sahil Kaushal
2024-04-24 9:50 ` levi.yun
2024-05-16 15:22 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 09/14] Platform/ARM: Add optional provision to fetch and print NOR Flash info Sahil Kaushal
2024-04-24 9:51 ` levi.yun
2024-05-16 15:23 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 10/14] Silicon/ARM/NeoverseN1Soc: Enable SCP QSPI flash region Sahil Kaushal
2024-04-24 9:50 ` levi.yun
2024-05-16 15:23 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14] Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp Sahil Kaushal
2024-04-24 9:50 ` levi.yun
2024-05-16 15:23 ` Sami Mujawar
2024-05-21 9:24 ` sahil
2024-05-21 12:59 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 12/14] Platform/ARM: Add CadenceQspiNorFlashDeviceLib for NorFlashDxe Sahil Kaushal
2024-04-24 9:55 ` levi.yun
2024-05-02 13:17 ` PierreGondois
2024-05-09 6:25 ` sahil [this message]
2024-05-16 15:25 ` Sami Mujawar
2024-05-16 15:24 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 13/14] Platform/ARM/N1Sdp: Persistent storage for N1Sdp Sahil Kaushal
2024-04-24 9:55 ` levi.yun
2024-05-16 15:24 ` Sami Mujawar
2024-04-23 5:56 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 14/14] Platform/ARM/N1Sdp: Enable FaultTolerantWrite Dxe driver " Sahil Kaushal
2024-04-24 9:51 ` levi.yun
2024-05-16 15:24 ` Sami Mujawar
2024-05-02 13:16 ` [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 00/14] Split NorFlashDxe driver and add CadenceQspiNorFlashDeviceLib library PierreGondois
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHDJ2V6RFrmfzJsYU6S6QNgjiJFQkt+bU1oUs3CfeF+=pZHLsg@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox