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 [email protected]
*Incorporated several review comments from [email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
+
+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 <[email protected]>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * 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 <[email protected]>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * 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
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 <[email protected]>
---
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
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)
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 [email protected]
> *Incorporated several review comments from [email protected]
>
> 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
Hi Amit,
> -----Original Message-----
> From: amit kachhap [mailto:[email protected]] On Behalf Of Amit Daniel
> Kachhap
> Sent: Wednesday, February 22, 2012 3:44 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; R, Durgadoss; [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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
On 23 February 2012 12:16, R, Durgadoss <[email protected]> wrote:
> Hi Amit,
>
>> -----Original Message-----
>> From: amit kachhap [mailto:[email protected]] On Behalf Of Amit Daniel
>> Kachhap
>> Sent: Wednesday, February 22, 2012 3:44 PM
>> To: [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; [email protected];
>> [email protected]; R, Durgadoss; [email protected]; [email protected]
>> 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 <[email protected]>
>> ---
>> ?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
>
Hi Amit,
> -----Original Message-----
> From: amit kachhap [mailto:[email protected]] On Behalf Of Amit Daniel
> Kachhap
> Sent: Wednesday, February 22, 2012 3:44 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; R, Durgadoss; [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> +
> +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 <[email protected]>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * 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 <[email protected]>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * 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
Hi Durgadoss,
Thanks for the detailed review comments.
On 24 February 2012 16:34, R, Durgadoss <[email protected]> wrote:
> Hi Amit,
>
>> -----Original Message-----
>> From: amit kachhap [mailto:[email protected]] On Behalf Of Amit Daniel
>> Kachhap
>> Sent: Wednesday, February 22, 2012 3:44 PM
>> To: [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; [email protected];
>> [email protected]; R, Durgadoss; [email protected]; [email protected]
>> 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 <[email protected]>
>> ---
>> ?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 <[email protected]>
>> +
>> +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 <[email protected]>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + * ?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 <[email protected]>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + * ?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
>