Re: [edk2-devel] [PATCH 4/5] Silicon/Broadcom/Bcm27xx: Tweak PCIe for CM4

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

Hi,

On 8/6/21 11:04 AM, Andrei Warkentin wrote:
> Ok, I misunderstood the patch set (I thought the PciHostBridgeLib itself would eventually move to DEN0115).
> 
> I still think that (in general) would be a good idea - if not for the benefit of the Pi, then for the next upstreamed platform where you could avoid implementing custom config access code...

Right, the only bit that goes away is the PciSegmentLibGetConfigBase() 
code to be replaced by the SMC call. Which I will do, but I think its 
better to fix to this one and make that a separate patch-set ideally 
with another platform in parallel.


> 
> Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
> 
> --
> Andrei Warkentin,
> Arm Enablement Architect,
> Cloud Platform Business Unit, VMware
> ________________________________
> From: Andrei Warkentin <awarkentin@vmware.com>
> Sent: Friday, August 6, 2021 7:02 PM
> To: devel@edk2.groups.io <devel@edk2.groups.io>; jeremy.linton@arm.com <jeremy.linton@arm.com>
> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; René Treffer <treffer+groups.io@measite.de>
> Subject: Re: [edk2-devel] [PATCH 4/5] Silicon/Broadcom/Bcm27xx: Tweak PCIe for CM4
> 
> Hi Jeremy,
> 
> Is any of this still conceptually necessary if we adopt the SMCCC interface within UEFI?
> 
> Instead of assuming the first downstream bus is bus 1, could you read the secondary BN from the RP?
> 
> --
> Andrei Warkentin,
> Arm Enablement Architect,
> Cloud Platform Business Unit, VMware
> ________________________________
> From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Jeremy Linton via groups.io <jeremy.linton=arm.com@groups.io>
> Sent: Thursday, August 5, 2021 7:35 PM
> To: devel@edk2.groups.io <devel@edk2.groups.io>
> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>; René Treffer <treffer+groups.io@measite.de>
> Subject: [edk2-devel] [PATCH 4/5] Silicon/Broadcom/Bcm27xx: Tweak PCIe for CM4
> 
> The CM4 has an actual pcie slot, so we need to move the linkup
> check to the configuration probe logic. Further the device
> restriction logic needs to be relaxed to support downstream
> PCIe switches.
> 
> Suggested-by: René Treffer <treffer+groups.io@measite.de>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>   .../Bcm2711PciHostBridgeLibConstructor.c           |  5 -----
>   .../Library/Bcm2711PciSegmentLib/PciSegmentLib.c   | 24 +++++++++++++++-------
>   2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciHostBridgeLib/Bcm2711PciHostBridgeLibConstructor.c b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciHostBridgeLib/Bcm2711PciHostBridgeLibConstructor.c
> index 8587d2d36d..4d4c584726 100644
> --- a/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciHostBridgeLib/Bcm2711PciHostBridgeLibConstructor.c
> +++ b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciHostBridgeLib/Bcm2711PciHostBridgeLibConstructor.c
> @@ -204,11 +204,6 @@ Bcm2711PciHostBridgeLibConstructor (
>     } while (((Data & 0x30) != 0x030) && (Timeout));
>     DEBUG ((DEBUG_VERBOSE, "PCIe link ready (status=%x) Timeout=%d\n", Data, Timeout));
> 
> -  if ((Data & 0x30) != 0x30) {
> -    DEBUG ((DEBUG_ERROR, "PCIe link not ready (status=%x)\n", Data));
> -    return EFI_DEVICE_ERROR;
> -  }
> -
>     if ((Data & 0x80) != 0x80) {
>       DEBUG ((DEBUG_ERROR, "PCIe link not in RC mode (status=%x)\n", Data));
>       return EFI_UNSUPPORTED;
> diff --git a/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c
> index 44ce3b4b99..3ccc131eab 100644
> --- a/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c
> +++ b/Silicon/Broadcom/Bcm27xx/Library/Bcm2711PciSegmentLib/PciSegmentLib.c
> @@ -78,6 +78,8 @@ PciSegmentLibGetConfigBase (
>     UINT64        Base;
>     UINT64        Offset;
>     UINT32        Dev;
> +  UINT32        Bus;
> +  UINT32        Data;
> 
>     Base = PCIE_REG_BASE;
>     Offset = Address & 0xFFF;         /* Pick off the 4k register offset */
> @@ -89,17 +91,25 @@ 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) can have more.
>          */
> -      if (Dev < 1) {
> -          MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
> -          mPciSegmentLastAccess = Address;
> -      } else {
> -          mPciSegmentLastAccess = 0;
> +      if (Dev > 0 && (Bus < 2)) {
>             return 0xFFFFFFFF;
>         }
> +
> +      /* Don't probe slots if the link is down */
> +      Data = MmioRead32 (PCIE_REG_BASE + PCIE_MISC_PCIE_STATUS);
> +      if ((Data & 0x30) != 0x30) {
> +          DEBUG ((DEBUG_ERROR, "PCIe link not ready (status=%x)\n", Data));
> +          return 0xFFFFFFFF;
> +      }
> +
> +      MmioWrite32 (PCIE_REG_BASE + PCIE_EXT_CFG_INDEX, Address);
> +      mPciSegmentLastAccess = Address;
>       }
>     }
>     return Base + Offset;
> --
> 2.13.7
> 
> 
> 
> 
> 
> 
>