[PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[Steve Muckle]

Invoking the cpufreq driver to set a frequency can be expensive. On platforms
with a cpufreq driver that does not support fast switching a thread must be
woken to complete the operation. IPIs will also occur if and when support to
process remote task wakeups is added in schedutil.

Currently schedutil calculates a raw frequency request from scheduler
utilization data. This raw frequency request does not correlate to supported
cpufreq driver frequencies aside from being capped by the CPU's maximum
frequency. Consequently, there may be many consecutive requests for different
raw frequency values which all translate to the same driver-supported
frequency. For example on a platform with 3 supported frequencies 200MHz,
400MHz, and 600MHz, raw requests for 257MHz, 389MHz, and 307MHz all map to a
driver-supported frequency of 400MHz in schedutil. Assuming these requests were
consecutive and there were no changes in policy limits (min/max), there is no
need to issue the second or third request.

In order to resolve a raw frequency request to a driver-supported one a new
cpufreq API is added, cpufreq_driver_resolve_freq(). This API relies on a new
cpufreq driver callback in the case of ->target() style drivers. Otherwise it
uses the existing frequency table operations.

Lookups are cached both in cpufreq_driver_resolve_freq() (for the benefit of the
driver) and in schedutil.

Changes since v2:
- incorporated feedback from Viresh to use resolve_freq driver callbacks
  only for ->target() style drivers, to use cpufreq's freq table operations,
  and move freq mapping caching into cpufreq policy
Changes since v1:
- incorporated feedback from Rafael to avoid referencing freq_table from
  schedutil by introducing a new cpufreq API

Steve Muckle (3):
  cpufreq: add cpufreq_driver_resolve_freq()
  cpufreq: schedutil: map raw required frequency to driver frequency
  cpufreq: acpi-cpufreq: use cached frequency mapping when possible

 drivers/cpufreq/acpi-cpufreq.c   |  5 ++++-
 drivers/cpufreq/cpufreq.c        | 25 +++++++++++++++++++++++++
 include/linux/cpufreq.h          | 16 ++++++++++++++++
 kernel/sched/cpufreq_schedutil.c | 31 +++++++++++++++++++++++--------
 4 files changed, 68 insertions(+), 9 deletions(-)

-- 
2.7.3

[PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

Cpufreq governors may need to know what a particular target frequency
maps to in the driver without necessarily wanting to set the frequency.
Support this operation via a new cpufreq API,
cpufreq_driver_resolve_freq(). This API returns the lowest driver
frequency equal or greater than the target frequency
(CPUFREQ_RELATION_L), subject to any policy (min/max) or driver
limitations. The mapping is also cached in the policy so that a
subsequent fast_switch operation can avoid repeating the same lookup.

The API will call a new cpufreq driver callback, resolve_freq(), if it
has been registered by the driver. Otherwise the frequency is resolved
via cpufreq_frequency_table_target(). Rather than require ->target()
style drivers to provide a resolve_freq() callback it is left to the
caller to ensure that the driver implements this callback if necessary
to use cpufreq_driver_resolve_freq().

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Steve Muckle <smuckle@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
 include/linux/cpufreq.h   | 16 ++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 118b4f30a406..b696baeb249d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy)
 }
 EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
 
+/**
+ * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported
+ * one.
+ * @target_freq: target frequency to resolve.
+ *
+ * The target to driver frequency mapping is cached in the policy.
+ *
+ * Return: Lowest driver-supported frequency greater than or equal to the
+ * given target_freq, subject to policy (min/max) and driver limitations.
+ */
+unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
+					 unsigned int target_freq)
+{
+	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	policy->cached_target_freq = target_freq;
+	if (cpufreq_driver->resolve_freq)
+		return cpufreq_driver->resolve_freq(policy, target_freq);
+	policy->cached_resolved_idx =
+		cpufreq_frequency_table_target(policy, target_freq,
+					       CPUFREQ_RELATION_L);
+	return policy->freq_table[policy->cached_resolved_idx].frequency;
+}
+
 /*********************************************************************
  *                          SYSFS INTERFACE                          *
  *********************************************************************/
@@ -2199,6 +2222,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 	policy->min = new_policy->min;
 	policy->max = new_policy->max;
 
+	policy->cached_target_freq = UINT_MAX;
+
 	pr_debug("new min and max freqs are %u - %u kHz\n",
 		 policy->min, policy->max);
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index c6410b1b2490..631ba33bbe9f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -120,6 +120,10 @@ struct cpufreq_policy {
 	bool			fast_switch_possible;
 	bool			fast_switch_enabled;
 
+	 /* Cached frequency lookup from cpufreq_driver_resolve_freq. */
+	unsigned int cached_target_freq;
+	int cached_resolved_idx;
+
 	/* Synchronization for frequency transitions */
 	bool			transition_ongoing; /* Tracks transition status */
 	spinlock_t		transition_lock;
@@ -270,6 +274,16 @@ struct cpufreq_driver {
 					unsigned int index);
 	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
 				       unsigned int target_freq);
+
+	/*
+	 * Caches and returns the lowest driver-supported frequency greater than
+	 * or equal to the target frequency, subject to any driver limitations.
+	 * Does not set the frequency. Only to be implemented for drivers with
+	 * target().
+	 */
+	unsigned int	(*resolve_freq)(struct cpufreq_policy *policy,
+					unsigned int target_freq);
+
 	/*
 	 * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
 	 * unset.
@@ -501,6 +515,8 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
 int __cpufreq_driver_target(struct cpufreq_policy *policy,
 				   unsigned int target_freq,
 				   unsigned int relation);
+unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
+					 unsigned int target_freq);
 int cpufreq_register_governor(struct cpufreq_governor *governor);
 void cpufreq_unregister_governor(struct cpufreq_governor *governor);
 
-- 
2.7.3

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 13-07-16, 13:25, Steve Muckle wrote:
> Cpufreq governors may need to know what a particular target frequency
> maps to in the driver without necessarily wanting to set the frequency.
> Support this operation via a new cpufreq API,
> cpufreq_driver_resolve_freq(). This API returns the lowest driver
> frequency equal or greater than the target frequency
> (CPUFREQ_RELATION_L), subject to any policy (min/max) or driver
> limitations. The mapping is also cached in the policy so that a
> subsequent fast_switch operation can avoid repeating the same lookup.
> 
> The API will call a new cpufreq driver callback, resolve_freq(), if it
> has been registered by the driver. Otherwise the frequency is resolved
> via cpufreq_frequency_table_target(). Rather than require ->target()
> style drivers to provide a resolve_freq() callback it is left to the
> caller to ensure that the driver implements this callback if necessary
> to use cpufreq_driver_resolve_freq().
> 
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Steve Muckle <smuckle@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
>  include/linux/cpufreq.h   | 16 ++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 118b4f30a406..b696baeb249d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy)
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
>  
> +/**
> + * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported
> + * one.
> + * @target_freq: target frequency to resolve.
> + *
> + * The target to driver frequency mapping is cached in the policy.
> + *
> + * Return: Lowest driver-supported frequency greater than or equal to the
> + * given target_freq, subject to policy (min/max) and driver limitations.
> + */
> +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> +					 unsigned int target_freq)
> +{
> +	target_freq = clamp_val(target_freq, policy->min, policy->max);
> +	policy->cached_target_freq = target_freq;
> +	if (cpufreq_driver->resolve_freq)
> +		return cpufreq_driver->resolve_freq(policy, target_freq);

Any reason why we still have this call around ? I thought the whole
attempt I made was to get rid of this :)

The core can do this pretty much now by itself, why do we still want
this call?

Also, your series doesn't add a user for it yet, so better drop it for
now.

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Rafael J. Wysocki]

On Thursday, July 21, 2016 12:59:26 PM Viresh Kumar wrote:
> On 13-07-16, 13:25, Steve Muckle wrote:
> > Cpufreq governors may need to know what a particular target frequency
> > maps to in the driver without necessarily wanting to set the frequency.
> > Support this operation via a new cpufreq API,
> > cpufreq_driver_resolve_freq(). This API returns the lowest driver
> > frequency equal or greater than the target frequency
> > (CPUFREQ_RELATION_L), subject to any policy (min/max) or driver
> > limitations. The mapping is also cached in the policy so that a
> > subsequent fast_switch operation can avoid repeating the same lookup.
> > 
> > The API will call a new cpufreq driver callback, resolve_freq(), if it
> > has been registered by the driver. Otherwise the frequency is resolved
> > via cpufreq_frequency_table_target(). Rather than require ->target()
> > style drivers to provide a resolve_freq() callback it is left to the
> > caller to ensure that the driver implements this callback if necessary
> > to use cpufreq_driver_resolve_freq().
> > 
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Steve Muckle <smuckle@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
> >  include/linux/cpufreq.h   | 16 ++++++++++++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 118b4f30a406..b696baeb249d 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy)
> >  }
> >  EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
> >  
> > +/**
> > + * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported
> > + * one.
> > + * @target_freq: target frequency to resolve.
> > + *
> > + * The target to driver frequency mapping is cached in the policy.
> > + *
> > + * Return: Lowest driver-supported frequency greater than or equal to the
> > + * given target_freq, subject to policy (min/max) and driver limitations.
> > + */
> > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > +					 unsigned int target_freq)
> > +{
> > +	target_freq = clamp_val(target_freq, policy->min, policy->max);
> > +	policy->cached_target_freq = target_freq;
> > +	if (cpufreq_driver->resolve_freq)
> > +		return cpufreq_driver->resolve_freq(policy, target_freq);
> 
> Any reason why we still have this call around ? I thought the whole
> attempt I made was to get rid of this :)
> 
> The core can do this pretty much now by itself, why do we still want
> this call?

In case some drivers that don't use frequency tables want to implemet
fast switching, for example.

> Also, your series doesn't add a user for it yet, so better drop it for
> now.

That's correct, but then it is not so much of a maintenance burden and I may
need it.

I'm going to apply the whole series.

Thanks,
Rafael

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 21-07-16, 22:31, Rafael J. Wysocki wrote:
> On Thursday, July 21, 2016 12:59:26 PM Viresh Kumar wrote:
> > On 13-07-16, 13:25, Steve Muckle wrote:
> > > Cpufreq governors may need to know what a particular target frequency
> > > maps to in the driver without necessarily wanting to set the frequency.
> > > Support this operation via a new cpufreq API,
> > > cpufreq_driver_resolve_freq(). This API returns the lowest driver
> > > frequency equal or greater than the target frequency
> > > (CPUFREQ_RELATION_L), subject to any policy (min/max) or driver
> > > limitations. The mapping is also cached in the policy so that a
> > > subsequent fast_switch operation can avoid repeating the same lookup.
> > > 
> > > The API will call a new cpufreq driver callback, resolve_freq(), if it
> > > has been registered by the driver. Otherwise the frequency is resolved
> > > via cpufreq_frequency_table_target(). Rather than require ->target()
> > > style drivers to provide a resolve_freq() callback it is left to the
> > > caller to ensure that the driver implements this callback if necessary
> > > to use cpufreq_driver_resolve_freq().
> > > 
> > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Signed-off-by: Steve Muckle <smuckle@linaro.org>
> > > ---
> > >  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
> > >  include/linux/cpufreq.h   | 16 ++++++++++++++++
> > >  2 files changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 118b4f30a406..b696baeb249d 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy)
> > >  }
> > >  EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
> > >  
> > > +/**
> > > + * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported
> > > + * one.
> > > + * @target_freq: target frequency to resolve.
> > > + *
> > > + * The target to driver frequency mapping is cached in the policy.
> > > + *
> > > + * Return: Lowest driver-supported frequency greater than or equal to the
> > > + * given target_freq, subject to policy (min/max) and driver limitations.
> > > + */
> > > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > > +					 unsigned int target_freq)
> > > +{
> > > +	target_freq = clamp_val(target_freq, policy->min, policy->max);
> > > +	policy->cached_target_freq = target_freq;
> > > +	if (cpufreq_driver->resolve_freq)
> > > +		return cpufreq_driver->resolve_freq(policy, target_freq);
> > 
> > Any reason why we still have this call around ? I thought the whole
> > attempt I made was to get rid of this :)
> > 
> > The core can do this pretty much now by itself, why do we still want
> > this call?
> 
> In case some drivers that don't use frequency tables want to implemet
> fast switching, for example.

Okay, but in that case shouldn't we do something like this:

unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
                                        unsigned int target_freq)
{
       target_freq = clamp_val(target_freq, policy->min, policy->max);
       policy->cached_target_freq = target_freq;

       if (cpufreq_driver->target_index) {
       		policy->cached_resolved_idx =
       		        cpufreq_frequency_table_target(policy, target_freq,
       		                                       CPUFREQ_RELATION_L);
       		return policy->freq_table[policy->cached_resolved_idx].frequency;
       }

       if (cpufreq_driver->resolve_freq)
               return cpufreq_driver->resolve_freq(policy, target_freq);
}

??

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Rafael J. Wysocki]

On Thursday, July 21, 2016 01:30:41 PM Viresh Kumar wrote:
> On 21-07-16, 22:31, Rafael J. Wysocki wrote:
> > On Thursday, July 21, 2016 12:59:26 PM Viresh Kumar wrote:
> > > On 13-07-16, 13:25, Steve Muckle wrote:
> > > > Cpufreq governors may need to know what a particular target frequency
> > > > maps to in the driver without necessarily wanting to set the frequency.
> > > > Support this operation via a new cpufreq API,
> > > > cpufreq_driver_resolve_freq(). This API returns the lowest driver
> > > > frequency equal or greater than the target frequency
> > > > (CPUFREQ_RELATION_L), subject to any policy (min/max) or driver
> > > > limitations. The mapping is also cached in the policy so that a
> > > > subsequent fast_switch operation can avoid repeating the same lookup.
> > > > 
> > > > The API will call a new cpufreq driver callback, resolve_freq(), if it
> > > > has been registered by the driver. Otherwise the frequency is resolved
> > > > via cpufreq_frequency_table_target(). Rather than require ->target()
> > > > style drivers to provide a resolve_freq() callback it is left to the
> > > > caller to ensure that the driver implements this callback if necessary
> > > > to use cpufreq_driver_resolve_freq().
> > > > 
> > > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Signed-off-by: Steve Muckle <smuckle@linaro.org>
> > > > ---
> > > >  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
> > > >  include/linux/cpufreq.h   | 16 ++++++++++++++++
> > > >  2 files changed, 41 insertions(+)
> > > > 
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index 118b4f30a406..b696baeb249d 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
> > > >  
> > > > +/**
> > > > + * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported
> > > > + * one.
> > > > + * @target_freq: target frequency to resolve.
> > > > + *
> > > > + * The target to driver frequency mapping is cached in the policy.
> > > > + *
> > > > + * Return: Lowest driver-supported frequency greater than or equal to the
> > > > + * given target_freq, subject to policy (min/max) and driver limitations.
> > > > + */
> > > > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > > > +					 unsigned int target_freq)
> > > > +{
> > > > +	target_freq = clamp_val(target_freq, policy->min, policy->max);
> > > > +	policy->cached_target_freq = target_freq;
> > > > +	if (cpufreq_driver->resolve_freq)
> > > > +		return cpufreq_driver->resolve_freq(policy, target_freq);
> > > 
> > > Any reason why we still have this call around ? I thought the whole
> > > attempt I made was to get rid of this :)
> > > 
> > > The core can do this pretty much now by itself, why do we still want
> > > this call?
> > 
> > In case some drivers that don't use frequency tables want to implemet
> > fast switching, for example.
> 
> Okay, but in that case shouldn't we do something like this:

That'd be fine by me.

Please send a patch on top of the Steve's series and I can apply it too
(unless Steve sees some major problems in it, which seems unlikely to me).

> unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
>                                         unsigned int target_freq)
> {
>        target_freq = clamp_val(target_freq, policy->min, policy->max);
>        policy->cached_target_freq = target_freq;
> 
>        if (cpufreq_driver->target_index) {
>        		policy->cached_resolved_idx =
>        		        cpufreq_frequency_table_target(policy, target_freq,
>        		                                       CPUFREQ_RELATION_L);
>        		return policy->freq_table[policy->cached_resolved_idx].frequency;
>        }
> 
>        if (cpufreq_driver->resolve_freq)
>                return cpufreq_driver->resolve_freq(policy, target_freq);
> }
> 
> ??

Thanks,
Rafael

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 21-07-16, 22:52, Rafael J. Wysocki wrote:
> That'd be fine by me.
> 
> Please send a patch on top of the Steve's series and I can apply it too
> (unless Steve sees some major problems in it, which seems unlikely to me).

Sure, thanks :)

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Rafael J. Wysocki]

On Thursday, July 21, 2016 01:52:45 PM Viresh Kumar wrote:
> On 21-07-16, 22:52, Rafael J. Wysocki wrote:
> > That'd be fine by me.
> > 
> > Please send a patch on top of the Steve's series and I can apply it too
> > (unless Steve sees some major problems in it, which seems unlikely to me).
> 
> Sure, thanks :)

It atually is slightly problematic as is, because if the driver doesn't
implement ->resolve_freq and doesn't use a frequency table, it will crash
AFAICS.

But I guess you can fix this too. :-)

Thanks,
Rafael

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 21-07-16, 23:19, Rafael J. Wysocki wrote:
> On Thursday, July 21, 2016 01:52:45 PM Viresh Kumar wrote:
> > On 21-07-16, 22:52, Rafael J. Wysocki wrote:
> > > That'd be fine by me.
> > > 
> > > Please send a patch on top of the Steve's series and I can apply it too
> > > (unless Steve sees some major problems in it, which seems unlikely to me).
> > 
> > Sure, thanks :)
> 
> It atually is slightly problematic as is, because if the driver doesn't
> implement ->resolve_freq and doesn't use a frequency table, it will crash
> AFAICS.
> 
> But I guess you can fix this too. :-)

I replied with exact same detail a minute ago :), yeah my patch is
going to fix that as well.

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote:
> Okay, but in that case shouldn't we do something like this:
> 
> unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
>                                         unsigned int target_freq)
> {
>        target_freq = clamp_val(target_freq, policy->min, policy->max);
>        policy->cached_target_freq = target_freq;
> 
>        if (cpufreq_driver->target_index) {
>        		policy->cached_resolved_idx =
>        		        cpufreq_frequency_table_target(policy, target_freq,
>        		                                       CPUFREQ_RELATION_L);
>        		return policy->freq_table[policy->cached_resolved_idx].frequency;
>        }
> 
>        if (cpufreq_driver->resolve_freq)
>                return cpufreq_driver->resolve_freq(policy, target_freq);
> }

Thanks for the review.

My thinking (noted in the commit text) was that the caller of
cpufreq_driver_resolve_freq() would verify that the driver supported the
proper calls before using this API. This way it can be checked once,
presumably in a governor's init routine. Checking the pointer over and
over again in a fast path is wasteful.

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

On Thu, Jul 21, 2016 at 04:21:31PM -0700, Steve Muckle wrote:
> On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote:
> > Okay, but in that case shouldn't we do something like this:
> > 
> > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> >                                         unsigned int target_freq)
> > {
> >        target_freq = clamp_val(target_freq, policy->min, policy->max);
> >        policy->cached_target_freq = target_freq;
> > 
> >        if (cpufreq_driver->target_index) {
> >        		policy->cached_resolved_idx =
> >        		        cpufreq_frequency_table_target(policy, target_freq,
> >        		                                       CPUFREQ_RELATION_L);
> >        		return policy->freq_table[policy->cached_resolved_idx].frequency;
> >        }
> > 
> >        if (cpufreq_driver->resolve_freq)
> >                return cpufreq_driver->resolve_freq(policy, target_freq);
> > }
> 
> Thanks for the review.
> 
> My thinking (noted in the commit text) was that the caller of
> cpufreq_driver_resolve_freq() would verify that the driver supported the
> proper calls before using this API. This way it can be checked once,
> presumably in a governor's init routine. Checking the pointer over and
> over again in a fast path is wasteful.

I guess this isn't immediately possible as the governor can't see
cpufreq_driver. I was hoping to change that however to allow
cpufreq_driver_resolve_freq() to be inlined in schedutil to get rid of
another function call...

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 21-07-16, 16:29, Steve Muckle wrote:
> On Thu, Jul 21, 2016 at 04:21:31PM -0700, Steve Muckle wrote:
> > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote:
> > > Okay, but in that case shouldn't we do something like this:
> > > 
> > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > >                                         unsigned int target_freq)
> > > {
> > >        target_freq = clamp_val(target_freq, policy->min, policy->max);
> > >        policy->cached_target_freq = target_freq;
> > > 
> > >        if (cpufreq_driver->target_index) {
> > >        		policy->cached_resolved_idx =
> > >        		        cpufreq_frequency_table_target(policy, target_freq,
> > >        		                                       CPUFREQ_RELATION_L);
> > >        		return policy->freq_table[policy->cached_resolved_idx].frequency;
> > >        }
> > > 
> > >        if (cpufreq_driver->resolve_freq)
> > >                return cpufreq_driver->resolve_freq(policy, target_freq);
> > > }
> > 
> > Thanks for the review.
> > 
> > My thinking (noted in the commit text) was that the caller of
> > cpufreq_driver_resolve_freq() would verify that the driver supported the
> > proper calls before using this API. This way it can be checked once,
> > presumably in a governor's init routine. Checking the pointer over and
> > over again in a fast path is wasteful.
> 
> I guess this isn't immediately possible as the governor can't see
> cpufreq_driver. I was hoping to change that however to allow
> cpufreq_driver_resolve_freq() to be inlined in schedutil to get rid of
> another function call...

Well, you can do that by moving the newly created routine to
cpufreq.h.

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 21-07-16, 16:21, Steve Muckle wrote:
> On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote:
> > Okay, but in that case shouldn't we do something like this:
> > 
> > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> >                                         unsigned int target_freq)
> > {
> >        target_freq = clamp_val(target_freq, policy->min, policy->max);
> >        policy->cached_target_freq = target_freq;
> > 
> >        if (cpufreq_driver->target_index) {
> >        		policy->cached_resolved_idx =
> >        		        cpufreq_frequency_table_target(policy, target_freq,
> >        		                                       CPUFREQ_RELATION_L);
> >        		return policy->freq_table[policy->cached_resolved_idx].frequency;
> >        }
> > 
> >        if (cpufreq_driver->resolve_freq)
> >                return cpufreq_driver->resolve_freq(policy, target_freq);
> > }
> 
> Thanks for the review.
> 
> My thinking (noted in the commit text) was that the caller of
> cpufreq_driver_resolve_freq() would verify that the driver supported the
> proper calls before using this API.

Okay, but the caller isn't doing that today. Right?

> This way it can be checked once,
> presumably in a governor's init routine. Checking the pointer over and
> over again in a fast path is wasteful.

But we just can not assume the callers to always check that the driver
has a ->target() and no ->resolve_freq(), and in that case not to call
this routine. We would be forced to add a WARN_ON() in that case here
to make sure we aren't trying to access a NULL ->resolve_freq.

Over that, it will be used for a very small number of drivers which
still use the ->target() callback and anyway we are going to do a
function call for them. We can add a likely() here if that helps, but
some sort of checking is surely required IMO.

And, this is a core API, which can be used for other governor's
tomorrow :)

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

On Thu, Jul 21, 2016 at 04:30:03PM -0700, Viresh Kumar wrote:
> On 21-07-16, 16:21, Steve Muckle wrote:
> > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote:
> > > Okay, but in that case shouldn't we do something like this:
> > > 
> > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > >                                         unsigned int target_freq)
> > > {
> > >        target_freq = clamp_val(target_freq, policy->min, policy->max);
> > >        policy->cached_target_freq = target_freq;
> > > 
> > >        if (cpufreq_driver->target_index) {
> > >        		policy->cached_resolved_idx =
> > >        		        cpufreq_frequency_table_target(policy, target_freq,
> > >        		                                       CPUFREQ_RELATION_L);
> > >        		return policy->freq_table[policy->cached_resolved_idx].frequency;
> > >        }
> > > 
> > >        if (cpufreq_driver->resolve_freq)
> > >                return cpufreq_driver->resolve_freq(policy, target_freq);
> > > }
> > 
> > Thanks for the review.
> > 
> > My thinking (noted in the commit text) was that the caller of
> > cpufreq_driver_resolve_freq() would verify that the driver supported the
> > proper calls before using this API.
> 
> Okay, but the caller isn't doing that today. Right?

There is no caller yet.

> > This way it can be checked once,
> > presumably in a governor's init routine. Checking the pointer over and
> > over again in a fast path is wasteful.
> 
> But we just can not assume the callers to always check that the driver
> has a ->target() and no ->resolve_freq(), and in that case not to call
> this routine. We would be forced to add a WARN_ON() in that case here
> to make sure we aren't trying to access a NULL ->resolve_freq.

Why not? Can we not catch that in code review?

If somehow this slips past and someone tries to use a driver with
schedutil that doesn't provide either target_index or resolve_freq, it's
not like it'll be a rare crash. It'll die immediately and in a very
obvious way.

> Over that, it will be used for a very small number of drivers which
> still use the ->target() callback and anyway we are going to do a
> function call for them. We can add a likely() here if that helps, but
> some sort of checking is surely required IMO.
> 
> And, this is a core API, which can be used for other governor's
> tomorrow :)

As another alternative, this could be caught in cpufreq driver
initialization? I believe you suggested that originally, but I avoided
it as I didn't want to have to implement resolve_freq() for every
target() style driver. It sounds like there aren't many though.

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

On Thu, Jul 21, 2016 at 04:36:48PM -0700, Steve Muckle wrote:
> On Thu, Jul 21, 2016 at 04:30:03PM -0700, Viresh Kumar wrote:
> > On 21-07-16, 16:21, Steve Muckle wrote:
> > > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote:
> > > > Okay, but in that case shouldn't we do something like this:
> > > > 
> > > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > > >                                         unsigned int target_freq)
> > > > {
> > > >        target_freq = clamp_val(target_freq, policy->min, policy->max);
> > > >        policy->cached_target_freq = target_freq;
> > > > 
> > > >        if (cpufreq_driver->target_index) {
> > > >        		policy->cached_resolved_idx =
> > > >        		        cpufreq_frequency_table_target(policy, target_freq,
> > > >        		                                       CPUFREQ_RELATION_L);
> > > >        		return policy->freq_table[policy->cached_resolved_idx].frequency;
> > > >        }
> > > > 
> > > >        if (cpufreq_driver->resolve_freq)
> > > >                return cpufreq_driver->resolve_freq(policy, target_freq);
> > > > }
> > > 
> > > Thanks for the review.
> > > 
> > > My thinking (noted in the commit text) was that the caller of
> > > cpufreq_driver_resolve_freq() would verify that the driver supported the
> > > proper calls before using this API.
> > 
> > Okay, but the caller isn't doing that today. Right?
> 
> There is no caller yet.

Sorry, of course this is not true.

I'm still of the opinion that modifying the governor (I could fix up
schedutil now) or adding a check in driver init would be better than any
unnecessary logic in the fast path.

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

On Thu, Jul 21, 2016 at 04:36:48PM -0700, Steve Muckle wrote:
> As another alternative, this could be caught in cpufreq driver
> initialization? I believe you suggested that originally, but I avoided
> it as I didn't want to have to implement resolve_freq() for every
> target() style driver. It sounds like there aren't many though.

Going back and checking I see I was thinking of your suggestion that
cpufreq_register_driver() check that only target() drivers offer a
resolve_freq() callback. I put a comment for this in cpufreq.h but not a
check - I could add a check in another patch if you like.

Long term as I was mentioning in the other thread I think it'd be good
if the current target() drivers were modified to supply resolve_freq(),
and that cpufreq_register_driver() were again changed to require it for
those drivers.

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 21-07-16, 17:44, Steve Muckle wrote:
> Going back and checking I see I was thinking of your suggestion that
> cpufreq_register_driver() check that only target() drivers offer a
> resolve_freq() callback. I put a comment for this in cpufreq.h but not a
> check - I could add a check in another patch if you like.

That can be done as we aren't supporting the ->resolve_freq() callback
for ->target_index() drivers.

> Long term as I was mentioning in the other thread I think it'd be good
> if the current target() drivers were modified to supply resolve_freq(),
> and that cpufreq_register_driver() were again changed to require it for
> those drivers.

There is no need for us to force this, its really optional for such
platforms. Worst case, schedutil wouldn't work at the best, so what?
Its a platform driver's choice, isn't it ?

-- 
viresh

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Steve Muckle]

On Fri, Jul 22, 2016 at 08:16:42AM -0700, Viresh Kumar wrote:
> > Long term as I was mentioning in the other thread I think it'd be good
> > if the current target() drivers were modified to supply resolve_freq(),
> > and that cpufreq_register_driver() were again changed to require it for
> > those drivers.
> 
> There is no need for us to force this, its really optional for such
> platforms. Worst case, schedutil wouldn't work at the best, so what?
> Its a platform driver's choice, isn't it ?

This would be in the context of then being able to remove the additional if
statement from the hot path.

To reply to the suggestion of using likely() here, I'm partial to
solving it without that because I'm guessing likely() has to be an
architecture-dependent thing? It seems cleaner to me if the existing
few target() drivers were augmented to provide the resolve_freq() calback
and its presence required. But it's certainly not a big deal and won't
affect any platforms I'm involved with, at least for now. Maybe I could
work on those target() drivers if things change.

Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()

[Viresh Kumar]

On 13-07-16, 13:25, Steve Muckle wrote:
> +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> +					 unsigned int target_freq)
> +{
> +	target_freq = clamp_val(target_freq, policy->min, policy->max);
> +	policy->cached_target_freq = target_freq;
> +	if (cpufreq_driver->resolve_freq)
> +		return cpufreq_driver->resolve_freq(policy, target_freq);
> +	policy->cached_resolved_idx =
> +		cpufreq_frequency_table_target(policy, target_freq,
> +					       CPUFREQ_RELATION_L);
> +	return policy->freq_table[policy->cached_resolved_idx].frequency;

FWIW, this may crash the kernel for a driver that provides ->target()
but no ->resolve_freq().

-- 
viresh

[PATCH v3 2/3] cpufreq: schedutil: map raw required frequency to driver frequency

[Steve Muckle]

The slow-path frequency transition path is relatively expensive as it
requires waking up a thread to do work. Should support be added for
remote CPU cpufreq updates that is also expensive since it requires an
IPI. These activities should be avoided if they are not necessary.

To that end, calculate the actual driver-supported frequency required by
the new utilization value in schedutil by using the recently added
cpufreq_driver_resolve_freq API. If it is the same as the previously
requested driver frequency then there is no need to continue with the
update assuming the cpu frequency limits have not changed. This will
have additional benefits should the semantics of the rate limit be
changed to apply solely to frequency transitions rather than to
frequency calculations in schedutil.

The last raw required frequency is cached. This allows the driver
frequency lookup to be skipped in the event that the new raw required
frequency matches the last one, assuming a frequency update has not been
forced due to limits changing (indicated by a next_freq value of
UINT_MAX, see sugov_should_update_freq).

Signed-off-by: Steve Muckle <smuckle@linaro.org>
---
 kernel/sched/cpufreq_schedutil.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 758efd7f3abe..a84641b222c1 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -47,6 +47,8 @@ struct sugov_cpu {
 	struct update_util_data update_util;
 	struct sugov_policy *sg_policy;
 
+	unsigned int cached_raw_freq;
+
 	/* The fields below are only needed when sharing a policy. */
 	unsigned long util;
 	unsigned long max;
@@ -106,7 +108,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
 
 /**
  * get_next_freq - Compute a new frequency for a given cpufreq policy.
- * @policy: cpufreq policy object to compute the new frequency for.
+ * @sg_cpu: schedutil cpu object to compute the new frequency for.
  * @util: Current CPU utilization.
  * @max: CPU capacity.
  *
@@ -121,14 +123,25 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
  * next_freq = C * curr_freq * util_raw / max
  *
  * Take C = 1.25 for the frequency tipping point at (util / max) = 0.8.
+ *
+ * The lowest driver-supported frequency which is equal or greater than the raw
+ * next_freq (as calculated above) is returned, subject to policy min/max and
+ * cpufreq driver limitations.
  */
-static unsigned int get_next_freq(struct cpufreq_policy *policy,
-				  unsigned long util, unsigned long max)
+static unsigned int get_next_freq(struct sugov_cpu *sg_cpu, unsigned long util,
+				  unsigned long max)
 {
+	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
+	struct cpufreq_policy *policy = sg_policy->policy;
 	unsigned int freq = arch_scale_freq_invariant() ?
 				policy->cpuinfo.max_freq : policy->cur;
 
-	return (freq + (freq >> 2)) * util / max;
+	freq = (freq + (freq >> 2)) * util / max;
+
+	if (freq == sg_cpu->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
+		return sg_policy->next_freq;
+	sg_cpu->cached_raw_freq = freq;
+	return cpufreq_driver_resolve_freq(policy, freq);
 }
 
 static void sugov_update_single(struct update_util_data *hook, u64 time,
@@ -143,13 +156,14 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 		return;
 
 	next_f = util == ULONG_MAX ? policy->cpuinfo.max_freq :
-			get_next_freq(policy, util, max);
+			get_next_freq(sg_cpu, util, max);
 	sugov_update_commit(sg_policy, time, next_f);
 }
 
-static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy,
+static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu,
 					   unsigned long util, unsigned long max)
 {
+	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
 	struct cpufreq_policy *policy = sg_policy->policy;
 	unsigned int max_f = policy->cpuinfo.max_freq;
 	u64 last_freq_update_time = sg_policy->last_freq_update_time;
@@ -189,7 +203,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy,
 		}
 	}
 
-	return get_next_freq(policy, util, max);
+	return get_next_freq(sg_cpu, util, max);
 }
 
 static void sugov_update_shared(struct update_util_data *hook, u64 time,
@@ -206,7 +220,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	sg_cpu->last_update = time;
 
 	if (sugov_should_update_freq(sg_policy, time)) {
-		next_f = sugov_next_freq_shared(sg_policy, util, max);
+		next_f = sugov_next_freq_shared(sg_cpu, util, max);
 		sugov_update_commit(sg_policy, time, next_f);
 	}
 
@@ -433,6 +447,7 @@ static int sugov_start(struct cpufreq_policy *policy)
 			sg_cpu->util = ULONG_MAX;
 			sg_cpu->max = 0;
 			sg_cpu->last_update = 0;
+			sg_cpu->cached_raw_freq = 0;
 			cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util,
 						     sugov_update_shared);
 		} else {
-- 
2.7.3

[PATCH v3 3/3] cpufreq: acpi-cpufreq: use cached frequency mapping when possible

[Steve Muckle]

A call to cpufreq_driver_resolve_freq will cache the mapping from
the desired target frequency to the frequency table index. If there
is a mapping for the desired target frequency then use it instead of
looking up the mapping again.

Signed-off-by: Steve Muckle <smuckle@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 11c9a078e0fd..297e9128fe9f 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -473,7 +473,10 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 	/*
 	 * Find the closest frequency above target_freq.
 	 */
-	index = cpufreq_table_find_index_dl(policy, target_freq);
+	if (policy->cached_target_freq == target_freq)
+		index = policy->cached_resolved_idx;
+	else
+		index = cpufreq_table_find_index_dl(policy, target_freq);
 
 	entry = &policy->freq_table[index];
 	next_freq = entry->frequency;
-- 
2.7.3

Re: [PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[Pingbo Wen]

On Thursday, July 14, 2016 04:25 AM, Steve Muckle wrote:
> Invoking the cpufreq driver to set a frequency can be expensive. On platforms
> with a cpufreq driver that does not support fast switching a thread must be
> woken to complete the operation. IPIs will also occur if and when support to
> process remote task wakeups is added in schedutil.
> 
> Currently schedutil calculates a raw frequency request from scheduler
> utilization data. This raw frequency request does not correlate to supported
> cpufreq driver frequencies aside from being capped by the CPU's maximum
> frequency. Consequently, there may be many consecutive requests for different
> raw frequency values which all translate to the same driver-supported
> frequency. For example on a platform with 3 supported frequencies 200MHz,
> 400MHz, and 600MHz, raw requests for 257MHz, 389MHz, and 307MHz all map to a
> driver-supported frequency of 400MHz in schedutil. Assuming these requests were
> consecutive and there were no changes in policy limits (min/max), there is no
> need to issue the second or third request.
> 
> In order to resolve a raw frequency request to a driver-supported one a new
> cpufreq API is added, cpufreq_driver_resolve_freq(). This API relies on a new
> cpufreq driver callback in the case of ->target() style drivers. Otherwise it
> uses the existing frequency table operations.
> 
> Lookups are cached both in cpufreq_driver_resolve_freq() (for the benefit of the
> driver) and in schedutil.
> 
> Changes since v2:
> - incorporated feedback from Viresh to use resolve_freq driver callbacks
>   only for ->target() style drivers, to use cpufreq's freq table operations,
>   and move freq mapping caching into cpufreq policy
> Changes since v1:
> - incorporated feedback from Rafael to avoid referencing freq_table from
>   schedutil by introducing a new cpufreq API
> 
> Steve Muckle (3):
>   cpufreq: add cpufreq_driver_resolve_freq()
>   cpufreq: schedutil: map raw required frequency to driver frequency

Tested the first two patches on db410c, only waking up irq_work 53
times, while previous was 257 times(79% decrease) in Android home idle
for 5 minutes.

>   cpufreq: acpi-cpufreq: use cached frequency mapping when possible
> 

Pingbo

Re: [PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[Steve Muckle]

On Thu, Jul 14, 2016 at 06:02:31PM +0800, Pingbo Wen wrote:
> > Steve Muckle (3):
> >   cpufreq: add cpufreq_driver_resolve_freq()
> >   cpufreq: schedutil: map raw required frequency to driver frequency
> 
> Tested the first two patches on db410c, only waking up irq_work 53
> times, while previous was 257 times(79% decrease) in Android home idle
> for 5 minutes.

Thanks Pingbo. My experience measuring the number of calls into the acpi
cpufreq fast switch path was similar. An arbitrary system workload I
chose showed a ~75% reduction in calls.

thanks,
Steve

Re: [PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[Viresh Kumar]

On 13-07-16, 13:25, Steve Muckle wrote:
> Invoking the cpufreq driver to set a frequency can be expensive. On platforms
> with a cpufreq driver that does not support fast switching a thread must be
> woken to complete the operation. IPIs will also occur if and when support to
> process remote task wakeups is added in schedutil.
> 
> Currently schedutil calculates a raw frequency request from scheduler
> utilization data. This raw frequency request does not correlate to supported
> cpufreq driver frequencies aside from being capped by the CPU's maximum
> frequency. Consequently, there may be many consecutive requests for different
> raw frequency values which all translate to the same driver-supported
> frequency. For example on a platform with 3 supported frequencies 200MHz,
> 400MHz, and 600MHz, raw requests for 257MHz, 389MHz, and 307MHz all map to a
> driver-supported frequency of 400MHz in schedutil. Assuming these requests were
> consecutive and there were no changes in policy limits (min/max), there is no
> need to issue the second or third request.
> 
> In order to resolve a raw frequency request to a driver-supported one a new
> cpufreq API is added, cpufreq_driver_resolve_freq(). This API relies on a new
> cpufreq driver callback in the case of ->target() style drivers. Otherwise it
> uses the existing frequency table operations.
> 
> Lookups are cached both in cpufreq_driver_resolve_freq() (for the benefit of the
> driver) and in schedutil.
> 
> Changes since v2:
> - incorporated feedback from Viresh to use resolve_freq driver callbacks
>   only for ->target() style drivers, to use cpufreq's freq table operations,
>   and move freq mapping caching into cpufreq policy

Sorry for the delay buddy :(

I have some concerns for the first patch. The second and third patch
look fine.  Feel free to add my 

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

for them.

-- 
viresh