On Wednesday, July 08, 2015 04:07:32 PM Viresh Kumar wrote:
> CPUFreq drivers today support a ->get(cpu) callback, which returns
> current rate of a CPU. The problem with ->get() is that it takes a cpu
> number as parameter and this unnecessarily makes things complex.
>
> Firstly the core gets the cpu number by doing operation 'policy->cpu' on
> the policy and then many drivers need to get the policy back and so do
> cpufreq_cpu_get(cpu) on the passed cpu.
>
> As cpufreq core works on policies, it would be better if we pass them
> 'policy' directly and drivers can use policy->cpu if that's all they
> need.
>
> Hence, this patch adds in another callback, ->get_rate() which does
> exactly the same work as ->get(), just that we pass 'policy' as
> parameter instead of 'cpu'.
>
> The plan is to migrate all drivers to this new callback and remove
> ->get() after that.
>
> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
> Hi Rafael,
>
> I hope you are fine with this stuff :), once you approve I will get
> other patches to migrate existing drivers to this interface.
I'm generally fine with it, but please target it at 4.4 at the earliest.
> drivers/cpufreq/cpufreq.c | 50
> ++++++++++++++++++++++++++++++++---------------
> include/linux/cpufreq.h | 1 +
> 2 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index b612411655f9..b66e169601e8 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -154,6 +154,20 @@ void disable_cpufreq(void)
> }
> static DEFINE_MUTEX(cpufreq_governor_mutex);
>
> +/* Operations for ->get() and ->get_rate() operations */
> +static inline bool driver_supports_get_rate(void)
> +{
> + return cpufreq_driver->get || cpufreq_driver->get_rate;
> +}
> +
> +static inline unsigned int policy_get_rate(struct cpufreq_policy *policy)
> +{
> + if (cpufreq_driver->get)
> + return cpufreq_driver->get(policy->cpu);
> + else
> + return cpufreq_driver->get_rate(policy);
> +}
> +
> bool have_governor_per_policy(void)
> {
> return !!(cpufreq_driver->flags & CPUFREQ_HAVE_GOVERNOR_PER_POLICY);
> @@ -596,8 +610,9 @@ static ssize_t show_scaling_cur_freq(struct
> cpufreq_policy *policy, char *buf)
> {
> ssize_t ret;
>
> - if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get)
> - ret = sprintf(buf, "%u\n", cpufreq_driver->get(policy->cpu));
> + if (cpufreq_driver && cpufreq_driver->setpolicy &&
> + driver_supports_get_rate())
> + ret = sprintf(buf, "%u\n", policy_get_rate(policy));
> else
> ret = sprintf(buf, "%u\n", policy->cur);
> return ret;
> @@ -1032,7 +1047,7 @@ static int cpufreq_add_dev_interface(struct
> cpufreq_policy *policy,
> return ret;
> drv_attr++;
> }
> - if (cpufreq_driver->get) {
> + if (driver_supports_get_rate()) {
> ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr);
> if (ret)
> return ret;
> @@ -1313,10 +1328,10 @@ static int cpufreq_add_dev(struct device *dev, struct
> subsys_interface *sif)
> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> }
>
> - if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
> - policy->cur = cpufreq_driver->get(policy->cpu);
> + if (driver_supports_get_rate() && !cpufreq_driver->setpolicy) {
> + policy->cur = policy_get_rate(policy);
> if (!policy->cur) {
> - pr_err("%s: ->get() failed\n", __func__);
> + pr_err("%s: ->get_rate() failed\n", __func__);
> goto err_get_freq;
> }
> }
> @@ -1594,14 +1609,17 @@ unsigned int cpufreq_quick_get(unsigned int cpu)
> struct cpufreq_policy *policy;
> unsigned int ret_freq = 0;
>
> - if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get)
> - return cpufreq_driver->get(cpu);
> -
> policy = cpufreq_cpu_get(cpu);
> - if (policy) {
> + if (!policy)
> + return 0;
> +
> + if (cpufreq_driver && cpufreq_driver->setpolicy &&
> + driver_supports_get_rate())
> + ret_freq = policy_get_rate(policy);
> + else
> ret_freq = policy->cur;
> - cpufreq_cpu_put(policy);
> - }
> +
> + cpufreq_cpu_put(policy);
>
> return ret_freq;
> }
> @@ -1631,10 +1649,10 @@ static unsigned int __cpufreq_get(struct
> cpufreq_policy *policy)
> {
> unsigned int ret_freq = 0;
>
> - if (!cpufreq_driver->get)
> + if (!driver_supports_get_rate())
> return ret_freq;
>
> - ret_freq = cpufreq_driver->get(policy->cpu);
> + ret_freq = policy_get_rate(policy);
>
> /* Updating inactive policies is invalid, so avoid doing that. */
> if (unlikely(policy_is_inactive(policy)))
> @@ -2345,8 +2363,8 @@ int cpufreq_update_policy(unsigned int cpu)
> * BIOS might change freq behind our back
> * -> ask driver for current freq and notify governors about a change
> */
> - if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
> - new_policy.cur = cpufreq_driver->get(cpu);
> + if (driver_supports_get_rate() && !cpufreq_driver->setpolicy) {
> + new_policy.cur = policy_get_rate(policy);
> if (WARN_ON(!new_policy.cur)) {
> ret = -EIO;
> goto unlock;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 29ad97c34fd5..a82049683016 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -263,6 +263,7 @@ struct cpufreq_driver {
>
> /* should be defined, if possible */
> unsigned int (*get)(unsigned int cpu);
> + unsigned int (*get_rate)(struct cpufreq_policy *policy);
>
> /* optional */
> int (*bios_limit)(int cpu, unsigned int *limit);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
