diff mbox series

[Linaro-uefi,Linaro-uefi,v1,03/21] Hisilicon: disable RC Option Rom

Message ID 1490015485-53685-4-git-send-email-chenhui.sun@linaro.org
State Superseded
Headers show
Series D02/D03 platforms bug fix | expand

Commit Message

Chenhui Sun March 20, 2017, 1:11 p.m. UTC 
The M3(the corprocessor)PCIe driver will read Option Rom header
durning enumeration, this opration will cause a completion error
when there is no device inserted to the RC port, and the Option rom
is uesless now. So we need to disable the RC Option Rom.

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      | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Leif Lindholm March 20, 2017, 4:05 p.m. UTC | #1 
On Mon, Mar 20, 2017 at 09:11:07PM +0800, Chenhui Sun wrote:
> The M3(the corprocessor)PCIe driver will read Option Rom header

"corprocessor" -> "coprocessor".

> durning enumeration, this opration will cause a completion error

"opration" -> "operation".

> when there is no device inserted to the RC port, and the Option rom
> is uesless now. So we need to disable the RC Option Rom.
> 
> 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      | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
> index 3bad240..57699e0 100644
> --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
> +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
> @@ -904,6 +904,44 @@ void PcieConfigContextHi1610(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port)
>      return;
>  }
>  
> +UINT32 SysRegRead(UINT32 SocType, UINT32 HostBridgeNum, UINT32 Port, UINTN Reg)

Coding style says this should be

UINT32
SysRegRead (
  UINT32 SocType,
  UINT32 HostBridgeNum,
  UINT32 Port,
  UINTN  Reg
  )

Also, whether the arguments are IN, OUT or IN OUT.
5.7.1.11 of https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf

> +{
> +  UINT32 Value;
> +  if (SocType == 0x1610) {
> +    RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + Reg, Value);

Space before "(".

> +  } else {
> +    //PCIE_APB_SLVAE_BASE is for 660,and each PCIe Ccontroller has the same APB_SLVAE_BASE
> +    //in the same hostbridge.
> +    RegRead(PCIE_APB_SLVAE_BASE[HostBridgeNum] + Reg, Value);

Space before "(".

> +  }
> +  return Value;
> +}
> +
> +VOID
> +DisableRcOptionRom (
> +  UINT32 soctype,

Since this is a new function, could you rename this variable
"SocType", like in the previous function? For coding style compliance.

> +  UINT32 HostBridgeNum,
> +  UINT32 Port,
> +  PCIE_PORT_TYPE PcieType
> +)

Please add IN/OUT indicators.

> +{
> +  UINT32 Value = 0;
> +  if (PcieType == PCIE_ROOT_COMPLEX) {
> +    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG);

Space before "(".

> +    Value |= BIT2; //cs2 enable
> +    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG, Value);

Space before "(".

> +
> +    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_EP_PCI_CFG_HDR12_REG);

Space before "(".

> +    Value &= ~BIT0; //disable option rom
> +    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_EP_PCI_CFG_HDR12_REG, Value);

Space before "(".

> +
> +    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG);

Space before "(".

> +    Value &= ~BIT2; //cs2 disable
> +    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG, Value);

Space before "(".

> +  }
> +  return;
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  PciePortInit (
> @@ -964,6 +1002,8 @@ PciePortInit (
>       /* Pcie Equalization*/
>       (VOID)PcieEqualization(soctype ,HostBridgeNum, PortIndex);
>  
> +     /* Disable RC Option Rom */
> +     DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType);

Space before "(".

>       /* assert LTSSM enable */
>       (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex);
>       if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) {
> -- 
> 1.9.1
>
Chenhui Sun March 24, 2017, 3:09 a.m. UTC | #2 
Hi Leif,

Thank you for pointing this out.  :)

Regards


在 2017/3/21 0:05, Leif Lindholm 写道:
> On Mon, Mar 20, 2017 at 09:11:07PM +0800, Chenhui Sun wrote:
>> The M3(the corprocessor)PCIe driver will read Option Rom header
> "corprocessor" -> "coprocessor".
>
>> durning enumeration, this opration will cause a completion error
> "opration" -> "operation".
>
>> when there is no device inserted to the RC port, and the Option rom
>> is uesless now. So we need to disable the RC Option Rom.
>>
>> 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      | 40 ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
>> index 3bad240..57699e0 100644
>> --- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
>> +++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
>> @@ -904,6 +904,44 @@ void PcieConfigContextHi1610(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port)
>>       return;
>>   }
>>   
>> +UINT32 SysRegRead(UINT32 SocType, UINT32 HostBridgeNum, UINT32 Port, UINTN Reg)
> Coding style says this should be
>
> UINT32
> SysRegRead (
>    UINT32 SocType,
>    UINT32 HostBridgeNum,
>    UINT32 Port,
>    UINTN  Reg
>    )
>
> Also, whether the arguments are IN, OUT or IN OUT.
> 5.7.1.11 of https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf
>
>> +{
>> +  UINT32 Value;
>> +  if (SocType == 0x1610) {
>> +    RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + Reg, Value);
> Space before "(".
>
>> +  } else {
>> +    //PCIE_APB_SLVAE_BASE is for 660,and each PCIe Ccontroller has the same APB_SLVAE_BASE
>> +    //in the same hostbridge.
>> +    RegRead(PCIE_APB_SLVAE_BASE[HostBridgeNum] + Reg, Value);
> Space before "(".
>
>> +  }
>> +  return Value;
>> +}
>> +
>> +VOID
>> +DisableRcOptionRom (
>> +  UINT32 soctype,
> Since this is a new function, could you rename this variable
> "SocType", like in the previous function? For coding style compliance.
>
>> +  UINT32 HostBridgeNum,
>> +  UINT32 Port,
>> +  PCIE_PORT_TYPE PcieType
>> +)
> Please add IN/OUT indicators.
>
>> +{
>> +  UINT32 Value = 0;
>> +  if (PcieType == PCIE_ROOT_COMPLEX) {
>> +    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG);
> Space before "(".
>
>> +    Value |= BIT2; //cs2 enable
>> +    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG, Value);
> Space before "(".
>
>> +
>> +    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_EP_PCI_CFG_HDR12_REG);
> Space before "(".
>
>> +    Value &= ~BIT0; //disable option rom
>> +    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_EP_PCI_CFG_HDR12_REG, Value);
> Space before "(".
>
>> +
>> +    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG);
> Space before "(".
>
>> +    Value &= ~BIT2; //cs2 disable
>> +    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG, Value);
> Space before "(".
>
>> +  }
>> +  return;
>> +}
>> +
>>   EFI_STATUS
>>   EFIAPI
>>   PciePortInit (
>> @@ -964,6 +1002,8 @@ PciePortInit (
>>        /* Pcie Equalization*/
>>        (VOID)PcieEqualization(soctype ,HostBridgeNum, PortIndex);
>>   
>> +     /* Disable RC Option Rom */
>> +     DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType);
> Space before "(".
>
>>        /* assert LTSSM enable */
>>        (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex);
>>        if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) {
>> -- 
>> 1.9.1
>>
diff mbox series

Patch

diff --git a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
index 3bad240..57699e0 100644
--- a/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
+++ b/Chips/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.c
@@ -904,6 +904,44 @@  void PcieConfigContextHi1610(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port)
     return;
 }
 
+UINT32 SysRegRead(UINT32 SocType, UINT32 HostBridgeNum, UINT32 Port, UINTN Reg)
+{
+  UINT32 Value;
+  if (SocType == 0x1610) {
+    RegRead(PCIE_APB_SLAVE_BASE_1610[HostBridgeNum][Port] + Reg, Value);
+  } else {
+    //PCIE_APB_SLVAE_BASE is for 660,and each PCIe Ccontroller has the same APB_SLVAE_BASE
+    //in the same hostbridge.
+    RegRead(PCIE_APB_SLVAE_BASE[HostBridgeNum] + Reg, Value);
+  }
+  return Value;
+}
+
+VOID
+DisableRcOptionRom (
+  UINT32 soctype,
+  UINT32 HostBridgeNum,
+  UINT32 Port,
+  PCIE_PORT_TYPE PcieType
+)
+{
+  UINT32 Value = 0;
+  if (PcieType == PCIE_ROOT_COMPLEX) {
+    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG);
+    Value |= BIT2; //cs2 enable
+    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG, Value);
+
+    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_EP_PCI_CFG_HDR12_REG);
+    Value &= ~BIT0; //disable option rom
+    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_EP_PCI_CFG_HDR12_REG, Value);
+
+    Value = SysRegRead(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG);
+    Value &= ~BIT2; //cs2 disable
+    SysRegWrite(soctype, HostBridgeNum, Port, PCIE_SYS_REG_OFFSET + PCIE_SYS_CTRL21_REG, Value);
+  }
+  return;
+}
+
 EFI_STATUS
 EFIAPI
 PciePortInit (
@@ -964,6 +1002,8 @@  PciePortInit (
      /* Pcie Equalization*/
      (VOID)PcieEqualization(soctype ,HostBridgeNum, PortIndex);
 
+     /* Disable RC Option Rom */
+     DisableRcOptionRom(soctype, HostBridgeNum, PortIndex, PcieCfg->PortInfo.PortType);
      /* assert LTSSM enable */
      (VOID)PcieEnableItssm(soctype, HostBridgeNum, PortIndex);
      if (FeaturePcdGet(PcdIsPciPerfTuningEnable)) {