| Message ID | 1388987892-23733-5-git-send-email-wens@csie.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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?
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
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 --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>;
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(-)