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

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Aug 29, 2016 at 05:37:09PM -0700, Ray Jui wrote:
> Hi Bjorn,
> 
> On 8/24/2016 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@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.
> 
> Although not yet tested, I suppose we can do iproc_pcie_check_link
> before calling pci_scan_root_bus so we can get rid of separate calls
> to pci_create_root_bus and pci_scan_child_bus. But then we need to
> create some dummy bus in the iproc_pcie_check_link function to allow
> access to the root bus for link check, which was the primary reason
> why we did pci_create_root_bus before iproc_pcie_check_link, i.e.,
> to avoid the use of dummy root bus.

I don't want a dummy root bus.

There should be some way to structure that code so you can write the
class code and the link status stuff without having a struct pci_bus.
The only reason you need the struct pci_bus in the first place is so
you can extract the struct iproc_pcie *, and you already have that in
iproc_pcie_check_link().

No, you won't be able to use pci_bus_find_capability(), but presumably
you already *know* where the capability is, since you know exactly
what device this is.

Bjorn