[PATCH v2 0/5] Various fixes to weights in the thermal framework

[PATCH v2 0/5] Various fixes to weights in the thermal framework

[Javi Merino]

Weights can be specified in the device tree, but the code currently
ignores the value.  Patch 1 fixes that by putting the weight in the
thermal_instance.  Patches 2-3 simplifies the code in the fair share
governor by using the weight in the now populated thermal_instance.
Patch 4 exports the weight to sysfs.  Patch 5 drops the requirement of
all weights needing to add up to a hundred.

Existing thermal zones using this governor should still work, as these
series shouldn't make any functional change for them.  However,
thermal zones that previously weren't able to use the fair share
governor (because they were being register through device tree or
because they weren't populating thermal_bind_params) now populate the
weight properly and are able to use the governor.

Changes since v1[0]:
   - Fix the call to thermal_zone_bind_cooling_device() in
     drivers/platform/x86/acerhdf.c and drivers/acpi/thermal.c

[0] http://thread.gmane.org/gmane.linux.power-management.general/55730

Javi Merino (4):
  thermal: fair_share: use the weight from the thermal instance
  thermal: fair_share: fix typo
  thermal: export weight to sysfs
  thermal: fair_share: generalize the weight concept

Kapileshwar Singh (1):
  thermal: of: fix cooling device weights in device tree

 Documentation/thermal/sysfs-api.txt                | 27 ++++++++--
 drivers/acpi/thermal.c                             |  9 ++--
 drivers/platform/x86/acerhdf.c                     |  3 +-
 drivers/thermal/db8500_thermal.c                   |  2 +-
 drivers/thermal/fair_share.c                       | 38 ++++++-------
 drivers/thermal/imx_thermal.c                      |  3 +-
 drivers/thermal/of-thermal.c                       |  5 +-
 drivers/thermal/samsung/exynos_thermal_common.c    |  3 +-
 drivers/thermal/thermal_core.c                     | 62 +++++++++++++++++++---
 drivers/thermal/thermal_core.h                     |  3 ++
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c |  3 +-
 include/linux/thermal.h                            | 19 +++++--
 12 files changed, 134 insertions(+), 43 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] thermal: of: fix cooling device weights in device tree

[Javi Merino]

From: Kapileshwar Singh <kapileshwar.singh@arm.com>

Currently you can specify the weight of the cooling device in the device
tree but that information is not populated to the
thermal_bind_params where the fair share governor expects it to
be.  The of thermal zone device doesn't have a thermal_bind_params
structure and arguably it's better to pass the weight inside the
thermal_instance as it is specific to the bind of a cooling device to a
thermal zone parameter.

Core thermal code is fixed to populate the weight in the instance from
the thermal_bind_params, so platform code that was passing the weight
inside the thermal_bind_params continue to work seamlessly.

While we are at it, create a default value for the weight parameter for
those thermal zones that currently don't define it and remove the
hardcoded default in of-thermal.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Kukjin Kim <kgene@kernel.org>
Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
---
 Documentation/thermal/sysfs-api.txt                |  4 +++-
 drivers/acpi/thermal.c                             |  9 ++++++---
 drivers/platform/x86/acerhdf.c                     |  3 ++-
 drivers/thermal/db8500_thermal.c                   |  2 +-
 drivers/thermal/fair_share.c                       |  2 +-
 drivers/thermal/imx_thermal.c                      |  3 ++-
 drivers/thermal/of-thermal.c                       |  5 +++--
 drivers/thermal/samsung/exynos_thermal_common.c    |  3 ++-
 drivers/thermal/thermal_core.c                     | 22 ++++++++++++++++------
 drivers/thermal/thermal_core.h                     |  1 +
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c |  3 ++-
 include/linux/thermal.h                            |  6 +++++-
 12 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 87519cb379ee..7ec632ed9769 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -95,7 +95,7 @@ temperature) and throttle appropriate devices.
 1.3 interface for binding a thermal zone device with a thermal cooling device
 1.3.1 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	int trip, struct thermal_cooling_device *cdev,
-	unsigned long upper, unsigned long lower);
+	unsigned long upper, unsigned long lower, unsigned int weight);
 
     This interface function bind a thermal cooling device to the certain trip
     point of a thermal zone device.
@@ -110,6 +110,8 @@ temperature) and throttle appropriate devices.
     lower:the Minimum cooling state can be used for this trip point.
           THERMAL_NO_LIMIT means no lower limit,
 	  and the cooling device can be in cooling state 0.
+    weight: the influence of this cooling device in this thermal
+            zone.  See 1.4.1 below for more information.
 
 1.3.2 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
 		int trip, struct thermal_cooling_device *cdev);
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index d24fa1964eb8..6d4e44ea74ac 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -800,7 +800,8 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
 				result =
 					thermal_zone_bind_cooling_device
 					(thermal, trip, cdev,
-					 THERMAL_NO_LIMIT, THERMAL_NO_LIMIT);
+					 THERMAL_NO_LIMIT, THERMAL_NO_LIMIT,
+					 THERMAL_WEIGHT_DEFAULT);
 			else
 				result =
 					thermal_zone_unbind_cooling_device
@@ -824,7 +825,8 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
 			if (bind)
 				result = thermal_zone_bind_cooling_device
 					(thermal, trip, cdev,
-					 THERMAL_NO_LIMIT, THERMAL_NO_LIMIT);
+					 THERMAL_NO_LIMIT, THERMAL_NO_LIMIT,
+					 THERMAL_WEIGHT_DEFAULT);
 			else
 				result = thermal_zone_unbind_cooling_device
 					(thermal, trip, cdev);
@@ -841,7 +843,8 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
 				result = thermal_zone_bind_cooling_device
 						(thermal, THERMAL_TRIPS_NONE,
 						 cdev, THERMAL_NO_LIMIT,
-						 THERMAL_NO_LIMIT);
+						 THERMAL_NO_LIMIT,
+						 THERMAL_WEIGHT_DEFAULT);
 			else
 				result = thermal_zone_unbind_cooling_device
 						(thermal, THERMAL_TRIPS_NONE,
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 594c918b553d..1ef02daddb60 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -372,7 +372,8 @@ static int acerhdf_bind(struct thermal_zone_device *thermal,
 		return 0;
 
 	if (thermal_zone_bind_cooling_device(thermal, 0, cdev,
-			THERMAL_NO_LIMIT, THERMAL_NO_LIMIT)) {
+			THERMAL_NO_LIMIT, THERMAL_NO_LIMIT,
+			THERMAL_WEIGHT_DEFAULT)) {
 		pr_err("error binding cooling dev\n");
 		return -EINVAL;
 	}
diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
index 20adfbe27df1..2fb273c4baa9 100644
--- a/drivers/thermal/db8500_thermal.c
+++ b/drivers/thermal/db8500_thermal.c
@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device *thermal,
 		upper = lower = i > max_state ? max_state : i;
 
 		ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-			upper, lower);
+			upper, lower, THERMAL_WEIGHT_DEFAULT);
 
 		dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
 			i, ret, ret ? "fail" : "succeed");
diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index 6e0a3fbfae86..c3b25187b467 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 			continue;
 
 		instance->target = get_target_state(tz, cdev,
-					tzp->tbp[i].weight, cur_trip_level);
+					instance->weight, cur_trip_level);
 
 		instance->cdev->updated = false;
 		thermal_cdev_update(cdev);
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 2ccbc0788353..fde4c2876d14 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -306,7 +306,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
 	ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
 					       THERMAL_NO_LIMIT,
-					       THERMAL_NO_LIMIT);
+					       THERMAL_NO_LIMIT,
+					       THERMAL_WEIGHT_DEFAULT);
 	if (ret) {
 		dev_err(&tz->device,
 			"binding zone %s with cdev %s failed:%d\n",
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index d717f3dab6f1..60e3a21461e7 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -227,7 +227,8 @@ static int of_thermal_bind(struct thermal_zone_device *thermal,
 			ret = thermal_zone_bind_cooling_device(thermal,
 						tbp->trip_id, cdev,
 						tbp->max,
-						tbp->min);
+						tbp->min,
+						tbp->usage);
 			if (ret)
 				return ret;
 		}
@@ -578,7 +579,7 @@ static int thermal_of_populate_bind_params(struct device_node *np,
 	u32 prop;
 
 	/* Default weight. Usage is optional */
-	__tbp->usage = 0;
+	__tbp->usage = THERMAL_WEIGHT_DEFAULT;
 	ret = of_property_read_u32(np, "contribution", &prop);
 	if (ret == 0)
 		__tbp->usage = prop;
diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c
index 6dc3815cc73f..03f2ebb1e5c3 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -163,7 +163,8 @@ static int exynos_bind(struct thermal_zone_device *thermal,
 		case MONITOR_ZONE:
 		case WARN_ZONE:
 			if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-								level, 0)) {
+						level, 0,
+						THERMAL_WEIGHT_DEFAULT)) {
 				dev_err(data->dev,
 					"error unbinding cdev inst=%d\n", i);
 				ret = -EINVAL;
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 87e0b0782023..9a424a66c83a 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -218,7 +218,8 @@ static void print_bind_err_msg(struct thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
 			struct thermal_cooling_device *cdev,
-			unsigned long *limits)
+			unsigned long *limits,
+			unsigned int weight)
 {
 	int i, ret;
 
@@ -233,7 +234,8 @@ static void __bind(struct thermal_zone_device *tz, int mask,
 				upper = limits[i * 2 + 1];
 			}
 			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-							       upper, lower);
+							       upper, lower,
+							       weight);
 			if (ret)
 				print_bind_err_msg(tz, cdev, ret);
 		}
@@ -280,7 +282,8 @@ static void bind_cdev(struct thermal_cooling_device *cdev)
 				continue;
 			tzp->tbp[i].cdev = cdev;
 			__bind(pos, tzp->tbp[i].trip_mask, cdev,
-			       tzp->tbp[i].binding_limits);
+			       tzp->tbp[i].binding_limits,
+			       tzp->tbp[i].weight);
 		}
 	}
 
@@ -319,7 +322,8 @@ static void bind_tz(struct thermal_zone_device *tz)
 				continue;
 			tzp->tbp[i].cdev = pos;
 			__bind(tz, tzp->tbp[i].trip_mask, pos,
-			       tzp->tbp[i].binding_limits);
+			       tzp->tbp[i].binding_limits,
+			       tzp->tbp[i].weight);
 		}
 	}
 exit:
@@ -711,7 +715,8 @@ passive_store(struct device *dev, struct device_attribute *attr,
 				thermal_zone_bind_cooling_device(tz,
 						THERMAL_TRIPS_NONE, cdev,
 						THERMAL_NO_LIMIT,
-						THERMAL_NO_LIMIT);
+						THERMAL_NO_LIMIT,
+						THERMAL_WEIGHT_DEFAULT);
 		}
 		mutex_unlock(&thermal_list_lock);
 		if (!tz->passive_delay)
@@ -913,6 +918,9 @@ thermal_cooling_device_trip_point_show(struct device *dev,
  * @lower:	the Minimum cooling state can be used for this trip point.
  *		THERMAL_NO_LIMIT means no lower limit,
  *		and the cooling device can be in cooling state 0.
+ * @weight:	The weight of the cooling device to be bound to the
+ *		thermal zone. Use THERMAL_WEIGHT_DEFAULT for the
+ *		default value
  *
  * This interface function bind a thermal cooling device to the certain trip
  * point of a thermal zone device.
@@ -923,7 +931,8 @@ thermal_cooling_device_trip_point_show(struct device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 				     int trip,
 				     struct thermal_cooling_device *cdev,
-				     unsigned long upper, unsigned long lower)
+				     unsigned long upper, unsigned long lower,
+				     unsigned int weight)
 {
 	struct thermal_instance *dev;
 	struct thermal_instance *pos;
@@ -968,6 +977,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	dev->upper = upper;
 	dev->lower = lower;
 	dev->target = THERMAL_NO_TARGET;
+	dev->weight = weight;
 
 	result = get_idr(&tz->idr, &tz->lock, &dev->id);
 	if (result)
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 0531c752fbbb..7a465e9d456c 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -48,6 +48,7 @@ struct thermal_instance {
 	struct device_attribute attr;
 	struct list_head tz_node; /* node in tz->thermal_instances */
 	struct list_head cdev_node; /* node in cdev->thermal_instances */
+	unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 3fb054a10f6a..4638718fabf6 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -146,7 +146,8 @@ static int ti_thermal_bind(struct thermal_zone_device *thermal,
 	return thermal_zone_bind_cooling_device(thermal, 0, cdev,
 	/* bind with min and max states defined by cpu_cooling */
 						THERMAL_NO_LIMIT,
-						THERMAL_NO_LIMIT);
+						THERMAL_NO_LIMIT,
+						THERMAL_WEIGHT_DEFAULT);
 }
 
 /* Unbind callback functions for thermal zone */
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index fc52e307efab..2ed7062fac1d 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT	((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 0
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
 				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
@@ -321,7 +324,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
 				     struct thermal_cooling_device *,
-				     unsigned long, unsigned long);
+				     unsigned long, unsigned long,
+				     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
 				       struct thermal_cooling_device *);
 void thermal_zone_device_update(struct thermal_zone_device *);
-- 
1.9.1

[PATCH v2 2/5] thermal: fair_share: use the weight from the thermal instance

[Javi Merino]

The fair share governor is not usable with thermal zones that use the
bind op and don't populate thermal_zone_parameters, the majority of
them.  Now that the weight is in the thermal instance, we can use that
in the fair share governor to allow every thermal zone to trivially use
this governor.  Furthermore, this simplifies the code.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/thermal/fair_share.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index c3b25187b467..9e392d34ac9f 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -88,24 +88,13 @@ static long get_target_state(struct thermal_zone_device *tz,
  */
 static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 {
-	const struct thermal_zone_params *tzp;
-	struct thermal_cooling_device *cdev;
 	struct thermal_instance *instance;
-	int i;
 	int cur_trip_level = get_trip_level(tz);
 
-	if (!tz->tzp || !tz->tzp->tbp)
-		return -EINVAL;
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		struct thermal_cooling_device *cdev = instance->cdev;
 
-	tzp = tz->tzp;
-
-	for (i = 0; i < tzp->num_tbps; i++) {
-		if (!tzp->tbp[i].cdev)
-			continue;
-
-		cdev = tzp->tbp[i].cdev;
-		instance = get_thermal_instance(tz, cdev, trip);
-		if (!instance)
+		if (instance->trip != trip)
 			continue;
 
 		instance->target = get_target_state(tz, cdev,
-- 
1.9.1

[PATCH v2 3/5] thermal: fair_share: fix typo

[Javi Merino]

s/asscciated/associated/

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/thermal/fair_share.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index 9e392d34ac9f..692f4053f08b 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -69,7 +69,7 @@ static long get_target_state(struct thermal_zone_device *tz,
 }
 
 /**
- * fair_share_throttle - throttles devices asscciated with the given zone
+ * fair_share_throttle - throttles devices associated with the given zone
  * @tz - thermal_zone_device
  *
  * Throttling Logic: This uses three parameters to calculate the new
-- 
1.9.1

[PATCH v2 4/5] thermal: export weight to sysfs

[Javi Merino]

It's useful to have access to the weights for the cooling devices for
thermal zones and change them if needed.  Export them to sysfs.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 Documentation/thermal/sysfs-api.txt | 15 +++++++++++++-
 drivers/thermal/thermal_core.c      | 40 +++++++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_core.h      |  2 ++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 7ec632ed9769..3625453ceef6 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -194,6 +194,8 @@ thermal_zone_bind_cooling_device/thermal_zone_unbind_cooling_device.
 /sys/class/thermal/thermal_zone[0-*]:
     |---cdev[0-*]:		[0-*]th cooling device in current thermal zone
     |---cdev[0-*]_trip_point:	Trip point that cdev[0-*] is associated with
+    |---cdev[0-*]_weight:       Influence of the cooling device in
+                                this thermal zone
 
 Besides the thermal zone device sysfs I/F and cooling device sysfs I/F,
 the generic thermal driver also creates a hwmon sysfs I/F for each _type_
@@ -267,6 +269,14 @@ cdev[0-*]_trip_point
 	point.
 	RO, Optional
 
+cdev[0-*]_weight
+        The influence of cdev[0-*] in this thermal zone. This value
+        is relative to the rest of cooling devices in the thermal
+        zone. For example, if a cooling device has a weight double
+        than that of other, it's twice as effective in cooling the
+        thermal zone.
+        RW, Optional
+
 passive
 	Attribute is only present for zones in which the passive cooling
 	policy is not supported by native thermal driver. Default is zero
@@ -320,7 +330,8 @@ passive, active. If an ACPI thermal zone supports critical, passive,
 active[0] and active[1] at the same time, it may register itself as a
 thermal_zone_device (thermal_zone1) with 4 trip points in all.
 It has one processor and one fan, which are both registered as
-thermal_cooling_device.
+thermal_cooling_device. Both are considered to have the same
+effectiveness in cooling the thermal zone.
 
 If the processor is listed in _PSL method, and the fan is listed in _AL0
 method, the sys I/F structure will be built like this:
@@ -342,8 +353,10 @@ method, the sys I/F structure will be built like this:
     |---trip_point_3_type:	active1
     |---cdev0:			--->/sys/class/thermal/cooling_device0
     |---cdev0_trip_point:	1	/* cdev0 can be used for passive */
+    |---cdev0_weight:           1024
     |---cdev1:			--->/sys/class/thermal/cooling_device3
     |---cdev1_trip_point:	2	/* cdev1 can be used for active[0]*/
+    |---cdev1_weight:           1024
 
 |cooling_device0:
     |---type:			Processor
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9a424a66c83a..eaa704043e51 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -904,6 +904,34 @@ thermal_cooling_device_trip_point_show(struct device *dev,
 		return sprintf(buf, "%d\n", instance->trip);
 }
 
+static ssize_t
+thermal_cooling_device_weight_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct thermal_instance *instance;
+
+	instance = container_of(attr, struct thermal_instance, weight_attr);
+
+	return sprintf(buf, "%d\n", instance->weight);
+}
+
+static ssize_t
+thermal_cooling_device_weight_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct thermal_instance *instance;
+	int ret, weight;
+
+	ret = kstrtoint(buf, 0, &weight);
+	if (ret)
+		return ret;
+
+	instance = container_of(attr, struct thermal_instance, weight_attr);
+	instance->weight = weight;
+
+	return count;
+}
 /* Device management */
 
 /**
@@ -998,6 +1026,16 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	if (result)
 		goto remove_symbol_link;
 
+	sprintf(dev->weight_attr_name, "cdev%d_weight", dev->id);
+	sysfs_attr_init(&dev->weight_attr.attr);
+	dev->weight_attr.attr.name = dev->weight_attr_name;
+	dev->weight_attr.attr.mode = S_IWUSR | S_IRUGO;
+	dev->weight_attr.show = thermal_cooling_device_weight_show;
+	dev->weight_attr.store = thermal_cooling_device_weight_store;
+	result = device_create_file(&tz->device, &dev->weight_attr);
+	if (result)
+		goto remove_trip_file;
+
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
@@ -1015,6 +1053,8 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	if (!result)
 		return 0;
 
+	device_remove_file(&tz->device, &dev->weight_attr);
+remove_trip_file:
 	device_remove_file(&tz->device, &dev->attr);
 remove_symbol_link:
 	sysfs_remove_link(&tz->device.kobj, dev->name);
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 7a465e9d456c..faebe881f062 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -46,6 +46,8 @@ struct thermal_instance {
 	unsigned long target;	/* expected cooling state */
 	char attr_name[THERMAL_NAME_LENGTH];
 	struct device_attribute attr;
+	char weight_attr_name[THERMAL_NAME_LENGTH];
+	struct device_attribute weight_attr;
 	struct list_head tz_node; /* node in tz->thermal_instances */
 	struct list_head cdev_node; /* node in cdev->thermal_instances */
 	unsigned int weight; /* The weight of the cooling device */
-- 
1.9.1

[PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Javi Merino]

The fair share governor has the concept of weights, which is the
influence of each cooling device in a thermal zone.  The current
implementation forces the weights of all cooling devices in a thermal
zone to add up to a 100.  This complicates setups, as you need to know
in advance how many cooling devices you are going to have.  If you bind a
new cooling device, you have to modify all the other cooling devices
weights, which is error prone.  Furthermore, you can't specify a
"default" weight for platforms since that default value depends on the
number of cooling devices in the platform.

This patch generalizes the concept of weight by allowing any number to
be a "weight".  Weights are now relative to each other.  Platforms that
don't specify weights get the same default value for all their cooling
devices, so all their cdevs are considered to be equally influential.

It's important to note that previous users of the weights don't need to
alter the code: percentages continue to work as they used to.  This
patch just removes the constraint of all the weights in a thermal zone
having to add up to a 100.  If they do, you get the same behavior as
before.  If they don't, fair share now works for that platform.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 Documentation/thermal/sysfs-api.txt |  8 +++++---
 drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
 include/linux/thermal.h             | 17 ++++++++++++-----
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 3625453ceef6..c9fd014f0c21 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
     This structure defines the following parameters that are used to bind
     a zone with a cooling device for a particular trip point.
     .cdev: The cooling device pointer
-    .weight: The 'influence' of a particular cooling device on this zone.
-             This is on a percentage scale. The sum of all these weights
-             (for a particular zone) cannot exceed 100.
+    .weight: The 'influence' of a particular cooling device on this
+             zone. This is relative to the rest of the cooling
+             devices. For example, if all cooling devices have a
+             weight of 1, then they all contribute the same. You can
+             use percentages if you want, but it's not mandatory.
     .trip_mask:This is a bit mask that gives the binding relation between
                this thermal zone and cdev, for a particular trip point.
                If nth bit is set, then the cdev and thermal zone are bound
diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index 692f4053f08b..5cd6ff1d0a4c 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
 }
 
 static long get_target_state(struct thermal_zone_device *tz,
-		struct thermal_cooling_device *cdev, int weight, int level)
+		struct thermal_cooling_device *cdev, int percentage, int level)
 {
 	unsigned long max_state;
 
 	cdev->ops->get_max_state(cdev, &max_state);
 
-	return (long)(weight * level * max_state) / (100 * tz->trips);
+	return (long)(percentage * level * max_state) / (100 * tz->trips);
 }
 
 /**
@@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
  *
  * Parameters used for Throttling:
  * P1. max_state: Maximum throttle state exposed by the cooling device.
- * P2. weight[i]/100:
+ * P2. percentage[i]/100:
  *	How 'effective' the 'i'th device is, in cooling the given zone.
  * P3. cur_trip_level/max_no_of_trips:
  *	This describes the extent to which the devices should be throttled.
@@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
 static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 {
 	struct thermal_instance *instance;
+	int total_weight = 0;
 	int cur_trip_level = get_trip_level(tz);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		if (instance->trip != trip)
+			continue;
+
+		total_weight += instance->weight;
+	}
+
+	if (!total_weight)
+		return -EINVAL;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		int percentage;
 		struct thermal_cooling_device *cdev = instance->cdev;
 
 		if (instance->trip != trip)
 			continue;
 
-		instance->target = get_target_state(tz, cdev,
-					instance->weight, cur_trip_level);
+		percentage = (instance->weight * 100) / total_weight;
+		instance->target = get_target_state(tz, cdev, percentage,
+						    cur_trip_level);
 
 		instance->cdev->updated = false;
 		thermal_cdev_update(cdev);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 2ed7062fac1d..2426426731ca 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -40,8 +40,12 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT	((u32)~0)
 
-/* Default weight of a bound cooling device */
-#define THERMAL_WEIGHT_DEFAULT 0
+/*
+ * Default weight of a bound cooling device.  By setting it to 1024 we
+ * give developers a range so that they can specify cooling devices
+ * that are less or more "influential" than the default
+ */
+#define THERMAL_WEIGHT_DEFAULT 1024
 
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
@@ -217,9 +221,12 @@ struct thermal_bind_params {
 
 	/*
 	 * This is a measure of 'how effectively these devices can
-	 * cool 'this' thermal zone. The shall be determined by platform
-	 * characterization. This is on a 'percentage' scale.
-	 * See Documentation/thermal/sysfs-api.txt for more information.
+	 * cool 'this' thermal zone. It shall be determined by
+	 * platform characterization. This value is relative to the
+	 * rest of the weights so a cooling device whose weight is
+	 * double that of another cooling device is twice as
+	 * effective. See Documentation/thermal/sysfs-api.txt for more
+	 * information.
 	 */
 	int weight;
 
-- 
1.9.1

Re: [PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Zhang Rui]

Hi, Javi,

In general, the patches look good to me.
They are all reasonable fixes/enhancements to me. Thanks a lot for your
work.
I just have one comment with this patch.

On Mon, 2015-02-02 at 12:51 +0000, Javi Merino wrote:
> The fair share governor has the concept of weights, which is the
> influence of each cooling device in a thermal zone.  The current
> implementation forces the weights of all cooling devices in a thermal
> zone to add up to a 100.  This complicates setups, as you need to know
> in advance how many cooling devices you are going to have.  If you bind a
> new cooling device, you have to modify all the other cooling devices
> weights, which is error prone.  Furthermore, you can't specify a
> "default" weight for platforms since that default value depends on the
> number of cooling devices in the platform.
> 
> This patch generalizes the concept of weight by allowing any number to
> be a "weight".  Weights are now relative to each other.  Platforms that
> don't specify weights get the same default value for all their cooling
> devices, so all their cdevs are considered to be equally influential.
> 
I don't think we have covered these two cases.

First of all. for the thermal zones that bind using tz->tbp, and all the
weights equals 0, it actually means THERMAL_WEIGHT_DEFAULT instead of
zero.
IMO, I think we should 
1. leave THERMAL_WEIGHT_DEFAULT as 0
2. remove
	if (!total_weight)
		return -EINVAL;
3. when calculating percentage,
	if (!total_weight)
		percentage = 100 / total_instances_of_the_current_trip;

Second, for the thermal zones that don't specify the weight for all of
its cooling devices. In this case, I think only the cooling devices with
weight should be used. Thus, we should
1. leave THERMAL_WEIGHT_DEFAULT as 0
2. for cooling devices do not have weight specified, we don't need to
change any code and its percentage is zero.

In all, I'd prefer to remove the change for THERMAL_WEIGHT_DEFAULT, and
change fair_share_throttle() as below, what do you think?

thanks,
rui

@@ -77,7 +77,7 @@ static long get_target_state(struct
thermal_zone_device *tz,
  *
  * Parameters used for Throttling:
  * P1. max_state: Maximum throttle state exposed by the cooling device.
- * P2. weight[i]/100:
+ * P2. percentage[i]/100:
  *     How 'effective' the 'i'th device is, in cooling the given zone.
  * P3. cur_trip_level/max_no_of_trips:
  *     This describes the extent to which the devices should be
throttled.
@@ -89,16 +89,31 @@ static long get_target_state(struct
thermal_zone_device *tz,
 static int fair_share_throttle(struct thermal_zone_device *tz, int
trip)
 {
        struct thermal_instance *instance;
+       int total_weight = 0;
+       int total_instance = 0;
        int cur_trip_level = get_trip_level(tz);
 
        list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+               if (instance->trip != trip)
+                       continue;
+
+               total_weight += instance->weight;
+               total_instance++;
+       }
+
+       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+               int percentage;
                struct thermal_cooling_device *cdev = instance->cdev;
 
                if (instance->trip != trip)
                        continue;
 
-               instance->target = get_target_state(tz, cdev,
-                                       instance->weight,
cur_trip_level);
+               if (!total_weight)
+                       percentage = 100 / total_instance;
+               else
+                       percentage = (instance->weight * 100) /
total_weight;
+               instance->target = get_target_state(tz, cdev,
percentage,
+                                                   cur_trip_level);
 
                instance->cdev->updated = false;
                thermal_cdev_update(cdev);


> It's important to note that previous users of the weights don't need to
> alter the code: percentages continue to work as they used to.  This
> patch just removes the constraint of all the weights in a thermal zone
> having to add up to a 100.  If they do, you get the same behavior as
> before.  If they don't, fair share now works for that platform.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
>  Documentation/thermal/sysfs-api.txt |  8 +++++---
>  drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
>  include/linux/thermal.h             | 17 ++++++++++++-----
>  3 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 3625453ceef6..c9fd014f0c21 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
>      This structure defines the following parameters that are used to bind
>      a zone with a cooling device for a particular trip point.
>      .cdev: The cooling device pointer
> -    .weight: The 'influence' of a particular cooling device on this zone.
> -             This is on a percentage scale. The sum of all these weights
> -             (for a particular zone) cannot exceed 100.
> +    .weight: The 'influence' of a particular cooling device on this
> +             zone. This is relative to the rest of the cooling
> +             devices. For example, if all cooling devices have a
> +             weight of 1, then they all contribute the same. You can
> +             use percentages if you want, but it's not mandatory.
>      .trip_mask:This is a bit mask that gives the binding relation between
>                 this thermal zone and cdev, for a particular trip point.
>                 If nth bit is set, then the cdev and thermal zone are bound
> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> index 692f4053f08b..5cd6ff1d0a4c 100644
> --- a/drivers/thermal/fair_share.c
> +++ b/drivers/thermal/fair_share.c
> @@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
>  }
>  
>  static long get_target_state(struct thermal_zone_device *tz,
> -		struct thermal_cooling_device *cdev, int weight, int level)
> +		struct thermal_cooling_device *cdev, int percentage, int level)
>  {
>  	unsigned long max_state;
>  
>  	cdev->ops->get_max_state(cdev, &max_state);
>  
> -	return (long)(weight * level * max_state) / (100 * tz->trips);
> +	return (long)(percentage * level * max_state) / (100 * tz->trips);
>  }
>  
>  /**
> @@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
>   *
>   * Parameters used for Throttling:
>   * P1. max_state: Maximum throttle state exposed by the cooling device.
> - * P2. weight[i]/100:
> + * P2. percentage[i]/100:
>   *	How 'effective' the 'i'th device is, in cooling the given zone.
>   * P3. cur_trip_level/max_no_of_trips:
>   *	This describes the extent to which the devices should be throttled.
> @@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
>  static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
>  {
>  	struct thermal_instance *instance;
> +	int total_weight = 0;
>  	int cur_trip_level = get_trip_level(tz);
>  
>  	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +		if (instance->trip != trip)
> +			continue;
> +
> +		total_weight += instance->weight;
> +	}
> +
> +	if (!total_weight)
> +		return -EINVAL;
> +
> +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +		int percentage;
>  		struct thermal_cooling_device *cdev = instance->cdev;
>  
>  		if (instance->trip != trip)
>  			continue;
>  
> -		instance->target = get_target_state(tz, cdev,
> -					instance->weight, cur_trip_level);
> +		percentage = (instance->weight * 100) / total_weight;
> +		instance->target = get_target_state(tz, cdev, percentage,
> +						    cur_trip_level);
>  
>  		instance->cdev->updated = false;
>  		thermal_cdev_update(cdev);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 2ed7062fac1d..2426426731ca 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -40,8 +40,12 @@
>  /* No upper/lower limit requirement */
>  #define THERMAL_NO_LIMIT	((u32)~0)
>  
> -/* Default weight of a bound cooling device */
> -#define THERMAL_WEIGHT_DEFAULT 0
> +/*
> + * Default weight of a bound cooling device.  By setting it to 1024 we
> + * give developers a range so that they can specify cooling devices
> + * that are less or more "influential" than the default
> + */
> +#define THERMAL_WEIGHT_DEFAULT 1024
>  
>  /* Unit conversion macros */
>  #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> @@ -217,9 +221,12 @@ struct thermal_bind_params {
>  
>  	/*
>  	 * This is a measure of 'how effectively these devices can
> -	 * cool 'this' thermal zone. The shall be determined by platform
> -	 * characterization. This is on a 'percentage' scale.
> -	 * See Documentation/thermal/sysfs-api.txt for more information.
> +	 * cool 'this' thermal zone. It shall be determined by
> +	 * platform characterization. This value is relative to the
> +	 * rest of the weights so a cooling device whose weight is
> +	 * double that of another cooling device is twice as
> +	 * effective. See Documentation/thermal/sysfs-api.txt for more
> +	 * information.
>  	 */
>  	int weight;
>  

Re: [PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Javi Merino]

Hi Rui,

On Fri, Feb 06, 2015 at 09:29:54AM +0000, Zhang Rui wrote:
> In general, the patches look good to me.
> They are all reasonable fixes/enhancements to me. Thanks a lot for your
> work.

Good

> I just have one comment with this patch.

> On Mon, 2015-02-02 at 12:51 +0000, Javi Merino wrote:
> > The fair share governor has the concept of weights, which is the
> > influence of each cooling device in a thermal zone.  The current
> > implementation forces the weights of all cooling devices in a thermal
> > zone to add up to a 100.  This complicates setups, as you need to know
> > in advance how many cooling devices you are going to have.  If you bind a
> > new cooling device, you have to modify all the other cooling devices
> > weights, which is error prone.  Furthermore, you can't specify a
> > "default" weight for platforms since that default value depends on the
> > number of cooling devices in the platform.
> > 
> > This patch generalizes the concept of weight by allowing any number to
> > be a "weight".  Weights are now relative to each other.  Platforms that
> > don't specify weights get the same default value for all their cooling
> > devices, so all their cdevs are considered to be equally influential.
> > 
> I don't think we have covered these two cases.
> 
> First of all. for the thermal zones that bind using tz->tbp, and all the
> weights equals 0, it actually means THERMAL_WEIGHT_DEFAULT instead of
> zero.
> IMO, I think we should 
> 1. leave THERMAL_WEIGHT_DEFAULT as 0
> 2. remove
> 	if (!total_weight)
> 		return -EINVAL;
> 3. when calculating percentage,
> 	if (!total_weight)
> 		percentage = 100 / total_instances_of_the_current_trip;
> 
> Second, for the thermal zones that don't specify the weight for all of
> its cooling devices. In this case, I think only the cooling devices with
> weight should be used. Thus, we should
> 1. leave THERMAL_WEIGHT_DEFAULT as 0
> 2. for cooling devices do not have weight specified, we don't need to
> change any code and its percentage is zero.
> 
> In all, I'd prefer to remove the change for THERMAL_WEIGHT_DEFAULT, and
> change fair_share_throttle() as below, what do you think?

I think it makes it unnecessarily complicated as the default weight
will now have two different behaviours:

1. Cooling device not active if any devices have a weight and you are
   using the fair_share governor.
2. Cooling device active if no other device have weights.

It's hard to document. It should be either one or the other.  The
current implementation in this series make the cdevs do option 2
always.  If you prefer option 1 I can respin the patches (maybe
changing the "if (!total_weight)" return 0 instead of -EINVAL).

Cheers,
Javi

> @@ -77,7 +77,7 @@ static long get_target_state(struct
> thermal_zone_device *tz,
>   *
>   * Parameters used for Throttling:
>   * P1. max_state: Maximum throttle state exposed by the cooling device.
> - * P2. weight[i]/100:
> + * P2. percentage[i]/100:
>   *     How 'effective' the 'i'th device is, in cooling the given zone.
>   * P3. cur_trip_level/max_no_of_trips:
>   *     This describes the extent to which the devices should be
> throttled.
> @@ -89,16 +89,31 @@ static long get_target_state(struct
> thermal_zone_device *tz,
>  static int fair_share_throttle(struct thermal_zone_device *tz, int
> trip)
>  {
>         struct thermal_instance *instance;
> +       int total_weight = 0;
> +       int total_instance = 0;
>         int cur_trip_level = get_trip_level(tz);
>  
>         list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +               if (instance->trip != trip)
> +                       continue;
> +
> +               total_weight += instance->weight;
> +               total_instance++;
> +       }
> +
> +       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +               int percentage;
>                 struct thermal_cooling_device *cdev = instance->cdev;
>  
>                 if (instance->trip != trip)
>                         continue;
>  
> -               instance->target = get_target_state(tz, cdev,
> -                                       instance->weight,
> cur_trip_level);
> +               if (!total_weight)
> +                       percentage = 100 / total_instance;
> +               else
> +                       percentage = (instance->weight * 100) /
> total_weight;
> +               instance->target = get_target_state(tz, cdev,
> percentage,
> +                                                   cur_trip_level);
>  
>                 instance->cdev->updated = false;
>                 thermal_cdev_update(cdev);
> 
> 
> > It's important to note that previous users of the weights don't need to
> > alter the code: percentages continue to work as they used to.  This
> > patch just removes the constraint of all the weights in a thermal zone
> > having to add up to a 100.  If they do, you get the same behavior as
> > before.  If they don't, fair share now works for that platform.
> > 
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Eduardo Valentin <edubezval@gmail.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> >  Documentation/thermal/sysfs-api.txt |  8 +++++---
> >  drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
> >  include/linux/thermal.h             | 17 ++++++++++++-----
> >  3 files changed, 35 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > index 3625453ceef6..c9fd014f0c21 100644
> > --- a/Documentation/thermal/sysfs-api.txt
> > +++ b/Documentation/thermal/sysfs-api.txt
> > @@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
> >      This structure defines the following parameters that are used to bind
> >      a zone with a cooling device for a particular trip point.
> >      .cdev: The cooling device pointer
> > -    .weight: The 'influence' of a particular cooling device on this zone.
> > -             This is on a percentage scale. The sum of all these weights
> > -             (for a particular zone) cannot exceed 100.
> > +    .weight: The 'influence' of a particular cooling device on this
> > +             zone. This is relative to the rest of the cooling
> > +             devices. For example, if all cooling devices have a
> > +             weight of 1, then they all contribute the same. You can
> > +             use percentages if you want, but it's not mandatory.
> >      .trip_mask:This is a bit mask that gives the binding relation between
> >                 this thermal zone and cdev, for a particular trip point.
> >                 If nth bit is set, then the cdev and thermal zone are bound
> > diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> > index 692f4053f08b..5cd6ff1d0a4c 100644
> > --- a/drivers/thermal/fair_share.c
> > +++ b/drivers/thermal/fair_share.c
> > @@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
> >  }
> >  
> >  static long get_target_state(struct thermal_zone_device *tz,
> > -		struct thermal_cooling_device *cdev, int weight, int level)
> > +		struct thermal_cooling_device *cdev, int percentage, int level)
> >  {
> >  	unsigned long max_state;
> >  
> >  	cdev->ops->get_max_state(cdev, &max_state);
> >  
> > -	return (long)(weight * level * max_state) / (100 * tz->trips);
> > +	return (long)(percentage * level * max_state) / (100 * tz->trips);
> >  }
> >  
> >  /**
> > @@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
> >   *
> >   * Parameters used for Throttling:
> >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > - * P2. weight[i]/100:
> > + * P2. percentage[i]/100:
> >   *	How 'effective' the 'i'th device is, in cooling the given zone.
> >   * P3. cur_trip_level/max_no_of_trips:
> >   *	This describes the extent to which the devices should be throttled.
> > @@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
> >  static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
> >  {
> >  	struct thermal_instance *instance;
> > +	int total_weight = 0;
> >  	int cur_trip_level = get_trip_level(tz);
> >  
> >  	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > +		if (instance->trip != trip)
> > +			continue;
> > +
> > +		total_weight += instance->weight;
> > +	}
> > +
> > +	if (!total_weight)
> > +		return -EINVAL;
> > +
> > +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > +		int percentage;
> >  		struct thermal_cooling_device *cdev = instance->cdev;
> >  
> >  		if (instance->trip != trip)
> >  			continue;
> >  
> > -		instance->target = get_target_state(tz, cdev,
> > -					instance->weight, cur_trip_level);
> > +		percentage = (instance->weight * 100) / total_weight;
> > +		instance->target = get_target_state(tz, cdev, percentage,
> > +						    cur_trip_level);
> >  
> >  		instance->cdev->updated = false;
> >  		thermal_cdev_update(cdev);
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 2ed7062fac1d..2426426731ca 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -40,8 +40,12 @@
> >  /* No upper/lower limit requirement */
> >  #define THERMAL_NO_LIMIT	((u32)~0)
> >  
> > -/* Default weight of a bound cooling device */
> > -#define THERMAL_WEIGHT_DEFAULT 0
> > +/*
> > + * Default weight of a bound cooling device.  By setting it to 1024 we
> > + * give developers a range so that they can specify cooling devices
> > + * that are less or more "influential" than the default
> > + */
> > +#define THERMAL_WEIGHT_DEFAULT 1024
> >  
> >  /* Unit conversion macros */
> >  #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > @@ -217,9 +221,12 @@ struct thermal_bind_params {
> >  
> >  	/*
> >  	 * This is a measure of 'how effectively these devices can
> > -	 * cool 'this' thermal zone. The shall be determined by platform
> > -	 * characterization. This is on a 'percentage' scale.
> > -	 * See Documentation/thermal/sysfs-api.txt for more information.
> > +	 * cool 'this' thermal zone. It shall be determined by
> > +	 * platform characterization. This value is relative to the
> > +	 * rest of the weights so a cooling device whose weight is
> > +	 * double that of another cooling device is twice as
> > +	 * effective. See Documentation/thermal/sysfs-api.txt for more
> > +	 * information.
> >  	 */
> >  	int weight;
> >  
> 
> 
> 

Re: [PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Zhang Rui]

On Fri, 2015-02-06 at 12:42 +0000, Javi Merino wrote:
> Hi Rui,
> 
> On Fri, Feb 06, 2015 at 09:29:54AM +0000, Zhang Rui wrote:
> > In general, the patches look good to me.
> > They are all reasonable fixes/enhancements to me. Thanks a lot for your
> > work.
> 
> Good
> 
> > I just have one comment with this patch.
> 
> > On Mon, 2015-02-02 at 12:51 +0000, Javi Merino wrote:
> > > The fair share governor has the concept of weights, which is the
> > > influence of each cooling device in a thermal zone.  The current
> > > implementation forces the weights of all cooling devices in a thermal
> > > zone to add up to a 100.  This complicates setups, as you need to know
> > > in advance how many cooling devices you are going to have.  If you bind a
> > > new cooling device, you have to modify all the other cooling devices
> > > weights, which is error prone.  Furthermore, you can't specify a
> > > "default" weight for platforms since that default value depends on the
> > > number of cooling devices in the platform.
> > > 
> > > This patch generalizes the concept of weight by allowing any number to
> > > be a "weight".  Weights are now relative to each other.  Platforms that
> > > don't specify weights get the same default value for all their cooling
> > > devices, so all their cdevs are considered to be equally influential.
> > > 
> > I don't think we have covered these two cases.
> > 
> > First of all. for the thermal zones that bind using tz->tbp, and all the
> > weights equals 0, it actually means THERMAL_WEIGHT_DEFAULT instead of
> > zero.
> > IMO, I think we should 
> > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > 2. remove
> > 	if (!total_weight)
> > 		return -EINVAL;
> > 3. when calculating percentage,
> > 	if (!total_weight)
> > 		percentage = 100 / total_instances_of_the_current_trip;
> > 
> > Second, for the thermal zones that don't specify the weight for all of
> > its cooling devices. In this case, I think only the cooling devices with
> > weight should be used. Thus, we should
> > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > 2. for cooling devices do not have weight specified, we don't need to
> > change any code and its percentage is zero.
> > 
> > In all, I'd prefer to remove the change for THERMAL_WEIGHT_DEFAULT, and
> > change fair_share_throttle() as below, what do you think?
> 
> I think it makes it unnecessarily complicated as the default weight
> will now have two different behaviours:
> 
> 1. Cooling device not active if any devices have a weight and you are
>    using the fair_share governor.
> 2. Cooling device active if no other device have weights.
> 
> It's hard to document. It should be either one or the other.  The
> current implementation in this series make the cdevs do option 2
> always.

I just want to make thermal core intelligent and simplify the platform
thermal driver. Because in most cases, an uninitialized value (zero)
means the driver does not care and it wants to follow the default
behavior.
With your change in patch 5/5, for a driver that uses tz->tzp->tbp
directly (not the of_thermal way), in order to use fair_share governor,
it needs to set tbp[i].weight to THERMAL_WEIGHT_DEFAULT explicitly, like
you do in of_thermal.c in patch 1/5, right?

CC Durgadoss, the author of the fair_share governor.

thanks,
rui
>   If you prefer option 1 I can respin the patches (maybe
> changing the "if (!total_weight)" return 0 instead of -EINVAL).
> 
> Cheers,
> Javi
> 
> > @@ -77,7 +77,7 @@ static long get_target_state(struct
> > thermal_zone_device *tz,
> >   *
> >   * Parameters used for Throttling:
> >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > - * P2. weight[i]/100:
> > + * P2. percentage[i]/100:
> >   *     How 'effective' the 'i'th device is, in cooling the given zone.
> >   * P3. cur_trip_level/max_no_of_trips:
> >   *     This describes the extent to which the devices should be
> > throttled.
> > @@ -89,16 +89,31 @@ static long get_target_state(struct
> > thermal_zone_device *tz,
> >  static int fair_share_throttle(struct thermal_zone_device *tz, int
> > trip)
> >  {
> >         struct thermal_instance *instance;
> > +       int total_weight = 0;
> > +       int total_instance = 0;
> >         int cur_trip_level = get_trip_level(tz);
> >  
> >         list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > +               if (instance->trip != trip)
> > +                       continue;
> > +
> > +               total_weight += instance->weight;
> > +               total_instance++;
> > +       }
> > +
> > +       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > +               int percentage;
> >                 struct thermal_cooling_device *cdev = instance->cdev;
> >  
> >                 if (instance->trip != trip)
> >                         continue;
> >  
> > -               instance->target = get_target_state(tz, cdev,
> > -                                       instance->weight,
> > cur_trip_level);
> > +               if (!total_weight)
> > +                       percentage = 100 / total_instance;
> > +               else
> > +                       percentage = (instance->weight * 100) /
> > total_weight;
> > +               instance->target = get_target_state(tz, cdev,
> > percentage,
> > +                                                   cur_trip_level);
> >  
> >                 instance->cdev->updated = false;
> >                 thermal_cdev_update(cdev);
> > 
> > 
> > > It's important to note that previous users of the weights don't need to
> > > alter the code: percentages continue to work as they used to.  This
> > > patch just removes the constraint of all the weights in a thermal zone
> > > having to add up to a 100.  If they do, you get the same behavior as
> > > before.  If they don't, fair share now works for that platform.
> > > 
> > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > Cc: Eduardo Valentin <edubezval@gmail.com>
> > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > ---
> > >  Documentation/thermal/sysfs-api.txt |  8 +++++---
> > >  drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
> > >  include/linux/thermal.h             | 17 ++++++++++++-----
> > >  3 files changed, 35 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > > index 3625453ceef6..c9fd014f0c21 100644
> > > --- a/Documentation/thermal/sysfs-api.txt
> > > +++ b/Documentation/thermal/sysfs-api.txt
> > > @@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
> > >      This structure defines the following parameters that are used to bind
> > >      a zone with a cooling device for a particular trip point.
> > >      .cdev: The cooling device pointer
> > > -    .weight: The 'influence' of a particular cooling device on this zone.
> > > -             This is on a percentage scale. The sum of all these weights
> > > -             (for a particular zone) cannot exceed 100.
> > > +    .weight: The 'influence' of a particular cooling device on this
> > > +             zone. This is relative to the rest of the cooling
> > > +             devices. For example, if all cooling devices have a
> > > +             weight of 1, then they all contribute the same. You can
> > > +             use percentages if you want, but it's not mandatory.
> > >      .trip_mask:This is a bit mask that gives the binding relation between
> > >                 this thermal zone and cdev, for a particular trip point.
> > >                 If nth bit is set, then the cdev and thermal zone are bound
> > > diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> > > index 692f4053f08b..5cd6ff1d0a4c 100644
> > > --- a/drivers/thermal/fair_share.c
> > > +++ b/drivers/thermal/fair_share.c
> > > @@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
> > >  }
> > >  
> > >  static long get_target_state(struct thermal_zone_device *tz,
> > > -		struct thermal_cooling_device *cdev, int weight, int level)
> > > +		struct thermal_cooling_device *cdev, int percentage, int level)
> > >  {
> > >  	unsigned long max_state;
> > >  
> > >  	cdev->ops->get_max_state(cdev, &max_state);
> > >  
> > > -	return (long)(weight * level * max_state) / (100 * tz->trips);
> > > +	return (long)(percentage * level * max_state) / (100 * tz->trips);
> > >  }
> > >  
> > >  /**
> > > @@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
> > >   *
> > >   * Parameters used for Throttling:
> > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > - * P2. weight[i]/100:
> > > + * P2. percentage[i]/100:
> > >   *	How 'effective' the 'i'th device is, in cooling the given zone.
> > >   * P3. cur_trip_level/max_no_of_trips:
> > >   *	This describes the extent to which the devices should be throttled.
> > > @@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
> > >  static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
> > >  {
> > >  	struct thermal_instance *instance;
> > > +	int total_weight = 0;
> > >  	int cur_trip_level = get_trip_level(tz);
> > >  
> > >  	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > +		if (instance->trip != trip)
> > > +			continue;
> > > +
> > > +		total_weight += instance->weight;
> > > +	}
> > > +
> > > +	if (!total_weight)
> > > +		return -EINVAL;
> > > +
> > > +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > +		int percentage;
> > >  		struct thermal_cooling_device *cdev = instance->cdev;
> > >  
> > >  		if (instance->trip != trip)
> > >  			continue;
> > >  
> > > -		instance->target = get_target_state(tz, cdev,
> > > -					instance->weight, cur_trip_level);
> > > +		percentage = (instance->weight * 100) / total_weight;
> > > +		instance->target = get_target_state(tz, cdev, percentage,
> > > +						    cur_trip_level);
> > >  
> > >  		instance->cdev->updated = false;
> > >  		thermal_cdev_update(cdev);
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index 2ed7062fac1d..2426426731ca 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -40,8 +40,12 @@
> > >  /* No upper/lower limit requirement */
> > >  #define THERMAL_NO_LIMIT	((u32)~0)
> > >  
> > > -/* Default weight of a bound cooling device */
> > > -#define THERMAL_WEIGHT_DEFAULT 0
> > > +/*
> > > + * Default weight of a bound cooling device.  By setting it to 1024 we
> > > + * give developers a range so that they can specify cooling devices
> > > + * that are less or more "influential" than the default
> > > + */
> > > +#define THERMAL_WEIGHT_DEFAULT 1024
> > >  
> > >  /* Unit conversion macros */
> > >  #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > > @@ -217,9 +221,12 @@ struct thermal_bind_params {
> > >  
> > >  	/*
> > >  	 * This is a measure of 'how effectively these devices can
> > > -	 * cool 'this' thermal zone. The shall be determined by platform
> > > -	 * characterization. This is on a 'percentage' scale.
> > > -	 * See Documentation/thermal/sysfs-api.txt for more information.
> > > +	 * cool 'this' thermal zone. It shall be determined by
> > > +	 * platform characterization. This value is relative to the
> > > +	 * rest of the weights so a cooling device whose weight is
> > > +	 * double that of another cooling device is twice as
> > > +	 * effective. See Documentation/thermal/sysfs-api.txt for more
> > > +	 * information.
> > >  	 */
> > >  	int weight;
> > >  
> > 
> > 
> > 

Re: [PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Javi Merino]

Hi Rui,

On Fri, Feb 06, 2015 at 02:06:32PM +0000, Zhang Rui wrote:
> On Fri, 2015-02-06 at 12:42 +0000, Javi Merino wrote:
> > On Fri, Feb 06, 2015 at 09:29:54AM +0000, Zhang Rui wrote:
> > > On Mon, 2015-02-02 at 12:51 +0000, Javi Merino wrote:
> > > > The fair share governor has the concept of weights, which is the
> > > > influence of each cooling device in a thermal zone.  The current
> > > > implementation forces the weights of all cooling devices in a thermal
> > > > zone to add up to a 100.  This complicates setups, as you need to know
> > > > in advance how many cooling devices you are going to have.  If you bind a
> > > > new cooling device, you have to modify all the other cooling devices
> > > > weights, which is error prone.  Furthermore, you can't specify a
> > > > "default" weight for platforms since that default value depends on the
> > > > number of cooling devices in the platform.
> > > > 
> > > > This patch generalizes the concept of weight by allowing any number to
> > > > be a "weight".  Weights are now relative to each other.  Platforms that
> > > > don't specify weights get the same default value for all their cooling
> > > > devices, so all their cdevs are considered to be equally influential.
> > > > 
> > > I don't think we have covered these two cases.
> > > 
> > > First of all. for the thermal zones that bind using tz->tbp, and all the
> > > weights equals 0, it actually means THERMAL_WEIGHT_DEFAULT instead of
> > > zero.
> > > IMO, I think we should 
> > > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > > 2. remove
> > > 	if (!total_weight)
> > > 		return -EINVAL;
> > > 3. when calculating percentage,
> > > 	if (!total_weight)
> > > 		percentage = 100 / total_instances_of_the_current_trip;
> > > 
> > > Second, for the thermal zones that don't specify the weight for all of
> > > its cooling devices. In this case, I think only the cooling devices with
> > > weight should be used. Thus, we should
> > > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > > 2. for cooling devices do not have weight specified, we don't need to
> > > change any code and its percentage is zero.
> > > 
> > > In all, I'd prefer to remove the change for THERMAL_WEIGHT_DEFAULT, and
> > > change fair_share_throttle() as below, what do you think?
> > 
> > I think it makes it unnecessarily complicated as the default weight
> > will now have two different behaviours:
> > 
> > 1. Cooling device not active if any devices have a weight and you are
> >    using the fair_share governor.
> > 2. Cooling device active if no other device have weights.
> > 
> > It's hard to document. It should be either one or the other.  The
> > current implementation in this series make the cdevs do option 2
> > always.
> 
> I just want to make thermal core intelligent and simplify the platform
> thermal driver. Because in most cases, an uninitialized value (zero)
> means the driver does not care and it wants to follow the default
> behavior.
> With your change in patch 5/5, for a driver that uses tz->tzp->tbp
> directly (not the of_thermal way), in order to use fair_share governor,
> it needs to set tbp[i].weight to THERMAL_WEIGHT_DEFAULT explicitly, like
> you do in of_thermal.c in patch 1/5, right?

Right.  You have a valid point, not specifying anything should give you the
default behavior.  THERMAL_WEIGHT_DEFAULT must stay 0.

Now, I'm still a bit uncomfortable with your proposal.  Imagine that a
platform has three cooling devices with default weight (0).  That
means that each would be getting 33%.  The developer sees that and
changes one of the cooling devices' weight to 50.  That has the
unintuitive effect of disabling the other two cooling devices.

Should we just say weight=0 is "cooling device disabled for the
fair_share governor".

> CC Durgadoss, the author of the fair_share governor.

I forgot to add him because scripts/get_maintainer.pl didn't show
him.  I'll add him to the patches that touch the fair_share governor
from now on.

Cheers,
Javi

> > > @@ -77,7 +77,7 @@ static long get_target_state(struct
> > > thermal_zone_device *tz,
> > >   *
> > >   * Parameters used for Throttling:
> > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > - * P2. weight[i]/100:
> > > + * P2. percentage[i]/100:
> > >   *     How 'effective' the 'i'th device is, in cooling the given zone.
> > >   * P3. cur_trip_level/max_no_of_trips:
> > >   *     This describes the extent to which the devices should be
> > > throttled.
> > > @@ -89,16 +89,31 @@ static long get_target_state(struct
> > > thermal_zone_device *tz,
> > >  static int fair_share_throttle(struct thermal_zone_device *tz, int
> > > trip)
> > >  {
> > >         struct thermal_instance *instance;
> > > +       int total_weight = 0;
> > > +       int total_instance = 0;
> > >         int cur_trip_level = get_trip_level(tz);
> > >  
> > >         list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > +               if (instance->trip != trip)
> > > +                       continue;
> > > +
> > > +               total_weight += instance->weight;
> > > +               total_instance++;
> > > +       }
> > > +
> > > +       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > +               int percentage;
> > >                 struct thermal_cooling_device *cdev = instance->cdev;
> > >  
> > >                 if (instance->trip != trip)
> > >                         continue;
> > >  
> > > -               instance->target = get_target_state(tz, cdev,
> > > -                                       instance->weight,
> > > cur_trip_level);
> > > +               if (!total_weight)
> > > +                       percentage = 100 / total_instance;
> > > +               else
> > > +                       percentage = (instance->weight * 100) /
> > > total_weight;
> > > +               instance->target = get_target_state(tz, cdev,
> > > percentage,
> > > +                                                   cur_trip_level);
> > >  
> > >                 instance->cdev->updated = false;
> > >                 thermal_cdev_update(cdev);
> > > 
> > > 
> > > > It's important to note that previous users of the weights don't need to
> > > > alter the code: percentages continue to work as they used to.  This
> > > > patch just removes the constraint of all the weights in a thermal zone
> > > > having to add up to a 100.  If they do, you get the same behavior as
> > > > before.  If they don't, fair share now works for that platform.
> > > > 
> > > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > > Cc: Eduardo Valentin <edubezval@gmail.com>
> > > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > > ---
> > > >  Documentation/thermal/sysfs-api.txt |  8 +++++---
> > > >  drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
> > > >  include/linux/thermal.h             | 17 ++++++++++++-----
> > > >  3 files changed, 35 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > > > index 3625453ceef6..c9fd014f0c21 100644
> > > > --- a/Documentation/thermal/sysfs-api.txt
> > > > +++ b/Documentation/thermal/sysfs-api.txt
> > > > @@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
> > > >      This structure defines the following parameters that are used to bind
> > > >      a zone with a cooling device for a particular trip point.
> > > >      .cdev: The cooling device pointer
> > > > -    .weight: The 'influence' of a particular cooling device on this zone.
> > > > -             This is on a percentage scale. The sum of all these weights
> > > > -             (for a particular zone) cannot exceed 100.
> > > > +    .weight: The 'influence' of a particular cooling device on this
> > > > +             zone. This is relative to the rest of the cooling
> > > > +             devices. For example, if all cooling devices have a
> > > > +             weight of 1, then they all contribute the same. You can
> > > > +             use percentages if you want, but it's not mandatory.
> > > >      .trip_mask:This is a bit mask that gives the binding relation between
> > > >                 this thermal zone and cdev, for a particular trip point.
> > > >                 If nth bit is set, then the cdev and thermal zone are bound
> > > > diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> > > > index 692f4053f08b..5cd6ff1d0a4c 100644
> > > > --- a/drivers/thermal/fair_share.c
> > > > +++ b/drivers/thermal/fair_share.c
> > > > @@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
> > > >  }
> > > >  
> > > >  static long get_target_state(struct thermal_zone_device *tz,
> > > > -		struct thermal_cooling_device *cdev, int weight, int level)
> > > > +		struct thermal_cooling_device *cdev, int percentage, int level)
> > > >  {
> > > >  	unsigned long max_state;
> > > >  
> > > >  	cdev->ops->get_max_state(cdev, &max_state);
> > > >  
> > > > -	return (long)(weight * level * max_state) / (100 * tz->trips);
> > > > +	return (long)(percentage * level * max_state) / (100 * tz->trips);
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
> > > >   *
> > > >   * Parameters used for Throttling:
> > > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > > - * P2. weight[i]/100:
> > > > + * P2. percentage[i]/100:
> > > >   *	How 'effective' the 'i'th device is, in cooling the given zone.
> > > >   * P3. cur_trip_level/max_no_of_trips:
> > > >   *	This describes the extent to which the devices should be throttled.
> > > > @@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
> > > >  static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
> > > >  {
> > > >  	struct thermal_instance *instance;
> > > > +	int total_weight = 0;
> > > >  	int cur_trip_level = get_trip_level(tz);
> > > >  
> > > >  	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > +		if (instance->trip != trip)
> > > > +			continue;
> > > > +
> > > > +		total_weight += instance->weight;
> > > > +	}
> > > > +
> > > > +	if (!total_weight)
> > > > +		return -EINVAL;
> > > > +
> > > > +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > +		int percentage;
> > > >  		struct thermal_cooling_device *cdev = instance->cdev;
> > > >  
> > > >  		if (instance->trip != trip)
> > > >  			continue;
> > > >  
> > > > -		instance->target = get_target_state(tz, cdev,
> > > > -					instance->weight, cur_trip_level);
> > > > +		percentage = (instance->weight * 100) / total_weight;
> > > > +		instance->target = get_target_state(tz, cdev, percentage,
> > > > +						    cur_trip_level);
> > > >  
> > > >  		instance->cdev->updated = false;
> > > >  		thermal_cdev_update(cdev);
> > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > > index 2ed7062fac1d..2426426731ca 100644
> > > > --- a/include/linux/thermal.h
> > > > +++ b/include/linux/thermal.h
> > > > @@ -40,8 +40,12 @@
> > > >  /* No upper/lower limit requirement */
> > > >  #define THERMAL_NO_LIMIT	((u32)~0)
> > > >  
> > > > -/* Default weight of a bound cooling device */
> > > > -#define THERMAL_WEIGHT_DEFAULT 0
> > > > +/*
> > > > + * Default weight of a bound cooling device.  By setting it to 1024 we
> > > > + * give developers a range so that they can specify cooling devices
> > > > + * that are less or more "influential" than the default
> > > > + */
> > > > +#define THERMAL_WEIGHT_DEFAULT 1024
> > > >  
> > > >  /* Unit conversion macros */
> > > >  #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > > > @@ -217,9 +221,12 @@ struct thermal_bind_params {
> > > >  
> > > >  	/*
> > > >  	 * This is a measure of 'how effectively these devices can
> > > > -	 * cool 'this' thermal zone. The shall be determined by platform
> > > > -	 * characterization. This is on a 'percentage' scale.
> > > > -	 * See Documentation/thermal/sysfs-api.txt for more information.
> > > > +	 * cool 'this' thermal zone. It shall be determined by
> > > > +	 * platform characterization. This value is relative to the
> > > > +	 * rest of the weights so a cooling device whose weight is
> > > > +	 * double that of another cooling device is twice as
> > > > +	 * effective. See Documentation/thermal/sysfs-api.txt for more
> > > > +	 * information.
> > > >  	 */
> > > >  	int weight;
> > > >  
> > > 
> > > 
> > > 
> 
> 
> 

Re: [PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Zhang Rui]

On Fri, 2015-02-06 at 14:43 +0000, Javi Merino wrote:
> Hi Rui,
> 
> On Fri, Feb 06, 2015 at 02:06:32PM +0000, Zhang Rui wrote:
> > On Fri, 2015-02-06 at 12:42 +0000, Javi Merino wrote:
> > > On Fri, Feb 06, 2015 at 09:29:54AM +0000, Zhang Rui wrote:
> > > > On Mon, 2015-02-02 at 12:51 +0000, Javi Merino wrote:
> > > > > The fair share governor has the concept of weights, which is the
> > > > > influence of each cooling device in a thermal zone.  The current
> > > > > implementation forces the weights of all cooling devices in a thermal
> > > > > zone to add up to a 100.  This complicates setups, as you need to know
> > > > > in advance how many cooling devices you are going to have.  If you bind a
> > > > > new cooling device, you have to modify all the other cooling devices
> > > > > weights, which is error prone.  Furthermore, you can't specify a
> > > > > "default" weight for platforms since that default value depends on the
> > > > > number of cooling devices in the platform.
> > > > > 
> > > > > This patch generalizes the concept of weight by allowing any number to
> > > > > be a "weight".  Weights are now relative to each other.  Platforms that
> > > > > don't specify weights get the same default value for all their cooling
> > > > > devices, so all their cdevs are considered to be equally influential.
> > > > > 
> > > > I don't think we have covered these two cases.
> > > > 
> > > > First of all. for the thermal zones that bind using tz->tbp, and all the
> > > > weights equals 0, it actually means THERMAL_WEIGHT_DEFAULT instead of
> > > > zero.
> > > > IMO, I think we should 
> > > > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > > > 2. remove
> > > > 	if (!total_weight)
> > > > 		return -EINVAL;
> > > > 3. when calculating percentage,
> > > > 	if (!total_weight)
> > > > 		percentage = 100 / total_instances_of_the_current_trip;
> > > > 
> > > > Second, for the thermal zones that don't specify the weight for all of
> > > > its cooling devices. In this case, I think only the cooling devices with
> > > > weight should be used. Thus, we should
> > > > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > > > 2. for cooling devices do not have weight specified, we don't need to
> > > > change any code and its percentage is zero.
> > > > 
> > > > In all, I'd prefer to remove the change for THERMAL_WEIGHT_DEFAULT, and
> > > > change fair_share_throttle() as below, what do you think?
> > > 
> > > I think it makes it unnecessarily complicated as the default weight
> > > will now have two different behaviours:
> > > 
> > > 1. Cooling device not active if any devices have a weight and you are
> > >    using the fair_share governor.
> > > 2. Cooling device active if no other device have weights.
> > > 
> > > It's hard to document. It should be either one or the other.  The
> > > current implementation in this series make the cdevs do option 2
> > > always.
> > 
> > I just want to make thermal core intelligent and simplify the platform
> > thermal driver. Because in most cases, an uninitialized value (zero)
> > means the driver does not care and it wants to follow the default
> > behavior.
> > With your change in patch 5/5, for a driver that uses tz->tzp->tbp
> > directly (not the of_thermal way), in order to use fair_share governor,
> > it needs to set tbp[i].weight to THERMAL_WEIGHT_DEFAULT explicitly, like
> > you do in of_thermal.c in patch 1/5, right?
> 
> Right.  You have a valid point, not specifying anything should give you the
> default behavior.  THERMAL_WEIGHT_DEFAULT must stay 0.
> 
> Now, I'm still a bit uncomfortable with your proposal.  Imagine that a
> platform has three cooling devices with default weight (0).  That
> means that each would be getting 33%.  The developer sees that and
> changes one of the cooling devices' weight to 50.  That has the
> unintuitive effect of disabling the other two cooling devices.
> 
If the developer wants to customize the weight, it means he/she wants to
specify the proper weight for every cooling devices, rather than setting
one to 50 and leaving the others 0, right?

thanks,
rui
> Should we just say weight=0 is "cooling device disabled for the
> fair_share governor".
> 
> > CC Durgadoss, the author of the fair_share governor.
> 
> I forgot to add him because scripts/get_maintainer.pl didn't show
> him.  I'll add him to the patches that touch the fair_share governor
> from now on.
> 
> Cheers,
> Javi
> 
> > > > @@ -77,7 +77,7 @@ static long get_target_state(struct
> > > > thermal_zone_device *tz,
> > > >   *
> > > >   * Parameters used for Throttling:
> > > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > > - * P2. weight[i]/100:
> > > > + * P2. percentage[i]/100:
> > > >   *     How 'effective' the 'i'th device is, in cooling the given zone.
> > > >   * P3. cur_trip_level/max_no_of_trips:
> > > >   *     This describes the extent to which the devices should be
> > > > throttled.
> > > > @@ -89,16 +89,31 @@ static long get_target_state(struct
> > > > thermal_zone_device *tz,
> > > >  static int fair_share_throttle(struct thermal_zone_device *tz, int
> > > > trip)
> > > >  {
> > > >         struct thermal_instance *instance;
> > > > +       int total_weight = 0;
> > > > +       int total_instance = 0;
> > > >         int cur_trip_level = get_trip_level(tz);
> > > >  
> > > >         list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > +               if (instance->trip != trip)
> > > > +                       continue;
> > > > +
> > > > +               total_weight += instance->weight;
> > > > +               total_instance++;
> > > > +       }
> > > > +
> > > > +       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > +               int percentage;
> > > >                 struct thermal_cooling_device *cdev = instance->cdev;
> > > >  
> > > >                 if (instance->trip != trip)
> > > >                         continue;
> > > >  
> > > > -               instance->target = get_target_state(tz, cdev,
> > > > -                                       instance->weight,
> > > > cur_trip_level);
> > > > +               if (!total_weight)
> > > > +                       percentage = 100 / total_instance;
> > > > +               else
> > > > +                       percentage = (instance->weight * 100) /
> > > > total_weight;
> > > > +               instance->target = get_target_state(tz, cdev,
> > > > percentage,
> > > > +                                                   cur_trip_level);
> > > >  
> > > >                 instance->cdev->updated = false;
> > > >                 thermal_cdev_update(cdev);
> > > > 
> > > > 
> > > > > It's important to note that previous users of the weights don't need to
> > > > > alter the code: percentages continue to work as they used to.  This
> > > > > patch just removes the constraint of all the weights in a thermal zone
> > > > > having to add up to a 100.  If they do, you get the same behavior as
> > > > > before.  If they don't, fair share now works for that platform.
> > > > > 
> > > > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > > > Cc: Eduardo Valentin <edubezval@gmail.com>
> > > > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > > > ---
> > > > >  Documentation/thermal/sysfs-api.txt |  8 +++++---
> > > > >  drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
> > > > >  include/linux/thermal.h             | 17 ++++++++++++-----
> > > > >  3 files changed, 35 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > > > > index 3625453ceef6..c9fd014f0c21 100644
> > > > > --- a/Documentation/thermal/sysfs-api.txt
> > > > > +++ b/Documentation/thermal/sysfs-api.txt
> > > > > @@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
> > > > >      This structure defines the following parameters that are used to bind
> > > > >      a zone with a cooling device for a particular trip point.
> > > > >      .cdev: The cooling device pointer
> > > > > -    .weight: The 'influence' of a particular cooling device on this zone.
> > > > > -             This is on a percentage scale. The sum of all these weights
> > > > > -             (for a particular zone) cannot exceed 100.
> > > > > +    .weight: The 'influence' of a particular cooling device on this
> > > > > +             zone. This is relative to the rest of the cooling
> > > > > +             devices. For example, if all cooling devices have a
> > > > > +             weight of 1, then they all contribute the same. You can
> > > > > +             use percentages if you want, but it's not mandatory.
> > > > >      .trip_mask:This is a bit mask that gives the binding relation between
> > > > >                 this thermal zone and cdev, for a particular trip point.
> > > > >                 If nth bit is set, then the cdev and thermal zone are bound
> > > > > diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> > > > > index 692f4053f08b..5cd6ff1d0a4c 100644
> > > > > --- a/drivers/thermal/fair_share.c
> > > > > +++ b/drivers/thermal/fair_share.c
> > > > > @@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
> > > > >  }
> > > > >  
> > > > >  static long get_target_state(struct thermal_zone_device *tz,
> > > > > -		struct thermal_cooling_device *cdev, int weight, int level)
> > > > > +		struct thermal_cooling_device *cdev, int percentage, int level)
> > > > >  {
> > > > >  	unsigned long max_state;
> > > > >  
> > > > >  	cdev->ops->get_max_state(cdev, &max_state);
> > > > >  
> > > > > -	return (long)(weight * level * max_state) / (100 * tz->trips);
> > > > > +	return (long)(percentage * level * max_state) / (100 * tz->trips);
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > @@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
> > > > >   *
> > > > >   * Parameters used for Throttling:
> > > > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > > > - * P2. weight[i]/100:
> > > > > + * P2. percentage[i]/100:
> > > > >   *	How 'effective' the 'i'th device is, in cooling the given zone.
> > > > >   * P3. cur_trip_level/max_no_of_trips:
> > > > >   *	This describes the extent to which the devices should be throttled.
> > > > > @@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
> > > > >  static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
> > > > >  {
> > > > >  	struct thermal_instance *instance;
> > > > > +	int total_weight = 0;
> > > > >  	int cur_trip_level = get_trip_level(tz);
> > > > >  
> > > > >  	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > > +		if (instance->trip != trip)
> > > > > +			continue;
> > > > > +
> > > > > +		total_weight += instance->weight;
> > > > > +	}
> > > > > +
> > > > > +	if (!total_weight)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > > +		int percentage;
> > > > >  		struct thermal_cooling_device *cdev = instance->cdev;
> > > > >  
> > > > >  		if (instance->trip != trip)
> > > > >  			continue;
> > > > >  
> > > > > -		instance->target = get_target_state(tz, cdev,
> > > > > -					instance->weight, cur_trip_level);
> > > > > +		percentage = (instance->weight * 100) / total_weight;
> > > > > +		instance->target = get_target_state(tz, cdev, percentage,
> > > > > +						    cur_trip_level);
> > > > >  
> > > > >  		instance->cdev->updated = false;
> > > > >  		thermal_cdev_update(cdev);
> > > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > > > index 2ed7062fac1d..2426426731ca 100644
> > > > > --- a/include/linux/thermal.h
> > > > > +++ b/include/linux/thermal.h
> > > > > @@ -40,8 +40,12 @@
> > > > >  /* No upper/lower limit requirement */
> > > > >  #define THERMAL_NO_LIMIT	((u32)~0)
> > > > >  
> > > > > -/* Default weight of a bound cooling device */
> > > > > -#define THERMAL_WEIGHT_DEFAULT 0
> > > > > +/*
> > > > > + * Default weight of a bound cooling device.  By setting it to 1024 we
> > > > > + * give developers a range so that they can specify cooling devices
> > > > > + * that are less or more "influential" than the default
> > > > > + */
> > > > > +#define THERMAL_WEIGHT_DEFAULT 1024
> > > > >  
> > > > >  /* Unit conversion macros */
> > > > >  #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > > > > @@ -217,9 +221,12 @@ struct thermal_bind_params {
> > > > >  
> > > > >  	/*
> > > > >  	 * This is a measure of 'how effectively these devices can
> > > > > -	 * cool 'this' thermal zone. The shall be determined by platform
> > > > > -	 * characterization. This is on a 'percentage' scale.
> > > > > -	 * See Documentation/thermal/sysfs-api.txt for more information.
> > > > > +	 * cool 'this' thermal zone. It shall be determined by
> > > > > +	 * platform characterization. This value is relative to the
> > > > > +	 * rest of the weights so a cooling device whose weight is
> > > > > +	 * double that of another cooling device is twice as
> > > > > +	 * effective. See Documentation/thermal/sysfs-api.txt for more
> > > > > +	 * information.
> > > > >  	 */
> > > > >  	int weight;
> > > > >  
> > > > 
> > > > 
> > > > 
> > 
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] thermal: fair_share: generalize the weight concept

[Javi Merino]

On Sat, Feb 07, 2015 at 01:57:25AM +0000, Zhang Rui wrote:
> On Fri, 2015-02-06 at 14:43 +0000, Javi Merino wrote:
> > Hi Rui,
> >
> > On Fri, Feb 06, 2015 at 02:06:32PM +0000, Zhang Rui wrote:
> > > On Fri, 2015-02-06 at 12:42 +0000, Javi Merino wrote:
> > > > On Fri, Feb 06, 2015 at 09:29:54AM +0000, Zhang Rui wrote:
> > > > > On Mon, 2015-02-02 at 12:51 +0000, Javi Merino wrote:
> > > > > > The fair share governor has the concept of weights, which is the
> > > > > > influence of each cooling device in a thermal zone.  The current
> > > > > > implementation forces the weights of all cooling devices in a thermal
> > > > > > zone to add up to a 100.  This complicates setups, as you need to know
> > > > > > in advance how many cooling devices you are going to have.  If you bind a
> > > > > > new cooling device, you have to modify all the other cooling devices
> > > > > > weights, which is error prone.  Furthermore, you can't specify a
> > > > > > "default" weight for platforms since that default value depends on the
> > > > > > number of cooling devices in the platform.
> > > > > >
> > > > > > This patch generalizes the concept of weight by allowing any number to
> > > > > > be a "weight".  Weights are now relative to each other.  Platforms that
> > > > > > don't specify weights get the same default value for all their cooling
> > > > > > devices, so all their cdevs are considered to be equally influential.
> > > > > >
> > > > > I don't think we have covered these two cases.
> > > > >
> > > > > First of all. for the thermal zones that bind using tz->tbp, and all the
> > > > > weights equals 0, it actually means THERMAL_WEIGHT_DEFAULT instead of
> > > > > zero.
> > > > > IMO, I think we should
> > > > > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > > > > 2. remove
> > > > >         if (!total_weight)
> > > > >                 return -EINVAL;
> > > > > 3. when calculating percentage,
> > > > >         if (!total_weight)
> > > > >                 percentage = 100 / total_instances_of_the_current_trip;
> > > > >
> > > > > Second, for the thermal zones that don't specify the weight for all of
> > > > > its cooling devices. In this case, I think only the cooling devices with
> > > > > weight should be used. Thus, we should
> > > > > 1. leave THERMAL_WEIGHT_DEFAULT as 0
> > > > > 2. for cooling devices do not have weight specified, we don't need to
> > > > > change any code and its percentage is zero.
> > > > >
> > > > > In all, I'd prefer to remove the change for THERMAL_WEIGHT_DEFAULT, and
> > > > > change fair_share_throttle() as below, what do you think?
> > > >
> > > > I think it makes it unnecessarily complicated as the default weight
> > > > will now have two different behaviours:
> > > >
> > > > 1. Cooling device not active if any devices have a weight and you are
> > > >    using the fair_share governor.
> > > > 2. Cooling device active if no other device have weights.
> > > >
> > > > It's hard to document. It should be either one or the other.  The
> > > > current implementation in this series make the cdevs do option 2
> > > > always.
> > >
> > > I just want to make thermal core intelligent and simplify the platform
> > > thermal driver. Because in most cases, an uninitialized value (zero)
> > > means the driver does not care and it wants to follow the default
> > > behavior.
> > > With your change in patch 5/5, for a driver that uses tz->tzp->tbp
> > > directly (not the of_thermal way), in order to use fair_share governor,
> > > it needs to set tbp[i].weight to THERMAL_WEIGHT_DEFAULT explicitly, like
> > > you do in of_thermal.c in patch 1/5, right?
> >
> > Right.  You have a valid point, not specifying anything should give you the
> > default behavior.  THERMAL_WEIGHT_DEFAULT must stay 0.
> >
> > Now, I'm still a bit uncomfortable with your proposal.  Imagine that a
> > platform has three cooling devices with default weight (0).  That
> > means that each would be getting 33%.  The developer sees that and
> > changes one of the cooling devices' weight to 50.  That has the
> > unintuitive effect of disabling the other two cooling devices.
> >
> If the developer wants to customize the weight, it means he/she wants to
> specify the proper weight for every cooling devices, rather than setting
> one to 50 and leaving the others 0, right?

Ok, I think that we have both stated our points of view regarding this
and this discussion is going nowhere.  At the end of the day the
patches won't be accepted unless you are happy with them and I don't
think rephrasing what I've already said will help in any way.

I'll send a v3 implementing your proposed changes next week. 

Cheers,
Javi

> > Should we just say weight=0 is "cooling device disabled for the
> > fair_share governor".
> >
> > > CC Durgadoss, the author of the fair_share governor.
> >
> > I forgot to add him because scripts/get_maintainer.pl didn't show
> > him.  I'll add him to the patches that touch the fair_share governor
> > from now on.
> >
> > Cheers,
> > Javi
> >
> > > > > @@ -77,7 +77,7 @@ static long get_target_state(struct
> > > > > thermal_zone_device *tz,
> > > > >   *
> > > > >   * Parameters used for Throttling:
> > > > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > > > - * P2. weight[i]/100:
> > > > > + * P2. percentage[i]/100:
> > > > >   *     How 'effective' the 'i'th device is, in cooling the given zone.
> > > > >   * P3. cur_trip_level/max_no_of_trips:
> > > > >   *     This describes the extent to which the devices should be
> > > > > throttled.
> > > > > @@ -89,16 +89,31 @@ static long get_target_state(struct
> > > > > thermal_zone_device *tz,
> > > > >  static int fair_share_throttle(struct thermal_zone_device *tz, int
> > > > > trip)
> > > > >  {
> > > > >         struct thermal_instance *instance;
> > > > > +       int total_weight = 0;
> > > > > +       int total_instance = 0;
> > > > >         int cur_trip_level = get_trip_level(tz);
> > > > >
> > > > >         list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > > +               if (instance->trip != trip)
> > > > > +                       continue;
> > > > > +
> > > > > +               total_weight += instance->weight;
> > > > > +               total_instance++;
> > > > > +       }
> > > > > +
> > > > > +       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > > +               int percentage;
> > > > >                 struct thermal_cooling_device *cdev = instance->cdev;
> > > > >
> > > > >                 if (instance->trip != trip)
> > > > >                         continue;
> > > > >
> > > > > -               instance->target = get_target_state(tz, cdev,
> > > > > -                                       instance->weight,
> > > > > cur_trip_level);
> > > > > +               if (!total_weight)
> > > > > +                       percentage = 100 / total_instance;
> > > > > +               else
> > > > > +                       percentage = (instance->weight * 100) /
> > > > > total_weight;
> > > > > +               instance->target = get_target_state(tz, cdev,
> > > > > percentage,
> > > > > +                                                   cur_trip_level);
> > > > >
> > > > >                 instance->cdev->updated = false;
> > > > >                 thermal_cdev_update(cdev);
> > > > >
> > > > >
> > > > > > It's important to note that previous users of the weights don't need to
> > > > > > alter the code: percentages continue to work as they used to.  This
> > > > > > patch just removes the constraint of all the weights in a thermal zone
> > > > > > having to add up to a 100.  If they do, you get the same behavior as
> > > > > > before.  If they don't, fair share now works for that platform.
> > > > > >
> > > > > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > > > > Cc: Eduardo Valentin <edubezval@gmail.com>
> > > > > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > > > > ---
> > > > > >  Documentation/thermal/sysfs-api.txt |  8 +++++---
> > > > > >  drivers/thermal/fair_share.c        | 23 ++++++++++++++++++-----
> > > > > >  include/linux/thermal.h             | 17 ++++++++++++-----
> > > > > >  3 files changed, 35 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > > > > > index 3625453ceef6..c9fd014f0c21 100644
> > > > > > --- a/Documentation/thermal/sysfs-api.txt
> > > > > > +++ b/Documentation/thermal/sysfs-api.txt
> > > > > > @@ -129,9 +129,11 @@ temperature) and throttle appropriate devices.
> > > > > >      This structure defines the following parameters that are used to bind
> > > > > >      a zone with a cooling device for a particular trip point.
> > > > > >      .cdev: The cooling device pointer
> > > > > > -    .weight: The 'influence' of a particular cooling device on this zone.
> > > > > > -             This is on a percentage scale. The sum of all these weights
> > > > > > -             (for a particular zone) cannot exceed 100.
> > > > > > +    .weight: The 'influence' of a particular cooling device on this
> > > > > > +             zone. This is relative to the rest of the cooling
> > > > > > +             devices. For example, if all cooling devices have a
> > > > > > +             weight of 1, then they all contribute the same. You can
> > > > > > +             use percentages if you want, but it's not mandatory.
> > > > > >      .trip_mask:This is a bit mask that gives the binding relation between
> > > > > >                 this thermal zone and cdev, for a particular trip point.
> > > > > >                 If nth bit is set, then the cdev and thermal zone are bound
> > > > > > diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> > > > > > index 692f4053f08b..5cd6ff1d0a4c 100644
> > > > > > --- a/drivers/thermal/fair_share.c
> > > > > > +++ b/drivers/thermal/fair_share.c
> > > > > > @@ -59,13 +59,13 @@ static int get_trip_level(struct thermal_zone_device *tz)
> > > > > >  }
> > > > > >
> > > > > >  static long get_target_state(struct thermal_zone_device *tz,
> > > > > > -             struct thermal_cooling_device *cdev, int weight, int level)
> > > > > > +             struct thermal_cooling_device *cdev, int percentage, int level)
> > > > > >  {
> > > > > >       unsigned long max_state;
> > > > > >
> > > > > >       cdev->ops->get_max_state(cdev, &max_state);
> > > > > >
> > > > > > -     return (long)(weight * level * max_state) / (100 * tz->trips);
> > > > > > +     return (long)(percentage * level * max_state) / (100 * tz->trips);
> > > > > >  }
> > > > > >
> > > > > >  /**
> > > > > > @@ -77,7 +77,7 @@ static long get_target_state(struct thermal_zone_device *tz,
> > > > > >   *
> > > > > >   * Parameters used for Throttling:
> > > > > >   * P1. max_state: Maximum throttle state exposed by the cooling device.
> > > > > > - * P2. weight[i]/100:
> > > > > > + * P2. percentage[i]/100:
> > > > > >   *   How 'effective' the 'i'th device is, in cooling the given zone.
> > > > > >   * P3. cur_trip_level/max_no_of_trips:
> > > > > >   *   This describes the extent to which the devices should be throttled.
> > > > > > @@ -89,16 +89,29 @@ static long get_target_state(struct thermal_zone_device *tz,
> > > > > >  static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
> > > > > >  {
> > > > > >       struct thermal_instance *instance;
> > > > > > +     int total_weight = 0;
> > > > > >       int cur_trip_level = get_trip_level(tz);
> > > > > >
> > > > > >       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > > > +             if (instance->trip != trip)
> > > > > > +                     continue;
> > > > > > +
> > > > > > +             total_weight += instance->weight;
> > > > > > +     }
> > > > > > +
> > > > > > +     if (!total_weight)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > > > > > +             int percentage;
> > > > > >               struct thermal_cooling_device *cdev = instance->cdev;
> > > > > >
> > > > > >               if (instance->trip != trip)
> > > > > >                       continue;
> > > > > >
> > > > > > -             instance->target = get_target_state(tz, cdev,
> > > > > > -                                     instance->weight, cur_trip_level);
> > > > > > +             percentage = (instance->weight * 100) / total_weight;
> > > > > > +             instance->target = get_target_state(tz, cdev, percentage,
> > > > > > +                                                 cur_trip_level);
> > > > > >
> > > > > >               instance->cdev->updated = false;
> > > > > >               thermal_cdev_update(cdev);
> > > > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > > > > index 2ed7062fac1d..2426426731ca 100644
> > > > > > --- a/include/linux/thermal.h
> > > > > > +++ b/include/linux/thermal.h
> > > > > > @@ -40,8 +40,12 @@
> > > > > >  /* No upper/lower limit requirement */
> > > > > >  #define THERMAL_NO_LIMIT     ((u32)~0)
> > > > > >
> > > > > > -/* Default weight of a bound cooling device */
> > > > > > -#define THERMAL_WEIGHT_DEFAULT 0
> > > > > > +/*
> > > > > > + * Default weight of a bound cooling device.  By setting it to 1024 we
> > > > > > + * give developers a range so that they can specify cooling devices
> > > > > > + * that are less or more "influential" than the default
> > > > > > + */
> > > > > > +#define THERMAL_WEIGHT_DEFAULT 1024
> > > > > >
> > > > > >  /* Unit conversion macros */
> > > > > >  #define KELVIN_TO_CELSIUS(t) (long)(((long)t-2732 >= 0) ?    \
> > > > > > @@ -217,9 +221,12 @@ struct thermal_bind_params {
> > > > > >
> > > > > >       /*
> > > > > >        * This is a measure of 'how effectively these devices can
> > > > > > -      * cool 'this' thermal zone. The shall be determined by platform
> > > > > > -      * characterization. This is on a 'percentage' scale.
> > > > > > -      * See Documentation/thermal/sysfs-api.txt for more information.
> > > > > > +      * cool 'this' thermal zone. It shall be determined by
> > > > > > +      * platform characterization. This value is relative to the
> > > > > > +      * rest of the weights so a cooling device whose weight is
> > > > > > +      * double that of another cooling device is twice as
> > > > > > +      * effective. See Documentation/thermal/sysfs-api.txt for more
> > > > > > +      * information.
> > > > > >        */
> > > > > >       int weight;
> > > > > >
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
>