| Message ID | 1371661969-7660-3-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 06/19/2013 10:12 AM, Lukasz Majewski wrote: > This commit adds boost frequency support in cpufreq core (Hardware & > +/********************************************************************* > * REGISTER / UNREGISTER CPUFREQ DRIVER * > *********************************************************************/ > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) > cpufreq_driver = driver_data; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > + if (!cpufreq_driver->boost_supported) > + boost.attr.mode = 0444; > + > + ret = cpufreq_sysfs_create_file(&(boost.attr)); > + if (ret) { > + pr_err("%s: cannot register global boost sysfs file\n", > + __func__); > + goto err_null_driver; > + } > + I do not think the boost sysfs should be created at all if boost is not supported. For intel_pstate the read-only boost would be there for no reason and would cause confusion on the part of the user IMHO > ret = subsys_interface_register(&cpufreq_interface); > if (ret) > goto err_null_driver; > @@ -1992,6 +2085,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) > pr_debug("unregistering driver %s\n", driver->name); > > subsys_interface_unregister(&cpufreq_interface); > + > + cpufreq_sysfs_remove_file(&(boost.attr)); > unregister_hotcpu_notifier(&cpufreq_cpu_notifier); > -- 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, 19 Jun 2013 10:48:53 -0700 Dirk Brandewie <dirk.brandewie@gmail.com> wrote: > On 06/19/2013 10:12 AM, Lukasz Majewski wrote: > > This commit adds boost frequency support in cpufreq core (Hardware & > > > +/********************************************************************* > > * REGISTER / UNREGISTER CPUFREQ > > DRIVER * > > *********************************************************************/ > > > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct > > cpufreq_driver *driver_data) cpufreq_driver = driver_data; > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > + if (!cpufreq_driver->boost_supported) > > + boost.attr.mode = 0444; > > + > > + ret = cpufreq_sysfs_create_file(&(boost.attr)); > > + if (ret) { > > + pr_err("%s: cannot register global boost sysfs > > file\n", > > + __func__); > > + goto err_null_driver; > > + } > > + > > I do not think the boost sysfs should be created at all if boost is > not supported. This was my first thought. But unfortunately this "boost" attribute is always exported at acpi-cpufreq.c and in my opinion is part of a legacy API. I totally agree with the idea of exporting boost only when supported, but I would like to know the community opinion about this (especially Viresh and Rafael shall speak up). > > For intel_pstate the read-only boost would be there for no reason and > would cause confusion on the part of the user IMHO You are probably right here. However I don't know what was the original rationale behind exporting this attribute as read only. > > > ret = subsys_interface_register(&cpufreq_interface); > > if (ret) > > goto err_null_driver; > > @@ -1992,6 +2085,8 @@ int cpufreq_unregister_driver(struct > > cpufreq_driver *driver) pr_debug("unregistering driver %s\n", > > driver->name); > > > > subsys_interface_unregister(&cpufreq_interface); > > + > > + cpufreq_sysfs_remove_file(&(boost.attr)); > > unregister_hotcpu_notifier(&cpufreq_cpu_notifier); > > > Best regards, Lukasz Majewski -- 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 19, 2013 10:31:02 PM Lukasz Majewski wrote: > On Wed, 19 Jun 2013 10:48:53 -0700 > Dirk Brandewie <dirk.brandewie@gmail.com> wrote: > > > On 06/19/2013 10:12 AM, Lukasz Majewski wrote: > > > This commit adds boost frequency support in cpufreq core (Hardware & > > > > > +/********************************************************************* > > > * REGISTER / UNREGISTER CPUFREQ > > > DRIVER * > > > *********************************************************************/ > > > > > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct > > > cpufreq_driver *driver_data) cpufreq_driver = driver_data; > > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > > > + if (!cpufreq_driver->boost_supported) > > > + boost.attr.mode = 0444; > > > + > > > + ret = cpufreq_sysfs_create_file(&(boost.attr)); > > > + if (ret) { > > > + pr_err("%s: cannot register global boost sysfs > > > file\n", > > > + __func__); > > > + goto err_null_driver; > > > + } > > > + > > > > I do not think the boost sysfs should be created at all if boost is > > not supported. > > This was my first thought. But unfortunately this "boost" attribute is > always exported at acpi-cpufreq.c and in my opinion is part of a > legacy API. > > I totally agree with the idea of exporting boost only when supported, > but I would like to know the community opinion about this (especially > Viresh and Rafael shall speak up). Simple: Export it only when supported. Thanks, Rafael
On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> wrote: > Boost sysfs attribute is always exported (to support legacy API). By > default boost is exported as read only. One global attribute is available at: > /sys/devices/system/cpu/cpufreq/boost. You asked me and Rafael a question and posted your next version without even waiting for our replies? That will waste your time and ours too reviewing it. -- 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 Thursday, June 20, 2013 10:43:08 AM Viresh Kumar wrote: > On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> wrote: > > > Boost sysfs attribute is always exported (to support legacy API). By > > default boost is exported as read only. One global attribute is available at: > > /sys/devices/system/cpu/cpufreq/boost. > > You asked me and Rafael a question and posted your next version without > even waiting for our replies? That will waste your time and ours too > reviewing it. I believe I replied to that (in a different branch of the thread). And the reply was: Do not expose if not supported. Thanks, Rafael
On Thu, 20 Jun 2013 22:03:45 +0200, Rafael J. Wysocki wrote: Hi Rafael, > On Thursday, June 20, 2013 10:43:08 AM Viresh Kumar wrote: > > On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> > > wrote: > > > > > Boost sysfs attribute is always exported (to support legacy API). > > > By default boost is exported as read only. One global attribute > > > is available at: /sys/devices/system/cpu/cpufreq/boost. > > > > You asked me and Rafael a question and posted your next version > > without even waiting for our replies? That will waste your time and > > ours too reviewing it. > > I believe I replied to that (in a different branch of the thread). > > And the reply was: Do not expose if not supported. Thanks for reply. Understood :-) > > Thanks, > Rafael > >
On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> wrote: > Boost sysfs attribute is always exported (to support legacy API). By > default boost is exported as read only. One global attribute is available at: > /sys/devices/system/cpu/cpufreq/boost. I assume you are going to fix this as discussed in other threads. > Changes for v4: > - Remove boost parameter from cpufreq_frequency_table_cpuinfo() function > - Introduce cpufreq_boost_supported() method > - Use of cpufreq_boost_supported() and cpufreq_boost_enabled() to decide > if frequency shall be skipped > - Rename set_boost_freq() to enable_boost() > - cpufreq_attr_available_freq() moved to freq_table.c > - Use policy list to get access to cpufreq policies > - Rename global boost flag (cpufreq_boost_enabled -> boost_enabled) > - pr_err corrected ( %sable) > - Remove sanity check at cpufreq_boost_trigger_state() entrance [to test if > boost is supported] > - Use either HW (boost_enable) callback or SW managed boost > - Introduce new cpufreq_boost_trigger_state_sw() method to handle boost > at SW. > - Protect boost_enabled manipulation with lock > - Always export boost attribute (preserve legacy behaviour). When boost > is not supported this attribute is read only Very well written changelog. But write it after --- > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 665e641..9141d33 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 boost_enabled; > 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 */ > @@ -316,6 +317,29 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); > /********************************************************************* > * SYSFS INTERFACE * > *********************************************************************/ > +ssize_t show_boost(struct kobject *kobj, > + struct attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", boost_enabled); > +} > + > +static ssize_t store_boost(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 enable boost!\n", __func__); > + return -EINVAL; > + } Probably do boost_enabled = true here. > + return count; > +} > +define_one_global_rw(boost); > /********************************************************************* > + * BOOST * > + *********************************************************************/ > +static int cpufreq_boost_trigger_state_sw(void) > +{ > + struct cpufreq_frequency_table *freq_table; > + struct cpufreq_policy *policy; > + int ret = -EINVAL; > + > + list_for_each_entry(policy, &cpufreq_policy_list, policy_list) { > + freq_table = cpufreq_frequency_get_table(policy->cpu); > + if (freq_table) > + ret = cpufreq_frequency_table_cpuinfo(policy, > + freq_table); > + } > + > + return ret; > + > +} add blank line here. > +int cpufreq_boost_trigger_state(int state) > +{ > + unsigned long flags; > + int ret = 0; > + > + if (boost_enabled != state) { > + write_lock_irqsave(&cpufreq_driver_lock, flags); > + boost_enabled = state; > + if (cpufreq_driver->enable_boost) > + ret = cpufreq_driver->enable_boost(state); > + else > + ret = cpufreq_boost_trigger_state_sw(); > + > + if (ret) { > + boost_enabled = 0; > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); > + pr_err("%s: BOOST cannot enable (%d)\n", > + __func__, ret); > + > + return ret; > + } > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); You can rewrite if (ret) and unlock() code to make it less redundant. unlock and return ret at the end and write other stuff before it. > + pr_debug("%s: cpufreq BOOST %s\n", __func__, > + state ? "enabled" : "disabled"); > + } > + > + return 0; > +} > + > +int cpufreq_boost_supported(void) > +{ > + return cpufreq_driver->boost_supported; > +} > + > +int cpufreq_boost_enabled(void) > +{ > + return boost_enabled; > +} EXPORT_SYMBOL_GPL ?? > +/********************************************************************* > * REGISTER / UNREGISTER CPUFREQ DRIVER * > *********************************************************************/ > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) > cpufreq_driver = driver_data; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > + if (!cpufreq_driver->boost_supported) > + boost.attr.mode = 0444; > + > + ret = cpufreq_sysfs_create_file(&(boost.attr)); > + if (ret) { > + pr_err("%s: cannot register global boost sysfs file\n", > + __func__); > + goto err_null_driver; > + } This would change. > ret = subsys_interface_register(&cpufreq_interface); > if (ret) > goto err_null_driver; > @@ -1992,6 +2085,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) > pr_debug("unregistering driver %s\n", driver->name); > > subsys_interface_unregister(&cpufreq_interface); > + > + cpufreq_sysfs_remove_file(&(boost.attr)); > unregister_hotcpu_notifier(&cpufreq_cpu_notifier); > > list_del(&cpufreq_policy_list); > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c > index d7a7966..9c8e71e 100644 > --- a/drivers/cpufreq/freq_table.c > +++ b/drivers/cpufreq/freq_table.c > @@ -34,6 +34,11 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, > > continue; > } > + if (cpufreq_boost_supported()) Probably remove this check. Assume somebody while testing exynos, just sent boost_supported as false. Then you will not skip this frequency and may burn your chip :) > + if (!cpufreq_boost_enabled() > + && table[i].index == CPUFREQ_BOOST_FREQ) > + continue; This should be enough. > pr_debug("table entry %u: %u kHz, %u index\n", > i, freq, table[i].index); > if (freq < min_freq) > @@ -171,7 +176,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,6 +192,9 @@ 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; Add a comment here describing your complex logic. > + if (show_boost ^ (table[i].index == CPUFREQ_BOOST_FREQ)) > + continue; > + > count += sprintf(&buf[count], "%d ", table[i].frequency); > } > count += sprintf(&buf[count], "\n"); > @@ -194,14 +203,34 @@ static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) > > } > > -struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { > - .attr = { .name = "scaling_available_frequencies", > - .mode = 0444, > - }, > - .show = show_available_freqs, > -}; > +#define cpufreq_attr_available_freq(_name) \ > +struct freq_attr cpufreq_freq_attr_##_name##_freqs = \ > +__ATTR_RO(_name##_frequencies) > + > +/** > + * show_scaling_available_frequencies - show normal boost frequencies for You missed this comment earlier. boost?? > + * the specified CPU > + */ > +static ssize_t scaling_available_frequencies_show(struct cpufreq_policy *policy, > + char *buf) > +{ > + return show_available_freqs(policy, buf, 0); > +} > +cpufreq_attr_available_freq(scaling_available); > EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); > > +/** > + * show_available_boost_freqs - show available boost frequencies for > + * the specified CPU > + */ > +static ssize_t scaling_boost_frequencies_show(struct cpufreq_policy *policy, > + char *buf) > +{ > + return show_available_freqs(policy, buf, 1); > +} > +cpufreq_attr_available_freq(scaling_boost); > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_boost_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 5348981..4783c4c 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -267,6 +267,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_supported; > + int (*enable_boost) (int state); > }; > > /* flags */ > @@ -408,6 +412,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 > @@ -426,11 +433,15 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, > unsigned int relation, > unsigned int *index); > > +int cpufreq_boost_trigger_state(int state); Why is this present here? -- 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, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote: > On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> wrote: > > Boost sysfs attribute is always exported (to support legacy API). By > > default boost is exported as read only. One global attribute is > > available at: /sys/devices/system/cpu/cpufreq/boost. > > I assume you are going to fix this as discussed in other threads. Yes. Boost attribute will be visible only when boost is supported. > > > Changes for v4: > > - Remove boost parameter from cpufreq_frequency_table_cpuinfo() > > function > > - Introduce cpufreq_boost_supported() method > > - Use of cpufreq_boost_supported() and cpufreq_boost_enabled() to > > decide if frequency shall be skipped > > - Rename set_boost_freq() to enable_boost() > > - cpufreq_attr_available_freq() moved to freq_table.c > > - Use policy list to get access to cpufreq policies > > - Rename global boost flag (cpufreq_boost_enabled -> boost_enabled) > > - pr_err corrected ( %sable) > > - Remove sanity check at cpufreq_boost_trigger_state() entrance [to > > test if boost is supported] > > - Use either HW (boost_enable) callback or SW managed boost > > - Introduce new cpufreq_boost_trigger_state_sw() method to handle > > boost at SW. > > - Protect boost_enabled manipulation with lock > > - Always export boost attribute (preserve legacy behaviour). When > > boost is not supported this attribute is read only > > Very well written changelog. But write it after --- I will stick to the rule proposed at patch 1/4, ver 4. > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 665e641..9141d33 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 boost_enabled; > > 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 */ @@ -316,6 +317,29 @@ > > EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /********************************************************************* > > * SYSFS > > INTERFACE * > > *********************************************************************/ > > +ssize_t show_boost(struct kobject *kobj, > > + struct attribute *attr, char *buf) > > +{ > > + return sprintf(buf, "%d\n", boost_enabled); > > +} > > + > > +static ssize_t store_boost(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 enable boost!\n", __func__); > > + return -EINVAL; > > + } > > Probably do boost_enabled = true here. I would prefer to set boot_enabled at cpufreq_boost_trigger_state() method. It is closer to the cpufreq_driver->enable_boost and cpufreq_boost_trigger_state_sw(); functions, which do change the freq. > > > + return count; > > +} > > +define_one_global_rw(boost); > > > /********************************************************************* > > + * > > BOOST * > > + > > *********************************************************************/ > > +static int cpufreq_boost_trigger_state_sw(void) +{ > > + struct cpufreq_frequency_table *freq_table; > > + struct cpufreq_policy *policy; > > + int ret = -EINVAL; > > + > > + list_for_each_entry(policy, &cpufreq_policy_list, > > policy_list) { > > + freq_table = > > cpufreq_frequency_get_table(policy->cpu); > > + if (freq_table) > > + ret = > > cpufreq_frequency_table_cpuinfo(policy, > > + freq_table); > > + } > > + > > + return ret; > > + > > +} > > add blank line here. OK. > > > +int cpufreq_boost_trigger_state(int state) > > +{ > > + unsigned long flags; > > + int ret = 0; > > + > > + if (boost_enabled != state) { > > + write_lock_irqsave(&cpufreq_driver_lock, flags); > > + boost_enabled = state; > > + if (cpufreq_driver->enable_boost) > > + ret = cpufreq_driver->enable_boost(state); ^^^^^^^^^^^^^ I would prefer to change this name to enable_boost_hw It is more informative, since it is tailored to hw based boost (Intel). > > + else > > + ret = cpufreq_boost_trigger_state_sw(); > > + > > + if (ret) { > > + boost_enabled = 0; > > + > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > + pr_err("%s: BOOST cannot enable (%d)\n", > > + __func__, ret); > > + > > + return ret; > > + } > > + write_unlock_irqrestore(&cpufreq_driver_lock, > > flags); > > You can rewrite if (ret) and unlock() code to make it less redundant. > unlock and return ret at the end and write other stuff before it. I will rewrite it as follow: if (ret) boost_enabled = 0; write_unlock_irqrestore(&cpufreq_driver_lock, flags); pr_debug("%s: cpufreq BOOST %s\n", __func__, state ? "enabled" : "disabled"); return ret; > > > + pr_debug("%s: cpufreq BOOST %s\n", __func__, > > + state ? "enabled" : "disabled"); > > + } > > + > > + return 0; > > +} > > + > > +int cpufreq_boost_supported(void) > > +{ > > + return cpufreq_driver->boost_supported; > > +} > > + > > +int cpufreq_boost_enabled(void) > > +{ > > + return boost_enabled; > > +} > > EXPORT_SYMBOL_GPL ?? I will export cpufreq_boost_enabled() and cpufreq_boost_supported() > > > +/********************************************************************* > > * REGISTER / UNREGISTER CPUFREQ > > DRIVER * > > *********************************************************************/ > > > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct > > cpufreq_driver *driver_data) cpufreq_driver = driver_data; > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > + if (!cpufreq_driver->boost_supported) > > + boost.attr.mode = 0444; Will be removed ^^^^^^^^^^^^^^^ > > + > > + ret = cpufreq_sysfs_create_file(&(boost.attr)); > > + if (ret) { > > + pr_err("%s: cannot register global boost sysfs > > file\n", > > + __func__); > > + goto err_null_driver; > > + } > > This would change. This will be only exported when cpufreq_boost_supported() is true. > > > ret = subsys_interface_register(&cpufreq_interface); > > if (ret) > > goto err_null_driver; > > @@ -1992,6 +2085,8 @@ int cpufreq_unregister_driver(struct > > cpufreq_driver *driver) pr_debug("unregistering driver %s\n", > > driver->name); > > > > subsys_interface_unregister(&cpufreq_interface); > > + > > + cpufreq_sysfs_remove_file(&(boost.attr)); > > unregister_hotcpu_notifier(&cpufreq_cpu_notifier); > > > > list_del(&cpufreq_policy_list); > > diff --git a/drivers/cpufreq/freq_table.c > > b/drivers/cpufreq/freq_table.c index d7a7966..9c8e71e 100644 > > --- a/drivers/cpufreq/freq_table.c > > +++ b/drivers/cpufreq/freq_table.c > > @@ -34,6 +34,11 @@ int cpufreq_frequency_table_cpuinfo(struct > > cpufreq_policy *policy, > > > > continue; > > } > > + if (cpufreq_boost_supported()) > > Probably remove this check. Assume somebody while testing exynos, > just sent boost_supported as false. Then you will not skip this > frequency and may burn your chip :) OK. > > > + if (!cpufreq_boost_enabled() > > + && table[i].index == CPUFREQ_BOOST_FREQ) > > + continue; > > This should be enough. Let's only rely on the cpufreq_boost_enabled() test here. > > > pr_debug("table entry %u: %u kHz, %u index\n", > > i, freq, table[i].index); > > if (freq < min_freq) > > @@ -171,7 +176,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,6 +192,9 @@ 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; > > Add a comment here describing your complex logic. OK. > > > + if (show_boost ^ (table[i].index == > > CPUFREQ_BOOST_FREQ)) > > + continue; > > + > > count += sprintf(&buf[count], "%d ", > > table[i].frequency); } > > count += sprintf(&buf[count], "\n"); > > @@ -194,14 +203,34 @@ static ssize_t show_available_freqs(struct > > cpufreq_policy *policy, char *buf) > > > > } > > > > -struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { > > - .attr = { .name = "scaling_available_frequencies", > > - .mode = 0444, > > - }, > > - .show = show_available_freqs, > > -}; > > +#define cpufreq_attr_available_freq(_name) \ > > +struct freq_attr cpufreq_freq_attr_##_name##_freqs = \ > > +__ATTR_RO(_name##_frequencies) > > + > > +/** > > + * show_scaling_available_frequencies - show normal boost > > frequencies for > > You missed this comment earlier. boost?? My mistake. This will be corrected. > > > + * the specified CPU > > + */ > > +static ssize_t scaling_available_frequencies_show(struct > > cpufreq_policy *policy, > > + char *buf) > > +{ > > + return show_available_freqs(policy, buf, 0); > > +} > > +cpufreq_attr_available_freq(scaling_available); > > EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); > > > > +/** > > + * show_available_boost_freqs - show available boost frequencies > > for > > + * the specified CPU > > + */ > > +static ssize_t scaling_boost_frequencies_show(struct > > cpufreq_policy *policy, > > + char *buf) > > +{ > > + return show_available_freqs(policy, buf, 1); > > +} > > +cpufreq_attr_available_freq(scaling_boost); > > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_boost_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 5348981..4783c4c 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -267,6 +267,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_supported; > > + int (*enable_boost) (int state); > > }; > > > > /* flags */ > > @@ -408,6 +412,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 @@ -426,11 +433,15 @@ int > > cpufreq_frequency_table_target(struct cpufreq_policy *policy, > > unsigned int relation, unsigned int *index); > > > > +int cpufreq_boost_trigger_state(int state); > > Why is this present here? We had agreed to talk only about cpufreq :-). This declaration will be removed.
On Wed, 26 Jun 2013 14:54:12 +0200, Lukasz Majewski wrote: > > > > > > +int cpufreq_boost_trigger_state(int state); > > > > Why is this present here? > > We had agreed to talk only about cpufreq :-). > > This declaration will be removed. Correction: This declaration is needed for allowing disabling cpufreq boost at thermal subsystem (please refer to [PATCH v4 7/7]).
On 26 June 2013 18:24, Lukasz Majewski <l.majewski@samsung.com> wrote: > On Wed, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote: >> On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> wrote: >> > +static ssize_t store_boost(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 enable boost!\n", __func__); >> > + return -EINVAL; >> > + } >> >> Probably do boost_enabled = true here. > > I would prefer to set boot_enabled at > cpufreq_boost_trigger_state() method. It is closer to the > cpufreq_driver->enable_boost and cpufreq_boost_trigger_state_sw(); > functions, which do change the freq. I said that as this will be more inclined towards the purpose of this routine. This routine should store boost as show_boost() is returning it. So, what would be better is if you just return 0 or err from cpufreq_boost_trigger_state() and then set boost here. This will also solve your problem where you revert back to older boost value for failure cases. >> > + ret = cpufreq_driver->enable_boost(state); > ^^^^^^^^^^^^^ > I would prefer to change this > name to enable_boost_hw > It is more informative, since it is tailored to hw based boost (Intel). Ok >> > + else >> > + ret = cpufreq_boost_trigger_state_sw(); then why not enable_boost_sw() here? that would be more relevant. > I will rewrite it as follow: > > if (ret) > boost_enabled = 0; > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > pr_debug("%s: cpufreq BOOST %s\n", __func__, > state ? "enabled" : "disabled"); So, you will not print error but current state? Probably printing error is better. -- 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 Thu, 27 Jun 2013 14:32:57 +0530, Viresh Kumar wrote: > On 26 June 2013 18:24, Lukasz Majewski <l.majewski@samsung.com> wrote: > > On Wed, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote: > >> On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > > >> > +static ssize_t store_boost(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 enable boost!\n", __func__); > >> > + return -EINVAL; > >> > + } > >> > >> Probably do boost_enabled = true here. > > > > I would prefer to set boot_enabled at > > cpufreq_boost_trigger_state() method. It is closer to the > > cpufreq_driver->enable_boost and cpufreq_boost_trigger_state_sw(); > > functions, which do change the freq. > > I said that as this will be more inclined towards the purpose of > this routine. This routine should store boost as show_boost() > is returning it. So, what would be better is if you just return > 0 or err from cpufreq_boost_trigger_state() and then set boost > here. This will also solve your problem where you revert back > to older boost value for failure cases. I thought about this idea, but at cpufreq_boost_trigger_state_sw() I iterate through all available policies and call cpufreq_frequency_table_cpuinfo()[*] on them. In this routine [*] I use cpufreq_boost_enabled() [**] route to search for maximal (boost) frequency. The [**] reads boost_enabled flag, which shall be updated before. When this search fails, then I restore the old value of boost_enabled. > > >> > + ret = > >> > cpufreq_driver->enable_boost(state); > > ^^^^^^^^^^^^^ > > I would prefer to change > > this name to enable_boost_hw > > It is more informative, since it is tailored to hw based boost > > (Intel). > > Ok OK. > > >> > + else > >> > + ret = cpufreq_boost_trigger_state_sw(); > > then why not enable_boost_sw() here? that would be more > relevant. Could you be more specific here? The distinction here is done on purpose: You can either call cpufreq_driver->enable_boost for HW controlled boost or cpufreq_boost_trigger_state_sw() for SW controlled one. I could write: if (cpufreq_driver->enable_boost) ret = cpufreq_driver->enable_boost(state); ret = cpufreq_boost_trigger_state_sw(); But then for Intel CPUs I will iterate over its policies to seek for CPUFREQ_BOOST_FREQ marked frequencies without any purpose, since HW is taking care of boosting. > > > I will rewrite it as follow: > > > > if (ret) > > boost_enabled = 0; > > > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > pr_debug("%s: cpufreq BOOST %s\n", __func__, > > state ? "enabled" : "disabled"); > > So, you will not print error but current state? Probably > printing error is better. I will change it to: write_unlock_irqrestore(&cpufreq_driver_lock, flags); if (ret) pr_err("%s: BOOST cannot enable (%d)\n", __func__, ret); return ret; I want to avoid time consuming operations (like print) with holding lock (and boost_enabled shall be modified under lock).
On 27 June 2013 15:18, Lukasz Majewski <l.majewski@samsung.com> wrote: > On Thu, 27 Jun 2013 14:32:57 +0530, Viresh Kumar wrote: > I thought about this idea, but at cpufreq_boost_trigger_state_sw() > I iterate through all available policies and call > cpufreq_frequency_table_cpuinfo()[*] on them. In this routine [*] I use > cpufreq_boost_enabled() [**] route to search for maximal (boost) > frequency. > The [**] reads boost_enabled flag, which shall be updated before. When > this search fails, then I restore the old value of boost_enabled. Ok. >> >> > + else >> >> > + ret = cpufreq_boost_trigger_state_sw(); >> >> then why not enable_boost_sw() here? that would be more >> relevant. > > Could you be more specific here? I meant rename cpufreq_boost_trigger_state_sw() to enable_boost_sw() :) -- 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 Thu, 27 Jun 2013 15:55:26 +0530, Viresh Kumar wrote: > On 27 June 2013 15:18, Lukasz Majewski <l.majewski@samsung.com> wrote: > > On Thu, 27 Jun 2013 14:32:57 +0530, Viresh Kumar wrote: > > > I thought about this idea, but at cpufreq_boost_trigger_state_sw() > > I iterate through all available policies and call > > cpufreq_frequency_table_cpuinfo()[*] on them. In this routine [*] I > > use cpufreq_boost_enabled() [**] route to search for maximal (boost) > > frequency. > > The [**] reads boost_enabled flag, which shall be updated before. > > When this search fails, then I restore the old value of > > boost_enabled. > > Ok. OK > > >> >> > + else > >> >> > + ret = > >> >> > cpufreq_boost_trigger_state_sw(); > >> > >> then why not enable_boost_sw() here? that would be more > >> relevant. > > > > Could you be more specific here? > > I meant rename cpufreq_boost_trigger_state_sw() to > enable_boost_sw() :) No problem: - For SW there will be cpufreq_boost_trigger_state_sw() renamed to enable_boost_sw() - For HW: cpufreq_driver->enable_boost(state) renamed to cpufreq_driver->enable_boost_hw();
On Wed, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote: > > +int cpufreq_boost_trigger_state(int state) > > +{ > > + unsigned long flags; > > + int ret = 0; > > + > > + if (boost_enabled != state) { > > + write_lock_irqsave(&cpufreq_driver_lock, flags); > > + boost_enabled = state; > > + if (cpufreq_driver->enable_boost) > > + ret = cpufreq_driver->enable_boost(state); > > + else > > + ret = cpufreq_boost_trigger_state_sw(); I will use only one call to cpufreq_driver->enable_boost(state) [*] with either cpufreq_boost_enable_sw() (function with SW boost handling) or the one provided by cpufreq driver. Only when cpufreq driver doesn't provide [*], it will be filled with "default" cpufreq_boost_enable_sw(). > > + > > + if (ret) { > > + boost_enabled = 0; > > + > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > + pr_err("%s: BOOST cannot enable (%d)\n", > > + __func__, ret); > > + > > + return ret; > > + } > > + write_unlock_irqrestore(&cpufreq_driver_lock, > > flags);
On 27 June 2013 21:25, Lukasz Majewski <l.majewski@samsung.com> wrote: > On Wed, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote: >> > + if (boost_enabled != state) { >> > + write_lock_irqsave(&cpufreq_driver_lock, flags); >> > + boost_enabled = state; >> > + if (cpufreq_driver->enable_boost) >> > + ret = cpufreq_driver->enable_boost(state); >> > + else >> > + ret = cpufreq_boost_trigger_state_sw(); > > I will use only one call to cpufreq_driver->enable_boost(state) [*] with > either cpufreq_boost_enable_sw() (function with SW boost handling) or > the one provided by cpufreq driver. > > Only when cpufreq driver doesn't provide [*], it will be filled with > "default" cpufreq_boost_enable_sw(). I didn't get it completely. You are saying you will send a function pointer now? -- 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 Fri, 28 Jun 2013 09:10:53 +0530, Viresh Kumar wrote: > On 27 June 2013 21:25, Lukasz Majewski <l.majewski@samsung.com> wrote: > > On Wed, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote: > >> > + if (boost_enabled != state) { > >> > + write_lock_irqsave(&cpufreq_driver_lock, flags); > >> > + boost_enabled = state; > >> > + if (cpufreq_driver->enable_boost) > >> > + ret = > >> > cpufreq_driver->enable_boost(state); > >> > + else > >> > + ret = cpufreq_boost_trigger_state_sw(); > > > > I will use only one call to cpufreq_driver->enable_boost(state) [*] > > with either cpufreq_boost_enable_sw() (function with SW boost > > handling) or the one provided by cpufreq driver. > > > > Only when cpufreq driver doesn't provide [*], it will be filled with > > "default" cpufreq_boost_enable_sw(). > > I didn't get it completely. You are saying you will send a function > pointer now? No, I will use: if (boost_enabled != state) { write_lock_irqsave(&cpufreq_driver_lock, flags); boost_enabled = state; ret = cpufreq_driver->enable_boost(state); ^^^^^^^^^^^^^^^^^^^^ only one callback call if (ret) boost_enabled = 0; write_unlock_irqrestore(&cpufreq_driver_lock, flags); if (ret) pr_err("%s: BOOST cannot enable (%d)\n", __func__, ret); } and @ cpufreq_register_driver() I will add following line: if (!cpufreq_driver->enable_boost) cpufreq_driver->enable_boost = &cpufreq_boost_enable_sw; When cpufreq driver doesn't define callback for enable_boost it will be filled with default SW cpufreq_boost_enable_sw callback.
On 28 June 2013 12:19, Lukasz Majewski <l.majewski@samsung.com> wrote: > No, I will use: > > if (boost_enabled != state) { > write_lock_irqsave(&cpufreq_driver_lock, flags); > boost_enabled = state; > > ret = cpufreq_driver->enable_boost(state); > ^^^^^^^^^^^^^^^^^^^^ only one callback call > if (ret) > boost_enabled = 0; > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > if (ret) > pr_err("%s: BOOST cannot enable (%d)\n", > __func__, ret); > } > > and @ cpufreq_register_driver() I will add following line: > > if (!cpufreq_driver->enable_boost) > cpufreq_driver->enable_boost = &cpufreq_boost_enable_sw; > > When cpufreq driver doesn't define callback for enable_boost it will be > filled with default SW cpufreq_boost_enable_sw callback. That's some smart code. Good. :) -- 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 Fri, 28 Jun 2013 12:21:21 +0530, Viresh Kumar wrote: > On 28 June 2013 12:19, Lukasz Majewski <l.majewski@samsung.com> wrote: > > No, I will use: > > > > if (boost_enabled != state) { > > write_lock_irqsave(&cpufreq_driver_lock, flags); > > boost_enabled = state; > > > > ret = cpufreq_driver->enable_boost(state); > > ^^^^^^^^^^^^^^^^^^^^ only one callback call > > if (ret) > > boost_enabled = 0; > > > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > if (ret) > > pr_err("%s: BOOST cannot enable (%d)\n", > > __func__, ret); > > } > > > > and @ cpufreq_register_driver() I will add following line: > > > > if (!cpufreq_driver->enable_boost) > > cpufreq_driver->enable_boost = &cpufreq_boost_enable_sw; > > > > When cpufreq driver doesn't define callback for enable_boost it > > will be filled with default SW cpufreq_boost_enable_sw callback. > > That's some smart code. Good. :) OK
On 20 June 2013 03:55, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Wednesday, June 19, 2013 10:31:02 PM Lukasz Majewski wrote: >> On Wed, 19 Jun 2013 10:48:53 -0700 >> Dirk Brandewie <dirk.brandewie@gmail.com> wrote: >> >> > On 06/19/2013 10:12 AM, Lukasz Majewski wrote: >> > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct >> > > + if (!cpufreq_driver->boost_supported) >> > > + boost.attr.mode = 0444; >> > > + >> > > + ret = cpufreq_sysfs_create_file(&(boost.attr)); >> > > + if (ret) { >> > > + pr_err("%s: cannot register global boost sysfs >> > > file\n", >> > > + __func__); >> > > + goto err_null_driver; >> > > + } >> > > + >> > >> > I do not think the boost sysfs should be created at all if boost is >> > not supported. >> >> This was my first thought. But unfortunately this "boost" attribute is >> always exported at acpi-cpufreq.c and in my opinion is part of a >> legacy API. >> >> I totally agree with the idea of exporting boost only when supported, >> but I would like to know the community opinion about this (especially >> Viresh and Rafael shall speak up). > > Simple: Export it only when supported. Guys, I got confused here. We originally decided to keep this feature as is for acpi-cpufreq. So, For acpi-cpufreq: - when boost isn't supported: create sysfs boost with ro permissions - when boost is supported: create sysfs boost with rw permissions So, For others: - create sysfs boost with rw permissions only when boost is supported . Do you want something else? or do you want to change behavior of acpi-cpufreq driver? I don't why it was designed this way and what applications use it. -- 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, July 17, 2013 01:28:29 PM Viresh Kumar wrote: > On 20 June 2013 03:55, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Wednesday, June 19, 2013 10:31:02 PM Lukasz Majewski wrote: > >> On Wed, 19 Jun 2013 10:48:53 -0700 > >> Dirk Brandewie <dirk.brandewie@gmail.com> wrote: > >> > >> > On 06/19/2013 10:12 AM, Lukasz Majewski wrote: > > >> > > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct > >> > > + if (!cpufreq_driver->boost_supported) > >> > > + boost.attr.mode = 0444; > >> > > + > >> > > + ret = cpufreq_sysfs_create_file(&(boost.attr)); > >> > > + if (ret) { > >> > > + pr_err("%s: cannot register global boost sysfs > >> > > file\n", > >> > > + __func__); > >> > > + goto err_null_driver; > >> > > + } > >> > > + > >> > > >> > I do not think the boost sysfs should be created at all if boost is > >> > not supported. > >> > >> This was my first thought. But unfortunately this "boost" attribute is > >> always exported at acpi-cpufreq.c and in my opinion is part of a > >> legacy API. > >> > >> I totally agree with the idea of exporting boost only when supported, > >> but I would like to know the community opinion about this (especially > >> Viresh and Rafael shall speak up). > > > > Simple: Export it only when supported. > > Guys, > > I got confused here. We originally decided to keep this feature as is > for acpi-cpufreq. > > So, For acpi-cpufreq: > - when boost isn't supported: create sysfs boost with ro permissions > - when boost is supported: create sysfs boost with rw permissions > > So, For others: > - create sysfs boost with rw permissions only when boost is supported > > . > > Do you want something else? or do you want to change behavior of > acpi-cpufreq driver? I don't why it was designed this way and what > applications use it. First off, I'm not sure how many applications actually use it and I think, if any, they should be able cope with the attribute not being present. Of course, if it turns out that yes, there are applications using it and no, they cannot cope with the missing attribute, we'll need to address this. That said such applications wouldn't work with earlier kernels in which that attribute wasn't present at all, so I suppose this is really unlikely. So, do whichever makes more sense to you: Design things to preserve the old behavior (which is sightly confusing) or design them to expose the attribute if the feature is actually supported and be prepared to address the (unlikely) case when some hypothetical applications break because of that. Thanks, Rafael
On 17 July 2013 17:01, Rafael J. Wysocki <rjw@sisk.pl> wrote: > First off, I'm not sure how many applications actually use it and I think, > if any, they should be able cope with the attribute not being present. > > Of course, if it turns out that yes, there are applications using it and no, > they cannot cope with the missing attribute, we'll need to address this. > That said such applications wouldn't work with earlier kernels in which that > attribute wasn't present at all, so I suppose this is really unlikely. > > So, do whichever makes more sense to you: Design things to preserve the old > behavior (which is sightly confusing) or design them to expose the attribute > if the feature is actually supported and be prepared to address the (unlikely) > case when some hypothetical applications break because of that. Okay. Its better to keep it the way Lukasz designed it in his last patchset. -- 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, 17 Jul 2013 18:31:19 +0530 Viresh Kumar viresh.kumar@linaro.org wrote, > On 17 July 2013 17:01, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > First off, I'm not sure how many applications actually use it and I > > think, if any, they should be able cope with the attribute not > > being present. > > > > Of course, if it turns out that yes, there are applications using > > it and no, they cannot cope with the missing attribute, we'll need > > to address this. That said such applications wouldn't work with > > earlier kernels in which that attribute wasn't present at all, so I > > suppose this is really unlikely. > > > > So, do whichever makes more sense to you: Design things to preserve > > the old behavior (which is sightly confusing) or design them to > > expose the attribute if the feature is actually supported and be > > prepared to address the (unlikely) case when some hypothetical > > applications break because of that. > > Okay. Its better to keep it the way Lukasz designed it in his last > patchset. To be 100% sure - we export boost only when supported (as proposed at v5).
On 17 July 2013 20:29, Lukasz Majewski <l.majewski@samsung.com> wrote:
> To be 100% sure - we export boost only when supported (as proposed at v5).
Yes.
--
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
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 665e641..9141d33 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 boost_enabled; 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 */ @@ -316,6 +317,29 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /********************************************************************* * SYSFS INTERFACE * *********************************************************************/ +ssize_t show_boost(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", boost_enabled); +} + +static ssize_t store_boost(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 enable boost!\n", __func__); + return -EINVAL; + } + + return count; +} +define_one_global_rw(boost); static struct cpufreq_governor *__find_governor(const char *str_governor) { @@ -1898,6 +1922,65 @@ static struct notifier_block __refdata cpufreq_cpu_notifier = { }; /********************************************************************* + * BOOST * + *********************************************************************/ +static int cpufreq_boost_trigger_state_sw(void) +{ + struct cpufreq_frequency_table *freq_table; + struct cpufreq_policy *policy; + int ret = -EINVAL; + + list_for_each_entry(policy, &cpufreq_policy_list, policy_list) { + freq_table = cpufreq_frequency_get_table(policy->cpu); + if (freq_table) + ret = cpufreq_frequency_table_cpuinfo(policy, + freq_table); + } + + return ret; + +} +int cpufreq_boost_trigger_state(int state) +{ + unsigned long flags; + int ret = 0; + + if (boost_enabled != state) { + write_lock_irqsave(&cpufreq_driver_lock, flags); + boost_enabled = state; + if (cpufreq_driver->enable_boost) + ret = cpufreq_driver->enable_boost(state); + else + ret = cpufreq_boost_trigger_state_sw(); + + if (ret) { + boost_enabled = 0; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + pr_err("%s: BOOST cannot enable (%d)\n", + __func__, ret); + + return ret; + } + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + + pr_debug("%s: cpufreq BOOST %s\n", __func__, + state ? "enabled" : "disabled"); + } + + return 0; +} + +int cpufreq_boost_supported(void) +{ + return cpufreq_driver->boost_supported; +} + +int cpufreq_boost_enabled(void) +{ + return boost_enabled; +} + +/********************************************************************* * REGISTER / UNREGISTER CPUFREQ DRIVER * *********************************************************************/ @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags); + if (!cpufreq_driver->boost_supported) + boost.attr.mode = 0444; + + ret = cpufreq_sysfs_create_file(&(boost.attr)); + if (ret) { + pr_err("%s: cannot register global boost sysfs file\n", + __func__); + goto err_null_driver; + } + ret = subsys_interface_register(&cpufreq_interface); if (ret) goto err_null_driver; @@ -1992,6 +2085,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) pr_debug("unregistering driver %s\n", driver->name); subsys_interface_unregister(&cpufreq_interface); + + cpufreq_sysfs_remove_file(&(boost.attr)); unregister_hotcpu_notifier(&cpufreq_cpu_notifier); list_del(&cpufreq_policy_list); diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c index d7a7966..9c8e71e 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -34,6 +34,11 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, continue; } + if (cpufreq_boost_supported()) + if (!cpufreq_boost_enabled() + && table[i].index == CPUFREQ_BOOST_FREQ) + continue; + pr_debug("table entry %u: %u kHz, %u index\n", i, freq, table[i].index); if (freq < min_freq) @@ -171,7 +176,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,6 +192,9 @@ 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 ^ (table[i].index == CPUFREQ_BOOST_FREQ)) + continue; + count += sprintf(&buf[count], "%d ", table[i].frequency); } count += sprintf(&buf[count], "\n"); @@ -194,14 +203,34 @@ static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) } -struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { - .attr = { .name = "scaling_available_frequencies", - .mode = 0444, - }, - .show = show_available_freqs, -}; +#define cpufreq_attr_available_freq(_name) \ +struct freq_attr cpufreq_freq_attr_##_name##_freqs = \ +__ATTR_RO(_name##_frequencies) + +/** + * show_scaling_available_frequencies - show normal boost frequencies for + * the specified CPU + */ +static ssize_t scaling_available_frequencies_show(struct cpufreq_policy *policy, + char *buf) +{ + return show_available_freqs(policy, buf, 0); +} +cpufreq_attr_available_freq(scaling_available); EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); +/** + * show_available_boost_freqs - show available boost frequencies for + * the specified CPU + */ +static ssize_t scaling_boost_frequencies_show(struct cpufreq_policy *policy, + char *buf) +{ + return show_available_freqs(policy, buf, 1); +} +cpufreq_attr_available_freq(scaling_boost); +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_boost_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 5348981..4783c4c 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -267,6 +267,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_supported; + int (*enable_boost) (int state); }; /* flags */ @@ -408,6 +412,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 @@ -426,11 +433,15 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int relation, unsigned int *index); +int cpufreq_boost_trigger_state(int state); +int cpufreq_boost_supported(void); +int cpufreq_boost_enabled(void); /* 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_scaling_boost_freqs; void cpufreq_frequency_table_get_attr(struct cpufreq_frequency_table *table, unsigned int cpu);