Re: [PATCH 10/20] of/irq: fix interrupt parent lookup procedure

[Thomas Abraham <thomas.abraham@xxxxxxxxxx>]

On 16 May 2012 00:11, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> On Tue, 15 May 2012 17:29:14 +0900, Kukjin Kim <kgene.kim@xxxxxxxxxxx> wrote:
>> Thomas Abraham wrote:
>> >
>> > The interrupt parent lookup fails for a node that is a interrupt-controller
>> > but does not have an explict interrupt-parent property and instead inherits
>> > this property from the root node.
>> >
>> > Consider the nodes listed below.
>> >
>> >     / {
>> >         interrupt-parent = <&intc_level1>;
>> >
>> >         intc_level1: interrupt-controller@xxx {
>> >                 interrupt-controller;
>> >                 #interrupt-cells = <3>;
>> >                 <rest of the properties here>;
>> >         };
>> >
>> >         intc_level2: interrupt-controller@yyy {
>> >                 interrupt-controller;
>> >                 #interrupt-cells = <2>;
>> >                 <rest of the properties here>;
>> >         };
>> >     };
>> >
>> > The interrupt parent lookup for interrupt-controller@yyy fails. It inherits
>> > the interrupt-parent property from the root node and the root node ('/')
>> > specifies a 'interrupt-parent' property which represents the default interrupt
>> > root controller. But, the property '#interrupt-cells' might not be specified
>> > in the root node.
>> >
>> > In case a interrupt controller node does not include a 'interrupt-parent'
>> > property but inherits that property from the root node, the check for
>> > 'interrupt-cells' property in the root node fails. Fix this removing the
>> > check for 'interrupt-cells' property.
>
> Hmmm... I dont see the bug...  From your description above, I see the
> following sequence.
>
> First iteration:
>        child = &ic@yyy;
>        cannot find "interrupt-parent" so take 'if' clause
>                p = of_get_parent(child);  (root)
>        iteration continues because #interrupt-cells not found in 'p'(root)
> Second iteration:
>        child = root;
>        found "interrupt-parent", so take 'else' clause
>                p = of_find_node_by_phandle();  (ic@xxx)
>        iteration stops because #interrupt-cells is found in 'p'(ic@xxx)
>
> What am I missing in my admittedly short look at the code?

Grant, sorry for the bad commit message for the patch. I got it
completely wrong. I intended to generalize a change which was need for
a using interrupt-map on Exynos5 for wakeup interrupts, and in process
I wrote down a incorrect commit message.

I will list out the problem I was trying to solve. The wakeup
interrupt controller on Exynos5 has two parents - GIC and Interrupt
Combiner. The node for the Exynos5 wakeup interrupt is listed below.

        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 &combiner 23 0>,
                                        <0x1 0 &combiner 24 0>,
                                        <0x2 0 &combiner 25 0>,
                                        <0x3 0 &combiner 25 1>,
                                        <0x4 0 &combiner 26 0>,
                                        <0x5 0 &combiner 26 1>,
                                        <0x6 0 &combiner 27 0>,
                                        <0x7 0 &combiner 27 1>,
                                        <0x8 0 &combiner 28 0>,
                                        <0x9 0 &combiner 28 1>,
                                        <0xa 0 &combiner 29 0>,
                                        <0xb 0 &combiner 29 1>,
                                        <0xc 0 &combiner 30 0>,
                                        <0xd 0 &combiner 30 1>,
                                        <0xe 0 &combiner 31 0>,
                                        <0xf 0 &combiner 31 1>,
                                        <0x10 0 &gic 0 32 0>;
                };
        };

The match table for of_irq_init() function is listed below (the third
in this list is for wakeup interrupt controller).

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,exynos5210-wakeup-eint-map",
                        .data = exynos_init_irq_eint, },
        {},
};

Without this patch, of_irq_init() finds that the interrupt parent of
"interrupt-map" node is "interrupt-controller@11400000" node. Which is
not correct. The interrupt parent of "interrupt-map" node should be
itself.

I feel that the check for "#interrupt-cells" property in do..while()
loop in of_irq_find_parent() function is not required. The presence of
"#interrupt-cells" property in the parent node should not be
considered as a necessary condition for the parent node to be
considered the interrupt-parent of the child node.

Thanks for reviewing this patch. Sorry for the delay in responding to
your email.

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