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

["PierreGondois" <pierre.gondois@arm.com>]

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]


Regards,

Pierre