Message-ID: <gnpoekmyk4elg53xabcsvj6sqacttby6dpryxcdepws3fpt2xj@y7efnnszvpem>
Date: Fri, 18 Apr 2025 22:31:19 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Qiang Yu <quic_qianyu@...cinc.com>
Cc: "Wenbin Yao (Consultant)" <quic_wenbyao@...cinc.com>, 
	jingoohan1@...il.com, lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org, 
	bhelgaas@...gle.com, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	quic_cang@...cinc.com, mrana@...cinc.com
Subject: Re: [PATCH] PCI: dwc: Set PORT_LOGIC_LINK_WIDTH to one lane

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

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

Powered by blists - more mailing lists