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

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

[René Treffer]

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.

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;
       }
+      MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
+      mPciSegmentLastAccess = Address;
     }
   }
   return Base + Offset;
-- 
2.27.0

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

[Ard Biesheuvel]

On Thu, 11 Mar 2021 at 23:56, René Treffer <treffer+groups.io@measite.de> 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.
>
> 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.

Please include a signed-off-by line

> ---
>  .../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;
>        }
> +      MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
> +      mPciSegmentLastAccess = Address;

This looks ok to me, but I'd like Jeremy to confirm, please.

>      }
>    }
>    return Base + Offset;
> --
> 2.27.0
>
>

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

[René Treffer]

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.

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 (bus 2, device 1,3,5,7). The root port gets configured with
subordniate=0x2, blocking e.g. a booting linux from discovering devices
behind the switch.

Devices behind the switch work after lifting the device limit on busses
other than 0 and 1.

Signed-off-by: René Treffer <treffer@measite.de>
---
 .../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;
       }
+      MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
+      mPciSegmentLastAccess = Address;
     }
   }
   return Base + Offset;
-- 
2.27.0

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

[Jeremy Linton]

Hi,


First, thanks for the patches, this really helps!

On 3/12/21 12:32 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.
> 
> 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 (bus 2, device 1,3,5,7). The root port gets configured with
> subordniate=0x2, blocking e.g. a booting linux from discovering devices
> behind the switch.
> 
> Devices behind the switch work after lifting the device limit on busses
> other than 0 and 1.
> 
> Signed-off-by: René Treffer <treffer@measite.de>
> ---
>   .../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)) {

Did you try this with another switch plugged in downstream the first one?

This looks right, because presumably the first new switch is sending the 
UR's properly for its subordinate buses.

Dumping the segment last access reset is fine too.

Although, this set has unicode whitespace characters, which presumably 
Ard fixed up in the previous patch since I see it there too.


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

So,

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks again!

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

[treffer]

On 13.03.21 06:08, Jeremy Linton wrote:
> Hi,
>
>
> First, thanks for the patches, this really helps!
>
> On 3/12/21 12:32 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.
>>
>> 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 (bus 2, device 1,3,5,7). The root port gets configured with
>> subordniate=0x2, blocking e.g. a booting linux from discovering devices
>> behind the switch.
>>
>> Devices behind the switch work after lifting the device limit on busses
>> other than 0 and 1.
>>
>> Signed-off-by: René Treffer <treffer@measite.de>
>> ---
>>   .../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)) {
>
> Did you try this with another switch plugged in downstream the first one?

I don't have a second switch yet.

The topology I tried is:

# lspci -vnnt
-[0000:00]---00.0-[01-06]----00.0-[02-06]--+-01.0-[03]----00.0  Toshiba
Corporation XG4 NVMe SSD Controller [1179:0115]
                                           +-03.0-[04]----00.0  Renesas
Technology Corp. uPD720201 USB 3.0 Host Controller [1912:0014]
                                           +-05.0-[05]--
                                           \-07.0-[06]--

# lspci -nn
00:00.0 PCI bridge [0604]: Broadcom Inc. and subsidiaries BCM2711 PCIe
Bridge [14e4:2711] (rev 20)
01:00.0 PCI bridge [0604]: ASMedia Technology Inc. ASM1184e PCIe Switch
Port [1b21:1184]
02:01.0 PCI bridge [0604]: ASMedia Technology Inc. ASM1184e PCIe Switch
Port [1b21:1184]
02:03.0 PCI bridge [0604]: ASMedia Technology Inc. ASM1184e PCIe Switch
Port [1b21:1184]
02:05.0 PCI bridge [0604]: ASMedia Technology Inc. ASM1184e PCIe Switch
Port [1b21:1184]
02:07.0 PCI bridge [0604]: ASMedia Technology Inc. ASM1184e PCIe Switch
Port [1b21:1184]
03:00.0 Non-Volatile memory controller [0108]: Toshiba Corporation XG4
NVMe SSD Controller [1179:0115] (rev 01)
04:00.0 USB controller [0c03]: Renesas Technology Corp. uPD720201 USB
3.0 Host Controller [1912:0014] (rev 03)

So far I have tried (behind the switch):
- NVMe card. Works, even as a boot device after enabling the NVMe
package for the RPI4
- USB card. Works. upd72020x-load works for uploading firmware (I have
misflashed the ROM though)
- SATA controller ASM1062 + 2xJMB575 based (10 ports) - got detected,
haven't tried more
- LSI HBA, IT mode. Didn't work due to "Resource conflict", breaks NVMe
booting / PCIe bus

The last setup looks like a bug. The LSI HBA resource conflict shouldn't
stop other cards from functioning, right?
I can give that another try and upload the debug logs if anyone is
interested.

The 64MB BAR space default seems to hurt in this setup, but I haven't
been able to get more working.

Devices ordered for future testing:
- An old RADEON HD 3450 256MB (I don't expect that one to work)
- An old Intel I350 dual port network card (hoping to test option ROM
emulation)
- Another PCIe switch (will take 2+ weeks)

I hope to test most of that next week and would probably send another
patch if these things work as intended.

Regards,
  René

> This looks right, because presumably the first new switch is sending
> the UR's properly for its subordinate buses.
>
> Dumping the segment last access reset is fine too.
>
> Although, this set has unicode whitespace characters, which presumably
> Ard fixed up in the previous patch since I see it there too.
>
>
>>             return 0xFFFFFFFF;
>>         }
>> +      MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
>> +      mPciSegmentLastAccess = Address;
>>       }
>>     }
>>     return Base + Offset;
>>
>
> So,
>
> Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>
>
> Thanks again!

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

[Jeremy Linton]

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;
>