[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 at 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 at 0xe6150000 {
+		cpg_clocks: cpg_clocks at 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 at 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 at 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 at 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 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 at 0xe6150000 {
> +		cpg_clocks: cpg_clocks at 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 at 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 at ffc80030 ? As you modify the CPG node address above 
it might make sense to rename it to clocks at 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 at 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 at 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

[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 at 0xe6150000 {
>> +             cpg_clocks: cpg_clocks at 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 at 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 at ffc80030 ? As you modify the CPG node address above
> it might make sense to rename it to clocks at 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 at 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 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@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";
-- 
1.9.1

[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

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