public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif@nuviainc.com>
To: Nhi Pham <nhi@os.amperecomputing.com>
Cc: devel@edk2.groups.io, patches@amperecomputing.com,
	vunguyen@os.amperecomputing.com,
	Thang Nguyen <thang@os.amperecomputing.com>,
	Chuong Tran <chuong@os.amperecomputing.com>,
	Phong Vo <phong@os.amperecomputing.com>,
	Michael D Kinney <michael.d.kinney@intel.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Nate DeSimone <nathaniel.l.desimone@intel.com>
Subject: Re: [PATCH v3 12/28] AmpereAltraPkg: Add Ac01PcieLib library instance
Date: Thu, 23 Sep 2021 14:49:31 +0100	[thread overview]
Message-ID: <20210923134931.gzxueebb4axflhdi@leviathan> (raw)
In-Reply-To: <20210915155527.8176-13-nhi@os.amperecomputing.com>

Hi Nhi,

I think all of my comments up to here have been reasonably simple.
This patch I have not reviewed before, so they could be more detailed.

If you'd like to submit a v4 of 1-11, we could possibly merge those
and reduce the churn a bit.

Anyway, review following:

On Wed, Sep 15, 2021 at 22:55:11 +0700, Nhi Pham wrote:
> From: Vu Nguyen <vunguyen@os.amperecomputing.com>
> 
> Provides essential functions to initialize the PCIe Root Complex of
> Ampere Altra processor.
> 
> Cc: Thang Nguyen <thang@os.amperecomputing.com>
> Cc: Chuong Tran <chuong@os.amperecomputing.com>
> Cc: Phong Vo <phong@os.amperecomputing.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> 
> Signed-off-by: Vu Nguyen <vunguyen@os.amperecomputing.com>
> ---
>  Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec                   |    6 +
>  Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc               |    1 +
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/Ac01PcieLib.inf  |   67 +
>  Silicon/Ampere/AmpereAltraPkg/Include/Ac01PcieCommon.h             |  128 ++
>  Silicon/Ampere/AmpereAltraPkg/Include/Library/Ac01PcieLib.h        |  163 ++
>  Silicon/Ampere/AmpereAltraPkg/Include/Library/BoardPcieLib.h       |   92 ++
>  Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h              |   19 +-
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h       |  649 ++++++++
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreCapCfg.h |   63 +
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.h  |   30 +
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c       | 1659 ++++++++++++++++++++
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreLib.c    |  556 +++++++
>  Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.c  |  646 ++++++++
>  13 files changed, 4077 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec b/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec
> index d5b12a81e9bf..93da9b38def5 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec
> +++ b/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec
> @@ -37,6 +37,12 @@ [LibraryClasses]
>    ##  @libraryclass  Defines a set of methods to communicate with secure parition over MM interface.
>    MmCommunicationLib|Silicon/Ampere/AmpereAltraPkg/Include/Library/MmCommunicationLib.h
>  
> +  ##  @libraryclass  Defines a set of methods to initialize Pcie
> +  Ac01PcieLib|Silicon/Ampere/AmpereAltraPkg/Include/Library/Ac01PcieLib.h
> +
> +  ##  @libraryclass  Defines a set of platform PCIe functions
> +  BoardPcieLib|Silicon/Ampere/AmpereAltraPkg/Include/Library/BoardPcieLib.h
> +
>    ##  @libraryclass  Defines a set of methods to access flash memory.
>    FlashLib|Silicon/Ampere/AmpereAltraPkg/Include/Library/FlashLib.h
>  
> diff --git a/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc b/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
> index 4af85f721cb3..d4e1b84c0276 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
> +++ b/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
> @@ -82,6 +82,7 @@ [LibraryClasses.common]
>    NVParamLib|Silicon/Ampere/AmpereAltraPkg/Library/NVParamLib/NVParamLib.inf
>    MailboxInterfaceLib|Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.inf
>    SystemFirmwareInterfaceLib|Silicon/Ampere/AmpereAltraPkg/Library/SystemFirmwareInterfaceLib/SystemFirmwareInterfaceLib.inf
> +  Ac01PcieLib|Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/Ac01PcieLib.inf
>    AmpereCpuLib|Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLib.inf
>    TimeBaseLib|EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.inf
>    I2cLib|Silicon/Ampere/AmpereAltraPkg/Library/DwI2cLib/DwI2cLib.inf
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/Ac01PcieLib.inf b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/Ac01PcieLib.inf
> new file mode 100644
> index 000000000000..e2b0f293eb24
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/Ac01PcieLib.inf
> @@ -0,0 +1,67 @@
> +## @file
> +#
> +# Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001B
> +  BASE_NAME                      = Ac01PcieLib
> +  FILE_GUID                      = 8ABFA0FC-313E-11E8-B467-0ED5F89F718B
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = Ac01PcieLib
> +
> +[Sources]
> +  PcieCore.c
> +  PcieCore.h
> +  PcieCoreCapCfg.h
> +  PcieCoreLib.c
> +  PciePatchAcpi.c
> +  PciePatchAcpi.h
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Ampere/AmpereAltraBinPkg/AmpereAltraBinPkg.dec
> +  Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec
> +  Silicon/Ampere/AmpereSiliconPkg/AmpereSiliconPkg.dec
> +
> +[LibraryClasses]
> +  AcpiLib
> +  AmpereCpuLib
> +  ArmLib
> +  BaseLib
> +  BaseMemoryLib
> +  BoardPcieLib
> +  DebugLib
> +  GpioLib
> +  IoLib
> +  MemoryAllocationLib
> +  PcdLib
> +  PciePhyLib
> +  SerialPortLib
> +  SystemFirmwareInterfaceLib
> +  TimerLib
> +  UefiBootServicesTableLib
> +  UefiLib
> +  UefiRuntimeServicesTableLib
> +
> +[Pcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
> +
> +[Protocols]
> +  gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> +  gEfiAcpiSdtProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> +
> +[Guids]
> +  gPlatformHobGuid
> +
> +[Depex]
> +  gEfiAcpiTableProtocolGuid AND gEfiAcpiSdtProtocolGuid
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Ac01PcieCommon.h b/Silicon/Ampere/AmpereAltraPkg/Include/Ac01PcieCommon.h
> new file mode 100644
> index 000000000000..b72c305f4637
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/Ac01PcieCommon.h
> @@ -0,0 +1,128 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef AC01_PCIE_COMMON_H_
> +#define AC01_PCIE_COMMON_H_
> +
> +#define PRESET_INVALID   0xFF
> +
> +//
> +// PCIe link width
> +//
> +enum {
> +  LNKW_NONE = 0,
> +  LNKW_X1 = 0x1,
> +  LNKW_X2 = 0x2,
> +  LNKW_X4 = 0x4,
> +  LNKW_X8 = 0x8,
> +  LNKW_X16 = 0x10,
> +};

Coding style: enums should
- be typedef:d to an UPPER_CASE_NAME
- have CamelCaseEnumerations
- end with a maximum element.

(https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/appendix_a_common_examples#type-declarations)

> +
> +//
> +// PCIe link speed
> +//
> +enum {
> +  SPEED_NONE = 0,
> +  SPEED_GEN1 = 0x1,
> +  SPEED_GEN2 = 0x2,
> +  SPEED_GEN3 = 0x4,
> +  SPEED_GEN4 = 0x8,
> +};
> +
> +//
> +// PCIe controller number
> +//
> +enum {
> +  PCIE_0 = 0,
> +  PCIE_1,
> +  PCIE_2,
> +  PCIE_3,
> +  PCIE_4,
> +  MAX_PCIE_A = PCIE_4,
> +  PCIE_5,
> +  PCIE_6,
> +  PCIE_7,
> +  MAX_PCIE,
> +  MAX_PCIE_B = MAX_PCIE
> +};
> +
> +//
> +// Root Complex type
> +//
> +enum {
> +  RCA,
> +  RCB
> +};
> +
> +//
> +// Root Complex number
> +//
> +enum {
> +  RCA0 = 0,
> +  RCA1,
> +  RCA2,
> +  RCA3,
> +  MAX_RCA,
> +  RCB0 = MAX_RCA,
> +  RCB1,
> +  RCB2,
> +  RCB3,
> +  MAX_RCB,
> +  MAX_RC = MAX_RCB
> +};
> +
> +//
> +// Data structure to store the PCIe controller information
> +//
> +typedef struct {
> +  UINT64  CsrAddr;               // Pointer to CSR Address

Not a pointer, an address. So comment should say "Base address of CSR
block"? I'd be tempted to suggest renaming the variable CsrBase.

> +  UINT64  SnpsRamAddr;           // Pointer to Synopsys SRAM address
> +  UINT8   MaxGen;                // Max speed Gen-1/-2/-3/-4
> +  UINT8   CurGen;                // Current speed Gen-1/-2/-3/-4

CurrentGen.
CamelCase may be annoying, but CmlCsWAbrvs is worse.

> +  UINT8   MaxWidth;              // Max lanes x2/x4/x8/x16
> +  UINT8   CurWidth;              // Current lanes x2/x4/x8/x16
> +  UINT8   ID;                    // ID of the controller within Root Complex
> +  UINT8   DevNum;                // Device number as part of Bus:Dev:Func
> +  BOOLEAN Active;                // Active? Used in bi-furcation mode
> +  BOOLEAN LinkUp;                // PHY and PCIE linkup
> +  BOOLEAN HotPlug;               // Hotplug support
> +} AC01_PCIE;

_CONTROLLER

> +
> +//
> +// Data structure to store the Root Complex information
> +//
> +typedef struct {
> +  UINT64    BaseAddr;
> +  UINT64    TcuAddr;
> +  UINT64    HBAddr;
> +  UINT64    MsgAddr;
> +  UINT64    SerdesAddr;
> +  UINT64    MmcfgAddr;
> +  UINT64    MmioAddr;
> +  UINT64    MmioSize;
> +  UINT64    Mmio32Addr;
> +  UINT64    Mmio32Size;
> +  UINT64    IoAddr;
> +  AC01_PCIE Pcie[MAX_PCIE_B];
> +  UINT8     MaxPcieController;
> +  UINT8     Type;
> +  UINT8     ID;
> +  UINT8     DevMapHigh:3;           // Copy of High Devmap programmed to Host bridge
> +  UINT8     DevMapLow:3;            // Copy of Low Devmap programmed to Host bridge
> +  UINT8     DefaultDevMapHigh:3;    // Default of High devmap based on board settings
> +  UINT8     DefaultDevMapLow:3;     // Default of Low devmap based on board settings
> +  UINT8     Socket;
> +  BOOLEAN   Active;
> +  UINT8     Logical;
> +  VOID      *RootBridge;            // Pointer to Stack PCI_ROOT_BRIDGE
> +  UINT32    Flags;                  // Flags
> +  UINT8     PresetGen3[MAX_PCIE_B]; // Preset for Gen3
> +  UINT8     PresetGen4[MAX_PCIE_B]; // Preset for Gen4
> +} AC01_RC;

_ROOT_COMPLEX

> +
> +#endif
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Library/Ac01PcieLib.h b/Silicon/Ampere/AmpereAltraPkg/Include/Library/Ac01PcieLib.h
> new file mode 100644
> index 000000000000..47c6452a0499
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/Library/Ac01PcieLib.h
> @@ -0,0 +1,163 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef AC01_PCIE_LIB_H_
> +#define AC01_PCIE_LIB_H_
> +
> +#include <Library/PciHostBridgeLib.h>
> +#include <Protocol/PciHostBridgeResourceAllocation.h>
> +
> +/**
> +  Get RootBridge disable status.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval BOOLEAN                   Return RootBridge disable status.
> +**/
> +BOOLEAN
> +Ac01PcieCheckRootBridgeDisabled (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  );
> +
> +/**
> +  Prepare to start PCIE core BSP driver
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieSetup (
> +  VOID
> +  );
> +
> +/**
> +  Prepare to end PCIE core BSP driver.
> +**/
> +VOID
> +Ac01PcieEnd (
> +  VOID
> +  );
> +
> +/**
> +  Get Total HostBridge.
> +
> +  @retval UINTN                     Return Total HostBridge.
> +**/
> +UINT8
> +Ac01PcieGetTotalHBs (
> +  VOID
> +  );
> +
> +/**
> +  Get Total RootBridge per HostBridge.
> +
> +  @param[in]  RCIndex               Index to identify of Root Complex.
> +
> +  @retval UINTN                     Return Total RootBridge per HostBridge.
> +**/
> +UINT8
> +Ac01PcieGetTotalRBsPerHB (
> +  IN UINTN RCIndex
> +  );
> +
> +/**
> +  Get RootBridge Attribute.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval UINTN                     Return RootBridge Attribute.
> +**/
> +UINTN
> +Ac01PcieGetRootBridgeAttribute (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  );
> +
> +/**
> +  Get RootBridge Segment number
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval UINTN                     Return RootBridge Segment number.
> +**/
> +UINTN
> +Ac01PcieGetRootBridgeSegmentNumber (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  );
> +
> +/**
> +  Initialize Host bridge
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieSetupHostBridge (
> +  IN UINTN HBIndex
> +  );
> +
> +/**
> +  Initialize Root bridge
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +  @param[in]  RootBridgeInstance    The pointer of instance of the Root bridge IO.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieSetupRootBridge (
> +  IN UINTN           HBIndex,
> +  IN UINTN           RBIndex,
> +  IN PCI_ROOT_BRIDGE *RootBridge
> +  );
> +
> +/**
> +  Reads/Writes an PCI configuration register.
> +
> +  @param[in]  RootInstance          Pointer to RootInstance structure.
> +  @param[in]  Address               Address which want to read or write to.
> +  @param[in]  Write                 Indicate that this is a read or write command.
> +  @param[in]  Width                 Specify the width of the data.
> +  @param[in, out]  Data             The buffer to hold the data.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieConfigRW (
> +  IN     VOID    *RootInstance,
> +  IN     UINT64  Address,
> +  IN     BOOLEAN Write,
> +  IN     UINTN   Width,
> +  IN OUT VOID    *Data
> +  );
> +
> +/**
> +  Callback funciton for EndEnumeration notification from PCI stack.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +  @param[in]  Phase                 The phase of enumeration as informed from PCI stack.
> +**/
> +VOID
> +Ac01PcieHostBridgeNotifyPhase (
> +  IN UINTN                                         HBIndex,
> +  IN UINTN                                         RBIndex,
> +  IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PHASE Phase
> +  );
> +
> +#endif /* AC01_PCIE_LIB_H_ */
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Library/BoardPcieLib.h b/Silicon/Ampere/AmpereAltraPkg/Include/Library/BoardPcieLib.h
> new file mode 100644
> index 000000000000..772a85dd5677
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/Library/BoardPcieLib.h
> @@ -0,0 +1,92 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef BOARD_PCIE_LIB_H_
> +#define BOARD_PCIE_LIB_H_
> +
> +#include <Ac01PcieCommon.h>
> +
> +/**
> +  Override the segment number for a root complex with a board specific number.
> +
> +  @param[in]  RC                    Root Complex instance with properties.
> +  @param[out] SegmentNumber         Return segment number.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BoardPcieGetRCSegmentNumber (
> +  IN  AC01_RC *RC,
> +  OUT UINTN   *SegmentNumber
> +  );
> +
> +/**
> +  Check if SMM PMU enabled in board screen.
> +
> +  @param[out]  IsSmmuPmuEnabled     TRUE if the SMMU PMU enabled, otherwise FALSE.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BoardPcieCheckSmmuPmuEnabled (
> +  OUT BOOLEAN *IsSmmuPmuEnabled
> +  );
> +
> +/**
> +  Build PCIe menu screen.
> +
> +  @param[in]  RCList                List of Root Complex with properties.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BoardPcieScreenInitialize (
> +  IN AC01_RC *RCList
> +  );
> +
> +/**
> +  Parse platform Root Complex information.
> +
> +  @param[out]  RC                   Root Complex instance to store the platform information.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BoardPcieParseRCParams (
> +  OUT AC01_RC *RC
> +  );
> +
> +/**
> +  Assert PERST of input PCIe controller
> +
> +  @param[in]  RC                    Root Complex instance.
> +  @param[in]  PcieIndex             PCIe controller index of input Root Complex.
> +  @param[in]  Bifurcation           Bifurcation mode of input Root Complex.
> +  @param[in]  IsPullToHigh          Target status for the PERST.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BoardPcieAssertPerst (
> +  IN AC01_RC *RC,
> +  IN UINT32  PcieIndex,
> +  IN UINT8   Bifurcation,
> +  IN BOOLEAN IsPullToHigh
> +  );
> +
> +#endif /* BOARD_PCIE_LIB_H_ */
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h b/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h
> index 66286bfff145..ed25c6eaa3b8 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h
> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h
> @@ -264,15 +264,30 @@
>  //
>  #define AC01_PCIE_MMIO_BASE         0x300000000000, 0x340000000000, 0x380000000000, 0x3C0000000000, 0x200000000000, 0x240000000000, 0x280000000000, 0x2C0000000000, 0x700000000000, 0x740000000000, 0x780000000000, 0x7C0000000000, 0x600000000000, 0x640000000000, 0x680000000000, 0x6C0000000000
>

Hmm, some of these items would appear to have esacped to an earlier
commit that doesn't use/need them.
("Ampere: Initial support for Ampere Altra processor and Mt. Jade platform")

They should be moved to their appropriate place in the timeline.

But this also made me notice something that had passed me by on
initial review of that patch. I will make a separate comment on patch
1 for this, so it doesn't get lost.

But to state it here. The use of lists in #defines kind of pushed me
off the chair at first notice (I must have parsed them as comments on
the first pass). But I'm coming around to it, and I don't see it
explicitly banned by the coding style.

*However* these currently have misleading names.
The names *must* highlight that they are lists.
I guess the least impact would be to append _LIST to the name of any
macro holding a list. Saves on figuring out practical plural forms...

> +//
> +// The size of MMIO space
> +//
> +#define AC01_PCIE_MMIO_SIZE         0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000, 0x3FFE0000000
> +
>  //
>  // The base address of MMIO32 Registers
>  //
> -#define AC01_PCIE_MMIO32_BASE       0x000020000000, 0x000028000000, 0x000030000000, 0x000038000000, 0x000001000000, 0x000008000000, 0x000010000000, 0x000018000000, 0x000060000000, 0x000068000000, 0x000070000000, 0x000078000000, 0x000040000000, 0x000048000000, 0x000050000000, 0x000058000000
> +#define AC01_PCIE_MMIO32_BASE       0x000020000000, 0x000028000000, 0x000030000000, 0x000038000000, 0x000004000000, 0x000008000000, 0x000010000000, 0x000018000000, 0x000060000000, 0x000068000000, 0x000070000000, 0x000078000000, 0x000040000000, 0x000048000000, 0x000050000000, 0x000058000000
> +
> +//
> +// The size of MMIO32 space
> +//
> +#define AC01_PCIE_MMIO32_SIZE       0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x4000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000, 0x8000000
>  
>  //
>  // The base address of MMIO32 Registers
>  //
> -#define AC01_PCIE_MMIO32_BASE_1P    0x000040000000, 0x000050000000, 0x000060000000, 0x000070000000, 0x000001000000, 0x000010000000, 0x000020000000, 0x000030000000, 0, 0, 0, 0, 0, 0, 0, 0
> +#define AC01_PCIE_MMIO32_BASE_1P    0x000040000000, 0x000050000000, 0x000060000000, 0x000070000000, 0x000008000000, 0x000010000000, 0x000020000000, 0x000030000000, 0, 0, 0, 0, 0, 0, 0, 0
> +
> +//
> +// The size of MMIO32 1P space
> +//
> +#define AC01_PCIE_MMIO32_SIZE_1P    0x10000000, 0x10000000, 0x10000000, 0x10000000, 0x8000000, 0x10000000, 0x10000000, 0x10000000, 0, 0, 0, 0, 0, 0, 0, 0
>  
>  //
>  // DSDT RCA2 PCIe Meme32 Attribute
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
> new file mode 100644
> index 000000000000..f84f7cec9fae
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
> @@ -0,0 +1,649 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef AC01_PCIE_CORE_H_
> +#define AC01_PCIE_CORE_H_
> +
> +#include <IndustryStandard/Pci.h>
> +#include <Library/ArmLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PciHostBridgeLib.h>
> +#include <Library/PciLib.h>
> +#include <Library/TimerLib.h>
> +#include <Library/UefiLib.h>
> +
> +#include <Ac01PcieCommon.h>
> +#include "PcieCoreCapCfg.h"
> +
> +#ifdef ENABLE_DEBUG_CFG
> +#define DEBUG_PCIE_CFG(arg...)               \
> +  if (DebugCodeEnabled()) {                  \
> +    DEBUG ((DEBUG_INFO,"PCICore (CFG): "));  \
> +    DEBUG ((DEBUG_INFO,## arg));             \
> +  }

Urgh, do we really need this debug infrastructure overlay?

> +#else
> +#define DEBUG_PCIE_CFG(arg...)
> +#endif
> +
> +#ifdef ENABLE_DEBUG_CSR
> +#define DEBUG_PCIE_CSR(arg...)               \
> +  if (DebugCodeEnabled()) {                  \
> +    DEBUG ((DEBUG_INFO,"PCICore (CSR): "));  \
> +    DEBUG ((DEBUG_INFO,## arg));              \
> +  }
> +#else
> +#define DEBUG_PCIE_CSR(arg...)
> +#endif
> +
> +#ifdef ENABLE_DEBUG_PHY
> +#define DEBUG_PCIE_PHY(arg...)               \
> +  if (DebugCodeEnabled()) {                  \
> +    DEBUG ((DEBUG_INFO,"PCICore (PHY): "));  \
> +    DEBUG ((DEBUG_INFO,## arg));             \
> +  }
> +#else
> +#define DEBUG_PCIE_PHY(arg...)
> +#endif
> +
> +#ifdef ENABLE_DEBUG_INFO
> +#define DEBUG_PCIE_INFO(arg...)                   \
> +  if (DebugCodeEnabled()) {                  \
> +    DEBUG ((DEBUG_INFO,"PCICore (DBG): "));  \
> +    DEBUG ((DEBUG_INFO,## arg));             \
> +  }
> +#else
> +#define DEBUG_PCIE_INFO(arg...)
> +#endif
> +
> +#define DEBUG_PCIE_ERR(arg...)                      \
> +  DEBUG ((DEBUG_ERROR,"PCICore (ERROR): "));  \
> +  DEBUG ((DEBUG_ERROR,## arg));
> +
> +#ifndef BIT
> +#define BIT(nr)                         (1 << (nr))
> +#endif
> +
> +
> +#define SLOT_POWER_LIMIT                 75     // Watt
> +
> +#define MAX_REINIT                       3
> +
> +#define LINK_CHECK_SUCCESS               0
> +#define LINK_CHECK_FAILED                -1
> +#define LINK_CHECK_WRONG_PARAMETER       1
> +
> +#define PFA_REG_ENABLE                   0
> +#define PFA_REG_READ                     1
> +#define PFA_REG_CLEAR                    2
> +
> +#define AMPERE_PCIE_VENDORID             0x1DEF
> +#define AC01_HOST_BRIDGE_DEVICEID_RCA    0xE100
> +#define AC01_HOST_BRIDGE_DEVICEID_RCB    0xE110
> +#define AC01_PCIE_BRIDGE_DEVICEID_RCA    0xE101
> +#define AC01_PCIE_BRIDGE_DEVICEID_RCB    0xE111
> +
> +#define MEMRDY_TIMEOUT                   10          // 10 us
> +#define PIPE_CLOCK_TIMEOUT               20000       // 20,000 us
> +#define LTSSM_TRANSITION_TIMEOUT         100000      // 100 ms in total
> +#define EP_LINKUP_TIMEOUT                (10 * 1000) // 10ms
> +#define LINK_WAIT_INTERVAL_US            50
> +
> +#define LINK_POLL_US_TIMER      1
> +#define IO_SPACE                0x2000
> +
> +#define TCU_OFFSET              0
> +#define HB_CSR_OFFSET           0x01000000
> +#define PCIE0_CSR_OFFSET        0x01010000
> +#define SNPSRAM_OFFSET          0x9000
> +#define SERDES_CSR_OFFSET       0x01200000
> +#define MMCONFIG_OFFSET         0x10000000
> +
> +
> +/* DATA LINK registers */
> +#define DLINK_VENDOR_CAP_ID       0x25
> +#define DLINK_VSEC                0x80000001
> +#define DATA_LINK_FEATURE_CAP_OFF 0X4
> +
> +/* PL16 CAP registers */
> +#define PL16_CAP_ID                     0x26
> +#define PL16G_CAP_OFF_20H_REG_OFF       0x20
> +#define PL16G_STATUS_REG_OFF            0x0C
> +#define PL16G_STATUS_EQ_CPL_GET(val)    (val & 0x1)
> +#define PL16G_STATUS_EQ_CPL_P1_GET(val) ((val & 0x2) >> 1)
> +#define PL16G_STATUS_EQ_CPL_P2_GET(val) ((val & 0x4) >> 2)
> +#define PL16G_STATUS_EQ_CPL_P3_GET(val) ((val & 0x8) >> 3)
> +#define DSP_16G_TX_PRESET0_SET(dst,src) (((dst) & ~0xF) | (((UINT32) (src)) & 0xF))
> +#define DSP_16G_TX_PRESET1_SET(dst,src) (((dst) & ~0xF00) | (((UINT32) (src) << 8) & 0xF00))
> +#define DSP_16G_TX_PRESET2_SET(dst,src) (((dst) & ~0xF0000) | (((UINT32) (src) << 16) & 0xF0000))
> +#define DSP_16G_TX_PRESET3_SET(dst,src) (((dst) & ~0xF000000) | (((UINT32) (src) << 24) & 0xF000000))
> +#define DSP_16G_RXTX_PRESET0_SET(dst,src) (((dst) & ~0xFF) | (((UINT32) (src)) & 0xFF))
> +#define DSP_16G_RXTX_PRESET1_SET(dst,src) (((dst) & ~0xFF00) | (((UINT32) (src) << 8) & 0xFF00))
> +#define DSP_16G_RXTX_PRESET2_SET(dst,src) (((dst) & ~0xFF0000) | (((UINT32) (src) << 16) & 0xFF0000))
> +#define DSP_16G_RXTX_PRESET3_SET(dst,src) (((dst) & ~0xFF000000) | (((UINT32) (src) << 24) & 0xFF000000))
> +
> +/* PCIe PF0_PORT_LOGIC registers */
> +#define PORT_LOCIG_VC0_P_RX_Q_CTRL_OFF      0x748
> +#define PORT_LOCIG_VC0_NP_RX_Q_CTRL_OFF     0x74C
> +
> +/* TCU registers */
> +#define SMMU_GBPA    0x044
> +
> +/* SNPSRAM Synopsys Memory Read/Write Margin registers */
> +#define SPRF_RMR                0x0
> +#define SPSRAM_RMR              0x4
> +#define TPRF_RMR                0x8
> +#define TPSRAM_RMR              0xC
> +
> +//
> +// Host bridge registers
> +//
> +#define HBRCAPDMR               0x0
> +#define HBRCBPDMR               0x4
> +#define HBPDVIDR                0x10
> +#define HBPRBNR                 0x14
> +#define HBPREVIDR               0x18
> +#define HBPSIDR                 0x1C
> +#define HBPCLSSR                0x20
> +
> +// HBRCAPDMR
> +#define RCAPCIDEVMAP_SET(dst, src) (((dst) & ~0x7) | (((UINT32) (src)) & 0x7))
> +#define RCAPCIDEVMAP_GET(val) ((val) & 0x7)
> +
> +// HBRCBPDMR
> +#define RCBPCIDEVMAPLO_SET(dst, src) (((dst) & ~0x7) | (((UINT32) (src)) & 0x7))
> +#define RCBPCIDEVMAPLO_GET(val) ((val) & 0x7)
> +
> +#define RCBPCIDEVMAPHI_SET(dst, src) (((dst) & ~0x70) | (((UINT32) (src) << 4) & 0x70))
> +#define RCBPCIDEVMAPHI_GET(val) (((val) & 0x7) >> 4)
> +
> +// HBPDVIDR
> +#define PCIVENDID_SET(dst, src) (((dst) & ~0xFFFF) | (((UINT32) (src))  & 0xFFFF))
> +#define PCIVENDID_GET(val) ((val) & 0xFFFF)
> +
> +#define PCIDEVID_SET(dst, src) (((dst) & ~0xFFFF0000) | (((UINT32) (src) << 16) & 0xFFFF0000))
> +#define PCIDEVID_GET(val) (((val) & 0xFFFF0000) >> 16)
> +
> +// HBPRBNR
> +#define PCIRBNUM_SET(dst, src) (((dst) & ~0x1F) | (((UINT32) (src)) & 0x1F))
> +
> +// HBPREVIDR
> +#define PCIREVID_SET(dst, src) (((dst) & ~0xFF) | (((UINT32) (src)) & 0xFF))
> +
> +// HBPSIDR
> +#define PCISUBSYSVENDID_SET(dst, src) (((dst) & ~0xFFFF) | (((UINT32) (src)) & 0xFFFF))
> +
> +#define PCISUBSYSID_SET(dst, src) (((dst) & ~0xFFFF0000) | (((UINT32) (src) << 16) & 0xFFFF0000))
> +
> +// HBPCLSSR
> +#define CACHELINESIZE_SET(dst, src) (((dst) & ~0xFF) | (((UINT32) (src)) & 0xFF))
> +
> +//
> +// PCIE core register
> +//
> +#define LINKCTRL                0x0
> +#define LINKSTAT                0x4
> +#define IRQSEL                  0xC
> +#define HOTPLUGSTAT             0x28
> +#define IRQENABLE               0x30
> +#define IRQEVENTSTAT            0x38
> +#define BLOCKEVENTSTAT          0x3c
> +#define RESET                   0xC000
> +#define CLOCK                   0xC004
> +#define MEMRDYR                 0xC104
> +#define RAMSDR                  0xC10C
> +
> +// LINKCTRL
> +#define LTSSMENB_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +#define DEVICETYPE_SET(dst, src) (((dst) & ~0xF0) | (((UINT32) (src) << 4) & 0xF0))
> +#define DEVICETYPE_GET(val) (((val) & 0xF0) >> 4)
> +
> +// LINKSTAT
> +#define PHY_STATUS_MASK         (1 << 2)
> +#define SMLH_LTSSM_STATE_MASK   0x3f00
> +#define SMLH_LTSSM_STATE_GET(val) ((val & 0x3F00) >> 8)
> +#define RDLH_SMLH_LINKUP_STATUS_GET(val)    (val & 0x3)
> +#define PHY_STATUS_MASK_BIT     0x04
> +#define SMLH_LINK_UP_MASK_BIT   0x02
> +#define RDLH_LINK_UP_MASK_BIT   0x01
> +
> +// IRQSEL
> +#define AER_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +#define PME_SET(dst, src) (((dst) & ~0x2) | (((UINT32) (src) << 1) & 0x2))
> +#define LINKAUTOBW_SET(dst, src) (((dst) & ~0x4) | (((UINT32) (src) << 2) & 0x4))
> +#define BWMGMT_SET(dst, src) (((dst) & ~0x8) | (((UINT32) (src) << 3) & 0x8))
> +#define EQRQST_SET(dst, src) (((dst) & ~0x10) | (((UINT32) (src) << 4) & 0x10))
> +#define INTPIN_SET(dst, src) (((dst) & ~0xFF00) | (((UINT32) (src) << 8) & 0xFF00))
> +
> +// SLOTCAP
> +#define SLOT_HPC_SET(dst, src) (((dst) & ~0x40) | (((UINT32) (src) << 6) & 0x40))
> +
> +// SLOT_CAPABILITIES_REG, PCIE_CAP_SLOT_POWER_LIMIT_VALUE bits[14:7]
> +#define SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET(dst, src) \
> +                            (((dst) & ~0x7F80) | (((UINT32)(src) << 7) & 0x7F80))
> +
> +// HOTPLUGSTAT
> +#define PWR_IND_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +#define ATTEN_IND_SET(dst, src) (((dst) & ~0x2) | (((UINT32) (src) << 1) & 0x2))
> +#define PWR_CTRL_SET(dst, src) (((dst) & ~0x4) | (((UINT32) (src) << 2) & 0x4))
> +#define EML_CTRL_SET(dst, src) (((dst) & ~0x8) | (((UINT32) (src) << 3) & 0x8))
> +
> +// IRQENABLE
> +#define LINKUP_SET(dst, src) (((dst) & ~0x40) | (((UINT32) (src) << 6) & 0x40))
> +
> +// IRQEVENTSTAT
> +#define BLOCK_INT_MASK          (1 << 4)
> +#define INT_MASK                (1 << 3)
> +
> +// BLOCKEVENTSTAT
> +#define LINKUP_MASK             (1 << 0)
> +
> +// RESET
> +#define DWCPCIE_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +#define RESET_MASK              0x1
> +
> +// CLOCK
> +#define AXIPIPE_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +
> +// RAMSDR
> +#define SD_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +
> +//
> +// PHY registers
> +//
> +#define RSTCTRL                 0x0
> +#define PHYCTRL                 0x4
> +#define RAMCTRL                 0x8
> +#define RAMSTAT                 0xC
> +#define PLLCTRL                 0x10
> +#define PHYLPKCTRL              0x14
> +#define PHYTERMOFFSET0          0x18
> +#define PHYTERMOFFSET1          0x1C
> +#define PHYTERMOFFSET2          0x20
> +#define PHYTERMOFFSET3          0x24
> +#define RXTERM                  0x28
> +#define PHYDIAGCTRL             0x2C
> +
> +// RSTCTRL
> +#define PHY_RESET_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +
> +// PHYCTRL
> +#define PWR_STABLE_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +
> +//
> +// PCIe config space registers
> +//
> +#define TYPE1_DEV_ID_VEND_ID_REG                0
> +#define TYPE1_CLASS_CODE_REV_ID_REG             0x8
> +#define TYPE1_CAP_PTR_REG                       0x34
> +#define SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG       0x18
> +#define BRIDGE_CTRL_INT_PIN_INT_LINE_REG        0x3c
> +#define CON_STATUS_REG                          (PM_CAP + 0x4)
> +#define LINK_CAPABILITIES_REG                   (PCIE_CAP + 0xc)
> +#define LINK_CONTROL_LINK_STATUS_REG            (PCIE_CAP + 0x10)
> +#define SLOT_CAPABILITIES_REG                   (PCIE_CAP + 0x14)
> +#define DEVICE_CONTROL2_DEVICE_STATUS2_REG      (PCIE_CAP + 0x28)
> +#define LINK_CAPABILITIES2_REG                  (PCIE_CAP + 0x2c)
> +#define LINK_CONTROL2_LINK_STATUS2_REG          (PCIE_CAP + 0x30)
> +#define UNCORR_ERR_STATUS_OFF                   (AER_CAP + 0x4)
> +#define UNCORR_ERR_MASK_OFF                     (AER_CAP + 0x8)
> +#define RESOURCE_CON_REG_VC0                    (VC_CAP + 0x14)
> +#define RESOURCE_CON_REG_VC1                    (VC_CAP + 0x20)
> +#define RESOURCE_STATUS_REG_VC1                 (VC_CAP + 0x24)
> +#define SD_CONTROL1_REG                         (RAS_DES_CAP+0xA0)
> +#define CCIX_TP_CAP_TP_HDR2_OFF                 (CCIX_TP_CAP + 0x8)
> +#define ESM_MNDTRY_RATE_CAP_OFF                 (CCIX_TP_CAP + 0xc)
> +#define ESM_STAT_OFF                            (CCIX_TP_CAP + 0x14)
> +#define ESM_CNTL_OFF                            (CCIX_TP_CAP + 0x18)
> +#define ESM_LN_EQ_CNTL_25G_0_OFF                (CCIX_TP_CAP + 0x2c)
> +#define PORT_LINK_CTRL_OFF                      0x710
> +#define FILTER_MASK_2_OFF                       0x720
> +#define GEN2_CTRL_OFF                           0x80c
> +#define GEN3_RELATED_OFF                        0x890
> +#define GEN3_EQ_CONTROL_OFF                     0x8A8
> +#define MISC_CONTROL_1_OFF                      0x8bc
> +#define AMBA_ERROR_RESPONSE_DEFAULT_OFF         0x8d0
> +#define AMBA_LINK_TIMEOUT_OFF                   0x8d4
> +#define AMBA_ORDERING_CTRL_OFF                  0x8d8
> +#define DTIM_CTRL0_OFF                          0xab0
> +#define AUX_CLK_FREQ_OFF                        0xb40
> +#define CCIX_CTRL_OFF                           0xc20
> +
> +#define DEV_MASK 0x00F8000
> +#define BUS_MASK 0xFF00000
> +#define BUS_NUM(Addr) ((((UINT64)(Addr)) & BUS_MASK) >> 20)
> +#define DEV_NUM(Addr) ((((UINT64)(Addr)) & DEV_MASK) >> 15)
> +#define CFG_REG(Addr) (((UINT64)Addr) & 0x7FFF)
> +
> +// TYPE1_DEV_ID_VEND_ID_REG
> +#define VENDOR_ID_SET(dst, src) (((dst) & ~0xFFFF) | (((UINT32) (src)) & 0xFFFF))
> +#define DEVICE_ID_SET(dst, src) (((dst) & ~0xFFFF0000) | (((UINT32) (src) << 16) & 0xFFFF0000))
> +
> +// TYPE1_CLASS_CODE_REV_ID_REG
> +#define BASE_CLASS_CODE_SET(dst, src) (((dst) & ~0xFF000000) | (((UINT32) (src) << 24) & 0xFF000000))
> +#define SUBCLASS_CODE_SET(dst, src) (((dst) & ~0xFF0000) | (((UINT32) (src) << 16) & 0xFF0000))
> +#define PROGRAM_INTERFACE_SET(dst, src) (((dst) & ~0xFF00) | (((UINT32) (src) << 8) & 0xFF00))
> +#define REVISION_ID_SET(dst, src) (((dst) & ~0xFF) | (((UINT32) (src)) & 0xFF))
> +
> +// SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG
> +#define DEFAULT_SUB_BUS       0xFF
> +#define SUB_BUS_SET(dst, src) (((dst) & ~0xFF0000) | (((UINT32) (src) << 16) & 0xFF0000))
> +#define SEC_BUS_SET(dst, src) (((dst) & ~0xFF00) | (((UINT32) (src) << 8) & 0xFF00))
> +#define PRIM_BUS_SET(dst, src) (((dst) & ~0xFF) | (((UINT32) (src)) & 0xFF))
> +
> +// BRIDGE_CTRL_INT_PIN_INT_LINE_REG
> +#define INT_PIN_SET(dst, src) (((dst) & ~0xFF00) | (((UINT32) (src) << 8) & 0xFF00))
> +
> +// CON_STATUS_REG
> +#define POWER_STATE_SET(dst, src) (((dst) & ~0x3) | (((UINT32) (src)) & 0x3))
> +
> +// DEVICE_CONTROL2_DEVICE_STATUS2_REG
> +#define CAP_CPL_TIMEOUT_VALUE_SET(dst, src) (((dst) & ~0xF) | (((UINT32) (src)) & 0xF))
> +
> +// LINK_CAPABILITIES_REG
> +#define CAP_ID                                  0x10
> +#define LINK_CAPABILITIES_REG_OFF               0xC
> +#define LINK_CONTROL_LINK_STATUS_OFF            0x10
> +#define CAP_MAX_LINK_WIDTH_X1                   0x1
> +#define CAP_MAX_LINK_WIDTH_X2                   0x2
> +#define CAP_MAX_LINK_WIDTH_X4                   0x4
> +#define CAP_MAX_LINK_WIDTH_X8                   0x8
> +#define CAP_MAX_LINK_WIDTH_X16                  0x10
> +#define CAP_MAX_LINK_WIDTH_GET(val)             ((val & 0x3F0) >> 4)
> +#define CAP_MAX_LINK_WIDTH_SET(dst, src)        (((dst) & ~0x3F0) | (((UINT32) (src) << 4) & 0x3F0))
> +#define MAX_LINK_SPEED_25                       0x1
> +#define MAX_LINK_SPEED_50                       0x2
> +#define MAX_LINK_SPEED_80                       0x3
> +#define MAX_LINK_SPEED_160                      0x4
> +#define MAX_LINK_SPEED_320                      0x5
> +#define CAP_MAX_LINK_SPEED_GET(val)             ((val & 0xF))
> +#define CAP_MAX_LINK_SPEED_SET(dst, src)        (((dst) & ~0xF) | (((UINT32) (src)) & 0xF))
> +#define CAP_SLOT_CLK_CONFIG_SET(dst, src)       (((dst) & ~0x10000000) | (((UINT32) (src) << 28) & 0x10000000))
> +#define NO_ASPM_SUPPORTED                       0x0
> +#define L0S_SUPPORTED                           0x1
> +#define L1_SUPPORTED                            0x2
> +#define L0S_L1_SUPPORTED                        0x3
> +#define CAP_ACTIVE_STATE_LINK_PM_SUPPORT_SET(dst, src) (((dst) & ~0xC00) | (((UINT32)(src) << 10) & 0xC00))
> +
> +// LINK_CONTROL_LINK_STATUS_REG
> +#define CAP_DLL_ACTIVE_GET(val)                 ((val & 0x20000000) >> 29)
> +#define CAP_NEGO_LINK_WIDTH_GET(val)            ((val & 0x3F00000) >> 20)
> +#define CAP_LINK_SPEED_GET(val)                 ((val & 0xF0000) >> 16)
> +#define CAP_LINK_SPEED_SET(dst, src)            (((dst) & ~0xF0000) | (((UINT32) (src) << 16) & 0xF0000))
> +#define CAP_LINK_SPEED_TO_VECTOR(val)           BIT((val)-1)
> +#define CAP_EN_CLK_POWER_MAN_GET(val)           ((val & 0x100) >> 8)
> +#define CAP_EN_CLK_POWER_MAN_SET(dst, src)      (((dst) & ~0x100) | (((UINT32) (src) << 8) & 0x100))
> +#define CAP_RETRAIN_LINK_SET(dst, src)          (((dst) & ~0x20) | (((UINT32) (src) << 5) & 0x20))
> +#define CAP_COMMON_CLK_SET(dst, src)            (((dst) & ~0x40) | (((UINT32) (src) << 6) & 0x40))
> +#define CAP_LINK_TRAINING_GET(val)              ((val & 0x8000000) >> 27)
> +#define CAP_LINK_DISABLE_SET(dst, src)          (((dst) & ~0x10) | (((UINT32)(src) << 4) & 0x10))
> +
> +// LINK_CAPABILITIES2_REG
> +#define LINK_SPEED_VECTOR_25                    BIT(0)

Use BIT0-BIT63 from Base.h instead of the macro when referring to constants.

> +#define LINK_SPEED_VECTOR_50                    BIT(1)
> +#define LINK_SPEED_VECTOR_80                    BIT(2)
> +#define LINK_SPEED_VECTOR_160                   BIT(3)
> +#define LINK_SPEED_VECTOR_320                   BIT(4)
> +#define CAP_SUPPORT_LINK_SPEED_VECTOR_GET(val)  ((val & 0xFE) >> 1)
> +#define CAP_SUPPORT_LINK_SPEED_VECTOR_SET(dst, src) (((dst) & ~0xFE) | (((UINT32) (src) << 1) & 0xFE))

Umm, all of these LINK_SPEED_VECTOR macros exist only in this file.
If they are intended to be used only in later patches, please hold
them back until then.

> +#define CAP_EQ_CPL_GET(val)                     ((val & 0x20000) >> 17)
> +#define CAP_EQ_CPL_P1_GET(val)                  ((val & 0x40000) >> 18)
> +#define CAP_EQ_CPL_P2_GET(val)                  ((val & 0x80000) >> 19)
> +#define CAP_EQ_CPL_P3_GET(val)                  ((val & 0x100000) >> 20)

Also unused.
Could this (very long) header be pruned of contents not actually used?

> +
> +// LINK_CONTROL2_LINK_STATUS2_REG
> +#define CAP_TARGET_LINK_SPEED_SET(dst, src)     (((dst) & ~0xF) | (((UINT32) (src)) & 0xF))
> +
> +// Secondary Capability
> +#define SPCIE_CAP_ID                0x19
> +#define CAP_OFF_0C                  0x0C
> +#define LINK_CONTROL3_REG_OFF       0x4
> +#define DSP_TX_PRESET0_SET(dst,src)  (((dst) & ~0xF) | (((UINT32) (src)) & 0xF))
> +#define DSP_TX_PRESET1_SET(dst,src)  (((dst) & ~0xF0000) | (((UINT32) (src) << 16) & 0xF0000))
> +
> +// UNCORR_ERR_STATUS_OFF
> +#define CMPLT_TIMEOUT_ERR_STATUS_GET(val) ((val & 0x4000) >> 14)
> +#define CMPLT_TIMEOUT_ERR_STATUS_SET(dst, src) (((dst) & ~0x4000) | (((UINT32) (src) << 14) & 0x4000))
> +
> +// UNCORR_ERR_MASK_OFF
> +#define CMPLT_TIMEOUT_ERR_MASK_SET(dst, src) (((dst) & ~0x4000) | (((UINT32) (src) << 14) & 0x4000))
> +#define SDES_ERR_MASK_SET(dst, src) (((dst) & ~0x20) | (((UINT32)(src) << 5) & 0x20))
> +
> +// RESOURCE_STATUS_REG_VC1
> +#define VC_NEGO_PENDING_VC1_GET(val) ((val & 0x20000) >> 17)
> +
> +// SD_CONTROL1_REG
> +#define FORCE_DETECT_LANE_EN_SET(dst, src) (((dst) & ~0x10000) | (((UINT32) (src) << 16) & 0x10000))
> +
> +// CCIX_TP_CAP_TP_HDR2_OFF
> +#define ESM_REACH_LENGTH_GET(val) ((val & 0x60000) >> 17)
> +#define ESM_CALIBRATION_TIME_GET(val) ((val & 0x700000) >> 20)
> +#define ESM_CALIBRATION_TIME_SET(dst, src) (((dst) & ~0x700000) | (((UINT32) (src) << 20) & 0x700000))
> +
> +// ESM_STAT_OFF
> +#define ESM_CALIB_CMPLT_GET(val) ((val & 0x80) >> 7)
> +#define ESM_CURNT_DATA_RATE_GET(val) ((val & 0x7F) >> 0)
> +
> +// ESM_CNTL_OFF
> +#define QUICK_EQ_TIMEOUT_SET(dst, src) (((dst) & ~0x1C000000) | (((UINT32) (src) << 26) & 0x1C000000))
> +#define LINK_REACH_TARGET_GET(val) ((val & 0x1000000) >> 24)
> +#define LINK_REACH_TARGET_SET(dst, src) (((dst) & ~0x1000000) | (((UINT32) (src) << 24) & 0x1000000))
> +#define ESM_EXT_EQ3_DSP_TIMEOUT_GET(val) ((val & 0x700000) >> 20)
> +#define ESM_EXT_EQ3_DSP_TIMEOUT_SET(dst, src) (((dst) & ~0x700000) | (((UINT32) (src) << 20) & 0x700000))
> +#define ESM_EXT_EQ2_USP_TIMEOUT_GET(val) ((val & 0x70000) >> 16)
> +#define ESM_EXT_EQ2_USP_TIMEOUT_SET(dst, src) (((dst) & ~0x70000) | (((UINT32) (src) << 16) & 0x70000))
> +#define ESM_ENABLE_SET(dst, src) (((dst) & ~0x8000) | (((UINT32) (src) << 15) & 0x8000))
> +#define ESM_DATA_RATE1_SET(dst, src) (((dst) & ~0x7F00) | (((UINT32) (src) << 8) & 0x7F00))
> +#define ESM_PERFORM_CAL_SET(dst, src) (((dst) & ~0x80) | (((UINT32) (src) << 7) & 0x80))
> +#define ESM_DATA_RATE0_SET(dst, src) (((dst) & ~0x7F) | (((UINT32) (src)) & 0x7F))
> +
> +// PORT_LINK_CTRL_OFF
> +#define LINK_CAPABLE_X1                 0x1
> +#define LINK_CAPABLE_X2                 0x3
> +#define LINK_CAPABLE_X4                 0x7
> +#define LINK_CAPABLE_X8                 0xF
> +#define LINK_CAPABLE_X16                0x1F
> +#define LINK_CAPABLE_X32                0x3F
> +#define LINK_CAPABLE_SET(dst, src) (((dst) & ~0x3F0000) | (((UINT32) (src) << 16) & 0x3F0000))
> +#define FAST_LINK_MODE_SET(dst, src) (((dst) & ~0x80) | (((UINT32) (src) << 7) & 0x80))
> +
> +// FILTER_MASK_2_OFF
> +#define CX_FLT_MASK_VENMSG0_DROP_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +#define CX_FLT_MASK_VENMSG1_DROP_SET(dst, src) (((dst) & ~0x2) | (((UINT32) (src) << 1) & 0x2))
> +#define CX_FLT_MASK_DABORT_4UCPL_SET(dst, src) (((dst) & ~0x4) | (((UINT32) (src) << 2) & 0x4))
> +
> +// GEN2_CTRL_OFF
> +#define NUM_OF_LANES_X2                 0x2
> +#define NUM_OF_LANES_X4                 0x4
> +#define NUM_OF_LANES_X8                 0x8
> +#define NUM_OF_LANES_X16                0x10
> +#define NUM_OF_LANES_SET(dst, src) (((dst) & ~0x1F00) | (((UINT32) (src) << 8) & 0x1F00))
> +
> +// GEN3_RELATED_OFF
> +#define RATE_SHADOW_SEL_SET(dst, src) (((dst) & ~0x3000000) | (((UINT32) (src) << 24) & 0x3000000))
> +#define EQ_PHASE_2_3_SET(dst, src) (((dst) & ~0x200) | (((UINT32) (src) << 9) & 0x200))
> +#define RXEQ_REGRDLESS_SET(dst, src) (((dst) & ~0x2000) | (((UINT32) (src) << 13) & 0x2000))
> +
> +// GEN3_EQ_CONTROL_OFF
> +#define GEN3_EQ_FB_MODE(dst, src) (((dst) & ~0xF) | ((UINT32) (src) & 0xF))
> +#define GEN3_EQ_PRESET_VEC(dst, src) (((dst) & 0xFF0000FF) | (((UINT32) (src) << 8) & 0xFFFF00))
> +#define GEN3_EQ_INIT_EVAL(dst,src) (((dst) & ~0x1000000) | (((UINT32) (src) << 24) & 0x1000000))
> +
> +// MISC_CONTROL_1_OFF
> +#define DBI_RO_WR_EN_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +
> +// AMBA_ERROR_RESPONSE_DEFAULT_OFF
> +#define AMBA_ERROR_RESPONSE_CRS_SET(dst, src) (((dst) & ~0x18) | (((UINT32) (src) << 3) & 0x18))
> +#define AMBA_ERROR_RESPONSE_GLOBAL_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +
> +// AMBA_LINK_TIMEOUT_OFF
> +#define LINK_TIMEOUT_PERIOD_DEFAULT_SET(dst, src) (((dst) & ~0xFF) | (((UINT32) (src)) & 0xFF))
> +
> +// AMBA_ORDERING_CTRL_OFF
> +#define AX_MSTR_ZEROLREAD_FW_SET(dst, src) (((dst) & ~0x80) | (((UINT32) (src) << 7) & 0x80))
> +
> +// DTIM_CTRL0_OFF
> +#define DTIM_CTRL0_ROOT_PORT_ID_SET(dst, src) (((dst) & ~0xFFFF) | (((UINT32) (src)) & 0xFFFF))
> +
> +// AUX_CLK_FREQ_OFF
> +#define AUX_CLK_500MHZ 500
> +#define AUX_CLK_FREQ_SET(dst, src) (((dst) & ~0x1FF) | (((UINT32) (src)) & 0x1FF))
> +
> +#define EXT_CAP_OFFSET_START 0x100
> +
> +// EVENT_COUNTER_CONTROL_REG
> +#define ECCR_GROUP_EVENT_SEL_SET(dst, src)          (((dst) & ~0xFFF0000) | (((UINT32)(src) << 16) & 0xFFF0000))
> +#define ECCR_GROUP_SEL_SET(dst, src)                (((dst) & ~0xF000000) | (((UINT32)(src) << 24) & 0xF000000))
> +#define ECCR_EVENT_SEL_SET(dst, src)                (((dst) & ~0xFF0000) | (((UINT32)(src) << 16) & 0xFF0000))
> +#define ECCR_LANE_SEL_SET(dst, src)                 (((dst) & ~0xF00) | (((UINT32)(src) << 8) & 0xF00))
> +#define ECCR_EVENT_COUNTER_ENABLE_SET(dst, src)     (((dst) & ~0x1C) | (((UINT32)(src) << 2) & 0x1C))
> +#define ECCR_EVENT_COUNTER_CLEAR_SET(dst, src)      (((dst) & ~0x3) | (((UINT32)(src)) & 0x3))
> +
> +// PFA Registers offests
> +#define RAS_DES_CAP_ID                              0xB
> +#define EVENT_COUNTER_CONTROL_REG_OFF               0x8
> +#define EVENT_COUNTER_DATA_REG_OFF                  0xC
> +
> +#define MAX_NUM_ERROR_CODE                          47
> +#define DEFAULT_COMMON_LANE_NUM                     15
> +
> +enum LTSSM_STATE {

See enum style rules mentioned for a previous file.

> +  S_DETECT_QUIET = 0,
> +  S_DETECT_ACT,
> +  S_POLL_ACTIVE,
> +  S_POLL_COMPLIANCE,
> +  S_POLL_CONFIG,
> +  S_PRE_DETECT_QUIET,
> +  S_DETECT_WAIT,
> +  S_CFG_LINKWD_START,
> +  S_CFG_LINKWD_ACEPT,
> +  S_CFG_LANENUM_WAI,
> +  S_CFG_LANENUM_ACEPT,
> +  S_CFG_COMPLETE,
> +  S_CFG_IDLE,
> +  S_RCVRY_LOCK,
> +  S_RCVRY_SPEED,
> +  S_RCVRY_RCVRCFG,
> +  S_RCVRY_IDLE,
> +  S_L0,
> +  S_L0S,
> +  S_L123_SEND_EIDLE,
> +  S_L1_IDLE,
> +  S_L2_IDLE,
> +  S_L2_WAKE,
> +  S_DISABLED_ENTRY,
> +  S_DISABLED_IDLE,
> +  S_DISABLED,
> +  S_LPBK_ENTRY,
> +  S_LPBK_ACTIVE,
> +  S_LPBK_EXIT,
> +  S_LPBK_EXIT_TIMEOUT,
> +  S_HOT_RESET_ENTRY,
> +  S_HOT_RESET,
> +  S_RCVRY_EQ0,
> +  S_RCVRY_EQ1,
> +  S_RCVRY_EQ2,
> +  S_RCVRY_EQ3,
> +  MAX_LTSSM_STATE
> +};
> +
> +INT32
> +Ac01PcieCfgOut32 (
> +  VOID   *Addr,
> +  UINT32 Val
> +  );
> +
> +INT32
> +Ac01PcieCfgOut16 (
> +  VOID   *Addr,
> +  UINT16 Val
> +  );
> +
> +INT32
> +Ac01PcieCfgOut8 (
> +  VOID  *Addr,
> +  UINT8 Val
> +  );
> +
> +INT32
> +Ac01PcieCfgIn32 (
> +  VOID   *Addr,
> +  UINT32 *Val
> +  );
> +
> +INT32
> +Ac01PcieCfgIn16 (
> +  VOID   *Addr,
> +  UINT16 *Val
> +  );
> +
> +INT32
> +Ac01PcieCfgIn8 (
> +  VOID  *Addr,
> +  UINT8 *Val
> +  );
> +
> +INT32
> +Ac01PcieCoreSetup (
> +  AC01_RC *RC
> +  );
> +
> +VOID
> +Ac01PcieCoreResetPort (
> +  AC01_RC *RC
> +  );
> +
> +VOID
> +Ac01PcieClearConfigPort (
> +  AC01_RC *RC
> +  );
> +
> +AC01_RC *
> +GetRCList (
> +  UINT8 Idx
> +  );
> +
> +VOID
> +Ac01PcieCoreBuildRCStruct (
> +  AC01_RC *RC,
> +  UINT64  RegBase,
> +  UINT64  MmioBase,
> +  UINT64  MmioSize,
> +  UINT64  Mmio32Base,
> +  UINT64  Mmio32Size
> +  );
> +
> +INT32
> +Ac01PcieCoreSetupRC (
> +  IN AC01_RC *RC,
> +  IN UINT8   ReInit,
> +  IN UINT8   ReInitPcieIndex
> +  );
> +
> +VOID
> +Ac01PcieCoreUpdateLink (
> +  IN  AC01_RC *RC,
> +  OUT BOOLEAN *IsNextRoundNeeded,
> +  OUT INT8    *FailedPciePtr,
> +  OUT INT8    *FailedPcieCount
> +  );
> +
> +VOID
> +Ac01PcieCoreEndEnumeration (
> +  AC01_RC *RC
> +  );
> +
> +INT32
> +Ac01PcieCoreQoSLinkCheckRecovery (
> +  IN AC01_RC *RC,
> +  IN INTN    PcieIndex
> +  );
> +
> +#endif /* AC01_PCIE_CORE_H_ */
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreCapCfg.h b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreCapCfg.h
> new file mode 100644
> index 000000000000..c3914673b852
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreCapCfg.h
> @@ -0,0 +1,63 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef AC01_PCIE_CAP_CFG_H_
> +#define AC01_PCIE_CAP_CFG_H_
> +
> +/* PCIe config space capabilities offset */
> +#define PM_CAP          0x40
> +#define MSI_CAP         0x50
> +#define PCIE_CAP        0x70
> +#define MSIX_CAP        0xB0
> +#define SLOT_CAP        0xC0
> +#define VPD_CAP         0xD0
> +#define SATA_CAP        0xE0
> +#define CFG_NEXT_CAP    0x40
> +#define PM_NEXT_CAP     0x70
> +#define MSI_NEXT_CAP    0x00
> +#define PCIE_NEXT_CAP   0x00
> +#define MSIX_NEXT_CAP   0x00
> +#define SLOT_NEXT_CAP   0x00
> +#define VPD_NEXT_CAP    0x00
> +#define SATA_NEXT_CAP   0x00
> +#define BASE_CAP        0x100
> +#define AER_CAP         0x100
> +#define VC_CAP          0x148
> +#define SN_CAP          0x178
> +#define PB_CAP          0x178
> +#define ARI_CAP         0x178
> +#define SPCIE_CAP_x8    0x148
> +#define SPCIE_CAP       0x178
> +#define PL16G_CAP       0x1A8
> +#define MARGIN_CAP      0x1D8
> +#define PL32G_CAP       0x220
> +#define SRIOV_CAP       0x220
> +#define TPH_CAP         0x220
> +#define ATS_CAP         0x220
> +#define ACS_CAP         0x230
> +#define PRS_CAP         0x238
> +#define LTR_CAP         0x248
> +#define L1SUB_CAP       0x248
> +#define PASID_CAP       0x248
> +#define DPA_CAP         0x248
> +#define DPC_CAP         0x248
> +#define MPCIE_CAP       0x248
> +#define FRSQ_CAP        0x248
> +#define RTR_CAP         0x248
> +#define LN_CAP          0x248
> +#define RAS_DES_CAP     0x248
> +#define VSECRAS_CAP     0x348
> +#define DLINK_CAP       0x380
> +#define PTM_CAP         0x38C
> +#define PTM_VSEC_CAP    0x38C
> +#define CCIX_TP_CAP     0x38C
> +#define CXS_CAP         0x3D0
> +#define RBAR_CAP        0x3E8
> +#define VF_RBAR_CAP     0x3E8
> +
> +#endif /* AC01_PCIE_CAP_CFG_H_ */
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.h b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.h
> new file mode 100644
> index 000000000000..2dfefdf97159
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.h
> @@ -0,0 +1,30 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef AC01_PCIE_PATCH_ACPI_H_
> +#define AC01_PCIE_PATCH_ACPI_H_
> +
> +EFI_STATUS
> +EFIAPI
> +AcpiPatchPciMem32 (
> +  INT8 *PciSegEnabled
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +AcpiInstallMcfg (
> +  INT8 *PciSegEnabled
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +AcpiInstallIort (
> +  INT8 *PciSegEnabled
> +  );
> +
> +#endif /* AC01_PCIE_PATCH_ACPI_H_ */
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> new file mode 100644
> index 000000000000..684dd808a96a
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> @@ -0,0 +1,1659 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Guid/PlatformInfoHobGuid.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/BoardPcieLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PciePhyLib.h>
> +#include <Library/SystemFirmwareInterfaceLib.h>
> +#include <PlatformInfoHob.h>
> +
> +#include "PcieCore.h"
> +
> +STATIC INT32
> +Ac01PcieCsrOut32 (
> +  VOID   *Addr,
> +  UINT32 Val
> +  )
> +{
> +  MmioWrite32 ((UINT64)Addr, Val);
> +  DEBUG_PCIE_CSR (
> +    "PCIE CSR WR: 0x%p value: 0x%08X (0x%08X)\n",
> +    Addr,
> +    Val,
> +    MmioRead32 ((UINT64)Addr)
> +    );
> +
> +  return 0;
> +}
> +
> +STATIC INT32
> +Ac01PcieCsrOut32Serdes (
> +  VOID   *Addr,
> +  UINT32 Val
> +  )
> +{
> +  MmioWrite32 ((UINT64)Addr, Val);
> +  DEBUG_PCIE_CSR (
> +    "PCIE CSR WR: 0x%p value: 0x%08X (0x%08X)\n",
> +    Addr,
> +    Val,
> +    MmioRead32 ((UINT64)Addr)
> +    );
> +
> +  return 0;
> +}
> +
> +STATIC INT32
> +Ac01PcieCsrIn32 (
> +  VOID   *Addr,
> +  UINT32 *Val
> +  )
> +{
> +  *Val = MmioRead32 ((UINT64)Addr);
> +  DEBUG_PCIE_CSR ("PCIE CSR RD: 0x%p value: 0x%08X\n", Addr, *Val);
> +
> +  return 0;
> +}
> +
> +STATIC INT32
> +Ac01PcieCsrIn32Serdes (
> +  VOID   *Addr,
> +  UINT32 *Val
> +  )
> +{
> +  *Val = MmioRead32 ((UINT64)Addr);
> +  DEBUG_PCIE_CSR ("PCIE CSR RD: 0x%p value: 0x%08X\n", Addr, *Val);
> +
> +  return 0;
> +}
> +
> +VOID
> +Ac01PcieMmioRd (
> +  UINT64 Addr,
> +  UINT32 *Val
> +  )
> +{
> +  Ac01PcieCsrIn32Serdes ((VOID *)Addr, (UINT32 *)Val);
> +}
> +
> +VOID
> +Ac01PcieMmioWr (
> +  UINT64 Addr,
> +  UINT32 Val
> +  )
> +{
> +  Ac01PcieCsrOut32Serdes ((VOID *)Addr, (UINT32)Val);
> +}
> +
> +VOID
> +Ac01PciePuts (

Wait, what. We have *two* sets of output overlays in this patch?

> +  CONST CHAR8 *Msg
> +  )
> +{
> +  DEBUG_PCIE_PHY ("%a\n", __FUNCTION__);

Err, no, just a further level of abstraction, seemingly throwing away
the subsystem aspect when actually called here?

> +}
> +
> +VOID
> +Ac01PciePutInt (
> +  UINT32 val
> +  )
> +{
> +  DEBUG_PCIE_PHY ("%a\n", __FUNCTION__);
> +}
> +
> +VOID
> +Ac01PciePutHex (
> +  UINT64 val
> +  )
> +{
> +  DEBUG_PCIE_PHY ("%a\n", __FUNCTION__);
> +}
> +
> +INT32
> +Ac01PcieDebugPrint (
> +  CONST CHAR8 *fmt,
> +  ...
> +  )
> +{
> +  DEBUG_PCIE_PHY ("%a\n", __FUNCTION__);
> +  return 0;
> +}
> +
> +VOID
> +Ac01PcieDelay (
> +  UINT32 Val
> +  )
> +{
> +  MicroSecondDelay (Val);

No, use MicroSecondDelay directly.

> +}
> +
> +/**
> +   Write 32-bit value to config space address
> +
> +   @param Addr          Address within config space
> +   @param Val           32-bit value to write
> +**/
> +INT32
> +Ac01PcieCfgOut32 (
> +  IN VOID   *Addr,
> +  IN UINT32 Val
> +  )
> +{
> +  MmioWrite32 ((UINT64)Addr, Val);
> +  DEBUG_PCIE_CFG (
> +    "PCIE CFG WR: 0x%p value: 0x%08X (0x%08X)\n",
> +    Addr,
> +    Val,
> +    MmioRead32 ((UINT64)Addr)
> +    );
> +
> +  return 0;
> +}
> +
> +/**
> +   Write 16-bit value to config space address
> +
> +   @param Addr          Address within config space
> +   @param Val           16-bit value to write
> +**/
> +INT32
> +Ac01PcieCfgOut16 (
> +  IN VOID   *Addr,
> +  IN UINT16 Val
> +  )
> +{
> +  UINT64 AlignedAddr = (UINT64)Addr & ~0x3;
> +  UINT32 Val32  = MmioRead32 (AlignedAddr);
> +
> +  switch ((UINT64)Addr & 0x3) {
> +  case 2:
> +    Val32 &= ~0xFFFF0000;
> +    Val32 |= (UINT32)Val << 16;
> +    break;
> +
> +  case 0:
> +  default:
> +    Val32 &= ~0xFFFF;
> +    Val32 |= Val;
> +    break;
> +  }
> +  MmioWrite32 (AlignedAddr, Val32);
> +  DEBUG_PCIE_CFG (
> +    "PCIE CFG WR16: 0x%p value: 0x%04X (0x%08llX 0x%08X)\n",
> +    Addr,
> +    Val,
> +    AlignedAddr,
> +    MmioRead32 ((UINT64)AlignedAddr)
> +    );
> +
> +  return 0;
> +}
> +
> +/**
> +   Write 8-bit value to config space address
> +
> +   @param Addr          Address within config space
> +   @param Val           8-bit value to write
> +**/
> +INT32
> +Ac01PcieCfgOut8 (
> +  IN VOID  *Addr,
> +  IN UINT8 Val
> +  )
> +{
> +  UINT64 AlignedAddr = (UINT64)Addr & ~0x3;
> +  UINT32 Val32  = MmioRead32 (AlignedAddr);
> +
> +  switch ((UINT64)Addr & 0x3) {
> +  case 0:
> +    Val32 &= ~0xFF;
> +    Val32 |= Val;
> +    break;
> +
> +  case 1:
> +    Val32 &= ~0xFF00;
> +    Val32 |= (UINT32)Val << 8;
> +    break;
> +
> +  case 2:
> +    Val32 &= ~0xFF0000;
> +    Val32 |= (UINT32)Val << 16;
> +    break;
> +
> +  case 3:
> +  default:
> +    Val32 &= ~0xFF000000;
> +    Val32 |= (UINT32)Val << 24;
> +    break;
> +  }
> +  MmioWrite32 (AlignedAddr, Val32);
> +  DEBUG_PCIE_CFG (
> +    "PCIE CFG WR8: 0x%p value: 0x%04X (0x%08llX 0x%08X)\n",
> +    Addr,
> +    Val,
> +    AlignedAddr,
> +    MmioRead32 ((UINT64)AlignedAddr)
> +    );
> +
> +  return 0;
> +}
> +
> +/**
> +   Read 32-bit value from config space address
> +
> +   @param Addr          Address within config space
> +   @param Val           Point to address for read value
> +**/
> +INT32
> +Ac01PcieCfgIn32 (
> +  IN  VOID   *Addr,
> +  OUT UINT32 *Val
> +  )
> +{
> +  UINT32 RegC, Reg18;
> +  UINT8  MfHt, Ht, Primary = 0, Sec = 0, Sub = 0;
> +
> +  if ((BUS_NUM (Addr) > 0) && (DEV_NUM (Addr) > 0) && (CFG_REG (Addr) == 0)) {
> +    *Val = MmioRead32 ((UINT64)Addr);
> +    DEBUG_PCIE_CFG (
> +      "PCIE CFG RD: B%X|D%X 0x%p value: 0x%08X\n",
> +      BUS_NUM (Addr),
> +      DEV_NUM (Addr),
> +      Addr,
> +      *Val
> +      );
> +
> +    if (*Val != 0xffffffff) {
> +      RegC = MmioRead32 ((UINT64)Addr + 0xC);
> +      DEBUG_PCIE_CFG ("Peek PCIE MfHt RD32: 0x%p value: 0x%08X\n", Addr + 0xc, RegC);
> +      MfHt = RegC >> 16;
> +      DEBUG_PCIE_CFG ("  Peek RD8 MfHt=0x%02X\n", MfHt);
> +
> +      Ht = MfHt & 0x7F;
> +      if (Ht != 0) { /* Type 1 header */
> +        Reg18 = MmioRead32 ((UINT64)Addr + 0x18);
> +        Primary = Reg18; Sec = Reg18 >> 8; Sub = Reg18 >> 16;
> +        DEBUG_PCIE_CFG (
> +          "  Bus Peek PCIE Sub:%01X Sec:%01X Primary:%01X  RD: 0x%p value: 0x%08X\n",
> +          Sub,
> +          Sec,
> +          Primary,
> +          Addr + 0x18,
> +          Reg18
> +          );
> +      }
> +      if ((Ht == 0) || (Primary != 0)) { /* Ampere Altra RPs Primary Bus is 0b */
> +        *Val = 0xffffffff;
> +        DEBUG_PCIE_CFG (
> +          "  Skip RD32 B%X|D%X PCIE CFG RD: 0x%p return 0xffffffff\n",
> +          BUS_NUM (Addr),
> +          DEV_NUM (Addr),
> +          Addr
> +          );
> +      }
> +    }
> +  } else {
> +    *Val = MmioRead32 ((UINT64)Addr);
> +  }
> +  DEBUG_PCIE_CFG ("PCIE CFG RD: 0x%p value: 0x%08X\n", Addr, *Val);
> +
> +  return 0;
> +}
> +
> +/**
> +   Read 16-bit value from config space address
> +
> +   @param Addr          Address within config space
> +   @param Val           Point to address for read value
> +**/
> +INT32
> +Ac01PcieCfgIn16 (
> +  IN  VOID   *Addr,
> +  OUT UINT16 *Val
> +  )
> +{
> +  UINT64 AlignedAddr = (UINT64)Addr & ~0x3;
> +  UINT32 RegC, Reg18;
> +  UINT8  MfHt, Primary = 0, Sec = 0, Sub = 0;
> +  UINT32 Val32;
> +
> +  if ((BUS_NUM (Addr) > 0) && (DEV_NUM (Addr) > 0) && (CFG_REG (Addr) == 0)) {
> +    *Val = MmioRead32 ((UINT64)Addr);
> +    DEBUG_PCIE_CFG (
> +      "PCIE CFG16 RD: B%X|D%X 0x%p value: 0x%08X\n",
> +      BUS_NUM (Addr),
> +      DEV_NUM (Addr),
> +      Addr,
> +      *Val
> +      );
> +
> +    if (*Val != 0xffff) {
> +      RegC = MmioRead32 ((UINT64)Addr + 0xC);
> +      DEBUG_PCIE_CFG ("  Peek PCIE MfHt RD: 0x%p value: 0x%08X\n", Addr + 0xc, RegC);
> +      MfHt = RegC >> 16;
> +      DEBUG_PCIE_CFG ("  Peek RD8 MfHt=0x%02X\n", MfHt);
> +
> +
> +      if ((MfHt & 0x7F)!= 0) { /* Type 1 header */
> +        Reg18 = MmioRead32 ((UINT64)Addr + 0x18);
> +        Primary = Reg18; Sec = Reg18 >> 8; Sub = Reg18 >> 16;
> +        DEBUG_PCIE_CFG (
> +          "  Bus Peek PCIE Sub:%01X Sec:%01X Primary:%01X  RD: 0x%p value: 0x%08X\n",
> +          Sub,
> +          Sec,
> +          Primary,
> +          Addr + 0x18,
> +          Reg18
> +          );
> +      }
> +      if ((MfHt == 0) || (Primary != 0)) { /* QS RPs Primary Bus is 0b */
> +        *Val = 0xffff;
> +        DEBUG_PCIE_CFG (
> +          "  Skip RD16 B%X|D%X PCIE CFG RD: 0x%p return 0xffff\n",
> +          BUS_NUM (Addr),
> +          DEV_NUM (Addr),
> +          Addr
> +          );
> +        return 0;
> +      }
> +    }
> +  }
> +
> +  Val32 = MmioRead32 (AlignedAddr);
> +  switch ((UINT64)Addr & 0x3) {
> +  case 2:
> +    *Val = Val32 >> 16;
> +    break;
> +
> +  case 0:
> +  default:
> +    *Val = Val32;
> +    break;
> +  }
> +  DEBUG_PCIE_CFG (
> +    "PCIE CFG RD16: 0x%p value: 0x%04X (0x%08llX 0x%08X)\n",
> +    Addr,
> +    *Val,
> +    AlignedAddr,
> +    Val32
> +    );
> +
> +  return 0;
> +}
> +
> +/**
> +   Read 8-bit value from config space address
> +
> +   @param Addr          Address within config space
> +   @param Val           Point to address for read value
> +**/
> +INT32
> +Ac01PcieCfgIn8 (
> +  IN  VOID  *Addr,
> +  OUT UINT8 *Val
> +  )
> +{
> +  UINT64 AlignedAddr = (UINT64)Addr & ~0x3;
> +  UINT32 Val32 = MmioRead32 (AlignedAddr);
> +  switch ((UINT64)Addr & 0x3) {
> +  case 3:
> +    *Val = Val32 >> 24;
> +    break;
> +
> +  case 2:
> +    *Val = Val32 >> 16;
> +    break;
> +
> +  case 1:
> +    *Val = Val32 >> 8;
> +    break;
> +
> +  case 0:
> +  default:
> +    *Val = Val32;
> +    break;
> +  }
> +  DEBUG_PCIE_CFG (
> +    "PCIE CFG RD8: 0x%p value: 0x%02X (0x%08llX 0x%08X)\n",
> +    Addr,
> +    *Val,
> +    AlignedAddr,
> +    Val32
> +    );
> +
> +  return 0;
> +}
> +
> +/**
> +   Return the next extended capability address
> +
> +   @param RC              Pointer to AC01_RC structure
> +   @param PcieIndex       PCIe index
> +   @param IsRC            0x1: Checking RC configuration space
> +                          0x0: Checking EP configuration space
> +   @param ExtendedCapId
> +**/
> +UINT64
> +PcieCheckCap (
> +  IN AC01_RC *RC,
> +  IN INTN    PcieIndex,
> +  IN BOOLEAN IsRC,
> +  IN UINT16  ExtendedCapId
> +  )
> +{
> +  VOID   *CfgAddr;
> +  UINT32 Val = 0, NextCap = 0, CapId = 0, ExCap = 0;
> +
> +  if (IsRC) {
> +    CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +  } else {
> +    CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 20));
> +  }
> +
> +  Ac01PcieCsrIn32 (CfgAddr + TYPE1_CAP_PTR_REG, &Val);
> +  NextCap = Val & 0xFF;
> +
> +  // Loop untill desired capability is found else return 0
> +  while (1) {
> +    if ((NextCap & 0x3) != 0) {
> +      /* Not alignment, just return */
> +      return 0;
> +    }
> +    Ac01PcieCsrIn32 (CfgAddr + NextCap, &Val);
> +    if (NextCap < EXT_CAP_OFFSET_START) {
> +      CapId = Val & 0xFF;
> +    } else {
> +      CapId = Val & 0xFFFF;
> +    }
> +
> +    if (CapId == ExtendedCapId) {
> +      return (UINT64)(CfgAddr + NextCap);
> +    }
> +
> +    if (NextCap < EXT_CAP_OFFSET_START) {
> +      NextCap = (Val & 0xFFFF) >> 8;
> +    } else {
> +      NextCap = (Val & 0xFFFF0000) >> 20;
> +    }
> +
> +    if ((NextCap == 0) && (ExCap == 0)) {
> +      ExCap = 1;
> +      NextCap = EXT_CAP_OFFSET_START;
> +    }
> +
> +    if ((NextCap == 0) && (ExCap == 1)) {
> +      return (UINT64)0;
> +    }
> +  }
> +}
> +
> +/**
> +   Read 8-bit value from config space address
> +
> +   @param RC          Pointer to AC01_RC strucutre
> +   @param RegBase     Base address of CSR, TCU, Hostbridge, Msg, Serdes, and MMCFG register
> +   @param MmioBase    Base address of 64-bit MMIO
> +   @param MmioSize    Size of 64-bit MMIO space
> +   @param Mmio32Base  Base address of 32-bit MMIO
> +   @param Mmio32Size  Size of 32-bit MMIO space
> +**/
> +VOID
> +Ac01PcieCoreBuildRCStruct (
> +  AC01_RC *RC,

RootComplex.

> +  UINT64  RegBase,
> +  UINT64  MmioBase,
> +  UINT64  MmioSize,
> +  UINT64  Mmio32Base,
> +  UINT64  Mmio32Size
> +  )
> +{
> +  INTN PcieIndex;
> +
> +  RC->BaseAddr = RegBase;
> +  RC->TcuAddr = RegBase + TCU_OFFSET;
> +  RC->HBAddr = RegBase + HB_CSR_OFFSET;
> +  RC->SerdesAddr = RegBase + SERDES_CSR_OFFSET;
> +  RC->MmcfgAddr = RegBase + MMCONFIG_OFFSET;
> +  RC->MmioAddr = MmioBase;
> +  RC->MmioSize = MmioSize;
> +  RC->Mmio32Addr = Mmio32Base;
> +  RC->Mmio32Size = Mmio32Size;
> +  RC->IoAddr = Mmio32Base + Mmio32Size - IO_SPACE;
> +
> +  RC->Type = (RC->ID < MAX_RCA) ? RCA : RCB;
> +  RC->MaxPcieController = (RC->Type == RCB) ? MAX_PCIE_B : MAX_PCIE_A;
> +
> +  BoardPcieParseRCParams (RC);
> +
> +  for (PcieIndex = 0; PcieIndex < RC->MaxPcieController; PcieIndex++) {
> +    RC->Pcie[PcieIndex].ID = PcieIndex;
> +    RC->Pcie[PcieIndex].CsrAddr = RC->BaseAddr + PCIE0_CSR_OFFSET + PcieIndex*0x10000;
> +    RC->Pcie[PcieIndex].SnpsRamAddr = RC->Pcie[PcieIndex].CsrAddr + SNPSRAM_OFFSET;
> +    RC->Pcie[PcieIndex].DevNum = PcieIndex + 1;
> +  }
> +
> +  DEBUG_PCIE_INFO (
> +    " + S%d - RC%a%d, MMCfgAddr:0x%lx, MmioAddr:0x%lx, MmioSize:0x%lx, Mmio32Addr:0x%lx, Mmio32Size:0x%lx, Enabled:%a\n",
> +    RC->Socket,
> +    (RC->Type == RCA) ? "A" : "B",
> +    RC->ID,
> +    RC->MmcfgAddr,
> +    RC->MmioAddr,
> +    RC->MmioSize,
> +    RC->Mmio32Addr,
> +    RC->Mmio32Size,
> +    (RC->Active) ? "Y" : "N"
> +    );
> +  DEBUG_PCIE_INFO (" +   DevMapLow/High: 0x%x/0x%x\n", RC->DevMapLow, RC->DevMapHigh);
> +  for (PcieIndex = 0; PcieIndex < RC->MaxPcieController; PcieIndex++) {
> +    DEBUG_PCIE_INFO (
> +      " +     PCIE%d:0x%lx - Enabled:%a - DevNum:0x%x\n",
> +      PcieIndex,
> +      RC->Pcie[PcieIndex].CsrAddr,
> +      (RC->Pcie[PcieIndex].Active) ? "Y" : "N",
> +      RC->Pcie[PcieIndex].DevNum
> +      );
> +  }
> +}
> +
> +/**
> +   Configure equalization settings
> +
> +   @param RC              Pointer to AC01_RC structure
> +   @param PcieIndex       PCIe index
> +**/
> +STATIC
> +VOID
> +Ac01PcieConfigureEqualization (
> +  IN AC01_RC *RC,

RootComplex.

> +  IN INTN    PcieIndex
> +  )
> +{
> +  VOID   *CfgAddr;
> +  UINT32 Val;
> +
> +  CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +  // Select the FoM method, need double-write to convey settings
> +  Ac01PcieCfgIn32 (CfgAddr + GEN3_EQ_CONTROL_OFF, &Val);
> +  Val = GEN3_EQ_FB_MODE (Val, 0x1);
> +  Val = GEN3_EQ_PRESET_VEC (Val, 0x3FF);
> +  Val = GEN3_EQ_INIT_EVAL (Val, 0x1);
> +  Ac01PcieCfgOut32 (CfgAddr + GEN3_EQ_CONTROL_OFF, Val);
> +  Ac01PcieCfgOut32 (CfgAddr + GEN3_EQ_CONTROL_OFF, Val);
> +  Ac01PcieCfgIn32 (CfgAddr + GEN3_EQ_CONTROL_OFF, &Val);
> +}
> +
> +/**
> +   Configure presets for GEN3 equalization
> +
> +   @param RC              Pointer to AC01_RC structure
> +   @param PcieIndex       PCIe index
> +**/
> +STATIC
> +VOID
> +Ac01PcieConfigurePresetGen3 (
> +  IN AC01_RC *RC,
> +  IN INTN    PcieIndex
> +  )
> +{
> +  VOID   *CfgAddr, *SpcieBaseAddr;
> +  UINT32 Val, Idx;
> +  CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +  // Bring to legacy mode
> +  Ac01PcieCfgIn32 (CfgAddr + GEN3_RELATED_OFF, &Val);
> +  Val = RATE_SHADOW_SEL_SET (Val, 0);
> +  Ac01PcieCfgOut32 (CfgAddr + GEN3_RELATED_OFF, Val);
> +  Val = EQ_PHASE_2_3_SET (Val, 0);
> +  Val = RXEQ_REGRDLESS_SET (Val, 1);
> +  Ac01PcieCfgOut32 (CfgAddr + GEN3_RELATED_OFF, Val);
> +
> +  // Generate SPCIE capability address
> +  SpcieBaseAddr = (VOID *)PcieCheckCap (RC, PcieIndex, 0x1, SPCIE_CAP_ID);
> +  if (SpcieBaseAddr == 0) {
> +    DEBUG_PCIE_ERR (
> +      "PCIE%d.%d: Cannot get SPCIE capability address\n",
> +      RC->ID,
> +      PcieIndex
> +      );
> +    return;
> +  }
> +
> +  for (Idx=0; Idx < RC->Pcie[PcieIndex].MaxWidth/2; Idx++) {
> +    // Program Preset to Gen3 EQ Lane Control
> +    Ac01PcieCfgIn32 (SpcieBaseAddr + CAP_OFF_0C + Idx*4, &Val);
> +    Val = DSP_TX_PRESET0_SET (Val, 0x7);
> +    Val = DSP_TX_PRESET1_SET (Val, 0x7);
> +    Ac01PcieCfgOut32 (SpcieBaseAddr + CAP_OFF_0C + Idx*4, Val);
> +  }
> +}
> +
> +/**
> +   Configure presets for GEN4 equalization
> +
> +   @param RC              Pointer to AC01_RC structure
> +   @param PcieIndex       PCIe index
> +**/
> +STATIC
> +VOID
> +Ac01PcieConfigurePresetGen4 (
> +  IN AC01_RC *RC,
> +  IN INTN    PcieIndex
> +  )
> +{
> +  UINT32 Val;
> +  VOID   *CfgAddr, *SpcieBaseAddr, *Pl16BaseAddr;
> +  UINT32 LinkWidth, i;
> +  UINT8  Preset;
> +
> +  CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +  // Bring to legacy mode
> +  Ac01PcieCfgIn32 (CfgAddr + GEN3_RELATED_OFF, &Val);
> +  Val = RATE_SHADOW_SEL_SET (Val, 1);
> +  Ac01PcieCfgOut32 (CfgAddr + GEN3_RELATED_OFF, Val);
> +  Val = EQ_PHASE_2_3_SET (Val, 0);
> +  Val = RXEQ_REGRDLESS_SET (Val, 1);
> +  Ac01PcieCfgOut32 (CfgAddr + GEN3_RELATED_OFF, Val);
> +
> +  // Generate the PL16 capability address
> +  Pl16BaseAddr = (VOID *)PcieCheckCap (RC, PcieIndex, 0x1, PL16_CAP_ID);
> +  if (Pl16BaseAddr == 0) {
> +    DEBUG_PCIE_ERR (
> +      "PCIE%d.%d: Cannot get PL16 capability address\n",
> +      RC->ID,
> +      PcieIndex
> +      );
> +    return;
> +  }
> +
> +  // Generate the SPCIE capability address
> +  SpcieBaseAddr = (VOID *)PcieCheckCap (RC, PcieIndex, 0x1, SPCIE_CAP_ID);
> +  if (SpcieBaseAddr == 0) {
> +    DEBUG_PCIE_ERR (
> +      "PCIE%d.%d: Cannot get SPICE capability address\n",
> +      RC->ID,
> +      PcieIndex
> +      );
> +    return;
> +  }
> +
> +  // Configure downstream Gen4 Tx preset
> +  if (RC->PresetGen4[PcieIndex] == PRESET_INVALID) {
> +    Preset = 0x57; // Default Gen4 preset
> +  } else {
> +    Preset = RC->PresetGen4[PcieIndex];
> +  }
> +
> +  LinkWidth = RC->Pcie[PcieIndex].MaxWidth;
> +  if (LinkWidth == 0x2) {
> +    Ac01PcieCfgIn32 (Pl16BaseAddr + PL16G_CAP_OFF_20H_REG_OFF, &Val);
> +    Val = DSP_16G_RXTX_PRESET0_SET (Val, Preset);
> +    Val = DSP_16G_RXTX_PRESET1_SET (Val, Preset);
> +    Ac01PcieCfgOut32 (Pl16BaseAddr + PL16G_CAP_OFF_20H_REG_OFF, Val);
> +  } else {
> +    for (i = 0; i < LinkWidth/4; i++) {
> +      Ac01PcieCfgIn32 (Pl16BaseAddr + PL16G_CAP_OFF_20H_REG_OFF + i*4, &Val);
> +      Val = DSP_16G_RXTX_PRESET0_SET (Val, Preset);
> +      Val = DSP_16G_RXTX_PRESET1_SET (Val, Preset);
> +      Val = DSP_16G_RXTX_PRESET2_SET (Val, Preset);
> +      Val = DSP_16G_RXTX_PRESET3_SET (Val, Preset);
> +      Ac01PcieCfgOut32 (Pl16BaseAddr + PL16G_CAP_OFF_20H_REG_OFF + i*4, Val);
> +    }
> +  }
> +
> +  // Configure Gen3 preset
> +  for (i = 0; i < LinkWidth/2; i++) {
> +    Ac01PcieCfgIn32 (SpcieBaseAddr + CAP_OFF_0C + i*4, &Val);
> +    Val = DSP_TX_PRESET0_SET (Val, 0x7);
> +    Val = DSP_TX_PRESET1_SET (Val, 0x7);
> +    Ac01PcieCfgOut32 (SpcieBaseAddr + CAP_OFF_0C + i*4, Val);
> +  }
> +}
> +
> +STATIC BOOLEAN
> +RasdpMitigationCheck (
> +  AC01_RC *RC,
> +  INTN    PcieIndex
> +  )
> +{
> +  PLATFORM_INFO_HOB  *PlatformHob;
> +  VOID               *Hob;
> +
> +  Hob = GetFirstGuidHob (&gPlatformHobGuid);
> +  PlatformHob = (PLATFORM_INFO_HOB *)GET_GUID_HOB_DATA (Hob);
> +  if ((PlatformHob->ScuProductId[0] & 0xff) == 0x01) {
> +    if (AsciiStrCmp ((CONST CHAR8 *)PlatformHob->CpuVer, "A0") == 0) {
> +      return ((RC->Type == RCB)||(PcieIndex > 0)) ? TRUE : FALSE;
> +    }
> +  }
> +
> +  return FALSE;
> +}
> +
> +/**
> +   Setup and initialize the AC01 PCIe Root Complex and underneath PCIe controllers
> +

This is a lot of functionality, please break it down into smaller functions.

> +   @param RC                    Pointer to Root Complex structure
> +   @param ReInit                Re-init status
> +   @param ReInitPcieIndex       PCIe index
> +**/
> +INT32
> +Ac01PcieCoreSetupRC (
> +  IN AC01_RC *RC,

RootComplex (global search/replace).

> +  IN UINT8   ReInit,
> +  IN UINT8   ReInitPcieIndex
> +  )
> +{
> +  VOID              *CsrAddr, *CfgAddr, *SnpsRamAddr, *DlinkBaseAddr;
> +  INTN              PcieIndex;
> +  INTN              TimeOut;
> +  UINT32            Val;
> +  PHY_CONTEXT       PhyCtx = { 0 };
> +  PHY_PLAT_RESOURCE PhyPlatResource = { 0 };
> +  INTN              Ret;
> +  UINT16            NextExtendedCapabilityOff;
> +  UINT32            VsecVal;
> +
> +  DEBUG_PCIE_INFO ("Initializing Socket%d RC%d\n", RC->Socket, RC->ID);
> +
> +  if (ReInit == 0) {
> +    // Initialize SERDES
> +    ZeroMem (&PhyCtx, sizeof (PhyCtx));
> +    PhyCtx.SdsAddr = RC->SerdesAddr;
> +    PhyCtx.PcieCtrlInfo |= ((RC->Socket & 0x1) << 2);
> +    PhyCtx.PcieCtrlInfo |= ((RC->ID & 0x7) << 4);
> +    PhyCtx.PcieCtrlInfo |= 0xF << 8;
> +    PhyPlatResource.MmioRd = Ac01PcieMmioRd;
> +    PhyPlatResource.MmioWr = Ac01PcieMmioWr;
> +    PhyPlatResource.UsDelay = Ac01PcieDelay;
> +    PhyPlatResource.Puts = Ac01PciePuts;

...added into a further abstraction layer?
And never used as far as I can tell.

> +    PhyPlatResource.PutInt = Ac01PciePutInt;

Also unused.

> +    PhyPlatResource.PutHex = Ac01PciePutInt;

With an unused alias. (Which would appear to be a copy/paste bug.)

> +    PhyPlatResource.PutHex64 = Ac01PciePutHex;

Or maybe not, because Int is 32-bit but Hex is 64-bit?

I'm sorry, but this is a complete mess.
Please tear out *all* customised debug printout.

> +    PhyPlatResource.DebugPrint = Ac01PcieDebugPrint;

Including that one.

> +    PhyCtx.PhyPlatResource = &PhyPlatResource;
> +
> +    Ret = SerdesInitClkrst (&PhyCtx);
> +    if (Ret != PHY_INIT_PASS) {
> +      return -1;
> +    }
> +  }
> +
> +  // Setup each controller
> +  for (PcieIndex = 0; PcieIndex < RC->MaxPcieController; PcieIndex++) {
> +
> +    if (ReInit == 1) {
> +      PcieIndex = ReInitPcieIndex;
> +    }
> +
> +    if (!RC->Pcie[PcieIndex].Active) {
> +      continue;
> +    }
> +
> +    DEBUG_PCIE_INFO ("Initializing Controller %d\n", PcieIndex);
> +
> +    CsrAddr = (VOID *)RC->Pcie[PcieIndex].CsrAddr;
> +    SnpsRamAddr = (VOID *)RC->Pcie[PcieIndex].SnpsRamAddr;
> +    CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +    // Put Controller into reset if not in reset already
> +    Ac01PcieCsrIn32 (CsrAddr +  RESET, &Val);
> +    if (!(Val & RESET_MASK)) {
> +      Val = DWCPCIE_SET (Val, 1);
> +      Ac01PcieCsrOut32 (CsrAddr + RESET, Val);
> +
> +      // Delay 50ms to ensure controller finish its reset
> +      MicroSecondDelay (50000);
> +    }
> +
> +    // Clear memory shutdown
> +    Ac01PcieCsrIn32 (CsrAddr + RAMSDR, &Val);
> +    Val = SD_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CsrAddr + RAMSDR, Val);
> +
> +    // Poll till mem is ready
> +    TimeOut = MEMRDY_TIMEOUT;
> +    do {
> +      Ac01PcieCsrIn32 (CsrAddr + MEMRDYR, &Val);
> +      if (Val & 1) {
> +        break;
> +      }
> +
> +      TimeOut--;
> +      MicroSecondDelay (1);
> +    } while (TimeOut > 0);
> +
> +    if (TimeOut <= 0) {
> +      DEBUG_PCIE_ERR ("- Pcie[%d] - Mem not ready\n", PcieIndex);
> +      return -1;
> +    }
> +
> +    // Hold link training
> +    Ac01PcieCsrIn32 (CsrAddr + LINKCTRL, &Val);
> +    Val = LTSSMENB_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CsrAddr + LINKCTRL, Val);
> +
> +    // Enable subsystem clock and release reset
> +    Ac01PcieCsrIn32 (CsrAddr + CLOCK, &Val);
> +    Val = AXIPIPE_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CsrAddr + CLOCK, Val);
> +    Ac01PcieCsrIn32 (CsrAddr +  RESET, &Val);
> +    Val = DWCPCIE_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CsrAddr + RESET, Val);
> +
> +    //
> +    // Controller does not provide any indicator for reset released.
> +    // Must wait at least 1us as per EAS.
> +    //
> +    MicroSecondDelay (1);
> +
> +    // Poll till PIPE clock is stable
> +    TimeOut = PIPE_CLOCK_TIMEOUT;
> +    do {
> +      Ac01PcieCsrIn32 (CsrAddr + LINKSTAT, &Val);
> +      if (!(Val & PHY_STATUS_MASK)) {
> +        break;
> +      }
> +
> +      TimeOut--;
> +      MicroSecondDelay (1);
> +    } while (TimeOut > 0);
> +
> +    if (TimeOut <= 0) {
> +      DEBUG_PCIE_ERR ("- Pcie[%d] - PIPE clock is not stable\n", PcieIndex);
> +      return -1;
> +    }
> +
> +    // Start PERST pulse
> +    BoardPcieAssertPerst (RC, PcieIndex, 0, TRUE);
> +
> +    // Allow programming to config space
> +    Ac01PcieCsrIn32 (CfgAddr + MISC_CONTROL_1_OFF, &Val);
> +    Val = DBI_RO_WR_EN_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CfgAddr + MISC_CONTROL_1_OFF, Val);
> +
> +    // In order to detect the NVMe disk for booting without disk,
> +    // need to set Hot-Plug Slot Capable during port initialization.
> +    // It will help PCI Linux driver to initialize its slot iomem resource,
> +    // the one is used for detecting the disk when it is inserted.
> +    Ac01PcieCsrIn32 (CfgAddr + SLOT_CAPABILITIES_REG, &Val);
> +    Val = SLOT_HPC_SET (Val, 1);
> +    // Program the power limit
> +    Val = SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET (Val, SLOT_POWER_LIMIT);
> +    Ac01PcieCsrOut32 (CfgAddr + SLOT_CAPABILITIES_REG, Val);
> +
> +
> +    // Apply RASDP error mitigation for all x8, x4, and x2 controllers
> +    // This includes all RCB root ports, and every RCA root port controller
> +    // except for index 0 (i.e. x16 controllers are exempted from this WA)
> +    if (RasdpMitigationCheck (RC, PcieIndex)) {
> +      // Change the Read Margin dual ported RAMs
> +      Val = 0x10; // Margin to 0x0 (most conservative setting)
> +      Ac01PcieCsrOut32 (SnpsRamAddr + TPSRAM_RMR, Val);
> +
> +      // Generate the DLINK capability address
> +      DlinkBaseAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +      NextExtendedCapabilityOff = 0x100; // This is the 1st extended capability offset
> +      do {
> +        Ac01PcieCsrIn32 (DlinkBaseAddr + NextExtendedCapabilityOff, &Val);
> +        if (Val == 0xFFFFFFFF) {
> +          DlinkBaseAddr = 0x0;
> +          break;
> +        }
> +        if ((Val & 0xFFFF) == DLINK_VENDOR_CAP_ID) {
> +          Ac01PcieCsrIn32 (DlinkBaseAddr + NextExtendedCapabilityOff + 0x4, &VsecVal);
> +          if (VsecVal == DLINK_VSEC) {
> +            DlinkBaseAddr = DlinkBaseAddr + NextExtendedCapabilityOff;
> +            break;
> +          }
> +        }
> +        NextExtendedCapabilityOff = (Val >> 20);
> +      } while (NextExtendedCapabilityOff != 0);
> +
> +      // Disable the scaled credit mode
> +      if (DlinkBaseAddr != 0x0) {
> +        Val = 1;
> +        Ac01PcieCsrOut32 (DlinkBaseAddr + DATA_LINK_FEATURE_CAP_OFF, Val);
> +        Ac01PcieCsrIn32 (DlinkBaseAddr + DATA_LINK_FEATURE_CAP_OFF, &Val);
> +        if (Val != 1) {
> +          DEBUG_PCIE_ERR ("- Pcie[%d] - Unable to disable scaled credit\n", PcieIndex);
> +          return -1;
> +        }
> +      } else {
> +        DEBUG_PCIE_ERR ("- Pcie[%d] - Unable to locate data link feature cap offset\n", PcieIndex);
> +        return -1;
> +      }
> +
> +      // Reduce Posted Credits to 1 packet header and data credit for all
> +      // impacted controllers.  Also zero credit scale values for both
> +      // data and packet headers.
> +      Val=0x40201020;
> +      Ac01PcieCsrOut32 (CfgAddr + PORT_LOCIG_VC0_P_RX_Q_CTRL_OFF, Val);
> +    }
> +
> +    // Program DTI for ATS support
> +    Ac01PcieCsrIn32 (CfgAddr + DTIM_CTRL0_OFF, &Val);
> +    Val = DTIM_CTRL0_ROOT_PORT_ID_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CfgAddr + DTIM_CTRL0_OFF, Val);
> +
> +    //
> +    // Program number of lanes used
> +    // - Reprogram LINK_CAPABLE of PORT_LINK_CTRL_OFF
> +    // - Reprogram NUM_OF_LANES of GEN2_CTRL_OFF
> +    // - Reprogram CAP_MAX_LINK_WIDTH of LINK_CAPABILITIES_REG
> +    //
> +    Ac01PcieCsrIn32 (CfgAddr + PORT_LINK_CTRL_OFF, &Val);
> +    switch (RC->Pcie[PcieIndex].MaxWidth) {
> +    case LNKW_X2:
> +      Val = LINK_CAPABLE_SET (Val, LINK_CAPABLE_X2);
> +      break;
> +
> +    case LNKW_X4:
> +      Val = LINK_CAPABLE_SET (Val, LINK_CAPABLE_X4);
> +      break;
> +
> +    case LNKW_X8:
> +      Val = LINK_CAPABLE_SET (Val, LINK_CAPABLE_X8);
> +      break;
> +
> +    case LNKW_X16:
> +    default:
> +      Val = LINK_CAPABLE_SET (Val, LINK_CAPABLE_X16);
> +      break;
> +    }
> +    Ac01PcieCsrOut32 (CfgAddr + PORT_LINK_CTRL_OFF, Val);
> +    Ac01PcieCsrIn32 (CfgAddr + GEN2_CTRL_OFF, &Val);
> +    switch (RC->Pcie[PcieIndex].MaxWidth) {
> +    case LNKW_X2:
> +      Val = NUM_OF_LANES_SET (Val, NUM_OF_LANES_X2);
> +      break;
> +
> +    case LNKW_X4:
> +      Val = NUM_OF_LANES_SET (Val, NUM_OF_LANES_X4);
> +      break;
> +
> +    case LNKW_X8:
> +      Val = NUM_OF_LANES_SET (Val, NUM_OF_LANES_X8);
> +      break;
> +
> +    case LNKW_X16:
> +    default:
> +      Val = NUM_OF_LANES_SET (Val, NUM_OF_LANES_X16);
> +      break;
> +    }
> +    Ac01PcieCsrOut32 (CfgAddr + GEN2_CTRL_OFF, Val);
> +    Ac01PcieCsrIn32 (CfgAddr + LINK_CAPABILITIES_REG, &Val);
> +    switch (RC->Pcie[PcieIndex].MaxWidth) {
> +    case LNKW_X2:
> +      Val = CAP_MAX_LINK_WIDTH_SET (Val, CAP_MAX_LINK_WIDTH_X2);
> +      break;
> +
> +    case LNKW_X4:
> +      Val = CAP_MAX_LINK_WIDTH_SET (Val, CAP_MAX_LINK_WIDTH_X4);
> +      break;
> +
> +    case LNKW_X8:
> +      Val = CAP_MAX_LINK_WIDTH_SET (Val, CAP_MAX_LINK_WIDTH_X8);
> +      break;
> +
> +    case LNKW_X16:
> +    default:
> +      Val = CAP_MAX_LINK_WIDTH_SET (Val, CAP_MAX_LINK_WIDTH_X16);
> +      break;
> +    }
> +
> +    switch (RC->Pcie[PcieIndex].MaxGen) {
> +    case SPEED_GEN1:
> +      Val = CAP_MAX_LINK_SPEED_SET (Val, MAX_LINK_SPEED_25);
> +      break;
> +
> +    case SPEED_GEN2:
> +      Val = CAP_MAX_LINK_SPEED_SET (Val, MAX_LINK_SPEED_50);
> +      break;
> +
> +    case SPEED_GEN3:
> +      Val = CAP_MAX_LINK_SPEED_SET (Val, MAX_LINK_SPEED_80);
> +      break;
> +
> +    default:
> +      Val = CAP_MAX_LINK_SPEED_SET (Val, MAX_LINK_SPEED_160);
> +      break;
> +    }
> +    /* Enable ASPM Capability */
> +    Val = CAP_ACTIVE_STATE_LINK_PM_SUPPORT_SET (Val, L0S_L1_SUPPORTED);
> +    Ac01PcieCsrOut32 (CfgAddr + LINK_CAPABILITIES_REG, Val);
> +
> +    Ac01PcieCsrIn32 (CfgAddr + LINK_CONTROL2_LINK_STATUS2_REG, &Val);
> +    switch (RC->Pcie[PcieIndex].MaxGen) {
> +    case SPEED_GEN1:
> +      Val = CAP_TARGET_LINK_SPEED_SET (Val, MAX_LINK_SPEED_25);
> +      break;
> +
> +    case SPEED_GEN2:
> +      Val = CAP_TARGET_LINK_SPEED_SET (Val, MAX_LINK_SPEED_50);
> +      break;
> +
> +    case SPEED_GEN3:
> +      Val = CAP_TARGET_LINK_SPEED_SET (Val, MAX_LINK_SPEED_80);
> +      break;
> +
> +    default:
> +      Val = CAP_TARGET_LINK_SPEED_SET (Val, MAX_LINK_SPEED_160);
> +      break;
> +    }
> +    Ac01PcieCsrOut32 (CfgAddr + LINK_CONTROL2_LINK_STATUS2_REG, Val);
> +
> +    // Set Zero byte request handling
> +    Ac01PcieCsrIn32 (CfgAddr + FILTER_MASK_2_OFF, &Val);
> +    Val = CX_FLT_MASK_VENMSG0_DROP_SET (Val, 0);
> +    Val = CX_FLT_MASK_VENMSG1_DROP_SET (Val, 0);
> +    Val = CX_FLT_MASK_DABORT_4UCPL_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CfgAddr + FILTER_MASK_2_OFF, Val);
> +    Ac01PcieCsrIn32 (CfgAddr + AMBA_ORDERING_CTRL_OFF, &Val);
> +    Val = AX_MSTR_ZEROLREAD_FW_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CfgAddr + AMBA_ORDERING_CTRL_OFF, Val);
> +
> +    //
> +    // Set Completion with CRS handling for CFG Request
> +    // Set Completion with CA/UR handling non-CFG Request
> +    //
> +    Ac01PcieCsrIn32 (CfgAddr + AMBA_ERROR_RESPONSE_DEFAULT_OFF, &Val);
> +    Val = AMBA_ERROR_RESPONSE_CRS_SET (Val, 0x2);
> +    Ac01PcieCsrOut32 (CfgAddr + AMBA_ERROR_RESPONSE_DEFAULT_OFF, Val);
> +
> +    // Set Legacy PCIE interrupt map to INTA
> +    Ac01PcieCsrIn32 (CfgAddr + BRIDGE_CTRL_INT_PIN_INT_LINE_REG, &Val);
> +    Val = INT_PIN_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CfgAddr + BRIDGE_CTRL_INT_PIN_INT_LINE_REG, Val);
> +    Ac01PcieCsrIn32 (CsrAddr + IRQSEL, &Val);
> +    Val = INTPIN_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CsrAddr + IRQSEL, Val);
> +
> +    if (RC->Pcie[PcieIndex].MaxGen != SPEED_GEN1) {
> +      Ac01PcieConfigureEqualization (RC, PcieIndex);
> +      if (RC->Pcie[PcieIndex].MaxGen == SPEED_GEN3) {
> +        Ac01PcieConfigurePresetGen3 (RC, PcieIndex);
> +      } else if (RC->Pcie[PcieIndex].MaxGen == SPEED_GEN4) {
> +        Ac01PcieConfigurePresetGen4 (RC, PcieIndex);
> +      }
> +    }
> +
> +    // Mask Completion Timeout
> +    Ac01PcieCsrIn32 (CfgAddr + AMBA_LINK_TIMEOUT_OFF, &Val);
> +    Val = LINK_TIMEOUT_PERIOD_DEFAULT_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CfgAddr + AMBA_LINK_TIMEOUT_OFF, Val);
> +    Ac01PcieCsrIn32 (CfgAddr + UNCORR_ERR_MASK_OFF, &Val);
> +    Val = CMPLT_TIMEOUT_ERR_MASK_SET (Val, 1);
> +    // AER surprise link down error should be masked due to hotplug is enabled
> +    // This event must be handled by Hotplug handler, instead of error handler
> +    Val = SDES_ERR_MASK_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CfgAddr + UNCORR_ERR_MASK_OFF, Val);
> +
> +    // Program Class Code
> +    Ac01PcieCsrIn32 (CfgAddr + TYPE1_CLASS_CODE_REV_ID_REG, &Val);
> +    Val = REVISION_ID_SET (Val, 4);
> +    Val = SUBCLASS_CODE_SET (Val, 4);
> +    Val = BASE_CLASS_CODE_SET (Val, 6);
> +    Ac01PcieCsrOut32 (CfgAddr + TYPE1_CLASS_CODE_REV_ID_REG, Val);
> +
> +    // Program VendorID and DeviceID
> +    Ac01PcieCsrIn32 (CfgAddr + TYPE1_DEV_ID_VEND_ID_REG, &Val);
> +    Val = VENDOR_ID_SET (Val, AMPERE_PCIE_VENDORID);
> +    if (RCA == RC->Type) {
> +      Val = DEVICE_ID_SET (Val, AC01_PCIE_BRIDGE_DEVICEID_RCA + PcieIndex);
> +    } else {
> +      Val = DEVICE_ID_SET (Val, AC01_PCIE_BRIDGE_DEVICEID_RCB + PcieIndex);
> +    }
> +    Ac01PcieCsrOut32 (CfgAddr + TYPE1_DEV_ID_VEND_ID_REG, Val);
> +
> +    // Enable common clock for downstream
> +    Ac01PcieCsrIn32 (CfgAddr + LINK_CONTROL_LINK_STATUS_REG, &Val);
> +    Val = CAP_SLOT_CLK_CONFIG_SET (Val, 1);
> +    Val = CAP_COMMON_CLK_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CfgAddr + LINK_CONTROL_LINK_STATUS_REG, Val);
> +
> +    // Match aux_clk to system
> +    Ac01PcieCsrIn32 (CfgAddr + AUX_CLK_FREQ_OFF, &Val);
> +    Val = AUX_CLK_FREQ_SET (Val, AUX_CLK_500MHZ);
> +    Ac01PcieCsrOut32 (CfgAddr + AUX_CLK_FREQ_OFF, Val);
> +
> +    // Assert PERST low to reset endpoint
> +    BoardPcieAssertPerst (RC, PcieIndex, 0, FALSE);
> +
> +    // Start link training
> +    Ac01PcieCsrIn32 (CsrAddr + LINKCTRL, &Val);
> +    Val = LTSSMENB_SET (Val, 1);
> +    Ac01PcieCsrOut32 (CsrAddr + LINKCTRL, Val);
> +
> +    // Complete the PERST pulse
> +    BoardPcieAssertPerst (RC, PcieIndex, 0, TRUE);
> +
> +    // Lock programming of config space
> +    Ac01PcieCsrIn32 (CfgAddr + MISC_CONTROL_1_OFF, &Val);
> +    Val = DBI_RO_WR_EN_SET (Val, 0);
> +    Ac01PcieCsrOut32 (CfgAddr + MISC_CONTROL_1_OFF, Val);
> +
> +    if (ReInit == 1) {
> +      return 0;
> +    }
> +  }
> +
> +  // Program VendorID and DeviceId
> +  if (!EFI_ERROR (MailboxMsgRegisterRead (RC->Socket, RC->HBAddr  + HBPDVIDR, &Val))) {
> +    Val = PCIVENDID_SET (Val, AMPERE_PCIE_VENDORID);
> +    if (RCA == RC->Type) {
> +      Val = PCIDEVID_SET (Val, AC01_HOST_BRIDGE_DEVICEID_RCA);
> +    } else {
> +      Val = PCIDEVID_SET (Val, AC01_HOST_BRIDGE_DEVICEID_RCB);
> +    }
> +    MailboxMsgRegisterWrite (RC->Socket, RC->HBAddr  + HBPDVIDR, Val);
> +  }
> +
> +  return 0;
> +}
> +
> +BOOLEAN
> +PcieLinkUpCheck (
> +  IN  AC01_PCIE *Pcie
> +  )
> +{
> +  VOID   *CsrAddr;
> +  UINT32 BlockEvent, LinkStat;
> +
> +  CsrAddr = (VOID *)Pcie->CsrAddr;
> +
> +  // Check if card present
> +  // smlh_ltssm_state[13:8] = 0
> +  // phy_status[2] = 0
> +  // smlh_link_up[1] = 0
> +  // rdlh_link_up[0] = 0
> +  Ac01PcieCsrIn32 (CsrAddr + LINKSTAT, &LinkStat);
> +  LinkStat = LinkStat & (SMLH_LTSSM_STATE_MASK | PHY_STATUS_MASK_BIT |
> +                         SMLH_LINK_UP_MASK_BIT | RDLH_LINK_UP_MASK_BIT);
> +  if (LinkStat == 0x0000) {
> +    return FALSE;
> +  }
> +
> +  Ac01PcieCsrIn32 (CsrAddr +  BLOCKEVENTSTAT, &BlockEvent);
> +  Ac01PcieCsrIn32 (CsrAddr +  LINKSTAT, &LinkStat);
> +
> +  if (((BlockEvent & LINKUP_MASK) != 0)
> +      && (SMLH_LTSSM_STATE_GET(LinkStat) == S_L0))
> +  {
> +    DEBUG_PCIE_INFO ("%a Linkup\n", __FUNCTION__);
> +    return TRUE;
> +  }
> +
> +  return FALSE;
> +}
> +
> +VOID
> +Ac01PcieCoreEndEnumeration (
> +  AC01_RC *RC
> +  )
> +{
> +  VOID            *Reg;
> +  UINTN           Index;
> +  UINT32          Val;
> +  VOID            *CfgAddr;
> +
> +  if (RC == NULL || !RC->Active) {
> +    return;
> +  }
> +
> +  // Clear uncorrectable error during enumuration phase. Mainly completion timeout.
> +  for (Index = 0; Index < RC->MaxPcieController; Index++) {
> +    if (!RC->Pcie[Index].Active) {
> +      continue;
> +    }
> +    if (!PcieLinkUpCheck(&RC->Pcie[Index])) {
> +      // If link down/disabled after enumeration, disable completed time out
> +      CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[Index].DevNum << 15));
> +      Ac01PcieCsrIn32 (CfgAddr + UNCORR_ERR_MASK_OFF, &Val);
> +      Val = CMPLT_TIMEOUT_ERR_MASK_SET (Val, 1);
> +      Ac01PcieCsrOut32 (CfgAddr + UNCORR_ERR_MASK_OFF, Val);
> +    }
> +    // Clear all errors
> +    Reg = (VOID *)((UINT64)RC->MmcfgAddr + ((Index + 1) << 15) + UNCORR_ERR_STATUS_OFF);
> +    Ac01PcieCfgIn32 (Reg, &Val);
> +    if (Val != 0) {
> +      // Clear error by writting
> +      Ac01PcieCfgOut32 (Reg, Val);
> +    }
> +  }
> +}
> +
> +/**
> +   Comparing current link status with the max capabilities of the link
> +
> +   @param RC            Pointer to AC01_RC structure
> +   @param PcieIndex     PCIe index
> +   @param EpMaxWidth    EP max link width
> +   @param EpMaxGen      EP max link speed
> +   @retval              -1: Link status do not match with link max capabilities
> +                         1: Link capabilites are invalid
> +                         0: Link status are correct
> +**/
> +INT32
> +Ac01PcieCoreLinkCheck (
> +  IN AC01_RC  *RC,
> +  IN INTN      PcieIndex,
> +  IN UINT8     EpMaxWidth,
> +  IN UINT8     EpMaxGen
> +  )
> +{
> +  VOID         *CsrAddr, *CfgAddr;
> +  UINT32        Val, LinkStat;
> +  UINT32        MaxWidth, MaxGen;
> +
> +  CsrAddr = (VOID *)RC->Pcie[PcieIndex].CsrAddr;
> +  CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +  Ac01PcieCsrIn32 (CfgAddr + LINK_CAPABILITIES_REG, &Val);
> +  if ((CAP_MAX_LINK_WIDTH_GET (Val) == 0) ||
> +      (CAP_MAX_LINK_SPEED_GET (Val) == 0)) {
> +    DEBUG_PCIE_INFO ("\tPCIE%d.%d: Wrong RC capabilities\n", RC->ID, PcieIndex);
> +    return LINK_CHECK_WRONG_PARAMETER;
> +  }
> +
> +  if ((EpMaxWidth == 0) || (EpMaxGen == 0)) {
> +    DEBUG_PCIE_INFO ("\tPCIE%d.%d: Wrong EP capabilities\n", RC->ID, PcieIndex);
> +    return LINK_CHECK_FAILED;
> +  }
> +
> +  // Compare RC and EP capabilities
> +  if (CAP_MAX_LINK_WIDTH_GET (Val) > EpMaxWidth) {
> +    MaxWidth = EpMaxWidth;
> +  } else {
> +    MaxWidth = CAP_MAX_LINK_WIDTH_GET (Val);
> +  }
> +
> +  // Compare RC and EP capabilities
> +  if (CAP_MAX_LINK_SPEED_GET (Val) > EpMaxGen) {
> +    MaxGen = EpMaxGen;
> +  } else {
> +    MaxGen = CAP_MAX_LINK_SPEED_GET (Val);
> +  }
> +
> +  Ac01PcieCsrIn32 (CsrAddr + LINKSTAT, &LinkStat);
> +  Ac01PcieCsrIn32 (CfgAddr + LINK_CONTROL_LINK_STATUS_REG, &Val);
> +  DEBUG_PCIE_INFO (
> +    "PCIE%d.%d: Link MaxWidth %d MaxGen %d, LINKSTAT 0x%x",
> +    RC->ID,
> +    PcieIndex,
> +    MaxWidth,
> +    MaxGen,
> +    LinkStat
> +    );
> +
> +  // Checking all conditions of the link
> +  // If one of them is not sastified, return link up fail
> +  if ((CAP_NEGO_LINK_WIDTH_GET (Val) != MaxWidth) ||
> +      (CAP_LINK_SPEED_GET (Val) != MaxGen) ||
> +      (RDLH_SMLH_LINKUP_STATUS_GET (LinkStat) != (SMLH_LINK_UP_MASK_BIT | RDLH_LINK_UP_MASK_BIT)))
> +  {
> +    DEBUG_PCIE_INFO ("\tLinkCheck FAILED\n");
> +    return LINK_CHECK_FAILED;
> +  }
> +
> +  DEBUG_PCIE_INFO ("\tLinkCheck SUCCESS\n");
> +  return LINK_CHECK_SUCCESS;
> +}
> +
> +INT32
> +Ac01PFAEnableAll (
> +  IN AC01_RC   *RC,
> +  IN INTN      PcieIndex,
> +  IN INTN      PFAMode
> +  )
> +{
> +  VOID   *RasDesVSecBase;
> +  VOID   *CfgAddr;
> +  UINT8  ErrCode, ErrGrpNum;
> +  UINT32 Val;
> +  INT32  Ret = LINK_CHECK_SUCCESS;
> +
> +  UINT32 ErrCtrlCfg[MAX_NUM_ERROR_CODE] = {
> +    0x000, 0x001, 0x002, 0x003, 0x004, 0x005, 0x006, 0x007, 0x008, 0x009, 0x00A, // Per Lane
> +    0x105, 0x106, 0x107, 0x108, 0x109, 0x10A,
> +    0x200, 0x201, 0x202, 0x203, 0x204, 0x205, 0x206, 0x207,
> +    0x300, 0x301, 0x302, 0x303, 0x304, 0x305,
> +    0x400, 0x401,                                                                // Per Lane
> +    0x500, 0x501, 0x502, 0x503, 0x504, 0x505, 0x506, 0x507, 0x508, 0x509, 0x50A, 0x50B, 0x50C, 0x50D
> +  };
> +
> +  CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +  // Allow programming to config space
> +  Ac01PcieCsrIn32 (CfgAddr + MISC_CONTROL_1_OFF, &Val);
> +  Val = DBI_RO_WR_EN_SET (Val, 1);
> +  Ac01PcieCsrOut32 (CfgAddr + MISC_CONTROL_1_OFF, Val);
> +
> +  // Generate the RAS DES capability address
> +  // RAS_DES_CAP_ID = 0xB
> +  RasDesVSecBase = (VOID *)PcieCheckCap (RC, PcieIndex, 0x1, RAS_DES_CAP_ID);
> +  if (RasDesVSecBase == 0) {
> +    DEBUG_PCIE_INFO ("PCIE%d.%d: Cannot get RAS DES capability address\n", RC->ID, PcieIndex);
> +    return LINK_CHECK_WRONG_PARAMETER;
> +  }
> +
> +  if (PFAMode == PFA_REG_ENABLE) {
> +    // PFA Counters Enable
> +    Ac01PcieCsrIn32 (RasDesVSecBase + EVENT_COUNTER_CONTROL_REG_OFF, &Val);
> +    Val = ECCR_EVENT_COUNTER_ENABLE_SET (Val, 0x7);
> +    Val = ECCR_EVENT_COUNTER_CLEAR_SET (Val, 0);
> +    Ac01PcieCsrOut32 (RasDesVSecBase + EVENT_COUNTER_CONTROL_REG_OFF, Val);
> +  } else if (PFAMode == PFA_REG_CLEAR) {
> +    // PFA Counters Clear
> +    Ac01PcieCsrIn32 (RasDesVSecBase + EVENT_COUNTER_CONTROL_REG_OFF, &Val);
> +    Val = ECCR_EVENT_COUNTER_ENABLE_SET (Val, 0);
> +    Val = ECCR_EVENT_COUNTER_CLEAR_SET (Val, 0x3);
> +    Ac01PcieCsrOut32 (RasDesVSecBase + EVENT_COUNTER_CONTROL_REG_OFF, Val);
> +  } else {

At least this else case needs to be a helper function.

> +    // PFA Counters Read
> +    for (ErrCode = 0; ErrCode < MAX_NUM_ERROR_CODE; ErrCode++) {
> +      ErrGrpNum = (ErrCtrlCfg[ErrCode] & 0xF00) >> 8;
> +      // Skipping per lane group
> +      // Checking common lane group because AER error are included in common group only
> +      if ((ErrGrpNum != 0) && (ErrGrpNum != 4)) {
> +        Ac01PcieCsrIn32 (RasDesVSecBase + EVENT_COUNTER_CONTROL_REG_OFF, &Val);
> +        if (RC->Type == RCA) { // RCA - 4 PCIe controller per port, 1 controller in charge of 4 lanes
> +          Val = ECCR_LANE_SEL_SET (Val, PcieIndex*4);
> +        } else { // RCB - 8 PCIe controller per port, 1 controller in charge of 2 lanes
> +          Val = ECCR_LANE_SEL_SET (Val, PcieIndex*2);
> +        }
> +        Val = ECCR_GROUP_EVENT_SEL_SET (Val, ErrCtrlCfg[ErrCode]);
> +        Ac01PcieCsrOut32 (RasDesVSecBase + EVENT_COUNTER_CONTROL_REG_OFF, Val);
> +
> +        // After setting Counter Control reg
> +        // This delay just to make sure Counter Data reg is update with new value
> +        MicroSecondDelay (1);
> +        Ac01PcieCsrIn32 (RasDesVSecBase + EVENT_COUNTER_DATA_REG_OFF, &Val);
> +        if (Val != 0) {
> +          Ret = LINK_CHECK_FAILED;
> +          DEBUG_PCIE_INFO (
> +            "\tS%d RC%d RP%d \t%s: %d \tGROUP:%d-EVENT:%d\n",
> +            RC->Socket,
> +            RC->ID,
> +            PcieIndex,
> +            Val,
> +            ((ErrCtrlCfg[ErrCode] & 0xF00) >> 8),  // Group
> +            (ErrCtrlCfg[ErrCode] & 0x0FF)          // Event
> +            );
> +        }
> +      }
> +    }
> +  }
> +
> +  // Disable programming to config space
> +  Ac01PcieCsrIn32 (CfgAddr + MISC_CONTROL_1_OFF, &Val);
> +  Val = DBI_RO_WR_EN_SET (Val, 0);
> +  Ac01PcieCsrOut32 (CfgAddr + MISC_CONTROL_1_OFF, Val);
> +
> +  return Ret;
> +}
> +
> +/**
> +   Get link capabilities link width and speed of endpoint
> +
> +   @param RC[in]           Pointer to AC01_RC structure
> +   @param PcieIndex[in]    PCIe controller index
> +   @param EpMaxWidth[out]  EP max link width
> +   @param EpMaxGen[out]    EP max link speed
> +**/
> +VOID
> +Ac01PcieCoreGetEndpointInfo (
> +  IN  AC01_RC  *RC,
> +  IN  INTN     PcieIndex,
> +  OUT UINT8    *EpMaxWidth,
> +  OUT UINT8    *EpMaxGen
> +  )
> +{
> +  VOID         *PcieCapBaseAddr, *EpCfgAddr;
> +  VOID         *RcCfgAddr;
> +  UINT32       Val, RestoreVal;
> +  UINTN        TimeOut;
> +
> +  RcCfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +  // Allow programming to config space
> +  Ac01PcieCsrIn32 (RcCfgAddr + MISC_CONTROL_1_OFF, &Val);
> +  Val = DBI_RO_WR_EN_SET (Val, 1);
> +  Ac01PcieCsrOut32 (RcCfgAddr + MISC_CONTROL_1_OFF, Val);
> +
> +  Ac01PcieCsrIn32 (RcCfgAddr + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, &Val);
> +  RestoreVal = Val;
> +  Val = SUB_BUS_SET (Val, DEFAULT_SUB_BUS);             /* Set DEFAULT_SUB_BUS to Subordinate bus */
> +  Val = SEC_BUS_SET (Val, RC->Pcie[PcieIndex].DevNum);  /* Set RC->Pcie[PcieIndex].DevNum to Secondary bus */
> +  Val = PRIM_BUS_SET (Val, 0x0);                        /* Set 0x0 to Primary bus */
> +  Ac01PcieCsrOut32 (RcCfgAddr + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, Val);
> +  EpCfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 20)); /* Bus 1, dev 0, func 0 */
> +
> +  // Loop read EpCfgAddr value until got valid value or
> +  // reach to timeout EP_LINKUP_TIMEOUT (or more depend on card)

A helper function?

> +  TimeOut = EP_LINKUP_TIMEOUT;
> +  do {
> +    Ac01PcieCsrIn32 (EpCfgAddr, &Val);
> +    if (Val != 0xFFFF0001 && Val != 0xFFFFFFFF) {
> +      break;
> +    }
> +    MicroSecondDelay (LINK_WAIT_INTERVAL_US);
> +    TimeOut -= LINK_WAIT_INTERVAL_US;
> +  } while (TimeOut > 0);
> +
> +  Ac01PcieCsrIn32 (EpCfgAddr, &Val);
> +
> +  // Check whether EP config space is accessible or not

This block, a helper function?

> +  if (Val == 0xFFFFFFFF) {
> +    *EpMaxWidth = 0;   // Invalid Width
> +    *EpMaxGen = 0;     // Invalid Speed
> +    DEBUG_PCIE_INFO ("PCIE%d.%d Cannot access EP config space!\n", RC->ID, PcieIndex);
> +  } else {
> +    PcieCapBaseAddr = (VOID *)PcieCheckCap (RC, PcieIndex, 0x0, CAP_ID);
> +    if (PcieCapBaseAddr == 0) {
> +      *EpMaxWidth = 0;   // Invalid Width
> +      *EpMaxGen = 0;     // Invalid Speed
> +      DEBUG_PCIE_INFO (
> +        "PCIE%d.%d Cannot get PCIe capability extended address!\n",
> +        RC->ID,
> +        PcieIndex
> +        );
> +    } else {
> +      Ac01PcieCsrIn32 (PcieCapBaseAddr + LINK_CAPABILITIES_REG_OFF, &Val);
> +      *EpMaxWidth = (Val >> 4) & 0x3F;
> +      *EpMaxGen = Val & 0xF;
> +      DEBUG_PCIE_INFO (
> +        "PCIE%d.%d EP MaxWidth %d EP MaxGen %d \n", RC->ID,
> +        PcieIndex,
> +        *EpMaxWidth,
> +        *EpMaxGen
> +        );
> +
> +      // From EP, enabling common clock for upstream
> +      Ac01PcieCsrIn32 (PcieCapBaseAddr + LINK_CONTROL_LINK_STATUS_OFF, &Val);
> +      Val = CAP_SLOT_CLK_CONFIG_SET (Val, 1);
> +      Val = CAP_COMMON_CLK_SET (Val, 1);
> +      Ac01PcieCsrOut32 (PcieCapBaseAddr + LINK_CONTROL_LINK_STATUS_OFF, Val);
> +    }
> +  }
> +
> +  // Restore value in order to not affect enumeration process
> +  Ac01PcieCsrOut32 (RcCfgAddr + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, RestoreVal);
> +
> +  // Disable programming to config space
> +  Ac01PcieCsrIn32 (RcCfgAddr + MISC_CONTROL_1_OFF, &Val);
> +  Val = DBI_RO_WR_EN_SET (Val, 0);
> +  Ac01PcieCsrOut32 (RcCfgAddr + MISC_CONTROL_1_OFF, Val);
> +}
> +
> +/**
> +  Check active PCIe controllers of RC, retrain or soft reset if needed
> +
> +  @param RC[in]         Pointer to AC01_RC structure
> +  @param PcieIndex[in]  PCIe controller index
> +
> +  @retval               -1: Link recovery had failed
> +                        1: Link width and speed are not correct
> +                        0: Link recovery succeed
> +**/
> +INT32
> +Ac01PcieCoreQoSLinkCheckRecovery (
> +  IN AC01_RC   *RC,
> +  IN INTN      PcieIndex
> +  )
> +{
> +  VOID        *CsrAddr;
> +  INTN        TimeOut;
> +  INT32       NumberOfReset = MAX_REINIT; // Number of soft reset retry
> +  UINT8       EpMaxWidth, EpMaxGen;
> +  INT32       LinkStatusCheck, RasdesChecking;
> +
> +  // PCIe controller is not active or Link is not up
> +  // Nothing to be done
> +  if ((!RC->Pcie[PcieIndex].Active) || (!RC->Pcie[PcieIndex].LinkUp)) {
> +    return LINK_CHECK_WRONG_PARAMETER;
> +  }
> +
> +  do {
> +
> +    if (RC->Pcie[PcieIndex].LinkUp) {
> +      // Enable all of RASDES register to detect any training error
> +      Ac01PFAEnableAll (RC, PcieIndex, PFA_REG_ENABLE);
> +
> +      // Accessing Endpoint and checking current link capabilities
> +      Ac01PcieCoreGetEndpointInfo (RC, PcieIndex, &EpMaxWidth, &EpMaxGen);
> +      LinkStatusCheck = Ac01PcieCoreLinkCheck (RC, PcieIndex, EpMaxWidth, EpMaxGen);
> +
> +      // Delay to allow the link to perform internal operation and generate
> +      // any error status update. This allows detection of any error observed
> +      // during initial link training. Possible evaluation time can be
> +      // between 100ms to 200ms.
> +      MicroSecondDelay (100000);
> +
> +      // Check for error
> +      RasdesChecking = Ac01PFAEnableAll (RC, PcieIndex, PFA_REG_READ);
> +
> +      // Clear error counter
> +      Ac01PFAEnableAll (RC, PcieIndex, PFA_REG_CLEAR);
> +
> +      // If link check functions return passed, then breaking out
> +      // else go to soft reset
> +      if (LinkStatusCheck != LINK_CHECK_FAILED &&
> +          RasdesChecking != LINK_CHECK_FAILED &&
> +          PcieLinkUpCheck (&RC->Pcie[PcieIndex]))
> +      {
> +        return LINK_CHECK_SUCCESS;
> +      }
> +
> +      RC->Pcie[PcieIndex].LinkUp = FALSE;
> +    }
> +
> +    // Trigger controller soft reset
> +    DEBUG_PCIE_INFO ("PCIE%d.%d Start link re-initialization..\n", RC->ID, PcieIndex);
> +    Ac01PcieCoreSetupRC (RC, 1, PcieIndex);
> +
> +    // Poll until link up
> +    // This checking for linkup status and
> +    // give LTSSM state the time to transit from DECTECT state to L0 state
> +    // Total delay are 100ms, smaller number of delay cannot always make sure
> +    // the state transition is completed
> +    TimeOut = LTSSM_TRANSITION_TIMEOUT;
> +    CsrAddr = (VOID *)RC->Pcie[PcieIndex].CsrAddr;
> +    do {

This do/while inside a do/while loop - helper?

> +      if (PcieLinkUpCheck (&RC->Pcie[PcieIndex])) {
> +        DEBUG_PCIE_INFO (
> +          "\tPCIE%d.%d LinkStat is correct after soft reset, transition time: %d\n",
> +          RC->ID,
> +          PcieIndex,
> +          TimeOut
> +          );
> +        RC->Pcie[PcieIndex].LinkUp = TRUE;
> +        break;
> +      }
> +
> +      MicroSecondDelay (100);
> +      TimeOut -= 100;
> +    } while (TimeOut > 0);
> +
> +    if (TimeOut <= 0) {
> +      DEBUG_PCIE_INFO ("\tPCIE%d.%d LinkStat TIMEOUT after re-init\n", RC->ID, PcieIndex);
> +    } else {
> +      DEBUG_PCIE_INFO ("PCIE%d.%d Link re-initialization passed!\n", RC->ID, PcieIndex);
> +    }
> +
> +    NumberOfReset--;
> +  } while (NumberOfReset > 0);
> +
> +  return LINK_CHECK_SUCCESS;
> +}
> +
> +VOID
> +Ac01PcieCoreUpdateLink (
> +  IN  AC01_RC *RC,
> +  OUT BOOLEAN *IsNextRoundNeeded,
> +  OUT INT8    *FailedPciePtr,
> +  OUT INT8    *FailedPcieCount
> +  )
> +{
> +  INTN      PcieIndex;
> +  AC01_PCIE *Pcie;
> +  UINT32    i;
> +  UINT32    Val;
> +  VOID      *CfgAddr;
> +
> +  *IsNextRoundNeeded = FALSE;
> +  *FailedPcieCount   = 0;
> +  for (i = 0; i < MAX_PCIE_B; i++) {
> +    FailedPciePtr[i] = -1;
> +  }
> +
> +  if (!RC->Active) {
> +    return;
> +  }
> +
> +  // Loop for all controllers
> +  for (PcieIndex = 0; PcieIndex < RC->MaxPcieController; PcieIndex++) {
> +    Pcie = &RC->Pcie[PcieIndex];
> +    CfgAddr = (VOID *)(RC->MmcfgAddr + (RC->Pcie[PcieIndex].DevNum << 15));
> +
> +    if (Pcie->Active && !Pcie->LinkUp) {
> +      if (PcieLinkUpCheck (Pcie)) {
> +        Pcie->LinkUp = TRUE;
> +        Ac01PcieCsrIn32 (CfgAddr + LINK_CONTROL_LINK_STATUS_REG, &Val);
> +        DEBUG_PCIE_INFO (
> +          "%a S%d RC%d RP%d NEGO_LINK_WIDTH: 0x%x LINK_SPEED: 0x%x\n",
> +          __FUNCTION__,
> +          RC->Socket,
> +          RC->ID,
> +          PcieIndex,
> +          CAP_NEGO_LINK_WIDTH_GET (Val),
> +          CAP_LINK_SPEED_GET (Val)
> +          );
> +
> +        // Doing link checking and recovery if needed
> +        Ac01PcieCoreQoSLinkCheckRecovery (RC, PcieIndex);
> +
> +        // Un-mask Completion Timeout

Sounds like a helper function.

> +        Ac01PcieCsrIn32 (CfgAddr + AMBA_LINK_TIMEOUT_OFF, &Val);
> +        Val = LINK_TIMEOUT_PERIOD_DEFAULT_SET (Val, 32);
> +        Ac01PcieCsrOut32 (CfgAddr + AMBA_LINK_TIMEOUT_OFF, Val);
> +        Ac01PcieCsrIn32 (CfgAddr + UNCORR_ERR_MASK_OFF, &Val);
> +        Val = CMPLT_TIMEOUT_ERR_MASK_SET (Val, 0);
> +        Ac01PcieCsrOut32 (CfgAddr + UNCORR_ERR_MASK_OFF, Val);
> +      } else {
> +        *IsNextRoundNeeded = FALSE;
> +        FailedPciePtr[*FailedPcieCount] = PcieIndex;
> +        *FailedPcieCount += 1;
> +      }
> +    }
> +  }
> +}
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreLib.c
> new file mode 100644
> index 000000000000..42a0f240763c
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCoreLib.c
> @@ -0,0 +1,556 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/AmpereCpuLib.h>
> +#include <Library/ArmGenericTimerCounterLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/BoardPcieLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PciHostBridgeLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/SerialPortLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Platform/Ac01.h>
> +#include <Protocol/PciHostBridgeResourceAllocation.h>
> +
> +#include "PcieCore.h"
> +#include "PciePatchAcpi.h"
> +
> +STATIC UINT64  RCRegBase[AC01_MAX_PCIE_ROOT_COMPLEX] = { AC01_PCIE_REGISTER_BASE };
> +STATIC UINT64  RCMmioBase[AC01_MAX_PCIE_ROOT_COMPLEX] = { AC01_PCIE_MMIO_BASE };
> +STATIC UINT64  RCMmioSize[AC01_MAX_PCIE_ROOT_COMPLEX] = {AC01_PCIE_MMIO_SIZE};
> +STATIC UINT64  RCMmio32Base[AC01_MAX_PCIE_ROOT_COMPLEX] = { AC01_PCIE_MMIO32_BASE };
> +STATIC UINT64  RCMmio32Base1P[AC01_MAX_PCIE_ROOT_COMPLEX] = { AC01_PCIE_MMIO32_BASE_1P };
> +STATIC UINT64  RCMmio32Size[AC01_MAX_PCIE_ROOT_COMPLEX] = {AC01_PCIE_MMIO32_SIZE};
> +STATIC UINT64  RCMmio32Size1P[AC01_MAX_PCIE_ROOT_COMPLEX] = {AC01_PCIE_MMIO32_SIZE_1P};
> +STATIC AC01_RC RCList[AC01_MAX_PCIE_ROOT_COMPLEX];
> +STATIC INT8    PciList[AC01_MAX_PCIE_ROOT_COMPLEX];

Global variables need m or g prefix.


> +
> +STATIC
> +VOID
> +SerialPrint (

Please, not yet another way to output debug info...
Nuke it.

> +  IN CONST CHAR8 *FormatString,
> +  ...
> +  )
> +{
> +  UINT8   Buf[64];
> +  VA_LIST Marker;
> +  UINTN   NumberOfPrinted;
> +
> +  VA_START (Marker, FormatString);
> +  NumberOfPrinted = AsciiVSPrint ((CHAR8 *)Buf, sizeof (Buf), FormatString, Marker);
> +  SerialPortWrite (Buf, NumberOfPrinted);
> +  VA_END (Marker);
> +}
> +
> +AC01_RC *
> +GetRCList (
> +  UINT8 Idx
> +  )
> +{
> +  return &RCList[Idx];
> +}
> +
> +/**
> +   Map BusDxe Host bridge and Root bridge Indexes to PCIE core BSP driver Root Complex Index.
> +
> +   @param  HBIndex               Index to identify of PCIE Host bridge.
> +   @param  RBIndex               Index to identify of PCIE Root bridge.

HB -> HostBridge
RB -> RootBridge
Search/replace.

> +   @retval UINT8                 Index to identify Root Complex instance from global RCList.
> +**/
> +STATIC
> +UINT8
> +GetRCIndex (
> +  IN UINT8 HBIndex,
> +  IN UINT8 RBIndex
> +  )
> +{
> +  //
> +  // BusDxe addresses resources based on Host bridge and Root bridge.
> +  // Map those to Root Complex index/instance known for Pcie Core BSP driver
> +  //
> +  return HBIndex * AC01_MAX_PCIE_ROOT_BRIDGE + RBIndex;
> +}
> +
> +/**
> +  Prepare to start PCIE core BSP driver
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieSetup (
> +  VOID
> +  )
> +{
> +  AC01_RC *RC;
> +  INTN    RCIndex;
> +
> +  ZeroMem (&RCList, sizeof (AC01_RC) * AC01_MAX_PCIE_ROOT_COMPLEX);
> +
> +  // Adjust MMIO32 base address 1P vs 2P
> +  if (!IsSlaveSocketPresent ()) {

Hmm. Another thing I missed in previous review.
TianoCore does not yet have a policy for inclusive language, but you
might want to give your set a renaming pass for good measure.

> +    CopyMem ((VOID *)&RCMmio32Base, (VOID *)&RCMmio32Base1P, sizeof (RCMmio32Base1P));
> +    CopyMem ((VOID *)&RCMmio32Size, (VOID *)&RCMmio32Size1P, sizeof (RCMmio32Size1P));
> +  }
> +
> +  for (RCIndex = 0; RCIndex < AC01_MAX_PCIE_ROOT_COMPLEX; RCIndex++) {
> +    RC = &RCList[RCIndex];
> +    RC->Socket = RCIndex / AC01_MAX_RCS_PER_SOCKET;
> +    RC->ID = RCIndex % AC01_MAX_RCS_PER_SOCKET;
> +
> +    Ac01PcieCoreBuildRCStruct (
> +      RC,
> +      RCRegBase[RCIndex],
> +      RCMmioBase[RCIndex],
> +      RCMmioSize[RCIndex],
> +      RCMmio32Base[RCIndex],
> +      RCMmio32Size[RCIndex]
> +      );
> +  }
> +
> +  // Build the UEFI menu
> +  BoardPcieScreenInitialize (RCList);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Get Total HostBridge.
> +
> +  @retval UINTN                     Return Total HostBridge.
> +**/
> +UINT8
> +Ac01PcieGetTotalHBs (
> +  VOID
> +  )
> +{
> +  return AC01_MAX_PCIE_ROOT_COMPLEX;
> +}
> +
> +/**
> +  Get Total RootBridge per HostBridge.
> +
> +  @param[in]  RCIndex               Index to identify of Root Complex.
> +
> +  @retval UINTN                     Return Total RootBridge per HostBridge.
> +**/
> +UINT8
> +Ac01PcieGetTotalRBsPerHB (
> +  IN UINTN RCIndex
> +  )
> +{
> +  return AC01_MAX_PCIE_ROOT_BRIDGE;
> +}
> +
> +/**
> +  Get RootBridge Attribute.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval UINTN                     Return RootBridge Attribute.
> +**/
> +UINTN
> +Ac01PcieGetRootBridgeAttribute (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  )
> +{
> +  return EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
> +}
> +
> +/**
> +  Get RootBridge Segment number
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval UINTN                     Return RootBridge Segment number.
> +**/
> +UINTN
> +Ac01PcieGetRootBridgeSegmentNumber (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  )
> +{
> +  UINTN   RCIndex;
> +  AC01_RC *RC;
> +  UINTN   SegmentNumber;
> +
> +  RCIndex = GetRCIndex (HBIndex, RBIndex);
> +  RC = &RCList[RCIndex];
> +  SegmentNumber = RCIndex;
> +
> +  // Get board specific overrides
> +  BoardPcieGetRCSegmentNumber (RC, &SegmentNumber);
> +  RC->Logical = SegmentNumber;
> +
> +  return SegmentNumber;
> +}
> +
> +STATIC
> +VOID
> +SortPciList (
> +  INT8 *PciList
> +  )
> +{
> +  INT8 Idx1, Idx2;
> +
> +  for (Idx2 = 0, Idx1 = 0; Idx2 < AC01_MAX_PCIE_ROOT_COMPLEX; Idx2++) {
> +    if (PciList[Idx2] < 0) {
> +      continue;
> +    }
> +    PciList[Idx1] = PciList[Idx2];
> +    if (Idx1 != Idx2) {
> +      PciList[Idx2] = -1;
> +    }
> +    Idx1++;
> +  }
> +
> +  for (Idx2 = 0; Idx2 < Idx1; Idx2++) {
> +    DEBUG_PCIE_INFO (
> +      " %a: PciList[%d]=%d TcuAddr=0x%llx\n",
> +      __FUNCTION__,
> +      Idx2,
> +      PciList[Idx2],
> +      RCList[PciList[Idx2]].TcuAddr
> +      );
> +  }
> +}
> +
> +/**
> +  Get RootBridge disable status.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval BOOLEAN                   Return RootBridge disable status.
> +**/
> +BOOLEAN
> +Ac01PcieCheckRootBridgeDisabled (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  )
> +{
> +  UINTN RCIndex;
> +  INT8  Ret;
> +
> +  RCIndex = HBIndex;
> +  Ret = !RCList[RCIndex].Active;
> +  if (Ret) {
> +    PciList[HBIndex] = -1;
> +  } else {
> +    PciList[HBIndex] = HBIndex;
> +  }
> +  if (HBIndex == (AC01_MAX_PCIE_ROOT_COMPLEX -1)) {
> +    SortPciList (PciList);
> +    if (!IsSlaveSocketPresent ()) {
> +      AcpiPatchPciMem32 (PciList);
> +    }
> +    AcpiInstallMcfg (PciList);
> +    AcpiInstallIort (PciList);
> +  }
> +  return Ret;
> +}
> +
> +/**
> +  Initialize Host bridge
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieSetupHostBridge (
> +  IN UINTN HBIndex
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Initialize Root bridge
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +  @param[in]  RootBridgeInstance    The pointer of instance of the Root bridge IO.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieSetupRootBridge (
> +  IN UINTN           HBIndex,
> +  IN UINTN           RBIndex,
> +  IN PCI_ROOT_BRIDGE *RootBridge
> +  )
> +{
> +  UINTN   RCIndex;
> +  AC01_RC *RC;
> +  UINT32  Result;
> +
> +  RCIndex = GetRCIndex (HBIndex, RBIndex);
> +  RC = &RCList[RCIndex];
> +  if (!RC->Active) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  RC->RootBridge = (VOID *)RootBridge;
> +
> +  // Initialize Root Complex and underneath controllers
> +  Result = Ac01PcieCoreSetupRC (RC, 0, 0);
> +  if (Result) {
> +    DEBUG_PCIE_ERR ("RootComplex[%d]: Init Failed\n", RCIndex);
> +
> +    goto Error;
> +  }
> +
> +  // Populate resource aperture
> +  RootBridge->Bus.Base = 0x0;
> +  RootBridge->Bus.Limit = 0xFF;
> +  RootBridge->Io.Base = RC->IoAddr;
> +  RootBridge->Io.Limit = RC->IoAddr + IO_SPACE - 1;
> +  RootBridge->Mem.Base = RC->Mmio32Addr;
> +  RootBridge->Mem.Limit = (RootBridge->Mem.Base) ? (RootBridge->Mem.Base + RC->Mmio32Size - 1) : 0;
> +  RootBridge->PMem.Base = RootBridge->Mem.Base;
> +  RootBridge->PMem.Limit = RootBridge->Mem.Limit;
> +  RootBridge->MemAbove4G.Base = MAX_UINT64;
> +  RootBridge->MemAbove4G.Limit = 0x0;
> +  RootBridge->PMemAbove4G.Base = RC->MmioAddr;
> +  RootBridge->PMemAbove4G.Limit = (RootBridge->PMemAbove4G.Base) ? (RootBridge->PMemAbove4G.Base + RC->MmioSize - 1) : 0;
> +
> +  DEBUG_PCIE_INFO (" +    Bus: 0x%lx - 0x%lx\n", RootBridge->Bus.Base, RootBridge->Bus.Limit);
> +  DEBUG_PCIE_INFO (" +     Io: 0x%lx - 0x%lx\n", RootBridge->Io.Base, RootBridge->Io.Limit);
> +  DEBUG_PCIE_INFO (" +    Mem: 0x%lx - 0x%lx\n", RootBridge->Mem.Base, RootBridge->Mem.Limit);
> +  DEBUG_PCIE_INFO (" +   PMem: 0x%lx - 0x%lx\n", RootBridge->PMem.Base, RootBridge->PMem.Limit);
> +  DEBUG_PCIE_INFO (" +  4GMem: 0x%lx - 0x%lx\n", RootBridge->MemAbove4G.Base, RootBridge->MemAbove4G.Limit);
> +  DEBUG_PCIE_INFO (" + 4GPMem: 0x%lx - 0x%lx\n", RootBridge->PMemAbove4G.Base, RootBridge->PMemAbove4G.Limit);
> +
> +  return EFI_SUCCESS;
> +
> +Error:
> +  RC->Active = FALSE;
> +  return EFI_DEVICE_ERROR;
> +}
> +
> +/**
> +  Reads/Writes an PCI configuration register.
> +
> +  @param[in]  RootInstance          Pointer to RootInstance structure.
> +  @param[in]  Address               Address which want to read or write to.
> +  @param[in]  Write                 Indicate that this is a read or write command.
> +  @param[in]  Width                 Specify the width of the data.
> +  @param[in, out]  Data             The buffer to hold the data.
> +
> +  @retval EFI_SUCCESS               The operation is successful.
> +  @retval Others                    An error occurred.
> +**/
> +EFI_STATUS
> +Ac01PcieConfigRW (
> +  IN     VOID    *RootInstance,
> +  IN     UINT64  Address,
> +  IN     BOOLEAN Write,
> +  IN     UINTN   Width,
> +  IN OUT VOID    *Data
> +  )
> +{
> +  AC01_RC *RC = NULL;
> +  VOID    *CfgBase = NULL;
> +  UINTN   RCIndex;
> +  UINT32  Reg;
> +  UINT32  Segment;
> +
> +  Segment = RShiftU64 (Address, 32) & 0xFFFF;
> +
> +  for (RCIndex = 0; RCIndex < AC01_MAX_PCIE_ROOT_COMPLEX; RCIndex++) {
> +    RC = &RCList[RCIndex];
> +    if (RC->RootBridge != NULL) {
> +      if (RC->RootBridge == RootInstance
> +         || (RootInstance == NULL
> +            && ((PCI_ROOT_BRIDGE *)RC->RootBridge)->Segment == Segment)) {
> +      break;
> +      }
> +    }
> +  }
> +
> +  if ((RCIndex == AC01_MAX_PCIE_ROOT_COMPLEX) || (RC == NULL)) {
> +    DEBUG_PCIE_ERR ("Can't find Root Bridge instance:%p\n", RootInstance);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Reg = Address & 0xFFF;
> +
> +  CfgBase = (VOID *)((UINT64)RC->MmcfgAddr + (Address & 0x0FFFF000));
> +  if (Write) {
> +    switch (Width) {
> +    case 1:
> +      Ac01PcieCfgOut8 ((VOID *)(CfgBase + (Reg & (~(Width - 1)))), *((UINT8 *)Data));
> +      break;
> +
> +    case 2:
> +      Ac01PcieCfgOut16 ((VOID *)(CfgBase + (Reg & (~(Width - 1)))), *((UINT16 *)Data));
> +      break;
> +
> +    case 4:
> +      Ac01PcieCfgOut32 ((VOID *)(CfgBase + (Reg & (~(Width - 1)))), *((UINT32 *)Data));
> +      break;
> +
> +    default:
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  } else {
> +    switch (Width) {
> +    case 1:
> +      Ac01PcieCfgIn8 ((VOID *)(CfgBase + (Reg & (~(Width - 1)))), (UINT8 *)Data);
> +      break;
> +
> +    case 2:
> +      Ac01PcieCfgIn16 ((VOID *)(CfgBase + (Reg & (~(Width - 1)))), (UINT16 *)Data);
> +      if (Reg == 0xAE && (*((UINT16 *)Data)) == 0xFFFF) {
> +        SerialPrint ("PANIC due to PCIE RC:%d link issue\n", RC->ID);
> +        // Loop forever waiting for failsafe/watch dog time out
> +        do {
> +        } while (1);
> +      }
> +      break;
> +
> +    case 4:
> +      Ac01PcieCfgIn32 ((VOID *)(CfgBase + (Reg & (~(Width - 1)))), (UINT32 *)Data);
> +      break;
> +
> +    default:
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +VOID
> +Ac01PcieCorePollLinkUp (
> +  VOID
> +  )
> +{
> +  INTN    RCIndex, PcieIndex, i;
> +  BOOLEAN IsNextRoundNeeded = FALSE, NextRoundNeeded;
> +  UINT64  PrevTick, CurrTick, ElapsedCycle;
> +  UINT64  TimerTicks64;
> +  UINT8   ReInit;
> +  INT8    FailedPciePtr[MAX_PCIE_B];
> +  INT8    FailedPcieCount;
> +
> +  ReInit = 0;
> +
> +_link_polling:
> +  NextRoundNeeded = 0;
> +  //
> +  // It is not guaranteed the timer service is ready prior to PCI Dxe.
> +  // Calculate system ticks for link training.
> +  //
> +  TimerTicks64 = ArmGenericTimerGetTimerFreq (); /* 1 Second */
> +  PrevTick = ArmGenericTimerGetSystemCount ();
> +  ElapsedCycle = 0;
> +
> +  do {
> +    // Update timer
> +    CurrTick = ArmGenericTimerGetSystemCount ();
> +    if (CurrTick < PrevTick) {
> +      ElapsedCycle += (UINT64)(~0x0ULL) - PrevTick;
> +      PrevTick = 0;
> +    }
> +    ElapsedCycle += (CurrTick - PrevTick);
> +    PrevTick = CurrTick;
> +  } while (ElapsedCycle < TimerTicks64);
> +
> +  for (RCIndex = 0; RCIndex < AC01_MAX_PCIE_ROOT_COMPLEX; RCIndex++) {
> +    Ac01PcieCoreUpdateLink (&RCList[RCIndex], &IsNextRoundNeeded, FailedPciePtr, &FailedPcieCount);
> +    if (IsNextRoundNeeded) {
> +      NextRoundNeeded = 1;
> +    }
> +  }
> +
> +  if (NextRoundNeeded && ReInit < MAX_REINIT) {
> +    // Timer is up. Give another chance to re-program controller
> +    ReInit++;
> +    for (RCIndex = 0; RCIndex < AC01_MAX_PCIE_ROOT_COMPLEX; RCIndex++) {
> +      Ac01PcieCoreUpdateLink (&RCList[RCIndex], &IsNextRoundNeeded, FailedPciePtr, &FailedPcieCount);
> +      if (IsNextRoundNeeded) {
> +        for (i = 0; i < FailedPcieCount; i++) {
> +          PcieIndex = FailedPciePtr[i];
> +          if (PcieIndex == -1) {
> +            continue;
> +          }
> +
> +          // Some controller still observes link-down. Re-init controller
> +          Ac01PcieCoreSetupRC (&RCList[RCIndex], 1, PcieIndex);
> +        }
> +      }
> +    }
> +
> +    goto _link_polling;
> +  }
> +}
> +
> +/**
> +  Prepare to end PCIE core BSP driver
> +**/
> +VOID
> +Ac01PcieEnd (
> +  VOID
> +  )
> +{
> +  Ac01PcieCorePollLinkUp ();
> +}
> +
> +/**
> +  Callback funciton for EndEnumeration notification from PCI stack.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +  @param[in]  Phase                 The phase of enumeration as informed from PCI stack.
> +**/
> +VOID
> +Ac01PcieHostBridgeNotifyPhase (
> +  IN UINTN                                         HBIndex,
> +  IN UINTN                                         RBIndex,
> +  IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PHASE Phase
> +  )
> +{
> +  AC01_RC *RC;
> +  UINTN   RCIndex;
> +
> +  RCIndex = GetRCIndex (HBIndex, RBIndex);
> +  RC = &RCList[RCIndex];
> +
> +  switch (Phase) {
> +  case EfiPciHostBridgeEndEnumeration:
> +    Ac01PcieCoreEndEnumeration (RC);
> +    break;
> +
> +  case EfiPciHostBridgeBeginEnumeration:
> +    /* 100ms that help fixing completion timeout issue */
> +    MicroSecondDelay (100000);
> +    break;
> +
> +  case EfiPciHostBridgeBeginBusAllocation:
> +  case EfiPciHostBridgeEndBusAllocation:
> +  case EfiPciHostBridgeBeginResourceAllocation:
> +  case EfiPciHostBridgeAllocateResources:
> +  case EfiPciHostBridgeSetResources:
> +  case EfiPciHostBridgeFreeResources:
> +  case EfiPciHostBridgeEndResourceAllocation:
> +  case EfiMaxPciHostBridgeEnumerationPhase:
> +    break;
> +  }
> +}
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.c
> new file mode 100644
> index 000000000000..b15dea459a07
> --- /dev/null
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PciePatchAcpi.c
> @@ -0,0 +1,646 @@
> +/** @file
> +
> +  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <AcpiHeader.h>
> +#include <IndustryStandard/Acpi30.h>
> +#include <IndustryStandard/IoRemappingTable.h>
> +#include <Library/AcpiLib.h>
> +#include <Library/AmpereCpuLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/BoardPcieLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Platform/Ac01.h>
> +#include <Protocol/AcpiTable.h>
> +
> +#include "PcieCore.h"
> +
> +#define ACPI_RESOURCE_NAME_ADDRESS16            0x88
> +#define ACPI_RESOURCE_NAME_ADDRESS64            0x8A
> +
> +#define RCA_NUM_TBU_PMU         6
> +#define RCB_NUM_TBU_PMU         10
> +
> +// Required to be 1 to match the kernel quirk for ECAM
> +#define EFI_ACPI_MCFG_OEM_REVISION 1
> +
> +STATIC UINT32 gTbuPmuIrqArray[] = { AC01_SMMU_TBU_PMU_IRQS };
> +STATIC UINT32 gTcuPmuIrqArray[] = { AC01_SMMU_TCU_PMU_IRQS };
> +
> +#pragma pack(1)
> +typedef struct
> +{
> +  UINT64 ullBaseAddress;

Absolutely no Hungarian notation.

> +  UINT16 usSegGroupNum;
> +  UINT8  ucStartBusNum;
> +  UINT8  ucEndBusNum;

Those three too.

> +  UINT32 Reserved2;
> +} EFI_MCFG_CONFIG_STRUCTURE;
> +
> +typedef struct
> +{
> +  EFI_ACPI_DESCRIPTION_HEADER Header;
> +  UINT64                      Reserved1;
> +} EFI_MCFG_TABLE_CONFIG;
> +
> +typedef struct {
> +  UINT64 AddressGranularity;
> +  UINT64 AddressMin;
> +  UINT64 AddressMax;
> +  UINT64 AddressTranslation;
> +  UINT64 RangeLength;
> +} QWordMemory;
> +
> +typedef struct ResourceEntry {
> +  UINT8  ResourceType;
> +  UINT16 ResourceSize;
> +  UINT8  Attribute;
> +  UINT8  Byte0;
> +  UINT8  Byte1;
> +  VOID   *ResourcePtr;
> +} RESOURCE;
> +
> +STATIC QWordMemory Qmem[] = {
> +  { AC01_PCIE_RCA2_QMEM },
> +  { AC01_PCIE_RCA3_QMEM },
> +  { AC01_PCIE_RCB0_QMEM },
> +  { AC01_PCIE_RCB1_QMEM },
> +  { AC01_PCIE_RCB2_QMEM },
> +  { AC01_PCIE_RCB3_QMEM }
> +};
> +
> +typedef struct {
> +  EFI_ACPI_6_0_IO_REMAPPING_NODE Node;
> +  UINT64                         Base;
> +  UINT32                         Flags;
> +  UINT32                         Reserved;
> +  UINT64                         VatosAddress;
> +  UINT32                         Model;
> +  UINT32                         Event;
> +  UINT32                         Pri;
> +  UINT32                         Gerr;
> +  UINT32                         Sync;
> +  UINT32                         ProximityDomain;
> +  UINT32                         DeviceIdMapping;
> +} EFI_ACPI_6_2_IO_REMAPPING_SMMU3_NODE;
> +
> +typedef struct {
> +  EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE Node;
> +  UINT32                             ItsIdentifier;
> +} AC01_ITS_NODE;
> +
> +
> +typedef struct {
> +  EFI_ACPI_6_0_IO_REMAPPING_RC_NODE  Node;
> +  EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE RcIdMapping;
> +} AC01_RC_NODE;
> +
> +typedef struct {
> +  EFI_ACPI_6_2_IO_REMAPPING_SMMU3_NODE Node;
> +  EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE   InterruptMsiMapping;
> +  EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE   InterruptMsiMappingSingle;
> +} AC01_SMMU_NODE;
> +
> +typedef struct {
> +  EFI_ACPI_6_0_IO_REMAPPING_TABLE Iort;
> +  AC01_ITS_NODE                   ItsNode[2];
> +  AC01_RC_NODE                    RcNode[2];
> +  AC01_SMMU_NODE                  SmmuNode[2];
> +} AC01_IO_REMAPPING_STRUCTURE;
> +
> +#define FIELD_OFFSET(type, name)            __builtin_offsetof (type, name)

Use OFFSET_OF from Base.h.
If we ever need toolchain-specific macros, they should explicitly be
guarded by macros so other toolchains don't try to invoke them. The
Base.h version does this.

> +#define __AC01_ID_MAPPING(In, Num, Out, Ref, Flags)    \
> +  {                                                    \
> +    In,                                                \
> +    Num,                                               \
> +    Out,                                               \
> +    FIELD_OFFSET (AC01_IO_REMAPPING_STRUCTURE, Ref),   \
> +    Flags                                              \
> +  }
> +
> +EFI_STATUS
> +EFIAPI
> +AcpiPatchPciMem32 (
> +  INT8 *PciSegEnabled
> +  )
> +{
> +  EFI_ACPI_SDT_PROTOCOL         *AcpiSdtProtocol;
> +  EFI_STATUS                    Status;
> +  UINTN                         Idx, Ix;

This borders very closely on code obfuscation.
I, J are fine and accepted.
The code has used Idx1, Idx2 in other places, and I won't kick back on
that.

> +  EFI_ACPI_DESCRIPTION_HEADER   *Table;
> +  UINTN                         TableKey;
> +  UINTN                         TableIndex;
> +  EFI_ACPI_HANDLE               TableHandle, SegHandle;
> +  CHAR8                         Buffer[256];
> +  CHAR8                         *KB, *B;

Use proper names.

> +  EFI_ACPI_DATA_TYPE            DataType;
> +  UINTN                         DataSize, Mem32;
> +  RESOURCE                      *Rs;
> +  QWordMemory                   *Qm;

Use proper names.

> +  UINT8                         Segment;

This list of local variables is in itself a good hint that this
function should be split up into some helpers.

> +
> +  Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdtProtocol);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG_PCIE_ERR ("Unable to locate ACPI table protocol Guid\n");
> +    return Status;
> +  }
> +
> +  TableIndex = 0;
> +  Status = AcpiLocateTableBySignature (
> +             AcpiSdtProtocol,
> +             EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> +             &TableIndex,
> +             &Table,
> +             &TableKey
> +             );
> +  Status = AcpiSdtProtocol->OpenSdt (TableKey, &TableHandle);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG_PCIE_ERR ("Unable to open DSDT table\n");
> +    return Status;
> +  }
> +
> +  for (Idx = 0; PciSegEnabled[Idx] != -1; Idx++) {
> +    if (PciSegEnabled[Idx] > SOCKET0_LAST_RC) { /* Physical segment */
> +      break;
> +    }
> +    if (PciSegEnabled[Idx] < SOCKET0_FIRST_RC) {
> +      continue;
> +    }
> +
> +    /* DSDT PCI devices to use Physical segment */
> +    AsciiSPrint (Buffer, sizeof (Buffer), "\\_SB.PCI%x.RBUF", PciSegEnabled[Idx]);
> +    Status = AcpiSdtProtocol->FindPath (TableHandle, (VOID *)Buffer, &SegHandle);
> +    if (EFI_ERROR (Status)) {
> +      continue;
> +    }
> +
> +    for (Ix = 0; Ix < 3; Ix++) {
> +      Status = AcpiSdtProtocol->GetOption (
> +                                  SegHandle,
> +                                  Ix,
> +                                  &DataType,
> +                                  (VOID *)&B,
> +                                  &DataSize
> +                                  );
> +      KB = B;

OK, that reads KiloByte = Byte to me.
Please use proper variable names.

> +      if (EFI_ERROR (Status)) {
> +        continue;
> +      }
> +
> +      if (Ix == 0) { /* B[0] == AML_NAME_OP */
> +        if (!((DataSize == 1) && (DataType == EFI_ACPI_DATA_TYPE_OPCODE))) {
> +          break;
> +        }
> +      } else if (Ix == 1) { /* *B == "RBUF"  */
> +        if (!((DataSize == 4) && (DataType == EFI_ACPI_DATA_TYPE_NAME_STRING))) {
> +          break;
> +        }
> +      } else { /* Ix:2 11 42 07 0A 6E 88 ... */
> +        if (DataType != EFI_ACPI_DATA_TYPE_CHILD) {
> +          break;
> +        }
> +
> +        KB += 5; /* Point to Resource type */
> +        Rs = (RESOURCE *)KB;
> +        Mem32 = 0;
> +        while ((Mem32 == 0) && ((KB - B) < DataSize)) {
> +          if (Rs->ResourceType == ACPI_RESOURCE_NAME_ADDRESS16) {
> +            KB += (Rs->ResourceSize + 3); /* Type + Size */
> +            Rs = (RESOURCE *)KB;
> +          } else if (Rs->ResourceType == ACPI_RESOURCE_NAME_ADDRESS64) {
> +
> +            if (Rs->Attribute == 0x00) { /* The first QWordMemory */
> +              Mem32 = 1;
> +              Segment = PciSegEnabled[Idx] - 2;
> +              Qm = (QWordMemory *)&(Rs->ResourcePtr);
> +              *Qm = Qmem[Segment]; /* Physical segment */
> +            }
> +            KB += (Rs->ResourceSize + 3); /* Type + Size */
> +            Rs = (RESOURCE *)KB;
> +          }
> +        }
> +        if (Mem32 != 0) {
> +          Status = AcpiSdtProtocol->SetOption (
> +                                      SegHandle,
> +                                      Ix,
> +                                      (VOID *)B,
> +                                      DataSize
> +                                      );
> +        }
> +      }
> +    }
> +
> +    AcpiSdtProtocol->Close (SegHandle);
> +  }
> +
> +  AcpiSdtProtocol->Close (TableHandle);
> +  AcpiUpdateChecksum ((UINT8 *)Table, Table->Length);
> +
> +  return Status;
> +}
> +
> +VOID
> +ConstructMcfg (
> +  VOID   *McfgPtr,

It's a pointer, it doesn't have to be named Ptr.

> +  INT8   *PciSegEnabled,
> +  UINT32 McfgCount
> +  )
> +{
> +  EFI_MCFG_TABLE_CONFIG     McfgHeader = {
> +    {
> +      EFI_ACPI_6_1_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
> +      McfgCount,
> +      1,
> +      0x00,                        // Checksum will be updated at runtime
> +      EFI_ACPI_OEM_ID,
> +      EFI_ACPI_OEM_TABLE_ID,
> +      EFI_ACPI_MCFG_OEM_REVISION,
> +      EFI_ACPI_CREATOR_ID,
> +      EFI_ACPI_CREATOR_REVISION
> +    },
> +    0x0000000000000000,            // Reserved
> +  };
> +  EFI_MCFG_CONFIG_STRUCTURE TMcfg = {
> +    .ullBaseAddress = 0,
> +    .usSegGroupNum = 0,
> +    .ucStartBusNum = 0,
> +    .ucEndBusNum = 255,
> +    .Reserved2 = 0,
> +  };
> +  UINT32                    Idx;
> +  VOID                      *TMcfgPtr = McfgPtr;

What does T mean? CamelCase please.

> +  AC01_RC                   *Rc;
> +
> +  CopyMem (TMcfgPtr, &McfgHeader, sizeof (EFI_MCFG_TABLE_CONFIG));
> +  TMcfgPtr += sizeof (EFI_MCFG_TABLE_CONFIG);
> +  for (Idx = 0; PciSegEnabled[Idx] != -1; Idx++) {
> +    Rc = GetRCList (PciSegEnabled[Idx]); /* Logical */
> +    TMcfg.ullBaseAddress = Rc->MmcfgAddr;
> +    TMcfg.usSegGroupNum = Rc->Logical;
> +    CopyMem (TMcfgPtr, &TMcfg, sizeof (EFI_MCFG_CONFIG_STRUCTURE));
> +    TMcfgPtr += sizeof (EFI_MCFG_CONFIG_STRUCTURE);
> +  }
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +AcpiInstallMcfg (
> +  INT8 *PciSegEnabled
> +  )
> +{
> +  UINT32                  RcCount, McfgCount;
> +  EFI_ACPI_TABLE_PROTOCOL *AcpiSdtProtocol;
> +  UINTN                   TableKey;
> +  EFI_STATUS              Status;
> +  VOID                    *McfgPtr;

It's a pointer, it doesn't need to be called pointer.

> +
> +  Status = gBS->LocateProtocol (
> +                  &gEfiAcpiTableProtocolGuid,
> +                  NULL,
> +                  (VOID **)&AcpiSdtProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG_PCIE_ERR ("MCFG: Unable to locate ACPI table entry\n");
> +    return Status;
> +  }
> +  for (RcCount = 0; PciSegEnabled[RcCount] != -1; RcCount++) {
> +  }
> +  McfgCount = sizeof (EFI_MCFG_TABLE_CONFIG) + sizeof (EFI_MCFG_CONFIG_STRUCTURE) * RcCount;
> +  McfgPtr = AllocateZeroPool (McfgCount);
> +  if (McfgPtr == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  ConstructMcfg (McfgPtr, PciSegEnabled, McfgCount);
> +  Status = AcpiSdtProtocol->InstallAcpiTable (
> +                              AcpiSdtProtocol,
> +                              McfgPtr,
> +                              McfgCount,
> +                              &TableKey
> +                              );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG_PCIE_ERR ("MCFG: Unable to install MCFG table entry\n");
> +  }
> +  FreePool (McfgPtr);
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +ConstructIort (
> +  VOID   *IortPtr,
> +  UINT32 RcCount,
> +  UINT32 SmmuPmuAgentCount,
> +  UINT32 HeaderCount,
> +  INT8   *PciSegEnabled

Segment

> +  )
> +{
> +  EFI_ACPI_6_0_IO_REMAPPING_TABLE TIort = {
> +    .Header = __ACPI_HEADER (
> +                EFI_ACPI_6_0_IO_REMAPPING_TABLE_SIGNATURE,
> +                AC01_IO_REMAPPING_STRUCTURE,
> +                EFI_ACPI_IO_REMAPPING_TABLE_REVISION
> +                ),
> +    .NumNodes = (3 * RcCount) + SmmuPmuAgentCount,
> +    .NodeOffset = sizeof (EFI_ACPI_6_0_IO_REMAPPING_TABLE),
> +    0
> +  };
> +
> +  AC01_ITS_NODE TItsNode = {

What's T? CamelCase - write it out.
(Please address throughout.)

> +    .Node = {
> +      {
> +        EFI_ACPI_IORT_TYPE_ITS_GROUP,
> +        sizeof (EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE) + 4,
> +        0x0,
> +        0x0,
> +        0x0,
> +        0x0,
> +      },
> +      .NumItsIdentifiers = 1,
> +    },
> +    .ItsIdentifier = 1,
> +  };
> +
> +  AC01_RC_NODE TRcNode = {
> +    {
> +      {
> +        EFI_ACPI_IORT_TYPE_ROOT_COMPLEX,
> +        sizeof (AC01_RC_NODE),
> +        0x1,
> +        0x0,
> +        0x1,
> +        FIELD_OFFSET (AC01_RC_NODE, RcIdMapping),
> +      },
> +      EFI_ACPI_IORT_MEM_ACCESS_PROP_CCA,
> +      0x0,
> +      0x0,
> +      EFI_ACPI_IORT_MEM_ACCESS_FLAGS_CPM |
> +      EFI_ACPI_IORT_MEM_ACCESS_FLAGS_DACS,
> +      EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED,
> +      .PciSegmentNumber = 0,
> +      .MemoryAddressSize = 64,
> +    },
> +    __AC01_ID_MAPPING (0x0, 0xffff, 0x0, SmmuNode, 0),
> +  };
> +
> +  AC01_SMMU_NODE TSmmuNode = {
> +    {
> +      {
> +        EFI_ACPI_IORT_TYPE_SMMUv3,
> +        sizeof (AC01_SMMU_NODE),
> +        0x2,  /* Revision */
> +        0x0,
> +        0x2,  /* Mapping Count */
> +        FIELD_OFFSET (AC01_SMMU_NODE, InterruptMsiMapping),
> +      },
> +      .Base = 0,
> +      EFI_ACPI_IORT_SMMUv3_FLAG_COHAC_OVERRIDE,
> +      0,
> +      0,
> +      0,
> +      0,
> +      0,
> +      0x0,
> +      0x0,
> +      0,  // Proximity domain - need fill in
> +      .DeviceIdMapping = 1,
> +    },
> +    __AC01_ID_MAPPING (0x0, 0xffff, 0, SmmuNode, 0),
> +    __AC01_ID_MAPPING (0x0, 0x1, 0, SmmuNode, 1),
> +  };
> +
> +  EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE TPmcgNode = {
> +    {
> +      EFI_ACPI_IORT_TYPE_PMCG,
> +      sizeof (EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE),
> +      0x1,
> +      0x0,
> +      0x0,
> +      0x0,
> +    },
> +    0, /* Page 0 Base. Need to be filled */
> +    0, /* GSIV. Need to be filled */
> +    0, /* Node reference. Need to be filled */
> +    0, /* Page 1 Base. Need to be filled. */
> +  };
> +
> +  UINT32  Idx, Idx1, SmmuNodeOffset[AC01_MAX_PCIE_ROOT_COMPLEX];
> +  VOID    *TIortPtr = IortPtr, *SmmuPtr, *PmcgPtr;
> +  UINT32  ItsOffset[AC01_MAX_PCIE_ROOT_COMPLEX];
> +  AC01_RC *Rc;
> +  UINTN   NumTbuPmu;

This splat serves as a good pointer that this should probably be more
than one function.

> +
> +  TIort.Header.Length = HeaderCount;
> +  CopyMem (TIortPtr, &TIort, sizeof (EFI_ACPI_6_0_IO_REMAPPING_TABLE));
> +  TIortPtr += sizeof (EFI_ACPI_6_0_IO_REMAPPING_TABLE);
> +  for (Idx = 0; Idx < RcCount; Idx++) {
> +    ItsOffset[Idx] = TIortPtr - IortPtr;
> +    TItsNode.ItsIdentifier = PciSegEnabled[Idx]; /* Physical */
> +    CopyMem (TIortPtr, &TItsNode, sizeof (AC01_ITS_NODE));
> +    TIortPtr += sizeof (AC01_ITS_NODE);
> +  }
> +
> +  SmmuPtr = TIortPtr + RcCount * sizeof (AC01_RC_NODE);
> +  PmcgPtr = SmmuPtr + RcCount * sizeof (AC01_SMMU_NODE);
> +  for (Idx = 0; Idx < RcCount; Idx++) {
> +    SmmuNodeOffset[Idx] = SmmuPtr - IortPtr;
> +    Rc = GetRCList (PciSegEnabled[Idx]); /* Physical RC */
> +    TSmmuNode.Node.Base = Rc->TcuAddr;
> +    TSmmuNode.InterruptMsiMapping.OutputBase = PciSegEnabled[Idx] << 16;
> +    TSmmuNode.InterruptMsiMapping.OutputReference = ItsOffset[Idx];
> +    TSmmuNode.InterruptMsiMappingSingle.OutputBase = PciSegEnabled[Idx] << 16;
> +    TSmmuNode.InterruptMsiMappingSingle.OutputReference = ItsOffset[Idx];
> +    /* All RCs on master be assigned to node 0, while remote RCs be assigned to first remote node */
> +    TSmmuNode.Node.Flags = EFI_ACPI_IORT_SMMUv3_FLAG_PROXIMITY_DOMAIN;
> +    TSmmuNode.Node.ProximityDomain = 0;
> +    if ((Rc->TcuAddr & SLAVE_SOCKET_BASE_ADDRESS_OFFSET) != 0) {
> +      /* RC on remote socket */
> +      TSmmuNode.Node.Flags = EFI_ACPI_IORT_SMMUv3_FLAG_PROXIMITY_DOMAIN;
> +      switch (CpuGetSubNumaMode ()) {
> +      case SUBNUMA_MODE_MONOLITHIC:
> +        TSmmuNode.Node.ProximityDomain += MONOLITIC_NUM_OF_REGION;
> +        break;
> +      case SUBNUMA_MODE_HEMISPHERE:
> +        TSmmuNode.Node.ProximityDomain += HEMISPHERE_NUM_OF_REGION;
> +        break;
> +      case SUBNUMA_MODE_QUADRANT:
> +        TSmmuNode.Node.ProximityDomain += QUADRANT_NUM_OF_REGION;
> +        break;
> +      }
> +    }
> +    CopyMem (SmmuPtr, &TSmmuNode, sizeof (AC01_SMMU_NODE));
> +    SmmuPtr += sizeof (AC01_SMMU_NODE);
> +
> +    if (!SmmuPmuAgentCount) {
> +      continue;
> +    }
> +
> +    /* Add PMCG nodes */
> +    if (Rc->Type == RCA) {
> +      NumTbuPmu = RCA_NUM_TBU_PMU;
> +    } else {
> +      NumTbuPmu = RCB_NUM_TBU_PMU;
> +    }
> +    for (Idx1 = 0; Idx1 < NumTbuPmu; Idx1++) {

This for loop should be a function, and quite possibly each of the
case statements should be too.

> +      TPmcgNode.Base = Rc->TcuAddr;
> +      if (NumTbuPmu == RCA_NUM_TBU_PMU) {
> +        switch (Idx1) {
> +        case 0:
> +          TPmcgNode.Base += 0x40000;
> +          break;
> +
> +        case 1:
> +          TPmcgNode.Base += 0x60000;
> +          break;
> +
> +        case 2:
> +          TPmcgNode.Base += 0xA0000;
> +          break;
> +
> +        case 3:
> +          TPmcgNode.Base += 0xE0000;
> +          break;
> +
> +        case 4:
> +          TPmcgNode.Base += 0x100000;
> +          break;
> +
> +        case 5:
> +          TPmcgNode.Base += 0x140000;
> +          break;
> +        }
> +      } else {
> +        switch (Idx1) {
> +        case 0:
> +          TPmcgNode.Base += 0x40000;
> +          break;
> +
> +        case 1:
> +          TPmcgNode.Base += 0x60000;
> +          break;
> +
> +        case 2:
> +          TPmcgNode.Base += 0xA0000;
> +          break;
> +
> +        case 3:
> +          TPmcgNode.Base += 0xE0000;
> +          break;
> +
> +        case 4:
> +          TPmcgNode.Base += 0x120000;
> +          break;
> +
> +        case 5:
> +          TPmcgNode.Base += 0x160000;
> +          break;
> +
> +        case 6:
> +          TPmcgNode.Base += 0x180000;
> +          break;
> +
> +        case 7:
> +          TPmcgNode.Base += 0x1C0000;
> +          break;
> +
> +        case 8:
> +          TPmcgNode.Base += 0x200000;
> +          break;
> +
> +        case 9:
> +          TPmcgNode.Base += 0x240000;
> +          break;
> +        }
> +      }
> +      TPmcgNode.Page1Base = TPmcgNode.Base + 0x12000;
> +      TPmcgNode.Base += 0x2000;

For the case statements there might be an argument made that the
live-coded values would not be more readable by turning them into
#defines. (I wouldn't agree, but the case could be made.)
No such argument can be made here. Please provide some helpfully named
#defines for those values.

> +      TPmcgNode.NodeReference = SmmuNodeOffset[Idx];
> +      TPmcgNode.OverflowInterruptGsiv = gTbuPmuIrqArray[PciSegEnabled[Idx]] + Idx1;
> +      CopyMem (PmcgPtr, &TPmcgNode, sizeof (TPmcgNode));
> +      PmcgPtr += sizeof (TPmcgNode);
> +    }
> +
> +    /* TCU PMCG */

A comment should be instantly distinguishable from a cat having walked
on the keyboard.

> +    TPmcgNode.Base = Rc->TcuAddr;
> +    TPmcgNode.Base += 0x2000;
> +    TPmcgNode.Page1Base = Rc->TcuAddr + 0x12000;

And those ones, with interestingly an identical pair of live-coded values.

> +    TPmcgNode.NodeReference = SmmuNodeOffset[Idx];
> +    TPmcgNode.OverflowInterruptGsiv = gTcuPmuIrqArray[PciSegEnabled[Idx]];
> +    CopyMem (PmcgPtr, &TPmcgNode, sizeof (TPmcgNode));
> +    PmcgPtr += sizeof (TPmcgNode);
> +  }
> +
> +  for (Idx = 0; Idx < RcCount; Idx++) {
> +    TRcNode.Node.PciSegmentNumber = GetRCList (PciSegEnabled[Idx])->Logical; /* Logical */

That comment serves no purpose. Improve it or delete it.

/
    Leif

> +    TRcNode.RcIdMapping.OutputReference = SmmuNodeOffset[Idx];
> +    CopyMem (TIortPtr, &TRcNode, sizeof (AC01_RC_NODE));
> +    TIortPtr += sizeof (AC01_RC_NODE);
> +  }
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +AcpiInstallIort (
> +  INT8 *PciSegEnabled
> +  )
> +{
> +  EFI_STATUS              Status;
> +  EFI_ACPI_TABLE_PROTOCOL *AcpiSdtProtocol;
> +  UINTN                   TableKey;
> +  VOID                    *IortPtr;
> +  BOOLEAN                 IsSmmuPmuEnabled;
> +  UINT32                  RcCount, SmmuPmuAgentCount, TotalCount;
> +
> +  Status = gBS->LocateProtocol (
> +                  &gEfiAcpiTableProtocolGuid,
> +                  NULL,
> +                  (VOID **)&AcpiSdtProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG_PCIE_ERR ("IORT: Unable to locate ACPI table entry\n");
> +    return Status;
> +  }
> +
> +  for (RcCount = 0, SmmuPmuAgentCount = 0; PciSegEnabled[RcCount] != -1; RcCount++) {
> +    if ((GetRCList (PciSegEnabled[RcCount]))->Type == RCA) {
> +      SmmuPmuAgentCount += RCA_NUM_TBU_PMU;
> +    } else {
> +      SmmuPmuAgentCount += RCB_NUM_TBU_PMU;
> +    }
> +    SmmuPmuAgentCount += 1; /* Only 1 TCU */
> +  }
> +
> +  BoardPcieCheckSmmuPmuEnabled (&IsSmmuPmuEnabled);
> +  if (!IsSmmuPmuEnabled) {
> +    SmmuPmuAgentCount = 0;
> +  }
> +
> +  TotalCount = sizeof (EFI_ACPI_6_0_IO_REMAPPING_TABLE) +
> +               RcCount * (sizeof (AC01_ITS_NODE) + sizeof (AC01_RC_NODE) + sizeof (AC01_SMMU_NODE)) +
> +               SmmuPmuAgentCount * sizeof (EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE);
> +
> +  IortPtr = AllocateZeroPool (TotalCount);
> +  if (IortPtr == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  ConstructIort (IortPtr, RcCount, SmmuPmuAgentCount, TotalCount, PciSegEnabled);
> +
> +  Status = AcpiSdtProtocol->InstallAcpiTable (
> +                              AcpiSdtProtocol,
> +                              IortPtr,
> +                              TotalCount,
> +                              &TableKey
> +                              );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG_PCIE_ERR ("IORT: Unable to install IORT table entry\n");
> +  }
> +  FreePool (IortPtr);
> +  return Status;
> +}
> -- 
> 2.17.1
> 

  reply	other threads:[~2021-09-23 13:49 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 15:54 [PATCH v3 00/28] Add new Ampere Mt. Jade platform Nhi Pham
2021-09-15 15:55 ` [PATCH v3 01/28] Ampere: Initial support for Ampere Altra processor and " Nhi Pham
2021-09-16 10:40   ` Leif Lindholm
2021-09-16 10:46     ` Leif Lindholm
2021-09-17  6:19       ` Nhi Pham
2021-09-23 13:54   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 02/28] AmpereAltraPkg: Add MmCommunication modules Nhi Pham
2021-09-15 15:55 ` [PATCH v3 03/28] AmperePlatformPkg: Implement FailSafe library Nhi Pham
2021-09-15 15:55 ` [PATCH v3 04/28] AmperePlatformPkg: Add FailSafe and WDT support Nhi Pham
2021-09-16 11:22   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 05/28] AmpereAltraPkg: Add DwI2cLib library Nhi Pham
2021-09-15 15:55 ` [PATCH v3 06/28] AmpereAltraPkg: Add DwGpioLib library Nhi Pham
2021-09-15 15:55 ` [PATCH v3 07/28] JadePkg: Implement RealTimeClockLib for PCF85063 Nhi Pham
2021-09-15 15:55 ` [PATCH v3 08/28] AmpereAltraPkg: Add BootProgress support Nhi Pham
2021-09-15 15:55 ` [PATCH v3 09/28] AmpereAltraPkg: Support UEFI non-volatile variable Nhi Pham
2021-09-23 11:49   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 10/28] AmpereSiliconPkg: Add PlatformManagerUiLib library instance Nhi Pham
2021-09-15 15:55 ` [PATCH v3 11/28] AmpereAltraPkg, JadePkg: Add ACPI support Nhi Pham
2021-09-15 15:55 ` [PATCH v3 12/28] AmpereAltraPkg: Add Ac01PcieLib library instance Nhi Pham
2021-09-23 13:49   ` Leif Lindholm [this message]
2021-09-27 14:33     ` Nhi Pham
2021-10-04 12:03     ` Nhi Pham
2021-10-05 19:59       ` Leif Lindholm
2021-10-06 12:55         ` [edk2-devel] " Nhi Pham
2021-10-07  9:06           ` Leif Lindholm
2021-10-07  9:13             ` Nhi Pham
2021-09-28 10:34   ` Leif Lindholm
2021-10-04 11:36     ` Nhi Pham
2021-09-15 15:55 ` [PATCH v3 13/28] JadePkg: Add BoardPcieLib " Nhi Pham
2021-09-24 12:35   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 14/28] Ampere: PCIe: Add PciHostBridgeLib " Nhi Pham
2021-09-24 13:12   ` Ard Biesheuvel
2021-09-15 15:55 ` [PATCH v3 15/28] Ampere: PCIe: Add PciSegmentLib " Nhi Pham
2021-09-24 13:16   ` Ard Biesheuvel
2021-09-28 10:26     ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 16/28] JadePkg: Enable PCIe-related libraries and device drivers Nhi Pham
2021-09-15 15:55 ` [PATCH v3 17/28] JadePkg: Add ASpeed GOP driver Nhi Pham
2021-09-15 15:55 ` [PATCH v3 18/28] Ampere: PCIe: Add PciPlatformDxe driver Nhi Pham
2021-09-24 12:59   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 19/28] AmpereAltraPkg: Add Random Number Generator Support Nhi Pham
2021-09-15 15:55 ` [PATCH v3 20/28] JadePkg: Add SMBIOS tables support Nhi Pham
2021-09-15 15:55 ` [PATCH v3 21/28] AmpereAltraPkg: Add DebugInfoPei module Nhi Pham
2021-09-24 12:43   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 22/28] AmpereAltraPkg: Add platform info screen Nhi Pham
2021-09-15 15:55 ` [PATCH v3 23/28] AmpereAltraPkg: Add configuration screen for memory Nhi Pham
2021-09-24 12:50   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 24/28] AmpereAltraPkg: Add configuration screen for CPU Nhi Pham
2021-09-24 12:51   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 25/28] AmpereAltraPkg: Add configuration screen for ACPI Nhi Pham
2021-09-24 12:53   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 26/28] AmpereAltraPkg: Add configuration screen for RAS Nhi Pham
2021-09-24 12:54   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 27/28] AmpereAltraPkg: Add configuration screen for Watchdog timer Nhi Pham
2021-09-24 12:55   ` Leif Lindholm
2021-09-15 15:55 ` [PATCH v3 28/28] AmpereAltraPkg: Add configuration screen for Pcie Devices Nhi Pham
2021-09-24 12:57   ` Leif Lindholm
2021-09-16 10:09 ` [PATCH v3 00/28] Add new Ampere Mt. Jade platform Leif Lindholm
2021-09-17  6:10   ` Nhi Pham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210923134931.gzxueebb4axflhdi@leviathan \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox