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.29779.1657551740819416550 for ; Mon, 11 Jul 2022 08:02:21 -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 C60B71596; Mon, 11 Jul 2022 08:02:20 -0700 (PDT) Received: from [10.57.43.54] (unknown [10.57.43.54]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 275C73F70D; Mon, 11 Jul 2022 08:02:18 -0700 (PDT) Message-ID: <40d296fe-1208-c217-8a48-6a37cf242c46@arm.com> Date: Mon, 11 Jul 2022 17:01:56 +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 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add support for override protocol To: Jeff Brasen , "devel@edk2.groups.io" Cc: "Sami.Mujawar@arm.com" , "Alexei.Fedorov@arm.com" References: <45abcaa79586b025205269d17957537d613c9bc2.1656603839.git.jbrasen@nvidia.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hello Jeff, To be sure I understand correctly, the SsdtPcieGenerator currently allows to generate something like this: Scope(_SB) { Device(PCI0) { Device (D00_) { // Device 0 Name(_ADR, 0x0000FFFF) } Method(_OSC,4) { [...] // Generic _OSC method } } } What you want to do is: 1. Use another _OSC method 2. Add more information in the D00_ object, i.e.: - a _DSD object - a _RST method 3. As _RST is added in 2., also add a _RST method to PCI0 Is it correct ? Or is there some other information you want to add ? Regards, Pierre On 7/8/22 17:36, Jeff Brasen wrote: > I think this would work. Instead of GeneratePciDeviceInfo the function we would want to break out would be GeneratePciSlots. I'll work on changing my patch series to this. Unless you have a better name will call this library SsdtPciSupportLib and place in under Library/Common > > Going to also pass PciInfo into AddOscMethod in this new approach in case client needs to have different _OSC per controller (And to GeneratePciSlots as well). > > Thanks, > Jeff > >> -----Original Message----- >> From: Pierre Gondois >> Sent: Monday, July 4, 2022 2:48 AM >> To: Jeff Brasen ; devel@edk2.groups.io >> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com >> Subject: Re: [PATCH 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add >> support for override protocol >> >> External email: Use caution opening links or attachments >> >> >> Hello Jeff, >> I think it would ideally be better to have the code adding/replacing the >> methods/objects you noted inside the edk2/edk2-platfoms repositories. The >> methods/objects that you want to replace seem to only concern: >> -the objects available in the 'Device (PCIx)' node -the _OSC method >> >> If a library with the following two methods (extracted from >> SsdtPcieLibArm.inf) was created, would it be sufficient for all the >> replacements you want to do ? >> Like this we would have a generic implementation and specific ones. >> >> EFI_STATUS >> EFIAPI >> GeneratePciDeviceInfo ( >> IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo, >> IN UINT32 Uid, >> IN OUT AML_OBJECT_NODE_HANDLE PciNode >> ) >> >> EFI_STATUS >> EFIAPI >> AddOscMethod ( >> IN OUT AML_OBJECT_NODE_HANDLE PciNode >> ) >> >> Regards, >> Pierre >> >> On 7/1/22 17:52, Jeff Brasen wrote: >>> Currently we do the following in this call. >>> >>> Remove and replace the _OSC method on certain targets. I originally >>> had this pass the template over but removed that when I added this >>> more generic override support Add a _RST method to the root port >>> device sub node Add a _DSD for device properties Conditionally add an >>> entry for the device attached to the PCIe bus if we need to add >>> properties or _RST methods. This will likely eventually move to >>> another driver (which is the purpose of patch 2/4 in this series) >>> >>> I figured trying to get the generator to handle that would be more difficult >> as these would be hard to generalize. >>> >>> >>> Thanks, >>> Jeff >>> >>>> -----Original Message----- >>>> From: Pierre Gondois >>>> Sent: Friday, July 1, 2022 6:40 AM >>>> To: Jeff Brasen ; devel@edk2.groups.io >>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com >>>> Subject: Re: [PATCH 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add >>>> support for override protocol >>>> >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> Hello Jeff, >>>> Is it possible to know what the UpdateTable() function would do ? >>>> Maybe it would be possible to integrate the alternative >>>> implementation without adding a new protocol. >>>> >>>> Regards, >>>> Pierre >>>> >>>> On 6/30/22 17:48, Jeff Brasen wrote: >>>>> Some platfoms may want to modify the ACPI table created. >>>>> Add support for protocol that can provide an alternative >> implementation. >>>>> >>>>> Signed-off-by: Jeff Brasen >>>>> --- >>>>> DynamicTablesPkg/DynamicTablesPkg.dec | 3 + >>>>> .../Protocol/SsdtPcieOverrideProtocol.h | 63 >> +++++++++++++++++++ >>>>> .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 31 ++++++++- >>>>> .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf | 4 ++ >>>>> 4 files changed, 98 insertions(+), 3 deletions(-) >>>>> create mode 100644 >>>>> DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h >>>>> >>>>> diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec >>>>> b/DynamicTablesPkg/DynamicTablesPkg.dec >>>>> index a890a048be..bb66bdaf14 100644 >>>>> --- a/DynamicTablesPkg/DynamicTablesPkg.dec >>>>> +++ b/DynamicTablesPkg/DynamicTablesPkg.dec >>>>> @@ -43,6 +43,9 @@ >>>>> # Dynamic Table Factory Protocol GUID >>>>> gEdkiiDynamicTableFactoryProtocolGuid = { 0x91d1e327, 0xfe5a, >>>>> 0x49b8, { 0xab, 0x65, 0xe, 0xce, 0x2d, 0xdb, 0x45, 0xec } } >>>>> >>>>> + # Protocol to override PCI SSDT table generation >>>>> + gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid = { 0x962e8b44, >>>>> + 0x23b3, 0x41da, { 0x9f, 0x36, 0xca, 0xde, 0x68, 0x49, 0xfb, 0xf6 } >>>>> + } >>>>> + >>>>> [PcdsFixedAtBuild] >>>>> >>>>> # Maximum number of Custom ACPI Generators diff --git >>>>> a/DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h >>>>> b/DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h >>>>> new file mode 100644 >>>>> index 0000000000..29568a0159 >>>>> --- /dev/null >>>>> +++ b/DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h >>>>> @@ -0,0 +1,63 @@ >>>>> +/** @file >>>>> + >>>>> + Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved. >>>>> + >>>>> + SPDX-License-Identifier: BSD-2-Clause-Patent >>>>> + >>>>> +**/ >>>>> + >>>>> +#ifndef SSDT_PCIE_OVERRIDE_PROTOCOL_H_ #define >>>>> +SSDT_PCIE_OVERRIDE_PROTOCOL_H_ >>>>> + >>>>> +#include >>>>> +#include >>>>> + >>>>> +/** This macro defines the SSDT PCI Override Protocol GUID. >>>>> + >>>>> + GUID: {D85A4835-5A82-4894-AC02-706F43D5978E} >>>>> +*/ >>>>> +#define EDKII_SSDT_PCI_OVERRIDE_PROTOCOL_GUID \ >>>>> + { 0x962e8b44, 0x23b3, 0x41da, \ >>>>> + { 0x9f, 0x36, 0xca, 0xde, 0x68, 0x49, 0xfb, 0xf6 } \ >>>>> + }; >>>>> + >>>>> +/** >>>>> + Forward declarations: >>>>> +*/ >>>>> +typedef struct SsdtOverridePciProtocol >>>>> +EDKII_SSDT_PCI_OVERRIDE_PROTOCOL; >>>>> + >>>>> +/** The UpdateTable function allows the override protocol to update >> the >>>>> + * PCIe SSDT table prior to being created. >>>>> + >>>>> + @param [in] This Pointer to the SSDT PCI Override Protocol. >>>>> + @param [in] PciInfo The PCIe configuration info for this node. >>>>> + @param [in] Uid UID that was selected for this PCIe node. >>>>> + @param [in, out] PciNode Pointer to the PCI node of this ACPI table. >>>>> + >>>>> + @retval EFI_SUCCESS Success. >>>>> + @retval EFI_INVALID_PARAMETER A parameter is invalid. >>>>> + @retval EFI_DEVICE_ERROR Failed to update the table. >>>>> +**/ >>>>> +typedef >>>>> +EFI_STATUS >>>>> +(EFIAPI *EDKII_SSDT_PCI_OVERRIDE_PROTOCOL_UPDATE_TABLE)( >>>>> + IN CONST EDKII_SSDT_PCI_OVERRIDE_PROTOCOL *CONST This, >>>>> + IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo, >>>>> + IN UINT32 Uid, >>>>> + IN OUT AML_ROOT_NODE_HANDLE *PciNode >>>>> + ); >>>>> + >>>>> +/** The EDKII_CONFIGURATION_MANAGER_PROTOCOL structure >>>> describes the >>>>> + Configuration Manager Protocol interface. >>>>> +*/ >>>>> +typedef struct SsdtOverridePciProtocol { >>>>> + /** The interface used to update the ACPI table for PCI. >>>>> + */ >>>>> + EDKII_SSDT_PCI_OVERRIDE_PROTOCOL_UPDATE_TABLE >> UpdateTable; >>>>> +} EDKII_SSDT_PCI_OVERRIDE_PROTOCOL; >>>>> + >>>>> +/** The SSDT PCI Override Protocol GUID. >>>>> +*/ >>>>> +extern EFI_GUID gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid; >>>>> + >>>>> +#endif // SSDT_PCIE_OVERRIDE_PROTOCOL_H_ >>>>> diff --git >>>>> >>>> >> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera >>>> t >>>>> or.c >>>>> >>>> >> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera >>>> t >>>>> or.c >>>>> index c5b23d91d0..d5982c24ff 100644 >>>>> --- >>>>> >>>> >> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera >>>> t >>>>> or.c >>>>> +++ >>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGen >>>>> +++ erator.c >>>>> @@ -20,6 +20,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> >>>>> // Module specific include files. >>>>> @@ -30,6 +31,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include "SsdtPcieGenerator.h" >>>>> >>>>> @@ -798,9 +800,10 @@ GeneratePciDevice ( >>>>> { >>>>> EFI_STATUS Status; >>>>> >>>>> - CHAR8 AslName[AML_NAME_SEG_SIZE + 1]; >>>>> - AML_OBJECT_NODE_HANDLE ScopeNode; >>>>> - AML_OBJECT_NODE_HANDLE PciNode; >>>>> + CHAR8 AslName[AML_NAME_SEG_SIZE + 1]; >>>>> + AML_OBJECT_NODE_HANDLE ScopeNode; >>>>> + AML_OBJECT_NODE_HANDLE PciNode; >>>>> + EDKII_SSDT_PCI_OVERRIDE_PROTOCOL *OverrideProtocol; >>>>> >>>>> ASSERT (Generator != NULL); >>>>> ASSERT (CfgMgrProtocol != NULL); @@ -860,6 +863,28 @@ >>>>> GeneratePciDevice ( >>>>> >>>>> // Add the template _OSC method. >>>>> Status = AddOscMethod (PciNode); >>>>> + if (EFI_ERROR (Status)) { >>>>> + ASSERT (0); >>>>> + return Status; >>>>> + } >>>>> + >>>>> + Status = gBS->LocateProtocol ( >>>>> + &gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid, >>>>> + NULL, >>>>> + (VOID **)&OverrideProtocol >>>>> + ); >>>>> + if (!EFI_ERROR (Status)) { >>>>> + Status = OverrideProtocol->UpdateTable ( >>>>> + OverrideProtocol, >>>>> + PciInfo, >>>>> + Uid, >>>>> + PciNode >>>>> + ); } else { >>>>> + // Not an error if override protocol is not found >>>>> + Status = EFI_SUCCESS; >>>>> + } >>>>> + >>>>> ASSERT_EFI_ERROR (Status); >>>>> return Status; >>>>> } >>>>> diff --git >>>>> >>>> >> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm. >>>>> inf >>>>> >>>> >> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm. >>>>> inf >>>>> index 431e32a777..8e916f15e9 100644 >>>>> --- >>>>> >>>> >> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm. >>>>> inf >>>>> +++ >>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLib >>>>> +++ Arm.inf >>>>> @@ -30,6 +30,10 @@ >>>>> AcpiHelperLib >>>>> AmlLib >>>>> BaseLib >>>>> + UefiBootServicesTableLib >>>>> >>>>> [Pcd] >>>>> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid >>>>> + >>>>> +[Protocols] >>>>> + gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid