Re: [PATCH v1 1/1] DynamicTablesPkg: Add support to specify FADT minor revision

["PierreGondois" <pierre.gondois@arm.com>]

Hello Sami,
The only configuration manager not using ACPI 6.4 tables is at:
Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c

I think the minor version of its FADT table needs to be updated along with
this patch. Otherwise:

Reviewed-by: <pierre.gondois@arm.com>

Regards,
Pierre

On 7/6/22 13:40, Sami Mujawar wrote:
> The CM_STD_OBJ_ACPI_TABLE_INFO.AcpiTableRevision can be used to specify
> the major revision number of the ACPI table that the generator must use.
> Although most ACPI tables only have a major revision number, the FADT
> table additionally has a minor revision number.
> 
> The FADT generator currently defaults to setting the latest supported
> ACPI revision for the FADT table i.e. ACPI 6.4. This means that the minor
> revision for the FADT table is always set to 4 and there is no provision
> for a user to specify the minor revision to be selected.
> 
> Therefore, update CM_STD_OBJ_ACPI_TABLE_INFO to introduce a new field
> MinorRevision which can be used to specify the minor revision for an
> ACPI table. Also update the FADT generator to validate the supported
> FADT revisions ans use the specified minor revision for the FADT table
> if supported. If an unsupported minor revision is specified the FADT
> generator defaults to the latest supported minor revision.
> 
> Since the CM_STD_OBJ_ACPI_TABLE_INFO.MinorRevision field is added to
> the end of the structure, it should not break existing platform code.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/2221_support_fadt_minor_rev_v1
> 
>   DynamicTablesPkg/Include/StandardNameSpaceObjects.h              | 10 ++++++-
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c | 29 ++++++++++++++++++--
>   2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Include/StandardNameSpaceObjects.h b/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
> index 8d0c7da15a73e4910f9099c68f6e5cc2f06c0ecb..8ec3238225abe4fc16a7337c29ecd655590b408f 100644
> --- a/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
> +++ b/DynamicTablesPkg/Include/StandardNameSpaceObjects.h
> @@ -1,6 +1,6 @@
>   /** @file
>   
> -  Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
> +  Copyright (c) 2017 - 2022, Arm Limited. All rights reserved.
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -105,6 +105,14 @@ typedef struct CmAStdObjAcpiTableInfo {
>     /// Generators shall populate this information using the revision of the
>     /// Configuration Manager (CM_STD_OBJ_CONFIGURATION_MANAGER_INFO.Revision).
>     UINT32    OemRevision;
> +
> +  /// The minor revision of an ACPI table if required by the table.
> +  /// Note: If this field is not populated (has value of Zero), then the
> +  /// Generators shall populate this information based on the latest minor
> +  /// revision of the table that is supported by the generator.
> +  /// e.g. This field can be used to specify the minor revision to be set
> +  /// for the FADT table.
> +  UINT8     MinorRevision;
>   } CM_STD_OBJ_ACPI_TABLE_INFO;
>   
>   /** A structure used to describe the SMBIOS table generators to be invoked.
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
> index 96295f539fb0505378e862edeef898be40257cdd..1d10ea55e2395c55291faa3c247e5c59e345650c 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
> @@ -1,7 +1,7 @@
>   /** @file
>     FADT Table Generator
>   
> -  Copyright (c) 2017 - 2021, ARM Limited. All rights reserved.
> +  Copyright (c) 2017 - 2022, Arm Limited. All rights reserved.
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>     @par Reference(s):
> @@ -167,7 +167,7 @@ EFI_ACPI_6_4_FIXED_ACPI_DESCRIPTION_TABLE  AcpiFadt = {
>     // UINT16     ArmBootArch
>     EFI_ACPI_6_4_ARM_PSCI_COMPLIANT,  // {Template}: ARM Boot Architecture Flags
>     // UINT8      MinorRevision
> -  EFI_ACPI_6_4_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION,
> +  EFI_ACPI_6_4_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION, // {Template}
>     // UINT64     XFirmwareCtrl
>     0,
>     // UINT64     XDsdt
> @@ -546,6 +546,31 @@ BuildFadtTable (
>       goto error_handler;
>     }
>   
> +  // Update the MinorRevision for the FADT table if it has been specified
> +  // otherwise default to the latest FADT minor revision supported.
> +  // Note:
> +  // Bits 0-3 - The low order bits correspond to the minor version of the
> +  //            specification version.
> +  // Bits 4-7 - The high order bits correspond to the version of the ACPI
> +  //            specification errata.
> +  if (AcpiTableInfo->MinorRevision != 0) {
> +    if (((AcpiTableInfo->MinorRevision & 0xF) >=
> +         EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION) &&
> +        ((AcpiTableInfo->MinorRevision & 0xF) <=
> +         EFI_ACPI_6_4_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
> +    {
> +      AcpiFadt.MinorVersion = AcpiTableInfo->MinorRevision;
> +    } else {
> +      DEBUG ((
> +        DEBUG_WARN,
> +        "WARNING: FADT: Unsupported FADT Minor Revision 0x%x specified, " \
> +        "defaulting to FADT Minor Revision 0x%x\n",
> +        AcpiTableInfo->MinorRevision,
> +        EFI_ACPI_6_4_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION
> +        ));
> +    }
> +  }
> +
>     // Update PmProfile Info
>     Status = FadtAddPmProfileInfo (CfgMgrProtocol);
>     if (EFI_ERROR (Status)) {