Re: [PATCH v1] MinPlatformPkg: Add Pcd for FADT REVISION and MINOR REVISION to update FADT entries from board package.

["Chiu, Chasel" <chasel.chiu@intel.com>]

Hi Aryeh,

Please see below inline for comments.

Thanks,
Chasel


> -----Original Message-----
> From: Chen, Aryeh <aryeh.chen@intel.com>
> Sent: Wednesday, April 12, 2023 4:20 AM
> To: devel@edk2.groups.io
> Cc: Chen, Aryeh <aryeh.chen@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
> Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH v1] MinPlatformPkg: Add Pcd for FADT REVISION and MINOR
> REVISION to update FADT entries from board package.
> 
> From: Aryeh Chen <aryeh.chen@intel.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4409
> 
> Add Pcd for FADT REVISION and MINOR REVISION to update FADT entries from
> board package.
> 
> Signed-off-by: Aryeh Chen <aryeh.chen@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 2 ++
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 2 ++
>  Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec               | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index 6e57b638e0..a844d9c445 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1096,6 +1096,7 @@ PlatformUpdateTables (
>    case EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
> FadtHeader = (EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *) Table; +
> FadtHeader->Header.Revision                   = PcdGet8 (PcdFadtRevision);
> FadtHeader->PreferredPmProfile                = PcdGet8
> (PcdFadtPreferredPmProfile);     FadtHeader->IaPcBootArch                      =
> PcdGet16 (PcdFadtIaPcBootArch);     FadtHeader->Flags                             =
> PcdGet32 (PcdFadtFlags);@@ -1113,6 +1114,7 @@ PlatformUpdateTables (
>      FadtHeader->Gpe1Blk                           = PcdGet16 (PcdAcpiGpe1BlockAddress);
> FadtHeader->Gpe1BlkLen                        = PcdGet8 (PcdAcpiGpe1BlockLength);
> FadtHeader->Gpe1Base                          = PcdGet8 (PcdAcpiGpe1Base);+
> FadtHeader->MinorVersion                      = PcdGet8 (PcdFadtMinorRevision);
> FadtHeader->XPm1aEvtBlk.Address               = PcdGet16
> (PcdAcpiPm1AEventBlockAddress);     FadtHeader->XPm1bEvtBlk.Address
> = PcdGet16 (PcdAcpiPm1BEventBlockAddress);diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> index 451034fb8a..9d83cbf73c 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> @@ -67,6 +67,8 @@
>    gMinPlatformPkgTokenSpaceGuid.PcdFadtFlags
> gMinPlatformPkgTokenSpaceGuid.PcdFadtDutyOffset
> gMinPlatformPkgTokenSpaceGuid.PcdFadtDutyWidth+
> gMinPlatformPkgTokenSpaceGuid.PcdFadtRevision+
> gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorRevision
> gPcAtChipsetPkgTokenSpaceGuid.PcdHpetBaseAddress
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddressdiff --git
> a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> index e6f714b181..6e44287256 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> @@ -346,6 +346,8 @@
> 
> gMinPlatformPkgTokenSpaceGuid.PcdFadtPreferredPmProfile|0x02|UINT8|0x9
> 0000025
> gMinPlatformPkgTokenSpaceGuid.PcdFadtIaPcBootArch|0x0001|UINT16|0x900
> 00026
> gMinPlatformPkgTokenSpaceGuid.PcdFadtFlags|0x000086A5|UINT32|0x90000
> 027+
> gMinPlatformPkgTokenSpaceGuid.PcdFadtRevision|0x06|UINT8|0x90000030+

To be more align with specification, it looks to me better naming is PcdFadtMajorVersion


> gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorRevision|0x03|UINT8|0x90000


To be more align with specification, it looks to me better naming is PcdFadtMinorVersion


> 031  [PcdsFixedAtBuild] --
> 2.26.2.windows.1