| Message ID | 046e3785229754332de62854439d2a4a17d637b7.1400302114.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
Viresh, On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > +1.7 get_intermediate and target_intermediate > +-------------------------------------------- > + > +Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION unset. > + > +get_intermediate should return a stable intermediate frequency platform wants to > +switch to, and target_intermediate() should set CPU to to that frequency, before > +jumping to the frequency corresponding to 'index'. Core will take care of > +sending notifications and driver doesn't have to handle them in > +target_intermediate() or target_index(). Is it worth documenting that if we implement target_intermediate() that target_index() must not fail? That means that any failure-prone things (like setting a regulator) should happen in target_index(). > 2. Frequency Table Helpers > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 9bf12a2..f38f2f2 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier); > static int __target_index(struct cpufreq_policy *policy, > struct cpufreq_frequency_table *freq_table, int index) > { > - struct cpufreq_freqs freqs; > + struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0}; > int retval = -EINVAL; > bool notify; > > notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION); > + if (!notify) > + goto skip_notify; I'm personally not a fan of using goto here. All you're trying to do is avoiding a level of indentation? If it really matters that much then create a sub function. IMHO goto should generally be reserved for error handling. > > - if (notify) { > - freqs.old = policy->cur; > - freqs.new = freq_table[index].frequency; > - freqs.flags = 0; > + /* Handle switching to intermediate frequency */ > + if (cpufreq_driver->get_intermediate) { > + freqs.new = cpufreq_driver->get_intermediate(policy, index); Would it be worth it to change this to? intermediate = 0 if (cpufreq_driver->get_intermediate) intermediate = cpufreq_driver->get_intermediate(); if (intermediate) ...the idea being that a driver may use an intermediate frequency for some transitions but not for all. For instance: on tegra if you happen to change to the exact clock frequency of the intermediate PLL it just stays there. There's no need for two notifications in that case. There may be other systems that can optimize some transitions to totally skip the intermediate stage (maybe you've got an non-glitching divider somewhere so you can optimize a transition from 1.4GHz to 700MHz to go w/ no intermediate jump). > - pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", > + pr_debug("%s: cpu: %d, switching to intermediate freq: oldfreq: %u, intermediate freq: %u\n", > __func__, policy->cpu, freqs.old, freqs.new); > > cpufreq_freq_transition_begin(policy, &freqs); > + retval = cpufreq_driver->target_intermediate(policy, freqs.new); It feels like you want to pass in "index" here too, just in case. A driver may need to make decisions about other clocks based on the eventual final frequency. They could cache it themselves from the get_intermediate() call, but that seems ugly. > @@ -2361,7 +2384,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) > !(driver_data->setpolicy || driver_data->target_index || > driver_data->target) || > (driver_data->setpolicy && (driver_data->target_index || > - driver_data->target))) > + driver_data->target)) || > + (!!driver_data->get_intermediate ^ !!driver_data->target_intermediate)) I'm OK with the !! trick, but using ^ here seems more confusing. Why not use "!="? (!!driver_data->get_intermediate != !!driver_data->target_intermediate)) -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Doug, On 20 May 2014 22:18, Doug Anderson <dianders@chromium.org> wrote: > Is it worth documenting that if we implement target_intermediate() > that target_index() must not fail? That means that any failure-prone > things (like setting a regulator) should happen in target_index(). You meant target_intermediate() is the last line, right ? Yeah, we can add that.. >> 2. Frequency Table Helpers >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 9bf12a2..f38f2f2 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier); >> static int __target_index(struct cpufreq_policy *policy, >> struct cpufreq_frequency_table *freq_table, int index) >> { >> - struct cpufreq_freqs freqs; >> + struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0}; >> int retval = -EINVAL; >> bool notify; >> >> notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION); >> + if (!notify) >> + goto skip_notify; > > I'm personally not a fan of using goto here. All you're trying to do > is avoiding a level of indentation? If it really matters that much > then create a sub function. IMHO goto should generally be reserved > for error handling. Yeah, I was trying that :) .. Another routine wouldn't be right here as the POST_NOTIFICATION will be handled in this routine only. I will try again to see if I can write some better code here, but wouldn't promise that :) >> - if (notify) { >> - freqs.old = policy->cur; >> - freqs.new = freq_table[index].frequency; >> - freqs.flags = 0; >> + /* Handle switching to intermediate frequency */ >> + if (cpufreq_driver->get_intermediate) { >> + freqs.new = cpufreq_driver->get_intermediate(policy, index); > > Would it be worth it to change this to? > > intermediate = 0 > if (cpufreq_driver->get_intermediate) > intermediate = cpufreq_driver->get_intermediate(); > if (intermediate) > > ...the idea being that a driver may use an intermediate frequency for > some transitions but not for all. For instance: on tegra if you > happen to change to the exact clock frequency of the intermediate PLL > it just stays there. There's no need for two notifications in that > case. There may be other systems that can optimize some transitions > to totally skip the intermediate stage (maybe you've got an > non-glitching divider somewhere so you can optimize a transition from > 1.4GHz to 700MHz to go w/ no intermediate jump). Hmm, will try to fix that as well. Looks like a valid point. >> - pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", >> + pr_debug("%s: cpu: %d, switching to intermediate freq: oldfreq: %u, intermediate freq: %u\n", >> __func__, policy->cpu, freqs.old, freqs.new); >> >> cpufreq_freq_transition_begin(policy, &freqs); >> + retval = cpufreq_driver->target_intermediate(policy, freqs.new); > > It feels like you want to pass in "index" here too, just in case. A > driver may need to make decisions about other clocks based on the > eventual final frequency. They could cache it themselves from the > get_intermediate() call, but that seems ugly. I had index here initially, but then it looked like they may need to perform get_intermediate() again from this routine and so sending the intermediate freq is probably better. So, probably just wait for some drivers which may need index here ? >> @@ -2361,7 +2384,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) >> !(driver_data->setpolicy || driver_data->target_index || >> driver_data->target) || >> (driver_data->setpolicy && (driver_data->target_index || >> - driver_data->target))) >> + driver_data->target)) || >> + (!!driver_data->get_intermediate ^ !!driver_data->target_intermediate)) > > I'm OK with the !! trick, but using ^ here seems more confusing. Why > not use "!="? > (!!driver_data->get_intermediate != !!driver_data->target_intermediate)) Will work as well :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt index b045fe5..b1bdb8a 100644 --- a/Documentation/cpu-freq/cpu-drivers.txt +++ b/Documentation/cpu-freq/cpu-drivers.txt @@ -26,6 +26,7 @@ Contents: 1.4 target/target_index or setpolicy? 1.5 target/target_index 1.6 setpolicy +1.7 get_intermediate and target_intermediate 2. Frequency Table Helpers @@ -79,6 +80,10 @@ cpufreq_driver.attr - A pointer to a NULL-terminated list of "struct freq_attr" which allow to export values to sysfs. +cpufreq_driver.get_intermediate +and target_intermediate Used to switch to stable frequency while + changing CPU frequency. + 1.2 Per-CPU Initialization -------------------------- @@ -151,7 +156,7 @@ Some cpufreq-capable processors switch the frequency between certain limits on their own. These shall use the ->setpolicy call -1.4. target/target_index +1.5. target/target_index ------------- The target_index call has two arguments: struct cpufreq_policy *policy, @@ -179,7 +184,7 @@ Here again the frequency table helper might assist you - see section 2 for details. -1.5 setpolicy +1.6 setpolicy --------------- The setpolicy call only takes a struct cpufreq_policy *policy as @@ -190,6 +195,16 @@ setting when policy->policy is CPUFREQ_POLICY_PERFORMANCE, and a powersaving-oriented setting when CPUFREQ_POLICY_POWERSAVE. Also check the reference implementation in drivers/cpufreq/longrun.c +1.7 get_intermediate and target_intermediate +-------------------------------------------- + +Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION unset. + +get_intermediate should return a stable intermediate frequency platform wants to +switch to, and target_intermediate() should set CPU to to that frequency, before +jumping to the frequency corresponding to 'index'. Core will take care of +sending notifications and driver doesn't have to handle them in +target_intermediate() or target_index(). 2. Frequency Table Helpers diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 9bf12a2..f38f2f2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier); static int __target_index(struct cpufreq_policy *policy, struct cpufreq_frequency_table *freq_table, int index) { - struct cpufreq_freqs freqs; + struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0}; int retval = -EINVAL; bool notify; notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION); + if (!notify) + goto skip_notify; - if (notify) { - freqs.old = policy->cur; - freqs.new = freq_table[index].frequency; - freqs.flags = 0; + /* Handle switching to intermediate frequency */ + if (cpufreq_driver->get_intermediate) { + freqs.new = cpufreq_driver->get_intermediate(policy, index); - pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", + pr_debug("%s: cpu: %d, switching to intermediate freq: oldfreq: %u, intermediate freq: %u\n", __func__, policy->cpu, freqs.old, freqs.new); cpufreq_freq_transition_begin(policy, &freqs); + retval = cpufreq_driver->target_intermediate(policy, freqs.new); + cpufreq_freq_transition_end(policy, &freqs, retval); + + if (retval) { + pr_err("%s: Failed to change to intermediate frequency: %d\n", + __func__, retval); + return retval; + } + + /* Set intermediate as old freq */ + freqs.old = freqs.new; } + freqs.new = freq_table[index].frequency; + + pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", __func__, + policy->cpu, freqs.old, freqs.new); + + cpufreq_freq_transition_begin(policy, &freqs); + +skip_notify: retval = cpufreq_driver->target_index(policy, index); - if (retval) + if (retval) { + /* We shouldn't fail after setting intermediate freq */ + WARN_ON(cpufreq_driver->get_intermediate); pr_err("%s: Failed to change cpu frequency: %d\n", __func__, retval); + } if (notify) cpufreq_freq_transition_end(policy, &freqs, retval); @@ -2361,7 +2384,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) !(driver_data->setpolicy || driver_data->target_index || driver_data->target) || (driver_data->setpolicy && (driver_data->target_index || - driver_data->target))) + driver_data->target)) || + (!!driver_data->get_intermediate ^ !!driver_data->target_intermediate)) 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 3f45889..bfcba11 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -226,6 +226,21 @@ struct cpufreq_driver { unsigned int relation); int (*target_index) (struct cpufreq_policy *policy, unsigned int index); + /* + * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION + * unset. + * + * get_intermediate should return a stable intermediate frequency + * platform wants to switch to and target_intermediate() should set CPU + * to to that frequency, before jumping to the frequency corresponding + * to 'index'. Core will take care of sending notifications and driver + * doesn't have to handle them in target_intermediate() or + * target_index(). + */ + unsigned int (*get_intermediate)(struct cpufreq_policy *policy, + unsigned int index); + int (*target_intermediate)(struct cpufreq_policy *policy, + unsigned int frequency); /* should be defined, if possible */ unsigned int (*get) (unsigned int cpu);