[RFC V2 0/6] cpufreq: transition-latency cleanups

[RFC V2 0/6] cpufreq: transition-latency cleanups

[Viresh Kumar]

Hi Rafael,

Here is the V2 and sending it as RFC this time.

This series tries to cleanup the code around transition-latency and its
users.  Some of the old legacy code, which may not make much sense now,
is dropped as well.

Some code consolidation also done across governors.

Based of: pm/linux-next
Tested on: ARM64 Hikey board.

V1->V2:
- While we still get rid of the limitation of 10ms for using
  ondemand/conservative, but we preserve the earlier behavior where the
  transition latency set to CPUFREQ_ETERNAL would not allow use of
  ondemand/conservative governors. Thanks to Dominik for his feedback on
  that.

--
viresh

Viresh Kumar (6):
  cpufreq: Replace "max_transition_latency" with "dynamic_switching"
  cpufreq: schedutil: Set dynamic_switching to true
  cpufreq: governor: Drop min_sampling_rate
  cpufreq: Use transition_delay_us for legacy governors as well
  cpufreq: Cap the default transition delay value to 10 ms
  cpufreq: arm_big_little: Make ->get_transition_latency() mandatory

 Documentation/admin-guide/pm/cpufreq.rst |  8 -------
 drivers/cpufreq/arm_big_little.c         | 10 ++++-----
 drivers/cpufreq/cpufreq.c                |  8 +++----
 drivers/cpufreq/cpufreq_conservative.c   |  6 ------
 drivers/cpufreq/cpufreq_governor.c       | 17 ++-------------
 drivers/cpufreq/cpufreq_governor.h       |  3 +--
 drivers/cpufreq/cpufreq_ondemand.c       | 12 -----------
 include/linux/cpufreq.h                  | 36 ++++++++++++++++++++++++--------
 kernel/sched/cpufreq_schedutil.c         | 12 ++---------
 9 files changed, 40 insertions(+), 72 deletions(-)

-- 
2.13.0.71.gd7076ec9c9cb

[RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Viresh Kumar]

There is no limitation in the ondemand or conservative governors which
disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic
dynamic frequency switching for platforms which didn't wanted these
governors to run.

Replace max_transition_latency with a boolean (dynamic_switching) and
check for transition_latency == CPUFREQ_ETERNAL along with that. This
makes it pretty straight forward to read/understand now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c          | 8 ++++----
 drivers/cpufreq/cpufreq_governor.h | 2 +-
 include/linux/cpufreq.h            | 9 ++-------
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9bf97a366029..dcef293c5e2c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1988,13 +1988,13 @@ static int cpufreq_init_governor(struct cpufreq_policy *policy)
 	if (!policy->governor)
 		return -EINVAL;
 
-	if (policy->governor->max_transition_latency &&
-	    policy->cpuinfo.transition_latency >
-	    policy->governor->max_transition_latency) {
+	/* Platform doesn't want dynamic frequency switching ? */
+	if (policy->governor->dynamic_switching &&
+	    policy->cpuinfo.transition_latency == CPUFREQ_ETERNAL) {
 		struct cpufreq_governor *gov = cpufreq_fallback_governor();
 
 		if (gov) {
-			pr_warn("%s governor failed, too long transition latency of HW, fallback to %s governor\n",
+			pr_warn("Transition latency set to CPUFREQ_ETERNAL, can't use %s governor. Fallback to %s governor\n",
 				policy->governor->name, gov->name);
 			policy->governor = gov;
 		} else {
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 0236ec2cd654..7b7839c45fba 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -160,7 +160,7 @@ void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy);
 #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)			\
 	{								\
 		.name = _name_,						\
-		.max_transition_latency	= TRANSITION_LATENCY_LIMIT,	\
+		.dynamic_switching = true,				\
 		.owner = THIS_MODULE,					\
 		.init = cpufreq_dbs_governor_init,			\
 		.exit = cpufreq_dbs_governor_exit,			\
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 905117bd5012..3d8c52b3b5df 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -487,14 +487,10 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div,
  * polling frequency is 1000 times the transition latency of the processor. The
  * ondemand governor will work on any processor with transition latency <= 10ms,
  * using appropriate sampling rate.
- *
- * For CPUs with transition latency > 10ms (mostly drivers with CPUFREQ_ETERNAL)
- * the ondemand governor will not work. All times here are in us (microseconds).
  */
 #define MIN_SAMPLING_RATE_RATIO		(2)
 #define LATENCY_MULTIPLIER		(1000)
 #define MIN_LATENCY_MULTIPLIER		(20)
-#define TRANSITION_LATENCY_LIMIT	(10 * 1000 * 1000)
 
 struct cpufreq_governor {
 	char	name[CPUFREQ_NAME_LEN];
@@ -507,9 +503,8 @@ struct cpufreq_governor {
 					 char *buf);
 	int	(*store_setspeed)	(struct cpufreq_policy *policy,
 					 unsigned int freq);
-	unsigned int max_transition_latency; /* HW must be able to switch to
-			next freq faster than this value in nano secs or we
-			will fallback to performance governor */
+	/* For governors which change frequency dynamically by themselves */
+	bool			dynamic_switching;
 	struct list_head	governor_list;
 	struct module		*owner;
 };
-- 
2.13.0.71.gd7076ec9c9cb

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Rafael J. Wysocki]

On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> There is no limitation in the ondemand or conservative governors which
> disallow the transition_latency to be greater than 10 ms.
>
> The max_transition_latency field is rather used to disallow automatic
> dynamic frequency switching for platforms which didn't wanted these
> governors to run.
>
> Replace max_transition_latency with a boolean (dynamic_switching) and
> check for transition_latency == CPUFREQ_ETERNAL along with that. This
> makes it pretty straight forward to read/understand now.

Well, using CPUFREQ_ETERNAL for that on the driver side is still not
particularly straightforward IMO, so maybe add a
"no_dynamic_switching" to the driver structure and set it to "true"
for the one driver in question?

Thanks,
Rafael

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Dominik Brodowski]

On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > There is no limitation in the ondemand or conservative governors which
> > disallow the transition_latency to be greater than 10 ms.
> >
> > The max_transition_latency field is rather used to disallow automatic
> > dynamic frequency switching for platforms which didn't wanted these
> > governors to run.
> >
> > Replace max_transition_latency with a boolean (dynamic_switching) and
> > check for transition_latency == CPUFREQ_ETERNAL along with that. This
> > makes it pretty straight forward to read/understand now.
> 
> Well, using CPUFREQ_ETERNAL for that on the driver side is still not
> particularly straightforward IMO, so maybe add a
> "no_dynamic_switching" to the driver structure and set it to "true"
> for the one driver in question?

IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and
where dynamic switching might be harmful or at least lead to undefined
behavior.

Best,
	Dominik

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Rafael J. Wysocki]

On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
> On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote:
>> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > There is no limitation in the ondemand or conservative governors which
>> > disallow the transition_latency to be greater than 10 ms.
>> >
>> > The max_transition_latency field is rather used to disallow automatic
>> > dynamic frequency switching for platforms which didn't wanted these
>> > governors to run.
>> >
>> > Replace max_transition_latency with a boolean (dynamic_switching) and
>> > check for transition_latency == CPUFREQ_ETERNAL along with that. This
>> > makes it pretty straight forward to read/understand now.
>>
>> Well, using CPUFREQ_ETERNAL for that on the driver side is still not
>> particularly straightforward IMO, so maybe add a
>> "no_dynamic_switching" to the driver structure and set it to "true"
>> for the one driver in question?
>
> IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and
> where dynamic switching might be harmful or at least lead to undefined
> behavior.

OK

Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic
switching" condition is somewhat convoluted, so why don't we have a
flag to *explicitly* say that instead?

Do you know which drivers they are or is it just all drivers that use
CPUFREQ_ETERNAL?

Thanks,
Rafael

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Rafael J. Wysocki]

On Friday, July 14, 2017 01:11:58 PM Rafael J. Wysocki wrote:
> On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote:
> >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> > There is no limitation in the ondemand or conservative governors which
> >> > disallow the transition_latency to be greater than 10 ms.
> >> >
> >> > The max_transition_latency field is rather used to disallow automatic
> >> > dynamic frequency switching for platforms which didn't wanted these
> >> > governors to run.
> >> >
> >> > Replace max_transition_latency with a boolean (dynamic_switching) and
> >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This
> >> > makes it pretty straight forward to read/understand now.
> >>
> >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not
> >> particularly straightforward IMO, so maybe add a
> >> "no_dynamic_switching" to the driver structure and set it to "true"
> >> for the one driver in question?
> >
> > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and
> > where dynamic switching might be harmful or at least lead to undefined
> > behavior.
> 
> OK
> 
> Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic
> switching" condition is somewhat convoluted, so why don't we have a
> flag to *explicitly* say that instead?
> 
> Do you know which drivers they are or is it just all drivers that use
> CPUFREQ_ETERNAL?

Well, after the $subject patch it effectively is all drivers that use
CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete
switch-over.

Thanks,
Rafael

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Dominik Brodowski]

[-- Attachment #1: Type: text/plain, Size: 4197 bytes --]

On Sat, Jul 15, 2017 at 12:06:24AM +0200, Rafael J. Wysocki wrote:
> On Friday, July 14, 2017 01:11:58 PM Rafael J. Wysocki wrote:
> > On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski
> > <linux@dominikbrodowski.net> wrote:
> > > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote:
> > >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >> > There is no limitation in the ondemand or conservative governors which
> > >> > disallow the transition_latency to be greater than 10 ms.
> > >> >
> > >> > The max_transition_latency field is rather used to disallow automatic
> > >> > dynamic frequency switching for platforms which didn't wanted these
> > >> > governors to run.
> > >> >
> > >> > Replace max_transition_latency with a boolean (dynamic_switching) and
> > >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This
> > >> > makes it pretty straight forward to read/understand now.
> > >>
> > >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not
> > >> particularly straightforward IMO, so maybe add a
> > >> "no_dynamic_switching" to the driver structure and set it to "true"
> > >> for the one driver in question?
> > >
> > > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and
> > > where dynamic switching might be harmful or at least lead to undefined
> > > behavior.
> > 
> > OK
> > 
> > Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic
> > switching" condition is somewhat convoluted, so why don't we have a
> > flag to *explicitly* say that instead?
> > 
> > Do you know which drivers they are or is it just all drivers that use
> > CPUFREQ_ETERNAL?
> 
> Well, after the $subject patch it effectively is all drivers that use
> CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete
> switch-over.

Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:

Using CPUFREQ_ETERNAL, as policy-setting drivers:

- intel_pstate.c - for the intel_pstate driver, which defers to the hardware
  to do frequency selection.

- longrun.c - hardware-based frequency selection.

=> Those drivers are not interested in kernel-based dynamic frequency
selection anyway.


Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
- arm_big_little.c
- arm_big_little_dt.c
- cpufreq-dt.c
- imx6q-cpufreq.c
- spear_cpufreq.c

=> As it seems to be an error case, it seems best to bail out on the
safe side and disallow dynamic frequency scaling. Platform experts might
know better, though.


Using CPUFREQ_ETERNAL unconditionally:
- cpufreq-nforce2.c - over a decade old driver; has a commented-out hack
  to mdelay(10ms) after each frequency transition. This smells like it might
  be unsafe to do dynamic switching more often than that.

- elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms
  and conditions seem to apply.

- gx-suspmod.c - works by a mechanism which reminds me of CPU frequency
  throttling, but chipset- and not CPU-based.

- pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some
  frequency switchign code, it has mdelay(10ms) calls.

- speedsstep-smi.c - this case was discussed previously.

=> For those drivers, dynamic frequency scaling should not be enabled IMO.


- sa1100-cpufreq.c and
- sa1110-cpufreq.c - If I remember correctly, those drivers were used for
  fast user-space based frequency scaling in the past. 

=> For these two drivers, enabling DFVS might be an option.


- sh-cpufreq.c - looks fast, but I have no clue.

- unicore2-cpufreq.c - same.

=> For those drivers, I have no clue. So to be on the safe side, I'd opt for
dynamic frequency scaling to be set to off.


To summarize: At first, I'd propose a *complete* switch-over from
CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed. Then, one
might discuss with the maintainers of individual drivers/platforms on
whether to relax this rule for a few of those drivers (sa11x0, sh-cpufreq,
unicore2-cpufreq and the drivers using CPUFREQ_ETERNAL as a fallback).

Best,
	Dominik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Rafael J. Wysocki]

On Saturday, July 15, 2017 07:08:28 AM Dominik Brodowski wrote:
> On Sat, Jul 15, 2017 at 12:06:24AM +0200, Rafael J. Wysocki wrote:
> > On Friday, July 14, 2017 01:11:58 PM Rafael J. Wysocki wrote:
> > > On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski
> > > <linux@dominikbrodowski.net> wrote:
> > > > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote:
> > > >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >> > There is no limitation in the ondemand or conservative governors which
> > > >> > disallow the transition_latency to be greater than 10 ms.
> > > >> >
> > > >> > The max_transition_latency field is rather used to disallow automatic
> > > >> > dynamic frequency switching for platforms which didn't wanted these
> > > >> > governors to run.
> > > >> >
> > > >> > Replace max_transition_latency with a boolean (dynamic_switching) and
> > > >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This
> > > >> > makes it pretty straight forward to read/understand now.
> > > >>
> > > >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not
> > > >> particularly straightforward IMO, so maybe add a
> > > >> "no_dynamic_switching" to the driver structure and set it to "true"
> > > >> for the one driver in question?
> > > >
> > > > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and
> > > > where dynamic switching might be harmful or at least lead to undefined
> > > > behavior.
> > > 
> > > OK
> > > 
> > > Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic
> > > switching" condition is somewhat convoluted, so why don't we have a
> > > flag to *explicitly* say that instead?
> > > 
> > > Do you know which drivers they are or is it just all drivers that use
> > > CPUFREQ_ETERNAL?
> > 
> > Well, after the $subject patch it effectively is all drivers that use
> > CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete
> > switch-over.
> 
> Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:
> 
> Using CPUFREQ_ETERNAL, as policy-setting drivers:
> 
> - intel_pstate.c - for the intel_pstate driver, which defers to the hardware
>   to do frequency selection.
> 
> - longrun.c - hardware-based frequency selection.

That may or may not be hardware-based, but if the ->setpolicy callback is
present, transition_latency doesn't matter anyway.

> => Those drivers are not interested in kernel-based dynamic frequency
> selection anyway.

Right.

> 
> Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
> - arm_big_little.c
> - arm_big_little_dt.c
> - cpufreq-dt.c
> - imx6q-cpufreq.c
> - spear_cpufreq.c
> 
> => As it seems to be an error case, it seems best to bail out on the
> safe side and disallow dynamic frequency scaling. Platform experts might
> know better, though.
> 

Well, Viresh should know what to do for some of them at least. :-)

> Using CPUFREQ_ETERNAL unconditionally:
> - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack
>   to mdelay(10ms) after each frequency transition. This smells like it might
>   be unsafe to do dynamic switching more often than that.
> 
> - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms
>   and conditions seem to apply.
> 
> - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency
>   throttling, but chipset- and not CPU-based.
> 
> - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some
>   frequency switchign code, it has mdelay(10ms) calls.
> 
> - speedsstep-smi.c - this case was discussed previously.
> 
> => For those drivers, dynamic frequency scaling should not be enabled IMO.

Agreed.

> - sa1100-cpufreq.c and
> - sa1110-cpufreq.c - If I remember correctly, those drivers were used for
>   fast user-space based frequency scaling in the past. 
> 
> => For these two drivers, enabling DFVS might be an option.
> 

OK, I'm not familiar with these.

> - sh-cpufreq.c - looks fast, but I have no clue.
> 
> - unicore2-cpufreq.c - same.
> 
> => For those drivers, I have no clue. So to be on the safe side, I'd opt for
> dynamic frequency scaling to be set to off.
> 

Agreed.

> To summarize: At first, I'd propose a *complete* switch-over from
> CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed.

So we seem to be in agreement over this.

> Then, one might discuss with the maintainers of individual drivers/platforms on
> whether to relax this rule for a few of those drivers (sa11x0, sh-cpufreq,
> unicore2-cpufreq and the drivers using CPUFREQ_ETERNAL as a fallback).

Right.

Thanks,
Rafael

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Viresh Kumar]

On 15-07-17, 14:26, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 07:08:28 AM Dominik Brodowski wrote:

> > Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:
> > 
> > Using CPUFREQ_ETERNAL, as policy-setting drivers:
> > 
> > - intel_pstate.c - for the intel_pstate driver, which defers to the hardware
> >   to do frequency selection.
> > 
> > - longrun.c - hardware-based frequency selection.
> 
> That may or may not be hardware-based, but if the ->setpolicy callback is
> present, transition_latency doesn't matter anyway.

Right and to avoid confusion its probably better to avoid setting
transition_latency completely from them. I will try to include that in
my series.

> > => Those drivers are not interested in kernel-based dynamic frequency
> > selection anyway.
> 
> Right.
> 
> > 
> > Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
> > - arm_big_little.c
> > - arm_big_little_dt.c
> > - cpufreq-dt.c
> > - imx6q-cpufreq.c
> > - spear_cpufreq.c
> > 
> > => As it seems to be an error case, it seems best to bail out on the
> > safe side and disallow dynamic frequency scaling. Platform experts might
> > know better, though.
> > 
> 
> Well, Viresh should know what to do for some of them at least. :-)

Yeah, they just don't know how much time it takes to change the
frequency. We shouldn't disallow DVFS for them.

> > Using CPUFREQ_ETERNAL unconditionally:
> > - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack
> >   to mdelay(10ms) after each frequency transition. This smells like it might
> >   be unsafe to do dynamic switching more often than that.
> > 
> > - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms
> >   and conditions seem to apply.
> > 
> > - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency
> >   throttling, but chipset- and not CPU-based.
> > 
> > - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some
> >   frequency switchign code, it has mdelay(10ms) calls.
> > 
> > - speedsstep-smi.c - this case was discussed previously.
> > 
> > => For those drivers, dynamic frequency scaling should not be enabled IMO.
> 
> Agreed.

+1 and these are the drivers which should have this new variable set
to avoid DVFS.

Anyone who is setting CPUFREQ_ETERNAL unconditionally should be
setting the new flag.

> > To summarize: At first, I'd propose a *complete* switch-over from
> > CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed.
> 
> So we seem to be in agreement over this.

The usage of CPUFREQ_ETERNAL had been confusing over the years, i.e.
some use it to not allow ondemand/conservative, while others use it as
they don't know their transition latencies.

A complete switch over may not be good for the later.

I would suggest we only move the platforms which set latency to
CPUFREQ_ETERNAL unconditionally to the "no DVFS" list. And everyone
else can still continue with CPUFREQ_ETERNAL. I have earlier proposed
finding their latencies dynamically and will try to include that for
them.

-- 
viresh

Re: [RFC V2 1/6] cpufreq: Replace "max_transition_latency" with "dynamic_switching"

[Rafael J. Wysocki]

On Monday, July 17, 2017 05:28:51 PM Viresh Kumar wrote:
> On 15-07-17, 14:26, Rafael J. Wysocki wrote:
> > On Saturday, July 15, 2017 07:08:28 AM Dominik Brodowski wrote:
> 
> > > Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:
> > > 
> > > Using CPUFREQ_ETERNAL, as policy-setting drivers:
> > > 
> > > - intel_pstate.c - for the intel_pstate driver, which defers to the hardware
> > >   to do frequency selection.
> > > 
> > > - longrun.c - hardware-based frequency selection.
> > 
> > That may or may not be hardware-based, but if the ->setpolicy callback is
> > present, transition_latency doesn't matter anyway.
> 
> Right and to avoid confusion its probably better to avoid setting
> transition_latency completely from them. I will try to include that in
> my series.
> 
> > > => Those drivers are not interested in kernel-based dynamic frequency
> > > selection anyway.
> > 
> > Right.
> > 
> > > 
> > > Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
> > > - arm_big_little.c
> > > - arm_big_little_dt.c
> > > - cpufreq-dt.c
> > > - imx6q-cpufreq.c
> > > - spear_cpufreq.c
> > > 
> > > => As it seems to be an error case, it seems best to bail out on the
> > > safe side and disallow dynamic frequency scaling. Platform experts might
> > > know better, though.
> > > 
> > 
> > Well, Viresh should know what to do for some of them at least. :-)
> 
> Yeah, they just don't know how much time it takes to change the
> frequency. We shouldn't disallow DVFS for them.
> 
> > > Using CPUFREQ_ETERNAL unconditionally:
> > > - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack
> > >   to mdelay(10ms) after each frequency transition. This smells like it might
> > >   be unsafe to do dynamic switching more often than that.
> > > 
> > > - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms
> > >   and conditions seem to apply.
> > > 
> > > - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency
> > >   throttling, but chipset- and not CPU-based.
> > > 
> > > - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some
> > >   frequency switchign code, it has mdelay(10ms) calls.
> > > 
> > > - speedsstep-smi.c - this case was discussed previously.
> > > 
> > > => For those drivers, dynamic frequency scaling should not be enabled IMO.
> > 
> > Agreed.
> 
> +1 and these are the drivers which should have this new variable set
> to avoid DVFS.
> 
> Anyone who is setting CPUFREQ_ETERNAL unconditionally should be
> setting the new flag.
> 
> > > To summarize: At first, I'd propose a *complete* switch-over from
> > > CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed.
> > 
> > So we seem to be in agreement over this.
> 
> The usage of CPUFREQ_ETERNAL had been confusing over the years, i.e.
> some use it to not allow ondemand/conservative, while others use it as
> they don't know their transition latencies.

So if we can identify these, we can introduce something like
CPUFREQ_LATENCY_UNKNOWN for them and get rid of CPUFREQ_ETERNAL.

> A complete switch over may not be good for the later.
> 
> I would suggest we only move the platforms which set latency to
> CPUFREQ_ETERNAL unconditionally to the "no DVFS" list. And everyone
> else can still continue with CPUFREQ_ETERNAL. I have earlier proposed
> finding their latencies dynamically and will try to include that for
> them.

I have to admint I'm not a super-fan of that (mostly because I tried to do
something similar in the past for genpd, but that didn't work out well IMO),
but as long as that is done in drivers and not in the core, I can live with it.

Thanks,
Rafael

[RFC V2 2/6] cpufreq: schedutil: Set dynamic_switching to true

[Viresh Kumar]

Set dynamic_switching to 'true' to disallow use of schedutil governor
for platforms with transition_latency set to CPUFREQ_ETERNAL, as they
may not want to do automatic dynamic frequency switching.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/cpufreq_schedutil.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 076a2e31951c..ab9d7a1b43dc 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -650,6 +650,7 @@ static void sugov_limits(struct cpufreq_policy *policy)
 static struct cpufreq_governor schedutil_gov = {
 	.name = "schedutil",
 	.owner = THIS_MODULE,
+	.dynamic_switching = true,
 	.init = sugov_init,
 	.exit = sugov_exit,
 	.start = sugov_start,
-- 
2.13.0.71.gd7076ec9c9cb

[RFC V2 3/6] cpufreq: governor: Drop min_sampling_rate

[Viresh Kumar]

The cpufreq core and governors aren't supposed to set a limit on how
fast we want to try changing the frequency. This is currently done for
the legacy governors with help of min_sampling_rate.

At worst, we may end up setting the sampling rate to a value lower than
the rate at which frequency can be changed and then one of the CPUs in
the policy will be only changing frequency for ever.

But that is something for the user to decide and there is no need to
have special handling for such cases in the core. Leave it for the user
to figure out.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/admin-guide/pm/cpufreq.rst |  8 --------
 drivers/cpufreq/cpufreq_conservative.c   |  6 ------
 drivers/cpufreq/cpufreq_governor.c       | 10 ++--------
 drivers/cpufreq/cpufreq_governor.h       |  1 -
 drivers/cpufreq/cpufreq_ondemand.c       | 12 ------------
 include/linux/cpufreq.h                  |  2 --
 6 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
index 463cf7e73db8..2eb3bf62393e 100644
--- a/Documentation/admin-guide/pm/cpufreq.rst
+++ b/Documentation/admin-guide/pm/cpufreq.rst
@@ -471,14 +471,6 @@ it is allowed to use (the ``scaling_max_freq`` policy limit).
 
 	# echo `$(($(cat cpuinfo_transition_latency) * 750 / 1000)) > ondemand/sampling_rate
 
-
-``min_sampling_rate``
-	The minimum value of ``sampling_rate``.
-
-	Equal to 10000 (10 ms) if :c:macro:`CONFIG_NO_HZ_COMMON` and
-	:c:data:`tick_nohz_active` are both set or to 20 times the value of
-	:c:data:`jiffies` in microseconds otherwise.
-
 ``up_threshold``
 	If the estimated CPU load is above this value (in percent), the governor
 	will set the frequency to the maximum value allowed for the policy.
diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 88220ff3e1c2..f20f20a77d4d 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -246,7 +246,6 @@ gov_show_one_common(sampling_rate);
 gov_show_one_common(sampling_down_factor);
 gov_show_one_common(up_threshold);
 gov_show_one_common(ignore_nice_load);
-gov_show_one_common(min_sampling_rate);
 gov_show_one(cs, down_threshold);
 gov_show_one(cs, freq_step);
 
@@ -254,12 +253,10 @@ gov_attr_rw(sampling_rate);
 gov_attr_rw(sampling_down_factor);
 gov_attr_rw(up_threshold);
 gov_attr_rw(ignore_nice_load);
-gov_attr_ro(min_sampling_rate);
 gov_attr_rw(down_threshold);
 gov_attr_rw(freq_step);
 
 static struct attribute *cs_attributes[] = {
-	&min_sampling_rate.attr,
 	&sampling_rate.attr,
 	&sampling_down_factor.attr,
 	&up_threshold.attr,
@@ -297,10 +294,7 @@ static int cs_init(struct dbs_data *dbs_data)
 	dbs_data->up_threshold = DEF_FREQUENCY_UP_THRESHOLD;
 	dbs_data->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
 	dbs_data->ignore_nice_load = 0;
-
 	dbs_data->tuners = tuners;
-	dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
-		jiffies_to_usecs(10);
 
 	return 0;
 }
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 47e24b5384b3..858081f9c3d7 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -47,14 +47,11 @@ ssize_t store_sampling_rate(struct gov_attr_set *attr_set, const char *buf,
 {
 	struct dbs_data *dbs_data = to_dbs_data(attr_set);
 	struct policy_dbs_info *policy_dbs;
-	unsigned int rate;
 	int ret;
-	ret = sscanf(buf, "%u", &rate);
+	ret = sscanf(buf, "%u", &dbs_data->sampling_rate);
 	if (ret != 1)
 		return -EINVAL;
 
-	dbs_data->sampling_rate = max(rate, dbs_data->min_sampling_rate);
-
 	/*
 	 * We are operating under dbs_data->mutex and so the list and its
 	 * entries can't be freed concurrently.
@@ -437,10 +434,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
 		latency = 1;
 
 	/* Bring kernel and HW constraints together */
-	dbs_data->min_sampling_rate = max(dbs_data->min_sampling_rate,
-					  MIN_LATENCY_MULTIPLIER * latency);
-	dbs_data->sampling_rate = max(dbs_data->min_sampling_rate,
-				      LATENCY_MULTIPLIER * latency);
+	dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency;
 
 	if (!have_governor_per_policy())
 		gov->gdbs_data = dbs_data;
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 7b7839c45fba..8463f5def0f5 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -41,7 +41,6 @@ enum {OD_NORMAL_SAMPLE, OD_SUB_SAMPLE};
 struct dbs_data {
 	struct gov_attr_set attr_set;
 	void *tuners;
-	unsigned int min_sampling_rate;
 	unsigned int ignore_nice_load;
 	unsigned int sampling_rate;
 	unsigned int sampling_down_factor;
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 3937acf7e026..6b423eebfd5d 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -319,7 +319,6 @@ gov_show_one_common(sampling_rate);
 gov_show_one_common(up_threshold);
 gov_show_one_common(sampling_down_factor);
 gov_show_one_common(ignore_nice_load);
-gov_show_one_common(min_sampling_rate);
 gov_show_one_common(io_is_busy);
 gov_show_one(od, powersave_bias);
 
@@ -329,10 +328,8 @@ gov_attr_rw(up_threshold);
 gov_attr_rw(sampling_down_factor);
 gov_attr_rw(ignore_nice_load);
 gov_attr_rw(powersave_bias);
-gov_attr_ro(min_sampling_rate);
 
 static struct attribute *od_attributes[] = {
-	&min_sampling_rate.attr,
 	&sampling_rate.attr,
 	&up_threshold.attr,
 	&sampling_down_factor.attr,
@@ -373,17 +370,8 @@ static int od_init(struct dbs_data *dbs_data)
 	if (idle_time != -1ULL) {
 		/* Idle micro accounting is supported. Use finer thresholds */
 		dbs_data->up_threshold = MICRO_FREQUENCY_UP_THRESHOLD;
-		/*
-		 * In nohz/micro accounting case we set the minimum frequency
-		 * not depending on HZ, but fixed (very low).
-		*/
-		dbs_data->min_sampling_rate = MICRO_FREQUENCY_MIN_SAMPLE_RATE;
 	} else {
 		dbs_data->up_threshold = DEF_FREQUENCY_UP_THRESHOLD;
-
-		/* For correct statistics, we need 10 ticks for each measure */
-		dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
-			jiffies_to_usecs(10);
 	}
 
 	dbs_data->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 3d8c52b3b5df..00e4c40a3249 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -488,9 +488,7 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div,
  * ondemand governor will work on any processor with transition latency <= 10ms,
  * using appropriate sampling rate.
  */
-#define MIN_SAMPLING_RATE_RATIO		(2)
 #define LATENCY_MULTIPLIER		(1000)
-#define MIN_LATENCY_MULTIPLIER		(20)
 
 struct cpufreq_governor {
 	char	name[CPUFREQ_NAME_LEN];
-- 
2.13.0.71.gd7076ec9c9cb

[RFC V2 4/6] cpufreq: Use transition_delay_us for legacy governors as well

[Viresh Kumar]

The policy->transition_delay_us field is used only by the schedutil
governor currently, and this field describes how fast the driver wants
the cpufreq governor to change CPUs frequency. It should rather be a
common thing across all governors, as it doesn't have any schedutil
dependency here.

Create a new helper cpufreq_policy_transition_delay_us() to get the
transition delay across all governors.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c |  9 +--------
 include/linux/cpufreq.h            | 15 +++++++++++++++
 kernel/sched/cpufreq_schedutil.c   | 11 +----------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 858081f9c3d7..eed069ecfd5e 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -389,7 +389,6 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
 	struct dbs_governor *gov = dbs_governor_of(policy);
 	struct dbs_data *dbs_data;
 	struct policy_dbs_info *policy_dbs;
-	unsigned int latency;
 	int ret = 0;
 
 	/* State should be equivalent to EXIT */
@@ -428,13 +427,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_policy_dbs_info;
 
-	/* policy latency is in ns. Convert it to us first */
-	latency = policy->cpuinfo.transition_latency / 1000;
-	if (latency == 0)
-		latency = 1;
-
-	/* Bring kernel and HW constraints together */
-	dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency;
+	dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
 
 	if (!have_governor_per_policy())
 		gov->gdbs_data = dbs_data;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 00e4c40a3249..14f0ab61ed17 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -532,6 +532,21 @@ static inline void cpufreq_policy_apply_limits(struct cpufreq_policy *policy)
 		__cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
 }
 
+static inline unsigned int
+cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
+{
+	unsigned int delay_us = LATENCY_MULTIPLIER, latency;
+
+	if (policy->transition_delay_us)
+		return policy->transition_delay_us;
+
+	latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
+	if (latency)
+		delay_us *= latency;
+
+	return delay_us;
+}
+
 /* Governor attribute set */
 struct gov_attr_set {
 	struct kobject kobj;
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index ab9d7a1b43dc..5c72c569ec2f 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -528,16 +528,7 @@ static int sugov_init(struct cpufreq_policy *policy)
 		goto stop_kthread;
 	}
 
-	if (policy->transition_delay_us) {
-		tunables->rate_limit_us = policy->transition_delay_us;
-	} else {
-		unsigned int lat;
-
-		tunables->rate_limit_us = LATENCY_MULTIPLIER;
-		lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
-		if (lat)
-			tunables->rate_limit_us *= lat;
-	}
+	tunables->rate_limit_us = cpufreq_policy_transition_delay_us(policy);
 
 	policy->governor_data = sg_policy;
 	sg_policy->tunables = tunables;
-- 
2.13.0.71.gd7076ec9c9cb

Re: [RFC V2 4/6] cpufreq: Use transition_delay_us for legacy governors as well

[Rafael J. Wysocki]

On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The policy->transition_delay_us field is used only by the schedutil
> governor currently, and this field describes how fast the driver wants
> the cpufreq governor to change CPUs frequency. It should rather be a
> common thing across all governors, as it doesn't have any schedutil
> dependency here.
>
> Create a new helper cpufreq_policy_transition_delay_us() to get the
> transition delay across all governors.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_governor.c |  9 +--------
>  include/linux/cpufreq.h            | 15 +++++++++++++++
>  kernel/sched/cpufreq_schedutil.c   | 11 +----------
>  3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 858081f9c3d7..eed069ecfd5e 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -389,7 +389,6 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
>         struct dbs_governor *gov = dbs_governor_of(policy);
>         struct dbs_data *dbs_data;
>         struct policy_dbs_info *policy_dbs;
> -       unsigned int latency;
>         int ret = 0;
>
>         /* State should be equivalent to EXIT */
> @@ -428,13 +427,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
>         if (ret)
>                 goto free_policy_dbs_info;
>
> -       /* policy latency is in ns. Convert it to us first */
> -       latency = policy->cpuinfo.transition_latency / 1000;
> -       if (latency == 0)
> -               latency = 1;
> -
> -       /* Bring kernel and HW constraints together */
> -       dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency;
> +       dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
>
>         if (!have_governor_per_policy())
>                 gov->gdbs_data = dbs_data;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 00e4c40a3249..14f0ab61ed17 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -532,6 +532,21 @@ static inline void cpufreq_policy_apply_limits(struct cpufreq_policy *policy)
>                 __cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
>  }
>
> +static inline unsigned int
> +cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
> +{
> +       unsigned int delay_us = LATENCY_MULTIPLIER, latency;
> +
> +       if (policy->transition_delay_us)
> +               return policy->transition_delay_us;
> +
> +       latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> +       if (latency)
> +               delay_us *= latency;
> +
> +       return delay_us;
> +}

Not in the header, please, and I don't think you need delay_us:

    latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
    if (latency)
        return latency * LATENCY_MULTIPLIER;

    return LATENCY_MULTIPLIER;

Thanks,
Rafael

[RFC V2 5/6] cpufreq: Cap the default transition delay value to 10 ms

[Viresh Kumar]

If transition_delay_us isn't defined by the cpufreq driver, the default
value of transition delay (time after which the cpufreq governor will
try updating the frequency again) is currently calculated by multiplying
transition_latency (nsec) with LATENCY_MULTIPLIER (1000) and then
converting this time to usec. That gives the exact same value as
transition_latency, just that the time unit is usec instead of nsec.

With acpi-cpufreq for example, transition_latency is set to around 10
usec and we get transition delay as 10 ms. Which seems to be a
reasonable amount of time to reevaluate the frequency again.

But for platforms where frequency switching isn't that fast (like ARM),
the transition_latency varies from 500 usec to 3 ms, and the transition
delay becomes 500 ms to 3 seconds. Of course, that is a pretty bad
default value to start with.

We can try to come across a better formula (instead of multiplying with
LATENCY_MULTIPLIER) to solve this problem, but will that be worth it ?

This patch tries a simple approach and caps the maximum value of default
transition delay to 10 ms. Of course, userspace can still come in and
change this value anytime or individual drivers can rather provide
transition_delay_us instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/cpufreq.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 14f0ab61ed17..f691ec2793c0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -544,7 +544,17 @@ cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
 	if (latency)
 		delay_us *= latency;
 
-	return delay_us;
+	/*
+	 * For platforms that can change the frequency very fast (< 10 us),
+	 * the above formula gives a decent transition delay. But for platforms
+	 * where transition_latency is in milliseconds, it ends up giving
+	 * unrealistic values.
+	 *
+	 * Cap the default transition delay to 10 ms, which seems to be a
+	 * reasonable amount of time after which we should reevaluate the
+	 * frequency.
+	 */
+	return min(delay_us, (unsigned int)10000);
 }
 
 /* Governor attribute set */
-- 
2.13.0.71.gd7076ec9c9cb

[RFC V2 6/6] cpufreq: arm_big_little: Make ->get_transition_latency() mandatory

[Viresh Kumar]

All users of arm_big_little driver are defining it and there is no need
to keep it optional.

Make it mandatory to remove the always true conditional statement.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/arm_big_little.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index 418042201e6d..d1eb2a53b61f 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -483,11 +483,8 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 		return ret;
 	}
 
-	if (arm_bL_ops->get_transition_latency)
-		policy->cpuinfo.transition_latency =
-			arm_bL_ops->get_transition_latency(cpu_dev);
-	else
-		policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+	policy->cpuinfo.transition_latency =
+				arm_bL_ops->get_transition_latency(cpu_dev);
 
 	if (is_bL_switching_enabled())
 		per_cpu(cpu_last_req_freq, policy->cpu) = clk_get_cpu_rate(policy->cpu);
@@ -622,7 +619,8 @@ int bL_cpufreq_register(struct cpufreq_arm_bL_ops *ops)
 		return -EBUSY;
 	}
 
-	if (!ops || !strlen(ops->name) || !ops->init_opp_table) {
+	if (!ops || !strlen(ops->name) || !ops->init_opp_table ||
+	    !ops->get_transition_latency) {
 		pr_err("%s: Invalid arm_bL_ops, exiting\n", __func__);
 		return -ENODEV;
 	}
-- 
2.13.0.71.gd7076ec9c9cb