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

["Jagadeesh Ujja" <jagadeesh.ujja@arm.com>]

Hi Sami,
The patch is tested and looks good to me:

Tested-by: Jagadeesh Ujja <Jagadeesh.Ujja@arm.com>

On Wed, Jul 6, 2022 at 8:57 PM Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Hi Pierre,
>
> On 06/07/2022 04:03 pm, Pierre Gondois wrote:
> > 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:
> >
> Thank you for bringing this up.
>
> As it stands the NXP platform would be using ACPI 6.4 which is the
> default case, and this patch will not change that. So, downgrading this
> to ACPI 6.2 may not be expected.
>
> I have copied the NXP package maintainers for further inputs. If needed,
> I could submit a edk2-platforms patch as required.
>
> Regards,
>
> Sami Mujawar
>
>
> > 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)) {
>
>
> 
>
>