Message-ID: <a1688079-6a39-3c25-756f-cd1e1256f468@broadcom.com>
Date:   Tue, 30 Aug 2016 09:36:52 -0700
From:   Ray Jui <ray.jui@...adcom.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Ley Foon Tan <lftan@...era.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-pci <linux-pci@...r.kernel.org>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        Jon Mason <jonmason@...adcom.com>,
        bcm-kernel-feedback-list@...adcom.com
Subject: Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only



On 8/30/2016 6:37 AM, Bjorn Helgaas wrote:
> 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@...nel.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@...era.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.

Okay we are on the same page for this.

> 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.

I'll need to review the check link function carefully and do some 
experiment to see what I can do to determine the link status without 
accessing any of the configuration registers, which is what you seem to 
imply here.

Thanks.

>
> Bjorn
>

Powered by blists - more mailing lists