Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()
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()
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()
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()
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()
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()
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()
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()
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()
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()
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()
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()
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
Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq()
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()
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
> > > > Signed-off-by: Steve Muckle
> > > > ---
> > > > 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()
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
> > > Signed-off-by: Steve Muckle
> > > ---
> > > 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()
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
> > Signed-off-by: Steve Muckle
> > ---
> > 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()
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
> Signed-off-by: Steve Muckle
> ---
> 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
