[PATCH V2 00/26] thermal: cpu_cooling: Fixes and cleanups

[PATCH V2 00/26] thermal: cpu_cooling: Fixes and cleanups

[Viresh Kumar]

Hi Eduardo,

Here is the second iteration of my cpu_cooling cleanups to make it work/look
better. As discussed earlier I picked one of your patches as mine were dependent
on it. Have done some fixes to your patch as well.

First few are updates to platform drivers to fix mask present in clip_cpus.
Next ones are cleanups of cpu_cooling.c to get things properly organized.

Tested-on: Exynos5250 (Dual ARM Cortex A15).
Rebased-over: v3.18-rc7
Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git
thermal/cpu-cooling-fixes

Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Hongbo Zhang <hongbo.zhang@linaro.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>

V1->V2:
- Dropped: cpu_cooling: Drop useless locking around idr_alloc/idr_remove
- Fixed $Subject of patches to prefix with: "thermal: " before "cpu_cooling:"
- Fixed some stupid mistakes highlighted by Eduardo and Javi.
  - s/ERR_PTR(cpufreq_dev->id)/ERR_PTR(ret)
  - s/sizeof(*cool_dev)/sizeof(*cpufreq_dev)

- Rebased over rc7 and that changed lots of things.
  - list of cooling devices was already present in rc7, so my patch to do the
    same is gone.
  - Other modifications done as well to fit with latest updates.

Eduardo Valentin (1):
  thermal: cpu_cooling: check for the readiness of cpufreq layer

Viresh Kumar (25):
  thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()
  thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()
  thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()
  thermal: cpu_cooling: random comment fixups
  thermal: cpu_cooling: fix doc comment over struct
    cpufreq_cooling_device
  thermal: cpu_cooling: Add comment to clarify relation between cooling
    state and frequency
  thermal: cpu_cooling: Pass variable instead of its type to sizeof()
  thermal: cpu_cooling: no need to set cpufreq_state to zero
  thermal: cpu_cooling: no need to set cpufreq_dev to NULL
  thermal: cpu_cooling: no need to initialze 'ret'
  thermal: cpu_cooling: propagate error returned by idr_alloc()
  thermal: cpu_cooling: Don't match min/max frequencies for all CPUs on
    cooling register
  thermal: cpu_cooling: don't iterate over all allowed_cpus to update
    cpufreq policy
  thermal: cpu_cooling: Don't check is_cpufreq_valid()
  thermal: cpu_cooling: do error handling at the bottom in
    __cpufreq_cooling_register()
  thermal: cpu_cooling: initialize 'cpufreq_val' on registration
  thermal: cpu_cooling: Merge cpufreq_apply_cooling() into
    cpufreq_set_cur_state()
  thermal: cpu_cooling: remove unnecessary wrapper get_cpu_frequency()
  thermal: cpu_cooling: find max level during device registration
  thermal: cpu_cooling: get_property() doesn't need to support GET_MAXL
    anymore
  thermal: cpu_cooling: use cpufreq_dev_list instead of
    cpufreq_dev_count
  thermal: cpu_cooling: Pass 'cpufreq_dev' to get_property()
  thermal: cpu_cooling: Store frequencies in descending order
  thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding
    level/freq
  thermal: cpu_cooling: update copyright tags

 drivers/thermal/cpu_cooling.c                      | 360 ++++++++-------------
 drivers/thermal/db8500_cpufreq_cooling.c           |  10 +-
 drivers/thermal/imx_thermal.c                      |   9 +-
 drivers/thermal/samsung/exynos_thermal_common.c    |  12 +-
 drivers/thermal/samsung/exynos_tmu.c               |   5 +-
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c |   6 -
 6 files changed, 148 insertions(+), 254 deletions(-)

-- 
2.0.3.693.g996b0fd

[PATCH V2 01/26] thermal: cpu_cooling: check for the readiness of cpufreq layer

[Viresh Kumar]

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)) {
-- 
2.0.3.693.g996b0fd

Re: [PATCH V2 01/26] thermal: cpu_cooling: check for the readiness of cpufreq layer

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 7158 bytes --]


On Thu, Dec 04, 2014 at 09:41:43AM +0530, Viresh Kumar wrote:
> 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(-)

Hi all,

Here a simple update about the progress in this patch. This is V4 (not V2)
of this patch. Viresh and I agreed to include it in this series because the
patch is related to the fixes and cleanups in cpu cooling. 

This version, on top of V3, has the error codes in the error messages,
as requested by Russel K. Viresh also insisted in having debug messages,
for debug purposes, when EPROBE_DEFER is used, which I did not oppose.

V3 is here:
https://patchwork.kernel.org/patch/5405201/

> 
> 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)) {
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH V2 02/26] thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd

Re: [PATCH V2 02/26] thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()

[Linus Walleij]

On Thu, Dec 4, 2014 at 5:11 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> 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>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH V2 03/26] thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()

[Viresh Kumar]

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,
-- 
2.0.3.693.g996b0fd

[PATCH V2 04/26] thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()

[Viresh Kumar]

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: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/samsung/exynos_thermal_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c
index 50a1f17..6dc3815 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -347,7 +347,6 @@ void exynos_report_trigger(struct thermal_sensor_conf *conf)
 int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
 {
 	int ret;
-	struct cpumask mask_val;
 	struct exynos_thermal_zone *th_zone;
 
 	if (!sensor_conf || !sensor_conf->read_temperature) {
@@ -367,9 +366,8 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
 	 *	 sensor
 	 */
 	if (sensor_conf->cooling_data.freq_clip_count > 0) {
-		cpumask_set_cpu(0, &mask_val);
 		th_zone->cool_dev[th_zone->cool_dev_size] =
-					cpufreq_cooling_register(&mask_val);
+				cpufreq_cooling_register(cpu_present_mask);
 		if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) {
 			ret = PTR_ERR(th_zone->cool_dev[th_zone->cool_dev_size]);
 			if (ret != -EPROBE_DEFER)
-- 
2.0.3.693.g996b0fd

[PATCH V2 05/26] thermal: cpu_cooling: random comment fixups

[Viresh Kumar]

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
  *
-- 
2.0.3.693.g996b0fd

[PATCH V2 06/26] thermal: cpu_cooling: fix doc comment over struct cpufreq_cooling_device

[Viresh Kumar]

cooling_cpufreq_lock isn't used to protect this structure and so the comment
over it is outdated. Fix it.

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

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 6f2d41e..cc10641 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -40,9 +40,8 @@
  *	frequency.
  * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  *
- * This structure is required for keeping information of each
- * cpufreq_cooling_device registered. In order to prevent corruption of this a
- * mutex lock cooling_cpufreq_lock is used.
+ * This structure is required for keeping information of each registered
+ * cpufreq_cooling_device.
  */
 struct cpufreq_cooling_device {
 	int id;
-- 
2.0.3.693.g996b0fd

Re: [PATCH V2 06/26] thermal: cpu_cooling: fix doc comment over struct cpufreq_cooling_device

[Javi Merino]

On Thu, Dec 04, 2014 at 04:11:48AM +0000, Viresh Kumar wrote:
> cooling_cpufreq_lock isn't used to protect this structure and so the comment
> over it is outdated. Fix it.

Instead of deleting this comment, move it to where it belongs.  From
my understanding, the cooling_cpufreq_lock protects the
cpufreq_dev_list.  Can you move the comment there?

Cheers,
Javi

Re: [PATCH V2 06/26] thermal: cpu_cooling: fix doc comment over struct cpufreq_cooling_device

[Viresh Kumar]

On 4 December 2014 at 16:02, Javi Merino <javi.merino@arm.com> wrote:
> Instead of deleting this comment, move it to where it belongs.  From
> my understanding, the cooling_cpufreq_lock protects the
> cpufreq_dev_list.  Can you move the comment there?

I will add a separate patch to add comment over cooling_cpufreq_lock
to define what all it protects.

Thanks.

[PATCH V2 07/26] thermal: cpu_cooling: Add comment to clarify relation between cooling state and frequency

[Viresh Kumar]

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
-- 
2.0.3.693.g996b0fd

[PATCH V2 08/26] thermal: cpu_cooling: Pass variable instead of its type to sizeof()

[Viresh Kumar]

Just following coding guidelines here.

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

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index a5a9317..5378561 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -476,8 +476,7 @@ __cpufreq_cooling_register(struct device_node *np,
 				return ERR_PTR(-EINVAL);
 		}
 	}
-	cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
-			      GFP_KERNEL);
+	cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);
 	if (!cpufreq_dev)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.0.3.693.g996b0fd

[PATCH V2 09/26] thermal: cpu_cooling: no need to set cpufreq_state to zero

[Viresh Kumar]

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 */
-- 
2.0.3.693.g996b0fd

[PATCH V2 10/26] thermal: cpu_cooling: no need to set cpufreq_dev to NULL

[Viresh Kumar]

It will be overwritten soon with return value of 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 30e2ecb..c144493 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -451,7 +451,7 @@ __cpufreq_cooling_register(struct device_node *np,
 			   const struct cpumask *clip_cpus)
 {
 	struct thermal_cooling_device *cool_dev;
-	struct cpufreq_cooling_device *cpufreq_dev = NULL;
+	struct cpufreq_cooling_device *cpufreq_dev;
 	unsigned int min = 0, max = 0;
 	char dev_name[THERMAL_NAME_LENGTH];
 	int ret = 0, i;
-- 
2.0.3.693.g996b0fd

[PATCH V2 11/26] thermal: cpu_cooling: no need to initialze 'ret'

[Viresh Kumar]

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))) {
-- 
2.0.3.693.g996b0fd

[PATCH V2 12/26] thermal: cpu_cooling: propagate error returned by idr_alloc()

[Viresh Kumar]

We aren't supposed to return our own error type here. Return what we got.

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 d57b8bb..5c9a2ef 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -485,7 +485,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
 	if (ret) {
 		kfree(cpufreq_dev);
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(ret);
 	}
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
-- 
2.0.3.693.g996b0fd

[PATCH V2 13/26] thermal: cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd

[PATCH V2 14/26] thermal: cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy

[Viresh Kumar]

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 f325738..7f27f1b 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -285,11 +285,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;
@@ -301,10 +300,8 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 	cpufreq_device->cpufreq_state = cooling_state;
 	cpufreq_device->cpufreq_val = clip_freq;
 
-	for_each_cpu(cpuid, mask) {
-		if (is_cpufreq_valid(cpuid))
-			cpufreq_update_policy(cpuid);
-	}
+	if (is_cpufreq_valid(cpu))
+		cpufreq_update_policy(cpu);
 
 	return 0;
 }
-- 
2.0.3.693.g996b0fd

[PATCH V2 15/26] thermal: cpu_cooling: Don't check is_cpufreq_valid()

[Viresh Kumar]

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;
 }
-- 
2.0.3.693.g996b0fd

[PATCH V2 16/26] thermal: cpu_cooling: do error handling at the bottom in __cpufreq_cooling_register()

[Viresh Kumar]

This makes life easy and bug free. And is scalable for future resource
allocations.

Acked-by: Javi Merino <javi.merino@arm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 1dd4cc4..491d90a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -448,8 +448,8 @@ __cpufreq_cooling_register(struct device_node *np,
 
 	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
 	if (ret) {
-		kfree(cpufreq_dev);
-		return ERR_PTR(ret);
+		cool_dev = ERR_PTR(ret);
+		goto free_cdev;
 	}
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
@@ -457,11 +457,9 @@ __cpufreq_cooling_register(struct device_node *np,
 
 	cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev,
 						      &cpufreq_cooling_ops);
-	if (IS_ERR(cool_dev)) {
-		release_idr(&cpufreq_idr, cpufreq_dev->id);
-		kfree(cpufreq_dev);
-		return cool_dev;
-	}
+	if (IS_ERR(cool_dev))
+		goto remove_idr;
+
 	cpufreq_dev->cool_dev = cool_dev;
 
 	mutex_lock(&cooling_cpufreq_lock);
@@ -476,6 +474,13 @@ __cpufreq_cooling_register(struct device_node *np,
 	mutex_unlock(&cooling_cpufreq_lock);
 
 	return cool_dev;
+
+remove_idr:
+	release_idr(&cpufreq_idr, cpufreq_dev->id);
+free_cdev:
+	kfree(cpufreq_dev);
+
+	return cool_dev;
 }
 
 /**
-- 
2.0.3.693.g996b0fd

[PATCH V2 17/26] thermal: cpu_cooling: initialize 'cpufreq_val' on registration

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd

[PATCH V2 18/26] thermal: cpu_cooling: Merge cpufreq_apply_cooling() into cpufreq_set_cur_state()

[Viresh Kumar]

cpufreq_apply_cooling() has a single caller, cpufreq_set_cur_state() and
cpufreq_set_cur_state() is an unnecessary wrapper over cpufreq_apply_cooling().

Get rid of it by merging both routines.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 52 +++++++++++++------------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 86bcf8d..a3dd74f 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -254,41 +254,6 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
 }
 
 /**
- * cpufreq_apply_cooling - function to apply frequency clipping.
- * @cpufreq_device: cpufreq_cooling_device pointer containing frequency
- *	clipping data.
- * @cooling_state: value of the cooling state.
- *
- * Function used to make sure the cpufreq layer is aware of current thermal
- * limits. The limits are applied by updating the cpufreq policy.
- *
- * Return: 0 on success, an error code otherwise (-EINVAL in case wrong
- * cooling state).
- */
-static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
-				 unsigned long cooling_state)
-{
-	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;
-
-	clip_freq = get_cpu_frequency(cpu, cooling_state);
-	if (!clip_freq)
-		return -EINVAL;
-
-	cpufreq_device->cpufreq_state = cooling_state;
-	cpufreq_device->cpufreq_val = clip_freq;
-
-	cpufreq_update_policy(cpu);
-
-	return 0;
-}
-
-/**
  * 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.
@@ -391,8 +356,23 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
 				 unsigned long state)
 {
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
+	unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus);
+	unsigned int clip_freq;
+
+	/* Check if the old cooling action is same as new cooling action */
+	if (cpufreq_device->cpufreq_state == state)
+		return 0;
 
-	return cpufreq_apply_cooling(cpufreq_device, state);
+	clip_freq = get_cpu_frequency(cpu, state);
+	if (!clip_freq)
+		return -EINVAL;
+
+	cpufreq_device->cpufreq_state = state;
+	cpufreq_device->cpufreq_val = clip_freq;
+
+	cpufreq_update_policy(cpu);
+
+	return 0;
 }
 
 /* Bind cpufreq callbacks to thermal cooling device ops */
-- 
2.0.3.693.g996b0fd

[PATCH V2 19/26] thermal: cpu_cooling: remove unnecessary wrapper get_cpu_frequency()

[Viresh Kumar]

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;
 	}
 
-- 
2.0.3.693.g996b0fd

[PATCH V2 20/26] thermal: cpu_cooling: find max level during device registration

[Viresh Kumar]

CPU frequency tables don't update after the driver is registered and so we don't
need to iterate over them to find total number of states every time
cpufreq_get_max_state() is called. Do it once at boot time.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 2c4c485..d34cc5b 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -52,6 +52,8 @@
  *	cooling	devices.
  * @cpufreq_val: integer value representing the absolute value of the clipped
  *	frequency.
+ * @max_level: maximum cooling level. One less than total number of valid
+ *	cpufreq frequencies.
  * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  *
  * This structure is required for keeping information of each registered
@@ -62,6 +64,7 @@ struct cpufreq_cooling_device {
 	struct thermal_cooling_device *cool_dev;
 	unsigned int cpufreq_state;
 	unsigned int cpufreq_val;
+	unsigned int max_level;
 	struct cpumask allowed_cpus;
 	struct list_head node;
 };
@@ -283,19 +286,9 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
 				 unsigned long *state)
 {
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
-	struct cpumask *mask = &cpufreq_device->allowed_cpus;
-	unsigned int cpu;
-	unsigned int count = 0;
-	int ret;
-
-	cpu = cpumask_any(mask);
-
-	ret = get_property(cpu, 0, &count, GET_MAXL);
-
-	if (count > 0)
-		*state = count;
 
-	return ret;
+	*state = cpufreq_device->max_level;
+	return 0;
 }
 
 /**
@@ -385,9 +378,11 @@ __cpufreq_cooling_register(struct device_node *np,
 	struct thermal_cooling_device *cool_dev;
 	struct cpufreq_cooling_device *cpufreq_dev;
 	char dev_name[THERMAL_NAME_LENGTH];
+	struct cpufreq_frequency_table *pos, *table;
 	int ret;
 
-	if (!cpufreq_frequency_get_table(cpumask_first(clip_cpus))) {
+	table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
+	if (!table) {
 		pr_debug("%s: CPUFreq table not found\n", __func__);
 		return ERR_PTR(-EPROBE_DEFER);
 	}
@@ -404,6 +399,13 @@ __cpufreq_cooling_register(struct device_node *np,
 		goto free_cdev;
 	}
 
+	/* Find max levels */
+	cpufreq_for_each_valid_entry(pos, table)
+		cpufreq_dev->max_level++;
+
+	/* max_level is an index, not a counter */
+	cpufreq_dev->max_level--;
+
 	cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
 
 	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
-- 
2.0.3.693.g996b0fd

[PATCH V2 21/26] thermal: cpu_cooling: get_property() doesn't need to support GET_MAXL anymore

[Viresh Kumar]

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);
 
-- 
2.0.3.693.g996b0fd

[PATCH V2 22/26] thermal: cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count

[Viresh Kumar]

As we already have a list of cpufreq_cooling_devices now, lets use it instead of
a local counter.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index d2e6f84..32ff6dc 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -71,8 +71,6 @@ struct cpufreq_cooling_device {
 static DEFINE_IDR(cpufreq_idr);
 static DEFINE_MUTEX(cooling_cpufreq_lock);
 
-static unsigned int cpufreq_dev_count;
-
 static LIST_HEAD(cpufreq_dev_list);
 
 /**
@@ -419,10 +417,9 @@ __cpufreq_cooling_register(struct device_node *np,
 	mutex_lock(&cooling_cpufreq_lock);
 
 	/* Register the notifier for first cpufreq cooling device */
-	if (cpufreq_dev_count == 0)
+	if (list_empty(&cpufreq_dev_list))
 		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
 					  CPUFREQ_POLICY_NOTIFIER);
-	cpufreq_dev_count++;
 	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
 
 	mutex_unlock(&cooling_cpufreq_lock);
@@ -495,10 +492,9 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 	cpufreq_dev = cdev->devdata;
 	mutex_lock(&cooling_cpufreq_lock);
 	list_del(&cpufreq_dev->node);
-	cpufreq_dev_count--;
 
 	/* Unregister the notifier for the last cpufreq cooling device */
-	if (cpufreq_dev_count == 0)
+	if (list_empty(&cpufreq_dev_list))
 		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
 					    CPUFREQ_POLICY_NOTIFIER);
 	mutex_unlock(&cooling_cpufreq_lock);
-- 
2.0.3.693.g996b0fd

[PATCH V2 23/26] thermal: cpu_cooling: Pass 'cpufreq_dev' to get_property()

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd

[PATCH V2 24/26] thermal: cpu_cooling: Store frequencies in descending order

[Viresh Kumar]

CPUFreq framework *doesn't* guarantee that frequencies present in cpufreq table
will be in ascending or descending order. But cpu_cooling somehow assumes that.

Probably because most of current users are creating this list from DT, which is
done with the help of OPP layer. And OPP layer creates the list in ascending
order of frequencies.

But cpu_cooling can be used for other platforms too, which don't have
frequencies arranged in any order.

This patch tries to fix this issue by creating another list of valid frequencies
in descending order. Care is also taken to throw warnings for duplicate entries.

Later patches would use it to simplify code.

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

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 7687922..cb5a4b9 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -65,6 +65,7 @@ struct cpufreq_cooling_device {
 	unsigned int cpufreq_state;
 	unsigned int cpufreq_val;
 	unsigned int max_level;
+	unsigned int *freq_table;	/* In descending order */
 	struct cpumask allowed_cpus;
 	struct list_head node;
 };
@@ -352,6 +353,20 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
 	.notifier_call = cpufreq_thermal_notifier,
 };
 
+static unsigned int find_next_max(struct cpufreq_frequency_table *table,
+				  unsigned int prev_max)
+{
+	struct cpufreq_frequency_table *pos;
+	unsigned int max = 0;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		if (pos->frequency > max && pos->frequency < prev_max)
+			max = pos->frequency;
+	}
+
+	return max;
+}
+
 /**
  * __cpufreq_cooling_register - helper function to create cpufreq cooling device
  * @np: a valid struct device_node to the cooling device device tree node
@@ -374,6 +389,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	struct cpufreq_cooling_device *cpufreq_dev;
 	char dev_name[THERMAL_NAME_LENGTH];
 	struct cpufreq_frequency_table *pos, *table;
+	unsigned int freq, i;
 	int ret;
 
 	table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
@@ -397,6 +413,14 @@ __cpufreq_cooling_register(struct device_node *np,
 	cpufreq_for_each_valid_entry(pos, table)
 		cpufreq_dev->max_level++;
 
+	cpufreq_dev->freq_table = kmalloc(sizeof(*cpufreq_dev->freq_table) *
+					  cpufreq_dev->max_level, GFP_KERNEL);
+	if (!cpufreq_dev->freq_table) {
+		return ERR_PTR(-ENOMEM);
+		cool_dev = ERR_PTR(-ENOMEM);
+		goto free_cdev;
+	}
+
 	/* max_level is an index, not a counter */
 	cpufreq_dev->max_level--;
 
@@ -405,7 +429,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
 	if (ret) {
 		cool_dev = ERR_PTR(ret);
-		goto free_cdev;
+		goto free_table;
 	}
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
@@ -416,6 +440,18 @@ __cpufreq_cooling_register(struct device_node *np,
 	if (IS_ERR(cool_dev))
 		goto remove_idr;
 
+	/* Fill freq-table in descending order of frequencies */
+	for (i = 0, freq = -1; i <= cpufreq_dev->max_level; i++) {
+		freq = find_next_max(table, freq);
+		cpufreq_dev->freq_table[i] = freq;
+
+		/* Warn for duplicate entries */
+		if (!freq)
+			pr_warn("%s: table has duplicate entries\n", __func__);
+		else
+			pr_debug("%s: freq:%u KHz\n", __func__, freq);
+	}
+
 	cpufreq_dev->cool_dev = cool_dev;
 
 	mutex_lock(&cooling_cpufreq_lock);
@@ -432,6 +468,8 @@ __cpufreq_cooling_register(struct device_node *np,
 
 remove_idr:
 	release_idr(&cpufreq_idr, cpufreq_dev->id);
+free_table:
+	kfree(cpufreq_dev->freq_table);
 free_cdev:
 	kfree(cpufreq_dev);
 
@@ -505,6 +543,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 
 	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
 	release_idr(&cpufreq_idr, cpufreq_dev->id);
+	kfree(cpufreq_dev->freq_table);
 	kfree(cpufreq_dev);
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
-- 
2.0.3.693.g996b0fd

[PATCH V2 25/26] thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd

Re: [PATCH V2 25/26] thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Javi Merino]

Hi Viresh,

One minor nit

On Thu, Dec 04, 2014 at 04:12:07AM +0000, Viresh Kumar wrote:
> 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.

Should be "Return:", as it was.  See Documentation/kernel-doc-nano-HOWTO.txt

Cheers,
Javi

Re: [PATCH V2 25/26] thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq

[Viresh Kumar]

On 4 December 2014 at 16:22, Javi Merino <javi.merino@arm.com> wrote:
>> - * Return: 0 on success, -EINVAL when invalid parameters are passed.
>> + * Returns: level on success, THERMAL_CSTATE_INVALID on error.
>
> Should be "Return:", as it was.  See Documentation/kernel-doc-nano-HOWTO.txt

I have seen such comment earlier and my bad for not keeping a note of that.
Probably we need a checkpatch warning for this :)

Will resend this one. Thanks for your reviews Javi.

[PATCH V2 26/26] thermal: cpu_cooling: update copyright tags

[Viresh Kumar]

Adding my copyright information for two purposes:
- To get cc'd for future patches to review (Only if people read this header
  while sending mail)
- Have done enough changes to earn a place here?

Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index d97e14d..a975fe2e 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -4,6 +4,8 @@
  *  Copyright (C) 2012	Samsung Electronics Co., Ltd(http://www.samsung.com)
  *  Copyright (C) 2012  Amit Daniel <amit.kachhap@linaro.org>
  *
+ *  Copyright (C) 2014  Viresh Kumar <viresh.kumar@linaro.org>
+ *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
-- 
2.0.3.693.g996b0fd

Re: [PATCH V2 00/26] thermal: cpu_cooling: Fixes and cleanups

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4322 bytes --]

On Thu, Dec 04, 2014 at 09:41:42AM +0530, Viresh Kumar wrote:
> Hi Eduardo,
> 
> Here is the second iteration of my cpu_cooling cleanups to make it work/look
> better. As discussed earlier I picked one of your patches as mine were dependent
> on it. Have done some fixes to your patch as well.
> 
> First few are updates to platform drivers to fix mask present in clip_cpus.
> Next ones are cleanups of cpu_cooling.c to get things properly organized.
> 
> Tested-on: Exynos5250 (Dual ARM Cortex A15).
> Rebased-over: v3.18-rc7
> Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git
> thermal/cpu-cooling-fixes
> 
> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Hongbo Zhang <hongbo.zhang@linaro.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> 
> V1->V2:
> - Dropped: cpu_cooling: Drop useless locking around idr_alloc/idr_remove
> - Fixed $Subject of patches to prefix with: "thermal: " before "cpu_cooling:"
> - Fixed some stupid mistakes highlighted by Eduardo and Javi.
>   - s/ERR_PTR(cpufreq_dev->id)/ERR_PTR(ret)
>   - s/sizeof(*cool_dev)/sizeof(*cpufreq_dev)
> 


The series is good to me. The comments in the list regarding kerneldoc I
ammended myself. I also tested them and could not spot any problems. For
this reason, I will be attempting to send this series in a second round
for 3.19.

However, this series depend on your rework in cpufreq side. Is that
going in for 3.19?

Cheers,

> - Rebased over rc7 and that changed lots of things.
>   - list of cooling devices was already present in rc7, so my patch to do the
>     same is gone.
>   - Other modifications done as well to fit with latest updates.
> 
> Eduardo Valentin (1):
>   thermal: cpu_cooling: check for the readiness of cpufreq layer
> 
> Viresh Kumar (25):
>   thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()
>   thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()
>   thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()
>   thermal: cpu_cooling: random comment fixups
>   thermal: cpu_cooling: fix doc comment over struct
>     cpufreq_cooling_device
>   thermal: cpu_cooling: Add comment to clarify relation between cooling
>     state and frequency
>   thermal: cpu_cooling: Pass variable instead of its type to sizeof()
>   thermal: cpu_cooling: no need to set cpufreq_state to zero
>   thermal: cpu_cooling: no need to set cpufreq_dev to NULL
>   thermal: cpu_cooling: no need to initialze 'ret'
>   thermal: cpu_cooling: propagate error returned by idr_alloc()
>   thermal: cpu_cooling: Don't match min/max frequencies for all CPUs on
>     cooling register
>   thermal: cpu_cooling: don't iterate over all allowed_cpus to update
>     cpufreq policy
>   thermal: cpu_cooling: Don't check is_cpufreq_valid()
>   thermal: cpu_cooling: do error handling at the bottom in
>     __cpufreq_cooling_register()
>   thermal: cpu_cooling: initialize 'cpufreq_val' on registration
>   thermal: cpu_cooling: Merge cpufreq_apply_cooling() into
>     cpufreq_set_cur_state()
>   thermal: cpu_cooling: remove unnecessary wrapper get_cpu_frequency()
>   thermal: cpu_cooling: find max level during device registration
>   thermal: cpu_cooling: get_property() doesn't need to support GET_MAXL
>     anymore
>   thermal: cpu_cooling: use cpufreq_dev_list instead of
>     cpufreq_dev_count
>   thermal: cpu_cooling: Pass 'cpufreq_dev' to get_property()
>   thermal: cpu_cooling: Store frequencies in descending order
>   thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding
>     level/freq
>   thermal: cpu_cooling: update copyright tags
> 
>  drivers/thermal/cpu_cooling.c                      | 360 ++++++++-------------
>  drivers/thermal/db8500_cpufreq_cooling.c           |  10 +-
>  drivers/thermal/imx_thermal.c                      |   9 +-
>  drivers/thermal/samsung/exynos_thermal_common.c    |  12 +-
>  drivers/thermal/samsung/exynos_tmu.c               |   5 +-
>  drivers/thermal/ti-soc-thermal/ti-thermal-common.c |   6 -
>  6 files changed, 148 insertions(+), 254 deletions(-)
> 
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH V2 00/26] thermal: cpu_cooling: Fixes and cleanups

[Viresh Kumar]

On 9 December 2014 at 01:31, Eduardo Valentin <edubezval@gmail.com> wrote:
> The series is good to me. The comments in the list regarding kerneldoc I
> ammended myself. I also tested them and could not spot any problems. For
> this reason, I will be attempting to send this series in a second round
> for 3.19.
>
> However, this series depend on your rework in cpufreq side. Is that
> going in for 3.19?

Yes it will. https://lkml.org/lkml/2014/12/8/704