Re: [edk2-devel] [PATCH v4 1/3] Silicon/ARM/NeoverseN1Soc: Port PCI Segment Library

["PierreGondois" <pierre.gondois@arm.com>]

Hi Khassim,

Thanks for the new serie. I have some comments:


On 12/14/21 8:43 PM, Khasim Mohammed via groups.io wrote:
> The BasePCISegment Library in MdePkg doesn't allow configuring
> multiple segments required for PCIe and CCIX root port
> enumeration. Therefore, a custom PCI Segment library is adapted
> from SynQuacerPciSegmentLib and ported for N1Sdp.
>
> In addition to this, the hardware has few other limitations which affects
> the access to the PCIe root port:
>   1. ECAM space is not contiguous, root port ECAM (BDF = 0:0:0) is isolated
>      from rest of the downstream hierarchy ECAM space.
>   2. Root port ECAM space is not capable of 8bit/16bit writes.
>   3. A slave error is generated when host accesses the configuration
>      space of non-available device or unimplemented function on a
>      given bus.
>
> The description of the workarounds included for these limitations can
> be found in the corresponding files of this patch.
>
> Change-Id: I0a124b0ea2fb7a8ee652de2d66b977d848c509b4
The 'Change-Id' should be removed I think.
> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
> ---
>  .../Library/PciSegmentLib/PciSegmentLib.c     | 1643 +++++++++++++++++
>  .../Library/PciSegmentLib/PciSegmentLib.inf   |   38 +
>  2 files changed, 1681 insertions(+)
>  create mode 100644 Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c
>  create mode 100644 Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.inf
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c b/Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c
> new file mode 100644
> index 0000000000..3a3cf3008a
> --- /dev/null
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c
> @@ -0,0 +1,1643 @@
> +/** @file
> +  PCI Segment Library for N1SDP SoC with multiple RCs
> +
> +  Having two distinct root complexes is not supported by the standard
> +  set of PciLib/PciExpressLib/PciSegmentLib, this PciSegmentLib
> +  reimplements the functionality to support multiple root ports on
> +  different segment numbers.
> +
> +  On the NeoverseN1Soc, a slave error is generated when host accesses the
> +  configuration space of non-available device or unimplemented function on a
> +  given bus. So this library introduces a workaround using IsBdfValid(),
> +  to return 0xFFFFFFFF for all such access.
> +
> +  In addition to this, the hardware has two other limitations which affect
> +  access to the PCIe root port:
> +    1. ECAM space is not contiguous, root port ECAM (BDF = 0:0:0) is isolated
> +       from rest of the downstream hierarchy ECAM space.
> +    2. Root port ECAM space is not capable of 8bit/16bit writes.
> +  The description of the workarounds included for these limitations can
> +  be found in the comments below.
> +
> +  Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PciLib.h>
I think the library can be removed now.
> +#include <Library/PciSegmentLib.h>
> +#include <NeoverseN1Soc.h>
> +
> +typedef enum {
> +  PciCfgWidthUint8      = 0,
> +  PciCfgWidthUint16,
> +  PciCfgWidthUint32,
> +  PciCfgWidthMax
> +} PCI_CFG_WIDTH;
> +
> +/**
> + Assert the validity of a PCI Segment address.
> + A valid PCI Segment address should not contain 1's in bits 28..31 and 48..63
> +
> +  @param A The address to validate.
> +  @param M Additional bits to assert to be zero.
> +**/
> +#define ASSERT_INVALID_PCI_SEGMENT_ADDRESS(A,M) \
> +  ASSERT (((A) & (0xffff0000f0000000ULL | (M))) == 0)
> +
> +#define BUS_OFFSET      20
> +#define DEV_OFFSET      15
> +#define FUNC_OFFSET     12
> +#define REG_OFFSET      4096
> +
> +/**
> +  Assert the validity of a PCI address. A valid PCI address should contain 1's.
> +
> +  @param  A The address to validate.
> +
> +**/
> +#define ASSERT_INVALID_PCI_ADDRESS(A) \
> +  ASSERT (((A) & ~0xffffffff) == 0)
I think the macro is not used anymore.
> +
> +#define EFI_PCIE_ADDRESS(bus, dev, func, reg) \
> +  (UINT64) ( \
> +  (((UINTN) bus)   << BUS_OFFSET)  | \
> +  (((UINTN) dev)   << DEV_OFFSET)  | \
> +  (((UINTN) func)  << FUNC_OFFSET) | \
> +  (((UINTN) (reg)) <  REG_OFFSET ?   \
> +   ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) (reg), 32))))
> +
> +#define GET_PCIE_BASE_ADDRESS(Address)  (Address & 0xF8000000)
> +
> +/* Root port Entry, BDF Entries Count */
> +#define BDF_TABLE_ENTRY_SIZE    4
> +#define BDF_TABLE_HEADER_COUNT  2
> +#define BDF_TABLE_HEADER_SIZE   8
> +
> +/* BDF table offsets for PCIe */
> +#define PCIE_BDF_TABLE_OFFSET   0
> +#define CCIX_BDF_TABLE_OFFSET   (16 * 1024)
> +
> +#define GET_BUS_NUM(Address)    (((Address) >> 20) & 0x7F)
> +#define GET_DEV_NUM(Address)    (((Address) >> 15) & 0x1F)
> +#define GET_FUNC_NUM(Address)   (((Address) >> 12) & 0x07)
> +#define GET_REG_NUM(Address)    ((Address) & 0xFFF)
> +
> +CONST STATIC UINTN mDummyConfigData = 0xFFFFFFFF;
> +
> +/**
> +  Check if the requested PCI address is a valid BDF address.
> +
> +  SCP performs the initial bus scan and prepares a table of valid BDF addresses
> +  and shares them through non-trusted SRAM. This function validates if the PCI
> +  address from any PCI request falls within the table of valid entries. If not,
> +  this function will return 0xFFFFFFFF. This is a workaround to avoid bus fault
> +  that happens when accessing unavailable PCI device due to RTL bug.
> +
> +  @param  Address The address that encodes the PCI Bus, Device, Function and
> +                  Register.
> +
> +  @return The base address of PCI Express.
> +
> +**/
> +STATIC
> +UINTN
> +IsBdfValid (
> +  IN UINTN                     Address
> +  )
> +{
> +  UINT8 Bus;
> +  UINT8 Device;
> +  UINT8 Function;
> +  UINTN BdfCount;
> +  UINTN BdfValue;
> +  UINTN Count;
> +  UINTN TableBase;
> +  UINTN PciAddress;
> +
> +  Bus      = GET_BUS_NUM (Address);
> +  Device   = GET_DEV_NUM (Address);
> +  Function = GET_FUNC_NUM (Address);
> +
> +  PciAddress = EFI_PCIE_ADDRESS (Bus, Device, Function, 0);
> +
> +  if (GET_PCIE_BASE_ADDRESS (Address) ==
> +        FixedPcdGet64 (PcdPcieExpressBaseAddress)) {
> +    TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
> +  } else {
> +    TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFSET;
> +  }
> +
> +  BdfCount = MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE);
> +
> +  /* Start from the second entry */
> +  for (Count = BDF_TABLE_HEADER_COUNT;
> +       Count < (BdfCount + BDF_TABLE_HEADER_COUNT);
> +       Count++) {
> +    BdfValue = MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE));
> +    if (BdfValue == PciAddress)
> +      break;
> +  }
> +
> +  if (Count == (BdfCount + BDF_TABLE_HEADER_COUNT)) {
> +    return mDummyConfigData;
> +  } else {
> +    return PciAddress;
> +  }
> +}
> +
> +/**
> +  Get the physical address of a configuration space register.
> +
> +  Implement a  workaround to avoid generation of slave errors from the bus. That
> +  is, retrieve the PCI Express Base Address via a PCD entry, add the incomming
> +  address with that base address and check whether this converted address
> +  points to a accessible BDF. If it is not accessible, return the address
> +  of a dummy location so that a read from it does not cause a slave error.
> +
> +  In addition to this, implement a workaround for accessing the root port's
> +  configuration space. The root port configuration space is not contiguous
> +  with the rest of the downstream hierarchy configuration space. So determine
> +  whether the specified address is for the root port and use a different base
> +  address for it.
> +
> +  @param  Address The address that encodes the PCI Bus, Device, Function and
> +                  Register.
> +
> +  @return Physical address of the configuration register that corresponds to the
> +          PCI configuration register specified by input parameter 'Address'.
> +
> +**/
> +STATIC
> +VOID*
> +GetPciExpressAddress (
> +  IN UINTN                     Address
> +  )
> +{
> +  BOOLEAN CheckRootPort;
> +  UINT8   Bus;
> +  UINT8   Device;
> +  UINT8   Function;
> +  UINT16  Register;
> +  UINTN   ConfigAddress;
> +
> +  Bus      = GET_BUS_NUM (Address);
> +  Device   = GET_DEV_NUM (Address);
> +  Function = GET_FUNC_NUM (Address);
> +  Register = GET_REG_NUM (Address);
> +
> +  CheckRootPort = (BOOLEAN) (Bus == 0) && (Device == 0) && (Function == 0);
> +
> +  if (GET_PCIE_BASE_ADDRESS (Address) ==
> +        FixedPcdGet64 (PcdPcieExpressBaseAddress)) {
> +    if (CheckRootPort == TRUE) {
> +      ConfigAddress = (UINTN) (PcdGet32 (PcdPcieRootPortConfigBaseAddress) +
> +        EFI_PCIE_ADDRESS (Bus, Device, Function, Register));
> +    } else {
> +      ConfigAddress = (UINTN) (PcdGet64 (PcdPcieExpressBaseAddress) +
> +        EFI_PCIE_ADDRESS (Bus, Device, Function, Register));
> +    }
> +  } else {
> +    if (CheckRootPort == TRUE) {
> +      ConfigAddress = (UINTN) (PcdGet32 (PcdCcixRootPortConfigBaseAddress) +
> +        EFI_PCIE_ADDRESS (Bus, Device, Function, Register));
> +    } else {
> +      ConfigAddress = (UINTN) PcdGet32 (PcdCcixExpressBaseAddress +
> +        EFI_PCIE_ADDRESS (Bus, Device, Function, Register));
> +    }
> +  }
> +
> +  if (CheckRootPort == FALSE) {
> +    if (IsBdfValid (Address) == mDummyConfigData) {
> +      ConfigAddress = (UINTN) &mDummyConfigData;
> +    }
> +  }
> +
> +  return (VOID *)ConfigAddress;
> +}
> +
> +/**
> +  Function to return PCIe Physical Address for different RCs.
> +  If address is invalid, then ASSERT().
> +
> +  @param  Address Address passed from bus layer.
> +
> +  @return Return PCIe base address.
> +
> +**/
> +STATIC
> +UINT64
> +PciSegmentLibGetConfigBase (
> +  IN  UINT64                   Address
> +  )
> +{
> +  switch ((UINT16)(Address >> 32)) {+  case 0:
> +    return FixedPcdGet32 (PcdPcieExpressBaseAddress);
> +  case 1:
> +    return FixedPcdGet32 (PcdCcixExpressBaseAddress);
> +  default:
> +    ASSERT (FALSE);
> +  }
> +  return 0;
> +}
> +
> +/**
> +  Internal worker function to read a PCI configuration register.
> +
> +  @param Address    The address that encodes the PCI Bus, Device, Function
> +                    and Register.
> +  @param Width      The width of data to read
> +
> +  @return The value read from the PCI configuration register.
> +**/
> +STATIC
> +UINT32
> +PciSegmentLibReadWorker (
> +  IN  UINT64                   Address,
> +  IN  PCI_CFG_WIDTH            Width
> +  )
> +{
> +  UINT64    Addr;
> +  UINT64    Base;
> +
> +  Base = PciSegmentLibGetConfigBase (Address);
> +  Addr = (UINT64)GetPciExpressAddress ((UINT32) Address + Base);

I think PciSegmentLibGetConfigBase() could be removed. Another macro to get the Pci segment should be created as:

#define GET_SEGV_NUM(Address)    (((Address) >> 32) & 0xFFFF)

Cf the bit layout at MdePkg/Include/Library/PciSegmentLib.h

And then in GetPciExpressAddress(), we get the Pci Segment value and choose which PcdXXXBaseAddress value to use.

Similar comment for the PciSegmentLibWriteWorker() function.
> +
> +  switch (Width) {
> +  case PciCfgWidthUint8:
> +    return MmioRead8 (Addr);
> +  case PciCfgWidthUint16:
> +    return MmioRead16 (Addr);
> +  case PciCfgWidthUint32:
> +    return MmioRead32 (Addr);
> +  default:
> +    ASSERT (FALSE);
> +  }
> +
> +  return 0;
> +}
> +
> +/**
> +  Internal worker function to write to a PCI configuration register.
> +
> +  @param Address   The address that encodes the PCI Bus, Device, Function
> +                   and Register.
> +  @param Width     The width of data to write
> +  @param Data      The value to write.
> +
> +  @return  The value written to the PCI configuration register.
> +**/
> +STATIC
> +UINT32
> +PciSegmentLibWriteWorker (
> +  IN  UINT64                   Address,
> +  IN  PCI_CFG_WIDTH            Width,
> +  IN  UINT32                   Data
> +  )
> +{
> +  UINT8  Bus, Device, Function;
> +  UINT8  Offset;
> +  UINT32 WData;
> +  UINT64 Addr;
> +  UINT64 Base;
> +
> +  Base = PciSegmentLibGetConfigBase (Address);
> +  Addr = (UINTN) Address + Base;
> +
> +  Bus = GET_BUS_NUM (Addr);
> +  Device = GET_DEV_NUM (Addr);
> +  Function = GET_FUNC_NUM (Addr);
> +
> +  // 8-bit and 16-bit writes to root port config space is not supported due to
> +  // a hardware limitation. As a workaround, perform a read-update-write
> +  // sequence on the whole 32-bit word of the root port config register such
> +  // that only the specified 8-bits of that word are updated.
> +
> +  switch (Width) {
> +  case PciCfgWidthUint8:
> +    if ((Bus == 0) && (Device == 0) && (Function == 0)) {
> +      Offset = Addr & 0x3;
> +      Addr &= 0xFFFFFFFC;
> +      WData = MmioRead32 ((UINTN) GetPciExpressAddress (Addr));
> +      WData &= ~(0xFF << (8 * Offset));
> +      WData |= (Data << (8 * Offset));
> +      MmioWrite32 ((UINTN) GetPciExpressAddress (Addr), WData);
> +      return Data;
> +    }
> +    MmioWrite8 ((UINTN) GetPciExpressAddress (Addr), Data);
> +    break;
> +  case PciCfgWidthUint16:
> +    if ((Bus == 0) && (Device == 0) && (Function == 0)) {
> +      Offset = Addr & 0x3;
> +      Addr &= 0xFFFFFFFC;
> +      WData = MmioRead32 ((UINTN) GetPciExpressAddress (Addr));
> +      WData &= ~(0xFFFF << (8 * Offset));
> +      WData |= (Data << (8 * Offset));
> +      MmioWrite32 ((UINTN) GetPciExpressAddress (Addr), WData);
> +      return Data;
> +    }

Some of the code for the 2 cases with 8/16 bits seems similar, is it possible to factorize it ? We could also store the result of GetPciExpressAddress (Addr) once for the whole function.

Also, is there an 'else' statement missing ? It seems that when not writing to a root complex, we are writing 2 times.

> +    MmioWrite16 ((UINTN) GetPciExpressAddress (Addr), Data);
> +    break;
> +  case PciCfgWidthUint32:
> +    MmioWrite32 ((UINTN) GetPciExpressAddress (Addr), Data);
> +    break;
> +  default:
> +    ASSERT (FALSE);
> +  }
> +
> +  return Data;
> +}
> +
[SNIP]