Hi Rafael/Eduardo,
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence ->usable() callback.
The first patch fixes few formatting issues, so that the third patch doesn't throw any checkpatch warnings. Second one fixes a potential bug in cpufreq-dt driver. Third one introduces ->usable() callback which will be used in the fourth patch.
Last three are fixes for cooling core, which may be applied separately by Eduardo if he wants. Sent them in this series as they were sort of connected with cpufreq in general.
Let me know if it still doesn't work properly.
-- viresh
Viresh Kumar (7): cpufreq: Fix formatting issues in 'struct cpufreq_driver' cpufreq-dt: pass 'policy->related_cpus' to of_cpufreq_cooling_register() cpufreq: Introduce ->usable() callback for cpufreq drivers cpufreq-dt: register cooling device from ->usable() callback cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy cpu_cooling: No need to check is_cpufreq_valid()
drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++--------------- drivers/cpufreq/cpufreq.c | 5 ++++ drivers/thermal/cpu_cooling.c | 44 ++++------------------------------- include/linux/cpufreq.h | 54 +++++++++++++++++++++++-------------------- 4 files changed, 70 insertions(+), 84 deletions(-)
Adding any new callback to 'struct cpufreq_driver' gives following checkpatch warning:
WARNING: Unnecessary space before function pointer arguments + void (*usable) (struct cpufreq_policy *policy);
This is because we have been using a tab spacing between function pointer name and its arguments and the new one tried to follow that.
Though we normally don't try to fix every checkpatch warning, specially around formatting issues as that creates unnecessary noise over lists. But I thought we better fix this so that new additions don't generate these warnings plus it looks far better/symmetric now.
So, remove these tab spacing issues in 'struct cpufreq_driver' only + fix alignment of all members.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/cpufreq.h | 50 ++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 503b085..db3c130 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -217,26 +217,26 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
struct cpufreq_driver { - char name[CPUFREQ_NAME_LEN]; - u8 flags; - void *driver_data; + char name[CPUFREQ_NAME_LEN]; + u8 flags; + void *driver_data;
/* needed by all drivers */ - int (*init) (struct cpufreq_policy *policy); - int (*verify) (struct cpufreq_policy *policy); + int (*init)(struct cpufreq_policy *policy); + int (*verify)(struct cpufreq_policy *policy);
/* define one out of two */ - int (*setpolicy) (struct cpufreq_policy *policy); + int (*setpolicy)(struct cpufreq_policy *policy);
/* * On failure, should always restore frequency to policy->restore_freq * (i.e. old freq). */ - int (*target) (struct cpufreq_policy *policy, /* Deprecated */ - unsigned int target_freq, - unsigned int relation); - int (*target_index) (struct cpufreq_policy *policy, - unsigned int index); + int (*target)(struct cpufreq_policy *policy, + unsigned int target_freq, + unsigned int relation); /* Deprecated */ + int (*target_index)(struct cpufreq_policy *policy, + unsigned int index); /* * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION * unset. @@ -252,27 +252,27 @@ struct cpufreq_driver { * wish to switch to intermediate frequency for some target frequency. * In that case core will directly call ->target_index(). */ - unsigned int (*get_intermediate)(struct cpufreq_policy *policy, - unsigned int index); - int (*target_intermediate)(struct cpufreq_policy *policy, - unsigned int index); + unsigned int (*get_intermediate)(struct cpufreq_policy *policy, + unsigned int index); + int (*target_intermediate)(struct cpufreq_policy *policy, + unsigned int index);
/* should be defined, if possible */ - unsigned int (*get) (unsigned int cpu); + unsigned int (*get)(unsigned int cpu);
/* optional */ - int (*bios_limit) (int cpu, unsigned int *limit); + int (*bios_limit)(int cpu, unsigned int *limit);
- int (*exit) (struct cpufreq_policy *policy); - void (*stop_cpu) (struct cpufreq_policy *policy); - int (*suspend) (struct cpufreq_policy *policy); - int (*resume) (struct cpufreq_policy *policy); - struct freq_attr **attr; + int (*exit)(struct cpufreq_policy *policy); + void (*stop_cpu)(struct cpufreq_policy *policy); + int (*suspend)(struct cpufreq_policy *policy); + int (*resume)(struct cpufreq_policy *policy); + struct freq_attr **attr;
/* platform specific boost support code */ - bool boost_supported; - bool boost_enabled; - int (*set_boost) (int state); + bool boost_supported; + bool boost_enabled; + int (*set_boost)(int state); };
/* flags */
On Wed, Nov 26, 2014 at 11:22:56AM +0530, Viresh Kumar wrote:
Adding any new callback to 'struct cpufreq_driver' gives following checkpatch warning:
WARNING: Unnecessary space before function pointer arguments
- void (*usable) (struct cpufreq_policy *policy);
This is because we have been using a tab spacing between function pointer name and its arguments and the new one tried to follow that.
Though we normally don't try to fix every checkpatch warning, specially around formatting issues as that creates unnecessary noise over lists. But I thought we better fix this so that new additions don't generate these warnings plus it looks far better/symmetric now.
So, remove these tab spacing issues in 'struct cpufreq_driver' only + fix alignment of all members.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Eduardo Valentin edubezval@gmail.com Tested-by: Eduardo Valentin edubezval@gmail.com
include/linux/cpufreq.h | 50 ++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 503b085..db3c130 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -217,26 +217,26 @@ __ATTR(_name, 0644, show_##_name, store_##_name) struct cpufreq_driver {
- char name[CPUFREQ_NAME_LEN];
- u8 flags;
- void *driver_data;
- char name[CPUFREQ_NAME_LEN];
- u8 flags;
- void *driver_data;
/* needed by all drivers */
- int (*init) (struct cpufreq_policy *policy);
- int (*verify) (struct cpufreq_policy *policy);
- int (*init)(struct cpufreq_policy *policy);
- int (*verify)(struct cpufreq_policy *policy);
/* define one out of two */
- int (*setpolicy) (struct cpufreq_policy *policy);
- int (*setpolicy)(struct cpufreq_policy *policy);
/* * On failure, should always restore frequency to policy->restore_freq * (i.e. old freq). */
- int (*target) (struct cpufreq_policy *policy, /* Deprecated */
unsigned int target_freq,unsigned int relation);- int (*target_index) (struct cpufreq_policy *policy,
unsigned int index);
- int (*target)(struct cpufreq_policy *policy,
unsigned int target_freq,unsigned int relation); /* Deprecated */- int (*target_index)(struct cpufreq_policy *policy,
unsigned int index);
- Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
- unset.
@@ -252,27 +252,27 @@ struct cpufreq_driver { * wish to switch to intermediate frequency for some target frequency. * In that case core will directly call ->target_index(). */
- unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
unsigned int index);- int (*target_intermediate)(struct cpufreq_policy *policy,
unsigned int index);
- unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
unsigned int index);- int (*target_intermediate)(struct cpufreq_policy *policy,
unsigned int index);/* should be defined, if possible */
- unsigned int (*get) (unsigned int cpu);
- unsigned int (*get)(unsigned int cpu);
/* optional */
- int (*bios_limit) (int cpu, unsigned int *limit);
- int (*bios_limit)(int cpu, unsigned int *limit);
- int (*exit) (struct cpufreq_policy *policy);
- void (*stop_cpu) (struct cpufreq_policy *policy);
- int (*suspend) (struct cpufreq_policy *policy);
- int (*resume) (struct cpufreq_policy *policy);
- struct freq_attr **attr;
- int (*exit)(struct cpufreq_policy *policy);
- void (*stop_cpu)(struct cpufreq_policy *policy);
- int (*suspend)(struct cpufreq_policy *policy);
- int (*resume)(struct cpufreq_policy *policy);
- struct freq_attr **attr;
/* platform specific boost support code */
- bool boost_supported;
- bool boost_enabled;
- int (*set_boost) (int state);
- bool boost_supported;
- bool boost_enabled;
- int (*set_boost)(int state);
}; /* flags */ -- 2.0.3.693.g996b0fd
The second parameter of of_cpufreq_cooling_register() should be the CPUs to which the frequency constraint will apply. As the cpufreq-dt driver now supports platforms with multiple 'struct cpufreq_policy' instances (i.e. > 1 clock domains for CPUs), passing 'cpu_present_mask' isn't correct anymore. As every policy will have a set of CPUs and that may not be equal to 'cpu_present_mask' always.
So, pass only mask of CPUs which are controlled by current policy.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-dt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 8cba13d..7374fc4 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -274,7 +274,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) * thermal DT code takes care of matching them. */ if (of_find_property(np, "#cooling-cells", NULL)) { - cdev = of_cpufreq_cooling_register(np, cpu_present_mask); + cdev = of_cpufreq_cooling_register(np, policy->related_cpus); if (IS_ERR(cdev)) dev_err(cpu_dev, "running cpufreq without cooling device: %ld\n",
On Wed, Nov 26, 2014 at 11:22:57AM +0530, Viresh Kumar wrote:
The second parameter of of_cpufreq_cooling_register() should be the CPUs to which the frequency constraint will apply. As the cpufreq-dt driver now supports platforms with multiple 'struct cpufreq_policy' instances (i.e. > 1 clock domains for CPUs), passing 'cpu_present_mask' isn't correct anymore. As every policy will have a set of CPUs and that may not be equal to 'cpu_present_mask' always.
So, pass only mask of CPUs which are controlled by current policy.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Eduardo Valentin edubezval@gmail.com Tested-by: Eduardo Valentin edubezval@gmail.com
drivers/cpufreq/cpufreq-dt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 8cba13d..7374fc4 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -274,7 +274,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) * thermal DT code takes care of matching them. */ if (of_find_property(np, "#cooling-cells", NULL)) {
cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
cdev = of_cpufreq_cooling_register(np, policy->related_cpus);-- 2.0.3.693.g996b0fd
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence we introduce ->usable() callback.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 5 +++++ include/linux/cpufreq.h | 4 ++++ 2 files changed, 9 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index de2c3e1..4fb95b9 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1285,8 +1285,13 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) up_write(&policy->rwsem);
kobject_uevent(&policy->kobj, KOBJ_ADD); + up_read(&cpufreq_rwsem);
+ /* Callback for handling stuff after policy is ready */ + if (cpufreq_driver->usable) + cpufreq_driver->usable(policy); + pr_debug("initialization complete\n");
return 0; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index db3c130..4795c0b 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -267,6 +267,10 @@ struct cpufreq_driver { void (*stop_cpu)(struct cpufreq_policy *policy); int (*suspend)(struct cpufreq_policy *policy); int (*resume)(struct cpufreq_policy *policy); + + /* Will be called after the driver is fully initialized */ + void (*usable)(struct cpufreq_policy *policy); + struct freq_attr **attr;
/* platform specific boost support code */
On Wed, Nov 26, 2014 at 11:22:58AM +0530, Viresh Kumar wrote:
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence we introduce ->usable() callback.
Reviewed-by: Eduardo Valentin edubezval@gmail.com Tested-by: Eduardo Valentin edubezval@gmail.com
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 5 +++++ include/linux/cpufreq.h | 4 ++++ 2 files changed, 9 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index de2c3e1..4fb95b9 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1285,8 +1285,13 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) up_write(&policy->rwsem); kobject_uevent(&policy->kobj, KOBJ_ADD);
- up_read(&cpufreq_rwsem);
- /* Callback for handling stuff after policy is ready */
- if (cpufreq_driver->usable)
cpufreq_driver->usable(policy);- pr_debug("initialization complete\n");
return 0; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index db3c130..4795c0b 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -267,6 +267,10 @@ struct cpufreq_driver { void (*stop_cpu)(struct cpufreq_policy *policy); int (*suspend)(struct cpufreq_policy *policy); int (*resume)(struct cpufreq_policy *policy);
- /* Will be called after the driver is fully initialized */
- void (*usable)(struct cpufreq_policy *policy);
- struct freq_attr **attr;
/* platform specific boost support code */ -- 2.0.3.693.g996b0fd
On Wednesday, November 26, 2014 11:22:58 AM Viresh Kumar wrote:
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence we introduce ->usable() callback.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 5 +++++ include/linux/cpufreq.h | 4 ++++ 2 files changed, 9 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index de2c3e1..4fb95b9 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1285,8 +1285,13 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) up_write(&policy->rwsem); kobject_uevent(&policy->kobj, KOBJ_ADD);
- up_read(&cpufreq_rwsem);
- /* Callback for handling stuff after policy is ready */
- if (cpufreq_driver->usable)
cpufreq_driver->usable(policy);- pr_debug("initialization complete\n");
return 0; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index db3c130..4795c0b 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -267,6 +267,10 @@ struct cpufreq_driver { void (*stop_cpu)(struct cpufreq_policy *policy); int (*suspend)(struct cpufreq_policy *policy); int (*resume)(struct cpufreq_policy *policy);
- /* Will be called after the driver is fully initialized */
- void (*usable)(struct cpufreq_policy *policy);
What about
void (*ready)(struct cpufreq_policy *policy);
- struct freq_attr **attr;
/* platform specific boost support code */
On 27 November 2014 at 05:55, Rafael J. Wysocki rjw@rjwysocki.net wrote:
void (*usable)(struct cpufreq_policy *policy);What about
void (*ready)(struct cpufreq_policy *policy);
That was the second option doing rounds in my thoughts :) As two people like it now (You and Me), lets move to that :)
-- viresh
Currently we are calling of_cpufreq_cooling_register() from ->init() callback. At this point of time cpufreq driver's policy isn't completely ready to be used as few of its fields/structure/pointers aren't yet initialized.
Because of_cpufreq_cooling_register() tries to access policy with help of cpufreq_cpu_get() and then tries to get freq-table as well, these calls fail.
To fix this, register the cooling device after the policy is ready to be used. And the right callback for it is the newly added ->usable() one.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 7374fc4..f7af5c8 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) { struct cpufreq_dt_platform_data *pd; struct cpufreq_frequency_table *freq_table; - struct thermal_cooling_device *cdev; struct device_node *np; struct private_data *priv; struct device *cpu_dev; @@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) goto out_free_priv; }
- /* - * For now, just loading the cooling device; - * thermal DT code takes care of matching them. - */ - if (of_find_property(np, "#cooling-cells", NULL)) { - cdev = of_cpufreq_cooling_register(np, policy->related_cpus); - if (IS_ERR(cdev)) - dev_err(cpu_dev, - "running cpufreq without cooling device: %ld\n", - PTR_ERR(cdev)); - else - priv->cdev = cdev; - } - priv->cpu_dev = cpu_dev; priv->cpu_reg = cpu_reg; policy->driver_data = priv; @@ -292,7 +277,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) if (ret) { dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, ret); - goto out_cooling_unregister; + goto out_free_cpufreq_table; }
policy->cpuinfo.transition_latency = transition_latency; @@ -305,8 +290,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
return 0;
-out_cooling_unregister: - cpufreq_cooling_unregister(priv->cdev); +out_free_cpufreq_table: dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); out_free_priv: kfree(priv); @@ -324,7 +308,8 @@ static int cpufreq_exit(struct cpufreq_policy *policy) { struct private_data *priv = policy->driver_data;
- cpufreq_cooling_unregister(priv->cdev); + if (priv->cdev) + cpufreq_cooling_unregister(priv->cdev); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); clk_put(policy->clk); if (!IS_ERR(priv->cpu_reg)) @@ -334,6 +319,33 @@ static int cpufreq_exit(struct cpufreq_policy *policy) return 0; }
+static void cpufreq_usable(struct cpufreq_policy *policy) +{ + struct private_data *priv = policy->driver_data; + struct device_node *np = of_node_get(priv->cpu_dev->of_node); + + if (WARN_ON(!np)) + return; + + /* + * For now, just loading the cooling device; + * thermal DT code takes care of matching them. + */ + if (of_find_property(np, "#cooling-cells", NULL)) { + priv->cdev = of_cpufreq_cooling_register(np, + policy->related_cpus); + if (IS_ERR(priv->cdev)) { + dev_err(priv->cpu_dev, + "running cpufreq without cooling device: %ld\n", + PTR_ERR(priv->cdev)); + + priv->cdev = NULL; + } + } + + of_node_put(np); +} + static struct cpufreq_driver dt_cpufreq_driver = { .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK, .verify = cpufreq_generic_frequency_table_verify, @@ -341,6 +353,7 @@ static struct cpufreq_driver dt_cpufreq_driver = { .get = cpufreq_generic_get, .init = cpufreq_init, .exit = cpufreq_exit, + .usable = cpufreq_usable, .name = "cpufreq-dt", .attr = cpufreq_generic_attr, };
On Wed, Nov 26, 2014 at 11:22:59AM +0530, Viresh Kumar wrote:
Currently we are calling of_cpufreq_cooling_register() from ->init() callback. At this point of time cpufreq driver's policy isn't completely ready to be used as few of its fields/structure/pointers aren't yet initialized.
Because of_cpufreq_cooling_register() tries to access policy with help of cpufreq_cpu_get() and then tries to get freq-table as well, these calls fail.
To fix this, register the cooling device after the policy is ready to be used. And the right callback for it is the newly added ->usable() one.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Eduardo Valentin edubezval@gmail.com Tested-by: Eduardo Valentin edubezval@gmail.com
drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 7374fc4..f7af5c8 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) { struct cpufreq_dt_platform_data *pd; struct cpufreq_frequency_table *freq_table;
- struct thermal_cooling_device *cdev; struct device_node *np; struct private_data *priv; struct device *cpu_dev;
@@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) goto out_free_priv; }
- /*
* For now, just loading the cooling device;* thermal DT code takes care of matching them.*/- if (of_find_property(np, "#cooling-cells", NULL)) {
cdev = of_cpufreq_cooling_register(np, policy->related_cpus);if (IS_ERR(cdev))dev_err(cpu_dev,"running cpufreq without cooling device: %ld\n",PTR_ERR(cdev));elsepriv->cdev = cdev;- }
- priv->cpu_dev = cpu_dev; priv->cpu_reg = cpu_reg; policy->driver_data = priv;
@@ -292,7 +277,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) if (ret) { dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, ret);
goto out_cooling_unregister;
goto out_free_cpufreq_table;policy->cpuinfo.transition_latency = transition_latency; @@ -305,8 +290,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) return 0; -out_cooling_unregister:
- cpufreq_cooling_unregister(priv->cdev);
+out_free_cpufreq_table: dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); out_free_priv: kfree(priv); @@ -324,7 +308,8 @@ static int cpufreq_exit(struct cpufreq_policy *policy) { struct private_data *priv = policy->driver_data;
- cpufreq_cooling_unregister(priv->cdev);
- if (priv->cdev)
cpufreq_cooling_unregister(priv->cdev);@@ -334,6 +319,33 @@ static int cpufreq_exit(struct cpufreq_policy *policy) return 0; } +static void cpufreq_usable(struct cpufreq_policy *policy) +{
- struct private_data *priv = policy->driver_data;
- struct device_node *np = of_node_get(priv->cpu_dev->of_node);
- if (WARN_ON(!np))
return;- /*
* For now, just loading the cooling device;* thermal DT code takes care of matching them.*/- if (of_find_property(np, "#cooling-cells", NULL)) {
priv->cdev = of_cpufreq_cooling_register(np,policy->related_cpus);if (IS_ERR(priv->cdev)) {dev_err(priv->cpu_dev,"running cpufreq without cooling device: %ld\n",PTR_ERR(priv->cdev));priv->cdev = NULL;}- }
- of_node_put(np);
+}
static struct cpufreq_driver dt_cpufreq_driver = { .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK, .verify = cpufreq_generic_frequency_table_verify, @@ -341,6 +353,7 @@ static struct cpufreq_driver dt_cpufreq_driver = { .get = cpufreq_generic_get, .init = cpufreq_init, .exit = cpufreq_exit,
- .usable = cpufreq_usable, .name = "cpufreq-dt", .attr = cpufreq_generic_attr,
};
2.0.3.693.g996b0fd
In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency constraints will be applied.
Same frequency constraint can be applied only to the CPUs belonging to the same cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single 'struct cpufreq_policy' structure managing them and so getting policies for all CPUs wouldn't make any sense as they will all return the same pointer.
So, remove this useless check of checking min/max for all CPUs. Also update doc comment to make this more obvious that clip_cpus should be same as policy->related_cpus.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Sorry if I don't understand cpu-cooling well, haven't done much work on it :(
drivers/thermal/cpu_cooling.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 1ab0018..1193cc4 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -420,6 +420,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = { * __cpufreq_cooling_register - helper function to create cpufreq cooling device * @np: a valid struct device_node to the cooling device device tree node * @clip_cpus: cpumask of cpus where the frequency constraints will happen. + * Normally this should be same as cpufreq policy->related_cpus. * * This interface function registers the cpufreq cooling device with the name * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq @@ -435,25 +436,9 @@ __cpufreq_cooling_register(struct device_node *np, { struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev = NULL; - unsigned int min = 0, max = 0; char dev_name[THERMAL_NAME_LENGTH]; - int ret = 0, i; - struct cpufreq_policy policy; + int ret = 0;
- /* Verify that all the clip cpus have same freq_min, freq_max limit */ - for_each_cpu(i, clip_cpus) { - /* continue if cpufreq policy not found and not return error */ - if (!cpufreq_get_policy(&policy, i)) - continue; - if (min == 0 && max == 0) { - min = policy.cpuinfo.min_freq; - max = policy.cpuinfo.max_freq; - } else { - if (min != policy.cpuinfo.min_freq || - max != policy.cpuinfo.max_freq) - return ERR_PTR(-EINVAL); - } - } cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL); if (!cpufreq_dev)
On Wed, Nov 26, 2014 at 11:23:00AM +0530, Viresh Kumar wrote:
In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency constraints will be applied.
Same frequency constraint can be applied only to the CPUs belonging to the same cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single 'struct cpufreq_policy' structure managing them and so getting policies for all CPUs wouldn't make any sense as they will all return the same pointer.
So, remove this useless check of checking min/max for all CPUs. Also update doc comment to make this more obvious that clip_cpus should be same as policy->related_cpus.
Shall we also review the users of this API then? Simple to double check if they are passing the correct value?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Sorry if I don't understand cpu-cooling well, haven't done much work on it :(
drivers/thermal/cpu_cooling.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 1ab0018..1193cc4 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -420,6 +420,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
- __cpufreq_cooling_register - helper function to create cpufreq cooling device
- @np: a valid struct device_node to the cooling device device tree node
- @clip_cpus: cpumask of cpus where the frequency constraints will happen.
- Normally this should be same as cpufreq policy->related_cpus.
- This interface function registers the cpufreq cooling device with the name
- "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
@@ -435,25 +436,9 @@ __cpufreq_cooling_register(struct device_node *np, { struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev = NULL;
- unsigned int min = 0, max = 0; char dev_name[THERMAL_NAME_LENGTH];
- int ret = 0, i;
- struct cpufreq_policy policy;
- int ret = 0;
- /* Verify that all the clip cpus have same freq_min, freq_max limit */
- for_each_cpu(i, clip_cpus) {
/* continue if cpufreq policy not found and not return error */if (!cpufreq_get_policy(&policy, i))continue;if (min == 0 && max == 0) {min = policy.cpuinfo.min_freq;max = policy.cpuinfo.max_freq;} else {if (min != policy.cpuinfo.min_freq ||max != policy.cpuinfo.max_freq)return ERR_PTR(-EINVAL);}- } cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL); if (!cpufreq_dev)
-- 2.0.3.693.g996b0fd
On 27 November 2014 at 21:05, Eduardo Valentin edubezval@gmail.com wrote:
Shall we also review the users of this API then? Simple to double check if they are passing the correct value?
They weren't :).. Fixed them, patches would follow.
All CPUs present in 'allowed_cpus' share the same 'struct cpufreq_policy' structure and so calling cpufreq_update_policy() for each of them doesn't make sense.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 1193cc4..41f5728 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -272,11 +272,10 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, unsigned long cooling_state) { - unsigned int cpuid, clip_freq; + unsigned int clip_freq; struct cpumask *mask = &cpufreq_device->allowed_cpus; unsigned int cpu = cpumask_any(mask);
- /* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == cooling_state) return 0; @@ -289,10 +288,8 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, cpufreq_device->cpufreq_val = clip_freq; notify_device = cpufreq_device;
- for_each_cpu(cpuid, mask) { - if (is_cpufreq_valid(cpuid)) - cpufreq_update_policy(cpuid); - } + if (is_cpufreq_valid(cpu)) + cpufreq_update_policy(cpu);
notify_device = NOTIFY_INVALID;
Because get_cpu_frequency() has returned a valid frequency, it means that the cpufreq policy is surely valid and so no point checking that again with is_cpufreq_valid(). Get rid of the routine as well as there are no more users.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 41f5728..3740b61 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -98,23 +98,6 @@ static void release_idr(struct idr *idr, int id)
/* Below code defines functions to be used for cpufreq as cooling device */
-/** - * is_cpufreq_valid - function to check frequency transitioning capability. - * @cpu: cpu for which check is needed. - * - * This function will check the current state of the system if - * it is capable of changing the frequency for a given @cpu. - * - * Return: 0 if the system is not currently capable of changing - * the frequency of given cpu. !0 in case the frequency is changeable. - */ -static int is_cpufreq_valid(int cpu) -{ - struct cpufreq_policy policy; - - return !cpufreq_get_policy(&policy, cpu); -} - enum cpufreq_cooling_property { GET_LEVEL, GET_FREQ, @@ -288,8 +271,7 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, cpufreq_device->cpufreq_val = clip_freq; notify_device = cpufreq_device;
- if (is_cpufreq_valid(cpu)) - cpufreq_update_policy(cpu); + cpufreq_update_policy(cpu);
notify_device = NOTIFY_INVALID;
Hello Viresh,
Thanks for providing a proposal.
On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
Hi Rafael/Eduardo,
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence ->usable() callback.
The first patch fixes few formatting issues, so that the third patch doesn't throw any checkpatch warnings. Second one fixes a potential bug in cpufreq-dt driver. Third one introduces ->usable() callback which will be used in the fourth patch.
Last three are fixes for cooling core, which may be applied separately by Eduardo if he wants. Sent them in this series as they were sort of connected with cpufreq in general.
Let me know if it still doesn't work properly.
For the series, the last three patches somehow breaks things. I didn't not investigate the reason now, because, well, I think we should take one thing at a time.
For the patches 1 to 4, I tried then and they do the trick. Now the sequencing is correct between cpufreq-dt and cpu cooling. That means I can also improve the thermal code by accepting the following patches: https://patchwork.kernel.org/patch/5326991/ https://patchwork.kernel.org/patch/5387161/
on top of the four first patches.
Cheers,
Eduardo Valentin
-- viresh
Viresh Kumar (7): cpufreq: Fix formatting issues in 'struct cpufreq_driver' cpufreq-dt: pass 'policy->related_cpus' to of_cpufreq_cooling_register() cpufreq: Introduce ->usable() callback for cpufreq drivers cpufreq-dt: register cooling device from ->usable() callback cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy cpu_cooling: No need to check is_cpufreq_valid()
drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++--------------- drivers/cpufreq/cpufreq.c | 5 ++++ drivers/thermal/cpu_cooling.c | 44 ++++------------------------------- include/linux/cpufreq.h | 54 +++++++++++++++++++++++-------------------- 4 files changed, 70 insertions(+), 84 deletions(-)
-- 2.0.3.693.g996b0fd
On Wednesday, November 26, 2014 01:54:31 PM Eduardo Valentin wrote:
--UugvWAfsgieZRqgk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable
Hello Viresh,
Thanks for providing a proposal.
On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
Hi Rafael/Eduardo, =20 Currently there is no callback for cpufreq drivers which is called once t=
he
policy is ready to be used. There are some requirements where such a call=
back is
required. =20 One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_=
policy'
for CPUs which isn't yet initialed at the time ->init() is called and so =
we face
issues while registering the cooling device. =20 Because we can't register cooling device from ->init(), we need a callbac=
k that
is called after the policy is ready to be used and hence ->usable() callb=
ack.
=20 The first patch fixes few formatting issues, so that the third patch does=
n't
throw any checkpatch warnings. Second one fixes a potential bug in cpufre=
q-dt
driver. Third one introduces ->usable() callback which will be used in the fourth patch. =20 Last three are fixes for cooling core, which may be applied separately by Eduardo if he wants. Sent them in this series as they were sort of connec=
ted
with cpufreq in general. =20 Let me know if it still doesn't work properly.
For the series, the last three patches somehow breaks things. I didn't not investigate the reason now, because, well, I think we should take one thing at a time.
Agreed!
On Wed, Nov 26, 2014 at 01:54:31PM -0400, Eduardo Valentin wrote:
Hello Viresh,
Thanks for providing a proposal.
On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
Hi Rafael/Eduardo,
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence ->usable() callback.
The first patch fixes few formatting issues, so that the third patch doesn't throw any checkpatch warnings. Second one fixes a potential bug in cpufreq-dt driver. Third one introduces ->usable() callback which will be used in the fourth patch.
Last three are fixes for cooling core, which may be applied separately by Eduardo if he wants. Sent them in this series as they were sort of connected with cpufreq in general.
Let me know if it still doesn't work properly.
For the series, the last three patches somehow breaks things. I didn't not investigate the reason now, because, well, I think we should take one thing at a time.
It turns out that I could not reproduce the issue I saw. So, I am assuming that was an issue in my environment.
Can you still post them separately?
For the patches 1 to 4, I tried then and they do the trick. Now the sequencing is correct between cpufreq-dt and cpu cooling. That means I can also improve the thermal code by accepting the following patches: https://patchwork.kernel.org/patch/5326991/ https://patchwork.kernel.org/patch/5387161/
on top of the four first patches.
Cheers,
Eduardo Valentin
-- viresh
Viresh Kumar (7): cpufreq: Fix formatting issues in 'struct cpufreq_driver' cpufreq-dt: pass 'policy->related_cpus' to of_cpufreq_cooling_register() cpufreq: Introduce ->usable() callback for cpufreq drivers cpufreq-dt: register cooling device from ->usable() callback cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy cpu_cooling: No need to check is_cpufreq_valid()
drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++--------------- drivers/cpufreq/cpufreq.c | 5 ++++ drivers/thermal/cpu_cooling.c | 44 ++++------------------------------- include/linux/cpufreq.h | 54 +++++++++++++++++++++++-------------------- 4 files changed, 70 insertions(+), 84 deletions(-)
-- 2.0.3.693.g996b0fd
On 27 November 2014 at 21:03, Eduardo Valentin edubezval@gmail.com wrote:
It turns out that I could not reproduce the issue I saw. So, I am assuming that was an issue in my environment.
Good to hear that :)
Can you still post them separately?
Yeah, I will..
I also have few more patches now and so will send them all together.
Viresh,
On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
Hi Rafael/Eduardo,
Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence ->usable() callback.
The first patch fixes few formatting issues, so that the third patch doesn't throw any checkpatch warnings. Second one fixes a potential bug in cpufreq-dt driver. Third one introduces ->usable() callback which will be used in the fourth patch.
Last three are fixes for cooling core, which may be applied separately by Eduardo if he wants. Sent them in this series as they were sort of connected with cpufreq in general.
Thanks for taking the time to review that code. In fact, it is good to have it under the eyes of someone with cpufreq experience. But, from my smoke testing, looks like they introduce regressions.
Would you mind splitting them from this series and sending a separate trhead? They do not help anyway to the original purpose of this series.
BR, Eduardo Valentin
Let me know if it still doesn't work properly.
-- viresh
Viresh Kumar (7): cpufreq: Fix formatting issues in 'struct cpufreq_driver' cpufreq-dt: pass 'policy->related_cpus' to of_cpufreq_cooling_register() cpufreq: Introduce ->usable() callback for cpufreq drivers cpufreq-dt: register cooling device from ->usable() callback cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy cpu_cooling: No need to check is_cpufreq_valid()
drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++--------------- drivers/cpufreq/cpufreq.c | 5 ++++ drivers/thermal/cpu_cooling.c | 44 ++++------------------------------- include/linux/cpufreq.h | 54 +++++++++++++++++++++++-------------------- 4 files changed, 70 insertions(+), 84 deletions(-)
-- 2.0.3.693.g996b0fd
linaro-kernel@lists.linaro.org
-
Eduardo Valentin -
Rafael J. Wysocki -
Viresh Kumar