From: Eduardo Valentin edubezval@gmail.com

In this patch, the cpu_cooling code checks for the usability of cpufreq layer before proceeding with the CPU cooling device registration. The main reason is: CPU cooling device is not usable if cpufreq cannot switch frequencies.

Similar checks are spread in thermal drivers. Thus, the advantage now is to have the check in a single place: cpu cooling device registration. For this reason, this patch also updates the existing drivers that depend on CPU cooling to simply propagate the error code of the cpu cooling registration call. Therefore, in case cpufreq is not ready, the thermal drivers will still return -EPROBE_DEFER, in an attempt to try again when cpufreq layer gets ready.

Cc: devicetree@vger.kernel.org Cc: Grant Likely grant.likely@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org Cc: linux-samsung-soc@vger.kernel.org Cc: Naveen Krishna Chatradhi ch.naveen@samsung.com Cc: Rob Herring robh+dt@kernel.org Cc: Zhang Rui rui.zhang@intel.com Acked-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Eduardo Valentin edubezval@gmail.com --- drivers/thermal/cpu_cooling.c | 5 +++++ drivers/thermal/db8500_cpufreq_cooling.c | 5 ----- drivers/thermal/imx_thermal.c | 5 ----- drivers/thermal/samsung/exynos_thermal_common.c | 8 +++++--- drivers/thermal/samsung/exynos_tmu.c | 5 ++++- drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 6 ------ 6 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index ad09e51..f98a763 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -443,6 +443,11 @@ __cpufreq_cooling_register(struct device_node *np, int ret = 0, i; struct cpufreq_policy policy;

+ if (!cpufreq_frequency_get_table(cpumask_first(clip_cpus))) { + pr_debug("%s: CPUFreq table not found\n", __func__); + return ERR_PTR(-EPROBE_DEFER); + } + /* 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 */ diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c index 786d192..1ac7ec6 100644 --- a/drivers/thermal/db8500_cpufreq_cooling.c +++ b/drivers/thermal/db8500_cpufreq_cooling.c @@ -18,7 +18,6 @@ */

#include <linux/cpu_cooling.h> -#include <linux/cpufreq.h> #include <linux/err.h> #include <linux/module.h> #include <linux/of.h> @@ -30,10 +29,6 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev) struct thermal_cooling_device *cdev; struct cpumask mask_val;

- /* make sure cpufreq driver has been initialized */ - if (!cpufreq_frequency_get_table(0)) - return -EPROBE_DEFER; - cpumask_set_cpu(0, &mask_val); cdev = cpufreq_cooling_register(&mask_val);

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 5a1f107..16405b4 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -9,7 +9,6 @@

#include <linux/clk.h> #include <linux/cpu_cooling.h> -#include <linux/cpufreq.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/init.h> @@ -459,10 +458,6 @@ static int imx_thermal_probe(struct platform_device *pdev) int measure_freq; int ret;

- if (!cpufreq_get_current_driver()) { - dev_dbg(&pdev->dev, "no cpufreq driver!"); - return -EPROBE_DEFER; - } data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c index b6be572..50a1f17 100644 --- a/drivers/thermal/samsung/exynos_thermal_common.c +++ b/drivers/thermal/samsung/exynos_thermal_common.c @@ -371,9 +371,11 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) th_zone->cool_dev[th_zone->cool_dev_size] = cpufreq_cooling_register(&mask_val); if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) { - dev_err(sensor_conf->dev, - "Failed to register cpufreq cooling device\n"); - ret = -EINVAL; + ret = PTR_ERR(th_zone->cool_dev[th_zone->cool_dev_size]); + if (ret != -EPROBE_DEFER) + dev_err(sensor_conf->dev, + "Failed to register cpufreq cooling device: %d\n", + ret); goto err_unregister; } th_zone->cool_dev_size++; diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index 49c0924..2afca9b 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -683,7 +683,10 @@ static int exynos_tmu_probe(struct platform_device *pdev) /* Register the sensor with thermal management interface */ ret = exynos_register_thermal(sensor_conf); if (ret) { - dev_err(&pdev->dev, "Failed to register thermal interface\n"); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, + "Failed to register thermal interface: %d\n", + ret); goto err_clk; } data->reg_conf = sensor_conf; diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c index 9eec26d..5f07d7e 100644 --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c @@ -28,7 +28,6 @@ #include <linux/kernel.h> #include <linux/workqueue.h> #include <linux/thermal.h> -#include <linux/cpufreq.h> #include <linux/cpumask.h> #include <linux/cpu_cooling.h> #include <linux/of.h> @@ -403,11 +402,6 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) if (!data) return -EINVAL;

- if (!cpufreq_get_current_driver()) { - dev_dbg(bgp->dev, "no cpufreq driver yet\n"); - return -EPROBE_DEFER; - } - /* Register cooling device */ data->cool_dev = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(data->cool_dev)) {

cpufreq_cooling_register() expects mask of all the CPUs where frequency constraint is applicable.

This platform has more than one CPU to which these constraints will apply and so passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a single cluster of CPUs and the constraint applies to all CPUs.

If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework isn't aware of any siblings sharing clock line.

Fix it by passing cpu_present_mask to cpufreq_cooling_register().

Cc: Hongbo Zhang hongbo.zhang@linaro.com Cc: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/db8500_cpufreq_cooling.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c index 1ac7ec6..3cc3dd9 100644 --- a/drivers/thermal/db8500_cpufreq_cooling.c +++ b/drivers/thermal/db8500_cpufreq_cooling.c @@ -27,11 +27,8 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev) { struct thermal_cooling_device *cdev; - struct cpumask mask_val; - - cpumask_set_cpu(0, &mask_val); - cdev = cpufreq_cooling_register(&mask_val);

+ cdev = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(cdev)) { dev_err(&pdev->dev, "Failed to register cooling device\n"); return PTR_ERR(cdev);

cpufreq_cooling_register() expects mask of all the CPUs where frequency constraint is applicable.

This platform has more than one CPU to which these constraints will apply and so passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a single cluster of CPUs and the constraint applies to all CPUs.

If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework isn't aware of any siblings sharing clock line.

Fix it by passing cpu_present_mask to cpufreq_cooling_register().

Cc: Shawn Guo shawn.guo@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/imx_thermal.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 16405b4..d80e36eb 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -453,7 +453,6 @@ static int imx_thermal_probe(struct platform_device *pdev) const struct of_device_id *of_id = of_match_device(of_imx_thermal_match, &pdev->dev); struct imx_thermal_data *data; - struct cpumask clip_cpus; struct regmap *map; int measure_freq; int ret; @@ -511,8 +510,7 @@ static int imx_thermal_probe(struct platform_device *pdev) regmap_write(map, MISC0 + REG_SET, MISC0_REFTOP_SELBIASOFF); regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);

- cpumask_set_cpu(0, &clip_cpus); - data->cdev = cpufreq_cooling_register(&clip_cpus); + data->cdev = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(data->cdev)) { ret = PTR_ERR(data->cdev); dev_err(&pdev->dev,

s/give/given

Acked-by: Eduardo Valentin edubezval@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index f98a763..6f2d41e 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -121,7 +121,7 @@ enum cpufreq_cooling_property { };

/** - * get_property - fetch a property of interest for a give cpu. + * get_property - fetch a property of interest for a given cpu. * @cpu: cpu for which the property is required * @input: query parameter * @output: query return @@ -131,6 +131,7 @@ enum cpufreq_cooling_property { * 1. get maximum cpu cooling states * 2. translate frequency to cooling state * 3. translate cooling state to frequency + * * Note that the code may be not in good shape * but it is written in this way in order to: * a) reduce duplicate code as most of the code can be shared. @@ -211,7 +212,7 @@ static int get_property(unsigned int cpu, unsigned long input, }

/** - * cpufreq_cooling_get_level - for a give cpu, return the cooling level. + * cpufreq_cooling_get_level - for a given cpu, return the cooling level. * @cpu: cpu for which the level is required * @freq: the frequency of interest *

This wasn't explained well anywhere and should be clearly specified. Lets add a top level comment for this.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index cc10641..a5a9317 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -28,6 +28,20 @@ #include <linux/cpu.h> #include <linux/cpu_cooling.h>

+/* + * Cooling state <-> CPUFreq frequency + * + * Cooling states are translated to frequencies throughout this driver and this + * is the relation between them. + * + * Highest cooling state corresponds to lowest possible frequency. + * + * i.e. + * level 0 --> 1st Max Freq + * level 1 --> 2nd Max Freq + * ... + */ + /** * struct cpufreq_cooling_device - data for cooling device with cpufreq * @id: unique integer value corresponding to each cpufreq_cooling_device

Its already zero, we allocated cpufreq_dev with kzalloc.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 5378561..30e2ecb 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -499,7 +499,7 @@ __cpufreq_cooling_register(struct device_node *np, return cool_dev; } cpufreq_dev->cool_dev = cool_dev; - cpufreq_dev->cpufreq_state = 0; + mutex_lock(&cooling_cpufreq_lock);

/* Register the notifier for first cpufreq cooling device */

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index c144493..d57b8bb 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -454,7 +454,7 @@ __cpufreq_cooling_register(struct device_node *np, struct cpufreq_cooling_device *cpufreq_dev; unsigned int min = 0, max = 0; char dev_name[THERMAL_NAME_LENGTH]; - int ret = 0, i; + int ret, i; struct cpufreq_policy policy;

if (!cpufreq_frequency_get_table(cpumask_first(clip_cpus))) {

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 --- 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 5c9a2ef..f325738 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -437,6 +437,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 @@ -452,30 +453,14 @@ __cpufreq_cooling_register(struct device_node *np, { struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; - unsigned int min = 0, max = 0; char dev_name[THERMAL_NAME_LENGTH]; - int ret, i; - struct cpufreq_policy policy; + int ret;

if (!cpufreq_frequency_get_table(cpumask_first(clip_cpus))) { pr_debug("%s: CPUFreq table not found\n", __func__); return ERR_PTR(-EPROBE_DEFER); }

- /* 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(*cpufreq_dev), GFP_KERNEL); if (!cpufreq_dev) return ERR_PTR(-ENOMEM);

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 7f27f1b..1dd4cc4 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -110,23 +110,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, @@ -300,8 +283,7 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, cpufreq_device->cpufreq_state = cooling_state; cpufreq_device->cpufreq_val = clip_freq;

- if (is_cpufreq_valid(cpu)) - cpufreq_update_policy(cpu); + cpufreq_update_policy(cpu);

return 0; }

There is no point checking for validity of 'cpufreq_val' from cpufreq_thermal_notifier() every time the routine is called. Its guaranteed to be 0 on the first call but will be valid otherwise.

Lets update it once while the device registers.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 491d90a..86bcf8d 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -316,11 +316,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, &cpufreq_dev->allowed_cpus)) continue;

- if (!cpufreq_dev->cpufreq_val) - cpufreq_dev->cpufreq_val = get_cpu_frequency( - cpumask_any(&cpufreq_dev->allowed_cpus), - cpufreq_dev->cpufreq_state); - max_freq = cpufreq_dev->cpufreq_val;

if (policy->max != max_freq) @@ -444,6 +439,13 @@ __cpufreq_cooling_register(struct device_node *np, if (!cpufreq_dev) return ERR_PTR(-ENOMEM);

+ cpufreq_dev->cpufreq_val = get_cpu_frequency(cpumask_any(clip_cpus), 0); + if (!cpufreq_dev->cpufreq_val) { + pr_err("%s: Failed to get frequency", __func__); + cool_dev = ERR_PTR(-EINVAL); + goto free_cdev; + } + cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);

ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);

get_cpu_frequency() isn't doing much by itself, just calling get_property(). And so this wrapper isn't required at all. Get rid of it.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 40 +++++++++------------------------------- 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index a3dd74f..2c4c485 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -230,30 +230,6 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) EXPORT_SYMBOL_GPL(cpufreq_cooling_get_level);

/** - * get_cpu_frequency - get the absolute value of frequency from level. - * @cpu: cpu for which frequency is fetched. - * @level: cooling level - * - * This function matches cooling level with frequency. Based on a cooling level - * of frequency, equals cooling state of cpu cooling device, it will return - * the corresponding frequency. - * e.g level=0 --> 1st MAX FREQ, level=1 ---> 2nd MAX FREQ, .... etc - * - * Return: 0 on error, the corresponding frequency otherwise. - */ -static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) -{ - int ret = 0; - unsigned int freq; - - ret = get_property(cpu, level, &freq, GET_FREQ); - if (ret) - return 0; - - return freq; -} - -/** * cpufreq_thermal_notifier - notifier callback for cpufreq policy change. * @nb: struct notifier_block * with callback info. * @event: value showing cpufreq event for which this function invoked. @@ -358,14 +334,15 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq; + int ret;

/* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;

- clip_freq = get_cpu_frequency(cpu, state); - if (!clip_freq) - return -EINVAL; + ret = get_property(cpu, state, &clip_freq, GET_FREQ); + if (ret) + return ret;

cpufreq_device->cpufreq_state = state; cpufreq_device->cpufreq_val = clip_freq; @@ -419,10 +396,11 @@ __cpufreq_cooling_register(struct device_node *np, if (!cpufreq_dev) return ERR_PTR(-ENOMEM);

- cpufreq_dev->cpufreq_val = get_cpu_frequency(cpumask_any(clip_cpus), 0); - if (!cpufreq_dev->cpufreq_val) { - pr_err("%s: Failed to get frequency", __func__); - cool_dev = ERR_PTR(-EINVAL); + ret = get_property(cpumask_any(clip_cpus), 0, &cpufreq_dev->cpufreq_val, + GET_FREQ); + if (ret) { + pr_err("%s: Failed to get frequency: %d", __func__, ret); + cool_dev = ERR_PTR(ret); goto free_cdev; }

We don't use get_property() to find max levels anymore as it is done at boot now. So, don't support GET_MAXL in get_property().

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index d34cc5b..d2e6f84 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -116,7 +116,6 @@ static void release_idr(struct idr *idr, int id) enum cpufreq_cooling_property { GET_LEVEL, GET_FREQ, - GET_MAXL, };

/** @@ -124,12 +123,11 @@ enum cpufreq_cooling_property { * @cpu: cpu for which the property is required * @input: query parameter * @output: query return - * @property: type of query (frequency, level, max level) + * @property: type of query (frequency, level) * * This is the common function to - * 1. get maximum cpu cooling states - * 2. translate frequency to cooling state - * 3. translate cooling state to frequency + * 1. translate frequency to cooling state + * 2. translate cooling state to frequency * * Note that the code may be not in good shape * but it is written in this way in order to: @@ -176,12 +174,6 @@ static int get_property(unsigned int cpu, unsigned long input, /* max_level is an index, not a counter */ max_level--;

- /* get max level */ - if (property == GET_MAXL) { - *output = (unsigned int)max_level; - return 0; - } - if (property == GET_FREQ) level = descend ? input : (max_level - input);

We already know the value of 'cpufreq_dev->max_level' and so there is no need calculating that once again. For this, we need to send 'cpufreq_dev' to get_property().

Make all necessary changes for this change. Because cpufreq_cooling_get_level() doesn't have access to 'cpufreq_dev', it is updated to iterate over the list of cpufreq_cooling_devices to get cooling device for the cpu number passed to it. This also makes it robust to return levels only for the CPU registered via a cooling device. We don't have to support anything that isn't registered yet.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 50 +++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 32ff6dc..7687922 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -118,7 +118,7 @@ enum cpufreq_cooling_property {

/** * get_property - fetch a property of interest for a given cpu. - * @cpu: cpu for which the property is required + * @cpufreq_dev: cpufreq_dev for which the property is required * @input: query parameter * @output: query return * @property: type of query (frequency, level) @@ -135,20 +135,20 @@ enum cpufreq_cooling_property { * * Return: 0 on success, -EINVAL when invalid parameters are passed. */ -static int get_property(unsigned int cpu, unsigned long input, - unsigned int *output, +static int get_property(struct cpufreq_cooling_device *cpufreq_dev, + unsigned long input, unsigned int *output, enum cpufreq_cooling_property property) { int i; - unsigned long max_level = 0, level = 0; + unsigned long level = 0; unsigned int freq = CPUFREQ_ENTRY_INVALID; int descend = -1; - struct cpufreq_frequency_table *pos, *table = - cpufreq_frequency_get_table(cpu); + struct cpufreq_frequency_table *pos, *table;

if (!output) return -EINVAL;

+ table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus)); if (!table) return -EINVAL;

@@ -162,18 +162,10 @@ static int get_property(unsigned int cpu, unsigned long input, descend = freq > pos->frequency;

freq = pos->frequency; - max_level++; }

- /* No valid cpu frequency entry */ - if (max_level == 0) - return -EINVAL; - - /* max_level is an index, not a counter */ - max_level--; - if (property == GET_FREQ) - level = descend ? input : (max_level - input); + level = descend ? input : (cpufreq_dev->max_level - input);

i = 0; cpufreq_for_each_valid_entry(pos, table) { @@ -186,7 +178,7 @@ static int get_property(unsigned int cpu, unsigned long input,

if (property == GET_LEVEL && (unsigned int)input == freq) { /* get level by frequency */ - *output = descend ? i : (max_level - i); + *output = descend ? i : (cpufreq_dev->max_level - i); return 0; } if (property == GET_FREQ && level == i) { @@ -213,12 +205,25 @@ static int get_property(unsigned int cpu, unsigned long input, */ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) { - unsigned int val; + struct cpufreq_cooling_device *cpufreq_dev;

- if (get_property(cpu, (unsigned long)freq, &val, GET_LEVEL)) - return THERMAL_CSTATE_INVALID; + mutex_lock(&cooling_cpufreq_lock); + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { + if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) { + unsigned int val; + + mutex_unlock(&cooling_cpufreq_lock); + if (get_property(cpufreq_dev, (unsigned long)freq, &val, + GET_LEVEL)) + return THERMAL_CSTATE_INVALID; + + return (unsigned long)val; + } + } + mutex_unlock(&cooling_cpufreq_lock);

- return (unsigned long)val; + pr_err("%s: cpu:%d not part of any cooling device\n", __func__, cpu); + return THERMAL_CSTATE_INVALID; } EXPORT_SYMBOL_GPL(cpufreq_cooling_get_level);

@@ -323,7 +328,7 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, if (cpufreq_device->cpufreq_state == state) return 0;

- ret = get_property(cpu, state, &clip_freq, GET_FREQ); + ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ); if (ret) return ret;

@@ -381,8 +386,7 @@ __cpufreq_cooling_register(struct device_node *np, if (!cpufreq_dev) return ERR_PTR(-ENOMEM);

- ret = get_property(cpumask_any(clip_cpus), 0, &cpufreq_dev->cpufreq_val, - GET_FREQ); + ret = get_property(cpufreq_dev, 0, &cpufreq_dev->cpufreq_val, GET_FREQ); if (ret) { pr_err("%s: Failed to get frequency: %d", __func__, ret); cool_dev = ERR_PTR(ret);

get_property() was an over complicated beast with BUGs. It used to believe that cpufreq table is present in ascending or descending order, which might not always be true.

Previous patch has created another freq table in descending order for us and we better use it now. With that get_property() simply goes away and another helper get_level() comes in.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 108 ++++++++---------------------------------- 1 file changed, 19 insertions(+), 89 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index cb5a4b9..d97e14d 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -112,85 +112,27 @@ static void release_idr(struct idr *idr, int id)

/* Below code defines functions to be used for cpufreq as cooling device */

-enum cpufreq_cooling_property { - GET_LEVEL, - GET_FREQ, -}; - /** - * get_property - fetch a property of interest for a given cpu. + * get_level: Find the level for a particular frequency * @cpufreq_dev: cpufreq_dev for which the property is required - * @input: query parameter - * @output: query return - * @property: type of query (frequency, level) - * - * This is the common function to - * 1. translate frequency to cooling state - * 2. translate cooling state to frequency + * @freq: Frequency * - * Note that the code may be not in good shape - * but it is written in this way in order to: - * a) reduce duplicate code as most of the code can be shared. - * b) make sure the logic is consistent when translating between - * cooling states and frequencies. - * - * Return: 0 on success, -EINVAL when invalid parameters are passed. + * Returns: level on success, THERMAL_CSTATE_INVALID on error. */ -static int get_property(struct cpufreq_cooling_device *cpufreq_dev, - unsigned long input, unsigned int *output, - enum cpufreq_cooling_property property) +static unsigned long get_level(struct cpufreq_cooling_device *cpufreq_dev, + unsigned int freq) { - int i; - unsigned long level = 0; - unsigned int freq = CPUFREQ_ENTRY_INVALID; - int descend = -1; - struct cpufreq_frequency_table *pos, *table; - - if (!output) - return -EINVAL; - - table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus)); - if (!table) - return -EINVAL; - - cpufreq_for_each_valid_entry(pos, table) { - /* ignore duplicate entry */ - if (freq == pos->frequency) - continue; - - /* get the frequency order */ - if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) - descend = freq > pos->frequency; - - freq = pos->frequency; - } - - if (property == GET_FREQ) - level = descend ? input : (cpufreq_dev->max_level - input); - - i = 0; - cpufreq_for_each_valid_entry(pos, table) { - /* ignore duplicate entry */ - if (freq == pos->frequency) - continue; + unsigned long level;

- /* now we have a valid frequency entry */ - freq = pos->frequency; + for (level = 0; level <= cpufreq_dev->max_level; level++) { + if (freq == cpufreq_dev->freq_table[level]) + return level;

- if (property == GET_LEVEL && (unsigned int)input == freq) { - /* get level by frequency */ - *output = descend ? i : (cpufreq_dev->max_level - i); - return 0; - } - if (property == GET_FREQ && level == i) { - /* get frequency by level */ - *output = freq; - return 0; - } - i++; + if (freq > cpufreq_dev->freq_table[level]) + break; }

- return -EINVAL; + return THERMAL_CSTATE_INVALID; }

/** @@ -211,14 +153,8 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) mutex_lock(&cooling_cpufreq_lock); list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) { - unsigned int val; - mutex_unlock(&cooling_cpufreq_lock); - if (get_property(cpufreq_dev, (unsigned long)freq, &val, - GET_LEVEL)) - return THERMAL_CSTATE_INVALID; - - return (unsigned long)val; + return get_level(cpufreq_dev, freq); } } mutex_unlock(&cooling_cpufreq_lock); @@ -323,16 +259,16 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq; - int ret; + + /* Request state should be less than max_level */ + if (WARN_ON(state > cpufreq_device->max_level)) + return -EINVAL;

/* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;

- ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ); - if (ret) - return ret; - + clip_freq = cpufreq_device->freq_table[state]; cpufreq_device->cpufreq_state = state; cpufreq_device->cpufreq_val = clip_freq;

@@ -402,13 +338,6 @@ __cpufreq_cooling_register(struct device_node *np, if (!cpufreq_dev) return ERR_PTR(-ENOMEM);

- ret = get_property(cpufreq_dev, 0, &cpufreq_dev->cpufreq_val, GET_FREQ); - if (ret) { - pr_err("%s: Failed to get frequency: %d", __func__, ret); - cool_dev = ERR_PTR(ret); - goto free_cdev; - } - /* Find max levels */ cpufreq_for_each_valid_entry(pos, table) cpufreq_dev->max_level++; @@ -452,6 +381,7 @@ __cpufreq_cooling_register(struct device_node *np, pr_debug("%s: freq:%u KHz\n", __func__, freq); }

+ cpufreq_dev->cpufreq_val = cpufreq_dev->freq_table[0]; cpufreq_dev->cool_dev = cool_dev;

mutex_lock(&cooling_cpufreq_lock);