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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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