On Wed, Oct 14, 2015 at 9:32 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote: > 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. Thanks for your suggestion. You are right, I have tested this can work as well. So, I can remove those 2 PCI_FIXUP* in the driver. > > > 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? This fixup is different from pci_set_bus_speed(). This fixup is to set the retrain bit in the LNKCTL register if the host can support higher speed than current speed, this is required by our hardware. But, pci_set_bus_speed() is just read the LNKCAP and LNKSTA registers and store in data structure. > > +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. Yes, some host drivers have similar code as well. Some host controller have the BAR configuration enabled, but it doesn't fit to kernel resources. Example the BAR is 64-bit, but the processor is 32-bit. It will fail at the host driver probing stage. pci 0000:00:00.0: BAR 0: [mem 0x00000000-0xffffffff 64bit pref] has bogus alignment Regards Ley Foon -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html