Urgh, I think I forgot to reply to this - apologies.
On Fri, Feb 24, 2023 at 10:26:42 +0700, Nhi Pham wrote:
> Hi Leif,
>
> Thanks for your reviewing. Most of your feedback is valid. I will fix them.
> There is a comment that need your more explanation.
>
> Please see the inline reply for more details.
>
> > > diff --git
> > > a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > index aa34a90b44c6..1346fa616ab3 100644
> > > ---
> > > a/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > +++
> > > b/Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/RootComplexNVParam.c
> > > @@ -37,7 +37,7 @@
> > > | Y | Y | Y | Y | 3 |
> > > ----------------------------------------
> > > - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights
> > > reserved.<BR>
> > > + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights
> > > reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > @@ -149,6 +149,10 @@ GetDefaultDevMap (
> > > AC01_ROOT_COMPLEX *RootComplex
> > > )
> > > {
> > > + BOOLEAN IsAc01;
> > > +
> > > + IsAc01 = IsAc01Processor ();
> > > +
> > > if (RootComplex->Pcie[PcieController0].Active
> > > && RootComplex->Pcie[PcieController1].Active
> > > && RootComplex->Pcie[PcieController2].Active
> > > @@ -169,14 +173,20 @@ GetDefaultDevMap (
> > > && RootComplex->Pcie[PcieController5].Active
> > > && RootComplex->Pcie[PcieController6].Active
> > > && RootComplex->Pcie[PcieController7].Active) {
> > > - RootComplex->DefaultDevMapHigh = DevMapMode4;
> > > + if (IsAc01) {
> > > + RootComplex->DefaultDevMapHigh = DevMapMode4;
> > > + }
> >
> > Who sets RootComplex->DefaultDevMapHigh if !IsAc01?
>
> It will be the default value (0) because Ampere Altra Max does not have Root
> Complex Type B.
OK.
So, this just looks a bit like a maintenance nightmare.
There are so many places that individually check for a specific
platform.
And this is not the most readable file to begin with.
So looking at this *existing* function, it seems to be determining how
many PCIe controllers are active. Could we simplify it with some
arithmetic instead of translating truth tables to C conditionals?
If so, it looks like we could have a single IsAc01 check, covering
cases 2-4?
/
Leif
> >
> > > } else if (RootComplex->Pcie[PcieController4].Active
> > > && RootComplex->Pcie[PcieController6].Active
> > > && RootComplex->Pcie[PcieController7].Active) {
> > > - RootComplex->DefaultDevMapHigh = DevMapMode3;
> > > + if (IsAc01) {
> > > + RootComplex->DefaultDevMapHigh = DevMapMode3;
> > > + }
> > same
> >
> > > } else if (RootComplex->Pcie[PcieController4].Active
> > > && RootComplex->Pcie[PcieController6].Active) {
> > > - RootComplex->DefaultDevMapHigh = DevMapMode2;
> > > + if (IsAc01) {
> > > + RootComplex->DefaultDevMapHigh = DevMapMode2;
> > > + }
> > same
> >
> > > } else {
> > > RootComplex->DefaultDevMapHigh = DevMapMode1;
> > > }
> > I feel this function in general could be rewritten more reader-friendly.
> >
> > > @@ -203,12 +213,17 @@ GetLaneAllocation (
> > > EFI_STATUS Status;
> > > INTN RPIndex;
> > > NVPARAM NvParamOffset;
> > > - UINT32 Value, Width;
> > > + UINT32 Value, Width, Controller;
> > > // Retrieve lane allocation and capabilities for each controller
> > > if (RootComplex->Type == RootComplexTypeA) {
> > > - NvParamOffset = (RootComplex->Socket == 0) ?
> > > NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
> > > - NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + if (RootComplex->ID < MaxPcieControllerOfRootComplexA) {
> > > + NvParamOffset = (RootComplex->Socket == 0) ?
> > > NV_SI_RO_BOARD_S0_RCA0_CFG : NV_SI_RO_BOARD_S1_RCA0_CFG;
> > > + NvParamOffset += RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = (RootComplex->Socket == 0) ?
> > > NV_SI_RO_BOARD_S0_RCA4_CFG : NV_SI_RO_BOARD_S1_RCA4_CFG;
> > > + NvParamOffset += (RootComplex->ID -
> > > MaxPcieControllerOfRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > Helper function.
> >
> > > } else {
> > > //
> > > // There're two NVParam entries per RootComplexTypeB
> > > @@ -222,7 +237,13 @@ GetLaneAllocation (
> > > Value = 0;
> > > }
> > > - for (RPIndex = 0; RPIndex < MaxPcieControllerOfRootComplexA;
> > > RPIndex++) {
> > > + if (IsAc01Processor ()) {
> > > + Controller = MaxPcieControllerOfRootComplexA;
> > > + } else {
> > > + Controller = RootComplex->MaxPcieController;
> > > + }
> > Straightforward enough, but could still be a helper function.
> >
> > > +
> > > + for (RPIndex = PcieController0; RPIndex < Controller; RPIndex++) {
> > > Width = (Value >> (RPIndex * BITS_PER_BYTE)) & BYTE_MASK;
> > > switch (Width) {
> > > case 1:
> > > @@ -245,7 +266,7 @@ GetLaneAllocation (
> > > if (RootComplex->Type == RootComplexTypeB) {
> > > NvParamOffset += NV_PARAM_ENTRYSIZE;
> > > - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > if (EFI_ERROR (Status)) {
> > > Value = 0;
> > > }
> > > @@ -288,9 +309,17 @@ GetGen3PresetNvParamOffset (
> > > if (RootComplex->Socket == 0) {
> > > if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET +
> > > RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > When we get to 4 levels of if, we absolutely need helper functions.
> >
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G3PRESET +
> > > RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G3PRESET +
> > > RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G3PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G3PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > }
> > > } else {
> > > //
> > > @@ -300,9 +329,17 @@ GetGen3PresetNvParamOffset (
> > > }
> > > } else if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET +
> > > (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G3PRESET +
> > > (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G3PRESET +
> > > (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G3PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G3PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > }
> > > } else {
> > > //
> > > @@ -324,9 +361,17 @@ GetGen4PresetNvParamOffset (
> > > if (RootComplex->Socket == 0) {
> > > if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET +
> > > RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA0_TXRX_G4PRESET +
> > > RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA0_TXRX_G4PRESET +
> > > RootComplex->ID * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S0_RCA4_TXRX_G4PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S0_RCA4_TXRX_G4PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > }
> > > } else {
> > > //
> > > @@ -336,9 +381,17 @@ GetGen4PresetNvParamOffset (
> > > }
> > > } else if (RootComplex->Type == RootComplexTypeA) {
> > > if (RootComplex->ID < MaxRootComplexA) {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET +
> > > (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA2_TXRX_G4PRESET +
> > > (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA2_TXRX_G4PRESET +
> > > (RootComplex->ID - 2) * NV_PARAM_ENTRYSIZE;
> > > + }
> > > } else {
> > > - NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + if (IsAc01Processor ()) {
> > > + NvParamOffset = NV_SI_RO_BOARD_S1_RCA4_TXRX_G4PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + } else {
> > > + NvParamOffset = NV_SI_RO_BOARD_MQ_S1_RCA4_TXRX_G4PRESET +
> > > (RootComplex->ID - MaxRootComplexA) * NV_PARAM_ENTRYSIZE;
> > > + }
> > This also looks like the exact same computation takes place 4 times
> > for different combos of G4PRESETs and rootcompled IDs. That should be
> > possible to parametrise and compute in a single function instead.
> Correct, will improve it.
> >
> > > }
> > > } else {
> > > //
> > > @@ -352,7 +405,7 @@ GetGen4PresetNvParamOffset (
> > > VOID
> > > GetPresetSetting (
> > > - AC01_ROOT_COMPLEX *RootComplex
> > > + AC01_ROOT_COMPLEX *RootComplex
> > > )
> > > {
> > > EFI_STATUS Status;
> > > @@ -377,7 +430,7 @@ GetPresetSetting (
> > > if (RootComplex->Type == RootComplexTypeB) {
> > > NvParamOffset += NV_PARAM_ENTRYSIZE;
> > > - Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > + Status = NVParamGet (NvParamOffset, NV_PERM_ALL, &Value);
> > > if (!EFI_ERROR (Status)) {
> > > for (Index = MaxPcieControllerOfRootComplexA; Index <
> > > MaxPcieController; Index++) {
> > > RootComplex->PresetGen3[Index] = (Value >> ((Index -
> > > MaxPcieControllerOfRootComplexA) * BITS_PER_BYTE)) & BYTE_MASK;
> > > @@ -414,7 +467,7 @@ GetMaxSpeedGen (
> > > UINT8 ErrataSpeedDevMap3[MaxPcieControllerOfRootComplexA] = {
> > > LINK_SPEED_GEN4, LINK_SPEED_GEN4, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; //
> > > Bifurcation 2: x8 x4 x4 (PCIE_ERRATA_SPEED1)
> > > UINT8 ErrataSpeedDevMap4[MaxPcieControllerOfRootComplexA] = {
> > > LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 }; //
> > > Bifurcation 3: x4 x4 x4 x4 (PCIE_ERRATA_SPEED1)
> > > UINT8 ErrataSpeedRcb[MaxPcieControllerOfRootComplexA] = {
> > > LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1, LINK_SPEED_GEN1 };
> > > // RootComplexTypeB PCIE_ERRATA_SPEED1
> > > - UINT8 Idx;
> > > + UINT8 Idx, Controller;
> > One definition per line?
> Will fix.
> >
> > > UINT8 *MaxGen;
> > > ASSERT (MaxPcieControllerOfRootComplexA == 4);
> > > @@ -452,7 +505,13 @@ GetMaxSpeedGen (
> > > }
> > > }
> > > - for (Idx = 0; Idx < MaxPcieControllerOfRootComplexA; Idx++) {
> > > + if (IsAc01Processor ()) {
> > > + Controller = MaxPcieControllerOfRootComplexA;
> > > + } else {
> > > + Controller = RootComplex->MaxPcieController;
> > > + }
> > Straightforward enough, but could still be a helper function.
> >
> > > +
> > > + for (Idx = 0; Idx < Controller; Idx++) {
> > > RootComplex->Pcie[Idx].MaxGen = RootComplex->Pcie[Idx].Active ?
> > > MaxGen[Idx] : LINK_SPEED_NONE;
> > > }
> > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > index ad648b1b9efd..12bd2c14544e 100644
> > > --- a/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c
> > > @@ -1,6 +1,6 @@
> > > /** @file
> > > - Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights
> > > reserved.<BR>
> > > + Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights
> > > reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > @@ -10,8 +10,10 @@
> > > #include <Guid/PlatformInfoHob.h>
> > > #include <Guid/RootComplexInfoHob.h>
> > > +#include <IndustryStandard/Pci.h>
> > > #include <Library/ArmGenericTimerCounterLib.h>
> > > #include <Library/BaseLib.h>
> > > +#include <Library/BaseMemoryLib.h>
> > > #include <Library/BoardPcieLib.h>
> > > #include <Library/DebugLib.h>
> > > #include <Library/HobLib.h>
> > > @@ -22,6 +24,25 @@
> > > #include "PcieCore.h"
> > > +VOID
> > > +EnableDbiAccess (
> > > + AC01_ROOT_COMPLEX *RootComplex,
> > > + UINT32 PcieIndex,
> > > + BOOLEAN EnableDbi
> > > + );
> > > +
> > > +BOOLEAN
> > > +EndpointCfgReady (
> > > + IN AC01_ROOT_COMPLEX *RootComplex,
> > > + IN UINT8 PcieIndex,
> > > + IN UINT32 Timeout
> > > + );
> > > +
> > > +BOOLEAN
> > > +PcieLinkUpCheck (
> > > + IN AC01_PCIE_CONTROLLER *Pcie
> > > + );
> > > +
> > > /**
> > > Return the next extended capability base address
> > > @@ -41,14 +62,38 @@ GetCapabilityBase (
> > > {
> > > BOOLEAN IsExtCapability = FALSE;
> > > PHYSICAL_ADDRESS CfgBase;
> > > + PHYSICAL_ADDRESS Ret = 0;
> > > + PHYSICAL_ADDRESS RootComplexCfgBase;
> > > UINT32 CapabilityId;
> > > UINT32 NextCapabilityPtr;
> > > UINT32 Val;
> > > + UINT32 RestoreVal;
> > > - if (IsRootComplex) {
> > > - CfgBase = RootComplex->MmcfgBase +
> > > (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
> > > - } else {
> > > + RootComplexCfgBase = RootComplex->MmcfgBase +
> > > (RootComplex->Pcie[PcieIndex].DevNum << DEV_SHIFT);
> > > + if (!IsRootComplex) {
> > > + // Allow programming to config space
> > > + EnableDbiAccess (RootComplex, PcieIndex, TRUE);
> > > +
> > > + Val = MmioRead32 (RootComplexCfgBase +
> > > SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG);
> > > + RestoreVal = Val;
> > > + Val = SUB_BUS_SET (Val, DEFAULT_SUB_BUS);
> > > + Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
> > > + Val = PRIM_BUS_SET (Val, 0x0);
> > > + MmioWrite32 (RootComplexCfgBase +
> > > SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, Val);
> > > CfgBase = RootComplex->MmcfgBase +
> > > (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> > > +
> > > + if (!EndpointCfgReady (RootComplex, PcieIndex, EP_LINKUP_TIMEOUT)) {
> > > + goto _CheckCapEnd;
> > > + }
> > > + } else {
> > > + CfgBase = RootComplexCfgBase;
> > > + }
> > > +
> > > + // Check if device provide capability
> > > + Val = MmioRead32 (CfgBase + PCI_COMMAND_OFFSET);
> > > + Val = GET_HIGH_16_BITS (Val); /* Status */
> > > + if (!(Val & EFI_PCI_STATUS_CAPABILITY)) {
> > > + goto _CheckCapEnd;
> > > }
> > > Val = MmioRead32 (CfgBase + TYPE1_CAP_PTR_REG);
> > > @@ -58,7 +103,8 @@ GetCapabilityBase (
> > > while (1) {
> > > if ((NextCapabilityPtr & WORD_ALIGN_MASK) != 0) {
> > > // Not alignment, just return
> > > - return 0;
> > > + Ret = 0;
> > > + goto _CheckCapEnd;
> > > }
> > > Val = MmioRead32 (CfgBase + NextCapabilityPtr);
> > > @@ -69,7 +115,8 @@ GetCapabilityBase (
> > > }
> > > if (CapabilityId == ExtCapabilityId) {
> > > - return (CfgBase + NextCapabilityPtr);
> > > + Ret = (CfgBase + NextCapabilityPtr);
> > > + goto _CheckCapEnd;
> > > }
> > > if (NextCapabilityPtr < EXT_CAPABILITY_START_BASE) {
> > > @@ -84,9 +131,20 @@ GetCapabilityBase (
> > > }
> > > if ((NextCapabilityPtr == 0) && IsExtCapability) {
> > > - return 0;
> > > + Ret = 0;
> > > + goto _CheckCapEnd;
> > > }
> > > }
> > > +
> > > +_CheckCapEnd:
> > > + if (!IsRootComplex) {
> > > + MmioWrite32 (RootComplexCfgBase +
> > > SEC_LAT_TIMER_SUB_BUS_SEC_BUS_PRI_BUS_REG, RestoreVal);
> > > +
> > > + // Disable programming to config space
> > > + EnableDbiAccess (RootComplex, PcieIndex, FALSE);
> > > + }
> > > +
> > > + return Ret;
> > > }
> > > /**
> > > @@ -677,6 +735,14 @@ Ac01PcieCoreSetupRC (
> > > // Hold link training
> > > StartLinkTraining (RootComplex, PcieIndex, FALSE);
> > > + // Clear BUSCTRL.CfgUrMask to set CRS (Configuration Request Retry
> > > Status) to 0xFFFF.FFFF
> > > + // rather than 0xFFFF.0001 as per PCIe specification requirement.
> > > Otherwise, this causes
> > > + // device drivers respond incorrectly on timeout due to long device
> > > operations.
> > > + TargetAddress = CsrBase + AC01_PCIE_CORE_BUS_CONTROL_REG;
> > > + Val = MmioRead32 (TargetAddress);
> > > + Val &= ~BUS_CTL_CFG_UR_MASK;
> > > + MmioWrite32 (TargetAddress, Val);
> > > +
> > > if (!EnableAxiPipeClock (RootComplex, PcieIndex)) {
> > > DEBUG ((DEBUG_ERROR, "- Pcie[%d] - PIPE clock is not stable\n",
> > > PcieIndex));
> > > return RETURN_DEVICE_ERROR;
> > > @@ -688,9 +754,14 @@ Ac01PcieCoreSetupRC (
> > > // Allow programming to config space
> > > EnableDbiAccess (RootComplex, PcieIndex, TRUE);
> > > - // Program the power limit
> > > TargetAddress = CfgBase + PCIE_CAPABILITY_BASE +
> > > SLOT_CAPABILITIES_REG;
> > > Val = MmioRead32 (TargetAddress);
> > > + // In order to detect the NVMe disk for booting without disk,
> > > + // need to set Hot-Plug Slot Capable during port initialization.
> > > + // It will help PCI Linux driver to initialize its slot iomem
> > > resource,
> > > + // the one is used for detecting the disk when it is inserted.
> > I don't quite understand the comment.
> > Is this for netbooting, then inserting an NVMe drive after boot?
> This is a part of PCIe Hotplug feature that will be upstreamed later. Now we
> will remove this change.
> >
> > > + Val = SLOT_HPC_SET (Val, 1);
> > > + // Program the power limit
> > > Val = SLOT_CAP_SLOT_POWER_LIMIT_VALUE_SET (Val,
> > > SLOT_POWER_LIMIT_75W);
> > > MmioWrite32 (TargetAddress, Val);
> > > @@ -755,6 +826,7 @@ Ac01PcieCoreSetupRC (
> > > // Link timeout after 1ms
> > > SetLinkTimeout (RootComplex, PcieIndex, 1);
> > > + // Mask Completion Timeout
> > > DisableCompletionTimeOut (RootComplex, PcieIndex, TRUE);
> > > ProgramRootPortInfo (RootComplex, PcieIndex);
> > > @@ -1067,21 +1139,20 @@ Ac01PFACommand (
> > > return Ret;
> > > }
> > > -UINT32
> > > +BOOLEAN
> > > EndpointCfgReady (
> > > IN AC01_ROOT_COMPLEX *RootComplex,
> > > - IN UINT8 PcieIndex
> > > + IN UINT8 PcieIndex,
> > > + IN UINT32 TimeOut
> > > )
> > > {
> > > PHYSICAL_ADDRESS CfgBase;
> > > - UINT32 TimeOut;
> > > UINT32 Val;
> > > CfgBase = RootComplex->MmcfgBase +
> > > (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> > > // Loop read CfgBase value until got valid value or
> > > - // reach to timeout EP_LINKUP_TIMEOUT (or more depend on card)
> > > - TimeOut = EP_LINKUP_TIMEOUT;
> > > + // reach to Timeout (or more depend on card)
> > > do {
> > > Val = MmioRead32 (CfgBase);
> > > if (Val != 0xFFFF0001 && Val != 0xFFFFFFFF) {
> > > @@ -1111,7 +1182,7 @@ Ac01PcieCoreGetEndpointInfo (
> > > OUT UINT8 *EpMaxGen
> > > )
> > > {
> > > - PHYSICAL_ADDRESS CfgBase;
> > > + PHYSICAL_ADDRESS CfgBase, EpCfgAddr;
> > One definition per line?
> >
> > > PHYSICAL_ADDRESS PcieCapBase;
> > > PHYSICAL_ADDRESS SecLatTimerAddr;
> > > PHYSICAL_ADDRESS TargetAddress;
> > > @@ -1133,8 +1204,23 @@ Ac01PcieCoreGetEndpointInfo (
> > > Val = SEC_BUS_SET (Val, RootComplex->Pcie[PcieIndex].DevNum);
> > > Val = PRIM_BUS_SET (Val, DEFAULT_PRIM_BUS);
> > > MmioWrite32 (SecLatTimerAddr, Val);
> > > + EpCfgAddr = RootComplex->MmcfgBase +
> > > (RootComplex->Pcie[PcieIndex].DevNum << BUS_SHIFT);
> > > - if (EndpointCfgReady (RootComplex, PcieIndex)) {
> > > + if (!EndpointCfgReady (RootComplex, PcieIndex,
> > > EP_LINKUP_EXTRA_TIMEOUT)) {
> > > + goto Exit;
> > > + }
> > > +
> > > + Val = MmioRead32 (EpCfgAddr);
> > > + // Check whether EP config space is accessible or not
> > > + if (Val == 0xFFFFFFFF) {
> > > + *EpMaxWidth = 0; // Invalid Width
> > > + *EpMaxGen = 0; // Invalid Speed
> > > + DEBUG ((DEBUG_ERROR, "PCIE%d.%d Cannot access EP config space!\n",
> > > RootComplex->ID, PcieIndex));
> > > + } else if (Val == 0xFFFF0001) {
> > > + *EpMaxWidth = 0; // Invalid Width
> > > + *EpMaxGen = 0; // Invalid Speed
> > > + DEBUG ((DEBUG_ERROR, "PCIE%d.%d EP config space still not ready to
> > > access, need poll more time!!!\n", RootComplex->ID, PcieIndex));
> > > + } else {
> > > PcieCapBase = GetCapabilityBase (RootComplex, PcieIndex, FALSE,
> > > PCIE_CAPABILITY_ID);
> > > if (PcieCapBase == 0) {
> > > DEBUG ((
> > > @@ -1164,6 +1250,7 @@ Ac01PcieCoreGetEndpointInfo (
> > > }
> > > }
> > > +Exit:
> > > // Restore value in order to not affect enumeration process
> > > MmioWrite32 (SecLatTimerAddr, RestoreVal);
> > > @@ -1280,6 +1367,30 @@ Ac01PcieCoreQoSLinkCheckRecovery (
> > > return LINK_CHECK_SUCCESS;
> > > }
> > > +BOOLEAN
> > > +Ac01PcieCoreCheckCardPresent (
> > > + IN AC01_PCIE_CONTROLLER *PcieController
> > > + )
> > > +{
> > > + EFI_PHYSICAL_ADDRESS TargetAddress;
> > > + UINT32 ControlValue;
> > > +
> > > + ControlValue = 0;
> > > +
> > > + TargetAddress = PcieController->CsrBase;
> > > +
> > > + ControlValue = MmioRead32 (TargetAddress +
> > > AC01_PCIE_CORE_LINK_CTRL_REG);
> > > +
> > > + if (0 == LTSSMENB_GET (ControlValue)) {
> > No jeopardy testing.
> Sorry, could you explain more about jeopardy testing?
> >
> > > + //
> > > + // LTSSMENB is clear to 0x00 by Hardware -> link partner is
> > > connected.
> > > + //
> > > + return TRUE;
> > > + }
> > > +
> > > + return FALSE;
> > > +}
> > > +
> > > VOID
> > > Ac01PcieCoreUpdateLink (
> > > IN AC01_ROOT_COMPLEX *RootComplex,
> > > @@ -1328,16 +1439,17 @@ Ac01PcieCoreUpdateLink (
> > > // Doing link checking and recovery if needed
> > > Ac01PcieCoreQoSLinkCheckRecovery (RootComplex, PcieIndex);
> > > - // Link timeout after 32ms
> > > - SetLinkTimeout (RootComplex, PcieIndex, 32);
> > > -
> > > // Un-mask Completion Timeout
> > > DisableCompletionTimeOut (RootComplex, PcieIndex, FALSE);
> > > } else {
> > > - *IsNextRoundNeeded = FALSE;
> > > FailedPciePtr[*FailedPcieCount] = PcieIndex;
> > > *FailedPcieCount += 1;
> > > +
> > > + if (Ac01PcieCoreCheckCardPresent (Pcie)) {
> > > + *IsNextRoundNeeded = TRUE;
> > > + DEBUG ((DEBUG_INFO, "PCIE%d.%d Link retry\n", RootComplex->ID,
> > > PcieIndex));
> > > + }
> > Another 4-level if-statement.
> > I'm not suggesting it's necessarily the latest addition that needs to
> > be broken out as a helper, but 4 is too deep.
>
> Thanks Leif a lot for the suggestions for improving the code readability.
>
> Best regards,
>
> Nhi
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101162): https://edk2.groups.io/g/devel/message/101162
Mute This Topic: https://groups.io/mt/96240128/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
