From: "Nhi Pham" <nhi@os.amperecomputing.com>
To: Leif Lindholm <quic_llindhol@quicinc.com>,
Nhi Pham <nhi@os.amperecomputing.com>
Cc: devel@edk2.groups.io, patches@amperecomputing.com,
ardb+tianocore@kernel.org,
Vu Nguyen <vunguyen@os.amperecomputing.com>
Subject: Re: [edk2-platforms][PATCH 1/2] Ampere: PCIe: Add support for Ampere Altra Max
Date: Fri, 24 Feb 2023 10:26:42 +0700 [thread overview]
Message-ID: <615aaced-6aca-91aa-0f7d-77ae51a1d6a9@amperemail.onmicrosoft.com> (raw)
In-Reply-To: <Y+zJBr+ymJaV1PS0@qc-i7.hemma.eciton.net>
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
next prev parent reply other threads:[~2023-02-24 3:26 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-13 4:25 [edk2-platforms][PATCH 0/2] Support Ampere Altra Max processor Nhi Pham
2023-01-13 4:25 ` [edk2-platforms][PATCH 1/2] Ampere: PCIe: Add support for Ampere Altra Max Nhi Pham
2023-02-15 11:59 ` Leif Lindholm
2023-02-24 3:26 ` Nhi Pham [this message]
2023-03-14 13:20 ` Leif Lindholm
2023-01-13 4:25 ` [edk2-platforms][PATCH 2/2] JadePkg: ACPI: Support ACPI tables " Nhi Pham
2023-02-15 12:03 ` [edk2-devel] " Leif Lindholm
2023-02-24 3:29 ` Nhi Pham
2023-02-24 18:03 ` Rebecca Cran
2023-03-29 4:49 ` [edk2-platforms][PATCH 0/2] Support Ampere Altra Max processor Nhi Pham
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=615aaced-6aca-91aa-0f7d-77ae51a1d6a9@amperemail.onmicrosoft.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox