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.web12.7343.1657119829867817313 for ; Wed, 06 Jul 2022 08:03:50 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@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 CFA79106F; Wed, 6 Jul 2022 08:03:49 -0700 (PDT) Received: from [10.57.40.86] (unknown [10.57.40.86]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 238173F792; Wed, 6 Jul 2022 08:03:46 -0700 (PDT) Message-ID: <6a6a750c-8daa-c383-6972-306bce254a23@arm.com> Date: Wed, 6 Jul 2022 17:03:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH v1 1/1] DynamicTablesPkg: Add support to specify FADT minor revision To: Sami Mujawar , devel@edk2.groups.io Cc: Alexei.Fedorov@arm.com, jbrasen@nvidia.com, Jagadeesh.Ujja@arm.com, Deepak.Pandey@arm.com, Chandni.Cherukuri@arm.com, Matteo.Carlini@arm.com, Akanksha.Jain2@arm.com, Ben.Adderson@arm.com, nd@arm.com References: <20220706114009.26492-1-sami.mujawar@arm.com> From: "PierreGondois" In-Reply-To: <20220706114009.26492-1-sami.mujawar@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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: 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 > --- > 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)) {