Re: [RFC] cpufreq: send notifications for intermediate (stable) frequencies

[Doug Anderson]

Viresh,

On Wed, May 14, 2014 at 10:56 PM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> Douglas Anderson, recently pointed out an interesting problem due to which his
> udelay() was expiring earlier than it should:
> https://lkml.org/lkml/2014/5/13/766
>
> While transitioning between frequencies few platforms may temporarily switch to
> a stable frequency, waiting for the main PLL to stabilize.
>
> For example: When we transition between very low frequencies on exynos, like
> between 200MHz and 300MHz, we may temporarily switch to a PLL running at 800MHz.
> No CPUFREQ notification is sent for that. That means there's a period of time
> when we're running at 800MHz but loops_per_jiffy is calibrated at between 200MHz
> and 300MHz. And so udelay behaves badly.
>
> To get this fixed in a generic way, lets introduce another callback safe_freq()
> for the cpufreq drivers.
>
> safe_freq() should return a stable intermediate frequency a platform might want
> to switch to, before jumping to the frequency corresponding to 'index'. Core
> will send the 'PRE' notification for this 'stable' frequency and 'POST' for the
> 'target' frequency. Though if ->target_index() fails, it will handle POST for
> 'stable' frequency only.
>
> Drivers must send 'POST' notification for 'stable' freq and 'PRE' for 'target'
> freq. If they can't switch to target frequency, they don't need to send any
> notification.

This will have the side effect of sending twice as many notifications.
 ...however it does allow for people registering for CPUFREQ
notifications to be more generic...

Thinking about it, I think you're right that this is the way to go.
The majority of the registrants of CPUFREQ that I see really ought to
be moved to common clock notifications (they are dealing with the fact
that a peripheral clock will get scaled as a side effect of CPUFREQ).
What's left is only a very small number of cases that would most
cleanly be dealt with by just seeing the extra notification.


> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> Doug/Stephen,
>
> If this doesn't look too ugly, then I would need patches from you to fix your
> platforms as I am not well aware of clk hierarchy of your platforms.

It probably makes sense to wait until Thomas Abraham's patch lands,
since he's redoing exynos cpufreq to use cpufreq-cpu0.  ...and maybe
Thomas would be willing to write this patch?


>  drivers/cpufreq/cpufreq.c | 13 +++++++++++--
>  include/linux/cpufreq.h   | 18 ++++++++++++++++++
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index a05c921..8d1cb4f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1874,11 +1874,17 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
>
>                 if (notify) {
>                         freqs.old = policy->cur;
> -                       freqs.new = freq_table[index].frequency;
> +                       /* Switch to some safe intermediate freq */
> +                       if (cpufreq_driver->safe_freq)

What do you think about calling this get_safe_freq().  It took me a
little while before I realized that this function didn't perform the
transition to the safe frequency--it just returned it.

...the comment adds extra confusion since it makes it sound like the
switch happens right here.


> +                               freqs.new = cpufreq_driver->safe_freq(policy,
> +                                                                     index);
> +                       else
> +                               freqs.new = freq_table[index].frequency;
>                         freqs.flags = 0;
>
>                         pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
> -                                __func__, policy->cpu, freqs.old, freqs.new);
> +                                __func__, policy->cpu, freqs.old,
> +                                freq_table[index].frequency);
>
>                         cpufreq_freq_transition_begin(policy, &freqs);
>                 }
> @@ -1887,6 +1893,9 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
>                 if (retval)
>                         pr_err("%s: Failed to change cpu frequency: %d\n",
>                                __func__, retval);
> +               else
> +                       /* Send POST notification for the target frequency */
> +                       freqs.new = freq_table[index].frequency;

Don't you need to set freqs.old to the safe_freq?

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/