[PATCH v2 1/5] clk: samsung: add common clock framework support for Samsung platforms

[Thomas Abraham]

Hi Tomasz,

On 31 October 2012 05:02, Tomasz Figa <tomasz.figa at gmail.com> wrote:
> Hi Thomas, Sylwester,
>
> On Wednesday 31 of October 2012 00:10:24 Sylwester Nawrocki wrote:
>> >>> +/* register a samsung pll type clock */
>> >>> +void __init samsung_clk_register_pll(const char *name, const char
>> >>> **pnames, +                             struct device_node *np,
>> >>> +                             int (*set_rate)(unsigned long rate),
>> >>> +                             unsigned long (*get_rate)(unsigned long
>> >>> rate)) +{
>> >>> +     struct samsung_pll_clock *clk_pll;
>> >>> +     struct clk *clk;
>> >>> +     struct clk_init_data init;
>> >>> +     int ret;
>> >>> +
>> >>> +     clk_pll = kzalloc(sizeof(*clk_pll), GFP_KERNEL);
>> >>> +     if (!clk_pll) {
>> >>> +             pr_err("%s: could not allocate pll clk %s\n", __func__,
>> >>> name); +             return;
>> >>> +     }
>> >>> +
>> >>> +     init.name = name;
>> >>> +     init.ops =&samsung_pll_clock_ops;
>> >>> +     init.flags = CLK_GET_RATE_NOCACHE;
>> >>> +     init.parent_names = pnames;
>> >>> +     init.num_parents = 1;
>> >>> +
>> >>> +     clk_pll->set_rate = set_rate;
>> >>> +     clk_pll->get_rate = get_rate;
>> >>> +     clk_pll->hw.init =&init;
>> >>> +
>> >>> +     /* register the clock */
>> >>> +     clk = clk_register(NULL,&clk_pll->hw);
>> >>> +     if (IS_ERR(clk)) {
>> >>> +             pr_err("%s: failed to register pll clock %s\n",
>> >>> __func__,
>> >>> +                             name);
>> >>> +             kfree(clk_pll);
>> >>> +             return;
>> >>> +     }
>> >>> +
>> >>> +#ifdef CONFIG_OF
>> >>> +     if (np)
>> >>> +             of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> >>> +#endif
>> >>
>> >> Is it really required to do clk_register() and of_clk_add_provider()
>> >> for
>> >> each single clock ? This seems more heavy than it could be. Looking at
>> >
>> > of_clk_add_provider call for every clock instance is not really
>> > required but it does allow platform code to lookup the clock and
>> > retrieve/display the clock speed. That was the intention to add a
>> > lookup for all the clocks.
>
> I'm not really sure if displaying clock speed is really a good
> justification for increasing the list of system clocks almost by a factor
> of three. This will make clock lookup a bit heavy.
>
> You might display speed of several most important clocks directly from the
> samsung clk driver using internal data, without the need of involving
> generic clock lookup for this purpose.

Ok. But that would mean that we implment the clk_get_rate() function
for such clocks again. For example, to get the rate of the aclk_200
clock, its parent clock would have to be divided and then displayed.
But it was easier to just use the clk_get api. But yes I agree, this
is not a compelling reason to register all the clocks.

>
>>
>> Hmm, do you mean calling clk_get() with NULL 'dev' argument ?
>> It's probably not a big deal to look up the clocks root node and
>> then use of_clk_get() function to find required clock ?
>
> I believe that the intention was for it to work on non-DT platforms as
> well. However I might have misunderstood your suggestion, could you
> elaborate?
>
>> >> drivers/clk/mxs/clk-imx28.c, it registers only single clock provider
>> >> for
>> >> whole group of clocks. Also, couldn't we statically define most of the
>> >> clocks and still register them so they can be used with platforms
>> >> using
>> >> FDT ? Something along the lines of imx28 implementation
>> >> (arch/arm/boot/dts /imx28.dtsi), where a clock is specified at
>> >> consumer device node by a phandle to the clock controller node and a
>> >> clock index ?
>> >
>> > We could do it that way. I was tempted to list out all the clocks in
>> > device tree and then register them so that there is no static
>> > definition of the clocks needed. You seem to prefer not to do that. I
>> > am fine with either way, static or device tree based registration.
>>
>> Ok, it's also worth noting that clk_get() would have been more expensive
>> when there would be a need to iterate over large number of clock
>> providers. Indexed look up might be a better alternative.
>
> I'm definitely for indexed lookup. With the ability to define constants in
> device tree sources the main drawback of this solution being less readable
> now disappeared and everything left are advantages.

Ok.

>
>> Exynos SoCs have fairly complex clock tree structure, I personally do
>> find it harder to understand from a bit bulky description in form of a
>> device tree node list. Comparing to the original Samsung clock API there
>> is now more clock objects, since, e.g. single struct clk_clksrc is now
>> represented by mux, div and gate clock group, which makes things
>> slightly more complicated, i.e. there is even more clocks to be listed.
>
> If it's about readability I tend to disagree. I find device tree much more
> readable as a way of describing hardware than hardcoded data structures,
> often using complex macros to keep the definitions short.
>
>> >> Besides that, what bothers me with in the current approach is the
>> >> clock consumers being defined through one big data structure together
>> >> with the actual clocks. Not all clock objects are going to have
>> >> consumers, some resources are waisted by using flat tables of those
>> >> big data structure objects. Perhaps we could use two tables, one for
>> >> the
>> >> platform clocks and one for the consumers ? These common clock driver
>> >> is intended to cover all Samsung SoC, I would expect all samsung
>> >> sub-archs getting converted to use it eventually, with as many of them
>> >> as possible then reworked to support device tree. It's a lot of work
>> >> and is going to take some time, but it would be good to have it
>> >> planned
>> >> in advance. That said I'm not sure the common samsung clock driver in
>> >> non-dt variant would be really a temporary thing.
>> >
>> > Non-dt support in Samsung common clock driver will be maintained. But
>> > for existing Exynos4 non-dt platforms, it should be possible to
>> > convert them to completely device tree based platforms.
>>
>> OK, let's then focus on device tree support for exynos4+ SoCs. I hope we
>> could have the clocks statically defined and still freely accessible in
>> the device tree.
>
> Using the approach with indexed clocks inside a single provider would allow
> to reuse the same internal SoC-specific data for both DT and non-DT
> variants, without any data duplication. This is definitely an advantage.

Ok.

>
>> >>> +
>> >>> +#ifdef CONFIG_OF
>> >>> +/* register a samsung gate type clock instantiated from device tree
>> >>> */
>> >>> +void __init samsung_of_clk_register_gate(struct device_node *np)
>> >>> +{
>> >>> +     struct samsung_gate_clock clk_gate;
>> >>> +     const char *clk_name = np->name;
>> >>> +     const char *parent_name;
>> >>> +     u32 reg_info[2];
>> >>> +
>> >>> +     of_property_read_string(np, "clock-output-names",&clk_name);
>> >>> +     parent_name = of_clk_get_parent_name(np, 0);
>> >>> +     if (of_property_read_u32_array(np, "reg-info", reg_info, 2))
>> >>> +             pr_err("%s: invalid register info in node\n",
>> >>> __func__);
>> >>> +
>> >>> +     clk_gate.name = clk_name;
>> >>> +     clk_gate.parent_name = parent_name;
>> >>> +     clk_gate.reg = (void __iomem *)(reg_base + reg_info[0]);
>> >>> +     clk_gate.bit_idx = reg_info[1];
>> >>> +     clk_gate.dev_name = NULL;
>> >>> +     clk_gate.flags = 0;
>> >>> +     clk_gate.gate_flags = 0;
>> >>
>> >> Some clocks need CLK_SET_RATE_PARENT for the drivers to work
>> >> as before. So far it is not set for any mux, div nor gate clock.
>> >
>> > Ok. I will fix this.
>> >
>> > So about the static vs device tree based clock registration, what
>> > would you suggest?
>>
>> Defining the clocks statically has my preference, it would be nice to
>> hear more opinions on that though. I think on a heavily utilised SoC
>> the list of clock nodes could have grown significantly. And with
>> preprocessor support at the dt compiler (not sure if it is already
>> there) indexed clock definitions could be made more explicit.
>>
>> These are roughly our conclusions from discussing this patch series
>> with Tomasz F.
>
> Yes, as I said, I'm definitely for the single clock provider approach (aka
> imx-like approach).

Ok. Thanks for your comments. I will redo this patch series.

Thanks,
Thomas.

>
> Best regards,
> Tomasz Figa
>