| Message ID | 1370502472-7249-3-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi, On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> wrote: > This commit adds support for software based frequency boosting. > Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal > condition limits. This can be done for some short time. > > Overclocking (boost) support came from cpufreq driver (which is platform > dependent). Hence the data structure describing it is defined at its file. > > To allow support for either SW and HW (Intel) based boosting, the cpufreq > core code has been extended to support both solutions. > > The main boost switch has been put at /sys/devices/system/cpu/cpufreq/boost. Log requires some better paragraphs but I am not concerned about it for now. > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > --- > drivers/cpufreq/cpufreq.c | 156 ++++++++++++++++++++++++++++++++++++++++++ > drivers/cpufreq/freq_table.c | 87 ++++++++++++++++++++++- > include/linux/cpufreq.h | 35 +++++++++- > 3 files changed, 275 insertions(+), 3 deletions(-) My initial view on this patch is: "It is overly engineered and needs to get simplified" > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index ca74e27..8cf9a92 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -133,6 +133,11 @@ bool have_governor_per_policy(void) > return cpufreq_driver->have_governor_per_policy; > } > > +/** > + * Global definition of cpufreq_boost structure > + */ > +struct cpufreq_boost *cpufreq_boost; Probably just a 'static bool' here cpufreq_boost_enabled. Which takes care of selection from sysfs. > static struct cpufreq_governor *__find_governor(const char *str_governor) > { > @@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu, > if (cpufreq_set_drv_attr_files(policy, cpufreq_driver->attr)) > goto err_out_kobj_put; > > + if (cpufreq_driver->boost) { > + if (sysfs_create_file(cpufreq_global_kobject, > + &(global_boost.attr))) This will report error for systems where we have two policy structures. As we are creating something already present. > + pr_warn("could not register global boost sysfs file\n"); > + else > + pr_debug("registered global boost sysfs file\n"); Please make all your prints to print function name too: pr_debug("%s: foo\n", __func__, foo); > + if (cpufreq_set_drv_attr_files(policy, > + cpufreq_driver->boost->attr)) Why is this required? Why do we want platforms to add some files in sysfs? > /********************************************************************* > + * BOOST * > + *********************************************************************/ > +int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state) > +{ > + struct cpufreq_boost *boost; > + unsigned long flags; > + int ret = 0; > + > + if (!policy || !policy->boost || !policy->boost->low_level_boost) > + return -ENODEV; > + > + boost = policy->boost; > + write_lock_irqsave(&cpufreq_driver_lock, flags); > + > + if (boost->status != state) { > + policy->boost->status = state; > + ret = boost->low_level_boost(policy, state); > + if (ret) { > + pr_err("BOOST cannot %sable low level code (%d)\n", > + state ? "en" : "dis", ret); > + return ret; > + } > + } > + > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); > + > + pr_debug("cpufreq BOOST %sabled\n", state ? "en" : "dis"); > + > + return 0; > +} > + > +/** > + * cpufreq_boost_notifier - notifier callback for cpufreq policy change. > + * <at> nb: struct notifier_block * with callback info. > + * <at> event: value showing cpufreq event for which this function invoked. > + * <at> data: callback-specific data > + */ > +static int cpufreq_boost_notifier(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct cpufreq_policy *policy = data; > + > + if (event == CPUFREQ_INCOMPATIBLE) { > + if (!policy || !policy->boost) > + return -ENODEV; > + > + if (policy->boost->status == CPUFREQ_BOOST_EN) { > + pr_info("NOTIFIER BOOST: MAX: %d e:%lu cpu: %d\n", > + policy->max, event, policy->cpu); > + cpufreq_boost_trigger_state(policy, 0); > + } > + } > + > + return 0; > +} > + > +/* Notifier for cpufreq policy change */ > +static struct notifier_block cpufreq_boost_notifier_block = { > + .notifier_call = cpufreq_boost_notifier, > +}; > + > +int cpufreq_boost_init(struct cpufreq_policy *policy) > +{ > + int ret = 0; > + > + if (!policy) > + return -EINVAL; Heh, policy can't be NULL here. > + if (!cpufreq_driver->boost) { > + pr_err("Boost mode not supported on this device\n"); Wow!! You want to screw everybody else's logs with this message. Its not a crime if you don't have boost mode supported :) Actually this routine must be called only if cpufreq_driver->boost is valid. > + return -ENODEV; > + } > + > + policy->boost = cpufreq_boost = cpufreq_driver->boost; Why are we copying same pointer to policy->boost? Driver is passing pointer to a single memory location, just save it globally. > + /* disable boost for newly created policy - as we e.g. change > + governor */ > + policy->boost->status = CPUFREQ_BOOST_DIS; Drivers supporting boost may want boost to be enabled by default, maybe without any sysfs calls. > + /* register policy notifier */ > + ret = cpufreq_register_notifier(&cpufreq_boost_notifier_block, > + CPUFREQ_POLICY_NOTIFIER); > + if (ret) { > + pr_err("CPUFREQ BOOST notifier not registered.\n"); > + return ret; > + } > + /* add policy to policies list headed at struct cpufreq_boost */ > + list_add_tail(&policy->boost_list, &cpufreq_boost->policies); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cpufreq_boost_init); Why do we need to maintain a list of boost here? notifiers? complex :( > +/********************************************************************* > * REGISTER / UNREGISTER CPUFREQ DRIVER * > *********************************************************************/ > > @@ -1954,6 +2106,10 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) > pr_debug("unregistering driver %s\n", driver->name); > > subsys_interface_unregister(&cpufreq_interface); > + > + if (cpufreq_driver->boost) > + sysfs_remove_file(cpufreq_global_kobject, &(global_boost.attr)); You haven't removed this from policy. Memory leak. > 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..0e95524 100644 > --- a/drivers/cpufreq/freq_table.c > +++ b/drivers/cpufreq/freq_table.c > @@ -3,6 +3,8 @@ > * > * Copyright (C) 2002 - 2003 Dominik Brodowski > * > + * Copyright (C) 2013 Lukasz Majewski <l.majewski@samsung.com> > + * You shouldn't add it unless you did some major work on this file. You aren't maintaining this file in 2013. > +static int cpufreq_frequency_table_skip_boost(struct cpufreq_policy *policy, > + unsigned int index) > +{ > + if (index == CPUFREQ_BOOST) > + if (!policy->boost || This shouldn't be true. If index has got CPUFREQ_BOOST, then driver has to support boost. > + policy->boost->status == CPUFREQ_BOOST_DIS) > + return 1; > + > + return 0; > +} > + > +unsigned int > +cpufreq_frequency_table_boost_max(struct cpufreq_frequency_table *freq_table) > +{ > + int index, boost_freq_max; > + > + for (index = 0, boost_freq_max = 0; > + freq_table[index].frequency != CPUFREQ_TABLE_END; index++) > + if (freq_table[index].index == CPUFREQ_BOOST) { > + if (freq_table[index].frequency > boost_freq_max) > + boost_freq_max = freq_table[index].frequency; > + } > + > + return boost_freq_max; > +} > +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_boost_max); why do we need this? > /* > * 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..1294c8c 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -88,6 +88,25 @@ struct cpufreq_real_policy { > struct cpufreq_governor *governor; /* see below */ > }; > > +#define CPUFREQ_BOOST_DIS (0) > +#define CPUFREQ_BOOST_EN (1) You don't need these.. Just create variable as bool and 0 & 1 would be fine. > +struct cpufreq_policy; > +struct cpufreq_boost { > + unsigned int max_boost_freq; /* maximum value of > + * boosted freq */ > + unsigned int max_normal_freq; /* non boost max freq */ > + int status; /* status of boost */ > + > + /* boost sysfs attributies */ > + struct freq_attr **attr; > + > + /* low-level trigger for boost */ > + int (*low_level_boost) (struct cpufreq_policy *policy, int state); > + > + struct list_head policies; > +}; We don't need it. Just add two more fields to cpufreq_driver: - have_boost_freqs and low_level_boost (maybe a better name. What's its use?) > struct cpufreq_policy { > /* CPUs sharing clock, require sw coordination */ > cpumask_var_t cpus; /* Online CPUs only */ > @@ -113,6 +132,9 @@ struct cpufreq_policy { > > struct cpufreq_real_policy user_policy; > > + struct cpufreq_boost *boost; > + struct list_head boost_list; We don't need both of these. > struct kobject kobj; > struct completion kobj_unregister; > }; > @@ -277,7 +302,6 @@ struct cpufreq_driver { > int cpufreq_register_driver(struct cpufreq_driver *driver_data); > int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); > > - ?? > void cpufreq_notify_transition(struct cpufreq_policy *policy, > struct cpufreq_freqs *freqs, unsigned int state); > > @@ -403,6 +427,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 ~2 s/CPUFREQ_BOOST/CPUFREQ_BOOST_FREQ -- 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, > > On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> wrote: > > This commit adds support for software based frequency boosting. > > Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal > > condition limits. This can be done for some short time. > > > > Overclocking (boost) support came from cpufreq driver (which is > > platform dependent). Hence the data structure describing it is > > defined at its file. > > > > To allow support for either SW and HW (Intel) based boosting, the > > cpufreq core code has been extended to support both solutions. > > > > The main boost switch has been put > > at /sys/devices/system/cpu/cpufreq/boost. > > Log requires some better paragraphs but I am not concerned about it > for now. > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > > --- > > drivers/cpufreq/cpufreq.c | 156 > > ++++++++++++++++++++++++++++++++++++++++++ > > drivers/cpufreq/freq_table.c | 87 ++++++++++++++++++++++- > > include/linux/cpufreq.h | 35 +++++++++- 3 files changed, 275 > > insertions(+), 3 deletions(-) > > My initial view on this patch is: "It is overly engineered and needs > to get simplified" > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index ca74e27..8cf9a92 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -133,6 +133,11 @@ bool have_governor_per_policy(void) > > return cpufreq_driver->have_governor_per_policy; > > } > > > > +/** > > + * Global definition of cpufreq_boost structure > > + */ > > +struct cpufreq_boost *cpufreq_boost; > > Probably just a 'static bool' here cpufreq_boost_enabled. Which takes > care of selection from sysfs. The pointer to struct cpufreq_boost is needed for further reference (as it is now done with struct cpufreq_driver pointer - *cpufreq_driver - defined at cpufreq.c file). > > > static struct cpufreq_governor *__find_governor(const char > > *str_governor) { > > @@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned > > int cpu, if (cpufreq_set_drv_attr_files(policy, > > cpufreq_driver->attr)) goto err_out_kobj_put; > > > > + if (cpufreq_driver->boost) { > > + if (sysfs_create_file(cpufreq_global_kobject, > > + &(global_boost.attr))) > > This will report error for systems where we have two policy > structures. As we are creating something already present. Good point, thanks. > > > + pr_warn("could not register global boost > > sysfs file\n"); > > + else > > + pr_debug("registered global boost sysfs > > file\n"); > > Please make all your prints to print function name too: > > pr_debug("%s: foo\n", __func__, foo); OK. > > > + if (cpufreq_set_drv_attr_files(policy, > > + > > cpufreq_driver->boost->attr)) > > Why is this required? Why do we want platforms to add some files > in sysfs? There are two kinds of attributes, which are exported by boost: 1. global boost (/sys/devices/system/cpu/cpufreq/boost) 2. attributes describing cpufreq abilities when boost is available (/sys/devices/syste/cpu/cpu0/cpufreq/): - scaling_boost_frequencies - which will show over clocked frequencies - the scaling_available_frequencies will also display boosted frequency (when boost enabled) Information from 2. is cpufreq_driver dependent. And that information shouldn't been displayed when boost is not available > > > /********************************************************************* > > + * > > BOOST * > > + > > *********************************************************************/ > > +int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int > > state) +{ > > + struct cpufreq_boost *boost; > > + unsigned long flags; > > + int ret = 0; > > + > > + if (!policy || !policy->boost > > || !policy->boost->low_level_boost) > > + return -ENODEV; > > + > > + boost = policy->boost; > > + write_lock_irqsave(&cpufreq_driver_lock, flags); > > + > > + if (boost->status != state) { > > + policy->boost->status = state; > > + ret = boost->low_level_boost(policy, state); > > + if (ret) { > > + pr_err("BOOST cannot %sable low level code > > (%d)\n", > > + state ? "en" : "dis", ret); > > + return ret; > > + } > > + } > > + > > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > + > > + pr_debug("cpufreq BOOST %sabled\n", state ? "en" : "dis"); > > + > > + return 0; > > +} > > + > > +/** > > + * cpufreq_boost_notifier - notifier callback for cpufreq policy > > change. > > + * <at> nb: struct notifier_block * with callback info. > > + * <at> event: value showing cpufreq event for which this > > function invoked. > > + * <at> data: callback-specific data > > + */ > > +static int cpufreq_boost_notifier(struct notifier_block *nb, > > + unsigned long event, void *data) > > +{ > > + struct cpufreq_policy *policy = data; > > + > > + if (event == CPUFREQ_INCOMPATIBLE) { > > + if (!policy || !policy->boost) > > + return -ENODEV; > > + > > + if (policy->boost->status == CPUFREQ_BOOST_EN) { > > + pr_info("NOTIFIER BOOST: MAX: %d e:%lu cpu: > > %d\n", > > + policy->max, event, policy->cpu); > > + cpufreq_boost_trigger_state(policy, 0); > > + } > > + } > > + > > + return 0; > > +} > > + > > +/* Notifier for cpufreq policy change */ > > +static struct notifier_block cpufreq_boost_notifier_block = { > > + .notifier_call = cpufreq_boost_notifier, > > +}; > > + > > +int cpufreq_boost_init(struct cpufreq_policy *policy) > > +{ > > + int ret = 0; > > + > > + if (!policy) > > + return -EINVAL; > > Heh, policy can't be NULL here. Extra precautions :-). I will remove it. > > > + if (!cpufreq_driver->boost) { > > + pr_err("Boost mode not supported on this device\n"); > > Wow!! You want to screw everybody else's logs with this message. > Its not a crime if you don't have boost mode supported :) Hmm, I've exaggerated a bit here.... :) > > Actually this routine must be called only if cpufreq_driver->boost > is valid. > > > + return -ENODEV; > > + } > > + > > + policy->boost = cpufreq_boost = cpufreq_driver->boost; > > Why are we copying same pointer to policy->boost? Driver is > passing pointer to a single memory location, just save it globally. This needs some explanation. The sysfs entry presented at [1] doesn't bring any useful information to reuse (like *policy). For this reason the global cpufreq_boost pointer is needed. However to efficiently manage the boost, it is necessary to keep per policy pointer to the only struct cpufreq_boost instance (defined at cpufreq_driver code). > > > + /* disable boost for newly created policy - as we e.g. > > change > > + governor */ > > + policy->boost->status = CPUFREQ_BOOST_DIS; > > Drivers supporting boost may want boost to be enabled by default, > maybe without any sysfs calls. This can be done by setting the struct cpufreq_boost status field to CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is defined) > > > + /* register policy notifier */ > > + ret = > > cpufreq_register_notifier(&cpufreq_boost_notifier_block, > > + CPUFREQ_POLICY_NOTIFIER); > > + if (ret) { > > + pr_err("CPUFREQ BOOST notifier not registered.\n"); > > + return ret; > > + } > > + /* add policy to policies list headed at struct > > cpufreq_boost */ > > + list_add_tail(&policy->boost_list, > > &cpufreq_boost->policies); + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(cpufreq_boost_init); > > Why do we need to maintain a list of boost here? notifiers? complex :( Notifier is needed to disable boost when policy is changed (for example when we change from ondemand/lab to performance governor). I wanted to avoid the situation when boost stays enabled across different governors. The list of in system available policies is defined to allow boost enable/disable for all policies available (by changing for example policy->max field). If we decide, that we will support only one policy (as it is now at e.g. Exynos), the list is unnecessary here. > > > +/********************************************************************* > > * REGISTER / UNREGISTER CPUFREQ > > DRIVER * > > *********************************************************************/ > > > > @@ -1954,6 +2106,10 @@ int cpufreq_unregister_driver(struct > > cpufreq_driver *driver) pr_debug("unregistering driver %s\n", > > driver->name); > > > > subsys_interface_unregister(&cpufreq_interface); > > + > > + if (cpufreq_driver->boost) > > + sysfs_remove_file(cpufreq_global_kobject, > > &(global_boost.attr)); > > You haven't removed this from policy. Memory leak. Yes, you are right. > > > 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..0e95524 100644 > > --- a/drivers/cpufreq/freq_table.c > > +++ b/drivers/cpufreq/freq_table.c > > @@ -3,6 +3,8 @@ > > * > > * Copyright (C) 2002 - 2003 Dominik Brodowski > > * > > + * Copyright (C) 2013 Lukasz Majewski <l.majewski@samsung.com> > > + * > > You shouldn't add it unless you did some major work on this file. You > aren't maintaining this file in 2013. OK, I will remove the entry. > > > +static int cpufreq_frequency_table_skip_boost(struct > > cpufreq_policy *policy, > > + unsigned int index) > > +{ > > + if (index == CPUFREQ_BOOST) > > + if (!policy->boost || > > This shouldn't be true. If index has got CPUFREQ_BOOST, then driver > has to support boost. Correct me if I'm wrong here, but in my understanding the boost shall be only supported when both CPUFREQ_BOOST index is defined in a freq_table and boost.state = CPUFREQ_BOOST_EN is set. Setting of CPUFREQ_BOOST shouldn't by default allow to use over clocking frequency. > > > + policy->boost->status == CPUFREQ_BOOST_DIS) > > + return 1; > > + > > + return 0; > > +} > > + > > +unsigned int > > +cpufreq_frequency_table_boost_max(struct cpufreq_frequency_table > > *freq_table) +{ > > + int index, boost_freq_max; > > + > > + for (index = 0, boost_freq_max = 0; > > + freq_table[index].frequency != CPUFREQ_TABLE_END; > > index++) > > + if (freq_table[index].index == CPUFREQ_BOOST) { > > + if (freq_table[index].frequency > > > boost_freq_max) > > + boost_freq_max = > > freq_table[index].frequency; > > + } > > + > > + return boost_freq_max; > > +} > > +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_boost_max); > > why do we need this? To evaluate the maximal boost frequency from the frequency table. It is then used as a delimiter (when LAB cooperates with thermal framework). > > > /* > > * 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..1294c8c 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -88,6 +88,25 @@ struct cpufreq_real_policy { > > struct cpufreq_governor *governor; /* see below */ > > }; > > > > +#define CPUFREQ_BOOST_DIS (0) > > +#define CPUFREQ_BOOST_EN (1) > > You don't need these.. Just create variable as bool and 0 & 1 would > be fine. Yes, this seems to be a clearer solution. > > > +struct cpufreq_policy; > > +struct cpufreq_boost { > > + unsigned int max_boost_freq; /* maximum value of > > + * boosted freq */ > > + unsigned int max_normal_freq; /* non boost max > > freq */ > > + int status; /* status of boost */ > > + > > + /* boost sysfs attributies */ > > + struct freq_attr **attr; > > + > > + /* low-level trigger for boost */ > > + int (*low_level_boost) (struct cpufreq_policy *policy, int > > state); + > > + struct list_head policies; > > +}; > > We don't need it. Just add two more fields to cpufreq_driver: > - have_boost_freqs and low_level_boost (maybe a better name. > What's its use?) The separate struct cpufreq_boost was created to explicitly separate boost fields from cpufreq_driver structure. If in your opinion this structure is redundant, I can remove it and extend cpufreq_driver structure. > > > struct cpufreq_policy { > > /* CPUs sharing clock, require sw coordination */ > > cpumask_var_t cpus; /* Online CPUs only */ > > @@ -113,6 +132,9 @@ struct cpufreq_policy { > > > > struct cpufreq_real_policy user_policy; > > > > + struct cpufreq_boost *boost; > > + struct list_head boost_list; > > We don't need both of these. *boost pointer is necessary when one wants to enable/disable boost from e.g governor code (which operates mostly on struct cpufreq_policy *policy pointers). The boost_list is necessary to connect policies in a list. > > > struct kobject kobj; > > struct completion kobj_unregister; > > }; > > > @@ -277,7 +302,6 @@ struct cpufreq_driver { > > int cpufreq_register_driver(struct cpufreq_driver *driver_data); > > int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); > > > > - > > ?? > > > void cpufreq_notify_transition(struct cpufreq_policy *policy, > > struct cpufreq_freqs *freqs, unsigned int state); > > > > @@ -403,6 +427,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 ~2 > > s/CPUFREQ_BOOST/CPUFREQ_BOOST_FREQ Ok, will be changed to something more descriptive. Thanks for thorough review :-)
On 6 June 2013 17:19, Lukasz Majewski <l.majewski@samsung.com> wrote: >> On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> wrote: >> > cpufreq_driver->boost->attr)) >> >> Why is this required? Why do we want platforms to add some files >> in sysfs? > > There are two kinds of attributes, which are exported by boost: > > 1. global boost (/sys/devices/system/cpu/cpufreq/boost) > > 2. attributes describing cpufreq abilities when boost is available > (/sys/devices/syste/cpu/cpu0/cpufreq/): > - scaling_boost_frequencies - which will show over clocked > frequencies > - the scaling_available_frequencies will also display boosted > frequency (when boost enabled) > > Information from 2. is cpufreq_driver dependent. And that information > shouldn't been displayed when boost is not available This is not how I wanted this to be coded. Lets keep things simple: - Implement it in the way cpufreq_freq_attr_scaling_available_freqs is implemented. And then drivers which need to see boost freqs can add it in their attr. >> > + policy->boost = cpufreq_boost = cpufreq_driver->boost; >> >> Why are we copying same pointer to policy->boost? Driver is >> passing pointer to a single memory location, just save it globally. > > This needs some explanation. > > The sysfs entry presented at [1] doesn't bring any useful information > to reuse (like *policy). For this reason the global cpufreq_boost > pointer is needed. > > However to efficiently manage the boost, it is necessary to keep per > policy pointer to the only struct cpufreq_boost instance (defined at > cpufreq_driver code). No we don't need to screw struct cpufreq_policy with it. Just need two variables here: - cpufreq_driver->boost: If driver supports boost or not. - If above is true then a global bool variable that will say boost is enabled from sysfs or not. >> > + /* disable boost for newly created policy - as we e.g. >> > change >> > + governor */ >> > + policy->boost->status = CPUFREQ_BOOST_DIS; >> >> Drivers supporting boost may want boost to be enabled by default, >> maybe without any sysfs calls. > > This can be done by setting the struct cpufreq_boost status field to > CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is > defined) This really isn't driver dependent.. But user dependent. Maybe lets keep it disabled and people can enable it from sysfs. >> Why do we need to maintain a list of boost here? notifiers? complex :( > > Notifier is needed to disable boost when policy is changed (for > example when we change from ondemand/lab to performance governor). > > I wanted to avoid the situation when boost stays enabled across > different governors. > > The list of in system available policies is defined to allow boost > enable/disable for all policies available (by changing for example > policy->max field). > > If we decide, that we will support only one policy (as it is now at > e.g. Exynos), the list is unnecessary here. What we discussed last in your earlier patchset was: - Keep boost feature separate from governors. - If it is enabled, then any governor can use it (if they want). - Lets not disable it if governor is changed. user must do it explicitly. >> > +static int cpufreq_frequency_table_skip_boost(struct >> > cpufreq_policy *policy, >> > + unsigned int index) >> > +{ >> > + if (index == CPUFREQ_BOOST) >> > + if (!policy->boost || >> >> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver >> has to support boost. > > Correct me if I'm wrong here, but in my understanding the boost shall be > only supported when both CPUFREQ_BOOST index is defined in a freq_table > and boost.state = CPUFREQ_BOOST_EN is set. > > Setting of CPUFREQ_BOOST shouldn't by default allow to use over > clocking frequency. For cpufreq core boost is enabled as soon as cpufreq_driver->boost is true. We can have additional checks to see if there is atleast one boost frequency but can skip this too. >> why do we need this? > > To evaluate the maximal boost frequency from the frequency table. It is > then used as a delimiter (when LAB cooperates with thermal framework). Introduce this with LAB then.. Lets keep it as simple as possible for now. One step at a time. >> > +struct cpufreq_boost { >> > + unsigned int max_boost_freq; /* maximum value of >> > + * boosted freq */ >> > + unsigned int max_normal_freq; /* non boost max >> > freq */ >> > + int status; /* status of boost */ >> > + >> > + /* boost sysfs attributies */ >> > + struct freq_attr **attr; >> > + >> > + /* low-level trigger for boost */ >> > + int (*low_level_boost) (struct cpufreq_policy *policy, int >> > state); + >> > + struct list_head policies; >> > +}; >> >> We don't need it. Just add two more fields to cpufreq_driver: >> - have_boost_freqs and low_level_boost (maybe a better name. >> What's its use?) > > The separate struct cpufreq_boost was created to explicitly separate > boost fields from cpufreq_driver structure. > > If in your opinion this structure is redundant, I can remove it and > extend cpufreq_driver structure. I am not against a structure (as putting related stuff in a struct is always better), but against so many fields in it to make things complicated. I will only keep have_boost_freqs and low_level_boost. Remove everything else. > *boost pointer is necessary when one wants to enable/disable boost from > e.g governor code (which operates mostly on struct cpufreq_policy > *policy pointers). We don't need to do this. boost can only be disabled from userspace by user. No intervention from governor. -- 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, > On 6 June 2013 17:19, Lukasz Majewski <l.majewski@samsung.com> wrote: > >> On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > > >> > cpufreq_driver->boost->attr)) > >> > >> Why is this required? Why do we want platforms to add some files > >> in sysfs? > > > > There are two kinds of attributes, which are exported by boost: > > > > 1. global boost (/sys/devices/system/cpu/cpufreq/boost) > > > > 2. attributes describing cpufreq abilities when boost is available > > (/sys/devices/syste/cpu/cpu0/cpufreq/): > > - scaling_boost_frequencies - which will show over clocked > > frequencies > > - the scaling_available_frequencies will also display > > boosted frequency (when boost enabled) > > > > Information from 2. is cpufreq_driver dependent. And that > > information shouldn't been displayed when boost is not available > > This is not how I wanted this to be coded. Lets keep things simple: > - Implement it in the way cpufreq_freq_attr_scaling_available_freqs > is implemented. And then drivers which need to see boost freqs > can add it in their attr. > > >> > + policy->boost = cpufreq_boost = cpufreq_driver->boost; > >> > >> Why are we copying same pointer to policy->boost? Driver is > >> passing pointer to a single memory location, just save it globally. > > > > This needs some explanation. > > > > The sysfs entry presented at [1] doesn't bring any useful > > information to reuse (like *policy). For this reason the global > > cpufreq_boost pointer is needed. > > > > However to efficiently manage the boost, it is necessary to keep per > > policy pointer to the only struct cpufreq_boost instance (defined at > > cpufreq_driver code). > > No we don't need to screw struct cpufreq_policy with it. > Just need two variables here: > - cpufreq_driver->boost: If driver supports boost or not. > - If above is true then a global bool variable that will say boost is > enabled from sysfs or not. > > >> > + /* disable boost for newly created policy - as we e.g. > >> > change > >> > + governor */ > >> > + policy->boost->status = CPUFREQ_BOOST_DIS; > >> > >> Drivers supporting boost may want boost to be enabled by default, > >> maybe without any sysfs calls. > > > > This can be done by setting the struct cpufreq_boost status field to > > CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is > > defined) > > This really isn't driver dependent.. But user dependent. Maybe lets > keep it disabled and people can enable it from sysfs. > > >> Why do we need to maintain a list of boost here? notifiers? > >> complex :( > > > > Notifier is needed to disable boost when policy is changed (for > > example when we change from ondemand/lab to performance governor). > > > > I wanted to avoid the situation when boost stays enabled across > > different governors. > > > > The list of in system available policies is defined to allow boost > > enable/disable for all policies available (by changing for example > > policy->max field). > > > > If we decide, that we will support only one policy (as it is now at > > e.g. Exynos), the list is unnecessary here. > > What we discussed last in your earlier patchset was: > - Keep boost feature separate from governors. > - If it is enabled, then any governor can use it (if they want). > - Lets not disable it if governor is changed. user must do it > explicitly. > > >> > +static int cpufreq_frequency_table_skip_boost(struct > >> > cpufreq_policy *policy, > >> > + unsigned int index) > >> > +{ > >> > + if (index == CPUFREQ_BOOST) > >> > + if (!policy->boost || > >> > >> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver > >> has to support boost. > > > > Correct me if I'm wrong here, but in my understanding the boost > > shall be only supported when both CPUFREQ_BOOST index is defined in > > a freq_table and boost.state = CPUFREQ_BOOST_EN is set. > > > > Setting of CPUFREQ_BOOST shouldn't by default allow to use over > > clocking frequency. > > For cpufreq core boost is enabled as soon as cpufreq_driver->boost is > true. We can have additional checks to see if there is atleast one > boost frequency but can skip this too. > > >> why do we need this? > > > > To evaluate the maximal boost frequency from the frequency table. > > It is then used as a delimiter (when LAB cooperates with thermal > > framework). > > Introduce this with LAB then.. Lets keep it as simple as possible for > now. One step at a time. > > >> > +struct cpufreq_boost { > >> > + unsigned int max_boost_freq; /* maximum value > >> > of > >> > + * boosted freq > >> > */ > >> > + unsigned int max_normal_freq; /* non boost max > >> > freq */ > >> > + int status; /* status of boost */ > >> > + > >> > + /* boost sysfs attributies */ > >> > + struct freq_attr **attr; > >> > + > >> > + /* low-level trigger for boost */ > >> > + int (*low_level_boost) (struct cpufreq_policy *policy, > >> > int state); + > >> > + struct list_head policies; > >> > +}; > >> > >> We don't need it. Just add two more fields to cpufreq_driver: > >> - have_boost_freqs and low_level_boost (maybe a better name. > >> What's its use?) > > > > The separate struct cpufreq_boost was created to explicitly separate > > boost fields from cpufreq_driver structure. > > > > If in your opinion this structure is redundant, I can remove it and > > extend cpufreq_driver structure. > > I am not against a structure (as putting related stuff in a struct is > always better), but against so many fields in it to make things > complicated. > > I will only keep have_boost_freqs and low_level_boost. Remove > everything else. I would prefer to have following fields in the cpufreq_boost structure: struct cpufreq_boost { unsigned int max_boost_freq; /*boost max freq*/ unsigned int max_normal_freq; /*max normal freq int (*low_level_boost) (int state); bool boost_en; /* indicate if boost is enabled */ } The max_{boost|normal}_freq fields will be filed at ret = cpufreq_driver->init(policy); Thanks to them I will avoid calling many times routine, which extracts from freq_table maximal boost and normal frequencies. I could define those variables in the exynos-cpufreq.c driver, but I think, that they are more suitable to be embedded at cpufreq_boost structure. > > > *boost pointer is necessary when one wants to enable/disable boost > > from e.g governor code (which operates mostly on struct > > cpufreq_policy *policy pointers). > > We don't need to do this. boost can only be disabled from userspace by > user. No intervention from governor.
Hi Lukasz, On 7 June 2013 18:57, Lukasz Majewski <l.majewski@samsung.com> wrote: I hope you agreed to all the other comments I gave as I don't see an explicit reply below each of these. I have seen people missing these in past, so what would be better to do is: - either reply below each one of them and say yes or no.. - Or write once below many comments and say: All above comments are accepted. So, that Reviewer is assured that you haven't missed anything. > I would prefer to have following fields in the cpufreq_boost structure: > struct cpufreq_boost { > unsigned int max_boost_freq; /*boost max freq*/ > unsigned int max_normal_freq; /*max normal freq > int (*low_level_boost) (int state); > bool boost_en; /* indicate if boost is enabled */ > } > > The max_{boost|normal}_freq fields will be filed at > ret = cpufreq_driver->init(policy); > > Thanks to them I will avoid calling many times routine, which extracts > from freq_table maximal boost and normal frequencies. > > I could define those variables in the exynos-cpufreq.c driver, but I > think, that they are more suitable to be embedded at cpufreq_boost > structure. I understand that you need these variables (I will still look how you are using them in next version). But they are per policy and driver isn't responsible for maintaining them. If they are required then cpufreq core must find them out and keep in struct cpufreq_policy (as they are policy dependent).. So, remove this structure from cpufreq_driver and embed variables directly. -- 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 Lukasz, > > On 7 June 2013 18:57, Lukasz Majewski <l.majewski@samsung.com> wrote: > > I hope you agreed to all the other comments I gave as I don't see an > explicit reply below each of these. I have seen people missing these > in past, so what would be better to do is: > - either reply below each one of them and say yes or no.. > - Or write once below many comments and say: All above comments > are accepted. > > So, that Reviewer is assured that you haven't missed anything. > Thanks for reminding :-). I've read through all the comments. I'm redesigning now the driver to remove redundant code. > > I would prefer to have following fields in the cpufreq_boost > > structure: struct cpufreq_boost { > > unsigned int max_boost_freq; /*boost max freq*/ > > unsigned int max_normal_freq; /*max normal freq > > int (*low_level_boost) (int state); > > bool boost_en; /* indicate if boost is enabled */ > > } > > > > The max_{boost|normal}_freq fields will be filed at > > ret = cpufreq_driver->init(policy); > > > > Thanks to them I will avoid calling many times routine, which > > extracts from freq_table maximal boost and normal frequencies. > > > > I could define those variables in the exynos-cpufreq.c driver, but I > > think, that they are more suitable to be embedded at cpufreq_boost > > structure. > > I understand that you need these variables (I will still look how you > are using them in next version). But they are per policy and driver > isn't responsible for maintaining them. If they are required then > cpufreq core must find them out and keep in struct cpufreq_policy (as > they are policy dependent).. > > So, remove this structure from cpufreq_driver and embed variables > directly. After refactoring the code. I admit, that we shall embed the struct cpu_boost fields directly to the coufreq_driver. There is no point to create structure with 2 fields.
Hi Viresh, > On 6 June 2013 17:19, Lukasz Majewski <l.majewski@samsung.com> wrote: > >> On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > > >> > cpufreq_driver->boost->attr)) > >> > >> Why is this required? Why do we want platforms to add some files > >> in sysfs? > > > > There are two kinds of attributes, which are exported by boost: > > > > 1. global boost (/sys/devices/system/cpu/cpufreq/boost) > > > > 2. attributes describing cpufreq abilities when boost is available > > (/sys/devices/syste/cpu/cpu0/cpufreq/): > > - scaling_boost_frequencies - which will show over clocked > > frequencies > > - the scaling_available_frequencies will also display > > boosted frequency (when boost enabled) > > > > Information from 2. is cpufreq_driver dependent. And that > > information shouldn't been displayed when boost is not available > > This is not how I wanted this to be coded. Lets keep things simple: > - Implement it in the way cpufreq_freq_attr_scaling_available_freqs > is implemented. And then drivers which need to see boost freqs > can add it in their attr. I've added scaling_boost_frequencies to cpufreq_driver attr. However, I would keep the boost attributes definition in the freq_table file (as I've proposed in my patch). > > >> > + policy->boost = cpufreq_boost = cpufreq_driver->boost; > >> > >> Why are we copying same pointer to policy->boost? Driver is > >> passing pointer to a single memory location, just save it globally. > > > > This needs some explanation. > > > > The sysfs entry presented at [1] doesn't bring any useful > > information to reuse (like *policy). For this reason the global > > cpufreq_boost pointer is needed. > > > > However to efficiently manage the boost, it is necessary to keep per > > policy pointer to the only struct cpufreq_boost instance (defined at > > cpufreq_driver code). > > No we don't need to screw struct cpufreq_policy with it. > Just need two variables here: > - cpufreq_driver->boost: If driver supports boost or not. This will be done as above. > - If above is true then a global bool variable that will say boost is > enabled from sysfs or not. One global flag will be defined at cpufreq.c to indicate if global boost sysfs attr has been created. > > >> > + /* disable boost for newly created policy - as we e.g. > >> > change > >> > + governor */ > >> > + policy->boost->status = CPUFREQ_BOOST_DIS; > >> > >> Drivers supporting boost may want boost to be enabled by default, > >> maybe without any sysfs calls. > > > > This can be done by setting the struct cpufreq_boost status field to > > CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is > > defined) > > This really isn't driver dependent.. But user dependent. Maybe lets > keep it disabled and people can enable it from sysfs. The cpufreq_driver struct will have boost_en field. This will allow keep boost state (it is similar to global boost_enable at acpi-cpufreq.c). > > >> Why do we need to maintain a list of boost here? notifiers? > >> complex :( > > > > Notifier is needed to disable boost when policy is changed (for > > example when we change from ondemand/lab to performance governor). > > > > I wanted to avoid the situation when boost stays enabled across > > different governors. > > > > The list of in system available policies is defined to allow boost > > enable/disable for all policies available (by changing for example > > policy->max field). > > > > If we decide, that we will support only one policy (as it is now at > > e.g. Exynos), the list is unnecessary here. > > What we discussed last in your earlier patchset was: > - Keep boost feature separate from governors. Ok. > - If it is enabled, then any governor can use it (if they want). Ok, lets do it in this way > - Lets not disable it if governor is changed. user must do it > explicitly. Ok, agree (notifier removed). > > >> > +static int cpufreq_frequency_table_skip_boost(struct > >> > cpufreq_policy *policy, > >> > + unsigned int index) > >> > +{ > >> > + if (index == CPUFREQ_BOOST) > >> > + if (!policy->boost || > >> > >> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver > >> has to support boost. > > > > Correct me if I'm wrong here, but in my understanding the boost > > shall be only supported when both CPUFREQ_BOOST index is defined in > > a freq_table and boost.state = CPUFREQ_BOOST_EN is set. > > > > Setting of CPUFREQ_BOOST shouldn't by default allow to use over > > clocking frequency. > > For cpufreq core boost is enabled as soon as cpufreq_driver->boost is > true. We can have additional checks to see if there is atleast one > boost frequency but can skip this too. Checks are needed to read max_normal frequency and max boost frequency from frequency table. In exynos cpufreq_driver->init() I will disable boost. > > >> why do we need this? > > > > To evaluate the maximal boost frequency from the frequency table. > > It is then used as a delimiter (when LAB cooperates with thermal > > framework). > > Introduce this with LAB then.. Lets keep it as simple as possible for > now. One step at a time. Sorry, I have LAB in back of my head. For now I'm forgetting about it :-) [*] > > >> > +struct cpufreq_boost { > >> > + unsigned int max_boost_freq; /* maximum value > >> > of > >> > + * boosted freq > >> > */ > >> > + unsigned int max_normal_freq; /* non boost max > >> > freq */ > >> > + int status; /* status of boost */ > >> > + > >> > + /* boost sysfs attributies */ > >> > + struct freq_attr **attr; > >> > + > >> > + /* low-level trigger for boost */ > >> > + int (*low_level_boost) (struct cpufreq_policy *policy, > >> > int state); + > >> > + struct list_head policies; > >> > +}; > >> > >> We don't need it. Just add two more fields to cpufreq_driver: > >> - have_boost_freqs and low_level_boost (maybe a better name. > >> What's its use?) > > > > The separate struct cpufreq_boost was created to explicitly separate > > boost fields from cpufreq_driver structure. > > > > If in your opinion this structure is redundant, I can remove it and > > extend cpufreq_driver structure. > > I am not against a structure (as putting related stuff in a struct is > always better), but against so many fields in it to make things > complicated. > > I will only keep have_boost_freqs and low_level_boost. Remove > everything else. As I've written at other mail. This struct will have only two fields, so I will embed those fields at cpufreq_driver. > > > *boost pointer is necessary when one wants to enable/disable boost > > from e.g governor code (which operates mostly on struct > > cpufreq_policy *policy pointers). > > We don't need to do this. boost can only be disabled from userspace by > user. No intervention from governor. Let's got for that option (as I've promissed at [*] :-) ).
On 7 June 2013 20:04, Lukasz Majewski <l.majewski@samsung.com> wrote: > After refactoring the code. I admit, that we shall embed the struct > cpu_boost fields directly to the coufreq_driver. There is no point to > create structure with 2 fields. Great!! I will wait for your next version. -- 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 ca74e27..8cf9a92 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -133,6 +133,11 @@ bool have_governor_per_policy(void) return cpufreq_driver->have_governor_per_policy; } +/** + * Global definition of cpufreq_boost structure + */ +struct cpufreq_boost *cpufreq_boost; + static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs) { struct cpufreq_policy *data; @@ -315,6 +320,45 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /********************************************************************* * SYSFS INTERFACE * *********************************************************************/ +ssize_t show_boost_status(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + struct cpufreq_boost *boost = cpufreq_boost; + + if (!boost) + return -ENODEV; + + return sprintf(buf, "%d\n", boost->status); +} + +static ssize_t store_boost_status(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + struct cpufreq_boost *boost = cpufreq_boost; + struct cpufreq_policy *p; + int ret, enable; + + if (!boost) + return -ENODEV; + + ret = sscanf(buf, "%d", &enable); + if (ret != 1 || enable < 0 || enable > 1) + return -EINVAL; + + /* Adjust all policies to support boost */ + list_for_each_entry(p, &boost->policies, boost_list) + if (cpufreq_boost_trigger_state(p, enable)) { + pr_err("Cannot %sable boost (policy 0x%p)!\n", + enable ? "en" : "dis", p); + 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) { @@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu, if (cpufreq_set_drv_attr_files(policy, cpufreq_driver->attr)) goto err_out_kobj_put; + if (cpufreq_driver->boost) { + if (sysfs_create_file(cpufreq_global_kobject, + &(global_boost.attr))) + pr_warn("could not register global boost sysfs file\n"); + else + pr_debug("registered global boost sysfs file\n"); + + if (cpufreq_set_drv_attr_files(policy, + cpufreq_driver->boost->attr)) + goto err_out_kobj_put; + } + if (cpufreq_driver->get) { ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr); if (ret) @@ -923,6 +979,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) init_completion(&policy->kobj_unregister); INIT_WORK(&policy->update, handle_update); + cpufreq_boost_init(policy); + /* call driver. From then on the cpufreq must be able * to accept all calls to ->verify and ->setpolicy for this CPU */ @@ -1860,6 +1918,100 @@ static struct notifier_block __refdata cpufreq_cpu_notifier = { }; /********************************************************************* + * BOOST * + *********************************************************************/ +int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state) +{ + struct cpufreq_boost *boost; + unsigned long flags; + int ret = 0; + + if (!policy || !policy->boost || !policy->boost->low_level_boost) + return -ENODEV; + + boost = policy->boost; + write_lock_irqsave(&cpufreq_driver_lock, flags); + + if (boost->status != state) { + policy->boost->status = state; + ret = boost->low_level_boost(policy, state); + if (ret) { + pr_err("BOOST cannot %sable low level code (%d)\n", + state ? "en" : "dis", ret); + return ret; + } + } + + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + + pr_debug("cpufreq BOOST %sabled\n", state ? "en" : "dis"); + + return 0; +} + +/** + * cpufreq_boost_notifier - notifier callback for cpufreq policy change. + * <at> nb: struct notifier_block * with callback info. + * <at> event: value showing cpufreq event for which this function invoked. + * <at> data: callback-specific data + */ +static int cpufreq_boost_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct cpufreq_policy *policy = data; + + if (event == CPUFREQ_INCOMPATIBLE) { + if (!policy || !policy->boost) + return -ENODEV; + + if (policy->boost->status == CPUFREQ_BOOST_EN) { + pr_info("NOTIFIER BOOST: MAX: %d e:%lu cpu: %d\n", + policy->max, event, policy->cpu); + cpufreq_boost_trigger_state(policy, 0); + } + } + + return 0; +} + +/* Notifier for cpufreq policy change */ +static struct notifier_block cpufreq_boost_notifier_block = { + .notifier_call = cpufreq_boost_notifier, +}; + +int cpufreq_boost_init(struct cpufreq_policy *policy) +{ + int ret = 0; + + if (!policy) + return -EINVAL; + + if (!cpufreq_driver->boost) { + pr_err("Boost mode not supported on this device\n"); + return -ENODEV; + } + + policy->boost = cpufreq_boost = cpufreq_driver->boost; + + /* disable boost for newly created policy - as we e.g. change + governor */ + policy->boost->status = CPUFREQ_BOOST_DIS; + + /* register policy notifier */ + ret = cpufreq_register_notifier(&cpufreq_boost_notifier_block, + CPUFREQ_POLICY_NOTIFIER); + if (ret) { + pr_err("CPUFREQ BOOST notifier not registered.\n"); + return ret; + } + /* add policy to policies list headed at struct cpufreq_boost */ + list_add_tail(&policy->boost_list, &cpufreq_boost->policies); + + return 0; +} +EXPORT_SYMBOL_GPL(cpufreq_boost_init); + +/********************************************************************* * REGISTER / UNREGISTER CPUFREQ DRIVER * *********************************************************************/ @@ -1954,6 +2106,10 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) pr_debug("unregistering driver %s\n", driver->name); subsys_interface_unregister(&cpufreq_interface); + + if (cpufreq_driver->boost) + sysfs_remove_file(cpufreq_global_kobject, &(global_boost.attr)); + 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..0e95524 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -3,6 +3,8 @@ * * Copyright (C) 2002 - 2003 Dominik Brodowski * + * Copyright (C) 2013 Lukasz Majewski <l.majewski@samsung.com> + * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. @@ -20,6 +22,36 @@ * FREQUENCY TABLE HELPERS * *********************************************************************/ +/********************************************************************* + * BOOST FREQ HELPERS * + *********************************************************************/ +static int cpufreq_frequency_table_skip_boost(struct cpufreq_policy *policy, + unsigned int index) +{ + if (index == CPUFREQ_BOOST) + if (!policy->boost || + policy->boost->status == CPUFREQ_BOOST_DIS) + return 1; + + return 0; +} + +unsigned int +cpufreq_frequency_table_boost_max(struct cpufreq_frequency_table *freq_table) +{ + int index, boost_freq_max; + + for (index = 0, boost_freq_max = 0; + freq_table[index].frequency != CPUFREQ_TABLE_END; index++) + if (freq_table[index].index == CPUFREQ_BOOST) { + if (freq_table[index].frequency > boost_freq_max) + boost_freq_max = freq_table[index].frequency; + } + + return boost_freq_max; +} +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_boost_max); + int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, struct cpufreq_frequency_table *table) { @@ -34,6 +66,10 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, continue; } + if (cpufreq_frequency_table_skip_boost(policy, + table[i].index)) + continue; + pr_debug("table entry %u: %u kHz, %u index\n", i, freq, table[i].index); if (freq < min_freq) @@ -70,6 +106,9 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy *policy, unsigned int freq = table[i].frequency; if (freq == CPUFREQ_ENTRY_INVALID) continue; + if (cpufreq_frequency_table_skip_boost(policy, + table[i].index)) + continue; if ((freq >= policy->min) && (freq <= policy->max)) count++; else if ((next_larger > freq) && (freq > policy->max)) @@ -122,6 +161,9 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int freq = table[i].frequency; if (freq == CPUFREQ_ENTRY_INVALID) continue; + if (cpufreq_frequency_table_skip_boost(policy, + table[i].index)) + continue; if ((freq < policy->min) || (freq > policy->max)) continue; switch (relation) { @@ -167,11 +209,15 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, } EXPORT_SYMBOL_GPL(cpufreq_frequency_table_target); +#define CPUFREQ_SHOW_NORMAL 0 +#define CPUFREQ_SHOW_BOOST 1 + 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 +232,59 @@ 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) + continue; + } else { + if (cpufreq_frequency_table_skip_boost(policy, + table[i].index)) + 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, CPUFREQ_SHOW_NORMAL); } 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, CPUFREQ_SHOW_BOOST); +} + +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..1294c8c 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -88,6 +88,25 @@ struct cpufreq_real_policy { struct cpufreq_governor *governor; /* see below */ }; +#define CPUFREQ_BOOST_DIS (0) +#define CPUFREQ_BOOST_EN (1) + +struct cpufreq_policy; +struct cpufreq_boost { + unsigned int max_boost_freq; /* maximum value of + * boosted freq */ + unsigned int max_normal_freq; /* non boost max freq */ + int status; /* status of boost */ + + /* boost sysfs attributies */ + struct freq_attr **attr; + + /* low-level trigger for boost */ + int (*low_level_boost) (struct cpufreq_policy *policy, int state); + + struct list_head policies; +}; + struct cpufreq_policy { /* CPUs sharing clock, require sw coordination */ cpumask_var_t cpus; /* Online CPUs only */ @@ -113,6 +132,9 @@ struct cpufreq_policy { struct cpufreq_real_policy user_policy; + struct cpufreq_boost *boost; + struct list_head boost_list; + struct kobject kobj; struct completion kobj_unregister; }; @@ -262,6 +284,9 @@ struct cpufreq_driver { int (*suspend) (struct cpufreq_policy *policy); int (*resume) (struct cpufreq_policy *policy); struct freq_attr **attr; + + /* platform specific boost support code */ + struct cpufreq_boost *boost; }; /* flags */ @@ -277,7 +302,6 @@ struct cpufreq_driver { int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); - void cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state); @@ -403,6 +427,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 ~2 + struct cpufreq_frequency_table { unsigned int index; /* any */ unsigned int frequency; /* kHz - doesn't need to be in ascending @@ -421,11 +448,17 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int relation, unsigned int *index); +unsigned int +cpufreq_frequency_table_boost_max(struct cpufreq_frequency_table *freq_table); +int cpufreq_boost_init(struct cpufreq_policy *policy); + /* the following 3 funtions are for cpufreq core use only */ struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu); +int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state); /* 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);