Re: [PATCH edk2-platforms 32/39] Silicon/NXP: Implement PciSegmentLib to support multiple RCs

[Vabhav Sharma <vabhav.sharma@nxp.com>]

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: Friday, April 20, 2018 12:57 AM
>To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
>Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
><udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Vabhav Sharma
><vabhav.sharma@nxp.com>
>Subject: Re: [PATCH edk2-platforms 32/39] Silicon/NXP: Implement
>PciSegmentLib to support multiple RCs
>
>On Fri, Feb 16, 2018 at 02:20:28PM +0530, Meenakshi wrote:
>> From: Vabhav <vabhav.sharma@nxp.com>
>>
>> Multiple root complex support is not provided by standard library
>> PciLib/PciExpressLib/PciSegmentLib, Reimplementing it and provide
>> function for reading/writing into PCIe configuration Space.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Vabhav <vabhav.sharma@nxp.com>
>> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
>> ---
>>  Silicon/NXP/Include/Pcie.h                         | 143 +++++
>>  Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c  | 604
>+++++++++++++++++++++
>>  .../NXP/Library/PciSegmentLib/PciSegmentLib.inf    |  41 ++
>>  3 files changed, 788 insertions(+)
>>  create mode 100644 Silicon/NXP/Include/Pcie.h  create mode 100644
>> Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
>>  create mode 100644
>> Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
>>
>> diff --git a/Silicon/NXP/Include/Pcie.h b/Silicon/NXP/Include/Pcie.h
>> new file mode 100644 index 0000000..a7e6f9b
>> --- /dev/null
>> +++ b/Silicon/NXP/Include/Pcie.h
>> @@ -0,0 +1,143 @@
>> +/** @file
>> +  PCI memory configuration for NXP
>> +
>> +  Copyright 2018 NXP
>> +
>> +  This program and the accompanying materials are licensed and made
>> + available  under the terms and conditions of the BSD License which
>> + accompanies this  distribution.  The full text of the license may be
>> + found at
>https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensou
>rce.org%2Flicenses%2Fbsd-
>license.php&data=02%7C01%7Cvabhav.sharma%40nxp.com%7C081a2ebc43af4a
>daaf8e08d5a62b9921%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
>36597628604269440&sdata=kt661HfkUD2E3sYswy0I4vVFTV3%2Fq%2FiM%2BiW
>egCISP%2BU%3D&reserved=0.
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS, WITHOUT  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __PCI_H__
>> +#define __PCI_H__
>
>I'm not super happy about reusing such a generic name for the include guard - or
>really even the filename. (MdePkg/Include/Pci.h has
>_PCI_H_.)
>
>Could you rename this header NxpPcie.h and change the include guard to
>_NXP_PCIE_H_?
I see, Sure.
>
>> +
>> +// Segment 0
>> +#define PCI_SEG0_NUM              0
>> +
>> +#define PCI_SEG0_BUSNUM_MIN       0x0
>> +#define PCI_SEG0_BUSNUM_MAX       0xff
>> +
>> +#define PCI_SEG0_PORTIO_MIN       0x0
>> +#define PCI_SEG0_PORTIO_MAX       0xffff
>> +
>> +#define PCI_SEG0_MMIO32_MIN       0x40000000
>> +#define PCI_SEG0_MMIO32_MAX       0x4fffffff
>> +#define PCI_SEG0_MMIO64_MIN       PCI_SEG0_MMIO_MEMBASE +
>SEG_MEM_SIZE
>> +#define PCI_SEG0_MMIO64_MAX       PCI_SEG0_MMIO_MEMBASE +
>SEG_MEM_LIMIT
>> +#define PCI_SEG0_MMIO_MEMBASE     FixedPcdGet64 (PcdPciExp1BaseAddr)
>> +
>> +#define PCI_SEG0_DBI_BASE         0x03400000
>> +
>> +// Segment 1
>> +#define PCI_SEG1_NUM              1
>> +
>> +#define PCI_SEG1_BUSNUM_MIN       0x0
>> +#define PCI_SEG1_BUSNUM_MAX       0xff
>> +
>> +#define PCI_SEG1_PORTIO_MIN       0x10000
>> +#define PCI_SEG1_PORTIO_MAX       0x1ffff
>> +
>> +#define PCI_SEG1_MMIO32_MIN       0x50000000
>> +#define PCI_SEG1_MMIO32_MAX       0x5fffffff
>> +#define PCI_SEG1_MMIO64_MIN       PCI_SEG1_MMIO_MEMBASE +
>SEG_MEM_SIZE
>> +#define PCI_SEG1_MMIO64_MAX       PCI_SEG1_MMIO_MEMBASE +
>SEG_MEM_LIMIT
>> +#define PCI_SEG1_MMIO_MEMBASE     FixedPcdGet64 (PcdPciExp2BaseAddr)
>> +
>> +#define PCI_SEG1_DBI_BASE         0x03500000
>> +
>> +// Segment 2
>> +#define PCI_SEG2_NUM              2
>> +
>> +#define PCI_SEG2_BUSNUM_MIN       0x0
>> +#define PCI_SEG2_BUSNUM_MAX       0xff
>> +
>> +#define PCI_SEG2_PORTIO_MIN       0x20000
>> +#define PCI_SEG2_PORTIO_MAX       0x2ffff
>> +
>> +#define PCI_SEG2_MMIO32_MIN       0x60000000
>> +#define PCI_SEG2_MMIO32_MAX       0x6fffffff
>> +#define PCI_SEG2_MMIO64_MIN       PCI_SEG2_MMIO_MEMBASE +
>SEG_MEM_SIZE
>> +#define PCI_SEG2_MMIO64_MAX       PCI_SEG2_MMIO_MEMBASE +
>SEG_MEM_LIMIT
>> +#define PCI_SEG2_MMIO_MEMBASE     FixedPcdGet64 (PcdPciExp3BaseAddr)
>> +
>> +#define PCI_SEG2_DBI_BASE         0x03600000
>> +
>> +// Segment 3
>> +#define PCI_SEG3_NUM              3
>> +
>> +#define PCI_SEG3_BUSNUM_MIN       0x0
>> +#define PCI_SEG3_BUSNUM_MAX       0xff
>> +
>> +#define PCI_SEG3_PORTIO_MIN       0x30000
>> +#define PCI_SEG3_PORTIO_MAX       0x3ffff
>> +
>> +#define PCI_SEG3_MMIO32_MIN       0x70000000
>> +#define PCI_SEG3_MMIO32_MAX       0x7fffffff
>> +#define PCI_SEG3_MMIO64_MIN       PCI_SEG3_MMIO_MEMBASE +
>SEG_MEM_SIZE
>> +#define PCI_SEG3_MMIO64_MAX       PCI_SEG3_MMIO_MEMBASE +
>SEG_MEM_LIMIT
>> +#define PCI_SEG3_MMIO_MEMBASE     FixedPcdGet64 (PcdPciExp4BaseAddr)
>> +
>> +#define PCI_SEG3_DBI_BASE         0x03700000
>> +
>> +// Segment configuration
>> +#define SEG_CFG_SIZE              0x00001000
>> +#define SEG_CFG_BUS               0x00000000
>> +#define SEG_MEM_SIZE              0x40000000
>> +#define SEG_MEM_LIMIT             0x7fffffff
>> +#define SEG_MEM_BUS               0x40000000
>> +#define SEG_IO_SIZE               0x00010000
>> +#define SEG_IO_BUS                0x00000000
>> +#define PCI_BASE_DIFF             0x800000000
>> +#define PCI_DBI_SIZE_DIFF         0x100000
>> +#define PCI_SEG0_PHY_CFG0_BASE    PCI_SEG0_MMIO_MEMBASE
>> +#define PCI_SEG0_PHY_CFG1_BASE    PCI_SEG0_PHY_CFG0_BASE +
>SEG_CFG_SIZE
>> +#define PCI_SEG0_PHY_MEM_BASE     PCI_SEG0_MMIO64_MIN
>> +#define PCI_SEG0_PHY_IO_BASE      PCI_SEG0_MMIO_MEMBASE +
>SEG_IO_SIZE
>> +
>> +// iATU configuration
>> +#define IATU_VIEWPORT_OFF                            0x900
>> +#define IATU_VIEWPORT_OUTBOUND                       0
>> +
>> +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0            0x904
>> +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM   0x0
>> +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_IO    0x2
>> +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_CFG0  0x4 #define
>> +IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_CFG1  0x5
>> +
>> +#define IATU_REGION_CTRL_2_OFF_OUTBOUND_0            0x908
>> +#define IATU_REGION_CTRL_2_OFF_OUTBOUND_0_REGION_EN  BIT31
>> +
>> +#define IATU_LWR_BASE_ADDR_OFF_OUTBOUND_0            0x90C
>> +#define IATU_UPPER_BASE_ADDR_OFF_OUTBOUND_0          0x910
>> +#define IATU_LIMIT_ADDR_OFF_OUTBOUND_0               0x914
>> +#define IATU_LWR_TARGET_ADDR_OFF_OUTBOUND_0          0x918
>> +#define IATU_UPPER_TARGET_ADDR_OFF_OUTBOUND_0        0x91C
>> +
>> +#define IATU_REGION_INDEX0                           0x0
>> +#define IATU_REGION_INDEX1                           0x1
>> +#define IATU_REGION_INDEX2                           0x2
>> +#define IATU_REGION_INDEX3                           0x3
>> +
>> +// PCIe Controller configuration
>> +#define NUM_PCIE_CONTROLLER  FixedPcdGet32 (PcdNumPciController)
>> +#define PCI_LUT_DBG          FixedPcdGet32 (PcdPcieLutDbg)
>> +#define PCI_LUT_BASE         FixedPcdGet32 (PcdPcieLutBase)
>> +#define LTSSM_STATE_MASK     0x3f
>> +#define LTSSM_PCIE_L0        0x11
>> +#define PCI_LINK_CAP         0x7c
>> +#define PCI_LINK_SPEED_MASK  0xf
>> +#define PCI_CLASS_BRIDGE_PCI 0x6040010
>> +#define PCI_CLASS_DEVICE     0x8
>> +#define PCI_DBI_RO_WR_EN     0x8bc
>> +#define PCI_BASE_ADDRESS_0   0x10
>> +
>> +VOID GetSerdesProtocolMaps (UINT64 *);
>> +
>> +BOOLEAN IsSerDesLaneProtocolConfigured (UINT64, UINT16);
>> +
>> +#endif
>> diff --git a/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
>> b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
>> new file mode 100644
>> index 0000000..acb614d
>> --- /dev/null
>> +++ b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
>> @@ -0,0 +1,604 @@
>> +/** @file
>> +  PCI Segment Library for NXP SoCs with multiple RCs
>> +
>> +  Copyright 2018 NXP
>> +
>> +  This program and the accompanying materials are  licensed and made
>> + available under the terms and conditions of  the BSD License which
>> + accompanies this distribution.  The full  text of the license may be
>> + found at
>> +
>https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensou
>rce.org%2Flicenses%2Fbsd-
>license.php&data=02%7C01%7Cvabhav.sharma%40nxp.com%7C081a2ebc43af4a
>daaf8e08d5a62b9921%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
>36597628604269440&sdata=kt661HfkUD2E3sYswy0I4vVFTV3%2Fq%2FiM%2BiW
>egCISP%2BU%3D&reserved=0.
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <Base.h>
>> +#include <Library/PciSegmentLib.h>
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Pcie.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)
>> +
>> +/**
>> +  Function to return PCIe Physical Address(PCIe view) or Controller
>> +  Address(CPU view) for different RCs
>> +
>> +  @param  Address Address passed from bus layer.
>> +  @param  Segment Segment number for Root Complex.
>> +
>> +  @return Return PCIe CPU or Controller address.
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +PciSegmentLibGetConfigBase (
>> +  IN  UINT64      Address,
>> +  IN  UINT16      Segment
>> +  )
>> +{
>> +
>> +  switch (Segment) {
>> +    // Root Complex 1
>> +    case PCI_SEG0_NUM:
>> +      // Reading bus number(bits 20-27)
>> +      if ((Address >> 20) & 1) {
>> +        return PCI_SEG0_MMIO_MEMBASE;
>> +      } else {
>> +        // On Bus 0 RCs are connected
>> +        return PCI_SEG0_DBI_BASE;
>> +      }
>> +    // Root Complex 2
>> +    case PCI_SEG1_NUM:
>> +      // Reading bus number(bits 20-27)
>> +      if ((Address >> 20) & 1) {
>> +        return PCI_SEG1_MMIO_MEMBASE;
>> +      } else {
>> +        // On Bus 0 RCs are connected
>> +        return PCI_SEG1_DBI_BASE;
>> +      }
>> +    // Root Complex 3
>> +    case PCI_SEG2_NUM:
>> +      // Reading bus number(bits 20-27)
>> +      if ((Address >> 20) & 1) {
>> +        return PCI_SEG2_MMIO_MEMBASE;
>> +      } else {
>> +        // On Bus 0 RCs are connected
>> +        return PCI_SEG2_DBI_BASE;
>> +      }
>> +    // Root Complex 4
>> +    case PCI_SEG3_NUM:
>> +      // Reading bus number(bits 20-27)
>> +      if ((Address >> 20) & 1) {
>> +        return PCI_SEG3_MMIO_MEMBASE;
>> +      } else {
>> +        // On Bus 0 RCs are connected
>> +        return PCI_SEG3_DBI_BASE;
>> +      }
>> +    default:
>> +      return 0;
>> +  }
>> +
>> +}
>> +
>> +/**
>> +  Internal worker function to read a PCI configuration register.
>> +
>> +  @param  Address The address that encodes the Segment, 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    Base;
>> +  UINT16    Offset;
>> +  UINT16    Segment;
>> +
>> +  //
>> +  // Reading Segment number(47-32) bits in Address  //  Segment =
>> + (Address >> 32);  //  // Reading Function(12-0) bits in Address  //
>> + Offset = (Address & 0xfff );
>> +
>> +  Base = PciSegmentLibGetConfigBase (Address, Segment);
>> +
>> +  //
>> +  // ignore devices > 0 on bus 0
>> +  //
>> +  if ((Address & 0xff00000) == 0 && (Address & 0xf8000) != 0) {
>> +    return MAX_UINT32;
>> +  }
>> +
>> +  //
>> +  // ignore device > 0 on bus 1
>> +  //
>> +  if ((Address & 0xfe00000) == 0 && (Address & 0xf8000) != 0) {
>> +    return MAX_UINT32;
>> +  }
>> +
>> +  switch (Width) {
>> +  case PciCfgWidthUint8:
>> +    return MmioRead8 (Base + (UINT8)Offset);  case PciCfgWidthUint16:
>> +    return MmioRead16 (Base + (UINT16)Offset);  case
>> + PciCfgWidthUint32:
>> +    return MmioRead32 (Base + (UINT32)Offset);
>> +  default:
>> +    ASSERT (FALSE);
>> +  }
>> +
>> +  return CHAR_NULL;
>> +}
>> +
>> +/**
>> +  Internal worker function to writes a PCI configuration register.
>> +
>> +  @param  Address The address that encodes the Segment, 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
>> +  )
>> +{
>> +  UINT64    Base;
>> +  UINT32    Offset;
>> +  UINT16    Segment;
>> +
>> +  //
>> +  // Reading Segment number(47-32 bits) in Address  Segment =
>> + (Address >> 32);  //  // Reading Function(12-0 bits) in Address  //
>> + Offset = (Address & 0xfff );
>
>Spurious space after 0xfff.
Alright, Thanks.
Same applies to PciSegmentLibReadWorker()
>
>Could we have some macros and #defines instead of live-coded values in this
>function?
Sure, I assume Similar changes in PciSegmentLibReadWorker() required.
>
>> +
>> +  Base = PciSegmentLibGetConfigBase (Address, Segment);
>> +
>> +  //
>> +  // ignore devices > 0 on bus 0
>> +  //
>> +  if ((Address & 0xff00000) == 0 && (Address & 0xf8000) != 0) {
>> +    return Data;
>> +  }
>> +
>> +  //
>> +  // ignore device > 0 on bus 1
>> +  //
>> +  if ((Address & 0xfe00000) == 0 && (Address & 0xf8000) != 0) {
>> +    return MAX_UINT32;
>> +  }
>> +
>> +  switch (Width) {
>> +  case PciCfgWidthUint8:
>> +    MmioWrite8 (Base + (UINT8)Offset, Data);
>> +    break;
>> +  case PciCfgWidthUint16:
>> +    MmioWrite16 (Base + (UINT16)Offset, Data);
>> +    break;
>> +  case PciCfgWidthUint32:
>> +    MmioWrite32 (Base + (UINT16)Offset, Data);
>> +    break;
>> +  default:
>> +    ASSERT (FALSE);
>> +  }
>> +
>> +  return Data;
>> +}
>> +
>> +/**
>> +  Register a PCI device so PCI configuration registers may be
>> +accessed after
>> +  SetVirtualAddressMap().
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +
>> +  @param  Address                  The address that encodes the PCI Bus, Device,
>> +                                   Function and Register.
>> +
>> +  @retval RETURN_SUCCESS           The PCI device was registered for runtime
>access.
>> +  @retval RETURN_UNSUPPORTED       An attempt was made to call this
>function
>> +                                   after ExitBootServices().
>> +  @retval RETURN_UNSUPPORTED       The resources required to access the
>PCI device
>> +                                   at runtime could not be mapped.
>> +  @retval RETURN_OUT_OF_RESOURCES  There are not enough resources
>available to
>> +                                   complete the registration.
>> +
>> +**/
>> +RETURN_STATUS
>> +EFIAPI
>> +PciSegmentRegisterForRuntimeAccess (
>> +  IN UINTN  Address
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 0);
>> +  return RETURN_UNSUPPORTED;
>> +}
>> +
>> +/**
>> +  Reads an 8-bit PCI configuration register.
>> +
>> +  Reads and returns the 8-bit PCI configuration register specified by Address.
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +
>> +  @param  Address   The address that encodes the PCI Segment, Bus, Device,
>Function,
>> +                    and Register.
>> +
>> +  @return The 8-bit PCI configuration register specified by Address.
>> +
>> +**/
>> +UINT8
>> +EFIAPI
>> +PciSegmentRead8 (
>> +  IN UINT64                    Address
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 0);
>> +
>> +  return (UINT8) PciSegmentLibReadWorker (Address, PciCfgWidthUint8);
>> +}
>> +
>> +/**
>> +  Writes an 8-bit PCI configuration register.
>> +
>> +  Writes the 8-bit PCI configuration register specified by Address with the value
>specified by Value.
>> +  Value is returned.  This function must guarantee that all PCI read and write
>operations are serialized.
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +
>> +  @param  Address     The address that encodes the PCI Segment, Bus, Device,
>Function, and Register.
>> +  @param  Value       The value to write.
>> +
>> +  @return The value written to the PCI configuration register.
>> +
>> +**/
>> +UINT8
>> +EFIAPI
>> +PciSegmentWrite8 (
>> +  IN UINT64                    Address,
>> +  IN UINT8                     Value
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 0);
>> +
>> +  return (UINT8) PciSegmentLibWriteWorker (Address, PciCfgWidthUint8,
>> +Value); }
>> +
>> +/**
>> +  Reads a 16-bit PCI configuration register.
>> +
>> +  Reads and returns the 16-bit PCI configuration register specified by Address.
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +  If Address is not aligned on a 16-bit boundary, then ASSERT().
>> +
>> +  @param  Address   The address that encodes the PCI Segment, Bus, Device,
>Function, and Register.
>> +
>> +  @return The 16-bit PCI configuration register specified by Address.
>> +
>> +**/
>> +UINT16
>> +EFIAPI
>> +PciSegmentRead16 (
>> +  IN UINT64                    Address
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 1);
>> +
>> +  return (UINT16) PciSegmentLibReadWorker (Address,
>> +PciCfgWidthUint16); }
>> +
>> +/**
>> +  Writes a 16-bit PCI configuration register.
>> +
>> +  Writes the 16-bit PCI configuration register specified by Address
>> + with the  value specified by Value.
>> +
>> +  Value is returned.
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +  If Address is not aligned on a 16-bit boundary, then ASSERT().
>> +
>> +  @param  Address     The address that encodes the PCI Segment, Bus, Device,
>Function, and Register.
>> +  @param  Value       The value to write.
>> +
>> +  @return The parameter of Value.
>> +
>> +**/
>> +UINT16
>> +EFIAPI
>> +PciSegmentWrite16 (
>> +  IN UINT64                    Address,
>> +  IN UINT16                    Value
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 1);
>> +
>> +  return (UINT16) PciSegmentLibWriteWorker (Address,
>> +PciCfgWidthUint16, Value); }
>> +
>> +/**
>> +  Reads a 32-bit PCI configuration register.
>> +
>> +  Reads and returns the 32-bit PCI configuration register specified by Address.
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +  If Address is not aligned on a 32-bit boundary, then ASSERT().
>> +
>> +  @param  Address   The address that encodes the PCI Segment, Bus, Device,
>Function,
>> +                    and Register.
>> +
>> +  @return The 32-bit PCI configuration register specified by Address.
>> +
>> +**/
>> +UINT32
>> +EFIAPI
>> +PciSegmentRead32 (
>> +  IN UINT64                    Address
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 3);
>> +
>> +  return PciSegmentLibReadWorker (Address, PciCfgWidthUint32); }
>> +
>> +/**
>> +  Writes a 32-bit PCI configuration register.
>> +
>> +  Writes the 32-bit PCI configuration register specified by Address
>> + with the  value specified by Value.
>> +
>> +  Value is returned.
>> +
>> +  If any reserved bits in Address are set, then ASSERT().
>> +  If Address is not aligned on a 32-bit boundary, then ASSERT().
>> +
>> +  @param  Address     The address that encodes the PCI Segment, Bus, Device,
>> +                      Function, and Register.
>> +  @param  Value       The value to write.
>> +
>> +  @return The parameter of Value.
>> +
>> +**/
>> +UINT32
>> +EFIAPI
>> +PciSegmentWrite32 (
>> +  IN UINT64                    Address,
>> +  IN UINT32                    Value
>> +  )
>> +{
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (Address, 3);
>> +
>> +  return PciSegmentLibWriteWorker (Address, PciCfgWidthUint32,
>> +Value); }
>> +
>> +/**
>> +  Reads a range of PCI configuration registers into a caller supplied buffer.
>> +
>> +  Reads the range of PCI configuration registers specified by
>> + StartAddress and  Size into the buffer specified by Buffer. This
>> + function only allows the PCI  configuration registers from a single
>> + PCI function to be read. Size is  returned.
>> +
>> +  If any reserved bits in StartAddress are set, then ASSERT().
>> +  If ((StartAddress & 0xFFF) + Size) > 0x1000, then ASSERT().
>> +  If Size > 0 and Buffer is NULL, then ASSERT().
>> +
>> +  @param  StartAddress  The starting address that encodes the PCI Segment,
>Bus,
>> +                        Device, Function and Register.
>> +  @param  Size          The size in bytes of the transfer.
>> +  @param  Buffer        The pointer to a buffer receiving the data read.
>> +
>> +  @return Size
>> +
>> +**/
>> +UINTN
>> +EFIAPI
>> +PciSegmentReadBuffer (
>> +  IN  UINT64                   StartAddress,
>> +  IN  UINTN                    Size,
>> +  OUT VOID                     *Buffer
>> +  )
>> +{
>> +  UINTN                             ReturnValue;
>> +
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (StartAddress, 0);  // 0xFFF is
>> + used as limit for 4KB config space  ASSERT (((StartAddress & 0xFFF)
>> + + Size) <= SIZE_4KB);
>> +
>> +  if (Size == 0) {
>> +    return Size;
>> +  }
>> +
>> +  ASSERT (Buffer != NULL);
>> +
>> +  //
>> +  // Save Size for return
>> +  //
>> +  ReturnValue = Size;
>> +
>> +  if ((StartAddress & BIT0) != 0) {
>> +    //
>> +    // Read a byte if StartAddress is byte aligned
>> +    //
>> +    *(volatile UINT8 *)Buffer = PciSegmentRead8 (StartAddress);
>
>Why volatile on Buffer?
I took reference from Socionext, Not required .
PCIe region has device attribute.
>
>> +    StartAddress += sizeof (UINT8);
>> +    Size -= sizeof (UINT8);
>> +    Buffer = (UINT8*)Buffer + BIT0;
>
>sizeof (UINT8) instead of BIT0?
>
>And this is a VOID *, so can just write.
>  Buffer += sizeof (UINT8);
>
Yes,Sure.
>> +  }
>> +
>> +  if (Size >= sizeof (UINT16) && (StartAddress & BIT1) != 0) {
>> +    //
>> +    // Read a word if StartAddress is word aligned
>> +    //
>> +    WriteUnaligned16 (Buffer, PciSegmentRead16 (StartAddress));
>> +    StartAddress += sizeof (UINT16);
>> +    Size -= sizeof (UINT16);
>> +    Buffer = (UINT16*)Buffer + BIT0;
>
>That is a very confusing use of pointer arithmetic.
>  Buffer += sizeof (UINT16);
>
Agree, alright.
>> +  }
>> +
>> +  while (Size >= sizeof (UINT32)) {
>> +    //
>> +    // Read as many double words as possible
>> +    //
>> +    WriteUnaligned32 (Buffer, PciSegmentRead32 (StartAddress));
>> +    StartAddress += sizeof (UINT32);
>> +    Size -= sizeof (UINT32);
>> +    Buffer = (UINT32*)Buffer + BIT0;
>
>  Buffer += sizeof (UINT32);
Ok
>
>> +  }
>> +
>> +  if (Size >= sizeof (UINT16)) {
>> +    //
>> +    // Read the last remaining word if exist
>> +    //
>> +    WriteUnaligned16 (Buffer, PciSegmentRead16 (StartAddress));
>> +    StartAddress += sizeof (UINT16);
>> +    Size -= sizeof (UINT16);
>> +    Buffer = (UINT16*)Buffer + BIT0;
>
>  Buffer += sizeof (UINT16);
Ok
>
>> +  }
>> +
>> +  if (Size >= sizeof (UINT8)) {
>> +    //
>> +    // Read the last remaining byte if exist
>> +    //
>> +    *(volatile UINT8 *)Buffer = PciSegmentRead8 (StartAddress);
>
>There is that scary volatile again.
Not required.
>
>> +  }
>> +
>> +  return ReturnValue;
>> +}
>> +
>> +
>> +/**
>> +  Copies the data in a caller supplied buffer to a specified range of
>> +PCI
>> +  configuration space.
>> +
>> +  Writes the range of PCI configuration registers specified by
>> + StartAddress and  Size from the buffer specified by Buffer. This
>> + function only allows the PCI  configuration registers from a single
>> + PCI function to be written. Size is  returned.
>> +
>> +  If any reserved bits in StartAddress are set, then ASSERT().
>> +  If ((StartAddress & 0xFFF) + Size) > 0x1000, then ASSERT().
>> +  If Size > 0 and Buffer is NULL, then ASSERT().
>> +
>> +  @param  StartAddress  The starting address that encodes the PCI Segment,
>Bus,
>> +                        Device, Function and Register.
>> +  @param  Size          The size in bytes of the transfer.
>> +  @param  Buffer        The pointer to a buffer containing the data to write.
>> +
>> +  @return The parameter of Size.
>> +
>> +**/
>> +UINTN
>> +EFIAPI
>> +PciSegmentWriteBuffer (
>> +  IN UINT64                    StartAddress,
>> +  IN UINTN                     Size,
>> +  IN VOID                      *Buffer
>> +  )
>> +{
>> +  UINTN                             ReturnValue;
>> +
>> +  ASSERT_INVALID_PCI_SEGMENT_ADDRESS (StartAddress, 0);  // 0xFFF is
>> + used as limit for 4KB config space  ASSERT (((StartAddress & 0xFFF)
>> + + Size) <= SIZE_4KB);
>
>Can you use (SIZE_4KB - 1) instead of 0xFFF?
Yes,Sure.
>
>> +
>> +  if (Size == 0) {
>> +    return Size;
>> +  }
>> +
>> +  ASSERT (Buffer != NULL);
>> +
>> +  //
>> +  // Save Size for return
>> +  //
>> +  ReturnValue = Size;
>> +
>> +  if ((StartAddress & BIT0) != 0) {
>> +    //
>> +    // Write a byte if StartAddress is byte aligned
>> +    //
>> +    PciSegmentWrite8 (StartAddress, *(UINT8*)Buffer);
>> +    StartAddress += sizeof (UINT8);
>> +    Size -= sizeof (UINT8);
>> +    Buffer = (UINT8*)Buffer + BIT0;
>
>Same comments for Buffer pointer update as previous function, throughout.
Ok
>
>/
>    Leif
>
>> +  }
>> +
>> +  if (Size >= sizeof (UINT16) && (StartAddress & BIT1) != 0) {
>> +    //
>> +    // Write a word if StartAddress is word aligned
>> +    //
>> +    PciSegmentWrite16 (StartAddress, ReadUnaligned16 (Buffer));
>> +    StartAddress += sizeof (UINT16);
>> +    Size -= sizeof (UINT16);
>> +    Buffer = (UINT16*)Buffer + BIT0;
>> +  }
>> +
>> +  while (Size >= sizeof (UINT32)) {
>> +    //
>> +    // Write as many double words as possible
>> +    //
>> +    PciSegmentWrite32 (StartAddress, ReadUnaligned32 (Buffer));
>> +    StartAddress += sizeof (UINT32);
>> +    Size -= sizeof (UINT32);
>> +    Buffer = (UINT32*)Buffer + BIT0;
>> +  }
>> +
>> +  if (Size >= sizeof (UINT16)) {
>> +    //
>> +    // Write the last remaining word if exist
>> +    //
>> +    PciSegmentWrite16 (StartAddress, ReadUnaligned16 (Buffer));
>> +    StartAddress += sizeof (UINT16);
>> +    Size -= sizeof (UINT16);
>> +    Buffer = (UINT16*)Buffer + BIT0;
>> +  }
>> +
>> +  if (Size >= sizeof (UINT8)) {
>> +    //
>> +    // Write the last remaining byte if exist
>> +    //
>> +    PciSegmentWrite8 (StartAddress, *(UINT8*)Buffer);  }
>> +
>> +  return ReturnValue;
>> +}
>> diff --git a/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
>> b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
>> new file mode 100644
>> index 0000000..1ac83d4
>> --- /dev/null
>> +++ b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
>> @@ -0,0 +1,41 @@
>> +## @file
>> +#  PCI Segment Library for NXP SoCs with multiple RCs # #  Copyright
>> +2018 NXP # #  This program and the accompanying materials #  are
>> +licensed and made available under the terms and conditions of the BSD
>> +License #  which accompanies this distribution. The full text of the
>> +license may be found at #
>https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensou
>rce.org%2Flicenses%2Fbsd-
>license.php&data=02%7C01%7Cvabhav.sharma%40nxp.com%7C081a2ebc43af4a
>daaf8e08d5a62b9921%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
>36597628604269440&sdata=kt661HfkUD2E3sYswy0I4vVFTV3%2Fq%2FiM%2BiW
>egCISP%2BU%3D&reserved=0.
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> +BASIS, #  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>EITHER EXPRESS OR IMPLIED.
>> +#
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x0001001A
>> +  BASE_NAME                      = PciSegmentLib
>> +  FILE_GUID                      = c9f59261-5a60-4a4c-82f6-1f520442e100
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = PciSegmentLib
>> +
>> +[Sources]
>> +  PciSegmentLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  Silicon/NXP/NxpQoriqLs.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  DebugLib
>> +  IoLib
>> +  PcdLib
>> +
>> +[Pcd]
>> +  gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr
>> +  gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr
>> +  gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr
>> +  gNxpQoriqLsTokenSpaceGuid.PcdPciExp4BaseAddr
>> --
>> 1.9.1
>>