Re: *_pcie_establish_link() usage

[Lorenzo Pieralisi]

On Wed, Aug 28, 2019 at 08:00:56AM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 01, 2019 at 04:25:29PM -0500, Bjorn Helgaas wrote:
> > Hi,
> > 
> > I got the following dmesg log from Fawad [1]:
> > 
> >   imx6q-pcie 1ffc000.pcie: host bridge /soc/pcie@1ffc000 ranges:
> >   imx6q-pcie 1ffc000.pcie:    IO 0x01f80000..0x01f8ffff -> 0x00000000
> >   imx6q-pcie 1ffc000.pcie:   MEM 0x01000000..0x01efffff -> 0x01000000
> >   imx6q-pcie 1ffc000.pcie: Link up
> >   imx6q-pcie 1ffc000.pcie: Link: Gen2 disabled
> >   imx6q-pcie 1ffc000.pcie: Link up, Gen1
> >   imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
> >   pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
> >   pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> > 
> > This is unrelated to the problem Fawad is working on, but the above
> > looks wrong to me because it associates the "Link up" and link speed
> > info with the host bridge (imx6q-pcie 1ffc000.pcie), not the Root Port
> > (pci 0000:00:00.0).
> > 
> > I see that *_pcie_establish_link() is generally called from
> > *_pcie_host_init(), typically via the struct
> > dw_pcie_host_ops.host_init pointer, e.g.,
> > 
> >   dra7xx_pcie_probe
> >     dra7xx_add_pcie_port(dra7xx)
> >       struct dw_pcie *pci = dra7xx->pci
> >       struct pcie_port *pp = &pci->pp
> >       dw_pcie_host_init(struct pcie_port *pp)
> > 	bridge = devm_pci_alloc_host_bridge
> > 	devm_of_pci_get_host_bridge_resources
> > 	pp->ops->host_init(pp)
> > 	  dra7xx_pcie_host_init               # .host_init
> > 	    dra7xx_pcie_establish_link        # <--- bring up link
> > 	    dw_pcie_wait_for_link
> > 	pci_scan_root_bus_bridge(bridge)      # <--- enumerate
> > 	pp->root_bus = bridge->bus
> > 	pci_bus_add_devices(pp->root_bus)
> > 
> > AFAICT, the *_pcie_establish_link() functions all operate on a single
> > PCIe link, i.e., they are bringing up the link going downstream from a
> > single Root Port.
> > 
> > It looks like this only allows a single Root Port per struct dw_pcie
> > device.  Is that true?  *Should* that be true?
> > 
> > It looks like we bring up the link before enumerating.  In some cases,
> > (meson_pcie_host_init(), qcom_pcie_host_init(),
> > uniphier_pcie_host_init()) if the link doesn't come up, we return
> > failure, which means dw_pcie_host_init() will not enumerate devices at
> > all.
> > 
> > That seems wrong -- can't we have Root Complex Integrated Endpoints
> > and even other Root Ports on that root bus?  Those should be
> > accessible and possibly useful even if we can't bring up a link on
> > *one* Root Port.
> > 
> > I would *expect* that we would enumerate all the devices on the root
> > bus.  Then if we find one or more Root Ports, we might try to bring up
> > the link for each one, and if successful, enumerate the downstream
> > devices.
> > 
> > I'm confused.  Is there some restriction that means there can only be
> > a single Root Port in this design, and no RCiEPs?  Even if there is,
> > can we change the code so it enumerates the root bus first and brings
> > up links as necessary so it matches the generic PCIe topology better?
> 
> Anyone?  I'll add this to a list of projects for interested people to
> work on unless somebody objects.

I think we should broach this topic at LPC in Lisbon, I expect some
of the dwc (and other host controllers) maintainers to be there.

Thanks for bringing this up.

Lorenzo

> > [1] https://lore.kernel.org/linux-pci/CAGgoGu7rot61LSgu2syOMq9Onx26_u3PEtS7pf_QNQRxOaifhg@xxxxxxxxxxxxxx/