Device node for a controller with two interrupt parents

[Thomas Abraham]

On 25 March 2012 00:37, Grant Likely <grant.likely at secretlab.ca> wrote:
> On Fri, 23 Mar 2012 16:18:09 +0530, Thomas Abraham <thomas.abraham at linaro.org> wrote:
>> Hi Grant,
>>
>> On 21 March 2012 20:43, Grant Likely <grant.likely at secretlab.ca> 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 at 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.