[PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Stephen Warren]

From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Convert tegra20 IRQ intialization to the GIC devicetree binding. Modify the
interrupt definitions in the dts files according to
Documentation/devicetree/bindings/arm/gic.txt

v2 (swarren):
* Removed some stale GIC init code from board-dt.c
* Undid some accidental 0x -> 0x0 search/replace.

Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra-paz00.dts |    2 +-
 arch/arm/boot/dts/tegra20.dtsi    |   48 ++++++++++++++++++++----------------
 arch/arm/mach-tegra/board-dt.c    |   11 --------
 arch/arm/mach-tegra/irq.c         |   18 ++++++++++++-
 4 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-paz00.dts b/arch/arm/boot/dts/tegra-paz00.dts
index 4f6a8aa..1a1d702 100644
--- a/arch/arm/boot/dts/tegra-paz00.dts
+++ b/arch/arm/boot/dts/tegra-paz00.dts
@@ -27,7 +27,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,nvec";
 		reg = <0x7000C500 0x100>;
-		interrupts = <124>;
+		interrupts = <0 92 0x04>;
 		clock-frequency = <80000>;
 		request-gpios = <&gpio 170 0>;
 		slave-addr = <138>;
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 795b921..db6f562 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -5,9 +5,9 @@
 	interrupt-parent = <&intc>;
 
 	intc: interrupt-controller@50041000 {
-		compatible = "nvidia,tegra20-gic";
+		compatible = "arm,cortex-a9-gic";
 		interrupt-controller;
-		#interrupt-cells = <1>;
+		#interrupt-cells = <3>;
 		reg = < 0x50041000 0x1000 >,
 		      < 0x50040100 0x0100 >;
 	};
@@ -17,7 +17,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,tegra20-i2c";
 		reg = <0x7000C000 0x100>;
-		interrupts = < 70 >;
+		interrupts = < 0 38 0x04 >;
 	};
 
 	i2c@7000c400 {
@@ -25,7 +25,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,tegra20-i2c";
 		reg = <0x7000C400 0x100>;
-		interrupts = < 116 >;
+		interrupts = < 0 84 0x04 >;
 	};
 
 	i2c@7000c500 {
@@ -33,7 +33,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,tegra20-i2c";
 		reg = <0x7000C500 0x100>;
-		interrupts = < 124 >;
+		interrupts = < 0 92 0x04 >;
 	};
 
 	i2c@7000d000 {
@@ -41,7 +41,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,tegra20-i2c";
 		reg = <0x7000D000 0x200>;
-		interrupts = < 85 >;
+		interrupts = < 0 53 0x04 >;
 	};
 
 	i2s@70002800 {
@@ -49,7 +49,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,tegra20-i2s";
 		reg = <0x70002800 0x200>;
-		interrupts = < 45 >;
+		interrupts = < 0 13 0x04 >;
 		dma-channel = < 2 >;
 	};
 
@@ -58,7 +58,7 @@
 		#size-cells = <0>;
 		compatible = "nvidia,tegra20-i2s";
 		reg = <0x70002a00 0x200>;
-		interrupts = < 35 >;
+		interrupts = < 0 3 0x04 >;
 		dma-channel = < 1 >;
 	};
 
@@ -72,7 +72,13 @@
 	gpio: gpio@6000d000 {
 		compatible = "nvidia,tegra20-gpio";
 		reg = < 0x6000d000 0x1000 >;
-		interrupts = < 64 65 66 67 87 119 121 >;
+		interrupts = < 0 32 0x04
+			       0 33 0x04
+			       0 34 0x04
+			       0 35 0x04
+			       0 55 0x04
+			       0 87 0x04
+			       0 89 0x04 >;
 		#gpio-cells = <2>;
 		gpio-controller;
 	};
@@ -89,79 +95,79 @@
 		compatible = "nvidia,tegra20-uart";
 		reg = <0x70006000 0x40>;
 		reg-shift = <2>;
-		interrupts = < 68 >;
+		interrupts = < 0 36 0x04 >;
 	};
 
 	serial@70006040 {
 		compatible = "nvidia,tegra20-uart";
 		reg = <0x70006040 0x40>;
 		reg-shift = <2>;
-		interrupts = < 69 >;
+		interrupts = < 0 37 0x04 >;
 	};
 
 	serial@70006200 {
 		compatible = "nvidia,tegra20-uart";
 		reg = <0x70006200 0x100>;
 		reg-shift = <2>;
-		interrupts = < 78 >;
+		interrupts = < 0 46 0x04 >;
 	};
 
 	serial@70006300 {
 		compatible = "nvidia,tegra20-uart";
 		reg = <0x70006300 0x100>;
 		reg-shift = <2>;
-		interrupts = < 122 >;
+		interrupts = < 0 90 0x04 >;
 	};
 
 	serial@70006400 {
 		compatible = "nvidia,tegra20-uart";
 		reg = <0x70006400 0x100>;
 		reg-shift = <2>;
-		interrupts = < 123 >;
+		interrupts = < 0 91 0x04 >;
 	};
 
 	sdhci@c8000000 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000000 0x200>;
-		interrupts = < 46 >;
+		interrupts = < 0 14 0x04 >;
 	};
 
 	sdhci@c8000200 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000200 0x200>;
-		interrupts = < 47 >;
+		interrupts = < 0 15 0x04 >;
 	};
 
 	sdhci@c8000400 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000400 0x200>;
-		interrupts = < 51 >;
+		interrupts = < 0 19 0x04 >;
 	};
 
 	sdhci@c8000600 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000600 0x200>;
-		interrupts = < 63 >;
+		interrupts = < 0 31 0x04 >;
 	};
 
 	usb@c5000000 {
 		compatible = "nvidia,tegra20-ehci", "usb-ehci";
 		reg = <0xc5000000 0x4000>;
-		interrupts = < 52 >;
+		interrupts = < 0 20 0x04 >;
 		phy_type = "utmi";
 	};
 
 	usb@c5004000 {
 		compatible = "nvidia,tegra20-ehci", "usb-ehci";
 		reg = <0xc5004000 0x4000>;
-		interrupts = < 53 >;
+		interrupts = < 0 21 0x04 >;
 		phy_type = "ulpi";
 	};
 
 	usb@c5008000 {
 		compatible = "nvidia,tegra20-ehci", "usb-ehci";
 		reg = <0xc5008000 0x4000>;
-		interrupts = < 129 >;
+		interrupts = < 0 97 0x04 >;
 		phy_type = "utmi";
 	};
 };
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 6e35417..bcbb677 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -87,11 +87,6 @@ static struct of_device_id tegra_dt_match_table[] __initdata = {
 	{}
 };
 
-static struct of_device_id tegra_dt_gic_match[] __initdata = {
-	{ .compatible = "nvidia,tegra20-gic", },
-	{}
-};
-
 static struct {
 	char *machine;
 	void (*init)(void);
@@ -105,14 +100,8 @@ static struct {
 
 static void __init tegra_dt_init(void)
 {
-	struct device_node *node;
 	int i;
 
-	node = of_find_matching_node_by_address(NULL, tegra_dt_gic_match,
-						TEGRA_ARM_INT_DIST_BASE);
-	if (node)
-		irq_domain_add_simple(node, INT_GIC_BASE);
-
 	tegra_clk_init_from_table(tegra_dt_clk_init_table);
 
 	/*
diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index 8ad82af..c6d02a4 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -21,6 +21,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/io.h>
+#include <linux/of_irq.h>
 
 #include <asm/hardware/gic.h>
 
@@ -109,6 +110,11 @@ static int tegra_retrigger(struct irq_data *d)
 	return 1;
 }
 
+static const struct of_device_id tegra_irq_match[] __initconst = {
+	{ .compatible = "arm,cortex-a9-gic", .data = gic_of_init },
+	{ /* sentinel */ }
+};
+
 void __init tegra_init_irq(void)
 {
 	int i;
@@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
 	gic_arch_extn.irq_unmask = tegra_unmask;
 	gic_arch_extn.irq_retrigger = tegra_retrigger;
 
-	gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
-		 IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
+#ifdef CONFIG_OF
+	/* Check if there is a devicetree present as of_irq_init doesn't
+	 * indicate if an interrupt controller node was found.
+	 */
+	if (of_find_node_by_path("/"))
+		of_irq_init(tegra_irq_match);
+	else
+#endif
+		gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
+			IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
 }
-- 
1.7.0.4

Re: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Cousson, Benoit]

Hi Stephen & Peter,

On 11/29/2011 1:54 AM, Stephen Warren wrote:
> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[...]

> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
>   	gic_arch_extn.irq_unmask = tegra_unmask;
>   	gic_arch_extn.irq_retrigger = tegra_retrigger;
>
> -	gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
> -		 IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
> +#ifdef CONFIG_OF
> +	/* Check if there is a devicetree present as of_irq_init doesn't
> +	 * indicate if an interrupt controller node was found.
> +	 */
> +	if (of_find_node_by_path("/"))
> +		of_irq_init(tegra_irq_match);
> +	else
> +#endif

For the same kind of need, I found the following API:

of_have_populated_dt()

Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.

Regards,
Benoit

RE: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Stephen Warren]

Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
> Hi Stephen & Peter,
> 
> On 11/29/2011 1:54 AM, Stephen Warren wrote:
> > From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> [...]
> 
> > @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
> >   	gic_arch_extn.irq_unmask = tegra_unmask;
> >   	gic_arch_extn.irq_retrigger = tegra_retrigger;
> >
> > -	gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
> > -		 IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
> > +#ifdef CONFIG_OF
> > +	/* Check if there is a devicetree present as of_irq_init doesn't
> > +	 * indicate if an interrupt controller node was found.
> > +	 */
> > +	if (of_find_node_by_path("/"))
> > +		of_irq_init(tegra_irq_match);
> > +	else
> > +#endif
> 
> For the same kind of need, I found the following API:
> 
> of_have_populated_dt()
> 
> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.

That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
function. Exynos has the following in mainline to solve this:

        if (!of_have_populated_dt())
                gic_init_bases(...);
#ifdef CONFIG_OF
        else
                of_irq_init(exynos4_dt_irq_match);
#endif

Perhaps we should add a dummy of_irq_init() too, so that we can remove
this ifdef.

-- 
nvpublic

Re: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Nicolas Ferre]

On 11/29/2011 06:13 PM, Stephen Warren :
> Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
>> Hi Stephen&  Peter,
>>
>> On 11/29/2011 1:54 AM, Stephen Warren wrote:
>>> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> [...]
>>
>>> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
>>>    	gic_arch_extn.irq_unmask = tegra_unmask;
>>>    	gic_arch_extn.irq_retrigger = tegra_retrigger;
>>>
>>> -	gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
>>> -		 IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
>>> +#ifdef CONFIG_OF
>>> +	/* Check if there is a devicetree present as of_irq_init doesn't
>>> +	 * indicate if an interrupt controller node was found.
>>> +	 */
>>> +	if (of_find_node_by_path("/"))
>>> +		of_irq_init(tegra_irq_match);
>>> +	else
>>> +#endif
>>
>> For the same kind of need, I found the following API:
>>
>> of_have_populated_dt()
>>
>> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.
>
> That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
> function. Exynos has the following in mainline to solve this:
>
>          if (!of_have_populated_dt())
>                  gic_init_bases(...);
> #ifdef CONFIG_OF
>          else
>                  of_irq_init(exynos4_dt_irq_match);
> #endif
>
> Perhaps we should add a dummy of_irq_init() too, so that we can remove
> this ifdef.

+1 for this idea: I have the same issue with at91.
I think that a macro is preferred instead a function: it will also 
remove the need for a protection for the matching table.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Cousson, Benoit]

On 11/29/2011 6:13 PM, Stephen Warren wrote:
> Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
>> Hi Stephen&  Peter,
>>
>> On 11/29/2011 1:54 AM, Stephen Warren wrote:
>>> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> [...]
>>
>>> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
>>>    	gic_arch_extn.irq_unmask = tegra_unmask;
>>>    	gic_arch_extn.irq_retrigger = tegra_retrigger;
>>>
>>> -	gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
>>> -		 IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
>>> +#ifdef CONFIG_OF
>>> +	/* Check if there is a devicetree present as of_irq_init doesn't
>>> +	 * indicate if an interrupt controller node was found.
>>> +	 */
>>> +	if (of_find_node_by_path("/"))
>>> +		of_irq_init(tegra_irq_match);
>>> +	else
>>> +#endif
>>
>> For the same kind of need, I found the following API:
>>
>> of_have_populated_dt()
>>
>> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.
>
> That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
> function. Exynos has the following in mainline to solve this:
>
>          if (!of_have_populated_dt())
>                  gic_init_bases(...);
> #ifdef CONFIG_OF
>          else
>                  of_irq_init(exynos4_dt_irq_match);
> #endif
>
> Perhaps we should add a dummy of_irq_init() too, so that we can remove
> this ifdef.

Yes, indeed, I think that's much better. Most of_XXX APIs do have a 
dummy version, but for some reason some of them do not.
That one clearly deserve a dummy version.

Regards,
Benoit

Re: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Rob Herring]

On 11/29/2011 11:47 AM, Cousson, Benoit wrote:
> On 11/29/2011 6:13 PM, Stephen Warren wrote:
>> Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
>>> Hi Stephen&  Peter,
>>>
>>> On 11/29/2011 1:54 AM, Stephen Warren wrote:
>>>> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> [...]
>>>
>>>> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
>>>>        gic_arch_extn.irq_unmask = tegra_unmask;
>>>>        gic_arch_extn.irq_retrigger = tegra_retrigger;
>>>>
>>>> -    gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
>>>> -         IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
>>>> +#ifdef CONFIG_OF
>>>> +    /* Check if there is a devicetree present as of_irq_init doesn't
>>>> +     * indicate if an interrupt controller node was found.
>>>> +     */
>>>> +    if (of_find_node_by_path("/"))
>>>> +        of_irq_init(tegra_irq_match);
>>>> +    else
>>>> +#endif
>>>
>>> For the same kind of need, I found the following API:
>>>
>>> of_have_populated_dt()
>>>
>>> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.
>>
>> That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
>> function. Exynos has the following in mainline to solve this:
>>
>>          if (!of_have_populated_dt())
>>                  gic_init_bases(...);
>> #ifdef CONFIG_OF
>>          else
>>                  of_irq_init(exynos4_dt_irq_match);
>> #endif
>>
>> Perhaps we should add a dummy of_irq_init() too, so that we can remove
>> this ifdef.
> 
> Yes, indeed, I think that's much better. Most of_XXX APIs do have a
> dummy version, but for some reason some of them do not.
> That one clearly deserve a dummy version.

I had an empty version originally and removed it based on Grant's review...

The original intent was this call should be in a DT board file and
therefore always enabled. Perhaps you can split tegra_init_irq into 2
functions with one having all but gic_init and then call that function
from board-dt.c along with of_irq_init.

Rob

RE: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Stephen Warren]

Rob Herring wrote at Tuesday, November 29, 2011 12:52 PM:
> On 11/29/2011 11:47 AM, Cousson, Benoit wrote:
> > On 11/29/2011 6:13 PM, Stephen Warren wrote:
> >> Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
> >>> Hi Stephen&  Peter,
> >>>
> >>> On 11/29/2011 1:54 AM, Stephen Warren wrote:
> >>>> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>>
> >>> [...]
> >>>
> >>>> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
> >>>>        gic_arch_extn.irq_unmask = tegra_unmask;
> >>>>        gic_arch_extn.irq_retrigger = tegra_retrigger;
> >>>>
> >>>> -    gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
> >>>> -         IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
> >>>> +#ifdef CONFIG_OF
> >>>> +    /* Check if there is a devicetree present as of_irq_init doesn't
> >>>> +     * indicate if an interrupt controller node was found.
> >>>> +     */
> >>>> +    if (of_find_node_by_path("/"))
> >>>> +        of_irq_init(tegra_irq_match);
> >>>> +    else
> >>>> +#endif
> >>>
> >>> For the same kind of need, I found the following API:
> >>>
> >>> of_have_populated_dt()
> >>>
> >>> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.
> >>
> >> That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
> >> function. Exynos has the following in mainline to solve this:
> >>
> >>          if (!of_have_populated_dt())
> >>                  gic_init_bases(...);
> >> #ifdef CONFIG_OF
> >>          else
> >>                  of_irq_init(exynos4_dt_irq_match);
> >> #endif
> >>
> >> Perhaps we should add a dummy of_irq_init() too, so that we can remove
> >> this ifdef.
> >
> > Yes, indeed, I think that's much better. Most of_XXX APIs do have a
> > dummy version, but for some reason some of them do not.
> > That one clearly deserve a dummy version.
> 
> I had an empty version originally and removed it based on Grant's review...
> 
> The original intent was this call should be in a DT board file and
> therefore always enabled. Perhaps you can split tegra_init_irq into 2
> functions with one having all but gic_init and then call that function
> from board-dt.c along with of_irq_init.

Well, that's certainly possible, but it seems a little inconsistent to
initialize the GIC from tegra_init_irq for non-DT, yet do it somewhere
else in the DT case. It seems like putting all the GIC stuff into
tegra_irq_init is the right thing to do. I note that Exynos works this
way already.

I couldn't find Grant's email to see his rationale for not wanting an
empty or_irq_init(); do you have a link?

Thanks.

-- 
nvpublic

Re: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Rob Herring]

On 11/29/2011 03:08 PM, Stephen Warren wrote:
> Rob Herring wrote at Tuesday, November 29, 2011 12:52 PM:
>> On 11/29/2011 11:47 AM, Cousson, Benoit wrote:
>>> On 11/29/2011 6:13 PM, Stephen Warren wrote:
>>>> Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
>>>>> Hi Stephen&  Peter,
>>>>>
>>>>> On 11/29/2011 1:54 AM, Stephen Warren wrote:
>>>>>> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
>>>>>>        gic_arch_extn.irq_unmask = tegra_unmask;
>>>>>>        gic_arch_extn.irq_retrigger = tegra_retrigger;
>>>>>>
>>>>>> -    gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
>>>>>> -         IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
>>>>>> +#ifdef CONFIG_OF
>>>>>> +    /* Check if there is a devicetree present as of_irq_init doesn't
>>>>>> +     * indicate if an interrupt controller node was found.
>>>>>> +     */
>>>>>> +    if (of_find_node_by_path("/"))
>>>>>> +        of_irq_init(tegra_irq_match);
>>>>>> +    else
>>>>>> +#endif
>>>>>
>>>>> For the same kind of need, I found the following API:
>>>>>
>>>>> of_have_populated_dt()
>>>>>
>>>>> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.
>>>>
>>>> That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
>>>> function. Exynos has the following in mainline to solve this:
>>>>
>>>>          if (!of_have_populated_dt())
>>>>                  gic_init_bases(...);
>>>> #ifdef CONFIG_OF
>>>>          else
>>>>                  of_irq_init(exynos4_dt_irq_match);
>>>> #endif
>>>>
>>>> Perhaps we should add a dummy of_irq_init() too, so that we can remove
>>>> this ifdef.
>>>
>>> Yes, indeed, I think that's much better. Most of_XXX APIs do have a
>>> dummy version, but for some reason some of them do not.
>>> That one clearly deserve a dummy version.
>>
>> I had an empty version originally and removed it based on Grant's review...
>>
>> The original intent was this call should be in a DT board file and
>> therefore always enabled. Perhaps you can split tegra_init_irq into 2
>> functions with one having all but gic_init and then call that function
>> from board-dt.c along with of_irq_init.
> 
> Well, that's certainly possible, but it seems a little inconsistent to
> initialize the GIC from tegra_init_irq for non-DT, yet do it somewhere
> else in the DT case. It seems like putting all the GIC stuff into
> tegra_irq_init is the right thing to do. I note that Exynos works this
> way already.
> 
> I couldn't find Grant's email to see his rationale for not wanting an
> empty or_irq_init(); do you have a link?
> 

My brain is rotting... I think it was gic_of_init that I was thinking of
but still can't find the email.

Will your code even link currently with !CONFIG_OF. I would think it
cannot resolve gic_of_init in that case.

I have a lot of concerns that supporting both CONFIG_OF and !CONFIG_OF
is going to be a pain as it's yet another variable to compile against.
In order to actually start reducing reducing the size of the ARM
platform code (that is the goal, right?), platforms need to be always OF
enabled and start removing the !CONFIG_OF code. So why not always turn
on CONFIG_OF for Tegra and not worry about this? Eventually, CONFIG_OF
will always be enabled anyway.

Rob

RE: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Stephen Warren]

Rob Herring wrote at Tuesday, November 29, 2011 3:25 PM:
> On 11/29/2011 03:08 PM, Stephen Warren wrote:
> > Rob Herring wrote at Tuesday, November 29, 2011 12:52 PM:
> >> On 11/29/2011 11:47 AM, Cousson, Benoit wrote:
> >>> On 11/29/2011 6:13 PM, Stephen Warren wrote:
> >>>> Cousson, Benoit wrote at Tuesday, November 29, 2011 6:01 AM:
> >>>>> Hi Stephen&  Peter,
> >>>>>
> >>>>> On 11/29/2011 1:54 AM, Stephen Warren wrote:
> >>>>>> From: pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> @@ -125,6 +131,14 @@ void __init tegra_init_irq(void)
> >>>>>>        gic_arch_extn.irq_unmask = tegra_unmask;
> >>>>>>        gic_arch_extn.irq_retrigger = tegra_retrigger;
> >>>>>>
> >>>>>> -    gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
> >>>>>> -         IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
> >>>>>> +#ifdef CONFIG_OF
> >>>>>> +    /* Check if there is a devicetree present as of_irq_init doesn't
> >>>>>> +     * indicate if an interrupt controller node was found.
> >>>>>> +     */
> >>>>>> +    if (of_find_node_by_path("/"))
> >>>>>> +        of_irq_init(tegra_irq_match);
> >>>>>> +    else
> >>>>>> +#endif
> >>>>>
> >>>>> For the same kind of need, I found the following API:
> >>>>>
> >>>>> of_have_populated_dt()
> >>>>>
> >>>>> Moreover, it returns false if !CONFIG_OF, so it will avoid the #ifdef.
> >>>>
> >>>> That sounds like a great idea. Unfortunately, of_irq_init() is a DT-only
> >>>> function. Exynos has the following in mainline to solve this:
> >>>>
> >>>>          if (!of_have_populated_dt())
> >>>>                  gic_init_bases(...);
> >>>> #ifdef CONFIG_OF
> >>>>          else
> >>>>                  of_irq_init(exynos4_dt_irq_match);
> >>>> #endif
> >>>>
> >>>> Perhaps we should add a dummy of_irq_init() too, so that we can remove
> >>>> this ifdef.
> >>>
> >>> Yes, indeed, I think that's much better. Most of_XXX APIs do have a
> >>> dummy version, but for some reason some of them do not.
> >>> That one clearly deserve a dummy version.
> >>
> >> I had an empty version originally and removed it based on Grant's review...
> >>
> >> The original intent was this call should be in a DT board file and
> >> therefore always enabled. Perhaps you can split tegra_init_irq into 2
> >> functions with one having all but gic_init and then call that function
> >> from board-dt.c along with of_irq_init.
> >
> > Well, that's certainly possible, but it seems a little inconsistent to
> > initialize the GIC from tegra_init_irq for non-DT, yet do it somewhere
> > else in the DT case. It seems like putting all the GIC stuff into
> > tegra_irq_init is the right thing to do. I note that Exynos works this
> > way already.
> >
> > I couldn't find Grant's email to see his rationale for not wanting an
> > empty or_irq_init(); do you have a link?
> >
> 
> My brain is rotting... I think it was gic_of_init that I was thinking of
> but still can't find the email.
> 
> Will your code even link currently with !CONFIG_OF. I would think it
> cannot resolve gic_of_init in that case.
> 
> I have a lot of concerns that supporting both CONFIG_OF and !CONFIG_OF
> is going to be a pain as it's yet another variable to compile against.
> In order to actually start reducing reducing the size of the ARM
> platform code (that is the goal, right?), platforms need to be always OF
> enabled and start removing the !CONFIG_OF code. So why not always turn
> on CONFIG_OF for Tegra and not worry about this? Eventually, CONFIG_OF
> will always be enabled anyway.

I've been looking at hooking up the Tegra GPIO controller's IRQ support
through device tree. It looks like I need to call irq_domain_add_simple()
for the GPIO node to make this work, and I think this needs to be triggered
by of_irq_init()'s match table, and hence I need a single of_irq_init()
call in Tegra's board-dt.c, which includes both the GIC and GPIO controller
entries.

Is my understanding here all correct? If so, I will just follow your
initial suggestion and have tegra_init_irq() do just:

if (!of_have_populated_dt())
        gic_init_bases(...);

... and move the of_irq_init call into board-dt.c.

Thanks.

-- 
nvpublic

Re: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Rob Herring]

On 11/29/2011 04:58 PM, Stephen Warren wrote:
> Rob Herring wrote at Tuesday, November 29, 2011 3:25 PM:
>> On 11/29/2011 03:08 PM, Stephen Warren wrote:

snip

>> My brain is rotting... I think it was gic_of_init that I was thinking of
>> but still can't find the email.
>>
>> Will your code even link currently with !CONFIG_OF. I would think it
>> cannot resolve gic_of_init in that case.
>>
>> I have a lot of concerns that supporting both CONFIG_OF and !CONFIG_OF
>> is going to be a pain as it's yet another variable to compile against.
>> In order to actually start reducing reducing the size of the ARM
>> platform code (that is the goal, right?), platforms need to be always OF
>> enabled and start removing the !CONFIG_OF code. So why not always turn
>> on CONFIG_OF for Tegra and not worry about this? Eventually, CONFIG_OF
>> will always be enabled anyway.
> 
> I've been looking at hooking up the Tegra GPIO controller's IRQ support
> through device tree. It looks like I need to call irq_domain_add_simple()
> for the GPIO node to make this work, and I think this needs to be triggered
> by of_irq_init()'s match table, and hence I need a single of_irq_init()
> call in Tegra's board-dt.c, which includes both the GIC and GPIO controller
> entries.
>
> Is my understanding here all correct? If so, I will just follow your
> initial suggestion and have tegra_init_irq() do just:

Yes, you should have 1 of_irq_init call. Really the irqdomain code
should go into the gpio code. Do you use generic irqchip for gpio? If
so, I'm working on a patch adding irqdomain to generic irqchip. I still
need to debug a problem with it and have been too busy to get back to it.

Rob

> if (!of_have_populated_dt())
>         gic_init_bases(...);
> 
> ... and move the of_irq_init call into board-dt.c.
> 
> Thanks.
> 

RE: [PATCH V2] arm/tegra: convert tegra20 to GIC devicetree binding

[Stephen Warren]

Rob Herring wrote at Tuesday, November 29, 2011 4:17 PM:
> On 11/29/2011 04:58 PM, Stephen Warren wrote:
...
> > I've been looking at hooking up the Tegra GPIO controller's IRQ support
> > through device tree. It looks like I need to call irq_domain_add_simple()
> > for the GPIO node to make this work, and I think this needs to be triggered
> > by of_irq_init()'s match table, and hence I need a single of_irq_init()
> > call in Tegra's board-dt.c, which includes both the GIC and GPIO controller
> > entries.
> >
> > Is my understanding here all correct? If so, I will just follow your
> > initial suggestion and have tegra_init_irq() do just:
> 
> Yes, you should have 1 of_irq_init call. Really the irqdomain code
> should go into the gpio code.

It sounds like you're saying:

* The of_irq_init() call doesn't have to include all the IRQ controllers
  in the system.
* So, it's fine for the Tegra GPIO controller's probe() function to
  simply call irq_domain_add_simple() for itself.

Is that correct? If so, I'll gladly add the call to the GPIO controller
and leave just the GIC to be handled via of_irq_init().

The only issue here is the usual device ordering issue; if the Tegra GPIO
controller doesn't probe before devices that use it, then the IRQ domain
won't exist. But, this is true for the interrupts themselves, so nothing
new there.

> Do you use generic irqchip for gpio?

No, it's custom code. Some of the code could probably use the generic
code, but there is some additional functionality beyond that.

> If
> so, I'm working on a patch adding irqdomain to generic irqchip. I still
> need to debug a problem with it and have been too busy to get back to it.

-- 
nvpublic