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

[Mike Turquette]

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...

Regards,
Mike

>
> Best regards,
> Tomasz
>