From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f179.google.com (mail-pg1-f179.google.com [209.85.215.179]) by mx.groups.io with SMTP id smtpd.web11.4516.1659070912390714234 for ; Thu, 28 Jul 2022 22:01:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=RjWwVbAv; spf=pass (domain: gmail.com, ip: 209.85.215.179, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f179.google.com with SMTP id l193so2769999pge.9 for ; Thu, 28 Jul 2022 22:01:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=jduDa3cVJ7OL7pO8POnIxXCBjHdHiFojrsCnHGMtPhw=; b=RjWwVbAvKA7pV0stMg1Yb/Juld+HX1v/Lh56Sj4VagpbX2TgFR6YuHzY713yEC+JIw L+rz5CSg4egG8wc9jtk8l5DNoNQ800ucCVkvyrWNp85+s9UfZ41DGAca5/0pU7g1uZeW akeqiU3vFJmstasbyLlcgBkcDyjUnNV0V9fupA3qRACYAtChIdg23jE1t513H8TFJlVk VfFWGqEoynqKnqI6V7LPOEkFJmc23xmCFQi2dA6IMSSrHiYNy568vgvxd8k6zxaoCTJy 79yhPNcAS3cmBwDcGfU+biGuntRYpc5TKaw9eiNRsyfH+RF00HpGXSUcHqG6qH8Rgr12 axXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=jduDa3cVJ7OL7pO8POnIxXCBjHdHiFojrsCnHGMtPhw=; b=EZxNhy557mypcN7sW/Tkf2W/E2axqkVwmuFlM6TeiAQit6BR2WRTEUrpC+Xdw4rdQ/ 0cyxs/2SEYbqnR+fOlmZIhUBQ5qZPbQ00SZ1yuS5USGe3FF1HfYIsJdaDBHXN6g3ilfx FiLgMURvzzcUQccRz1e8NfdMYp/9mL2M+zUkWJvqZlror5cXmOAZa0hyW1bolruR/a+7 m13v1jFLuaNP8yLQtfY41MSqgfdK+BV9hZEd5X84vl20EaUeOxnS/0D6E6+77pBxijbB T93d6wGGKgUJY468RAq8DibUJ/5iDmHbCMZMuBoE5vBnYDqqFApShsg9HvonE1bTqsW4 l/wg== X-Gm-Message-State: AJIora+UIg3id1p5zxsdLkuQcnPDjO2gFOFCPzwUuYU/iA5xJriemmj7 q98If0+cRmFDQ9Kn/a2j3/j2AyQZGQE= X-Google-Smtp-Source: AGRyM1vHQO86Wc26sXod9RxL2+soQpCHJxROesIdPsFkUVQZ6uwQFSbswwjl28q8lrPAD74p0Hj9OQ== X-Received: by 2002:a63:5c22:0:b0:41a:56e8:27d with SMTP id q34-20020a635c22000000b0041a56e8027dmr1620073pgb.22.1659070911630; Thu, 28 Jul 2022 22:01:51 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:c8a3:f4a8:3813:162d? ([2001:4898:80e8:8:48be:f4a8:3813:162d]) by smtp.gmail.com with ESMTPSA id y2-20020a17090a2b4200b001ecd954f3b6sm4874535pjc.7.2022.07.28.22.01.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 28 Jul 2022 22:01:51 -0700 (PDT) Message-ID: <257a2613-93d9-6d18-a20d-005cc89e6a99@gmail.com> Date: Thu, 28 Jul 2022 22:01:50 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space To: Pierre Gondois , devel@edk2.groups.io Cc: Joe Lopez , Sami Mujawar References: <20220728043147.395-1-kuqin12@gmail.com> <20220728043147.395-6-kuqin12@gmail.com> From: "Kun Qin" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Pierre, Thank you for the suggestions. I will update the patch per your feedback and send for review in v3. Regards, Kun On 7/28/2022 6:07 AM, Pierre Gondois wrote: > Hello Kun, > With the changes below: > Reviewed-by: Pierre Gondois > > 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 >> Signed-off-by: Kun Qin >> --- >> >> 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; >>   }