From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH v2 5/7] Platform/ARM/N1Sdp: Introduce platform specific asl tables To: PierreGondois ,devel@edk2.groups.io From: "Khasim Mohammed" X-Originating-Location: Cambridge, England, GB (217.140.99.251) X-Originating-Platform: Linux Chrome 95 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Tue, 26 Oct 2021 10:46:13 -0700 References: <0046893d-21b8-2f3b-4193-370021c5dce2@arm.com> In-Reply-To: <0046893d-21b8-2f3b-4193-370021c5dce2@arm.com> Message-ID: <7340.1635270373555490731@groups.io> Content-Type: multipart/alternative; boundary="GALEz9dao4xizrfrU6ZB" --GALEz9dao4xizrfrU6ZB Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Tue, Oct 19, 2021 at 01:14 AM, PierreGondois wrote: >=20 > Hi Khasim, >=20 > 2 minor comments: >=20 > On 10/10/21 19:29, Khasim Mohammed via groups.io wrote: >=20 >> This patch creates Dsdt.asl, SsdtPci.asl and SsdtRemotePci.asl files >> to provide the platform specific APCI table entries. >>=20 >> Three PCI root ports are available on N1Sdp, PCI0 is the default root po= rt >>=20 >> PCI1 is the CCIX root port and PCI2 is the Remote host root port. >>=20 >> 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. >>=20 >> 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/ConfigurationManagerDxe/AslTable= s/Dsdt.asl >>=20 >> create mode 100644 >> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTable= s/SsdtPci.asl >>=20 >> create mode 100644 >> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTable= s/SsdtRemotePci.asl >>=20 >>=20 >> diff --git >> a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTab= les/Dsdt.asl >> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTab= les/Dsdt.asl >>=20 >> new file mode 100644 >> index 0000000000..0d7dde1a73 >> --- /dev/null >> +++ >> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTab= les/Dsdt.asl >>=20 >> @@ -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 >> +**/ >=20 > Is it possible to add a '@par Reference(s):' section in the header > (e.g.: > edk2/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser= .c) >=20 >=20 > I believe you used these documents: >=20 > -ACPI for CoreSight 1.1, Platform Design Document > -ACPI for Arm Components=C2=A0 1.0, Platform Design Document >=20 >=20 >> + >> +#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 >> + >=20 > [snip] >=20 >> diff --git >> a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTab= les/SsdtPci.asl >> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTab= les/SsdtPci.asl >>=20 >> new file mode 100644 >> index 0000000000..87a4fdaf88 >> --- /dev/null >> +++ >> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTab= les/SsdtPci.asl >>=20 >> @@ -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-INTC, >> 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 >> + } >=20 > Isn't it necessary to have an _OSC object as in > edk2-platforms/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationMan= agerDxe/AslTables/SsdtPci.asl >=20 > ? >=20 > Same question for SsdtRemotePci.asl >=20 > [snip] Hi Pierre, Yes, the _OSC object should have been implemented but unfortunately there a= re few PCIe Quirks and we are managing with additional patches to kernel. I= am not sure if any of the _OSC functionality would result in exporting und= esirable functionality due to these hardware errors. I have a made a note of this suggestion, we will experiment the _OSC object= and functionality and if everything works as expected then I will submit a= follow on patch later. Thanks. Regards, Khasim >=20 >=20 > Regards, >=20 > Pierre --GALEz9dao4xizrfrU6ZB Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Tue, Oct 19, 2021 at 01:14 AM, PierreGondois wrote:
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 port<= br />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-b= y: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Khasim Sye= d Mohammed <khasim.mohammed@arm.com>
Signed-off-by: Chandni Cher= ukuri <chandni.cherukuri@arm.com>
Signed-off-by: anukou01 <an= urag.koul@arm.com>
Signed-off-by: Manoj Kumar <manoj.kumar3@arm.= com>
---
.../AslTables/Dsdt.asl | 477 ++++++++++++++++++
= .../AslTables/SsdtPci.asl | 247 +++++++++
.../AslTables/SsdtRemotePci.= asl | 156 ++++++
3 files changed, 880 insertions(+)
create mode 1= 00644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTa= bles/Dsdt.asl
create mode 100644 Platform/ARM/N1Sdp/ConfigurationManag= er/ConfigurationManagerDxe/AslTables/SsdtPci.asl
create mode 100644 Pl= atform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/Ssd= tRemotePci.asl

diff --git a/Platform/ARM/N1Sdp/ConfigurationMana= ger/ConfigurationManagerDxe/AslTables/Dsdt.asl b/Platform/ARM/N1Sdp/Configu= rationManager/ConfigurationManagerDxe/AslTables/Dsdt.asl
new file mode= 100644
index 0000000000..0d7dde1a73
--- /dev/null
+++ b/Pla= tform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/Dsdt= .asl
@@ -0,0 +1,477 @@
+/** @file
+ Differentiated System De= scription Table Fields (DSDT)
+
+ Copyright (c) 2021, ARM Limited= . All rights reserved.<BR>
+
+ 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/Ior= tParser.c)

I believe you used these documents:

-ACPI = for CoreSight 1.1, Platform Design Document
-ACPI for Arm Components&n= bsp; 1.0, Platform Design Document

+
+#include "N1SdpAcpiHeader.h"
+#include "NeoverseN1= Soc.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_M= ASTER 1
+#define CS_LINK_SLAVE 0
+
[snip]
diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/Configurat= ionManagerDxe/AslTables/SsdtPci.asl b/Platform/ARM/N1Sdp/ConfigurationManag= er/ConfigurationManagerDxe/AslTables/SsdtPci.asl
new file mode 100644<= br />index 0000000000..87a4fdaf88
--- /dev/null
+++ b/Platform/AR= M/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtPci.asl<= br />@@ -0,0 +1,247 @@
+/** @file
+ Secondary System Description = Table (SSDT)
+
+ Copyright (c) 2021, ARM Limited. All rights rese= rved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent<= br />+**/
+
+#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 pro= vide additional
+ configuration information such as whether the interr= upt is Level or
+ Edge triggered, it is active High or Low, Shared or = Exclusive, etc.
+
+ In the second model, the PCI interrupts are h= ardwired to specific
+ interrupt inputs on the interrupt controller an= d 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 th= e
+ PCI interrupt is hardwired.
+
+ We use the first model w= ith link indirection to set the correct
+ interrupt type as PCI defaul= ts (Level Triggered, Active Low) are not
+ compatible with GICv2.
+*/
+#define LNK_DEVICE(Unique_Id, Link_Name, irq) \
+ Device(Li= nk_Name) { \
+ Name(_HID, EISAID("PNP0C0F")) \
+ Name(_UID, Uniqu= e_Id) \
+ Name(_PRS, ResourceTemplate() { \
+ Interrupt(ResourceP= roducer, 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-INTC, 3-I= NTD) */ \
+ Link, /* Interrupt allocated via Link device */ \
+ Z= ero /* global system interrupt number (no used) */ \
+}
+
+/= *
+ See Reference [1] 6.1.1
+ "High word-Device #, Low word-Funct= ion #. (for example, device 3,
+ function 2 is 0x00030002). To refer t= o all the functions on a device #,
+ use a function number of FFFF)."<= br />+*/
+#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, LN= KA, 201)
+ LNK_DEVICE(2, LNKB, 202)
+ LNK_DEVICE(3, LNKC, 203)+ LNK_DEVICE(4, LNKD, 204)
+ LNK_DEVICE(5, LNKE, 233)
+ LNK_DE= VICE(6, LNKF, 234)
+ LNK_DEVICE(7, LNKG, 235)
+ LNK_DEVICE(8, LNK= H, 236)
+
+ // PCI Root Complex
+ Device(PCI0) {
+ Name= (_HID, EISAID("PNP0A08")) // PCI Express Root Bridge
+ Name (_CID, EI= SAID("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 Comp= lex 0
+ Device (RP0) {
+ Name(_ADR, 0xF0000000) // Dev 0, Func 0<= br />+ }
Isn't it necessary to have an _OSC object as in
edk2-platforms/Platfor= m/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtPc= i.asl
?

Same question for SsdtRemotePci.asl

[sni= p]
Hi Pierre,

Yes, the _OSC object should have been implemented but= unfortunately there are few PCIe Quirks and we are managing with additiona= l patches to kernel. I am not sure if any of the _OSC functionality would r= esult in exporting undesirable functionality due to these hardware errors.&= nbsp;

I have a made a note of this suggestion, we will experimen= t the _OSC object and functionality and if everything works as expected the= n I will submit a follow on patch later. 

Thanks.

Regards,
Khasim



Regards,

Pierre
--GALEz9dao4xizrfrU6ZB--