[PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[Tim Harvey]

When using interrupt-maps, the size of a map entry is #address-cells +
 #interrupt-cells for the parent interrupt controller.  For the ARM GIC
address-cells should be 0 as this is not used.

This patch fixes the example by correctly specifying #address-cells = 0.

Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: linux-samsung-soc <linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Richard Zhu <r65037-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
Cc: linux-tegra <linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Kukjin Kim <kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/gic.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index bae0d87..b59ff1f 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -55,7 +55,7 @@ Example:
 	intc: interrupt-controller@fff11000 {
 		compatible = "arm,cortex-a9-gic";
 		#interrupt-cells = <3>;
-		#address-cells = <1>;
+		#address-cells = <0>;
 		interrupt-controller;
 		reg = <0xfff11000 0x1000>,
 		      <0xfff10100 0x100>;
-- 
1.8.3.2

Re: [PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[Stephen Warren]

On 03/13/2014 11:40 AM, Tim Harvey wrote:
> When using interrupt-maps, the size of a map entry is #address-cells +
>  #interrupt-cells for the parent interrupt controller.  For the ARM GIC
> address-cells should be 0 as this is not used.
> 
> This patch fixes the example by correctly specifying #address-cells = 0.

If the #address-cells property is required (well, or even optional...)
in the node, shouldn't it be included in the list of required/optional
properties above, and not solely included in the example?

Re: [PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[Jason Gunthorpe]

On Thu, Mar 13, 2014 at 11:44:33AM -0600, Stephen Warren wrote:
> On 03/13/2014 11:40 AM, Tim Harvey wrote:
> > When using interrupt-maps, the size of a map entry is #address-cells +
> >  #interrupt-cells for the parent interrupt controller.  For the ARM GIC
> > address-cells should be 0 as this is not used.
> > 
> > This patch fixes the example by correctly specifying #address-cells = 0.
> 
> If the #address-cells property is required (well, or even optional...)
> in the node, shouldn't it be included in the list of required/optional
> properties above, and not solely included in the example?

AFAIK, #address-cells = 0 is the same as not including a
#address-cells at all.

Omitting entirely is what other interrupt-controller bindings are
doing, so I'd just drop mention of #address-cells completely.

Jason

Re: [PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[Tim Harvey]

On Thu, Mar 13, 2014 at 11:25 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Mar 13, 2014 at 11:44:33AM -0600, Stephen Warren wrote:
>> On 03/13/2014 11:40 AM, Tim Harvey wrote:
>> > When using interrupt-maps, the size of a map entry is #address-cells +
>> >  #interrupt-cells for the parent interrupt controller.  For the ARM GIC
>> > address-cells should be 0 as this is not used.
>> >
>> > This patch fixes the example by correctly specifying #address-cells = 0.
>>
>> If the #address-cells property is required (well, or even optional...)
>> in the node, shouldn't it be included in the list of required/optional
>> properties above, and not solely included in the example?
>
> AFAIK, #address-cells = 0 is the same as not including a
> #address-cells at all.
>
> Omitting entirely is what other interrupt-controller bindings are
> doing, so I'd just drop mention of #address-cells completely.
>
> Jason

Jason / Stephen,

Yes, it does indeed default to 0 if not specified.  For arm-gic it
would appear #address-cells must be 0, so should we remove all places
its set explicitly to 0 in dts as wel as the example in
Documentation/devicetree/bindings?

Looking through arch/arm/boot/dts I'm seeing other dts configs using
'arm,cortex-a9-gic' which I'm guessing are equally as wrong as the
imx6 situation:
 - arch/arm/boot/dts/highbank.dts
 - arch/arm/boot/dts/sh73a0.dtsi

I'm also seeing a number of places where its set explicitly to 0:
 - Documentation/devicetree/bindings/arm/gic.txt
 - Documentation/devicetree/bindings/arm/vexpress.txt
 - arch/arm/boot/dts/bcm11351.dtsi
 - arch/arm/boot/dts/hi3620.dtsi
 - arch/arm/boot/dts/imx6qdl.dtsi
 - arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts

If I extend this search to the other compatible gic interrupt
controllers implemented in drivers/irqchip/irq-gic.c
(arm,cortex-a15-gic, qcom,msm-8660-qgic, qcom,msm-qgic2) I find even
more examples where #address-cells is set explicitly to 0 or to 1 (and
I would believe a value of 1 is invalid, just as it is in my case).

I'm happy to broaden my patch, but I certainly want to make sure I'm
doing the right thing with it, as I don't have the ability to test all
the other devicetree configs and am by no means a devicetree expert.

Recommendations?

Tim

Re: [PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[Shawn Guo]

On Thu, Mar 13, 2014 at 03:25:50PM -0700, Tim Harvey wrote:
> On Thu, Mar 13, 2014 at 11:25 AM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > On Thu, Mar 13, 2014 at 11:44:33AM -0600, Stephen Warren wrote:
> >> On 03/13/2014 11:40 AM, Tim Harvey wrote:
> >> > When using interrupt-maps, the size of a map entry is #address-cells +
> >> >  #interrupt-cells for the parent interrupt controller.  For the ARM GIC
> >> > address-cells should be 0 as this is not used.
> >> >
> >> > This patch fixes the example by correctly specifying #address-cells = 0.
> >>
> >> If the #address-cells property is required (well, or even optional...)
> >> in the node, shouldn't it be included in the list of required/optional
> >> properties above, and not solely included in the example?
> >
> > AFAIK, #address-cells = 0 is the same as not including a
> > #address-cells at all.
> >
> > Omitting entirely is what other interrupt-controller bindings are
> > doing, so I'd just drop mention of #address-cells completely.
> >
> > Jason
> 
> Jason / Stephen,
> 
> Yes, it does indeed default to 0 if not specified.  For arm-gic it
> would appear #address-cells must be 0, so should we remove all places
> its set explicitly to 0 in dts as wel as the example in
> Documentation/devicetree/bindings?
> 
> Looking through arch/arm/boot/dts I'm seeing other dts configs using
> 'arm,cortex-a9-gic' which I'm guessing are equally as wrong as the
> imx6 situation:
>  - arch/arm/boot/dts/highbank.dts
>  - arch/arm/boot/dts/sh73a0.dtsi
> 
> I'm also seeing a number of places where its set explicitly to 0:
>  - Documentation/devicetree/bindings/arm/gic.txt
>  - Documentation/devicetree/bindings/arm/vexpress.txt
>  - arch/arm/boot/dts/bcm11351.dtsi
>  - arch/arm/boot/dts/hi3620.dtsi
>  - arch/arm/boot/dts/imx6qdl.dtsi
>  - arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
> 
> If I extend this search to the other compatible gic interrupt
> controllers implemented in drivers/irqchip/irq-gic.c
> (arm,cortex-a15-gic, qcom,msm-8660-qgic, qcom,msm-qgic2) I find even
> more examples where #address-cells is set explicitly to 0 or to 1 (and
> I would believe a value of 1 is invalid, just as it is in my case).
> 
> I'm happy to broaden my patch, but I certainly want to make sure I'm
> doing the right thing with it, as I don't have the ability to test all
> the other devicetree configs and am by no means a devicetree expert.
> 
> Recommendations?

Copy Rob, who might want to comment a bit.

My understanding is that #address-cells and #size-cells are only valid
for a node that may have child nodes.  Will gic node possibly get any
child node?

Shawn

Re: [PATCH 1/2] ARM: DT: fix gic interrupt controller documentation

[Grant Likely]

On Thu, Mar 13, 2014 at 6:25 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Thu, Mar 13, 2014 at 11:44:33AM -0600, Stephen Warren wrote:
>> On 03/13/2014 11:40 AM, Tim Harvey wrote:
>> > When using interrupt-maps, the size of a map entry is #address-cells +
>> >  #interrupt-cells for the parent interrupt controller.  For the ARM GIC
>> > address-cells should be 0 as this is not used.
>> >
>> > This patch fixes the example by correctly specifying #address-cells = 0.
>>
>> If the #address-cells property is required (well, or even optional...)
>> in the node, shouldn't it be included in the list of required/optional
>> properties above, and not solely included in the example?
>
> AFAIK, #address-cells = 0 is the same as not including a
> #address-cells at all.
>
> Omitting entirely is what other interrupt-controller bindings are
> doing, so I'd just drop mention of #address-cells completely.

Correct. Just drop it.

g.

[PATCH 2/2] ARM: dts: imx: fix invalid #address-cells value

[Tim Harvey]

The invalid value of #address-cells in the imx6 pcie host controller node
causes of_irq_parse_raw() to incorrectly advance through an interrupt-map
table of more than one interrupt.

This patch resolves this issue and allows proper interrupt mapping for an
imx6 pcie host connected to a P2P bridge when using legacy interrupts.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: linux-tegra <linux-tegra@vger.kernel.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Grant Likely <grant.likely@linaro.org>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index af8d28d..2285299 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -49,7 +49,7 @@
 	intc: interrupt-controller@00a01000 {
 		compatible = "arm,cortex-a9-gic";
 		#interrupt-cells = <3>;
-		#address-cells = <1>;
+		#address-cells = <0>;
 		#size-cells = <1>;
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
-- 
1.8.3.2

Re: [PATCH 2/2] ARM: dts: imx: fix invalid #address-cells value

[Shawn Guo]

On Thu, Mar 13, 2014 at 10:40:05AM -0700, Tim Harvey wrote:
> The invalid value of #address-cells in the imx6 pcie host controller node
> causes of_irq_parse_raw() to incorrectly advance through an interrupt-map
> table of more than one interrupt.
> 
> This patch resolves this issue and allows proper interrupt mapping for an
> imx6 pcie host connected to a P2P bridge when using legacy interrupts.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Ben Dooks <ben-linux@fluff.org>
> Cc: linux-tegra <linux-tegra@vger.kernel.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index af8d28d..2285299 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -49,7 +49,7 @@
>  	intc: interrupt-controller@00a01000 {
>  		compatible = "arm,cortex-a9-gic";
>  		#interrupt-cells = <3>;
> -		#address-cells = <1>;
> +		#address-cells = <0>;
>  		#size-cells = <1>;

So we should probably just drop both #address-cells and #size-cells?  If
so, please help patch imx6sl.dtsi as well.

Shawn

>  		interrupt-controller;
>  		reg = <0x00a01000 0x1000>,
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
>