diff mbox

[v2,4/8] arm: dts: sun4i: rename clock node names to clk@N

Message ID 1388987892-23733-5-git-send-email-wens@csie.org
State New, archived
Headers show

Commit Message

Chen-Yu Tsai Jan. 6, 2014, 5:58 a.m. UTC
Device tree naming conventions state that node names should match
node function. Change fully functioning clock nodes to match.

Also add the output name for pll5 to use as the clock name.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun4i-a10.dtsi | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

Maxime Ripard Jan. 7, 2014, 10:38 p.m. UTC | #1
On Mon, Jan 06, 2014 at 01:58:08PM +0800, Chen-Yu Tsai wrote:
> Device tree naming conventions state that node names should match
> node function. Change fully functioning clock nodes to match.
> 
> Also add the output name for pll5 to use as the clock name.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  arch/arm/boot/dts/sun4i-a10.dtsi | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index 3ba2b46..45d5283 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -50,42 +50,46 @@
>  			clock-frequency = <0>;
>  		};
>  
> -		osc24M: osc24M@01c20050 {
> +		osc24M: clk@01c20050 {
>  			#clock-cells = <0>;
>  			compatible = "allwinner,sun4i-osc-clk";
>  			reg = <0x01c20050 0x4>;
>  			clock-frequency = <24000000>;
> +			clock-output-names = "osc24M";
>  		};
>  
> -		osc32k: osc32k {
> +		osc32k: clk@0 {
>  			#clock-cells = <0>;
>  			compatible = "fixed-clock";
>  			clock-frequency = <32768>;
> +			clock-output-names = "osc32k";
>  		};
>  
> -		pll1: pll1@01c20000 {
> +		pll1: clk@01c20000 {
>  			#clock-cells = <0>;
>  			compatible = "allwinner,sun4i-pll1-clk";
>  			reg = <0x01c20000 0x4>;
>  			clocks = <&osc24M>;
> +			clock-output-names = "pll1";
>  		};
>  
> -		pll4: pll4@01c20018 {
> +		pll4: clk@01c20018 {
>  			#clock-cells = <0>;
>  			compatible = "allwinner,sun4i-pll1-clk";
>  			reg = <0x01c20018 0x4>;
>  			clocks = <&osc24M>;
> +			clock-output-names = "pll4";
>  		};
>  
> -		pll5: pll5@01c20020 {
> +		pll5: clk@01c20020 {
>  			#clock-cells = <1>;
>  			compatible = "allwinner,sun4i-pll5-clk";
>  			reg = <0x01c20020 0x4>;
>  			clocks = <&osc24M>;
> -			clock-output-names = "pll5_ddr", "pll5_other";
> +			clock-output-names = "pll5_ddr", "pll5_other", "pll5";

Hmmm, I don't really like that bit too much.

This "pll5" clock doesn't actually exist at the hardware point of
view, which is not really what the DT is used for.

I can think of two ways to do what you want withouth this: 
  - either hardcode the name, since we have a compatible of our own here
  - or use strchr to take anyhing until '_' and use that as a name
Chen-Yu Tsai Jan. 8, 2014, 1:38 a.m. UTC | #2
On Wed, Jan 8, 2014 at 6:38 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Mon, Jan 06, 2014 at 01:58:08PM +0800, Chen-Yu Tsai wrote:
>> Device tree naming conventions state that node names should match
>> node function. Change fully functioning clock nodes to match.
>>
>> Also add the output name for pll5 to use as the clock name.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  arch/arm/boot/dts/sun4i-a10.dtsi | 26 +++++++++++++++-----------
>>  1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
>> index 3ba2b46..45d5283 100644
>> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
>> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
>> @@ -50,42 +50,46 @@
>>                       clock-frequency = <0>;
>>               };
>>
>> -             osc24M: osc24M@01c20050 {
>> +             osc24M: clk@01c20050 {
>>                       #clock-cells = <0>;
>>                       compatible = "allwinner,sun4i-osc-clk";
>>                       reg = <0x01c20050 0x4>;
>>                       clock-frequency = <24000000>;
>> +                     clock-output-names = "osc24M";
>>               };
>>
>> -             osc32k: osc32k {
>> +             osc32k: clk@0 {
>>                       #clock-cells = <0>;
>>                       compatible = "fixed-clock";
>>                       clock-frequency = <32768>;
>> +                     clock-output-names = "osc32k";
>>               };
>>
>> -             pll1: pll1@01c20000 {
>> +             pll1: clk@01c20000 {
>>                       #clock-cells = <0>;
>>                       compatible = "allwinner,sun4i-pll1-clk";
>>                       reg = <0x01c20000 0x4>;
>>                       clocks = <&osc24M>;
>> +                     clock-output-names = "pll1";
>>               };
>>
>> -             pll4: pll4@01c20018 {
>> +             pll4: clk@01c20018 {
>>                       #clock-cells = <0>;
>>                       compatible = "allwinner,sun4i-pll1-clk";
>>                       reg = <0x01c20018 0x4>;
>>                       clocks = <&osc24M>;
>> +                     clock-output-names = "pll4";
>>               };
>>
>> -             pll5: pll5@01c20020 {
>> +             pll5: clk@01c20020 {
>>                       #clock-cells = <1>;
>>                       compatible = "allwinner,sun4i-pll5-clk";
>>                       reg = <0x01c20020 0x4>;
>>                       clocks = <&osc24M>;
>> -                     clock-output-names = "pll5_ddr", "pll5_other";
>> +                     clock-output-names = "pll5_ddr", "pll5_other", "pll5";
>
> Hmmm, I don't really like that bit too much.
>
> This "pll5" clock doesn't actually exist at the hardware point of
> view, which is not really what the DT is used for.

You are right. pll5 only has 2 outputs. I was matching the format of
pll6, which I'd like to include in this discussion.

Does pll6 actually have 3 outputs? or are we just using the third
output as a shortcut for mbus input of pll6*2 ?

> I can think of two ways to do what you want withouth this:
>   - either hardcode the name, since we have a compatible of our own here

Since we have seperate compatibles for pll5 and pll6, I would prefer
to add a .name to clk_factors_config, instead of adding it in the code.
Emilio, is that okay with you?

>   - or use strchr to take anyhing until '_' and use that as a name

Thanks
ChenYu
Maxime Ripard Jan. 9, 2014, 8:53 a.m. UTC | #3
On Wed, Jan 08, 2014 at 09:38:52AM +0800, Chen-Yu Tsai wrote:
> On Wed, Jan 8, 2014 at 6:38 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Jan 06, 2014 at 01:58:08PM +0800, Chen-Yu Tsai wrote:
> >> Device tree naming conventions state that node names should match
> >> node function. Change fully functioning clock nodes to match.
> >>
> >> Also add the output name for pll5 to use as the clock name.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> ---
> >>  arch/arm/boot/dts/sun4i-a10.dtsi | 26 +++++++++++++++-----------
> >>  1 file changed, 15 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> >> index 3ba2b46..45d5283 100644
> >> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> >> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> >> @@ -50,42 +50,46 @@
> >>                       clock-frequency = <0>;
> >>               };
> >>
> >> -             osc24M: osc24M@01c20050 {
> >> +             osc24M: clk@01c20050 {
> >>                       #clock-cells = <0>;
> >>                       compatible = "allwinner,sun4i-osc-clk";
> >>                       reg = <0x01c20050 0x4>;
> >>                       clock-frequency = <24000000>;
> >> +                     clock-output-names = "osc24M";
> >>               };
> >>
> >> -             osc32k: osc32k {
> >> +             osc32k: clk@0 {
> >>                       #clock-cells = <0>;
> >>                       compatible = "fixed-clock";
> >>                       clock-frequency = <32768>;
> >> +                     clock-output-names = "osc32k";
> >>               };
> >>
> >> -             pll1: pll1@01c20000 {
> >> +             pll1: clk@01c20000 {
> >>                       #clock-cells = <0>;
> >>                       compatible = "allwinner,sun4i-pll1-clk";
> >>                       reg = <0x01c20000 0x4>;
> >>                       clocks = <&osc24M>;
> >> +                     clock-output-names = "pll1";
> >>               };
> >>
> >> -             pll4: pll4@01c20018 {
> >> +             pll4: clk@01c20018 {
> >>                       #clock-cells = <0>;
> >>                       compatible = "allwinner,sun4i-pll1-clk";
> >>                       reg = <0x01c20018 0x4>;
> >>                       clocks = <&osc24M>;
> >> +                     clock-output-names = "pll4";
> >>               };
> >>
> >> -             pll5: pll5@01c20020 {
> >> +             pll5: clk@01c20020 {
> >>                       #clock-cells = <1>;
> >>                       compatible = "allwinner,sun4i-pll5-clk";
> >>                       reg = <0x01c20020 0x4>;
> >>                       clocks = <&osc24M>;
> >> -                     clock-output-names = "pll5_ddr", "pll5_other";
> >> +                     clock-output-names = "pll5_ddr", "pll5_other", "pll5";
> >
> > Hmmm, I don't really like that bit too much.
> >
> > This "pll5" clock doesn't actually exist at the hardware point of
> > view, which is not really what the DT is used for.
> 
> You are right. pll5 only has 2 outputs. I was matching the format of
> pll6, which I'd like to include in this discussion.
> 
> Does pll6 actually have 3 outputs? or are we just using the third
> output as a shortcut for mbus input of pll6*2 ?

Hmmm, indeed. I don't really get why pll6 has a third output
either. Emilio?
Emilio López Jan. 9, 2014, 3:47 p.m. UTC | #4
Hi,

2014/1/9 Maxime Ripard <maxime.ripard@free-electrons.com>:
>> You are right. pll5 only has 2 outputs. I was matching the format of
>> pll6, which I'd like to include in this discussion.
>>
>> Does pll6 actually have 3 outputs? or are we just using the third
>> output as a shortcut for mbus input of pll6*2 ?
>
> Hmmm, indeed. I don't really get why pll6 has a third output
> either. Emilio?

Citing the A20 user manual (my comments on the right)

-----8<------
For SATA, the output =(24MHz*N*K)/M/6 <-- we call this pll6_sata
If the SATA is on, the clock output should be equal to 100MHz;
For other module, the clock output = (24MHz*N*K)/2 <-- we call this pll6_other
PLL6*2 = 24MHz*N*K <-- this would be the third output, which we call pll6
-----8<------

This last output is used by things like mbus and LCD

Now, pll5 says

-----8<------
The PLL5 output for DDR = (24MHz*N*K)/M. <-- we call this pll5_ddr
The PLL5 output for other module =(24MHz*N*K)/P. <-- we call this pll5_other
-----8<------

There does not seem to be anything connected to "pll5" with a rate of
24MHz*N*K, but personally I would not be opposed to adding it to the
DT for consistency with pll6. After all, it actually is the common
ancestor of pll5_ddr and pll5_other.

There's also some ASCII art on the code to visualize these clocks better

http://git.linaro.org/people/mike.turquette/linux.git/blob/refs/heads/clk-next:/drivers/clk/sunxi/clk-sunxi.c#l842

I hope this clarifies things.

Cheers,

Emilio
Chen-Yu Tsai Jan. 9, 2014, 4:02 p.m. UTC | #5
Hi,

On Thu, Jan 9, 2014 at 11:47 PM, Emilio López <emilio@elopez.com.ar> wrote:
> Hi,
>
> 2014/1/9 Maxime Ripard <maxime.ripard@free-electrons.com>:
>>> You are right. pll5 only has 2 outputs. I was matching the format of
>>> pll6, which I'd like to include in this discussion.
>>>
>>> Does pll6 actually have 3 outputs? or are we just using the third
>>> output as a shortcut for mbus input of pll6*2 ?
>>
>> Hmmm, indeed. I don't really get why pll6 has a third output
>> either. Emilio?
>
> Citing the A20 user manual (my comments on the right)
>
> -----8<------
> For SATA, the output =(24MHz*N*K)/M/6 <-- we call this pll6_sata
> If the SATA is on, the clock output should be equal to 100MHz;
> For other module, the clock output = (24MHz*N*K)/2 <-- we call this pll6_other
> PLL6*2 = 24MHz*N*K <-- this would be the third output, which we call pll6
> -----8<------
>
> This last output is used by things like mbus and LCD

A wild guess, maybe those modules have a frequency doubler after the
PLL6 input?  Though pll6 direct output seems more reasonable.

Do we want to match the hardware exactly? If so we might want to ask Allwinner.

> Now, pll5 says
>
> -----8<------
> The PLL5 output for DDR = (24MHz*N*K)/M. <-- we call this pll5_ddr
> The PLL5 output for other module =(24MHz*N*K)/P. <-- we call this pll5_other
> -----8<------
>
> There does not seem to be anything connected to "pll5" with a rate of
> 24MHz*N*K, but personally I would not be opposed to adding it to the
> DT for consistency with pll6. After all, it actually is the common
> ancestor of pll5_ddr and pll5_other.

I already posted a version without adding "pll5". The names are coded into
factors_data instead.


Cheers,
ChenYu

> There's also some ASCII art on the code to visualize these clocks better
>
> http://git.linaro.org/people/mike.turquette/linux.git/blob/refs/heads/clk-next:/drivers/clk/sunxi/clk-sunxi.c#l842
>
> I hope this clarifies things.
>
> Cheers,
>
> Emilio
diff mbox

Patch

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 3ba2b46..45d5283 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -50,42 +50,46 @@ 
 			clock-frequency = <0>;
 		};
 
-		osc24M: osc24M@01c20050 {
+		osc24M: clk@01c20050 {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-osc-clk";
 			reg = <0x01c20050 0x4>;
 			clock-frequency = <24000000>;
+			clock-output-names = "osc24M";
 		};
 
-		osc32k: osc32k {
+		osc32k: clk@0 {
 			#clock-cells = <0>;
 			compatible = "fixed-clock";
 			clock-frequency = <32768>;
+			clock-output-names = "osc32k";
 		};
 
-		pll1: pll1@01c20000 {
+		pll1: clk@01c20000 {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-pll1-clk";
 			reg = <0x01c20000 0x4>;
 			clocks = <&osc24M>;
+			clock-output-names = "pll1";
 		};
 
-		pll4: pll4@01c20018 {
+		pll4: clk@01c20018 {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-pll1-clk";
 			reg = <0x01c20018 0x4>;
 			clocks = <&osc24M>;
+			clock-output-names = "pll4";
 		};
 
-		pll5: pll5@01c20020 {
+		pll5: clk@01c20020 {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-pll5-clk";
 			reg = <0x01c20020 0x4>;
 			clocks = <&osc24M>;
-			clock-output-names = "pll5_ddr", "pll5_other";
+			clock-output-names = "pll5_ddr", "pll5_other", "pll5";
 		};
 
-		pll6: pll6@01c20028 {
+		pll6: clk@01c20028 {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-pll6-clk";
 			reg = <0x01c20028 0x4>;
@@ -108,7 +112,7 @@ 
 			clocks = <&cpu>;
 		};
 
-		axi_gates: axi_gates@01c2005c {
+		axi_gates: clk@01c2005c {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-axi-gates-clk";
 			reg = <0x01c2005c 0x4>;
@@ -123,7 +127,7 @@ 
 			clocks = <&axi>;
 		};
 
-		ahb_gates: ahb_gates@01c20060 {
+		ahb_gates: clk@01c20060 {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-ahb-gates-clk";
 			reg = <0x01c20060 0x8>;
@@ -148,7 +152,7 @@ 
 			clocks = <&ahb>;
 		};
 
-		apb0_gates: apb0_gates@01c20068 {
+		apb0_gates: clk@01c20068 {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-apb0-gates-clk";
 			reg = <0x01c20068 0x4>;
@@ -172,7 +176,7 @@ 
 			clocks = <&apb1_mux>;
 		};
 
-		apb1_gates: apb1_gates@01c2006c {
+		apb1_gates: clk@01c2006c {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-apb1-gates-clk";
 			reg = <0x01c2006c 0x4>;