Device node for a controller with two interrupt parents

[Grant Likely]

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.

g.