Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 05/14] Platform/ARM: Create NorFlashDeviceLib library interface for flash specific functions

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

Hi Sahil,

Please find my response inline marked [SAMI].

Regards,

Sami mujawar

On 21/05/2024 09:37 am, Sahil wrote:
> Hi Sami,
> Thank you for reviewing the patches.
> Please find my comments inline below marked as [SAHIL].
> Also, for the documentation headers, I will try to add in
> NorFlashDeviceLib.h and keep it consistent with
> CadenceQspiNorFlashDeviceLib.
>
>
> On Thu, 16 May 2024 at 20:48, Sami Mujawar via groups.io
> <sami.mujawar=arm.com@groups.io> wrote:
>> Hi Sahil,
>>
>> Thank you for this patch.
>>
>> I have some feedback marked inline as [SAMI].
>>
>> Other than that, is is possible to add documentation header for the functions and data streuctures in this file, please?
>>
>> With that fixed,
>>
>> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 23/04/2024 06:56 am, Sahil Kaushal wrote:
>>
>> From: sahil <sahil@arm.com>
>>
>> NorFlashDeviceLib can be used to provide implementations of different
>> NOR Flash to NorFlashDxe, i.e. NorFlashDxe links with NorFlashDeviceLib
>> and the platforms can specify their respective NorFlashDeviceLib
>> instances.
>>
>> This patch splits NorFlash.h and moves out the function prototypes and
>> macros that are expected by NorFlashDxe to be implemented by any
>> Nor Flash implementation to NorFlashDeviceLib.h file.
>>
>> Signed-off-by: sahil <sahil@arm.com>
>> ---
>>   Platform/ARM/ARM.dec                              |   1 +
>>   Platform/ARM/Drivers/NorFlashDxe/NorFlash.h       | 143 +-----------------
>>   Platform/ARM/Drivers/NorFlashDxe/NorFlashCommon.h |   1 +
>>   Platform/ARM/Include/Library/NorFlashDeviceLib.h  | 156 ++++++++++++++++++++
>>   4 files changed, 159 insertions(+), 142 deletions(-)
>>
>> diff --git a/Platform/ARM/ARM.dec b/Platform/ARM/ARM.dec
>> index be7e6dc83fde..86d1fcb4878e 100644
>> --- a/Platform/ARM/ARM.dec
>> +++ b/Platform/ARM/ARM.dec
>> @@ -17,6 +17,7 @@
>>
>>
>>   [LibraryClasses]
>>
>>     BdsLib|Include/Library/BdsLib.h
>>
>> +  NorFlashDeviceLib|Include/Library/NorFlashDeviceLib.h
>>
>>     NorFlashPlatformLib|Include/Library/NorFlashPlatformLib.h
>>
>>
>>
>>   [Guids]
>>
>> diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlash.h b/Platform/ARM/Drivers/NorFlashDxe/NorFlash.h
>> index bd5c6a949cf0..6cb1f64b9875 100644
>> --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlash.h
>> +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlash.h
>> @@ -20,6 +20,7 @@
>>
>>
>>   #include <Library/DebugLib.h>
>>
>>   #include <Library/IoLib.h>
>>
>> +#include <Library/NorFlashDeviceLib.h>
>>
>>
>>
>>   #define NOR_FLASH_ERASE_RETRY  10
>>
>>
>>
>> @@ -40,7 +41,6 @@
>>   #define CREATE_NOR_ADDRESS(BaseAddr, OffsetAddr)       ((BaseAddr) + ((OffsetAddr) << 2))
>>
>>   #define CREATE_DUAL_CMD(Cmd)                           ( ( Cmd << 16) | ( Cmd & LOW_16_BITS) )
>>
>>   #define SEND_NOR_COMMAND(BaseAddr, Offset, Cmd)        MmioWrite32 (CREATE_NOR_ADDRESS(BaseAddr,Offset), CREATE_DUAL_CMD(Cmd))
>>
>> -#define GET_NOR_BLOCK_ADDRESS(BaseAddr, Lba, LbaSize)  ( BaseAddr + (UINTN)((Lba) * LbaSize) )
>>
>>
>>
>>   // Status Register Bits
>>
>>   #define P30_SR_BIT_WRITE            (BIT7 << 16 | BIT7)
>>
>> @@ -105,145 +105,4 @@
>>   #define P30_CMD_READ_CONFIGURATION_REGISTER_SETUP  0x0060
>>
>>   #define P30_CMD_READ_CONFIGURATION_REGISTER        0x0003
>>
>>
>>
>> -typedef struct _NOR_FLASH_INSTANCE NOR_FLASH_INSTANCE;
>>
>> -
>>
>> -#pragma pack (1)
>>
>> -typedef struct {
>>
>> -  VENDOR_DEVICE_PATH          Vendor;
>>
>> -  UINT8                       Index;
>>
>> -  EFI_DEVICE_PATH_PROTOCOL    End;
>>
>> -} NOR_FLASH_DEVICE_PATH;
>>
>> -#pragma pack ()
>>
>> -
>>
>> -struct _NOR_FLASH_INSTANCE {
>>
>> -  UINT32                                 Signature;
>>
>> -  EFI_HANDLE                             Handle;
>>
>> -
>>
>> -  UINTN                                  DeviceBaseAddress;
>>
>> -  UINTN                                  RegionBaseAddress;
>>
>> -  UINTN                                  Size;
>>
>> -  EFI_LBA                                StartLba;
>>
>> -
>>
>> -  EFI_BLOCK_IO_PROTOCOL                  BlockIoProtocol;
>>
>> -  EFI_BLOCK_IO_MEDIA                     Media;
>>
>> -  EFI_DISK_IO_PROTOCOL                   DiskIoProtocol;
>>
>> -
>>
>> -  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL    FvbProtocol;
>>
>> -  VOID                                   *ShadowBuffer;
>>
>> -
>>
>> -  NOR_FLASH_DEVICE_PATH                  DevicePath;
>>
>> -};
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashReadCfiData (
>>
>> -  IN  UINTN   DeviceBaseAddress,
>>
>> -  IN  UINTN   CFI_Offset,
>>
>> -  IN  UINT32  NumberOfBytes,
>>
>> -  OUT UINT32  *Data
>>
>> -  );
>>
>> [SAMI] Where is this function implemented ?
> [SAHIL]
> This function is not implemented anywhere. It is the same for
> NorFlashWrite() as well.
> I have migrated them as-is from NorFlash.h but If needed I can push a
> patch removing both from the library interface.
> Shall I push it?
[SAMI] Yes, please drop the dead code. [/SAMI]
> [/SAHIL]
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashWriteBuffer (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN UINTN               TargetAddress,
>>
>> -  IN UINTN               BufferSizeInBytes,
>>
>> -  IN UINT32              *Buffer
>>
>> -  );
>>
>> -
>>
>> -//
>>
>> -// NorFlash.c
>>
>> -//
>>
>> -EFI_STATUS
>>
>> -NorFlashWriteSingleBlock (
>>
>> -  IN        NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN        EFI_LBA             Lba,
>>
>> -  IN        UINTN               Offset,
>>
>> -  IN OUT    UINTN               *NumBytes,
>>
>> -  IN        UINT8               *Buffer
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashWriteBlocks (
>>
>> -  IN  NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN  EFI_LBA             Lba,
>>
>> -  IN  UINTN               BufferSizeInBytes,
>>
>> -  IN  VOID                *Buffer
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashReadBlocks (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN EFI_LBA             Lba,
>>
>> -  IN UINTN               BufferSizeInBytes,
>>
>> -  OUT VOID               *Buffer
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashRead (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN EFI_LBA             Lba,
>>
>> -  IN UINTN               Offset,
>>
>> -  IN UINTN               BufferSizeInBytes,
>>
>> -  OUT VOID               *Buffer
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashWrite (
>>
>> -  IN        NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN        EFI_LBA             Lba,
>>
>> -  IN        UINTN               Offset,
>>
>> -  IN OUT    UINTN               *NumBytes,
>>
>> -  IN        UINT8               *Buffer
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashReset (
>>
>> -  IN  NOR_FLASH_INSTANCE  *Instance
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashEraseSingleBlock (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN UINTN               BlockAddress
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashUnlockSingleBlockIfNecessary (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN UINTN               BlockAddress
>>
>> -  );
>>
>> [SAMI] Should this be a STATIC function in the Device Library implementations? Would it make more sense to have NorFlashBlockIsLocked() exposed via this library interface instead.
>>
>> Also, would it make sense to integrate the check to see if the block is locked in the NorFlashUnlockSingleBlock() instead and expose it via the library interface?
>>
>> [/SAMI]
> [SAHIL]
> I think it is okay to just expose
> NorFlashUnlockSingleBlockIfNecessary() through the interface as it
> takes care of both NorFlashBlockIsLocked() and
> NorFlashUnlockSingleBlock(). Also, at the moment these functions are
> not used anywhere outside the library implementations.
> But if we still want to make it STATIC and expose other functions, do
> you want me to expose both NorFlashBlockIsLocked() and
> NorFlashUnlockSingleBlock()? Or just NorFlashUnlockSingleBlock() with
> the NorFlashBlockIsLocked() check in it? [/SAHIL]

[SAMI]  If the functions are not used outside the library just make them 
STATIC and in that case there is no need to modify the existing 
functionality.

If we need to export them at a later point in time it can always be done.

[/SAMI]

>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashWriteSingleWord (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN UINTN               WordAddress,
>>
>> -  IN UINT32              WriteData
>>
>> -  );
>>
>> [SAMI] Should this function be made STATIC in the Device Library implementations?
> [SAHIL] I will push a new patch making NorFlashWriteBuffer() and
> NorFlashWriteSingleWord () STATIC.
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashWriteFullBlock (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN EFI_LBA             Lba,
>>
>> -  IN UINT32              *DataBuffer,
>>
>> -  IN UINT32              BlockSizeInWords
>>
>> -  );
>>
>> -
>>
>> -EFI_STATUS
>>
>> -NorFlashUnlockAndEraseSingleBlock (
>>
>> -  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> -  IN UINTN               BlockAddress
>>
>> -  );
>>
>> -
>>
>> -VOID
>>
>> -EFIAPI
>>
>> -NorFlashLock (
>>
>> -  IN EFI_TPL  *OriginalTPL
>>
>> -  );
>>
>> -
>>
>> -VOID
>>
>> -EFIAPI
>>
>> -NorFlashUnlock (
>>
>> -  IN EFI_TPL OriginalTPL
>>
>> -  );
>>
>> -
>>
>>   #endif /* __NOR_FLASH_H__ */
>>
>> diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashCommon.h b/Platform/ARM/Drivers/NorFlashDxe/NorFlashCommon.h
>> index c0a3b5861532..7fcb949843e8 100644
>> --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashCommon.h
>> +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashCommon.h
>> @@ -19,6 +19,7 @@
>>   #include <Protocol/FirmwareVolumeBlock.h>
>>
>>
>>
>>   #include <Library/DebugLib.h>
>>
>> +#include <Library/NorFlashDeviceLib.h>
>>
>>   #include <Library/NorFlashPlatformLib.h>
>>
>>   #include <Library/UefiLib.h>
>>
>>   #include <Library/UefiRuntimeLib.h>
>>
>> diff --git a/Platform/ARM/Include/Library/NorFlashDeviceLib.h b/Platform/ARM/Include/Library/NorFlashDeviceLib.h
>> new file mode 100644
>> index 000000000000..e5017130a091
>> --- /dev/null
>> +++ b/Platform/ARM/Include/Library/NorFlashDeviceLib.h
>> @@ -0,0 +1,156 @@
>> +/** @file  NorFlashDeviceLib.h
>>
>> +
>>
>> +  Copyright (c) 2011 - 2024, Arm Limited. All rights reserved.<BR>
>>
>> +
>>
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> +
>>
>> +**/
>>
>> +
>>
>> +#ifndef NOR_FLASH_DEVICE_LIB_H_
>>
>> +#define NOR_FLASH_DEVICE_LIB_H_
>>
>> +
>>
>> +#include <Protocol/BlockIo.h>
>>
>> +#include <Protocol/DiskIo.h>
>>
>> +#include <Protocol/FirmwareVolumeBlock.h>
>>
>> +
>>
>> +typedef struct _NOR_FLASH_INSTANCE NOR_FLASH_INSTANCE;
>>
>> +
>>
>> +#define GET_NOR_BLOCK_ADDRESS(BaseAddr, Lba, LbaSize)  ( BaseAddr + (UINTN)((Lba) * LbaSize) )
>>
>> +
>>
>> +#pragma pack (1)
>>
>> +typedef struct {
>>
>> +  VENDOR_DEVICE_PATH          Vendor;
>>
>> +  UINT8                       Index;
>>
>> +  EFI_DEVICE_PATH_PROTOCOL    End;
>>
>> +} NOR_FLASH_DEVICE_PATH;
>>
>> +#pragma pack ()
>>
>> +
>>
>> +struct _NOR_FLASH_INSTANCE {
>>
>> +  UINT32                                 Signature;
>>
>> +  EFI_HANDLE                             Handle;
>>
>> +
>>
>> +  UINTN                                  DeviceBaseAddress;
>>
>> +  UINTN                                  RegionBaseAddress;
>>
>> +  UINTN                                  Size;
>>
>> +  EFI_LBA                                StartLba;
>>
>> +
>>
>> +  EFI_BLOCK_IO_PROTOCOL                  BlockIoProtocol;
>>
>> +  EFI_BLOCK_IO_MEDIA                     Media;
>>
>> +  EFI_DISK_IO_PROTOCOL                   DiskIoProtocol;
>>
>> +
>>
>> +  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL    FvbProtocol;
>>
>> +  VOID                                   *ShadowBuffer;
>>
>> +
>>
>> +  NOR_FLASH_DEVICE_PATH                  DevicePath;
>>
>> +};
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashReadCfiData (
>>
>> +  IN  UINTN   DeviceBaseAddress,
>>
>> +  IN  UINTN   CFI_Offset,
>>
>> +  IN  UINT32  NumberOfBytes,
>>
>> +  OUT UINT32  *Data
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashWriteBuffer (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN UINTN               TargetAddress,
>>
>> +  IN UINTN               BufferSizeInBytes,
>>
>> +  IN UINT32              *Buffer
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashWriteFullBlock (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN EFI_LBA             Lba,
>>
>> +  IN UINT32              *DataBuffer,
>>
>> +  IN UINT32              BlockSizeInWords
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashUnlockAndEraseSingleBlock (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN UINTN               BlockAddress
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashWriteSingleBlock (
>>
>> +  IN        NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN        EFI_LBA             Lba,
>>
>> +  IN        UINTN               Offset,
>>
>> +  IN OUT    UINTN               *NumBytes,
>>
>> +  IN        UINT8               *Buffer
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashWriteBlocks (
>>
>> +  IN  NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN  EFI_LBA             Lba,
>>
>> +  IN  UINTN               BufferSizeInBytes,
>>
>> +  IN  VOID                *Buffer
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashReadBlocks (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN EFI_LBA             Lba,
>>
>> +  IN UINTN               BufferSizeInBytes,
>>
>> +  OUT VOID               *Buffer
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashRead (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN EFI_LBA             Lba,
>>
>> +  IN UINTN               Offset,
>>
>> +  IN UINTN               BufferSizeInBytes,
>>
>> +  OUT VOID               *Buffer
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashWrite (
>>
>> +  IN        NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN        EFI_LBA             Lba,
>>
>> +  IN        UINTN               Offset,
>>
>> +  IN OUT    UINTN               *NumBytes,
>>
>> +  IN        UINT8               *Buffer
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashReset (
>>
>> +  IN  NOR_FLASH_INSTANCE  *Instance
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashEraseSingleBlock (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN UINTN               BlockAddress
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashUnlockSingleBlockIfNecessary (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN UINTN               BlockAddress
>>
>> +  );
>>
>> +
>>
>> +EFI_STATUS
>>
>> +NorFlashWriteSingleWord (
>>
>> +  IN NOR_FLASH_INSTANCE  *Instance,
>>
>> +  IN UINTN               WordAddress,
>>
>> +  IN UINT32              WriteData
>>
>> +  );
>>
>> +
>>
>> +VOID
>>
>> +EFIAPI
>>
>> +NorFlashLock (
>>
>> +  IN EFI_TPL  *OriginalTPL
>>
>> +  );
>>
>> +
>>
>> +VOID
>>
>> +EFIAPI
>>
>> +NorFlashUnlock (
>>
>> +  IN EFI_TPL  OriginalTPL
>>
>> +  );
>>
>> +
>>
>> +#endif /* NOR_FLASH_DEVICE_LIB_H_ */
>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>> 


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