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

[Ley Foon Tan]

On Thu, Aug 25, 2016 at 1:54 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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 don't have internal register bit to trigger link retrain, but need to set
PCI_EXP_LNKCTL_RL bit in Link Control register of PCIe Capabilities Structure.
So, this requires the altera_pcie_cfg_read() and altera_pcie_cfg_write().
I can restructure the altera_pcie_cfg_read() and
altera_pcie_cfg_write() and have
new _altera_pcie_cfg_read() and _altera_pcie_cfg_write() that avoid
the dependency of struct pci_bus. By doing this, we can retrain the link before
pci_scan_root_bus and remove _FIXUP()

Will send new v3 patch, please take a look.

>
>> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html