Re: [edk2-devel] [PATCH] ArmPlatformPkg/NorFlashDxe: use correct PCD accessors

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

On Mon, Jan 11, 2021 at 10:57 AM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
>
> Commit 8015f3f6d4005d83 ("ArmPlatformPkg: Enable support for flash in
> 64-bit address space") updated the NorFlash DXE and StMM drivers to
> take alternate PCDs into account when discovering the base of the
> NOR flash regions.
>
> This introduced a disparity between the declarations of the PCD references
> in the .INF files, which permits the use of dynamic PCDs, and the code
> itself, which now uses FixedPcdGet() accessors. On platforms that actually
> use dynamic PCDs, this results in a build error.
>
> So let's clean this up:
> - for the DXE version, use the generic PcdGet() accessors, so dynamic PCDs
>   are permitted
> - for the standalone MM version, redeclare the PCDs as [FixedPcd] in the
>   .INF description, and switch to the FixedPcdGet() accessors.
>
> Cc: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
> Cc: Masahisa Kojima <masahisa.kojima@linaro.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

I've tested this change on Arm Reference Design platforms (SgiPkg).
Tested-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>

Also, can you let us know which platform (that used dynamic PCD) broke the
build?

Thanks,
Vijay

> ---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf |  3 ++-
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c            |  4 ++--
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c   | 10 +++++-----
>  3 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
> index b2f72fb4de20..56347a8bc7c1 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
> @@ -49,7 +49,7 @@ [Guids]
>  [Protocols]
>    gEfiSmmFirmwareVolumeBlockProtocolGuid
>
> -[Pcd.common]
> +[FixedPcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> @@ -60,6 +60,7 @@ [Pcd.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>
> +[FeaturePcd]
>    gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked
>
>  [Depex]
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 28dc8e125c78..f412731200cf 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -422,8 +422,8 @@ NorFlashFvbInitialize (
>        EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
>    ASSERT_EFI_ERROR (Status);
>
> -  mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> -    FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase);
> +  mFlashNvStorageVariableBase = (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> +    PcdGet64 (PcdFlashNvStorageVariableBase64) : PcdGet32 (PcdFlashNvStorageVariableBase);
>
>    // Set the index of the first LBA for the FVB
>    Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> index 8a4fb395d286..4ebbc06e1de3 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> @@ -299,15 +299,15 @@ NorFlashInitialise (
>    for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
>      // Check if this NOR Flash device contain the variable storage region
>
> -   if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
> +   if (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
>       ContainVariableStorage =
> -       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) &&
> -       (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
> +       (NorFlashDevices[Index].RegionBaseAddress <= FixedPcdGet64 (PcdFlashNvStorageVariableBase64)) &&
> +       (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) + FixedPcdGet32 (PcdFlashNvStorageVariableSize) <=
>          NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
>     } else {
>       ContainVariableStorage =
> -       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> -       (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
> +       (NorFlashDevices[Index].RegionBaseAddress <= FixedPcdGet32 (PcdFlashNvStorageVariableBase)) &&
> +       (FixedPcdGet32 (PcdFlashNvStorageVariableBase) + FixedPcdGet32 (PcdFlashNvStorageVariableSize) <=
>          NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
>    }
>
> --
> 2.17.1
>
>
>
> 
>
>