Re: [PATCH v4 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Magnus,

On Tue, Sep 1, 2015 at 12:48 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Mon, Aug 31, 2015 at 10:03 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, Aug 31, 2015 at 1:58 PM, Magnus Damm <magnus.damm@gmail.com> 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