From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by mx.groups.io with SMTP id smtpd.web10.8577.1678800035679780863 for ; Tue, 14 Mar 2023 06:20:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=dPr44bmU; spf=permerror, err=parse error for token &{10 18 %{ir}.%{v}.%{d}.spf.has.pphosted.com}: invalid domain name (domain: quicinc.com, ip: 205.220.168.131, mailfrom: quic_llindhol@quicinc.com) Received: from pps.filterd (m0279864.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 32EBhp3f028831; Tue, 14 Mar 2023 13:20:32 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=qcppdkim1; bh=Evj/xpz28F3e+Hi49JLTenXZqQWiyVijd77VwUgYZeM=; b=dPr44bmUpW52sRX8Its+lV/kcnLGLho2l8WYQ0sh4DBQbpb313ExerQiFtQhxYPeOEpa L5t06XovtYVLm+/zVzcdZSzMBuABXMdSQAk0e7RxBD7qFVz8eskXeSBEAFXclOZjTMsl NCBEjh70DAXeRJUq91/gly+u26oAbfpkprQ2wm4EO3IE++zMnX7KeVfhg/7gTV2Jg9FR 26+OTZYK+hnK8fpFRh3ZOeKLIAcY0Rs0oc67YMcXDECyToJgb6DcDb9zJqmGqQu20Acs SzPdJ6tfFjLaF8TgQeioF5Rm6Nw7P3rON0iwakTqsGQLboRDI2SG47Ks066P3k9Nuh+r vw== Received: from nasanppmta02.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3pa7w6trtv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 14 Mar 2023 13:20:32 +0000 Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 32EDKVxE015963 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 14 Mar 2023 13:20:31 GMT Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Tue, 14 Mar 2023 06:20:29 -0700 Date: Tue, 14 Mar 2023 13:20:26 +0000 From: "Leif Lindholm" To: Nhi Pham CC: Nhi Pham , , , , Vu Nguyen Subject: Re: [edk2-platforms][PATCH 1/2] Ampere: PCIe: Add support for Ampere Altra Max Message-ID: References: <20230113042517.3107802-1-nhi@os.amperecomputing.com> <20230113042517.3107802-2-nhi@os.amperecomputing.com> <615aaced-6aca-91aa-0f7d-77ae51a1d6a9@amperemail.onmicrosoft.com> MIME-Version: 1.0 In-Reply-To: <615aaced-6aca-91aa-0f7d-77ae51a1d6a9@amperemail.onmicrosoft.com> X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: pPR_9BNjznefTpSC2Vf9rFcoCa6q4RyA X-Proofpoint-GUID: pPR_9BNjznefTpSC2Vf9rFcoCa6q4RyA X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-03-14_06,2023-03-14_02,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 mlxlogscore=999 malwarescore=0 impostorscore=0 phishscore=0 mlxscore=0 bulkscore=0 priorityscore=1501 clxscore=1015 lowpriorityscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2303140114 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline Urgh, I think I forgot to reply to this - apologies. On Fri, Feb 24, 2023 at 10:26:42 +0700, Nhi Pham wrote: > Hi Leif, > > Thanks for your reviewing. Most of your feedback is valid. I will fix them. > There is a comment that need your more explanation. > > Please see the inline reply for more details. > > > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c > > > index aa34a90b44c6..1346fa616ab3 100644 > > > --- a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c > > > +++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c > > > @@ -37,7 +37,7 @@ > > > | Y | Y | Y | Y | 3 | > > > ---------------------------------------- > > > - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.
> > > + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.
> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > @@ -149,6 +149,10 @@ GetDefaultDevMap ( > > > AC01_ROOT_COMPLEX *RootComplex > > > ) > > > { > > > + BOOLEAN IsAc01; > > > + > > > + IsAc01 = IsAc01Processor (); > > > + > > > if (RootComplex->Pcie[PcieController0].Active > > > && RootComplex->Pcie[PcieController1].Active > > > && RootComplex->Pcie[PcieController2].Active > > > @@ -169,14 +173,20 @@ GetDefaultDevMap ( > > > && RootComplex->Pcie[PcieController5].Active > > > && RootComplex->Pcie[PcieController6].Active > > > && RootComplex->Pcie[PcieController7].Active) { > > > - RootComplex->DefaultDevMapHigh = DevMapMode4; > > > + if (IsAc01) { > > > + RootComplex->DefaultDevMapHigh = DevMapMode4; > > > + } > > > > Who sets RootComplex->DefaultDevMapHigh if !IsAc01? > > It will be the default value (0) because Ampere Altra Max does not have Root > Complex Type B. OK. So, this just looks a bit like a maintenance nightmare. There are so many places that individually check for a specific platform. And this is not the most readable file to begin with. So looking at this *existing* function, it seems to be determining how many PCIe controllers are active. Could we simplify it with some arithmetic instead of translating truth tables to C conditionals? If so, it looks like we could have a single IsAc01 check, covering cases 2-4? / Leif > > > > > } else if (RootComplex->Pcie[PcieController4].Active > > > && RootComplex->Pcie[PcieController6].Active > > > && RootComplex->Pcie[PcieController7].Active) { > > > - RootComplex->DefaultDevMapHigh = DevMapMode3; > > > + if (IsAc01) { > > > + RootComplex->DefaultDevMapHigh = DevMapMode3; > > > + } > > same > > > > > } else if (RootComplex->Pcie[PcieController4].Active > > > && RootComplex->Pcie[PcieController6].Active) { > > > - RootComplex->DefaultDevMapHigh = DevMapMode2; > > > + if (IsAc01) { > > > + RootComplex->DefaultDevMapHigh = DevMapMode2; > > > + } > > same > > > > > } else { > > > RootComplex->DefaultDevMapHigh = DevMapMode1; > > > } > > I feel this function in general could be rewritten more reader-friendly. > > > > > @@ -203,12 +213,17 @@ GetLaneAllocation ( > > > EFI_STATUS Status; > > > INTN RPIndex; > > > NVPARAM NvParamOffset; > > > - UINT32 Value, Width; > > > + UINT32 Value, Width, Controller; > > > // Retrieve lane allocation and capabilities for each controller > > > if (RootComplex->Type == RootComplexTypeA) { > > > - NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG; > > > - NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) { > > > + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG; > > > + NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = (RootComplex->Socket == 0) ? NV_SI_RO_BOARD_S0_RCA4_CFG : NV_SI_RO_BOARD_S1_RCA4_CFG; > > > + NvParamOffset += (RootComplex->ID - MaxPcieControllerOfRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } > > Helper function. > > > > > } else { > > > // > > > // There're two NVParam entries per RootComplexTypeB > > > @@ -222,7 +237,13 @@ GetLaneAllocation ( > > > Value = 0; > > > } > > > - for (RPIndex = 0; RPIndex < MaxPcieControllerOfRootComplexA; RPIndex++) { > > > + if (IsAc01Processor ()) { > > > + Controller = MaxPcieControllerOfRootComplexA; > > > + } else { > > > + Controller = RootComplex->MaxPcieController; > > > + } > > Straightforward enough, but could still be a helper function. > > > > > + > > > + for (RPIndex = PcieController0; RPIndex < Controller; RPIndex++) { > > > Width = (Value >> (RPIndex * BITS_PER_BYTE)) & BYTE_MASK; > > > switch (Width) { > > > case 1: > > > @@ -245,7 +266,7 @@ GetLaneAllocation ( > > > if (RootComplex->Type == RootComplexTypeB) { > > > NvParamOffset += NV_PARAM_ENTRYSIZE; > > > - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value); > > > + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value); > > > if (EFI_ERROR (Status)) { > > > Value = 0; > > > } > > > @@ -288,9 +309,17 @@ GetGen3PresetNvParamOffset ( > > > if (RootComplex->Socket == 0) { > > > if (RootComplex->Type == RootComplexTypeA) { > > > if (RootComplex->ID < MaxRootComplexA) { > > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > When we get to 4 levels of if, we absolutely need helper functions. > > > > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + } > > > } else { > > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } > > > } > > > } else { > > > // > > > @@ -300,9 +329,17 @@ GetGen3PresetNvParamOffset ( > > > } > > > } else if (RootComplex->Type == RootComplexTypeA) { > > > if (RootComplex->ID < MaxRootComplexA) { > > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE; > > > + } > > > } else { > > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } > > > } > > > } else { > > > // > > > @@ -324,9 +361,17 @@ GetGen4PresetNvParamOffset ( > > > if (RootComplex->Socket == 0) { > > > if (RootComplex->Type == RootComplexTypeA) { > > > if (RootComplex->ID < MaxRootComplexA) { > > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET + RootComplex->ID * NV_PARAM_ENTRYSIZE; > > > + } > > > } else { > > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } > > > } > > > } else { > > > // > > > @@ -336,9 +381,17 @@ GetGen4PresetNvParamOffset ( > > > } > > > } else if (RootComplex->Type == RootComplexTypeA) { > > > if (RootComplex->ID < MaxRootComplexA) { > > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET + (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE; > > > + } > > > } else { > > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + if (IsAc01Processor ()) { > > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } else { > > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET + (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE; > > > + } > > This also looks like the exact same computation takes place 4 times > > for different combos of G4PRESETs and rootcompled IDs. That should be > > possible to parametrise and compute in a single function instead. > Correct, will improve it. > > > > > } > > > } else { > > > // > > > @@ -352,7 +405,7 @@ GetGen4PresetNvParamOffset ( > > > VOID > > > GetPresetSetting ( > > > - AC01_ROOT_COMPLEX *RootComplex > > > + AC01_ROOT_COMPLEX *RootComplex > > > ) > > > { > > > EFI_STATUS Status; > > > @@ -377,7 +430,7 @@ GetPresetSetting ( > > > if (RootComplex->Type == RootComplexTypeB) { > > > NvParamOffset += NV_PARAM_ENTRYSIZE; > > > - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value); > > > + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value); > > > if (!EFI_ERROR (Status)) { > > > for (Index = MaxPcieControllerOfRootComplexA; Index < MaxPcieController; Index++) { > > > RootComplex->PresetGen3[Index] = (Value >> ((Index - MaxPcieControllerOfRootComplexA) * BITS_PER_BYTE)) & BYTE_MASK; > > > @@ -414,7 +467,7 @@ GetMaxSpeedGen ( > > > UINT8 ErrataSpeedDevMap3[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN4, LINK_SPEED_GEN4, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 2: x8 x4 x4 (PCIE_ERRATA_SPEED1) > > > UINT8 ErrataSpeedDevMap4[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // Bifurcation 3: x4 x4 x4 x4 (PCIE_ERRATA_SPEED1) > > > UINT8 ErrataSpeedRcb[MaxPcieControllerOfRootComplexA] = { LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; // RootComplexTypeB PCIE_ERRATA_SPEED1 > > > - UINT8 Idx; > > > + UINT8 Idx, Controller; > > One definition per line? > Will fix. > > > > > UINT8 *MaxGen; > > > ASSERT (MaxPcieControllerOfRootComplexA == 4); > > > @@ -452,7 +505,13 @@ GetMaxSpeedGen ( > > > } > > > } > > > - for (Idx = 0; Idx < MaxPcieControllerOfRootComplexA; Idx++) { > > > + if (IsAc01Processor ()) { > > > + Controller = MaxPcieControllerOfRootComplexA; > > > + } else { > > > + Controller = RootComplex->MaxPcieController; > > > + } > > Straightforward enough, but could still be a helper function. > > > > > + > > > + for (Idx = 0; Idx < Controller; Idx++) { > > > RootComplex->Pcie[Idx].MaxGen = RootComplex->Pcie[Idx].Active ? MaxGen[Idx] : LINK_SPEED_NONE; > > > } > > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c > > > index ad648b1b9efd..12bd2c14544e 100644 > > > --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c > > > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c > > > @@ -1,6 +1,6 @@ > > > /** @file > > > - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.
> > > + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.
> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > @@ -10,8 +10,10 @@ > > > #include > > > #include > > > +#include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -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 >