From: Vu Nguyen <vunguyen@os.amperecomputing.com>
This updates the PCIe modules to add support for
Ampere Altra Max processor which features 128 PCIe
Gen4 lanes (distributed across eight x16 RCAs) using
32 controllers.
Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
---
.../AmpereAltraPkg/Include/NVParamDef.h | 64 ++++++--
.../Library/Ac01PcieLib/PcieCore.h | 8 +-
.../Library/BoardPcieLib/BoardPcieLib.c | 63 +++++++-
.../Drivers/PcieInitPei/PcieInitPei.c | 16 +-
.../Drivers/PcieInitPei/RootComplexNVParam.c | 101 +++++++++---
.../Library/Ac01PcieLib/PcieCore.c | 150 +++++++++++++++---
6 files changed, 333 insertions(+), 69 deletions(-)
diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h b/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
index 3259fa1ea45c..4dfc353d2340 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
+++ b/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
@@ -29,7 +29,7 @@
As each non-volatile parameter requires 8 bytes, there is a total of 8K
parameters.
- Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+ Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -425,10 +425,10 @@
#define NV_SI_RO_BOARD_S0_RCA5_CFG ((107 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
#define NV_SI_RO_BOARD_S0_RCA6_CFG ((108 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
#define NV_SI_RO_BOARD_S0_RCA7_CFG ((109 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020003 */
-#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET ((110 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET ((111 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET ((112 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET ((113 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET ((110 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET ((111 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET ((112 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET ((113 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
#define NV_SI_RO_BOARD_S0_RCB0A_TXRX_G3PRESET ((114 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S0_RCB0B_TXRX_G3PRESET ((115 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S0_RCB1A_TXRX_G3PRESET ((116 * 8) + NV_BOARD_PARAM_START)
@@ -437,10 +437,10 @@
#define NV_SI_RO_BOARD_S0_RCB2B_TXRX_G3PRESET ((119 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S0_RCB3A_TXRX_G3PRESET ((120 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S0_RCB3B_TXRX_G3PRESET ((121 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET ((122 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET ((123 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET ((124 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET ((125 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET ((122 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET ((123 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET ((124 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET ((125 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET ((126 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G4PRESET ((127 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G4PRESET ((128 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
@@ -462,8 +462,8 @@
#define NV_SI_RO_BOARD_S1_RCA5_CFG ((144 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
#define NV_SI_RO_BOARD_S1_RCA6_CFG ((145 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
#define NV_SI_RO_BOARD_S1_RCA7_CFG ((146 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020003 */
-#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET ((147 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET ((148 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET ((147 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET ((148 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
#define NV_SI_RO_BOARD_S1_RCB0A_TXRX_G3PRESET ((149 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S1_RCB0B_TXRX_G3PRESET ((150 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S1_RCB1A_TXRX_G3PRESET ((151 * 8) + NV_BOARD_PARAM_START)
@@ -472,10 +472,10 @@
#define NV_SI_RO_BOARD_S1_RCB2B_TXRX_G3PRESET ((154 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S1_RCB3A_TXRX_G3PRESET ((155 * 8) + NV_BOARD_PARAM_START)
#define NV_SI_RO_BOARD_S1_RCB3B_TXRX_G3PRESET ((156 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET ((157 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET ((158 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET ((159 * 8) + NV_BOARD_PARAM_START)
-#define NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET ((160 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET ((157 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET ((158 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET ((159 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET ((160 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET ((161 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G4PRESET ((162 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
#define NV_SI_RO_BOARD_S1_RCB0A_TXRX_G4PRESET ((163 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
@@ -523,7 +523,39 @@
#define NV_SI_RO_BOARD_RAS_DDR_CE_TH1 ((205 * 8) + NV_BOARD_PARAM_START) /* Default: 0x000001F4 */
#define NV_SI_RO_BOARD_RAS_DDR_CE_TH2 ((206 * 8) + NV_BOARD_PARAM_START) /* Default: 0x00001388 */
#define NV_SI_RO_BOARD_RAS_DDR_CE_THC ((207 * 8) + NV_BOARD_PARAM_START) /* Default: 0x00000000 */
-#define NV_PMPRO_REGION4_LOAD_END (NV_SI_RO_BOARD_RAS_DDR_CE_THC)
+#define NV_SI_RO_BOARD_MQ_SX_RCA0_TXRX_20GPRESET ((208 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_MQ_SX_RCA1_TXRX_20GPRESET ((209 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_MQ_SX_RCA0_TXRX_25GPRESET ((210 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_MQ_SX_RCA1_TXRX_25GPRESET ((211 * 8) + NV_BOARD_PARAM_START)
+#define NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET ((212 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA1_TXRX_G3PRESET ((213 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA2_TXRX_G3PRESET ((214 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA3_TXRX_G3PRESET ((215 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET ((216 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA5_TXRX_G3PRESET ((217 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA6_TXRX_G3PRESET ((218 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA7_TXRX_G3PRESET ((219 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET ((220 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S1_RCA3_TXRX_G3PRESET ((221 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET ((222 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S1_RCA5_TXRX_G3PRESET ((223 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S1_RCA6_TXRX_G3PRESET ((224 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G3PRESET ((225 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
+#define NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET ((226 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA1_TXRX_G4PRESET ((227 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA2_TXRX_G4PRESET ((228 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA3_TXRX_G4PRESET ((229 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET ((230 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA5_TXRX_G4PRESET ((231 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA6_TXRX_G4PRESET ((232 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S0_RCA7_TXRX_G4PRESET ((233 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET ((234 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S1_RCA3_TXRX_G4PRESET ((235 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET ((236 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S1_RCA5_TXRX_G4PRESET ((237 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S1_RCA6_TXRX_G4PRESET ((238 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G4PRESET ((239 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
+#define NV_PMPRO_REGION4_LOAD_END (NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G4PRESET)
//
// NOTE: Add before NV_BOARD_PARAM_MAX and increase its value
//
diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
index 1db8a68b3df4..a18fff7dbb75 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
@@ -1,6 +1,6 @@
/** @file
- Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+ Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -39,6 +39,7 @@
#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 EP_LINKUP_EXTRA_TIMEOUT (500 * 1000) // 500ms
#define LINK_WAIT_INTERVAL_US 50
#define PFA_MODE_ENABLE 0
@@ -80,6 +81,7 @@
#define AC01_PCIE_CORE_IRQ_ENABLE_REG 0x30
#define AC01_PCIE_CORE_IRQ_EVENT_STAT_REG 0x38
#define AC01_PCIE_CORE_BLOCK_EVENT_STAT_REG 0x3C
+#define AC01_PCIE_CORE_BUS_CONTROL_REG 0x40
#define AC01_PCIE_CORE_RESET_REG 0xC000
#define AC01_PCIE_CORE_CLOCK_REG 0xC004
#define AC01_PCIE_CORE_MEM_READY_REG 0xC104
@@ -87,6 +89,7 @@
// AC01_PCIE_CORE_LINK_CTRL_REG
#define LTSSMENB_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
+#define LTSSMENB_GET(dst) ((dst) & (BIT0))
#define HOLD_LINK_TRAINING 0
#define START_LINK_TRAINING 1
#define DEVICETYPE_SET(dst, src) (((dst) & ~0xF0) | (((UINT32) (src) << 4) & 0xF0))
@@ -120,6 +123,9 @@
// AC01_PCIE_CORE_BLOCK_EVENT_STAT_REG
#define LINKUP_MASK 0x1
+// AC01_PCIE_CORE_BUS_CONTROL_REG
+#define BUS_CTL_CFG_UR_MASK 0x8
+
// AC01_PCIE_CORE_RESET_REG
#define DWC_PCIE_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
#define RESET_MASK 0x1
diff --git a/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c b/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
index f49764097219..4e0ba551e09b 100644
--- a/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
+++ b/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
@@ -2,7 +2,7 @@
Pcie board specific driver to handle asserting PERST signal to Endpoint
card. PERST asserting is via group of GPIO pins to CPLD as Platform Specification.
- Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+ Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -20,6 +20,8 @@
#define RCB_MAX_PERST_GROUPVAL 46
#define DEFAULT_SEGMENT_NUMBER 0x0F
+#define PCIE_PERST_DELAY (100 * 1000) // 100ms
+
VOID
BoardPcieReleaseAllPerst (
IN UINT8 SocketId
@@ -32,6 +34,8 @@ BoardPcieReleaseAllPerst (
for (GpioIndex = 0; GpioIndex < 6; GpioIndex++) {
GpioModeConfig (GpioPin + GpioIndex, GpioConfigOutHigh);
}
+
+ MicroSecondDelay (PCIE_PERST_DELAY);
}
/**
@@ -56,11 +60,54 @@ BoardPcieAssertPerst (
if (!IsPullToHigh) {
if (RootComplex->Type == RootComplexTypeA) {
- //
- // RootComplexTypeA: RootComplex->ID: 0->3 ; PcieIndex: 0->3
- //
- GpioGroupVal = RCA_MAX_PERST_GROUPVAL - PcieIndex
- - RootComplex->ID * MaxPcieControllerOfRootComplexA;
+ if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
+ /* Ampere Altra: 4 */
+ //
+ // RootComplexTypeA: RootComplex->ID: 0->3 ; PcieIndex: 0->3
+ //
+ GpioGroupVal = RCA_MAX_PERST_GROUPVAL - PcieIndex
+ - RootComplex->ID * MaxPcieControllerOfRootComplexA;
+ } else {
+ /* Ampere Altra Max RootComplex->ID: 4:7 */
+ if (PcieIndex < 2) {
+ switch (RootComplex->ID) {
+ case 4:
+ GpioGroupVal = 34 - (PcieIndex * 2);
+ break;
+ case 5:
+ GpioGroupVal = 38 - (PcieIndex * 2);
+ break;
+ case 6:
+ GpioGroupVal = 30 - (PcieIndex * 2);
+ break;
+ case 7:
+ GpioGroupVal = 26 - (PcieIndex * 2);
+ break;
+ }
+ } else {
+ /* PcieIndex 2:3 */
+ switch (RootComplex->ID) {
+ case 4:
+ GpioGroupVal = 46 - ((PcieIndex - 2) * 2);
+ break;
+
+ case 5:
+ GpioGroupVal = 42 - ((PcieIndex - 2) * 2);
+ break;
+
+ case 6:
+ GpioGroupVal = 18 - ((PcieIndex - 2) * 2);
+ break;
+
+ case 7:
+ GpioGroupVal = 22 - ((PcieIndex - 2) * 2);
+ break;
+
+ default:
+ DEBUG ((DEBUG_ERROR, "Invalid Root Complex ID %d\n", RootComplex->ID));
+ }
+ }
+ }
} else {
//
// RootComplexTypeB: RootComplex->ID: 4->7 ; PcieIndex: 0->7
@@ -81,7 +128,7 @@ BoardPcieAssertPerst (
}
// Keep reset as low as 100 ms as specification
- MicroSecondDelay (100 * 1000);
+ MicroSecondDelay (PCIE_PERST_DELAY);
} else {
BoardPcieReleaseAllPerst (RootComplex->Socket);
}
@@ -113,5 +160,5 @@ BoardPcieGetSegmentNumber (
return Ac01BoardSegment[RootComplex->Socket][RootComplex->ID];
}
- return DEFAULT_SEGMENT_NUMBER;
+ return (RootComplex->ID - 2);
}
diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
index 17f6112ea207..a69193b07005 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
+++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
@@ -1,6 +1,6 @@
/** @file
- Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+ Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -94,7 +94,7 @@ BuildRootComplexData (
RootComplex = &mRootComplexList[RCIndex];
RootComplex->Active = ConfigFound ? RootComplexConfig.RCStatus[RCIndex] : TRUE;
RootComplex->DevMapLow = ConfigFound ? RootComplexConfig.RCBifurcationLow[RCIndex] : 0;
- RootComplex->DevMapHigh = ConfigFound ? RootComplexConfig.RCBifurcationLow[RCIndex] : 0;
+ RootComplex->DevMapHigh = ConfigFound ? RootComplexConfig.RCBifurcationHigh[RCIndex] : 0;
RootComplex->Socket = RCIndex / AC01_PCIE_MAX_RCS_PER_SOCKET;
RootComplex->ID = RCIndex % AC01_PCIE_MAX_RCS_PER_SOCKET;
RootComplex->CsrBase = mCsrBase[RCIndex];
@@ -106,7 +106,12 @@ BuildRootComplexData (
RootComplex->MmioSize = mMmioSize[RCIndex];
RootComplex->Mmio32Base = mMmio32Base[RCIndex];
RootComplex->Mmio32Size = mMmio32Size[RCIndex];
- RootComplex->Type = (RootComplex->ID < MaxRootComplexA) ? RootComplexTypeA : RootComplexTypeB;
+ if (IsAc01Processor ()) {
+ RootComplex->Type = (RootComplex->ID < MaxRootComplexA) ? RootComplexTypeA : RootComplexTypeB;
+ } else {
+ RootComplex->Type = RootComplexTypeA;
+ }
+
RootComplex->MaxPcieController = (RootComplex->Type == RootComplexTypeB)
? MaxPcieControllerOfRootComplexB : MaxPcieControllerOfRootComplexA;
RootComplex->Logical = BoardPcieGetSegmentNumber (RootComplex);
@@ -168,11 +173,14 @@ PcieInitEntry (
continue;
}
+ DEBUG ((DEBUG_INIT, "Initializing S%d-RC%d...", RootComplex->Socket, RootComplex->ID));
Status = Ac01PcieCoreSetupRC (RootComplex, FALSE, 0);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "RootComplex[%d]: Init Failed\n", Index));
+ DEBUG ((DEBUG_ERROR, "Failed\n"));
RootComplex->Active = FALSE;
continue;
+ } else {
+ DEBUG ((DEBUG_INIT, "Done + DevMapLow/High: %d/%d\n", RootComplex->DevMapLow, RootComplex->DevMapHigh));
}
}
diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
index aa34a90b44c6..1346fa616ab3 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
+++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
@@ -37,7 +37,7 @@
| Y | Y | Y | Y | 3 |
----------------------------------------
- Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+ Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -149,6 +149,10 @@ GetDefaultDevMap (
AC01_ROOT_COMPLEX *RootComplex
)
{
+ BOOLEAN IsAc01;
+
+ IsAc01 = IsAc01Processor ();
+
if (RootComplex->Pcie[PcieController0].Active
&& RootComplex->Pcie[PcieController1].Active
&& RootComplex->Pcie[PcieController2].Active
@@ -169,14 +173,20 @@ GetDefaultDevMap (
&& RootComplex->Pcie[PcieController5].Active
&& RootComplex->Pcie[PcieController6].Active
&& RootComplex->Pcie[PcieController7].Active) {
- RootComplex->DefaultDevMapHigh = DevMapMode4;
+ if (IsAc01) {
+ RootComplex->DefaultDevMapHigh = DevMapMode4;
+ }
} else if (RootComplex->Pcie[PcieController4].Active
&& RootComplex->Pcie[PcieController6].Active
&& RootComplex->Pcie[PcieController7].Active) {
- RootComplex->DefaultDevMapHigh = DevMapMode3;
+ if (IsAc01) {
+ RootComplex->DefaultDevMapHigh = DevMapMode3;
+ }
} else if (RootComplex->Pcie[PcieController4].Active
&& RootComplex->Pcie[PcieController6].Active) {
- RootComplex->DefaultDevMapHigh = DevMapMode2;
+ if (IsAc01) {
+ RootComplex->DefaultDevMapHigh = DevMapMode2;
+ }
} else {
RootComplex->DefaultDevMapHigh = DevMapMode1;
}
@@ -203,12 +213,17 @@ GetLaneAllocation (
EFI_STATUS Status;
INTN RPIndex;
NVPARAM NvParamOffset;
- UINT32 Value, Width;
+ UINT32 Value, Width, Controller;
// Retrieve lane allocation and capabilities for each controller
if (RootComplex->Type == RootComplexTypeA) {
- NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
- NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
+ NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
+ NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA4_CFG : NV_SI_RO_BOARD_S1_RCA4_CFG;
+ NvParamOffset += (RootComplex->ID - MaxPcieControllerOfRootComplexA) * NV_PARAM_ENTRYSIZE;
+ }
} else {
//
// There're two NVParam entries per RootComplexTypeB
@@ -222,7 +237,13 @@ GetLaneAllocation (
Value = 0;
}
- for (RPIndex = 0; RPIndex < MaxPcieControllerOfRootComplexA; RPIndex++) {
+ if (IsAc01Processor ()) {
+ Controller = MaxPcieControllerOfRootComplexA;
+ } else {
+ Controller = RootComplex->MaxPcieController;
+ }
+
+ for (RPIndex = PcieController0; RPIndex < Controller; RPIndex++) {
Width = (Value >> (RPIndex * BITS_PER_BYTE)) & BYTE_MASK;
switch (Width) {
case 1:
@@ -245,7 +266,7 @@ GetLaneAllocation (
if (RootComplex->Type == RootComplexTypeB) {
NvParamOffset += NV_PARAM_ENTRYSIZE;
- Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
+ Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
if (EFI_ERROR (Status)) {
Value = 0;
}
@@ -288,9 +309,17 @@ GetGen3PresetNvParamOffset (
if (RootComplex->Socket == 0) {
if (RootComplex->Type == RootComplexTypeA) {
if (RootComplex->ID < MaxRootComplexA) {
- NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ }
} else {
- NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ }
}
} else {
//
@@ -300,9 +329,17 @@ GetGen3PresetNvParamOffset (
}
} else if (RootComplex->Type == RootComplexTypeA) {
if (RootComplex->ID < MaxRootComplexA) {
- NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
+ }
} else {
- NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ }
}
} else {
//
@@ -324,9 +361,17 @@ GetGen4PresetNvParamOffset (
if (RootComplex->Socket == 0) {
if (RootComplex->Type == RootComplexTypeA) {
if (RootComplex->ID < MaxRootComplexA) {
- NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
+ }
} else {
- NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ }
}
} else {
//
@@ -336,9 +381,17 @@ GetGen4PresetNvParamOffset (
}
} else if (RootComplex->Type == RootComplexTypeA) {
if (RootComplex->ID < MaxRootComplexA) {
- NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
+ }
} else {
- NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ if (IsAc01Processor ()) {
+ NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ } else {
+ NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
+ }
}
} else {
//
@@ -352,7 +405,7 @@ GetGen4PresetNvParamOffset (
VOID
GetPresetSetting (
- AC01_ROOT_COMPLEX *RootComplex
+ AC01_ROOT_COMPLEX *RootComplex
)
{
EFI_STATUS Status;
@@ -377,7 +430,7 @@ GetPresetSetting (
if (RootComplex->Type == RootComplexTypeB) {
NvParamOffset += NV_PARAM_ENTRYSIZE;
- Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
+ Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
if (!EFI_ERROR (Status)) {
for (Index = MaxPcieControllerOfRootComplexA; Index < MaxPcieController; Index++) {
RootComplex->PresetGen3[Index] = (Value >> ((Index - MaxPcieControllerOfRootComplexA) * BITS_PER_BYTE)) & BYTE_MASK;
@@ -414,7 +467,7 @@ GetMaxSpeedGen (
UINT8 ErrataSpeedDevMap3[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN4, LINK_SPEED_GEN4, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 2: x8 x4 x4 (PCIE_ERRATA_SPEED1)
UINT8 ErrataSpeedDevMap4[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 3: x4 x4 x4 x4 (PCIE_ERRATA_SPEED1)
UINT8 ErrataSpeedRcb[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // RootComplexTypeB PCIE_ERRATA_SPEED1
- UINT8 Idx;
+ UINT8 Idx, Controller;
UINT8 *MaxGen;
ASSERT (MaxPcieControllerOfRootComplexA == 4);
@@ -452,7 +505,13 @@ GetMaxSpeedGen (
}
}
- for (Idx = 0; Idx < MaxPcieControllerOfRootComplexA; Idx++) {
+ if (IsAc01Processor ()) {
+ Controller = MaxPcieControllerOfRootComplexA;
+ } else {
+ Controller = RootComplex->MaxPcieController;
+ }
+
+ for (Idx = 0; Idx < Controller; Idx++) {
RootComplex->Pcie[Idx].MaxGen = RootComplex->Pcie[Idx].Active ? MaxGen[Idx] : LINK_SPEED_NONE;
}
diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
index ad648b1b9efd..12bd2c14544e 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
@@ -1,6 +1,6 @@
/** @file
- Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+ Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -10,8 +10,10 @@
#include <Guid/PlatformInfoHob.h>
#include <Guid/RootComplexInfoHob.h>
+#include <IndustryStandard/Pci.h>
#include <Library/ArmGenericTimerCounterLib.h>
#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/BoardPcieLib.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
@@ -22,6 +24,25 @@
#include "PcieCore.h"
+VOID
+EnableDbiAccess (
+ AC01_ROOT_COMPLEX *RootComplex,
+ UINT32 PcieIndex,
+ BOOLEAN EnableDbi
+ );
+
+BOOLEAN
+EndpointCfgReady (
+ IN AC01_ROOT_COMPLEX *RootComplex,
+ IN UINT8 PcieIndex,
+ IN UINT32 Timeout
+ );
+
+BOOLEAN
+PcieLinkUpCheck (
+ IN AC01_PCIE_CONTROLLER *Pcie
+ );
+
/**
Return the next extended capability base address
@@ -41,14 +62,38 @@ GetCapabilityBase (
{
BOOLEAN IsExtCapability = FALSE;
PHYSICAL_ADDRESS CfgBase;
+ PHYSICAL_ADDRESS Ret = 0;
+ PHYSICAL_ADDRESS RootComplexCfgBase;
UINT32 CapabilityId;
UINT32 NextCapabilityPtr;
UINT32 Val;
+ UINT32 RestoreVal;
- if (IsRootComplex) {
- CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
- } else {
+ RootComplexCfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
+ if (!IsRootComplex) {
+ // Allow programming to config space
+ EnableDbiAccess (RootComplex, PcieIndex, TRUE);
+
+ Val = MmioRead32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG);
+ RestoreVal = Val;
+ Val = SUB_BUS_SET (Val, DEFAULT_SUB_BUS);
+ Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
+ Val = PRIM_BUS_SET (Val, 0x0);
+ MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, Val);
CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
+
+ if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_TIMEOUT)) {
+ goto _CheckCapEnd;
+ }
+ } else {
+ CfgBase = RootComplexCfgBase;
+ }
+
+ // Check if device provide capability
+ Val = MmioRead32 (CfgBase + PCI_COMMAND_OFFSET);
+ Val = GET_HIGH_16_BITS (Val); /* Status */
+ if (!(Val & EFI_PCI_STATUS_CAPABILITY)) {
+ goto _CheckCapEnd;
}
Val = MmioRead32 (CfgBase + TYPE1_CAP_PTR_REG);
@@ -58,7 +103,8 @@ GetCapabilityBase (
while (1) {
if ((NextCapabilityPtr & WORD_ALIGN_MASK) != 0) {
// Not alignment, just return
- return 0;
+ Ret = 0;
+ goto _CheckCapEnd;
}
Val = MmioRead32 (CfgBase + NextCapabilityPtr);
@@ -69,7 +115,8 @@ GetCapabilityBase (
}
if (CapabilityId == ExtCapabilityId) {
- return (CfgBase + NextCapabilityPtr);
+ Ret = (CfgBase + NextCapabilityPtr);
+ goto _CheckCapEnd;
}
if (NextCapabilityPtr < EXT_CAPABILITY_START_BASE) {
@@ -84,9 +131,20 @@ GetCapabilityBase (
}
if ((NextCapabilityPtr == 0) && IsExtCapability) {
- return 0;
+ Ret = 0;
+ goto _CheckCapEnd;
}
}
+
+_CheckCapEnd:
+ if (!IsRootComplex) {
+ MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, RestoreVal);
+
+ // Disable programming to config space
+ EnableDbiAccess (RootComplex, PcieIndex, FALSE);
+ }
+
+ return Ret;
}
/**
@@ -677,6 +735,14 @@ Ac01PcieCoreSetupRC (
// Hold link training
StartLinkTraining (RootComplex, PcieIndex, FALSE);
+ // Clear BUSCTRL.CfgUrMask to set CRS (Configuration Request Retry Status) to 0xFFFF.FFFF
+ // rather than 0xFFFF.0001 as per PCIe specification requirement. Otherwise, this causes
+ // device drivers respond incorrectly on timeout due to long device operations.
+ TargetAddress = CsrBase + AC01_PCIE_CORE_BUS_CONTROL_REG;
+ Val = MmioRead32 (TargetAddress);
+ Val &= ~BUS_CTL_CFG_UR_MASK;
+ MmioWrite32 (TargetAddress, Val);
+
if (!EnableAxiPipeClock (RootComplex, PcieIndex)) {
DEBUG ((DEBUG_ERROR, "- Pcie[%d] - PIPE clock is not stable\n", PcieIndex));
return RETURN_DEVICE_ERROR;
@@ -688,9 +754,14 @@ Ac01PcieCoreSetupRC (
// Allow programming to config space
EnableDbiAccess (RootComplex, PcieIndex, TRUE);
- // Program the power limit
TargetAddress = CfgBase + PCIE_CAPABILITY_BASE + SLOT_CAPABILITIES_REG;
Val = MmioRead32 (TargetAddress);
+ // 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.
+ Val = SLOT_HPC_SET (Val, 1);
+ // Program the power limit
Val = SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET (Val, SLOT_POWER_LIMIT_75W);
MmioWrite32 (TargetAddress, Val);
@@ -755,6 +826,7 @@ Ac01PcieCoreSetupRC (
// Link timeout after 1ms
SetLinkTimeout (RootComplex, PcieIndex, 1);
+ // Mask Completion Timeout
DisableCompletionTimeOut (RootComplex, PcieIndex, TRUE);
ProgramRootPortInfo (RootComplex, PcieIndex);
@@ -1067,21 +1139,20 @@ Ac01PFACommand (
return Ret;
}
-UINT32
+BOOLEAN
EndpointCfgReady (
IN AC01_ROOT_COMPLEX *RootComplex,
- IN UINT8 PcieIndex
+ IN UINT8 PcieIndex,
+ IN UINT32 TimeOut
)
{
PHYSICAL_ADDRESS CfgBase;
- UINT32 TimeOut;
UINT32 Val;
CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
// Loop read CfgBase value until got valid value or
- // reach to timeout EP_LINKUP_TIMEOUT (or more depend on card)
- TimeOut = EP_LINKUP_TIMEOUT;
+ // reach to Timeout (or more depend on card)
do {
Val = MmioRead32 (CfgBase);
if (Val != 0xFFFF0001 && Val != 0xFFFFFFFF) {
@@ -1111,7 +1182,7 @@ Ac01PcieCoreGetEndpointInfo (
OUT UINT8 *EpMaxGen
)
{
- PHYSICAL_ADDRESS CfgBase;
+ PHYSICAL_ADDRESS CfgBase, EpCfgAddr;
PHYSICAL_ADDRESS PcieCapBase;
PHYSICAL_ADDRESS SecLatTimerAddr;
PHYSICAL_ADDRESS TargetAddress;
@@ -1133,8 +1204,23 @@ Ac01PcieCoreGetEndpointInfo (
Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
Val = PRIM_BUS_SET (Val, DEFAULT_PRIM_BUS);
MmioWrite32 (SecLatTimerAddr, Val);
+ EpCfgAddr = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
- if (EndpointCfgReady (RootComplex, PcieIndex)) {
+ if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_EXTRA_TIMEOUT)) {
+ goto Exit;
+ }
+
+ Val = MmioRead32 (EpCfgAddr);
+ // Check whether EP config space is accessible or not
+ if (Val == 0xFFFFFFFF) {
+ *EpMaxWidth = 0; // Invalid Width
+ *EpMaxGen = 0; // Invalid Speed
+ DEBUG ((DEBUG_ERROR, "PCIE%d.%d Cannot access EP config space!\n", RootComplex->ID, PcieIndex));
+ } else if (Val == 0xFFFF0001) {
+ *EpMaxWidth = 0; // Invalid Width
+ *EpMaxGen = 0; // Invalid Speed
+ DEBUG ((DEBUG_ERROR, "PCIE%d.%d EP config space still not ready to access, need poll more time!!!\n", RootComplex->ID, PcieIndex));
+ } else {
PcieCapBase = GetCapabilityBase (RootComplex, PcieIndex, FALSE, PCIE_CAPABILITY_ID);
if (PcieCapBase == 0) {
DEBUG ((
@@ -1164,6 +1250,7 @@ Ac01PcieCoreGetEndpointInfo (
}
}
+Exit:
// Restore value in order to not affect enumeration process
MmioWrite32 (SecLatTimerAddr, RestoreVal);
@@ -1280,6 +1367,30 @@ Ac01PcieCoreQoSLinkCheckRecovery (
return LINK_CHECK_SUCCESS;
}
+BOOLEAN
+Ac01PcieCoreCheckCardPresent (
+ IN AC01_PCIE_CONTROLLER *PcieController
+ )
+{
+ EFI_PHYSICAL_ADDRESS TargetAddress;
+ UINT32 ControlValue;
+
+ ControlValue = 0;
+
+ TargetAddress = PcieController->CsrBase;
+
+ ControlValue = MmioRead32 (TargetAddress + AC01_PCIE_CORE_LINK_CTRL_REG);
+
+ if (0 == LTSSMENB_GET (ControlValue)) {
+ //
+ // LTSSMENB is clear to 0x00 by Hardware -> link partner is connected.
+ //
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
VOID
Ac01PcieCoreUpdateLink (
IN AC01_ROOT_COMPLEX *RootComplex,
@@ -1328,16 +1439,17 @@ Ac01PcieCoreUpdateLink (
// Doing link checking and recovery if needed
Ac01PcieCoreQoSLinkCheckRecovery (RootComplex, PcieIndex);
- // Link timeout after 32ms
- SetLinkTimeout (RootComplex, PcieIndex, 32);
-
// Un-mask Completion Timeout
DisableCompletionTimeOut (RootComplex, PcieIndex, FALSE);
} else {
- *IsNextRoundNeeded = FALSE;
FailedPciePtr[*FailedPcieCount] = PcieIndex;
*FailedPcieCount += 1;
+
+ if (Ac01PcieCoreCheckCardPresent (Pcie)) {
+ *IsNextRoundNeeded = TRUE;
+ DEBUG ((DEBUG_INFO, "PCIE%d.%d Link retry\n", RootComplex->ID, PcieIndex));
+ }
}
}
}
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98437): https://edk2.groups.io/g/devel/message/98437
Mute This Topic: https://groups.io/mt/96240128/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Fri, Jan 13, 2023 at 11:25:16 +0700, Nhi Pham wrote:
> From: Vu Nguyen <vunguyen@os.amperecomputing.com>
>
> This updates the PCIe modules to add support for
> Ampere Altra Max processor which features 128 PCIe
> Gen4 lanes (distributed across eight x16 RCAs) using
> 32 controllers.
>
> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
> ---
> .../AmpereAltraPkg/Include/NVParamDef.h | 64 ++++++--
> .../Library/Ac01PcieLib/PcieCore.h | 8 +-
> .../Library/BoardPcieLib/BoardPcieLib.c | 63 +++++++-
> .../Drivers/PcieInitPei/PcieInitPei.c | 16 +-
> .../Drivers/PcieInitPei/RootComplexNVParam.c | 101 +++++++++---
> .../Library/Ac01PcieLib/PcieCore.c | 150 +++++++++++++++---
> 6 files changed, 333 insertions(+), 69 deletions(-)
>
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h b/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
> index 3259fa1ea45c..4dfc353d2340 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
> @@ -29,7 +29,7 @@
> As each non-volatile parameter requires 8 bytes, there is a total of 8K
> parameters.
>
> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -425,10 +425,10 @@
> #define NV_SI_RO_BOARD_S0_RCA5_CFG ((107 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
> #define NV_SI_RO_BOARD_S0_RCA6_CFG ((108 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
> #define NV_SI_RO_BOARD_S0_RCA7_CFG ((109 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020003 */
> -#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET ((110 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET ((111 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET ((112 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET ((113 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET ((110 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET ((111 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET ((112 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET ((113 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
Could you split the comment additions into a separate patch?
I think there are no functional changes to this file, but that is very
hard to determine.
> #define NV_SI_RO_BOARD_S0_RCB0A_TXRX_G3PRESET ((114 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S0_RCB0B_TXRX_G3PRESET ((115 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S0_RCB1A_TXRX_G3PRESET ((116 * 8) + NV_BOARD_PARAM_START)
> @@ -437,10 +437,10 @@
> #define NV_SI_RO_BOARD_S0_RCB2B_TXRX_G3PRESET ((119 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S0_RCB3A_TXRX_G3PRESET ((120 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S0_RCB3B_TXRX_G3PRESET ((121 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET ((122 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET ((123 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET ((124 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET ((125 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET ((122 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET ((123 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET ((124 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET ((125 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> #define NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET ((126 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> #define NV_SI_RO_BOARD_S0_RCA1_TXRX_G4PRESET ((127 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> #define NV_SI_RO_BOARD_S0_RCA2_TXRX_G4PRESET ((128 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> @@ -462,8 +462,8 @@
> #define NV_SI_RO_BOARD_S1_RCA5_CFG ((144 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
> #define NV_SI_RO_BOARD_S1_RCA6_CFG ((145 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
> #define NV_SI_RO_BOARD_S1_RCA7_CFG ((146 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020003 */
> -#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET ((147 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET ((148 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET ((147 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET ((148 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> #define NV_SI_RO_BOARD_S1_RCB0A_TXRX_G3PRESET ((149 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S1_RCB0B_TXRX_G3PRESET ((150 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S1_RCB1A_TXRX_G3PRESET ((151 * 8) + NV_BOARD_PARAM_START)
> @@ -472,10 +472,10 @@
> #define NV_SI_RO_BOARD_S1_RCB2B_TXRX_G3PRESET ((154 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S1_RCB3A_TXRX_G3PRESET ((155 * 8) + NV_BOARD_PARAM_START)
> #define NV_SI_RO_BOARD_S1_RCB3B_TXRX_G3PRESET ((156 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET ((157 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET ((158 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET ((159 * 8) + NV_BOARD_PARAM_START)
> -#define NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET ((160 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET ((157 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET ((158 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET ((159 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET ((160 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> #define NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET ((161 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> #define NV_SI_RO_BOARD_S1_RCA3_TXRX_G4PRESET ((162 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> #define NV_SI_RO_BOARD_S1_RCB0A_TXRX_G4PRESET ((163 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> @@ -523,7 +523,39 @@
> #define NV_SI_RO_BOARD_RAS_DDR_CE_TH1 ((205 * 8) + NV_BOARD_PARAM_START) /* Default: 0x000001F4 */
> #define NV_SI_RO_BOARD_RAS_DDR_CE_TH2 ((206 * 8) + NV_BOARD_PARAM_START) /* Default: 0x00001388 */
> #define NV_SI_RO_BOARD_RAS_DDR_CE_THC ((207 * 8) + NV_BOARD_PARAM_START) /* Default: 0x00000000 */
> -#define NV_PMPRO_REGION4_LOAD_END (NV_SI_RO_BOARD_RAS_DDR_CE_THC)
> +#define NV_SI_RO_BOARD_MQ_SX_RCA0_TXRX_20GPRESET ((208 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_MQ_SX_RCA1_TXRX_20GPRESET ((209 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_MQ_SX_RCA0_TXRX_25GPRESET ((210 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_MQ_SX_RCA1_TXRX_25GPRESET ((211 * 8) + NV_BOARD_PARAM_START)
> +#define NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET ((212 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA1_TXRX_G3PRESET ((213 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA2_TXRX_G3PRESET ((214 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA3_TXRX_G3PRESET ((215 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET ((216 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA5_TXRX_G3PRESET ((217 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA6_TXRX_G3PRESET ((218 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA7_TXRX_G3PRESET ((219 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET ((220 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA3_TXRX_G3PRESET ((221 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET ((222 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA5_TXRX_G3PRESET ((223 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA6_TXRX_G3PRESET ((224 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G3PRESET ((225 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET ((226 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA1_TXRX_G4PRESET ((227 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA2_TXRX_G4PRESET ((228 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA3_TXRX_G4PRESET ((229 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET ((230 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA5_TXRX_G4PRESET ((231 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA6_TXRX_G4PRESET ((232 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S0_RCA7_TXRX_G4PRESET ((233 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET ((234 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA3_TXRX_G4PRESET ((235 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET ((236 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA5_TXRX_G4PRESET ((237 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA6_TXRX_G4PRESET ((238 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G4PRESET ((239 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
> +#define NV_PMPRO_REGION4_LOAD_END (NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G4PRESET)
> //
> // NOTE: Add before NV_BOARD_PARAM_MAX and increase its value
> //
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
> index 1db8a68b3df4..a18fff7dbb75 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
> @@ -1,6 +1,6 @@
> /** @file
>
> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -39,6 +39,7 @@
> #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 EP_LINKUP_EXTRA_TIMEOUT (500 * 1000) // 500ms
> #define LINK_WAIT_INTERVAL_US 50
>
> #define PFA_MODE_ENABLE 0
> @@ -80,6 +81,7 @@
> #define AC01_PCIE_CORE_IRQ_ENABLE_REG 0x30
> #define AC01_PCIE_CORE_IRQ_EVENT_STAT_REG 0x38
> #define AC01_PCIE_CORE_BLOCK_EVENT_STAT_REG 0x3C
> +#define AC01_PCIE_CORE_BUS_CONTROL_REG 0x40
> #define AC01_PCIE_CORE_RESET_REG 0xC000
> #define AC01_PCIE_CORE_CLOCK_REG 0xC004
> #define AC01_PCIE_CORE_MEM_READY_REG 0xC104
> @@ -87,6 +89,7 @@
>
> // AC01_PCIE_CORE_LINK_CTRL_REG
> #define LTSSMENB_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> +#define LTSSMENB_GET(dst) ((dst) & (BIT0))
> #define HOLD_LINK_TRAINING 0
> #define START_LINK_TRAINING 1
> #define DEVICETYPE_SET(dst, src) (((dst) & ~0xF0) | (((UINT32) (src) << 4) & 0xF0))
> @@ -120,6 +123,9 @@
> // AC01_PCIE_CORE_BLOCK_EVENT_STAT_REG
> #define LINKUP_MASK 0x1
>
> +// AC01_PCIE_CORE_BUS_CONTROL_REG
> +#define BUS_CTL_CFG_UR_MASK 0x8
> +
> // AC01_PCIE_CORE_RESET_REG
> #define DWC_PCIE_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
> #define RESET_MASK 0x1
> diff --git a/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c b/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
> index f49764097219..4e0ba551e09b 100644
> --- a/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
> +++ b/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
> @@ -2,7 +2,7 @@
> Pcie board specific driver to handle asserting PERST signal to Endpoint
> card. PERST asserting is via group of GPIO pins to CPLD as Platform Specification.
>
> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -20,6 +20,8 @@
> #define RCB_MAX_PERST_GROUPVAL 46
> #define DEFAULT_SEGMENT_NUMBER 0x0F
>
> +#define PCIE_PERST_DELAY (100 * 1000) // 100ms
> +
> VOID
> BoardPcieReleaseAllPerst (
> IN UINT8 SocketId
> @@ -32,6 +34,8 @@ BoardPcieReleaseAllPerst (
> for (GpioIndex = 0; GpioIndex < 6; GpioIndex++) {
> GpioModeConfig (GpioPin + GpioIndex, GpioConfigOutHigh);
> }
> +
> + MicroSecondDelay (PCIE_PERST_DELAY);
> }
>
> /**
> @@ -56,11 +60,54 @@ BoardPcieAssertPerst (
>
> if (!IsPullToHigh) {
> if (RootComplex->Type == RootComplexTypeA) {
> - //
> - // RootComplexTypeA: RootComplex->ID: 0->3 ; PcieIndex: 0->3
> - //
> - GpioGroupVal = RCA_MAX_PERST_GROUPVAL - PcieIndex
> - - RootComplex->ID * MaxPcieControllerOfRootComplexA;
> + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
> + /* Ampere Altra: 4 */
> + //
> + // RootComplexTypeA: RootComplex->ID: 0->3 ; PcieIndex: 0->3
> + //
> + GpioGroupVal = RCA_MAX_PERST_GROUPVAL - PcieIndex
> + - RootComplex->ID * MaxPcieControllerOfRootComplexA;
> + } else {
> + /* Ampere Altra Max RootComplex->ID: 4:7 */
> + if (PcieIndex < 2) {
> + switch (RootComplex->ID) {
> + case 4:
> + GpioGroupVal = 34 - (PcieIndex * 2);
> + break;
> + case 5:
> + GpioGroupVal = 38 - (PcieIndex * 2);
> + break;
> + case 6:
> + GpioGroupVal = 30 - (PcieIndex * 2);
> + break;
> + case 7:
> + GpioGroupVal = 26 - (PcieIndex * 2);
> + break;
No default?
> + }
> + } else {
> + /* PcieIndex 2:3 */
> + switch (RootComplex->ID) {
> + case 4:
> + GpioGroupVal = 46 - ((PcieIndex - 2) * 2);
> + break;
> +
> + case 5:
> + GpioGroupVal = 42 - ((PcieIndex - 2) * 2);
> + break;
> +
> + case 6:
> + GpioGroupVal = 18 - ((PcieIndex - 2) * 2);
> + break;
> +
> + case 7:
> + GpioGroupVal = 22 - ((PcieIndex - 2) * 2);
> + break;
> +
> + default:
> + DEBUG ((DEBUG_ERROR, "Invalid Root Complex ID %d\n", RootComplex->ID));
> + }
> + }
> + }
It would greatly improve readability to break this lookup sequence
into a separate helper function.
> } else {
> //
> // RootComplexTypeB: RootComplex->ID: 4->7 ; PcieIndex: 0->7
> @@ -81,7 +128,7 @@ BoardPcieAssertPerst (
> }
>
> // Keep reset as low as 100 ms as specification
> - MicroSecondDelay (100 * 1000);
> + MicroSecondDelay (PCIE_PERST_DELAY);
> } else {
> BoardPcieReleaseAllPerst (RootComplex->Socket);
> }
> @@ -113,5 +160,5 @@ BoardPcieGetSegmentNumber (
> return Ac01BoardSegment[RootComplex->Socket][RootComplex->ID];
> }
>
> - return DEFAULT_SEGMENT_NUMBER;
> + return (RootComplex->ID - 2);
> }
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
> index 17f6112ea207..a69193b07005 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
> +++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
> @@ -1,6 +1,6 @@
> /** @file
>
> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -94,7 +94,7 @@ BuildRootComplexData (
> RootComplex = &mRootComplexList[RCIndex];
> RootComplex->Active = ConfigFound ? RootComplexConfig.RCStatus[RCIndex] : TRUE;
> RootComplex->DevMapLow = ConfigFound ? RootComplexConfig.RCBifurcationLow[RCIndex] : 0;
> - RootComplex->DevMapHigh = ConfigFound ? RootComplexConfig.RCBifurcationLow[RCIndex] : 0;
> + RootComplex->DevMapHigh = ConfigFound ? RootComplexConfig.RCBifurcationHigh[RCIndex] : 0;
This is counterintuitive enough as part of a patch that "adds support"
that I think it deserves to be broken out into a separate patch such
that it can explain why it's being done in a dedicated commit message.
> RootComplex->Socket = RCIndex / AC01_PCIE_MAX_RCS_PER_SOCKET;
> RootComplex->ID = RCIndex % AC01_PCIE_MAX_RCS_PER_SOCKET;
> RootComplex->CsrBase = mCsrBase[RCIndex];
> @@ -106,7 +106,12 @@ BuildRootComplexData (
> RootComplex->MmioSize = mMmioSize[RCIndex];
> RootComplex->Mmio32Base = mMmio32Base[RCIndex];
> RootComplex->Mmio32Size = mMmio32Size[RCIndex];
> - RootComplex->Type = (RootComplex->ID < MaxRootComplexA) ? RootComplexTypeA : RootComplexTypeB;
> + if (IsAc01Processor ()) {
> + RootComplex->Type = (RootComplex->ID < MaxRootComplexA) ? RootComplexTypeA : RootComplexTypeB;
> + } else {
> + RootComplex->Type = RootComplexTypeA;
> + }
> +
While much smaller, again I think it would help readability to break
this abstraction out into a separate helper function.
> RootComplex->MaxPcieController = (RootComplex->Type == RootComplexTypeB)
> ? MaxPcieControllerOfRootComplexB : MaxPcieControllerOfRootComplexA;
> RootComplex->Logical = BoardPcieGetSegmentNumber (RootComplex);
> @@ -168,11 +173,14 @@ PcieInitEntry (
> continue;
> }
>
> + DEBUG ((DEBUG_INIT, "Initializing S%d-RC%d...", RootComplex->Socket, RootComplex->ID));
> Status = Ac01PcieCoreSetupRC (RootComplex, FALSE, 0);
> if (EFI_ERROR (Status)) {
> - DEBUG ((DEBUG_ERROR, "RootComplex[%d]: Init Failed\n", Index));
> + DEBUG ((DEBUG_ERROR, "Failed\n"));
> RootComplex->Active = FALSE;
> continue;
> + } else {
> + DEBUG ((DEBUG_INIT, "Done + DevMapLow/High: %d/%d\n", RootComplex->DevMapLow, RootComplex->DevMapHigh));
> }
> }
>
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> index aa34a90b44c6..1346fa616ab3 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> +++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> @@ -37,7 +37,7 @@
> | Y | Y | Y | Y | 3 |
> ----------------------------------------
>
> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -149,6 +149,10 @@ GetDefaultDevMap (
> AC01_ROOT_COMPLEX *RootComplex
> )
> {
> + BOOLEAN IsAc01;
> +
> + IsAc01 = IsAc01Processor ();
> +
> if (RootComplex->Pcie[PcieController0].Active
> && RootComplex->Pcie[PcieController1].Active
> && RootComplex->Pcie[PcieController2].Active
> @@ -169,14 +173,20 @@ GetDefaultDevMap (
> && RootComplex->Pcie[PcieController5].Active
> && RootComplex->Pcie[PcieController6].Active
> && RootComplex->Pcie[PcieController7].Active) {
> - RootComplex->DefaultDevMapHigh = DevMapMode4;
> + if (IsAc01) {
> + RootComplex->DefaultDevMapHigh = DevMapMode4;
> + }
Who sets RootComplex->DefaultDevMapHigh if !IsAc01?
> } else if (RootComplex->Pcie[PcieController4].Active
> && RootComplex->Pcie[PcieController6].Active
> && RootComplex->Pcie[PcieController7].Active) {
> - RootComplex->DefaultDevMapHigh = DevMapMode3;
> + if (IsAc01) {
> + RootComplex->DefaultDevMapHigh = DevMapMode3;
> + }
same
> } else if (RootComplex->Pcie[PcieController4].Active
> && RootComplex->Pcie[PcieController6].Active) {
> - RootComplex->DefaultDevMapHigh = DevMapMode2;
> + if (IsAc01) {
> + RootComplex->DefaultDevMapHigh = DevMapMode2;
> + }
same
> } else {
> RootComplex->DefaultDevMapHigh = DevMapMode1;
> }
I feel this function in general could be rewritten more reader-friendly.
> @@ -203,12 +213,17 @@ GetLaneAllocation (
> EFI_STATUS Status;
> INTN RPIndex;
> NVPARAM NvParamOffset;
> - UINT32 Value, Width;
> + UINT32 Value, Width, Controller;
>
> // Retrieve lane allocation and capabilities for each controller
> if (RootComplex->Type == RootComplexTypeA) {
> - NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
> - NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
> + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
> + NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA4_CFG : NV_SI_RO_BOARD_S1_RCA4_CFG;
> + NvParamOffset += (RootComplex->ID - MaxPcieControllerOfRootComplexA) * NV_PARAM_ENTRYSIZE;
> + }
Helper function.
> } else {
> //
> // There're two NVParam entries per RootComplexTypeB
> @@ -222,7 +237,13 @@ GetLaneAllocation (
> Value = 0;
> }
>
> - for (RPIndex = 0; RPIndex < MaxPcieControllerOfRootComplexA; RPIndex++) {
> + if (IsAc01Processor ()) {
> + Controller = MaxPcieControllerOfRootComplexA;
> + } else {
> + Controller = RootComplex->MaxPcieController;
> + }
Straightforward enough, but could still be a helper function.
> +
> + for (RPIndex = PcieController0; RPIndex < Controller; RPIndex++) {
> Width = (Value >> (RPIndex * BITS_PER_BYTE)) & BYTE_MASK;
> switch (Width) {
> case 1:
> @@ -245,7 +266,7 @@ GetLaneAllocation (
>
> if (RootComplex->Type == RootComplexTypeB) {
> NvParamOffset += NV_PARAM_ENTRYSIZE;
> - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> if (EFI_ERROR (Status)) {
> Value = 0;
> }
> @@ -288,9 +309,17 @@ GetGen3PresetNvParamOffset (
> if (RootComplex->Socket == 0) {
> if (RootComplex->Type == RootComplexTypeA) {
> if (RootComplex->ID < MaxRootComplexA) {
> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
When we get to 4 levels of if, we absolutely need helper functions.
> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + }
> } else {
> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + }
> }
> } else {
> //
> @@ -300,9 +329,17 @@ GetGen3PresetNvParamOffset (
> }
> } else if (RootComplex->Type == RootComplexTypeA) {
> if (RootComplex->ID < MaxRootComplexA) {
> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> + }
> } else {
> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + }
> }
> } else {
> //
> @@ -324,9 +361,17 @@ GetGen4PresetNvParamOffset (
> if (RootComplex->Socket == 0) {
> if (RootComplex->Type == RootComplexTypeA) {
> if (RootComplex->ID < MaxRootComplexA) {
> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> + }
> } else {
> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + }
> }
> } else {
> //
> @@ -336,9 +381,17 @@ GetGen4PresetNvParamOffset (
> }
> } else if (RootComplex->Type == RootComplexTypeA) {
> if (RootComplex->ID < MaxRootComplexA) {
> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> + }
> } else {
> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + if (IsAc01Processor ()) {
> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + } else {
> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> + }
This also looks like the exact same computation takes place 4 times
for different combos of G4PRESETs and rootcompled IDs. That should be
possible to parametrise and compute in a single function instead.
> }
> } else {
> //
> @@ -352,7 +405,7 @@ GetGen4PresetNvParamOffset (
>
> VOID
> GetPresetSetting (
> - AC01_ROOT_COMPLEX *RootComplex
> + AC01_ROOT_COMPLEX *RootComplex
> )
> {
> EFI_STATUS Status;
> @@ -377,7 +430,7 @@ GetPresetSetting (
>
> if (RootComplex->Type == RootComplexTypeB) {
> NvParamOffset += NV_PARAM_ENTRYSIZE;
> - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> if (!EFI_ERROR (Status)) {
> for (Index = MaxPcieControllerOfRootComplexA; Index < MaxPcieController; Index++) {
> RootComplex->PresetGen3[Index] = (Value >> ((Index - MaxPcieControllerOfRootComplexA) * BITS_PER_BYTE)) & BYTE_MASK;
> @@ -414,7 +467,7 @@ GetMaxSpeedGen (
> UINT8 ErrataSpeedDevMap3[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN4, LINK_SPEED_GEN4, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 2: x8 x4 x4 (PCIE_ERRATA_SPEED1)
> UINT8 ErrataSpeedDevMap4[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 3: x4 x4 x4 x4 (PCIE_ERRATA_SPEED1)
> UINT8 ErrataSpeedRcb[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // RootComplexTypeB PCIE_ERRATA_SPEED1
> - UINT8 Idx;
> + UINT8 Idx, Controller;
One definition per line?
> UINT8 *MaxGen;
>
> ASSERT (MaxPcieControllerOfRootComplexA == 4);
> @@ -452,7 +505,13 @@ GetMaxSpeedGen (
> }
> }
>
> - for (Idx = 0; Idx < MaxPcieControllerOfRootComplexA; Idx++) {
> + if (IsAc01Processor ()) {
> + Controller = MaxPcieControllerOfRootComplexA;
> + } else {
> + Controller = RootComplex->MaxPcieController;
> + }
Straightforward enough, but could still be a helper function.
> +
> + for (Idx = 0; Idx < Controller; Idx++) {
> RootComplex->Pcie[Idx].MaxGen = RootComplex->Pcie[Idx].Active ? MaxGen[Idx] : LINK_SPEED_NONE;
> }
>
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> index ad648b1b9efd..12bd2c14544e 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> @@ -1,6 +1,6 @@
> /** @file
>
> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -10,8 +10,10 @@
>
> #include <Guid/PlatformInfoHob.h>
> #include <Guid/RootComplexInfoHob.h>
> +#include <IndustryStandard/Pci.h>
> #include <Library/ArmGenericTimerCounterLib.h>
> #include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> #include <Library/BoardPcieLib.h>
> #include <Library/DebugLib.h>
> #include <Library/HobLib.h>
> @@ -22,6 +24,25 @@
>
> #include "PcieCore.h"
>
> +VOID
> +EnableDbiAccess (
> + AC01_ROOT_COMPLEX *RootComplex,
> + UINT32 PcieIndex,
> + BOOLEAN EnableDbi
> + );
> +
> +BOOLEAN
> +EndpointCfgReady (
> + IN AC01_ROOT_COMPLEX *RootComplex,
> + IN UINT8 PcieIndex,
> + IN UINT32 Timeout
> + );
> +
> +BOOLEAN
> +PcieLinkUpCheck (
> + IN AC01_PCIE_CONTROLLER *Pcie
> + );
> +
> /**
> Return the next extended capability base address
>
> @@ -41,14 +62,38 @@ GetCapabilityBase (
> {
> BOOLEAN IsExtCapability = FALSE;
> PHYSICAL_ADDRESS CfgBase;
> + PHYSICAL_ADDRESS Ret = 0;
> + PHYSICAL_ADDRESS RootComplexCfgBase;
> UINT32 CapabilityId;
> UINT32 NextCapabilityPtr;
> UINT32 Val;
> + UINT32 RestoreVal;
>
> - if (IsRootComplex) {
> - CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
> - } else {
> + RootComplexCfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
> + if (!IsRootComplex) {
> + // Allow programming to config space
> + EnableDbiAccess (RootComplex, PcieIndex, TRUE);
> +
> + Val = MmioRead32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG);
> + RestoreVal = Val;
> + Val = SUB_BUS_SET (Val, DEFAULT_SUB_BUS);
> + Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
> + Val = PRIM_BUS_SET (Val, 0x0);
> + MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, Val);
> CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> +
> + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_TIMEOUT)) {
> + goto _CheckCapEnd;
> + }
> + } else {
> + CfgBase = RootComplexCfgBase;
> + }
> +
> + // Check if device provide capability
> + Val = MmioRead32 (CfgBase + PCI_COMMAND_OFFSET);
> + Val = GET_HIGH_16_BITS (Val); /* Status */
> + if (!(Val & EFI_PCI_STATUS_CAPABILITY)) {
> + goto _CheckCapEnd;
> }
>
> Val = MmioRead32 (CfgBase + TYPE1_CAP_PTR_REG);
> @@ -58,7 +103,8 @@ GetCapabilityBase (
> while (1) {
> if ((NextCapabilityPtr & WORD_ALIGN_MASK) != 0) {
> // Not alignment, just return
> - return 0;
> + Ret = 0;
> + goto _CheckCapEnd;
> }
>
> Val = MmioRead32 (CfgBase + NextCapabilityPtr);
> @@ -69,7 +115,8 @@ GetCapabilityBase (
> }
>
> if (CapabilityId == ExtCapabilityId) {
> - return (CfgBase + NextCapabilityPtr);
> + Ret = (CfgBase + NextCapabilityPtr);
> + goto _CheckCapEnd;
> }
>
> if (NextCapabilityPtr < EXT_CAPABILITY_START_BASE) {
> @@ -84,9 +131,20 @@ GetCapabilityBase (
> }
>
> if ((NextCapabilityPtr == 0) && IsExtCapability) {
> - return 0;
> + Ret = 0;
> + goto _CheckCapEnd;
> }
> }
> +
> +_CheckCapEnd:
> + if (!IsRootComplex) {
> + MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, RestoreVal);
> +
> + // Disable programming to config space
> + EnableDbiAccess (RootComplex, PcieIndex, FALSE);
> + }
> +
> + return Ret;
> }
>
> /**
> @@ -677,6 +735,14 @@ Ac01PcieCoreSetupRC (
> // Hold link training
> StartLinkTraining (RootComplex, PcieIndex, FALSE);
>
> + // Clear BUSCTRL.CfgUrMask to set CRS (Configuration Request Retry Status) to 0xFFFF.FFFF
> + // rather than 0xFFFF.0001 as per PCIe specification requirement. Otherwise, this causes
> + // device drivers respond incorrectly on timeout due to long device operations.
> + TargetAddress = CsrBase + AC01_PCIE_CORE_BUS_CONTROL_REG;
> + Val = MmioRead32 (TargetAddress);
> + Val &= ~BUS_CTL_CFG_UR_MASK;
> + MmioWrite32 (TargetAddress, Val);
> +
> if (!EnableAxiPipeClock (RootComplex, PcieIndex)) {
> DEBUG ((DEBUG_ERROR, "- Pcie[%d] - PIPE clock is not stable\n", PcieIndex));
> return RETURN_DEVICE_ERROR;
> @@ -688,9 +754,14 @@ Ac01PcieCoreSetupRC (
> // Allow programming to config space
> EnableDbiAccess (RootComplex, PcieIndex, TRUE);
>
> - // Program the power limit
> TargetAddress = CfgBase + PCIE_CAPABILITY_BASE + SLOT_CAPABILITIES_REG;
> Val = MmioRead32 (TargetAddress);
> + // 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.
I don't quite understand the comment.
Is this for netbooting, then inserting an NVMe drive after boot?
> + Val = SLOT_HPC_SET (Val, 1);
> + // Program the power limit
> Val = SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET (Val, SLOT_POWER_LIMIT_75W);
> MmioWrite32 (TargetAddress, Val);
>
> @@ -755,6 +826,7 @@ Ac01PcieCoreSetupRC (
> // Link timeout after 1ms
> SetLinkTimeout (RootComplex, PcieIndex, 1);
>
> + // Mask Completion Timeout
> DisableCompletionTimeOut (RootComplex, PcieIndex, TRUE);
>
> ProgramRootPortInfo (RootComplex, PcieIndex);
> @@ -1067,21 +1139,20 @@ Ac01PFACommand (
> return Ret;
> }
>
> -UINT32
> +BOOLEAN
> EndpointCfgReady (
> IN AC01_ROOT_COMPLEX *RootComplex,
> - IN UINT8 PcieIndex
> + IN UINT8 PcieIndex,
> + IN UINT32 TimeOut
> )
> {
> PHYSICAL_ADDRESS CfgBase;
> - UINT32 TimeOut;
> UINT32 Val;
>
> CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
>
> // Loop read CfgBase value until got valid value or
> - // reach to timeout EP_LINKUP_TIMEOUT (or more depend on card)
> - TimeOut = EP_LINKUP_TIMEOUT;
> + // reach to Timeout (or more depend on card)
> do {
> Val = MmioRead32 (CfgBase);
> if (Val != 0xFFFF0001 && Val != 0xFFFFFFFF) {
> @@ -1111,7 +1182,7 @@ Ac01PcieCoreGetEndpointInfo (
> OUT UINT8 *EpMaxGen
> )
> {
> - PHYSICAL_ADDRESS CfgBase;
> + PHYSICAL_ADDRESS CfgBase, EpCfgAddr;
One definition per line?
> PHYSICAL_ADDRESS PcieCapBase;
> PHYSICAL_ADDRESS SecLatTimerAddr;
> PHYSICAL_ADDRESS TargetAddress;
> @@ -1133,8 +1204,23 @@ Ac01PcieCoreGetEndpointInfo (
> Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
> Val = PRIM_BUS_SET (Val, DEFAULT_PRIM_BUS);
> MmioWrite32 (SecLatTimerAddr, Val);
> + EpCfgAddr = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
>
> - if (EndpointCfgReady (RootComplex, PcieIndex)) {
> + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_EXTRA_TIMEOUT)) {
> + goto Exit;
> + }
> +
> + Val = MmioRead32 (EpCfgAddr);
> + // Check whether EP config space is accessible or not
> + if (Val == 0xFFFFFFFF) {
> + *EpMaxWidth = 0; // Invalid Width
> + *EpMaxGen = 0; // Invalid Speed
> + DEBUG ((DEBUG_ERROR, "PCIE%d.%d Cannot access EP config space!\n", RootComplex->ID, PcieIndex));
> + } else if (Val == 0xFFFF0001) {
> + *EpMaxWidth = 0; // Invalid Width
> + *EpMaxGen = 0; // Invalid Speed
> + DEBUG ((DEBUG_ERROR, "PCIE%d.%d EP config space still not ready to access, need poll more time!!!\n", RootComplex->ID, PcieIndex));
> + } else {
> PcieCapBase = GetCapabilityBase (RootComplex, PcieIndex, FALSE, PCIE_CAPABILITY_ID);
> if (PcieCapBase == 0) {
> DEBUG ((
> @@ -1164,6 +1250,7 @@ Ac01PcieCoreGetEndpointInfo (
> }
> }
>
> +Exit:
> // Restore value in order to not affect enumeration process
> MmioWrite32 (SecLatTimerAddr, RestoreVal);
>
> @@ -1280,6 +1367,30 @@ Ac01PcieCoreQoSLinkCheckRecovery (
> return LINK_CHECK_SUCCESS;
> }
>
> +BOOLEAN
> +Ac01PcieCoreCheckCardPresent (
> + IN AC01_PCIE_CONTROLLER *PcieController
> + )
> +{
> + EFI_PHYSICAL_ADDRESS TargetAddress;
> + UINT32 ControlValue;
> +
> + ControlValue = 0;
> +
> + TargetAddress = PcieController->CsrBase;
> +
> + ControlValue = MmioRead32 (TargetAddress + AC01_PCIE_CORE_LINK_CTRL_REG);
> +
> + if (0 == LTSSMENB_GET (ControlValue)) {
No jeopardy testing.
> + //
> + // LTSSMENB is clear to 0x00 by Hardware -> link partner is connected.
> + //
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
> +
> VOID
> Ac01PcieCoreUpdateLink (
> IN AC01_ROOT_COMPLEX *RootComplex,
> @@ -1328,16 +1439,17 @@ Ac01PcieCoreUpdateLink (
> // Doing link checking and recovery if needed
> Ac01PcieCoreQoSLinkCheckRecovery (RootComplex, PcieIndex);
>
> - // Link timeout after 32ms
> - SetLinkTimeout (RootComplex, PcieIndex, 32);
> -
> // Un-mask Completion Timeout
> DisableCompletionTimeOut (RootComplex, PcieIndex, FALSE);
>
> } else {
> - *IsNextRoundNeeded = FALSE;
> FailedPciePtr[*FailedPcieCount] = PcieIndex;
> *FailedPcieCount += 1;
> +
> + if (Ac01PcieCoreCheckCardPresent (Pcie)) {
> + *IsNextRoundNeeded = TRUE;
> + DEBUG ((DEBUG_INFO, "PCIE%d.%d Link retry\n", RootComplex->ID, PcieIndex));
> + }
Another 4-level if-statement.
I'm not suggesting it's necessarily the latest addition that needs to
be broken out as a helper, but 4 is too deep.
Regards,
Leif
> }
> }
> }
> --
> 2.25.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100237): https://edk2.groups.io/g/devel/message/100237
Mute This Topic: https://groups.io/mt/96240128/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Leif,
Thanks for your reviewing. Most of your feedback is valid. I will fix
them. There is a comment that need your more explanation.
Please see the inline reply for more details.
On 2/15/2023 6:59 PM, Leif Lindholm wrote:
> On Fri, Jan 13, 2023 at 11:25:16 +0700, Nhi Pham wrote:
>> From: Vu Nguyen <vunguyen@os.amperecomputing.com>
>>
>> This updates the PCIe modules to add support for
>> Ampere Altra Max processor which features 128 PCIe
>> Gen4 lanes (distributed across eight x16 RCAs) using
>> 32 controllers.
>>
>> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
>> ---
>> .../AmpereAltraPkg/Include/NVParamDef.h | 64 ++++++--
>> .../Library/Ac01PcieLib/PcieCore.h | 8 +-
>> .../Library/BoardPcieLib/BoardPcieLib.c | 63 +++++++-
>> .../Drivers/PcieInitPei/PcieInitPei.c | 16 +-
>> .../Drivers/PcieInitPei/RootComplexNVParam.c | 101 +++++++++---
>> .../Library/Ac01PcieLib/PcieCore.c | 150 +++++++++++++++---
>> 6 files changed, 333 insertions(+), 69 deletions(-)
>>
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h b/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
>> index 3259fa1ea45c..4dfc353d2340 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h
>> @@ -29,7 +29,7 @@
>> As each non-volatile parameter requires 8 bytes, there is a total of 8K
>> parameters.
>>
>> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
>> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -425,10 +425,10 @@
>> #define NV_SI_RO_BOARD_S0_RCA5_CFG ((107 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
>> #define NV_SI_RO_BOARD_S0_RCA6_CFG ((108 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
>> #define NV_SI_RO_BOARD_S0_RCA7_CFG ((109 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020003 */
>> -#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET ((110 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET ((111 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET ((112 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET ((113 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET ((110 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S0_RCA1_TXRX_G3PRESET ((111 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S0_RCA2_TXRX_G3PRESET ((112 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S0_RCA3_TXRX_G3PRESET ((113 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
> Could you split the comment additions into a separate patch?
> I think there are no functional changes to this file, but that is very
> hard to determine.
Thanks, I will split it out.
>
>> #define NV_SI_RO_BOARD_S0_RCB0A_TXRX_G3PRESET ((114 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S0_RCB0B_TXRX_G3PRESET ((115 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S0_RCB1A_TXRX_G3PRESET ((116 * 8) + NV_BOARD_PARAM_START)
>> @@ -437,10 +437,10 @@
>> #define NV_SI_RO_BOARD_S0_RCB2B_TXRX_G3PRESET ((119 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S0_RCB3A_TXRX_G3PRESET ((120 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S0_RCB3B_TXRX_G3PRESET ((121 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET ((122 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET ((123 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET ((124 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET ((125 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET ((122 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S0_RCA5_TXRX_G3PRESET ((123 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S0_RCA6_TXRX_G3PRESET ((124 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S0_RCA7_TXRX_G3PRESET ((125 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> #define NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET ((126 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> #define NV_SI_RO_BOARD_S0_RCA1_TXRX_G4PRESET ((127 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> #define NV_SI_RO_BOARD_S0_RCA2_TXRX_G4PRESET ((128 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> @@ -462,8 +462,8 @@
>> #define NV_SI_RO_BOARD_S1_RCA5_CFG ((144 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
>> #define NV_SI_RO_BOARD_S1_RCA6_CFG ((145 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020202 */
>> #define NV_SI_RO_BOARD_S1_RCA7_CFG ((146 * 8) + NV_BOARD_PARAM_START) /* Default: 0x02020003 */
>> -#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET ((147 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET ((148 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET ((147 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S1_RCA3_TXRX_G3PRESET ((148 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> #define NV_SI_RO_BOARD_S1_RCB0A_TXRX_G3PRESET ((149 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S1_RCB0B_TXRX_G3PRESET ((150 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S1_RCB1A_TXRX_G3PRESET ((151 * 8) + NV_BOARD_PARAM_START)
>> @@ -472,10 +472,10 @@
>> #define NV_SI_RO_BOARD_S1_RCB2B_TXRX_G3PRESET ((154 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S1_RCB3A_TXRX_G3PRESET ((155 * 8) + NV_BOARD_PARAM_START)
>> #define NV_SI_RO_BOARD_S1_RCB3B_TXRX_G3PRESET ((156 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET ((157 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET ((158 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET ((159 * 8) + NV_BOARD_PARAM_START)
>> -#define NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET ((160 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET ((157 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S1_RCA5_TXRX_G3PRESET ((158 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S1_RCA6_TXRX_G3PRESET ((159 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_S1_RCA7_TXRX_G3PRESET ((160 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> #define NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET ((161 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> #define NV_SI_RO_BOARD_S1_RCA3_TXRX_G4PRESET ((162 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> #define NV_SI_RO_BOARD_S1_RCB0A_TXRX_G4PRESET ((163 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> @@ -523,7 +523,39 @@
>> #define NV_SI_RO_BOARD_RAS_DDR_CE_TH1 ((205 * 8) + NV_BOARD_PARAM_START) /* Default: 0x000001F4 */
>> #define NV_SI_RO_BOARD_RAS_DDR_CE_TH2 ((206 * 8) + NV_BOARD_PARAM_START) /* Default: 0x00001388 */
>> #define NV_SI_RO_BOARD_RAS_DDR_CE_THC ((207 * 8) + NV_BOARD_PARAM_START) /* Default: 0x00000000 */
>> -#define NV_PMPRO_REGION4_LOAD_END (NV_SI_RO_BOARD_RAS_DDR_CE_THC)
>> +#define NV_SI_RO_BOARD_MQ_SX_RCA0_TXRX_20GPRESET ((208 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_MQ_SX_RCA1_TXRX_20GPRESET ((209 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_MQ_SX_RCA0_TXRX_25GPRESET ((210 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_MQ_SX_RCA1_TXRX_25GPRESET ((211 * 8) + NV_BOARD_PARAM_START)
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET ((212 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA1_TXRX_G3PRESET ((213 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA2_TXRX_G3PRESET ((214 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA3_TXRX_G3PRESET ((215 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET ((216 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA5_TXRX_G3PRESET ((217 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA6_TXRX_G3PRESET ((218 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA7_TXRX_G3PRESET ((219 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET ((220 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA3_TXRX_G3PRESET ((221 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET ((222 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA5_TXRX_G3PRESET ((223 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA6_TXRX_G3PRESET ((224 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G3PRESET ((225 * 8) + NV_BOARD_PARAM_START) /* Default: 0xFFFFFFFF */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET ((226 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA1_TXRX_G4PRESET ((227 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA2_TXRX_G4PRESET ((228 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA3_TXRX_G4PRESET ((229 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET ((230 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA5_TXRX_G4PRESET ((231 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA6_TXRX_G4PRESET ((232 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S0_RCA7_TXRX_G4PRESET ((233 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET ((234 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA3_TXRX_G4PRESET ((235 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET ((236 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA5_TXRX_G4PRESET ((237 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA6_TXRX_G4PRESET ((238 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G4PRESET ((239 * 8) + NV_BOARD_PARAM_START) /* Default: 0x57575757 */
>> +#define NV_PMPRO_REGION4_LOAD_END (NV_SI_RO_BOARD_MQ_S1_RCA7_TXRX_G4PRESET)
>> //
>> // NOTE: Add before NV_BOARD_PARAM_MAX and increase its value
>> //
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
>> index 1db8a68b3df4..a18fff7dbb75 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.h
>> @@ -1,6 +1,6 @@
>> /** @file
>>
>> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
>> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -39,6 +39,7 @@
>> #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 EP_LINKUP_EXTRA_TIMEOUT (500 * 1000) // 500ms
>> #define LINK_WAIT_INTERVAL_US 50
>>
>> #define PFA_MODE_ENABLE 0
>> @@ -80,6 +81,7 @@
>> #define AC01_PCIE_CORE_IRQ_ENABLE_REG 0x30
>> #define AC01_PCIE_CORE_IRQ_EVENT_STAT_REG 0x38
>> #define AC01_PCIE_CORE_BLOCK_EVENT_STAT_REG 0x3C
>> +#define AC01_PCIE_CORE_BUS_CONTROL_REG 0x40
>> #define AC01_PCIE_CORE_RESET_REG 0xC000
>> #define AC01_PCIE_CORE_CLOCK_REG 0xC004
>> #define AC01_PCIE_CORE_MEM_READY_REG 0xC104
>> @@ -87,6 +89,7 @@
>>
>> // AC01_PCIE_CORE_LINK_CTRL_REG
>> #define LTSSMENB_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
>> +#define LTSSMENB_GET(dst) ((dst) & (BIT0))
>> #define HOLD_LINK_TRAINING 0
>> #define START_LINK_TRAINING 1
>> #define DEVICETYPE_SET(dst, src) (((dst) & ~0xF0) | (((UINT32) (src) << 4) & 0xF0))
>> @@ -120,6 +123,9 @@
>> // AC01_PCIE_CORE_BLOCK_EVENT_STAT_REG
>> #define LINKUP_MASK 0x1
>>
>> +// AC01_PCIE_CORE_BUS_CONTROL_REG
>> +#define BUS_CTL_CFG_UR_MASK 0x8
>> +
>> // AC01_PCIE_CORE_RESET_REG
>> #define DWC_PCIE_SET(dst, src) (((dst) & ~0x1) | (((UINT32) (src)) & 0x1))
>> #define RESET_MASK 0x1
>> diff --git a/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c b/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
>> index f49764097219..4e0ba551e09b 100644
>> --- a/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
>> +++ b/Platform/Ampere/JadePkg/Library/BoardPcieLib/BoardPcieLib.c
>> @@ -2,7 +2,7 @@
>> Pcie board specific driver to handle asserting PERST signal to Endpoint
>> card. PERST asserting is via group of GPIO pins to CPLD as Platform Specification.
>>
>> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
>> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -20,6 +20,8 @@
>> #define RCB_MAX_PERST_GROUPVAL 46
>> #define DEFAULT_SEGMENT_NUMBER 0x0F
>>
>> +#define PCIE_PERST_DELAY (100 * 1000) // 100ms
>> +
>> VOID
>> BoardPcieReleaseAllPerst (
>> IN UINT8 SocketId
>> @@ -32,6 +34,8 @@ BoardPcieReleaseAllPerst (
>> for (GpioIndex = 0; GpioIndex < 6; GpioIndex++) {
>> GpioModeConfig (GpioPin + GpioIndex, GpioConfigOutHigh);
>> }
>> +
>> + MicroSecondDelay (PCIE_PERST_DELAY);
>> }
>>
>> /**
>> @@ -56,11 +60,54 @@ BoardPcieAssertPerst (
>>
>> if (!IsPullToHigh) {
>> if (RootComplex->Type == RootComplexTypeA) {
>> - //
>> - // RootComplexTypeA: RootComplex->ID: 0->3 ; PcieIndex: 0->3
>> - //
>> - GpioGroupVal = RCA_MAX_PERST_GROUPVAL - PcieIndex
>> - - RootComplex->ID * MaxPcieControllerOfRootComplexA;
>> + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
>> + /* Ampere Altra: 4 */
>> + //
>> + // RootComplexTypeA: RootComplex->ID: 0->3 ; PcieIndex: 0->3
>> + //
>> + GpioGroupVal = RCA_MAX_PERST_GROUPVAL - PcieIndex
>> + - RootComplex->ID * MaxPcieControllerOfRootComplexA;
>> + } else {
>> + /* Ampere Altra Max RootComplex->ID: 4:7 */
>> + if (PcieIndex < 2) {
>> + switch (RootComplex->ID) {
>> + case 4:
>> + GpioGroupVal = 34 - (PcieIndex * 2);
>> + break;
>> + case 5:
>> + GpioGroupVal = 38 - (PcieIndex * 2);
>> + break;
>> + case 6:
>> + GpioGroupVal = 30 - (PcieIndex * 2);
>> + break;
>> + case 7:
>> + GpioGroupVal = 26 - (PcieIndex * 2);
>> + break;
> No default?
Will add default case.
>
>> + }
>> + } else {
>> + /* PcieIndex 2:3 */
>> + switch (RootComplex->ID) {
>> + case 4:
>> + GpioGroupVal = 46 - ((PcieIndex - 2) * 2);
>> + break;
>> +
>> + case 5:
>> + GpioGroupVal = 42 - ((PcieIndex - 2) * 2);
>> + break;
>> +
>> + case 6:
>> + GpioGroupVal = 18 - ((PcieIndex - 2) * 2);
>> + break;
>> +
>> + case 7:
>> + GpioGroupVal = 22 - ((PcieIndex - 2) * 2);
>> + break;
>> +
>> + default:
>> + DEBUG ((DEBUG_ERROR, "Invalid Root Complex ID %d\n", RootComplex->ID));
>> + }
>> + }
>> + }
> It would greatly improve readability to break this lookup sequence
> into a separate helper function.
Will improve it.
>
>> } else {
>> //
>> // RootComplexTypeB: RootComplex->ID: 4->7 ; PcieIndex: 0->7
>> @@ -81,7 +128,7 @@ BoardPcieAssertPerst (
>> }
>>
>> // Keep reset as low as 100 ms as specification
>> - MicroSecondDelay (100 * 1000);
>> + MicroSecondDelay (PCIE_PERST_DELAY);
>> } else {
>> BoardPcieReleaseAllPerst (RootComplex->Socket);
>> }
>> @@ -113,5 +160,5 @@ BoardPcieGetSegmentNumber (
>> return Ac01BoardSegment[RootComplex->Socket][RootComplex->ID];
>> }
>>
>> - return DEFAULT_SEGMENT_NUMBER;
>> + return (RootComplex->ID - 2);
>> }
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
>> index 17f6112ea207..a69193b07005 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c
>> @@ -1,6 +1,6 @@
>> /** @file
>>
>> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
>> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -94,7 +94,7 @@ BuildRootComplexData (
>> RootComplex = &mRootComplexList[RCIndex];
>> RootComplex->Active = ConfigFound ? RootComplexConfig.RCStatus[RCIndex] : TRUE;
>> RootComplex->DevMapLow = ConfigFound ? RootComplexConfig.RCBifurcationLow[RCIndex] : 0;
>> - RootComplex->DevMapHigh = ConfigFound ? RootComplexConfig.RCBifurcationLow[RCIndex] : 0;
>> + RootComplex->DevMapHigh = ConfigFound ? RootComplexConfig.RCBifurcationHigh[RCIndex] : 0;
> This is counterintuitive enough as part of a patch that "adds support"
> that I think it deserves to be broken out into a separate patch such
> that it can explain why it's being done in a dedicated commit message.
Will put this fix in a separate patch.
>
>> RootComplex->Socket = RCIndex / AC01_PCIE_MAX_RCS_PER_SOCKET;
>> RootComplex->ID = RCIndex % AC01_PCIE_MAX_RCS_PER_SOCKET;
>> RootComplex->CsrBase = mCsrBase[RCIndex];
>> @@ -106,7 +106,12 @@ BuildRootComplexData (
>> RootComplex->MmioSize = mMmioSize[RCIndex];
>> RootComplex->Mmio32Base = mMmio32Base[RCIndex];
>> RootComplex->Mmio32Size = mMmio32Size[RCIndex];
>> - RootComplex->Type = (RootComplex->ID < MaxRootComplexA) ? RootComplexTypeA : RootComplexTypeB;
>> + if (IsAc01Processor ()) {
>> + RootComplex->Type = (RootComplex->ID < MaxRootComplexA) ? RootComplexTypeA : RootComplexTypeB;
>> + } else {
>> + RootComplex->Type = RootComplexTypeA;
>> + }
>> +
> While much smaller, again I think it would help readability to break
> this abstraction out into a separate helper function.
Agree, will add helper functions. Also, check the later comments for
this patch to improve the code readability.
>
>> RootComplex->MaxPcieController = (RootComplex->Type == RootComplexTypeB)
>> ? MaxPcieControllerOfRootComplexB : MaxPcieControllerOfRootComplexA;
>> RootComplex->Logical = BoardPcieGetSegmentNumber (RootComplex);
>> @@ -168,11 +173,14 @@ PcieInitEntry (
>> continue;
>> }
>>
>> + DEBUG ((DEBUG_INIT, "Initializing S%d-RC%d...", RootComplex->Socket, RootComplex->ID));
>> Status = Ac01PcieCoreSetupRC (RootComplex, FALSE, 0);
>> if (EFI_ERROR (Status)) {
>> - DEBUG ((DEBUG_ERROR, "RootComplex[%d]: Init Failed\n", Index));
>> + DEBUG ((DEBUG_ERROR, "Failed\n"));
>> RootComplex->Active = FALSE;
>> continue;
>> + } else {
>> + DEBUG ((DEBUG_INIT, "Done + DevMapLow/High: %d/%d\n", RootComplex->DevMapLow, RootComplex->DevMapHigh));
>> }
>> }
>>
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
>> index aa34a90b44c6..1346fa616ab3 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
>> @@ -37,7 +37,7 @@
>> | Y | Y | Y | Y | 3 |
>> ----------------------------------------
>>
>> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
>> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -149,6 +149,10 @@ GetDefaultDevMap (
>> AC01_ROOT_COMPLEX *RootComplex
>> )
>> {
>> + BOOLEAN IsAc01;
>> +
>> + IsAc01 = IsAc01Processor ();
>> +
>> if (RootComplex->Pcie[PcieController0].Active
>> && RootComplex->Pcie[PcieController1].Active
>> && RootComplex->Pcie[PcieController2].Active
>> @@ -169,14 +173,20 @@ GetDefaultDevMap (
>> && RootComplex->Pcie[PcieController5].Active
>> && RootComplex->Pcie[PcieController6].Active
>> && RootComplex->Pcie[PcieController7].Active) {
>> - RootComplex->DefaultDevMapHigh = DevMapMode4;
>> + if (IsAc01) {
>> + RootComplex->DefaultDevMapHigh = DevMapMode4;
>> + }
> Who sets RootComplex->DefaultDevMapHigh if !IsAc01?
It will be the default value (0) because Ampere Altra Max does not have
Root Complex Type B.
>
>> } else if (RootComplex->Pcie[PcieController4].Active
>> && RootComplex->Pcie[PcieController6].Active
>> && RootComplex->Pcie[PcieController7].Active) {
>> - RootComplex->DefaultDevMapHigh = DevMapMode3;
>> + if (IsAc01) {
>> + RootComplex->DefaultDevMapHigh = DevMapMode3;
>> + }
> same
>
>> } else if (RootComplex->Pcie[PcieController4].Active
>> && RootComplex->Pcie[PcieController6].Active) {
>> - RootComplex->DefaultDevMapHigh = DevMapMode2;
>> + if (IsAc01) {
>> + RootComplex->DefaultDevMapHigh = DevMapMode2;
>> + }
> same
>
>> } else {
>> RootComplex->DefaultDevMapHigh = DevMapMode1;
>> }
> I feel this function in general could be rewritten more reader-friendly.
>
>> @@ -203,12 +213,17 @@ GetLaneAllocation (
>> EFI_STATUS Status;
>> INTN RPIndex;
>> NVPARAM NvParamOffset;
>> - UINT32 Value, Width;
>> + UINT32 Value, Width, Controller;
>>
>> // Retrieve lane allocation and capabilities for each controller
>> if (RootComplex->Type == RootComplexTypeA) {
>> - NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
>> - NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
>> + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
>> + NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA4_CFG : NV_SI_RO_BOARD_S1_RCA4_CFG;
>> + NvParamOffset += (RootComplex->ID - MaxPcieControllerOfRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + }
> Helper function.
>
>> } else {
>> //
>> // There're two NVParam entries per RootComplexTypeB
>> @@ -222,7 +237,13 @@ GetLaneAllocation (
>> Value = 0;
>> }
>>
>> - for (RPIndex = 0; RPIndex < MaxPcieControllerOfRootComplexA; RPIndex++) {
>> + if (IsAc01Processor ()) {
>> + Controller = MaxPcieControllerOfRootComplexA;
>> + } else {
>> + Controller = RootComplex->MaxPcieController;
>> + }
> Straightforward enough, but could still be a helper function.
>
>> +
>> + for (RPIndex = PcieController0; RPIndex < Controller; RPIndex++) {
>> Width = (Value >> (RPIndex * BITS_PER_BYTE)) & BYTE_MASK;
>> switch (Width) {
>> case 1:
>> @@ -245,7 +266,7 @@ GetLaneAllocation (
>>
>> if (RootComplex->Type == RootComplexTypeB) {
>> NvParamOffset += NV_PARAM_ENTRYSIZE;
>> - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
>> + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
>> if (EFI_ERROR (Status)) {
>> Value = 0;
>> }
>> @@ -288,9 +309,17 @@ GetGen3PresetNvParamOffset (
>> if (RootComplex->Socket == 0) {
>> if (RootComplex->Type == RootComplexTypeA) {
>> if (RootComplex->ID < MaxRootComplexA) {
>> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
> When we get to 4 levels of if, we absolutely need helper functions.
>
>> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + }
>> } else {
>> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + }
>> }
>> } else {
>> //
>> @@ -300,9 +329,17 @@ GetGen3PresetNvParamOffset (
>> }
>> } else if (RootComplex->Type == RootComplexTypeA) {
>> if (RootComplex->ID < MaxRootComplexA) {
>> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
>> + }
>> } else {
>> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + }
>> }
>> } else {
>> //
>> @@ -324,9 +361,17 @@ GetGen4PresetNvParamOffset (
>> if (RootComplex->Socket == 0) {
>> if (RootComplex->Type == RootComplexTypeA) {
>> if (RootComplex->ID < MaxRootComplexA) {
>> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
>> + }
>> } else {
>> - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + }
>> }
>> } else {
>> //
>> @@ -336,9 +381,17 @@ GetGen4PresetNvParamOffset (
>> }
>> } else if (RootComplex->Type == RootComplexTypeA) {
>> if (RootComplex->ID < MaxRootComplexA) {
>> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
>> + }
>> } else {
>> - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + if (IsAc01Processor ()) {
>> + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + } else {
>> + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
>> + }
> This also looks like the exact same computation takes place 4 times
> for different combos of G4PRESETs and rootcompled IDs. That should be
> possible to parametrise and compute in a single function instead.
Correct, will improve it.
>
>> }
>> } else {
>> //
>> @@ -352,7 +405,7 @@ GetGen4PresetNvParamOffset (
>>
>> VOID
>> GetPresetSetting (
>> - AC01_ROOT_COMPLEX *RootComplex
>> + AC01_ROOT_COMPLEX *RootComplex
>> )
>> {
>> EFI_STATUS Status;
>> @@ -377,7 +430,7 @@ GetPresetSetting (
>>
>> if (RootComplex->Type == RootComplexTypeB) {
>> NvParamOffset += NV_PARAM_ENTRYSIZE;
>> - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
>> + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
>> if (!EFI_ERROR (Status)) {
>> for (Index = MaxPcieControllerOfRootComplexA; Index < MaxPcieController; Index++) {
>> RootComplex->PresetGen3[Index] = (Value >> ((Index - MaxPcieControllerOfRootComplexA) * BITS_PER_BYTE)) & BYTE_MASK;
>> @@ -414,7 +467,7 @@ GetMaxSpeedGen (
>> UINT8 ErrataSpeedDevMap3[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN4, LINK_SPEED_GEN4, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 2: x8 x4 x4 (PCIE_ERRATA_SPEED1)
>> UINT8 ErrataSpeedDevMap4[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 3: x4 x4 x4 x4 (PCIE_ERRATA_SPEED1)
>> UINT8 ErrataSpeedRcb[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // RootComplexTypeB PCIE_ERRATA_SPEED1
>> - UINT8 Idx;
>> + UINT8 Idx, Controller;
> One definition per line?
Will fix.
>
>> UINT8 *MaxGen;
>>
>> ASSERT (MaxPcieControllerOfRootComplexA == 4);
>> @@ -452,7 +505,13 @@ GetMaxSpeedGen (
>> }
>> }
>>
>> - for (Idx = 0; Idx < MaxPcieControllerOfRootComplexA; Idx++) {
>> + if (IsAc01Processor ()) {
>> + Controller = MaxPcieControllerOfRootComplexA;
>> + } else {
>> + Controller = RootComplex->MaxPcieController;
>> + }
> Straightforward enough, but could still be a helper function.
>
>> +
>> + for (Idx = 0; Idx < Controller; Idx++) {
>> RootComplex->Pcie[Idx].MaxGen = RootComplex->Pcie[Idx].Active ? MaxGen[Idx] : LINK_SPEED_NONE;
>> }
>>
>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
>> index ad648b1b9efd..12bd2c14544e 100644
>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
>> @@ -1,6 +1,6 @@
>> /** @file
>>
>> - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
>> + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -10,8 +10,10 @@
>>
>> #include <Guid/PlatformInfoHob.h>
>> #include <Guid/RootComplexInfoHob.h>
>> +#include <IndustryStandard/Pci.h>
>> #include <Library/ArmGenericTimerCounterLib.h>
>> #include <Library/BaseLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> #include <Library/BoardPcieLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/HobLib.h>
>> @@ -22,6 +24,25 @@
>>
>> #include "PcieCore.h"
>>
>> +VOID
>> +EnableDbiAccess (
>> + AC01_ROOT_COMPLEX *RootComplex,
>> + UINT32 PcieIndex,
>> + BOOLEAN EnableDbi
>> + );
>> +
>> +BOOLEAN
>> +EndpointCfgReady (
>> + IN AC01_ROOT_COMPLEX *RootComplex,
>> + IN UINT8 PcieIndex,
>> + IN UINT32 Timeout
>> + );
>> +
>> +BOOLEAN
>> +PcieLinkUpCheck (
>> + IN AC01_PCIE_CONTROLLER *Pcie
>> + );
>> +
>> /**
>> Return the next extended capability base address
>>
>> @@ -41,14 +62,38 @@ GetCapabilityBase (
>> {
>> BOOLEAN IsExtCapability = FALSE;
>> PHYSICAL_ADDRESS CfgBase;
>> + PHYSICAL_ADDRESS Ret = 0;
>> + PHYSICAL_ADDRESS RootComplexCfgBase;
>> UINT32 CapabilityId;
>> UINT32 NextCapabilityPtr;
>> UINT32 Val;
>> + UINT32 RestoreVal;
>>
>> - if (IsRootComplex) {
>> - CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
>> - } else {
>> + RootComplexCfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
>> + if (!IsRootComplex) {
>> + // Allow programming to config space
>> + EnableDbiAccess (RootComplex, PcieIndex, TRUE);
>> +
>> + Val = MmioRead32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG);
>> + RestoreVal = Val;
>> + Val = SUB_BUS_SET (Val, DEFAULT_SUB_BUS);
>> + Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
>> + Val = PRIM_BUS_SET (Val, 0x0);
>> + MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, Val);
>> CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
>> +
>> + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_TIMEOUT)) {
>> + goto _CheckCapEnd;
>> + }
>> + } else {
>> + CfgBase = RootComplexCfgBase;
>> + }
>> +
>> + // Check if device provide capability
>> + Val = MmioRead32 (CfgBase + PCI_COMMAND_OFFSET);
>> + Val = GET_HIGH_16_BITS (Val); /* Status */
>> + if (!(Val & EFI_PCI_STATUS_CAPABILITY)) {
>> + goto _CheckCapEnd;
>> }
>>
>> Val = MmioRead32 (CfgBase + TYPE1_CAP_PTR_REG);
>> @@ -58,7 +103,8 @@ GetCapabilityBase (
>> while (1) {
>> if ((NextCapabilityPtr & WORD_ALIGN_MASK) != 0) {
>> // Not alignment, just return
>> - return 0;
>> + Ret = 0;
>> + goto _CheckCapEnd;
>> }
>>
>> Val = MmioRead32 (CfgBase + NextCapabilityPtr);
>> @@ -69,7 +115,8 @@ GetCapabilityBase (
>> }
>>
>> if (CapabilityId == ExtCapabilityId) {
>> - return (CfgBase + NextCapabilityPtr);
>> + Ret = (CfgBase + NextCapabilityPtr);
>> + goto _CheckCapEnd;
>> }
>>
>> if (NextCapabilityPtr < EXT_CAPABILITY_START_BASE) {
>> @@ -84,9 +131,20 @@ GetCapabilityBase (
>> }
>>
>> if ((NextCapabilityPtr == 0) && IsExtCapability) {
>> - return 0;
>> + Ret = 0;
>> + goto _CheckCapEnd;
>> }
>> }
>> +
>> +_CheckCapEnd:
>> + if (!IsRootComplex) {
>> + MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, RestoreVal);
>> +
>> + // Disable programming to config space
>> + EnableDbiAccess (RootComplex, PcieIndex, FALSE);
>> + }
>> +
>> + return Ret;
>> }
>>
>> /**
>> @@ -677,6 +735,14 @@ Ac01PcieCoreSetupRC (
>> // Hold link training
>> StartLinkTraining (RootComplex, PcieIndex, FALSE);
>>
>> + // Clear BUSCTRL.CfgUrMask to set CRS (Configuration Request Retry Status) to 0xFFFF.FFFF
>> + // rather than 0xFFFF.0001 as per PCIe specification requirement. Otherwise, this causes
>> + // device drivers respond incorrectly on timeout due to long device operations.
>> + TargetAddress = CsrBase + AC01_PCIE_CORE_BUS_CONTROL_REG;
>> + Val = MmioRead32 (TargetAddress);
>> + Val &= ~BUS_CTL_CFG_UR_MASK;
>> + MmioWrite32 (TargetAddress, Val);
>> +
>> if (!EnableAxiPipeClock (RootComplex, PcieIndex)) {
>> DEBUG ((DEBUG_ERROR, "- Pcie[%d] - PIPE clock is not stable\n", PcieIndex));
>> return RETURN_DEVICE_ERROR;
>> @@ -688,9 +754,14 @@ Ac01PcieCoreSetupRC (
>> // Allow programming to config space
>> EnableDbiAccess (RootComplex, PcieIndex, TRUE);
>>
>> - // Program the power limit
>> TargetAddress = CfgBase + PCIE_CAPABILITY_BASE + SLOT_CAPABILITIES_REG;
>> Val = MmioRead32 (TargetAddress);
>> + // 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.
> I don't quite understand the comment.
> Is this for netbooting, then inserting an NVMe drive after boot?
This is a part of PCIe Hotplug feature that will be upstreamed later.
Now we will remove this change.
>
>> + Val = SLOT_HPC_SET (Val, 1);
>> + // Program the power limit
>> Val = SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET (Val, SLOT_POWER_LIMIT_75W);
>> MmioWrite32 (TargetAddress, Val);
>>
>> @@ -755,6 +826,7 @@ Ac01PcieCoreSetupRC (
>> // Link timeout after 1ms
>> SetLinkTimeout (RootComplex, PcieIndex, 1);
>>
>> + // Mask Completion Timeout
>> DisableCompletionTimeOut (RootComplex, PcieIndex, TRUE);
>>
>> ProgramRootPortInfo (RootComplex, PcieIndex);
>> @@ -1067,21 +1139,20 @@ Ac01PFACommand (
>> return Ret;
>> }
>>
>> -UINT32
>> +BOOLEAN
>> EndpointCfgReady (
>> IN AC01_ROOT_COMPLEX *RootComplex,
>> - IN UINT8 PcieIndex
>> + IN UINT8 PcieIndex,
>> + IN UINT32 TimeOut
>> )
>> {
>> PHYSICAL_ADDRESS CfgBase;
>> - UINT32 TimeOut;
>> UINT32 Val;
>>
>> CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
>>
>> // Loop read CfgBase value until got valid value or
>> - // reach to timeout EP_LINKUP_TIMEOUT (or more depend on card)
>> - TimeOut = EP_LINKUP_TIMEOUT;
>> + // reach to Timeout (or more depend on card)
>> do {
>> Val = MmioRead32 (CfgBase);
>> if (Val != 0xFFFF0001 && Val != 0xFFFFFFFF) {
>> @@ -1111,7 +1182,7 @@ Ac01PcieCoreGetEndpointInfo (
>> OUT UINT8 *EpMaxGen
>> )
>> {
>> - PHYSICAL_ADDRESS CfgBase;
>> + PHYSICAL_ADDRESS CfgBase, EpCfgAddr;
> One definition per line?
>
>> PHYSICAL_ADDRESS PcieCapBase;
>> PHYSICAL_ADDRESS SecLatTimerAddr;
>> PHYSICAL_ADDRESS TargetAddress;
>> @@ -1133,8 +1204,23 @@ Ac01PcieCoreGetEndpointInfo (
>> Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
>> Val = PRIM_BUS_SET (Val, DEFAULT_PRIM_BUS);
>> MmioWrite32 (SecLatTimerAddr, Val);
>> + EpCfgAddr = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
>>
>> - if (EndpointCfgReady (RootComplex, PcieIndex)) {
>> + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_EXTRA_TIMEOUT)) {
>> + goto Exit;
>> + }
>> +
>> + Val = MmioRead32 (EpCfgAddr);
>> + // Check whether EP config space is accessible or not
>> + if (Val == 0xFFFFFFFF) {
>> + *EpMaxWidth = 0; // Invalid Width
>> + *EpMaxGen = 0; // Invalid Speed
>> + DEBUG ((DEBUG_ERROR, "PCIE%d.%d Cannot access EP config space!\n", RootComplex->ID, PcieIndex));
>> + } else if (Val == 0xFFFF0001) {
>> + *EpMaxWidth = 0; // Invalid Width
>> + *EpMaxGen = 0; // Invalid Speed
>> + DEBUG ((DEBUG_ERROR, "PCIE%d.%d EP config space still not ready to access, need poll more time!!!\n", RootComplex->ID, PcieIndex));
>> + } else {
>> PcieCapBase = GetCapabilityBase (RootComplex, PcieIndex, FALSE, PCIE_CAPABILITY_ID);
>> if (PcieCapBase == 0) {
>> DEBUG ((
>> @@ -1164,6 +1250,7 @@ Ac01PcieCoreGetEndpointInfo (
>> }
>> }
>>
>> +Exit:
>> // Restore value in order to not affect enumeration process
>> MmioWrite32 (SecLatTimerAddr, RestoreVal);
>>
>> @@ -1280,6 +1367,30 @@ Ac01PcieCoreQoSLinkCheckRecovery (
>> return LINK_CHECK_SUCCESS;
>> }
>>
>> +BOOLEAN
>> +Ac01PcieCoreCheckCardPresent (
>> + IN AC01_PCIE_CONTROLLER *PcieController
>> + )
>> +{
>> + EFI_PHYSICAL_ADDRESS TargetAddress;
>> + UINT32 ControlValue;
>> +
>> + ControlValue = 0;
>> +
>> + TargetAddress = PcieController->CsrBase;
>> +
>> + ControlValue = MmioRead32 (TargetAddress + AC01_PCIE_CORE_LINK_CTRL_REG);
>> +
>> + if (0 == LTSSMENB_GET (ControlValue)) {
> No jeopardy testing.
Sorry, could you explain more about jeopardy testing?
>
>> + //
>> + // LTSSMENB is clear to 0x00 by Hardware -> link partner is connected.
>> + //
>> + return TRUE;
>> + }
>> +
>> + return FALSE;
>> +}
>> +
>> VOID
>> Ac01PcieCoreUpdateLink (
>> IN AC01_ROOT_COMPLEX *RootComplex,
>> @@ -1328,16 +1439,17 @@ Ac01PcieCoreUpdateLink (
>> // Doing link checking and recovery if needed
>> Ac01PcieCoreQoSLinkCheckRecovery (RootComplex, PcieIndex);
>>
>> - // Link timeout after 32ms
>> - SetLinkTimeout (RootComplex, PcieIndex, 32);
>> -
>> // Un-mask Completion Timeout
>> DisableCompletionTimeOut (RootComplex, PcieIndex, FALSE);
>>
>> } else {
>> - *IsNextRoundNeeded = FALSE;
>> FailedPciePtr[*FailedPcieCount] = PcieIndex;
>> *FailedPcieCount += 1;
>> +
>> + if (Ac01PcieCoreCheckCardPresent (Pcie)) {
>> + *IsNextRoundNeeded = TRUE;
>> + DEBUG ((DEBUG_INFO, "PCIE%d.%d Link retry\n", RootComplex->ID, PcieIndex));
>> + }
> Another 4-level if-statement.
> I'm not suggesting it's necessarily the latest addition that needs to
> be broken out as a helper, but 4 is too deep.
Thanks Leif a lot for the suggestions for improving the code readability.
Best regards,
Nhi
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100481): https://edk2.groups.io/g/devel/message/100481
Mute This Topic: https://groups.io/mt/96240128/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Urgh, I think I forgot to reply to this - apologies.
On Fri, Feb 24, 2023 at 10:26:42 +0700, Nhi Pham wrote:
> Hi Leif,
>
> Thanks for your reviewing. Most of your feedback is valid. I will fix them.
> There is a comment that need your more explanation.
>
> Please see the inline reply for more details.
>
> > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > index aa34a90b44c6..1346fa616ab3 100644
> > > --- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > +++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > @@ -37,7 +37,7 @@
> > > | Y | Y | Y | Y | 3 |
> > > ----------------------------------------
> > > - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> > > + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > @@ -149,6 +149,10 @@ GetDefaultDevMap (
> > > AC01_ROOT_COMPLEX *RootComplex
> > > )
> > > {
> > > + BOOLEAN IsAc01;
> > > +
> > > + IsAc01 = IsAc01Processor ();
> > > +
> > > if (RootComplex->Pcie[PcieController0].Active
> > > && RootComplex->Pcie[PcieController1].Active
> > > && RootComplex->Pcie[PcieController2].Active
> > > @@ -169,14 +173,20 @@ GetDefaultDevMap (
> > > && RootComplex->Pcie[PcieController5].Active
> > > && RootComplex->Pcie[PcieController6].Active
> > > && RootComplex->Pcie[PcieController7].Active) {
> > > - RootComplex->DefaultDevMapHigh = DevMapMode4;
> > > + if (IsAc01) {
> > > + RootComplex->DefaultDevMapHigh = DevMapMode4;
> > > + }
> >
> > Who sets RootComplex->DefaultDevMapHigh if !IsAc01?
>
> It will be the default value (0) because Ampere Altra Max does not have Root
> Complex Type B.
OK.
So, this just looks a bit like a maintenance nightmare.
There are so many places that individually check for a specific
platform.
And this is not the most readable file to begin with.
So looking at this *existing* function, it seems to be determining how
many PCIe controllers are active. Could we simplify it with some
arithmetic instead of translating truth tables to C conditionals?
If so, it looks like we could have a single IsAc01 check, covering
cases 2-4?
/
Leif
> >
> > > } else if (RootComplex->Pcie[PcieController4].Active
> > > && RootComplex->Pcie[PcieController6].Active
> > > && RootComplex->Pcie[PcieController7].Active) {
> > > - RootComplex->DefaultDevMapHigh = DevMapMode3;
> > > + if (IsAc01) {
> > > + RootComplex->DefaultDevMapHigh = DevMapMode3;
> > > + }
> > same
> >
> > > } else if (RootComplex->Pcie[PcieController4].Active
> > > && RootComplex->Pcie[PcieController6].Active) {
> > > - RootComplex->DefaultDevMapHigh = DevMapMode2;
> > > + if (IsAc01) {
> > > + RootComplex->DefaultDevMapHigh = DevMapMode2;
> > > + }
> > same
> >
> > > } else {
> > > RootComplex->DefaultDevMapHigh = DevMapMode1;
> > > }
> > I feel this function in general could be rewritten more reader-friendly.
> >
> > > @@ -203,12 +213,17 @@ GetLaneAllocation (
> > > EFI_STATUS Status;
> > > INTN RPIndex;
> > > NVPARAM NvParamOffset;
> > > - UINT32 Value, Width;
> > > + UINT32 Value, Width, Controller;
> > > // Retrieve lane allocation and capabilities for each controller
> > > if (RootComplex->Type == RootComplexTypeA) {
> > > - NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
> > > - NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
> > > + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
> > > + NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA4_CFG : NV_SI_RO_BOARD_S1_RCA4_CFG;
> > > + NvParamOffset += (RootComplex->ID - MaxPcieControllerOfRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > Helper function.
> >
> > > } else {
> > > //
> > > // There're two NVParam entries per RootComplexTypeB
> > > @@ -222,7 +237,13 @@ GetLaneAllocation (
> > > Value = 0;
> > > }
> > > - for (RPIndex = 0; RPIndex < MaxPcieControllerOfRootComplexA; RPIndex++) {
> > > + if (IsAc01Processor ()) {
> > > + Controller = MaxPcieControllerOfRootComplexA;
> > > + } else {
> > > + Controller = RootComplex->MaxPcieController;
> > > + }
> > Straightforward enough, but could still be a helper function.
> >
> > > +
> > > + for (RPIndex = PcieController0; RPIndex < Controller; RPIndex++) {
> > > Width = (Value >> (RPIndex * BITS_PER_BYTE)) & BYTE_MASK;
> > > switch (Width) {
> > > case 1:
> > > @@ -245,7 +266,7 @@ GetLaneAllocation (
> > > if (RootComplex->Type == RootComplexTypeB) {
> > > NvParamOffset += NV_PARAM_ENTRYSIZE;
> > > - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > if (EFI_ERROR (Status)) {
> > > Value = 0;
> > > }
> > > @@ -288,9 +309,17 @@ GetGen3PresetNvParamOffset (
> > > if (RootComplex->Socket == 0) {
> > > if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > When we get to 4 levels of if, we absolutely need helper functions.
> >
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > }
> > > } else {
> > > //
> > > @@ -300,9 +329,17 @@ GetGen3PresetNvParamOffset (
> > > }
> > > } else if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > }
> > > } else {
> > > //
> > > @@ -324,9 +361,17 @@ GetGen4PresetNvParamOffset (
> > > if (RootComplex->Socket == 0) {
> > > if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > }
> > > } else {
> > > //
> > > @@ -336,9 +381,17 @@ GetGen4PresetNvParamOffset (
> > > }
> > > } else if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > This also looks like the exact same computation takes place 4 times
> > for different combos of G4PRESETs and rootcompled IDs. That should be
> > possible to parametrise and compute in a single function instead.
> Correct, will improve it.
> >
> > > }
> > > } else {
> > > //
> > > @@ -352,7 +405,7 @@ GetGen4PresetNvParamOffset (
> > > VOID
> > > GetPresetSetting (
> > > - AC01_ROOT_COMPLEX *RootComplex
> > > + AC01_ROOT_COMPLEX *RootComplex
> > > )
> > > {
> > > EFI_STATUS Status;
> > > @@ -377,7 +430,7 @@ GetPresetSetting (
> > > if (RootComplex->Type == RootComplexTypeB) {
> > > NvParamOffset += NV_PARAM_ENTRYSIZE;
> > > - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > if (!EFI_ERROR (Status)) {
> > > for (Index = MaxPcieControllerOfRootComplexA; Index < MaxPcieController; Index++) {
> > > RootComplex->PresetGen3[Index] = (Value >> ((Index - MaxPcieControllerOfRootComplexA) * BITS_PER_BYTE)) & BYTE_MASK;
> > > @@ -414,7 +467,7 @@ GetMaxSpeedGen (
> > > UINT8 ErrataSpeedDevMap3[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN4, LINK_SPEED_GEN4, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 2: x8 x4 x4 (PCIE_ERRATA_SPEED1)
> > > UINT8 ErrataSpeedDevMap4[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 3: x4 x4 x4 x4 (PCIE_ERRATA_SPEED1)
> > > UINT8 ErrataSpeedRcb[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // RootComplexTypeB PCIE_ERRATA_SPEED1
> > > - UINT8 Idx;
> > > + UINT8 Idx, Controller;
> > One definition per line?
> Will fix.
> >
> > > UINT8 *MaxGen;
> > > ASSERT (MaxPcieControllerOfRootComplexA == 4);
> > > @@ -452,7 +505,13 @@ GetMaxSpeedGen (
> > > }
> > > }
> > > - for (Idx = 0; Idx < MaxPcieControllerOfRootComplexA; Idx++) {
> > > + if (IsAc01Processor ()) {
> > > + Controller = MaxPcieControllerOfRootComplexA;
> > > + } else {
> > > + Controller = RootComplex->MaxPcieController;
> > > + }
> > Straightforward enough, but could still be a helper function.
> >
> > > +
> > > + for (Idx = 0; Idx < Controller; Idx++) {
> > > RootComplex->Pcie[Idx].MaxGen = RootComplex->Pcie[Idx].Active ? MaxGen[Idx] : LINK_SPEED_NONE;
> > > }
> > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > index ad648b1b9efd..12bd2c14544e 100644
> > > --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > @@ -1,6 +1,6 @@
> > > /** @file
> > > - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
> > > + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > @@ -10,8 +10,10 @@
> > > #include <Guid/PlatformInfoHob.h>
> > > #include <Guid/RootComplexInfoHob.h>
> > > +#include <IndustryStandard/Pci.h>
> > > #include <Library/ArmGenericTimerCounterLib.h>
> > > #include <Library/BaseLib.h>
> > > +#include <Library/BaseMemoryLib.h>
> > > #include <Library/BoardPcieLib.h>
> > > #include <Library/DebugLib.h>
> > > #include <Library/HobLib.h>
> > > @@ -22,6 +24,25 @@
> > > #include "PcieCore.h"
> > > +VOID
> > > +EnableDbiAccess (
> > > + AC01_ROOT_COMPLEX *RootComplex,
> > > + UINT32 PcieIndex,
> > > + BOOLEAN EnableDbi
> > > + );
> > > +
> > > +BOOLEAN
> > > +EndpointCfgReady (
> > > + IN AC01_ROOT_COMPLEX *RootComplex,
> > > + IN UINT8 PcieIndex,
> > > + IN UINT32 Timeout
> > > + );
> > > +
> > > +BOOLEAN
> > > +PcieLinkUpCheck (
> > > + IN AC01_PCIE_CONTROLLER *Pcie
> > > + );
> > > +
> > > /**
> > > Return the next extended capability base address
> > > @@ -41,14 +62,38 @@ GetCapabilityBase (
> > > {
> > > BOOLEAN IsExtCapability = FALSE;
> > > PHYSICAL_ADDRESS CfgBase;
> > > + PHYSICAL_ADDRESS Ret = 0;
> > > + PHYSICAL_ADDRESS RootComplexCfgBase;
> > > UINT32 CapabilityId;
> > > UINT32 NextCapabilityPtr;
> > > UINT32 Val;
> > > + UINT32 RestoreVal;
> > > - if (IsRootComplex) {
> > > - CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
> > > - } else {
> > > + RootComplexCfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
> > > + if (!IsRootComplex) {
> > > + // Allow programming to config space
> > > + EnableDbiAccess (RootComplex, PcieIndex, TRUE);
> > > +
> > > + Val = MmioRead32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG);
> > > + RestoreVal = Val;
> > > + Val = SUB_BUS_SET (Val, DEFAULT_SUB_BUS);
> > > + Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
> > > + Val = PRIM_BUS_SET (Val, 0x0);
> > > + MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, Val);
> > > CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> > > +
> > > + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_TIMEOUT)) {
> > > + goto _CheckCapEnd;
> > > + }
> > > + } else {
> > > + CfgBase = RootComplexCfgBase;
> > > + }
> > > +
> > > + // Check if device provide capability
> > > + Val = MmioRead32 (CfgBase + PCI_COMMAND_OFFSET);
> > > + Val = GET_HIGH_16_BITS (Val); /* Status */
> > > + if (!(Val & EFI_PCI_STATUS_CAPABILITY)) {
> > > + goto _CheckCapEnd;
> > > }
> > > Val = MmioRead32 (CfgBase + TYPE1_CAP_PTR_REG);
> > > @@ -58,7 +103,8 @@ GetCapabilityBase (
> > > while (1) {
> > > if ((NextCapabilityPtr & WORD_ALIGN_MASK) != 0) {
> > > // Not alignment, just return
> > > - return 0;
> > > + Ret = 0;
> > > + goto _CheckCapEnd;
> > > }
> > > Val = MmioRead32 (CfgBase + NextCapabilityPtr);
> > > @@ -69,7 +115,8 @@ GetCapabilityBase (
> > > }
> > > if (CapabilityId == ExtCapabilityId) {
> > > - return (CfgBase + NextCapabilityPtr);
> > > + Ret = (CfgBase + NextCapabilityPtr);
> > > + goto _CheckCapEnd;
> > > }
> > > if (NextCapabilityPtr < EXT_CAPABILITY_START_BASE) {
> > > @@ -84,9 +131,20 @@ GetCapabilityBase (
> > > }
> > > if ((NextCapabilityPtr == 0) && IsExtCapability) {
> > > - return 0;
> > > + Ret = 0;
> > > + goto _CheckCapEnd;
> > > }
> > > }
> > > +
> > > +_CheckCapEnd:
> > > + if (!IsRootComplex) {
> > > + MmioWrite32 (RootComplexCfgBase + SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, RestoreVal);
> > > +
> > > + // Disable programming to config space
> > > + EnableDbiAccess (RootComplex, PcieIndex, FALSE);
> > > + }
> > > +
> > > + return Ret;
> > > }
> > > /**
> > > @@ -677,6 +735,14 @@ Ac01PcieCoreSetupRC (
> > > // Hold link training
> > > StartLinkTraining (RootComplex, PcieIndex, FALSE);
> > > + // Clear BUSCTRL.CfgUrMask to set CRS (Configuration Request Retry Status) to 0xFFFF.FFFF
> > > + // rather than 0xFFFF.0001 as per PCIe specification requirement. Otherwise, this causes
> > > + // device drivers respond incorrectly on timeout due to long device operations.
> > > + TargetAddress = CsrBase + AC01_PCIE_CORE_BUS_CONTROL_REG;
> > > + Val = MmioRead32 (TargetAddress);
> > > + Val &= ~BUS_CTL_CFG_UR_MASK;
> > > + MmioWrite32 (TargetAddress, Val);
> > > +
> > > if (!EnableAxiPipeClock (RootComplex, PcieIndex)) {
> > > DEBUG ((DEBUG_ERROR, "- Pcie[%d] - PIPE clock is not stable\n", PcieIndex));
> > > return RETURN_DEVICE_ERROR;
> > > @@ -688,9 +754,14 @@ Ac01PcieCoreSetupRC (
> > > // Allow programming to config space
> > > EnableDbiAccess (RootComplex, PcieIndex, TRUE);
> > > - // Program the power limit
> > > TargetAddress = CfgBase + PCIE_CAPABILITY_BASE + SLOT_CAPABILITIES_REG;
> > > Val = MmioRead32 (TargetAddress);
> > > + // 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.
> > I don't quite understand the comment.
> > Is this for netbooting, then inserting an NVMe drive after boot?
> This is a part of PCIe Hotplug feature that will be upstreamed later. Now we
> will remove this change.
> >
> > > + Val = SLOT_HPC_SET (Val, 1);
> > > + // Program the power limit
> > > Val = SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET (Val, SLOT_POWER_LIMIT_75W);
> > > MmioWrite32 (TargetAddress, Val);
> > > @@ -755,6 +826,7 @@ Ac01PcieCoreSetupRC (
> > > // Link timeout after 1ms
> > > SetLinkTimeout (RootComplex, PcieIndex, 1);
> > > + // Mask Completion Timeout
> > > DisableCompletionTimeOut (RootComplex, PcieIndex, TRUE);
> > > ProgramRootPortInfo (RootComplex, PcieIndex);
> > > @@ -1067,21 +1139,20 @@ Ac01PFACommand (
> > > return Ret;
> > > }
> > > -UINT32
> > > +BOOLEAN
> > > EndpointCfgReady (
> > > IN AC01_ROOT_COMPLEX *RootComplex,
> > > - IN UINT8 PcieIndex
> > > + IN UINT8 PcieIndex,
> > > + IN UINT32 TimeOut
> > > )
> > > {
> > > PHYSICAL_ADDRESS CfgBase;
> > > - UINT32 TimeOut;
> > > UINT32 Val;
> > > CfgBase = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> > > // Loop read CfgBase value until got valid value or
> > > - // reach to timeout EP_LINKUP_TIMEOUT (or more depend on card)
> > > - TimeOut = EP_LINKUP_TIMEOUT;
> > > + // reach to Timeout (or more depend on card)
> > > do {
> > > Val = MmioRead32 (CfgBase);
> > > if (Val != 0xFFFF0001 && Val != 0xFFFFFFFF) {
> > > @@ -1111,7 +1182,7 @@ Ac01PcieCoreGetEndpointInfo (
> > > OUT UINT8 *EpMaxGen
> > > )
> > > {
> > > - PHYSICAL_ADDRESS CfgBase;
> > > + PHYSICAL_ADDRESS CfgBase, EpCfgAddr;
> > One definition per line?
> >
> > > PHYSICAL_ADDRESS PcieCapBase;
> > > PHYSICAL_ADDRESS SecLatTimerAddr;
> > > PHYSICAL_ADDRESS TargetAddress;
> > > @@ -1133,8 +1204,23 @@ Ac01PcieCoreGetEndpointInfo (
> > > Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
> > > Val = PRIM_BUS_SET (Val, DEFAULT_PRIM_BUS);
> > > MmioWrite32 (SecLatTimerAddr, Val);
> > > + EpCfgAddr = RootComplex->MmcfgBase + (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> > > - if (EndpointCfgReady (RootComplex, PcieIndex)) {
> > > + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_EXTRA_TIMEOUT)) {
> > > + goto Exit;
> > > + }
> > > +
> > > + Val = MmioRead32 (EpCfgAddr);
> > > + // Check whether EP config space is accessible or not
> > > + if (Val == 0xFFFFFFFF) {
> > > + *EpMaxWidth = 0; // Invalid Width
> > > + *EpMaxGen = 0; // Invalid Speed
> > > + DEBUG ((DEBUG_ERROR, "PCIE%d.%d Cannot access EP config space!\n", RootComplex->ID, PcieIndex));
> > > + } else if (Val == 0xFFFF0001) {
> > > + *EpMaxWidth = 0; // Invalid Width
> > > + *EpMaxGen = 0; // Invalid Speed
> > > + DEBUG ((DEBUG_ERROR, "PCIE%d.%d EP config space still not ready to access, need poll more time!!!\n", RootComplex->ID, PcieIndex));
> > > + } else {
> > > PcieCapBase = GetCapabilityBase (RootComplex, PcieIndex, FALSE, PCIE_CAPABILITY_ID);
> > > if (PcieCapBase == 0) {
> > > DEBUG ((
> > > @@ -1164,6 +1250,7 @@ Ac01PcieCoreGetEndpointInfo (
> > > }
> > > }
> > > +Exit:
> > > // Restore value in order to not affect enumeration process
> > > MmioWrite32 (SecLatTimerAddr, RestoreVal);
> > > @@ -1280,6 +1367,30 @@ Ac01PcieCoreQoSLinkCheckRecovery (
> > > return LINK_CHECK_SUCCESS;
> > > }
> > > +BOOLEAN
> > > +Ac01PcieCoreCheckCardPresent (
> > > + IN AC01_PCIE_CONTROLLER *PcieController
> > > + )
> > > +{
> > > + EFI_PHYSICAL_ADDRESS TargetAddress;
> > > + UINT32 ControlValue;
> > > +
> > > + ControlValue = 0;
> > > +
> > > + TargetAddress = PcieController->CsrBase;
> > > +
> > > + ControlValue = MmioRead32 (TargetAddress + AC01_PCIE_CORE_LINK_CTRL_REG);
> > > +
> > > + if (0 == LTSSMENB_GET (ControlValue)) {
> > No jeopardy testing.
> Sorry, could you explain more about jeopardy testing?
> >
> > > + //
> > > + // LTSSMENB is clear to 0x00 by Hardware -> link partner is connected.
> > > + //
> > > + return TRUE;
> > > + }
> > > +
> > > + return FALSE;
> > > +}
> > > +
> > > VOID
> > > Ac01PcieCoreUpdateLink (
> > > IN AC01_ROOT_COMPLEX *RootComplex,
> > > @@ -1328,16 +1439,17 @@ Ac01PcieCoreUpdateLink (
> > > // Doing link checking and recovery if needed
> > > Ac01PcieCoreQoSLinkCheckRecovery (RootComplex, PcieIndex);
> > > - // Link timeout after 32ms
> > > - SetLinkTimeout (RootComplex, PcieIndex, 32);
> > > -
> > > // Un-mask Completion Timeout
> > > DisableCompletionTimeOut (RootComplex, PcieIndex, FALSE);
> > > } else {
> > > - *IsNextRoundNeeded = FALSE;
> > > FailedPciePtr[*FailedPcieCount] = PcieIndex;
> > > *FailedPcieCount += 1;
> > > +
> > > + if (Ac01PcieCoreCheckCardPresent (Pcie)) {
> > > + *IsNextRoundNeeded = TRUE;
> > > + DEBUG ((DEBUG_INFO, "PCIE%d.%d Link retry\n", RootComplex->ID, PcieIndex));
> > > + }
> > Another 4-level if-statement.
> > I'm not suggesting it's necessarily the latest addition that needs to
> > be broken out as a helper, but 4 is too deep.
>
> Thanks Leif a lot for the suggestions for improving the code readability.
>
> Best regards,
>
> Nhi
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101162): https://edk2.groups.io/g/devel/message/101162
Mute This Topic: https://groups.io/mt/96240128/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.