[RFC Patch v2 0/3] add temporary parent migration support

[Chander Kashyap]

On 6 September 2013 00:02, Mike Turquette <mturquette at linaro.org> wrote:
> On Wed, Sep 4, 2013 at 11:01 AM, Tomasz Figa <tomasz.figa at gmail.com> wrote:
>> On Wednesday 04 of September 2013 10:43:28 Mike Turquette wrote:
>>> Quoting Tomasz Figa (2013-09-03 15:36:50)
>>>
>>> > Hi Chander,
>>> >
>>> > On Tuesday 03 of September 2013 17:04:28 Chander Kashyap wrote:
>>> > > Some platform has provision to change cpu parent clock during
>>> > > cpu frequency scaling. This patch series provides a mechanism to
>>> > > implement the same using CCF.
>>> > >
>>> > > Patch1 provides mechanism to migrate to new parent temporarily.
>>> > >
>>> > > Patch2 updates the user of clk_register_mux and DEFINE_CLK_MUX which
>>> > > are modified to add support for clk migration.
>>> > >
>>> > > Patch3 adds support to Exynos5250 to use the clock parent migration
>>> > > feature implemented in CCF.
>>> >
>>> > I don't really like this approach. A need to change mux setting
>>> > temporarily is heavily platform-specific and I don't think it should
>>> > be
>>> > handled by generic code.
>>>
>>> I agree with Tomasz.
>>>
>>> > First of all there are many factor that you would
>>> >
>>> > have to account for to make this solution generic, such as:
>>> >  - board specific alternative parents,
>>> >  - exact moment of parent change,
>>> >  - some other platform specific conditions, like CPU voltage that must
>>> >  be>
>>> > changed when mux is changed, because it changes CPU frequency,
>>> >
>>> >  - and probably a lot of more factors that only people working with
>>> >  all
>>> >
>>> > the platforms supported (and unsupported yet) by Linux.
>>> >
>>> > I can see at least two solutions for this problem that don't require
>>> > changing core code of common clock framework:
>>> >
>>> > 1) Implementing a special clock type using normal mux ops, but also
>>> > registering a notifier for its PRE_RATE_CHANGE and POST_RATE_CHANGE
>>> > events to perform parent switching.
>>>
>>> Creating a custom clock type is the way to go here. It is possible to
>>> wrap the mux clk_ops to re-use that code, or just write a custom clock
>>> type from scratch.
>>>
>>> I do not like using the clock rate-change notifiers for this purpose.
>>> The notifiers provide hooks to drivers that need to take care around
>>> clock transitions. Using the notifiers from within the clock framework
>>> indicates poor design.
>>
>> I was not sure how a .set_parent() from inside a .set_rate() would
>> interact with rate setting, so I mentioned notifiers here, but now as I
>> think of it, CCF is supposed to be re-entrant, so things should be fine.
>>
>>> > 2) Using normal mux clock, but registering such notifiers in clock
>>> > controller or cpufreq driver.
>>>
>>> This depends on what the data sheet or reference manual states. If using
>>> a temporary parent is a property of the clock programming sequence
>>> (e.g. to have a glitch-less transition) then that logic belongs in the
>>> clock provider driver (i.e. a custom clock type needs to be created
>>> with this logic).
>>>
>>> However if using a temporary parent is not required for programming the
>>> clock, but is instead a requirement of the clock consumer (e.g. a CPU,
>>> or some I/O controller) then perhaps putting this logic in that driver
>>> is the right way to go. In that case the logic could be explicit:
>>>
>>>       clk_set_parent(clk, temp_parent);
>>>       clk_set_rate(clk, target_rate);
>>>       clk_set_parent(clk, target_parent);
>>>
>>> Or it could implicit with the use of rate-change notifiers. Again the
>>> rate-change notifiers exist for clock consumer drivers to use, so this
>>> is OK.
>>>
>>> I have a hunch that the right way to do this is for a custom clock type
>>> to be created which simply calls clk_set_parent from within the clock's
>>> .set_rate callback, but I'll wait on feedback from Chander on the needs
>>> of his platform.
>>
>> I believe Chander has exactly the same use case for this as we have for
>> Exynos 4210 and 4x12.
>>
>> On these SoCs, CPU frequency is being scaled by reconfiguring PLL, which
>> needs to be masked for locking time. To let the CPU operate normally, a
>> mux that allows switching CPU clock domain between two PLLs can be
>> switched to the other PLL (MPLL) until the main ARM PLL (APLL) starts
>> operating again.
>
> Right, this is the "glitchless" operation I mentioned earlier and it
> is not uncommon in PLLs.
>
>>
>> However this issue is not limited to clocks, because there must be also a
>> voltage transition involved if the utility PLL has frequency higher than
>> the one currently being reconfigured.
>
> Right, that is an altogether different issue. What I would like to see
> is the temporary parent managed by calls to clk_set_parent from within
> a custom .set_rate callback.
>
> As for the voltage scaling, it would be cool to see this working with
> the voltage notifier series I posted recently. Both parents of the
> PLLs can have their own operating point tables and each transition
> would fire off notifiers that scale voltage. Something like this:
>
> clk_set_rate(pll, rate)
> -> enter .set_rate
>   -> clk_set_parent(pll, temp_parent)
>     -> temp_parent PRE_RATE_CHANGE notifier is triggered, maybe scale voltage
>     -> .set_parent callback
>     -> temp_parent POST_RATE_CHANGE notifier is triggered, maybe scale voltage
>   -> pll PRE_RATE_CHANGE notifier is triggered, maybe scale voltage
>   -> .set_rate callback
>   -> pll POST_RATE_CHANGE notifier is triggered, maybe scale voltage
>   -> clk_set_parent(pll, original_parent)
>     -> original_parent PRE_RATE_CHANGE notifier is triggered, maybe
> scale voltage
>     -> .set_parent callback
>     -> original_parent POST_RATE_CHANGE notifier is triggered, maybe
> scale voltage
>
> There is some inefficiency in calling the notifiers three times, but
> the solution is generic.
>
> I'm sending this from the gmail web interface so hopefully the spacing
> is preserved...
>

Thanks Mike. Looking at the feedback it seems doing this in generic
way is not clean enough.
I will redesign it within platform and resend.

> Regards,
> Mike
>
>>
>> Best regards,
>> Tomasz
>>



-- 
with warm regards,
Chander Kashyap