| Message ID | 1407860725-25202-3-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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! / > --------------- > ¯\_(?)_/¯
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 > >
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! / > --------------- > ¯\_(?)_/¯
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.
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 >
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 >
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).
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
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 >
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
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; }
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(+)