Re: [PATCH 3/5] Platform/RaspberryPi: Add PCIe SSDT

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 5 Aug 2021 at 18:36, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Since we plan on toggling between XHCI and PCI the PCI
> root needs to be in its own SSDT. This is all thats needed
> of UEFI. The SMC conduit is provided directly to the running
> OS. When the OS detects this PCIe port, on a machine without
> a MADT it attempts to connect to the SMC conduit. The RPi
> definition doesn't have any power mgmt, and only provides
> a description of the root port.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  Platform/RaspberryPi/AcpiTables/AcpiTables.inf     |   3 +
>  Platform/RaspberryPi/AcpiTables/Pci.asl            | 237 +++++++++++++++++++++
>  Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c |   6 +
>  3 files changed, 246 insertions(+)
>  create mode 100644 Platform/RaspberryPi/AcpiTables/Pci.asl
>
> diff --git a/Platform/RaspberryPi/AcpiTables/AcpiTables.inf b/Platform/RaspberryPi/AcpiTables/AcpiTables.inf
> index f3e8d950c1..da2a6db85f 100644
> --- a/Platform/RaspberryPi/AcpiTables/AcpiTables.inf
> +++ b/Platform/RaspberryPi/AcpiTables/AcpiTables.inf
> @@ -39,6 +39,7 @@
>    Pptt.aslc
>    SsdtThermal.asl
>    Xhci.asl
> +  Pci.asl
>
>  [Packages]
>    ArmPkg/ArmPkg.dec
> @@ -59,6 +60,8 @@
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
>    gArmTokenSpaceGuid.PcdGicDistributorBase
>    gBcm27xxTokenSpaceGuid.PcdBcm27xxPciCpuMmioAdr
> +  gBcm27xxTokenSpaceGuid.PcdBcm27xxPciBusMmioAdr
> +  gBcm27xxTokenSpaceGuid.PcdBcm27xxPciBusMmioLen
>    gBcm27xxTokenSpaceGuid.PcdBcm27xxPciRegBase
>    gBcm27xxTokenSpaceGuid.PcdBcmGenetRegistersAddress
>    gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl
> new file mode 100644
> index 0000000000..34474f13ef
> --- /dev/null
> +++ b/Platform/RaspberryPi/AcpiTables/Pci.asl
> @@ -0,0 +1,237 @@
> +/** @file
> + *
> + *  Copyright (c) 2019 Linaro, Limited. All rights reserved.
> + *  Copyright (c) 2021 Arm
> + *
> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> + *
> + **/
> +
> +#include <IndustryStandard/Bcm2711.h>
> +
> +#include "AcpiTables.h"
> +
> +/*
> + * The following can be used to remove parenthesis from
> + * defined macros that the compiler complains about.
> + */
> +#define ISOLATE_ARGS(...)               __VA_ARGS__
> +#define REMOVE_PARENTHESES(x)           ISOLATE_ARGS x
> +
> +#define SANITIZED_PCIE_CPU_MMIO_WINDOW  REMOVE_PARENTHESES(PCIE_CPU_MMIO_WINDOW)
> +#define SANITIZED_PCIE_MMIO_LEN         REMOVE_PARENTHESES(PCIE_BRIDGE_MMIO_LEN)
> +#define SANITIZED_PCIE_PCI_MMIO_BEGIN   REMOVE_PARENTHESES(PCIE_TOP_OF_MEM_WIN)
> +
> +/*
> + * According to UEFI boot log for the VLI device on Pi 4.
> + */
> +#define RT_REG_LENGTH                 0x1000
> +
> +// copy paste job from juno
> +#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) */                           \
> +          }
> +#define ROOT_PRT_ENTRY(Pin, Link)   PRT_ENTRY(0x0000FFFF, Pin, Link)
> +

This can be done in a much simpler way - SynQuacer uses this, for instance

        Name (_PRT, Package () {
            Package () { 0xFFFF, 0, Zero, 222 },   // INTA
            Package () { 0xFFFF, 1, Zero, 222 },   // INTB
            Package () { 0xFFFF, 2, Zero, 222 },   // INTC
            Package () { 0xFFFF, 3, Zero, 222 },   // INTD
        })

> +DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2)
> +{
> +  Scope (\_SB_)
> +  {
> +
> +    Device (SCB0) {
> +      Name (_HID, "ACPI0004")
> +      Name (_UID, 0x0)

Even if this file and the xhci one should never be exposed to the OS
at the same time, can we please use unique UIDs?


> +      Name (_CCA, 0x0)
> +
> +      Method (_CRS, 0, Serialized) {
> +        // Container devices with _DMA must have _CRS,
> +        // meaning SCB0 to provide all resources that
> +        // PCI0 consumes (except interrupts).
> +        Name (RBUF, ResourceTemplate () {
> +            QWordMemory (ResourceProducer,
> +                ,
> +                MinFixed,
> +                MaxFixed,
> +                NonCacheable,
> +                ReadWrite,
> +                0x0,
> +                SANITIZED_PCIE_CPU_MMIO_WINDOW, // MIN
> +                SANITIZED_PCIE_CPU_MMIO_WINDOW, // MAX
> +                0x0,
> +                0x1,                            // LEN
> +                ,
> +                ,
> +                MMIO
> +                )
> +        })
> +        CreateQwordField (RBUF, MMIO._MAX, MMBE)
> +        CreateQwordField (RBUF, MMIO._LEN, MMLE)
> +        Add (MMBE, RT_REG_LENGTH - 1, MMBE)
> +        Add (MMLE, RT_REG_LENGTH - 1, MMLE)
> +        Return (RBUF)
> +      }
> +
> +      Name (_DMA, ResourceTemplate() {
> +        // PCIe can only DMA to first 3GB with early SOC's
> +        // But we keep the restriction on the later ones
> +        // To avoid DMA translation problems.
> +        QWordMemory (ResourceProducer,
> +            ,
> +            MinFixed,
> +            MaxFixed,
> +            NonCacheable,
> +            ReadWrite,
> +            0x0,
> +            0x0,        // MIN
> +            0xbfffffff, // MAX
> +            0x0,        // TRA
> +            0xc0000000, // LEN
> +            ,
> +            ,
> +            )
> +      })
> +
> +      //
> +      // PCI Root Complex
> +      //
> +      LNK_DEVICE(1, LNKA, 175)
> +      LNK_DEVICE(2, LNKB, 176)
> +      LNK_DEVICE(3, LNKC, 177)
> +      LNK_DEVICE(4, LNKD, 178)
> +
> +      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, 0)    // Mark the PCI noncoherent
> +
> +        // Root Complex 0
> +        Device (RP0) {
> +         Name(_ADR, 0xF0000000)    // Dev 0, Func 0
> +        }
> +

Can we just drop this?

> +        Name (_DMA, ResourceTemplate() {
> +          QWordMemory (ResourceConsumer,
> +            ,
> +            MinFixed,
> +            MaxFixed,
> +            NonCacheable,
> +            ReadWrite,
> +            0x0,
> +            0x0,        // MIN
> +            0xbfffffff, // MAX
> +            0x0,        // TRA
> +            0xc0000000, // LEN
> +            ,
> +            ,
> +            )
> +        })
> +

Do we need this method on the host bridge device as well as on the container?

> +        // PCI Routing Table
> +        Name(_PRT, Package() {
> +          ROOT_PRT_ENTRY(0, LNKA),   // INTA
> +          ROOT_PRT_ENTRY(1, LNKB),   // INTB
> +          ROOT_PRT_ENTRY(2, LNKC),   // INTC
> +          ROOT_PRT_ENTRY(3, LNKD),   // INTD
> +        })
> +        // Root complex resources
> +        Method (_CRS, 0, Serialized) {
> +          Name (RBUF, ResourceTemplate () {
> +            WordBusNumber ( // Bus numbers assigned to this root
> +              ResourceProducer,
> +              MinFixed, MaxFixed, PosDecode,
> +              0,   // AddressGranularity
> +              0,   // AddressMinimum - Minimum Bus Number
> +              255, // AddressMaximum - Maximum Bus Number
> +              0,   // AddressTranslation - Set to 0
> +              256  // RangeLength - Number of Busses
> +            )
> +
> +            QWordMemory ( // 32-bit BAR Windows in 64-bit addr
> +              ResourceProducer, PosDecode,
> +              MinFixed, MaxFixed,
> +              NonCacheable, ReadWrite,        //cacheable? is that right?
> +              0x00000000,                     // Granularity
> +              0,                              // SANITIZED_PCIE_PCI_MMIO_BEGIN
> +              1,                              // SANITIZED_PCIE_MMIO_LEN + SANITIZED_PCIE_PCI_MMIO_BEGIN
> +              SANITIZED_PCIE_CPU_MMIO_WINDOW, // SANITIZED_PCIE_PCI_MMIO_BEGIN - SANITIZED_PCIE_CPU_MMIO_WINDOW
> +              2                               // SANITIZED_PCIE_MMIO_LEN + 1
> +              ,,,MMI1,,TypeTranslation
> +            )
> +          }) // end Name(RBUF)
> +
> +          // Work around ASL's inability to add in a resource definition
> +          // or for that matter compute the min,max,len properly
> +          CreateQwordField (RBUF, MMI1._MIN, MMIB)
> +          CreateQwordField (RBUF, MMI1._MAX, MMIE)
> +          CreateQwordField (RBUF, MMI1._TRA, MMIT)
> +          CreateQwordField (RBUF, MMI1._LEN, MMIL)
> +          Add (MMIB, SANITIZED_PCIE_PCI_MMIO_BEGIN, MMIB)
> +          Add (SANITIZED_PCIE_MMIO_LEN, SANITIZED_PCIE_PCI_MMIO_BEGIN, MMIE)
> +          Subtract (MMIT, SANITIZED_PCIE_PCI_MMIO_BEGIN, MMIT)
> +          Add (SANITIZED_PCIE_MMIO_LEN, 1 , MMIL)
> +
> +          Return (RBUF)
> +        } // end Method(_CRS)
> +        //
> +        // OS Control Handoff
> +        //
> +        Name(SUPP, Zero) // PCI _OSC Support Field value
> +        Name(CTRL, Zero) // PCI _OSC Control Field value
> +
> +        // See [1] 6.2.10, [2] 4.5
> +        Method(_OSC,4) {
> +          // Note, This code is very similar to the code in the PCIe firmware
> +          // specification which can be used as a reference
> +          // Check for proper UUID
> +          If(LEqual(Arg0,ToUUID("33DB4D5B-1FF7-401C-9657-7441C03DD766"))) {
> +            // Create DWord-adressable fields from the Capabilities Buffer
> +            CreateDWordField(Arg3,0,CDW1)
> +            CreateDWordField(Arg3,4,CDW2)
> +            CreateDWordField(Arg3,8,CDW3)
> +            // Save Capabilities DWord2 & 3
> +            Store(CDW2,SUPP)
> +            Store(CDW3,CTRL)
> +            // Mask out Native HotPlug
> +            And(CTRL,0x1E,CTRL)
> +            // Always allow native PME, AER (no dependencies)
> +            // Never allow SHPC (no SHPC controller in this system)
> +            And(CTRL,0x1D,CTRL)
> +
> +            If(LNotEqual(Arg1,One)) { // Unknown revision
> +              Or(CDW1,0x08,CDW1)
> +            }
> +
> +            If(LNotEqual(CDW3,CTRL)) {  // Capabilities bits were masked
> +              Or(CDW1,0x10,CDW1)
> +            }
> +            // Update DWORD3 in the buffer
> +            Store(CTRL,CDW3)
> +            Return(Arg3)
> +          } Else {
> +            Or(CDW1,4,CDW1) // Unrecognized UUID
> +            Return(Arg3)
> +          }
> +        } // End _OSC
> +      } // PCI0
> +    } //end SCB0
> +  } //end scope sb
> +} //end definition block
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index 7c5786303d..4c40820858 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -821,6 +821,12 @@ STATIC CONST NAMESPACE_TABLES SdtTables[] = {
>      PcdToken(PcdXhciPci),
>      NULL
>    },
> +  {
> +    SIGNATURE_64 ('R', 'P', 'I', '4', 'P', 'C', 'I', 'E'),
> +    PcdToken(PcdXhciPci),
> +    0,
> +    NULL
> +  },
>  #endif
>    { // DSDT
>      SIGNATURE_64 ('R', 'P', 'I', 0, 0, 0, 0, 0),
> --
> 2.13.7
>