From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geert Uytterhoeven Date: Tue, 01 Sep 2015 11:33:18 +0000 Subject: Re: [PATCH v4 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support Message-Id: List-Id: References: <20150829091323.28546.28287.sendpatchset@little-apple> <20150829091346.28546.42552.sendpatchset@little-apple> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Magnus Damm Cc: linux-clk , Kuninori Morimoto , Gaku Inami , Michael Turquette , Linux-sh list , Stephen Boyd , Simon Horman , Laurent Pinchart Hi Magnus, On Tue, Sep 1, 2015 at 12:48 PM, Magnus Damm wrote: > On Mon, Aug 31, 2015 at 10:03 PM, Geert Uytterhoeven > wrote: >> On Mon, Aug 31, 2015 at 1:58 PM, Magnus Damm wrote: >>>>> --- /dev/null >>>>> +++ work/drivers/clk/shmobile/clk-rcar-gen3.c 2015-08-29 16:30:59.032366518 +0900 >>>>> @@ -0,0 +1,249 @@ >>>> >>>>> +#define RCAR_GEN3_CLK_MAIN 0 >>>>> +#define RCAR_GEN3_CLK_PLL0 1 >>>>> +#define RCAR_GEN3_CLK_PLL1 2 >>>>> +#define RCAR_GEN3_CLK_PLL2 3 >>>>> +#define RCAR_GEN3_CLK_PLL3 4 >>>>> +#define RCAR_GEN3_CLK_PLL4 5 >>>> >>>> These values are intimately tied to R8A7795_CLK_* in >>>> include/dt-bindings/clock/r8a7795-clock.h. >>>> Perhaps we can use these instead? >>> >>> You are right that they are connected together, but I'm not sure if I >>> prefer to include that file. If we start sharing files between the CPG >>> and the integration code then a dependency hell breaks loose when it >>> comes to merge order. If we want to share the #defines then I propose >>> that we handle that incrementally once all pieces are merged. >> >> OK, we can move them to include/dt-bindings/clock/rcar-gen3-clock.h >> later. > > So in the future we may then want to change r8a7795-clock.h into: > > #define R8A7795_CLK_MAIN RCAR_GEN3_CLK_MAIN > > ? Exactly. >>> On R-Car Gen2 we have a single CPG driver that is potentially shared >>> across multiple SoCs in the same generation / family. In that case we >>> use strings and the clock-output-names as part of the ABI if I'm not >>> mistaken. On this driver we use clock index numbers instead, so I'd >>> like to see these defines as part of a shared DT binding ABI where new >>> SoCs can simply add new clocks at the end. >> >> OK, so rcar_gen3_cpg.clks[] is really intended to be variable-sized. >> >> Hence I suggest to write it that way: >> >> struct rcar_gen3_cpg { >> struct clk_onecell_data data; >> spinlock_t lock; >> void __iomem *reg; >> struct clk *clks[0]; >> }; >> >> Then you can allocate it with >> >> kzalloc(sizeof(*cpg) + num_clks * sizeof(rcar_gen3_cpg.clks[0], ...) >> >> and kill RCAR_GEN3_CLK_NR (one less definition that can go out of sync). > > I see RCAR_GEN3_CLK_NR as the number of different clocks that the Gen3 > CPG driver can support. Exactly which one that the SoC specific DTS > wants to use depends on each SoC. So for future SoCs we may want to > keep on adding new clocks at the end. And the clock-indices list may > be sparsely populated in DTS. OK. And it's just a few pointers (initially I thought it contained full clk structs). > I am not 100% sure about how to handle error cases. Say a new DTS is > booted on an old kernel then some clocks may not be supported by the > CPG driver. For those cases the switch/case statement in the driver > will simply return errors for the unsupported clocks. And the rest of > the kernel has to figure out how to deal with that. That seems sane, > right? Indeed. Those clocks will not exist, and any driver trying to use them will fail to clk_get() them. Note that fixed-factor-clocks pointing to them will still be instantiated, with a zero parent rate. 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