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

[Qiang Yu <quic_qianyu@quicinc.com>]

On 4/24/2025 10:49 AM, Wenbin Yao (Consultant) wrote:
> On 4/23/2025 9:24 PM, Niklas Cassel wrote:
>> On Tue, Apr 22, 2025 at 06:36:23PM +0800, Wenbin Yao wrote:
>>> As per DWC PCIe registers description 4.30a, section 1.13.43, 
>>> NUM_OF_LANES
>>> named as PORT_LOGIC_LINK_WIDTH in PCIe DWC driver, 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. This section explains the 
>>> conditions
>>> need be satisfied for entering Polling.Configuration:
>>>
>>> "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.
>>>
>>> Otherwise, after a 24 ms timeout the next state is:
>>> Polling.Configuration if
>>> (i) Any Lane, which detected a Receiver during Detect, received eight
>>> consecutive training sequences and a minimum of 1024 TS1 Ordered 
>>> Sets are
>>> transmitted after receiving one TS1 or TS2 Ordered Set.
>>> And
>>> (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: 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."
>>>
>>> In a PCIe link that supports multiple lanes, if 
>>> PORT_LOGIC_LINK_WIDTH is
>>> set to lane width hardware supports, all lanes that detect a receiver
>>> during the Detect phase must receive eight consecutive training 
>>> sequences.
>>> Otherwise, the LTSSM cannot enter Polling.Configuration and link 
>>> training
>>> will fail.
>>>
>>> Therefore, always set PORT_LOGIC_LINK_WIDTH to 1, regardless of the 
>>> number
>>> of lanes the port actually supports, to make linking up more robust. 
>>> This
>>> setting will not affect the intended link width if all lanes are
>>> functional. Additionally, the link can still be established with at 
>>> least
>>> one lane if other lanes are faulty.
>>>
>>> Co-developed-by: Qiang Yu <quic_qianyu@quicinc.com>
>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>>> ---
>>> Changes in v2:
>>> - Reword commit message.
>>> - Link to v1: 
>>> https://lore.kernel.org/all/1524e971-8433-1e2d-b39e-65bad0d6c6ce@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 97d76d3dc..be348b341 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>>> @@ -797,22 +797,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
>>>
>> I still see the link to my EP (which also have this patch) using all
>> four lanes according to lspci, so:
>>
>> Tested-by: Niklas Cassel <cassel@kernel.org>
Thank you, Niklas, for kindly testing this patch and providing feedback.
Wenbin seems to have misunderstood you; sorry for this.
>
> This setting will not affect the intended link width if all lanes are
> functional. Additionally, the link can still be established with at least
> one lane if other lanes are faulty.
>
-- 
With best wishes
Qiang Yu