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

[Scott Branden]

Hi Bjorn,

Ray is off this week and will likely have some comments next week.

On 16-08-24 10:54 AM, Bjorn Helgaas wrote:
> [+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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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

Regards,
 Scott