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,


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!