[PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Tomasz Figa]

On 12.01.2014 09:23, Lukasz Majewski wrote:
> Dear All,
>
>> On 11.01.2014 06:25, Thomas Abraham wrote:
>>> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski
>>> <l.majewski at samsung.com> wrote:
>>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>>>> <l.majewski at samsung.com> wrote:
[snip]
>>>>>>> +
>>>>>>> +static const struct samsung_core_clock_freq_table
>>>>>>> exyno4210_armclk_table = {
>>>>>>> +     .freq           = exynos4210_armclk_freqs,
>>>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>>>>>>> +     .data           = (void *)exynos4210_armclk_data,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw,
>>>>>>> unsigned long drate,
>>>>>>> +                                     unsigned long prate)
>>>>>>> +{
>>>>>>> +     struct samsung_core_clock *armclk;
>>>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>>>>>>> +     unsigned long *freq_data;
>>>>>>> +     unsigned long mux_reg, idx;
>>>>>>> +     void __iomem *base;
>>>>>>> +
>>>>>>> +     if (drate == prate)
>>>>>>> +             return 0;
>>>>>>> +
>>>>>>> +     armclk = container_of(hw, struct samsung_core_clock, hw);
>>>>>>> +     freq_tbl = armclk->freq_table;
>>>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
>>>>>>> +     base = armclk->ctrl_base;
>>>>>>> +
>>>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
>>>>>>> += 2)
>>>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>>>>>>> +                     break;
>>>>>>> +
>>>>>>> +     if (!armclk->fout_pll)
>>>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>>>>>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>>>>>>
>>>>>> I'm a bit confused here for two reasons. Please correct me if I'm
>>>>>> wrong.
>>>>>>
>>>>>> 1. You go into this ->set_rate() because of calling clk_set_rate
>>>>>> at "arm_clk" clock (numbered as 12 at clk-exynos4.c) at
>>>>>> cpufreq-cpu0.c
>>>>>>
>>>>>> In a Exynos4210 we have:
>>>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>>>>>> -> div_core2 -> arm_clk
>>>>>>
>>>>>> In the code you call directly the fout_apll which changes
>>>>>> frequency. Then the change shall be propagated to all registered
>>>>>> clocks.
>>>>>> I think, that DIV and DIV1 shall be reduced before PLL change
>>>>>> [*], to reflect the changes at CCF.
>>>>>
>>>>> The core clock implementation encapsulates multiple clock blocks
>>>>> (such as dividers and muxes) which are in between the output of
>>>>> the APLL and the point that actually is the cpu domain clock
>>>>> output.
>>>>
>>>> No problem with that. I mostly agree...
>>>>
>>>>> When a clock
>>>>> frequency change has to be made, all these clock blocks
>>>>> encapsulated within the core clock are programmed by
>>>>> pre-determined values.
>>>>
>>>> And what about the situation with already defined clocks (like
>>>> "div_core" and "div_core2"). Those will not be updated when you
>>>> first call clk_set_rate() and change by hand DIV and DIV1.
>>>>
>>>> What if you would like to have the PCLK_DBG clock used in the
>>>> future? You would add it to CCF and the change will not propagate.
>>>
>>> I did intend to remove individual clock blocks which are now
>>> encapsulated within the core clock type from the clock driver file.
>>> I missed doing that in this patch series.
>>
>> Yes, they should be removed, since they are encapsulated inside the
>> core clock now.
>
> This was not my point here.
>
> You are creating an opaque clock for cpu (like
> "samsung_clock_cpu" [*]) , which encapsulates many other clocks
> (mostly DIVs and MUXes to be more precise).
>
> I don't think, that those clocks shall be removed from CCF.
>
> I do believe, that they shall be passed as the argument to [*] and
> recalculated.
>
> Why the advent of [*] clock forbids me from creating and using clocks
> based on "div_core", "div_core2" and PCLK_DBG?
>
> What if I would like to see output of those particular clocks
> at /sys/kernel/debug/ or use them at a custom driver?

Having those clocks defined as separate entities in CCF would make them 
accessible outside the core clock, including changing their 
configuration, but they should be only changed on APLL change and 
atomically. By registering them as standard CCF clocks you are providing 
an interface to something that is not allowed.

If there was a _real_ need to have them defined as separate clocks, you 
would have to implement a separate clock type that would only allow 
retrieving the rate from it. Maybe a read-only flag for the divider 
clock could do, as mux clock already has such.

>
>>
>>>
>>>>
>>>>> This
>>>>> approach allows very fast clock speed switching, instead of
>>>>> traversing the entire CCF clock tree searching for individual
>>>>> clock blocks to be programmed.
>>>>
>>>> Those are mostly DIV and MUXes. Recalculation shouldn't be time
>>>> consuming.
>>>
>>> I was mainly referring to the time taken to search the clock tree
>>> for these individual clock blocks.
>>
>> Hmm, why couldn't you simply look-up all the needed clock at
>> initialization and keep references to them?
>
> +1
>
>>
>> Still, I think it is fine to directly program registers of
>> encapsulated clocks,
>
> And then recalculate values of relevant clocks build on top of those
> registers.
>
>> since they are not visible outside anymore and
>> there is no need for them to be visible.
>
> Why do you assume, that those clocks won't be needed anymore? For me
> clocks like [*] are valid clocks from CCF/user point of view.

I fail to see any use cases for them. Even now from 7 of core clock 
dividers of Exynos4210, in clk-exynos4.c only div_core and div_core2 are 
defined.

Then as I noted above, the values set to those internal blocks are 
completely tied to used APLL setting and you can't change them 
separately. Any potential use case would be limited to reading the 
frequency.

>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +     if (drate < prate) {
>>>>>>> +             mux_reg = readl(base + SRC_CPU);
>>>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>>>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) !=
>>>>>>> 2)
>>>>>>> +                     ;
>>>>>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>>>>>
>>>>>> 2. I think, the above shall be done in a following way:
>>>>>>
>>>>>>           clk_set_parent(mout_core, mout_mpll);
>>>>>>           clk_set_rate(armclk->fout_pll, drate);
>>>>>>           clk_set_parent(mout_core, mout_apll);
>>>>>>
>>>>>> The direct write to registers [**] doesn't look compliant to CCF.
>>>>>>
>>>>>
>>>>> As mentioned above, the clock block encapsulates these clock
>>>>> blocks into a single clock and only this single encapsulated
>>>>> clock is registered with CCF. The internal implementation of how
>>>>> the different clock blocks are managed within this clock is
>>>>> independent of the CCF.
>>>>
>>>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
>>>> (without CCF).
>>>>
>>>> But on the situation [**] the MUX can be changed by
>>>> clk_set_parent() as it is now done at exynosXXXX-cpufreq.c code.
>>>
>>> The mux is also encapsulated into a larger clock type and this new
>>> clock type know how the mux has to be configured.
>>
>> IMHO it's fine to encapsulate the mux as well. There are no users of
>> it other than the core clock.
>
> In spite of the above comment, it looks far more better to change clocks
> with CCF API (like clk_set_parent), not by writing directly to
> registers.

This also depends on whether other entities should be able to 
reconfigure the mux in question. If it needs to be switched only when 
the APLL is reconfigured by core clock, then I don't think there is a 
need to provide access to it outside.

Best regards,
Tomasz