lkml.org 
[lkml]   [2016]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] PCI: altera: Retrain link in rootport mode only
From
Date


On 8/30/2016 10:00 AM, Bjorn Helgaas wrote:
> On Tue, Aug 30, 2016 at 09:36:52AM -0700, Ray Jui wrote:
>>
>>
>> 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@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.
>>
>> 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.
>
> No, that's not what I'm trying to say. You can access the
> configuration registers if you need to. But you shouldn't need a
> struct pci_bus to do that. All you do with the struct pci_bus is get
> the corresponding struct iproc_pcie.
>
> It will require some restructuring, of course, e.g., making low-level
> accessors that take the struct iproc_pcie, and wrappers around them
> that take a struct pci_bus. The usual config accesses can go through
> the wrapper, and the iproc-internal accesses can use the low-level
> accessors directly.
>
> Bjorn
>

Okay I got it. Thanks for the clarifications. I'll look into this when I
have a chance.

Ray

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:1.445 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site