| Message ID | 1476324020-57155-2-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
Could we have a short body explaining what this code does? Improved PHY training, tuned clocks, ...? On Thu, Oct 13, 2016 at 10:00:11AM +0800, Heyi Guo wrote: > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: hensonwang <wanghuiqiang@huawei.com> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > --- > .../Hi1610/Drivers/PcieInit1610/PcieInitLib.c | 65 ++++++++++++++++++++++ > Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h | 1 + > 2 files changed, 66 insertions(+) > > diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > index 4ddb116..d2928ee 100755 > --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c > @@ -185,6 +185,70 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > > } > > +EFI_STATUS PciPerfTuning(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) This looks like a local only function, so should it be STATIC? > +{ > + UINT32 Value = 0; > + > + if(Port >= PCIE_MAX_PORT_NUM) > + { > + return EFI_INVALID_PARAMETER; > + } > + > + if (0x1610 == soctype) Please flip the comparison. On avrage, I would also request a > + { > + //PCIe_SYS_CTRL13 This comment adds no information that is not discovered as easliy from reading the code. That said, the code is just reading from some random register, and needs a comment describing what operation is actually being performed. This comment applies to many places in this function. > + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL13_REG, Value); Quite long lines. Could PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 be assigned to a temporary variable to improve readability? Also, could 0x1000 be given a decriptive #define instead of being invoked as a magic value? Is PCIE_APB_SLVAE_... a typo of PCIE_APB_SLAVE... ? > + Value |= (BIT13 | BIT12); > + Value |= BIT10; Why is this split over two lines, and what do these bits do? > + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL13_REG, Value); > + > + //PCIe_SYS_CTRL6 > + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_CTRL_6_REG, Value); > + Value |= (BIT13 | BIT12); > + Value |= (BIT17 | BIT19 | BIT21 | BIT23 | BIT25 | BIT27 | BIT29); Why is this split over two lines, and what do these bits do? > + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_CTRL_6_REG, Value); > + > + //PCIe_SYS_CTRL54 > + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL54_REG, Value); > + Value &= ~(BIT30); > + Value &= ~(0xff<<16); What effect do these changes have? > + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL54_REG, Value); > + > + //PCIe_SYS_CTRL19 > + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL19_REG, Value); > + Value |= (BIT28 | BIT30); What effect do these changes have? > + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL19_REG, Value); > + } > + else > + { > + PcieChangeRwMode(HostBridgeNum, Port, PCIE_SYS_CONTROL); > + //PCIe_SYS_CTRL13 > + PcieRegRead(Port, PCIE_SYS_CTRL13_REG); > + Value |= ((BIT13) | (BIT12)); Why the extra parentheses? > + Value |= (BIT10); Why on a separate line? What do these bit changes do? > + PcieRegWrite(Port, PCIE_SYS_CTRL13_REG, Value); > + > + //PCIe_SYS_CTRL6 > + PcieRegRead(Port, PCIE_CTRL_6_REG); > + Value |= ((BIT13) | (BIT12)); > + Value |= ((BIT17) | (BIT19) | (BIT21) | (BIT23) | (BIT25) | (BIT27) | (BIT29)); Why the extra parantheses, why split over two lines, what does this code do? > + PcieRegWrite(Port, PCIE_CTRL_6_REG, Value); > + > + //PCIe_SYS_CTRL54 > + PcieRegRead(Port, PCIE_SYS_CTRL54_REG); > + Value &= ~(BIT30); > + Value &= ~(0xff<<16); What does this code do? > + PcieRegWrite(Port, PCIE_SYS_CTRL54_REG, Value); > + > + //PCIe_SYS_CTRL19 > + PcieRegRead(Port, PCIE_SYS_CTRL19_REG); > + Value |= ((BIT28) | (BIT30)); What does this code do? / Leif > + PcieRegWrite(Port, PCIE_SYS_CTRL19_REG, Value); > + PcieChangeRwMode(HostBridgeNum, Port, PCIE_CONFIG_REG); > + } > + return EFI_SUCCESS; > +} > + > EFI_STATUS PcieDisableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > { > PCIE_CTRL_7_U pcie_ctrl7; > @@ -940,6 +1004,7 @@ PciePortInit ( > > /* assert LTSSM enable */ > (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); > + (VOID)PciPerfTuning(soctype, HostBridgeNum, PortIndex); > > PcieConfigContextHi1610(soctype, HostBridgeNum, PortIndex); > /* > diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > index bdd80f8..539d567 100644 > --- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > +++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h > @@ -8981,6 +8981,7 @@ typedef union tagIepMsiCtrlIntStatus > #define PCIE_SYS_CTRL24_REG (PCI_SYS_BASE + 0x1b4) > #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_STATE5_REG (PCI_SYS_BASE + 0x30) > #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) > #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38) > -- > 1.9.1 >
在 10/13/2016 5:29 PM, Leif Lindholm 写道: > Could we have a short body explaining what this code does? > Improved PHY training, tuned clocks, ...? > > On Thu, Oct 13, 2016 at 10:00:11AM +0800, Heyi Guo wrote: >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: hensonwang <wanghuiqiang@huawei.com> >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >> --- >> .../Hi1610/Drivers/PcieInit1610/PcieInitLib.c | 65 ++++++++++++++++++++++ >> Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h | 1 + >> 2 files changed, 66 insertions(+) >> >> diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c >> index 4ddb116..d2928ee 100755 >> --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c >> +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c >> @@ -185,6 +185,70 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) >> >> } >> >> +EFI_STATUS PciPerfTuning(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) > This looks like a local only function, so should it be STATIC? > >> +{ >> + UINT32 Value = 0; >> + >> + if(Port >= PCIE_MAX_PORT_NUM) >> + { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + if (0x1610 == soctype) > Please flip the comparison. > On avrage, I would also request a Sorry, is there anything missed here? Request a what? Heyi > >> + { >> + //PCIe_SYS_CTRL13 > This comment adds no information that is not discovered as easliy from > reading the code. That said, the code is just reading from some random > register, and needs a comment describing what operation is actually > being performed. This comment applies to many places in this function. > >> + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL13_REG, Value); > Quite long lines. Could > PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 > be assigned to a temporary variable to improve readability? > > Also, could 0x1000 be given a decriptive #define instead of being > invoked as a magic value? > > Is > PCIE_APB_SLVAE_... > a typo of > PCIE_APB_SLAVE... > ? > >> + Value |= (BIT13 | BIT12); >> + Value |= BIT10; > Why is this split over two lines, and what do these bits do? > >> + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL13_REG, Value); >> + >> + //PCIe_SYS_CTRL6 >> + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_CTRL_6_REG, Value); >> + Value |= (BIT13 | BIT12); >> + Value |= (BIT17 | BIT19 | BIT21 | BIT23 | BIT25 | BIT27 | BIT29); > Why is this split over two lines, and what do these bits do? > >> + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_CTRL_6_REG, Value); >> + >> + //PCIe_SYS_CTRL54 >> + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL54_REG, Value); >> + Value &= ~(BIT30); >> + Value &= ~(0xff<<16); > What effect do these changes have? > >> + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL54_REG, Value); >> + >> + //PCIe_SYS_CTRL19 >> + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL19_REG, Value); >> + Value |= (BIT28 | BIT30); > What effect do these changes have? > >> + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL19_REG, Value); >> + } >> + else >> + { >> + PcieChangeRwMode(HostBridgeNum, Port, PCIE_SYS_CONTROL); >> + //PCIe_SYS_CTRL13 >> + PcieRegRead(Port, PCIE_SYS_CTRL13_REG); >> + Value |= ((BIT13) | (BIT12)); > Why the extra parentheses? > >> + Value |= (BIT10); > Why on a separate line? > What do these bit changes do? > >> + PcieRegWrite(Port, PCIE_SYS_CTRL13_REG, Value); >> + >> + //PCIe_SYS_CTRL6 >> + PcieRegRead(Port, PCIE_CTRL_6_REG); >> + Value |= ((BIT13) | (BIT12)); >> + Value |= ((BIT17) | (BIT19) | (BIT21) | (BIT23) | (BIT25) | (BIT27) | (BIT29)); > Why the extra parantheses, why split over two lines, what does this > code do? > >> + PcieRegWrite(Port, PCIE_CTRL_6_REG, Value); >> + >> + //PCIe_SYS_CTRL54 >> + PcieRegRead(Port, PCIE_SYS_CTRL54_REG); >> + Value &= ~(BIT30); >> + Value &= ~(0xff<<16); > What does this code do? > >> + PcieRegWrite(Port, PCIE_SYS_CTRL54_REG, Value); >> + >> + //PCIe_SYS_CTRL19 >> + PcieRegRead(Port, PCIE_SYS_CTRL19_REG); >> + Value |= ((BIT28) | (BIT30)); > What does this code do? > > / > Leif > >> + PcieRegWrite(Port, PCIE_SYS_CTRL19_REG, Value); >> + PcieChangeRwMode(HostBridgeNum, Port, PCIE_CONFIG_REG); >> + } >> + return EFI_SUCCESS; >> +} >> + >> EFI_STATUS PcieDisableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) >> { >> PCIE_CTRL_7_U pcie_ctrl7; >> @@ -940,6 +1004,7 @@ PciePortInit ( >> >> /* assert LTSSM enable */ >> (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); >> + (VOID)PciPerfTuning(soctype, HostBridgeNum, PortIndex); >> >> PcieConfigContextHi1610(soctype, HostBridgeNum, PortIndex); >> /* >> diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h >> index bdd80f8..539d567 100644 >> --- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h >> +++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h >> @@ -8981,6 +8981,7 @@ typedef union tagIepMsiCtrlIntStatus >> #define PCIE_SYS_CTRL24_REG (PCI_SYS_BASE + 0x1b4) >> #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_STATE5_REG (PCI_SYS_BASE + 0x30) >> #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) >> #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38) >> -- >> 1.9.1 >>
diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c index 4ddb116..d2928ee 100755 --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c @@ -185,6 +185,70 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) } +EFI_STATUS PciPerfTuning(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) +{ + UINT32 Value = 0; + + if(Port >= PCIE_MAX_PORT_NUM) + { + return EFI_INVALID_PARAMETER; + } + + if (0x1610 == soctype) + { + //PCIe_SYS_CTRL13 + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL13_REG, Value); + Value |= (BIT13 | BIT12); + Value |= BIT10; + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL13_REG, Value); + + //PCIe_SYS_CTRL6 + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_CTRL_6_REG, Value); + Value |= (BIT13 | BIT12); + Value |= (BIT17 | BIT19 | BIT21 | BIT23 | BIT25 | BIT27 | BIT29); + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_CTRL_6_REG, Value); + + //PCIe_SYS_CTRL54 + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL54_REG, Value); + Value &= ~(BIT30); + Value &= ~(0xff<<16); + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL54_REG, Value); + + //PCIe_SYS_CTRL19 + RegRead(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL19_REG, Value); + Value |= (BIT28 | BIT30); + RegWrite(PCIE_APB_SLVAE_BASE_1610[HostBridgeNum][Port] + 0x1000 + PCIE_SYS_CTRL19_REG, Value); + } + else + { + PcieChangeRwMode(HostBridgeNum, Port, PCIE_SYS_CONTROL); + //PCIe_SYS_CTRL13 + PcieRegRead(Port, PCIE_SYS_CTRL13_REG); + Value |= ((BIT13) | (BIT12)); + Value |= (BIT10); + PcieRegWrite(Port, PCIE_SYS_CTRL13_REG, Value); + + //PCIe_SYS_CTRL6 + PcieRegRead(Port, PCIE_CTRL_6_REG); + Value |= ((BIT13) | (BIT12)); + Value |= ((BIT17) | (BIT19) | (BIT21) | (BIT23) | (BIT25) | (BIT27) | (BIT29)); + PcieRegWrite(Port, PCIE_CTRL_6_REG, Value); + + //PCIe_SYS_CTRL54 + PcieRegRead(Port, PCIE_SYS_CTRL54_REG); + Value &= ~(BIT30); + Value &= ~(0xff<<16); + PcieRegWrite(Port, PCIE_SYS_CTRL54_REG, Value); + + //PCIe_SYS_CTRL19 + PcieRegRead(Port, PCIE_SYS_CTRL19_REG); + Value |= ((BIT28) | (BIT30)); + PcieRegWrite(Port, PCIE_SYS_CTRL19_REG, Value); + PcieChangeRwMode(HostBridgeNum, Port, PCIE_CONFIG_REG); + } + return EFI_SUCCESS; +} + EFI_STATUS PcieDisableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) { PCIE_CTRL_7_U pcie_ctrl7; @@ -940,6 +1004,7 @@ PciePortInit ( /* assert LTSSM enable */ (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex); + (VOID)PciPerfTuning(soctype, HostBridgeNum, PortIndex); PcieConfigContextHi1610(soctype, HostBridgeNum, PortIndex); /* diff --git a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h index bdd80f8..539d567 100644 --- a/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h +++ b/Chips/Hisilicon/Include/Regs/HisiPcieV1RegOffset.h @@ -8981,6 +8981,7 @@ typedef union tagIepMsiCtrlIntStatus #define PCIE_SYS_CTRL24_REG (PCI_SYS_BASE + 0x1b4) #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_STATE5_REG (PCI_SYS_BASE + 0x30) #define PCIE_SYS_STATE6_REG (PCI_SYS_BASE + 0x34) #define PCIE_SYS_STATE7_REG (PCI_SYS_BASE + 0x38)