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