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