On 25 March 2012 00:37, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote: > On Fri, 23 Mar 2012 16:18:09 +0530, Thomas Abraham <thomas.abraham@xxxxxxxxxx> wrote: >> Hi Grant, >> >> On 21 March 2012 20:43, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote: >> > Okay, so you're saying there are three important aspects to this >> > device: >> > 1) it terminates interrupts from other devices (therefore needs an >> > interrupt controller driver) >> > 2) it passes some interrupts through untouched (interrupt controller >> > driver doesn't need to touch them; it directly raises an irq on the >> > gic or combiner) >> > 3) It is able generate interrupt signals on it's own (independent of >> > any attached devices) >> > >> > Do I understand correctly? >> >> #1 is applicable but not #2 and #3 (if I have interpreted the above >> correctly). The wakeup interrupt controller looks for an edge or level >> on pins (which are connected to external devices) and generates a >> interrupt (to gic or combiner) when the set condition on that pin is >> found (edge or level). >> >> > >> > Your patch above solves the problem for #2 above, but it breaks #1 >> > because interrupts from external devices can no longer be terminated >> > on the wakeup controller node (they'll always get passed through). >> >> Ok. >> >> > >> > --- Possible solution 1 --- >> > If other devices *don't* use the wakeup node as their interrupt >> > parent, then you should be able to simply drop the >> > interrupt-controller property and make the other devices directly >> > reference the gic and combiner. >> >> Other devices use wakeup node as interrupt controller. The wakeup >> interrupt controller supports masking, unmasking and prioritization of >> the wakeup interrupts. The interrupt-controller property can be >> dropped but then of_irq_init() function cannot be used. >> >> > >> > --- Possible solution 2 --- >> > Split the interrupt map into a separate node: >> > >> > >> > wakeup_eint: interrupt-controller@11000000 { >> > compatible = "samsung,exynos4210-wakeup-eint"; >> > reg = <0x11000000 0x1000>; >> > interrupt-controller; >> > #interrupt-cells = <1>; >> > interrupt-parent = <&wakeup_map>; >> > interrupts = <0 1 2 3 f 5 6 7 8 9 10 11 12 13 14 15 16>; >> > >> > wakeup_map: interrupt-map { >> > #interrupt-cells = <1>; >> > #address-cells = <0>; >> > interrupt-map = <0 &gic 0 16 0>, >> > <1 &gic 0 17 0>, >> > <2 &gic 0 18 0>, >> > <3 &gic 0 19 0>, >> > <4 &gic 0 20 0>, >> > <5 &gic 0 21 0>, >> > <6 &gic 0 22 0>, >> > <7 &gic 0 23 0>, >> > <8 &gic 0 24 0>, >> > <9 &gic 0 25 0>, >> > <10 &gic 0 26 0>, >> > <11 &gic 0 27 0>, >> > <12 &gic 0 28 0>, >> > <13 &gic 0 29 0>, >> > <14 &gic 0 30 0>, >> > <15 &gic 0 31 0>, >> > <16 &combiner 2 4>; >> > }; >> > }; >> >> I have tested with this and it works but of_irq_init() function cannot >> be used because 'wakeup_map' is set as interrupt parent and >> 'wakeup_map' is not a interrupt-controller. So of_irq_init() does not >> invoke the initialization function for the wakeup node. If the machine >> init code explicitly looks for this node and calls the corresponding >> initialization function, it works fine. > > That's a bug in of_irq_init() then. It should be fixed. > >> >> > >> > --- possible solution 3 --- >> > 'interrupts' just isn't sufficient for some devices; add a binding for >> > a 'interrupts-multiparent' that can be used instead of 'interrupts' >> > and uses the format <phandle> <specifier> [<phandle> <specifier> [...]]. >> >> This would be the ideal case. In addition to this, the >> interrupt-parent property handling should be modified to support >> multiple parent phandles like <&parent1 [&parent2] [&parent3] ....>. >> This will be useful to extend the capability of of_irq_init() to >> handle interrupt-controller node with multiple interrupt parents. >> >> > >> > I'm not opposed to this solution since it is a more natural binding >> > for multiparented interrupt controllers, but I won't commit to it >> > without feedback and agreement from Mitch, Ben, David Gibson, etc. >> >> Ok. For now, I will go ahead and use solution #2 which you and David >> have suggested. And correspondingly add explicit lookup of wakeup node >> and call to its initialization function in the machine init code. > > I'd really prefer a fix to of_irq_init() instead of hacking around it. Ok. I have prepared a fix for of_irq_init() to complete the init callback for all interrupt controller nodes. So solution #2, works fine without any hacks with this patch. I will post this patch now. Thanks, Thomas. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html