From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id BE2DCAC1290 for ; Mon, 11 Mar 2024 14:16:29 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=IeRtLuqzlT2uuU7vgaMBX5f4lZHqMi4o3brluqTH1E0=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:From:Subject:To:Cc:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20240206; t=1710166588; v=1; b=jbhFxee55UmiJ4Jkow0YFUpP8AKsQiZVDwghVQjFLgRHLBgnZU5UWwkEHfg0nCGKzWtP1Ivg edmLlPmoPqwfJhAjVVB3HkV6/wwXPk0MIRSrFyjEf4s0igV5bpEW5BdLqbSqEXuHo2jw2i1NaWZ LR41oH4vp7jDeSb+/DAZdrCcCTER25DXtTnAbibdIExV8x+4obptl3axnVRyXLikVid6OF71VPI QSgypgIluu7oLSutBKB5wyyNmWUcBlj13EwPBUP7TXccPC7jx5um2Gu7ZWraweUPCmx6YgZZ2ly 5HmMs3bZWOgimdilobXsTRZi+6bwgpO7r4RJtc0LXxKgw== X-Received: by 127.0.0.2 with SMTP id jACqYY7687511xU3m2YfLBVL; Mon, 11 Mar 2024 07:16:28 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.63874.1710166587832363552 for ; Mon, 11 Mar 2024 07:16:28 -0700 X-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 400B1FEC; Mon, 11 Mar 2024 07:17:04 -0700 (PDT) X-Received: from [10.34.100.133] (e126645.nice.arm.com [10.34.100.133]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7FB6C3F762; Mon, 11 Mar 2024 07:16:26 -0700 (PDT) Message-ID: <405fb4ba-bfde-41a2-9f79-078d5c895d78@arm.com> Date: Mon, 11 Mar 2024 07:16:28 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: "PierreGondois" Subject: Re: [edk2-devel] [PATCH v2 4/4] DynamicTablesPkg: Adds ACPI SSDT HPET Table generator To: Abdul Lateef Attar , devel@edk2.groups.io Cc: Abdul Lateef Attar , Sami Mujawar References: <514bf3127bf6d19396d323dda1d67672095a7b18.1709566848.git.AbdulLateef.Attar@amd.com> In-Reply-To: <514bf3127bf6d19396d323dda1d67672095a7b18.1709566848.git.AbdulLateef.Attar@amd.com> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pierre.gondois@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 4efumBPDWSmcgXWmEkjCSnHzx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=jbhFxee5; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=arm.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hello Abdul, On 3/4/24 16:43, Abdul Lateef Attar wrote: > From: Abdul Lateef Attar >=20 > Adds generic ACPI SSDT HPET table generator library. > Register/Deregister HPET table. > Adds ACPI namespace object for HPET device. > Adds Address space for HPET device. >=20 > Cc: Sami Mujawar > Cc: Pierre Gondois > Signed-off-by: Abdul Lateef Attar ` > --- > DynamicTablesPkg/DynamicTables.dsc.inc | 2 + > DynamicTablesPkg/Include/AcpiTableGenerator.h | 2 + > .../Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf | 36 +++ > .../Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c | 266 ++++++++++++++++++ > 4 files changed, 306 insertions(+) > create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSs= dtHpetLib.inf > create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHp= etGenerator.c >=20 > diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/Dy= namicTables.dsc.inc > index 477dc6b6a9..fc2ac5962e 100644 > --- a/DynamicTablesPkg/DynamicTables.dsc.inc > +++ b/DynamicTablesPkg/DynamicTables.dsc.inc > @@ -36,6 +36,7 @@ > DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf > DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf > + DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf (note for later): The HPET table seems to be intel specific actually, > =20 > [Components.IA32, Components.X64] > # > @@ -46,6 +47,7 @@ > NULL|DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf > NULL|DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > NULL|DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf > + NULL|DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib= .inf > } > =20 > [Components.ARM, Components.AARCH64] > diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTable= sPkg/Include/AcpiTableGenerator.h > index a32ef46ecb..ef651aa2aa 100644 > --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h > +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h > @@ -1,6 +1,7 @@ > /** @file > =20 > Copyright (c) 2017 - 2022, Arm Limited. All rights reserved.
> + Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved. > =20 > SPDX-License-Identifier: BSD-2-Clause-Patent > =20 > @@ -101,6 +102,7 @@ typedef enum StdAcpiTableId { > EStdAcpiTableIdPcct, ///< PCCT Generator > EStdAcpiTableIdHpet, ///< HPET Generator > EStdAcpiTableIdWsmt, ///< WSMT Generator > + EStdAcpiTableIdSsdtHpet, ///< SSDT HPET Generator > EStdAcpiTableIdMax > } ESTD_ACPI_TABLE_ID; > =20 > diff --git a/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLi= b.inf b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf > new file mode 100644 > index 0000000000..7586b31adf > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf > @@ -0,0 +1,36 @@ > +## @file > +# SSDT HPET Table Generator > +# > +# Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION =3D 1.27 > + BASE_NAME =3D AcpiSsdtHpetLib > + FILE_GUID =3D 85262912-AD7F-4EE0-8BB1-EE177275A54E > + VERSION_STRING =3D 1.0 > + MODULE_TYPE =3D DXE_DRIVER > + LIBRARY_CLASS =3D NULL|DXE_DRIVER > + CONSTRUCTOR =3D AcpiSsdtHpetLibConstructor > + DESTRUCTOR =3D AcpiSsdtHpetLibDestructor > + > +[Sources] > + SsdtHpetGenerator.c > + > +[Packages] > + DynamicTablesPkg/DynamicTablesPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + PcAtChipsetPkg/PcAtChipsetPkg.dec > + > +[LibraryClasses] > + AcpiHelperLib > + AmlLib > + BaseLib > + DebugLib > + PcdLib > + > +[Pcd] > + gPcAtChipsetPkgTokenSpaceGuid.PcdHpetBaseAddress Cf. [1], I think this should be removed along the dependency over the PcAtChipsetPkg dependency. > diff --git a/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenera= tor.c b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c > new file mode 100644 > index 0000000000..3d401204ae > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c > @@ -0,0 +1,266 @@ > +/** @file > + SSDT HPET Table Generator > + > + Copyright (c) 2017 - 2023, Arm Limited. All rights reserved. > + Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Reference(s): > + - ACPI 6.5 Specification, Aug 29, 2022 Is it possible to reference the HPET spec. with a link aswell if possible ? Same comment for the other generators with their respective spec. > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/** The Creator ID for the ACPI tables generated using > + the standard ACPI table generators. > +*/ > +#define TABLE_GENERATOR_CREATOR_ID_GENERIC SIGNATURE_32('D', 'Y', 'N', = 'T') > + > +/** This macro defines the HPET Table Generator revision. > +*/ > +#define HPET_GENERATOR_REVISION CREATE_REVISION (1, 0) Is it possible to move this definition down, next to the ACPI_TABLE_GENERAT= OR definition (just for consistency with other generators). Same remark for the other generators added in this patchset. > + > +#define SB_SCOPE "\\_SB_" > + > +/** Construct the SSDT HPET devices Table. > + > + This function invokes the Configuration Manager protocol interface > + to get the required hardware information for generating the ACPI > + table if required. > + > + If this function allocates any resources then they must be freed > + in the FreeXXXXTableResources function. > + > + @param [in] This Pointer to the table generator. > + @param [in] AcpiTableInfo Pointer to the ACPI Table Info. > + @param [in] CfgMgrProtocol Pointer to the Configuration Manager > + Protocol Interface. > + @param [out] Table Pointer to the constructed ACPI Table. > + > + @retval EFI_SUCCESS Table generated successfully. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object was not found. > + @retval EFI_BAD_BUFFER_SIZE The size returned by the Configuration > + Manager is less than the Object size for= the > + requested object. > +**/ > +STATIC > +EFI_STATUS > +BuildSsdtHpetTable ( > + IN CONST ACPI_TABLE_GENERATOR *CONST This, > + IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo, > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol= , > + OUT EFI_ACPI_DESCRIPTION_HEADER **CONST Table > + ) > +{ > + EFI_STATUS Status; > + AML_ROOT_NODE_HANDLE RootNode; > + AML_OBJECT_NODE_HANDLE ScopeNode; > + AML_OBJECT_NODE_HANDLE HpetNode; > + AML_OBJECT_NODE_HANDLE CrsNode; > + UINT32 EisaId; > + > + ASSERT (This !=3D NULL); > + ASSERT (AcpiTableInfo !=3D NULL); > + ASSERT (CfgMgrProtocol !=3D NULL); > + ASSERT (Table !=3D NULL); > + ASSERT (AcpiTableInfo->TableGeneratorId =3D=3D This->GeneratorID); > + ASSERT (AcpiTableInfo->AcpiTableSignature =3D=3D This->AcpiTableSignat= ure); > + > + Status =3D AddSsdtAcpiHeader ( > + CfgMgrProtocol, > + This, > + AcpiTableInfo, > + &RootNode > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status =3D AmlCodeGenScope (SB_SCOPE, RootNode, &ScopeNode); > + if (EFI_ERROR (Status)) { > + goto exit_handler; > + } > + > + Status =3D AmlCodeGenDevice ("HPET", ScopeNode, &HpetNode); > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + goto exit_handler; > + } > + > + Status =3D AmlGetEisaIdFromString ("PNP0103", &EisaId); > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + goto exit_handler; > + } > + > + Status =3D AmlCodeGenNameInteger ("_HID", EisaId, HpetNode, NULL); > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + goto exit_handler; > + } > + > + Status =3D AmlCodeGenNameInteger ("_UID", 0x00, HpetNode, NULL); > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + goto exit_handler; > + } > + > + Status =3D AmlCodeGenNameResourceTemplate ("_CRS", HpetNode, &CrsNode)= ; > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + goto exit_handler; > + } > + > + Status =3D AmlCodeGenRdMemory32Fixed (FALSE, PcdGet32 (PcdHpetBaseAddr= ess), SIZE_1KB, CrsNode, NULL); I didn't find anything in the spec. stating the memory range should be read= only or read-write. There are examples for both in edk2-platforms. Did you see some= thing specific going in the way of a read only configuration ? --- [1] - to avoid making the DynamicTablesPkg dependent over the PcAtChipsetPkg.de= c package (not that it is explicitly forbidden). - to avoid making the DynamicTablesPkg fetch configuration information from PCDs instead of CmObjects (there is a counter example, but this should= be avoided I think). would it be possible to create a HPET object ? The base address would be fe= tched through this object instead. The object would contain: - UINT32 BaseAddress - (maybe, depending on whether this is usefull) BOOLEAN IsReadOnly This would mean that the ConfigurationManager used for your platform will have the dependency over the PcAtChipsetPkg and populate the HPET CmObject with: PcdGet32 (PcdHpetBaseAddress) > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + goto exit_handler; > + } > + > + Status =3D AmlSerializeDefinitionBlock ( > + RootNode, > + Table > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: SSDT-HPET: Failed to Serialize SSDT Table Data." > + " Status =3D %r\n", > + Status > + )); > + goto exit_handler; > + } > + > +exit_handler: > + // Delete the RootNode and its attached children. > + return AmlDeleteTree (RootNode); > +} > + > +/** Free any resources allocated for constructing the > + SSDT HPET ACPI table. > + > + @param [in] This Pointer to the table generator. > + @param [in] AcpiTableInfo Pointer to the ACPI Table Info. > + @param [in] CfgMgrProtocol Pointer to the Configuration Manager > + Protocol Interface. > + @param [in, out] Table Pointer to the ACPI Table. > + > + @retval EFI_SUCCESS The resources were freed successfully. > + @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid. > +**/ > +STATIC > +EFI_STATUS > +FreeSsdtHpetTableResources ( > + IN CONST ACPI_TABLE_GENERATOR *CONST This, > + IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableI= nfo, > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProt= ocol, > + IN OUT EFI_ACPI_DESCRIPTION_HEADER **CONST Table > + ) > +{ > + ASSERT (This !=3D NULL); > + ASSERT (AcpiTableInfo !=3D NULL); > + ASSERT (CfgMgrProtocol !=3D NULL); > + ASSERT (AcpiTableInfo->TableGeneratorId =3D=3D This->GeneratorID); > + ASSERT (AcpiTableInfo->AcpiTableSignature =3D=3D This->AcpiTableSignat= ure); > + > + if ((Table =3D=3D NULL) || (*Table =3D=3D NULL)) { > + DEBUG ((DEBUG_ERROR, "ERROR: SSDT-HPET: Invalid Table Pointer\n")); > + ASSERT ((Table !=3D NULL) && (*Table !=3D NULL)); > + return EFI_INVALID_PARAMETER; > + } > + > + FreePool (*Table); > + *Table =3D NULL; > + return EFI_SUCCESS; > +} > + > +/** The interface for the SSDT HPET Table Generator. > +*/ > +STATIC > +CONST > +ACPI_TABLE_GENERATOR mSsdtPlatHpetGenerator =3D { > + // Generator ID > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtHpet), > + // Generator Description > + L"ACPI.STD.SSDT.HPET.GENERATOR", > + // ACPI Table Signature > + EFI_ACPI_6_5_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, > + // ACPI Table Revision - Unused > + 0, > + // Minimum ACPI Table Revision - Unused > + 0, > + // Creator ID > + TABLE_GENERATOR_CREATOR_ID_GENERIC, @Sami, shouldn't all the generators use this Creator Id instead of 'ARMH' ? If yes the definition of: TABLE_GENERATOR_CREATOR_ID_GENERIC should be placed there I think: DynamicTablesPkg/Include/AcpiTableGenerator.h > + // Creator Revision > + HPET_GENERATOR_REVISION, > + // Build Table function > + BuildSsdtHpetTable, > + // Free Resource function > + FreeSsdtHpetTableResources, > + // FreeSsdtPlatDevicesTableResources, Should be removed I think. > + // Extended build function not needed > + NULL, > + // Extended build function not implemented by the generator. > + // Hence extended free resource function is not required. > + NULL > +}; > + > +/** Register the Generator with the ACPI Table Factory. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS The Generator is registered. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_ALREADY_STARTED The Generator for the Table ID > + is already registered. > +**/ > +EFI_STATUS > +EFIAPI > +AcpiSsdtHpetLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D RegisterAcpiTableGenerator (&mSsdtPlatHpetGenerator); > + DEBUG ((DEBUG_INFO, "SSDT-HPET: Register Generator. Status =3D %r\n", = Status)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} > + > +/** Deregister the Generator from the ACPI Table Factory. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS The Generator is deregistered. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The Generator is not registered. > +**/ > +EFI_STATUS > +EFIAPI > +AcpiSsdtHpetLibDestructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D DeregisterAcpiTableGenerator (&mSsdtPlatHpetGenerator); > + DEBUG ((DEBUG_INFO, "SSDT-HPET: Deregister Generator. Status =3D %r\n"= , Status)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} Regards, Pierre -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116634): https://edk2.groups.io/g/devel/message/116634 Mute This Topic: https://groups.io/mt/104724534/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-