[PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

[PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

[Wenbin Yao (Consultant)]

PORT_LOGIC_LINK_WIDTH field of the PCIE_LINK_WIDTH_SPEED_CONTROL register
indicates the number of lanes to check for exit from Electrical Idle in
Polling.Active and L2.Idle. It is used to limit the effective link width to
ignore broken or unused lanes that detect a receiver to prevent one or more
bad Receivers or Transmitters from holding up a valid Link from being
configured.

In a PCIe link that support muiltiple lanes, setting PORT_LOGIC_LINK_WIDTH
to 1 will not affect the link width that is actually intended to be used.
But setting it to a value other than 1 will lead to link training fail if
one or more lanes are broken.

Hence, always set PORT_LOGIC_LINK_WIDTH to 1 no matter how many lanes the
port actually supports to make linking up more robust. Link can still be
established with one lane at least if other lanes are broken.

Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
---
  drivers/pci/controller/dwc/pcie-designware.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c 
b/drivers/pci/controller/dwc/pcie-designware.c
index 6d6cbc8b5b2c..d40afe74ddd1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -752,22 +752,19 @@ static void dw_pcie_link_set_max_link_width(struct 
dw_pcie *pci, u32 num_lanes)
      /* Set link width speed control register */
      lwsc = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
      lwsc &= ~PORT_LOGIC_LINK_WIDTH_MASK;
+    lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
      switch (num_lanes) {
      case 1:
          plc |= PORT_LINK_MODE_1_LANES;
-        lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
          break;
      case 2:
          plc |= PORT_LINK_MODE_2_LANES;
-        lwsc |= PORT_LOGIC_LINK_WIDTH_2_LANES;
          break;
      case 4:
          plc |= PORT_LINK_MODE_4_LANES;
-        lwsc |= PORT_LOGIC_LINK_WIDTH_4_LANES;
          break;
      case 8:
          plc |= PORT_LINK_MODE_8_LANES;
-        lwsc |= PORT_LOGIC_LINK_WIDTH_8_LANES;
          break;
      default:
          dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);
-- 
2.34.1

Re: [PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

[Wenbin Yao (Consultant)]

On 12/12/2024 4:19 PM, Wenbin Yao (Consultant) wrote:
> PORT_LOGIC_LINK_WIDTH field of the PCIE_LINK_WIDTH_SPEED_CONTROL register
> indicates the number of lanes to check for exit from Electrical Idle in
> Polling.Active and L2.Idle. It is used to limit the effective link 
> width to
> ignore broken or unused lanes that detect a receiver to prevent one or 
> more
> bad Receivers or Transmitters from holding up a valid Link from being
> configured.
>
> In a PCIe link that support muiltiple lanes, setting 
> PORT_LOGIC_LINK_WIDTH
> to 1 will not affect the link width that is actually intended to be used.
> But setting it to a value other than 1 will lead to link training fail if
> one or more lanes are broken.
>
> Hence, always set PORT_LOGIC_LINK_WIDTH to 1 no matter how many lanes the
> port actually supports to make linking up more robust. Link can still be
> established with one lane at least if other lanes are broken.
>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c 
> b/drivers/pci/controller/dwc/pcie-designware.c
> index 6d6cbc8b5b2c..d40afe74ddd1 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -752,22 +752,19 @@ static void 
> dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>      /* Set link width speed control register */
>      lwsc = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
>      lwsc &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> +    lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
>      switch (num_lanes) {
>      case 1:
>          plc |= PORT_LINK_MODE_1_LANES;
> -        lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
>          break;
>      case 2:
>          plc |= PORT_LINK_MODE_2_LANES;
> -        lwsc |= PORT_LOGIC_LINK_WIDTH_2_LANES;
>          break;
>      case 4:
>          plc |= PORT_LINK_MODE_4_LANES;
> -        lwsc |= PORT_LOGIC_LINK_WIDTH_4_LANES;
>          break;
>      case 8:
>          plc |= PORT_LINK_MODE_8_LANES;
> -        lwsc |= PORT_LOGIC_LINK_WIDTH_8_LANES;
>          break;
>      default:
>          dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);

Hello, do you have any futher comments?

-- 
With best wishes
Wenbin

Re: [PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

[Manivannan Sadhasivam]

On Thu, Dec 12, 2024 at 04:19:12PM +0800, Wenbin Yao (Consultant) wrote:
> PORT_LOGIC_LINK_WIDTH field of the PCIE_LINK_WIDTH_SPEED_CONTROL register
> indicates the number of lanes to check for exit from Electrical Idle in
> Polling.Active and L2.Idle. It is used to limit the effective link width to
> ignore broken or unused lanes that detect a receiver to prevent one or more
> bad Receivers or Transmitters from holding up a valid Link from being
> configured.
> 
> In a PCIe link that support muiltiple lanes, setting PORT_LOGIC_LINK_WIDTH
> to 1 will not affect the link width that is actually intended to be used.

Where in the spec it is defined?

> But setting it to a value other than 1 will lead to link training fail if
> one or more lanes are broken.
> 

Which means the link partner is not able to downsize the link during LTSSM?

> Hence, always set PORT_LOGIC_LINK_WIDTH to 1 no matter how many lanes the
> port actually supports to make linking up more robust. Link can still be
> established with one lane at least if other lanes are broken.
> 

This looks like a specific endpoint/controller issue to me. Where exactly did
you see the issue?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

[Qiang Yu]

On 4/8/2025 1:51 AM, Manivannan Sadhasivam wrote:
> On Thu, Dec 12, 2024 at 04:19:12PM +0800, Wenbin Yao (Consultant) wrote:
>> PORT_LOGIC_LINK_WIDTH field of the PCIE_LINK_WIDTH_SPEED_CONTROL register
>> indicates the number of lanes to check for exit from Electrical Idle in
>> Polling.Active and L2.Idle. It is used to limit the effective link width to
>> ignore broken or unused lanes that detect a receiver to prevent one or more
>> bad Receivers or Transmitters from holding up a valid Link from being
>> configured.
>>
>> In a PCIe link that support muiltiple lanes, setting PORT_LOGIC_LINK_WIDTH
>> to 1 will not affect the link width that is actually intended to be used.
> Where in the spec it is defined?
As per DWC registers data book, NUM_OF_LANES is referred to as the 
"Predetermined Number of Lanes" in section 4.2.6.2.1 of the PCI Express 
Base 3.0 Specification, revision 1.0.
Section 4.2.6.2.1 explains the condtions need be satisfied for enter 
Poll.Configuration from Polling.Active.
The original statement is

"Next state is Polling.Configuration after at least 1024 TS1 Ordered 
Sets were transmitted, and all Lanes that detected a Receiver during 
Detect receive eight consecutive training sequences (or
their complement) satisfying any of the following conditions:
...
Otherwise, after a 24 ms timeout the next state is:
Polling.Configuration if
...
(ii) At least a predetermined set of Lanes that detected a Receiver 
during Detect have detected an exit from Electrical Idle at least once 
since entering Polling.Active.
     Note: _*This may prevent one or more bad Receivers or Transmitters 
from holding up a valid Link from being configured*_, and allow for 
additional training in Polling.Configuration. *_The exact set of 
predetermined Lanes is implementation specific_*. Note that up to the 
1.1 specification this predetermined set was equal to the total set of 
Lanes that detected a Receiver.
     Note: Any Lane that receives eight consecutive TS1 or TS2 Ordered 
Sets should have detected an exit from Electrical Idle at least once 
since entering Polling.Active."
>
>> But setting it to a value other than 1 will lead to link training fail if
>> one or more lanes are broken.
>>
> Which means the link partner is not able to downsize the link during LTSSM?
Yes, According to the theory metioned above, let's say in a 8 lanes PCIe 
link, if we set NUM_OF_LANES to 8, then all lanes that detect a Receiver 
during Detect need to receive eight consecutive training sequences, 
otherwise the LTSSM can not enter Poll.Configuration and linktraing will 
fail.
>
>> Hence, always set PORT_LOGIC_LINK_WIDTH to 1 no matter how many lanes the
>> port actually supports to make linking up more robust. Link can still be
>> established with one lane at least if other lanes are broken.
>>
> This looks like a specific endpoint/controller issue to me. Where exactly did
> you see the issue?
Althouh we met this issue on some Modem platforms where PCIe port works 
in EP mode. But this is not a specific endpoint/controller issue. This 
register will be set to 1 by default after reset in new QCOM platform. 
But upstream kernel will still program it to other value here.
>
> - Mani
>
-- 
With best wishes
Qiang Yu

Re: [PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

[Manivannan Sadhasivam]

On Mon, Apr 14, 2025 at 04:45:26PM +0800, Qiang Yu wrote:
> 
> On 4/8/2025 1:51 AM, Manivannan Sadhasivam wrote:
> > On Thu, Dec 12, 2024 at 04:19:12PM +0800, Wenbin Yao (Consultant) wrote:
> > > PORT_LOGIC_LINK_WIDTH field of the PCIE_LINK_WIDTH_SPEED_CONTROL register
> > > indicates the number of lanes to check for exit from Electrical Idle in
> > > Polling.Active and L2.Idle. It is used to limit the effective link width to
> > > ignore broken or unused lanes that detect a receiver to prevent one or more
> > > bad Receivers or Transmitters from holding up a valid Link from being
> > > configured.
> > > 
> > > In a PCIe link that support muiltiple lanes, setting PORT_LOGIC_LINK_WIDTH
> > > to 1 will not affect the link width that is actually intended to be used.
> > Where in the spec it is defined?
> As per DWC registers data book, NUM_OF_LANES is referred to as the
> "Predetermined Number of Lanes" in section 4.2.6.2.1 of the PCI Express Base
> 3.0 Specification, revision 1.0.
> Section 4.2.6.2.1 explains the condtions need be satisfied for enter
> Poll.Configuration from Polling.Active.
> The original statement is
> 
> "Next state is Polling.Configuration after at least 1024 TS1 Ordered Sets
> were transmitted, and all Lanes that detected a Receiver during Detect
> receive eight consecutive training sequences (or
> their complement) satisfying any of the following conditions:
> ...
> Otherwise, after a 24 ms timeout the next state is:
> Polling.Configuration if
> ...
> (ii) At least a predetermined set of Lanes that detected a Receiver during
> Detect have detected an exit from Electrical Idle at least once since
> entering Polling.Active.
>     Note: _*This may prevent one or more bad Receivers or Transmitters from
> holding up a valid Link from being configured*_, and allow for additional
> training in Polling.Configuration. *_The exact set of predetermined Lanes is
> implementation specific_*. Note that up to the 1.1 specification this
> predetermined set was equal to the total set of Lanes that detected a
> Receiver.

Ok, this is the most relevant part of the spec. It says that atleast the
predetermined set of lanes that detected a receiver during Detect.Active state
should detect an exit from Electrical Idle at least once. So this condition can
only be false if one or more lanes are faulty (not unused or broken). If the
lanes are unused or broken, then they should not have detected the Receivers in
the Detect.Active state itself.

So this was the source of confusion.

>     Note: Any Lane that receives eight consecutive TS1 or TS2 Ordered Sets
> should have detected an exit from Electrical Idle at least once since
> entering Polling.Active."
> > 
> > > But setting it to a value other than 1 will lead to link training fail if
> > > one or more lanes are broken.
> > > 
> > Which means the link partner is not able to downsize the link during LTSSM?
> Yes, According to the theory metioned above, let's say in a 8 lanes PCIe
> link, if we set NUM_OF_LANES to 8, then all lanes that detect a Receiver
> during Detect need to receive eight consecutive training sequences,
> otherwise the LTSSM can not enter Poll.Configuration and linktraing will
> fail.

Correct. This information should be part of the patch description.

> > 
> > > Hence, always set PORT_LOGIC_LINK_WIDTH to 1 no matter how many lanes the
> > > port actually supports to make linking up more robust. Link can still be
> > > established with one lane at least if other lanes are broken.
> > > 
> > This looks like a specific endpoint/controller issue to me. Where exactly did
> > you see the issue?
> Althouh we met this issue on some Modem platforms where PCIe port works in
> EP mode. But this is not a specific endpoint/controller issue. This register
> will be set to 1 by default after reset in new QCOM platform. But upstream
> kernel will still program it to other value here.

Yeah, now it makes sense to me and I agree that there is no need to set it to
MLW lanes.

Please reword the patch description to change 'broken or unused lanes' to
'faulty lanes', add reference to relevant sections of the PCIe and DWC specs
and also add above mentioned paragraph.

- Mani

-- 
மணிவண்ணன் சதாசிவம்