Device node for a controller with two interrupt parents

Device node for a controller with two interrupt parents

[Thomas Abraham]

Hi,

Exynos5 includes a gpio wakeup interrupt controller that generates 32
interrupts. The first 16 interrupts are routed to the interrupt
combiner controller. The last 16 are muxed into one interrupt and this
interrupt line is connected to the GIC interrupt controller.

So, the wakeup interrupt controller node in device tree requires two
interrupt parents. I do not know how to handle this. Any suggestions
will be very helpful.

Thanks,
Thomas.

Re: Device node for a controller with two interrupt parents

[David Gibson]

On Wed, Mar 21, 2012 at 07:55:43AM +0530, Thomas Abraham wrote:
> Hi,
> 
> Exynos5 includes a gpio wakeup interrupt controller that generates 32
> interrupts. The first 16 interrupts are routed to the interrupt
> combiner controller. The last 16 are muxed into one interrupt and this
> interrupt line is connected to the GIC interrupt controller.
> 
> So, the wakeup interrupt controller node in device tree requires two
> interrupt parents. I do not know how to handle this. Any suggestions
> will be very helpful.

This has occurred before, for example on the MAL device on 440EP (see
the bamboo board dts for example).  The semi-standard approach is to
make the node an interrupt-nexus for itself.  That is in the node's
interrupts property, just list 0..N giving as many interrupts as you
need.  Set the node's interrupt-parent to point to the node itself,
then add interrupt-map and interrupt-map-mask properties which remap
those interrupts 0..N to the correct interrupts on the actual
interrupt controllers.  Each entry in the interrupt map specifies an
interrupt parent phandle, so you can distribute the irqs to multiple
interrupt controllers that way.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Device node for a controller with two interrupt parents

[Thomas Abraham]

Hi David,

On 21 March 2012 09:11, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Mar 21, 2012 at 07:55:43AM +0530, Thomas Abraham wrote:
>> Hi,
>>
>> Exynos5 includes a gpio wakeup interrupt controller that generates 32
>> interrupts. The first 16 interrupts are routed to the interrupt
>> combiner controller. The last 16 are muxed into one interrupt and this
>> interrupt line is connected to the GIC interrupt controller.
>>
>> So, the wakeup interrupt controller node in device tree requires two
>> interrupt parents. I do not know how to handle this. Any suggestions
>> will be very helpful.
>
> This has occurred before, for example on the MAL device on 440EP (see
> the bamboo board dts for example).  The semi-standard approach is to
> make the node an interrupt-nexus for itself.  That is in the node's
> interrupts property, just list 0..N giving as many interrupts as you
> need.  Set the node's interrupt-parent to point to the node itself,
> then add interrupt-map and interrupt-map-mask properties which remap
> those interrupts 0..N to the correct interrupts on the actual
> interrupt controllers.  Each entry in the interrupt map specifies an
> interrupt parent phandle, so you can distribute the irqs to multiple
> interrupt controllers that way.

Thanks for your suggestion and pointing out an example. I tried this
approach for Exynos4 and Exynos5. It mostly works but there are two
issues here.

1. In the Exynos5 case, the wakeup interrupt controller (which has two
separate interrupt parents - gic and combiner) is itself a interrupt
controller and has the 'interrupt-controller' property. So
of_irq_map_raw() function does not process the interrupt-map in the
wakeup interrupt controller device node. I did the following change to
get past this but I am not sure if this the correct thing to do.

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 9cf0060..892ac95 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -152,7 +152,8 @@ int of_irq_map_raw(struct device_node *parent,
const __be32 *intspec,
                /* Now check if cursor is an interrupt-controller and if it is
                 * then we are done
                 */
-               if (of_get_property(ipar, "interrupt-controller", NULL) !=
+               if (!of_get_property(ipar, "interrupt-map", &imaplen) &&
+                       of_get_property(ipar, "interrupt-controller", NULL) !=
                                NULL) {
                        pr_debug(" -> got it !\n");
                        for (i = 0; i < intsize; i++)


2. The of_irq_init() function (mainly used on the arm platforms) looks
for nodes with 'interrupt-controller' and initializes them with the
parents initialized first. In the Exynos4/5 case, the wakeup interrupt
has two interrupt parents and of_irq_init() function does not seem to
be consider this case. And hence, the wakeup interrupt controller is
being initialized, without the combiner being initialized.

The following are the interrupt-controller nodes, that I have been
testing with (slightly modified for testing)

       gic:interrupt-controller@10490000 {
               compatible = "arm,cortex-a9-gic";
               #interrupt-cells = <3>;
               #address-cells = <0>;
               #size-cells = <0>;
               interrupt-controller;
               cpu-offset = <0x8000>;
               reg = <0x10490000 0x1000>, <0x10480000 0x100>;
       };

       combiner:interrupt-controller@10440000 {
               compatible = "samsung,exynos4210-combiner";
               #interrupt-cells = <2>;
               interrupt-controller;
               samsung,combiner-nr = <16>;
               reg = <0x10440000 0x1000>;
               interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
                            <0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
                            <0 8 0>, <0 9 0>, <0 10 0>, <0 11 0>,
                            <0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
       };

       wakeup_eint: interrupt-controller@11000000 {
               compatible = "samsung,exynos4210-wakeup-eint";
               reg = <0x11000000 0x1000>;
               interrupt-controller;
               #interrupt-cells = <1>;
               interrupt-parent = <&wakeup_eint>;
               interrupts = <0x0>, <0x1>, <0x2>, <0x3>,
                               <0x4>, <0x5>, <0x6>, <0x7>,
                               <0x8>, <0x9>, <0xa>, <0xb>,
                               <0xc>, <0xd>, <0xe>, <0xf>,
                               <0x10>;
               #address-cells = <0>;
               #size-cells = <0>;
               interrupt-map = <0x0 &gic 0 16 0>,
                               <0x1 &gic 0 17 0>,
                               <0x2 &gic 0 18 0>,
                               <0x3 &gic 0 19 0>,
                               <0x4 &gic 0 20 0>,
                               <0x5 &gic 0 21 0>,
                               <0x6 &gic 0 22 0>,
                               <0x7 &gic 0 23 0>,
                               <0x8 &gic 0 24 0>,
                               <0x9 &gic 0 25 0>,
                               <0xa &gic 0 26 0>,
                               <0xb &gic 0 27 0>,
                               <0xc &gic 0 28 0>,
                               <0xd &gic 0 29 0>,
                               <0xe &gic 0 30 0>,
                               <0xf &gic 0 31 0>,
                               <0x10 &combiner 2 4>;
       };

Thanks,
Thomas.

Re: Device node for a controller with two interrupt parents

[Grant Likely]

On Wed, 21 Mar 2012 19:05:26 +0530, Thomas Abraham <thomas.abraham@linaro.org> wrote:
> Hi David,
> 
> On 21 March 2012 09:11, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Wed, Mar 21, 2012 at 07:55:43AM +0530, Thomas Abraham wrote:
> >> Hi,
> >>
> >> Exynos5 includes a gpio wakeup interrupt controller that generates 32
> >> interrupts. The first 16 interrupts are routed to the interrupt
> >> combiner controller. The last 16 are muxed into one interrupt and this
> >> interrupt line is connected to the GIC interrupt controller.
> >>
> >> So, the wakeup interrupt controller node in device tree requires two
> >> interrupt parents. I do not know how to handle this. Any suggestions
> >> will be very helpful.
> >
> > This has occurred before, for example on the MAL device on 440EP (see
> > the bamboo board dts for example).  The semi-standard approach is to
> > make the node an interrupt-nexus for itself.  That is in the node's
> > interrupts property, just list 0..N giving as many interrupts as you
> > need.  Set the node's interrupt-parent to point to the node itself,
> > then add interrupt-map and interrupt-map-mask properties which remap
> > those interrupts 0..N to the correct interrupts on the actual
> > interrupt controllers.  Each entry in the interrupt map specifies an
> > interrupt parent phandle, so you can distribute the irqs to multiple
> > interrupt controllers that way.
> 
> Thanks for your suggestion and pointing out an example. I tried this
> approach for Exynos4 and Exynos5. It mostly works but there are two
> issues here.
> 
> 1. In the Exynos5 case, the wakeup interrupt controller (which has two
> separate interrupt parents - gic and combiner) is itself a interrupt
> controller and has the 'interrupt-controller' property. So
> of_irq_map_raw() function does not process the interrupt-map in the
> wakeup interrupt controller device node. I did the following change to
> get past this but I am not sure if this the correct thing to do.
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 9cf0060..892ac95 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -152,7 +152,8 @@ int of_irq_map_raw(struct device_node *parent,
> const __be32 *intspec,
>                 /* Now check if cursor is an interrupt-controller and if it is
>                  * then we are done
>                  */
> -               if (of_get_property(ipar, "interrupt-controller", NULL) !=
> +               if (!of_get_property(ipar, "interrupt-map", &imaplen) &&
> +                       of_get_property(ipar, "interrupt-controller", NULL) !=
>                                 NULL) {
>                         pr_debug(" -> got it !\n");
>                         for (i = 0; i < intsize; i++)

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?

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).

--- 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.

--- 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>;
		};
	};

--- 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> [...]].

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.


> 
> 
> 2. The of_irq_init() function (mainly used on the arm platforms) looks
> for nodes with 'interrupt-controller' and initializes them with the
> parents initialized first. In the Exynos4/5 case, the wakeup interrupt
> has two interrupt parents and of_irq_init() function does not seem to
> be consider this case. And hence, the wakeup interrupt controller is
> being initialized, without the combiner being initialized.
> 
> The following are the interrupt-controller nodes, that I have been
> testing with (slightly modified for testing)
> 
>        gic:interrupt-controller@10490000 {
>                compatible = "arm,cortex-a9-gic";
>                #interrupt-cells = <3>;
>                #address-cells = <0>;
>                #size-cells = <0>;
>                interrupt-controller;
>                cpu-offset = <0x8000>;
>                reg = <0x10490000 0x1000>, <0x10480000 0x100>;
>        };
> 
>        combiner:interrupt-controller@10440000 {
>                compatible = "samsung,exynos4210-combiner";
>                #interrupt-cells = <2>;
>                interrupt-controller;
>                samsung,combiner-nr = <16>;
>                reg = <0x10440000 0x1000>;
>                interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
>                             <0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
>                             <0 8 0>, <0 9 0>, <0 10 0>, <0 11 0>,
>                             <0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
>        };
> 
>        wakeup_eint: interrupt-controller@11000000 {
>                compatible = "samsung,exynos4210-wakeup-eint";
>                reg = <0x11000000 0x1000>;
>                interrupt-controller;
>                #interrupt-cells = <1>;
>                interrupt-parent = <&wakeup_eint>;
>                interrupts = <0x0>, <0x1>, <0x2>, <0x3>,
>                                <0x4>, <0x5>, <0x6>, <0x7>,
>                                <0x8>, <0x9>, <0xa>, <0xb>,
>                                <0xc>, <0xd>, <0xe>, <0xf>,
>                                <0x10>;
>                #address-cells = <0>;
>                #size-cells = <0>;
>                interrupt-map = <0x0 &gic 0 16 0>,
>                                <0x1 &gic 0 17 0>,
>                                <0x2 &gic 0 18 0>,
>                                <0x3 &gic 0 19 0>,
>                                <0x4 &gic 0 20 0>,
>                                <0x5 &gic 0 21 0>,
>                                <0x6 &gic 0 22 0>,
>                                <0x7 &gic 0 23 0>,
>                                <0x8 &gic 0 24 0>,
>                                <0x9 &gic 0 25 0>,
>                                <0xa &gic 0 26 0>,
>                                <0xb &gic 0 27 0>,
>                                <0xc &gic 0 28 0>,
>                                <0xd &gic 0 29 0>,
>                                <0xe &gic 0 30 0>,
>                                <0xf &gic 0 31 0>,
>                                <0x10 &combiner 2 4>;
>        };
> 
> Thanks,
> Thomas.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.

Re: Device node for a controller with two interrupt parents

[Thomas Abraham]

Hi Grant,

On 21 March 2012 20:43, Grant Likely <grant.likely@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@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.

>
> --- 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.

Thanks for your suggestions.

Regards,
Thomas.

[...]

Re: Device node for a controller with two interrupt parents

[Grant Likely]

[-- Attachment #1: Type: text/plain, Size: 5490 bytes --]

On Fri, 23 Mar 2012 16:18:09 +0530, Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Hi Grant,
> 
> On 21 March 2012 20:43, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> 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.

g.


[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Device node for a controller with two interrupt parents

[Thomas Abraham]

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

[PATCH] of/irq: of_irq_init: Call initialization function for all controllers

[Thomas Abraham]

The of_irq_init function stops processing the interrupt controller hierarchy
when there are no more interrupt controller parents identified. Though this
condition suffices most cases, there are cases where a interrupt controller's
parent controller does not participate in the initialization of the interrupt
hierarchy. An example of such a case is the use of a interrupt nexus node
by a interrupt controller node which delivers interrupts to separate interrupt
parent controllers.

Instead of stopping the processing of interrupt controller hierarchy in such
a case, the orphan interrupt controller node's descriptor can be identified
and its 'logical' parent in the descriptor is set as NULL. The processing of
interrupt hierarchy is then restarted by looking for descriptors which have
a NULL interrupt parent.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/of/irq.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 9cf0060..70c6ece 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -400,6 +400,38 @@ struct intc_desc {
 };
 
 /**
+ * of_irq_mark_orphan_desc - Set parent as NULL for a orphan intc_desc
+ * @intc_desc_list: the list of intc_desc to search for orphan intc_desc
+ *
+ * This is a helper function for the of_irq_init function and is invoked
+ * when there are child nodes available in intc_desc_list but there are
+ * no parent nodes in intc_parent_list. When invoked, this function
+ * searches for a intc_desc instance that does not have a parent intc_desc
+ * instance in intc_desc_list. The very reason of the invocation of this
+ * function ensures that a orphan intc_desc will be found. When found, the
+ * interrupt_parent of the orphan intc_desc is set to NULL.
+ */
+static void of_irq_mark_orphan_desc(struct list_head *intc_desc_list)
+{
+	struct intc_desc *desc, *temp_desc;
+
+	list_for_each_entry_safe(desc, temp_desc, intc_desc_list, list) {
+		struct intc_desc *td1, *td2;
+		list_for_each_entry_safe(td1, td2, intc_desc_list, list) {
+			if (desc->interrupt_parent == td1->dev)
+				break;
+		}
+		if (desc->interrupt_parent == td1->dev)
+			continue;
+
+		pr_debug("%s: set interrupt_parent of 'intc_desc' with dev name"
+			" %s as NULL\n", __func__, desc->dev->full_name);
+		desc->interrupt_parent = NULL;
+		return;
+	}
+}
+
+/**
  * of_irq_init - Scan and init matching interrupt controllers in DT
  * @matches: 0 terminated array of nodes to match and init function to call
  *
@@ -481,8 +513,19 @@ void __init of_irq_init(const struct of_device_id *matches)
 		/* Get the next pending parent that might have children */
 		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
 		if (list_empty(&intc_parent_list) || !desc) {
+			/*
+			 * This has reached a point where there are children in
+			 * the intc_desc_list but no parent in intc_parent_list.
+			 * This means there is a child desc in intc_desc_list
+			 * whose parent is not one of the remaining elements of
+			 * the intc_desc_list. Such a child node is marked as
+			 * orphan (interrupt_parent is set to NULL) and the
+			 * process continues with parent set to NULL.
+			 */
 			pr_err("of_irq_init: children remain, but no parents\n");
-			break;
+			of_irq_mark_orphan_desc(&intc_desc_list);
+			parent = NULL;
+			continue;
 		}
 		list_del(&desc->list);
 		parent = desc->dev;
-- 
1.6.6.rc2

Re: [PATCH] of/irq: of_irq_init: Call initialization function for all controllers

[Rob Herring]

On 03/25/2012 07:38 AM, Thomas Abraham wrote:
> The of_irq_init function stops processing the interrupt controller hierarchy
> when there are no more interrupt controller parents identified. Though this
> condition suffices most cases, there are cases where a interrupt controller's
> parent controller does not participate in the initialization of the interrupt
> hierarchy. An example of such a case is the use of a interrupt nexus node
> by a interrupt controller node which delivers interrupts to separate interrupt
> parent controllers.
> 
> Instead of stopping the processing of interrupt controller hierarchy in such
> a case, the orphan interrupt controller node's descriptor can be identified
> and its 'logical' parent in the descriptor is set as NULL. The processing of
> interrupt hierarchy is then restarted by looking for descriptors which have
> a NULL interrupt parent.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---

Wouldn't this accomplish the same thing? You just need to add the
wakeup-map node name to your matches list.


diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 9cf0060..deeaf00 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -416,8 +416,6 @@ void __init of_irq_init(const struct of_device_id
*matches)
        INIT_LIST_HEAD(&intc_parent_list);

        for_each_matching_node(np, matches) {
-               if (!of_find_property(np, "interrupt-controller", NULL))
-                       continue;
                /*
                 * Here, we allocate and populate an intc_desc with the node
                 * pointer, interrupt-parent device_node etc.


>  drivers/of/irq.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 44 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 9cf0060..70c6ece 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -400,6 +400,38 @@ struct intc_desc {
>  };
>  
>  /**
> + * of_irq_mark_orphan_desc - Set parent as NULL for a orphan intc_desc
> + * @intc_desc_list: the list of intc_desc to search for orphan intc_desc
> + *
> + * This is a helper function for the of_irq_init function and is invoked
> + * when there are child nodes available in intc_desc_list but there are
> + * no parent nodes in intc_parent_list. When invoked, this function
> + * searches for a intc_desc instance that does not have a parent intc_desc
> + * instance in intc_desc_list. The very reason of the invocation of this
> + * function ensures that a orphan intc_desc will be found. When found, the
> + * interrupt_parent of the orphan intc_desc is set to NULL.
> + */
> +static void of_irq_mark_orphan_desc(struct list_head *intc_desc_list)
> +{
> +	struct intc_desc *desc, *temp_desc;
> +
> +	list_for_each_entry_safe(desc, temp_desc, intc_desc_list, list) {
> +		struct intc_desc *td1, *td2;
> +		list_for_each_entry_safe(td1, td2, intc_desc_list, list) {
> +			if (desc->interrupt_parent == td1->dev)
> +				break;
> +		}
> +		if (desc->interrupt_parent == td1->dev)
> +			continue;
> +
> +		pr_debug("%s: set interrupt_parent of 'intc_desc' with dev name"
> +			" %s as NULL\n", __func__, desc->dev->full_name);
> +		desc->interrupt_parent = NULL;
> +		return;
> +	}
> +}
> +
> +/**
>   * of_irq_init - Scan and init matching interrupt controllers in DT
>   * @matches: 0 terminated array of nodes to match and init function to call
>   *
> @@ -481,8 +513,19 @@ void __init of_irq_init(const struct of_device_id *matches)
>  		/* Get the next pending parent that might have children */
>  		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
>  		if (list_empty(&intc_parent_list) || !desc) {
> +			/*
> +			 * This has reached a point where there are children in
> +			 * the intc_desc_list but no parent in intc_parent_list.
> +			 * This means there is a child desc in intc_desc_list
> +			 * whose parent is not one of the remaining elements of
> +			 * the intc_desc_list. Such a child node is marked as
> +			 * orphan (interrupt_parent is set to NULL) and the
> +			 * process continues with parent set to NULL.
> +			 */
>  			pr_err("of_irq_init: children remain, but no parents\n");
> -			break;
> +			of_irq_mark_orphan_desc(&intc_desc_list);
> +			parent = NULL;
> +			continue;
>  		}
>  		list_del(&desc->list);
>  		parent = desc->dev;

Re: [PATCH] of/irq: of_irq_init: Call initialization function for all controllers

[Thomas Abraham]

On 25 March 2012 20:50, Rob Herring <robherring2@gmail.com> wrote:
> On 03/25/2012 07:38 AM, Thomas Abraham wrote:
>> The of_irq_init function stops processing the interrupt controller hierarchy
>> when there are no more interrupt controller parents identified. Though this
>> condition suffices most cases, there are cases where a interrupt controller's
>> parent controller does not participate in the initialization of the interrupt
>> hierarchy. An example of such a case is the use of a interrupt nexus node
>> by a interrupt controller node which delivers interrupts to separate interrupt
>> parent controllers.
>>
>> Instead of stopping the processing of interrupt controller hierarchy in such
>> a case, the orphan interrupt controller node's descriptor can be identified
>> and its 'logical' parent in the descriptor is set as NULL. The processing of
>> interrupt hierarchy is then restarted by looking for descriptors which have
>> a NULL interrupt parent.
>>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>
> Wouldn't this accomplish the same thing? You just need to add the
> wakeup-map node name to your matches list.
>
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 9cf0060..deeaf00 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -416,8 +416,6 @@ void __init of_irq_init(const struct of_device_id
> *matches)
>        INIT_LIST_HEAD(&intc_parent_list);
>
>        for_each_matching_node(np, matches) {
> -               if (!of_find_property(np, "interrupt-controller", NULL))
> -                       continue;
>                /*
>                 * Here, we allocate and populate an intc_desc with the node
>                 * pointer, interrupt-parent device_node etc.
>

Hi Rob,

I tested with this, but the init function of wakeup controller is not
called. The following is the example nodes that I used for testing.

 gic:interrupt-controller@10490000 {
               compatible = "arm,cortex-a9-gic";
               #interrupt-cells = <3>;
               #address-cells = <0>;
               #size-cells = <0>;
               interrupt-controller;
               cpu-offset = <0x8000>;
               reg = <0x10490000 0x1000>, <0x10480000 0x100>;
       };

       combiner:interrupt-controller@10440000 {
               compatible = "samsung,exynos4210-combiner";
               #interrupt-cells = <2>;
               interrupt-controller;
               samsung,combiner-nr = <16>;
               reg = <0x10440000 0x1000>;
               interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
                            <0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
                            <0 8 0>, <0 9 0>, <0 10 0>, <0 11 0>,
                            <0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
       };

       wakeup_eint: interrupt-controller@11400000 {
               compatible = "samsung,exynos5210-wakeup-eint";
               reg = <0x11400000 0x1000>;
               interrupt-controller;
               #interrupt-cells = <2>;
               interrupt-parent = <&wakeup_map>;
               interrupts = <0x0 0>, <0x1 0>, <0x2 0>, <0x3 0>,
                            <0x4 0>, <0x5 0>, <0x6 0>, <0x7 0>,
                            <0x8 0>, <0x9 0>, <0xa 0>, <0xb 0>,
                            <0xc 0>, <0xd 0>, <0xe 0>, <0xf 0>,
                            <0x10 0>;

               wakeup_map: interrupt-map {
                       compatible = "samsung,exynos5210-wakeup-eint-map";
                       #interrupt-cells = <2>;
                       #address-cells = <0>;
                       #size-cells = <0>;
                       interrupt-map = <0x0 0 &gic 0 16 0>,
                                       <0x1 0 &gic 0 17 0>,
                                       <0x2 0 &gic 0 18 0>,
                                       <0x3 0 &gic 0 19 1>,
                                       <0x4 0 &gic 0 20 0>,
                                       <0x5 0 &gic 0 21 1>,
                                       <0x6 0 &gic 0 22 0>,
                                       <0x7 0 &gic 0 23 1>,
                                       <0x8 0 &gic 0 24 0>,
                                       <0x9 0 &gic 0 25 1>,
                                       <0xa 0 &gic 0 26 0>,
                                       <0xb 0 &gic 0 27 1>,
                                       <0xc 0 &gic 0 28 0>,
                                       <0xd 0 &gic 0 29 1>,
                                       <0xe 0 &gic 0 30 0>,
                                       <0xf 0 &gic 0 31 1>,
                                       <0x10 0 &combiner 2 4>;
               };
       };

And following is the match table used for testing.

static const struct of_device_id exynos4_dt_irq_match[] = {
       { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
       { .compatible = "samsung,exynos4210-combiner",
                       .data = combiner_of_init, },
       { .compatible = "samsung,exynos4210-wakeup-eint-map",
                       .data = exynos_init_irq_eint, },
       {},
};

The ' interrupt-map' map sub-node of 'interrupt-controller@11400000'
node does not have a interrupt-parent property. So it inherits it from
its parent node, which is 'interrupt-map' itself. So the parent of
wakeup-map is not gic or combiner and hence the initialization
function of wakeup controller is not called.

If a interrupt-parent property is added to 'interrupt-map' node (which
is probably not the right thing to do), and set the interrupt parent
as gic or combiner, there is a possibility that the interrupt-map is
initialized before the combiner (which is not correct since
interrupt-map uses combiner as one of its parents). But by placing
'wakeup_eint' node ahead of combiner node, this can be overcome but
relying on placement of nodes in dts file is not a reliable solution.

Thanks,
Thomas.

>
>>  drivers/of/irq.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 44 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 9cf0060..70c6ece 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -400,6 +400,38 @@ struct intc_desc {
>>  };
>>
>>  /**
>> + * of_irq_mark_orphan_desc - Set parent as NULL for a orphan intc_desc
>> + * @intc_desc_list: the list of intc_desc to search for orphan intc_desc
>> + *
>> + * This is a helper function for the of_irq_init function and is invoked
>> + * when there are child nodes available in intc_desc_list but there are
>> + * no parent nodes in intc_parent_list. When invoked, this function
>> + * searches for a intc_desc instance that does not have a parent intc_desc
>> + * instance in intc_desc_list. The very reason of the invocation of this
>> + * function ensures that a orphan intc_desc will be found. When found, the
>> + * interrupt_parent of the orphan intc_desc is set to NULL.
>> + */
>> +static void of_irq_mark_orphan_desc(struct list_head *intc_desc_list)
>> +{
>> +     struct intc_desc *desc, *temp_desc;
>> +
>> +     list_for_each_entry_safe(desc, temp_desc, intc_desc_list, list) {
>> +             struct intc_desc *td1, *td2;
>> +             list_for_each_entry_safe(td1, td2, intc_desc_list, list) {
>> +                     if (desc->interrupt_parent == td1->dev)
>> +                             break;
>> +             }
>> +             if (desc->interrupt_parent == td1->dev)
>> +                     continue;
>> +
>> +             pr_debug("%s: set interrupt_parent of 'intc_desc' with dev name"
>> +                     " %s as NULL\n", __func__, desc->dev->full_name);
>> +             desc->interrupt_parent = NULL;
>> +             return;
>> +     }
>> +}
>> +
>> +/**
>>   * of_irq_init - Scan and init matching interrupt controllers in DT
>>   * @matches: 0 terminated array of nodes to match and init function to call
>>   *
>> @@ -481,8 +513,19 @@ void __init of_irq_init(const struct of_device_id *matches)
>>               /* Get the next pending parent that might have children */
>>               desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
>>               if (list_empty(&intc_parent_list) || !desc) {
>> +                     /*
>> +                      * This has reached a point where there are children in
>> +                      * the intc_desc_list but no parent in intc_parent_list.
>> +                      * This means there is a child desc in intc_desc_list
>> +                      * whose parent is not one of the remaining elements of
>> +                      * the intc_desc_list. Such a child node is marked as
>> +                      * orphan (interrupt_parent is set to NULL) and the
>> +                      * process continues with parent set to NULL.
>> +                      */
>>                       pr_err("of_irq_init: children remain, but no parents\n");
>> -                     break;
>> +                     of_irq_mark_orphan_desc(&intc_desc_list);
>> +                     parent = NULL;
>> +                     continue;
>>               }
>>               list_del(&desc->list);
>>               parent = desc->dev;
>

Re: [PATCH] of/irq: of_irq_init: Call initialization function for all controllers

[Rob Herring]

On 03/25/2012 11:16 AM, Thomas Abraham wrote:
> On 25 March 2012 20:50, Rob Herring <robherring2@gmail.com> wrote:
>> On 03/25/2012 07:38 AM, Thomas Abraham wrote:
>>> The of_irq_init function stops processing the interrupt controller hierarchy
>>> when there are no more interrupt controller parents identified. Though this
>>> condition suffices most cases, there are cases where a interrupt controller's
>>> parent controller does not participate in the initialization of the interrupt
>>> hierarchy. An example of such a case is the use of a interrupt nexus node
>>> by a interrupt controller node which delivers interrupts to separate interrupt
>>> parent controllers.
>>>
>>> Instead of stopping the processing of interrupt controller hierarchy in such
>>> a case, the orphan interrupt controller node's descriptor can be identified
>>> and its 'logical' parent in the descriptor is set as NULL. The processing of
>>> interrupt hierarchy is then restarted by looking for descriptors which have
>>> a NULL interrupt parent.
>>>
>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>> Cc: Grant Likely <grant.likely@secretlab.ca>
>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>> ---
>>
>> Wouldn't this accomplish the same thing? You just need to add the
>> wakeup-map node name to your matches list.
>>
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 9cf0060..deeaf00 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -416,8 +416,6 @@ void __init of_irq_init(const struct of_device_id
>> *matches)
>>        INIT_LIST_HEAD(&intc_parent_list);
>>
>>        for_each_matching_node(np, matches) {
>> -               if (!of_find_property(np, "interrupt-controller", NULL))
>> -                       continue;
>>                /*
>>                 * Here, we allocate and populate an intc_desc with the node
>>                 * pointer, interrupt-parent device_node etc.
>>
> 
> Hi Rob,
> 
> I tested with this, but the init function of wakeup controller is not
> called. The following is the example nodes that I used for testing.
> 
>  gic:interrupt-controller@10490000 {
>                compatible = "arm,cortex-a9-gic";
>                #interrupt-cells = <3>;
>                #address-cells = <0>;
>                #size-cells = <0>;
>                interrupt-controller;
>                cpu-offset = <0x8000>;
>                reg = <0x10490000 0x1000>, <0x10480000 0x100>;
>        };
> 
>        combiner:interrupt-controller@10440000 {
>                compatible = "samsung,exynos4210-combiner";
>                #interrupt-cells = <2>;
>                interrupt-controller;
>                samsung,combiner-nr = <16>;
>                reg = <0x10440000 0x1000>;
>                interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
>                             <0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
>                             <0 8 0>, <0 9 0>, <0 10 0>, <0 11 0>,
>                             <0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
>        };
> 
>        wakeup_eint: interrupt-controller@11400000 {
>                compatible = "samsung,exynos5210-wakeup-eint";
>                reg = <0x11400000 0x1000>;
>                interrupt-controller;
>                #interrupt-cells = <2>;
>                interrupt-parent = <&wakeup_map>;
>                interrupts = <0x0 0>, <0x1 0>, <0x2 0>, <0x3 0>,
>                             <0x4 0>, <0x5 0>, <0x6 0>, <0x7 0>,
>                             <0x8 0>, <0x9 0>, <0xa 0>, <0xb 0>,
>                             <0xc 0>, <0xd 0>, <0xe 0>, <0xf 0>,
>                             <0x10 0>;
> 
>                wakeup_map: interrupt-map {
>                        compatible = "samsung,exynos5210-wakeup-eint-map";
>                        #interrupt-cells = <2>;
>                        #address-cells = <0>;
>                        #size-cells = <0>;
>                        interrupt-map = <0x0 0 &gic 0 16 0>,
>                                        <0x1 0 &gic 0 17 0>,
>                                        <0x2 0 &gic 0 18 0>,
>                                        <0x3 0 &gic 0 19 1>,
>                                        <0x4 0 &gic 0 20 0>,
>                                        <0x5 0 &gic 0 21 1>,
>                                        <0x6 0 &gic 0 22 0>,
>                                        <0x7 0 &gic 0 23 1>,
>                                        <0x8 0 &gic 0 24 0>,
>                                        <0x9 0 &gic 0 25 1>,
>                                        <0xa 0 &gic 0 26 0>,
>                                        <0xb 0 &gic 0 27 1>,
>                                        <0xc 0 &gic 0 28 0>,
>                                        <0xd 0 &gic 0 29 1>,
>                                        <0xe 0 &gic 0 30 0>,
>                                        <0xf 0 &gic 0 31 1>,
>                                        <0x10 0 &combiner 2 4>;
>                };
>        };
> 
> And following is the match table used for testing.
> 
> static const struct of_device_id exynos4_dt_irq_match[] = {
>        { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
>        { .compatible = "samsung,exynos4210-combiner",
>                        .data = combiner_of_init, },
>        { .compatible = "samsung,exynos4210-wakeup-eint-map",

Looks like you have a mismatch here: 5210 or 4210?


>                        .data = exynos_init_irq_eint, },
>        {},
> };
> 
> The ' interrupt-map' map sub-node of 'interrupt-controller@11400000'
> node does not have a interrupt-parent property. So it inherits it from
> its parent node, which is 'interrupt-map' itself. So the parent of
> wakeup-map is not gic or combiner and hence the initialization
> function of wakeup controller is not called.
> 

That should be fine. If a node's interrupt-parent is itself, then that's
treated as a root interrupt controller.

> If a interrupt-parent property is added to 'interrupt-map' node (which
> is probably not the right thing to do), and set the interrupt parent
> as gic or combiner, there is a possibility that the interrupt-map is
> initialized before the combiner (which is not correct since
> interrupt-map uses combiner as one of its parents). But by placing
> 'wakeup_eint' node ahead of combiner node, this can be overcome but
> relying on placement of nodes in dts file is not a reliable solution.

Your fix doesn't really guarantee the proper order either. It's still a
side effect of the implementation. Perhaps a retry mechanism would work.
Then the init for wakeup_eint can retry if the gic is not yet setup.

Rob


> Thanks,
> Thomas.
> 
>>
>>>  drivers/of/irq.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 files changed, 44 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>> index 9cf0060..70c6ece 100644
>>> --- a/drivers/of/irq.c
>>> +++ b/drivers/of/irq.c
>>> @@ -400,6 +400,38 @@ struct intc_desc {
>>>  };
>>>
>>>  /**
>>> + * of_irq_mark_orphan_desc - Set parent as NULL for a orphan intc_desc
>>> + * @intc_desc_list: the list of intc_desc to search for orphan intc_desc
>>> + *
>>> + * This is a helper function for the of_irq_init function and is invoked
>>> + * when there are child nodes available in intc_desc_list but there are
>>> + * no parent nodes in intc_parent_list. When invoked, this function
>>> + * searches for a intc_desc instance that does not have a parent intc_desc
>>> + * instance in intc_desc_list. The very reason of the invocation of this
>>> + * function ensures that a orphan intc_desc will be found. When found, the
>>> + * interrupt_parent of the orphan intc_desc is set to NULL.
>>> + */
>>> +static void of_irq_mark_orphan_desc(struct list_head *intc_desc_list)
>>> +{
>>> +     struct intc_desc *desc, *temp_desc;
>>> +
>>> +     list_for_each_entry_safe(desc, temp_desc, intc_desc_list, list) {
>>> +             struct intc_desc *td1, *td2;
>>> +             list_for_each_entry_safe(td1, td2, intc_desc_list, list) {
>>> +                     if (desc->interrupt_parent == td1->dev)
>>> +                             break;
>>> +             }
>>> +             if (desc->interrupt_parent == td1->dev)
>>> +                     continue;
>>> +
>>> +             pr_debug("%s: set interrupt_parent of 'intc_desc' with dev name"
>>> +                     " %s as NULL\n", __func__, desc->dev->full_name);
>>> +             desc->interrupt_parent = NULL;
>>> +             return;
>>> +     }
>>> +}
>>> +
>>> +/**
>>>   * of_irq_init - Scan and init matching interrupt controllers in DT
>>>   * @matches: 0 terminated array of nodes to match and init function to call
>>>   *
>>> @@ -481,8 +513,19 @@ void __init of_irq_init(const struct of_device_id *matches)
>>>               /* Get the next pending parent that might have children */
>>>               desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
>>>               if (list_empty(&intc_parent_list) || !desc) {
>>> +                     /*
>>> +                      * This has reached a point where there are children in
>>> +                      * the intc_desc_list but no parent in intc_parent_list.
>>> +                      * This means there is a child desc in intc_desc_list
>>> +                      * whose parent is not one of the remaining elements of
>>> +                      * the intc_desc_list. Such a child node is marked as
>>> +                      * orphan (interrupt_parent is set to NULL) and the
>>> +                      * process continues with parent set to NULL.
>>> +                      */
>>>                       pr_err("of_irq_init: children remain, but no parents\n");
>>> -                     break;
>>> +                     of_irq_mark_orphan_desc(&intc_desc_list);
>>> +                     parent = NULL;
>>> +                     continue;
>>>               }
>>>               list_del(&desc->list);
>>>               parent = desc->dev;
>>

Re: [PATCH] of/irq: of_irq_init: Call initialization function for all controllers

[Thomas Abraham]

On 26 March 2012 18:34, Rob Herring <robherring2@gmail.com> wrote:
> On 03/25/2012 11:16 AM, Thomas Abraham wrote:
>> On 25 March 2012 20:50, Rob Herring <robherring2@gmail.com> wrote:
>>> On 03/25/2012 07:38 AM, Thomas Abraham wrote:
>>>> The of_irq_init function stops processing the interrupt controller hierarchy
>>>> when there are no more interrupt controller parents identified. Though this
>>>> condition suffices most cases, there are cases where a interrupt controller's
>>>> parent controller does not participate in the initialization of the interrupt
>>>> hierarchy. An example of such a case is the use of a interrupt nexus node
>>>> by a interrupt controller node which delivers interrupts to separate interrupt
>>>> parent controllers.
>>>>
>>>> Instead of stopping the processing of interrupt controller hierarchy in such
>>>> a case, the orphan interrupt controller node's descriptor can be identified
>>>> and its 'logical' parent in the descriptor is set as NULL. The processing of
>>>> interrupt hierarchy is then restarted by looking for descriptors which have
>>>> a NULL interrupt parent.
>>>>
>>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>>> Cc: Grant Likely <grant.likely@secretlab.ca>
>>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>>> ---
>>>
>>> Wouldn't this accomplish the same thing? You just need to add the
>>> wakeup-map node name to your matches list.
>>>
>>>
>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>> index 9cf0060..deeaf00 100644
>>> --- a/drivers/of/irq.c
>>> +++ b/drivers/of/irq.c
>>> @@ -416,8 +416,6 @@ void __init of_irq_init(const struct of_device_id
>>> *matches)
>>>        INIT_LIST_HEAD(&intc_parent_list);
>>>
>>>        for_each_matching_node(np, matches) {
>>> -               if (!of_find_property(np, "interrupt-controller", NULL))
>>> -                       continue;
>>>                /*
>>>                 * Here, we allocate and populate an intc_desc with the node
>>>                 * pointer, interrupt-parent device_node etc.
>>>
>>
>> Hi Rob,
>>
>> I tested with this, but the init function of wakeup controller is not
>> called. The following is the example nodes that I used for testing.
>>
>>  gic:interrupt-controller@10490000 {
>>                compatible = "arm,cortex-a9-gic";
>>                #interrupt-cells = <3>;
>>                #address-cells = <0>;
>>                #size-cells = <0>;
>>                interrupt-controller;
>>                cpu-offset = <0x8000>;
>>                reg = <0x10490000 0x1000>, <0x10480000 0x100>;
>>        };
>>
>>        combiner:interrupt-controller@10440000 {
>>                compatible = "samsung,exynos4210-combiner";
>>                #interrupt-cells = <2>;
>>                interrupt-controller;
>>                samsung,combiner-nr = <16>;
>>                reg = <0x10440000 0x1000>;
>>                interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
>>                             <0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
>>                             <0 8 0>, <0 9 0>, <0 10 0>, <0 11 0>,
>>                             <0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
>>        };
>>
>>        wakeup_eint: interrupt-controller@11400000 {
>>                compatible = "samsung,exynos5210-wakeup-eint";
>>                reg = <0x11400000 0x1000>;
>>                interrupt-controller;
>>                #interrupt-cells = <2>;
>>                interrupt-parent = <&wakeup_map>;
>>                interrupts = <0x0 0>, <0x1 0>, <0x2 0>, <0x3 0>,
>>                             <0x4 0>, <0x5 0>, <0x6 0>, <0x7 0>,
>>                             <0x8 0>, <0x9 0>, <0xa 0>, <0xb 0>,
>>                             <0xc 0>, <0xd 0>, <0xe 0>, <0xf 0>,
>>                             <0x10 0>;
>>
>>                wakeup_map: interrupt-map {
>>                        compatible = "samsung,exynos5210-wakeup-eint-map";
>>                        #interrupt-cells = <2>;
>>                        #address-cells = <0>;
>>                        #size-cells = <0>;
>>                        interrupt-map = <0x0 0 &gic 0 16 0>,
>>                                        <0x1 0 &gic 0 17 0>,
>>                                        <0x2 0 &gic 0 18 0>,
>>                                        <0x3 0 &gic 0 19 1>,
>>                                        <0x4 0 &gic 0 20 0>,
>>                                        <0x5 0 &gic 0 21 1>,
>>                                        <0x6 0 &gic 0 22 0>,
>>                                        <0x7 0 &gic 0 23 1>,
>>                                        <0x8 0 &gic 0 24 0>,
>>                                        <0x9 0 &gic 0 25 1>,
>>                                        <0xa 0 &gic 0 26 0>,
>>                                        <0xb 0 &gic 0 27 1>,
>>                                        <0xc 0 &gic 0 28 0>,
>>                                        <0xd 0 &gic 0 29 1>,
>>                                        <0xe 0 &gic 0 30 0>,
>>                                        <0xf 0 &gic 0 31 1>,
>>                                        <0x10 0 &combiner 2 4>;
>>                };
>>        };
>>
>> And following is the match table used for testing.
>>
>> static const struct of_device_id exynos4_dt_irq_match[] = {
>>        { .compatible = "arm,cortex-a9-gic", .data = gic_of_init, },
>>        { .compatible = "samsung,exynos4210-combiner",
>>                        .data = combiner_of_init, },
>>        { .compatible = "samsung,exynos4210-wakeup-eint-map",
>
> Looks like you have a mismatch here: 5210 or 4210?

Sorry, that was a typo. I checked the code again. It is 4210.

>
>
>>                        .data = exynos_init_irq_eint, },
>>        {},
>> };
>>
>> The ' interrupt-map' map sub-node of 'interrupt-controller@11400000'
>> node does not have a interrupt-parent property. So it inherits it from
>> its parent node, which is 'interrupt-map' itself. So the parent of
>> wakeup-map is not gic or combiner and hence the initialization
>> function of wakeup controller is not called.
>>
>
> That should be fine. If a node's interrupt-parent is itself, then that's
> treated as a root interrupt controller.

I checked this again and found that of_irq_find_parent() function when
called with pointer to "interrupt-map" node as the parameter, does not
return "interrupt-map" as the parent in the example nodes listed
above. This might be wrong and may be caused due to the check

of_get_property(p, "#interrupt-cells", NULL) == NULL

in the do..while loop in of_irq_find_parent() function. So this
explains why the 'interrupt-map' was not treated as the root interrupt
controller.

>
>> If a interrupt-parent property is added to 'interrupt-map' node (which
>> is probably not the right thing to do), and set the interrupt parent
>> as gic or combiner, there is a possibility that the interrupt-map is
>> initialized before the combiner (which is not correct since
>> interrupt-map uses combiner as one of its parents). But by placing
>> 'wakeup_eint' node ahead of combiner node, this can be overcome but
>> relying on placement of nodes in dts file is not a reliable solution.
>
> Your fix doesn't really guarantee the proper order either. It's still a
> side effect of the implementation. Perhaps a retry mechanism would work.
> Then the init for wakeup_eint can retry if the gic is not yet setup.

Ok. A retry mechanism would be most robust solution for this case.

Thanks,
Thomas.

[...]

Re: [PATCH] of/irq: of_irq_init: Call initialization function for all controllers

[Thomas Abraham]

On 26 March 2012 21:06, Thomas Abraham <thomas.abraham@linaro.org> wrote:
> On 26 March 2012 18:34, Rob Herring <robherring2@gmail.com> wrote:

>> Your fix doesn't really guarantee the proper order either. It's still a
>> side effect of the implementation. Perhaps a retry mechanism would work.
>> Then the init for wakeup_eint can retry if the gic is not yet setup.
>
> Ok. A retry mechanism would be most robust solution for this case.
>

Hi Rob,

I have attempted an implementation of the the retry mechanism in the
of_irq_init() function. It allows initialization functions of
interrupt controller nodes to report -EAGAIN indicating that the
initialization was not complete and should be retried.

I have tested this and it is working fine. But the complexity of the
interrupt controller's intialization function increases because it has
to undo some of the intialization steps it completed before finding
that the its parent controller was not initialized.

The following is the diff of the changes for of_irq_init() function.
Please let know if this is acceptable.

Thanks,
Thomas.

@@ -452,7 +454,6 @@ void __init of_irq_init(const struct of_device_id *matches)
 			if (desc->interrupt_parent != parent)
 				continue;

-			list_del(&desc->list);
 			match = of_match_node(matches, desc->dev);
 			if (WARN(!match->data,
 			    "of_irq_init: no init function for %s\n",
@@ -466,6 +467,14 @@ void __init of_irq_init(const struct of_device_id *matches)
 				 desc->dev, desc->interrupt_parent);
 			irq_init_cb = match->data;
 			ret = irq_init_cb(desc->dev, desc->interrupt_parent);
+			if (ret == -EAGAIN)
+				/*
+				 * Interrupt controller's initialization did not
+				 * complete and should be retried. So let its
+				 * intc_desc be on intc_desc_list.
+				 */
+				continue;
+			list_del(&desc->list);
 			if (ret) {
 				kfree(desc);
 				continue;
@@ -482,7 +491,18 @@ void __init of_irq_init(const struct of_device_id *matches)
 		desc = list_first_entry(&intc_parent_list, typeof(*desc), list);
 		if (list_empty(&intc_parent_list) || !desc) {
 			pr_err("of_irq_init: children remain, but no parents\n");
-			break;
+			/*
+			 * If a search with NULL as parent did not result in any
+			 * new parent being found, then the scan for matching
+			 * interrupt controller nodes is considered as complete.
+			 * Otherwise, if there are pending elements on the
+			 * intc_desc_list, then retry this process again with
+			 * NULL as parent.
+			 */
+			if (!parent)
+				break;
+			parent = NULL;
+			continue;
 		}
 		list_del(&desc->list);
 		parent = desc->dev;

Re: Device node for a controller with two interrupt parents

[David Gibson]

On Wed, Mar 21, 2012 at 07:05:26PM +0530, Thomas Abraham wrote:
> Hi David,
> 
> On 21 March 2012 09:11, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Wed, Mar 21, 2012 at 07:55:43AM +0530, Thomas Abraham wrote:
> >> Hi,
> >>
> >> Exynos5 includes a gpio wakeup interrupt controller that generates 32
> >> interrupts. The first 16 interrupts are routed to the interrupt
> >> combiner controller. The last 16 are muxed into one interrupt and this
> >> interrupt line is connected to the GIC interrupt controller.
> >>
> >> So, the wakeup interrupt controller node in device tree requires two
> >> interrupt parents. I do not know how to handle this. Any suggestions
> >> will be very helpful.
> >
> > This has occurred before, for example on the MAL device on 440EP (see
> > the bamboo board dts for example).  The semi-standard approach is to
> > make the node an interrupt-nexus for itself.  That is in the node's
> > interrupts property, just list 0..N giving as many interrupts as you
> > need.  Set the node's interrupt-parent to point to the node itself,
> > then add interrupt-map and interrupt-map-mask properties which remap
> > those interrupts 0..N to the correct interrupts on the actual
> > interrupt controllers.  Each entry in the interrupt map specifies an
> > interrupt parent phandle, so you can distribute the irqs to multiple
> > interrupt controllers that way.
> 
> Thanks for your suggestion and pointing out an example. I tried this
> approach for Exynos4 and Exynos5. It mostly works but there are two
> issues here.
> 
> 1. In the Exynos5 case, the wakeup interrupt controller (which has two
> separate interrupt parents - gic and combiner) is itself a interrupt
> controller and has the 'interrupt-controller' property. So
> of_irq_map_raw() function does not process the interrupt-map in the
> wakeup interrupt controller device node. I did the following change to
> get past this but I am not sure if this the correct thing to do.

That might work, but it obviously won't help you with existing
kernels.  I think a better idea is not to try to make the
interrupt-controller and interrupt-nexus the same node in this case.
Instead add an intermediate nexus node to remap the
interrupt-controller's output interrupts into it's various parents.
You could make this fake nexus node a subnode of the interrupt
controller itself.

It's a bit of a hack, but it should work with existing parsing code,
and I think it's better than inventing a whole new convention to cover
this case.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson