[PATCH 0/4] thermal: Adding generic cpu cooling devices

[PATCH 0/4] thermal: Adding generic cpu cooling devices

[Amit Daniel Kachhap]

Changes since RFC:
 *Changed the cpu cooling registration/unregistration API's to instance based
 *Changed the STATE_ACTIVE trip type to pass correct instance id
 *Adding support to restore back the policy->max_freq after doing frequency 
  clipping.
 *Moved the trip cooling stats from sysfs node to debugfs node as suggested
  by Greg KH greg@kroah.com 
 *Incorporated several review comments from eduardo.valentin@ti.com

Todo:
 *Report time spent in each trip point along with the cooling statistics
 *Add opp library support in cpufreq cooling api's

Brief Description:

1) The generic cooling devices code is placed inside driver/thermal/* as 
placing inside acpi folder will need un-necessary enabling of acpi code.

2) This patchset adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
cooling device instance number and may be helpful for cpufreq cooling devices
to take the correct cooling action.

3) This patchset adds generic cpu cooling low level implementation through
frequency clipping and cpu hotplug. In future, other cpu related cooling
devices may be added here. An ACPI version of this already exists
(drivers/acpi/processor_thermal.c). But this will be useful for platforms
like ARM using the generic thermal interface along with the generic cpu
cooling devices. The cooling device registration API's return cooling device
pointers which can be easily binded with the thermal zone trip points.

4)This patchset provides a simple way of reporting cooling achieved in a
trip type. This will help in fine cooling the cooling devices attached.

Amit Daniel Kachhap (4):
  thermal: Add a new trip type to use cooling device instance number
  thermal: Add generic cpufreq cooling implementation
  thermal: Add generic cpuhotplug cooling implementation
  thermal: Add support to report cooling statistics achieved by cooling
    devices

 Documentation/thermal/cpu-cooling-api.txt |   57 ++++
 Documentation/thermal/sysfs-api.txt       |    4 +-
 drivers/thermal/Kconfig                   |   11 +
 drivers/thermal/Makefile                  |    1 +
 drivers/thermal/cpu_cooling.c             |  484 +++++++++++++++++++++++++++++
 drivers/thermal/thermal_sys.c             |  165 ++++++++++-
 include/linux/cpu_cooling.h               |   71 +++++
 include/linux/thermal.h                   |   14 +
 8 files changed, 802 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/thermal/cpu-cooling-api.txt
 create mode 100644 drivers/thermal/cpu_cooling.c
 create mode 100644 include/linux/cpu_cooling.h

[PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

[Amit Daniel Kachhap]

This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
device instance number. This helps the cooling device registered as
different instances to perform appropriate cooling action decision in
the set_cur_state call back function.

Also since the trip temperature's are in ascending order so some logic
is put in place to skip the un-necessary checks.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
---
 Documentation/thermal/sysfs-api.txt |    4 +-
 drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--
 include/linux/thermal.h             |    1 +
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 1733ab9..7a0c080 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -184,8 +184,8 @@ trip_point_[0-*]_temp
 
 trip_point_[0-*]_type
 	Strings which indicate the type of the trip point.
-	E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
-	thermal zone.
+	E.g. it can be one of critical, hot, passive, active[0-1],
+	state-active[0-*] for ACPI thermal zone.
 	RO, Optional
 
 cdev[0-*]
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 220ce7e..d4c9b20 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
 		return sprintf(buf, "passive\n");
 	case THERMAL_TRIP_ACTIVE:
 		return sprintf(buf, "active\n");
+	case THERMAL_TRIP_STATE_ACTIVE:
+		return sprintf(buf, "state-active\n");
 	default:
 		return sprintf(buf, "unknown\n");
 	}
@@ -1034,10 +1036,10 @@ 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, ret = 0, inst_id;
+	long temp, trip_temp, max_state, last_trip_change = 0;
 	enum thermal_trip_type trip_type;
-	struct thermal_cooling_device_instance *instance;
+	struct thermal_cooling_device_instance *instance, *state_instance;
 	struct thermal_cooling_device *cdev;
 
 	mutex_lock(&tz->lock);
@@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 					cdev->ops->set_cur_state(cdev, 0);
 			}
 			break;
+		case THERMAL_TRIP_STATE_ACTIVE:
+			list_for_each_entry(instance, &tz->cooling_devices,
+					    node) {
+				if (instance->trip != count)
+					continue;
+
+				if (temp <= last_trip_change)
+					continue;
+
+				inst_id = 0;
+				/*
+				*For this instance how many instance of same
+				*cooling device occured before
+				*/
+
+				list_for_each_entry(state_instance,
+						&tz->cooling_devices, node) {
+					if (instance->cdev ==
+							state_instance->cdev)
+						inst_id++;
+					if (state_instance->trip == count)
+						break;
+				}
+
+				cdev = instance->cdev;
+				cdev->ops->get_max_state(cdev, &max_state);
+
+				if ((temp >= trip_temp) &&
+						(inst_id <= max_state))
+					cdev->ops->set_cur_state(cdev, inst_id);
+				else if ((temp < trip_temp) &&
+						(--inst_id <= max_state))
+					cdev->ops->set_cur_state(cdev, inst_id);
+
+				last_trip_change = trip_temp;
+			}
+			break;
 		case THERMAL_TRIP_PASSIVE:
 			if (temp >= trip_temp || tz->passive)
 				thermal_zone_device_passive(tz, temp,
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 796f1ff..8df901f 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -42,6 +42,7 @@ enum thermal_trip_type {
 	THERMAL_TRIP_PASSIVE,
 	THERMAL_TRIP_HOT,
 	THERMAL_TRIP_CRITICAL,
+	THERMAL_TRIP_STATE_ACTIVE,
 };
 
 struct thermal_zone_device_ops {
-- 
1.7.1

[PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Amit Daniel Kachhap]

This patch adds support for generic cpu thermal cooling low level
implementations using frequency scaling up/down based on the request
from user. Different cpu related cooling devices can be registered by the
user and the binding of these cooling devices to the corresponding
trip points can be easily done as the registration API's return the
cooling device pointer. The user of these api's are responsible for
passing clipping frequency in percentages.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
---
 Documentation/thermal/cpu-cooling-api.txt |   40 ++++
 drivers/thermal/Kconfig                   |   11 +
 drivers/thermal/Makefile                  |    1 +
 drivers/thermal/cpu_cooling.c             |  310 +++++++++++++++++++++++++++++
 include/linux/cpu_cooling.h               |   54 +++++
 5 files changed, 416 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/thermal/cpu-cooling-api.txt
 create mode 100644 drivers/thermal/cpu_cooling.c
 create mode 100644 include/linux/cpu_cooling.h

diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
new file mode 100644
index 0000000..04de67c
--- /dev/null
+++ b/Documentation/thermal/cpu-cooling-api.txt
@@ -0,0 +1,40 @@
+CPU cooling api's How To
+===================================
+
+Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
+
+Updated: 13 Dec 2011
+
+Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
+
+0. Introduction
+
+The generic cpu cooling(freq clipping, cpuhotplug) provides
+registration/unregistration api's to the user. The binding of the cooling
+devices to the trip point is left for the user. The registration api's returns
+the cooling device pointer.
+
+1. cpufreq cooling api's
+
+1.1 cpufreq registration api's
+1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val)
+
+    This interface function registers the cpufreq cooling device with the name
+    "thermal-cpufreq-%x". This api can support multiple instance of cpufreq cooling
+    devices.
+
+    tab_ptr: The table containing the percentage of frequency to be clipped for
+    each cooling state.
+	.freq_clip_pctg: Percentage of frequency to be clipped for each allowed
+	 cpus.
+	.polling_interval: polling interval for this cooling state.
+    tab_size: the total number of cooling state.
+    mask_val: all the allowed cpu's where frequency clipping can happen.
+
+1.1.2 void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
+
+    This interface function unregisters the "thermal-cpufreq-%x" cooling device.
+
+    cdev: Cooling device pointer which has to be unregistered.
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index f7f71b2..298c1cd 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -18,3 +18,14 @@ config THERMAL_HWMON
 	depends on THERMAL
 	depends on HWMON=y || HWMON=THERMAL
 	default y
+
+config CPU_THERMAL
+	bool "generic cpu cooling support"
+	depends on THERMAL
+	help
+	  This implements the generic cpu cooling mechanism through frequency
+	  reduction, cpu hotplug and any other ways of reducing temperature. An
+	  ACPI version of this already exists(drivers/acpi/processor_thermal.c).
+	  This will be useful for platforms using the generic thermal interface
+	  and not the ACPI interface.
+	  If you want this support, you should say Y or M here.
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 31108a0..655cbc4 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
+obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
new file mode 100644
index 0000000..298f550
--- /dev/null
+++ b/drivers/thermal/cpu_cooling.c
@@ -0,0 +1,310 @@
+/*
+ *  linux/drivers/thermal/cpu_cooling.c
+ *
+ *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
+ *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  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.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+#include <linux/cpufreq.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/cpu.h>
+#include <linux/cpu_cooling.h>
+
+#ifdef CONFIG_CPU_FREQ
+struct cpufreq_cooling_device {
+	int id;
+	struct thermal_cooling_device *cool_dev;
+	struct freq_pctg_table *tab_ptr;
+	unsigned int tab_size;
+	unsigned int cpufreq_state;
+	const struct cpumask *allowed_cpus;
+	struct list_head node;
+};
+
+static LIST_HEAD(cooling_cpufreq_list);
+static DEFINE_MUTEX(cooling_cpufreq_lock);
+static DEFINE_IDR(cpufreq_idr);
+static struct cpufreq_cooling_device *notify_cpufreq;
+static DEFINE_PER_CPU(unsigned int, max_policy_freq);
+#endif /*CONFIG_CPU_FREQ*/
+
+static int get_idr(struct idr *idr, struct mutex *lock, int *id)
+{
+	int err;
+again:
+	if (unlikely(idr_pre_get(idr, GFP_KERNEL) == 0))
+		return -ENOMEM;
+
+	if (lock)
+		mutex_lock(lock);
+	err = idr_get_new(idr, NULL, id);
+	if (lock)
+		mutex_unlock(lock);
+	if (unlikely(err == -EAGAIN))
+		goto again;
+	else if (unlikely(err))
+		return err;
+
+	*id = *id & MAX_ID_MASK;
+	return 0;
+}
+
+static void release_idr(struct idr *idr, struct mutex *lock, int id)
+{
+	if (lock)
+		mutex_lock(lock);
+	idr_remove(idr, id);
+	if (lock)
+		mutex_unlock(lock);
+}
+
+#ifdef CONFIG_CPU_FREQ
+/*Below codes defines functions to be used for cpufreq as cooling device*/
+static bool is_cpufreq_valid(int cpu)
+{
+	struct cpufreq_policy policy;
+	if (!cpufreq_get_policy(&policy, cpu))
+		return true;
+	return false;
+}
+
+static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
+				unsigned long cooling_state)
+{
+	int cpuid, this_cpu = smp_processor_id();
+
+	if (!is_cpufreq_valid(this_cpu))
+		return 0;
+
+	if (cooling_state > cpufreq_device->tab_size)
+		return -EINVAL;
+
+	/*Check if last cooling level is same as current cooling level*/
+	if (cpufreq_device->cpufreq_state == cooling_state)
+		return 0;
+
+	cpufreq_device->cpufreq_state = cooling_state;
+
+	/*cpufreq thermal notifier uses this cpufreq device pointer*/
+	notify_cpufreq = cpufreq_device;
+
+	for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
+		if (is_cpufreq_valid(cpuid))
+			cpufreq_update_policy(cpuid);
+	}
+
+	return 0;
+}
+
+static int thermal_cpufreq_notifier(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct cpufreq_policy *policy = data;
+	struct freq_pctg_table *th_table;
+	unsigned long max_freq = 0;
+	unsigned int th_pctg = 0, level;
+
+	if (event != CPUFREQ_ADJUST)
+		return 0;
+
+	if (!notify_cpufreq)
+		return 0;
+
+	level = notify_cpufreq->cpufreq_state;
+
+	if (level > 0) {
+		th_table =
+			&(notify_cpufreq->tab_ptr[level - 1]);
+		th_pctg = th_table->freq_clip_pctg;
+		max_freq =
+		(policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
+
+		if (per_cpu(max_policy_freq, policy->cpu) == 0)
+			per_cpu(max_policy_freq, policy->cpu) = policy->max;
+	} else {
+		if (per_cpu(max_policy_freq, policy->cpu) != 0) {
+			max_freq = per_cpu(max_policy_freq, policy->cpu);
+			per_cpu(max_policy_freq, policy->cpu) = 0;
+		} else {
+			max_freq = policy->max;
+		}
+	}
+
+	cpufreq_verify_within_limits(policy, 0, max_freq);
+
+	return 0;
+}
+
+/*
+ * cpufreq cooling device callback functions
+ */
+static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct cpufreq_cooling_device *cpufreq_device = NULL;
+
+	mutex_lock(&cooling_cpufreq_lock);
+	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
+		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
+			break;
+
+	mutex_unlock(&cooling_cpufreq_lock);
+	if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
+		return -EINVAL;
+
+	*state = cpufreq_device->tab_size;
+	return 0;
+}
+
+static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct cpufreq_cooling_device *cpufreq_device = NULL;
+
+	mutex_lock(&cooling_cpufreq_lock);
+	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
+		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
+			break;
+
+	mutex_unlock(&cooling_cpufreq_lock);
+	if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
+		return -EINVAL;
+	*state = cpufreq_device->cpufreq_state;
+	return 0;
+}
+
+/*This cooling may be as PASSIVE/ACTIVE type*/
+static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long state)
+{
+	struct cpufreq_cooling_device *cpufreq_device = NULL;
+
+	mutex_lock(&cooling_cpufreq_lock);
+	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
+		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
+			break;
+
+	mutex_unlock(&cooling_cpufreq_lock);
+	if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
+		return -EINVAL;
+
+	cpufreq_apply_cooling(cpufreq_device, state);
+	return 0;
+}
+
+/* bind cpufreq callbacks to cpufreq cooling device */
+static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
+	.get_max_state = cpufreq_get_max_state,
+	.get_cur_state = cpufreq_get_cur_state,
+	.set_cur_state = cpufreq_set_cur_state,
+};
+
+static struct notifier_block thermal_cpufreq_notifier_block = {
+	.notifier_call = thermal_cpufreq_notifier,
+};
+
+struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val)
+{
+	struct thermal_cooling_device *cool_dev;
+	struct cpufreq_cooling_device *cpufreq_dev = NULL;
+	unsigned int count = 0;
+	char dev_name[THERMAL_NAME_LENGTH];
+	int ret = 0, id = 0;
+
+	if (tab_ptr == NULL || tab_size == 0)
+		return ERR_PTR(-EINVAL);
+
+	list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node)
+		count++;
+
+	cpufreq_dev =
+		kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
+
+	if (!cpufreq_dev)
+		return ERR_PTR(-ENOMEM);
+
+	cpufreq_dev->tab_ptr = tab_ptr;
+	cpufreq_dev->tab_size = tab_size;
+	cpufreq_dev->allowed_cpus = mask_val;
+
+	ret = get_idr(&cpufreq_idr, &cooling_cpufreq_lock, &cpufreq_dev->id);
+	if (ret) {
+		kfree(cpufreq_dev);
+		return ERR_PTR(-EINVAL);
+	}
+
+	sprintf(dev_name, "thermal-cpufreq-%d", cpufreq_dev->id);
+
+	cool_dev = thermal_cooling_device_register(dev_name, cpufreq_dev,
+						&cpufreq_cooling_ops);
+	if (!cool_dev) {
+		release_idr(&cpufreq_idr, &cooling_cpufreq_lock,
+						cpufreq_dev->id);
+		kfree(cpufreq_dev);
+		return ERR_PTR(-EINVAL);
+	}
+	cpufreq_dev->id = id;
+	cpufreq_dev->cool_dev = cool_dev;
+	mutex_lock(&cooling_cpufreq_lock);
+	list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list);
+	mutex_unlock(&cooling_cpufreq_lock);
+
+	if (count == 0)
+		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
+						CPUFREQ_POLICY_NOTIFIER);
+	return cool_dev;
+}
+EXPORT_SYMBOL(cpufreq_cooling_register);
+
+void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
+{
+	struct cpufreq_cooling_device *cpufreq_dev = NULL;
+	unsigned int count = 0;
+
+	mutex_lock(&cooling_cpufreq_lock);
+	list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
+		if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
+			break;
+		count++;
+	}
+
+	if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
+		mutex_unlock(&cooling_cpufreq_lock);
+		return;
+	}
+
+	list_del(&cpufreq_dev->node);
+	mutex_unlock(&cooling_cpufreq_lock);
+
+	if (count == 1)
+		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
+					CPUFREQ_POLICY_NOTIFIER);
+
+	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
+	release_idr(&cpufreq_idr, &cooling_cpufreq_lock, cpufreq_dev->id);
+	kfree(cpufreq_dev);
+}
+EXPORT_SYMBOL(cpufreq_cooling_unregister);
+#endif /*CONFIG_CPU_FREQ*/
diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
new file mode 100644
index 0000000..5dc5632
--- /dev/null
+++ b/include/linux/cpu_cooling.h
@@ -0,0 +1,54 @@
+/*
+ *  linux/include/linux/cpu_cooling.h
+ *
+ *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
+ *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  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.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef __CPU_COOLING_H__
+#define __CPU_COOLING_H__
+
+#include <linux/thermal.h>
+
+struct freq_pctg_table {
+	unsigned int freq_clip_pctg;
+	unsigned int polling_interval;
+};
+
+#ifdef CONFIG_CPU_FREQ
+struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val);
+
+void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
+#else /*!CONFIG_CPU_FREQ*/
+static inline struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val)
+{
+	return NULL;
+}
+static inline void cpufreq_cooling_unregister(
+				struct thermal_cooling_device *cdev)
+{
+	return;
+}
+#endif	/*CONFIG_CPU_FREQ*/
+
+#endif /* __CPU_COOLING_H__ */
-- 
1.7.1

[PATCH 3/4] thermal: Add generic cpuhotplug cooling implementation

[Amit Daniel Kachhap]

This patch adds support for generic cpu thermal cooling low level
implementations using cpuhotplug based on the thermal level requested
from user. Different cpu related cooling devices can be registered by the
user and the binding of these cooling devices to the corresponding
trip points can be easily done as the registration API's return the
cooling device pointer. The user of these api's are responsible for
passing the cpumask.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
---
 Documentation/thermal/cpu-cooling-api.txt |   17 +++
 drivers/thermal/cpu_cooling.c             |  174 +++++++++++++++++++++++++++++
 include/linux/cpu_cooling.h               |   17 +++
 3 files changed, 208 insertions(+), 0 deletions(-)

diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
index 04de67c..bdaf509 100644
--- a/Documentation/thermal/cpu-cooling-api.txt
+++ b/Documentation/thermal/cpu-cooling-api.txt
@@ -38,3 +38,20 @@ the cooling device pointer.
     This interface function unregisters the "thermal-cpufreq-%x" cooling device.
 
     cdev: Cooling device pointer which has to be unregistered.
+
+1.2 cpuhotplug registration api's
+
+1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val)
+
+    This interface function registers the cpuhotplug cooling device with the name
+    "cpu-hotplug-%x". This api can support multiple instance of cpuhotplug cooling
+    devices.
+
+    mask_val: all the allowed cpu's which can be hotplugged out.
+
+1.1.2 void cpuhotplug_cooling_unregister(struct thermal_cooling_device *cdev)
+
+    This interface function unregisters the "thermal-cpuhotplug-%x" cooling device.
+
+    cdev: Cooling device pointer which has to be unregistered.
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 298f550..3d2d87f 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -48,6 +48,21 @@ static struct cpufreq_cooling_device *notify_cpufreq;
 static DEFINE_PER_CPU(unsigned int, max_policy_freq);
 #endif /*CONFIG_CPU_FREQ*/
 
+#ifdef CONFIG_HOTPLUG_CPU
+struct hotplug_cooling_device {
+	int id;
+	struct thermal_cooling_device *cool_dev;
+	unsigned int hotplug_state;
+	const struct cpumask *allowed_cpus;
+	struct list_head node;
+};
+
+static LIST_HEAD(cooling_cpuhotplug_list);
+static DEFINE_MUTEX(cooling_cpuhotplug_lock);
+static DEFINE_IDR(cpuhotplug_idr);
+#endif /*CONFIG_HOTPLUG_CPU*/
+
+
 static int get_idr(struct idr *idr, struct mutex *lock, int *id)
 {
 	int err;
@@ -308,3 +323,162 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
 }
 EXPORT_SYMBOL(cpufreq_cooling_unregister);
 #endif /*CONFIG_CPU_FREQ*/
+
+#ifdef CONFIG_HOTPLUG_CPU
+/*
+ * cpu hotplug cooling device callback functions
+ */
+static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct hotplug_cooling_device *hotplug_dev = NULL;
+
+	mutex_lock(&cooling_cpuhotplug_lock);
+	list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
+		if (hotplug_dev && hotplug_dev->cool_dev == cdev)
+			break;
+
+	mutex_unlock(&cooling_cpuhotplug_lock);
+	if (!hotplug_dev || hotplug_dev->cool_dev != cdev)
+		return -EINVAL;
+
+	*state = 1;
+	return 0;
+}
+
+static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct hotplug_cooling_device *hotplug_dev = NULL;
+
+	mutex_lock(&cooling_cpuhotplug_lock);
+	list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
+		if (hotplug_dev && hotplug_dev->cool_dev == cdev)
+			break;
+
+	mutex_unlock(&cooling_cpuhotplug_lock);
+	if (!hotplug_dev || hotplug_dev->cool_dev != cdev)
+		return -EINVAL;
+
+	/*
+	* This cooling device may be of type ACTIVE, so state field can
+	* be 0 or 1
+	*/
+	*state = hotplug_dev->hotplug_state;
+	return 0;
+}
+
+/*This cooling may be as ACTIVE type*/
+static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long state)
+{
+	int cpuid, this_cpu = smp_processor_id();
+	struct hotplug_cooling_device *hotplug_dev = NULL;
+
+	mutex_lock(&cooling_cpuhotplug_lock);
+	list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
+		if (hotplug_dev && hotplug_dev->cool_dev == cdev)
+			break;
+
+	mutex_unlock(&cooling_cpuhotplug_lock);
+	if (!hotplug_dev || hotplug_dev->cool_dev != cdev)
+		return -EINVAL;
+
+	if (hotplug_dev->hotplug_state == state)
+		return 0;
+
+	/*
+	* This cooling device may be of type ACTIVE, so state field can
+	* be 0 or 1
+	*/
+	if (state == 1) {
+		for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
+			if (cpu_online(cpuid) && (cpuid != this_cpu))
+				cpu_down(cpuid);
+		}
+	} else if (state == 0) {
+		for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
+			if (!cpu_online(cpuid) && (cpuid != this_cpu))
+				cpu_up(cpuid);
+		}
+	} else {
+		return -EINVAL;
+	}
+
+	hotplug_dev->hotplug_state = state;
+
+	return 0;
+}
+/* bind hotplug callbacks to cpu hotplug cooling device */
+static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
+	.get_max_state = cpuhotplug_get_max_state,
+	.get_cur_state = cpuhotplug_get_cur_state,
+	.set_cur_state = cpuhotplug_set_cur_state,
+};
+
+struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val)
+{
+	struct thermal_cooling_device *cool_dev;
+	struct hotplug_cooling_device *hotplug_dev;
+	int ret = 0;
+	char dev_name[THERMAL_NAME_LENGTH];
+
+	hotplug_dev =
+		kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
+
+	if (!hotplug_dev)
+		return ERR_PTR(-ENOMEM);
+
+	ret = get_idr(&cpuhotplug_idr, &cooling_cpuhotplug_lock,
+			&hotplug_dev->id);
+	if (ret) {
+		kfree(hotplug_dev);
+		return ERR_PTR(-EINVAL);
+	}
+
+	sprintf(dev_name, "cpu-hotplug-%u", hotplug_dev->id);
+
+	hotplug_dev->hotplug_state = 0;
+	hotplug_dev->allowed_cpus = mask_val;
+	cool_dev = thermal_cooling_device_register(dev_name, hotplug_dev,
+						&cpuhotplug_cooling_ops);
+	if (!cool_dev) {
+		release_idr(&cpuhotplug_idr, &cooling_cpuhotplug_lock,
+				hotplug_dev->id);
+		kfree(hotplug_dev);
+		return ERR_PTR(-EINVAL);
+	}
+
+	hotplug_dev->cool_dev = cool_dev;
+	mutex_lock(&cooling_cpuhotplug_lock);
+	list_add_tail(&hotplug_dev->node, &cooling_cpuhotplug_list);
+	mutex_unlock(&cooling_cpuhotplug_lock);
+
+	return cool_dev;
+}
+EXPORT_SYMBOL(cpuhotplug_cooling_register);
+
+void cpuhotplug_cooling_unregister(struct thermal_cooling_device *cdev)
+{
+	struct hotplug_cooling_device *hotplug_dev = NULL;
+
+	mutex_lock(&cooling_cpuhotplug_lock);
+	list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
+		if (hotplug_dev && hotplug_dev->cool_dev == cdev)
+			break;
+
+	if (!hotplug_dev || hotplug_dev->cool_dev != cdev) {
+		mutex_unlock(&cooling_cpuhotplug_lock);
+		return;
+	}
+
+	list_del(&hotplug_dev->node);
+	mutex_unlock(&cooling_cpuhotplug_lock);
+	thermal_cooling_device_unregister(hotplug_dev->cool_dev);
+	release_idr(&cpuhotplug_idr, &cooling_cpuhotplug_lock,
+						hotplug_dev->id);
+	kfree(hotplug_dev);
+}
+EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
+#endif /*CONFIG_HOTPLUG_CPU*/
diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
index 5dc5632..8d195d5 100644
--- a/include/linux/cpu_cooling.h
+++ b/include/linux/cpu_cooling.h
@@ -50,5 +50,22 @@ static inline void cpufreq_cooling_unregister(
 	return;
 }
 #endif	/*CONFIG_CPU_FREQ*/
+#ifdef CONFIG_HOTPLUG_CPU
+extern struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val);
+
+extern void cpuhotplug_cooling_unregister(struct thermal_cooling_device *cdev);
+#else /*!CONFIG_HOTPLUG_CPU*/
+static inline struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val)
+{
+	return NULL;
+}
+static inline void cpuhotplug_cooling_unregister(
+				struct thermal_cooling_device *cdev)
+{
+	return;
+}
+#endif /*CONFIG_HOTPLUG_CPU*/
 
 #endif /* __CPU_COOLING_H__ */
-- 
1.7.1

[PATCH 4/4] thermal: Add support to report cooling statistics achieved by cooling devices

[Amit Daniel Kachhap]

Add a debugfs node code to report effective cooling of all cooling devices
attached to each trip points of a thermal zone. The cooling data reported
will be absolute if the higher temperature trip points are arranged first
otherwise the cooling stats is the cumulative effect of the earlier
invoked cooling handlers.

The basic assumption is that cooling devices will bring down the temperature
in a symmetric manner and those statistics can be stored back and used for
further tuning of the system.

e.g.
cat /sys/kernel/debug/thermal/thermal_zone0/trip_0_cooling
6000
Here trip_0 cooling devices produce 6 degree Celsius temperature drop.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
---
 drivers/thermal/thermal_sys.c |  120 +++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h       |   13 +++++
 2 files changed, 133 insertions(+), 0 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index d4c9b20..9784551 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -34,6 +34,7 @@
 #include <linux/reboot.h>
 #include <net/netlink.h>
 #include <net/genetlink.h>
+#include <linux/debugfs.h>
 
 MODULE_AUTHOR("Zhang Rui");
 MODULE_DESCRIPTION("Generic thermal management sysfs support");
@@ -93,6 +94,97 @@ static void release_idr(struct idr *idr, struct mutex *lock, int id)
 		mutex_unlock(lock);
 }
 
+static void update_cooling_stats(struct thermal_zone_device *tz, long cur_temp)
+{
+	int count, max_index, cur_interval, used_data = 0;
+	long trip_temp, max_temp = 0, cool_temp;
+
+	if (cur_temp >= tz->last_temperature)
+		return;
+
+	/* find the trip according to last temperature */
+	for (count = 0; count < tz->trips; count++) {
+		tz->ops->get_trip_temp(tz, count, &trip_temp);
+		if (tz->last_temperature >= trip_temp) {
+			if (max_temp < trip_temp) {
+				max_temp = trip_temp;
+				max_index = count;
+			}
+		}
+	}
+
+	if (!max_temp) {
+		tz->last_trip_level = -1;
+		return;
+	}
+
+	cur_interval = tz->stat[max_index].interval_ptr;
+	cool_temp = tz->last_temperature - cur_temp;
+
+	if (tz->last_trip_level != max_index) {
+		if (++cur_interval == INTERVAL_HISTORY)
+			cur_interval = 0;
+		tz->stat[max_index].cool_temp[cur_interval] = cool_temp;
+		tz->stat[max_index].interval_ptr = cur_interval;
+		tz->last_trip_level = max_index;
+	} else {
+		tz->stat[max_index].cool_temp[cur_interval] += cool_temp;
+	}
+
+	/*Average out the cooling for this trip level*/
+	for (count = 0; count < INTERVAL_HISTORY; count++) {
+		if (tz->stat[max_index].cool_temp[count] > 0) {
+			tz->stat[max_index].avg_cool +=
+					tz->stat[max_index].cool_temp[count];
+			used_data++;
+		}
+	}
+	if (used_data > 1)
+		tz->stat[max_index].avg_cool =
+				tz->stat[max_index].avg_cool / used_data;
+	return;
+}
+
+#if defined(CONFIG_DEBUG_FS)
+/* debugfs support to debug cooling info per trip type */
+static struct dentry *thermal_debugfs_root;
+static int thermal_register_debugfs(struct thermal_zone_device *tz)
+{
+	char name[THERMAL_NAME_LENGTH];
+	int count = 0, err = 0;
+	struct dentry *d;
+
+	if (thermal_debugfs_root == NULL)
+		return -EINVAL;
+
+	sprintf(name, "thermal_zone%d", tz->id);
+	d = debugfs_create_dir(name, thermal_debugfs_root);
+	if (!d)
+		return -ENOMEM;
+	tz->d_entry = d;
+	for (count = 0; count < tz->trips; count++) {
+		sprintf(name, "trip_%d_cooling", count);
+		d = debugfs_create_u32(name, S_IRUGO, tz->d_entry,
+					(u32 *)&tz->stat[count].avg_cool);
+		if (!d) {
+			err = -ENOMEM;
+			goto err_debugfs;
+		}
+	}
+	return 0;
+
+err_debugfs:
+	debugfs_remove_recursive(tz->d_entry);
+	return err;
+}
+
+static void thermal_unregister_debugfs(struct thermal_zone_device *tz)
+{
+	debugfs_remove_recursive(tz->d_entry);
+}
+
+#endif
+
 /* sys I/F for thermal zone */
 
 #define to_thermal_zone(_dev) \
@@ -1051,6 +1143,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 		goto leave;
 	}
 
+	update_cooling_stats(tz, temp);
+
 	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);
@@ -1211,6 +1305,7 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
 	tz->tc2 = tc2;
 	tz->passive_delay = passive_delay;
 	tz->polling_delay = polling_delay;
+	tz->last_trip_level = -1;
 
 	dev_set_name(&tz->device, "thermal_zone%d", tz->id);
 	result = device_register(&tz->device);
@@ -1220,6 +1315,13 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
 		return ERR_PTR(result);
 	}
 
+	/*Allocate variables for cooling stats*/
+	tz->stat  = devm_kzalloc(&tz->device,
+				sizeof(struct thermal_cooling_stats) * trips,
+				GFP_KERNEL);
+	if (!tz->stat)
+		goto unregister;
+
 	/* sys I/F */
 	if (type) {
 		result = device_create_file(&tz->device, &dev_attr_type);
@@ -1257,6 +1359,12 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
 	if (result)
 		goto unregister;
 
+#if defined(CONFIG_DEBUG_FS)
+	result = thermal_register_debugfs(tz);
+	if (result)
+		goto unregister;
+#endif
+
 	mutex_lock(&thermal_list_lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
 	if (ops->bind)
@@ -1321,6 +1429,10 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	for (count = 0; count < tz->trips; count++)
 		TRIP_POINT_ATTR_REMOVE(&tz->device, count);
 
+#if defined(CONFIG_DEBUG_FS)
+	thermal_unregister_debugfs(tz);
+#endif
+
 	thermal_remove_hwmon_sysfs(tz);
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
 	idr_destroy(&tz->idr);
@@ -1440,6 +1552,11 @@ static int __init thermal_init(void)
 		mutex_destroy(&thermal_list_lock);
 	}
 	result = genetlink_init();
+#if defined(CONFIG_DEBUG_FS)
+	thermal_debugfs_root = debugfs_create_dir("thermal", NULL);
+	if (!thermal_debugfs_root)
+		result = -ENOMEM;
+#endif
 	return result;
 }
 
@@ -1451,6 +1568,9 @@ static void __exit thermal_exit(void)
 	mutex_destroy(&thermal_idr_lock);
 	mutex_destroy(&thermal_list_lock);
 	genetlink_exit();
+#if defined(CONFIG_DEBUG_FS)
+	debugfs_remove_recursive(thermal_debugfs_root);
+#endif
 }
 
 fs_initcall(thermal_init);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 8df901f..6f15f85 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -73,6 +73,14 @@ struct thermal_cooling_device_ops {
 #define THERMAL_TRIPS_NONE -1
 #define THERMAL_MAX_TRIPS 12
 #define THERMAL_NAME_LENGTH 20
+#define INTERVAL_HISTORY 12
+
+struct thermal_cooling_stats {
+	int cool_temp[INTERVAL_HISTORY];
+	int interval_ptr;
+	int avg_cool;
+};
+
 struct thermal_cooling_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -103,6 +111,11 @@ struct thermal_zone_device {
 	struct list_head cooling_devices;
 	struct idr idr;
 	struct mutex lock;	/* protect cooling devices list */
+	struct thermal_cooling_stats *stat;
+	int last_trip_level;
+#if defined(CONFIG_DEBUG_FS)
+	struct dentry *d_entry;
+#endif
 	struct list_head node;
 	struct delayed_work poll_queue;
 };
-- 
1.7.1

Re: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Peter Meerwald]

Hi, only textual nitpicking below...

> user and the binding of these cooling devices to the corresponding
> trip points can be easily done as the registration API's return the
> cooling device pointer. The user of these api's are responsible for

API vs. api
use plural s: APIs

> +    This interface function registers the cpufreq cooling device with the name
> +    "thermal-cpufreq-%x". This api can support multiple instance of cpufreq cooling
> +    devices.
instances

> +	.freq_clip_pctg: Percentage of frequency to be clipped for each allowed
> +	 cpus.
cpu

> +    tab_size: the total number of cooling state.
states

p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [linux-pm] [PATCH 0/4] thermal: Adding generic cpu cooling devices

[Eduardo Valentin]

Hello Amit,

Thanks for keeping this up.

On Wed, Feb 22, 2012 at 03:44:06PM +0530, Amit Daniel Kachhap wrote:
> Changes since RFC:
>  *Changed the cpu cooling registration/unregistration API's to instance based
>  *Changed the STATE_ACTIVE trip type to pass correct instance id
>  *Adding support to restore back the policy->max_freq after doing frequency 
>   clipping.

Have checked if this does not overlap with what the user set's as max_freq?
Not sure if we have governors that set max_freq as well. Anyways, looks like
just saving the current max and restoring it may lead to overlaps. I didn't check though.

>  *Moved the trip cooling stats from sysfs node to debugfs node as suggested
>   by Greg KH greg@kroah.com 
>  *Incorporated several review comments from eduardo.valentin@ti.com
> 
> Todo:
>  *Report time spent in each trip point along with the cooling statistics
>  *Add opp library support in cpufreq cooling api's
> 
> Brief Description:
> 
> 1) The generic cooling devices code is placed inside driver/thermal/* as 
> placing inside acpi folder will need un-necessary enabling of acpi code.
> 
> 2) This patchset adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
> cooling device instance number and may be helpful for cpufreq cooling devices
> to take the correct cooling action.

As already said on your previous series, the naming is not good. Problem
is that it may confuse people who have ACPI nomenclature in mind. Active,
on those terms, is related to cooling devices that consumes more power,
like activating fans.

> 
> 3) This patchset adds generic cpu cooling low level implementation through
> frequency clipping and cpu hotplug. In future, other cpu related cooling
> devices may be added here. An ACPI version of this already exists
> (drivers/acpi/processor_thermal.c). But this will be useful for platforms
> like ARM using the generic thermal interface along with the generic cpu
> cooling devices. The cooling device registration API's return cooling device
> pointers which can be easily binded with the thermal zone trip points.

Yeah, but here I have to agree with Matthew Garrett's comment on your previous
version. If it is supposed to be generic, then it has to couple of ACPI as well,
otherwise it is generic for non-ACPI right? :-)

One suggestion is to think on how we could make the passive trip type more generic,
so that it could fit any policy there, either ACPI based or non-ACPI.

Talking about policy, I also think that having one specific policy on a generic
cooling strategy is maybe not a good way to go. The percentages stepping is
one proposal, but some other system may want to put its own way of cooling
using the available opps. That comes again back to the point that we probably
want something more generalized there in the generic code.

For instance, while updating the zone, you could have a generic callback
operation which would dictate the relation between trip with passive cooling state.
Then, ACPI would then define how it wants this relation to be. Then you could propose the
percentage based approach, but other people could also have its own way of correlation
or how the zone has to be updated while doing passive cooling.

Another thing is that you may want to include some examples on your documentation.
Examples of system setup with trips, temps, percentages and behavior.

> 
> 4)This patchset provides a simple way of reporting cooling achieved in a
> trip type. This will help in fine cooling the cooling devices attached.
> 
> Amit Daniel Kachhap (4):
>   thermal: Add a new trip type to use cooling device instance number
>   thermal: Add generic cpufreq cooling implementation
>   thermal: Add generic cpuhotplug cooling implementation
>   thermal: Add support to report cooling statistics achieved by cooling
>     devices
> 
>  Documentation/thermal/cpu-cooling-api.txt |   57 ++++
>  Documentation/thermal/sysfs-api.txt       |    4 +-
>  drivers/thermal/Kconfig                   |   11 +
>  drivers/thermal/Makefile                  |    1 +
>  drivers/thermal/cpu_cooling.c             |  484 +++++++++++++++++++++++++++++
>  drivers/thermal/thermal_sys.c             |  165 ++++++++++-
>  include/linux/cpu_cooling.h               |   71 +++++
>  include/linux/thermal.h                   |   14 +
>  8 files changed, 802 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>  create mode 100644 drivers/thermal/cpu_cooling.c
>  create mode 100644 include/linux/cpu_cooling.h
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

RE: [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

[R, Durgadoss]

Hi Amit,

> -----Original Message-----
> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
> Kachhap
> Sent: Wednesday, February 22, 2012 3:44 PM
> To: linux-pm@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device
> instance number
> 
> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
> device instance number. This helps the cooling device registered as
> different instances to perform appropriate cooling action decision in
> the set_cur_state call back function.
> 
> Also since the trip temperature's are in ascending order so some logic
> is put in place to skip the un-necessary checks.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
>  Documentation/thermal/sysfs-api.txt |    4 +-
>  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--
>  include/linux/thermal.h             |    1 +
>  3 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-
> api.txt
> index 1733ab9..7a0c080 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
> 
>  trip_point_[0-*]_type
>  	Strings which indicate the type of the trip point.
> -	E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
> -	thermal zone.
> +	E.g. it can be one of critical, hot, passive, active[0-1],
> +	state-active[0-*] for ACPI thermal zone.
>  	RO, Optional
> 
>  cdev[0-*]
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 220ce7e..d4c9b20 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct
> device_attribute *attr,
>  		return sprintf(buf, "passive\n");
>  	case THERMAL_TRIP_ACTIVE:
>  		return sprintf(buf, "active\n");
> +	case THERMAL_TRIP_STATE_ACTIVE:
> +		return sprintf(buf, "state-active\n");
>  	default:
>  		return sprintf(buf, "unknown\n");
>  	}
> @@ -1034,10 +1036,10 @@ 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, ret = 0, inst_id;
> +	long temp, trip_temp, max_state, last_trip_change = 0;
>  	enum thermal_trip_type trip_type;
> -	struct thermal_cooling_device_instance *instance;
> +	struct thermal_cooling_device_instance *instance, *state_instance;
>  	struct thermal_cooling_device *cdev;
> 
>  	mutex_lock(&tz->lock);
> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct
> thermal_zone_device *tz)
>  					cdev->ops->set_cur_state(cdev, 0);
>  			}
>  			break;
> +		case THERMAL_TRIP_STATE_ACTIVE:
> +			list_for_each_entry(instance, &tz->cooling_devices,
> +					    node) {
> +				if (instance->trip != count)
> +					continue;
> +
> +				if (temp <= last_trip_change)
> +					continue;
> +
> +				inst_id = 0;
> +				/*
> +				*For this instance how many instance of same
> +				*cooling device occured before
> +				*/
> +
> +				list_for_each_entry(state_instance,
> +						&tz->cooling_devices, node) {
> +					if (instance->cdev ==
> +							state_instance->cdev)
> +						inst_id++;
> +					if (state_instance->trip == count)
> +						break;
> +				}

Can you explain a bit more on this loop and the set_cur_state below ?
Sorry, I don't get the logic behind this..

Thanks,
Durga

> +
> +				cdev = instance->cdev;
> +				cdev->ops->get_max_state(cdev, &max_state);
> +
> +				if ((temp >= trip_temp) &&
> +						(inst_id <= max_state))
> +					cdev->ops->set_cur_state(cdev, inst_id);
> +				else if ((temp < trip_temp) &&
> +						(--inst_id <= max_state))
> +					cdev->ops->set_cur_state(cdev, inst_id);
> +
> +				last_trip_change = trip_temp;
> +			}
> +			break;
>  		case THERMAL_TRIP_PASSIVE:
>  			if (temp >= trip_temp || tz->passive)
>  				thermal_zone_device_passive(tz, temp,
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 796f1ff..8df901f 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -42,6 +42,7 @@ enum thermal_trip_type {
>  	THERMAL_TRIP_PASSIVE,
>  	THERMAL_TRIP_HOT,
>  	THERMAL_TRIP_CRITICAL,
> +	THERMAL_TRIP_STATE_ACTIVE,
>  };
> 
>  struct thermal_zone_device_ops {
> --
> 1.7.1

Re: [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

[Amit Kachhap]

On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote:
> Hi Amit,
>
>> -----Original Message-----
>> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
>> Kachhap
>> Sent: Wednesday, February 22, 2012 3:44 PM
>> To: linux-pm@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
>> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
>> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
>> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device
>> instance number
>>
>> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
>> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
>> device instance number. This helps the cooling device registered as
>> different instances to perform appropriate cooling action decision in
>> the set_cur_state call back function.
>>
>> Also since the trip temperature's are in ascending order so some logic
>> is put in place to skip the un-necessary checks.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>>  Documentation/thermal/sysfs-api.txt |    4 +-
>>  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--
>>  include/linux/thermal.h             |    1 +
>>  3 files changed, 45 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-
>> api.txt
>> index 1733ab9..7a0c080 100644
>> --- a/Documentation/thermal/sysfs-api.txt
>> +++ b/Documentation/thermal/sysfs-api.txt
>> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
>>
>>  trip_point_[0-*]_type
>>       Strings which indicate the type of the trip point.
>> -     E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
>> -     thermal zone.
>> +     E.g. it can be one of critical, hot, passive, active[0-1],
>> +     state-active[0-*] for ACPI thermal zone.
>>       RO, Optional
>>
>>  cdev[0-*]
>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> index 220ce7e..d4c9b20 100644
>> --- a/drivers/thermal/thermal_sys.c
>> +++ b/drivers/thermal/thermal_sys.c
>> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct
>> device_attribute *attr,
>>               return sprintf(buf, "passive\n");
>>       case THERMAL_TRIP_ACTIVE:
>>               return sprintf(buf, "active\n");
>> +     case THERMAL_TRIP_STATE_ACTIVE:
>> +             return sprintf(buf, "state-active\n");
>>       default:
>>               return sprintf(buf, "unknown\n");
>>       }
>> @@ -1034,10 +1036,10 @@ 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, ret = 0, inst_id;
>> +     long temp, trip_temp, max_state, last_trip_change = 0;
>>       enum thermal_trip_type trip_type;
>> -     struct thermal_cooling_device_instance *instance;
>> +     struct thermal_cooling_device_instance *instance, *state_instance;
>>       struct thermal_cooling_device *cdev;
>>
>>       mutex_lock(&tz->lock);
>> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct
>> thermal_zone_device *tz)
>>                                       cdev->ops->set_cur_state(cdev, 0);
>>                       }
>>                       break;
>> +             case THERMAL_TRIP_STATE_ACTIVE:
>> +                     list_for_each_entry(instance, &tz->cooling_devices,
>> +                                         node) {
>> +                             if (instance->trip != count)
>> +                                     continue;
>> +
>> +                             if (temp <= last_trip_change)
>> +                                     continue;
>> +
>> +                             inst_id = 0;
>> +                             /*
>> +                             *For this instance how many instance of same
>> +                             *cooling device occured before
>> +                             */
>> +
>> +                             list_for_each_entry(state_instance,
>> +                                             &tz->cooling_devices, node) {
>> +                                     if (instance->cdev ==
>> +                                                     state_instance->cdev)
>> +                                             inst_id++;
>> +                                     if (state_instance->trip == count)
>> +                                             break;
>> +                             }
>
> Can you explain a bit more on this loop and the set_cur_state below ?
> Sorry, I don't get the logic behind this..

This loop is basically finding the instance id of the same cooling device.
Say we have done like this,
thermal_zone_bind_cooling_device(thermal, 2, cdev);
thermal_zone_bind_cooling_device(thermal, 3, cdev);
thermal_zone_bind_cooling_device(thermal, 4, cdev);

In above same cooling device cdev is binded to trip no 2,3 and 4 with
inst_id generated as 1,2,3 respectively. so set_cur_state for those
trip reached will be called as,
set_cur_state(cdev, 1);
set_cur_state(cdev, 2);
set_cur_state(cdev, 3);

Thanks,
Amit D

>
> Thanks,
> Durga
>
>> +
>> +                             cdev = instance->cdev;
>> +                             cdev->ops->get_max_state(cdev, &max_state);
>> +
>> +                             if ((temp >= trip_temp) &&
>> +                                             (inst_id <= max_state))
>> +                                     cdev->ops->set_cur_state(cdev, inst_id);
>> +                             else if ((temp < trip_temp) &&
>> +                                             (--inst_id <= max_state))
>> +                                     cdev->ops->set_cur_state(cdev, inst_id);
>> +
>> +                             last_trip_change = trip_temp;
>> +                     }
>> +                     break;
>>               case THERMAL_TRIP_PASSIVE:
>>                       if (temp >= trip_temp || tz->passive)
>>                               thermal_zone_device_passive(tz, temp,
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 796f1ff..8df901f 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -42,6 +42,7 @@ enum thermal_trip_type {
>>       THERMAL_TRIP_PASSIVE,
>>       THERMAL_TRIP_HOT,
>>       THERMAL_TRIP_CRITICAL,
>> +     THERMAL_TRIP_STATE_ACTIVE,
>>  };
>>
>>  struct thermal_zone_device_ops {
>> --
>> 1.7.1
>

RE: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[R, Durgadoss]

Hi Amit,

> -----Original Message-----
> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
> Kachhap
> Sent: Wednesday, February 22, 2012 3:44 PM
> To: linux-pm@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
> Subject: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
> 
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling up/down based on the request
> from user. Different cpu related cooling devices can be registered by the

I believe what you mean by 'user' is another Driver using this code.. right ??

> user and the binding of these cooling devices to the corresponding
> trip points can be easily done as the registration API's return the
> cooling device pointer. The user of these api's are responsible for
> passing clipping frequency in percentages.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
>  Documentation/thermal/cpu-cooling-api.txt |   40 ++++
>  drivers/thermal/Kconfig                   |   11 +
>  drivers/thermal/Makefile                  |    1 +
>  drivers/thermal/cpu_cooling.c             |  310 +++++++++++++++++++++++++++++
>  include/linux/cpu_cooling.h               |   54 +++++
>  5 files changed, 416 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>  create mode 100644 drivers/thermal/cpu_cooling.c
>  create mode 100644 include/linux/cpu_cooling.h
> 
> diff --git a/Documentation/thermal/cpu-cooling-api.txt
> b/Documentation/thermal/cpu-cooling-api.txt
> new file mode 100644
> index 0000000..04de67c
> --- /dev/null
> +++ b/Documentation/thermal/cpu-cooling-api.txt
> @@ -0,0 +1,40 @@
> +CPU cooling api's How To
> +===================================
> +
> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
> +
> +Updated: 13 Dec 2011
> +
> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
> +
> +0. Introduction
> +
> +The generic cpu cooling(freq clipping, cpuhotplug) provides
> +registration/unregistration api's to the user. The binding of the cooling
> +devices to the trip point is left for the user. The registration api's returns
> +the cooling device pointer.
> +
> +1. cpufreq cooling api's
> +
> +1.1 cpufreq registration api's
> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val)
> +
> +    This interface function registers the cpufreq cooling device with the name
> +    "thermal-cpufreq-%x". This api can support multiple instance of cpufreq
> cooling
> +    devices.
> +
> +    tab_ptr: The table containing the percentage of frequency to be clipped
> for
> +    each cooling state.
> +	.freq_clip_pctg: Percentage of frequency to be clipped for each allowed
> +	 cpus.
> +	.polling_interval: polling interval for this cooling state.
> +    tab_size: the total number of cooling state.

Although I can understand that the table size is equal to 
the total number of cooling states, it is better to name it 'num_cooling_states'
(or something) that means what it is..

> +    mask_val: all the allowed cpu's where frequency clipping can happen.
> +
> +1.1.2 void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> +
> +    This interface function unregisters the "thermal-cpufreq-%x" cooling
> device.
> +
> +    cdev: Cooling device pointer which has to be unregistered.
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f7f71b2..298c1cd 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -18,3 +18,14 @@ config THERMAL_HWMON
>  	depends on THERMAL
>  	depends on HWMON=y || HWMON=THERMAL
>  	default y
> +
> +config CPU_THERMAL
> +	bool "generic cpu cooling support"
> +	depends on THERMAL
> +	help
> +	  This implements the generic cpu cooling mechanism through frequency
> +	  reduction, cpu hotplug and any other ways of reducing temperature. An
> +	  ACPI version of this already exists(drivers/acpi/processor_thermal.c).
> +	  This will be useful for platforms using the generic thermal interface
> +	  and not the ACPI interface.
> +	  If you want this support, you should say Y or M here.
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 31108a0..655cbc4 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -3,3 +3,4 @@
>  #
> 
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
> +obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> new file mode 100644
> index 0000000..298f550
> --- /dev/null
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -0,0 +1,310 @@
> +/*
> + *  linux/drivers/thermal/cpu_cooling.c
> + *
> + *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  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.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/cpu.h>
> +#include <linux/cpu_cooling.h>
> +
> +#ifdef CONFIG_CPU_FREQ

Except the _idr methods, all the code is inside this #ifdef.
So, I think it is better to add this dependency in Kconfig,
and leave this code clean w/o many #ifdef's.

> +struct cpufreq_cooling_device {
> +	int id;
> +	struct thermal_cooling_device *cool_dev;
> +	struct freq_pctg_table *tab_ptr;
> +	unsigned int tab_size;
> +	unsigned int cpufreq_state;
> +	const struct cpumask *allowed_cpus;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(cooling_cpufreq_list);
> +static DEFINE_MUTEX(cooling_cpufreq_lock);
> +static DEFINE_IDR(cpufreq_idr);
> +static struct cpufreq_cooling_device *notify_cpufreq;

Please move this after the DEFINE_PER_CPU.
Hard to notice here..

> +static DEFINE_PER_CPU(unsigned int, max_policy_freq);
> +#endif /*CONFIG_CPU_FREQ*/
> +
> +static int get_idr(struct idr *idr, struct mutex *lock, int *id)
> +{
> +	int err;
> +again:
> +	if (unlikely(idr_pre_get(idr, GFP_KERNEL) == 0))
> +		return -ENOMEM;
> +
> +	if (lock)
> +		mutex_lock(lock);
> +	err = idr_get_new(idr, NULL, id);
> +	if (lock)
> +		mutex_unlock(lock);
> +	if (unlikely(err == -EAGAIN))
> +		goto again;
> +	else if (unlikely(err))
> +		return err;
> +
> +	*id = *id & MAX_ID_MASK;
> +	return 0;
> +}
> +
> +static void release_idr(struct idr *idr, struct mutex *lock, int id)
> +{
> +	if (lock)
> +		mutex_lock(lock);
> +	idr_remove(idr, id);
> +	if (lock)
> +		mutex_unlock(lock);
> +}
> +
> +#ifdef CONFIG_CPU_FREQ
> +/*Below codes defines functions to be used for cpufreq as cooling device*/
> +static bool is_cpufreq_valid(int cpu)
> +{
> +	struct cpufreq_policy policy;
> +	if (!cpufreq_get_policy(&policy, cpu))
> +		return true;
> +	return false;

Why not just do a return !cpufreq_get_policy(&policy, cpu);

> +}
> +
> +static int cpufreq_apply_cooling(struct cpufreq_cooling_device
> *cpufreq_device,
> +				unsigned long cooling_state)
> +{
> +	int cpuid, this_cpu = smp_processor_id();
> +
> +	if (!is_cpufreq_valid(this_cpu))

You are not using this_cpu anywhere else..so, directly use
Smp_processor_id() here..

> +		return 0;
> +
> +	if (cooling_state > cpufreq_device->tab_size)
> +		return -EINVAL;
> +
> +	/*Check if last cooling level is same as current cooling level*/

Use either 'state' or 'level' in comments as well as the variable name
Makes it easy to read..

> +	if (cpufreq_device->cpufreq_state == cooling_state)
> +		return 0;
> +
> +	cpufreq_device->cpufreq_state = cooling_state;
> +
> +	/*cpufreq thermal notifier uses this cpufreq device pointer*/
> +	notify_cpufreq = cpufreq_device;
> +
> +	for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
> +		if (is_cpufreq_valid(cpuid))
> +			cpufreq_update_policy(cpuid);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct cpufreq_policy *policy = data;
> +	struct freq_pctg_table *th_table;
> +	unsigned long max_freq = 0;
> +	unsigned int th_pctg = 0, level;
> +
> +	if (event != CPUFREQ_ADJUST)
> +		return 0;
> +
> +	if (!notify_cpufreq)
> +		return 0;

Why not combine both if's with an || ?

> +
> +	level = notify_cpufreq->cpufreq_state;

Yes..here it is..please use level/state..

> +
> +	if (level > 0) {
> +		th_table =
> +			&(notify_cpufreq->tab_ptr[level - 1]);
> +		th_pctg = th_table->freq_clip_pctg;
> +		max_freq =
> +		(policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
> +
> +		if (per_cpu(max_policy_freq, policy->cpu) == 0)
> +			per_cpu(max_policy_freq, policy->cpu) = policy->max;
> +	} else {
> +		if (per_cpu(max_policy_freq, policy->cpu) != 0) {
> +			max_freq = per_cpu(max_policy_freq, policy->cpu);
> +			per_cpu(max_policy_freq, policy->cpu) = 0;
> +		} else {
> +			max_freq = policy->max;
> +		}
> +	}
> +
> +	cpufreq_verify_within_limits(policy, 0, max_freq);
> +
> +	return 0;
> +}
> +
> +/*
> + * cpufreq cooling device callback functions
> + */
> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	struct cpufreq_cooling_device *cpufreq_device = NULL;

Why assigning NULL ?

> +
> +	mutex_lock(&cooling_cpufreq_lock);
> +	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
> +		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
> +			break;
> +
> +	mutex_unlock(&cooling_cpufreq_lock);
> +	if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
> +		return -EINVAL;
> +
> +	*state = cpufreq_device->tab_size;
> +	return 0;
> +}

The above can be simplified this way:
	int ret = -EINVAL;
	mutex_lock(&cooling_cpufreq_lock);
	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
		if (cpufreq_device->cool_dev == cdev) {
			*state = cpufreq_device->tab_size;
			ret = 0;
			break;
		}

	mutex_unlock(&cooling_cpufreq_lock);
	return ret;

I think the same can be done for the get_ function below..and similar ones
in the patch 3/4.

> +
> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	struct cpufreq_cooling_device *cpufreq_device = NULL;
> +
> +	mutex_lock(&cooling_cpufreq_lock);
> +	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
> +		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
> +			break;
> +
> +	mutex_unlock(&cooling_cpufreq_lock);
> +	if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
> +		return -EINVAL;
> +	*state = cpufreq_device->cpufreq_state;
> +	return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/ACTIVE type*/
> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long state)
> +{
> +	struct cpufreq_cooling_device *cpufreq_device = NULL;
> +
> +	mutex_lock(&cooling_cpufreq_lock);
> +	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
> +		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
> +			break;
> +
> +	mutex_unlock(&cooling_cpufreq_lock);
> +	if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
> +		return -EINVAL;
> +
> +	cpufreq_apply_cooling(cpufreq_device, state);
> +	return 0;
> +}
> +
> +/* bind cpufreq callbacks to cpufreq cooling device */
> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> +	.get_max_state = cpufreq_get_max_state,
> +	.get_cur_state = cpufreq_get_cur_state,
> +	.set_cur_state = cpufreq_set_cur_state,
> +};
> +
> +static struct notifier_block thermal_cpufreq_notifier_block = {
> +	.notifier_call = thermal_cpufreq_notifier,
> +};
> +
> +struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val)
> +{
> +	struct thermal_cooling_device *cool_dev;
> +	struct cpufreq_cooling_device *cpufreq_dev = NULL;
> +	unsigned int count = 0;
> +	char dev_name[THERMAL_NAME_LENGTH];
> +	int ret = 0, id = 0;
> +
> +	if (tab_ptr == NULL || tab_size == 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node)
> +		count++;
> +
> +	cpufreq_dev =
> +		kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
> +
> +	if (!cpufreq_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cpufreq_dev->tab_ptr = tab_ptr;
> +	cpufreq_dev->tab_size = tab_size;
> +	cpufreq_dev->allowed_cpus = mask_val;
> +
> +	ret = get_idr(&cpufreq_idr, &cooling_cpufreq_lock, &cpufreq_dev->id);
> +	if (ret) {
> +		kfree(cpufreq_dev);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	sprintf(dev_name, "thermal-cpufreq-%d", cpufreq_dev->id);
> +
> +	cool_dev = thermal_cooling_device_register(dev_name, cpufreq_dev,
> +						&cpufreq_cooling_ops);
> +	if (!cool_dev) {
> +		release_idr(&cpufreq_idr, &cooling_cpufreq_lock,
> +						cpufreq_dev->id);
> +		kfree(cpufreq_dev);
> +		return ERR_PTR(-EINVAL);
> +	}
> +	cpufreq_dev->id = id;
> +	cpufreq_dev->cool_dev = cool_dev;
> +	mutex_lock(&cooling_cpufreq_lock);
> +	list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list);
> +	mutex_unlock(&cooling_cpufreq_lock);
> +
> +	if (count == 0)
> +		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> +						CPUFREQ_POLICY_NOTIFIER);

Why do we register only when count is 0 ?
Or should this be 'if (count > 0)' ?

> +	return cool_dev;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +
> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> +{
> +	struct cpufreq_cooling_device *cpufreq_dev = NULL;
> +	unsigned int count = 0;
> +
> +	mutex_lock(&cooling_cpufreq_lock);
> +	list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
> +		if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
> +			break;
> +		count++;
> +	}
> +
> +	if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
> +		mutex_unlock(&cooling_cpufreq_lock);
> +		return;
> +	}
> +
> +	list_del(&cpufreq_dev->node);
> +	mutex_unlock(&cooling_cpufreq_lock);
> +
> +	if (count == 1)

Same here..I do not get the idea behind this..
Shouldn't this be 'if (count > 0)' ?

In general,
I would like to see a real driver using these API's. This will help
everybody to understand the working of these API's much better.

Thanks,
Durga

> +		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> +					CPUFREQ_POLICY_NOTIFIER);
> +
> +	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
> +	release_idr(&cpufreq_idr, &cooling_cpufreq_lock, cpufreq_dev->id);
> +	kfree(cpufreq_dev);
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#endif /*CONFIG_CPU_FREQ*/
> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> new file mode 100644
> index 0000000..5dc5632
> --- /dev/null
> +++ b/include/linux/cpu_cooling.h
> @@ -0,0 +1,54 @@
> +/*
> + *  linux/include/linux/cpu_cooling.h
> + *
> + *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  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.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef __CPU_COOLING_H__
> +#define __CPU_COOLING_H__
> +
> +#include <linux/thermal.h>
> +
> +struct freq_pctg_table {
> +	unsigned int freq_clip_pctg;
> +	unsigned int polling_interval;
> +};
> +
> +#ifdef CONFIG_CPU_FREQ
> +struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val);
> +
> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
> +#else /*!CONFIG_CPU_FREQ*/
> +static inline struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val)
> +{
> +	return NULL;
> +}
> +static inline void cpufreq_cooling_unregister(
> +				struct thermal_cooling_device *cdev)
> +{
> +	return;
> +}
> +#endif	/*CONFIG_CPU_FREQ*/
> +
> +#endif /* __CPU_COOLING_H__ */
> --
> 1.7.1

Re: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Amit Kachhap]

Hi Durgadoss,

Thanks for the detailed review comments.

On 24 February 2012 16:34, R, Durgadoss <durgadoss.r@intel.com> wrote:
> Hi Amit,
>
>> -----Original Message-----
>> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
>> Kachhap
>> Sent: Wednesday, February 22, 2012 3:44 PM
>> To: linux-pm@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
>> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
>> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
>> Subject: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
>>
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling up/down based on the request
>> from user. Different cpu related cooling devices can be registered by the
>
> I believe what you mean by 'user' is another Driver using this code.. right ??
Yes.
>
>> user and the binding of these cooling devices to the corresponding
>> trip points can be easily done as the registration API's return the
>> cooling device pointer. The user of these api's are responsible for
>> passing clipping frequency in percentages.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>>  Documentation/thermal/cpu-cooling-api.txt |   40 ++++
>>  drivers/thermal/Kconfig                   |   11 +
>>  drivers/thermal/Makefile                  |    1 +
>>  drivers/thermal/cpu_cooling.c             |  310 +++++++++++++++++++++++++++++
>>  include/linux/cpu_cooling.h               |   54 +++++
>>  5 files changed, 416 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>>  create mode 100644 drivers/thermal/cpu_cooling.c
>>  create mode 100644 include/linux/cpu_cooling.h
>>
>> diff --git a/Documentation/thermal/cpu-cooling-api.txt
>> b/Documentation/thermal/cpu-cooling-api.txt
>> new file mode 100644
>> index 0000000..04de67c
>> --- /dev/null
>> +++ b/Documentation/thermal/cpu-cooling-api.txt
>> @@ -0,0 +1,40 @@
>> +CPU cooling api's How To
>> +===================================
>> +
>> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> +
>> +Updated: 13 Dec 2011
>> +
>> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
>> +
>> +0. Introduction
>> +
>> +The generic cpu cooling(freq clipping, cpuhotplug) provides
>> +registration/unregistration api's to the user. The binding of the cooling
>> +devices to the trip point is left for the user. The registration api's returns
>> +the cooling device pointer.
>> +
>> +1. cpufreq cooling api's
>> +
>> +1.1 cpufreq registration api's
>> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val)
>> +
>> +    This interface function registers the cpufreq cooling device with the name
>> +    "thermal-cpufreq-%x". This api can support multiple instance of cpufreq
>> cooling
>> +    devices.
>> +
>> +    tab_ptr: The table containing the percentage of frequency to be clipped
>> for
>> +    each cooling state.
>> +     .freq_clip_pctg: Percentage of frequency to be clipped for each allowed
>> +      cpus.
>> +     .polling_interval: polling interval for this cooling state.
>> +    tab_size: the total number of cooling state.
>
> Although I can understand that the table size is equal to
> the total number of cooling states, it is better to name it 'num_cooling_states'
> (or something) that means what it is..
Yes your idea makes more sense. Will accept it in the next version.
>
>> +    mask_val: all the allowed cpu's where frequency clipping can happen.
>> +
>> +1.1.2 void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>> +
>> +    This interface function unregisters the "thermal-cpufreq-%x" cooling
>> device.
>> +
>> +    cdev: Cooling device pointer which has to be unregistered.
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index f7f71b2..298c1cd 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -18,3 +18,14 @@ config THERMAL_HWMON
>>       depends on THERMAL
>>       depends on HWMON=y || HWMON=THERMAL
>>       default y
>> +
>> +config CPU_THERMAL
>> +     bool "generic cpu cooling support"
>> +     depends on THERMAL
>> +     help
>> +       This implements the generic cpu cooling mechanism through frequency
>> +       reduction, cpu hotplug and any other ways of reducing temperature. An
>> +       ACPI version of this already exists(drivers/acpi/processor_thermal.c).
>> +       This will be useful for platforms using the generic thermal interface
>> +       and not the ACPI interface.
>> +       If you want this support, you should say Y or M here.
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 31108a0..655cbc4 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -3,3 +3,4 @@
>>  #
>>
>>  obj-$(CONFIG_THERMAL)                += thermal_sys.o
>> +obj-$(CONFIG_CPU_THERMAL)    += cpu_cooling.o
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> new file mode 100644
>> index 0000000..298f550
>> --- /dev/null
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -0,0 +1,310 @@
>> +/*
>> + *  linux/drivers/thermal/cpu_cooling.c
>> + *
>> + *  Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
>> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  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.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpu_cooling.h>
>> +
>> +#ifdef CONFIG_CPU_FREQ
>
> Except the _idr methods, all the code is inside this #ifdef.
> So, I think it is better to add this dependency in Kconfig,
> and leave this code clean w/o many #ifdef's.
ok
>
>> +struct cpufreq_cooling_device {
>> +     int id;
>> +     struct thermal_cooling_device *cool_dev;
>> +     struct freq_pctg_table *tab_ptr;
>> +     unsigned int tab_size;
>> +     unsigned int cpufreq_state;
>> +     const struct cpumask *allowed_cpus;
>> +     struct list_head node;
>> +};
>> +
>> +static LIST_HEAD(cooling_cpufreq_list);
>> +static DEFINE_MUTEX(cooling_cpufreq_lock);
>> +static DEFINE_IDR(cpufreq_idr);
>> +static struct cpufreq_cooling_device *notify_cpufreq;
>
> Please move this after the DEFINE_PER_CPU.
> Hard to notice here..
ok
>
>> +static DEFINE_PER_CPU(unsigned int, max_policy_freq);
>> +#endif /*CONFIG_CPU_FREQ*/
>> +
>> +static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>> +{
>> +     int err;
>> +again:
>> +     if (unlikely(idr_pre_get(idr, GFP_KERNEL) == 0))
>> +             return -ENOMEM;
>> +
>> +     if (lock)
>> +             mutex_lock(lock);
>> +     err = idr_get_new(idr, NULL, id);
>> +     if (lock)
>> +             mutex_unlock(lock);
>> +     if (unlikely(err == -EAGAIN))
>> +             goto again;
>> +     else if (unlikely(err))
>> +             return err;
>> +
>> +     *id = *id & MAX_ID_MASK;
>> +     return 0;
>> +}
>> +
>> +static void release_idr(struct idr *idr, struct mutex *lock, int id)
>> +{
>> +     if (lock)
>> +             mutex_lock(lock);
>> +     idr_remove(idr, id);
>> +     if (lock)
>> +             mutex_unlock(lock);
>> +}
>> +
>> +#ifdef CONFIG_CPU_FREQ
>> +/*Below codes defines functions to be used for cpufreq as cooling device*/
>> +static bool is_cpufreq_valid(int cpu)
>> +{
>> +     struct cpufreq_policy policy;
>> +     if (!cpufreq_get_policy(&policy, cpu))
>> +             return true;
>> +     return false;
>
> Why not just do a return !cpufreq_get_policy(&policy, cpu);
ok
>
>> +}
>> +
>> +static int cpufreq_apply_cooling(struct cpufreq_cooling_device
>> *cpufreq_device,
>> +                             unsigned long cooling_state)
>> +{
>> +     int cpuid, this_cpu = smp_processor_id();
>> +
>> +     if (!is_cpufreq_valid(this_cpu))
>
> You are not using this_cpu anywhere else..so, directly use
> Smp_processor_id() here..
>
Ok agreed.
>> +             return 0;
>> +
>> +     if (cooling_state > cpufreq_device->tab_size)
>> +             return -EINVAL;
>> +
>> +     /*Check if last cooling level is same as current cooling level*/
>
> Use either 'state' or 'level' in comments as well as the variable name
> Makes it easy to read..

Ok, will use state.
>
>> +     if (cpufreq_device->cpufreq_state == cooling_state)
>> +             return 0;
>> +
>> +     cpufreq_device->cpufreq_state = cooling_state;
>> +
>> +     /*cpufreq thermal notifier uses this cpufreq device pointer*/
>> +     notify_cpufreq = cpufreq_device;
>> +
>> +     for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
>> +             if (is_cpufreq_valid(cpuid))
>> +                     cpufreq_update_policy(cpuid);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
>> +                                     unsigned long event, void *data)
>> +{
>> +     struct cpufreq_policy *policy = data;
>> +     struct freq_pctg_table *th_table;
>> +     unsigned long max_freq = 0;
>> +     unsigned int th_pctg = 0, level;
>> +
>> +     if (event != CPUFREQ_ADJUST)
>> +             return 0;
>> +
>> +     if (!notify_cpufreq)
>> +             return 0;
>
> Why not combine both if's with an || ?
Ok
>
>> +
>> +     level = notify_cpufreq->cpufreq_state;
>
> Yes..here it is..please use level/state..
Ok, will use  state
>
>> +
>> +     if (level > 0) {
>> +             th_table =
>> +                     &(notify_cpufreq->tab_ptr[level - 1]);
>> +             th_pctg = th_table->freq_clip_pctg;
>> +             max_freq =
>> +             (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
>> +
>> +             if (per_cpu(max_policy_freq, policy->cpu) == 0)
>> +                     per_cpu(max_policy_freq, policy->cpu) = policy->max;
>> +     } else {
>> +             if (per_cpu(max_policy_freq, policy->cpu) != 0) {
>> +                     max_freq = per_cpu(max_policy_freq, policy->cpu);
>> +                     per_cpu(max_policy_freq, policy->cpu) = 0;
>> +             } else {
>> +                     max_freq = policy->max;
>> +             }
>> +     }
>> +
>> +     cpufreq_verify_within_limits(policy, 0, max_freq);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * cpufreq cooling device callback functions
>> + */
>> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long *state)
>> +{
>> +     struct cpufreq_cooling_device *cpufreq_device = NULL;
>
> Why assigning NULL ?
yes it is not needed.
>
>> +
>> +     mutex_lock(&cooling_cpufreq_lock);
>> +     list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
>> +             if (cpufreq_device && cpufreq_device->cool_dev == cdev)
>> +                     break;
>> +
>> +     mutex_unlock(&cooling_cpufreq_lock);
>> +     if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
>> +             return -EINVAL;
>> +
>> +     *state = cpufreq_device->tab_size;
>> +     return 0;
>> +}
>
> The above can be simplified this way:
>        int ret = -EINVAL;
>        mutex_lock(&cooling_cpufreq_lock);
>        list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
>                if (cpufreq_device->cool_dev == cdev) {
>                        *state = cpufreq_device->tab_size;
>                        ret = 0;
>                        break;
>                }
>
>        mutex_unlock(&cooling_cpufreq_lock);
>        return ret;
>
> I think the same can be done for the get_ function below..and similar ones
> in the patch 3/4.

Ok accepted.
>
>> +
>> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long *state)
>> +{
>> +     struct cpufreq_cooling_device *cpufreq_device = NULL;
>> +
>> +     mutex_lock(&cooling_cpufreq_lock);
>> +     list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
>> +             if (cpufreq_device && cpufreq_device->cool_dev == cdev)
>> +                     break;
>> +
>> +     mutex_unlock(&cooling_cpufreq_lock);
>> +     if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
>> +             return -EINVAL;
>> +     *state = cpufreq_device->cpufreq_state;
>> +     return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/ACTIVE type*/
>> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long state)
>> +{
>> +     struct cpufreq_cooling_device *cpufreq_device = NULL;
>> +
>> +     mutex_lock(&cooling_cpufreq_lock);
>> +     list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node)
>> +             if (cpufreq_device && cpufreq_device->cool_dev == cdev)
>> +                     break;
>> +
>> +     mutex_unlock(&cooling_cpufreq_lock);
>> +     if (!cpufreq_device || cpufreq_device->cool_dev != cdev)
>> +             return -EINVAL;
>> +
>> +     cpufreq_apply_cooling(cpufreq_device, state);
>> +     return 0;
>> +}
>> +
>> +/* bind cpufreq callbacks to cpufreq cooling device */
>> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
>> +     .get_max_state = cpufreq_get_max_state,
>> +     .get_cur_state = cpufreq_get_cur_state,
>> +     .set_cur_state = cpufreq_set_cur_state,
>> +};
>> +
>> +static struct notifier_block thermal_cpufreq_notifier_block = {
>> +     .notifier_call = thermal_cpufreq_notifier,
>> +};
>> +
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val)
>> +{
>> +     struct thermal_cooling_device *cool_dev;
>> +     struct cpufreq_cooling_device *cpufreq_dev = NULL;
>> +     unsigned int count = 0;
>> +     char dev_name[THERMAL_NAME_LENGTH];
>> +     int ret = 0, id = 0;
>> +
>> +     if (tab_ptr == NULL || tab_size == 0)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node)
>> +             count++;
>> +
>> +     cpufreq_dev =
>> +             kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
>> +
>> +     if (!cpufreq_dev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     cpufreq_dev->tab_ptr = tab_ptr;
>> +     cpufreq_dev->tab_size = tab_size;
>> +     cpufreq_dev->allowed_cpus = mask_val;
>> +
>> +     ret = get_idr(&cpufreq_idr, &cooling_cpufreq_lock, &cpufreq_dev->id);
>> +     if (ret) {
>> +             kfree(cpufreq_dev);
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     sprintf(dev_name, "thermal-cpufreq-%d", cpufreq_dev->id);
>> +
>> +     cool_dev = thermal_cooling_device_register(dev_name, cpufreq_dev,
>> +                                             &cpufreq_cooling_ops);
>> +     if (!cool_dev) {
>> +             release_idr(&cpufreq_idr, &cooling_cpufreq_lock,
>> +                                             cpufreq_dev->id);
>> +             kfree(cpufreq_dev);
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +     cpufreq_dev->id = id;
>> +     cpufreq_dev->cool_dev = cool_dev;
>> +     mutex_lock(&cooling_cpufreq_lock);
>> +     list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list);
>> +     mutex_unlock(&cooling_cpufreq_lock);
>> +
>> +     if (count == 0)
>> +             cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
>> +                                             CPUFREQ_POLICY_NOTIFIER);
>
> Why do we register only when count is 0 ?
> Or should this be 'if (count > 0)' ?
Count represents the number of cpufreq type cooling devices. So for
the first call of this API ,  cpufreq_register_notifier is called.
Other call to cpufreq_cooling_register will use the same notifier. May
be using bool flag instead of count variable is better.
>
>> +     return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +
>> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>> +{
>> +     struct cpufreq_cooling_device *cpufreq_dev = NULL;
>> +     unsigned int count = 0;
>> +
>> +     mutex_lock(&cooling_cpufreq_lock);
>> +     list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
>> +             if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
>> +                     break;
>> +             count++;
>> +     }
>> +
>> +     if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
>> +             mutex_unlock(&cooling_cpufreq_lock);
>> +             return;
>> +     }
>> +
>> +     list_del(&cpufreq_dev->node);
>> +     mutex_unlock(&cooling_cpufreq_lock);
>> +
>> +     if (count == 1)
>
> Same here..I do not get the idea behind this..
> Shouldn't this be 'if (count > 0)' ?
Same as above.
>
> In general,
> I would like to see a real driver using these API's. This will help
> everybody to understand the working of these API's much better.
Actually RFC version of the driver is already posted which uses these
api's. (https://lkml.org/lkml/2011/12/21/169). I forgot to add this
link in this patchset.
I will post the new version of the driver shortly.
>
> Thanks,
> Durga
>
>> +             cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
>> +                                     CPUFREQ_POLICY_NOTIFIER);
>> +
>> +     thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
>> +     release_idr(&cpufreq_idr, &cooling_cpufreq_lock, cpufreq_dev->id);
>> +     kfree(cpufreq_dev);
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#endif /*CONFIG_CPU_FREQ*/
>> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
>> new file mode 100644
>> index 0000000..5dc5632
>> --- /dev/null
>> +++ b/include/linux/cpu_cooling.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + *  linux/include/linux/cpu_cooling.h
>> + *
>> + *  Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
>> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  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.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +
>> +#ifndef __CPU_COOLING_H__
>> +#define __CPU_COOLING_H__
>> +
>> +#include <linux/thermal.h>
>> +
>> +struct freq_pctg_table {
>> +     unsigned int freq_clip_pctg;
>> +     unsigned int polling_interval;
>> +};
>> +
>> +#ifdef CONFIG_CPU_FREQ
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val);
>> +
>> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
>> +#else /*!CONFIG_CPU_FREQ*/
>> +static inline struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val)
>> +{
>> +     return NULL;
>> +}
>> +static inline void cpufreq_cooling_unregister(
>> +                             struct thermal_cooling_device *cdev)
>> +{
>> +     return;
>> +}
>> +#endif       /*CONFIG_CPU_FREQ*/
>> +
>> +#endif /* __CPU_COOLING_H__ */
>> --
>> 1.7.1
>

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Sundar]

Hi Amit,

I am new here; so please bear with my questions/doubts :)

On Wed, Feb 22, 2012 at 3:44 PM, Amit Daniel Kachhap
<amit.kachhap@linaro.org> wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling up/down based on the request
> from user. Different cpu related cooling devices can be registered by the
> user and the binding of these cooling devices to the corresponding

"Different cpu related cooling devices": Do you mean cooling devices
for different CPUs (num_cpus) or are you referring to different
customers aka consumer drivers who could use this framework and impose
the cooling.

> trip points can be easily done as the registration API's return the
> cooling device pointer. The user of these api's are responsible for
> passing clipping frequency in percentages.

Why do you want to pass the clipping frequency in percentages? Wouldnt
it be simpler for any platform sensor driver to just pass the
frequency it wants to throttle the CPU?

> +
> +    This interface function registers the cpufreq cooling device with the name
> +    "thermal-cpufreq-%x". This api can support multiple instance of cpufreq cooling
> +    devices.

When you refer to cooling devices, is it the number of instances
per-CPU that is supported? Sorry I am unable to follow.

> +       .polling_interval: polling interval for this cooling state.
> +    tab_size: the total number of cooling state.

By cooling_state, I assume you are referring to the thermal state for
the platform? or only for the CPU?

> +    mask_val: all the allowed cpu's where frequency clipping can happen.

Why should this be a new variable? The policy->affected_cpus should be
the same as this IMO?

> +       help
> +         This implements the generic cpu cooling mechanism through frequency
> +         reduction, cpu hotplug and any other ways of reducing temperature. An

Apart from reducing the CPU frequency, (probably) CPU hotplug, what
other means of reducing CPU temperature? Or are you referring to any
platform temperature controls?

It isnt very clear from the documentation (at least to me) if this is
only for CPU cooling or generic platform cooling.

Cheers!
-- 
---------
The views expressed in this email are personal and do not necessarily
echo my employers'.

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Amit Kachhap]

On 11 March 2012 09:41, Sundar <sunder.svit@gmail.com> wrote:
> Hi Amit,
>
> I am new here; so please bear with my questions/doubts :)
>
> On Wed, Feb 22, 2012 at 3:44 PM, Amit Daniel Kachhap
> <amit.kachhap@linaro.org> wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling up/down based on the request
>> from user. Different cpu related cooling devices can be registered by the
>> user and the binding of these cooling devices to the corresponding
>
> "Different cpu related cooling devices": Do you mean cooling devices
> for different CPUs (num_cpus) or are you referring to different
> customers aka consumer drivers who could use this framework and impose
> the cooling.
Correct but the consumer driver only has to use the other
thermal-sys.c functions. Only register cpu cooling devices is
implemented in this work.
>
>> trip points can be easily done as the registration API's return the
>> cooling device pointer. The user of these api's are responsible for
>> passing clipping frequency in percentages.
>
> Why do you want to pass the clipping frequency in percentages? Wouldnt
> it be simpler for any platform sensor driver to just pass the
> frequency it wants to throttle the CPU?
Yes i also agree.
>
>> +
>> +    This interface function registers the cpufreq cooling device with the name
>> +    "thermal-cpufreq-%x". This api can support multiple instance of cpufreq cooling
>> +    devices.
>
> When you refer to cooling devices, is it the number of instances
> per-CPU that is supported? Sorry I am unable to follow.
CPU cooling apis can be used as below
1)per cpus if different frequency clipping is needed. so multiple
instance of this api can be called.
2)for all the cpus as provided with mask when same frequency clipping
is needed. In this case single instance is fine.
>
>> +       .polling_interval: polling interval for this cooling state.
>> +    tab_size: the total number of cooling state.
>
> By cooling_state, I assume you are referring to the thermal state for
> the platform? or only for the CPU?
Yes thermal state of the platform.
>
>> +    mask_val: all the allowed cpu's where frequency clipping can happen.
>
> Why should this be a new variable? The policy->affected_cpus should be
> the same as this IMO?
yes this should be same. I will check this.
>
>> +       help
>> +         This implements the generic cpu cooling mechanism through frequency
>> +         reduction, cpu hotplug and any other ways of reducing temperature. An
>
> Apart from reducing the CPU frequency, (probably) CPU hotplug, what
> other means of reducing CPU temperature? Or are you referring to any
> platform temperature controls?
No only CPU related. Other control ways I thought was cpuidle with low
power states and cpu_power factor modifications used in the schedular.
>
> It isnt very clear from the documentation (at least to me) if this is
> only for CPU cooling or generic platform cooling.
>
> Cheers!
> --
> ---------
> The views expressed in this email are personal and do not necessarily
> echo my employers'.

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Sundar]

Hi Amit,

Thanks for the replies. One more query

On Tue, Mar 13, 2012 at 10:37 AM, Amit Kachhap <amit.kachhap@linaro.org> wrote:

>> "Different cpu related cooling devices": Do you mean cooling devices
>> for different CPUs (num_cpus) or are you referring to different
>> customers aka consumer drivers who could use this framework and impose
>> the cooling.
> Correct but the consumer driver only has to use the other
> thermal-sys.c functions. Only register cpu cooling devices is
> implemented in this work.

Am i right in noting that this framework then doesnt handle other
devices for cooling like Graphics, LCD etc?

Cheers!
-- 
---------
The views expressed in this email are personal and do not necessarily
echo my employers.

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Amit Kucheria]

Sundar,

On Tue, Mar 13, 2012 at 11:45 AM, Sundar <sunder.svit@gmail.com> wrote:
> Hi Amit,
>
> Thanks for the replies. One more query
>
> On Tue, Mar 13, 2012 at 10:37 AM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>
>>> "Different cpu related cooling devices": Do you mean cooling devices
>>> for different CPUs (num_cpus) or are you referring to different
>>> customers aka consumer drivers who could use this framework and impose
>>> the cooling.
>> Correct but the consumer driver only has to use the other
>> thermal-sys.c functions. Only register cpu cooling devices is
>> implemented in this work.
>
> Am i right in noting that this framework then doesnt handle other
> devices for cooling like Graphics, LCD etc?

At the moment it doesn't. But there was some discussion around
creating something that will work with devfreq. This would allow
peripheral drivers to be plugged in as well. Amit is investigating
that at present.

/Amit

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Sundar]

On Tue, Mar 13, 2012 at 3:30 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> Sundar,

Hi Amit,

> At the moment it doesn't. But there was some discussion around
> creating something that will work with devfreq. This would allow
> peripheral drivers to be plugged in as well. Amit is investigating
> that at present.

What if we work towards a generic constraint framework which models
thermals as a performance constraint.

Drivers can register to this constraint; platform code can then decide
to issue restrictions either to the CPU or other power-hungry
peripherals based on the platform conditions.

That also allows to model CPU frequency as a generic constraint but
via an actual consumer, say the thermal driver.

Cheers!

-- 
---------
The views expressed in this email are personal and do not necessarily
echo my employers.

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Amit Kachhap]

On 13 March 2012 15:44, Sundar <sunder.svit@gmail.com> wrote:
> On Tue, Mar 13, 2012 at 3:30 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>> Sundar,
>
> Hi Amit,
>
>> At the moment it doesn't. But there was some discussion around
>> creating something that will work with devfreq. This would allow
>> peripheral drivers to be plugged in as well. Amit is investigating
>> that at present.
>
> What if we work towards a generic constraint framework which models
> thermals as a performance constraint.
>
> Drivers can register to this constraint; platform code can then decide
> to issue restrictions either to the CPU or other power-hungry
> peripherals based on the platform conditions.
>
> That also allows to model CPU frequency as a generic constraint but
> via an actual consumer, say the thermal driver.

Yes that should be helpful. Even the things your are suggesting are
somewhat same with some patches submitted which sets cpufreq min/max
constraint.

>
> Cheers!
>
> --
> ---------
> The views expressed in this email are personal and do not necessarily
> echo my employers.

Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation

[Sundar]

On Tue, Mar 13, 2012 at 4:22 PM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
> Yes that should be helpful. Even the things your are suggesting are
> somewhat same with some patches submitted which sets cpufreq min/max
> constraint.

Hmm..let me see how soon can I code up a rough implementation!

Cheers!

-- 
---------
The views expressed in this email are personal and do not necessarily
echo my employers.

Re: [linux-pm] [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

[Eduardo Valentin]

Hello,

On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote:
> On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote:
> > Hi Amit,
> >
> >> -----Original Message-----
> >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
> >> Kachhap
> >> Sent: Wednesday, February 22, 2012 3:44 PM
> >> To: linux-pm@lists.linux-foundation.org
> >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
> >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
> >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
> >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device
> >> instance number
> >>
> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
> >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
> >> device instance number. This helps the cooling device registered as
> >> different instances to perform appropriate cooling action decision in
> >> the set_cur_state call back function.
> >>
> >> Also since the trip temperature's are in ascending order so some logic
> >> is put in place to skip the un-necessary checks.
> >>
> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >> ---
> >>  Documentation/thermal/sysfs-api.txt |    4 +-
> >>  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--
> >>  include/linux/thermal.h             |    1 +
> >>  3 files changed, 45 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-
> >> api.txt
> >> index 1733ab9..7a0c080 100644
> >> --- a/Documentation/thermal/sysfs-api.txt
> >> +++ b/Documentation/thermal/sysfs-api.txt
> >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
> >>
> >>  trip_point_[0-*]_type
> >>       Strings which indicate the type of the trip point.
> >> -     E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
> >> -     thermal zone.
> >> +     E.g. it can be one of critical, hot, passive, active[0-1],
> >> +     state-active[0-*] for ACPI thermal zone.
> >>       RO, Optional
> >>
> >>  cdev[0-*]
> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> >> index 220ce7e..d4c9b20 100644
> >> --- a/drivers/thermal/thermal_sys.c
> >> +++ b/drivers/thermal/thermal_sys.c
> >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct
> >> device_attribute *attr,
> >>               return sprintf(buf, "passive\n");
> >>       case THERMAL_TRIP_ACTIVE:
> >>               return sprintf(buf, "active\n");
> >> +     case THERMAL_TRIP_STATE_ACTIVE:
> >> +             return sprintf(buf, "state-active\n");
> >>       default:
> >>               return sprintf(buf, "unknown\n");
> >>       }
> >> @@ -1034,10 +1036,10 @@ 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, ret = 0, inst_id;
> >> +     long temp, trip_temp, max_state, last_trip_change = 0;
> >>       enum thermal_trip_type trip_type;
> >> -     struct thermal_cooling_device_instance *instance;
> >> +     struct thermal_cooling_device_instance *instance, *state_instance;
> >>       struct thermal_cooling_device *cdev;
> >>
> >>       mutex_lock(&tz->lock);
> >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct
> >> thermal_zone_device *tz)
> >>                                       cdev->ops->set_cur_state(cdev, 0);
> >>                       }
> >>                       break;
> >> +             case THERMAL_TRIP_STATE_ACTIVE:
> >> +                     list_for_each_entry(instance, &tz->cooling_devices,
> >> +                                         node) {
> >> +                             if (instance->trip != count)
> >> +                                     continue;
> >> +
> >> +                             if (temp <= last_trip_change)
> >> +                                     continue;
> >> +
> >> +                             inst_id = 0;
> >> +                             /*
> >> +                             *For this instance how many instance of same
> >> +                             *cooling device occured before
> >> +                             */
> >> +
> >> +                             list_for_each_entry(state_instance,
> >> +                                             &tz->cooling_devices, node) {
> >> +                                     if (instance->cdev ==
> >> +                                                     state_instance->cdev)
> >> +                                             inst_id++;
> >> +                                     if (state_instance->trip == count)
> >> +                                             break;
> >> +                             }
> >
> > Can you explain a bit more on this loop and the set_cur_state below ?
> > Sorry, I don't get the logic behind this..
> 
> This loop is basically finding the instance id of the same cooling device.
> Say we have done like this,
> thermal_zone_bind_cooling_device(thermal, 2, cdev);
> thermal_zone_bind_cooling_device(thermal, 3, cdev);
> thermal_zone_bind_cooling_device(thermal, 4, cdev);
> 
> In above same cooling device cdev is binded to trip no 2,3 and 4 with
> inst_id generated as 1,2,3 respectively. so set_cur_state for those
> trip reached will be called as,
> set_cur_state(cdev, 1);
> set_cur_state(cdev, 2);
> set_cur_state(cdev, 3);

In this case, why a simple state = get_cur_state() followed by a
set_cur_state(++state) / set_cur_state(--state) is not enough?

Another thing is if we want to do jumps in the sequence?

 set_cur_state(cdev, 1);
 set_cur_state(cdev, 3);
 set_cur_state(cdev, 6);

But for that we need a table mapping, trip vs. state.


What do you think?

> 
> Thanks,
> Amit D
> 
> >
> > Thanks,
> > Durga
> >
> >> +
> >> +                             cdev = instance->cdev;
> >> +                             cdev->ops->get_max_state(cdev, &max_state);
> >> +
> >> +                             if ((temp >= trip_temp) &&
> >> +                                             (inst_id <= max_state))
> >> +                                     cdev->ops->set_cur_state(cdev, inst_id);
> >> +                             else if ((temp < trip_temp) &&
> >> +                                             (--inst_id <= max_state))
> >> +                                     cdev->ops->set_cur_state(cdev, inst_id);
> >> +
> >> +                             last_trip_change = trip_temp;
> >> +                     }
> >> +                     break;
> >>               case THERMAL_TRIP_PASSIVE:
> >>                       if (temp >= trip_temp || tz->passive)
> >>                               thermal_zone_device_passive(tz, temp,
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index 796f1ff..8df901f 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -42,6 +42,7 @@ enum thermal_trip_type {
> >>       THERMAL_TRIP_PASSIVE,
> >>       THERMAL_TRIP_HOT,
> >>       THERMAL_TRIP_CRITICAL,
> >> +     THERMAL_TRIP_STATE_ACTIVE,
> >>  };
> >>
> >>  struct thermal_zone_device_ops {
> >> --
> >> 1.7.1
> >
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [linux-pm] [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

[Amit Kachhap]

Hi Eduardo,

On 3 April 2012 19:45, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> Hello,
>
> On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote:
>> On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote:
>> > Hi Amit,
>> >
>> >> -----Original Message-----
>> >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
>> >> Kachhap
>> >> Sent: Wednesday, February 22, 2012 3:44 PM
>> >> To: linux-pm@lists.linux-foundation.org
>> >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
>> >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
>> >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
>> >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device
>> >> instance number
>> >>
>> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
>> >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
>> >> device instance number. This helps the cooling device registered as
>> >> different instances to perform appropriate cooling action decision in
>> >> the set_cur_state call back function.
>> >>
>> >> Also since the trip temperature's are in ascending order so some logic
>> >> is put in place to skip the un-necessary checks.
>> >>
>> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> >> ---
>> >>  Documentation/thermal/sysfs-api.txt |    4 +-
>> >>  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--
>> >>  include/linux/thermal.h             |    1 +
>> >>  3 files changed, 45 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-
>> >> api.txt
>> >> index 1733ab9..7a0c080 100644
>> >> --- a/Documentation/thermal/sysfs-api.txt
>> >> +++ b/Documentation/thermal/sysfs-api.txt
>> >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
>> >>
>> >>  trip_point_[0-*]_type
>> >>       Strings which indicate the type of the trip point.
>> >> -     E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
>> >> -     thermal zone.
>> >> +     E.g. it can be one of critical, hot, passive, active[0-1],
>> >> +     state-active[0-*] for ACPI thermal zone.
>> >>       RO, Optional
>> >>
>> >>  cdev[0-*]
>> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> >> index 220ce7e..d4c9b20 100644
>> >> --- a/drivers/thermal/thermal_sys.c
>> >> +++ b/drivers/thermal/thermal_sys.c
>> >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct
>> >> device_attribute *attr,
>> >>               return sprintf(buf, "passive\n");
>> >>       case THERMAL_TRIP_ACTIVE:
>> >>               return sprintf(buf, "active\n");
>> >> +     case THERMAL_TRIP_STATE_ACTIVE:
>> >> +             return sprintf(buf, "state-active\n");
>> >>       default:
>> >>               return sprintf(buf, "unknown\n");
>> >>       }
>> >> @@ -1034,10 +1036,10 @@ 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, ret = 0, inst_id;
>> >> +     long temp, trip_temp, max_state, last_trip_change = 0;
>> >>       enum thermal_trip_type trip_type;
>> >> -     struct thermal_cooling_device_instance *instance;
>> >> +     struct thermal_cooling_device_instance *instance, *state_instance;
>> >>       struct thermal_cooling_device *cdev;
>> >>
>> >>       mutex_lock(&tz->lock);
>> >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct
>> >> thermal_zone_device *tz)
>> >>                                       cdev->ops->set_cur_state(cdev, 0);
>> >>                       }
>> >>                       break;
>> >> +             case THERMAL_TRIP_STATE_ACTIVE:
>> >> +                     list_for_each_entry(instance, &tz->cooling_devices,
>> >> +                                         node) {
>> >> +                             if (instance->trip != count)
>> >> +                                     continue;
>> >> +
>> >> +                             if (temp <= last_trip_change)
>> >> +                                     continue;
>> >> +
>> >> +                             inst_id = 0;
>> >> +                             /*
>> >> +                             *For this instance how many instance of same
>> >> +                             *cooling device occured before
>> >> +                             */
>> >> +
>> >> +                             list_for_each_entry(state_instance,
>> >> +                                             &tz->cooling_devices, node) {
>> >> +                                     if (instance->cdev ==
>> >> +                                                     state_instance->cdev)
>> >> +                                             inst_id++;
>> >> +                                     if (state_instance->trip == count)
>> >> +                                             break;
>> >> +                             }
>> >
>> > Can you explain a bit more on this loop and the set_cur_state below ?
>> > Sorry, I don't get the logic behind this..
>>
>> This loop is basically finding the instance id of the same cooling device.
>> Say we have done like this,
>> thermal_zone_bind_cooling_device(thermal, 2, cdev);
>> thermal_zone_bind_cooling_device(thermal, 3, cdev);
>> thermal_zone_bind_cooling_device(thermal, 4, cdev);
>>
>> In above same cooling device cdev is binded to trip no 2,3 and 4 with
>> inst_id generated as 1,2,3 respectively. so set_cur_state for those
>> trip reached will be called as,
>> set_cur_state(cdev, 1);
>> set_cur_state(cdev, 2);
>> set_cur_state(cdev, 3);
>
> In this case, why a simple state = get_cur_state() followed by a
> set_cur_state(++state) / set_cur_state(--state) is not enough?

Thanks for looking into the patch. Well actually what you are
suggesting is exactly happening in PASSIVE trip types where the states
are incremented or decremented based on thermal trend. On the contrary
what this part of code is doing is to jump to a fixed state as and
when a trip point is reached.  The cooling effect of a frequency level
is known beforehand and hence jumping into that is safe and also this
does not cause performance degradation by going into a much lower
frequency state just for temperature stablization.

>
> Another thing is if we want to do jumps in the sequence?
>
>  set_cur_state(cdev, 1);
>  set_cur_state(cdev, 3);
>  set_cur_state(cdev, 6);
>
> But for that we need a table mapping, trip vs. state.
>
>
> What do you think?
In the current thermal_zone_device_update implementation all the
checks are currently based on increase in temperature. So even though
we want to go to  set_cur_state(cdev, 6) we have to follow
set_cur_state(cdev, 1), set_cur_state(cdev, 2) ,..... and finally
set_cur_state(cdev, 6). So mapping table may not be needed as state
transition is one by one. This a type of limitation but I think there
can be some kind of modification in the way the
thermal_zone_device_update calls all the lower temperature cooling
devices instead of jumping directly to the final trip point cooling
devices.
>
>>
>> Thanks,
>> Amit D
>>
>> >
>> > Thanks,
>> > Durga
>> >
>> >> +
>> >> +                             cdev = instance->cdev;
>> >> +                             cdev->ops->get_max_state(cdev, &max_state);
>> >> +
>> >> +                             if ((temp >= trip_temp) &&
>> >> +                                             (inst_id <= max_state))
>> >> +                                     cdev->ops->set_cur_state(cdev, inst_id);
>> >> +                             else if ((temp < trip_temp) &&
>> >> +                                             (--inst_id <= max_state))
>> >> +                                     cdev->ops->set_cur_state(cdev, inst_id);
>> >> +
>> >> +                             last_trip_change = trip_temp;
>> >> +                     }
>> >> +                     break;
>> >>               case THERMAL_TRIP_PASSIVE:
>> >>                       if (temp >= trip_temp || tz->passive)
>> >>                               thermal_zone_device_passive(tz, temp,
>> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> >> index 796f1ff..8df901f 100644
>> >> --- a/include/linux/thermal.h
>> >> +++ b/include/linux/thermal.h
>> >> @@ -42,6 +42,7 @@ enum thermal_trip_type {
>> >>       THERMAL_TRIP_PASSIVE,
>> >>       THERMAL_TRIP_HOT,
>> >>       THERMAL_TRIP_CRITICAL,
>> >> +     THERMAL_TRIP_STATE_ACTIVE,
>> >>  };
>> >>
>> >>  struct thermal_zone_device_ops {
>> >> --
>> >> 1.7.1
>> >
>> _______________________________________________
>> linux-pm mailing list
>> linux-pm@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [linux-pm] [PATCH 1/4] thermal: Add a new trip type to use cooling device instance number

[Eduardo Valentin]

Hello,

On Wed, Apr 04, 2012 at 09:53:15AM +0530, Amit Kachhap wrote:
> Hi Eduardo,
> 
> On 3 April 2012 19:45, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> > Hello,
> >
> > On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote:
> >> On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote:
> >> > Hi Amit,
> >> >
> >> >> -----Original Message-----
> >> >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
> >> >> Kachhap
> >> >> Sent: Wednesday, February 22, 2012 3:44 PM
> >> >> To: linux-pm@lists.linux-foundation.org
> >> >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
> >> >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org;
> >> >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org
> >> >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device
> >> >> instance number
> >> >>
> >> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
> >> >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
> >> >> device instance number. This helps the cooling device registered as
> >> >> different instances to perform appropriate cooling action decision in
> >> >> the set_cur_state call back function.
> >> >>
> >> >> Also since the trip temperature's are in ascending order so some logic
> >> >> is put in place to skip the un-necessary checks.
> >> >>
> >> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >> >> ---
> >> >>  Documentation/thermal/sysfs-api.txt |    4 +-
> >> >>  drivers/thermal/thermal_sys.c       |   45 ++++++++++++++++++++++++++++++++--
> >> >>  include/linux/thermal.h             |    1 +
> >> >>  3 files changed, 45 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-
> >> >> api.txt
> >> >> index 1733ab9..7a0c080 100644
> >> >> --- a/Documentation/thermal/sysfs-api.txt
> >> >> +++ b/Documentation/thermal/sysfs-api.txt
> >> >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
> >> >>
> >> >>  trip_point_[0-*]_type
> >> >>       Strings which indicate the type of the trip point.
> >> >> -     E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
> >> >> -     thermal zone.
> >> >> +     E.g. it can be one of critical, hot, passive, active[0-1],
> >> >> +     state-active[0-*] for ACPI thermal zone.
> >> >>       RO, Optional
> >> >>
> >> >>  cdev[0-*]
> >> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> >> >> index 220ce7e..d4c9b20 100644
> >> >> --- a/drivers/thermal/thermal_sys.c
> >> >> +++ b/drivers/thermal/thermal_sys.c
> >> >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct
> >> >> device_attribute *attr,
> >> >>               return sprintf(buf, "passive\n");
> >> >>       case THERMAL_TRIP_ACTIVE:
> >> >>               return sprintf(buf, "active\n");
> >> >> +     case THERMAL_TRIP_STATE_ACTIVE:
> >> >> +             return sprintf(buf, "state-active\n");
> >> >>       default:
> >> >>               return sprintf(buf, "unknown\n");
> >> >>       }
> >> >> @@ -1034,10 +1036,10 @@ 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, ret = 0, inst_id;
> >> >> +     long temp, trip_temp, max_state, last_trip_change = 0;
> >> >>       enum thermal_trip_type trip_type;
> >> >> -     struct thermal_cooling_device_instance *instance;
> >> >> +     struct thermal_cooling_device_instance *instance, *state_instance;
> >> >>       struct thermal_cooling_device *cdev;
> >> >>
> >> >>       mutex_lock(&tz->lock);
> >> >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct
> >> >> thermal_zone_device *tz)
> >> >>                                       cdev->ops->set_cur_state(cdev, 0);
> >> >>                       }
> >> >>                       break;
> >> >> +             case THERMAL_TRIP_STATE_ACTIVE:
> >> >> +                     list_for_each_entry(instance, &tz->cooling_devices,
> >> >> +                                         node) {
> >> >> +                             if (instance->trip != count)
> >> >> +                                     continue;
> >> >> +
> >> >> +                             if (temp <= last_trip_change)
> >> >> +                                     continue;
> >> >> +
> >> >> +                             inst_id = 0;
> >> >> +                             /*
> >> >> +                             *For this instance how many instance of same
> >> >> +                             *cooling device occured before
> >> >> +                             */
> >> >> +
> >> >> +                             list_for_each_entry(state_instance,
> >> >> +                                             &tz->cooling_devices, node) {
> >> >> +                                     if (instance->cdev ==
> >> >> +                                                     state_instance->cdev)
> >> >> +                                             inst_id++;
> >> >> +                                     if (state_instance->trip == count)
> >> >> +                                             break;
> >> >> +                             }
> >> >
> >> > Can you explain a bit more on this loop and the set_cur_state below ?
> >> > Sorry, I don't get the logic behind this..
> >>
> >> This loop is basically finding the instance id of the same cooling device.
> >> Say we have done like this,
> >> thermal_zone_bind_cooling_device(thermal, 2, cdev);
> >> thermal_zone_bind_cooling_device(thermal, 3, cdev);
> >> thermal_zone_bind_cooling_device(thermal, 4, cdev);
> >>
> >> In above same cooling device cdev is binded to trip no 2,3 and 4 with
> >> inst_id generated as 1,2,3 respectively. so set_cur_state for those
> >> trip reached will be called as,
> >> set_cur_state(cdev, 1);
> >> set_cur_state(cdev, 2);
> >> set_cur_state(cdev, 3);
> >
> > In this case, why a simple state = get_cur_state() followed by a
> > set_cur_state(++state) / set_cur_state(--state) is not enough?
> 
> Thanks for looking into the patch. Well actually what you are
> suggesting is exactly happening in PASSIVE trip types where the states
> are incremented or decremented based on thermal trend. On the contrary
> what this part of code is doing is to jump to a fixed state as and
> when a trip point is reached.  The cooling effect of a frequency level
> is known beforehand and hence jumping into that is safe and also this
> does not cause performance degradation by going into a much lower
> frequency state just for temperature stablization.

Yeah, I see that you want to match an instance of the cooling device with
a cooling state for that cooling device.

> 
> >
> > Another thing is if we want to do jumps in the sequence?
> >
> >  set_cur_state(cdev, 1);
> >  set_cur_state(cdev, 3);
> >  set_cur_state(cdev, 6);
> >
> > But for that we need a table mapping, trip vs. state.
> >
> >
> > What do you think?
> In the current thermal_zone_device_update implementation all the
> checks are currently based on increase in temperature. So even though
> we want to go to  set_cur_state(cdev, 6) we have to follow
> set_cur_state(cdev, 1), set_cur_state(cdev, 2) ,..... and finally
> set_cur_state(cdev, 6). So mapping table may not be needed as state
> transition is one by one. This a type of limitation but I think there
> can be some kind of modification in the way the
> thermal_zone_device_update calls all the lower temperature cooling
> devices instead of jumping directly to the final trip point cooling
> devices.


But, because you also force the thing to be increasing / decreasing 1 by 1,
I don't understand why you don't have a similar implementation of the
PASSIVE type. Just get its cur state and set the next state based on the curr.
How that would be different than what you are currently doing?

If you want to have a 1 to 1 relation between cooling device instance and cooling
device state, then you could do the jumps I mentioned, but only if you would
have the cooling state reference stored somewhere.

What bugs me is the fact that you have to be iterating in the cooling device list to determine
the next cooling state.

> >
> >>
> >> Thanks,
> >> Amit D
> >>
> >> >
> >> > Thanks,
> >> > Durga
> >> >
> >> >> +
> >> >> +                             cdev = instance->cdev;
> >> >> +                             cdev->ops->get_max_state(cdev, &max_state);
> >> >> +
> >> >> +                             if ((temp >= trip_temp) &&
> >> >> +                                             (inst_id <= max_state))
> >> >> +                                     cdev->ops->set_cur_state(cdev, inst_id);
> >> >> +                             else if ((temp < trip_temp) &&
> >> >> +                                             (--inst_id <= max_state))
> >> >> +                                     cdev->ops->set_cur_state(cdev, inst_id);
> >> >> +
> >> >> +                             last_trip_change = trip_temp;
> >> >> +                     }
> >> >> +                     break;
> >> >>               case THERMAL_TRIP_PASSIVE:
> >> >>                       if (temp >= trip_temp || tz->passive)
> >> >>                               thermal_zone_device_passive(tz, temp,
> >> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> >> index 796f1ff..8df901f 100644
> >> >> --- a/include/linux/thermal.h
> >> >> +++ b/include/linux/thermal.h
> >> >> @@ -42,6 +42,7 @@ enum thermal_trip_type {
> >> >>       THERMAL_TRIP_PASSIVE,
> >> >>       THERMAL_TRIP_HOT,
> >> >>       THERMAL_TRIP_CRITICAL,
> >> >> +     THERMAL_TRIP_STATE_ACTIVE,
> >> >>  };
> >> >>
> >> >>  struct thermal_zone_device_ops {
> >> >> --
> >> >> 1.7.1
> >> >
> >> _______________________________________________
> >> linux-pm mailing list
> >> linux-pm@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm