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.web09.3072.1628285756557219255 for ; Fri, 06 Aug 2021 14:35:56 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: jeremy.linton@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 387CD6D; Fri, 6 Aug 2021 14:35:56 -0700 (PDT) Received: from [192.168.122.166] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DCD203F40C; Fri, 6 Aug 2021 14:35:55 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 3/5] Platform/RaspberryPi: Add PCIe SSDT To: devel@edk2.groups.io, ardb@kernel.org Cc: Peter Batard , Ard Biesheuvel , Andrei Warkentin , Sunny Wang , Samer El-Haj-Mahmoud References: <20210805163551.488035-1-jeremy.linton@arm.com> <20210805163551.488035-4-jeremy.linton@arm.com> From: "Jeremy Linton" Message-ID: <068bba5f-aa3e-3036-d17d-8a444feb01ed@arm.com> Date: Fri, 6 Aug 2021 16:35:55 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, So I've tested with all the comments below and everything seems to be working fine, so no issues there. I will re-post RSN. Thanks, On 8/6/21 8:42 AM, Ard Biesheuvel via groups.io wrote: > On Thu, 5 Aug 2021 at 18:36, Jeremy Linton 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 >> --- >> 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 >> + >> +#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[] = {Yah, I will fix that. >> 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 >> > > > > >