Re: [edk2-platforms][PATCH 1/1] Silicon/Broadcom/Bcm27xx: Allow more than one device on pcie busses >1

["Jeremy Linton" <jeremy.linton@arm.com>]

Hi,

On 3/11/21 4:56 PM, René 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 
part of this patch. If nothing is pulled into the slot, then the PCIe 
link is down. At that point, access to BUS > 0 will fault. So we need an 
additional check.

> 
> On the compute module 4 it is possible to add a pcie switch (tested with
> asm1184e) which adds 5 new pcie busses.
> 
> 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=0x2 after enumeration. That blocks e.g. linux from discovering
> devices behind the switch.
> 
> Devices behind the switch work after lifting the device limit on busses
> other than 0 and 1.
> ---
>   .../Library/Bcm2711PciSegmentLib/PciSegmentLib.c   | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> 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/PciSegmentLib.c
> +++ b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c
> @@ -78,6 +78,7 @@ PciSegmentLibGetConfigBase (
>     UINT64        Base;
>     UINT64        Offset;
>     UINT32        Dev;
> +  UINT32        Bus;
>   
>     Base = PCIE_REG_BASE;
>     Offset = Address & 0xFFF;         /* Pick off the 4k register offset */
> @@ -89,17 +90,16 @@ PciSegmentLibGetConfigBase (
>       Base += PCIE_EXT_CFG_DATA;
>       if (mPciSegmentLastAccess != Address) {
>         Dev = EFI_PCI_ADDR_DEV (Address);
> +      Bus = EFI_PCI_ADDR_BUS (Address);
>         /*
> -       * Scan things out directly rather than translating the "bus" to
> a device, etc..
> -       * only we need to limit each bus to a single device.
> +       * There can only be a single device on bus 1 (downstream of root).
> +       * Subsequent busses (behind a PCIe switch) could have more.
>          */
> -      if (Dev < 1) {
> -          MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
> -          mPciSegmentLastAccess = Address;
> -      } else {
> -          mPciSegmentLastAccess = 0;
> +      if (Dev > 0 && (Bus == 1 || Bus == 0)) {
>             return 0xFFFFFFFF;
>         }

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. 
I finally got a CM4 last week, and have been plugging various things 
into it. This patch given the link check seems pretty solid, and 
surprisingly with the PCI/SMC+linux we even have AER!

That said, there is another link down "bug" in the constructor which 
shows up with debug builds when we exit out with a !0 return code.




> +      MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
> +      mPciSegmentLastAccess = Address;
>       }
>     }
>     return Base + Offset;
>