diff mbox

[v9,02/12] PCI: OF: Parse and map the IRQ when adding the PCI device.

Message ID 1407860725-25202-3-git-send-email-Liviu.Dudau@arm.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Liviu Dudau Aug. 12, 2014, 4:25 p.m. UTC
Enhance the default implementation of pcibios_add_device() to
parse and map the IRQ of the device if a DT binding is available.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Wei Yang Aug. 14, 2014, 2:58 p.m. UTC | #1
On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
>Enhance the default implementation of pcibios_add_device() to
>parse and map the IRQ of the device if a DT binding is available.
>
>Cc: Bjorn Helgaas <bhelgaas@google.com>
>Cc: Grant Likely <grant.likely@linaro.org>
>Cc: Rob Herring <robh+dt@kernel.org>
>Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>---
> drivers/pci/pci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 1c8592b..29d1775 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -17,6 +17,7 @@
> #include <linux/spinlock.h>
> #include <linux/string.h>
> #include <linux/log2.h>
>+#include <linux/of_pci.h>
> #include <linux/pci-aspm.h>
> #include <linux/pm_wakeup.h>
> #include <linux/interrupt.h>
>@@ -1453,6 +1454,8 @@ EXPORT_SYMBOL(pcim_pin_device);
>  */
> int __weak pcibios_add_device(struct pci_dev *dev)
> {
>+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>+
> 	return 0;
> }

Liviu,

For this, my suggestion is to add arch dependent function to setup the irq
line for pci devices. I can't find an obvious reason this won't work on other
archs, but maybe this will hurt some of them?

>
>-- 
>2.0.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liviu Dudau Aug. 14, 2014, 3:49 p.m. UTC | #2
On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
> >Enhance the default implementation of pcibios_add_device() to
> >parse and map the IRQ of the device if a DT binding is available.
> >
> >Cc: Bjorn Helgaas <bhelgaas@google.com>
> >Cc: Grant Likely <grant.likely@linaro.org>
> >Cc: Rob Herring <robh+dt@kernel.org>
> >Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >---
> > drivers/pci/pci.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >index 1c8592b..29d1775 100644
> >--- a/drivers/pci/pci.c
> >+++ b/drivers/pci/pci.c
> >@@ -17,6 +17,7 @@
> > #include <linux/spinlock.h>
> > #include <linux/string.h>
> > #include <linux/log2.h>
> >+#include <linux/of_pci.h>
> > #include <linux/pci-aspm.h>
> > #include <linux/pm_wakeup.h>
> > #include <linux/interrupt.h>
> >@@ -1453,6 +1454,8 @@ EXPORT_SYMBOL(pcim_pin_device);
> >  */
> > int __weak pcibios_add_device(struct pci_dev *dev)
> > {
> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> >+
> > 	return 0;
> > }
> 
> Liviu,
> 
> For this, my suggestion is to add arch dependent function to setup the irq
> line for pci devices. I can't find an obvious reason this won't work on other
> archs, but maybe this will hurt some of them?

Hi Wei,

I'm not sure I understand your point. Architectures that support OF will obviously
benefit from this common approach, and for the other ones the function is empty
so it will not change existing behaviour. If you are suggesting that I should
create a new API that each architecture could go and implement for setting up the
IRQ line then I would agree that it would be nice to have that, but the question
is how many architectures are outside OF that need this?

If I understood you correctly, it is a nice idea but slightly outside the scope
of my current patchset.

Best regards,
Liviu

> 
> >
> >-- 
> >2.0.4
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Richard Yang
> Help you, Help me
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Wei Yang Aug. 15, 2014, 8:56 a.m. UTC | #3
On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
>On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
>> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
>> >Enhance the default implementation of pcibios_add_device() to
>> >parse and map the IRQ of the device if a DT binding is available.
>> >
>> >Cc: Bjorn Helgaas <bhelgaas@google.com>
>> >Cc: Grant Likely <grant.likely@linaro.org>
>> >Cc: Rob Herring <robh+dt@kernel.org>
>> >Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>> >---
>> > drivers/pci/pci.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> >diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> >index 1c8592b..29d1775 100644
>> >--- a/drivers/pci/pci.c
>> >+++ b/drivers/pci/pci.c
>> >@@ -17,6 +17,7 @@
>> > #include <linux/spinlock.h>
>> > #include <linux/string.h>
>> > #include <linux/log2.h>
>> >+#include <linux/of_pci.h>
>> > #include <linux/pci-aspm.h>
>> > #include <linux/pm_wakeup.h>
>> > #include <linux/interrupt.h>
>> >@@ -1453,6 +1454,8 @@ EXPORT_SYMBOL(pcim_pin_device);
>> >  */
>> > int __weak pcibios_add_device(struct pci_dev *dev)
>> > {
>> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> >+
>> > 	return 0;
>> > }
>> 
>> Liviu,
>> 
>> For this, my suggestion is to add arch dependent function to setup the irq
>> line for pci devices. I can't find an obvious reason this won't work on other
>> archs, but maybe this will hurt some of them?
>
>Hi Wei,
>
>I'm not sure I understand your point. Architectures that support OF will obviously
>benefit from this common approach, and for the other ones the function is empty
>so it will not change existing behaviour. If you are suggesting that I should
>create a new API that each architecture could go and implement for setting up the
>IRQ line then I would agree that it would be nice to have that, but the question
>is how many architectures are outside OF that need this?

My suggestion is to define the pcibios_add_device() for arm arch, like the one
in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
patch set address the pci bus setup mostly on arm arch.

For those archs not support OF, this function is empty and has no effect. I
agree on this one.

For those archs rely on OF, we still have two cases:
    1. they would have implement this function like powerpc
    2. have other way to fix it up,  otherwise how it works now?
If my assumption is correct, this change will either have no effect, or fix up
the irq line the second time. Not harmful, but not necessary.

I am not familiar with other arch, so the second case is my deduction. If this
is not correct, please let me know.

>
>If I understood you correctly, it is a nice idea but slightly outside the scope
>of my current patchset.
>
>Best regards,
>Liviu
>
>> 
>> >
>> >-- 
>> >2.0.4
>> >
>> >--
>> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> >the body of a message to majordomo@vger.kernel.org
>> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> -- 
>> Richard Yang
>> Help you, Help me
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>
>-- 
>====================
>| I would like to |
>| fix the world,  |
>| but they're not |
>| giving me the   |
> \ source code!  /
>  ---------------
>    ¯\_(?)_/¯
Liviu Dudau Aug. 15, 2014, 10:30 a.m. UTC | #4
On Fri, Aug 15, 2014 at 09:56:32AM +0100, Wei Yang wrote:
> On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
> >On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
> >> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
> >> >Enhance the default implementation of pcibios_add_device() to
> >> >parse and map the IRQ of the device if a DT binding is available.
> >> >
> >> >Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> >Cc: Grant Likely <grant.likely@linaro.org>
> >> >Cc: Rob Herring <robh+dt@kernel.org>
> >> >Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >> >---
> >> > drivers/pci/pci.c | 3 +++
> >> > 1 file changed, 3 insertions(+)
> >> >
> >> >diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> >index 1c8592b..29d1775 100644
> >> >--- a/drivers/pci/pci.c
> >> >+++ b/drivers/pci/pci.c
> >> >@@ -17,6 +17,7 @@
> >> > #include <linux/spinlock.h>
> >> > #include <linux/string.h>
> >> > #include <linux/log2.h>
> >> >+#include <linux/of_pci.h>
> >> > #include <linux/pci-aspm.h>
> >> > #include <linux/pm_wakeup.h>
> >> > #include <linux/interrupt.h>
> >> >@@ -1453,6 +1454,8 @@ EXPORT_SYMBOL(pcim_pin_device);
> >> >  */
> >> > int __weak pcibios_add_device(struct pci_dev *dev)
> >> > {
> >> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> >> >+
> >> > 	return 0;
> >> > }
> >> 
> >> Liviu,
> >> 
> >> For this, my suggestion is to add arch dependent function to setup the irq
> >> line for pci devices. I can't find an obvious reason this won't work on other
> >> archs, but maybe this will hurt some of them?
> >
> >Hi Wei,
> >
> >I'm not sure I understand your point. Architectures that support OF will obviously
> >benefit from this common approach, and for the other ones the function is empty
> >so it will not change existing behaviour. If you are suggesting that I should
> >create a new API that each architecture could go and implement for setting up the
> >IRQ line then I would agree that it would be nice to have that, but the question
> >is how many architectures are outside OF that need this?
> 
> My suggestion is to define the pcibios_add_device() for arm arch, like the one
> in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
> patch set address the pci bus setup mostly on arm arch.

And also arm64 at the least.

> 
> For those archs not support OF, this function is empty and has no effect. I
> agree on this one.
> 
> For those archs rely on OF, we still have two cases:
>     1. they would have implement this function like powerpc

Actually, powerpc seems to be the only OF platform reimplementing this function.
s390 and x86 are not OF platforms.

>     2. have other way to fix it up,  otherwise how it works now?

Don't forget that my patchset aims to replace existing house-made code with a more
generic version. When architectures and platforms switch to my code they will have
to add this back in their code if it's needed.

> If my assumption is correct, this change will either have no effect, or fix up
> the irq line the second time. Not harmful, but not necessary.

Well, it will become necessary as old code gets dismantled and converted towards
this patchset. To give you an example that I'm familiar with, for arch/arm the
host bridge drivers have moved into drivers/pci/host, but they still depend/use
the bios32 infrastructure that takes care of setting up the irq. When they switch
to my version they would have to go and debug the "irq not being assigned" issue
and it is quite likely that some of the people doing the conversion will complain
about my code rather than understanding the issue. What I'm trying to do is to
make switching to my patchset as painless as possible, with a cleanup to remove
redundant operations coming after the switchover.

Does that sound like a reasonable plan?

Best regards,
Liviu

> 
> I am not familiar with other arch, so the second case is my deduction. If this
> is not correct, please let me know.
> 
> >
> >If I understood you correctly, it is a nice idea but slightly outside the scope
> >of my current patchset.
> >
> >Best regards,
> >Liviu
> >
> >> 
> >> >
> >> >-- 
> >> >2.0.4
> >> >
> >> >--
> >> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> >the body of a message to majordomo@vger.kernel.org
> >> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> 
> >> -- 
> >> Richard Yang
> >> Help you, Help me
> >> 
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> 
> >
> 
> -- 
> Richard Yang
> Help you, Help me
> 
>
Wei Yang Aug. 18, 2014, 1:44 a.m. UTC | #5
On Fri, Aug 15, 2014 at 11:30:52AM +0100, Liviu Dudau wrote:
>On Fri, Aug 15, 2014 at 09:56:32AM +0100, Wei Yang wrote:
>> On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
>> >On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
>> >> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
>> >> >Enhance the default implementation of pcibios_add_device() to
>> >> >parse and map the IRQ of the device if a DT binding is available.
>> >> >
>> >> >Cc: Bjorn Helgaas <bhelgaas@google.com>
>> >> >Cc: Grant Likely <grant.likely@linaro.org>
>> >> >Cc: Rob Herring <robh+dt@kernel.org>
>> >> >Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>> >> >---
>> >> > drivers/pci/pci.c | 3 +++
>> >> > 1 file changed, 3 insertions(+)
>> >> >
>> >> >diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> >> >index 1c8592b..29d1775 100644
>> >> >--- a/drivers/pci/pci.c
>> >> >+++ b/drivers/pci/pci.c
>> >> >@@ -17,6 +17,7 @@
>> >> > #include <linux/spinlock.h>
>> >> > #include <linux/string.h>
>> >> > #include <linux/log2.h>
>> >> >+#include <linux/of_pci.h>
>> >> > #include <linux/pci-aspm.h>
>> >> > #include <linux/pm_wakeup.h>
>> >> > #include <linux/interrupt.h>
>> >> >@@ -1453,6 +1454,8 @@ EXPORT_SYMBOL(pcim_pin_device);
>> >> >  */
>> >> > int __weak pcibios_add_device(struct pci_dev *dev)
>> >> > {
>> >> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> >> >+
>> >> > 	return 0;
>> >> > }
>> >> 
>> >> Liviu,
>> >> 
>> >> For this, my suggestion is to add arch dependent function to setup the irq
>> >> line for pci devices. I can't find an obvious reason this won't work on other
>> >> archs, but maybe this will hurt some of them?
>> >
>> >Hi Wei,
>> >
>> >I'm not sure I understand your point. Architectures that support OF will obviously
>> >benefit from this common approach, and for the other ones the function is empty
>> >so it will not change existing behaviour. If you are suggesting that I should
>> >create a new API that each architecture could go and implement for setting up the
>> >IRQ line then I would agree that it would be nice to have that, but the question
>> >is how many architectures are outside OF that need this?
>> 
>> My suggestion is to define the pcibios_add_device() for arm arch, like the one
>> in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
>> patch set address the pci bus setup mostly on arm arch.
>
>And also arm64 at the least.
>
>> 
>> For those archs not support OF, this function is empty and has no effect. I
>> agree on this one.
>> 
>> For those archs rely on OF, we still have two cases:
>>     1. they would have implement this function like powerpc
>
>Actually, powerpc seems to be the only OF platform reimplementing this function.
>s390 and x86 are not OF platforms.
>
>>     2. have other way to fix it up,  otherwise how it works now?
>
>Don't forget that my patchset aims to replace existing house-made code with a more
>generic version. When architectures and platforms switch to my code they will have
>to add this back in their code if it's needed.
>
>> If my assumption is correct, this change will either have no effect, or fix up
>> the irq line the second time. Not harmful, but not necessary.
>
>Well, it will become necessary as old code gets dismantled and converted towards
>this patchset. To give you an example that I'm familiar with, for arch/arm the
>host bridge drivers have moved into drivers/pci/host, but they still depend/use
>the bios32 infrastructure that takes care of setting up the irq. When they switch
>to my version they would have to go and debug the "irq not being assigned" issue
>and it is quite likely that some of the people doing the conversion will complain
>about my code rather than understanding the issue. What I'm trying to do is to
>make switching to my patchset as painless as possible, with a cleanup to remove
>redundant operations coming after the switchover.
>

This means this is a temporary version for the switchover period and will be
reverted after switchover?

>Does that sound like a reasonable plan?
>
>Best regards,
>Liviu
>
>> 
>> I am not familiar with other arch, so the second case is my deduction. If this
>> is not correct, please let me know.
>> 
>> >
>> >If I understood you correctly, it is a nice idea but slightly outside the scope
>> >of my current patchset.
>> >
>> >Best regards,
>> >Liviu
>> >
>> >> 
>> >> >
>> >> >-- 
>> >> >2.0.4
>> >> >
>> >> >--
>> >> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> >> >the body of a message to majordomo@vger.kernel.org
>> >> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> 
>> >> -- 
>> >> Richard Yang
>> >> Help you, Help me
>> >> 
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> 
>> >
>> 
>> -- 
>> Richard Yang
>> Help you, Help me
>> 
>> 
>
>-- 
>====================
>| I would like to |
>| fix the world,  |
>| but they're not |
>| giving me the   |
> \ source code!  /
>  ---------------
>    ¯\_(?)_/¯
Catalin Marinas Aug. 18, 2014, 2:25 p.m. UTC | #6
On Fri, Aug 15, 2014 at 11:30:52AM +0100, Liviu Dudau wrote:
> On Fri, Aug 15, 2014 at 09:56:32AM +0100, Wei Yang wrote:
> > On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
> > >On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
> > >> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
> > >> > int __weak pcibios_add_device(struct pci_dev *dev)
> > >> > {
> > >> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> > >> >+
> > >> > 	return 0;
> > >> > }
> > >> 
> > >> For this, my suggestion is to add arch dependent function to setup the irq
> > >> line for pci devices. I can't find an obvious reason this won't work on other
> > >> archs, but maybe this will hurt some of them?
> > >
> > >I'm not sure I understand your point. Architectures that support OF will obviously
> > >benefit from this common approach, and for the other ones the function is empty
> > >so it will not change existing behaviour. If you are suggesting that I should
> > >create a new API that each architecture could go and implement for setting up the
> > >IRQ line then I would agree that it would be nice to have that, but the question
> > >is how many architectures are outside OF that need this?
> > 
> > My suggestion is to define the pcibios_add_device() for arm arch, like the one
> > in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
> > patch set address the pci bus setup mostly on arm arch.
> 
> And also arm64 at the least.
...
> Well, it will become necessary as old code gets dismantled and converted towards
> this patchset. To give you an example that I'm familiar with, for arch/arm the
> host bridge drivers have moved into drivers/pci/host, but they still depend/use
> the bios32 infrastructure that takes care of setting up the irq. When they switch
> to my version they would have to go and debug the "irq not being assigned" issue
> and it is quite likely that some of the people doing the conversion will complain
> about my code rather than understanding the issue. What I'm trying to do is to
> make switching to my patchset as painless as possible, with a cleanup to remove
> redundant operations coming after the switchover.

While the goal is fine, until we see a common pattern for what needs to
go into pcibios_add_device() I think we should have an arm64-specific
implementation (and probably an arm32 specific one as well). I can see
powerpc uses it for setting the DMA ops. Would we have a similar need on
arm64 to choose between coherent and non-coherent dma_ops?

Also at some point we'll get ACPI support, so I'm not sure what we do
with assigning the dev->irq here but definitely of_* functions won't
work.
Liviu Dudau Aug. 18, 2014, 9:26 p.m. UTC | #7
On Mon, Aug 18, 2014 at 09:44:04AM +0800, Wei Yang wrote:
> On Fri, Aug 15, 2014 at 11:30:52AM +0100, Liviu Dudau wrote:
> >On Fri, Aug 15, 2014 at 09:56:32AM +0100, Wei Yang wrote:
> >> On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
> >> >On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
> >> >> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
> >> >> >Enhance the default implementation of pcibios_add_device() to
> >> >> >parse and map the IRQ of the device if a DT binding is available.
> >> >> >
> >> >> >Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> >> >Cc: Grant Likely <grant.likely@linaro.org>
> >> >> >Cc: Rob Herring <robh+dt@kernel.org>
> >> >> >Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >> >> >---
> >> >> > drivers/pci/pci.c | 3 +++
> >> >> > 1 file changed, 3 insertions(+)
> >> >> >
> >> >> >diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> >> >index 1c8592b..29d1775 100644
> >> >> >--- a/drivers/pci/pci.c
> >> >> >+++ b/drivers/pci/pci.c
> >> >> >@@ -17,6 +17,7 @@
> >> >> > #include <linux/spinlock.h>
> >> >> > #include <linux/string.h>
> >> >> > #include <linux/log2.h>
> >> >> >+#include <linux/of_pci.h>
> >> >> > #include <linux/pci-aspm.h>
> >> >> > #include <linux/pm_wakeup.h>
> >> >> > #include <linux/interrupt.h>
> >> >> >@@ -1453,6 +1454,8 @@ EXPORT_SYMBOL(pcim_pin_device);
> >> >> >  */
> >> >> > int __weak pcibios_add_device(struct pci_dev *dev)
> >> >> > {
> >> >> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> >> >> >+
> >> >> > 	return 0;
> >> >> > }
> >> >> 
> >> >> Liviu,
> >> >> 
> >> >> For this, my suggestion is to add arch dependent function to setup the irq
> >> >> line for pci devices. I can't find an obvious reason this won't work on other
> >> >> archs, but maybe this will hurt some of them?
> >> >
> >> >Hi Wei,
> >> >
> >> >I'm not sure I understand your point. Architectures that support OF will obviously
> >> >benefit from this common approach, and for the other ones the function is empty
> >> >so it will not change existing behaviour. If you are suggesting that I should
> >> >create a new API that each architecture could go and implement for setting up the
> >> >IRQ line then I would agree that it would be nice to have that, but the question
> >> >is how many architectures are outside OF that need this?
> >> 
> >> My suggestion is to define the pcibios_add_device() for arm arch, like the one
> >> in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
> >> patch set address the pci bus setup mostly on arm arch.
> >
> >And also arm64 at the least.
> >
> >> 
> >> For those archs not support OF, this function is empty and has no effect. I
> >> agree on this one.
> >> 
> >> For those archs rely on OF, we still have two cases:
> >>     1. they would have implement this function like powerpc
> >
> >Actually, powerpc seems to be the only OF platform reimplementing this function.
> >s390 and x86 are not OF platforms.
> >
> >>     2. have other way to fix it up,  otherwise how it works now?
> >
> >Don't forget that my patchset aims to replace existing house-made code with a more
> >generic version. When architectures and platforms switch to my code they will have
> >to add this back in their code if it's needed.
> >
> >> If my assumption is correct, this change will either have no effect, or fix up
> >> the irq line the second time. Not harmful, but not necessary.
> >
> >Well, it will become necessary as old code gets dismantled and converted towards
> >this patchset. To give you an example that I'm familiar with, for arch/arm the
> >host bridge drivers have moved into drivers/pci/host, but they still depend/use
> >the bios32 infrastructure that takes care of setting up the irq. When they switch
> >to my version they would have to go and debug the "irq not being assigned" issue
> >and it is quite likely that some of the people doing the conversion will complain
> >about my code rather than understanding the issue. What I'm trying to do is to
> >make switching to my patchset as painless as possible, with a cleanup to remove
> >redundant operations coming after the switchover.
> >
> 
> This means this is a temporary version for the switchover period and will be
> reverted after switchover?

At least one of the redundant operations will be removed. Most likely the one
in the arch code or in the driver. Do you have any concerns with that idea?

Best regards,
Liviu

> 
> >Does that sound like a reasonable plan?
> >
> >Best regards,
> >Liviu
> >
> >> 
> >> I am not familiar with other arch, so the second case is my deduction. If this
> >> is not correct, please let me know.
> >> 
> >> >
> >> >If I understood you correctly, it is a nice idea but slightly outside the scope
> >> >of my current patchset.
> >> >
> >> >Best regards,
> >> >Liviu
> >> >
> >> >> 
> >> >> >
> >> >> >-- 
> >> >> >2.0.4
> >> >> >
> >> >> >--
> >> >> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> >> >the body of a message to majordomo@vger.kernel.org
> >> >> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> >> 
> >> >> -- 
> >> >> Richard Yang
> >> >> Help you, Help me
> >> >> 
> >> >> --
> >> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> >> the body of a message to majordomo@vger.kernel.org
> >> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> >> 
> >> >
> >> 
> >> -- 
> >> Richard Yang
> >> Help you, Help me
> >> 
> >> 
> >
> >-- 
> >====================
> >| I would like to |
> >| fix the world,  |
> >| but they're not |
> >| giving me the   |
> > \ source code!  /
> >  ---------------
> >    ¯\_(?)_/¯
> 
> -- 
> Richard Yang
> Help you, Help me
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Liviu Dudau Aug. 18, 2014, 9:30 p.m. UTC | #8
On Mon, Aug 18, 2014 at 03:25:50PM +0100, Catalin Marinas wrote:
> On Fri, Aug 15, 2014 at 11:30:52AM +0100, Liviu Dudau wrote:
> > On Fri, Aug 15, 2014 at 09:56:32AM +0100, Wei Yang wrote:
> > > On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
> > > >On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
> > > >> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
> > > >> > int __weak pcibios_add_device(struct pci_dev *dev)
> > > >> > {
> > > >> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> > > >> >+
> > > >> > 	return 0;
> > > >> > }
> > > >> 
> > > >> For this, my suggestion is to add arch dependent function to setup the irq
> > > >> line for pci devices. I can't find an obvious reason this won't work on other
> > > >> archs, but maybe this will hurt some of them?
> > > >
> > > >I'm not sure I understand your point. Architectures that support OF will obviously
> > > >benefit from this common approach, and for the other ones the function is empty
> > > >so it will not change existing behaviour. If you are suggesting that I should
> > > >create a new API that each architecture could go and implement for setting up the
> > > >IRQ line then I would agree that it would be nice to have that, but the question
> > > >is how many architectures are outside OF that need this?
> > > 
> > > My suggestion is to define the pcibios_add_device() for arm arch, like the one
> > > in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
> > > patch set address the pci bus setup mostly on arm arch.
> > 
> > And also arm64 at the least.
> ...
> > Well, it will become necessary as old code gets dismantled and converted towards
> > this patchset. To give you an example that I'm familiar with, for arch/arm the
> > host bridge drivers have moved into drivers/pci/host, but they still depend/use
> > the bios32 infrastructure that takes care of setting up the irq. When they switch
> > to my version they would have to go and debug the "irq not being assigned" issue
> > and it is quite likely that some of the people doing the conversion will complain
> > about my code rather than understanding the issue. What I'm trying to do is to
> > make switching to my patchset as painless as possible, with a cleanup to remove
> > redundant operations coming after the switchover.
> 
> While the goal is fine, until we see a common pattern for what needs to
> go into pcibios_add_device() I think we should have an arm64-specific
> implementation (and probably an arm32 specific one as well). I can see
> powerpc uses it for setting the DMA ops. Would we have a similar need on
> arm64 to choose between coherent and non-coherent dma_ops?

At this point I would like to hear more from people doing the conversion of
the drivers. I cannot answer fully for all arm or arm64 drivers.

> 
> Also at some point we'll get ACPI support, so I'm not sure what we do
> with assigning the dev->irq here but definitely of_* functions won't
> work.

Yes, maybe adding a check if the IRQ has not been assigned already?
I have no idea what the order will be between ACPI ops and of_* ones.

Best regards,
Liviu

> 
> -- 
> Catalin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Catalin Marinas Aug. 18, 2014, 10:09 p.m. UTC | #9
On Mon, Aug 18, 2014 at 10:30:54PM +0100, Liviu Dudau wrote:
> On Mon, Aug 18, 2014 at 03:25:50PM +0100, Catalin Marinas wrote:
> > On Fri, Aug 15, 2014 at 11:30:52AM +0100, Liviu Dudau wrote:
> > > On Fri, Aug 15, 2014 at 09:56:32AM +0100, Wei Yang wrote:
> > > > On Thu, Aug 14, 2014 at 04:49:59PM +0100, Liviu Dudau wrote:
> > > > >On Thu, Aug 14, 2014 at 03:58:04PM +0100, Wei Yang wrote:
> > > > >> On Tue, Aug 12, 2014 at 05:25:15PM +0100, Liviu Dudau wrote:
> > > > >> > int __weak pcibios_add_device(struct pci_dev *dev)
> > > > >> > {
> > > > >> >+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> > > > >> >+
> > > > >> > 	return 0;
> > > > >> > }
> > > > >> 
> > > > >> For this, my suggestion is to add arch dependent function to setup the irq
> > > > >> line for pci devices. I can't find an obvious reason this won't work on other
> > > > >> archs, but maybe this will hurt some of them?
> > > > >
> > > > >I'm not sure I understand your point. Architectures that support OF will obviously
> > > > >benefit from this common approach, and for the other ones the function is empty
> > > > >so it will not change existing behaviour. If you are suggesting that I should
> > > > >create a new API that each architecture could go and implement for setting up the
> > > > >IRQ line then I would agree that it would be nice to have that, but the question
> > > > >is how many architectures are outside OF that need this?
> > > > 
> > > > My suggestion is to define the pcibios_add_device() for arm arch, like the one
> > > > in arch/powerpc/kernel/pci-common.c. If my understanding is correct, this
> > > > patch set address the pci bus setup mostly on arm arch.
> > > 
> > > And also arm64 at the least.
> > ...
> > > Well, it will become necessary as old code gets dismantled and converted towards
> > > this patchset. To give you an example that I'm familiar with, for arch/arm the
> > > host bridge drivers have moved into drivers/pci/host, but they still depend/use
> > > the bios32 infrastructure that takes care of setting up the irq. When they switch
> > > to my version they would have to go and debug the "irq not being assigned" issue
> > > and it is quite likely that some of the people doing the conversion will complain
> > > about my code rather than understanding the issue. What I'm trying to do is to
> > > make switching to my patchset as painless as possible, with a cleanup to remove
> > > redundant operations coming after the switchover.
> > 
> > While the goal is fine, until we see a common pattern for what needs to
> > go into pcibios_add_device() I think we should have an arm64-specific
> > implementation (and probably an arm32 specific one as well). I can see
> > powerpc uses it for setting the DMA ops. Would we have a similar need on
> > arm64 to choose between coherent and non-coherent dma_ops?
> 
> At this point I would like to hear more from people doing the conversion of
> the drivers. I cannot answer fully for all arm or arm64 drivers.

As I read the description of pcibios_add_device(), it is meant as an
architecture hook. While it's nice to generalise this, I'm not sure we
we can find a common denominator. For example, we may want to call
set_dma_ops() as powerpc does but the ops would be arm64 specific (so
even the __weak implementation may not be used by any architecture).
Wei Yang Aug. 19, 2014, 1:44 a.m. UTC | #10
On Mon, Aug 18, 2014 at 03:25:50PM +0100, Catalin Marinas wrote:
>...
>> Well, it will become necessary as old code gets dismantled and converted towards
>> this patchset. To give you an example that I'm familiar with, for arch/arm the
>> host bridge drivers have moved into drivers/pci/host, but they still depend/use
>> the bios32 infrastructure that takes care of setting up the irq. When they switch
>> to my version they would have to go and debug the "irq not being assigned" issue
>> and it is quite likely that some of the people doing the conversion will complain
>> about my code rather than understanding the issue. What I'm trying to do is to
>> make switching to my patchset as painless as possible, with a cleanup to remove
>> redundant operations coming after the switchover.
>
>While the goal is fine, until we see a common pattern for what needs to
>go into pcibios_add_device() I think we should have an arm64-specific
>implementation (and probably an arm32 specific one as well). I can see
>powerpc uses it for setting the DMA ops. Would we have a similar need on
>arm64 to choose between coherent and non-coherent dma_ops?

Liviu,

I have the same feeling with Catalin. An arm64-specific implementation of
pcibios_add_device() would be better.

No more other concerns from my side.

>
>Also at some point we'll get ACPI support, so I'm not sure what we do
>with assigning the dev->irq here but definitely of_* functions won't
>work.
>
>-- 
>Catalin
Liviu Dudau Aug. 19, 2014, 12:05 p.m. UTC | #11
On Tue, Aug 19, 2014 at 09:44:01AM +0800, Wei Yang wrote:
> On Mon, Aug 18, 2014 at 03:25:50PM +0100, Catalin Marinas wrote:
> >...
> >> Well, it will become necessary as old code gets dismantled and converted towards
> >> this patchset. To give you an example that I'm familiar with, for arch/arm the
> >> host bridge drivers have moved into drivers/pci/host, but they still depend/use
> >> the bios32 infrastructure that takes care of setting up the irq. When they switch
> >> to my version they would have to go and debug the "irq not being assigned" issue
> >> and it is quite likely that some of the people doing the conversion will complain
> >> about my code rather than understanding the issue. What I'm trying to do is to
> >> make switching to my patchset as painless as possible, with a cleanup to remove
> >> redundant operations coming after the switchover.
> >
> >While the goal is fine, until we see a common pattern for what needs to
> >go into pcibios_add_device() I think we should have an arm64-specific
> >implementation (and probably an arm32 specific one as well). I can see
> >powerpc uses it for setting the DMA ops. Would we have a similar need on
> >arm64 to choose between coherent and non-coherent dma_ops?
> 
> Liviu,
> 
> I have the same feeling with Catalin. An arm64-specific implementation of
> pcibios_add_device() would be better.
> 
> No more other concerns from my side.

OK, I will make the change when returning from my holiday at the beginning of September,
when I plan to send v10.

Best regards,
Liviu

> 
> >
> >Also at some point we'll get ACPI support, so I'm not sure what we do
> >with assigning the dev->irq here but definitely of_* functions won't
> >work.
> >
> >-- 
> >Catalin
> 
> -- 
> Richard Yang
> Help you, Help me
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Arnd Bergmann Aug. 19, 2014, 12:39 p.m. UTC | #12
On Tuesday 19 August 2014, Catalin Marinas wrote:
> On Mon, Aug 18, 2014 at 10:30:54PM +0100, Liviu Dudau wrote:
> > At this point I would like to hear more from people doing the conversion of
> > the drivers. I cannot answer fully for all arm or arm64 drivers.
> 
> As I read the description of pcibios_add_device(), it is meant as an
> architecture hook. While it's nice to generalise this, I'm not sure we
> we can find a common denominator. For example, we may want to call
> set_dma_ops() as powerpc does but the ops would be arm64 specific (so
> even the __weak implementation may not be used by any architecture).

Right. I'm a strong advocate of the idea of having no architecture specific
parts in the PCI support, but I can see the set_dma_ops() call as a
good reason to make an exception here.

We could add another layer of indirection for the purpose of API purity
here, to call an architecture specific DMA setup function, but it doesn't
really add that much benefits, in particular when the DMA setup is currently
in flux as well.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1c8592b..29d1775 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -17,6 +17,7 @@ 
 #include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/log2.h>
+#include <linux/of_pci.h>
 #include <linux/pci-aspm.h>
 #include <linux/pm_wakeup.h>
 #include <linux/interrupt.h>
@@ -1453,6 +1454,8 @@  EXPORT_SYMBOL(pcim_pin_device);
  */
 int __weak pcibios_add_device(struct pci_dev *dev)
 {
+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+
 	return 0;
 }