Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Ray, Scott, Jon, bcm-kernel-feedback-list]

On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
> >> Altera PCIe IP can be configured as rootport or device and they might have
> >> same vendor ID. It will cause the system hang issue if Altera PCIe is in
> >> endpoint mode and work with other PCIe rootport that from other vendors.
> >> So, add the rootport mode checking in link retrain fixup function.
> >>
> >> Signed-off-by: Ley Foon Tan <lftan@altera.com>
> >> ---
> >> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
> >> ---
> >>  drivers/pci/host/pcie-altera.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> >> index 58eef99..33b6968 100644
> >> --- a/drivers/pci/host/pcie-altera.c
> >> +++ b/drivers/pci/host/pcie-altera.c
> >> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
> >>       u16 linkcap, linkstat;
> >>       struct altera_pcie *pcie = dev->bus->sysdata;
> >>
> >> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> >> +             return;
> >> +
> >>       if (!altera_pcie_link_is_up(pcie))
> >>               return;
> >
> > Instead of making this a PCI fixup, can you make an
> > altera_pcie_host_init() function, call it from altera_pcie_probe(),
> > and do the link retrain there?  Then you wouldn't need to worry about
> > whether this is a Root Port or an Endpoint, plus it would make the
> > altera driver structure more like the other drivers.
> >
> > You would call altera_pcie_host_init() before pci_scan_root_bus(), so
> > you wouldn't have a pci_dev yet, so you wouldn't be able to use
> > pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
> > assume there's some device-dependent way to access it using
> > cra_writel()?
> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit. 

Why not?  I don't mean it has to be cra_write(), but isn't there some
way you can write that bit before we scan the root bus?  It doesn't
make sense that we have to scan the bus before we can train the link.

We want to be able to tell the PCI core "all the device-specific root
complex initialization has been done, here are the config accessors
you need, please scan for devices."  I want to keep device-specific
things like this quirk directly in the driver and out of the
enumeration process.

> We can use
> pci_bus_find_capability() and pci_bus_read_config_word() with struct
> pci_bus instead.
> But this only can be called after pci_scan_root_bus().

> Found
> iproc_pcie_check_link() have similar implementation.

You're right, and I don't like iproc_pcie_check_link() either, for the
same reasons.

The iproc_pcie_check_link() is a little better because it's called
before enumeration:

  pci_create_root_bus()
  iproc_pcie_check_link()
  pci_scan_child_bus()

But it would be a lot better if iproc_pcie_check_link() were done
first, before pci_create_root_bus().  Then it would be more like the
structure of other drivers, and we could use pci_scan_root_bus()
instead.

Comments, iproc folks?

Bjorn