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

[Liviu Dudau]

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@xxxxxxxxxx>
> >> >Cc: Grant Likely <grant.likely@xxxxxxxxxx>
> >> >Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> >> >Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> >> >---
> >> > 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@xxxxxxxxxxxxxxx
> >> >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@xxxxxxxxxxxxxxx
> >> 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!  /
  ---------------
    Â\_(ã)_/Â

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