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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Apr 20, 2018 at 06:40:27AM +0000, Vabhav Sharma wrote:
> 
> 
> >-----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:

Sorry - I missed thie before but spotted it while scrolling through
the reply: This cast of the offset is not only unnecessary, it is a
bug - offset is masked with 0xfff above, meaning it carries 12 bits of
value. Casting it to UINT8 discards the top 4.

(And because an explicit cast amounts to the same as telling the
compiler "I know better than you", this does not even result in a
warning.)

Anyway, please drop this and the subsequent two casts.

> >> +    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.

Err, yes please.
Umm, actually - the base functionality of those two functions are
identical (which is why I must have missed the ReadWorker with a
double page-down). Could that be turned into a helper function that
calculates addresses and offsets, called by readworker and writeworker
separately?

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

(Same problem with the casts here.)

> >> +    break;
> >> +  case PciCfgWidthUint16:
> >> +    MmioWrite16 (Base + (UINT16)Offset, Data);
> >> +    break;
> >> +  case PciCfgWidthUint32:
> >> +    MmioWrite32 (Base + (UINT16)Offset, Data);

/
    Leif

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