Re: [PATCH 3/4 v3][RFC] arm64: renesas: Add initial r8a7795 SoC support

[Michael Turquette <mturquette@baylibre.com>]

Quoting Geert Uytterhoeven (2015-08-04 05:34:06)
> On Tue, Aug 4, 2015 at 2:22 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Monday 03 August 2015 01:53:23 Kuninori Morimoto wrote:
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> @@ -0,0 +1,93 @@
> >> +/ {
> 
> >> +     clocks {
> >
> > Let's try to make it right from the start on Gen3. The CPG node should be a
> > direct child of the bus node mentioned above, and the MSTP clocks should be
> > children of the CPG node.
> 
> Agreed.
> 
> > I'm not sure where to put the non-memory-mapped clocks though, should they be
> > directly under the root node ? It would make sense for extal_clk, but how
> > about the fixed-factor clocks ? Should they be children of the CPG node too ?
> 
> I believe the current trend is to put clocks like "extal_clk" under the root
> node.
> As the fixed-factor clocks are generated by the CPG module, and we do have
> a device node for it, I'd make them children of the CPG node, too.
> 
> Any comments from the clk+dt experts?

I don't know if anyone is an expert ;-)

extal_clk should be under the root node. That is true for all
board-level clocks and clock controllers.

Within the SoC we want to model the clock controller as a node in DT,
not necessarily all of the individual clocks. So you definitely need a
"cpg" node in DT with #clock-cells > 0.

Whether or not you enumerate the individual clocks in DT is up to you. I
do not like the data-driven approach of putting the clock definition
data into DT. It makes it awkward to do things like set a flag on a
single clock later on. Simply using the clock controller phandle plus
one or more offsets is preferred over a per-clock phandle.

Stephen and I have been discussing what a formal clock-controller
binding would look like, and one item we came up with is that any
sub-nodes of the controller would not be allowed to have a #clock-cells
property.

Also, while you're thinking about the perfect clock binding, please do
consider dropping clock-output-names if you can. Specifying clock-names
alongside the clocks property inside of the consumer node is a bit more
elegant in my opinion. This is also a bit easier if you think about
expressing your clock data with C inside of your provider driver.

Regards,
Mike

> 
> Thanks!
> 
> >> +             #address-cells = <2>;
> >> +             #size-cells = <2>;
> >> +             ranges;
> >> +
> >> +             extal_clk: extal_clk {
> >> +                     compatible = "fixed-clock";
> >> +                     #clock-cells = <0>;
> >> +                     clock-frequency = <0>;
> >> +                     clock-output-names = "extal";
> >> +             };
> >> +             cpg_clocks: cpg_clocks@e6150000 {
> >> +                     compatible = "renesas,r8a7795-cpg-clocks",
> >> +                                  "renesas,rcar-gen3-cpg-clocks";
> >> +                     reg = <0 0xe6150000 0 0x1000>;
> >> +                     clocks = <&extal_clk>;
> >> +                     #clock-cells = <1>;
> >> +                     clock-output-names = "main", "pll0", "pll1","pll2",
> >> +                                          "pll3", "pll4";
> >> +             };
> >> +             p_clk: p_clk {
> >> +                     compatible = "fixed-factor-clock";
> >> +                     clocks = <&cpg_clocks RCAR_GEN3_CLK_PLL1>;
> >> +                     #clock-cells = <0>;
> >> +                     clock-div = <24>;
> >> +                     clock-mult = <1>;
> >> +                     clock-output-names = "p";
> >> +             };
> >> +             mstp3_clks: mstp3_clks@e615013c {
> >> +                     compatible = "renesas,r8a7795-mstp-clocks",
> >> +                                  "renesas,cpg-mstp-clocks";
> >> +                     reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
> >> +                     clocks =  <&p_clk>;
> >> +                     #clock-cells = <1>;
> >> +                     renesas,clock-indices = <RCAR_GEN3_CLK_IRDA>;
> >> +                     clock-output-names = "irda";
> >> +             };
> >> +     };
> >> +};
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html