Messages in this thread | | | Subject | Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only | From | Ray Jui <> | Date | Tue, 30 Aug 2016 10:04:58 -0700 |
| |
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
| |