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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Wednesday 19 August 2015 09:49:28 Geert Uytterhoeven wrote:
> On Tue, Aug 18, 2015 at 2:20 AM, Michael Turquette wrote:
> > Quoting Geert Uytterhoeven (2015-08-04 05:34:06)
> >> On Tue, Aug 4, 2015 at 2:22 PM, Laurent Pinchart 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.
> 
> OK.

I agree too.

> > 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.
> 
> OK.

Ditto.

> > 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.
> 
> Does that mean #clock-cells is inherited from the parent (cpg) node, or does
> that preclude having any "fixed-factor-clock" or "renesas,cpg-div6-clock"
> (both use #clock-cells = <0>), or "renesas,cpg-mstp-clocks" subnodes?

We were thinking about moving the fixed factor clocks and the gate clocks as 
subnodes as the CPG DT node, as those clocks are provided by the CPG. This 
would require setting #clock-cells to 1 in the CPG node and to 0 in some of 
the subnodes. If that's not allowed, how should the fixed factor clocks and 
gate clocks be declared ?

> > 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.
> 
> Makes sense. I don't think anything relies on the "clock-output-names"
> ... currently.
> 
> I was going to use it for identifying the GIC clock, though:
> 
> --- a/drivers/clk/shmobile/clk-mstp.c
> +++ b/drivers/clk/shmobile/clk-mstp.c
> @@ -144,6 +144,9 @@ cpg_mstp_clock_register(const char *name, const char
> *paren init.name = name;
>         init.ops = &cpg_mstp_clock_ops;
>         init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
> +       /* INTC-SYS is the module clock of the GIC, and must not be disabled
> */
> +       if (!strcmp(name, "intc-sys"))
> +               init.flags |= CLK_ENABLE_HAND_OFF;
>         init.parent_names = &parent_name;
>         init.num_parents = 1;
> 
> (FTR, on R-Car Gen3 the clock is called "intc-ap", not "intc-sys", doh...).
> 
> This does put some reliance on (undocumented) naming in DT, though, so not
> having the "clock-output-names" would solve that.

Using the clock name is indeed very messy, let's avoid that.

> However, while "intc-sys"/"intc-ap" (if existent) is always MSTP408, MSTP408
> is used for other modules on R-Car Gen1 and most older SoCs. so it would
> complicate the driver code.

How about setting the flag based on information provided in DT ?

> >>>> +             #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";
> >>>> +             };
> >>>> +     };
> >>>> +};

-- 
Regards,

Laurent Pinchart