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.89.1617232852796246044 for ; Wed, 31 Mar 2021 16:20:53 -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 4CBFC31B; Wed, 31 Mar 2021 16:20:42 -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 B99C93F719; Wed, 31 Mar 2021 16:20:41 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/1] Silicon/Broadcom/Bcm27xx: Allow more than one device on pcie busses >1 To: =?UTF-8?Q?Ren=c3=a9_Treffer?= , devel@edk2.groups.io Cc: Pete Batard , Leif Lindholm , Ard Biesheuvel , "Andrei Warkentin (awarkentin@vmware.com)" , Samer El-Haj-Mahmoud References: From: "Jeremy Linton" Message-ID: Date: Wed, 31 Mar 2021 18:20:40 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi, On 3/11/21 4:56 PM, Ren=C3=A9 Treffer wrote: > There is only a single pcie port on the bcm2711 so limiting the number = of > devices to 1 worked as long as there is no way to add a pcie switch. I thought this got merged, but I just rebased and realized it didn't. Which is just as well, because there is a bug on the CM4 that should be=20 part of this patch. If nothing is pulled into the slot, then the PCIe=20 link is down. At that point, access to BUS > 0 will fault. So we need an=20 additional check. >=20 > On the compute module 4 it is possible to add a pcie switch (tested wit= h > asm1184e) which adds 5 new pcie busses. >=20 > In the current state the pci enumeration fails for the pcie switch > internal bus (usually bus 2, device 1,3,5,7). The root port gets > configured with > subordniate=3D0x2 after enumeration. That blocks e.g. linux from discov= ering > devices behind the switch. >=20 > Devices behind the switch work after lifting the device limit on busses > other than 0 and 1. > --- > =C2=A0.../Library/Bcm2711PciSegmentLib/PciSegmentLib.c=C2=A0=C2=A0 | 1= 4 +++++++------- > =C2=A01 file changed, 7 insertions(+), 7 deletions(-) >=20 > diff --git > a/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c > b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c > index 44ce3b4b99..4af9374d23 100644 > --- a/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentL= ib.c > +++ b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentL= ib.c > @@ -78,6 +78,7 @@ PciSegmentLibGetConfigBase ( > =C2=A0=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Base; > =C2=A0=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Offset; > =C2=A0=C2=A0 UINT32=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Dev; > +=C2=A0 UINT32=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Bus; > =20 > =C2=A0=C2=A0 Base =3D PCIE_REG_BASE; > =C2=A0=C2=A0 Offset =3D Address & 0xFFF;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 /* Pick off the 4k register offset */ > @@ -89,17 +90,16 @@ PciSegmentLibGetConfigBase ( > =C2=A0=C2=A0=C2=A0=C2=A0 Base +=3D PCIE_EXT_CFG_DATA; > =C2=A0=C2=A0=C2=A0=C2=A0 if (mPciSegmentLastAccess !=3D Address) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Dev =3D EFI_PCI_ADDR_DEV (Address= ); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Bus =3D EFI_PCI_ADDR_BUS (Address); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Scan things out directly rather= than translating the "bus" to > a device, etc.. > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * only we need to limit each bus = to a single device. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * There can only be a single devi= ce on bus 1 (downstream of root). > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Subsequent busses (behind a PCI= e switch) could have more. > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */ > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (Dev < 1) { > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MmioWrite32 (PC= IE_REG_BASE + PCIE_EXT_CFG_INDEX, Address); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mPciSegmentLast= Access =3D Address; > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else { > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mPciSegmentLast= Access =3D 0; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (Dev > 0 && (Bus =3D=3D 1 || Bus =3D= =3D 0)) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0x= FFFFFFFF; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } something like: if (!(MmioRead32 (PCI_REG_BASE + PCI_MISC_PCI_STATUS) & 0x30)) return 0xFFFFFFFF; //link down So, if you respin with that and the SOB Ard mentioned, I think its good.=20 I finally got a CM4 last week, and have been plugging various things=20 into it. This patch given the link check seems pretty solid, and=20 surprisingly with the PCI/SMC+linux we even have AER! That said, there is another link down "bug" in the constructor which=20 shows up with debug builds when we exit out with a !0 return code. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_C= FG_INDEX, Address); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mPciSegmentLastAccess =3D Address; > =C2=A0=C2=A0=C2=A0=C2=A0 } > =C2=A0=C2=A0 } > =C2=A0=C2=A0 return Base + Offset; >=20