Re: [edk2-devel] [PATCH v2 5/7] Platform/ARM/N1Sdp: Introduce platform specific asl tables

["Khasim Mohammed" <khasim.mohammed@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 7025 bytes --]

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
>> 
>> 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 <sami.mujawar@arm.com>
>> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
>> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
>> Signed-off-by: anukou01 <anurag.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 100644
>> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/Dsdt.asl
>> 
>> create mode 100644
>> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtPci.asl
>> 
>> create mode 100644
>> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtRemotePci.asl
>> 
>> 
>> diff --git
>> a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/Dsdt.asl
>> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/Dsdt.asl
>> 
>> new file mode 100644
>> index 0000000000..0d7dde1a73
>> --- /dev/null
>> +++
>> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/Dsdt.asl
>> 
>> @@ -0,0 +1,477 @@
>> +/** @file
>> + Differentiated System Description 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/IortParser.c)
> 
> 
> I believe you used these documents:
> 
> -ACPI for CoreSight 1.1, Platform Design Document
> -ACPI for Arm Components  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/ConfigurationManagerDxe/AslTables/SsdtPci.asl
>> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtPci.asl
>> 
>> new file mode 100644
>> index 0000000000..87a4fdaf88
>> --- /dev/null
>> +++
>> b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtPci.asl
>> 
>> @@ -0,0 +1,247 @@
>> +/** @file
>> + Secondary System Description Table (SSDT)
>> +
>> + Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
>> +
>> + 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
>> + }
> 
> Isn't it necessary to have an _OSC object as in
> edk2-platforms/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/AslTables/SsdtPci.asl
> 
> ?
> 
> Same question for SsdtRemotePci.asl
> 
> [snip]

Hi Pierre,

Yes, the _OSC object should have been implemented but unfortunately there are 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 undesirable 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

> 
> 
> Regards,
> 
> Pierre

[-- Attachment #2: Type: text/html, Size: 7316 bytes --]