Re: [RFC 1/4] WIP: MIPS: ath79: make ar933x clks more devicetree-friendly

[Alban <albeu-GANU6spQydw@public.gmane.org>]

On Thu, 21 Jan 2016 13:17:11 +0300
Antony Pavlov <antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, 21 Jan 2016 09:12:17 +0100
> Alban <albeu-GANU6spQydw@public.gmane.org> wrote:
> 
> > On Thu, 21 Jan 2016 03:12:15 +0300
> > Antony Pavlov <antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > On Mon, 18 Jan 2016 20:57:25 +0100
> > > Alban <albeu-GANU6spQydw@public.gmane.org> wrote:
> > > 
> > > > On Mon, 18 Jan 2016 02:56:24 +0300
> > > > Antony Pavlov <antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > 
> > > > > At the moment ar933x of-enabled drivers use use clock names
> > > > > (e.g. "uart" or "ahb") to get clk descriptor.
> > > > > On the other hand
> > > > > Documentation/devicetree/bindings/clock/clock-bindings.txt states
> > > > > that the 'clocks' property is required for passing clk to clock
> > > > > consumers.
> > > > 
> > > > This patch is not need, you should set the clock-names property in
> > > > the relevant device nodes instead.
> > > 
> > > This patch is needed for AR9331!
> > > 
> > > In ar933x_clocks_init() we have
> > > 
> > >         ath79_add_sys_clkdev("ref", ref_rate);
> > >         clks[0] = ath79_add_sys_clkdev("cpu", cpu_rate);
> > >         clks[1] = ath79_add_sys_clkdev("ddr", ddr_rate);
> > >         clks[2] = ath79_add_sys_clkdev("ahb", ahb_rate);
> > > 
> > >         clk_add_alias("wdt", NULL, "ahb", NULL);
> > >         clk_add_alias("uart", NULL, "ref", NULL);
> > > 
> > > "uart" is an alias for "ref". But "ref" is not visible via device tree!
> > > 
> > > I see this error message on ar933x-uart start:
> > >  
> > >      ERROR: could not get clock /ahb/apb/uart@18020000:uart(0)
> > 
> > The ref clock should be defined in the board DTS, I now see that it is
> > missing in yours. What you need to do is to define the clock-names
> > property in the Soc DTS, that allow the names lookup to work. Then in
> > the board DTS you can define the clock property to connect it to the
> > proper parent. 
> > 
> > I'm also working on supporting the QCA9558 and the clock tree is similar.
> > See https://github.com/AlbanBedel/linux/commit/d6c8f8adfce08972c6
> > as example.
> 
> Current ath79 clock.c code does not read ref clock from devicetree!
> So you can set any clock rate value in board DTS but it will has no effect
> on the real clk calculation.

Yes, the ath79 clock is broken in this respect, however the uart driver
isn't and it will use the clock it receive. The fixed oscillator works
fine on QCA9558 which also use the ref clock for the uart.

> A more reasonable solution is used for CI20 board.
> In arch/mips/boot/dts/ingenic/jz4780.dtsi we have
> 
> 	ext: ext {
> 		compatible = "fixed-clock";
> 		#clock-cells = <0>;
> 	};
> 
> ...
> 
> 	cgu: jz4780-cgu@10000000 {
> 		compatible = "ingenic,jz4780-cgu";
> 		reg = <0x10000000 0x100>;
> 
> 		clocks = <&ext>, <&rtc>;
> 		clock-names = "ext", "rtc";
> 
> 		#clock-cells = <1>;
> 	};
> 
> 
> In arch/mips/boot/dts/ingenic/ci20.dts we have
> 
> &ext {
> 	clock-frequency = <48000000>;
> };
> 
> At last drivers/clk/ingenic/jz4780-cgu.c registers this "ext" clock
> as a parent of most other subordianate clocks. So there is no magic
> frequency constants in drivers/clk/ingenic!
> 
> In arch/mips/ath79/clocks.c we have a very different situation:
> the reference clock frequences are already hardcoded in C-code so there is
> no need to mention them in devicetree files.

No, we need to fix the code to only use the hard coded values for the
legacy platforms.

Alban
--
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