[Linaro-uefi,Linaro-uefi,v1,04/21] Hisilicon: Add reconfig lane number feature

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

Commit Message

Chenhui Sun March 20, 2017, 1:11 p.m.
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(-)

Comments

Leif Lindholm March 20, 2017, 7:28 p.m. | #1
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
>
Chenhui Sun March 24, 2017, 3:06 a.m. | #2
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
>>
Leif Lindholm April 3, 2017, 1:21 p.m. | #3
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
> >>
>

Patch

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)