[edk2-devel] [PATCH] ArmPlatformPkg: Enable support for flash in 64-bit address space
Vijayenthiran Subramaniam
vijayenthiran.subramaniam at arm.com
Mon Nov 30 13:43:23 UTC 2020
Hi Sami,
On Wed, Nov 25, 2020 at 2:45 PM Sami Mujawar <sami.mujawar at arm.com> wrote:
>
> Hi Vijay,
>
> Thank you for this patch.
>
> Please see my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Vijayenthiran Subramaniam via groups.io
> Sent: 25 November 2020 01:30 PM
> To: devel at edk2.groups.io; leif at nuviainc.com; Ard Biesheuvel <Ard.Biesheuvel at arm.com>
> Cc: Thomas Abraham <thomas.abraham at arm.com>; Aditya Angadi <Aditya.Angadi at arm.com>
> Subject: [edk2-devel] [PATCH] ArmPlatformPkg: Enable support for flash in 64-bit address space
>
> The existing NOR flash Dxe driver supports NOR flash devices connected
> in the 32-bit address space. Extend this driver to allow NOR flash
> devices connected to 64-bit address space to be usable as well.
>
> Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam at arm.com>
> ---
> .../Drivers/NorFlashDxe/NorFlashDxe.c | 13 ++++++--
> .../Drivers/NorFlashDxe/NorFlashDxe.inf | 3 ++
> .../Drivers/NorFlashDxe/NorFlashFvbDxe.c | 31 ++++++++++++++-----
> 3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index d9e196cbf1..f3fbbafb7d 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -1298,9 +1298,16 @@ NorFlashInitialise (
>
> for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
> // Check if this NOR Flash device contain the variable storage region
> - ContainVariableStorage =
> - (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> - (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> +
> + if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
> + ContainVariableStorage =
> + (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) &&
> + (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> + } else {
> + ContainVariableStorage =
> + (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> + (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> + }
> [SAMI] Since you are updating the code here, can you limit the maximum line length, please?
> I prefer 80 chars but coding guidelines allows up to 120 see https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/3_quick_reference#3-3-files-general-rules
> [/SAMI]
>
> Status = NorFlashCreateInstance (
> NorFlashDevices[Index].DeviceBaseAddress,
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index a647c01687..b2a941d672 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -54,10 +54,13 @@
> gEfiDiskIoProtocolGuid
>
> [Pcd.common]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> index 9cdd85096a..ecbe009495 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> @@ -58,8 +58,17 @@ InitializeFvAndVariableStoreHeaders (
> Headers = AllocateZeroPool(HeadersLength);
>
> // FirmwareVolumeHeader->FvLength is declared to have the Variable area AND the FTW working area AND the FTW Spare contiguous.
> - ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase));
> - ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase));
> + if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
> + ASSERT(PcdGet64(PcdFlashNvStorageVariableBase64) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet64(PcdFlashNvStorageFtwWorkingBase64));
>
> [SAMI] Please put a space between PcdGetxx and '('. Same applies to ASSERT () as well.
> [/SAMI]
>
> + } else {
> + ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase));
> + }
> +
> + if (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) != 0) {
> + ASSERT(PcdGet64(PcdFlashNvStorageFtwWorkingBase64) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet64(PcdFlashNvStorageFtwSpareBase64));
> + } else {
> + ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase));
> + }
>
>
> // Check if the size of the area is at least one block size
> ASSERT((PcdGet32(PcdFlashNvStorageVariableSize) > 0) && (PcdGet32(PcdFlashNvStorageVariableSize) / Instance->Media.BlockSize > 0));
> @@ -67,9 +76,16 @@ InitializeFvAndVariableStoreHeaders (
> ASSERT((PcdGet32(PcdFlashNvStorageFtwSpareSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwSpareSize) / Instance->Media.BlockSize > 0));
>
> // Ensure the Variable area Base Addresses are aligned on a block size boundaries
> - ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0);
> - ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0);
> - ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0);
> + if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
> + ASSERT(PcdGet64(PcdFlashNvStorageVariableBase64) % Instance->Media.BlockSize == 0);
> + ASSERT(PcdGet64(PcdFlashNvStorageFtwWorkingBase64) % Instance->Media.BlockSize == 0);
> + ASSERT(PcdGet64(PcdFlashNvStorageFtwSpareBase64) % Instance->Media.BlockSize == 0);
> + }
> + else {
>
> [SAMI] Move else to previous line e.g. '} else {'
> [/SAMI]
>
> + ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0);
> + ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0);
> + ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0);
> + }
>
> [SAMI] This entire code block from line 60 to 90 is asserting when an invalid parameter is detected so could ideally be in DEBUG_CODE ().
> But more importantly, in release builds the asserts would be turned off leading to incorrect values being used. I think this code can be improved to return an error code if a parameter is invalid. This would be useful as some of these PCDs are dynamic.
> [/SAMI]
>
>
> //
> // EFI_FIRMWARE_VOLUME_HEADER
> @@ -736,10 +752,11 @@ NorFlashFvbInitialize (
> EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> ASSERT_EFI_ERROR (Status);
>
> - mFlashNvStorageVariableBase = PcdGet32 (PcdFlashNvStorageVariableBase);
> + mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> + FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase);
>
> // Set the index of the first LBA for the FVB
> - Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
> + Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
>
> BootMode = GetBootModeHob ();
> if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) {
> --
> 2.17.1
>
Thanks for the review. I will address the changes in the next version
of the patch.
Regards,
Vijay
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68078): https://edk2.groups.io/g/devel/message/68078
Mute This Topic: https://groups.io/mt/78499161/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list