Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver

[Arnd Bergmann]

On Wednesday 14 October 2015 18:01:46 Ley Foon Tan wrote:
> On Wed, Oct 14, 2015 at 5:36 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Wednesday 14 October 2015 17:28:45 Ley Foon Tan wrote:
> >> On Wed, Oct 14, 2015 at 5:09 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >
> > Could we perhaps have a helper function that lets us register
> > fixups dynamically?
> >
> >> The linker script keeps all pci fixup callbacks in pci fixup regions
> >> during kernel compile time. So, it needs to be builtin module. Do you
> >> know any way we can update those fixup regions?
> >
> > The only method I'm aware of at the moment is move the fixups to
> > drivers/pci/quirks.c and enclose them in an #ifdef if you want them
> > to not appear in kernels that don't support your SoC.
> By looking at the drivers/pci/quirks.c, it looks like it is mainly for
> the pci endpoint devices.
> Fixups for host controller are in the driver itself.
> 

But if it's for the host itself, there are usually other ways to
do this without needing a fixup: you already have the device structure
present in the driver, so you should just be able to modify it there.


I'm looking at the code in your fixups now:

+static void altera_pcie_retrain(struct pci_dev *dev)
+{
+       u16 linkcap, linkstat;
+
+       /*
+        * Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
+        * current speed is 2.5 GB/s.
+        */
+       pcie_capability_read_word(dev, PCI_EXP_LNKCAP, &linkcap);
+
+       if ((linkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
+               return;
+
+       pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &linkstat);
+       if ((linkstat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB)
+               pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
+                                        PCI_EXP_LNKCTL_RL);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ALTERA, PCI_ANY_ID, altera_pcie_retrain);

This looks related to the code in pci_set_bus_speed(). What is
missing from that code?

+static void altera_pcie_fixup_res(struct pci_dev *dev)
+{
+       /*
+        * Prevent enumeration of root port.
+        */
+       if (!dev->bus->parent && dev->devfn == 0) {
+               int i;
+
+               for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+                       dev->resource[i].start = 0;
+                       dev->resource[i].end   = 0;
+                       dev->resource[i].flags   = 0;
+               }
+       }
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ALTERA, PCI_ANY_ID,
+                        altera_pcie_fixup_res);

This seems really odd, too. Why is this needed?
I think I've seen similar code in other host drivers, so
it might be time to teach the PCI core about this kind of
device.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/