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

[Ley Foon Tan]

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