[edk2-devel] [edk2-platforms][PATCH 1/2] Ampere: PCIe: Add support for Ampere Altra Max
Leif Lindholm
quic_llindhol at quicinc.com
Wed Feb 15 11:59:02 UTC 2023
On Fri, Jan 13, 2023 at 11:25:16 +0700, Nhi Pham wrote:
> From: Vu Nguyen <vunguyen at 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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list