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

Message ID 1476324020-57155-2-git-send-email-heyi.guo@linaro.org
State New
Headers show

Commit Message

gary guo Oct. 13, 2016, 2 a.m.
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(+)

Comments

Leif Lindholm Oct. 13, 2016, 9:29 a.m. | #1
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
>
gary guo Oct. 14, 2016, 6:25 a.m. | #2
在 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
>>

Patch

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)