From: "PierreGondois" <pierre.gondois@arm.com>
To: devel@edk2.groups.io, kuqin12@gmail.com
Cc: Joe Lopez <joelopez@microsoft.com>, Sami Mujawar <Sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space
Date: Thu, 28 Jul 2022 15:07:23 +0200 [thread overview]
Message-ID: <eb6d4606-ad0c-985c-c1da-5e70d29a5207@arm.com> (raw)
In-Reply-To: <20220728043147.395-6-kuqin12@gmail.com>
Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
Thanks!
On 7/28/22 06:31, Kun Qin via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>
> Certain OSes will complain if the ECAM config space is not reserved in
> the ACPI namespace.
>
> This change adds a function to reserve PNP motherboard resources for a
> given PCI node.
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>
> Notes:
> v2:
> - Only create RES0 after config space checking [Pierre]
>
> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 169 ++++++++++++++++++++
> 1 file changed, 169 insertions(+)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index ceffe2838c03..c03550baabf2 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -616,6 +616,167 @@ GeneratePciCrs (
> return Status;
> }
>
> +/** Generate a Pci Resource Template to hold Address Space Info
> +
> + @param [in] PciNode RootNode of the AML tree.
> + @param [in, out] CrsNode CRS node of the AML tree to populate.
I think CrsNode is an 'out' only parameter. Also the description of PciNode
seems incorrect.
> +
> + @retval EFI_SUCCESS The function completed successfully.
> + @retval EFI_INVALID_PARAMETER Invalid input parameter.
> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +PopulateBasicPciResObjects (
Would it be possible to rename it:
GenerateMotherboardDevice()
or with a name related to 'motherboard' ?
> + IN AML_OBJECT_NODE_HANDLE PciNode,
> + IN OUT AML_OBJECT_NODE_HANDLE *CrsNode
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 EisaId;
> + AML_OBJECT_NODE_HANDLE ResNode;
> +
> + if (CrsNode == NULL) {
> + ASSERT (0);
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // ASL: Device (PCIx) {}
> + Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + // ASL: Name (_HID, EISAID ("PNP0C02"))
> + Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboard Resources */
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + // ASL: Name (_CRS, ResourceTemplate () {})
> + Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + return Status;
> +}
> +
> +/** Generate a Pci Resource Template to hold Address Space Info
> +
> + @param [in] Generator The SSDT Pci generator.
> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> + Protocol interface.
> + @param [in] PciInfo Pci device information.
> + @param [in, out] PciNode RootNode of the AML tree to populate.
> +
> + @retval EFI_SUCCESS The function completed successfully.
> + @retval EFI_INVALID_PARAMETER Invalid parameter.
> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GeneratePciRes (
Would it be possible to rename it:
ReserveEcamSpace()
or with a name related to 'ecam' ?
> + IN ACPI_PCI_GENERATOR *Generator,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> + IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
> + IN OUT AML_OBJECT_NODE_HANDLE PciNode
> + )
> +{
> + EFI_STATUS Status;
> + AML_OBJECT_NODE_HANDLE CrsNode;
> + BOOLEAN Translation;
> + UINT32 Index;
> + CM_ARM_OBJ_REF *RefInfo;
> + UINT32 RefCount;
> + CM_ARM_PCI_ADDRESS_MAP_INFO *AddrMapInfo;
> + BOOLEAN IsPosDecode;
> +
> + // Get the array of CM_ARM_OBJ_REF referencing the
> + // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
> + Status = GetEArmObjCmRef (
> + CfgMgrProtocol,
> + PciInfo->AddressMapToken,
> + &RefInfo,
> + &RefCount
> + );
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + for (Index = 0; Index < RefCount; Index++) {
> + // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
> + Status = GetEArmObjPciAddressMapInfo (
> + CfgMgrProtocol,
> + RefInfo[Index].ReferenceToken,
> + &AddrMapInfo,
> + NULL
> + );
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
> + if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
> + IsPosDecode = TRUE;
> + } else {
> + IsPosDecode = FALSE;
> + }
I think this should be done in:
case PCI_SS_CONFIG:
to only do it when necessary.
> +
> + switch (AddrMapInfo->SpaceCode) {
> + case PCI_SS_CONFIG:
> + Status = PopulateBasicPciResObjects (PciNode, &CrsNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + break;
> + }
> +
> + Status = AmlCodeGenRdQWordMemory (
> + FALSE,
> + IsPosDecode,
> + TRUE,
> + TRUE,
> + FALSE, // non-cacheable
> + TRUE,
> + 0,
> + AddrMapInfo->PciAddress,
> + AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> + Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
> + AddrMapInfo->AddressSize,
> + 0,
> + NULL,
> + 0,
> + TRUE,
> + CrsNode,
> + NULL
> + );
> + break;
> + default:
> + break;
> + } // switch
> +
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> + }
> +
> + return Status;
> +}
> +
> /** Generate a Pci device.
>
> @param [in] Generator The SSDT Pci generator.
> @@ -702,9 +863,17 @@ GeneratePciDevice (
> return Status;
> }
>
> + // Add the PNP Motherboard Resources Device to reserve ECAM space
> + Status = GeneratePciRes (Generator, CfgMgrProtocol, PciInfo, PciNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> // Add the template _OSC method.
> Status = AddOscMethod (PciInfo, PciNode);
> ASSERT_EFI_ERROR (Status);
> +
> return Status;
> }
>
next prev parent reply other threads:[~2022-07-28 13:07 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
2022-07-28 4:31 ` [PATCH v2 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf Kun Qin
2022-07-28 4:31 ` [PATCH v2 2/6] DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing Kun Qin
2022-07-28 4:31 ` [PATCH v2 3/6] DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers Kun Qin
2022-07-28 4:31 ` [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables Kun Qin
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
2022-07-29 5:00 ` Kun Qin
2022-07-29 15:11 ` PierreGondois
2022-07-28 4:31 ` [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space Kun Qin
2022-07-28 13:07 ` PierreGondois [this message]
2022-07-29 5:01 ` [edk2-devel] " Kun Qin
2022-07-28 4:31 ` [PATCH v2 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config Kun Qin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eb6d4606-ad0c-985c-c1da-5e70d29a5207@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox