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 <vungu...@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 <n...@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.
Agree, will add helper functions. Also, check the later comments for this patch to improve the code readability.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.
It will be the default value (0) because Ampere Altra Max does not have Root Complex Type B.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.cindex 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 controllerif (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 (VOIDGetPresetSetting ( - 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.
This is a part of PCIe Hotplug feature that will be upstreamed later. Now we will remove this change.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.cindex 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 limitTargetAddress = 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 TimeoutDisableCompletionTimeOut (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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
