| Message ID | 1370941408-29304-2-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, Change subject to: "cpufreq: Add boost frequency support in core" On 11 June 2013 14:33, Lukasz Majewski <l.majewski@samsung.com> wrote: > This commit adds support for software based frequency boosting. No. It adds support for both software and hardware boosting. So just write: This commit adds boost frequency support in cpufreq core (Hardware & Software). > Some SoC (like Exynos4 - e.g. 4x12) allow setting frequency above > its normal condition limits. Such a change shall be only done for a short s/condition/operation s/change/mode s/done/used > time. > > Overclocking (boost) support is essentially provided by platform > dependent cpufreq driver. > > This commit unifies support for SW and HW (Intel) over clocking solutions > in the core cpufreq driver. Previously the "boost" sysfs attribute was > defined at acpi driver code. > By default boost is disabled. One global attribute is available at: > /sys/devices/system/cpu/cpufreq/boost. Enter a blank line here. > It only shows up when cpufreq driver supports overclocking. > Under the hood frequencies dedicated for boosting are marked with a > special flag (CPUFREQ_BOOST_FREQ) at driver's frequency table. > It is the user's concern to enable/disable overclocking with proper call to > sysfs. Good. > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > > --- > Changes for v2: > - Removal of cpufreq_boost structure and move its fields to cpufreq_driver > structure > - Flag to indicate if global boost attribute is already defined > - Extent the pr_{err|debbug} functions to show current function names > --- You don't have to manually add "---" here. Just keep a blank line instead. > drivers/cpufreq/cpufreq.c | 69 ++++++++++++++++++++++++++++++++++++++++++ > drivers/cpufreq/freq_table.c | 57 ++++++++++++++++++++++++++++++++-- > include/linux/cpufreq.h | 12 ++++++++ > 3 files changed, 136 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 1b8a48e..98ba5f1 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -40,6 +40,7 @@ > * also protects the cpufreq_cpu_data array. > */ > static struct cpufreq_driver *cpufreq_driver; > +static bool cpufreq_boost_sysfs_defined; > static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); > #ifdef CONFIG_HOTPLUG_CPU > /* This one keeps track of the previously set governor of a removed CPU */ > @@ -315,6 +316,33 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); > /********************************************************************* > * SYSFS INTERFACE * > *********************************************************************/ > +ssize_t show_boost_status(struct kobject *kobj, > + struct attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", cpufreq_driver->boost_en); This isn't correct. It shows if cpufreq driver supports boost or not and it should show if boost is enabled from sysfs when cpufreq driver supports boost. > +} > + > +static ssize_t store_boost_status(struct kobject *kobj, struct attribute *attr, > + const char *buf, size_t count) > +{ > + int ret, enable; > + > + ret = sscanf(buf, "%d", &enable); > + if (ret != 1 || enable < 0 || enable > 1) > + return -EINVAL; > + > + if (cpufreq_boost_trigger_state(enable)) { > + pr_err("%s: Cannot %sable boost!\n", __func__, > + enable ? "en" : "dis"); %sable doesn't look very much readable. Use complete strings: "enable" and "disable". > + return -EINVAL; > + } > + > + return count; > +} > + > +static struct global_attr global_boost = __ATTR(boost, 0644, > + show_boost_status, > + store_boost_status); User define_one_global_rw. > static struct cpufreq_governor *__find_governor(const char *str_governor) > { > @@ -754,6 +782,17 @@ static int cpufreq_add_dev_interface(unsigned int cpu, Why not do this in cpufreq_register_driver()? > goto err_out_kobj_put; > drv_attr++; > } > + if (cpufreq_driver->low_level_boost && !cpufreq_boost_sysfs_defined) { I thought low_level_boost() is a function which will only be supported for drivers using hardware boost feature, like intel. And so we must have used boost_en here. > + ret = sysfs_create_file(cpufreq_global_kobject, > + &(global_boost.attr)); cpufreq_sysfs_create_file(), check 2361be23666232dbb4851a527f466c4cbf5340fc for details. > + if (ret) { > + pr_err("%s: cannot register global boost sysfs file\n", > + __func__); > + goto err_out_kobj_put; > + } > + cpufreq_boost_sysfs_defined = 1; > + } > + > if (cpufreq_driver->get) { > ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr); > if (ret) > @@ -1853,6 +1892,30 @@ static struct notifier_block __refdata cpufreq_cpu_notifier = { > }; > > /********************************************************************* > + * BOOST * > + *********************************************************************/ > +int cpufreq_boost_trigger_state(int state) > +{ > + int ret = 0; > + > + if (!cpufreq_driver->low_level_boost) > + return -ENODEV; I am certainly not aligned with your design. What's the use of this field? And please update documentation too for these new entries in cpufreq_driver structure. > + if (cpufreq_driver->boost_en != state) { So, you are using boost_en to see if boost is enabled from sysfs? Then you have put it at wrong place. I thought there would be three variables: - cpufreq_driver->boost_supported: boost is enabled for driver - cpufreq_driver->low_level_boost(): to set desired boost state (Only for hardware boosting) - boost_enabled: global variable in cpufreq.c file, used only if cpufreq_driver->boost_supported is true. > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c > index d7a7966..4e4f692 100644 > --- a/drivers/cpufreq/freq_table.c > +++ b/drivers/cpufreq/freq_table.c > @@ -20,6 +20,27 @@ > * FREQUENCY TABLE HELPERS * > *********************************************************************/ > > +unsigned int > +cpufreq_frequency_table_max(struct cpufreq_frequency_table *freq_table, > + int boost) > +{ > + int i = 0, boost_freq_max = 0, freq_max = 0; > + > + for (; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) { > + if (freq_table[i].index == CPUFREQ_BOOST_FREQ) { > + if (freq_table[i].frequency > boost_freq_max) > + boost_freq_max = freq_table[i].frequency; Do above only when boost==true and below when boost==false. > + } else { > + if (freq_table[i].frequency > freq_max) > + freq_max = freq_table[i].frequency; > + } > + } > + > + return boost ? boost_freq_max : freq_max; > + > +} > +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_max); > + > int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, > struct cpufreq_frequency_table *table) > { > @@ -171,7 +192,8 @@ static DEFINE_PER_CPU(struct cpufreq_frequency_table *, cpufreq_show_table); > /** > * show_available_freqs - show available frequencies for the specified CPU > */ > -static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) > +static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf, > + int show_boost) > { > unsigned int i = 0; > unsigned int cpu = policy->cpu; > @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) > for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) { > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > continue; > + if (show_boost) > + if (table[i].index != CPUFREQ_BOOST_FREQ) > + continue; > + Looks wrong. You will show boost freqs when show_boost is false. > count += sprintf(&buf[count], "%d ", table[i].frequency); > } > count += sprintf(&buf[count], "\n"); > > return count; > +} > > +/** > + * show_available_normal_freqs - show normal boost frequencies for > + * the specified CPU > + */ > +static ssize_t show_available_normal_freqs(struct cpufreq_policy *policy, > + char *buf) > +{ > + return show_available_freqs(policy, buf, 0); > } > > struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { > .attr = { .name = "scaling_available_frequencies", > .mode = 0444, > }, > - .show = show_available_freqs, > + .show = show_available_normal_freqs, > }; > EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); > > +/** > + * show_available_boost_freqs - show available boost frequencies for > + * the specified CPU > + */ > +static ssize_t show_available_boost_freqs(struct cpufreq_policy *policy, > + char *buf) > +{ > + return show_available_freqs(policy, buf, 1); > +} > + > +struct freq_attr cpufreq_freq_attr_boost_available_freqs = { > + .attr = { .name = "scaling_boost_frequencies", > + .mode = 0444, > + }, > + .show = show_available_boost_freqs, > +}; > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_boost_available_freqs); > + Code redundancy can be reduced by creating a macro for declaring **_availabe_freqs, its attributes and export symbol. > /* > * if you use these, you must assure that the frequency table is valid > * all the time between get_attr and put_attr! With this patch alone, we would be using boost frequencies even in normal cases where we haven't enabled boost. -- 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
Hi Viresh, > Hi, > > Change subject to: "cpufreq: Add boost frequency support in core" Ok. > > On 11 June 2013 14:33, Lukasz Majewski <l.majewski@samsung.com> wrote: > > This commit adds support for software based frequency boosting. > > No. It adds support for both software and hardware boosting. So just > write: This commit adds boost frequency support in cpufreq core > (Hardware & Software). Ok. > > > Some SoC (like Exynos4 - e.g. 4x12) allow setting frequency above > > its normal condition limits. Such a change shall be only done for a > > short > > s/condition/operation > s/change/mode > s/done/used > Ok. > > time. > > > > Overclocking (boost) support is essentially provided by platform > > dependent cpufreq driver. > > > > This commit unifies support for SW and HW (Intel) over clocking > > solutions in the core cpufreq driver. Previously the "boost" sysfs > > attribute was defined at acpi driver code. > > By default boost is disabled. One global attribute is available at: > > /sys/devices/system/cpu/cpufreq/boost. > > Enter a blank line here. Ok > > > It only shows up when cpufreq driver supports overclocking. > > Under the hood frequencies dedicated for boosting are marked with a > > special flag (CPUFREQ_BOOST_FREQ) at driver's frequency table. > > It is the user's concern to enable/disable overclocking with proper > > call to sysfs. > > Good. > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > > > > --- ^^^^ [*] this --- was added manually by me. > > Changes for v2: > > - Removal of cpufreq_boost structure and move its fields to > > cpufreq_driver structure > > - Flag to indicate if global boost attribute is already defined > > - Extent the pr_{err|debbug} functions to show current function > > names --- > > You don't have to manually add "---" here. Just keep a blank line > instead. One "---" is added by git automatically. The [*] was added to distinct the changelog from rest of the commit. At least older versions of GIT required this to not include changelog to commit messages. > > > drivers/cpufreq/cpufreq.c | 69 > > ++++++++++++++++++++++++++++++++++++++++++ > > drivers/cpufreq/freq_table.c | 57 > > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | > > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 1b8a48e..98ba5f1 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -40,6 +40,7 @@ > > * also protects the cpufreq_cpu_data array. > > */ > > static struct cpufreq_driver *cpufreq_driver; > > +static bool cpufreq_boost_sysfs_defined; > > static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); > > #ifdef CONFIG_HOTPLUG_CPU > > /* This one keeps track of the previously set governor of a > > removed CPU */ @@ -315,6 +316,33 @@ > > EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /********************************************************************* > > * SYSFS > > INTERFACE * > > *********************************************************************/ > > +ssize_t show_boost_status(struct kobject *kobj, > > + struct attribute *attr, char *buf) > > +{ > > + return sprintf(buf, "%d\n", cpufreq_driver->boost_en); > > This isn't correct. It shows if cpufreq driver supports boost or > not and it should show if boost is enabled from sysfs when > cpufreq driver supports boost. The cpufreq_driver->low_level_boost() is only valid when cpufreq driver supports boost. Otherwise it is NULL. Thereof you will not see those attributes exported to /sysfs until cpufreq driver supports boost. When "boost" attribute is exported - then it returns state of boosting (if it is enabled or not). > > > +} > > + > > +static ssize_t store_boost_status(struct kobject *kobj, struct > > attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int ret, enable; > > + > > + ret = sscanf(buf, "%d", &enable); > > + if (ret != 1 || enable < 0 || enable > 1) > > + return -EINVAL; > > + > > + if (cpufreq_boost_trigger_state(enable)) { > > + pr_err("%s: Cannot %sable boost!\n", __func__, > > + enable ? "en" : "dis"); > > %sable doesn't look very much readable. Use complete strings: > "enable" and "disable". Ok. > > > + return -EINVAL; > > + } > > + > > + return count; > > +} > > + > > +static struct global_attr global_boost = __ATTR(boost, 0644, > > + show_boost_status, > > + store_boost_status); > > User define_one_global_rw. Ok, will reuse available macros (this code was taken directly from acpi-cpufreq.c file). > > > static struct cpufreq_governor *__find_governor(const char > > *str_governor) { > > @@ -754,6 +782,17 @@ static int cpufreq_add_dev_interface(unsigned > > int cpu, > > Why not do this in cpufreq_register_driver()? Good point. I will move this code to cpufreq_register_driver. > > > goto err_out_kobj_put; > > drv_attr++; > > } > > + if (cpufreq_driver->low_level_boost > > && !cpufreq_boost_sysfs_defined) { > > I thought low_level_boost() is a function which will only be supported > for drivers > using hardware boost feature, like intel. I think that we shall give users some flexibility and don't assume that low_level_boost is only used for one solution/vendor. It is also needed with software controlled boost. Please refer to patch 3/3. > And so we must have used > boost_en here. boost_en has two meanings: 0 - either boost disabled or not supported (when low_level_boost=NULL). 1 - boost is enabled. > > > + ret = sysfs_create_file(cpufreq_global_kobject, > > + &(global_boost.attr)); > > cpufreq_sysfs_create_file(), check > 2361be23666232dbb4851a527f466c4cbf5340fc for details. Ok, I will rebase those changes to newest kernel/git/rafael/linux-pm.git ,branch kernel_pm/pm-cpufreq > > > + if (ret) { > > + pr_err("%s: cannot register global boost > > sysfs file\n", > > + __func__); > > + goto err_out_kobj_put; > > + } > > + cpufreq_boost_sysfs_defined = 1; > > + } > > + > > if (cpufreq_driver->get) { > > ret = sysfs_create_file(&policy->kobj, > > &cpuinfo_cur_freq.attr); if (ret) > > @@ -1853,6 +1892,30 @@ static struct notifier_block __refdata > > cpufreq_cpu_notifier = { }; > > > > /********************************************************************* > > + * > > BOOST * > > + > > *********************************************************************/ > > +int cpufreq_boost_trigger_state(int state) +{ > > + int ret = 0; > > + > > + if (!cpufreq_driver->low_level_boost) > > + return -ENODEV; > > I am certainly not aligned with your design. What's the > use of this field? I had to rewrite a bit :-) patches since we decided to drop struct cpufreq_boost. I've added two fields to cpufreq_driver: - low_level_boost: * When boost is not supported (default) it is set to NULL. This field has two purposes: Indicates if boost is available on the system and provides address of low level callback * When cpufreq driver assigns pointer to this field, it means that it is supported - boost_en: * It shows if boost is enabled or disabled/not supported to the rest of the system. > And please update documentation too for these > new entries in cpufreq_driver structure. Ok I will extend it. > > > + if (cpufreq_driver->boost_en != state) { > > So, you are using boost_en to see if boost is enabled from sysfs? > Then you have put it at wrong place. I check if boost is already enabled. > > I thought there would be three variables: > - cpufreq_driver->boost_supported: boost is enabled for driver For this purpose I check the low_level_boost pointer if it's NULL or not. > - cpufreq_driver->low_level_boost(): to set desired boost state > (Only for hardware boosting) It has two purposes (as described above) and can be used (defined) by software and hardware boost solutions. > - boost_enabled: global variable in cpufreq.c file, used only if > cpufreq_driver->boost_supported is true. I think that boost enabled flag shall be defined at cpufreq driver (please look into acpi_cpufreq.c - which used another set of global flags). It will then provide one flag for cpufreq.c (core) and drivers. However I've added one global flag: cpufreq_boost_sysfs_defined which indicates if /sys/devices/system/cpu/cpufreq/boost attribute has been already defined (to prevent multiple definitions attempts). > > > > diff --git a/drivers/cpufreq/freq_table.c > > b/drivers/cpufreq/freq_table.c index d7a7966..4e4f692 100644 > > --- a/drivers/cpufreq/freq_table.c > > +++ b/drivers/cpufreq/freq_table.c > > @@ -20,6 +20,27 @@ > > * FREQUENCY TABLE > > HELPERS * > > *********************************************************************/ > > > > +unsigned int > > +cpufreq_frequency_table_max(struct cpufreq_frequency_table > > *freq_table, > > + int boost) > > +{ > > + int i = 0, boost_freq_max = 0, freq_max = 0; > > + > > + for (; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) { > > + if (freq_table[i].index == CPUFREQ_BOOST_FREQ) { > > + if (freq_table[i].frequency > > > boost_freq_max) > > + boost_freq_max = > > freq_table[i].frequency; > > Do above only when boost==true and below when boost==false. Ok. > > > + } else { > > + if (freq_table[i].frequency > freq_max) > > + freq_max = freq_table[i].frequency; > > + } > > + } > > + > > + return boost ? boost_freq_max : freq_max; > > + > > +} > > +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_max); > > + > > int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, > > struct cpufreq_frequency_table > > *table) { > > @@ -171,7 +192,8 @@ static DEFINE_PER_CPU(struct > > cpufreq_frequency_table *, cpufreq_show_table); /** > > * show_available_freqs - show available frequencies for the > > specified CPU */ > > -static ssize_t show_available_freqs(struct cpufreq_policy *policy, > > char *buf) +static ssize_t show_available_freqs(struct > > cpufreq_policy *policy, char *buf, > > + int show_boost) > > { > > unsigned int i = 0; > > unsigned int cpu = policy->cpu; > > @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct > > cpufreq_policy *policy, char *buf) for (i = 0; > > (table[i].frequency != CPUFREQ_TABLE_END); i++) { if > > (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; > > + if (show_boost) > > + if (table[i].index != CPUFREQ_BOOST_FREQ) > > + continue; > > + > > Looks wrong. You will show boost freqs when show_boost is false. My purpose here is to display frequencies only tagged with CPUFREQ_BOOST_FREQ and when show_boost is true. When show_boost is false, the operation of the function is unchanged. > > > count += sprintf(&buf[count], "%d ", > > table[i].frequency); } > > count += sprintf(&buf[count], "\n"); > > > > return count; > > +} > > > > +/** > > + * show_available_normal_freqs - show normal boost frequencies for > > + * the specified CPU > > + */ > > +static ssize_t show_available_normal_freqs(struct cpufreq_policy > > *policy, > > + char *buf) > > +{ > > + return show_available_freqs(policy, buf, 0); > > } > > > > struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { > > .attr = { .name = "scaling_available_frequencies", > > .mode = 0444, > > }, > > - .show = show_available_freqs, > > + .show = show_available_normal_freqs, > > }; > > EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); > > > > +/** > > + * show_available_boost_freqs - show available boost frequencies > > for > > + * the specified CPU > > + */ > > +static ssize_t show_available_boost_freqs(struct cpufreq_policy > > *policy, > > + char *buf) > > +{ > > + return show_available_freqs(policy, buf, 1); > > +} > > + > > +struct freq_attr cpufreq_freq_attr_boost_available_freqs = { > > + .attr = { .name = "scaling_boost_frequencies", > > + .mode = 0444, > > + }, > > + .show = show_available_boost_freqs, > > +}; > > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_boost_available_freqs); > > + > > Code redundancy can be reduced by creating a macro for declaring > **_availabe_freqs, its attributes and export symbol. Yes, you are right here. Those two structures only differ with different names. > > > /* > > * if you use these, you must assure that the frequency table is > > valid > > * all the time between get_attr and put_attr! > > With this patch alone, we would be using boost frequencies even in > normal cases where we haven't enabled boost. Correct me if I'm wrong here, but the cpufreq_freq_attr_boost_available_freqs will be added to cpufreq driver's freq_attr table (i.e. *exynos_cpufreq_attr[]). It is cpufreq driver's responsibility to add this attribute. By default all other drivers add only cpufreq_freq_attr_boost_available_freqs.
On 12 June 2013 13:09, Lukasz Majewski <l.majewski@samsung.com> wrote: > Hi Viresh, Hi Lukasz, >> Hi, Please don't remove the line which says, who wrote last mail at what time. These >, >>, >>>, >>>>, ... have a meaning. They help us understand who wrote what in bottom posting. And as you removed my line, nobody can see who wrote above "Hi" to you :) >> You don't have to manually add "---" here. Just keep a blank line >> instead. > > One "---" is added by git automatically. The [*] was added to distinct > the changelog from rest of the commit. At least older versions of GIT > required this to not include changelog to commit messages. You don't have to add an extra "---" line. Just write your changelog after "---" added by git and give a blank line between your last changelog line and below ones (probably just to make it more readable and not a must, but i am not sure). >> > drivers/cpufreq/cpufreq.c | 69 >> > ++++++++++++++++++++++++++++++++++++++++++ >> > drivers/cpufreq/freq_table.c | 57 >> > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | >> > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) > I think that we shall give users some flexibility and don't assume that > low_level_boost is only used for one solution/vendor. > > It is also needed with software controlled boost. Please refer to patch > 3/3. You didn't get me. I am not asking to keep it only for Intel. But keep this variable as is (s/low_level_boost/set_boost_freq), and make it optional. So, few drivers can implement it but not everybody is required to. So, Add another variable: boost_supported, which will tell cpufreq core that boost is supported by governor or not. And a global variable in cpufreq.c boost_enabled to track status of what user has requested. > However I've added one global flag: cpufreq_boost_sysfs_defined > which indicates if /sys/devices/system/cpu/cpufreq/boost attribute has > been already defined (to prevent multiple definitions attempts). You don't need this variable anymore as sysfs create file is now moved to cpufreq_register_driver(), so this can't be called twice. >> > char *buf) +static ssize_t show_available_freqs(struct >> > cpufreq_policy *policy, char *buf, >> > + int show_boost) >> > { >> > unsigned int i = 0; >> > unsigned int cpu = policy->cpu; >> > @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct >> > cpufreq_policy *policy, char *buf) for (i = 0; >> > (table[i].frequency != CPUFREQ_TABLE_END); i++) { if >> > (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; >> > + if (show_boost) >> > + if (table[i].index != CPUFREQ_BOOST_FREQ) >> > + continue; >> > + >> >> Looks wrong. You will show boost freqs when show_boost is false. > > My purpose here is to display frequencies only tagged with > CPUFREQ_BOOST_FREQ and when show_boost is true. > > When show_boost is false, the operation of the function is unchanged. Which is wrong. When show_boost is false, it means that user don't want to see boost frequencies and so you should skip them. >> With this patch alone, we would be using boost frequencies even in >> normal cases where we haven't enabled boost. > > Correct me if I'm wrong here, but the > cpufreq_freq_attr_boost_available_freqs will be added to cpufreq > driver's freq_attr table (i.e. *exynos_cpufreq_attr[]). > It is cpufreq driver's responsibility to add this attribute. By default > all other drivers add only cpufreq_freq_attr_boost_available_freqs. You are just talking about showing boost freqs in sysfs. I am talking about the frequencies that governors will select when boost is disabled from sysfs. Because we don't skip boost frequencies in target() routines, we will set them as and when governor requests them. -- 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
On Wed, 12 Jun 2013 13:39:18 +0530, Viresh Kumar wrote: Hi Viresh, > On 12 June 2013 13:09, Lukasz Majewski <l.majewski@samsung.com> wrote: > > Hi Viresh, > > Hi Lukasz, > > >> Hi, > > Please don't remove the line which says, who wrote last mail at what > time. These >, >>, >>>, >>>>, ... have a meaning. They help us > understand who wrote what in bottom posting. And as you removed my > line, nobody can see who wrote above "Hi" to you :) Ok, mailer adjusted. > > >> You don't have to manually add "---" here. Just keep a blank line > >> instead. > > > > One "---" is added by git automatically. The [*] was added to > > distinct the changelog from rest of the commit. At least older > > versions of GIT required this to not include changelog to commit > > messages. > > You don't have to add an extra "---" line. Just write your changelog > after "---" added by git and give a blank line between your last > changelog line and below ones (probably just to make it more > readable and not a must, but i am not sure). I got your point. If you prefer, I will stick to it. No problem. As a side note: In the other open source project (u-boot) we use the pattern which I've used previously. It has one big advantage - I can edit change log at git gui (just below sign-of-by). It is simply more convenient for me :-). Those changes between "---" are simply skipped by git am afterwards. > > >> > drivers/cpufreq/cpufreq.c | 69 > >> > ++++++++++++++++++++++++++++++++++++++++++ > >> > drivers/cpufreq/freq_table.c | 57 > >> > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | > >> > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) > > > I think that we shall give users some flexibility and don't assume > > that low_level_boost is only used for one solution/vendor. > > > > It is also needed with software controlled boost. Please refer to > > patch 3/3. > > You didn't get me. I am not asking to keep it only for Intel. But keep > this variable as is (s/low_level_boost/set_boost_freq), and make it > optional. So, few drivers can implement it but not everybody is > required to. The low_level_boost (set_boost_freq)[*] is optional. However it seems to me, that the burden of changing available set of frequencies (when boost is enabled) must be put to cpufreq driver anyway. Without this function [*] defined, we cannot enable frequency boosting. > > So, Add another variable: boost_supported, which will tell cpufreq > core that boost is supported by governor or not. ^^^^^^^shouldn't it be cpufreq driver? Ok, boost_supported seems needed. In my opinion it shall be defined at cpufreq_driver structure (since it provides boosting infrastructure anyway). > > And a global variable in cpufreq.c boost_enabled to track status of > what user has requested. I think, that boost_enable shall be also defined at cpufreq driver (as proposed in the patch). We keep pointer to cpufreq driver at cpufreq.c anyway. Moreover, boost_enable flag is already defined at acpi-cpufreq.c (as static). We will have two flags for the same purpose. > > > However I've added one global flag: cpufreq_boost_sysfs_defined > > which indicates if /sys/devices/system/cpu/cpufreq/boost attribute > > has been already defined (to prevent multiple definitions attempts). > > You don't need this variable anymore as sysfs create file is now > moved to cpufreq_register_driver(), so this can't be called twice. Yes, in this situation the cpufreq_boost_sysfs_defined flag is redundant. > > >> > char *buf) +static ssize_t show_available_freqs(struct > >> > cpufreq_policy *policy, char *buf, > >> > + int show_boost) > >> > { > >> > unsigned int i = 0; > >> > unsigned int cpu = policy->cpu; > >> > @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct > >> > cpufreq_policy *policy, char *buf) for (i = 0; > >> > (table[i].frequency != CPUFREQ_TABLE_END); i++) { if > >> > (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; > >> > + if (show_boost) > >> > + if (table[i].index != CPUFREQ_BOOST_FREQ) > >> > + continue; > >> > + > >> > >> Looks wrong. You will show boost freqs when show_boost is false. > > > > My purpose here is to display frequencies only tagged with > > CPUFREQ_BOOST_FREQ and when show_boost is true. > > > > When show_boost is false, the operation of the function is > > unchanged. > > Which is wrong. When show_boost is false, it means that user don't > want to see boost frequencies and so you should skip them. So we want as follows: show_boost = 1 ---> show only frequencies tagged as CPUFREQ_BOOST_FREQ show_boost = 0 ---> show only "normal" (non boost) frequencies > > >> With this patch alone, we would be using boost frequencies even in > >> normal cases where we haven't enabled boost. > > > > Correct me if I'm wrong here, but the > > cpufreq_freq_attr_boost_available_freqs will be added to cpufreq > > driver's freq_attr table (i.e. *exynos_cpufreq_attr[]). > > It is cpufreq driver's responsibility to add this attribute. By > > default all other drivers add only > > cpufreq_freq_attr_boost_available_freqs. > > You are just talking about showing boost freqs in sysfs. I am talking > about the frequencies that governors will select when boost is > disabled from sysfs. Because we don't skip boost frequencies in > target() routines, we will set them as and when governor requests > them. I think, that it is the main issue here and it shall be cleared out: Frequencies marked as: CPUFREQ_BOOST_FREQ are added permanently to the freq_table. That is the distinction to the original overclocking patch posted with LAB, where freq_boost structure was modified and boost frequencies were either valid or invalid. Then we can in SW control boost in two ways: 1. change policy->max value (to the maximal boost frequency) - as it is done now (v3) at Exynos. This is the simple solution (patch 3/3) 2. Modify all freq_table helper functions to be aware of boost and skip boost frequencies when boost_enable = 0. (as it was done at v2). This requires code modification at freq_table.c and reevaluation of policy. Maybe you have any other idea?
On 12 June 2013 14:39, Lukasz Majewski <l.majewski@samsung.com> wrote: > On Wed, 12 Jun 2013 13:39:18 +0530, Viresh Kumar wrote: >> On 12 June 2013 13:09, Lukasz Majewski <l.majewski@samsung.com> wrote: >> You don't have to add an extra "---" line. Just write your changelog >> after "---" added by git and give a blank line between your last >> changelog line and below ones (probably just to make it more >> readable and not a must, but i am not sure). > > I got your point. If you prefer, I will stick to it. No problem. Its not how I prefer it, but how everybody does it :) > As a side note: > > In the other open source project (u-boot) we use the pattern which I've > used previously. It has one big advantage - I can edit change log at > git gui (just below sign-of-by). It is simply more convenient for > me :-). Those changes between "---" are simply skipped by git am > afterwards. Yes, I am still asking you to follow the same steps: git send-email --annotate ***patches*** and then write whatever you don't want to be logged by git am after the "---" already present in the patch. >> >> > drivers/cpufreq/cpufreq.c | 69 >> >> > ++++++++++++++++++++++++++++++++++++++++++ >> >> > drivers/cpufreq/freq_table.c | 57 >> >> > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | >> >> > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) >> >> > I think that we shall give users some flexibility and don't assume >> > that low_level_boost is only used for one solution/vendor. >> > >> > It is also needed with software controlled boost. Please refer to >> > patch 3/3. >> >> You didn't get me. I am not asking to keep it only for Intel. But keep >> this variable as is (s/low_level_boost/set_boost_freq), and make it >> optional. So, few drivers can implement it but not everybody is >> required to. > > The low_level_boost (set_boost_freq)[*] is optional. However it seems to > me, that the burden of changing available set of frequencies (when > boost is enabled) must be put to cpufreq driver anyway. Driver shouldn't play with boost freqs at runtime. This has to be handled by cpufreq core. The only thing cpufreq driver must be doing in low_level_boost or set_boost_freq (as what I suggested it to be), is to set the boost frequency requested by core. And so this routine would only be defined by drivers that have a special way of setting boost frequencies. For others ->target() routine should be able to set all freqs, boost or non boost. > Without this function [*] defined, we cannot enable frequency boosting. why? >> So, Add another variable: boost_supported, which will tell cpufreq >> core that boost is supported by governor or not. > ^^^^^^^shouldn't it be cpufreq driver? Yeah, sorry :( > Ok, boost_supported seems needed. In my opinion it shall be defined at > cpufreq_driver structure (since it provides boosting infrastructure > anyway). that's what I asked. >> And a global variable in cpufreq.c boost_enabled to track status of >> what user has requested. > > I think, that boost_enable shall be also defined at cpufreq driver (as > proposed in the patch). Not really. Driver should only care about if it supports boost or not. If it is enabled/disabled by sysfs or not should be kept inside core in a global variable. Moreover, if we have 5-6 cpufreq drivers compiled in (for multi-arch compiled kernels), we will save memory wasted by this variable if it is present in cpufreq_driver. > Moreover, boost_enable flag is already defined at > acpi-cpufreq.c (as static). We will have two flags for the same > purpose. No, we don't have to. Just expose another API from cpufreq core to get status of boost. > So we want as follows: > show_boost = 1 ---> show only frequencies tagged as CPUFREQ_BOOST_FREQ > show_boost = 0 ---> show only "normal" (non boost) frequencies Yes. >> You are just talking about showing boost freqs in sysfs. I am talking >> about the frequencies that governors will select when boost is >> disabled from sysfs. Because we don't skip boost frequencies in >> target() routines, we will set them as and when governor requests >> them. > > I think, that it is the main issue here and it shall be cleared out: > > Frequencies marked as: CPUFREQ_BOOST_FREQ are added permanently to the > freq_table. > That is the distinction to the original overclocking patch posted with >> LAB, where freq_boost structure was modified and boost frequencies were > either valid or invalid. Yes. > Then we can in SW control boost in two ways: > 1. change policy->max value (to the maximal boost frequency) - as it is > done now (v3) at Exynos. This is the simple solution (patch 3/3) Drivers aren't supposed to set policy->max. It should be taken care of by core or freq_table.c file. > 2. Modify all freq_table helper functions to be aware of boost and > skip boost frequencies when boost_enable = 0. (as it was done at v2). > This requires code modification at freq_table.c and reevaluation of > policy. Yes, the core must be aware of it and must take the right decision here. So, we need to take care of boost freq in freq_table.c -- 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
On Wednesday, June 12, 2013 09:39:38 AM Lukasz Majewski wrote: > Hi Viresh, > > > Hi, > > > > Change subject to: "cpufreq: Add boost frequency support in core" > > Ok. > > > > > On 11 June 2013 14:33, Lukasz Majewski <l.majewski@samsung.com> wrote: > > > This commit adds support for software based frequency boosting. > > > > No. It adds support for both software and hardware boosting. So just > > write: This commit adds boost frequency support in cpufreq core > > (Hardware & Software). > > Ok. > > > > > Some SoC (like Exynos4 - e.g. 4x12) allow setting frequency above > > > its normal condition limits. Such a change shall be only done for a > > > short > > > > s/condition/operation > > s/change/mode > > s/done/used > > > > Ok. > > > > time. > > > > > > Overclocking (boost) support is essentially provided by platform > > > dependent cpufreq driver. > > > > > > This commit unifies support for SW and HW (Intel) over clocking > > > solutions in the core cpufreq driver. Previously the "boost" sysfs > > > attribute was defined at acpi driver code. > > > By default boost is disabled. One global attribute is available at: > > > /sys/devices/system/cpu/cpufreq/boost. > > > > Enter a blank line here. > > Ok > > > > > > It only shows up when cpufreq driver supports overclocking. > > > Under the hood frequencies dedicated for boosting are marked with a > > > special flag (CPUFREQ_BOOST_FREQ) at driver's frequency table. > > > It is the user's concern to enable/disable overclocking with proper > > > call to sysfs. > > > > Good. > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > > > > > > --- > ^^^^ > [*] this --- was added manually by me. > > > > Changes for v2: > > > - Removal of cpufreq_boost structure and move its fields to > > > cpufreq_driver structure > > > - Flag to indicate if global boost attribute is already defined > > > - Extent the pr_{err|debbug} functions to show current function > > > names --- > > > > You don't have to manually add "---" here. Just keep a blank line > > instead. > > One "---" is added by git automatically. The [*] was added to distinct > the changelog from rest of the commit. At least older versions of GIT > required this to not include changelog to commit messages. Which doesn't matter anyway. None of them will show up in git log, so don't worry. :-) [...] > > > And so we must have used > > boost_en here. > > boost_en has two meanings: > 0 - either boost disabled or not supported (when low_level_boost=NULL). > 1 - boost is enabled. Gosh, please don't do that. Use *two* flags, one meaning "supported" and the second meaning "enabled". Thanks, Rafael
On Wed, 12 Jun 2013 13:23:20 +0200, Rafael J. Wysocki wrote: Hi Rafael, > On Wednesday, June 12, 2013 09:39:38 AM Lukasz Majewski wrote: > > Hi Viresh, > > > > > Hi, > > > > > > Change subject to: "cpufreq: Add boost frequency support in core" > > > > Ok. > > > > > > > > On 11 June 2013 14:33, Lukasz Majewski <l.majewski@samsung.com> > > > wrote: > > > > This commit adds support for software based frequency boosting. > > > > > > No. It adds support for both software and hardware boosting. So > > > just write: This commit adds boost frequency support in cpufreq > > > core (Hardware & Software). > > > > Ok. > > > > > > > Some SoC (like Exynos4 - e.g. 4x12) allow setting frequency > > > > above its normal condition limits. Such a change shall be only > > > > done for a short > > > > > > s/condition/operation > > > s/change/mode > > > s/done/used > > > > > > > Ok. > > > > > > time. > > > > > > > > Overclocking (boost) support is essentially provided by platform > > > > dependent cpufreq driver. > > > > > > > > This commit unifies support for SW and HW (Intel) over clocking > > > > solutions in the core cpufreq driver. Previously the "boost" > > > > sysfs attribute was defined at acpi driver code. > > > > By default boost is disabled. One global attribute is available > > > > at: /sys/devices/system/cpu/cpufreq/boost. > > > > > > Enter a blank line here. > > > > Ok > > > > > > > > > It only shows up when cpufreq driver supports overclocking. > > > > Under the hood frequencies dedicated for boosting are marked > > > > with a special flag (CPUFREQ_BOOST_FREQ) at driver's frequency > > > > table. It is the user's concern to enable/disable overclocking > > > > with proper call to sysfs. > > > > > > Good. > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > > > > > > > > --- > > ^^^^ > > [*] this --- was added manually by me. > > > > > > Changes for v2: > > > > - Removal of cpufreq_boost structure and move its fields to > > > > cpufreq_driver structure > > > > - Flag to indicate if global boost attribute is already defined > > > > - Extent the pr_{err|debbug} functions to show current function > > > > names --- > > > > > > You don't have to manually add "---" here. Just keep a blank line > > > instead. > > > > One "---" is added by git automatically. The [*] was added to > > distinct the changelog from rest of the commit. At least older > > versions of GIT required this to not include changelog to commit > > messages. > > Which doesn't matter anyway. None of them will show up in git log, > so don't worry. :-) :-) > > [...] > > > > > > And so we must have used > > > boost_en here. > > > > boost_en has two meanings: > > 0 - either boost disabled or not supported (when > > low_level_boost=NULL). 1 - boost is enabled. > > Gosh, please don't do that. Use *two* flags, one meaning "supported" > and the second meaning "enabled". That was already pointed out by Viresh. I will stick to his guidelines :-). > > Thanks, > Rafael > >
On Wed, 12 Jun 2013 14:55:45 +0530, Viresh Kumar wrote: > On 12 June 2013 14:39, Lukasz Majewski <l.majewski@samsung.com> wrote: > > On Wed, 12 Jun 2013 13:39:18 +0530, Viresh Kumar wrote: > >> On 12 June 2013 13:09, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > > >> You don't have to add an extra "---" line. Just write your > >> changelog after "---" added by git and give a blank line between > >> your last changelog line and below ones (probably just to make it > >> more readable and not a must, but i am not sure). > > > > I got your point. If you prefer, I will stick to it. No problem. > > Its not how I prefer it, but how everybody does it :) Ok, :-) > > > As a side note: > > > > In the other open source project (u-boot) we use the pattern which > > I've used previously. It has one big advantage - I can edit change > > log at git gui (just below sign-of-by). It is simply more > > convenient for me :-). Those changes between "---" are simply > > skipped by git am afterwards. > > Yes, I am still asking you to follow the same steps: > > git send-email --annotate ***patches*** > > and then write whatever you don't want to be logged by git am after > the "---" already present in the patch. Ok, > > >> >> > drivers/cpufreq/cpufreq.c | 69 > >> >> > ++++++++++++++++++++++++++++++++++++++++++ > >> >> > drivers/cpufreq/freq_table.c | 57 > >> >> > ++++++++++++++++++++++++++++++++-- > >> >> > include/linux/cpufreq.h | 12 ++++++++ 3 files changed, > >> >> > 136 insertions(+), 2 deletions(-) > >> > >> > I think that we shall give users some flexibility and don't > >> > assume that low_level_boost is only used for one solution/vendor. > >> > > >> > It is also needed with software controlled boost. Please refer to > >> > patch 3/3. > >> > >> You didn't get me. I am not asking to keep it only for Intel. But > >> keep this variable as is (s/low_level_boost/set_boost_freq), and > >> make it optional. So, few drivers can implement it but not > >> everybody is required to. > > > > The low_level_boost (set_boost_freq)[*] is optional. However it > > seems to me, that the burden of changing available set of > > frequencies (when boost is enabled) must be put to cpufreq driver > > anyway. > > Driver shouldn't play with boost freqs at runtime. This has to be > handled by cpufreq core. The only thing cpufreq driver must be doing > in low_level_boost or set_boost_freq (as what I suggested it to be), So you want the set_boost_freq to not be used with software based boosting. Ok. > is to set the boost frequency requested by core. And so this routine > would only be defined by drivers that have a special way of setting > boost frequencies. For others ->target() routine should be able to > set all freqs, boost or non boost. > > > Without this function [*] defined, we cannot enable frequency > > boosting. > > why? Hmm, please read my further comment. > > >> So, Add another variable: boost_supported, which will tell cpufreq > >> core that boost is supported by governor or not. > > ^^^^^^^shouldn't it be cpufreq > > driver? > > Yeah, sorry :( Ok. > > > Ok, boost_supported seems needed. In my opinion it shall be defined > > at cpufreq_driver structure (since it provides boosting > > infrastructure anyway). > > that's what I asked. Ok :-) . > > >> And a global variable in cpufreq.c boost_enabled to track status of > >> what user has requested. > > > > I think, that boost_enable shall be also defined at cpufreq driver > > (as proposed in the patch). > > Not really. Driver should only care about if it supports boost or not. > If it is enabled/disabled by sysfs or not should be kept inside core > in a global variable. Ok, > > Moreover, if we have 5-6 cpufreq drivers compiled in (for multi-arch > compiled kernels), we will save memory wasted by this variable if > it is present in cpufreq_driver. Ok. I didn't thought about this use case. Agree. > > > Moreover, boost_enable flag is already defined at > > acpi-cpufreq.c (as static). We will have two flags for the same > > purpose. > > No, we don't have to. Just expose another API from cpufreq core > to get status of boost. Ok. > > > So we want as follows: > > show_boost = 1 ---> show only frequencies tagged as > > CPUFREQ_BOOST_FREQ show_boost = 0 ---> show only "normal" (non > > boost) frequencies > > Yes. Ok, > > >> You are just talking about showing boost freqs in sysfs. I am > >> talking about the frequencies that governors will select when > >> boost is disabled from sysfs. Because we don't skip boost > >> frequencies in target() routines, we will set them as and when > >> governor requests them. > > > > I think, that it is the main issue here and it shall be cleared out: > > > > Frequencies marked as: CPUFREQ_BOOST_FREQ are added permanently to > > the freq_table. > > That is the distinction to the original overclocking patch posted > > with > >> LAB, where freq_boost structure was modified and boost frequencies > >> were > > either valid or invalid. > > Yes. > > > Then we can in SW control boost in two ways: > > 1. change policy->max value (to the maximal boost frequency) - as > > it is done now (v3) at Exynos. This is the simple solution (patch > > 3/3) > > Drivers aren't supposed to set policy->max. It should be taken care > of by core or freq_table.c file. This seems the right thing, but it would require some functions defined at freq_table.c to be extended to accept bool "boost" parameter (which would not consider freqs tagged as CPUFREQ_BOOST_FREQ). Then each per cpu policy, shall be read and its max freq shall be updated when someone enables boost from sysfs (at cpufreq_boost_trigger_state). This is my idea to manage boost mode from core code. Am I right? > > > 2. Modify all freq_table helper functions to be aware of boost and > > skip boost frequencies when boost_enable = 0. (as it was done at > > v2). This requires code modification at freq_table.c and > > reevaluation of policy. > > Yes, the core must be aware of it and must take the right decision > here. So, we need to take care of boost freq in freq_table.c Ok. Then we agreed :-).
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1b8a48e..98ba5f1 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -40,6 +40,7 @@ * also protects the cpufreq_cpu_data array. */ static struct cpufreq_driver *cpufreq_driver; +static bool cpufreq_boost_sysfs_defined; static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); #ifdef CONFIG_HOTPLUG_CPU /* This one keeps track of the previously set governor of a removed CPU */ @@ -315,6 +316,33 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /********************************************************************* * SYSFS INTERFACE * *********************************************************************/ +ssize_t show_boost_status(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", cpufreq_driver->boost_en); +} + +static ssize_t store_boost_status(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + int ret, enable; + + ret = sscanf(buf, "%d", &enable); + if (ret != 1 || enable < 0 || enable > 1) + return -EINVAL; + + if (cpufreq_boost_trigger_state(enable)) { + pr_err("%s: Cannot %sable boost!\n", __func__, + enable ? "en" : "dis"); + return -EINVAL; + } + + return count; +} + +static struct global_attr global_boost = __ATTR(boost, 0644, + show_boost_status, + store_boost_status); static struct cpufreq_governor *__find_governor(const char *str_governor) { @@ -754,6 +782,17 @@ static int cpufreq_add_dev_interface(unsigned int cpu, goto err_out_kobj_put; drv_attr++; } + if (cpufreq_driver->low_level_boost && !cpufreq_boost_sysfs_defined) { + ret = sysfs_create_file(cpufreq_global_kobject, + &(global_boost.attr)); + if (ret) { + pr_err("%s: cannot register global boost sysfs file\n", + __func__); + goto err_out_kobj_put; + } + cpufreq_boost_sysfs_defined = 1; + } + if (cpufreq_driver->get) { ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr); if (ret) @@ -1853,6 +1892,30 @@ static struct notifier_block __refdata cpufreq_cpu_notifier = { }; /********************************************************************* + * BOOST * + *********************************************************************/ +int cpufreq_boost_trigger_state(int state) +{ + int ret = 0; + + if (!cpufreq_driver->low_level_boost) + return -ENODEV; + + if (cpufreq_driver->boost_en != state) { + ret = cpufreq_driver->low_level_boost(state); + if (ret) { + pr_err("%s: BOOST cannot %sable low level code (%d)\n", + __func__, state ? "en" : "dis", ret); + return ret; + } + } + + pr_debug("%s: cpufreq BOOST %sabled\n", __func__, state ? "en" : "dis"); + + return 0; +} + +/********************************************************************* * REGISTER / UNREGISTER CPUFREQ DRIVER * *********************************************************************/ @@ -1947,6 +2010,12 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) pr_debug("unregistering driver %s\n", driver->name); subsys_interface_unregister(&cpufreq_interface); + + if (cpufreq_driver->low_level_boost && cpufreq_boost_sysfs_defined) { + sysfs_remove_file(cpufreq_global_kobject, &(global_boost.attr)); + cpufreq_boost_sysfs_defined = 0; + } + unregister_hotcpu_notifier(&cpufreq_cpu_notifier); write_lock_irqsave(&cpufreq_driver_lock, flags); diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c index d7a7966..4e4f692 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -20,6 +20,27 @@ * FREQUENCY TABLE HELPERS * *********************************************************************/ +unsigned int +cpufreq_frequency_table_max(struct cpufreq_frequency_table *freq_table, + int boost) +{ + int i = 0, boost_freq_max = 0, freq_max = 0; + + for (; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) { + if (freq_table[i].index == CPUFREQ_BOOST_FREQ) { + if (freq_table[i].frequency > boost_freq_max) + boost_freq_max = freq_table[i].frequency; + } else { + if (freq_table[i].frequency > freq_max) + freq_max = freq_table[i].frequency; + } + } + + return boost ? boost_freq_max : freq_max; + +} +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_max); + int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, struct cpufreq_frequency_table *table) { @@ -171,7 +192,8 @@ static DEFINE_PER_CPU(struct cpufreq_frequency_table *, cpufreq_show_table); /** * show_available_freqs - show available frequencies for the specified CPU */ -static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) +static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf, + int show_boost) { unsigned int i = 0; unsigned int cpu = policy->cpu; @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) { if (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; + if (show_boost) + if (table[i].index != CPUFREQ_BOOST_FREQ) + continue; + count += sprintf(&buf[count], "%d ", table[i].frequency); } count += sprintf(&buf[count], "\n"); return count; +} +/** + * show_available_normal_freqs - show normal boost frequencies for + * the specified CPU + */ +static ssize_t show_available_normal_freqs(struct cpufreq_policy *policy, + char *buf) +{ + return show_available_freqs(policy, buf, 0); } struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { .attr = { .name = "scaling_available_frequencies", .mode = 0444, }, - .show = show_available_freqs, + .show = show_available_normal_freqs, }; EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); +/** + * show_available_boost_freqs - show available boost frequencies for + * the specified CPU + */ +static ssize_t show_available_boost_freqs(struct cpufreq_policy *policy, + char *buf) +{ + return show_available_freqs(policy, buf, 1); +} + +struct freq_attr cpufreq_freq_attr_boost_available_freqs = { + .attr = { .name = "scaling_boost_frequencies", + .mode = 0444, + }, + .show = show_available_boost_freqs, +}; +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_boost_available_freqs); + /* * if you use these, you must assure that the frequency table is valid * all the time between get_attr and put_attr! diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..d045da2 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -262,6 +262,10 @@ struct cpufreq_driver { int (*suspend) (struct cpufreq_policy *policy); int (*resume) (struct cpufreq_policy *policy); struct freq_attr **attr; + + /* platform specific boost support code */ + bool boost_en; + int (*low_level_boost) (int state); }; /* flags */ @@ -403,6 +407,9 @@ extern struct cpufreq_governor cpufreq_gov_conservative; #define CPUFREQ_ENTRY_INVALID ~0 #define CPUFREQ_TABLE_END ~1 +/* Define index for boost frequency */ +#define CPUFREQ_BOOST_FREQ ~2 + struct cpufreq_frequency_table { unsigned int index; /* any */ unsigned int frequency; /* kHz - doesn't need to be in ascending @@ -421,11 +428,16 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int relation, unsigned int *index); +unsigned int +cpufreq_frequency_table_max(struct cpufreq_frequency_table *freq_table, int); +int cpufreq_boost_trigger_state(int state); + /* the following 3 funtions are for cpufreq core use only */ struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu); /* the following are really really optional */ extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs; +extern struct freq_attr cpufreq_freq_attr_boost_available_freqs; void cpufreq_frequency_table_get_attr(struct cpufreq_frequency_table *table, unsigned int cpu);