Re: [PATCH 03/16] PCI: dwc: Add more verbose link-up message

[Serge Semin <fancer.lancer@gmail.com>]

On Tue, Mar 29, 2022 at 09:47:02AM -0500, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 04:37:21AM +0300, Serge Semin wrote:
> > Printing just "link up" isn't that much informative especially when it
> > comes to working with the PCI Express bus. Even if the link is up, due to
> > multiple reasons the bus performance can degrade to slower speeds or to
> > narrower width than both Root Port and its partner is capable of. In that
> > case it would be handy to know the link specifications as early as
> > possible. So let's add a more verbose message to the busy-wait link-state
> > method, which will contain the link speed generation and the PCIe bus
> > width in case if the link up state is discovered. Otherwise an error will
> > be printed to the system log.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 22 +++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 6e81264fdfb4..f1693e25afcb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -528,14 +528,26 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >  
> >  	/* Check if the link is up or not */
> >  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> > -		if (dw_pcie_link_up(pci)) {
> > -			dev_info(pci->dev, "Link up\n");
> > -			return 0;
> > -		}
> > +		if (dw_pcie_link_up(pci))
> > +			break;
> > +
> >  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> >  	}
> >  
> > -	dev_info(pci->dev, "Phy link never came up\n");
> > +	if (retries < LINK_WAIT_MAX_RETRIES) {
> > +		u32 offset, val;
> > +
> > +		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > +		val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> > +
> > +		dev_info(pci->dev, "PCIe Gen.%u x%u link up\n",
> > +			 FIELD_GET(PCI_EXP_LNKSTA_CLS, val),
> > +			 FIELD_GET(PCI_EXP_LNKSTA_NLW, val));
> 

> Given these are standard registers can we do this in the core code? The 
> main issue I think is that the config space accessors don't work until 
> you create the bus struct. That still should be early enough.

AFAICS there are generic methods in the core code to get and print the
link status. See the __pcie_print_link_status() method implementation.
But as you said they rely on having the bus struct instance created and
properly initialized. It's created in the framework of the PCI Host bridge
registration procedure:
pci_host_probe()
+-> pci_scan_root_bus_bridge()
    +-> pci_register_host_bridge()
        +-> pci_alloc_bus(NULL); 
        +-> ...
As for me it would be more logical to have the PCIe link established
(at least activated) and it' status logged before any of the denoted
actions are made since further initialization rely on the PCIe bus
transfers. Moreover the PCIe host probe procedure doesn't really
perform any link up/down related activity, so there is no logical
place to implement the link state checking except someplace at the
traceback top position, but again the bus struct instance isn't
available at that stage. Of course we could implement an alternative
__pcie_print_HOST_link_status() method, which wouldn't need the bus
struct passed. But that would have required some more modifications
(and may cause some functionality duplication) than fixing a few lines
of code and wasn't a subject of this patchset. As such I decided to
stick with having the local link status logging procedure especially
seeing it's done in the framework of the link-wait method, which is
called right after the DW PCIe LTSSM is activated (at least for the DW
PCIe Host controller).

> 
> I think it is possible some implementations don't report the link state 
> in these registers. Maybe we don't really need to care.

I don't see a way to disable the PCIe capability in the DW PCIe
controllers. So if some implementations lack of these registers
reporting the link state, then either those implementations must have
been broken or they violate the PCIe Base Specification [1]. IMO that
must be considered as abnormal situation and needs to be specifically
handled.

[1] PCI Express® Base Specification Revision 5.0, p. 742.

-Sergey

> 
> Rob