Re: [Linaro-uefi] [PATCH 02/11] Hisilicon/Hi1610/PCIe: Added performance tuning

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