[PATCH 0/2] ARM: shmobile: r8a7779/marzen CCF DT updates

[PATCH 0/2] ARM: shmobile: r8a7779/marzen CCF DT updates

[Geert Uytterhoeven]

	Hi Simon,

Here are a few updates for r8a7779/marzen CCF DT:
  - [1/2] ARM: shmobile: r8a7779 dtsi: Correct #address-cells/#size-cells
          for clocks
  - [2/2] ARM: shmobile: marzen dts: Remove superfluous include

This was only compile-tested, due to lack of hardware.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/2] ARM: shmobile: r8a7779 dtsi: Correct #address-cells/#size-cells for clocks

[Geert Uytterhoeven]

Warning (ranges_format): /clocks has empty "ranges" property but its #address-cells (2) differs from / (1)
Warning (ranges_format): /clocks has empty "ranges" property but its #size-cells (2) differs from / (1)

As r8a7779 doesn't support LPAE, change #address-cells and #size-cells from
"<2>" to "<1>" to fix this.

Also correct the unit-address for the cpg_clocks node, and add missing
unit-addresses for the mstp*_clks nodes.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Is it correct that r8a7779 doesn't support LPAE? I don't have the datasheet,
but the similar r8a7778 doesn't.

 arch/arm/boot/dts/r8a7779.dtsi | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7779.dtsi b/arch/arm/boot/dts/r8a7779.dtsi
index 038c16a18373..dc624e0d7506 100644
--- a/arch/arm/boot/dts/r8a7779.dtsi
+++ b/arch/arm/boot/dts/r8a7779.dtsi
@@ -284,8 +284,8 @@
 	};
 
 	clocks {
-		#address-cells = <2>;
-		#size-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <1>;
 		ranges;
 
 		/* External root clock */
@@ -298,9 +298,9 @@
 		};
 
 		/* Special CPG clocks */
-		cpg_clocks: cpg_clocks@0xe6150000 {
+		cpg_clocks: cpg_clocks@0xffc80000 {
 			compatible = "renesas,r8a7779-cpg-clocks";
-			reg = <0 0xffc80000 0 0x30>;
+			reg = <0xffc80000 0x30>;
 			clocks = <&extal_clk>;
 			#clock-cells = <1>;
 			clock-output-names = "plla", "z", "zs", "s",
@@ -342,10 +342,10 @@
 		};
 
 		/* Gate clocks */
-		mstp0_clks: mstp0_clks {
+		mstp0_clks: mstp0_clks@ffc80030 {
 			compatible = "renesas,r8a7779-mstp-clocks",
 			             "renesas,cpg-mstp-clocks";
-			reg = <0 0xffc80030 0 4>;
+			reg = <0xffc80030 4>;
 			clocks = <&cpg_clocks R8A7779_CLK_S>,
 			         <&cpg_clocks R8A7779_CLK_P>,
 				 <&cpg_clocks R8A7779_CLK_P>,
@@ -379,10 +379,10 @@
 				"scif1", "scif0", "i2c3", "i2c2", "i2c1",
 				"i2c0";
 		};
-		mstp1_clks: mstp1_clks {
+		mstp1_clks: mstp1_clks@ffc80034 {
 			compatible = "renesas,r8a7779-mstp-clocks",
 			             "renesas,cpg-mstp-clocks";
-			reg = <0 0xffc80034 0 4>, <0 0xffc80044 0 4>;
+			reg = <0xffc80034 4>, <0xffc80044 4>;
 			clocks = <&cpg_clocks R8A7779_CLK_P>,
 				 <&cpg_clocks R8A7779_CLK_P>,
 				 <&cpg_clocks R8A7779_CLK_S>,
@@ -408,10 +408,10 @@
 				"ether", "sata",
 				"pcie", "vin3";
 		};
-		mstp3_clks: mstp3_clks {
+		mstp3_clks: mstp3_clks@ffc8003c {
 			compatible = "renesas,r8a7779-mstp-clocks",
 			             "renesas,cpg-mstp-clocks";
-			reg = <0 0xffc8003c 0 4>;
+			reg = <0xffc8003c 4>;
 			clocks = <&s4_clk>, <&s4_clk>, <&s4_clk>, <&s4_clk>,
 				 <&s4_clk>, <&s4_clk>;
 			#clock-cells = <1>;
-- 
1.9.1

[PATCH 2/2] ARM: shmobile: marzen dts: Remove superfluous include

[Geert Uytterhoeven]

<dt-bindings/interrupt-controller/irq.h> is already included by
r8a7779.dtsi.

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 arch/arm/boot/dts/r8a7779-marzen.dts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/r8a7779-marzen.dts b/arch/arm/boot/dts/r8a7779-marzen.dts
index 321290828eee..649129906d6f 100644
--- a/arch/arm/boot/dts/r8a7779-marzen.dts
+++ b/arch/arm/boot/dts/r8a7779-marzen.dts
@@ -12,7 +12,6 @@
 /dts-v1/;
 #include "r8a7779.dtsi"
 #include <dt-bindings/gpio/gpio.h>
-#include <dt-bindings/interrupt-controller/irq.h>
 
 / {
 	model = "marzen";
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ARM: shmobile: marzen dts: Remove superfluous include

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 22 May 2014 14:33:17 Geert Uytterhoeven wrote:
> <dt-bindings/interrupt-controller/irq.h> is already included by
> r8a7779.dtsi.

Isn't it better, like with C code, to explicitly include headers that provide 
the macros you need, instead of relying on implicit #include's that could 
change later ? OK, in this specific case, it's very unlikely that r8a7779.dtsi 
would stop including <dt-bindings/interrupt-controller/irq.h>, but in the 
general case I believe explicit includes to be good practice.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  arch/arm/boot/dts/r8a7779-marzen.dts | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/r8a7779-marzen.dts
> b/arch/arm/boot/dts/r8a7779-marzen.dts index 321290828eee..649129906d6f
> 100644
> --- a/arch/arm/boot/dts/r8a7779-marzen.dts
> +++ b/arch/arm/boot/dts/r8a7779-marzen.dts
> @@ -12,7 +12,6 @@
>  /dts-v1/;
>  #include "r8a7779.dtsi"
>  #include <dt-bindings/gpio/gpio.h>
> -#include <dt-bindings/interrupt-controller/irq.h>
> 
>  / {
>  	model = "marzen";

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] ARM: shmobile: marzen dts: Remove superfluous include

[Geert Uytterhoeven]

On Thu, May 22, 2014 at 2:50 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 22 May 2014 14:33:17 Geert Uytterhoeven wrote:
>> <dt-bindings/interrupt-controller/irq.h> is already included by
>> r8a7779.dtsi.
>
> Isn't it better, like with C code, to explicitly include headers that provide
> the macros you need, instead of relying on implicit #include's that could
> change later ? OK, in this specific case, it's very unlikely that r8a7779.dtsi
> would stop including <dt-bindings/interrupt-controller/irq.h>, but in the
> general case I believe explicit includes to be good practice.

You're right. I was a bit overzealous.

Simon, please drop this.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] ARM: shmobile: r8a7779 dtsi: Correct #address-cells/#size-cells for clocks

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 22 May 2014 14:33:16 Geert Uytterhoeven wrote:
> Warning (ranges_format): /clocks has empty "ranges" property but its
> #address-cells (2) differs from / (1) Warning (ranges_format): /clocks has
> empty "ranges" property but its #size-cells (2) differs from / (1)
> 
> As r8a7779 doesn't support LPAE, change #address-cells and #size-cells from
> "<2>" to "<1>" to fix this.
> 
> Also correct the unit-address for the cpg_clocks node, and add missing
> unit-addresses for the mstp*_clks nodes.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> ---
> Is it correct that r8a7779 doesn't support LPAE? I don't have the datasheet,
> but the similar r8a7778 doesn't.
> 
>  arch/arm/boot/dts/r8a7779.dtsi | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/r8a7779.dtsi b/arch/arm/boot/dts/r8a7779.dtsi
> index 038c16a18373..dc624e0d7506 100644
> --- a/arch/arm/boot/dts/r8a7779.dtsi
> +++ b/arch/arm/boot/dts/r8a7779.dtsi
> @@ -284,8 +284,8 @@
>  	};
> 
>  	clocks {
> -		#address-cells = <2>;
> -		#size-cells = <2>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;

This looks good to me.

>  		ranges;
> 
>  		/* External root clock */
> @@ -298,9 +298,9 @@
>  		};
> 
>  		/* Special CPG clocks */
> -		cpg_clocks: cpg_clocks@0xe6150000 {
> +		cpg_clocks: cpg_clocks@0xffc80000 {

No need for a 0x prefix.

>  			compatible = "renesas,r8a7779-cpg-clocks";
> -			reg = <0 0xffc80000 0 0x30>;
> +			reg = <0xffc80000 0x30>;

This looks good.

>  			clocks = <&extal_clk>;
>  			#clock-cells = <1>;
>  			clock-output-names = "plla", "z", "zs", "s",
> @@ -342,10 +342,10 @@
>  		};
> 
>  		/* Gate clocks */
> -		mstp0_clks: mstp0_clks {
> +		mstp0_clks: mstp0_clks@ffc80030 {

The @address is only mandatory to differentiate between nodes with identical 
names. As the node name is supposed to describe the device function, maybe we 
could rename it to clocks@ffc80030 ? As you modify the CPG node address above 
it might make sense to rename it to clocks@ffc80000 too.

I'm not advocating for a rename now, but as your patch touches the names, I 
think they should be fixed properly (or not at all).

>  			compatible = "renesas,r8a7779-mstp-clocks",
>  			             "renesas,cpg-mstp-clocks";
> -			reg = <0 0xffc80030 0 4>;
> +			reg = <0xffc80030 4>;
>  			clocks = <&cpg_clocks R8A7779_CLK_S>,
>  			         <&cpg_clocks R8A7779_CLK_P>,
>  				 <&cpg_clocks R8A7779_CLK_P>,
> @@ -379,10 +379,10 @@
>  				"scif1", "scif0", "i2c3", "i2c2", "i2c1",
>  				"i2c0";
>  		};
> -		mstp1_clks: mstp1_clks {
> +		mstp1_clks: mstp1_clks@ffc80034 {
>  			compatible = "renesas,r8a7779-mstp-clocks",
>  			             "renesas,cpg-mstp-clocks";
> -			reg = <0 0xffc80034 0 4>, <0 0xffc80044 0 4>;
> +			reg = <0xffc80034 4>, <0xffc80044 4>;
>  			clocks = <&cpg_clocks R8A7779_CLK_P>,
>  				 <&cpg_clocks R8A7779_CLK_P>,
>  				 <&cpg_clocks R8A7779_CLK_S>,
> @@ -408,10 +408,10 @@
>  				"ether", "sata",
>  				"pcie", "vin3";
>  		};
> -		mstp3_clks: mstp3_clks {
> +		mstp3_clks: mstp3_clks@ffc8003c {
>  			compatible = "renesas,r8a7779-mstp-clocks",
>  			             "renesas,cpg-mstp-clocks";
> -			reg = <0 0xffc8003c 0 4>;
> +			reg = <0xffc8003c 4>;
>  			clocks = <&s4_clk>, <&s4_clk>, <&s4_clk>, <&s4_clk>,
>  				 <&s4_clk>, <&s4_clk>;
>  			#clock-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] ARM: shmobile: marzen dts: Remove superfluous include

[Simon Horman]

On Thu, May 22, 2014 at 02:58:57PM +0200, Geert Uytterhoeven wrote:
> On Thu, May 22, 2014 at 2:50 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Thursday 22 May 2014 14:33:17 Geert Uytterhoeven wrote:
> >> <dt-bindings/interrupt-controller/irq.h> is already included by
> >> r8a7779.dtsi.
> >
> > Isn't it better, like with C code, to explicitly include headers that provide
> > the macros you need, instead of relying on implicit #include's that could
> > change later ? OK, in this specific case, it's very unlikely that r8a7779.dtsi
> > would stop including <dt-bindings/interrupt-controller/irq.h>, but in the
> > general case I believe explicit includes to be good practice.
> 
> You're right. I was a bit overzealous.
> 
> Simon, please drop this.

Done.

Re: [PATCH 1/2] ARM: shmobile: r8a7779 dtsi: Correct #address-cells/#size-cells for clocks

[Geert Uytterhoeven]

Hi Laurent,

On Fri, May 23, 2014 at 1:08 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> -             cpg_clocks: cpg_clocks@0xe6150000 {
>> +             cpg_clocks: cpg_clocks@0xffc80000 {
>
> No need for a 0x prefix.

Thanks, missed that one.

>>                       compatible = "renesas,r8a7779-cpg-clocks";
>> -                     reg = <0 0xffc80000 0 0x30>;
>> +                     reg = <0xffc80000 0x30>;
>
> This looks good.
>
>>                       clocks = <&extal_clk>;
>>                       #clock-cells = <1>;
>>                       clock-output-names = "plla", "z", "zs", "s",
>> @@ -342,10 +342,10 @@
>>               };
>>
>>               /* Gate clocks */
>> -             mstp0_clks: mstp0_clks {
>> +             mstp0_clks: mstp0_clks@ffc80030 {
>
> The @address is only mandatory to differentiate between nodes with identical
> names. As the node name is supposed to describe the device function, maybe we
> could rename it to clocks@ffc80030 ? As you modify the CPG node address above
> it might make sense to rename it to clocks@ffc80000 too.

Sounds good!

> I'm not advocating for a rename now, but as your patch touches the names, I
> think they should be fixed properly (or not at all).

In hindsight, I should have separated the fix and the "obvious small things",
as the latter are subject to bike-shedding ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds