[PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Viresh Kumar]

For some platforms, the frequency returned by hardware may be slightly
different from what is provided in the frequency table. For example,
hardware may return 499 MHz instead of 500 MHz. In such cases it is
better to avoid getting into unnecessary frequency updates, as we may
end up switching policy->cur between the two and sending unnecessary
pre/post update notifications, etc.

This patch has chosen allows the hardware frequency and table frequency
to deviate by 1 MHz for now, we may want to increase it a bit later on
if someone still complains.

Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0d58b0f8f3af..233e8af48848 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -28,6 +28,7 @@
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
 #include <linux/tick.h>
+#include <linux/units.h>
 #include <trace/events/power.h>
 
 static LIST_HEAD(cpufreq_policy_list);
@@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
 		return new_freq;
 
 	if (policy->cur != new_freq) {
+		/*
+		 * For some platforms, the frequency returned by hardware may be
+		 * slightly different from what is provided in the frequency
+		 * table, for example hardware may return 499 MHz instead of 500
+		 * MHz. In such cases it is better to avoid getting into
+		 * unnecessary frequency updates.
+		 */
+		if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
+			return policy->cur;
+
 		cpufreq_out_of_sync(policy, new_freq);
 		if (update)
 			schedule_work(&policy->update);
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Rex-BC Chen]

On Wed, 2022-05-04 at 13:51 +0530, Viresh Kumar wrote:
> For some platforms, the frequency returned by hardware may be
> slightly
> different from what is provided in the frequency table. For example,
> hardware may return 499 MHz instead of 500 MHz. In such cases it is
> better to avoid getting into unnecessary frequency updates, as we may
> end up switching policy->cur between the two and sending unnecessary
> pre/post update notifications, etc.
> 
> This patch has chosen allows the hardware frequency and table
> frequency
> to deviate by 1 MHz for now, we may want to increase it a bit later
> on
> if someone still complains.
> 
> Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> 

Hello Viresh,

Thanks for your help!
Jia-wei verified this patch in his device, and I help him to add this.

Tested-by: Jia-wei Chang <jia-wei.chang@mediatek.com>

BRs,
Rex

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Vincent Guittot]

On Wed, 4 May 2022 at 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> For some platforms, the frequency returned by hardware may be slightly
> different from what is provided in the frequency table. For example,

Do you have more details ?

Do you mean that between 2 consecutives reads you can get either
500Mhz or 499Mhz ?

Or is it a fixed mismatch between the table and the freq returned by HW ?

> hardware may return 499 MHz instead of 500 MHz. In such cases it is
> better to avoid getting into unnecessary frequency updates, as we may
> end up switching policy->cur between the two and sending unnecessary
> pre/post update notifications, etc.
>
> This patch has chosen allows the hardware frequency and table frequency
> to deviate by 1 MHz for now, we may want to increase it a bit later on
> if someone still complains.
>
> Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 0d58b0f8f3af..233e8af48848 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -28,6 +28,7 @@
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/tick.h>
> +#include <linux/units.h>
>  #include <trace/events/power.h>
>
>  static LIST_HEAD(cpufreq_policy_list);
> @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>                 return new_freq;
>
>         if (policy->cur != new_freq) {
> +               /*
> +                * For some platforms, the frequency returned by hardware may be
> +                * slightly different from what is provided in the frequency
> +                * table, for example hardware may return 499 MHz instead of 500
> +                * MHz. In such cases it is better to avoid getting into
> +                * unnecessary frequency updates.
> +                */
> +               if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
> +                       return policy->cur;
> +
>                 cpufreq_out_of_sync(policy, new_freq);
>                 if (update)
>                         schedule_work(&policy->update);
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Viresh Kumar]

On 05-05-22, 09:28, Vincent Guittot wrote:
> On Wed, 4 May 2022 at 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > For some platforms, the frequency returned by hardware may be slightly
> > different from what is provided in the frequency table. For example,
> 
> Do you have more details ?

This is where the problem was discussed.

https://lore.kernel.org/lkml/20220422075239.16437-8-rex-bc.chen@mediatek.com/

> Do you mean that between 2 consecutives reads you can get either
> 500Mhz or 499Mhz ?

No, the hardware always returns something like 499,999,726 Hz, but the
OPP table contains the value 500 MHz. The field policy->cur is set
based on opp table eventually (target_index) and so contains 500MHz,
almost always. But when cpufreq_get() is called, it finds the current
freq is 499 MHz, instead of 500 MHz. And so the issue.

> Or is it a fixed mismatch between the table and the freq returned by HW ?

Yes.

-- 
viresh

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Vincent Guittot]

On Thu, 5 May 2022 at 09:44, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 05-05-22, 09:28, Vincent Guittot wrote:
> > On Wed, 4 May 2022 at 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > For some platforms, the frequency returned by hardware may be slightly
> > > different from what is provided in the frequency table. For example,
> >
> > Do you have more details ?
>
> This is where the problem was discussed.
>
> https://lore.kernel.org/lkml/20220422075239.16437-8-rex-bc.chen@mediatek.com/

Thanks for the link

>
> > Do you mean that between 2 consecutives reads you can get either
> > 500Mhz or 499Mhz ?
>
> No, the hardware always returns something like 499,999,726 Hz, but the

Part of your problem is that cpufreq use khz whereas clock uses hz

Would it be better to do something like below in cpufreq_generic_get

(clk_get_rate(policy->clk) + 500) / 1000

so you round to closest instead of always floor rounding


> OPP table contains the value 500 MHz. The field policy->cur is set
> based on opp table eventually (target_index) and so contains 500MHz,
> almost always. But when cpufreq_get() is called, it finds the current
> freq is 499 MHz, instead of 500 MHz. And so the issue.
>
> > Or is it a fixed mismatch between the table and the freq returned by HW ?
>
> Yes.
>
> --
> viresh

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Viresh Kumar]

On 05-05-22, 10:21, Vincent Guittot wrote:
> Part of your problem is that cpufreq use khz whereas clock uses hz

Not in this case at least as the value mentioned in OPP table DT is in
Hz.

> Would it be better to do something like below in cpufreq_generic_get
> 
> (clk_get_rate(policy->clk) + 500) / 1000
> 
> so you round to closest instead of always floor rounding

That would be a fine thing to do anyway, though I am not sure if it
will fix the problem at hand.

If the hardware returns 499,999,499 Hz, we will still have the
problem.

-- 
viresh

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Vincent Guittot]

On Thu, 5 May 2022 at 10:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 05-05-22, 10:21, Vincent Guittot wrote:
> > Part of your problem is that cpufreq use khz whereas clock uses hz
>
> Not in this case at least as the value mentioned in OPP table DT is in
> Hz.

But dev_pm_opp_init_cpufreq_table make it kHz anyway

>
> > Would it be better to do something like below in cpufreq_generic_get
> >
> > (clk_get_rate(policy->clk) + 500) / 1000
> >
> > so you round to closest instead of always floor rounding
>
> That would be a fine thing to do anyway, though I am not sure if it
> will fix the problem at hand.
>
> If the hardware returns 499,999,499 Hz, we will still have the
> problem.

But in this case, cpufreq table should use 499,999Khz IMO. We already
have OPP/cpufreq table being updated at boot with actual value.

>
> --
> viresh

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Viresh Kumar]

On 05-05-22, 11:40, Vincent Guittot wrote:
> On Thu, 5 May 2022 at 10:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 05-05-22, 10:21, Vincent Guittot wrote:
> > > Part of your problem is that cpufreq use khz whereas clock uses hz
> >
> > Not in this case at least as the value mentioned in OPP table DT is in
> > Hz.
> 
> But dev_pm_opp_init_cpufreq_table make it kHz anyway

Yes.

> > > Would it be better to do something like below in cpufreq_generic_get
> > >
> > > (clk_get_rate(policy->clk) + 500) / 1000
> > >
> > > so you round to closest instead of always floor rounding
> >
> > That would be a fine thing to do anyway, though I am not sure if it
> > will fix the problem at hand.
> >
> > If the hardware returns 499,999,499 Hz, we will still have the
> > problem.
> 
> But in this case, cpufreq table should use 499,999Khz IMO.

I did think about it earlier, but then left it.

> We already
> have OPP/cpufreq table being updated at boot with actual value.

I don't think we update the frequency values there yet, but yes one
way to fix it is via DT.

-- 
viresh

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Matthias Brugger]

On 04/05/2022 10:21, Viresh Kumar wrote:
> For some platforms, the frequency returned by hardware may be slightly
> different from what is provided in the frequency table. For example,
> hardware may return 499 MHz instead of 500 MHz. In such cases it is
> better to avoid getting into unnecessary frequency updates, as we may
> end up switching policy->cur between the two and sending unnecessary
> pre/post update notifications, etc.
> 
> This patch has chosen allows the hardware frequency and table frequency
> to deviate by 1 MHz for now, we may want to increase it a bit later on
> if someone still complains.
> 
> Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/cpufreq/cpufreq.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 0d58b0f8f3af..233e8af48848 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -28,6 +28,7 @@
>   #include <linux/suspend.h>
>   #include <linux/syscore_ops.h>
>   #include <linux/tick.h>
> +#include <linux/units.h>
>   #include <trace/events/power.h>
>   
>   static LIST_HEAD(cpufreq_policy_list);
> @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>   		return new_freq;
>   
>   	if (policy->cur != new_freq) {
> +		/*
> +		 * For some platforms, the frequency returned by hardware may be
> +		 * slightly different from what is provided in the frequency
> +		 * table, for example hardware may return 499 MHz instead of 500
> +		 * MHz. In such cases it is better to avoid getting into
> +		 * unnecessary frequency updates.
> +		 */
> +		if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
> +			return policy->cur;
> +
>   		cpufreq_out_of_sync(policy, new_freq);
>   		if (update)
>   			schedule_work(&policy->update);

Re: [PATCH] cpufreq: Avoid unnecessary frequency updates due to mismatch

[Rafael J. Wysocki]

On Thu, May 5, 2022 at 3:32 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>
>
>
> On 04/05/2022 10:21, Viresh Kumar wrote:
> > For some platforms, the frequency returned by hardware may be slightly
> > different from what is provided in the frequency table. For example,
> > hardware may return 499 MHz instead of 500 MHz. In such cases it is
> > better to avoid getting into unnecessary frequency updates, as we may
> > end up switching policy->cur between the two and sending unnecessary
> > pre/post update notifications, etc.
> >
> > This patch has chosen allows the hardware frequency and table frequency
> > to deviate by 1 MHz for now, we may want to increase it a bit later on
> > if someone still complains.
> >
> > Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>
> > ---
> >   drivers/cpufreq/cpufreq.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 0d58b0f8f3af..233e8af48848 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -28,6 +28,7 @@
> >   #include <linux/suspend.h>
> >   #include <linux/syscore_ops.h>
> >   #include <linux/tick.h>
> > +#include <linux/units.h>
> >   #include <trace/events/power.h>
> >
> >   static LIST_HEAD(cpufreq_policy_list);
> > @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
> >               return new_freq;
> >
> >       if (policy->cur != new_freq) {
> > +             /*
> > +              * For some platforms, the frequency returned by hardware may be
> > +              * slightly different from what is provided in the frequency
> > +              * table, for example hardware may return 499 MHz instead of 500
> > +              * MHz. In such cases it is better to avoid getting into
> > +              * unnecessary frequency updates.
> > +              */
> > +             if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
> > +                     return policy->cur;
> > +
> >               cpufreq_out_of_sync(policy, new_freq);
> >               if (update)
> >                       schedule_work(&policy->update);

Applied as 5.19 material, thanks!