[edk2-devel] [PATCH] ArmPlatformPkg: Enable support for flash in 64-bit address space
Sami Mujawar
sami.mujawar at arm.com
Wed Nov 25 14:44:49 UTC 2020
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67980): https://edk2.groups.io/g/devel/message/67980
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