thermal: cpu_cooling: Fixes and cleanups
Subsystems (5)
v2 cover thermal: cpu_cooling: Fixes and cleanups
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
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(-)
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 1/26 thermal: cpu_cooling: check for the readiness of cpufreq layer
From: Eduardo Valentin <[email protected]>
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.
--- 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(-)
drivers/thermal/cpu_cooling.c +5 -0
@@ -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 */
drivers/thermal/db8500_cpufreq_cooling.c +0 -5
@@ -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);
drivers/thermal/imx_thermal.c +0 -5
@@ -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 ;
drivers/thermal/samsung/exynos_thermal_common.c +5 -3
@@ -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++;
drivers/thermal/samsung/exynos_tmu.c +4 -1
@@ -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;
drivers/thermal/ti-soc-thermal/ti-thermal-common.c +0 -6
@@ -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)) {
Signature · 1 line
2.0.3.693.g996b0fd
Eduardo Valentin
maintainerRe: [PATCH V2 01/26] thermal: cpu_cooling: check for the readiness of cpufreq layer
Quoted 28 lines · Viresh Kumar
From: Eduardo Valentin <[email protected]>
In this patch, the
cpu_coolingcode 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 at vger.kernel.orgCc Grant Likely <[email protected]>Cc Kukjin Kim <[email protected]>Cc linux-arm-kernel at lists.infradead.orgCc linux-kernel at vger.kernel.orgCc linux-pm at vger.kernel.orgCc linux-samsung-soc at vger.kernel.orgCc Naveen Krishna Chatradhi <[email protected]>Cc Rob Herring <[email protected]>Cc Zhang Rui <[email protected]>Acked-by Viresh Kumar <[email protected]>Signed-off-by Viresh Kumar <[email protected]>Signed-off-by Eduardo Valentin <[email protected]>
Quoted 8 lines
--- 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/
Quoted 125 lines
drivers/thermal/cpu_cooling.c +5
-0@@ -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 */ drivers/thermal/db8500_cpufreq_cooling.c +0
-5@@ -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);drivers/thermal/imx_thermal.c +0
-5@@ -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 ;drivers/thermal/samsung/exynos_thermal_common.c +5
-3@@ -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++;drivers/thermal/samsung/exynos_tmu.c +4
-1@@ -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;drivers/thermal/ti-soc-thermal/ti-thermal-common.c +0
-6@@ -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)) {Signature · 1 line
2.0.3.693.g996b0fd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141204/96c5a650/attachment.sig>
PATCH v2 2/26 thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()
In reply to Viresh Kumarcpufreq_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().
--- drivers/thermal/db8500_cpufreq_cooling.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/thermal/db8500_cpufreq_cooling.c +1 -4
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
Re: [PATCH V2 02/26] thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register()
Quoted 7 lines · 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.
Quoted 8 lines
If CPU0 is hoplugged out then we may face strange BUGs as
cpu_coolingframework isn't aware of any siblings sharing clock line.Fix it by passing
cpu_present_masktocpufreq_cooling_register().Cc Hongbo Zhang <[email protected]>Cc Linus Walleij <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Yours,
Linus Walleij
PATCH v2 3/26 thermal: imx: pass cpu_present_mask to cpufreq_cooling_register()
In reply to Viresh Kumarcpufreq_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().
--- drivers/thermal/imx_thermal.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/thermal/imx_thermal.c +1 -3
@@ -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,
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 4/26 thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register()
In reply to Viresh Kumarcpufreq_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().
--- drivers/thermal/samsung/exynos_thermal_common.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/thermal/samsung/exynos_thermal_common.c +1 -3
@@ -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 )
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 5/26 thermal: cpu_cooling: random comment fixups
In reply to Viresh Kumars/give/given
--- drivers/thermal/cpu_cooling.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/thermal/cpu_cooling.c +3 -2
@@ -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 *
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 6/26 thermal: cpu_cooling: fix doc comment over struct cpufreq_cooling_device
In reply to Viresh Kumarcooling_cpufreq_lock isn't used to protect this structure and so the comment over it is outdated. Fix it.
--- drivers/thermal/cpu_cooling.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/thermal/cpu_cooling.c +2 -3
@@ -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;
Signature · 1 line
2.0.3.693.g996b0fd
Re: [PATCH V2 06/26] thermal: cpu_cooling: fix doc comment over struct cpufreq_cooling_device
Quoted 2 lines · Viresh Kumar
cooling_cpufreq_lockisn'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
Quoted 3 lines · Javi Merino
Instead of deleting this comment, move it to where it belongs. From my understanding, the
cooling_cpufreq_lockprotects thecpufreq_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 7/26 thermal: cpu_cooling: Add comment to clarify relation between cooling state and frequency
In reply to Viresh KumarThis wasn't explained well anywhere and should be clearly specified. Lets add a top level comment for this.
--- drivers/thermal/cpu_cooling.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/thermal/cpu_cooling.c +14 -0
@@ -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
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 8/26 thermal: cpu_cooling: Pass variable instead of its type to sizeof()
In reply to Viresh KumarJust following coding guidelines here.
--- drivers/thermal/cpu_cooling.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/thermal/cpu_cooling.c +1 -2
@@ -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 );
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 9/26 thermal: cpu_cooling: no need to set cpufreq_state to zero
In reply to Viresh KumarIts already zero, we allocated cpufreq_dev with kzalloc.
--- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thermal/cpu_cooling.c +1 -1
@@ -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 */
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 10/26 thermal: cpu_cooling: no need to set cpufreq_dev to NULL
In reply to Viresh KumarIt will be overwritten soon with return value of kzalloc().
--- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thermal/cpu_cooling.c +1 -1
@@ -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;
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 11/26 thermal: cpu_cooling: no need to initialze 'ret'
In reply to Viresh Kumar--- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thermal/cpu_cooling.c +1 -1
@@ -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))) {
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 12/26 thermal: cpu_cooling: propagate error returned by idr_alloc()
In reply to Viresh KumarWe aren't supposed to return our own error type here. Return what we got.
--- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thermal/cpu_cooling.c +1 -1
@@ -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",
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 13/26 thermal: cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register
In reply to Viresh KumarIn __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.
--- drivers/thermal/cpu_cooling.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)
drivers/thermal/cpu_cooling.c +2 -17
@@ -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 );
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 14/26 thermal: cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy
In reply to Viresh KumarAll 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.
--- drivers/thermal/cpu_cooling.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/thermal/cpu_cooling.c +3 -6
@@ -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 ; }
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 15/26 thermal: cpu_cooling: Don't check is_cpufreq_valid()
In reply to Viresh KumarBecause 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.
--- drivers/thermal/cpu_cooling.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
drivers/thermal/cpu_cooling.c +1 -19
@@ -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 ; }
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 16/26 thermal: cpu_cooling: do error handling at the bottom in __cpufreq_cooling_register()
In reply to Viresh KumarThis makes life easy and bug free. And is scalable for future resource allocations.
--- drivers/thermal/cpu_cooling.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/thermal/cpu_cooling.c +12 -7
@@ -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; }/**
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 17/26 thermal: cpu_cooling: initialize 'cpufreq_val' on registration
In reply to Viresh KumarThere 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.
--- drivers/thermal/cpu_cooling.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/thermal/cpu_cooling.c +7 -5
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 18/26 thermal: cpu_cooling: Merge cpufreq_apply_cooling() into cpufreq_set_cur_state()
In reply to Viresh Kumarcpufreq_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.
--- drivers/thermal/cpu_cooling.c | 52 +++++++++++++------------------------------ 1 file changed, 16 insertions(+), 36 deletions(-)
drivers/thermal/cpu_cooling.c +16 -36
@@ -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 */
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 19/26 thermal: cpu_cooling: remove unnecessary wrapper get_cpu_frequency()
In reply to Viresh Kumarget_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.
--- drivers/thermal/cpu_cooling.c | 40 +++++++++------------------------------- 1 file changed, 9 insertions(+), 31 deletions(-)
drivers/thermal/cpu_cooling.c +9 -31
@@ -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; }
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 20/26 thermal: cpu_cooling: find max level during device registration
In reply to Viresh KumarCPU 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.
--- drivers/thermal/cpu_cooling.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
drivers/thermal/cpu_cooling.c +15 -13
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 21/26 thermal: cpu_cooling: get_property() doesn't need to support GET_MAXL anymore
In reply to Viresh KumarWe 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().
--- drivers/thermal/cpu_cooling.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/thermal/cpu_cooling.c +3 -11
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 22/26 thermal: cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count
In reply to Viresh KumarAs we already have a list of cpufreq_cooling_devices now, lets use it instead of a local counter.
--- drivers/thermal/cpu_cooling.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/thermal/cpu_cooling.c +2 -6
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 23/26 thermal: cpu_cooling: Pass 'cpufreq_dev' to get_property()
In reply to Viresh KumarWe 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.
--- drivers/thermal/cpu_cooling.c | 50 +++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 23 deletions(-)
drivers/thermal/cpu_cooling.c +27 -23
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 24/26 thermal: cpu_cooling: Store frequencies in descending order
In reply to Viresh KumarCPUFreq 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.
--- drivers/thermal/cpu_cooling.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
drivers/thermal/cpu_cooling.c +40 -1
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
PATCH v2 25/26 thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq
In reply to Viresh Kumarget_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.
--- drivers/thermal/cpu_cooling.c | 108 ++++++++---------------------------------- 1 file changed, 19 insertions(+), 89 deletions(-)
drivers/thermal/cpu_cooling.c +19 -89
@@ -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);
Signature · 1 line
2.0.3.693.g996b0fd
Re: [PATCH V2 25/26] thermal: cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq
Hi Viresh,
One minor nit
Quoted 37 lines · 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 helperget_level()comes in.Signed-off-by Viresh Kumar <[email protected]>--- drivers/thermal/cpu_cooling.c | 108 ++++++++---------------------------------- 1 file changed, 19 insertions(+), 89 deletions(-)drivers/thermal/cpu_cooling.c +1
-13@@ -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
drivers/thermal/cpu_cooling.c static void release_idr(struct idr *idr, int id) · line 120
Quoted 10 lines
drivers/thermal/cpu_cooling.c +2
-7+* @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
Quoted 4 lines · Javi Merino
Quoted 2 lines
- * Return: 0 on success,
-EINVALwhen invalid parameters are passed.
+ * Returns: level on success,THERMAL_CSTATE_INVALIDon 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
In reply to Viresh KumarAdding 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?
--- drivers/thermal/cpu_cooling.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/thermal/cpu_cooling.c +2 -0
@@ -4,6 +4,8 @@* Copyright (C) 2012 Samsung Electronics Co., Ltd(http://www.samsung.com) * Copyright (C) 2012 Amit Daniel <[email protected]> * +* Copyright (C) 2014 Viresh Kumar <[email protected]> +* * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * 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
Signature · 1 line
2.0.3.693.g996b0fd
Eduardo Valentin
maintainerQuoted 21 lines · Viresh Kumar
Hi Eduardo,
Here is the second iteration of my
cpu_coolingcleanups 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 ofcpu_cooling.cto 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-fixesCc Amit Daniel Kachhap <[email protected]>Cc Chanwoo Choi <[email protected]>Cc Hongbo Zhang <[email protected]>Cc Kyungmin Park <[email protected]>Cc Lukasz Majewski <[email protected]>Cc Shawn Guo <[email protected]>
Quoted 7 lines
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,
Quoted 55 lines
- 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 layerViresh Kumar (25):
thermal: db8500: passcpu_present_masktocpufreq_cooling_register()
thermal: imx: passcpu_present_masktocpufreq_cooling_register()
thermal: exynos: passcpu_present_masktocpufreq_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 tosizeof()
thermal:cpu_cooling: no need to setcpufreq_stateto zero
thermal:cpu_cooling: no need to setcpufreq_devto NULL
thermal:cpu_cooling: no need to initialze 'ret'
thermal:cpu_cooling: propagate error returned byidr_alloc()
thermal:cpu_cooling: Don't match min/max frequencies for all CPUs on
cooling register
thermal:cpu_cooling: don't iterate over allallowed_cpusto update
cpufreq policy
thermal:cpu_cooling: Don't checkis_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: Mergecpufreq_apply_cooling()into
cpufreq_set_cur_state()
thermal:cpu_cooling: remove unnecessary wrapperget_cpu_frequency()
thermal:cpu_cooling: find max level during device registration
thermal:cpu_cooling:get_property()doesn't need to supportGET_MAXL
anymore
thermal:cpu_cooling: usecpufreq_dev_listinstead of
cpufreq_dev_count
thermal:cpu_cooling: Pass 'cpufreq_dev' toget_property()
thermal:cpu_cooling: Store frequencies in descending order
thermal:cpu_cooling: Usecpufreq_dev->freq_tablefor finding
level/freq
thermal:cpu_cooling: update copyright tagsdrivers/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(-)Signature · 1 line
2.0.3.693.g996b0fd
Quoted 7 lines · Eduardo Valentin
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