From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.7627.1610450000475218478 for ; Tue, 12 Jan 2021 03:13:20 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: vijayenthiran.subramaniam@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1B8CA1042 for ; Tue, 12 Jan 2021 03:13:20 -0800 (PST) Received: from mail-yb1-f178.google.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 078EE3F66E for ; Tue, 12 Jan 2021 03:13:20 -0800 (PST) Received: by mail-yb1-f178.google.com with SMTP id 18so1822853ybx.2 for ; Tue, 12 Jan 2021 03:13:20 -0800 (PST) X-Gm-Message-State: AOAM532Y1aP1OolOUhBrKfVMA0h8L/92QIqIR0mD2XWgMe4ZS3Gi96en MBWzKF9FvEPn10sfAZuVoqwEe1R0oFs2Y2OnMvM= X-Google-Smtp-Source: ABdhPJxy1BEockMayqFGXuA7If/3Fs0noU1pL8XqXdi6vQ0WYZD7v8pT6ByiJNWIimVkMbsSQXbDnlPXkQOBSvsC+CE= X-Received: by 2002:a25:ac25:: with SMTP id w37mr5539710ybi.522.1610449999730; Tue, 12 Jan 2021 03:13:19 -0800 (PST) MIME-Version: 1.0 References: <20210111105719.18700-1-ard.biesheuvel@arm.com> In-Reply-To: <20210111105719.18700-1-ard.biesheuvel@arm.com> From: "Vijayenthiran Subramaniam" Date: Tue, 12 Jan 2021 11:13:05 +0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmPlatformPkg/NorFlashDxe: use correct PCD accessors To: Ard Biesheuvel Cc: leif@nuviainc.com, devel@edk2.groups.io, Vijayenthiran Subramaniam , Masahisa Kojima , Sami Mujawar , Thomas Abraham Content-Type: text/plain; charset="UTF-8" On Mon, Jan 11, 2021 at 10:57 AM Ard Biesheuvel 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 > Cc: Masahisa Kojima > Cc: Sami Mujawar > Signed-off-by: Ard Biesheuvel I've tested this change on Arm Reference Design platforms (SgiPkg). Tested-by: Vijayenthiran Subramaniam 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 > > > > > >