Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1246401 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Arnd Bergmann <arnd@arndb.de> - 2015-10-14 10:30 +0200
    Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Ley Foon Tan <lftan@altera.com> - 2015-10-14 10:40 +0200
      Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Arnd Bergmann <arnd@arndb.de> - 2015-10-14 11:10 +0200
        Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Ley Foon Tan <lftan@altera.com> - 2015-10-14 11:30 +0200
          Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Arnd Bergmann <arnd@arndb.de> - 2015-10-14 11:40 +0200
            Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Ley Foon Tan <lftan@altera.com> - 2015-10-14 12:10 +0200
              Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Arnd Bergmann <arnd@arndb.de> - 2015-10-14 15:40 +0200
                Re: [PATCH v9 3/6] pci:host: Add Altera PCIe host controller driver Ley Foon Tan <lftan@altera.com> - 2015-10-16 10:50 +0200

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

On Wednesday 14 October 2015 10:41:29 Ley Foon Tan wrote:
> +static int altera_pcie_remove(struct platform_device *pdev)
> +{
> +	struct altera_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	altera_pcie_free_irq_domain(pcie);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}

I just noticed this. Does it actually work to unload the module
and tear down all the pci_dev structures in a safe way?

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

[toc] | [next] | [standalone]

#1246408

On Wed, Oct 14, 2015 at 4:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 14 October 2015 10:41:29 Ley Foon Tan wrote:
>> +static int altera_pcie_remove(struct platform_device *pdev)
>> +{
>> +     struct altera_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> +     altera_pcie_free_irq_domain(pcie);
>> +     platform_set_drvdata(pdev, NULL);
>> +     return 0;
>> +}
>
> I just noticed this. Does it actually work to unload the module
> and tear down all the pci_dev structures in a safe way?
Good catch. It only can be compiled as builtin-moduley now, so we can
remove this _remove callback function.
Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1246435

On Wednesday 14 October 2015 16:32:25 Ley Foon Tan wrote:
> On Wed, Oct 14, 2015 at 4:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 14 October 2015 10:41:29 Ley Foon Tan wrote:
> >> +static int altera_pcie_remove(struct platform_device *pdev)
> >> +{
> >> +     struct altera_pcie *pcie = platform_get_drvdata(pdev);
> >> +
> >> +     altera_pcie_free_irq_domain(pcie);
> >> +     platform_set_drvdata(pdev, NULL);
> >> +     return 0;
> >> +}
> >
> > I just noticed this. Does it actually work to unload the module
> > and tear down all the pci_dev structures in a safe way?
> Good catch. It only can be compiled as builtin-moduley now, so we can
> remove this _remove callback function.

I think we should change both: make it possible to load the
driver dynamically, and remove the altera_pcie_remove function.

You can prevent the module from being unloaded if you also remove
the module_platform_driver() directive and add a module_init()
without a matching module_exit().

Please also add a '.suppress_bind_attrs = true,' flag in the driver
struct to prevent manual unbinding.

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

[toc] | [prev] | [next] | [standalone]

#1246452

On Wed, Oct 14, 2015 at 5:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 14 October 2015 16:32:25 Ley Foon Tan wrote:
>> On Wed, Oct 14, 2015 at 4:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday 14 October 2015 10:41:29 Ley Foon Tan wrote:
>> >> +static int altera_pcie_remove(struct platform_device *pdev)
>> >> +{
>> >> +     struct altera_pcie *pcie = platform_get_drvdata(pdev);
>> >> +
>> >> +     altera_pcie_free_irq_domain(pcie);
>> >> +     platform_set_drvdata(pdev, NULL);
>> >> +     return 0;
>> >> +}
>> >
>> > I just noticed this. Does it actually work to unload the module
>> > and tear down all the pci_dev structures in a safe way?
>> Good catch. It only can be compiled as builtin-moduley now, so we can
>> remove this _remove callback function.
>
> I think we should change both: make it possible to load the
> driver dynamically, and remove the altera_pcie_remove function.
This driver depends on the pci fixups to work correctly. But, fixups
callback functions in this driver are not being call if the driver is
loadable module.
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?

>
> You can prevent the module from being unloaded if you also remove
> the module_platform_driver() directive and add a module_init()
> without a matching module_exit().
>
> Please also add a '.suppress_bind_attrs = true,' flag in the driver
> struct to prevent manual unbinding.
I think we don't need these if it only can work as builtin module.

Thanks for reviewing.

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

[toc] | [prev] | [next] | [standalone]

#1246464

On Wednesday 14 October 2015 17:28:45 Ley Foon Tan wrote:
> On Wed, Oct 14, 2015 at 5:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 14 October 2015 16:32:25 Ley Foon Tan wrote:
> >> On Wed, Oct 14, 2015 at 4:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Wednesday 14 October 2015 10:41:29 Ley Foon Tan wrote:
> >> >> +static int altera_pcie_remove(struct platform_device *pdev)
> >> >> +{
> >> >> +     struct altera_pcie *pcie = platform_get_drvdata(pdev);
> >> >> +
> >> >> +     altera_pcie_free_irq_domain(pcie);
> >> >> +     platform_set_drvdata(pdev, NULL);
> >> >> +     return 0;
> >> >> +}
> >> >
> >> > I just noticed this. Does it actually work to unload the module
> >> > and tear down all the pci_dev structures in a safe way?
> >> Good catch. It only can be compiled as builtin-moduley now, so we can
> >> remove this _remove callback function.
> >
> > I think we should change both: make it possible to load the
> > driver dynamically, and remove the altera_pcie_remove function.
> This driver depends on the pci fixups to work correctly. But, fixups
> callback functions in this driver are not being call if the driver is
> loadable module.

Ah, I see. We should find a better way to deal with this, as we
are getting an increasing number of host driver specific fixups.

Bjorn, do you have any idea here?

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.

> > You can prevent the module from being unloaded if you also remove
> > the module_platform_driver() directive and add a module_init()
> > without a matching module_exit().
> >
> > Please also add a '.suppress_bind_attrs = true,' flag in the driver
> > struct to prevent manual unbinding.
> I think we don't need these if it only can work as builtin module.

No, this is orthogonal, you need it either way, as built-in drivers
can still be unbound by writing to sysfs. Try writing the device name
to /sys/bus/platform/drivers/altera-pcie/unbind and watch it blow up ;-)

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

[toc] | [prev] | [next] | [standalone]

#1246482

On Wed, Oct 14, 2015 at 5:36 PM, Arnd Bergmann <arnd@arndb.de> 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@arndb.de> wrote:
>> > On Wednesday 14 October 2015 16:32:25 Ley Foon Tan wrote:
>> >> On Wed, Oct 14, 2015 at 4:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > On Wednesday 14 October 2015 10:41:29 Ley Foon Tan wrote:
>> >> >> +static int altera_pcie_remove(struct platform_device *pdev)
>> >> >> +{
>> >> >> +     struct altera_pcie *pcie = platform_get_drvdata(pdev);
>> >> >> +
>> >> >> +     altera_pcie_free_irq_domain(pcie);
>> >> >> +     platform_set_drvdata(pdev, NULL);
>> >> >> +     return 0;
>> >> >> +}
>> >> >
>> >> > I just noticed this. Does it actually work to unload the module
>> >> > and tear down all the pci_dev structures in a safe way?
>> >> Good catch. It only can be compiled as builtin-moduley now, so we can
>> >> remove this _remove callback function.
>> >
>> > I think we should change both: make it possible to load the
>> > driver dynamically, and remove the altera_pcie_remove function.
>> This driver depends on the pci fixups to work correctly. But, fixups
>> callback functions in this driver are not being call if the driver is
>> loadable module.
>
> Ah, I see. We should find a better way to deal with this, as we
> are getting an increasing number of host driver specific fixups.
>
> Bjorn, do you have any idea here?
>
> 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.

>
>> > You can prevent the module from being unloaded if you also remove
>> > the module_platform_driver() directive and add a module_init()
>> > without a matching module_exit().
>> >
>> > Please also add a '.suppress_bind_attrs = true,' flag in the driver
>> > struct to prevent manual unbinding.
>> I think we don't need these if it only can work as builtin module.
>
> No, this is orthogonal, you need it either way, as built-in drivers
> can still be unbound by writing to sysfs. Try writing the device name
> to /sys/bus/platform/drivers/altera-pcie/unbind and watch it blow up ;-)
Oh I see. Will update with your suggestion.

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1246747

On Wednesday 14 October 2015 18:01:46 Ley Foon Tan wrote:
> On Wed, Oct 14, 2015 at 5:36 PM, Arnd Bergmann <arnd@arndb.de> 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@arndb.de> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248482

On Wed, Oct 14, 2015 at 9:32 PM, Arnd Bergmann <arnd@arndb.de> 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@arndb.de> 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@arndb.de> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web