Hi Mike,
On Tue, Aug 18, 2015 at 2:20 AM, Michael Turquette
<mturquette@xxxxxxxxxxxx> wrote:
> Quoting Geert Uytterhoeven (2015-08-04 05:34:06)
>> On Tue, Aug 4, 2015 at 2:22 PM, Laurent Pinchart
>> <laurent.pinchart@xxxxxxxxxxxxxxxx> 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.
> 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.
> 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?
> 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.
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.
>> >> + #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@xxxxxxxxxxxxxx
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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
