[PATCH v3 0/4] Hierarchical thermal zones

[PATCH v3 0/4] Hierarchical thermal zones

[Javi Merino]

This series adds the ability to create a hierarchy of thermal zones.
Thermal zones created via platform code or device tree can be set up
to calculate their temperature as the maximum or weighted average of
all its underlying thermal zones.  This came up from discussions
during LPC.

The first patch adds the basic support to thermal core.  Patch 2
extends the devicetree bindings to cope with a hierarchy of thermal
zones.  Patch 3 adds device tree support.  The last patch exports the
hierarchy to sysfs, adding knobs to change the aggregation function
and adjust the weights of thermal zones.

Changes since v2:
  - The aggregation function can be maximum or weighted average
  - Separated the update of the devicetree binding and the of-thermal
    into two separate patches

Javi Merino (4):
  thermal: Add support for hierarchical thermal zones
  devicetree: bindings: let thermal-sensor point to other thermal zones
  thermal: of: parse stacked thermal zones from device tree
  thermal: show the sub thermal zones in sysfs

 .../devicetree/bindings/thermal/thermal.txt        | 154 +++++++++-
 Documentation/thermal/sysfs-api.txt                |  72 +++++
 drivers/thermal/of-thermal.c                       | 100 ++++++
 drivers/thermal/thermal_core.c                     | 335 ++++++++++++++++++++-
 include/linux/thermal.h                            |  44 ++-
 5 files changed, 696 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH v3 1/4] thermal: Add support for hierarchical thermal zones

[Javi Merino]

Add the ability to stack thermal zones on top of each other, creating a
hierarchy of thermal zones.

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 |  39 +++++++
 drivers/thermal/thermal_core.c      | 210 +++++++++++++++++++++++++++++++++++-
 include/linux/thermal.h             |  41 ++++++-
 3 files changed, 284 insertions(+), 6 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 10f062ea6bc2..dda24a0bdeec 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -72,6 +72,45 @@ temperature) and throttle appropriate devices.
     It deletes the corresponding entry form /sys/class/thermal folder and
     unbind all the thermal cooling devices it uses.
 
+
+1.1.3 int thermal_zone_add_subtz(struct thermal_zone_device *tz,
+                                 struct thermal_zone_device *subtz)
+
+    Add subtz to the list of slaves of tz.  This lets you create a
+    hierarchy of thermal zones.  The hierarchy must be a Direct
+    Acyclic Graph (DAG).  If a loop is detected,
+    thermal_zone_get_temp() will return -EDEADLK to prevent the
+    deadlock.  thermal_zone_add_subtz() does not affect subtz.
+
+    When calculating the temperature of thermal zone tz, report it as
+    an aggregation of the temperatures of the sub thermal zones.  For
+    details of the aggregation function see the enum
+    thermal_aggregation_function.
+
+    For example, if you have an SoC with a thermal sensor in each cpu
+    of the two cpus you could have a thermal zone to monitor each
+    sensor, let's call them cpu0_tz and cpu1_tz.  You could then have a
+    a SoC thermal zone (soc_tz) without a get_temp() op which can be
+    configured like this:
+
+    thermal_zone_add_subtz(soc_tz, cpu0_tz);
+    thermal_zone_add_subtz(soc_tz, cpu1_tz);
+
+    Now soc_tz's temperature is the aggregation of cpu0_tz and
+    cpu1_tz.
+
+
+1.1.4 int thermal_zone_del_subtz(struct thermal_zone_device *tz,
+                                 struct thermal_zone_device *subtz)
+
+    Delete subtz from the slave thermal zones of tz.  This basically
+    reverses what thermal_zone_add_subtz() does.  If tz still has
+    other slave thermal zones, its temperature would be the
+    aggregation of the remaining slave thermal zones.  If tz no longer
+    has slave thermal zones, thermal_zone_get_temp() will return
+    -EINVAL.
+
+
 1.2 thermal cooling device interface
 1.2.1 struct thermal_cooling_device *thermal_cooling_device_register(char *name,
 		void *devdata, struct thermal_cooling_device_ops *)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525cc9c1c..98bdb63b3b95 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -61,6 +61,20 @@ static DEFINE_MUTEX(thermal_governor_lock);
 
 static struct thermal_governor *def_governor;
 
+/**
+ * struct thermal_zone_link - a link between "master" and "slave" thermal zones
+ * @weight:	weight of the @slave thermal zone in the master
+ *		calculation.  Used when the master thermal zone's
+ *		aggregation function is THERMAL_AGG_WEIGHTED_AVG
+ * @slave:	pointer to the slave thermal zone
+ * @node:	list node in the master thermal zone's slave_tzs list.
+ */
+struct thermal_zone_link {
+	int weight;
+	struct thermal_zone_device *slave;
+	struct list_head node;
+};
+
 static struct thermal_governor *__find_governor(const char *name)
 {
 	struct thermal_governor *pos;
@@ -465,6 +479,80 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 }
 
 /**
+ * get_subtz_temp() - get the aggregate temperature of all the sub thermal zones
+ * @tz:	thermal zone pointer
+ * @temp: pointer in which to store the result
+ *
+ * Go through all the thermal zones listed in @tz slave_tzs and
+ * calculate the aggregate temperature of all of them.  The result is
+ * stored in @temp.  The function used for aggregation is defined by
+ * the thermal zone's "slave_agg_function" parameter.  It can only be
+ * THERMAL_AGG_MAX or THERMAL_AGG_WEIGHTED_AVG.  If THERMAL_AGG_MAX is
+ * used then the @temp holds the maximum temperature of all slave
+ * thermal zones.  With THERMAL_AGG_WEIGHTED_AVG a weighted average of
+ * all temperatures is calculated.  Each thermal zone's temperature is
+ * multiplied by its weight and the result is divided by the sum of
+ * all weights.  If all weights are zero, the result is the average
+ * weight of the thermal zones.
+ *
+ * Return: 0 on success, -EINVAL if there are no slave thermal zones,
+ * -E* if thermal_zone_get_temp() fails for any of the slave thermal
+ * zones.
+ */
+static int get_subtz_temp(struct thermal_zone_device *tz, int *temp)
+{
+	struct thermal_zone_link *link;
+	int ret_temp, total_weight = 0;
+	bool all_weights_are_zero = false;
+
+	if (list_empty(&tz->slave_tzs))
+		return -EINVAL;
+
+	if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG) {
+		int total_instances = 0;
+
+		list_for_each_entry(link, &tz->slave_tzs, node) {
+			total_weight += link->weight;
+			total_instances++;
+		}
+
+		if (!total_weight) {
+			all_weights_are_zero = true;
+			total_weight = total_instances;
+		} else {
+			all_weights_are_zero = false;
+		}
+
+		ret_temp = 0;
+	} else if (tz->slave_agg_function == THERMAL_AGG_MAX) {
+		ret_temp = INT_MIN;
+	}
+
+	list_for_each_entry(link, &tz->slave_tzs, node) {
+		int this_temp, ret;
+
+		ret = thermal_zone_get_temp(link->slave, &this_temp);
+		if (ret)
+			return ret;
+
+		if (tz->slave_agg_function == THERMAL_AGG_MAX) {
+			if (this_temp > ret_temp)
+				ret_temp = this_temp;
+		} else if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG) {
+			int weight = all_weights_are_zero ? 1 : link->weight;
+
+			ret_temp += weight * this_temp;
+		}
+	}
+
+	if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG)
+		ret_temp /= total_weight;
+
+	*temp = ret_temp;
+	return 0;
+}
+
+/**
  * thermal_zone_get_temp() - returns the temperature of a thermal zone
  * @tz: a valid pointer to a struct thermal_zone_device
  * @temp: a valid pointer to where to store the resulting temperature.
@@ -481,11 +569,22 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
 	int crit_temp = INT_MAX;
 	enum thermal_trip_type type;
 
-	if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
+	if (!tz || IS_ERR(tz))
 		goto exit;
 
+	/* Avoid loops */
+	if (tz->slave_tz_visited)
+		return -EDEADLK;
+
 	mutex_lock(&tz->lock);
 
+	tz->slave_tz_visited = true;
+
+	if (!tz->ops->get_temp) {
+		ret = get_subtz_temp(tz, temp);
+		goto unlock;
+	}
+
 	ret = tz->ops->get_temp(tz, temp);
 
 	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
@@ -506,7 +605,9 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
 		if (!ret && *temp < crit_temp)
 			*temp = tz->emul_temperature;
 	}
- 
+
+unlock:
+	tz->slave_tz_visited = false;
 	mutex_unlock(&tz->lock);
 exit:
 	return ret;
@@ -540,9 +641,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 {
 	int count;
 
-	if (!tz->ops->get_temp)
-		return;
-
 	update_temperature(tz);
 
 	for (count = 0; count < tz->trips; count++)
@@ -1785,10 +1883,17 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp))
 		return ERR_PTR(-EINVAL);
 
+	if (tzp && (tzp->agg_func != THERMAL_AGG_MAX) &&
+	    (tzp->agg_func != THERMAL_AGG_WEIGHTED_AVG))
+		return ERR_PTR(-EINVAL);
+
 	tz = kzalloc(sizeof(struct thermal_zone_device), GFP_KERNEL);
 	if (!tz)
 		return ERR_PTR(-ENOMEM);
 
+	if (tzp)
+		tz->slave_agg_function = tzp->agg_func;
+	INIT_LIST_HEAD(&tz->slave_tzs);
 	INIT_LIST_HEAD(&tz->thermal_instances);
 	idr_init(&tz->idr);
 	mutex_init(&tz->lock);
@@ -1921,6 +2026,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	const struct thermal_zone_params *tzp;
 	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *pos = NULL;
+	struct thermal_zone_link *link, *tmp;
 
 	if (!tz)
 		return;
@@ -1958,6 +2064,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 
 	mutex_unlock(&thermal_list_lock);
 
+	list_for_each_entry_safe(link, tmp, &tz->slave_tzs, node)
+		thermal_zone_del_subtz(tz, link->slave);
+
 	thermal_zone_device_set_polling(tz, 0);
 
 	if (tz->type[0])
@@ -1980,6 +2089,97 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
 
 /**
+ * thermal_zone_add_subtz() - add a sub thermal zone to a thermal zone
+ * @tz:	pointer to the master thermal zone
+ * @subtz: pointer to the slave thermal zone
+ * @weight: relative contribution of the sub thermal zone
+ *
+ * Add @subtz to the list of slave thermal zones of @tz.  If @tz
+ * doesn't provide a get_temp() op, its temperature will be calculated
+ * as a combination of the temperatures of its sub thermal zones.  The
+ * @weight is the relative contribution of this thermal zone when
+ * using THERMAL_AGG_WEIGHTED_AVG.  Set @weight to
+ * THERMAL_WEIGHT_DEFAULT for all sub thermal zones to do a simple
+ * average.  See get_sub_tz_temp() for more information on how the
+ * temperature for @tz is calculated.
+ *
+ * Return: 0 on success, -EINVAL if @tz or @subtz are not valid
+ * pointers, -EEXIST if the link already exists or -ENOMEM if we ran
+ * out of memory.
+ */
+int thermal_zone_add_subtz(struct thermal_zone_device *tz,
+			   struct thermal_zone_device *subtz, int weight)
+{
+	int ret;
+	struct thermal_zone_link *link;
+
+	if (IS_ERR_OR_NULL(tz) || IS_ERR_OR_NULL(subtz))
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+
+	list_for_each_entry(link, &tz->slave_tzs, node) {
+		if (link->slave == subtz) {
+			ret = -EEXIST;
+			goto unlock;
+		}
+	}
+
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	link->slave = subtz;
+	link->weight = weight;
+	list_add_tail(&link->node, &tz->slave_tzs);
+
+	ret = 0;
+
+unlock:
+	mutex_unlock(&tz->lock);
+
+	return ret;
+}
+
+/**
+ * thermal_zone_del_subtz() - delete a sub thermal zone from its master thermal zone
+ * @tz:	pointer to the master thermal zone
+ * @subtz: pointer to the slave thermal zone
+ *
+ * Remove @subtz from the list of slave thermal zones of @tz.
+ *
+ * Return: 0 on success, -EINVAL if either thermal is invalid or if
+ * @subtz is not a slave of @tz.
+ */
+int thermal_zone_del_subtz(struct thermal_zone_device *tz,
+			   struct thermal_zone_device *subtz)
+{
+	int ret = -EINVAL;
+	struct thermal_zone_link *link;
+
+	if (IS_ERR_OR_NULL(tz) || IS_ERR_OR_NULL(subtz))
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+
+	list_for_each_entry(link, &tz->slave_tzs, node) {
+		if (link->slave == subtz) {
+			list_del(&link->node);
+			kfree(link);
+
+			ret = 0;
+			break;
+		}
+	}
+
+	mutex_unlock(&tz->lock);
+
+	return ret;
+}
+
+/**
  * thermal_zone_get_zone_by_name() - search for a zone and returns its ref
  * @name: thermal zone name to fetch the temperature
  *
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 4014a59828fc..dc3d2ab77ab6 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -40,7 +40,7 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT	((u32)~0)
 
-/* Default weight of a bound cooling device */
+/* Default weight for cooling devices and sub thermal zones */
 #define THERMAL_WEIGHT_DEFAULT 0
 
 /* Unit conversion macros */
@@ -89,6 +89,18 @@ enum thermal_trend {
 	THERMAL_TREND_DROP_FULL, /* apply lowest cooling action */
 };
 
+/**
+ * enum thermal_aggregation_function - aggregation function for sub thermal zones
+ * @THERMAL_AGG_MAX:	calculate the maximum of all sub thermal zones
+
+ * @THERMAL_AGG_WEIGHTED_AVG:	calculate the weighted average of all
+ *				sub thermal zones
+ */
+enum thermal_aggregation_function {
+	THERMAL_AGG_MAX,
+	THERMAL_AGG_WEIGHTED_AVG,
+};
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
@@ -170,6 +182,12 @@ struct thermal_attr {
  * @ops:	operations this &thermal_zone_device supports
  * @tzp:	thermal zone parameters
  * @governor:	pointer to the governor for this thermal zone
+ * @slave_agg_function: aggretion function to calculate the
+ *			temperature as a combination of the slave thermal zones
+ *			of this thermal zone.  See enum
+ *			thermal_aggregation_function for valid values
+ * @slave_tzs:	list of thermal zones that are a slave of this thermal zone
+ * @slave_tz_visited: used to detect loops in stacked thermal zones
  * @governor_data:	private pointer for governor data
  * @thermal_instances:	list of &struct thermal_instance of this thermal zone
  * @idr:	&struct idr to generate unique id for this zone's cooling
@@ -197,6 +215,9 @@ struct thermal_zone_device {
 	struct thermal_zone_device_ops *ops;
 	struct thermal_zone_params *tzp;
 	struct thermal_governor *governor;
+	enum thermal_aggregation_function slave_agg_function;
+	struct list_head slave_tzs;
+	bool slave_tz_visited;
 	void *governor_data;
 	struct list_head thermal_instances;
 	struct idr idr;
@@ -311,6 +332,13 @@ struct thermal_zone_params {
 	 * 		Used by thermal zone drivers (default 0).
 	 */
 	int offset;
+
+	/*
+	 * aggregation function to use when calculating the
+	 * temperature as a combination of multiple sub thermal
+	 * zones
+	 */
+	enum thermal_aggregation_function agg_func;
 };
 
 struct thermal_genl_event {
@@ -405,6 +433,10 @@ struct thermal_cooling_device *
 thermal_of_cooling_device_register(struct device_node *np, char *, void *,
 				   const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
+int thermal_zone_add_subtz(struct thermal_zone_device *,
+			   struct thermal_zone_device *, int);
+int thermal_zone_del_subtz(struct thermal_zone_device *,
+			   struct thermal_zone_device *);
 struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
 int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
 
@@ -457,6 +489,13 @@ thermal_of_cooling_device_register(struct device_node *np,
 static inline void thermal_cooling_device_unregister(
 	struct thermal_cooling_device *cdev)
 { }
+static inline int thermal_zone_add_subtz(struct thermal_zone_device *tz,
+					 struct thermal_zone_device *subtz,
+					 int weight)
+{ return -ENODEV; }
+static inline int thermal_zone_del_subtz(struct thermal_zone_device *tz,
+					 struct thermal_zone_device *subtz)
+{ return -ENODEV; }
 static inline struct thermal_zone_device *thermal_zone_get_zone_by_name(
 		const char *name)
 { return ERR_PTR(-ENODEV); }
-- 
1.9.1

Re: [PATCH v3 1/4] thermal: Add support for hierarchical thermal zones

[Eduardo Valentin]

Thanks for moving this forward Javi,

Few comments inline.

On Wed, Nov 25, 2015 at 03:09:43PM +0000, Javi Merino wrote:
> Add the ability to stack thermal zones on top of each other, creating a
> hierarchy of thermal zones.
> 
> 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 |  39 +++++++
>  drivers/thermal/thermal_core.c      | 210 +++++++++++++++++++++++++++++++++++-
>  include/linux/thermal.h             |  41 ++++++-
>  3 files changed, 284 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 10f062ea6bc2..dda24a0bdeec 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -72,6 +72,45 @@ temperature) and throttle appropriate devices.
>      It deletes the corresponding entry form /sys/class/thermal folder and
>      unbind all the thermal cooling devices it uses.
>  
> +
> +1.1.3 int thermal_zone_add_subtz(struct thermal_zone_device *tz,
> +                                 struct thermal_zone_device *subtz)

This does not match the signature proposed below in the code. But hold
on, check my comments there too.

> +
> +    Add subtz to the list of slaves of tz.  This lets you create a
> +    hierarchy of thermal zones.  The hierarchy must be a Direct
> +    Acyclic Graph (DAG).  If a loop is detected,
> +    thermal_zone_get_temp() will return -EDEADLK to prevent the
> +    deadlock.  thermal_zone_add_subtz() does not affect subtz.
> +
> +    When calculating the temperature of thermal zone tz, report it as
> +    an aggregation of the temperatures of the sub thermal zones.  For
> +    details of the aggregation function see the enum
> +    thermal_aggregation_function.
> +
> +    For example, if you have an SoC with a thermal sensor in each cpu
> +    of the two cpus you could have a thermal zone to monitor each
> +    sensor, let's call them cpu0_tz and cpu1_tz.  You could then have a
> +    a SoC thermal zone (soc_tz) without a get_temp() op which can be
> +    configured like this:
> +
> +    thermal_zone_add_subtz(soc_tz, cpu0_tz);
> +    thermal_zone_add_subtz(soc_tz, cpu1_tz);
> +
> +    Now soc_tz's temperature is the aggregation of cpu0_tz and
> +    cpu1_tz.
> +
> +
> +1.1.4 int thermal_zone_del_subtz(struct thermal_zone_device *tz,
> +                                 struct thermal_zone_device *subtz)
> +
> +    Delete subtz from the slave thermal zones of tz.  This basically
> +    reverses what thermal_zone_add_subtz() does.  If tz still has
> +    other slave thermal zones, its temperature would be the
> +    aggregation of the remaining slave thermal zones.  If tz no longer
> +    has slave thermal zones, thermal_zone_get_temp() will return
> +    -EINVAL.
> +
> +
>  1.2 thermal cooling device interface
>  1.2.1 struct thermal_cooling_device *thermal_cooling_device_register(char *name,
>  		void *devdata, struct thermal_cooling_device_ops *)
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525cc9c1c..98bdb63b3b95 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -61,6 +61,20 @@ static DEFINE_MUTEX(thermal_governor_lock);
>  
>  static struct thermal_governor *def_governor;
>  
> +/**
> + * struct thermal_zone_link - a link between "master" and "slave" thermal zones
> + * @weight:	weight of the @slave thermal zone in the master
> + *		calculation.  Used when the master thermal zone's
> + *		aggregation function is THERMAL_AGG_WEIGHTED_AVG
> + * @slave:	pointer to the slave thermal zone
> + * @node:	list node in the master thermal zone's slave_tzs list.
> + */
> +struct thermal_zone_link {
> +	int weight;
> +	struct thermal_zone_device *slave;
> +	struct list_head node;
> +};
> +

How about this struct be in thermal_core.h?  I think we should support
also aggregation functions implemented by drivers, and therefore they
should have access to this data. Also they will probably need a
driver_data too.

>  static struct thermal_governor *__find_governor(const char *name)
>  {
>  	struct thermal_governor *pos;
> @@ -465,6 +479,80 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>  }
>  
>  /**
> + * get_subtz_temp() - get the aggregate temperature of all the sub thermal zones
> + * @tz:	thermal zone pointer
> + * @temp: pointer in which to store the result
> + *
> + * Go through all the thermal zones listed in @tz slave_tzs and
> + * calculate the aggregate temperature of all of them.  The result is
> + * stored in @temp.  The function used for aggregation is defined by
> + * the thermal zone's "slave_agg_function" parameter.  It can only be
> + * THERMAL_AGG_MAX or THERMAL_AGG_WEIGHTED_AVG.  If THERMAL_AGG_MAX is
> + * used then the @temp holds the maximum temperature of all slave
> + * thermal zones.  With THERMAL_AGG_WEIGHTED_AVG a weighted average of
> + * all temperatures is calculated.  Each thermal zone's temperature is
> + * multiplied by its weight and the result is divided by the sum of
> + * all weights.  If all weights are zero, the result is the average
> + * weight of the thermal zones.
> + *
> + * Return: 0 on success, -EINVAL if there are no slave thermal zones,
> + * -E* if thermal_zone_get_temp() fails for any of the slave thermal
> + * zones.
> + */
> +static int get_subtz_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct thermal_zone_link *link;
> +	int ret_temp, total_weight = 0;
> +	bool all_weights_are_zero = false;
> +
> +	if (list_empty(&tz->slave_tzs))
> +		return -EINVAL;
> +
> +	if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG) {
> +		int total_instances = 0;
> +
> +		list_for_each_entry(link, &tz->slave_tzs, node) {
> +			total_weight += link->weight;
> +			total_instances++;
> +		}
> +
> +		if (!total_weight) {
> +			all_weights_are_zero = true;
> +			total_weight = total_instances;
> +		} else {
> +			all_weights_are_zero = false;
> +		}
> +
> +		ret_temp = 0;
> +	} else if (tz->slave_agg_function == THERMAL_AGG_MAX) {
> +		ret_temp = INT_MIN;
> +	}
> +
> +	list_for_each_entry(link, &tz->slave_tzs, node) {
> +		int this_temp, ret;
> +
> +		ret = thermal_zone_get_temp(link->slave, &this_temp);
> +		if (ret)
> +			return ret;
> +
> +		if (tz->slave_agg_function == THERMAL_AGG_MAX) {
> +			if (this_temp > ret_temp)
> +				ret_temp = this_temp;
> +		} else if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG) {
> +			int weight = all_weights_are_zero ? 1 : link->weight;
> +
> +			ret_temp += weight * this_temp;
> +		}
> +	}
> +
> +	if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG)
> +		ret_temp /= total_weight;
> +
> +	*temp = ret_temp;
> +	return 0;
> +}
> +
> +/**
>   * thermal_zone_get_temp() - returns the temperature of a thermal zone
>   * @tz: a valid pointer to a struct thermal_zone_device
>   * @temp: a valid pointer to where to store the resulting temperature.
> @@ -481,11 +569,22 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>  	int crit_temp = INT_MAX;
>  	enum thermal_trip_type type;
>  
> -	if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
> +	if (!tz || IS_ERR(tz))
>  		goto exit;
>  
> +	/* Avoid loops */
> +	if (tz->slave_tz_visited)
> +		return -EDEADLK;
> +
>  	mutex_lock(&tz->lock);
>  
> +	tz->slave_tz_visited = true;
> +
> +	if (!tz->ops->get_temp) {
> +		ret = get_subtz_temp(tz, temp);
> +		goto unlock;
> +	}
> +

We probably need to revisit this. Are you able to set emulation
temperature in thermal zone that has aggregation functions?

>  	ret = tz->ops->get_temp(tz, temp);
>  
>  	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
> @@ -506,7 +605,9 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>  		if (!ret && *temp < crit_temp)
>  			*temp = tz->emul_temperature;
>  	}
> - 
> +
> +unlock:
> +	tz->slave_tz_visited = false;
>  	mutex_unlock(&tz->lock);
>  exit:
>  	return ret;
> @@ -540,9 +641,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  {
>  	int count;
>  
> -	if (!tz->ops->get_temp)
> -		return;
> -
>  	update_temperature(tz);
>  
>  	for (count = 0; count < tz->trips; count++)
> @@ -1785,10 +1883,17 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp))
>  		return ERR_PTR(-EINVAL);
>  
> +	if (tzp && (tzp->agg_func != THERMAL_AGG_MAX) &&
> +	    (tzp->agg_func != THERMAL_AGG_WEIGHTED_AVG))
> +		return ERR_PTR(-EINVAL);
> +
>  	tz = kzalloc(sizeof(struct thermal_zone_device), GFP_KERNEL);
>  	if (!tz)
>  		return ERR_PTR(-ENOMEM);
>  
> +	if (tzp)
> +		tz->slave_agg_function = tzp->agg_func;
> +	INIT_LIST_HEAD(&tz->slave_tzs);
>  	INIT_LIST_HEAD(&tz->thermal_instances);
>  	idr_init(&tz->idr);
>  	mutex_init(&tz->lock);
> @@ -1921,6 +2026,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  	const struct thermal_zone_params *tzp;
>  	struct thermal_cooling_device *cdev;
>  	struct thermal_zone_device *pos = NULL;
> +	struct thermal_zone_link *link, *tmp;
>  
>  	if (!tz)
>  		return;
> @@ -1958,6 +2064,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  
>  	mutex_unlock(&thermal_list_lock);
>  
> +	list_for_each_entry_safe(link, tmp, &tz->slave_tzs, node)
> +		thermal_zone_del_subtz(tz, link->slave);
> +
>  	thermal_zone_device_set_polling(tz, 0);
>  
>  	if (tz->type[0])
> @@ -1980,6 +2089,97 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>  
>  /**
> + * thermal_zone_add_subtz() - add a sub thermal zone to a thermal zone
> + * @tz:	pointer to the master thermal zone
> + * @subtz: pointer to the slave thermal zone
> + * @weight: relative contribution of the sub thermal zone
> + *
> + * Add @subtz to the list of slave thermal zones of @tz.  If @tz
> + * doesn't provide a get_temp() op, its temperature will be calculated
> + * as a combination of the temperatures of its sub thermal zones.  The
> + * @weight is the relative contribution of this thermal zone when
> + * using THERMAL_AGG_WEIGHTED_AVG.  Set @weight to
> + * THERMAL_WEIGHT_DEFAULT for all sub thermal zones to do a simple
> + * average.  See get_sub_tz_temp() for more information on how the
> + * temperature for @tz is calculated.
> + *
> + * Return: 0 on success, -EINVAL if @tz or @subtz are not valid
> + * pointers, -EEXIST if the link already exists or -ENOMEM if we ran
> + * out of memory.
> + */
> +int thermal_zone_add_subtz(struct thermal_zone_device *tz,
> +			   struct thermal_zone_device *subtz, int weight)
> +{
> +	int ret;
> +	struct thermal_zone_link *link;
> +
> +	if (IS_ERR_OR_NULL(tz) || IS_ERR_OR_NULL(subtz))
> +		return -EINVAL;
> +
> +	mutex_lock(&tz->lock);
> +
> +	list_for_each_entry(link, &tz->slave_tzs, node) {
> +		if (link->slave == subtz) {
> +			ret = -EEXIST;
> +			goto unlock;
> +		}
> +	}
> +
> +	link = kzalloc(sizeof(*link), GFP_KERNEL);
> +	if (!link) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	link->slave = subtz;
> +	link->weight = weight;
> +	list_add_tail(&link->node, &tz->slave_tzs);
> +
> +	ret = 0;
> +
> +unlock:
> +	mutex_unlock(&tz->lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * thermal_zone_del_subtz() - delete a sub thermal zone from its master thermal zone
> + * @tz:	pointer to the master thermal zone
> + * @subtz: pointer to the slave thermal zone
> + *
> + * Remove @subtz from the list of slave thermal zones of @tz.
> + *
> + * Return: 0 on success, -EINVAL if either thermal is invalid or if
> + * @subtz is not a slave of @tz.
> + */
> +int thermal_zone_del_subtz(struct thermal_zone_device *tz,
> +			   struct thermal_zone_device *subtz)
> +{
> +	int ret = -EINVAL;
> +	struct thermal_zone_link *link;
> +
> +	if (IS_ERR_OR_NULL(tz) || IS_ERR_OR_NULL(subtz))
> +		return -EINVAL;
> +
> +	mutex_lock(&tz->lock);
> +
> +	list_for_each_entry(link, &tz->slave_tzs, node) {
> +		if (link->slave == subtz) {
> +			list_del(&link->node);
> +			kfree(link);
> +
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&tz->lock);
> +
> +	return ret;
> +}
> +
> +/**
>   * thermal_zone_get_zone_by_name() - search for a zone and returns its ref
>   * @name: thermal zone name to fetch the temperature
>   *
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 4014a59828fc..dc3d2ab77ab6 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -40,7 +40,7 @@
>  /* No upper/lower limit requirement */
>  #define THERMAL_NO_LIMIT	((u32)~0)
>  
> -/* Default weight of a bound cooling device */
> +/* Default weight for cooling devices and sub thermal zones */
>  #define THERMAL_WEIGHT_DEFAULT 0
>  
>  /* Unit conversion macros */
> @@ -89,6 +89,18 @@ enum thermal_trend {
>  	THERMAL_TREND_DROP_FULL, /* apply lowest cooling action */
>  };
>  
> +/**
> + * enum thermal_aggregation_function - aggregation function for sub thermal zones
> + * @THERMAL_AGG_MAX:	calculate the maximum of all sub thermal zones
> +
> + * @THERMAL_AGG_WEIGHTED_AVG:	calculate the weighted average of all
> + *				sub thermal zones
> + */
> +enum thermal_aggregation_function {
> +	THERMAL_AGG_MAX,
> +	THERMAL_AGG_WEIGHTED_AVG,
> +};
> +
>  struct thermal_zone_device_ops {
>  	int (*bind) (struct thermal_zone_device *,
>  		     struct thermal_cooling_device *);
> @@ -170,6 +182,12 @@ struct thermal_attr {
>   * @ops:	operations this &thermal_zone_device supports
>   * @tzp:	thermal zone parameters
>   * @governor:	pointer to the governor for this thermal zone
> + * @slave_agg_function: aggretion function to calculate the
> + *			temperature as a combination of the slave thermal zones
> + *			of this thermal zone.  See enum
> + *			thermal_aggregation_function for valid values
> + * @slave_tzs:	list of thermal zones that are a slave of this thermal zone
> + * @slave_tz_visited: used to detect loops in stacked thermal zones
>   * @governor_data:	private pointer for governor data
>   * @thermal_instances:	list of &struct thermal_instance of this thermal zone
>   * @idr:	&struct idr to generate unique id for this zone's cooling
> @@ -197,6 +215,9 @@ struct thermal_zone_device {
>  	struct thermal_zone_device_ops *ops;
>  	struct thermal_zone_params *tzp;
>  	struct thermal_governor *governor;
> +	enum thermal_aggregation_function slave_agg_function;
> +	struct list_head slave_tzs;
> +	bool slave_tz_visited;
>  	void *governor_data;
>  	struct list_head thermal_instances;
>  	struct idr idr;
> @@ -311,6 +332,13 @@ struct thermal_zone_params {
>  	 * 		Used by thermal zone drivers (default 0).
>  	 */
>  	int offset;
> +
> +	/*
> +	 * aggregation function to use when calculating the
> +	 * temperature as a combination of multiple sub thermal
> +	 * zones
> +	 */
> +	enum thermal_aggregation_function agg_func;
>  };
>  
>  struct thermal_genl_event {
> @@ -405,6 +433,10 @@ struct thermal_cooling_device *
>  thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>  				   const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
> +int thermal_zone_add_subtz(struct thermal_zone_device *,
> +			   struct thermal_zone_device *, int);
> +int thermal_zone_del_subtz(struct thermal_zone_device *,
> +			   struct thermal_zone_device *);
>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
>  int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>  
> @@ -457,6 +489,13 @@ thermal_of_cooling_device_register(struct device_node *np,
>  static inline void thermal_cooling_device_unregister(
>  	struct thermal_cooling_device *cdev)
>  { }
> +static inline int thermal_zone_add_subtz(struct thermal_zone_device *tz,
> +					 struct thermal_zone_device *subtz,
> +					 int weight)
> +{ return -ENODEV; }
> +static inline int thermal_zone_del_subtz(struct thermal_zone_device *tz,
> +					 struct thermal_zone_device *subtz)
> +{ return -ENODEV; }
>  static inline struct thermal_zone_device *thermal_zone_get_zone_by_name(
>  		const char *name)
>  { return ERR_PTR(-ENODEV); }
> -- 
> 1.9.1
> 

Re: [PATCH v3 1/4] thermal: Add support for hierarchical thermal zones

[Eduardo Valentin]

On Wed, Mar 02, 2016 at 07:12:46PM -0800, Eduardo Valentin wrote:
> 
> Thanks for moving this forward Javi,
> 
> Few comments inline.
> 
> On Wed, Nov 25, 2015 at 03:09:43PM +0000, Javi Merino wrote:
> > Add the ability to stack thermal zones on top of each other, creating a
> > hierarchy of thermal zones.

> > +
> > +	if (!tz->ops->get_temp) {
> > +		ret = get_subtz_temp(tz, temp);
> > +		goto unlock;
> > +	}

In fact, if we follow the semantics in DT, we should allow both,
regular sensors (.get_temp) and subtzs.

[PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Javi Merino]

The thermal-sensor property of the thermal zone node accepts phandles to
thermal sensors.  However, thermal zones can be created as an
aggregation of other thermal zones.  Extend the thermal-sensors property
to allow phandles to other thermal zones.  This patch also adds an
example that showcases how a board thermal zone can be created from the
aggregation of the cpu, gpu and lcd thermal zones.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Javi Merino <javi.merino@arm.com>
---

Notes:
    Hi devicetree,
    
    Is it ok to extend the definition of the thermal-sensors property like
    this?  IOW are phandles strongly typed?

 .../devicetree/bindings/thermal/thermal.txt        | 154 ++++++++++++++++++++-
 1 file changed, 151 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
index 41b817f7b670..52b7e9ae3b4d 100644
--- a/Documentation/devicetree/bindings/thermal/thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/thermal.txt
@@ -145,9 +145,12 @@ Required properties:
   Size: one cell
 
 - thermal-sensors:	A list of thermal sensor phandles and sensor specifier
-  Type: list of 	used while monitoring the thermal zone.
-  phandles + sensor
-  specifier
+  Type: list of 	used while monitoring the thermal zone. The phandles
+  phandles + sensor	can point to thermal sensors or other thermal zone
+  specifier		nodes. If it points to other thermal zone
+			nodes you should omit the sensor specifier
+			and set #thermal-sensor-cells to 0 for the
+			thermal zone.
 
 - trips:		A sub-node which is a container of only trip point nodes
   Type: sub-node	required to describe the thermal zone.
@@ -603,3 +606,148 @@ thermal-zones {
 The above example is a mix of previous examples, a sensor IP with several internal
 sensors used to monitor different zones, one of them is composed by several sensors and
 with different cooling devices.
+
+(e) Board thermal with stacked thermal zones
+
+Instead of setting up one thermal zone combining multiple thermal
+zones and multiple trip points for each cooling device, we can create
+a hierarchy of thermal zones.
+
+#include <dt-bindings/thermal/thermal.h>
+
+&i2c1 {
+	...
+	/*
+	 * An IC with several temperature sensor.
+	 */
+	adc_dummy: sensor@0x50 {
+		...
+		#thermal-sensor-cells = <1>; /* sensor internal ID */
+	};
+};
+
+thermal-zones {
+
+        cpu_thermal: cpu_thermal {
+		polling-delay-passive = <1000>; /* milliseconds */
+		polling-delay = <2500>; /* milliseconds */
+
+		sustainable-power = <2500>;
+
+		thermal-sensors = <&adc_dummy 0>
+
+		trips {
+			cpu_trip: cpu-trip {
+				temperature = <60000>; /* millicelsius */
+				hysteresis = <2000>; /* millicelsius */
+				type = "passive";
+			};
+                };
+
+		cooling-maps {
+			map0 {
+				trip = <&cpu_trip>;
+				cooling-device = <&cpu0 0 2>;
+			};
+                };
+        };
+
+        gpu_thermal: gpu_thermal {
+		polling-delay-passive = <1000>; /* milliseconds */
+		polling-delay = <2500>; /* milliseconds */
+
+		sustainable-power = <2500>;
+
+		thermal-sensors = <&adc_dummy 2>
+
+		trips {
+			gpu_trip: gpu-trip {
+				temperature = <55000>; /* millicelsius */
+				hysteresis = <2000>; /* millicelsius */
+				type = "passive";
+			}
+                };
+
+		cooling-maps {
+			map0 {
+				trip = <&gpu_trip>;
+				cooling-device = <&gpu0 0 2>;
+			};
+                };
+        };
+
+        lcd_thermal: lcd_thermal {
+		polling-delay-passive = <1000>; /* milliseconds */
+		polling-delay = <2500>; /* milliseconds */
+
+		sustainable-power = <2500>;
+
+		thermal-sensors = <&adc_dummy 1>
+
+		trips {
+			lcd_trip: lcp-trip {
+				temperature = <53000>; /* millicelsius */
+				hysteresis = <2000>; /* millicelsius */
+				type = "passive";
+			};
+                };
+
+		cooling-maps {
+			map0 {
+				trip = <&lcd_trip>;
+				cooling-device = <&lcd0 5 10>;
+			};
+                };
+        };
+
+	board_thermal: board-thermal {
+		polling-delay-passive = <1000>; /* milliseconds */
+		polling-delay = <2500>; /* milliseconds */
+
+		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>
+
+		sustainable-power = <2500>;
+
+		trips {
+			warm_trip: warm-trip {
+				temperature = <62000>; /* millicelsius */
+				hysteresis = <2000>; /* millicelsius */
+				type = "passive";
+			};
+			crit_trip: crit-trip {
+				temperature = <68000>; /* millicelsius */
+				hysteresis = <2000>; /* millicelsius */
+				type = "critical";
+			};
+		};
+
+		cooling-maps {
+			map0 {
+				trip = <&warm_trip>;
+				cooling-device = <&cpu0 2 2>;
+				contribution = <55>;
+			};
+			map1 {
+				trip = <&warm_trip>;
+				cooling-device = <&gpu0 2 2>;
+				contribution = <20>;
+			};
+			map2 {
+				trip = <&lcd_trip>;
+				cooling-device = <&lcd0 7 10>;
+				contribution = <15>;
+			};
+		};
+	};
+};
+
+The above example is a different take at example (d).  We create one
+thermal zone per sensor: cpu_thermal, gpu_thermal and lcd_thermal.
+Each of which has its own trip point for each own cooling device.  We
+then define a board_thermal thermal zone that is a combination of all
+the other thermal zones.  If the board hits its warm_trip, then all
+cooling devices are throttled.
+
+This example illustrates how we can throttle each device individually
+if its too hot and at the same time have some control over the whole
+system.
-- 
1.9.1

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Mark Rutland]

On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> The thermal-sensor property of the thermal zone node accepts phandles to
> thermal sensors.  However, thermal zones can be created as an
> aggregation of other thermal zones.  Extend the thermal-sensors property
> to allow phandles to other thermal zones.  This patch also adds an
> example that showcases how a board thermal zone can be created from the
> aggregation of the cpu, gpu and lcd thermal zones.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> 
> Notes:
>     Hi devicetree,
>     
>     Is it ok to extend the definition of the thermal-sensors property like
>     this?  IOW are phandles strongly typed?

I think it's OK so long as each thermal zone has #theremal-sensor-cells
set explicitly, if used as a sensor, and we can agree on the semantics
of what it means for a thermal zone to be a sensor.

I don't really follow why you need the zone to be a sensor, and can't
simply refer to the sensor from two zones. Are you trying to imply an
ordering of trip points (e.g. that the sub-zones' trips should be taken
into account first)?

> 
>  .../devicetree/bindings/thermal/thermal.txt        | 154 ++++++++++++++++++++-
>  1 file changed, 151 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 41b817f7b670..52b7e9ae3b4d 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -145,9 +145,12 @@ Required properties:
>    Size: one cell
>  
>  - thermal-sensors:	A list of thermal sensor phandles and sensor specifier
> -  Type: list of 	used while monitoring the thermal zone.
> -  phandles + sensor
> -  specifier
> +  Type: list of 	used while monitoring the thermal zone. The phandles
> +  phandles + sensor	can point to thermal sensors or other thermal zone
> +  specifier		nodes. If it points to other thermal zone
> +			nodes you should omit the sensor specifier
> +			and set #thermal-sensor-cells to 0 for the
> +			thermal zone.

The example misses #thermal-sensor-cells = <0> for each of the zones.

Can a zone normal have multiple sensors? If so, what is the aggregate
value if a zone is used as a sensor? Max? Min? Scaled by contribution
somehow?

Thanks,
Mark.

>  
>  - trips:		A sub-node which is a container of only trip point nodes
>    Type: sub-node	required to describe the thermal zone.
> @@ -603,3 +606,148 @@ thermal-zones {
>  The above example is a mix of previous examples, a sensor IP with several internal
>  sensors used to monitor different zones, one of them is composed by several sensors and
>  with different cooling devices.
> +
> +(e) Board thermal with stacked thermal zones
> +
> +Instead of setting up one thermal zone combining multiple thermal
> +zones and multiple trip points for each cooling device, we can create
> +a hierarchy of thermal zones.
> +
> +#include <dt-bindings/thermal/thermal.h>
> +
> +&i2c1 {
> +	...
> +	/*
> +	 * An IC with several temperature sensor.
> +	 */
> +	adc_dummy: sensor@0x50 {
> +		...
> +		#thermal-sensor-cells = <1>; /* sensor internal ID */
> +	};
> +};
> +
> +thermal-zones {
> +
> +        cpu_thermal: cpu_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 0>
> +
> +		trips {
> +			cpu_trip: cpu-trip {
> +				temperature = <60000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&cpu_trip>;
> +				cooling-device = <&cpu0 0 2>;
> +			};
> +                };
> +        };
> +
> +        gpu_thermal: gpu_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 2>
> +
> +		trips {
> +			gpu_trip: gpu-trip {
> +				temperature = <55000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			}
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&gpu_trip>;
> +				cooling-device = <&gpu0 0 2>;
> +			};
> +                };
> +        };
> +
> +        lcd_thermal: lcd_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 1>
> +
> +		trips {
> +			lcd_trip: lcp-trip {
> +				temperature = <53000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&lcd_trip>;
> +				cooling-device = <&lcd0 5 10>;
> +			};
> +                };
> +        };
> +
> +	board_thermal: board-thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>
> +
> +		sustainable-power = <2500>;
> +
> +		trips {
> +			warm_trip: warm-trip {
> +				temperature = <62000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +			crit_trip: crit-trip {
> +				temperature = <68000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "critical";
> +			};
> +		};
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&warm_trip>;
> +				cooling-device = <&cpu0 2 2>;
> +				contribution = <55>;
> +			};
> +			map1 {
> +				trip = <&warm_trip>;
> +				cooling-device = <&gpu0 2 2>;
> +				contribution = <20>;
> +			};
> +			map2 {
> +				trip = <&lcd_trip>;
> +				cooling-device = <&lcd0 7 10>;
> +				contribution = <15>;
> +			};
> +		};
> +	};
> +};
> +
> +The above example is a different take at example (d).  We create one
> +thermal zone per sensor: cpu_thermal, gpu_thermal and lcd_thermal.
> +Each of which has its own trip point for each own cooling device.  We
> +then define a board_thermal thermal zone that is a combination of all
> +the other thermal zones.  If the board hits its warm_trip, then all
> +cooling devices are throttled.
> +
> +This example illustrates how we can throttle each device individually
> +if its too hot and at the same time have some control over the whole
> +system.
> -- 
> 1.9.1
> 

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Javi Merino]

On Wed, Nov 25, 2015 at 05:54:41PM +0000, Mark Rutland wrote:
> On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > The thermal-sensor property of the thermal zone node accepts phandles to
> > thermal sensors.  However, thermal zones can be created as an
> > aggregation of other thermal zones.  Extend the thermal-sensors property
> > to allow phandles to other thermal zones.  This patch also adds an
> > example that showcases how a board thermal zone can be created from the
> > aggregation of the cpu, gpu and lcd thermal zones.
> > 
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Eduardo Valentin <edubezval@gmail.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: Kumar Gala <galak@codeaurora.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> > 
> > Notes:
> >     Hi devicetree,
> >     
> >     Is it ok to extend the definition of the thermal-sensors property like
> >     this?  IOW are phandles strongly typed?
> 
> I think it's OK so long as each thermal zone has #thermal-sensor-cells
> set explicitly, if used as a sensor, and we can agree on the semantics
> of what it means for a thermal zone to be a sensor.
> 
> I don't really follow why you need the zone to be a sensor, and can't
> simply refer to the sensor from two zones. Are you trying to imply an
> ordering of trip points (e.g. that the sub-zones' trips should be taken
> into account first)?

No, it doesn't affect the ordering of trip points.

This came out of a discussion at LPC.  Currently thermal zones can
only have on thermal sensor associated with them.  After some
discussion, Mike Turquette suggested that we could use an approach
similar to what it's done with power domains and stack them.

> >  .../devicetree/bindings/thermal/thermal.txt        | 154 ++++++++++++++++++++-
> >  1 file changed, 151 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> > index 41b817f7b670..52b7e9ae3b4d 100644
> > --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> > @@ -145,9 +145,12 @@ Required properties:
> >    Size: one cell
> >  
> >  - thermal-sensors:	A list of thermal sensor phandles and sensor specifier
> > -  Type: list of 	used while monitoring the thermal zone.
> > -  phandles + sensor
> > -  specifier
> > +  Type: list of 	used while monitoring the thermal zone. The phandles
> > +  phandles + sensor	can point to thermal sensors or other thermal zone
> > +  specifier		nodes. If it points to other thermal zone
> > +			nodes you should omit the sensor specifier
> > +			and set #thermal-sensor-cells to 0 for the
> > +			thermal zone.
> 
> The example misses #thermal-sensor-cells = <0> for each of the zones.

You're right, I'll fix it for the next version

> Can a zone normal have multiple sensors? If so, what is the aggregate
> value if a zone is used as a sensor? Max? Min? Scaled by contribution
> somehow?

No, currently a thermal zone can only specify one sensor in its
thermal-sensors property

Cheers,
Javi

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Javi Merino]

On Wed, Nov 25, 2015 at 05:54:41PM +0000, Mark Rutland wrote:
> On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > The thermal-sensor property of the thermal zone node accepts phandles to
> > thermal sensors.  However, thermal zones can be created as an
> > aggregation of other thermal zones.  Extend the thermal-sensors property
> > to allow phandles to other thermal zones.  This patch also adds an
> > example that showcases how a board thermal zone can be created from the
> > aggregation of the cpu, gpu and lcd thermal zones.
> > 
> > Cc: Zhang Rui <rui.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Signed-off-by: Javi Merino <javi.merino-5wv7dgnIgG8@public.gmane.org>
> > ---
> > 
> > Notes:
> >     Hi devicetree,
> >     
> >     Is it ok to extend the definition of the thermal-sensors property like
> >     this?  IOW are phandles strongly typed?
> 
> I think it's OK so long as each thermal zone has #thermal-sensor-cells
> set explicitly, if used as a sensor, and we can agree on the semantics
> of what it means for a thermal zone to be a sensor.
> 
> I don't really follow why you need the zone to be a sensor, and can't
> simply refer to the sensor from two zones. Are you trying to imply an
> ordering of trip points (e.g. that the sub-zones' trips should be taken
> into account first)?

No, it doesn't affect the ordering of trip points.

This came out of a discussion at LPC.  Currently thermal zones can
only have on thermal sensor associated with them.  After some
discussion, Mike Turquette suggested that we could use an approach
similar to what it's done with power domains and stack them.

> >  .../devicetree/bindings/thermal/thermal.txt        | 154 ++++++++++++++++++++-
> >  1 file changed, 151 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> > index 41b817f7b670..52b7e9ae3b4d 100644
> > --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> > @@ -145,9 +145,12 @@ Required properties:
> >    Size: one cell
> >  
> >  - thermal-sensors:	A list of thermal sensor phandles and sensor specifier
> > -  Type: list of 	used while monitoring the thermal zone.
> > -  phandles + sensor
> > -  specifier
> > +  Type: list of 	used while monitoring the thermal zone. The phandles
> > +  phandles + sensor	can point to thermal sensors or other thermal zone
> > +  specifier		nodes. If it points to other thermal zone
> > +			nodes you should omit the sensor specifier
> > +			and set #thermal-sensor-cells to 0 for the
> > +			thermal zone.
> 
> The example misses #thermal-sensor-cells = <0> for each of the zones.

You're right, I'll fix it for the next version

> Can a zone normal have multiple sensors? If so, what is the aggregate
> value if a zone is used as a sensor? Max? Min? Scaled by contribution
> somehow?

No, currently a thermal zone can only specify one sensor in its
thermal-sensors property

Cheers,
Javi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Sascha Hauer]

On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> The thermal-sensor property of the thermal zone node accepts phandles to
> thermal sensors.  However, thermal zones can be created as an
> aggregation of other thermal zones.  Extend the thermal-sensors property
> to allow phandles to other thermal zones.  This patch also adds an
> example that showcases how a board thermal zone can be created from the
> aggregation of the cpu, gpu and lcd thermal zones.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> 
> Notes:
>     Hi devicetree,
>     
>     Is it ok to extend the definition of the thermal-sensors property like
>     this?  IOW are phandles strongly typed?
> 
>  .../devicetree/bindings/thermal/thermal.txt        | 154 ++++++++++++++++++++-
>  1 file changed, 151 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 41b817f7b670..52b7e9ae3b4d 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -145,9 +145,12 @@ Required properties:
>    Size: one cell
>  
>  - thermal-sensors:	A list of thermal sensor phandles and sensor specifier
> -  Type: list of 	used while monitoring the thermal zone.
> -  phandles + sensor
> -  specifier
> +  Type: list of 	used while monitoring the thermal zone. The phandles
> +  phandles + sensor	can point to thermal sensors or other thermal zone
> +  specifier		nodes. If it points to other thermal zone
> +			nodes you should omit the sensor specifier
> +			and set #thermal-sensor-cells to 0 for the
> +			thermal zone.
>  
>  - trips:		A sub-node which is a container of only trip point nodes
>    Type: sub-node	required to describe the thermal zone.
> @@ -603,3 +606,148 @@ thermal-zones {
>  The above example is a mix of previous examples, a sensor IP with several internal
>  sensors used to monitor different zones, one of them is composed by several sensors and
>  with different cooling devices.
> +
> +(e) Board thermal with stacked thermal zones
> +
> +Instead of setting up one thermal zone combining multiple thermal
> +zones and multiple trip points for each cooling device, we can create
> +a hierarchy of thermal zones.
> +
> +#include <dt-bindings/thermal/thermal.h>
> +
> +&i2c1 {
> +	...
> +	/*
> +	 * An IC with several temperature sensor.
> +	 */
> +	adc_dummy: sensor@0x50 {
> +		...
> +		#thermal-sensor-cells = <1>; /* sensor internal ID */
> +	};
> +};
> +
> +thermal-zones {
> +
> +        cpu_thermal: cpu_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 0>
> +
> +		trips {
> +			cpu_trip: cpu-trip {
> +				temperature = <60000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&cpu_trip>;
> +				cooling-device = <&cpu0 0 2>;
> +			};
> +                };
> +        };
> +
> +        gpu_thermal: gpu_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 2>
> +
> +		trips {
> +			gpu_trip: gpu-trip {
> +				temperature = <55000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			}
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&gpu_trip>;
> +				cooling-device = <&gpu0 0 2>;
> +			};
> +                };
> +        };
> +
> +        lcd_thermal: lcd_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 1>
> +
> +		trips {
> +			lcd_trip: lcp-trip {
> +				temperature = <53000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&lcd_trip>;
> +				cooling-device = <&lcd0 5 10>;
> +			};
> +                };
> +        };
> +
> +	board_thermal: board-thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>

This seems inconsistent. Why can a thermal zone only have multiple
thermal sensors when they are thermal zones themselves? Either we assume
that one thermal zone has a single sensor or we assume that it can have
multiple sensors, but this should not depend on the zone being a sub zone
or not.

I think the thermal-sensors property should always point to one or
multiple sensors. I see no point in "This property either points to
exactly one sensor or multiple other thermal zones (from which we only
use the temperature)"

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Eduardo Valentin]

On Mon, Jan 04, 2016 at 03:17:09PM +0100, Sascha Hauer wrote:
> On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > The thermal-sensor property of the thermal zone node accepts phandles to
> > thermal sensors.  However, thermal zones can be created as an
> > +
> > +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>
> 
> This seems inconsistent. Why can a thermal zone only have multiple
> thermal sensors when they are thermal zones themselves? Either we assume
> that one thermal zone has a single sensor or we assume that it can have
> multiple sensors, but this should not depend on the zone being a sub zone
> or not.
> 
> I think the thermal-sensors property should always point to one or
> multiple sensors. I see no point in "This property either points to
> exactly one sensor or multiple other thermal zones (from which we only
> use the temperature)"


Agreed here. In fact, if we are going to allow thermal zones to be
treated as sensors, it means there should be no limits on what you put
of there, as long as all items have #thermal-sensor-cells. So, mixing
one (or more) regular sensors, with other thermal zones shall be
allowed, if we agree in this semantics.

> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Javi Merino]

On Wed, Mar 02, 2016 at 07:21:53PM -0800, Eduardo Valentin wrote:
> On Mon, Jan 04, 2016 at 03:17:09PM +0100, Sascha Hauer wrote:
> > On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > > The thermal-sensor property of the thermal zone node accepts phandles to
> > > thermal sensors.  However, thermal zones can be created as an
> > > +
> > > +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>
> > 
> > This seems inconsistent. Why can a thermal zone only have multiple
> > thermal sensors when they are thermal zones themselves? Either we assume
> > that one thermal zone has a single sensor or we assume that it can have
> > multiple sensors, but this should not depend on the zone being a sub zone
> > or not.
> > 
> > I think the thermal-sensors property should always point to one or
> > multiple sensors. I see no point in "This property either points to
> > exactly one sensor or multiple other thermal zones (from which we only
> > use the temperature)"
> 
> 
> Agreed here. In fact, if we are going to allow thermal zones to be
> treated as sensors, it means there should be no limits on what you put
> of there, as long as all items have #thermal-sensor-cells. So, mixing
> one (or more) regular sensors, with other thermal zones shall be
> allowed, if we agree in this semantics.

Eduardo, thanks for the review.  There doesn't seem to be much
interest in this and I currently have no time to work on it so I am
dropping this series for the time being.  I'm happy for this to be
picked up by somebody else (or who knows, maybe I will be able to work
on it again in the future).

Javi

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Eduardo Valentin]

On Mon, Mar 21, 2016 at 11:55:11AM +0000, Javi Merino wrote:
> On Wed, Mar 02, 2016 at 07:21:53PM -0800, Eduardo Valentin wrote:
> > On Mon, Jan 04, 2016 at 03:17:09PM +0100, Sascha Hauer wrote:
> > > On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > > > The thermal-sensor property of the thermal zone node accepts phandles to
> > > > thermal sensors.  However, thermal zones can be created as an
> > > > +
> > > > +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>
> > > 
> > > This seems inconsistent. Why can a thermal zone only have multiple
> > > thermal sensors when they are thermal zones themselves? Either we assume
> > > that one thermal zone has a single sensor or we assume that it can have
> > > multiple sensors, but this should not depend on the zone being a sub zone
> > > or not.
> > > 
> > > I think the thermal-sensors property should always point to one or
> > > multiple sensors. I see no point in "This property either points to
> > > exactly one sensor or multiple other thermal zones (from which we only
> > > use the temperature)"
> > 
> > 
> > Agreed here. In fact, if we are going to allow thermal zones to be
> > treated as sensors, it means there should be no limits on what you put
> > of there, as long as all items have #thermal-sensor-cells. So, mixing
> > one (or more) regular sensors, with other thermal zones shall be
> > allowed, if we agree in this semantics.
> 
> Eduardo, thanks for the review.  There doesn't seem to be much
> interest in this and I currently have no time to work on it so I am
> dropping this series for the time being.  I'm happy for this to be
> picked up by somebody else (or who knows, maybe I will be able to work
> on it again in the future).

Ok Javi, as this was one of the topics agreed for a change, I will take it over.

Thanks for your help.

> 
> Javi

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Eduardo Valentin]

On Mon, Mar 21, 2016 at 11:55:11AM +0000, Javi Merino wrote:
> On Wed, Mar 02, 2016 at 07:21:53PM -0800, Eduardo Valentin wrote:
> > On Mon, Jan 04, 2016 at 03:17:09PM +0100, Sascha Hauer wrote:
> > > On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> > > > The thermal-sensor property of the thermal zone node accepts phandles to
> > > > thermal sensors.  However, thermal zones can be created as an
> > > > +
> > > > +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>
> > > 
> > > This seems inconsistent. Why can a thermal zone only have multiple
> > > thermal sensors when they are thermal zones themselves? Either we assume
> > > that one thermal zone has a single sensor or we assume that it can have
> > > multiple sensors, but this should not depend on the zone being a sub zone
> > > or not.
> > > 
> > > I think the thermal-sensors property should always point to one or
> > > multiple sensors. I see no point in "This property either points to
> > > exactly one sensor or multiple other thermal zones (from which we only
> > > use the temperature)"
> > 
> > 
> > Agreed here. In fact, if we are going to allow thermal zones to be
> > treated as sensors, it means there should be no limits on what you put
> > of there, as long as all items have #thermal-sensor-cells. So, mixing
> > one (or more) regular sensors, with other thermal zones shall be
> > allowed, if we agree in this semantics.
> 
> Eduardo, thanks for the review.  There doesn't seem to be much
> interest in this and I currently have no time to work on it so I am
> dropping this series for the time being.  I'm happy for this to be
> picked up by somebody else (or who knows, maybe I will be able to work
> on it again in the future).

Ok Javi, as this was one of the topics agreed for a change, I will take it over.

Thanks for your help.

> 
> Javi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] devicetree: bindings: let thermal-sensor point to other thermal zones

[Eduardo Valentin]

On Wed, Nov 25, 2015 at 03:09:44PM +0000, Javi Merino wrote:
> The thermal-sensor property of the thermal zone node accepts phandles to
> thermal sensors.  However, thermal zones can be created as an
> aggregation of other thermal zones.  Extend the thermal-sensors property
> to allow phandles to other thermal zones.  This patch also adds an
> example that showcases how a board thermal zone can be created from the
> aggregation of the cpu, gpu and lcd thermal zones.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> 
> Notes:
>     Hi devicetree,
>     
>     Is it ok to extend the definition of the thermal-sensors property like
>     this?  IOW are phandles strongly typed?
> 
>  .../devicetree/bindings/thermal/thermal.txt        | 154 ++++++++++++++++++++-
>  1 file changed, 151 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 41b817f7b670..52b7e9ae3b4d 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -145,9 +145,12 @@ Required properties:
>    Size: one cell
>  
>  - thermal-sensors:	A list of thermal sensor phandles and sensor specifier
> -  Type: list of 	used while monitoring the thermal zone.
> -  phandles + sensor
> -  specifier
> +  Type: list of 	used while monitoring the thermal zone. The phandles
> +  phandles + sensor	can point to thermal sensors or other thermal zone
> +  specifier		nodes. If it points to other thermal zone
> +			nodes you should omit the sensor specifier
> +			and set #thermal-sensor-cells to 0 for the
> +			thermal zone.
>  
>  - trips:		A sub-node which is a container of only trip point nodes
>    Type: sub-node	required to describe the thermal zone.
> @@ -603,3 +606,148 @@ thermal-zones {
>  The above example is a mix of previous examples, a sensor IP with several internal
>  sensors used to monitor different zones, one of them is composed by several sensors and
>  with different cooling devices.
> +
> +(e) Board thermal with stacked thermal zones
> +
> +Instead of setting up one thermal zone combining multiple thermal
> +zones and multiple trip points for each cooling device, we can create
> +a hierarchy of thermal zones.
> +
> +#include <dt-bindings/thermal/thermal.h>
> +
> +&i2c1 {
> +	...
> +	/*
> +	 * An IC with several temperature sensor.
> +	 */
> +	adc_dummy: sensor@0x50 {
> +		...
> +		#thermal-sensor-cells = <1>; /* sensor internal ID */
> +	};
> +};
> +
> +thermal-zones {
> +
> +        cpu_thermal: cpu_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 0>
> +
> +		trips {
> +			cpu_trip: cpu-trip {
> +				temperature = <60000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&cpu_trip>;
> +				cooling-device = <&cpu0 0 2>;
> +			};
> +                };
> +        };
> +
> +        gpu_thermal: gpu_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 2>
> +
> +		trips {
> +			gpu_trip: gpu-trip {
> +				temperature = <55000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			}
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&gpu_trip>;
> +				cooling-device = <&gpu0 0 2>;
> +			};
> +                };
> +        };
> +
> +        lcd_thermal: lcd_thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		sustainable-power = <2500>;
> +
> +		thermal-sensors = <&adc_dummy 1>
> +
> +		trips {
> +			lcd_trip: lcp-trip {
> +				temperature = <53000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +                };
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&lcd_trip>;
> +				cooling-device = <&lcd0 5 10>;
> +			};
> +                };
> +        };
> +
> +	board_thermal: board-thermal {
> +		polling-delay-passive = <1000>; /* milliseconds */
> +		polling-delay = <2500>; /* milliseconds */
> +
> +		thermal-sensors = <&cpu_thermal &gpu_thermal &lcd_thermal>

All the participating thermal zones need to have a #thermal-sensor-cells
entry (missing above).

> +
> +		sustainable-power = <2500>;
> +
> +		trips {
> +			warm_trip: warm-trip {
> +				temperature = <62000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "passive";
> +			};
> +			crit_trip: crit-trip {
> +				temperature = <68000>; /* millicelsius */
> +				hysteresis = <2000>; /* millicelsius */
> +				type = "critical";
> +			};
> +		};
> +
> +		cooling-maps {
> +			map0 {
> +				trip = <&warm_trip>;
> +				cooling-device = <&cpu0 2 2>;
> +				contribution = <55>;
> +			};
> +			map1 {
> +				trip = <&warm_trip>;
> +				cooling-device = <&gpu0 2 2>;
> +				contribution = <20>;
> +			};
> +			map2 {
> +				trip = <&lcd_trip>;
> +				cooling-device = <&lcd0 7 10>;
> +				contribution = <15>;
> +			};
> +		};
> +	};
> +};
> +
> +The above example is a different take at example (d).  We create one
> +thermal zone per sensor: cpu_thermal, gpu_thermal and lcd_thermal.
> +Each of which has its own trip point for each own cooling device.  We
> +then define a board_thermal thermal zone that is a combination of all
> +the other thermal zones.  If the board hits its warm_trip, then all
> +cooling devices are throttled.
> +
> +This example illustrates how we can throttle each device individually
> +if its too hot and at the same time have some control over the whole
> +system.
> -- 
> 1.9.1
> 

[PATCH v3 3/4] thermal: of: parse stacked thermal zones from device tree

[Javi Merino]

Now that devicetree specifies how to create a hierarchy of thermal
zones, let of-thermal parse it.

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

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 42b7d4253b94..7bbc0c32ee59 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -166,6 +166,27 @@ of_thermal_get_trip_points(struct thermal_zone_device *tz)
 EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
 
 /**
+ * of_thermal_is_thermal_zone - check that a device node corresponds to a thermal zone
+ * @np:	the device node
+ *
+ * Valid thermal zone device nodes must provide the
+ * polling-delay-passive and polling-delay properties.  This function
+ * checks if @np is thermal-zone node.
+ *
+ * Return: true if @np is a valid device for a thermal zone.
+ */
+static bool of_thermal_is_thermal_zone(struct device_node *np)
+{
+	u32 out;
+
+	if ((of_property_read_u32(np, "polling-delay-passive", &out)) ||
+	    (of_property_read_u32(np, "polling-delay", &out)))
+		return false;
+
+	return true;
+}
+
+/**
  * of_thermal_set_emul_temp - function to set emulated temperature
  *
  * @tz:	pointer to a thermal zone
@@ -858,6 +879,82 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz)
 }
 
 /**
+ * link_stacked_thermal_zones - link thermal zones that are a superset of thermal zones
+ * @np:	device node for the root of the thermal zones
+ *
+ * A thermal zone can specify other thermal zones as its input using
+ * the thermal-sensors property of device tree.  This function parses
+ * all the thermal zones in dt and adds the thermal zones in their
+ * thermal-sensors as sub-thermalzones.
+ */
+static void link_stacked_thermal_zones(struct device_node *np)
+{
+	struct device_node *child;
+
+	for_each_child_of_node(np, child) {
+		int i, num_sensors;
+		struct thermal_zone_device *tz;
+
+		num_sensors = of_count_phandle_with_args(child,
+							 "thermal-sensors",
+							 "#thermal-sensor-cells");
+		if (num_sensors <= 0)
+			continue;
+
+		tz = thermal_zone_get_zone_by_name(child->name);
+		if (IS_ERR_OR_NULL(tz)) {
+			/*
+			 * If the tz is available we should have added
+			 * it in of_parse_thermal_zones()
+			 */
+			WARN(of_device_is_available(child),
+			     "Couldn't find thermal zone for %s\n",
+			     of_node_full_name(child));
+			continue;
+		}
+
+		for (i = 0; i < num_sensors; i++) {
+			struct of_phandle_args sensor_specs;
+			struct thermal_zone_device *subtz;
+			int ret;
+
+			ret = of_parse_phandle_with_args(child,
+							 "thermal-sensors",
+							 "#thermal-sensor-cells",
+							 i, &sensor_specs);
+			if (ret) {
+				pr_warn("Failed to parse thermal-sensors of %s: %d\n",
+					of_node_full_name(child), ret);
+				of_node_put(sensor_specs.np);
+				continue;
+			}
+
+			if (!of_thermal_is_thermal_zone(sensor_specs.np)) {
+				of_node_put(sensor_specs.np);
+				continue;
+			}
+
+			subtz = thermal_zone_get_zone_by_name(
+				sensor_specs.np->name);
+			if (IS_ERR_OR_NULL(subtz)) {
+				pr_warn("Couldn't find thermal zone for %s, is it disabled?\n",
+					of_node_full_name(sensor_specs.np));
+				of_node_put(sensor_specs.np);
+				continue;
+			}
+
+			ret = thermal_zone_add_subtz(tz, subtz,
+						     THERMAL_WEIGHT_DEFAULT);
+			if (ret)
+				pr_warn("Failed to add thermal zone %s to %s: %d\n",
+					subtz->type, tz->type, ret);
+
+			of_node_put(sensor_specs.np);
+		}
+	}
+}
+
+/**
  * of_parse_thermal_zones - parse device tree thermal data
  *
  * Initialization function that can be called by machine initialization
@@ -936,6 +1033,9 @@ int __init of_parse_thermal_zones(void)
 			/* attempting to build remaining zones still */
 		}
 	}
+
+	link_stacked_thermal_zones(np);
+
 	of_node_put(np);
 
 	return 0;
-- 
1.9.1

[PATCH v3 4/4] thermal: show the sub thermal zones in sysfs

[Javi Merino]

When a thermal zone is created that has sub thermal zones via
devicetree or via thermal_zone_add_subtz() expose that relationship in
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 |  33 ++++++++++
 drivers/thermal/thermal_core.c      | 125 ++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h             |   3 +
 3 files changed, 161 insertions(+)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index dda24a0bdeec..5636a531f0b1 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -219,11 +219,16 @@ Thermal zone device sys I/F, created once it's registered:
     |---temp:			Current temperature
     |---mode:			Working mode of the thermal zone
     |---policy:			Thermal governor used for this zone
+    |---aggregation_function:	Function to use when aggregating the
+				temperature of this zone's slave thermal zones
     |---available_policies:	Available thermal governors for this zone
     |---trip_point_[0-*]_temp:	Trip point temperature
     |---trip_point_[0-*]_type:	Trip point type
     |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
     |---emul_temp:		Emulated temperature set node
+    |---subtz[0-*]:		Slave thermal zones of this thermal zone
+    |---subtz[0-*]_weight:	Weight of the slave thermal zone on
+				this thermal zone
     |---sustainable_power:      Sustainable dissipatable power
     |---k_po:                   Proportional term during temperature overshoot
     |---k_pu:                   Proportional term during temperature undershoot
@@ -296,6 +301,19 @@ policy
 	One of the various thermal governors used for a particular zone.
 	RW, Required
 
+aggregation_function
+	The aggregation function used to calculate the temperature of
+	this thermal zone when it has sub thermal zones.  It can
+	either be "max" or "weighted_average".  If it's "max", the
+	temperature of this thermal zone is the maximum of the
+	temperatures of the sub thermal zones.  If
+	aggregation_function is "weighted_average", the temperature of
+	this thermal zone is the weighted average of the temperatures
+	of the sub thermal zones.  The weights are specified in
+	subtz[0-*]_weights.  The aggregation_function sysfs entry
+	doesn't exist for thermal zones without sub thermal zones.
+	RW, Optional
+
 available_policies
 	Available thermal governors which can be used for a particular zone.
 	RO, Required
@@ -359,6 +377,21 @@ emul_temp
 	  because userland can easily disable the thermal policy by simply
 	  flooding this sysfs node with low temperature values.
 
+subtz[0-*]
+	sysfs link to the slave thermal zone device.
+	RO, Optional
+
+subtz[0-*]_weight
+	The influence of subtz[0-*] in this thermal zone.  This
+	parameter is only used when using "weighted_average" as the
+	aggregation_function.  When calculating the temperature of
+	this thermal zone, the temperature of each of the thermal
+	zone's slaves is multiplied by its weight.  The result is
+	divided by the sum of all weights.  If all weights are 0,
+	the temperature of this thermal zone is the average of the
+	temperatures of the sub thermal zones.
+	RW, Optional
+
 sustainable_power
 	An estimate of the sustained power that can be dissipated by
 	the thermal zone. Used by the power allocator governor. For
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 98bdb63b3b95..2bb1af0a9349 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -63,14 +63,23 @@ static struct thermal_governor *def_governor;
 
 /**
  * struct thermal_zone_link - a link between "master" and "slave" thermal zones
+ * @id:		link number in the master thermal zone
+ * @name:	filename in the master thermal zone's sysfs directory
  * @weight:	weight of the @slave thermal zone in the master
  *		calculation.  Used when the master thermal zone's
  *		aggregation function is THERMAL_AGG_WEIGHTED_AVG
+ * @weight_attr_name:	filename of the weight attribute in the master's thermal
+ *			zone sysfs directory
+ * @weight_attr:	device attribute for the weight entry in sysfs
  * @slave:	pointer to the slave thermal zone
  * @node:	list node in the master thermal zone's slave_tzs list.
  */
 struct thermal_zone_link {
+	int id;
+	char name[THERMAL_NAME_LENGTH];
 	int weight;
+	char weight_attr_name[THERMAL_NAME_LENGTH];
+	struct device_attribute weight_attr;
 	struct thermal_zone_device *slave;
 	struct list_head node;
 };
@@ -990,6 +999,45 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
 
 static ssize_t
+agg_func_show(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+
+	if (tz->slave_agg_function == THERMAL_AGG_MAX)
+		return sprintf(buf, "max\n");
+	else if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG)
+		return sprintf(buf, "weighted_average\n");
+	else
+		return sprintf(buf, "(invalid)\n");
+}
+
+static ssize_t
+agg_func_store(struct device *dev, struct device_attribute *attr,
+	       const char *buf, size_t count)
+{
+	int ret = 0;
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	char func_name[THERMAL_NAME_LENGTH];
+
+	snprintf(func_name, sizeof(func_name), "%s", buf);
+
+	mutex_lock(&tz->lock);
+
+	if (sysfs_streq(func_name, "max"))
+		tz->slave_agg_function = THERMAL_AGG_MAX;
+	else if (sysfs_streq(func_name, "weighted_average"))
+		tz->slave_agg_function = THERMAL_AGG_WEIGHTED_AVG;
+	else
+		ret = -EINVAL;
+
+	mutex_unlock(&tz->lock);
+
+	return ret ? ret : count;
+}
+static DEVICE_ATTR(aggregation_function, S_IRUGO | S_IWUSR, agg_func_show,
+		   agg_func_store);
+
+static ssize_t
 sustainable_power_show(struct device *dev, struct device_attribute *devattr,
 		       char *buf)
 {
@@ -1305,6 +1353,35 @@ thermal_cooling_device_weight_store(struct device *dev,
 
 	return count;
 }
+
+static ssize_t
+thermal_subtz_weight_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct thermal_zone_link *link;
+
+	link = container_of(attr, struct thermal_zone_link, weight_attr);
+
+	return sprintf(buf, "%d\n", link->weight);
+}
+
+static ssize_t
+thermal_subtz_weight_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct thermal_zone_link *link;
+	int ret, weight;
+
+	ret = kstrtoint(buf, 0, &weight);
+	if (ret)
+		return ret;
+
+	link = container_of(attr, struct thermal_zone_link, weight_attr);
+	link->weight = weight;
+
+	return count;
+}
+
 /* Device management */
 
 /**
@@ -2135,6 +2212,47 @@ int thermal_zone_add_subtz(struct thermal_zone_device *tz,
 	link->weight = weight;
 	list_add_tail(&link->node, &tz->slave_tzs);
 
+	if (list_is_singular(&tz->slave_tzs)) {
+		ret = device_create_file(&tz->device,
+					 &dev_attr_aggregation_function);
+		if (ret)
+			pr_warn("Failed to create aggregation_function sysfs file: %d\n",
+				ret);
+			/*
+			 * Fall through: we failed to create the sysfs file, but
+			 * it's not fatal
+			 */
+	}
+
+	ret = get_idr(&tz->link_idr, NULL, &link->id);
+	if (ret) {
+		/*
+		 * Even if we can't create the symlink in sysfs,
+		 * we've linked the thermal zones, so return success
+		 */
+		ret = 0;
+		goto unlock;
+	}
+
+	snprintf(link->name, ARRAY_SIZE(link->name), "subtz%d", link->id);
+	ret = sysfs_create_link(&tz->device.kobj, &subtz->device.kobj,
+				link->name);
+	if (ret)
+		pr_warn("Failed to create symlink to sub thermal zone: %d\n",
+			ret);
+
+	snprintf(link->weight_attr_name, THERMAL_NAME_LENGTH, "subtz%d_weight",
+		 link->id);
+	sysfs_attr_init(&link->weight_attr.attr);
+	link->weight_attr.attr.name = link->weight_attr_name;
+	link->weight_attr.attr.mode = S_IWUSR | S_IRUGO;
+	link->weight_attr.show = thermal_subtz_weight_show;
+	link->weight_attr.store = thermal_subtz_weight_store;
+	ret = device_create_file(&tz->device, &link->weight_attr);
+	if (ret)
+		pr_warn("Failed to create sub thermal zone weight: %d\n",
+			ret);
+
 	ret = 0;
 
 unlock:
@@ -2166,6 +2284,10 @@ int thermal_zone_del_subtz(struct thermal_zone_device *tz,
 
 	list_for_each_entry(link, &tz->slave_tzs, node) {
 		if (link->slave == subtz) {
+			device_remove_file(&tz->device, &link->weight_attr);
+			sysfs_remove_link(&tz->device.kobj, link->name);
+			release_idr(&tz->link_idr, NULL, link->id);
+
 			list_del(&link->node);
 			kfree(link);
 
@@ -2174,6 +2296,9 @@ int thermal_zone_del_subtz(struct thermal_zone_device *tz,
 		}
 	}
 
+	if (list_empty(&tz->slave_tzs))
+		device_remove_file(&tz->device, &dev_attr_aggregation_function);
+
 	mutex_unlock(&tz->lock);
 
 	return ret;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index dc3d2ab77ab6..f14884353230 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -192,6 +192,8 @@ struct thermal_attr {
  * @thermal_instances:	list of &struct thermal_instance of this thermal zone
  * @idr:	&struct idr to generate unique id for this zone's cooling
  *		devices
+ * @link_idr:	&struct idr to generate unique ids for this zone's links to
+ *		other thermal zones
  * @lock:	lock to protect thermal_instances list
  * @node:	node in thermal_tz_list (in thermal_core.c)
  * @poll_queue:	delayed work for polling
@@ -221,6 +223,7 @@ struct thermal_zone_device {
 	void *governor_data;
 	struct list_head thermal_instances;
 	struct idr idr;
+	struct idr link_idr;
 	struct mutex lock;
 	struct list_head node;
 	struct delayed_work poll_queue;
-- 
1.9.1

Re: [PATCH v3 4/4] thermal: show the sub thermal zones in sysfs

[Eduardo Valentin]

On Wed, Nov 25, 2015 at 03:09:46PM +0000, Javi Merino wrote:
> When a thermal zone is created that has sub thermal zones via
> devicetree or via thermal_zone_add_subtz() expose that relationship in
> 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 |  33 ++++++++++
>  drivers/thermal/thermal_core.c      | 125 ++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h             |   3 +
>  3 files changed, 161 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index dda24a0bdeec..5636a531f0b1 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -219,11 +219,16 @@ Thermal zone device sys I/F, created once it's registered:
>      |---temp:			Current temperature
>      |---mode:			Working mode of the thermal zone
>      |---policy:			Thermal governor used for this zone
> +    |---aggregation_function:	Function to use when aggregating the
> +				temperature of this zone's slave thermal zones
>      |---available_policies:	Available thermal governors for this zone
>      |---trip_point_[0-*]_temp:	Trip point temperature
>      |---trip_point_[0-*]_type:	Trip point type
>      |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
>      |---emul_temp:		Emulated temperature set node
> +    |---subtz[0-*]:		Slave thermal zones of this thermal zone
> +    |---subtz[0-*]_weight:	Weight of the slave thermal zone on
> +				this thermal zone
>      |---sustainable_power:      Sustainable dissipatable power
>      |---k_po:                   Proportional term during temperature overshoot
>      |---k_pu:                   Proportional term during temperature undershoot
> @@ -296,6 +301,19 @@ policy
>  	One of the various thermal governors used for a particular zone.
>  	RW, Required
>  
> +aggregation_function
> +	The aggregation function used to calculate the temperature of
> +	this thermal zone when it has sub thermal zones.  It can
> +	either be "max" or "weighted_average".  If it's "max", the
> +	temperature of this thermal zone is the maximum of the
> +	temperatures of the sub thermal zones.  If
> +	aggregation_function is "weighted_average", the temperature of
> +	this thermal zone is the weighted average of the temperatures
> +	of the sub thermal zones.  The weights are specified in
> +	subtz[0-*]_weights.  The aggregation_function sysfs entry
> +	doesn't exist for thermal zones without sub thermal zones.
> +	RW, Optional
> +
>  available_policies
>  	Available thermal governors which can be used for a particular zone.
>  	RO, Required
> @@ -359,6 +377,21 @@ emul_temp
>  	  because userland can easily disable the thermal policy by simply
>  	  flooding this sysfs node with low temperature values.
>  
> +subtz[0-*]
> +	sysfs link to the slave thermal zone device.
> +	RO, Optional
> +
> +subtz[0-*]_weight
> +	The influence of subtz[0-*] in this thermal zone.  This
> +	parameter is only used when using "weighted_average" as the
> +	aggregation_function.  When calculating the temperature of
> +	this thermal zone, the temperature of each of the thermal
> +	zone's slaves is multiplied by its weight.  The result is
> +	divided by the sum of all weights.  If all weights are 0,
> +	the temperature of this thermal zone is the average of the
> +	temperatures of the sub thermal zones.
> +	RW, Optional
> +
>  sustainable_power
>  	An estimate of the sustained power that can be dissipated by
>  	the thermal zone. Used by the power allocator governor. For
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 98bdb63b3b95..2bb1af0a9349 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -63,14 +63,23 @@ static struct thermal_governor *def_governor;
>  
>  /**
>   * struct thermal_zone_link - a link between "master" and "slave" thermal zones
> + * @id:		link number in the master thermal zone
> + * @name:	filename in the master thermal zone's sysfs directory
>   * @weight:	weight of the @slave thermal zone in the master
>   *		calculation.  Used when the master thermal zone's
>   *		aggregation function is THERMAL_AGG_WEIGHTED_AVG
> + * @weight_attr_name:	filename of the weight attribute in the master's thermal
> + *			zone sysfs directory
> + * @weight_attr:	device attribute for the weight entry in sysfs
>   * @slave:	pointer to the slave thermal zone
>   * @node:	list node in the master thermal zone's slave_tzs list.
>   */
>  struct thermal_zone_link {
> +	int id;
> +	char name[THERMAL_NAME_LENGTH];
>  	int weight;
> +	char weight_attr_name[THERMAL_NAME_LENGTH];

Are you sure you need this?

> +	struct device_attribute weight_attr;
>  	struct thermal_zone_device *slave;
>  	struct list_head node;
>  };
> @@ -990,6 +999,45 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
>  static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
>  
>  static ssize_t
> +agg_func_show(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +
> +	if (tz->slave_agg_function == THERMAL_AGG_MAX)
> +		return sprintf(buf, "max\n");
> +	else if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG)
> +		return sprintf(buf, "weighted_average\n");
> +	else
> +		return sprintf(buf, "(invalid)\n");
> +}
> +
> +static ssize_t
> +agg_func_store(struct device *dev, struct device_attribute *attr,
> +	       const char *buf, size_t count)
> +{
> +	int ret = 0;
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	char func_name[THERMAL_NAME_LENGTH];
> +
> +	snprintf(func_name, sizeof(func_name), "%s", buf);
> +
> +	mutex_lock(&tz->lock);
> +
> +	if (sysfs_streq(func_name, "max"))
> +		tz->slave_agg_function = THERMAL_AGG_MAX;
> +	else if (sysfs_streq(func_name, "weighted_average"))
> +		tz->slave_agg_function = THERMAL_AGG_WEIGHTED_AVG;
> +	else
> +		ret = -EINVAL;
> +
> +	mutex_unlock(&tz->lock);
> +
> +	return ret ? ret : count;
> +}
> +static DEVICE_ATTR(aggregation_function, S_IRUGO | S_IWUSR, agg_func_show,
> +		   agg_func_store);
> +
> +static ssize_t
>  sustainable_power_show(struct device *dev, struct device_attribute *devattr,
>  		       char *buf)
>  {
> @@ -1305,6 +1353,35 @@ thermal_cooling_device_weight_store(struct device *dev,
>  
>  	return count;
>  }
> +
> +static ssize_t
> +thermal_subtz_weight_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct thermal_zone_link *link;
> +
> +	link = container_of(attr, struct thermal_zone_link, weight_attr);
> +
> +	return sprintf(buf, "%d\n", link->weight);
> +}
> +
> +static ssize_t
> +thermal_subtz_weight_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct thermal_zone_link *link;
> +	int ret, weight;
> +
> +	ret = kstrtoint(buf, 0, &weight);
> +	if (ret)
> +		return ret;
> +
> +	link = container_of(attr, struct thermal_zone_link, weight_attr);
> +	link->weight = weight;
> +
> +	return count;
> +}
> +

Now thinking of this, I see how you could input:

(0.25 * T0 - 2C) + (0.75 * T1 + 1C) //where T0 is sensor of subtz0 and
T1 is sensor of subtz1

for example. Which is a typical case, from what I have been seeing. 

How about if we change weight to: (ak/bk)*Tk + ck ?

>  /* Device management */
>  
>  /**
> @@ -2135,6 +2212,47 @@ int thermal_zone_add_subtz(struct thermal_zone_device *tz,
>  	link->weight = weight;
>  	list_add_tail(&link->node, &tz->slave_tzs);
>  
> +	if (list_is_singular(&tz->slave_tzs)) {
> +		ret = device_create_file(&tz->device,
> +					 &dev_attr_aggregation_function);
> +		if (ret)
> +			pr_warn("Failed to create aggregation_function sysfs file: %d\n",
> +				ret);
> +			/*
> +			 * Fall through: we failed to create the sysfs file, but
> +			 * it's not fatal
> +			 */
> +	}
> +
> +	ret = get_idr(&tz->link_idr, NULL, &link->id);
> +	if (ret) {
> +		/*
> +		 * Even if we can't create the symlink in sysfs,
> +		 * we've linked the thermal zones, so return success
> +		 */
> +		ret = 0;
> +		goto unlock;
> +	}
> +
> +	snprintf(link->name, ARRAY_SIZE(link->name), "subtz%d", link->id);
> +	ret = sysfs_create_link(&tz->device.kobj, &subtz->device.kobj,
> +				link->name);
> +	if (ret)
> +		pr_warn("Failed to create symlink to sub thermal zone: %d\n",
> +			ret);
> +
> +	snprintf(link->weight_attr_name, THERMAL_NAME_LENGTH, "subtz%d_weight",
> +		 link->id);
> +	sysfs_attr_init(&link->weight_attr.attr);
> +	link->weight_attr.attr.name = link->weight_attr_name;
> +	link->weight_attr.attr.mode = S_IWUSR | S_IRUGO;
> +	link->weight_attr.show = thermal_subtz_weight_show;
> +	link->weight_attr.store = thermal_subtz_weight_store;
> +	ret = device_create_file(&tz->device, &link->weight_attr);
> +	if (ret)
> +		pr_warn("Failed to create sub thermal zone weight: %d\n",
> +			ret);
> +
>  	ret = 0;
>  
>  unlock:
> @@ -2166,6 +2284,10 @@ int thermal_zone_del_subtz(struct thermal_zone_device *tz,
>  
>  	list_for_each_entry(link, &tz->slave_tzs, node) {
>  		if (link->slave == subtz) {
> +			device_remove_file(&tz->device, &link->weight_attr);
> +			sysfs_remove_link(&tz->device.kobj, link->name);
> +			release_idr(&tz->link_idr, NULL, link->id);
> +
>  			list_del(&link->node);
>  			kfree(link);
>  
> @@ -2174,6 +2296,9 @@ int thermal_zone_del_subtz(struct thermal_zone_device *tz,
>  		}
>  	}
>  
> +	if (list_empty(&tz->slave_tzs))
> +		device_remove_file(&tz->device, &dev_attr_aggregation_function);
> +
>  	mutex_unlock(&tz->lock);
>  
>  	return ret;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index dc3d2ab77ab6..f14884353230 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -192,6 +192,8 @@ struct thermal_attr {
>   * @thermal_instances:	list of &struct thermal_instance of this thermal zone
>   * @idr:	&struct idr to generate unique id for this zone's cooling
>   *		devices
> + * @link_idr:	&struct idr to generate unique ids for this zone's links to
> + *		other thermal zones
>   * @lock:	lock to protect thermal_instances list
>   * @node:	node in thermal_tz_list (in thermal_core.c)
>   * @poll_queue:	delayed work for polling
> @@ -221,6 +223,7 @@ struct thermal_zone_device {
>  	void *governor_data;
>  	struct list_head thermal_instances;
>  	struct idr idr;
> +	struct idr link_idr;
>  	struct mutex lock;
>  	struct list_head node;
>  	struct delayed_work poll_queue;
> -- 
> 1.9.1
> 

Re: [PATCH v3 4/4] thermal: show the sub thermal zones in sysfs

[Eduardo Valentin]

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

A couple of size checks:

On Wed, Nov 25, 2015 at 03:09:46PM +0000, Javi Merino wrote:
> When a thermal zone is created that has sub thermal zones via
> devicetree or via thermal_zone_add_subtz() expose that relationship in
> 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 |  33 ++++++++++
>  drivers/thermal/thermal_core.c      | 125 ++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h             |   3 +
>  3 files changed, 161 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index dda24a0bdeec..5636a531f0b1 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -219,11 +219,16 @@ Thermal zone device sys I/F, created once it's registered:
>      |---temp:			Current temperature
>      |---mode:			Working mode of the thermal zone
>      |---policy:			Thermal governor used for this zone
> +    |---aggregation_function:	Function to use when aggregating the
> +				temperature of this zone's slave thermal zones
>      |---available_policies:	Available thermal governors for this zone
>      |---trip_point_[0-*]_temp:	Trip point temperature
>      |---trip_point_[0-*]_type:	Trip point type
>      |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
>      |---emul_temp:		Emulated temperature set node
> +    |---subtz[0-*]:		Slave thermal zones of this thermal zone
> +    |---subtz[0-*]_weight:	Weight of the slave thermal zone on
> +				this thermal zone
>      |---sustainable_power:      Sustainable dissipatable power
>      |---k_po:                   Proportional term during temperature overshoot
>      |---k_pu:                   Proportional term during temperature undershoot
> @@ -296,6 +301,19 @@ policy
>  	One of the various thermal governors used for a particular zone.
>  	RW, Required
>  
> +aggregation_function
> +	The aggregation function used to calculate the temperature of
> +	this thermal zone when it has sub thermal zones.  It can
> +	either be "max" or "weighted_average".  If it's "max", the
> +	temperature of this thermal zone is the maximum of the
> +	temperatures of the sub thermal zones.  If
> +	aggregation_function is "weighted_average", the temperature of
> +	this thermal zone is the weighted average of the temperatures
> +	of the sub thermal zones.  The weights are specified in
> +	subtz[0-*]_weights.  The aggregation_function sysfs entry
> +	doesn't exist for thermal zones without sub thermal zones.
> +	RW, Optional
> +
>  available_policies
>  	Available thermal governors which can be used for a particular zone.
>  	RO, Required
> @@ -359,6 +377,21 @@ emul_temp
>  	  because userland can easily disable the thermal policy by simply
>  	  flooding this sysfs node with low temperature values.
>  
> +subtz[0-*]
> +	sysfs link to the slave thermal zone device.
> +	RO, Optional
> +
> +subtz[0-*]_weight
> +	The influence of subtz[0-*] in this thermal zone.  This
> +	parameter is only used when using "weighted_average" as the
> +	aggregation_function.  When calculating the temperature of
> +	this thermal zone, the temperature of each of the thermal
> +	zone's slaves is multiplied by its weight.  The result is
> +	divided by the sum of all weights.  If all weights are 0,
> +	the temperature of this thermal zone is the average of the
> +	temperatures of the sub thermal zones.
> +	RW, Optional
> +
>  sustainable_power
>  	An estimate of the sustained power that can be dissipated by
>  	the thermal zone. Used by the power allocator governor. For
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 98bdb63b3b95..2bb1af0a9349 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -63,14 +63,23 @@ static struct thermal_governor *def_governor;
>  
>  /**
>   * struct thermal_zone_link - a link between "master" and "slave" thermal zones
> + * @id:		link number in the master thermal zone
> + * @name:	filename in the master thermal zone's sysfs directory
>   * @weight:	weight of the @slave thermal zone in the master
>   *		calculation.  Used when the master thermal zone's
>   *		aggregation function is THERMAL_AGG_WEIGHTED_AVG
> + * @weight_attr_name:	filename of the weight attribute in the master's thermal
> + *			zone sysfs directory
> + * @weight_attr:	device attribute for the weight entry in sysfs
>   * @slave:	pointer to the slave thermal zone
>   * @node:	list node in the master thermal zone's slave_tzs list.
>   */
>  struct thermal_zone_link {
> +	int id;
> +	char name[THERMAL_NAME_LENGTH];
>  	int weight;
> +	char weight_attr_name[THERMAL_NAME_LENGTH];
> +	struct device_attribute weight_attr;
>  	struct thermal_zone_device *slave;
>  	struct list_head node;
>  };
> @@ -990,6 +999,45 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
>  static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
>  
>  static ssize_t
> +agg_func_show(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +
> +	if (tz->slave_agg_function == THERMAL_AGG_MAX)
> +		return sprintf(buf, "max\n");
> +	else if (tz->slave_agg_function == THERMAL_AGG_WEIGHTED_AVG)
> +		return sprintf(buf, "weighted_average\n");
> +	else
> +		return sprintf(buf, "(invalid)\n");

Do we need to check for buf size (snprintf)?


> +}
> +
> +static ssize_t
> +agg_func_store(struct device *dev, struct device_attribute *attr,
> +	       const char *buf, size_t count)
> +{
> +	int ret = 0;
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	char func_name[THERMAL_NAME_LENGTH];
> +
> +	snprintf(func_name, sizeof(func_name), "%s", buf);
> +
> +	mutex_lock(&tz->lock);
> +
> +	if (sysfs_streq(func_name, "max"))
> +		tz->slave_agg_function = THERMAL_AGG_MAX;
> +	else if (sysfs_streq(func_name, "weighted_average"))
> +		tz->slave_agg_function = THERMAL_AGG_WEIGHTED_AVG;
> +	else
> +		ret = -EINVAL;
> +
> +	mutex_unlock(&tz->lock);
> +
> +	return ret ? ret : count;
> +}
> +static DEVICE_ATTR(aggregation_function, S_IRUGO | S_IWUSR, agg_func_show,
> +		   agg_func_store);
> +
> +static ssize_t
>  sustainable_power_show(struct device *dev, struct device_attribute *devattr,
>  		       char *buf)
>  {
> @@ -1305,6 +1353,35 @@ thermal_cooling_device_weight_store(struct device *dev,
>  
>  	return count;
>  }
> +
> +static ssize_t
> +thermal_subtz_weight_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct thermal_zone_link *link;
> +
> +	link = container_of(attr, struct thermal_zone_link, weight_attr);
> +
> +	return sprintf(buf, "%d\n", link->weight);
> +}
> +
> +static ssize_t
> +thermal_subtz_weight_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct thermal_zone_link *link;
> +	int ret, weight;
> +
> +	ret = kstrtoint(buf, 0, &weight);
> +	if (ret)
> +		return ret;
> +
> +	link = container_of(attr, struct thermal_zone_link, weight_attr);
> +	link->weight = weight;
> +
> +	return count;
> +}
> +
>  /* Device management */
>  
>  /**
> @@ -2135,6 +2212,47 @@ int thermal_zone_add_subtz(struct thermal_zone_device *tz,
>  	link->weight = weight;
>  	list_add_tail(&link->node, &tz->slave_tzs);
>  
> +	if (list_is_singular(&tz->slave_tzs)) {
> +		ret = device_create_file(&tz->device,
> +					 &dev_attr_aggregation_function);
> +		if (ret)
> +			pr_warn("Failed to create aggregation_function sysfs file: %d\n",
> +				ret);
> +			/*
> +			 * Fall through: we failed to create the sysfs file, but
> +			 * it's not fatal
> +			 */
> +	}
> +
> +	ret = get_idr(&tz->link_idr, NULL, &link->id);
> +	if (ret) {
> +		/*
> +		 * Even if we can't create the symlink in sysfs,
> +		 * we've linked the thermal zones, so return success
> +		 */
> +		ret = 0;
> +		goto unlock;
> +	}
> +
> +	snprintf(link->name, ARRAY_SIZE(link->name), "subtz%d", link->id);
> +	ret = sysfs_create_link(&tz->device.kobj, &subtz->device.kobj,
> +				link->name);
> +	if (ret)
> +		pr_warn("Failed to create symlink to sub thermal zone: %d\n",
> +			ret);
> +
> +	snprintf(link->weight_attr_name, THERMAL_NAME_LENGTH, "subtz%d_weight",
> +		 link->id);

please use sizeof(link->weight_att_name) instead of THERMAL_NAME_LENGTH.

> +	sysfs_attr_init(&link->weight_attr.attr);
> +	link->weight_attr.attr.name = link->weight_attr_name;
> +	link->weight_attr.attr.mode = S_IWUSR | S_IRUGO;
> +	link->weight_attr.show = thermal_subtz_weight_show;
> +	link->weight_attr.store = thermal_subtz_weight_store;
> +	ret = device_create_file(&tz->device, &link->weight_attr);
> +	if (ret)
> +		pr_warn("Failed to create sub thermal zone weight: %d\n",
> +			ret);
> +
>  	ret = 0;
>  
>  unlock:
> @@ -2166,6 +2284,10 @@ int thermal_zone_del_subtz(struct thermal_zone_device *tz,
>  
>  	list_for_each_entry(link, &tz->slave_tzs, node) {
>  		if (link->slave == subtz) {
> +			device_remove_file(&tz->device, &link->weight_attr);
> +			sysfs_remove_link(&tz->device.kobj, link->name);
> +			release_idr(&tz->link_idr, NULL, link->id);
> +
>  			list_del(&link->node);
>  			kfree(link);
>  
> @@ -2174,6 +2296,9 @@ int thermal_zone_del_subtz(struct thermal_zone_device *tz,
>  		}
>  	}
>  
> +	if (list_empty(&tz->slave_tzs))
> +		device_remove_file(&tz->device, &dev_attr_aggregation_function);
> +
>  	mutex_unlock(&tz->lock);
>  
>  	return ret;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index dc3d2ab77ab6..f14884353230 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -192,6 +192,8 @@ struct thermal_attr {
>   * @thermal_instances:	list of &struct thermal_instance of this thermal zone
>   * @idr:	&struct idr to generate unique id for this zone's cooling
>   *		devices
> + * @link_idr:	&struct idr to generate unique ids for this zone's links to
> + *		other thermal zones
>   * @lock:	lock to protect thermal_instances list
>   * @node:	node in thermal_tz_list (in thermal_core.c)
>   * @poll_queue:	delayed work for polling
> @@ -221,6 +223,7 @@ struct thermal_zone_device {
>  	void *governor_data;
>  	struct list_head thermal_instances;
>  	struct idr idr;
> +	struct idr link_idr;
>  	struct mutex lock;
>  	struct list_head node;
>  	struct delayed_work poll_queue;
> -- 
> 1.9.1
> 

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