Re: [PATCH V2 01/35] cpufreq: Implement light weight ->target_index() routine

Hi Viresh,

On Tue, Aug 13, 2013 at 7:02 PM, Viresh Kumar viresh.kumar@linaro.org wrote:

Currently prototype of cpufreq_drivers target routines is:

int target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation);

And most of the drivers call cpufreq_frequency_table_target() to get a valid index of their frequency table which is closest to the target_freq. And they don't use target_freq and relation after it.

So, it makes sense to just do this work in cpufreq core before calling cpufreq_frequency_table_target() and simply pass index instead. But this can be done only with drivers which expose their frequency table with cpufreq core. For others we need to stick with the old prototype of target() until those drivers are converted to expose frequency tables.

This patch implements the new light weight prototype for target_index() routine. It looks like this:

int target_index(struct cpufreq_policy *policy, unsigned int index);

This new API is fine but I have another idea. Say During the registration of the frequency table cpufreq_policy can be registered as SCALE_DIRECT or SCALE_STEPS. With SCALE_DIRECT flag, valid frequency will be requested. With this flags the governor itself can can figure out if frequency scaling is required or not and very few calls to __cpufreq_driver_target will happen. But i agree that in this approach cpufreq_frequency_table_target is still required but again it can be optimized by binary search as currently the search is linear.

Thanks, Amit

CPUFreq core will call cpufreq_frequency_table_target() before calling this routine and pass index to it. Because CPUFreq core now requires to call routines present in freq_table.c CONFIG_CPU_FREQ_TABLE must be enabled all the time.

This also marks target() interface as deprecated. So, that new drivers avoid using it. And

Documentation is updated accordingly.

Cc: Andrew Lunn andrew@lunn.ch Cc: David S. Miller davem@davemloft.net Cc: Dmitry Eremin-Solenikov dbaryshkov@gmail.com Cc: Eric Miao eric.y.miao@gmail.com Cc: Hans-Christian Egtvedt egtvedt@samfundet.no Cc: Jesper Nilsson jesper.nilsson@axis.com Cc: John Crispin blogic@openwrt.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Linus Walleij linus.walleij@linaro.org Cc: linux-cris-kernel@axis.com Cc: Mikael Starvik starvik@axis.com Cc: Santosh Shilimkar santosh.shilimkar@ti.com Cc: Sekhar Nori nsekhar@ti.com Cc: Shawn Guo shawn.guo@linaro.org Cc: sparclinux@vger.kernel.org Cc: Stephen Warren swarren@nvidia.com Cc: Steven Miao realmz6@gmail.com Cc: Tony Luck tony.luck@intel.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

---

Documentation/cpu-freq/cpu-drivers.txt | 27 +++++++++++------ Documentation/cpu-freq/governors.txt | 4 +-- drivers/cpufreq/Kconfig | 1 + drivers/cpufreq/cpufreq.c | 55 +++++++++++++++++++++++++++------- include/linux/cpufreq.h | 4 ++- 5 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt index 40282e6..8b1a445 100644 --- a/Documentation/cpu-freq/cpu-drivers.txt +++ b/Documentation/cpu-freq/cpu-drivers.txt @@ -23,8 +23,8 @@ Contents: 1.1 Initialization 1.2 Per-CPU Initialization 1.3 verify -1.4 target or setpolicy? -1.5 target +1.4 target/target_index or setpolicy? +1.5 target/target_index 1.6 setpolicy 2. Frequency Table Helpers

@@ -56,7 +56,8 @@ cpufreq_driver.init - A pointer to the per-CPU initialization cpufreq_driver.verify - A pointer to a "verification" function.

cpufreq_driver.setpolicy _or_ -cpufreq_driver.target - See below on the differences. +cpufreq_driver.target/ +target_index - See below on the differences.

And optionally

@@ -66,7 +67,7 @@ cpufreq_driver.resume - A pointer to a per-CPU resume function which is called with interrupts disabled and _before_ the pre-suspend frequency and/or policy is restored by a call to

•                           ->target or ->setpolicy.
  

•                           ->target/target_index or ->setpolicy.
  
  

cpufreq_driver.attr - A pointer to a NULL-terminated list of "struct freq_attr" which allow to @@ -103,8 +104,8 @@ policy->governor must contain the "default policy" for this CPU. A few moments later, cpufreq_driver.verify and either cpufreq_driver.setpolicy or

•                           cpufreq_driver.target is called with
  

•                           these values.
  

•                           cpufreq_driver.target/target_index is called
  

•                           with these values.
  
  

For setting some of these values (cpuinfo.min[max]_freq, policy->min[max]), the frequency table helpers might be helpful. See the section 2 for more information @@ -133,20 +134,28 @@ range) is within policy->min and policy->max. If necessary, increase policy->max first, and only if this is no solution, decrease policy->min.

-1.4 target or setpolicy?

+1.4 target/target_index or setpolicy?

Most cpufreq drivers or even most cpu frequency scaling algorithms only allow the CPU to be set to one frequency. For these, you use the -->target call. +->target/target_index call.

Some cpufreq-capable processors switch the frequency between certain limits on their own. These shall use the ->setpolicy call

-1.4. target

+1.4. target/target_index

+The target_index call has two arguments: struct cpufreq_policy *policy, +and unsigned int index (into the exposed frequency table).

+The CPUfreq driver must set the new frequency when called here. The +actual frequency must be determined by freq_table[index].frequency.

+Deprecated: +---------- The target call has three arguments: struct cpufreq_policy *policy, unsigned int target_frequency, unsigned int relation.

diff --git a/Documentation/cpu-freq/governors.txt b/Documentation/cpu-freq/governors.txt index 219970b..77ec215 100644 --- a/Documentation/cpu-freq/governors.txt +++ b/Documentation/cpu-freq/governors.txt @@ -40,7 +40,7 @@ Most cpufreq drivers (in fact, all except one, longrun) or even most cpu frequency scaling algorithms only offer the CPU to be set to one frequency. In order to offer dynamic frequency scaling, the cpufreq core must be able to tell these drivers of a "target frequency". So -these specific drivers will be transformed to offer a "->target" +these specific drivers will be transformed to offer a "->target/target_index" call instead of the existing "->setpolicy" call. For "longrun", all stays the same, though.

@@ -71,7 +71,7 @@ CPU can be set to switch independently | CPU can only be set / the limits of policy->{min,max} / \ / \

•   Using the ->setpolicy call,              Using the ->target call,
  

•   Using the ->setpolicy call,              Using the ->target/target_index call,
        the limits and the                    the frequency closest
         "policy" is set.                     to target_freq is set.
                                              It is assured that it
  

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index 534fcb8..2d06754 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -2,6 +2,7 @@ menu "CPU Frequency scaling"

config CPU_FREQ bool "CPU Frequency scaling"

•   select CPU_FREQ_TABLE
    help
      CPU Frequency scaling allows you to change the clock speed of
      CPUs on the fly. This is a nice method to save power, because
  

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 37a6874..f1b0e0f 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -47,6 +47,11 @@ static LIST_HEAD(cpufreq_policy_list); static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); #endif

+static inline bool has_target(void) +{

•   return cpufreq_driver->target_index || cpufreq_driver->target;
  

+}

/*

• cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure
• all cpufreq/hotplug/workqueue/etc related lock issues.

@@ -377,7 +382,7 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy, *policy = CPUFREQ_POLICY_POWERSAVE; err = 0; }

•   } else if (cpufreq_driver->target) {
  

•   } else if (has_target()) {
            struct cpufreq_governor *t;
  
            mutex_lock(&cpufreq_governor_mutex);
  

@@ -539,7 +544,7 @@ static ssize_t show_scaling_available_governors(struct cpufreq_policy *policy, ssize_t i = 0; struct cpufreq_governor *t;

•   if (!cpufreq_driver->target) {
  

•   if (!has_target()) {
            i += sprintf(buf, "performance powersave");
            goto out;
    }
  

@@ -822,7 +827,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy, if (ret) goto err_out_kobj_put; }

•   if (cpufreq_driver->target) {
  

•   if (has_target()) {
            ret = sysfs_create_file(&policy->kobj, &scaling_cur_freq.attr);
            if (ret)
                    goto err_out_kobj_put;
  

@@ -871,10 +876,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, struct device *dev, bool frozen) {

•   int ret = 0, has_target = !!cpufreq_driver->target;
  

•   int ret = 0;
    unsigned long flags;
  
  

•   if (has_target) {
  

•   if (has_target()) {
            ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
            if (ret) {
                    pr_err("%s: Failed to stop governor\n", __func__);
  

@@ -893,7 +898,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,

    unlock_policy_rwsem_write(policy->cpu);

•   if (has_target) {
  

•   if (has_target()) {
            if ((ret = __cpufreq_governor(policy, CPUFREQ_GOV_START)) ||
                    (ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))) {
                    pr_err("%s: Failed to start governor\n", __func__);
  

@@ -1204,7 +1209,7 @@ static int __cpufreq_remove_dev(struct device *dev, return -EINVAL; }

•   if (cpufreq_driver->target) {
  

•   if (has_target()) {
            ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
            if (ret) {
                    pr_err("%s: Failed to stop governor\n", __func__);
  

@@ -1244,7 +1249,7 @@ static int __cpufreq_remove_dev(struct device *dev,

    /* If cpu is last user of policy, free policy */
    if (cpus == 1) {

•           if (cpufreq_driver->target) {
  

•           if (has_target()) {
                    ret = __cpufreq_governor(policy,
                                    CPUFREQ_GOV_POLICY_EXIT);
                    if (ret) {
  

@@ -1282,7 +1287,7 @@ static int __cpufreq_remove_dev(struct device *dev, if (!frozen) cpufreq_policy_free(policy); } else {

•           if (cpufreq_driver->target) {
  

•           if (has_target()) {
                    if ((ret = __cpufreq_governor(policy, CPUFREQ_GOV_START)) ||
                                    (ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))) {
                            pr_err("%s: Failed to start governor\n",
  

@@ -1646,11 +1651,39 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n", policy->cpu, target_freq, relation, old_target_freq);

•   /*
  

•    * This might look like a redundant call as we are checking it again
  

•    * after finding index. But it is left intentionally for cases where
  

•    * exactly same freq is called again and so we can save on few function
  

•    * calls.
  

•    */
    if (target_freq == policy->cur)
            return 0;
  
    if (cpufreq_driver->target)
            retval = cpufreq_driver->target(policy, target_freq, relation);
  

•   else if (cpufreq_driver->target_index) {
  

•           struct cpufreq_frequency_table *freq_table;
  

•           int index;
  

•           freq_table = cpufreq_frequency_get_table(policy->cpu);
  

•           if (unlikely(!freq_table)) {
  

•                   pr_err("%s: Unable to find freq_table\n", __func__);
  

•                   return retval;
  

•           }
  

•           retval = cpufreq_frequency_table_target(policy, freq_table,
  

•                           target_freq, relation, &index);
  

•           if (unlikely(retval)) {
  

•                   pr_err("%s: Unable to find matching freq\n", __func__);
  

•                   return retval;
  

•           }
  

•           if (freq_table[index].frequency == policy->cur)
  

•                   return 0;
  

•           retval = cpufreq_driver->target_index(policy, index);
  

•   }
  
    return retval;
  

} @@ -1983,7 +2016,7 @@ int cpufreq_update_policy(unsigned int cpu) pr_debug("Driver did not initialize current freq"); policy->cur = new_policy.cur; } else {

•                   if (policy->cur != new_policy.cur && cpufreq_driver->target)
  

•                   if (policy->cur != new_policy.cur && has_target())
                            cpufreq_out_of_sync(cpu, policy->cur,
                                                            new_policy.cur);
            }
  

@@ -2058,7 +2091,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) return -ENODEV;

    if (!driver_data || !driver_data->verify || !driver_data->init ||

•       ((!driver_data->setpolicy) && (!driver_data->target)))
  

•       (!driver_data->setpolicy && !has_target()))
            return -EINVAL;
  
    pr_debug("trying to register driver %s\n", driver_data->name);
  

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4907eb2..ff9c8df 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -195,9 +195,11 @@ struct cpufreq_driver {

    /* define one out of two */
    int     (*setpolicy)    (struct cpufreq_policy *policy);

•   int     (*target)       (struct cpufreq_policy *policy,
  

•   int     (*target)       (struct cpufreq_policy *policy, /* Deprecated */
                             unsigned int target_freq,
                             unsigned int relation);
  

•   int     (*target_index) (struct cpufreq_policy *policy,
  

•                            unsigned int index);
  
    /* should be defined, if possible */
    unsigned int    (*get)  (unsigned int cpu);
  

-- 1.7.12.rc2.18.g61b472e

-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html