Re: [edk2-devel] [PATCH] ArmPlatformPkg: Enable support for flash in 64-bit address space

["Vijayenthiran Subramaniam" <vijayenthiran.subramaniam@arm.com>]

Hi Ard,

On Wed, Nov 25, 2020 at 2:59 PM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
>
> On 11/25/20 2:29 PM, Vijayenthiran Subramaniam wrote:
> > 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.
> >
>
> Why?

There’s a derivative SGI/RD platform which has NOR flash connected to the 64-bit
MMIO space. We are planning to upstream support for the platform to
edk2-platform’s SgiPkg in the upcoming days. As a preparation, this patch
updates the NorFlashDxe to support 64-bit addresses.

Regards,
Vijayenthiran

>
> > Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@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);
> > +  }
> >
> >       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));
> > +  } 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 {
> > +    ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0);
> > +    ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0);
> > +    ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0);
> > +  }
> >
> >     //
> >     // 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) {
> >