Hi Magnus,
On Tue, Sep 1, 2015 at 12:48 PM, Magnus Damm <[email protected]> wrote:
> On Mon, Aug 31, 2015 at 10:03 PM, Geert Uytterhoeven
> <[email protected]> wrote:
>> On Mon, Aug 31, 2015 at 1:58 PM, Magnus Damm <[email protected]> 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 -- [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
