[PATCH 0/4] Thermal Framework Enhancements

[PATCH 0/4] Thermal Framework Enhancements

[Durgadoss R]

This patch series attempts to add some simple governors/
throttling algorithms to the generic thermal layer.
Although this patch set creates simple governors which depend
on the platform data provided, we can start here, and write
some really smart algorithms that will do the wonder!!

Patch 1/4: Creates necessary infrastructure required to
	   add throttling algorithms in thermal_sys.c
	   1. Introduces the get_trend callback
	   2. Introduces the notify_thermal_framework() API
	   3. Exposes sysfs to show/store the throttle_policy

Patch 2/4: Introduce fair_share governor
	   This throttles the cooling_devices according to their
	   weights (and hence the name; Suggestion are welcome :-).
	   The weights in turn describe the effectiveness of a
	   particular cooling device in cooling a thermal zone.

Patch 3/4: Introduce step_wise governor
	   This throttles/de-throttles the cooling devices one
	   step at a time. This is exactly similar to the code
	   we have in thermal_zone_device_update function. The
	   intention is to move all 'throttling logic' related
	   code outside thermal_sys.c and keep them separate.

Patch 4/4: Platform data patch
	   This patch is not for merge. Just as an example to
	   show how we can provide platform data to thermal
	   framework. Not that we do not know how to fill structures,
	   I felt the patch set is in-complete without this. That's
	   why it is here. From next versions, I will ignore this one.

TODO on these patch sets:
* Sync up with Rui's latest patches
* Add more protection and tidy up the existing ones
* Expose the weights and cooling devices through sysfs (Read-Only)
* Remove all throttling related code(if we all agree) from thermal_sys.c
* In fair_share, before setting new state, check if there are other zones,
  which do not want the 'state' to be changed.
  (To do this, we have to loop through the thermal_tz_list and 
  thermal_cdev_list inside fair_share.c. Need to see how good it is
  to make this lists public)
* If we all agree, use step_wise and remove linear_throttle from thermal_sys.c
* Enhance notify_user_space(), so that the use land can extract some sane
  information out of UEvent.

WishList:
* Find a way to provide platform data so that we can map cooling devices
  for a trip point in a thermal zone.
* Make all _throttle methods have same signature. This way we can make use
  of function pointers and make the code a bit simpler.
* The simple governors heavily depend on the platform data provided. We
  can think of some really smart logic, that depends on very minimal platform
  data, and do things on its own :-)
* Make other subsystem core files register with the thermal framework as a
  thermal sensor or as a cooling device as appropriate.
  (I am thinking of Power Supply, Video, cpufreq for now)
* Ah, Find time to do all this !

Durgadoss R (4):
  RFC Thermal: Enhance Generic Thermal layer with policies
  RFC Thermal: Introduce fair-share thermal governor
  RFC Thermal: Introduce a step-wise thermal governor
  RFC Thermal: Platform layer changes to provide thermal data

 Documentation/thermal/sysfs-api.txt |   26 +++
 arch/x86/platform/mrst/mrst.c       |   39 ++++
 drivers/thermal/Kconfig             |   13 ++
 drivers/thermal/Makefile            |    4 +-
 drivers/thermal/fair_share.c        |  111 ++++++++++
 drivers/thermal/step_wise.c         |   86 ++++++++
 drivers/thermal/thermal_sys.c       |  383 ++++++++++++++++++++++++++---------
 include/linux/thermal.h             |   55 +++++
 8 files changed, 620 insertions(+), 97 deletions(-)
 create mode 100644 drivers/thermal/fair_share.c
 create mode 100644 drivers/thermal/step_wise.c

[PATCH 1/4] RFC Thermal: Enhance Generic Thermal layer with policies

[Durgadoss R]

This patch enhances the generic thermal layer with the
infrastructure required to implement thermal governors.
This introduces a platform level thermal_zone_params
structure tp provide platform specific data, a
notify_thermal_framework() API for use by the sensor
drivers, and a simple 'get_trend' callback inside the
thermal_zone_device_ops.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 Documentation/thermal/sysfs-api.txt |   26 +++
 drivers/thermal/thermal_sys.c       |  383 ++++++++++++++++++++++++++---------
 include/linux/thermal.h             |   55 +++++
 3 files changed, 368 insertions(+), 96 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 1733ab9..632a163 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -103,6 +103,32 @@ temperature) and throttle appropriate devices.
     trip: indicates which trip point the cooling devices is associated with
 	  in this thermal zone.
 
+1.4 Thermal Zone Parameters
+1.4.1 struct thermal_zone_params
+    This structure defines the platform level parameters for a thermal zone.
+    This data, for each thermal zone should come from the platform layer.
+    This is an optional feature where some platforms can choose not to
+    provide this data.
+1.4.2 struct thermal_zone_params attributes
+    .thermal_zone_name: Name of the thermal zone, for which these parameters
+			are being defined.
+    .num_cdevs: Number of cooling devices associated with this
+			  thermal zone.
+    .cdevs_name: Names of the cooling devices associated with this
+			   thermal zone.
+    .cdevs: Pointers to cooling devices
+    .weights: This parameter defines the 'influence' of a particular cooling
+	      device on this thermal zone, on a percentage scale. The sum of
+	      all these weights cannot exceed 100. The order of values in
+	      this array should match with that of the cooling_devices_name.
+1.4.3 An example thermal_zone_params structure
+	struct thermal_zone_params tzp = {
+                .thermal_zone_name = "CPU",
+                .num_cdevs = 2,
+                .cdevs_name = {"CPU", "Memory"},
+                .weights = {70, 30},
+        };
+
 2. sysfs attributes structure
 
 RO	read only value
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 022bacb..0412c7f 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -60,6 +60,9 @@ static LIST_HEAD(thermal_tz_list);
 static LIST_HEAD(thermal_cdev_list);
 static DEFINE_MUTEX(thermal_list_lock);
 
+int (*get_platform_thermal_params)(struct thermal_zone_device *tzd);
+EXPORT_SYMBOL(get_platform_thermal_params);
+
 static int get_idr(struct idr *idr, struct mutex *lock, int *id)
 {
 	int err;
@@ -666,81 +669,275 @@ static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
 				      msecs_to_jiffies(delay));
 }
 
-static void thermal_zone_device_passive(struct thermal_zone_device *tz,
-					int temp, int trip_temp, int trip)
+static void thermal_zone_device_check(struct work_struct *work)
 {
-	int trend = 0;
-	struct thermal_cooling_device_instance *instance;
-	struct thermal_cooling_device *cdev;
-	long state, max_state;
+	struct thermal_zone_device *tz = container_of(work, struct
+						      thermal_zone_device,
+						      poll_queue.work);
+	thermal_zone_device_update(tz);
+}
 
-	/*
-	 * Above Trip?
-	 * -----------
-	 * Calculate the thermal trend (using the passive cooling equation)
-	 * and modify the performance limit for all passive cooling devices
-	 * accordingly.  Note that we assume symmetry.
-	 */
-	if (temp >= trip_temp) {
-		tz->passive = true;
-
-		trend = (tz->tc1 * (temp - tz->last_temperature)) +
-			(tz->tc2 * (temp - trip_temp));
-
-		/* Heating up? */
-		if (trend > 0) {
-			list_for_each_entry(instance, &tz->cooling_devices,
-					    node) {
-				if (instance->trip != trip)
-					continue;
-				cdev = instance->cdev;
-				cdev->ops->get_cur_state(cdev, &state);
-				cdev->ops->get_max_state(cdev, &max_state);
-				if (state++ < max_state)
-					cdev->ops->set_cur_state(cdev, state);
-			}
-		} else if (trend < 0) { /* Cooling off? */
-			list_for_each_entry(instance, &tz->cooling_devices,
-					    node) {
-				if (instance->trip != trip)
-					continue;
-				cdev = instance->cdev;
-				cdev->ops->get_cur_state(cdev, &state);
-				cdev->ops->get_max_state(cdev, &max_state);
-				if (state > 0)
-					cdev->ops->set_cur_state(cdev, --state);
-			}
+static int set_throttle_policy(struct thermal_zone_device *tz,
+				const char *policy)
+{
+	struct thermal_zone_params *tzp = tz->tzp;
+
+	if (!strcmp(policy, "fair_share")) {
+		tzp->throttle_policy = THERMAL_FAIR_SHARE;
+	} else if (!strcmp(policy, "step_wise")) {
+		tzp->throttle_policy = THERMAL_STEP_WISE;
+	} else if (!strcmp(policy, "user_space")) {
+		tzp->throttle_policy = THERMAL_USER_SPACE;
+	} else {
+		dev_err(&tz->device, "Setting throttling policy failed:\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t show_throttle_policy(struct device *dev,
+			struct device_attribute *devattr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_params *tzp = tz->tzp;
+
+	switch (tzp->throttle_policy) {
+	case THERMAL_FAIR_SHARE:
+		return sprintf(buf, "fair_share\n");
+	case THERMAL_STEP_WISE:
+		return sprintf(buf, "step_wise\n");
+	case THERMAL_USER_SPACE:
+	default:
+		return sprintf(buf, "user_space\n");
+	}
+}
+
+static ssize_t store_throttle_policy(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	int ret = set_throttle_policy(tz, buf);
+
+	return ret ? ret : count;
+}
+
+static int create_policy_attr(struct thermal_zone_device *tz)
+{
+	sysfs_attr_init(&tz->policy_attr.attr);
+	tz->policy_attr.attr.name = "throttle_policy";
+	tz->policy_attr.attr.mode = S_IRUGO | S_IWUSR;
+	tz->policy_attr.show = show_throttle_policy;
+	tz->policy_attr.store = store_throttle_policy;
+	return device_create_file(&tz->device, &tz->policy_attr);
+}
+
+static void update_tz_params(struct thermal_zone_params *tzp,
+			struct thermal_cooling_device *cdev, int flag)
+{
+	int i;
+
+	for (i = 0; i < tzp->num_cdevs; i++) {
+		if (!strcmp(cdev->type, tzp->cdevs_name[i])) {
+			tzp->cdevs[i] = flag ? cdev : NULL;
+			break;
 		}
+	}
+}
+
+static void retrieve_zone_params(struct thermal_zone_device *tz)
+{
+	int ret;
+
+	/* Check whether the platform data pointer is defined */
+	if (!get_platform_thermal_params)
+		return;
+
+	ret = get_platform_thermal_params(tz);
+	if (ret) {
+		dev_err(&tz->device,
+		"parameters for zone %s not defined:%d\n", tz->type, ret);
+		return;
+	}
+
+	ret = create_policy_attr(tz);
+	if (ret) {
+		dev_err(&tz->device,
+		"create_policy_attr for zone %s failed:%d\n", tz->type, ret);
 		return;
 	}
+}
 
+static void notify_user_space(struct thermal_zone_device *tz, int trip)
+{
 	/*
-	 * Below Trip?
-	 * -----------
-	 * Implement passive cooling hysteresis to slowly increase performance
-	 * and avoid thrashing around the passive trip point.  Note that we
-	 * assume symmetry.
+	 * TODO: Add more parameters (like zone id, trip etc) to
+	 * send to the user land, so that they can use it efficiently.
 	 */
+	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);
+}
+
+static void __dethrottle(struct thermal_cooling_device *cdev)
+{
+	unsigned long cur_state, max_state;
+
+	cdev->ops->get_cur_state(cdev, &cur_state);
+	cdev->ops->get_max_state(cdev, &max_state);
+
+	if (cur_state - 1 > 0)
+		cdev->ops->set_cur_state(cdev, cur_state - 1);
+	else
+		cdev->ops->set_cur_state(cdev, 0);
+}
+
+static void __throttle(struct thermal_cooling_device *cdev)
+{
+	unsigned long cur_state, max_state;
+
+	cdev->ops->get_cur_state(cdev, &cur_state);
+	cdev->ops->get_max_state(cdev, &max_state);
+
+	if (cur_state + 1 < max_state)
+		cdev->ops->set_cur_state(cdev, cur_state + 1);
+	else
+		cdev->ops->set_cur_state(cdev, max_state);
+}
+
+static void linear_throttle(struct thermal_zone_device *tz,
+			int trip, enum thermal_trip_type trip_type)
+{
+	enum thermal_trend trend;
+	struct thermal_cooling_device *cdev;
+	struct thermal_cooling_device_instance *instance;
+
+	/* Heavy Assumption */
+	if (!tz->ops->get_trend)
+		trend = THERMAL_TREND_HEATING;
+	else
+		tz->ops->get_trend(tz, trip, &trend);
+
 	list_for_each_entry(instance, &tz->cooling_devices, node) {
 		if (instance->trip != trip)
 			continue;
+
 		cdev = instance->cdev;
-		cdev->ops->get_cur_state(cdev, &state);
-		cdev->ops->get_max_state(cdev, &max_state);
-		if (state > 0)
-			cdev->ops->set_cur_state(cdev, --state);
-		if (state == 0)
-			tz->passive = false;
+
+		if (trend == THERMAL_TREND_HEATING)
+			__throttle(cdev);
+		else
+			__dethrottle(cdev);
 	}
 }
 
-static void thermal_zone_device_check(struct work_struct *work)
+#ifdef CONFIG_FAIR_SHARE
+static int fs_throttle(struct thermal_zone_device *tz)
 {
-	struct thermal_zone_device *tz = container_of(work, struct
-						      thermal_zone_device,
-						      poll_queue.work);
-	thermal_zone_device_update(tz);
+	return fair_share_throttle(tz);
+}
+#else
+static inline int fs_throttle(struct thermal_zone_device *tz)
+{
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_STEP_WISE
+static int sw_throttle(struct thermal_zone_device *tz, int trip,
+			enum thermal_trip_type trip_type)
+{
+	return step_wise_throttle(tz, trip, trip_type);
+}
+#else
+static inline int sw_throttle(struct thermal_zone_device *tz, int trip,
+			enum thermal_trip_type trip_type)
+{
+	return 0;
+}
+#endif
+
+static void handle_non_critical_trips(struct thermal_zone_device *tz,
+			int trip, enum thermal_trip_type trip_type)
+{
+	/*
+	 * To handle other trip points, we assume that the platform
+	 * data existence is a must. For now, to keep things not break,
+	 * do the same linear_throttle for both active/passive trips.
+	 * But eventually we should move towards a better solution.
+	 *
+	 * The other option would be to just do a notify_user_space,
+	 * and return.
+	 *
+	 * The step-wise throttle is the same as linear-throttle, but it
+	 * expects platform data i.e thermal_zone_params.
+	 */
+	if (!tz->tzp) {
+		linear_throttle(tz, trip, trip_type);
+		return;
+	}
+
+	switch (tz->tzp->throttle_policy) {
+	case THERMAL_FAIR_SHARE:
+		fs_throttle(tz);
+		break;
+	case THERMAL_STEP_WISE:
+		sw_throttle(tz, trip, trip_type);
+		break;
+	case THERMAL_USER_SPACE:
+		notify_user_space(tz, trip);
+		break;
+	}
+}
+
+static void handle_critical_trips(struct thermal_zone_device *tz,
+				int trip, enum thermal_trip_type trip_type)
+{
+	int ret;
+	long temp, trip_temp;
+
+	tz->ops->get_temp(tz, &temp);
+	tz->ops->get_trip_temp(tz, trip, &trip_temp);
+
+	/*
+	 * If we have not crossed the trip_temp, we do not care.
+	 * If we do not have a way to notify, then there is
+	 * nothing much we can do.
+	 */
+	if (temp < trip_temp || !tz->ops->notify)
+		return;
+
+	ret = tz->ops->notify(tz, trip, trip_type);
+
+	if (trip_type == THERMAL_TRIP_CRITICAL && !ret) {
+		pr_emerg("Critical temperature reached(%ld C),shutting down\n",
+			 temp/1000);
+		orderly_poweroff(true);
+	}
+}
+
+/**
+ * notify_thermal_framework - Sensor drivers use this API to notify framework
+ * @tz:		thermal zone device
+ * @trip:	indicates which trip point has been crossed
+ *
+ * This function handles the trip events from sensor drivers. It starts
+ * throttling the cooling devices according to the policy configured.
+ * For CRITICAL and HOT trip points, this notifies the respective drivers,
+ * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
+ */
+void notify_thermal_framework(struct thermal_zone_device *tz, int trip)
+{
+	enum thermal_trip_type trip_type;
+
+	tz->ops->get_trip_type(tz, trip, &trip_type);
+
+	if (trip_type == THERMAL_TRIP_CRITICAL ||
+			trip_type == THERMAL_TRIP_HOT) {
+		handle_critical_trips(tz, trip, trip_type);
+	} else {
+		handle_non_critical_trips(tz, trip, trip_type);
+	}
 }
+EXPORT_SYMBOL(notify_thermal_framework);
 
 /**
  * thermal_zone_bind_cooling_device - bind a cooling device to a thermal zone
@@ -943,6 +1140,8 @@ thermal_cooling_device_register(char *type, void *devdata,
 	mutex_lock(&thermal_list_lock);
 	list_add(&cdev->node, &thermal_cdev_list);
 	list_for_each_entry(pos, &thermal_tz_list, node) {
+		if (pos->tzp)
+			update_tz_params(pos->tzp, cdev, 1);
 		if (!pos->ops->bind)
 			continue;
 		result = pos->ops->bind(pos, cdev);
@@ -993,6 +1192,8 @@ void thermal_cooling_device_unregister(struct
 		if (!tz->ops->unbind)
 			continue;
 		tz->ops->unbind(tz, cdev);
+		if (tz->tzp)
+			update_tz_params(tz->tzp, cdev, 0);
 	}
 	mutex_unlock(&thermal_list_lock);
 	if (cdev->type[0])
@@ -1013,11 +1214,9 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
 
 void thermal_zone_device_update(struct thermal_zone_device *tz)
 {
-	int count, ret = 0;
-	long temp, trip_temp;
+	int count;
+	long temp;
 	enum thermal_trip_type trip_type;
-	struct thermal_cooling_device_instance *instance;
-	struct thermal_cooling_device *cdev;
 
 	mutex_lock(&tz->lock);
 
@@ -1029,52 +1228,23 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 
 	for (count = 0; count < tz->trips; count++) {
 		tz->ops->get_trip_type(tz, count, &trip_type);
-		tz->ops->get_trip_temp(tz, count, &trip_temp);
 
 		switch (trip_type) {
 		case THERMAL_TRIP_CRITICAL:
-			if (temp >= trip_temp) {
-				if (tz->ops->notify)
-					ret = tz->ops->notify(tz, count,
-							      trip_type);
-				if (!ret) {
-					pr_emerg("Critical temperature reached (%ld C), shutting down\n",
-						 temp/1000);
-					orderly_poweroff(true);
-				}
-			}
+			handle_critical_trips(tz, count, trip_type);
 			break;
 		case THERMAL_TRIP_HOT:
-			if (temp >= trip_temp)
-				if (tz->ops->notify)
-					tz->ops->notify(tz, count, trip_type);
+			handle_critical_trips(tz, count, trip_type);
 			break;
 		case THERMAL_TRIP_ACTIVE:
-			list_for_each_entry(instance, &tz->cooling_devices,
-					    node) {
-				if (instance->trip != count)
-					continue;
-
-				cdev = instance->cdev;
-
-				if (temp >= trip_temp)
-					cdev->ops->set_cur_state(cdev, 1);
-				else
-					cdev->ops->set_cur_state(cdev, 0);
-			}
+			handle_non_critical_trips(tz, count, trip_type);
 			break;
 		case THERMAL_TRIP_PASSIVE:
-			if (temp >= trip_temp || tz->passive)
-				thermal_zone_device_passive(tz, temp,
-							    trip_temp, count);
+			handle_non_critical_trips(tz, count, trip_type);
 			break;
 		}
 	}
 
-	if (tz->forced_passive)
-		thermal_zone_device_passive(tz, temp, tz->forced_passive,
-					    THERMAL_TRIPS_NONE);
-
 	tz->last_temperature = temp;
 
 leave:
@@ -1214,9 +1384,26 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
 
 	thermal_zone_device_update(tz);
 
+	retrieve_zone_params(tz);
+
+	if (!tz->tzp)
+		goto exit_success;
+
+	/*
+	 * A new thermal zone has been added. Look up the existing
+	 * cooling devices list and fill the thermal zone parameters
+	 * for this zone.
+	 */
+	mutex_lock(&thermal_list_lock);
+
+	list_for_each_entry(pos, &thermal_cdev_list, node)
+		update_tz_params(tz->tzp, pos, 1);
+
+	mutex_unlock(&thermal_list_lock);
+
+exit_success:
 	if (!result)
 		return tz;
-
 unregister:
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
 	device_unregister(&tz->device);
@@ -1266,6 +1453,10 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 		device_remove_file(&tz->device,
 				   &trip_point_attrs[count * 2 + 1]);
 	}
+
+	if (tz->tzp)
+		device_remove_file(&tz->device, &tz->policy_attr);
+
 	thermal_remove_hwmon_sysfs(tz);
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
 	idr_destroy(&tz->idr);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 796f1ff..4b8a730 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -29,9 +29,25 @@
 #include <linux/device.h>
 #include <linux/workqueue.h>
 
+#define THERMAL_USER_SPACE	0
+/* Following are kernel space policies */
+#define THERMAL_FAIR_SHARE	1
+#define	THERMAL_STEP_WISE	2
+#define	THERMAL_POLICY_DEFAULT	THERMAL_USER_SPACE
+
 struct thermal_zone_device;
 struct thermal_cooling_device;
 
+/*
+ * The platform layer shall define a 'function' that provides the
+ * parameters for all thermal zones in the platform. This pointer
+ * should point to that 'function'.
+ *
+ * In thermal_zone_device_register() we update the parameters
+ * for the particular thermal zone.
+ */
+extern int (*get_platform_thermal_params)(struct thermal_zone_device *tzd);
+
 enum thermal_device_mode {
 	THERMAL_DEVICE_DISABLED = 0,
 	THERMAL_DEVICE_ENABLED,
@@ -44,12 +60,21 @@ enum thermal_trip_type {
 	THERMAL_TRIP_CRITICAL,
 };
 
+enum thermal_trend {
+	THERMAL_TREND_NONE,
+	THERMAL_TREND_HEATING,
+	THERMAL_TREND_COOLING,
+	THERMAL_TREND_DEFAULT = THERMAL_TREND_HEATING,
+};
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
 	int (*unbind) (struct thermal_zone_device *,
 		       struct thermal_cooling_device *);
 	int (*get_temp) (struct thermal_zone_device *, unsigned long *);
+	int (*get_trend) (struct thermal_zone_device *, int,
+				enum thermal_trend *);
 	int (*get_mode) (struct thermal_zone_device *,
 			 enum thermal_device_mode *);
 	int (*set_mode) (struct thermal_zone_device *,
@@ -69,9 +94,17 @@ struct thermal_cooling_device_ops {
 	int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
 };
 
+
 #define THERMAL_TRIPS_NONE -1
 #define THERMAL_MAX_TRIPS 12
 #define THERMAL_NAME_LENGTH 20
+
+/* Really no great reason for '12'. But at max one device per trip point */
+#define MAX_COOLING_DEVICES	THERMAL_MAX_TRIPS
+
+int step_wise_throttle(struct thermal_zone_device *, int, enum thermal_trip_type);
+int fair_share_throttle(struct thermal_zone_device *);
+
 struct thermal_cooling_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -104,7 +137,29 @@ struct thermal_zone_device {
 	struct mutex lock;	/* protect cooling devices list */
 	struct list_head node;
 	struct delayed_work poll_queue;
+	struct device_attribute policy_attr;
+	struct thermal_zone_params *tzp;
 };
+
+struct thermal_zone_params {
+	char *thermal_zone_name;
+	int throttle_policy;
+
+	/* Number of cooling devices associated with this thermal zone */
+	int num_cdevs;
+	char *cdevs_name[MAX_COOLING_DEVICES];
+	struct thermal_cooling_device *cdevs[MAX_COOLING_DEVICES];
+
+	/*
+	 * This is a measure of 'how effectively these devices can
+	 * cool 'this' thermal zone. The shall be determined by platform
+	 * characterization. This is on a 'percentage' scale.
+	 *
+	 * See Documentation/thermal/sysfs-api.txt for more information.
+	 */
+	int weights[MAX_COOLING_DEVICES];
+};
+
 /* Adding event notification support elements */
 #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
 #define THERMAL_GENL_VERSION                    0x01
-- 
1.7.0.4

Re: [PATCH 1/4] RFC Thermal: Enhance Generic Thermal layer with policies

[Eduardo Valentin]

Hello Durgadoss,

Things are moving fast! Losts of work on going.
This is good. Just that now we have some overlaps. I'd propose to be
more structured and align between ourselves. Having branches would
be a nice starting point. I can collect your changes and post to gitorious
or something if you agree.

I like the approach Durgadoss is proposing. So we can have different
policy behavior on different zones.

But we need to see how we merge both of your work. There is still
Amit's work, and I haven't sent my changes yet :-)

On Mon, Jun 11, 2012 at 11:09:31PM +0530, Durgadoss R wrote:
> This patch enhances the generic thermal layer with the
> infrastructure required to implement thermal governors.
> This introduces a platform level thermal_zone_params
> structure tp provide platform specific data, a
> notify_thermal_framework() API for use by the sensor
> drivers, and a simple 'get_trend' callback inside the
> thermal_zone_device_ops.

This patch introduces several things. I see at least three
major changes. Can you split it into smaller pieces?


> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  Documentation/thermal/sysfs-api.txt |   26 +++
>  drivers/thermal/thermal_sys.c       |  383 ++++++++++++++++++++++++++---------
>  include/linux/thermal.h             |   55 +++++
>  3 files changed, 368 insertions(+), 96 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 1733ab9..632a163 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -103,6 +103,32 @@ temperature) and throttle appropriate devices.
>      trip: indicates which trip point the cooling devices is associated with
>  	  in this thermal zone.
>  
> +1.4 Thermal Zone Parameters
> +1.4.1 struct thermal_zone_params
> +    This structure defines the platform level parameters for a thermal zone.
> +    This data, for each thermal zone should come from the platform layer.
> +    This is an optional feature where some platforms can choose not to
> +    provide this data.
> +1.4.2 struct thermal_zone_params attributes
> +    .thermal_zone_name: Name of the thermal zone, for which these parameters
> +			are being defined.
> +    .num_cdevs: Number of cooling devices associated with this
> +			  thermal zone.
> +    .cdevs_name: Names of the cooling devices associated with this
> +			   thermal zone.
> +    .cdevs: Pointers to cooling devices
> +    .weights: This parameter defines the 'influence' of a particular cooling
> +	      device on this thermal zone, on a percentage scale. The sum of
> +	      all these weights cannot exceed 100. The order of values in
> +	      this array should match with that of the cooling_devices_name.
> +1.4.3 An example thermal_zone_params structure
> +	struct thermal_zone_params tzp = {
> +                .thermal_zone_name = "CPU",
> +                .num_cdevs = 2,
> +                .cdevs_name = {"CPU", "Memory"},
> +                .weights = {70, 30},
> +        };
> +

Nice! Only missing point from the documentation above is that it is not
so clear how platform code is suppose to provide the zone params structure...

>  2. sysfs attributes structure
>  
>  RO	read only value
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 022bacb..0412c7f 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -60,6 +60,9 @@ static LIST_HEAD(thermal_tz_list);
>  static LIST_HEAD(thermal_cdev_list);
>  static DEFINE_MUTEX(thermal_list_lock);
>  
> +int (*get_platform_thermal_params)(struct thermal_zone_device *tzd);
> +EXPORT_SYMBOL(get_platform_thermal_params);
> +

Are we sure we want to go in this way? Shouldn't the zone register be
enough to provide this data per zone?

>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>  {
>  	int err;
> @@ -666,81 +669,275 @@ static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
>  				      msecs_to_jiffies(delay));
>  }
>  
> -static void thermal_zone_device_passive(struct thermal_zone_device *tz,
> -					int temp, int trip_temp, int trip)
> +static void thermal_zone_device_check(struct work_struct *work)

I know this is not really related to your patch but, this workqueue handler
name is a bit misleading. I'd propose to at least have the _wq suffix.

>  {
> -	int trend = 0;
> -	struct thermal_cooling_device_instance *instance;
> -	struct thermal_cooling_device *cdev;
> -	long state, max_state;
> +	struct thermal_zone_device *tz = container_of(work, struct
> +						      thermal_zone_device,
> +						      poll_queue.work);
> +	thermal_zone_device_update(tz);
> +}
>  
> -	/*
> -	 * Above Trip?
> -	 * -----------
> -	 * Calculate the thermal trend (using the passive cooling equation)
> -	 * and modify the performance limit for all passive cooling devices
> -	 * accordingly.  Note that we assume symmetry.
> -	 */
> -	if (temp >= trip_temp) {
> -		tz->passive = true;
> -
> -		trend = (tz->tc1 * (temp - tz->last_temperature)) +
> -			(tz->tc2 * (temp - trip_temp));
> -
> -		/* Heating up? */
> -		if (trend > 0) {
> -			list_for_each_entry(instance, &tz->cooling_devices,
> -					    node) {
> -				if (instance->trip != trip)
> -					continue;
> -				cdev = instance->cdev;
> -				cdev->ops->get_cur_state(cdev, &state);
> -				cdev->ops->get_max_state(cdev, &max_state);
> -				if (state++ < max_state)
> -					cdev->ops->set_cur_state(cdev, state);
> -			}
> -		} else if (trend < 0) { /* Cooling off? */
> -			list_for_each_entry(instance, &tz->cooling_devices,
> -					    node) {
> -				if (instance->trip != trip)
> -					continue;
> -				cdev = instance->cdev;
> -				cdev->ops->get_cur_state(cdev, &state);
> -				cdev->ops->get_max_state(cdev, &max_state);
> -				if (state > 0)
> -					cdev->ops->set_cur_state(cdev, --state);
> -			}
> +static int set_throttle_policy(struct thermal_zone_device *tz,
> +				const char *policy)
> +{
> +	struct thermal_zone_params *tzp = tz->tzp;
> +
> +	if (!strcmp(policy, "fair_share")) {
> +		tzp->throttle_policy = THERMAL_FAIR_SHARE;
> +	} else if (!strcmp(policy, "step_wise")) {
> +		tzp->throttle_policy = THERMAL_STEP_WISE;
> +	} else if (!strcmp(policy, "user_space")) {
> +		tzp->throttle_policy = THERMAL_USER_SPACE;
> +	} else {
> +		dev_err(&tz->device, "Setting throttling policy failed:\n");
> +		return -EINVAL;
> +	}

For the comparison above: sysfs_streq.

How do we handle locking and make sure the policy switch is sane and not having concurrency issues?

> +
> +	return 0;
> +}
> +
> +static ssize_t show_throttle_policy(struct device *dev,
> +			struct device_attribute *devattr, char *buf)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	struct thermal_zone_params *tzp = tz->tzp;
> +
> +	switch (tzp->throttle_policy) {
> +	case THERMAL_FAIR_SHARE:
> +		return sprintf(buf, "fair_share\n");
> +	case THERMAL_STEP_WISE:
> +		return sprintf(buf, "step_wise\n");
> +	case THERMAL_USER_SPACE:
> +	default:
> +		return sprintf(buf, "user_space\n");
> +	}
> +}
> +
> +static ssize_t store_throttle_policy(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	int ret = set_throttle_policy(tz, buf);
> +
> +	return ret ? ret : count;
> +}
> +
> +static int create_policy_attr(struct thermal_zone_device *tz)

This function name is not so good as the function also creates the device file.

> +{
> +	sysfs_attr_init(&tz->policy_attr.attr);
> +	tz->policy_attr.attr.name = "throttle_policy";
> +	tz->policy_attr.attr.mode = S_IRUGO | S_IWUSR;
> +	tz->policy_attr.show = show_throttle_policy;
> +	tz->policy_attr.store = store_throttle_policy;
> +	return device_create_file(&tz->device, &tz->policy_attr);
> +}
> +
> +static void update_tz_params(struct thermal_zone_params *tzp,

Any better naming for this function?

> +			struct thermal_cooling_device *cdev, int flag)
> +{
> +	int i;
> +
> +	for (i = 0; i < tzp->num_cdevs; i++) {
> +		if (!strcmp(cdev->type, tzp->cdevs_name[i])) {
> +			tzp->cdevs[i] = flag ? cdev : NULL;

Is flag bool? If so, please use it.

> +			break;
>  		}
> +	}
> +}
> +
> +static void retrieve_zone_params(struct thermal_zone_device *tz)
> +{
> +	int ret;
> +
> +	/* Check whether the platform data pointer is defined */
> +	if (!get_platform_thermal_params)
> +		return;

Another way would be to, in the register, you receive the plat data as parameter
and you simply store it.

> +
> +	ret = get_platform_thermal_params(tz);
> +	if (ret) {
> +		dev_err(&tz->device,
> +		"parameters for zone %s not defined:%d\n", tz->type, ret);
> +		return;

checkpatch.pl --strict says this about the above:
CHECK: Alignment should match open parenthesis
#205: FILE: drivers/thermal/thermal_sys.c:759:
+		dev_err(&tz->device,
+		"parameters for zone %s not defined:%d\n", tz->type, ret);



> +	}
> +
> +	ret = create_policy_attr(tz);
> +	if (ret) {
> +		dev_err(&tz->device,
> +		"create_policy_attr for zone %s failed:%d\n", tz->type, ret);

CHECK: Alignment should match open parenthesis
#212: FILE: drivers/thermal/thermal_sys.c:766:
+		dev_err(&tz->device,
+		"create_policy_attr for zone %s failed:%d\n", tz->type, ret);


>  		return;
>  	}
> +}
>  
> +static void notify_user_space(struct thermal_zone_device *tz, int trip)
> +{
>  	/*
> -	 * Below Trip?
> -	 * -----------
> -	 * Implement passive cooling hysteresis to slowly increase performance
> -	 * and avoid thrashing around the passive trip point.  Note that we
> -	 * assume symmetry.
> +	 * TODO: Add more parameters (like zone id, trip etc) to
> +	 * send to the user land, so that they can use it efficiently.
>  	 */
> +	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);

This is not good. I believe we have a userspace notification mechanism already.

Rui, in general we need to standardize the kernel <-> userland userspace communication/
notification system. Is the existing netlink enough? Why would we need a sysfs notify?

> +}
> +
> +static void __dethrottle(struct thermal_cooling_device *cdev)
> +{
> +	unsigned long cur_state, max_state;
> +
> +	cdev->ops->get_cur_state(cdev, &cur_state);
> +	cdev->ops->get_max_state(cdev, &max_state);
> +
> +	if (cur_state - 1 > 0)
> +		cdev->ops->set_cur_state(cdev, cur_state - 1);
> +	else
> +		cdev->ops->set_cur_state(cdev, 0);
> +}
> +
> +static void __throttle(struct thermal_cooling_device *cdev)
> +{
> +	unsigned long cur_state, max_state;
> +
> +	cdev->ops->get_cur_state(cdev, &cur_state);
> +	cdev->ops->get_max_state(cdev, &max_state);
> +
> +	if (cur_state + 1 < max_state)
> +		cdev->ops->set_cur_state(cdev, cur_state + 1);
> +	else
> +		cdev->ops->set_cur_state(cdev, max_state);
> +}
> +
> +static void linear_throttle(struct thermal_zone_device *tz,
> +			int trip, enum thermal_trip_type trip_type)
> +{
> +	enum thermal_trend trend;
> +	struct thermal_cooling_device *cdev;
> +	struct thermal_cooling_device_instance *instance;
> +
> +	/* Heavy Assumption */
> +	if (!tz->ops->get_trend)
> +		trend = THERMAL_TREND_HEATING;
> +	else
> +		tz->ops->get_trend(tz, trip, &trend);
> +
>  	list_for_each_entry(instance, &tz->cooling_devices, node) {
>  		if (instance->trip != trip)
>  			continue;
> +
>  		cdev = instance->cdev;
> -		cdev->ops->get_cur_state(cdev, &state);
> -		cdev->ops->get_max_state(cdev, &max_state);
> -		if (state > 0)
> -			cdev->ops->set_cur_state(cdev, --state);
> -		if (state == 0)
> -			tz->passive = false;
> +
> +		if (trend == THERMAL_TREND_HEATING)
> +			__throttle(cdev);
> +		else
> +			__dethrottle(cdev);

I like the above. Looks way cleaner.

On the other hand, does it make sense to make linear_throttle a
real throttle_policy, instead of treating it as a separate case?

Meaning, also having its own file, just like the others.

If no platform data is set, just create a throttle policy which
is linear_throttle.

>  	}
>  }
>  
> -static void thermal_zone_device_check(struct work_struct *work)
> +#ifdef CONFIG_FAIR_SHARE
> +static int fs_throttle(struct thermal_zone_device *tz)
>  {
> -	struct thermal_zone_device *tz = container_of(work, struct
> -						      thermal_zone_device,
> -						      poll_queue.work);
> -	thermal_zone_device_update(tz);
> +	return fair_share_throttle(tz);
> +}
> +#else
> +static inline int fs_throttle(struct thermal_zone_device *tz)
> +{
> +	return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_STEP_WISE
> +static int sw_throttle(struct thermal_zone_device *tz, int trip,
> +			enum thermal_trip_type trip_type)
> +{
> +	return step_wise_throttle(tz, trip, trip_type);
> +}
> +#else
> +static inline int sw_throttle(struct thermal_zone_device *tz, int trip,
> +			enum thermal_trip_type trip_type)
> +{
> +	return 0;
> +}
> +#endif

Nip: Another way, instead of having the ifdeferry here, we could have a header
file which does the config check above.

> +
> +static void handle_non_critical_trips(struct thermal_zone_device *tz,
> +			int trip, enum thermal_trip_type trip_type)
> +{
> +	/*
> +	 * To handle other trip points, we assume that the platform
> +	 * data existence is a must. For now, to keep things not break,
> +	 * do the same linear_throttle for both active/passive trips.
> +	 * But eventually we should move towards a better solution.
> +	 *
> +	 * The other option would be to just do a notify_user_space,
> +	 * and return.
> +	 *
> +	 * The step-wise throttle is the same as linear-throttle, but it
> +	 * expects platform data i.e thermal_zone_params.
> +	 */
> +	if (!tz->tzp) {
> +		linear_throttle(tz, trip, trip_type);
> +		return;
> +	}
> +
> +	switch (tz->tzp->throttle_policy) {
> +	case THERMAL_FAIR_SHARE:
> +		fs_throttle(tz);
> +		break;
> +	case THERMAL_STEP_WISE:
> +		sw_throttle(tz, trip, trip_type);
> +		break;
> +	case THERMAL_USER_SPACE:
> +		notify_user_space(tz, trip);
> +		break;
> +	}
> +}
> +
> +static void handle_critical_trips(struct thermal_zone_device *tz,
> +				int trip, enum thermal_trip_type trip_type)
> +{
> +	int ret;
> +	long temp, trip_temp;
> +
> +	tz->ops->get_temp(tz, &temp);
> +	tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +
> +	/*
> +	 * If we have not crossed the trip_temp, we do not care.
> +	 * If we do not have a way to notify, then there is
> +	 * nothing much we can do.
> +	 */
> +	if (temp < trip_temp || !tz->ops->notify)
> +		return;


Can someone explain me why .notify is a must for thermal_shutdow?

> +
> +	ret = tz->ops->notify(tz, trip, trip_type);
> +
> +	if (trip_type == THERMAL_TRIP_CRITICAL && !ret) {
> +		pr_emerg("Critical temperature reached(%ld C),shutting down\n",
> +			 temp/1000);
> +		orderly_poweroff(true);
> +	}
> +}
> +
> +/**
> + * notify_thermal_framework - Sensor drivers use this API to notify framework
> + * @tz:		thermal zone device
> + * @trip:	indicates which trip point has been crossed
> + *
> + * This function handles the trip events from sensor drivers. It starts
> + * throttling the cooling devices according to the policy configured.
> + * For CRITICAL and HOT trip points, this notifies the respective drivers,
> + * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
> + */
> +void notify_thermal_framework(struct thermal_zone_device *tz, int trip)
> +{
> +	enum thermal_trip_type trip_type;
> +
> +	tz->ops->get_trip_type(tz, trip, &trip_type);
> +
> +	if (trip_type == THERMAL_TRIP_CRITICAL ||
> +			trip_type == THERMAL_TRIP_HOT) {
> +		handle_critical_trips(tz, trip, trip_type);
> +	} else {
> +		handle_non_critical_trips(tz, trip, trip_type);
> +	}
>  }
> +EXPORT_SYMBOL(notify_thermal_framework);

It is not clear what is the difference of the above and the existing
thermal_zone_device_update().

Is this one supposed to be called from interrupt context?

device_update has mutex locking, this one does not have...

>  
>  /**
>   * thermal_zone_bind_cooling_device - bind a cooling device to a thermal zone
> @@ -943,6 +1140,8 @@ thermal_cooling_device_register(char *type, void *devdata,
>  	mutex_lock(&thermal_list_lock);
>  	list_add(&cdev->node, &thermal_cdev_list);
>  	list_for_each_entry(pos, &thermal_tz_list, node) {
> +		if (pos->tzp)
> +			update_tz_params(pos->tzp, cdev, 1);
>  		if (!pos->ops->bind)
>  			continue;
>  		result = pos->ops->bind(pos, cdev);
> @@ -993,6 +1192,8 @@ void thermal_cooling_device_unregister(struct
>  		if (!tz->ops->unbind)
>  			continue;
>  		tz->ops->unbind(tz, cdev);
> +		if (tz->tzp)
> +			update_tz_params(tz->tzp, cdev, 0);
>  	}
>  	mutex_unlock(&thermal_list_lock);
>  	if (cdev->type[0])
> @@ -1013,11 +1214,9 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>  
>  void thermal_zone_device_update(struct thermal_zone_device *tz)
>  {
> -	int count, ret = 0;
> -	long temp, trip_temp;
> +	int count;
> +	long temp;
>  	enum thermal_trip_type trip_type;
> -	struct thermal_cooling_device_instance *instance;
> -	struct thermal_cooling_device *cdev;
>  
>  	mutex_lock(&tz->lock);
>  
> @@ -1029,52 +1228,23 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  
>  	for (count = 0; count < tz->trips; count++) {
>  		tz->ops->get_trip_type(tz, count, &trip_type);
> -		tz->ops->get_trip_temp(tz, count, &trip_temp);
>  
>  		switch (trip_type) {
>  		case THERMAL_TRIP_CRITICAL:
> -			if (temp >= trip_temp) {
> -				if (tz->ops->notify)
> -					ret = tz->ops->notify(tz, count,
> -							      trip_type);
> -				if (!ret) {
> -					pr_emerg("Critical temperature reached (%ld C), shutting down\n",
> -						 temp/1000);
> -					orderly_poweroff(true);
> -				}
> -			}
> +			handle_critical_trips(tz, count, trip_type);
>  			break;
>  		case THERMAL_TRIP_HOT:
> -			if (temp >= trip_temp)
> -				if (tz->ops->notify)
> -					tz->ops->notify(tz, count, trip_type);
> +			handle_critical_trips(tz, count, trip_type);
>  			break;
>  		case THERMAL_TRIP_ACTIVE:
> -			list_for_each_entry(instance, &tz->cooling_devices,
> -					    node) {
> -				if (instance->trip != count)
> -					continue;
> -
> -				cdev = instance->cdev;
> -
> -				if (temp >= trip_temp)
> -					cdev->ops->set_cur_state(cdev, 1);
> -				else
> -					cdev->ops->set_cur_state(cdev, 0);
> -			}
> +			handle_non_critical_trips(tz, count, trip_type);
>  			break;
>  		case THERMAL_TRIP_PASSIVE:
> -			if (temp >= trip_temp || tz->passive)
> -				thermal_zone_device_passive(tz, temp,
> -							    trip_temp, count);
> +			handle_non_critical_trips(tz, count, trip_type);
>  			break;

Something like this might look better?
+		switch (trip_type) {
+		case THERMAL_TRIP_CRITICAL:
+		case THERMAL_TRIP_HOT:
+			handle_critical_trips(tz, count, trip_type);
+			break;
+		case THERMAL_TRIP_ACTIVE:
+		case THERMAL_TRIP_PASSIVE:
+			handle_non_critical_trips(tz, count, trip_type);
+			break;
+		}

>  		}
>  	}
>  
> -	if (tz->forced_passive)
> -		thermal_zone_device_passive(tz, temp, tz->forced_passive,
> -					    THERMAL_TRIPS_NONE);
> -
>  	tz->last_temperature = temp;
>  
>  leave:
> @@ -1214,9 +1384,26 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
>  
>  	thermal_zone_device_update(tz);
>  
> +	retrieve_zone_params(tz);

Again, this could be just a simple parameter...

> +
> +	if (!tz->tzp)
> +		goto exit_success;
> +
> +	/*
> +	 * A new thermal zone has been added. Look up the existing
> +	 * cooling devices list and fill the thermal zone parameters
> +	 * for this zone.
> +	 */
> +	mutex_lock(&thermal_list_lock);
> +
> +	list_for_each_entry(pos, &thermal_cdev_list, node)
> +		update_tz_params(tz->tzp, pos, 1);
> +
> +	mutex_unlock(&thermal_list_lock);
> +
> +exit_success:
>  	if (!result)
>  		return tz;
> -
>  unregister:
>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>  	device_unregister(&tz->device);
> @@ -1266,6 +1453,10 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  		device_remove_file(&tz->device,
>  				   &trip_point_attrs[count * 2 + 1]);
>  	}
> +
> +	if (tz->tzp)
> +		device_remove_file(&tz->device, &tz->policy_attr);
> +
>  	thermal_remove_hwmon_sysfs(tz);
>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>  	idr_destroy(&tz->idr);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 796f1ff..4b8a730 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -29,9 +29,25 @@
>  #include <linux/device.h>
>  #include <linux/workqueue.h>
>  
> +#define THERMAL_USER_SPACE	0
> +/* Following are kernel space policies */
> +#define THERMAL_FAIR_SHARE	1
> +#define	THERMAL_STEP_WISE	2
> +#define	THERMAL_POLICY_DEFAULT	THERMAL_USER_SPACE
> +
>  struct thermal_zone_device;
>  struct thermal_cooling_device;
>  
> +/*
> + * The platform layer shall define a 'function' that provides the
> + * parameters for all thermal zones in the platform. This pointer
> + * should point to that 'function'.
> + *
> + * In thermal_zone_device_register() we update the parameters
> + * for the particular thermal zone.
> + */
> +extern int (*get_platform_thermal_params)(struct thermal_zone_device *tzd);
> +
>  enum thermal_device_mode {
>  	THERMAL_DEVICE_DISABLED = 0,
>  	THERMAL_DEVICE_ENABLED,
> @@ -44,12 +60,21 @@ enum thermal_trip_type {
>  	THERMAL_TRIP_CRITICAL,
>  };
>  
> +enum thermal_trend {
> +	THERMAL_TREND_NONE,

Same comment on Rui's patch applies here... Does TREND_NONE stands for 'TREND_STABLE'?

> +	THERMAL_TREND_HEATING,
> +	THERMAL_TREND_COOLING,
> +	THERMAL_TREND_DEFAULT = THERMAL_TREND_HEATING,
> +};
> +
>  struct thermal_zone_device_ops {
>  	int (*bind) (struct thermal_zone_device *,
>  		     struct thermal_cooling_device *);
>  	int (*unbind) (struct thermal_zone_device *,
>  		       struct thermal_cooling_device *);
>  	int (*get_temp) (struct thermal_zone_device *, unsigned long *);
> +	int (*get_trend) (struct thermal_zone_device *, int,
> +				enum thermal_trend *);

Cool! We just need to device in which patch this change goes in as we have a collision :-)

>  	int (*get_mode) (struct thermal_zone_device *,
>  			 enum thermal_device_mode *);
>  	int (*set_mode) (struct thermal_zone_device *,
> @@ -69,9 +94,17 @@ struct thermal_cooling_device_ops {
>  	int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
>  };
>  
> +
>  #define THERMAL_TRIPS_NONE -1
>  #define THERMAL_MAX_TRIPS 12
>  #define THERMAL_NAME_LENGTH 20
> +
> +/* Really no great reason for '12'. But at max one device per trip point */
> +#define MAX_COOLING_DEVICES	THERMAL_MAX_TRIPS
> +
> +int step_wise_throttle(struct thermal_zone_device *, int, enum thermal_trip_type);


WARNING: line over 80 characters
#598: FILE: include/linux/thermal.h:105:
+int step_wise_throttle(struct thermal_zone_device *, int, enum thermal_trip_type);

total: 0 errors, 1 warnings, 3 checks, 588 lines checked


> +int fair_share_throttle(struct thermal_zone_device *);
> +
>  struct thermal_cooling_device {
>  	int id;
>  	char type[THERMAL_NAME_LENGTH];
> @@ -104,7 +137,29 @@ struct thermal_zone_device {
>  	struct mutex lock;	/* protect cooling devices list */
>  	struct list_head node;
>  	struct delayed_work poll_queue;
> +	struct device_attribute policy_attr;
> +	struct thermal_zone_params *tzp;
>  };
> +
> +struct thermal_zone_params {
> +	char *thermal_zone_name;
> +	int throttle_policy;
> +
> +	/* Number of cooling devices associated with this thermal zone */
> +	int num_cdevs;
> +	char *cdevs_name[MAX_COOLING_DEVICES];
> +	struct thermal_cooling_device *cdevs[MAX_COOLING_DEVICES];
> +
> +	/*
> +	 * This is a measure of 'how effectively these devices can
> +	 * cool 'this' thermal zone. The shall be determined by platform
> +	 * characterization. This is on a 'percentage' scale.
> +	 *
> +	 * See Documentation/thermal/sysfs-api.txt for more information.
> +	 */
> +	int weights[MAX_COOLING_DEVICES];

Would it make sense to have an array of structs here instead of two arrays?

Instead of doing:
+	{
+		.thermal_zone_name = "CPU",
+		.throttle_policy = THERMAL_FAIR_SHARE,
+		.num_cdevs = 2,
+		.cdevs_name = {"CPU", "Battery"},
+		.weights = {80, 20},
+	},

one would do:
+	{
+		.thermal_zone_name = "CPU",
+		.throttle_policy = THERMAL_FAIR_SHARE,
+		.num_cdevs = 2,
+		.cdevs = {
+			{
+				.name = "CPU",
+				.weight = 80,
+			},
+			{
+				.name = "Battery",
+				.weight = 20,
+			},
+		},
+	},

It looks lengthier, I know. But at least looks more well structured I'd say.

One can aways define a macro:
+#define CDEV_PDATA_WEIGHT_ENTRY(n, w) \
+{				\
+	.name = n,		\
+	.weight = (w),		\
+}
...
+	{
+		.thermal_zone_name = "CPU",
+		.throttle_policy = THERMAL_FAIR_SHARE,
+		.num_cdevs = 2,
+		.cdevs = {
+			CDEV_PDATA_WEIGHT_ENTRY("CPU", 80),
+			CDEV_PDATA_WEIGHT_ENTRY("Battery", 20),
+		},
+	},

> +};
> +
>  /* Adding event notification support elements */
>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
>  #define THERMAL_GENL_VERSION                    0x01
> -- 
> 1.7.0.4
> 

RE: [PATCH 1/4] RFC Thermal: Enhance Generic Thermal layer with policies

[R, Durgadoss]

Hi Eduardo,

> Hello Durgadoss,
> 
> Things are moving fast! Losts of work on going.
> This is good. Just that now we have some overlaps. I'd propose to be
> more structured and align between ourselves. Having branches would
> be a nice starting point. I can collect your changes and post to gitorious
> or something if you agree.
> 
> I like the approach Durgadoss is proposing. So we can have different
> policy behavior on different zones.

Thank you..

> 
> But we need to see how we merge both of your work. There is still
> Amit's work, and I haven't sent my changes yet :-)

Yes in the next patch set I will try to be synced up with Rui's patches.

> 
> This patch introduces several things. I see at least three
> major changes. Can you split it into smaller pieces?

True and agree with you.
I had various reasons due to which I could not do that.
Now things are clear, and will make it more organized in the next set.

> > +    .weights: This parameter defines the 'influence' of a particular cooling
> > +	      device on this thermal zone, on a percentage scale. The sum of
> > +	      all these weights cannot exceed 100. The order of values in
> > +	      this array should match with that of the cooling_devices_name.
> > +1.4.3 An example thermal_zone_params structure
> > +	struct thermal_zone_params tzp = {
> > +                .thermal_zone_name = "CPU",
> > +                .num_cdevs = 2,
> > +                .cdevs_name = {"CPU", "Memory"},
> > +                .weights = {70, 30},
> > +        };
> > +
> 
> Nice! Only missing point from the documentation above is that it is not
> so clear how platform code is suppose to provide the zone params structure...

I hope (luckily) my patch 4/4 explains that :-)

> >
> > +int (*get_platform_thermal_params)(struct thermal_zone_device *tzd);
> > +EXPORT_SYMBOL(get_platform_thermal_params);
> > +
> 
> Are we sure we want to go in this way? Shouldn't the zone register be
> enough to provide this data per zone?

As I mentioned in other patch, this can here or can be in a platform
thermal driver file. I think keeping it here makes it easier, otherwise
we need to make sure every driver (that wants to fetch pdata) has
this defined there.
I am not saying it is a difficult thing, but thinking from the view of
somebody who just writes driver for a thermal chip, this approach
can make his/her life easy.

> > +	struct thermal_zone_params *tzp = tz->tzp;
> > +
> > +	if (!strcmp(policy, "fair_share")) {
> > +		tzp->throttle_policy = THERMAL_FAIR_SHARE;
> > +	} else if (!strcmp(policy, "step_wise")) {
> > +		tzp->throttle_policy = THERMAL_STEP_WISE;
> > +	} else if (!strcmp(policy, "user_space")) {
> > +		tzp->throttle_policy = THERMAL_USER_SPACE;
> > +	} else {
> > +		dev_err(&tz->device, "Setting throttling policy failed:\n");
> > +		return -EINVAL;
> > +	}
> 
> For the comparison above: sysfs_streq.

I am not aware of the API. Will look at it. Thank you.

> 
> How do we handle locking and make sure the policy switch is sane and not having
> concurrency issues?

Will add protection.

> > +
> > +static int create_policy_attr(struct thermal_zone_device *tz)
> 
> This function name is not so good as the function also creates the device file.

You seem to be good at naming :-)
Any free suggestions for this name here ?

> > +static void update_tz_params(struct thermal_zone_params *tzp,
> 
> Any better naming for this function?

I can think through, but suggestions are welcome :-)

> 
> Is flag bool? If so, please use it.

Yes will use bool. 

> > +	ret = get_platform_thermal_params(tz);
> > +	if (ret) {
> > +		dev_err(&tz->device,
> > +		"parameters for zone %s not defined:%d\n", tz->type, ret);
> > +		return;
> 
> checkpatch.pl --strict says this about the above:
> CHECK: Alignment should match open parenthesis
> #205: FILE: drivers/thermal/thermal_sys.c:759:
> +		dev_err(&tz->device,
> +		"parameters for zone %s not defined:%d\n", tz->type, ret);

I don't run it with --strict.
Will try next time, if it does not affect readability will make this change.

> > +	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);
> 
> This is not good. I believe we have a userspace notification mechanism already.
> 
> Rui, in general we need to standardize the kernel <-> userland userspace
> communication/
> notification system. Is the existing netlink enough? Why would we need a sysfs
> notify?

I agree on the standardization.
I added the existing netlink mechanism a year ago, and we have a couple of
systems using that. That time, the inspiration came from one of the acpi
drivers.

Now, I would like to move towards a UEvent approach (as more user land
apps are moving towards that).

> > -		if (state > 0)
> > -			cdev->ops->set_cur_state(cdev, --state);
> > -		if (state == 0)
> > -			tz->passive = false;
> > +
> > +		if (trend == THERMAL_TREND_HEATING)
> > +			__throttle(cdev);
> > +		else
> > +			__dethrottle(cdev);
> 
> I like the above. Looks way cleaner.

Thank you.

> 
> On the other hand, does it make sense to make linear_throttle a
> real throttle_policy, instead of treating it as a separate case?
> 
> Meaning, also having its own file, just like the others.

Exactly..Will do.

> > +static inline int sw_throttle(struct thermal_zone_device *tz, int trip,
> > +			enum thermal_trip_type trip_type)
> > +{
> > +	return 0;
> > +}
> > +#endif
> 
> Nip: Another way, instead of having the ifdeferry here, we could have a header
> file which does the config check above.

Do you think it is Ok to have this inside thermal.h ?
Or is it better to create one more new .h say thermal_policy.h or thermal_governor.h ?

> 
> 
> Can someone explain me why .notify is a must for thermal_shutdow?

I have no idea. I also wanted to know the reason.
I thought instead of asking in an e-mail, if I touch code more people will answer,
and looks like it worked :-)

If I don't get any valid reason, I will remove this check/modify this as necessary.

> 
> It is not clear what is the difference of the above and the existing
> thermal_zone_device_update().

Again not intended to be different.
I will clean it up.

> 
> Is this one supposed to be called from interrupt context?
> 
> device_update has mutex locking, this one does not have...

Interrupt context is fine, but only from a bottom half.
So, I will add protection here and clean this.

> Something like this might look better?

I don't know why I did not do this :-(

> +		switch (trip_type) {
> +		case THERMAL_TRIP_CRITICAL:
> +		case THERMAL_TRIP_HOT:
> +			handle_critical_trips(tz, count, trip_type);
> +			break;
> +		case THERMAL_TRIP_ACTIVE:
> +		case THERMAL_TRIP_PASSIVE:
> +			handle_non_critical_trips(tz, count, trip_type);
> +			break;


> >  };
> >
> > +enum thermal_trend {
> > +	THERMAL_TREND_NONE,
> 
> Same comment on Rui's patch applies here... Does TREND_NONE stands for
> 'TREND_STABLE'?

We can use 'stable'. I will sync up with Rui's Patches.

> > +	 * See Documentation/thermal/sysfs-api.txt for more information.
> > +	 */
> > +	int weights[MAX_COOLING_DEVICES];
> 
> Would it make sense to have an array of structs here instead of two arrays?
> 
> Instead of doing:
> +	{
> +		.thermal_zone_name = "CPU",
> +		.throttle_policy = THERMAL_FAIR_SHARE,
> +		.num_cdevs = 2,
> +		.cdevs_name = {"CPU", "Battery"},
> +		.weights = {80, 20},
> +	},
> 
> one would do:
> +	{
> +		.thermal_zone_name = "CPU",
> +		.throttle_policy = THERMAL_FAIR_SHARE,
> +		.num_cdevs = 2,
> +		.cdevs = {
> +			{
> +				.name = "CPU",
> +				.weight = 80,
> +			},
> +			{
> +				.name = "Battery",
> +				.weight = 20,
> +			},
> +		},
> +	},
> 
> It looks lengthier, I know. But at least looks more well structured I'd say.

I dropped it for the same reason.
But, this is going to be inside a platform file, so we can choose to do this.

Thanks,
Durga

[PATCH 2/4] RFC Thermal: Introduce fair-share thermal governor

[Durgadoss R]

This patch introduces a simple 'weight' based
governor named fair-share governor. Whenever the
thermal framework gets notified of the trip point
violation, this governor (if configured), throttles
the cooling devices associated with a thermal zone.

This mapping between a thermal zone and a cooling device
and the effectiveness of cooling are provided in the
platform layer.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/Kconfig      |    6 ++
 drivers/thermal/Makefile     |    3 +-
 drivers/thermal/fair_share.c |  111 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 1 deletions(-)
 create mode 100644 drivers/thermal/fair_share.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 514a691..f5132f3 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -26,3 +26,9 @@ config SPEAR_THERMAL
 	help
 	  Enable this to plug the SPEAr thermal sensor driver into the Linux
 	  thermal framework
+
+config FAIR_SHARE
+	bool "Fair-share thermal governor"
+	depends on THERMAL
+	help
+	  Enable this to manage platform thermals using fair-share governor.
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index a9fff0b..4ffe1a8 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -3,4 +3,5 @@
 #
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
-obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
\ No newline at end of file
+obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
+obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
new file mode 100644
index 0000000..59af81d
--- /dev/null
+++ b/drivers/thermal/fair_share.c
@@ -0,0 +1,111 @@
+/*
+ *  fair_share.c - A simple weight based Thermal governor
+ *
+ *  Copyright (C) 2012 Intel Corp
+ *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/thermal.h>
+
+/**
+ * get_trip_level: - obtains the current trip level for a zone
+ * @tz:		thermal zone device
+ */
+int get_trip_level(struct thermal_zone_device *tz)
+{
+	int count = 0;
+	unsigned long cur_temp, trip_temp;
+
+	if (tz->trips == 0 || !tz->ops->get_trip_temp)
+		return 0;
+
+	tz->ops->get_temp(tz, &cur_temp);
+
+	for (count = 0; count < tz->trips; count++) {
+		tz->ops->get_trip_temp(tz, count, &trip_temp);
+		if (cur_temp < trip_temp)
+			break;
+	}
+	return count;
+}
+
+/**
+ * fair_share_throttle - throttles devices asscciated with the given zone
+ * @tz - thermal_zone_device
+ *
+ * Throttling Logic: This uses three parameters to calculate the new
+ * throttle state of the cooling devices associated with the given zone.
+ *
+ * P1. max_state: Maximum throttle state exposed by the cooling device.
+ * P2. weight[i]/100:
+ *	How 'effective' the 'i'th device is, in cooling the given zone.
+ * P3. cur_trip_level/max_no_of_trips:
+ *	This describes the extent to which the devices should be throttled.
+ *	We do not want to throttle too much when we trip a lower temperature,
+ *	whereas the throttling is at full swing if we trip critical levels.
+ *	(Heavily assumes the trip points are in ascending order)
+ * new_state of cooling device = P3 * P2 * P1
+ */
+int fair_share_throttle(struct thermal_zone_device *tz)
+{
+	struct thermal_zone_params *tzp;
+	struct thermal_cooling_device *cdev;
+	unsigned long max_state, new_state;
+	int i;
+
+	int cur_trip_level = get_trip_level(tz);
+
+	/* Do not throttle:
+	 * if there are no parameters defined for this zone
+	 * if current trip level is 0 (for performance reasons)
+	 */
+	if (!tz->tzp || cur_trip_level == 0)
+		return 0;
+
+	tzp = tz->tzp;
+
+	for (i = 0; i < tzp->num_cdevs; i++) {
+		/*
+		 * Do not throttle:
+		 * if this device cannot cool the zone, or
+		 * if the cooling device does not exist anymore
+		 */
+		if (tzp->weights[i] == 0 || !tzp->cdevs[i])
+			continue;
+
+		cdev = tzp->cdevs[i];
+		cdev->ops->get_max_state(cdev, &max_state);
+
+		new_state =
+		(long)(tzp->weights[i] * cur_trip_level * max_state) /
+		(100 * tz->trips);
+
+		cdev->ops->set_cur_state(cdev, new_state);
+	}
+	return 0;
+}
+EXPORT_SYMBOL(fair_share_throttle);
+
+MODULE_AUTHOR("Durgadoss R");
+MODULE_DESCRIPTION("A simple weight based thermal throttling governor");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH 2/4] RFC Thermal: Introduce fair-share thermal governor

[Eduardo Valentin]

Hello Durgadoss,

On Mon, Jun 11, 2012 at 11:09:32PM +0530, Durgadoss R wrote:
> This patch introduces a simple 'weight' based
> governor named fair-share governor. Whenever the
> thermal framework gets notified of the trip point
> violation, this governor (if configured), throttles
> the cooling devices associated with a thermal zone.
> 
> This mapping between a thermal zone and a cooling device
> and the effectiveness of cooling are provided in the
> platform layer.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/Kconfig      |    6 ++
>  drivers/thermal/Makefile     |    3 +-
>  drivers/thermal/fair_share.c |  111 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 119 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/thermal/fair_share.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 514a691..f5132f3 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -26,3 +26,9 @@ config SPEAR_THERMAL
>  	help
>  	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>  	  thermal framework
> +
> +config FAIR_SHARE
> +	bool "Fair-share thermal governor"
> +	depends on THERMAL
> +	help
> +	  Enable this to manage platform thermals using fair-share governor.

A simple description of the policy would improve the helper...

> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index a9fff0b..4ffe1a8 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -3,4 +3,5 @@
>  #
>  
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
> -obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
> \ No newline at end of file
> +obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
> +obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> new file mode 100644
> index 0000000..59af81d
> --- /dev/null
> +++ b/drivers/thermal/fair_share.c
> @@ -0,0 +1,111 @@
> +/*
> + *  fair_share.c - A simple weight based Thermal governor
> + *
> + *  Copyright (C) 2012 Intel Corp
> + *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +
> +/**
> + * get_trip_level: - obtains the current trip level for a zone
> + * @tz:		thermal zone device
> + */
> +int get_trip_level(struct thermal_zone_device *tz)
> +{
> +	int count = 0;
> +	unsigned long cur_temp, trip_temp;
> +
> +	if (tz->trips == 0 || !tz->ops->get_trip_temp)
> +		return 0;
> +
> +	tz->ops->get_temp(tz, &cur_temp);
> +
> +	for (count = 0; count < tz->trips; count++) {
> +		tz->ops->get_trip_temp(tz, count, &trip_temp);
> +		if (cur_temp < trip_temp)
> +			break;
> +	}
> +	return count;
> +}
> +
> +/**
> + * fair_share_throttle - throttles devices asscciated with the given zone
> + * @tz - thermal_zone_device
> + *
> + * Throttling Logic: This uses three parameters to calculate the new
> + * throttle state of the cooling devices associated with the given zone.
> + *
> + * P1. max_state: Maximum throttle state exposed by the cooling device.
> + * P2. weight[i]/100:
> + *	How 'effective' the 'i'th device is, in cooling the given zone.
> + * P3. cur_trip_level/max_no_of_trips:
> + *	This describes the extent to which the devices should be throttled.
> + *	We do not want to throttle too much when we trip a lower temperature,
> + *	whereas the throttling is at full swing if we trip critical levels.
> + *	(Heavily assumes the trip points are in ascending order)
> + * new_state of cooling device = P3 * P2 * P1
> + */
> +int fair_share_throttle(struct thermal_zone_device *tz)
> +{
> +	struct thermal_zone_params *tzp;
> +	struct thermal_cooling_device *cdev;
> +	unsigned long max_state, new_state;
> +	int i;
> +
> +	int cur_trip_level = get_trip_level(tz);
> +
> +	/* Do not throttle:
> +	 * if there are no parameters defined for this zone
> +	 * if current trip level is 0 (for performance reasons)
> +	 */

Nip: I suppose the kernel coding for comments should be:
+	/*
+	 * Do not throttle:
+	 * if there are no parameters defined for this zone
+	 * if current trip level is 0 (for performance reasons)
+	 */


> +	if (!tz->tzp || cur_trip_level == 0)
> +		return 0;
> +
> +	tzp = tz->tzp;
> +
> +	for (i = 0; i < tzp->num_cdevs; i++) {
> +		/*
> +		 * Do not throttle:
> +		 * if this device cannot cool the zone, or
> +		 * if the cooling device does not exist anymore
> +		 */
> +		if (tzp->weights[i] == 0 || !tzp->cdevs[i])
> +			continue;
> +
> +		cdev = tzp->cdevs[i];
> +		cdev->ops->get_max_state(cdev, &max_state);
> +
> +		new_state =
> +		(long)(tzp->weights[i] * cur_trip_level * max_state) /
> +		(100 * tz->trips);

I think, while merging your work with Rui's, you need to take care of the
cooling state range bound to the trip point..

> +
> +		cdev->ops->set_cur_state(cdev, new_state);
> +	}

Nip: Add an extra line here.

> +	return 0;
> +}
> +EXPORT_SYMBOL(fair_share_throttle);
> +
> +MODULE_AUTHOR("Durgadoss R");
> +MODULE_DESCRIPTION("A simple weight based thermal throttling governor");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.0.4
> 

RE: [PATCH 2/4] RFC Thermal: Introduce fair-share thermal governor

[R, Durgadoss]

Hi Eduardo,

> > +	int cur_trip_level = get_trip_level(tz);
> > +
> > +	/* Do not throttle:
> > +	 * if there are no parameters defined for this zone
> > +	 * if current trip level is 0 (for performance reasons)
> > +	 */
> 
> Nip: I suppose the kernel coding for comments should be:

Missed it here :-) Will fix..
Surprised checkpatch did not complain !!

> 
> I think, while merging your work with Rui's, you need to take care of the
> cooling state range bound to the trip point..

Yes Sure. I have not started looking into them yet.
I will Sync up before sending my next set.

Thanks,
Durga

[PATCH 3/4] RFC Thermal: Introduce a step-wise thermal governor

[Durgadoss R]

This patch adds a simple step-wise governor to the
generic thermal layer. This algorithm throttles the
cooling devices in a linear fashion. If the 'trend'
is heating, it throttles by one step. And if the
thermal trend is cooling it de-throttles by one step.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/Kconfig     |    7 +++
 drivers/thermal/Makefile    |    1 +
 drivers/thermal/step_wise.c |   86 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 0 deletions(-)
 create mode 100644 drivers/thermal/step_wise.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index f5132f3..74992cd 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -32,3 +32,10 @@ config FAIR_SHARE
 	depends on THERMAL
 	help
 	  Enable this to manage platform thermals using fair-share governor.
+
+config STEP_WISE
+	bool "step_wise thermal governor"
+	depends on THERMAL
+	help
+	  Enable this to manage platform thermals using a simple linear
+          throttling algorithm
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 4ffe1a8..c2c0ce0 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -5,3 +5,4 @@
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
 obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
+obj-$(CONFIG_STEP_WISE)			+= step_wise.o
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
new file mode 100644
index 0000000..7ce923a
--- /dev/null
+++ b/drivers/thermal/step_wise.c
@@ -0,0 +1,86 @@
+/*
+ *  step_wise.c - A step-by-step Thermal throttling governor
+ *
+ *  Copyright (C) 2012 Intel Corp
+ *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/thermal.h>
+
+unsigned long get_new_state(unsigned long cur_state, enum thermal_trend trend)
+{
+	switch (trend) {
+	case THERMAL_TREND_HEATING:
+		return cur_state + 1;
+	case THERMAL_TREND_COOLING:
+		return cur_state - 1;
+	default:
+		return cur_state + 1;
+	}
+}
+
+/**
+ * step_wise_throttle - throttles devices asscciated with the given zone
+ * @tz - thermal_zone_device
+ *
+ * Throttling Logic: This uses the trend of the thermal zone to throttle.
+ * If the thermal zone is 'heating up' this throttles all the cooling
+ * devices associated with the zone by one step. If the zone is
+ * 'cooling down' it brings back the performance of the devices by one step.
+ */
+int step_wise_throttle(struct thermal_zone_device *tz,
+			int trip, enum thermal_trip_type trip_type)
+{
+	struct thermal_zone_params *tzp = tz->tzp;
+	struct thermal_cooling_device *cdev;
+	unsigned long max_state, cur_state, new_state;
+	enum thermal_trend trend;
+	int i;
+
+	if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend))
+		trend = THERMAL_TREND_DEFAULT;
+
+	for (i = 0; i < tzp->num_cdevs; i++) {
+		if (!tzp->cdevs[i])
+			continue;
+
+		cdev = tzp->cdevs[i];
+		cdev->ops->get_cur_state(cdev, &cur_state);
+		cdev->ops->get_max_state(cdev, &max_state);
+
+		new_state = get_new_state(cur_state, trend);
+		if (new_state >= max_state)
+			cdev->ops->set_cur_state(cdev, max_state);
+		else if (new_state <= 0)
+			cdev->ops->set_cur_state(cdev, 0);
+		else
+			cdev->ops->set_cur_state(cdev, new_state);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(step_wise_throttle);
+
+MODULE_AUTHOR("Durgadoss R");
+MODULE_DESCRIPTION("A step-by-step thermal throttling governor");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH 3/4] RFC Thermal: Introduce a step-wise thermal governor

[Eduardo Valentin]

Hello Rui,

On Mon, Jun 11, 2012 at 11:09:33PM +0530, Durgadoss R wrote:
> This patch adds a simple step-wise governor to the
> generic thermal layer. This algorithm throttles the
> cooling devices in a linear fashion. If the 'trend'
> is heating, it throttles by one step. And if the
> thermal trend is cooling it de-throttles by one step.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/Kconfig     |    7 +++
>  drivers/thermal/Makefile    |    1 +
>  drivers/thermal/step_wise.c |   86 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/thermal/step_wise.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f5132f3..74992cd 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -32,3 +32,10 @@ config FAIR_SHARE
>  	depends on THERMAL
>  	help
>  	  Enable this to manage platform thermals using fair-share governor.
> +
> +config STEP_WISE
> +	bool "step_wise thermal governor"
> +	depends on THERMAL
> +	help
> +	  Enable this to manage platform thermals using a simple linear
> +          throttling algorithm

Same as before, add some description of the governor-policy.

> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 4ffe1a8..c2c0ce0 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -5,3 +5,4 @@
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
>  obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
>  obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
> +obj-$(CONFIG_STEP_WISE)			+= step_wise.o
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> new file mode 100644
> index 0000000..7ce923a
> --- /dev/null
> +++ b/drivers/thermal/step_wise.c
> @@ -0,0 +1,86 @@
> +/*
> + *  step_wise.c - A step-by-step Thermal throttling governor
> + *
> + *  Copyright (C) 2012 Intel Corp
> + *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +
> +unsigned long get_new_state(unsigned long cur_state, enum thermal_trend trend)
> +{
> +	switch (trend) {
> +	case THERMAL_TREND_HEATING:
> +		return cur_state + 1;
> +	case THERMAL_TREND_COOLING:
> +		return cur_state - 1;
> +	default:
> +		return cur_state + 1;
> +	}
> +}
> +
> +/**
> + * step_wise_throttle - throttles devices asscciated with the given zone
> + * @tz - thermal_zone_device
> + *
> + * Throttling Logic: This uses the trend of the thermal zone to throttle.
> + * If the thermal zone is 'heating up' this throttles all the cooling
> + * devices associated with the zone by one step. If the zone is
> + * 'cooling down' it brings back the performance of the devices by one step.
> + */
> +int step_wise_throttle(struct thermal_zone_device *tz,
> +			int trip, enum thermal_trip_type trip_type)
> +{
> +	struct thermal_zone_params *tzp = tz->tzp;
> +	struct thermal_cooling_device *cdev;
> +	unsigned long max_state, cur_state, new_state;
> +	enum thermal_trend trend;
> +	int i;
> +
> +	if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend))
> +		trend = THERMAL_TREND_DEFAULT;
> +
> +	for (i = 0; i < tzp->num_cdevs; i++) {
> +		if (!tzp->cdevs[i])
> +			continue;
> +
> +		cdev = tzp->cdevs[i];
> +		cdev->ops->get_cur_state(cdev, &cur_state);
> +		cdev->ops->get_max_state(cdev, &max_state);
> +
> +		new_state = get_new_state(cur_state, trend);
> +		if (new_state >= max_state)
> +			cdev->ops->set_cur_state(cdev, max_state);
> +		else if (new_state <= 0)

new_state can not be negative as it is declared as unsigned.

> +			cdev->ops->set_cur_state(cdev, 0);
> +		else
> +			cdev->ops->set_cur_state(cdev, new_state);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(step_wise_throttle);


Again, what was the difference between linear and step_wise?

Is it so that linear applies only to the passive trip points and
step wise applies to all cooling devices in the specific zone?

I guess we need some more clarity on the differences.

> +
> +MODULE_AUTHOR("Durgadoss R");
> +MODULE_DESCRIPTION("A step-by-step thermal throttling governor");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.0.4
> 

RE: [PATCH 3/4] RFC Thermal: Introduce a step-wise thermal governor

[R, Durgadoss]

Hi Eduardo,

> > +	  Enable this to manage platform thermals using a simple linear
> > +          throttling algorithm
> 
> Same as before, add some description of the governor-policy.

I added comments while defining this function inside the file.
Never mind, will make it more readable in next version :-)

> > +		else if (new_state <= 0)
> 
> new_state can not be negative as it is declared as unsigned.

Good one. Thank you. will fix.

> 
> Again, what was the difference between linear and step_wise?
> 

Not intended to be different.

> Is it so that linear applies only to the passive trip points and
> step wise applies to all cooling devices in the specific zone?

No. Right now it is like that.
Now that we agree to clean thermal_sys.c from throttling code,
I will add that functionality here.

> 
> I guess we need some more clarity on the differences.

I hope my next patch set will make things clear.

Thanks,
Durga

[PATCH 4/4] RFC Thermal: Platform layer changes to provide thermal data

[Durgadoss R]

This patch shows how can we add platform specific thermal data
required by the enhanced thermal framework.
This is just an example patch, and _not_ for merge.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 arch/x86/platform/mrst/mrst.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/arch/x86/platform/mrst/mrst.c b/arch/x86/platform/mrst/mrst.c
index e31bcd8..eeb3cd0 100644
--- a/arch/x86/platform/mrst/mrst.c
+++ b/arch/x86/platform/mrst/mrst.c
@@ -30,6 +30,7 @@
 #include <linux/mfd/intel_msic.h>
 #include <linux/gpio.h>
 #include <linux/i2c/tc35876x.h>
+#include <linux/thermal.h>
 
 #include <asm/setup.h>
 #include <asm/mpspec_def.h>
@@ -78,6 +79,27 @@ struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];
 EXPORT_SYMBOL_GPL(sfi_mrtc_array);
 int sfi_mrtc_num;
 
+#define MRST_THERMAL_ZONES	3
+struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
+	{ .thermal_zone_name = "CPU",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 2,
+	.cdevs_name = {"CPU", "Battery"},
+	.weights = {80, 20}, },
+
+	{ .thermal_zone_name = "Battery",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 1,
+	.cdevs_name = {"Battery"},
+	.weights = {100}, },
+
+	{ .thermal_zone_name = "Skin",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 2,
+	.cdevs_name = {"Display", "Battery"},
+	.weights = {50, 50}, }
+};
+
 static void mrst_power_off(void)
 {
 }
@@ -983,10 +1005,27 @@ static int __init sfi_parse_devs(struct sfi_table_header *table)
 	return 0;
 }
 
+static int mrst_get_thermal_params(struct thermal_zone_device *tz)
+{
+	int i;
+
+	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
+		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
+			tz->tzp = &tzp[i];
+			return 0;
+		}
+	}
+	return -ENODEV;
+}
+
 static int __init mrst_platform_init(void)
 {
 	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
 	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
+
+	/* Set platform thermal data pointer */
+	get_platform_thermal_params = mrst_get_thermal_params;
+
 	return 0;
 }
 arch_initcall(mrst_platform_init);
-- 
1.7.0.4

Re: [PATCH 4/4] RFC Thermal: Platform layer changes to provide thermal data

[Eduardo Valentin]

Hello Rui,

On Mon, Jun 11, 2012 at 11:09:34PM +0530, Durgadoss R wrote:
> This patch shows how can we add platform specific thermal data
> required by the enhanced thermal framework.
> This is just an example patch, and _not_ for merge.

Nice! Thanks for sending an example. This makes things a lot easier.

> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  arch/x86/platform/mrst/mrst.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/platform/mrst/mrst.c b/arch/x86/platform/mrst/mrst.c
> index e31bcd8..eeb3cd0 100644
> --- a/arch/x86/platform/mrst/mrst.c
> +++ b/arch/x86/platform/mrst/mrst.c
> @@ -30,6 +30,7 @@
>  #include <linux/mfd/intel_msic.h>
>  #include <linux/gpio.h>
>  #include <linux/i2c/tc35876x.h>
> +#include <linux/thermal.h>
>  
>  #include <asm/setup.h>
>  #include <asm/mpspec_def.h>
> @@ -78,6 +79,27 @@ struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];
>  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
>  int sfi_mrtc_num;
>  
> +#define MRST_THERMAL_ZONES	3
> +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> +	{ .thermal_zone_name = "CPU",
> +	.throttle_policy = THERMAL_FAIR_SHARE,
> +	.num_cdevs = 2,
> +	.cdevs_name = {"CPU", "Battery"},
> +	.weights = {80, 20}, },
> +
> +	{ .thermal_zone_name = "Battery",
> +	.throttle_policy = THERMAL_FAIR_SHARE,
> +	.num_cdevs = 1,
> +	.cdevs_name = {"Battery"},
> +	.weights = {100}, },
> +
> +	{ .thermal_zone_name = "Skin",
> +	.throttle_policy = THERMAL_FAIR_SHARE,
> +	.num_cdevs = 2,
> +	.cdevs_name = {"Display", "Battery"},
> +	.weights = {50, 50}, }
> +};

Please consider the ordering comment I sent on your patch 01/04.

> +
>  static void mrst_power_off(void)
>  {
>  }
> @@ -983,10 +1005,27 @@ static int __init sfi_parse_devs(struct sfi_table_header *table)
>  	return 0;
>  }
>  
> +static int mrst_get_thermal_params(struct thermal_zone_device *tz)
> +{
> +	int i;
> +
> +	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
> +		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
> +			tz->tzp = &tzp[i];
> +			return 0;
> +		}
> +	}

If you send the pdata while registering the zone, and store it in the zone data structures,
you wont need to be iterating on all pdata array and matching strings.

Besides, I suppose the pdata you are talking about it actually board specifics, right?

> +	return -ENODEV;
> +}
> +
>  static int __init mrst_platform_init(void)
>  {
>  	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
>  	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
> +
> +	/* Set platform thermal data pointer */
> +	get_platform_thermal_params = mrst_get_thermal_params;
> +
>  	return 0;
>  }
>  arch_initcall(mrst_platform_init);
> -- 
> 1.7.0.4
> 

RE: [PATCH 4/4] RFC Thermal: Platform layer changes to provide thermal data

[R, Durgadoss]

Hi Eduardo,

> 
> Hello Rui,

I hope you meant Durga :-)

> 
> On Mon, Jun 11, 2012 at 11:09:34PM +0530, Durgadoss R wrote:
> > This patch shows how can we add platform specific thermal data
> > required by the enhanced thermal framework.
> > This is just an example patch, and _not_ for merge.
> 
> Nice! Thanks for sending an example. This makes things a lot easier.

Happy, that it is useful.
I initially thought of dropping it, but luckily did not :-)

[A big cut.]

> > +static int mrst_get_thermal_params(struct thermal_zone_device *tz)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
> > +		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
> > +			tz->tzp = &tzp[i];
> > +			return 0;
> > +		}
> > +	}
> 
> If you send the pdata while registering the zone, and store it in the zone data
> structures,
> you wont need to be iterating on all pdata array and matching strings.
> 

Even if we have this data coming while registering the zone, that platform
specific thermal driver has to fetch the data from a board specific file.
I understand that we can hard code things inside the platform thermal driver
file, but in most cases people use some standard thermal sensor chips
(for which drivers are available in Linux). So, these chips can be used on
different platforms, with different platform data.

So, I would prefer having this inside a platform specific file, and not hard code
inside any thermal driver.

> Besides, I suppose the pdata you are talking about it actually board specifics,
> right?

Yes. It is board specific, but as I mentioned above, the driver may be a generic one.

Thanks,
Durga

Re: [PATCH 0/4] Thermal Framework Enhancements

[Zhang Rui]

On 一, 2012-06-11 at 23:09 +0530, Durgadoss R wrote:
> This patch series attempts to add some simple governors/
> throttling algorithms to the generic thermal layer.

Oh, this is something what I want to do, but not sure when.
Thanks, Durga.

> Although this patch set creates simple governors which depend
> on the platform data provided, we can start here, and write
> some really smart algorithms that will do the wonder!!
> 
> Patch 1/4: Creates necessary infrastructure required to
> 	   add throttling algorithms in thermal_sys.c
> 	   1. Introduces the get_trend callback
seems duplicate work of my patch set. :(

> 	   2. Introduces the notify_thermal_framework() API

> 	   3. Exposes sysfs to show/store the throttle_policy
> 
yes, I think we need this.

> Patch 2/4: Introduce fair_share governor
> 	   This throttles the cooling_devices according to their
> 	   weights (and hence the name; Suggestion are welcome :-).
> 	   The weights in turn describe the effectiveness of a
> 	   particular cooling device in cooling a thermal zone.
> 

> Patch 3/4: Introduce step_wise governor
> 	   This throttles/de-throttles the cooling devices one
> 	   step at a time. This is exactly similar to the code
> 	   we have in thermal_zone_device_update function. The
> 	   intention is to move all 'throttling logic' related
> 	   code outside thermal_sys.c and keep them separate.

totally agree.
This is in my TODO list as well. :)

BTW, we may need an easy "userspace" governor as well.

> 
> Patch 4/4: Platform data patch
> 	   This patch is not for merge. Just as an example to
> 	   show how we can provide platform data to thermal
> 	   framework. Not that we do not know how to fill structures,
> 	   I felt the patch set is in-complete without this. That's
> 	   why it is here. From next versions, I will ignore this one.
> 
> TODO on these patch sets:
> * Sync up with Rui's latest patches

agreed.

> * Add more protection and tidy up the existing ones
> * Expose the weights and cooling devices through sysfs (Read-Only)

what do you mean "cooling devices"?

> * Remove all throttling related code(if we all agree) from thermal_sys.c

Ack!

> * In fair_share, before setting new state, check if there are other zones,
>   which do not want the 'state' to be changed.

Yep, I think this is handled in my arbitrator patch. :p

>   (To do this, we have to loop through the thermal_tz_list and 
>   thermal_cdev_list inside fair_share.c. Need to see how good it is
>   to make this lists public)

IMO, these governors should just update the thermal_instance, and then
invokes thermal_zone_do_update(). They should not change the cooling
state directly.

> * If we all agree, use step_wise and remove linear_throttle from thermal_sys.c

agreed.

> * Enhance notify_user_space(), so that the use land can extract some sane
>   information out of UEvent.
> 
> WishList:
> * Find a way to provide platform data so that we can map cooling devices
>   for a trip point in a thermal zone.

hmm, what information do we need except "weight"?
I think we can register the weight information when do the binding, like
I did for "upper" and "lower" limits.

> * Make all _throttle methods have same signature. This way we can make use
>   of function pointers and make the code a bit simpler.

what does signature mean?

> * The simple governors heavily depend on the platform data provided. We
>   can think of some really smart logic, that depends on very minimal platform
>   data, and do things on its own :-)
> * Make other subsystem core files register with the thermal framework as a
>   thermal sensor or as a cooling device as appropriate.
>   (I am thinking of Power Supply, Video, cpufreq for now)

about cpufreq, I think Amit has some work on this. hah

> * Ah, Find time to do all this !
> 
me too. :)

Again, thanks for your work, they are good ideas.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/4] Thermal Framework Enhancements

[R, Durgadoss]

Thanks Rui for taking a Quick Look.
I am not replying to all your comments, because most of them look fine to me.

[A big cut]

> > 	   1. Introduces the get_trend callback
> seems duplicate work of my patch set. :(

Yes I agree. I was in the middle of testing yesterday,
so could not change it before today.
I will review your 12 patches, and pull in here the needed ones.
So, my next patch set will have those changes, and they don’t conflict.

> 
> > 	   2. Introduces the notify_thermal_framework() API
> 
> > 	   3. Exposes sysfs to show/store the throttle_policy
> >
> yes, I think we need this.
> 
> > Patch 2/4: Introduce fair_share governor
> > 	   This throttles the cooling_devices according to their
> > 	   weights (and hence the name; Suggestion are welcome :-).
> > 	   The weights in turn describe the effectiveness of a
> > 	   particular cooling device in cooling a thermal zone.
> >
> 
> > Patch 3/4: Introduce step_wise governor
> > 	   This throttles/de-throttles the cooling devices one
> > 	   step at a time. This is exactly similar to the code
> > 	   we have in thermal_zone_device_update function. The
> > 	   intention is to move all 'throttling logic' related
> > 	   code outside thermal_sys.c and keep them separate.
> 
> totally agree.

Oh that’s nice. Will incorporate this change in my next patch set.
Thank you for this.

> > * Add more protection and tidy up the existing ones
> > * Expose the weights and cooling devices through sysfs (Read-Only)
> 
> what do you mean "cooling devices"?

I meant for a thermal zone, we should expose:
one Sysfs: that will list the weights of all cooling devices associated with this
other Sysfs: that will list the names of cooling devices (in the same order as
weights)
This is just to let user land know of the binding
I should have put 'names of cooling devices'.

> 
> Yep, I think this is handled in my arbitrator patch. :p

ok...I have not looked at it yet.
Will have a look and incorporate the change in next patch set.

> 
> >   (To do this, we have to loop through the thermal_tz_list and
> >   thermal_cdev_list inside fair_share.c. Need to see how good it is
> >   to make this lists public)
> 
> IMO, these governors should just update the thermal_instance, and then
> invokes thermal_zone_do_update(). They should not change the cooling
> state directly.
> 

Give me some time to think about this one, will come back to you :-)

> > * Make all _throttle methods have same signature. This way we can make use
> >   of function pointers and make the code a bit simpler.
> 
> what does signature mean?

Oh I meant they are all same 'taking similar arguments and same return type'.
This way we can use an arr[function ptrs] that will be populated in each governor's
init. This way inside the notify_framework method we don’t so many case statements.
We could just do arr[throttle_policy](...)

Thanks,
Durga

RE: [PATCH 0/4] Thermal Framework Enhancements

[Zhang Rui]

On 二, 2012-06-12 at 02:59 -0600, R, Durgadoss wrote:
> Thanks Rui for taking a Quick Look.
> I am not replying to all your comments, because most of them look fine to me.
> 
> [A big cut]
> 
> > > 	   1. Introduces the get_trend callback
> > seems duplicate work of my patch set. :(
> 
> Yes I agree. I was in the middle of testing yesterday,
> so could not change it before today.
> I will review your 12 patches, and pull in here the needed ones.
> So, my next patch set will have those changes, and they don’t conflict.
> 
> > 
> > > 	   2. Introduces the notify_thermal_framework() API
> > 
> > > 	   3. Exposes sysfs to show/store the throttle_policy
> > >
> > yes, I think we need this.
> > 
> > > Patch 2/4: Introduce fair_share governor
> > > 	   This throttles the cooling_devices according to their
> > > 	   weights (and hence the name; Suggestion are welcome :-).
> > > 	   The weights in turn describe the effectiveness of a
> > > 	   particular cooling device in cooling a thermal zone.
> > >
> > 
> > > Patch 3/4: Introduce step_wise governor
> > > 	   This throttles/de-throttles the cooling devices one
> > > 	   step at a time. This is exactly similar to the code
> > > 	   we have in thermal_zone_device_update function. The
> > > 	   intention is to move all 'throttling logic' related
> > > 	   code outside thermal_sys.c and keep them separate.
> > 
> > totally agree.
> 
> Oh that’s nice. Will incorporate this change in my next patch set.
> Thank you for this.
> 
> > > * Add more protection and tidy up the existing ones
> > > * Expose the weights and cooling devices through sysfs (Read-Only)
> > 
> > what do you mean "cooling devices"?
> 
> I meant for a thermal zone, we should expose:
> one Sysfs: that will list the weights of all cooling devices associated with this
> other Sysfs: that will list the names of cooling devices (in the same order as
> weights)
> This is just to let user land know of the binding
> I should have put 'names of cooling devices'.
> 
so I have two questions,
1. should kernel thermal manager handle these weights?
2. if yes, IMO, we can add one field in thermal_instance to describe
this weight. and then, the problem to me is that, we should easily
introduce an attribute for each cooling device instance, say,
/sys/class/thermal/thermal_zone0/cdev0_trip_point
                                /cdev0_weight
...

> > 
> > Yep, I think this is handled in my arbitrator patch. :p
> 
> ok...I have not looked at it yet.
> Will have a look and incorporate the change in next patch set.
> 
> > 
> > >   (To do this, we have to loop through the thermal_tz_list and
> > >   thermal_cdev_list inside fair_share.c. Need to see how good it is
> > >   to make this lists public)
> > 
> > IMO, these governors should just update the thermal_instance, and then
> > invokes thermal_zone_do_update(). They should not change the cooling
> > state directly.
> > 
> 
> Give me some time to think about this one, will come back to you :-)
> 
> > > * Make all _throttle methods have same signature. This way we can make use
> > >   of function pointers and make the code a bit simpler.
> > 
> > what does signature mean?
> 
> Oh I meant they are all same 'taking similar arguments and same return type'.
> This way we can use an arr[function ptrs] that will be populated in each governor's
> init. This way inside the notify_framework method we don’t so many case statements.
> We could just do arr[throttle_policy](...)
> 
okay.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Thermal Framework Enhancements

[Eduardo Valentin]

Hello,

On Mon, Jun 11, 2012 at 11:09:30PM +0530, Durgadoss R wrote:
> This patch series attempts to add some simple governors/
> throttling algorithms to the generic thermal layer.
> Although this patch set creates simple governors which depend
> on the platform data provided, we can start here, and write
> some really smart algorithms that will do the wonder!!
> 
> Patch 1/4: Creates necessary infrastructure required to
> 	   add throttling algorithms in thermal_sys.c
> 	   1. Introduces the get_trend callback
> 	   2. Introduces the notify_thermal_framework() API
> 	   3. Exposes sysfs to show/store the throttle_policy

You forgot to mention the linear throttle and the notification
mechanism that you have introduced.

As I mentioned in the patch itself, I'd propose to split it into smaller changes.

> 
> Patch 2/4: Introduce fair_share governor
> 	   This throttles the cooling_devices according to their
> 	   weights (and hence the name; Suggestion are welcome :-).
> 	   The weights in turn describe the effectiveness of a
> 	   particular cooling device in cooling a thermal zone.

For the purpose you mentioned the name sounds Ok.

On the other hand, Now I got a bit confused on this strategy.

Do you still keep the trip to cooling device binding constraints?
Does it make sense to have this binding coming from the pdata
description as well?

> 
> Patch 3/4: Introduce step_wise governor
> 	   This throttles/de-throttles the cooling devices one
> 	   step at a time. This is exactly similar to the code
> 	   we have in thermal_zone_device_update function. The
> 	   intention is to move all 'throttling logic' related
> 	   code outside thermal_sys.c and keep them separate.
> 
> Patch 4/4: Platform data patch
> 	   This patch is not for merge. Just as an example to
> 	   show how we can provide platform data to thermal
> 	   framework. Not that we do not know how to fill structures,
> 	   I felt the patch set is in-complete without this. That's
> 	   why it is here. From next versions, I will ignore this one.
> 
> TODO on these patch sets:
> * Sync up with Rui's latest patches
> * Add more protection and tidy up the existing ones
> * Expose the weights and cooling devices through sysfs (Read-Only)
> * Remove all throttling related code(if we all agree) from thermal_sys.c

I do. +1

> * In fair_share, before setting new state, check if there are other zones,
>   which do not want the 'state' to be changed.
>   (To do this, we have to loop through the thermal_tz_list and 
>   thermal_cdev_list inside fair_share.c. Need to see how good it is
>   to make this lists public)
> * If we all agree, use step_wise and remove linear_throttle from thermal_sys.c

Well, I guess I need first to understand the difference between those two.
For instance, does it make sense to have a separate file for linear_throttle?

> * Enhance notify_user_space(), so that the use land can extract some sane
>   information out of UEvent.
> 
> WishList:
> * Find a way to provide platform data so that we can map cooling devices
>   for a trip point in a thermal zone.

Ohh ok.. Indeed, we need a way to describe the mappings and bindings.
I am not sure how that goes into ACPI, but I guess it comes from firmware.

On non-ACPI world we still need to have that mapped somehow.

Your weighting patch is at least one attempt to start doing this mapping.
I guess we need more brainstorming here..

> * Make all _throttle methods have same signature. This way we can make use
>   of function pointers and make the code a bit simpler.
> * The simple governors heavily depend on the platform data provided. We
>   can think of some really smart logic, that depends on very minimal platform
>   data, and do things on its own :-)

Not sure what exactly you meant here..

> * Make other subsystem core files register with the thermal framework as a
>   thermal sensor or as a cooling device as appropriate.
>   (I am thinking of Power Supply, Video, cpufreq for now)
> * Ah, Find time to do all this !

Heh... I suppose, as I mentioned before, we need to be more structured and
organize tree/branches and split the work.

I will send a couple of clean up patches as starters.

> 
> Durgadoss R (4):
>   RFC Thermal: Enhance Generic Thermal layer with policies
>   RFC Thermal: Introduce fair-share thermal governor
>   RFC Thermal: Introduce a step-wise thermal governor
>   RFC Thermal: Platform layer changes to provide thermal data
> 
>  Documentation/thermal/sysfs-api.txt |   26 +++
>  arch/x86/platform/mrst/mrst.c       |   39 ++++
>  drivers/thermal/Kconfig             |   13 ++
>  drivers/thermal/Makefile            |    4 +-
>  drivers/thermal/fair_share.c        |  111 ++++++++++
>  drivers/thermal/step_wise.c         |   86 ++++++++
>  drivers/thermal/thermal_sys.c       |  383 ++++++++++++++++++++++++++---------
>  include/linux/thermal.h             |   55 +++++
>  8 files changed, 620 insertions(+), 97 deletions(-)
>  create mode 100644 drivers/thermal/fair_share.c
>  create mode 100644 drivers/thermal/step_wise.c
> 

RE: [PATCH 0/4] Thermal Framework Enhancements

[R, Durgadoss]

Hi Eduardo,

> 
> >
> > Patch 2/4: Introduce fair_share governor
> > 	   This throttles the cooling_devices according to their
> > 	   weights (and hence the name; Suggestion are welcome :-).
> > 	   The weights in turn describe the effectiveness of a
> > 	   particular cooling device in cooling a thermal zone.
> 
> For the purpose you mentioned the name sounds Ok.
> 
> On the other hand, Now I got a bit confused on this strategy.
> 
> Do you still keep the trip to cooling device binding constraints?
> Does it make sense to have this binding coming from the pdata
> description as well?

Yes it makes sense.
Now that we can have multiple active and passive trip points,
providing 'per-trip point binding' tends to become complex [1].

> > * Add more protection and tidy up the existing ones
> > * Expose the weights and cooling devices through sysfs (Read-Only)
> > * Remove all throttling related code(if we all agree) from thermal_sys.c
> 
> I do. +1

Thank you.

> > * If we all agree, use step_wise and remove linear_throttle from thermal_sys.c
> 
> Well, I guess I need first to understand the difference between those two.
> For instance, does it make sense to have a separate file for linear_throttle?

There is no difference. I remember specifying it in one of the comments in the
patches.

I was not sure about our opinions on separating throttle logic from thermal_sys.c
(even for simple linear throttle). That's why kept both of them here.

Now that we agree, I will remove throttling code inside thermal_sys.c,
and keep only step_wise_throttle. I hope this will make things clear,
and I will be able to split patches in a better way and make them smaller.
Will do this in my next patch set.

> > * Find a way to provide platform data so that we can map cooling devices
> >   for a trip point in a thermal zone.
> 
> Ohh ok.. Indeed, we need a way to describe the mappings and bindings.
> I am not sure how that goes into ACPI, but I guess it comes from firmware.
> 
> On non-ACPI world we still need to have that mapped somehow.

[1]
I thought through this, and (to me) it looks like a 3D mapping between
thermal zones -> cooling devices -> trip points of a thermal zone.
So, I simplified it and ended up with this 'weight' approach for fair_share
This can be a start, and we can take it forward from here..

I think this is a separate topic in itself, which would require one more
complete mail chain :-)

> 
> Your weighting patch is at least one attempt to start doing this mapping.
> I guess we need more brainstorming here..

Yes. Agree with you.

Thanks,
Durga