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 shoul=
d be
> > children of the CPG node.
> =

> Agreed.
> =

> > I'm not sure where to put the non-memory-mapped clocks though, should t=
hey be
> > directly under the root node ? It would make sense for extal_clk, but h=
ow
> > 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 r=
oot
> 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 =3D <2>;
> >> +             #size-cells =3D <2>;
> >> +             ranges;
> >> +
> >> +             extal_clk: extal_clk {
> >> +                     compatible =3D "fixed-clock";
> >> +                     #clock-cells =3D <0>;
> >> +                     clock-frequency =3D <0>;
> >> +                     clock-output-names =3D "extal";
> >> +             };
> >> +             cpg_clocks: cpg_clocks@e6150000 {
> >> +                     compatible =3D "renesas,r8a7795-cpg-clocks",
> >> +                                  "renesas,rcar-gen3-cpg-clocks";
> >> +                     reg =3D <0 0xe6150000 0 0x1000>;
> >> +                     clocks =3D <&extal_clk>;
> >> +                     #clock-cells =3D <1>;
> >> +                     clock-output-names =3D "main", "pll0", "pll1","p=
ll2",
> >> +                                          "pll3", "pll4";
> >> +             };
> >> +             p_clk: p_clk {
> >> +                     compatible =3D "fixed-factor-clock";
> >> +                     clocks =3D <&cpg_clocks RCAR_GEN3_CLK_PLL1>;
> >> +                     #clock-cells =3D <0>;
> >> +                     clock-div =3D <24>;
> >> +                     clock-mult =3D <1>;
> >> +                     clock-output-names =3D "p";
> >> +             };
> >> +             mstp3_clks: mstp3_clks@e615013c {
> >> +                     compatible =3D "renesas,r8a7795-mstp-clocks",
> >> +                                  "renesas,cpg-mstp-clocks";
> >> +                     reg =3D <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
> >> +                     clocks =3D  <&p_clk>;
> >> +                     #clock-cells =3D <1>;
> >> +                     renesas,clock-indices =3D <RCAR_GEN3_CLK_IRDA>;
> >> +                     clock-output-names =3D "irda";
> >> +             };
> >> +     };
> >> +};
> =

> Gr{oetje,eeting}s,
> =

>                         Geert
> =

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m6=
8k.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