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 6:12 PM
>To: Vabhav Sharma <vabhav.sharma@nxp.com>
>Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
>ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
><udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
>Subject: Re: [PATCH edk2-platforms 32/39] Silicon/NXP: Implement
>PciSegmentLib to support multiple RCs
>
>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%2Fope
>> >nsou
>> >rce.org%2Flicenses%2Fbsd-
>>
>>license.php&data=02%7C01%7Cvabhav.sharma%40nxp.com%7C081a2ebc43af4
>a
>>
>>daaf8e08d5a62b9921%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
>6
>>
>>36597628604269440&sdata=kt661HfkUD2E3sYswy0I4vVFTV3%2Fq%2FiM%2Bi
>W
>> >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%2Fope
>> >nsou
>> >rce.org%2Flicenses%2Fbsd-
>>
>>license.php&data=02%7C01%7Cvabhav.sharma%40nxp.com%7C081a2ebc43af4
>a
>>
>>daaf8e08d5a62b9921%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
>6
>>
>>36597628604269440&sdata=kt661HfkUD2E3sYswy0I4vVFTV3%2Fq%2FiM%2Bi
>W
>> >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.
>
Yes you are right, Thanks.
I will update.
>> >> +    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?
Ok, Yes I will incorporate the changes for offset and address calculation.
>
>> >> +
>> >> +  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.)
Ok,I will update
Thanks.
>
>> >> +    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%2Fope
>> >nsou
>> >rce.org%2Flicenses%2Fbsd-
>>
>>license.php&data=02%7C01%7Cvabhav.sharma%40nxp.com%7C081a2ebc43af4
>a
>>
>>daaf8e08d5a62b9921%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
>6
>>
>>36597628604269440&sdata=kt661HfkUD2E3sYswy0I4vVFTV3%2Fq%2FiM%2Bi
>W
>> >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
>> >>