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.6758.1634631268126346698 for ; Tue, 19 Oct 2021 01:14:28 -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 86776D6E; Tue, 19 Oct 2021 01:14:21 -0700 (PDT) Received: from [192.168.1.16] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id F313E3F73D; Tue, 19 Oct 2021 01:14:19 -0700 (PDT) Message-ID: <0046893d-21b8-2f3b-4193-370021c5dce2@arm.com> Date: Tue, 19 Oct 2021 09:14:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [edk2-devel] [PATCH v2 5/7] Platform/ARM/N1Sdp: Introduce platform specific asl tables To: devel@edk2.groups.io, khasim.mohammed@arm.com Cc: nd@arm.com, Sami Mujawar , Chandni Cherukuri , anukou01 , Manoj Kumar References: <20211010182956.13526-1-khasim.mohammed@arm.com> <20211010182956.13526-6-khasim.mohammed@arm.com> From: "PierreGondois" In-Reply-To: <20211010182956.13526-6-khasim.mohammed@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Khasim, 2 minor comments: On 10/10/21 19:29, Khasim Mohammed via groups.io wrote: > This patch creates Dsdt.asl, SsdtPci.asl and SsdtRemotePci.asl files > to provide the platform specific APCI table entries. > > Three PCI root ports are available on N1Sdp, PCI0 is the default root p= ort > PCI1 is the CCIX root port and PCI2 is the Remote host root port. > > The Remote host specific entries are defined in a separate file > SsdtRemotePci.asl to avoid confusions with other PCI entries > and for better readability and understanding of interfaces. > > Signed-off-by: Sami Mujawar > Signed-off-by: Khasim Syed Mohammed > Signed-off-by: Chandni Cherukuri > Signed-off-by: anukou01 > Signed-off-by: Manoj Kumar > --- > .../AslTables/Dsdt.asl | 477 ++++++++++++++++++ > .../AslTables/SsdtPci.asl | 247 +++++++++ > .../AslTables/SsdtRemotePci.asl | 156 ++++++ > 3 files changed, 880 insertions(+) > create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/Configurati= onManagerDxe/AslTables/Dsdt.asl > create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/Configurati= onManagerDxe/AslTables/SsdtPci.asl > create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/Configurati= onManagerDxe/AslTables/SsdtRemotePci.asl > > diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManag= erDxe/AslTables/Dsdt.asl b/Platform/ARM/N1Sdp/ConfigurationManager/Config= urationManagerDxe/AslTables/Dsdt.asl > new file mode 100644 > index 0000000000..0d7dde1a73 > --- /dev/null > +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/A= slTables/Dsdt.asl > @@ -0,0 +1,477 @@ > +/** @file > + Differentiated System Description Table Fields (DSDT) > + > + Copyright (c) 2021, ARM Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ Is it possible to add a '@par Reference(s):' section in the header (e.g.: edk2/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser= .c) I believe you used these documents: -ACPI for CoreSight 1.1, Platform Design Document -ACPI for Arm Components=C2=A0 1.0, Platform Design Document > + > +#include "N1SdpAcpiHeader.h" > +#include "NeoverseN1Soc.h" > + > +#define ACPI_GRAPH_REV 0 > +#define ACPI_GRAPH_UUID "ab02a46b-74c7-45a2-bd68-f7d344ef2153" > + > +#define CORESIGHT_GRAPH_UUID "3ecbc8b6-1d0e-4fb3-8107-e627f805c6cd" > + > +#define CS_LINK_MASTER 1 > +#define CS_LINK_SLAVE 0 > + [snip] > diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManag= erDxe/AslTables/SsdtPci.asl b/Platform/ARM/N1Sdp/ConfigurationManager/Con= figurationManagerDxe/AslTables/SsdtPci.asl > new file mode 100644 > index 0000000000..87a4fdaf88 > --- /dev/null > +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/A= slTables/SsdtPci.asl > @@ -0,0 +1,247 @@ > +/** @file > + Secondary System Description Table (SSDT) > + > + Copyright (c) 2021, ARM Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include "N1SdpAcpiHeader.h" > + > +/* > + See ACPI 6.4 Section 6.2.13 > + > + There are two ways that _PRT can be used. > + > + In the first model, a PCI Link device is used to provide additional > + configuration information such as whether the interrupt is Level or > + Edge triggered, it is active High or Low, Shared or Exclusive, etc. > + > + In the second model, the PCI interrupts are hardwired to specific > + interrupt inputs on the interrupt controller and are not > + configurable. In this case, the Source field in _PRT does not > + reference a device, but instead contains the value zero, and the > + Source Index field contains the global system interrupt to which the > + PCI interrupt is hardwired. > + > + We use the first model with link indirection to set the correct > + interrupt type as PCI defaults (Level Triggered, Active Low) are not > + compatible with GICv2. > +*/ > +#define LNK_DEVICE(Unique_Id, Link_Name, irq) = \ > + Device(Link_Name) { = \ > + Name(_HID, EISAID("PNP0C0F")) = \ > + Name(_UID, Unique_Id) = \ > + Name(_PRS, ResourceTemplate() { = \ > + Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { irq } = \ > + }) = \ > + Method (_CRS, 0) { Return (_PRS) } = \ > + Method (_SRS, 1) { } = \ > + Method (_DIS) { } = \ > +} > + > +#define PRT_ENTRY(Address, Pin, Link) = \ > + Package (4) { = \ > + Address, /* uses the same format as _ADR */ = \ > + Pin, /* The PCI pin number of the device (0-INTA, 1-INTB, 2-I= NTC, 3-INTD) */ \ > + Link, /* Interrupt allocated via Link device */ = \ > + Zero /* global system interrupt number (no used) */ = \ > +} > + > +/* > + See Reference [1] 6.1.1 > + "High word-Device #, Low word-Function #. (for example, device 3, > + function 2 is 0x00030002). To refer to all the functions on a device= #, > + use a function number of FFFF)." > +*/ > +#define ROOT_PRT_ENTRY(Pin, Link) PRT_ENTRY(0x0000FFFF, Pin, Link) = // Device 0 for Bridge. > + > +DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "N1Sdp", > + EFI_ACPI_ARM_OEM_REVISION) > +{ > + Scope (_SB) { > + > + // PCI Root Complex > + LNK_DEVICE(1, LNKA, 201) > + LNK_DEVICE(2, LNKB, 202) > + LNK_DEVICE(3, LNKC, 203) > + LNK_DEVICE(4, LNKD, 204) > + LNK_DEVICE(5, LNKE, 233) > + LNK_DEVICE(6, LNKF, 234) > + LNK_DEVICE(7, LNKG, 235) > + LNK_DEVICE(8, LNKH, 236) > + > + // PCI Root Complex > + Device(PCI0) { > + Name (_HID, EISAID("PNP0A08")) // PCI Express Root Bridge > + Name (_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge > + Name (_SEG, Zero) // PCI Segment Group number > + Name (_BBN, Zero) // PCI Base Bus Number > + Name (_CCA, 1) // Cache Coherency Attribute > + > + // Root Complex 0 > + Device (RP0) { > + Name(_ADR, 0xF0000000) // Dev 0, Func 0 > + } Isn't it necessary to have an _OSC object as in edk2-platforms/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationMan= agerDxe/AslTables/SsdtPci.asl ? Same question for SsdtRemotePci.asl [snip] Regards, Pierre