dt: thermal: Fix broken cooling-maps

Allow cooling devices sharing same trip point with same contribution value to share the cooling map as well. Otherwise the same information will be duplicated for each device sharing the trip point.

Signed-off-by Viresh Kumar <[email protected]>

---
 Documentation/devicetree/bindings/thermal/thermal.txt | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Documentation/devicetree/bindings/thermal/thermal.txt +2 -2

index cc553f0952c5..eb7ee91556a5 100644
@@ -97,8 +97,8 @@ get assigned to trip points of the zone. The cooling devices are expected
 to be loaded in the target system.
 
 Required properties:
-- cooling-device:	A phandle of a cooling device with its specifier,
-  Type: phandle +	referring to which cooling device is used in this
+- cooling-device:	A list of phandles of cooling devices with their specifiers,
+  Type: phandle +	referring to which cooling devices are used in this
     cooling specifier	binding. In the cooling specifier, the first cell
 			is the minimum cooling state and the second cell
 			is the maximum cooling state used in this map.

@@ -276,12 +276,7 @@ thermal-zones {
 			};
 			map1 {
 				trip = <&cpu_alert1>;

Allow cooling devices sharing same trip point with same contribution value to share the cooling map as well. Otherwise the same information will be duplicated for each device sharing the trip point.

Signed-off-by Viresh Kumar <[email protected]>

---
 Documentation/devicetree/bindings/thermal/thermal.txt | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Each CPU can (and does) participate in cooling down the system but the DT only captures the CPU0 in the cooling maps. Things work by chance as under normal circumstances its the CPU0 which is used by the operating systems to probe the cooling devices. But as soon as that ordering changes and any other CPU is used to bring up the cooling device, we will start seeing errors.

On the other hand, the hardware is partially defined in DT in these cases and we must do a better job by capturing all devices.

Add all devices (CPUs here) in the cooling maps which are also affected by the trip point.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

arch/arm64/boot/dts/hisilicon/hi6220.dtsi +8 -1

index 247024df714f..919d36b91bf3 100644
@@ -886,7 +886,14 @@
 				cooling-maps {
 					map0 {
 						trip = <&target>;
-						cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+						cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+								 <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
 					};
 				};
 			};

A cooling map entry may now contain a list of phandles and their arguments representing multiple devices which share the trip point.

This patch updates the thermal OF core to parse them properly.

Tested on Hikey960.

Signed-off-by Viresh Kumar <[email protected]>

---
This is a follow up patch to the DT bindings patchset:

https://lkml.kernel.org/r/[email protected]

 drivers/thermal/of-thermal.c | 140 +++++++++++++++++++++++++----------
 1 file changed, 102 insertions(+), 38 deletions(-)

drivers/thermal/of-thermal.c +6 -6

index 977a8307fbb1..79c06c4c861b 100644
@@ -19,22 +19,33 @@
 /***   Private data structures to represent thermal device tree data ***/
 
 /**
- * struct __thermal_bind_param - a match between trip and cooling device
+ * struct __thermal_cooling_bind_param - a cooling device for a trip point
  * @cooling_device: a pointer to identify the referred cooling device
- * @trip_id: the trip point index
- * @usage: the percentage (from 0 to 100) of cooling contribution
  * @min: minimum cooling state used at this trip point
  * @max: maximum cooling state used at this trip point
  */
 
-struct __thermal_bind_params {
+struct __thermal_cooling_bind_param {
 	struct device_node *cooling_device;
-	unsigned int trip_id;
-	unsigned int usage;
 	unsigned long min;
 	unsigned long max;
 };
 
+/**
+ * struct __thermal_bind_param - a match between trip and cooling device
+ * @tcbp: a pointer to an array of cooling devices
+ * @count: number of elements in array

drivers/thermal/of-thermal.c +32 -18

+
 /**
  * struct __thermal_zone - internal representation of a thermal zone
  * @mode: current thermal zone device mode (enabled/disabled)

@@ -192,25 +203,31 @@ static int of_thermal_bind(struct thermal_zone_device *thermal,
 			   struct thermal_cooling_device *cdev)
 {
 	struct __thermal_zone *data = thermal->devdata;
-	int i;
+	struct __thermal_bind_params *tbp;
+	struct __thermal_cooling_bind_param *tcbp;
+	int i, j;
 
 	if (!data || IS_ERR(data))
 		return -ENODEV;
 
 	/* find where to bind */
 	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
+		tbp = data->tbps + i;
 
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
+		for (j = 0; j < tbp->count; j++) {
+			tcbp = tbp->tcbp + j;
 
-			ret = thermal_zone_bind_cooling_device(thermal,
+			if (tcbp->cooling_device == cdev->np) {
+				int ret;
+
+				ret = thermal_zone_bind_cooling_device(thermal,
 						tbp->trip_id, cdev,
-						tbp->max,
-						tbp->min,
+						tcbp->max,
+						tcbp->min,
 						tbp->usage);
-			if (ret)
-				return ret;
+				if (ret)
+					return ret;
+			}
 		}
 	}
 

@@ -221,22 +238,28 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal,
 			     struct thermal_cooling_device *cdev)
 {
 	struct __thermal_zone *data = thermal->devdata;
-	int i;
+	struct __thermal_bind_params *tbp;
+	struct __thermal_cooling_bind_param *tcbp;
+	int i, j;
 
 	if (!data || IS_ERR(data))
 		return -ENODEV;
 
 	/* find where to unbind */
 	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
+		tbp = data->tbps + i;
+
+		for (j = 0; j < tbp->count; j++) {
+			tcbp = tbp->tcbp + j;
 
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
+			if (tcbp->cooling_device == cdev->np) {
+				int ret;
 
-			ret = thermal_zone_unbind_cooling_device(thermal,
-						tbp->trip_id, cdev);
-			if (ret)
-				return ret;
+				ret = thermal_zone_unbind_cooling_device(thermal,
+							tbp->trip_id, cdev);
+				if (ret)
+					return ret;
+			}
 		}
 	}
 

@@ -652,8 +675,9 @@ static int thermal_of_populate_bind_params(struct device_node *np,
 					   int ntrips)
 {
 	struct of_phandle_args cooling_spec;
+	struct __thermal_cooling_bind_param *__tcbp;
 	struct device_node *trip;
-	int ret, i;
+	int ret, i, count;
 	u32 prop;
 
 	/* Default weight. Usage is optional */

drivers/thermal/of-thermal.c +0 -6

 		goto end;
 	}
-	__tbp->cooling_device = cooling_spec.np;
-	if (cooling_spec.args_count >= 2) { /* at least min and max */
-		__tbp->min = cooling_spec.args[0];
-		__tbp->max = cooling_spec.args[1];
-	} else {
-		pr_err("wrong reference to cooling device, missing limits\n");

drivers/thermal/of-thermal.c +41 -5

+			goto free_tcbp;
+		}
+
+		__tcbp[i].cooling_device = cooling_spec.np;
+
+		if (cooling_spec.args_count >= 2) { /* at least min and max */
+			__tcbp[i].min = cooling_spec.args[0];
+			__tcbp[i].max = cooling_spec.args[1];
+		} else {
+			pr_err("wrong reference to cooling device, missing limits\n");
+		}
 	}
 
+	__tbp->tcbp= __tcbp;
+	__tbp->count = count;
+
+	goto end;
+
+free_tcbp:
+	for (i = i - 1; i >= 0; i--)
+		of_node_put(__tcbp[i].cooling_device);
+	kfree(__tcbp);
 end:
 	of_node_put(trip);
 

@@ -900,8 +948,16 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 	return tz;
 
 free_tbps:
-	for (i = i - 1; i >= 0; i--)
-		of_node_put(tz->tbps[i].cooling_device);
+	for (i = i - 1; i >= 0; i--) {
+		struct __thermal_bind_params *tbp = tz->tbps + i;
+		int j;
+
+		for (j = 0; j < tbp->count; j++)
+			of_node_put(tbp->tcbp[j].cooling_device);
+
+		kfree(tbp->tcbp);
+	}
+
 	kfree(tz->tbps);
 free_trips:
 	for (i = 0; i < tz->ntrips; i++)

@@ -917,10 +973,18 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 
 static inline void of_thermal_free_zone(struct __thermal_zone *tz)
 {
-	int i;
+	struct __thermal_bind_params *tbp;
+	int i, j;
+
+	for (i = 0; i < tz->num_tbps; i++) {
+		tbp = tz->tbps + i;
+
+		for (j = 0; j < tbp->count; j++)
+			of_node_put(tbp->tcbp[j].cooling_device);
+
+		kfree(tbp->tcbp);
+	}
 
-	for (i = 0; i < tz->num_tbps; i++)
-		of_node_put(tz->tbps[i].cooling_device);
 	kfree(tz->tbps);
 	for (i = 0; i < tz->ntrips; i++)
 		of_node_put(tz->trips[i].np);

Signature · 1 line

2.18.0.rc1.242.g61856ae69a2c

A cooling map entry may now contain a list of phandles and their arguments representing multiple devices which share the trip point.

This patch updates the thermal OF core to parse them properly. The trip point and contribution value is shared by multiple cooling devices now and so a new structure is created, struct __thermal_cooling_bind_param, which represents a cooling device and its min/max states and the existing struct __thermal_bind_params now contains an array of this new cooling device structure.

Tested on Hikey960.

Signed-off-by Viresh Kumar <[email protected]>

---
V2:
- Updated commit log to include new structures information
- Updated two error messages to give better error log

 drivers/thermal/of-thermal.c | 142 +++++++++++++++++++++++++----------
 1 file changed, 103 insertions(+), 39 deletions(-)

drivers/thermal/of-thermal.c +66 -34

index 977a8307fbb1..16b890a4fb50 100644
@@ -19,22 +19,33 @@
 /***   Private data structures to represent thermal device tree data ***/
 
 /**
- * struct __thermal_bind_param - a match between trip and cooling device
+ * struct __thermal_cooling_bind_param - a cooling device for a trip point
  * @cooling_device: a pointer to identify the referred cooling device
- * @trip_id: the trip point index
- * @usage: the percentage (from 0 to 100) of cooling contribution
  * @min: minimum cooling state used at this trip point
  * @max: maximum cooling state used at this trip point
  */
 
-struct __thermal_bind_params {
+struct __thermal_cooling_bind_param {
 	struct device_node *cooling_device;
-	unsigned int trip_id;
-	unsigned int usage;
 	unsigned long min;
 	unsigned long max;
 };
 
+/**
+ * struct __thermal_bind_param - a match between trip and cooling device
+ * @tcbp: a pointer to an array of cooling devices
+ * @count: number of elements in array
+ * @trip_id: the trip point index
+ * @usage: the percentage (from 0 to 100) of cooling contribution
+ */
+
+struct __thermal_bind_params {
+	struct __thermal_cooling_bind_param *tcbp;
+	unsigned int count;
+	unsigned int trip_id;
+	unsigned int usage;
+};
+
 /**
  * struct __thermal_zone - internal representation of a thermal zone
  * @mode: current thermal zone device mode (enabled/disabled)

@@ -192,25 +203,31 @@ static int of_thermal_bind(struct thermal_zone_device *thermal,
 			   struct thermal_cooling_device *cdev)
 {
 	struct __thermal_zone *data = thermal->devdata;
-	int i;
+	struct __thermal_bind_params *tbp;
+	struct __thermal_cooling_bind_param *tcbp;
+	int i, j;
 
 	if (!data || IS_ERR(data))
 		return -ENODEV;
 
 	/* find where to bind */
 	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
+		tbp = data->tbps + i;
 
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
+		for (j = 0; j < tbp->count; j++) {
+			tcbp = tbp->tcbp + j;
 
-			ret = thermal_zone_bind_cooling_device(thermal,
+			if (tcbp->cooling_device == cdev->np) {
+				int ret;
+
+				ret = thermal_zone_bind_cooling_device(thermal,
 						tbp->trip_id, cdev,
-						tbp->max,
-						tbp->min,
+						tcbp->max,
+						tcbp->min,
 						tbp->usage);
-			if (ret)
-				return ret;
+				if (ret)
+					return ret;
+			}
 		}
 	}
 

@@ -221,22 +238,28 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal,
 			     struct thermal_cooling_device *cdev)
 {
 	struct __thermal_zone *data = thermal->devdata;
-	int i;
+	struct __thermal_bind_params *tbp;
+	struct __thermal_cooling_bind_param *tcbp;
+	int i, j;
 
 	if (!data || IS_ERR(data))
 		return -ENODEV;
 
 	/* find where to unbind */
 	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
+		tbp = data->tbps + i;
+
+		for (j = 0; j < tbp->count; j++) {
+			tcbp = tbp->tcbp + j;
 
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
+			if (tcbp->cooling_device == cdev->np) {
+				int ret;
 
-			ret = thermal_zone_unbind_cooling_device(thermal,
-						tbp->trip_id, cdev);
-			if (ret)
-				return ret;
+				ret = thermal_zone_unbind_cooling_device(thermal,
+							tbp->trip_id, cdev);
+				if (ret)
+					return ret;
+			}
 		}
 	}
 

@@ -652,8 +675,9 @@ static int thermal_of_populate_bind_params(struct device_node *np,
 					   int ntrips)
 {
 	struct of_phandle_args cooling_spec;
+	struct __thermal_cooling_bind_param *__tcbp;
 	struct device_node *trip;
-	int ret, i;
+	int ret, i, count;
 	u32 prop;
 
 	/* Default weight. Usage is optional */

@@ -680,20 +704,44 @@ static int thermal_of_populate_bind_params(struct device_node *np,
 		goto end;
 	}
 
-	ret = of_parse_phandle_with_args(np, "cooling-device", "#cooling-cells",
-					 0, &cooling_spec);
-	if (ret < 0) {
-		pr_err("missing cooling_device property\n");
+	count = of_count_phandle_with_args(np, "cooling-device",
+					   "#cooling-cells");
+	if (!count) {
+		pr_err("Add a cooling_device property with at least one device\n");
 		goto end;
 	}
-	__tbp->cooling_device = cooling_spec.np;
-	if (cooling_spec.args_count >= 2) { /* at least min and max */
-		__tbp->min = cooling_spec.args[0];
-		__tbp->max = cooling_spec.args[1];
-	} else {
-		pr_err("wrong reference to cooling device, missing limits\n");
+
+	__tcbp = kcalloc(count, sizeof(*__tcbp), GFP_KERNEL);
+	if (!__tcbp)
+		goto end;
+
+	for (i = 0; i < count; i++) {
+		ret = of_parse_phandle_with_args(np, "cooling-device",
+				"#cooling-cells", i, &cooling_spec);
+		if (ret < 0) {
+			pr_err("Invalid cooling-device entry\n");
+			goto free_tcbp;
+		}
+
+		__tcbp[i].cooling_device = cooling_spec.np;

drivers/thermal/of-thermal.c +29 -5

+	__tbp->count = count;
+
+	goto end;
+
+free_tcbp:
+	for (i = i - 1; i >= 0; i--)
+		of_node_put(__tcbp[i].cooling_device);
+	kfree(__tcbp);
 end:
 	of_node_put(trip);
 

@@ -900,8 +948,16 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 	return tz;
 
 free_tbps:
-	for (i = i - 1; i >= 0; i--)
-		of_node_put(tz->tbps[i].cooling_device);
+	for (i = i - 1; i >= 0; i--) {
+		struct __thermal_bind_params *tbp = tz->tbps + i;
+		int j;
+
+		for (j = 0; j < tbp->count; j++)
+			of_node_put(tbp->tcbp[j].cooling_device);
+
+		kfree(tbp->tcbp);
+	}
+
 	kfree(tz->tbps);
 free_trips:
 	for (i = 0; i < tz->ntrips; i++)

@@ -917,10 +973,18 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 
 static inline void of_thermal_free_zone(struct __thermal_zone *tz)
 {
-	int i;
+	struct __thermal_bind_params *tbp;
+	int i, j;
+
+	for (i = 0; i < tz->num_tbps; i++) {
+		tbp = tz->tbps + i;
+
+		for (j = 0; j < tbp->count; j++)
+			of_node_put(tbp->tcbp[j].cooling_device);
+
+		kfree(tbp->tcbp);
+	}
 
-	for (i = 0; i < tz->num_tbps; i++)
-		of_node_put(tz->tbps[i].cooling_device);
 	kfree(tz->tbps);
 	for (i = 0; i < tz->ntrips; i++)
 		of_node_put(tz->trips[i].np);

Signature · 1 line

2.18.0.rc1.242.g61856ae69a2c

Hi,

This is an attempt to fix the broken or partially defined DT bindings for cooling-maps. We should list every device that participates in cooling down at a certain trip point, instead of just the first in the list as that depends on certain ordering of events to work properly.

The first patch extends the binding to allow a list of phandles in "cooling-device" property and the second patch fixes one of the platform's DT.

This will be followed up by fixing all platform DT bindings that have these issues after this set is accepted.

The kernel also requires some changes to handle the phandle list, but wouldn't break with these changes as it reads the first phandle in the list for now. We can update that separately.

--
viresh

Viresh Kumar (2):
dt-bindings: thermal: Allow multiple devices to share cooling map
arm64: dts: hi6220: Add all CPUs in cooling maps

 Documentation/devicetree/bindings/thermal/thermal.txt | 11 +++--------
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi             |  9 ++++++++-
 2 files changed, 11 insertions(+), 9 deletions(-)

Hi Viresh,

Quoted 25 lines · Viresh Kumar

Hi,

This is an attempt to fix the broken or partially defined DT bindings for cooling-maps. We should list every device that participates in cooling down at a certain trip point, instead of just the first in the list as that depends on certain ordering of events to work properly.

The first patch extends the binding to allow a list of phandles in "cooling-device" property and the second patch fixes one of the platform's DT.

This will be followed up by fixing all platform DT bindings that have these issues after this set is accepted.

The kernel also requires some changes to handle the phandle list, but wouldn't break with these changes as it reads the first phandle in the list for now. We can update that separately.

--
viresh

Viresh Kumar (2):
dt-bindings: thermal: Allow multiple devices to share cooling map
arm64: dts: hi6220: Add all CPUs in cooling maps

Quoted 4 lines

 Documentation/devicetree/bindings/thermal/thermal.txt | 11 +++--------
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi             |  9 ++++++++-
 2 files changed, 11 insertions(+), 9 deletions(-)

Thanks!
Applied both to the hisilicon dt tree.

Quoted 33 lines · Wei Xu

Hi Viresh,

Quoted 29 lines · Viresh Kumar

Hi,

This is an attempt to fix the broken or partially defined DT bindings for cooling-maps. We should list every device that participates in cooling down at a certain trip point, instead of just the first in the list as that depends on certain ordering of events to work properly.

The first patch extends the binding to allow a list of phandles in "cooling-device" property and the second patch fixes one of the platform's DT.

This will be followed up by fixing all platform DT bindings that have these issues after this set is accepted.

The kernel also requires some changes to handle the phandle list, but wouldn't break with these changes as it reads the first phandle in the list for now. We can update that separately.

--
viresh

Viresh Kumar (2):
dt-bindings: thermal: Allow multiple devices to share cooling map
arm64: dts: hi6220: Add all CPUs in cooling maps

 Documentation/devicetree/bindings/thermal/thermal.txt | 11 +++--------
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi             |  9 ++++++++-
 2 files changed, 11 insertions(+), 9 deletions(-)

Quoted 2 lines

Thanks!
Applied both to the hisilicon dt tree.

Hi Wei,

I expected the first patch to go via the thermal tree as it is for the thermal core maintainers. Second patch can very well go from your tree, but only after the 1st one is applied by thermal maintainers.

Hi,

This is an attempt to fix the broken or partially defined DT bindings for cooling-maps. We should list every device that participates in cooling down at a certain trip point, instead of just the first in the list as that depends on certain ordering of events to work properly.

The first patch extends the binding to allow a list of phandles in "cooling-device" property and the second patch fixes one of the platform's DT.

This will be followed up by fixing all platform DT bindings that have these issues after this set is accepted.

The kernel also requires some changes to handle the phandle list, but wouldn't break with these changes as it reads the first phandle in the list for now. We can update that separately.

Quoted 19 lines · Viresh Kumar

Hi,

This is an attempt to fix the broken or partially defined DT bindings
for cooling-maps. We should list every device that participates in cooling down at a certain trip point, instead of just the first in the
list as that depends on certain ordering of events to work properly.

The first patch extends the binding to allow a list of phandles in "cooling-device" property and the second patch fixes one of the platform's DT.

This will be followed up by fixing all platform DT bindings that have
these issues after this set is accepted.

Quoted 5 lines

The kernel also requires some changes to handle the phandle list, but
wouldn't break with these changes as it reads the first phandle in the
list for now. We can update that separately.

@Zhang: Are you going to apply this for 4.19-rc1 ? There are lot of patches that
I am holding up until this gets merged,

I suppose this patch should go via Eduardo' tree. Eduardo, can you please take a look at this patch set?

Quoted 3 lines · Zhang Rui

I suppose this patch should go via Eduardo' tree. Eduardo, can you please take a look at this patch set?

Zhang,

Since Eduardo isn't replying, will it be possible for you to pick it up as I
don't want to miss 4.19-rc1.