From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id E14CBAC1475 for ; Thu, 2 May 2024 16:36:46 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=nP5n7WkkVQACBNdNCtvQKexYoxN9wydoe68+eRb+UkM=; 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:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20240206; t=1714667805; v=1; b=kWNi3TMed0fSurC2K8Ey81zb9CtePS38IzEepkONwI2a/zdzc3iN2xLvtZc7wotxXPyOnH15 lirXPzwN9aU+wxuKC4XqFT+WNg4VI36XyQlnEEHunks/7KrVdq9ZmiczKfs4HerIfKXwCk6tVCE v8zPvlqQqadoQ7CSg/pr0uXGAStVQZsnfTt5ctDZfL7JXvJkoJVlkE+c2BWEVV6eJnyO2j3CKso bQLeWIhBX5OWENTKU9EH1ALiZwfoLkwtIMVisoL3OUNJDng2iJiEHA5UMESEqcxrxReM/bSMx3j 0CX3ScELV4THU6C03RT+hRPPUhsXN16Jxnz4ciF+x9YHA== X-Received: by 127.0.0.2 with SMTP id 2K2wYY7687511xzFmBRoNN1p; Thu, 02 May 2024 09:36:45 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.1104.1714667804487716435 for ; Thu, 02 May 2024 09:36:44 -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 A7F462F4; Thu, 2 May 2024 09:37:09 -0700 (PDT) X-Received: from [10.34.111.156] (e126645.nice.Arm.com [10.34.111.156]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 073613F793; Thu, 2 May 2024 09:36:40 -0700 (PDT) Message-ID: Date: Thu, 2 May 2024 18:36:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: "PierreGondois" Subject: Re: [edk2-devel] [RESEND PATCH v4 2/5] DynamicTablesPkg: Adds ACPI HPET Table generator To: Abdul Lateef Attar , devel@edk2.groups.io Cc: Sami Mujawar References: <788e6fea909b4422c9df46652eac03f3dcf6e4f7.1714369949.git.AbdulLateef.Attar@amd.com> In-Reply-To: <788e6fea909b4422c9df46652eac03f3dcf6e4f7.1714369949.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 Resent-Date: Thu, 02 May 2024 09:36:44 -0700 Resent-From: pierre.gondois@arm.com Reply-To: devel@edk2.groups.io,pierre.gondois@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 6cgl9799zZO8XCzt5EPmwnvSx7686176AA= 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=kWNi3TMe; 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 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io Hello Abdul, some comments on the patch: On 4/29/24 08:03, Abdul Lateef Attar wrote: > Adds generic ACPI HPET table generator library. > Register/Deregister HPET table. > Update the HPET table during boot as per specification. >=20 > Cc: Sami Mujawar > Cc: Pierre Gondois > Signed-off-by: Abdul Lateef Attar > --- > DynamicTablesPkg/DynamicTables.dsc.inc | 2 + > DynamicTablesPkg/Include/AcpiTableGenerator.h | 1 + > .../Include/ArchNameSpaceObjects.h | 9 + > .../Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf | 31 +++ > .../Library/Acpi/AcpiHpetLib/HpetGenerator.c | 246 ++++++++++++++++++ > 5 files changed, 289 insertions(+) > create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLi= b.inf > create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenera= tor.c >=20 > diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/Dy= namicTables.dsc.inc > index 92f3a138e4..b2ef36eb8a 100644 > --- a/DynamicTablesPkg/DynamicTables.dsc.inc > +++ b/DynamicTablesPkg/DynamicTables.dsc.inc > @@ -34,6 +34,7 @@ > # Generators > # > DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf > + DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf I think this should be moved to the following section if the table if for I= ntel arch. Like this it would allow avoiding to build the generator for other archs. [Components.IA32, Components.X64] # # Generators # ... also if the table is Intel specific, maybe the generator should be placed u= nder: DynamicTablesPkg/Library/Acpi/X64/ (or a better folder name) also I think the CmObject should be moved to: X64NameSpaceObjects.h > =20 > [Components.IA32, Components.X64] > # > @@ -42,6 +43,7 @@ > DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDx= e.inf { > > NULL|DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf > + NULL|DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > } > =20 > [Components.ARM, Components.AARCH64] > diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTable= sPkg/Include/AcpiTableGenerator.h > index d0eda011c3..18b5f99f47 100644 > --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h > +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h > @@ -99,6 +99,7 @@ typedef enum StdAcpiTableId { > EStdAcpiTableIdSsdtCpuTopology, ///< SSDT Cpu Topology > EStdAcpiTableIdSsdtPciExpress, ///< SSDT Pci Express G= enerator > EStdAcpiTableIdPcct, ///< PCCT Generator > + EStdAcpiTableIdHpet, ///< HPET Generator > EStdAcpiTableIdMax > } ESTD_ACPI_TABLE_ID; > =20 > diff --git a/DynamicTablesPkg/Include/ArchNameSpaceObjects.h b/DynamicTab= lesPkg/Include/ArchNameSpaceObjects.h > index b421c4cd29..b90e573a88 100644 > --- a/DynamicTablesPkg/Include/ArchNameSpaceObjects.h > +++ b/DynamicTablesPkg/Include/ArchNameSpaceObjects.h > @@ -39,6 +39,7 @@ typedef enum ArchObjectID { > EArchObjFadtArmBootArch, ///< 11 - ARM boot arch information > EArchObjFadtHypervisorVendorId, ///< 12 - Hypervisor vendor identity = information > EArchObjFadtMiscInfo, ///< 13 - Legacy fields; RTC, latency= , flush stride, etc > + EArchObjHpetBaseAddress, ///< 14 - HPET Base Address > EArchObjMax > } E_ARCH_OBJECT_ID; > =20 > @@ -214,4 +215,12 @@ typedef struct CmArchFadtMiscInfo { > UINT8 Century; > } CM_ARCH_FADT_MISC_INFO; > =20 > +/** A structure that describes the > + HPET Base Address. > + > + ID: EArchObjHpetBaseAddress > +*/ > +typedef struct CmArchHpetBaseAddress { > + UINT64 BaseAddress; > +} CM_ARCH_HPET_BASE_ADDRESS; Would it make sense to have these fields for the CmObj ? typedef struct CmArchHpetBaseAddress { UINT32 EventTimerBlockId; UINT32 BaseAddressLower32Bit; UINT8 HpetNumber; UINT16 MainCounterMinimumClockTickInPeriodicMode; UINT8 PageProtectionAndOemAttribute; } CM_ARCH_HPET_BASE_ADDRESS; The reason being that: - Ideally the DynamicTables generators should just generate ACPI tables from the CmObject they are given. The ConfigurationManager being a database providing information. Doing Mmio accesses to populate the ACPI tables breaks a bit the desi= gn: the generators would also become a source of information. - If the Mmio accesses are possible from the generator, they should also be possible from the ConfigurationManager. - Imagining a platform with an invalid value for the MainCounterMinimumCloc= kTickInPeriodicMode, as the generator is generic to all platforms, it becomes hard to patc= h this. The ConfigurationManager is supposed to be platform specific, so if the MainCounterMinimumClockTickInPeriodicMode register is invalid, it is easy to patch this specific ConfigurationManager and keep a gen= eric HPET generator. (I am refering to MmioRead32 accesses from [1]) > #endif > diff --git a/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf b/= DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > new file mode 100644 > index 0000000000..f0441107fc > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > @@ -0,0 +1,31 @@ > +## @file > +# 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 AcpiHpetLib > + FILE_GUID =3D 4E75F653-C356-48B3-B32C-D1B901ECF90A > + VERSION_STRING =3D 1.0 > + MODULE_TYPE =3D DXE_DRIVER > + LIBRARY_CLASS =3D NULL|DXE_DRIVER > + CONSTRUCTOR =3D AcpiHpetLibConstructor > + DESTRUCTOR =3D AcpiHpetLibDestructor > + > +[Sources] > + HpetGenerator.c > + > +[Packages] > + DynamicTablesPkg/DynamicTablesPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + IoLib > + > diff --git a/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c b/= DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c > new file mode 100644 > index 0000000000..937879b7b3 > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c > @@ -0,0 +1,246 @@ > +/** @file > + 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 > + - HPET spec, version 1.0a > + http://www.intel.com/content/dam/www/public/us/en/documents/technica= l-specifications/software-developers-hpet-spec-1-0a.pdf > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/// > +/// HPET General Register Offsets > +/// > +#define HPET_GENERAL_CAPABILITIES_ID_OFFSET 0x000 > + > +/** 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') > + > +/** The AcpiHpet is a template EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE= _HEADER > + structure used for generating the HPET Table. > +*/ > +STATIC > +EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER mAcpiHpet =3D { > + ACPI_HEADER ( > + EFI_ACPI_6_5_HIGH_PRECISION_EVENT_TIMER_TABLE_SIGNATURE, > + EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER, > + EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_REVISION > + ), > + // EventTimerBlockId > + 0, > + // BaseAddressLower32Bit > + { EFI_ACPI_6_5_SYSTEM_MEMORY, 0,0, EFI_ACP= I_RESERVED_BYTE, 0 }, > + // HpetNumber > + 0, > + // MainCounterMinimumClockTickInPeriodicMode > + 0, > + // PageProtectionAndOemAttribute > + EFI_ACPI_NO_PAGE_PROTECTION > +}; > + > +/** This macro expands to a function that retrieves the > + HPET base address from the Configuration Manager. > +*/ > +GET_OBJECT_LIST ( > + EObjNameSpaceArch, > + EArchObjHpetBaseAddress, > + CM_ARCH_HPET_BASE_ADDRESS > + ); > + > +/** Construct the HPET table. > + > + This function invokes the Configuration Manager protocol interface > + to get the required hardware information for generating the ACPI > + table. > + > + 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 > +BuildHpetTable ( > + 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; > + CM_ARCH_HPET_BASE_ADDRESS *HpetBaseAddress; > + > + 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); > + > + if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) || > + (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision)) > + { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: HPET: Requested table revision =3D %d, is not supported." > + "Supported table revision: Minimum =3D %d, Maximum =3D %d\n", > + AcpiTableInfo->AcpiTableRevision, > + This->MinAcpiTableRevision, > + This->AcpiTableRevision > + )); > + return EFI_INVALID_PARAMETER; > + } > + > + *Table =3D NULL; > + > + Status =3D AddAcpiHeader ( > + CfgMgrProtocol, > + This, > + (EFI_ACPI_DESCRIPTION_HEADER *)&mAcpiHpet, > + AcpiTableInfo, > + sizeof (EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER) > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: HPET: Failed to add ACPI header. Status =3D %r\n", > + Status > + )); > + return Status; > + } > + > + // Get the HPET Base Address from the Platform Configuration Manager > + Status =3D GetEArchObjHpetBaseAddress ( > + CfgMgrProtocol, > + CM_NULL_TOKEN, > + &HpetBaseAddress, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: HPET: Failed to get HPET Base Address. Status =3D %r\n", > + Status > + )); > + return Status; > + } > + > + mAcpiHpet.BaseAddressLower32Bit.Address =3D HpetBaseAddres= s->BaseAddress; > + mAcpiHpet.EventTimerBlockId =3D MmioRead32 ((U= INTN)(HpetBaseAddress->BaseAddress + HPET_GENERAL_CAPABILITIES_ID_OFFSET)); > + mAcpiHpet.MainCounterMinimumClockTickInPeriodicMode =3D (UINT16)MmioRe= ad32 ((UINTN)(HpetBaseAddress->BaseAddress + HPET_GENERAL_CAPABILITIES_ID_O= FFSET + 4)); [1] > + *Table =3D (EFI_ACPI_DESC= RIPTION_HEADER *)&mAcpiHpet; > + > + return Status; > +} > + > +/** This macro defines the HPET Table Generator revision. > +*/ > +#define HPET_GENERATOR_REVISION CREATE_REVISION (1, 0) > + > +/** The interface for the HPET Table Generator. > +*/ > +STATIC > +CONST > +ACPI_TABLE_GENERATOR mHpetGenerator =3D { > + // Generator ID > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdHpet), > + // Generator Description > + L"ACPI.STD.HPET.GENERATOR", > + // ACPI Table Signature > + EFI_ACPI_6_5_HIGH_PRECISION_EVENT_TIMER_TABLE_SIGNATURE, > + // ACPI Table Revision supported by this Generator > + EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_REVISION, > + // Minimum supported ACPI Table Revision > + EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_REVISION, > + // Creator ID > + TABLE_GENERATOR_CREATOR_ID_GENERIC, > + // Creator Revision > + HPET_GENERATOR_REVISION, > + // Build Table function > + BuildHpetTable, > + // No additional resources are allocated by the generator. > + // Hence the Free Resource function is not required. > + NULL, > + // 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 > +AcpiHpetLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D RegisterAcpiTableGenerator (&mHpetGenerator); > + DEBUG ((DEBUG_INFO, "HPET: Register Generator. Status =3D %r\n", Statu= s)); > + 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 > +AcpiHpetLibDestructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D DeregisterAcpiTableGenerator (&mHpetGenerator); > + DEBUG ((DEBUG_INFO, "HPET: Deregister Generator. Status =3D %r\n", Sta= tus)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} -=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 (#118529): https://edk2.groups.io/g/devel/message/118529 Mute This Topic: https://groups.io/mt/105796051/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-