| Message ID | 1490015485-53685-5-git-send-email-chenhui.sun@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | D02/D03 platforms bug fix | expand |
On Mon, Mar 20, 2017 at 09:11:08PM +0800, Chenhui Sun wrote: > In some cases, the PCIe device may close part of lanes in > config state of LTSSM, the hip06 RC should reconfig lane number > and try to linkup again. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Chenhui Sun <sunchenhui@huawei.com> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > Signed-off-by: Yi Li <phoenix.liyi@huawei.com> > --- > .../Hi1610/Drivers/PcieInit1610/PcieInitLib.c | 121 ++++++++++++++++++++- > Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h | 3 + > 2 files changed, 122 insertions(+), 2 deletions(-) > > diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > index 57699e0..b45d54f 100644 > --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > @@ -42,6 +42,7 @@ extern PCIE_DRIVER_CFG gastr_pcie_driver_cfg; > extern PCIE_IATU gastr_pcie_iatu_cfg; > extern PCIE_IATU_VA mPcieIatuTable; > > +EFI_STATUS PciePortInit (UINT32 soctype, UINT32 HostBridgeNum, PCIE_DRIVER_CFG *PcieCfg); A blank line after this definition would be nice. Also, I think the EFI_API needs to be here as well. > VOID PcieRegWrite(UINT32 Port, UINTN Offset, UINT32 Value) > { > RegWrite((UINT64)mPcieIntCfg.RegResource[Port] + Offset, Value); Space between function name and "(". This applies globally in this patch (I won't repeat this comment). (But no space between the (UINT64) and mPcieInitCfg, that bit is done the correct way.) > @@ -152,8 +153,123 @@ VOID PcieRxValidCtrl(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, BOOLEAN > } > } > } > +/* > + * The ltssm register is assigned in an asynchronous way, the value > + * of register may not right in metastable state. > + * Read the register twice to get stable value. > + */ > +VOID PcieGetLtssmValue ( > + UINT32 HostBridgeNum, > + UINT32 Port, > + UINT32* Value UINT32 *Value > +) > +{ > + UINT32 ValueA; > + UINT32 ValueB = 0; > + UINT32 Count; > + > + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueA); > + ValueA = ValueA & PCIE_LTSSM_STATE_MASK; > + > + Count = 0; > + while (Count < 2) { > + > + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueB); > + ValueB = ValueB & PCIE_LTSSM_STATE_MASK; > + > + /* Get the same state in continuous two times*/ > + if (ValueA == ValueB) { > + break; > + } > + > + /* If the second value not equel to the first, "equel" -> "equal". > + * we return the second one as the stable Also, this fits on a single line, so please join them. > + */ > + ValueA = ValueB; > + Count++; > + } > + > + *Value = ValueB; > + > + return; > + > +} > + > +/* > + * In some cases, the PCIe device may close part of lanes in > + * config state of LTSSM, the hip06 RC should reconfig lane num > + * and try to linkup again. > + */ > +VOID PcieReconfigLaneNum ( > + UINT32 soctype, > + UINT32 HostBridgeNum, > + UINT32 Port, > + PCIE_DRIVER_CFG *PcieCfg > +) > +{ > + EFI_STATUS Status; > + UINT32 LtssmStatus; > + UINT32 RegVal; > + UINT32 LoopCnt = 0; > + UINT32 LaneNumCnt = 0; > + PCIE_PORT_WIDTH PortWidth = PcieCfg->PortInfo.PortWidth; > + > + // 500 * 200us = 100ms, so it takes 100 ms must to reconfig lane numbers > + while (LoopCnt < 500) { > + > + /* > + * The minimum lanenum is 1, no need to try any more. > + */ Please align the * characters. > + if (PortWidth <= 1) { > + DEBUG((DEBUG_ERROR, "PcieReconfigLanenum PortWidth <= 1 !\n")); > + return; > + } > + > + /* > + * Check the lane num config state is normal or not. > + */ > + PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); > + if ((LtssmStatus == PCIE_LTSSM_CFG_LANENUM_ACPT) || (LtssmStatus == PCIE_LTSSM_CFG_COMPLETE)) { > + LaneNumCnt++; > + } else if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { > + PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); > + if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { > + break; > + } > + } else { > + LaneNumCnt = 0; > + } > + > + /* > + * The lane num config state is abnormal, need to reconfig > + * the lane num and try to establish link again. > + */ > + if (LaneNumCnt > 5) { Why 5? Could there be a #define with a descriptive name? > + /* Disable LTSSM */ > + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); > + RegVal &= ~(BIT11); What is BIT11? Can there be a comment explaining > + RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); > + PcieCfg->PortInfo.PortWidth = (PCIE_PORT_WIDTH)((UINT8)PcieCfg->PortInfo.PortWidth >> 1); This shift deserves a comment. Also, consider if PcieCfg->PortInfo.PortWidth >>= 1; would not be more clear. > + > + Status = PciePortInit(soctype, HostBridgeNum, PcieCfg); > + if(EFI_ERROR(Status)) { > + DEBUG((DEBUG_ERROR, "PcieReconfigLanenum HostBridge %d, Pcie Port %d Init Failed! \n", HostBridgeNum, Port)); > + } > + return; > + } > + > + LoopCnt++; > + /* Pcie 3.0 Spec,part 4.2.6.3.4.1: the Upstream Lanes are permitted > + * delay up to 1 ms before transitioning to Configuration.Lanenum.Accept. > + * So the delay time 200 us * 5(LanNumCnt) = 1ms, not beyond the reasonable range. > + */ This comment is excellent, thank you. > + MicroSecondDelay(200); > + } > + > + return ; > +} > > -EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > +EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, PCIE_DRIVER_CFG *PcieCfg) > { > PCIE_CTRL_7_U pcie_ctrl7; > UINT32 Value = 0; > @@ -168,6 +284,7 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > Value |= BIT11|BIT30|BIT31; > RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + 0x1114, Value); > (VOID)PcieRxValidCtrl(soctype, HostBridgeNum, Port, 1); > + PcieReconfigLaneNum(soctype, HostBridgeNum, Port, PcieCfg); > return EFI_SUCCESS; > } > else > @@ -1005,7 +1122,7 @@ PciePortInit ( > /* Disable RC Option Rom */ > DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType); > /* assert LTSSM enable */ > - (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); > + (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex, PcieCfg); > if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) { > //PCIe will still work even if performance tuning fails, > //and there is warning message inside the function to print > diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > index 539d567..b750910 100644 > --- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > +++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > @@ -8982,6 +8982,7 @@ typedef union tagIepMsiCtrlIntStatus > #define PCIE_SYS_CTRL28_REG (PCI_SYS_BASE + 0x1c4) > #define PCIE_SYS_CTRL29_REG (PCI_SYS_BASE + 0x1c8) > #define PCIE_SYS_CTRL54_REG (PCI_SYS_BASE + 0x274) > +#define PCIE_SYS_STATE4_REG (PCI_SYS_BASE + 0x31C) > #define PCIE_SYS_STATE5_REG (PCI_SYS_BASE + 0x30) > #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) > #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38) > @@ -12694,6 +12695,8 @@ typedef union tagPortlogic93 > #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE3_REG (PCIE_SUBCTRL_BASE + 0x6814) > #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE4_REG (PCIE_SUBCTRL_BASE + 0x6818) > #define PCIE_LTSSM_STATE_MASK (0x3f) > +#define PCIE_LTSSM_CFG_LANENUM_ACPT 0x0a > +#define PCIE_LTSSM_CFG_COMPLETE 0x0b > #define PCIE_LTSSM_LINKUP_STATE (0x11) > #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS0_REG (PCIE_SUBCTRL_BASE + 0x6880) > #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS1_REG (PCIE_SUBCTRL_BASE + 0x6884) > -- > 1.9.1 >
Hi Leif, 在 2017/3/21 3:28, Leif Lindholm 写道: > On Mon, Mar 20, 2017 at 09:11:08PM +0800, Chenhui Sun wrote: >> In some cases, the PCIe device may close part of lanes in >> config state of LTSSM, the hip06 RC should reconfig lane number >> and try to linkup again. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Chenhui Sun <sunchenhui@huawei.com> >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >> Signed-off-by: Yi Li <phoenix.liyi@huawei.com> >> --- >> .../Hi1610/Drivers/PcieInit1610/PcieInitLib.c | 121 ++++++++++++++++++++- >> Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h | 3 + >> 2 files changed, 122 insertions(+), 2 deletions(-) >> >> diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c >> index 57699e0..b45d54f 100644 >> --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c >> +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c >> @@ -42,6 +42,7 @@ extern PCIE_DRIVER_CFG gastr_pcie_driver_cfg; >> extern PCIE_IATU gastr_pcie_iatu_cfg; >> extern PCIE_IATU_VA mPcieIatuTable; >> >> +EFI_STATUS PciePortInit (UINT32 soctype, UINT32 HostBridgeNum, PCIE_DRIVER_CFG *PcieCfg); > A blank line after this definition would be nice. > Also, I think the EFI_API needs to be here as well. EFI_STATUS EFIAPI PciePortInit ( IN UINT32 soctype, IN UINT32 HostBridgeNum, IN PCIE_DRIVER_CFG *PcieCfg ); Is this the correct style? > >> VOID PcieRegWrite(UINT32 Port, UINTN Offset, UINT32 Value) >> { >> RegWrite((UINT64)mPcieIntCfg.RegResource[Port] + Offset, Value); > Space between function name and "(". This applies globally in this > patch (I won't repeat this comment). > > (But no space between the (UINT64) and mPcieInitCfg, that bit is done > the correct way.) This code is not the new added, so I will refine the follow new added code according your comments. is that ok? Thanks Chenhui >> @@ -152,8 +153,123 @@ VOID PcieRxValidCtrl(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, BOOLEAN >> } >> } >> } >> +/* >> + * The ltssm register is assigned in an asynchronous way, the value >> + * of register may not right in metastable state. >> + * Read the register twice to get stable value. >> + */ >> +VOID PcieGetLtssmValue ( >> + UINT32 HostBridgeNum, >> + UINT32 Port, >> + UINT32* Value > UINT32 *Value > >> +) >> +{ >> + UINT32 ValueA; >> + UINT32 ValueB = 0; >> + UINT32 Count; >> + >> + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueA); >> + ValueA = ValueA & PCIE_LTSSM_STATE_MASK; >> + >> + Count = 0; >> + while (Count < 2) { >> + >> + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueB); >> + ValueB = ValueB & PCIE_LTSSM_STATE_MASK; >> + >> + /* Get the same state in continuous two times*/ >> + if (ValueA == ValueB) { >> + break; >> + } >> + >> + /* If the second value not equel to the first, > "equel" -> "equal". > >> + * we return the second one as the stable > Also, this fits on a single line, so please join them. > >> + */ >> + ValueA = ValueB; >> + Count++; >> + } >> + >> + *Value = ValueB; >> + >> + return; >> + >> +} >> + >> +/* >> + * In some cases, the PCIe device may close part of lanes in >> + * config state of LTSSM, the hip06 RC should reconfig lane num >> + * and try to linkup again. >> + */ >> +VOID PcieReconfigLaneNum ( >> + UINT32 soctype, >> + UINT32 HostBridgeNum, >> + UINT32 Port, >> + PCIE_DRIVER_CFG *PcieCfg >> +) >> +{ >> + EFI_STATUS Status; >> + UINT32 LtssmStatus; >> + UINT32 RegVal; >> + UINT32 LoopCnt = 0; >> + UINT32 LaneNumCnt = 0; >> + PCIE_PORT_WIDTH PortWidth = PcieCfg->PortInfo.PortWidth; >> + >> + // 500 * 200us = 100ms, so it takes 100 ms must to reconfig lane numbers >> + while (LoopCnt < 500) { >> + >> + /* >> + * The minimum lanenum is 1, no need to try any more. >> + */ > Please align the * characters. > >> + if (PortWidth <= 1) { >> + DEBUG((DEBUG_ERROR, "PcieReconfigLanenum PortWidth <= 1 !\n")); >> + return; >> + } >> + >> + /* >> + * Check the lane num config state is normal or not. >> + */ >> + PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); >> + if ((LtssmStatus == PCIE_LTSSM_CFG_LANENUM_ACPT) || (LtssmStatus == PCIE_LTSSM_CFG_COMPLETE)) { >> + LaneNumCnt++; >> + } else if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { >> + PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); >> + if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { >> + break; >> + } >> + } else { >> + LaneNumCnt = 0; >> + } >> + >> + /* >> + * The lane num config state is abnormal, need to reconfig >> + * the lane num and try to establish link again. >> + */ >> + if (LaneNumCnt > 5) { > Why 5? Could there be a #define with a descriptive name? > >> + /* Disable LTSSM */ >> + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); >> + RegVal &= ~(BIT11); > What is BIT11? > Can there be a comment explaining > >> + RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); >> + PcieCfg->PortInfo.PortWidth = (PCIE_PORT_WIDTH)((UINT8)PcieCfg->PortInfo.PortWidth >> 1); > This shift deserves a comment. > Also, consider if > PcieCfg->PortInfo.PortWidth >>= 1; > would not be more clear. > >> + >> + Status = PciePortInit(soctype, HostBridgeNum, PcieCfg); >> + if(EFI_ERROR(Status)) { >> + DEBUG((DEBUG_ERROR, "PcieReconfigLanenum HostBridge %d, Pcie Port %d Init Failed! \n", HostBridgeNum, Port)); >> + } >> + return; >> + } >> + >> + LoopCnt++; >> + /* Pcie 3.0 Spec,part 4.2.6.3.4.1: the Upstream Lanes are permitted >> + * delay up to 1 ms before transitioning to Configuration.Lanenum.Accept. >> + * So the delay time 200 us * 5(LanNumCnt) = 1ms, not beyond the reasonable range. >> + */ > This comment is excellent, thank you. > >> + MicroSecondDelay(200); >> + } >> + >> + return ; >> +} >> >> -EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) >> +EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, PCIE_DRIVER_CFG *PcieCfg) >> { >> PCIE_CTRL_7_U pcie_ctrl7; >> UINT32 Value = 0; >> @@ -168,6 +284,7 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) >> Value |= BIT11|BIT30|BIT31; >> RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + 0x1114, Value); >> (VOID)PcieRxValidCtrl(soctype, HostBridgeNum, Port, 1); >> + PcieReconfigLaneNum(soctype, HostBridgeNum, Port, PcieCfg); >> return EFI_SUCCESS; >> } >> else >> @@ -1005,7 +1122,7 @@ PciePortInit ( >> /* Disable RC Option Rom */ >> DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType); >> /* assert LTSSM enable */ >> - (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); >> + (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex, PcieCfg); >> if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) { >> //PCIe will still work even if performance tuning fails, >> //and there is warning message inside the function to print >> diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h >> index 539d567..b750910 100644 >> --- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h >> +++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h >> @@ -8982,6 +8982,7 @@ typedef union tagIepMsiCtrlIntStatus >> #define PCIE_SYS_CTRL28_REG (PCI_SYS_BASE + 0x1c4) >> #define PCIE_SYS_CTRL29_REG (PCI_SYS_BASE + 0x1c8) >> #define PCIE_SYS_CTRL54_REG (PCI_SYS_BASE + 0x274) >> +#define PCIE_SYS_STATE4_REG (PCI_SYS_BASE + 0x31C) >> #define PCIE_SYS_STATE5_REG (PCI_SYS_BASE + 0x30) >> #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) >> #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38) >> @@ -12694,6 +12695,8 @@ typedef union tagPortlogic93 >> #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE3_REG (PCIE_SUBCTRL_BASE + 0x6814) >> #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE4_REG (PCIE_SUBCTRL_BASE + 0x6818) >> #define PCIE_LTSSM_STATE_MASK (0x3f) >> +#define PCIE_LTSSM_CFG_LANENUM_ACPT 0x0a >> +#define PCIE_LTSSM_CFG_COMPLETE 0x0b >> #define PCIE_LTSSM_LINKUP_STATE (0x11) >> #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS0_REG (PCIE_SUBCTRL_BASE + 0x6880) >> #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS1_REG (PCIE_SUBCTRL_BASE + 0x6884) >> -- >> 1.9.1 >>
On Fri, Mar 24, 2017 at 11:06:00AM +0800, Chenhui Sun wrote: > Hi Leif, > > > 在 2017/3/21 3:28, Leif Lindholm 写道: > >On Mon, Mar 20, 2017 at 09:11:08PM +0800, Chenhui Sun wrote: > >>In some cases, the PCIe device may close part of lanes in > >>config state of LTSSM, the hip06 RC should reconfig lane number > >>and try to linkup again. > >> > >>Contributed-under: TianoCore Contribution Agreement 1.0 > >>Signed-off-by: Chenhui Sun <sunchenhui@huawei.com> > >>Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > >>Signed-off-by: Yi Li <phoenix.liyi@huawei.com> > >>--- > >> .../Hi1610/Drivers/PcieInit1610/PcieInitLib.c | 121 ++++++++++++++++++++- > >> Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h | 3 + > >> 2 files changed, 122 insertions(+), 2 deletions(-) > >> > >>diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > >>index 57699e0..b45d54f 100644 > >>--- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > >>+++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > >>@@ -42,6 +42,7 @@ extern PCIE_DRIVER_CFG gastr_pcie_driver_cfg; > >> extern PCIE_IATU gastr_pcie_iatu_cfg; > >> extern PCIE_IATU_VA mPcieIatuTable; > >>+EFI_STATUS PciePortInit (UINT32 soctype, UINT32 HostBridgeNum, PCIE_DRIVER_CFG *PcieCfg); > >A blank line after this definition would be nice. > >Also, I think the EFI_API needs to be here as well. > > EFI_STATUS > EFIAPI > PciePortInit ( > IN UINT32 soctype, > IN UINT32 HostBridgeNum, > IN PCIE_DRIVER_CFG *PcieCfg > ); > > Is this the correct style? Preferably, yes :) > > > > >> VOID PcieRegWrite(UINT32 Port, UINTN Offset, UINT32 Value) > >> { > >> RegWrite((UINT64)mPcieIntCfg.RegResource[Port] + Offset, Value); > >Space between function name and "(". This applies globally in this > >patch (I won't repeat this comment). > > > >(But no space between the (UINT64) and mPcieInitCfg, that bit is done > >the correct way.) > This code is not the new added, so I will refine the follow new added code > according your comments. > is that ok? Yes please. Sometimes when I review lots of code, I get blind to what is context and what is modification. Just ignore me when that happens :) I never want style fixes as part of functional patches, only that the modified lines conform. Best Regards, Leif > > Thanks > Chenhui > >>@@ -152,8 +153,123 @@ VOID PcieRxValidCtrl(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, BOOLEAN > >> } > >> } > >> } > >>+/* > >>+ * The ltssm register is assigned in an asynchronous way, the value > >>+ * of register may not right in metastable state. > >>+ * Read the register twice to get stable value. > >>+ */ > >>+VOID PcieGetLtssmValue ( > >>+ UINT32 HostBridgeNum, > >>+ UINT32 Port, > >>+ UINT32* Value > > UINT32 *Value > > > >>+) > >>+{ > >>+ UINT32 ValueA; > >>+ UINT32 ValueB = 0; > >>+ UINT32 Count; > >>+ > >>+ RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueA); > >>+ ValueA = ValueA & PCIE_LTSSM_STATE_MASK; > >>+ > >>+ Count = 0; > >>+ while (Count < 2) { > >>+ > >>+ RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueB); > >>+ ValueB = ValueB & PCIE_LTSSM_STATE_MASK; > >>+ > >>+ /* Get the same state in continuous two times*/ > >>+ if (ValueA == ValueB) { > >>+ break; > >>+ } > >>+ > >>+ /* If the second value not equel to the first, > >"equel" -> "equal". > > > >>+ * we return the second one as the stable > >Also, this fits on a single line, so please join them. > > > >>+ */ > >>+ ValueA = ValueB; > >>+ Count++; > >>+ } > >>+ > >>+ *Value = ValueB; > >>+ > >>+ return; > >>+ > >>+} > >>+ > >>+/* > >>+ * In some cases, the PCIe device may close part of lanes in > >>+ * config state of LTSSM, the hip06 RC should reconfig lane num > >>+ * and try to linkup again. > >>+ */ > >>+VOID PcieReconfigLaneNum ( > >>+ UINT32 soctype, > >>+ UINT32 HostBridgeNum, > >>+ UINT32 Port, > >>+ PCIE_DRIVER_CFG *PcieCfg > >>+) > >>+{ > >>+ EFI_STATUS Status; > >>+ UINT32 LtssmStatus; > >>+ UINT32 RegVal; > >>+ UINT32 LoopCnt = 0; > >>+ UINT32 LaneNumCnt = 0; > >>+ PCIE_PORT_WIDTH PortWidth = PcieCfg->PortInfo.PortWidth; > >>+ > >>+ // 500 * 200us = 100ms, so it takes 100 ms must to reconfig lane numbers > >>+ while (LoopCnt < 500) { > >>+ > >>+ /* > >>+ * The minimum lanenum is 1, no need to try any more. > >>+ */ > >Please align the * characters. > > > >>+ if (PortWidth <= 1) { > >>+ DEBUG((DEBUG_ERROR, "PcieReconfigLanenum PortWidth <= 1 !\n")); > >>+ return; > >>+ } > >>+ > >>+ /* > >>+ * Check the lane num config state is normal or not. > >>+ */ > >>+ PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); > >>+ if ((LtssmStatus == PCIE_LTSSM_CFG_LANENUM_ACPT) || (LtssmStatus == PCIE_LTSSM_CFG_COMPLETE)) { > >>+ LaneNumCnt++; > >>+ } else if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { > >>+ PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); > >>+ if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { > >>+ break; > >>+ } > >>+ } else { > >>+ LaneNumCnt = 0; > >>+ } > >>+ > >>+ /* > >>+ * The lane num config state is abnormal, need to reconfig > >>+ * the lane num and try to establish link again. > >>+ */ > >>+ if (LaneNumCnt > 5) { > >Why 5? Could there be a #define with a descriptive name? > > > >>+ /* Disable LTSSM */ > >>+ RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); > >>+ RegVal &= ~(BIT11); > >What is BIT11? > >Can there be a comment explaining > > > >>+ RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); > >>+ PcieCfg->PortInfo.PortWidth = (PCIE_PORT_WIDTH)((UINT8)PcieCfg->PortInfo.PortWidth >> 1); > >This shift deserves a comment. > >Also, consider if > > PcieCfg->PortInfo.PortWidth >>= 1; > >would not be more clear. > > > >>+ > >>+ Status = PciePortInit(soctype, HostBridgeNum, PcieCfg); > >>+ if(EFI_ERROR(Status)) { > >>+ DEBUG((DEBUG_ERROR, "PcieReconfigLanenum HostBridge %d, Pcie Port %d Init Failed! \n", HostBridgeNum, Port)); > >>+ } > >>+ return; > >>+ } > >>+ > >>+ LoopCnt++; > >>+ /* Pcie 3.0 Spec,part 4.2.6.3.4.1: the Upstream Lanes are permitted > >>+ * delay up to 1 ms before transitioning to Configuration.Lanenum.Accept. > >>+ * So the delay time 200 us * 5(LanNumCnt) = 1ms, not beyond the reasonable range. > >>+ */ > >This comment is excellent, thank you. > > > >>+ MicroSecondDelay(200); > >>+ } > >>+ > >>+ return ; > >>+} > >>-EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > >>+EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, PCIE_DRIVER_CFG *PcieCfg) > >> { > >> PCIE_CTRL_7_U pcie_ctrl7; > >> UINT32 Value = 0; > >>@@ -168,6 +284,7 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > >> Value |= BIT11|BIT30|BIT31; > >> RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + 0x1114, Value); > >> (VOID)PcieRxValidCtrl(soctype, HostBridgeNum, Port, 1); > >>+ PcieReconfigLaneNum(soctype, HostBridgeNum, Port, PcieCfg); > >> return EFI_SUCCESS; > >> } > >> else > >>@@ -1005,7 +1122,7 @@ PciePortInit ( > >> /* Disable RC Option Rom */ > >> DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType); > >> /* assert LTSSM enable */ > >>- (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); > >>+ (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex, PcieCfg); > >> if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) { > >> //PCIe will still work even if performance tuning fails, > >> //and there is warning message inside the function to print > >>diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > >>index 539d567..b750910 100644 > >>--- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > >>+++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > >>@@ -8982,6 +8982,7 @@ typedef union tagIepMsiCtrlIntStatus > >> #define PCIE_SYS_CTRL28_REG (PCI_SYS_BASE + 0x1c4) > >> #define PCIE_SYS_CTRL29_REG (PCI_SYS_BASE + 0x1c8) > >> #define PCIE_SYS_CTRL54_REG (PCI_SYS_BASE + 0x274) > >>+#define PCIE_SYS_STATE4_REG (PCI_SYS_BASE + 0x31C) > >> #define PCIE_SYS_STATE5_REG (PCI_SYS_BASE + 0x30) > >> #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) > >> #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38) > >>@@ -12694,6 +12695,8 @@ typedef union tagPortlogic93 > >> #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE3_REG (PCIE_SUBCTRL_BASE + 0x6814) > >> #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE4_REG (PCIE_SUBCTRL_BASE + 0x6818) > >> #define PCIE_LTSSM_STATE_MASK (0x3f) > >>+#define PCIE_LTSSM_CFG_LANENUM_ACPT 0x0a > >>+#define PCIE_LTSSM_CFG_COMPLETE 0x0b > >> #define PCIE_LTSSM_LINKUP_STATE (0x11) > >> #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS0_REG (PCIE_SUBCTRL_BASE + 0x6880) > >> #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS1_REG (PCIE_SUBCTRL_BASE + 0x6884) > >>-- > >>1.9.1 > >> >
diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c index 57699e0..b45d54f 100644 --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c @@ -42,6 +42,7 @@ extern PCIE_DRIVER_CFG gastr_pcie_driver_cfg; extern PCIE_IATU gastr_pcie_iatu_cfg; extern PCIE_IATU_VA mPcieIatuTable; +EFI_STATUS PciePortInit (UINT32 soctype, UINT32 HostBridgeNum, PCIE_DRIVER_CFG *PcieCfg); VOID PcieRegWrite(UINT32 Port, UINTN Offset, UINT32 Value) { RegWrite((UINT64)mPcieIntCfg.RegResource[Port] + Offset, Value); @@ -152,8 +153,123 @@ VOID PcieRxValidCtrl(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, BOOLEAN } } } +/* + * The ltssm register is assigned in an asynchronous way, the value + * of register may not right in metastable state. + * Read the register twice to get stable value. + */ +VOID PcieGetLtssmValue ( + UINT32 HostBridgeNum, + UINT32 Port, + UINT32* Value +) +{ + UINT32 ValueA; + UINT32 ValueB = 0; + UINT32 Count; + + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueA); + ValueA = ValueA & PCIE_LTSSM_STATE_MASK; + + Count = 0; + while (Count < 2) { + + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_SYS_REG_OFFSET + PCIE_SYS_STATE4_REG, ValueB); + ValueB = ValueB & PCIE_LTSSM_STATE_MASK; + + /* Get the same state in continuous two times*/ + if (ValueA == ValueB) { + break; + } + + /* If the second value not equel to the first, + * we return the second one as the stable + */ + ValueA = ValueB; + Count++; + } + + *Value = ValueB; + + return; + +} + +/* + * In some cases, the PCIe device may close part of lanes in + * config state of LTSSM, the hip06 RC should reconfig lane num + * and try to linkup again. + */ +VOID PcieReconfigLaneNum ( + UINT32 soctype, + UINT32 HostBridgeNum, + UINT32 Port, + PCIE_DRIVER_CFG *PcieCfg +) +{ + EFI_STATUS Status; + UINT32 LtssmStatus; + UINT32 RegVal; + UINT32 LoopCnt = 0; + UINT32 LaneNumCnt = 0; + PCIE_PORT_WIDTH PortWidth = PcieCfg->PortInfo.PortWidth; + + // 500 * 200us = 100ms, so it takes 100 ms must to reconfig lane numbers + while (LoopCnt < 500) { + + /* + * The minimum lanenum is 1, no need to try any more. + */ + if (PortWidth <= 1) { + DEBUG((DEBUG_ERROR, "PcieReconfigLanenum PortWidth <= 1 !\n")); + return; + } + + /* + * Check the lane num config state is normal or not. + */ + PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); + if ((LtssmStatus == PCIE_LTSSM_CFG_LANENUM_ACPT) || (LtssmStatus == PCIE_LTSSM_CFG_COMPLETE)) { + LaneNumCnt++; + } else if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { + PcieGetLtssmValue(HostBridgeNum, Port, &LtssmStatus); + if (LtssmStatus == PCIE_LTSSM_LINKUP_STATE) { + break; + } + } else { + LaneNumCnt = 0; + } + + /* + * The lane num config state is abnormal, need to reconfig + * the lane num and try to establish link again. + */ + if (LaneNumCnt > 5) { + /* Disable LTSSM */ + RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); + RegVal &= ~(BIT11); + RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + PCIE_CTRL_7_REG, RegVal); + PcieCfg->PortInfo.PortWidth = (PCIE_PORT_WIDTH)((UINT8)PcieCfg->PortInfo.PortWidth >> 1); + + Status = PciePortInit(soctype, HostBridgeNum, PcieCfg); + if(EFI_ERROR(Status)) { + DEBUG((DEBUG_ERROR, "PcieReconfigLanenum HostBridge %d, Pcie Port %d Init Failed! \n", HostBridgeNum, Port)); + } + return; + } + + LoopCnt++; + /* Pcie 3.0 Spec,part 4.2.6.3.4.1: the Upstream Lanes are permitted + * delay up to 1 ms before transitioning to Configuration.Lanenum.Accept. + * So the delay time 200 us * 5(LanNumCnt) = 1ms, not beyond the reasonable range. + */ + MicroSecondDelay(200); + } + + return ; +} -EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) +EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port, PCIE_DRIVER_CFG *PcieCfg) { PCIE_CTRL_7_U pcie_ctrl7; UINT32 Value = 0; @@ -168,6 +284,7 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) Value |= BIT11|BIT30|BIT31; RegWrite(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + 0x1114, Value); (VOID)PcieRxValidCtrl(soctype, HostBridgeNum, Port, 1); + PcieReconfigLaneNum(soctype, HostBridgeNum, Port, PcieCfg); return EFI_SUCCESS; } else @@ -1005,7 +1122,7 @@ PciePortInit ( /* Disable RC Option Rom */ DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType); /* assert LTSSM enable */ - (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); + (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex, PcieCfg); if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) { //PCIe will still work even if performance tuning fails, //and there is warning message inside the function to print diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h index 539d567..b750910 100644 --- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h +++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h @@ -8982,6 +8982,7 @@ typedef union tagIepMsiCtrlIntStatus #define PCIE_SYS_CTRL28_REG (PCI_SYS_BASE + 0x1c4) #define PCIE_SYS_CTRL29_REG (PCI_SYS_BASE + 0x1c8) #define PCIE_SYS_CTRL54_REG (PCI_SYS_BASE + 0x274) +#define PCIE_SYS_STATE4_REG (PCI_SYS_BASE + 0x31C) #define PCIE_SYS_STATE5_REG (PCI_SYS_BASE + 0x30) #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38) @@ -12694,6 +12695,8 @@ typedef union tagPortlogic93 #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE3_REG (PCIE_SUBCTRL_BASE + 0x6814) #define PCIE_SUBCTRL_SC_PCIE0_SYS_STATE4_REG (PCIE_SUBCTRL_BASE + 0x6818) #define PCIE_LTSSM_STATE_MASK (0x3f) +#define PCIE_LTSSM_CFG_LANENUM_ACPT 0x0a +#define PCIE_LTSSM_CFG_COMPLETE 0x0b #define PCIE_LTSSM_LINKUP_STATE (0x11) #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS0_REG (PCIE_SUBCTRL_BASE + 0x6880) #define PCIE_SUBCTRL_SC_PCIE0_AXI_MSTR_OOO_WR_STS1_REG (PCIE_SUBCTRL_BASE + 0x6884)