* Re: [PATCH 1/2] PM / QoS: unconditionally build the feature
From: mark gross @ 2012-02-12 2:06 UTC (permalink / raw)
To: Jean Pihet
Cc: Venkatesh Pallipadi, Mark Gross, linux-kernel,
Linux PM mailing list, Jean Pihet
In-Reply-To: <1328603646-2714-2-git-send-email-j-pihet@ti.com>
acked-by <markgross@thegnar.org>
On Tue, Feb 07, 2012 at 09:34:05AM +0100, Jean Pihet wrote:
> The PM QoS feature depends on CONFIG_PM which depends on
> PM_SLEEP || PM_RUNTIME. This breaks CPU C-states with kernels
> not having these CONFIGs.
>
> This patch allows the feature in all cases.
>
> Reported-by: Venkatesh Pallipadi <venki@google.com>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Mark Gross <markgross@thegnar.org>
> ---
> include/linux/pm_qos.h | 29 +----------------------------
> kernel/power/Makefile | 3 ++-
> 2 files changed, 3 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index e5bbcba..105be69 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -63,7 +63,6 @@ static inline int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
> return req->dev != 0;
> }
>
> -#ifdef CONFIG_PM
> int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> enum pm_qos_req_action action, int value);
> void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
> @@ -78,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
> int pm_qos_request_active(struct pm_qos_request *req);
> s32 pm_qos_read_value(struct pm_qos_constraints *c);
>
> +#ifdef CONFIG_PM
> s32 __dev_pm_qos_read_value(struct device *dev);
> s32 dev_pm_qos_read_value(struct device *dev);
> int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
> @@ -95,33 +95,6 @@ void dev_pm_qos_constraints_destroy(struct device *dev);
> int dev_pm_qos_add_ancestor_request(struct device *dev,
> struct dev_pm_qos_request *req, s32 value);
> #else
> -static inline int pm_qos_update_target(struct pm_qos_constraints *c,
> - struct plist_node *node,
> - enum pm_qos_req_action action,
> - int value)
> - { return 0; }
> -static inline void pm_qos_add_request(struct pm_qos_request *req,
> - int pm_qos_class, s32 value)
> - { return; }
> -static inline void pm_qos_update_request(struct pm_qos_request *req,
> - s32 new_value)
> - { return; }
> -static inline void pm_qos_remove_request(struct pm_qos_request *req)
> - { return; }
> -
> -static inline int pm_qos_request(int pm_qos_class)
> - { return 0; }
> -static inline int pm_qos_add_notifier(int pm_qos_class,
> - struct notifier_block *notifier)
> - { return 0; }
> -static inline int pm_qos_remove_notifier(int pm_qos_class,
> - struct notifier_block *notifier)
> - { return 0; }
> -static inline int pm_qos_request_active(struct pm_qos_request *req)
> - { return 0; }
> -static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
> - { return 0; }
> -
> static inline s32 __dev_pm_qos_read_value(struct device *dev)
> { return 0; }
> static inline s32 dev_pm_qos_read_value(struct device *dev)
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index 07e0e28..66d808e 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -1,7 +1,8 @@
>
> ccflags-$(CONFIG_PM_DEBUG) := -DDEBUG
>
> -obj-$(CONFIG_PM) += main.o qos.o
> +obj-y += qos.o
> +obj-$(CONFIG_PM) += main.o
> obj-$(CONFIG_VT_CONSOLE_SLEEP) += console.o
> obj-$(CONFIG_FREEZER) += process.o
> obj-$(CONFIG_SUSPEND) += suspend.o
> --
> 1.7.5.4
>
^ permalink raw reply
* Per CPU frequency constraints (was Re: [PATCH v2 0/8] RFC: CPU frequency min/max as PM QoS params)
From: Antti P Miettinen @ 2012-02-08 8:49 UTC (permalink / raw)
To: linux-pm; +Cc: cpufreq
In-Reply-To: <87vcnp93bl.fsf@amiettinen-lnx.nvidia.com>
Antti P Miettinen <amiettinen@nvidia.com> writes:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> On Sunday, January 22, 2012, Antti P Miettinen wrote:
> [..]
>>> Seems that the device specific constraints are not yet in use in
>>> 3.3-rc1, or am I not looking hard enough?
>>
>> They are in use through generic PM domains (drivers/base/power/domain*.c
>> and friends) and ARM/shmobile uses those.
>>
>> Thanks,
>> Rafael
>
> Sorry for the delay - got pre-empted by other stuff. I took a look at
> the per device constraints. Do I understand it correctly that the idea
> is that there is only one constraint per device? If we want to make
> frequency and latency per CPU I guess we'd need separate constraints
> associated with the CPU device. Or do I misunderstand something?
>
> Or would global CPU frequency be more in line with global CPU latency
> after all?
>
> --Antti
Ok - here's something - try not to laugh too hard. It's not pretty but
should serve as something for discussion the issues. This is on top of
the v3 patch series, against linus/master (3.3-rc2+). Previously I did
not CC people in my followups, I only posted to the lists. This was
probably wrong - sorry about that. This time I'm CCing people - if you
do not want to be CCd, please tell me. If you want this in some other
form (just a big patch with the previous series?), please tell me.
Anyway - does anyone have good solutions to the below issues? In general
- is per CPU worth the trouble?
1. Per device request ID space
Should each device have it's own ID space? I changed the one (implicitly
"latency") constraint to an array. But not all requests are relevant for
all devices so we waste some space. Is this an issue?
2. Systems with large number of processors
Dynamic minors for misc devices run out for those. Also the
pm_qos_object array is large for such systems. Also, hotpluggable CPUs
are kind of an issue. My laptop has possible-mask of 16 CPUs even though
I will never plug more CPUs to this system - at least not at
runtime. Also having a file handle per QoS request feels a bit
excessive. Can we require the data via the file handle to be more
structured line dev+id+value? Would require more elaborate book keeping
to clean up upon file close. Some better solution for the user space
interface?
3. Normal pm_qos_request vs dev_pm_qos_request
Maybe some refactoring could minimize the acrobatics related to the
function pointers and stuff I added to pm_qos_object.
And, yes, general cleanup and splitting to coherent changes would
definitely be required but I wanted to get opinions about what direction
to go with this.
--Antti
From: Antti P Miettinen <amiettinen@nvidia.com>
Date: Tue, 7 Feb 2012 15:20:15 +0200
Subject: [PATCH] cpufreq: PM QoS: Per CPU frequency constraints
Change frequency minimum and maximum into per device
constraints.
Signed-off-by: Antti P Miettinen <amiettinen@nvidia.com>
---
drivers/base/power/qos.c | 121 +++++++++++++++++++---------
drivers/base/power/runtime.c | 2 +-
drivers/cpufreq/cpufreq.c | 132 ++++++++++++++++++++++++++----
drivers/input/input-cfboost.c | 29 ++++++--
drivers/input/touchscreen/st1232.c | 3 +-
include/linux/pm_qos.h | 51 ++++++++++---
kernel/power/qos.c | 155 ++++++++++++++++++++----------------
7 files changed, 350 insertions(+), 143 deletions(-)
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index c5d3588..325930b 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -44,7 +44,23 @@
static DEFINE_MUTEX(dev_pm_qos_mtx);
-static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
+static const s32 pm_qos_dev_default[] = {
+ PM_QOS_DEV_LAT_DEFAULT_VALUE,
+ PM_QOS_CPU_FREQ_MIN_DEFAULT_VALUE,
+ PM_QOS_CPU_FREQ_MAX_DEFAULT_VALUE,
+};
+
+static const s32 pm_qos_dev_type[] = {
+ PM_QOS_MIN,
+ PM_QOS_MAX,
+ PM_QOS_MIN,
+};
+
+static struct blocking_notifier_head dev_pm_notifiers[] = {
+ BLOCKING_NOTIFIER_INIT(dev_pm_notifiers[0]),
+ BLOCKING_NOTIFIER_INIT(dev_pm_notifiers[1]),
+ BLOCKING_NOTIFIER_INIT(dev_pm_notifiers[2]),
+};
/**
* __dev_pm_qos_read_value - Get PM QoS constraint for a given device.
@@ -52,24 +68,24 @@ static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
*
* This routine must be called with dev->power.lock held.
*/
-s32 __dev_pm_qos_read_value(struct device *dev)
+s32 __dev_pm_qos_read_value(struct device *dev, int id)
{
struct pm_qos_constraints *c = dev->power.constraints;
-
- return c ? pm_qos_read_value(c) : 0;
+ BUG_ON(id >= PM_QOS_DEV_NUM_CLASSES);
+ return c ? pm_qos_read_value(&c[id]) : pm_qos_dev_default[id];
}
/**
* dev_pm_qos_read_value - Get PM QoS constraint for a given device (locked).
* @dev: Device to get the PM QoS constraint value for.
*/
-s32 dev_pm_qos_read_value(struct device *dev)
+s32 dev_pm_qos_read_value(struct device *dev, int id)
{
unsigned long flags;
s32 ret;
spin_lock_irqsave(&dev->power.lock, flags);
- ret = __dev_pm_qos_read_value(dev);
+ ret = __dev_pm_qos_read_value(dev, id);
spin_unlock_irqrestore(&dev->power.lock, flags);
return ret;
@@ -89,14 +105,16 @@ static int apply_constraint(struct dev_pm_qos_request *req,
enum pm_qos_req_action action, int value)
{
int ret, curr_value;
+ struct pm_qos_constraints *c;
- ret = pm_qos_update_target(req->dev->power.constraints,
+ c = &req->dev->power.constraints[req->dev_class];
+ ret = pm_qos_update_target(c,
&req->node, action, value);
if (ret) {
/* Call the global callbacks if needed */
- curr_value = pm_qos_read_value(req->dev->power.constraints);
- blocking_notifier_call_chain(&dev_pm_notifiers,
+ curr_value = pm_qos_read_value(c);
+ blocking_notifier_call_chain(&dev_pm_notifiers[req->dev_class],
(unsigned long)curr_value,
req);
}
@@ -105,33 +123,38 @@ static int apply_constraint(struct dev_pm_qos_request *req,
}
/*
- * dev_pm_qos_constraints_allocate
+ * __dev_pm_qos_constraints_allocate
* @dev: device to allocate data for
*
* Called at the first call to add_request, for constraint data allocation
* Must be called with the dev_pm_qos_mtx mutex held
*/
-static int dev_pm_qos_constraints_allocate(struct device *dev)
+static int __dev_pm_qos_constraints_allocate(struct device *dev)
{
struct pm_qos_constraints *c;
struct blocking_notifier_head *n;
+ int i;
- c = kzalloc(sizeof(*c), GFP_KERNEL);
+ c = kzalloc(sizeof(*c) * PM_QOS_DEV_NUM_CLASSES, GFP_KERNEL);
if (!c)
return -ENOMEM;
- n = kzalloc(sizeof(*n), GFP_KERNEL);
+ n = kzalloc(sizeof(*n) * PM_QOS_DEV_NUM_CLASSES, GFP_KERNEL);
if (!n) {
kfree(c);
return -ENOMEM;
}
- BLOCKING_INIT_NOTIFIER_HEAD(n);
- plist_head_init(&c->list);
- c->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
- c->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
- c->type = PM_QOS_MIN;
- c->notifiers = n;
+ for (i = 0; i < PM_QOS_DEV_NUM_CLASSES; ++i) {
+ BLOCKING_INIT_NOTIFIER_HEAD(&n[i]);
+
+ plist_head_init(&c[i].list);
+ c[i].target_value = pm_qos_dev_default[i];
+ c[i].default_value = pm_qos_dev_default[i];
+ c[i].type = pm_qos_dev_type[i];
+ c[i].notifiers = &n[i];
+
+ }
spin_lock_irq(&dev->power.lock);
dev->power.constraints = c;
@@ -140,6 +163,16 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
return 0;
}
+int dev_pm_qos_constraints_allocate(struct device *dev)
+{
+ int ret;
+ mutex_lock(&dev_pm_qos_mtx);
+ ret = __dev_pm_qos_constraints_allocate(dev);
+ mutex_unlock(&dev_pm_qos_mtx);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_qos_constraints_allocate);
+
/**
* dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
* @dev: target device
@@ -165,6 +198,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
{
struct dev_pm_qos_request *req, *tmp;
struct pm_qos_constraints *c;
+ int i;
mutex_lock(&dev_pm_qos_mtx);
@@ -173,21 +207,25 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
if (!c)
goto out;
- /* Flush the constraints list for the device */
- plist_for_each_entry_safe(req, tmp, &c->list, node) {
- /*
- * Update constraints list and call the notification
- * callbacks if needed
- */
- apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
- memset(req, 0, sizeof(*req));
+ for (i = 0; i < PM_QOS_DEV_NUM_CLASSES; ++i) {
+ c = &dev->power.constraints[i];
+ /* Flush the constraints list for the device */
+ plist_for_each_entry_safe(req, tmp, &c->list, node) {
+ /*
+ * Update constraints list and call the notification
+ * callbacks if needed
+ */
+ apply_constraint(req, PM_QOS_REMOVE_REQ,
+ PM_QOS_DEFAULT_VALUE);
+ memset(req, 0, sizeof(*req));
+ }
}
spin_lock_irq(&dev->power.lock);
dev->power.constraints = NULL;
spin_unlock_irq(&dev->power.lock);
- kfree(c->notifiers);
+ kfree(c[0].notifiers);
kfree(c);
out:
@@ -213,7 +251,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
* from the system.
*/
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
- s32 value)
+ int id, s32 value)
{
int ret = 0;
@@ -225,6 +263,7 @@ int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
return -EINVAL;
req->dev = dev;
+ req->dev_class = id;
mutex_lock(&dev_pm_qos_mtx);
@@ -346,7 +385,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
* Will register the notifier into a notification chain that gets called
* upon changes to the target value for the device.
*/
-int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
+int dev_pm_qos_add_notifier(struct device *dev, int id,
+ struct notifier_block *notifier)
{
int retval = 0;
@@ -355,7 +395,7 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
/* Silently return if the constraints object is not present. */
if (dev->power.constraints)
retval = blocking_notifier_chain_register(
- dev->power.constraints->notifiers,
+ dev->power.constraints[id].notifiers,
notifier);
mutex_unlock(&dev_pm_qos_mtx);
@@ -373,7 +413,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
* Will remove the notifier from the notification chain that gets called
* upon changes to the target value.
*/
-int dev_pm_qos_remove_notifier(struct device *dev,
+int dev_pm_qos_remove_notifier(struct device *dev, int id,
struct notifier_block *notifier)
{
int retval = 0;
@@ -383,7 +423,7 @@ int dev_pm_qos_remove_notifier(struct device *dev,
/* Silently return if the constraints object is not present. */
if (dev->power.constraints)
retval = blocking_notifier_chain_unregister(
- dev->power.constraints->notifiers,
+ dev->power.constraints[id].notifiers,
notifier);
mutex_unlock(&dev_pm_qos_mtx);
@@ -400,9 +440,10 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
* Will register the notifier into a notification chain that gets called
* upon changes to the target value for any device.
*/
-int dev_pm_qos_add_global_notifier(struct notifier_block *notifier)
+int dev_pm_qos_add_global_notifier(struct notifier_block *notifier, int id)
{
- return blocking_notifier_chain_register(&dev_pm_notifiers, notifier);
+ return blocking_notifier_chain_register(&dev_pm_notifiers[id],
+ notifier);
}
EXPORT_SYMBOL_GPL(dev_pm_qos_add_global_notifier);
@@ -415,9 +456,10 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_global_notifier);
* Will remove the notifier from the notification chain that gets called
* upon changes to the target value for any device.
*/
-int dev_pm_qos_remove_global_notifier(struct notifier_block *notifier)
+int dev_pm_qos_remove_global_notifier(struct notifier_block *notifier, int id)
{
- return blocking_notifier_chain_unregister(&dev_pm_notifiers, notifier);
+ return blocking_notifier_chain_unregister(&dev_pm_notifiers[id],
+ notifier);
}
EXPORT_SYMBOL_GPL(dev_pm_qos_remove_global_notifier);
@@ -428,7 +470,8 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_global_notifier);
* @value: Constraint latency value.
*/
int dev_pm_qos_add_ancestor_request(struct device *dev,
- struct dev_pm_qos_request *req, s32 value)
+ struct dev_pm_qos_request *req,
+ int id, s32 value)
{
struct device *ancestor = dev->parent;
int error = -ENODEV;
@@ -437,7 +480,7 @@ int dev_pm_qos_add_ancestor_request(struct device *dev,
ancestor = ancestor->parent;
if (ancestor)
- error = dev_pm_qos_add_request(ancestor, req, value);
+ error = dev_pm_qos_add_request(ancestor, req, id, value);
if (error)
req->dev = NULL;
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 541f821..cc424bc 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -445,7 +445,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
goto out;
}
- qos.constraint_ns = __dev_pm_qos_read_value(dev);
+ qos.constraint_ns = __dev_pm_qos_read_value(dev, PM_QOS_DEV_LATENCY);
if (qos.constraint_ns < 0) {
/* Negative constraint means "never suspend". */
retval = -EPERM;
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d233a8b..89c61ba 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1634,11 +1634,14 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
struct cpufreq_policy *policy)
{
int ret = 0;
+ struct device *dev = get_cpu_device(policy->cpu);
unsigned int pmin = policy->min;
unsigned int pmax = policy->max;
- unsigned int qmin = min(pm_qos_request(PM_QOS_CPU_FREQ_MIN),
+ unsigned int qmin = min(dev_pm_qos_read_value(dev,
+ PM_QOS_CPU_FREQ_MIN),
data->max);
- unsigned int qmax = max(pm_qos_request(PM_QOS_CPU_FREQ_MAX),
+ unsigned int qmax = max(dev_pm_qos_read_value(dev,
+ PM_QOS_CPU_FREQ_MAX),
data->min);
pr_debug("setting new policy for CPU %u: %u/%u - %u/%u kHz\n",
@@ -1930,23 +1933,22 @@ static struct notifier_block max_freq_notifier = {
static int cpu_freq_notify(struct notifier_block *b,
unsigned long l, void *v)
{
- int cpu;
- pr_debug("PM QoS %s %lu\n",
- b == &min_freq_notifier ? "min" : "max", l);
- for_each_online_cpu(cpu) {
- struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
- if (policy) {
- cpufreq_update_policy(policy->cpu);
- cpufreq_cpu_put(policy);
- }
+ struct dev_pm_qos_request *req = v;
+ int cpu = req->dev->id;
+ struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+ pr_debug("CPU%d PM QoS %s: %lu\n",
+ cpu, b == &min_freq_notifier ? "min" : "max", l);
+ if (policy) {
+ cpufreq_update_policy(policy->cpu);
+ cpufreq_cpu_put(policy);
}
+
return NOTIFY_OK;
}
static int __init cpufreq_core_init(void)
{
int cpu;
- int rc;
for_each_possible_cpu(cpu) {
per_cpu(cpufreq_policy_cpu, cpu) = -1;
@@ -1956,13 +1958,107 @@ static int __init cpufreq_core_init(void)
cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
BUG_ON(!cpufreq_global_kobject);
register_syscore_ops(&cpufreq_syscore_ops);
- rc = pm_qos_add_notifier(PM_QOS_CPU_FREQ_MIN,
- &min_freq_notifier);
- BUG_ON(rc);
- rc = pm_qos_add_notifier(PM_QOS_CPU_FREQ_MAX,
- &max_freq_notifier);
- BUG_ON(rc);
return 0;
}
core_initcall(cpufreq_core_init);
+
+struct cpufreq_pm_qos {
+ struct pm_qos_object obj_min;
+ struct pm_qos_object obj_max;
+ char name_min[32];
+ char name_max[32];
+};
+static DEFINE_PER_CPU(struct cpufreq_pm_qos, cf_pq);
+
+static void *pm_qos_cpu_add(void *p)
+{
+ struct pm_qos_object *o = p;
+ struct dev_pm_qos_request *req = kzalloc(sizeof(*req), GFP_KERNEL);
+ struct device *dev = o->data;
+ BUG_ON(!dev);
+ if (!req)
+ return 0;
+ dev_pm_qos_add_request(dev, req, o->pm_qos_dev_class,
+ PM_QOS_DEFAULT_VALUE);
+ return req;
+}
+
+static s32 pm_qos_cpu_read(void *p)
+{
+ struct pm_qos_object *o = p;
+ struct device *dev = o->data;
+ BUG_ON(!dev);
+ return dev_pm_qos_read_value(dev, o->pm_qos_dev_class);
+}
+
+static void pm_qos_cpu_write(void *p, s32 value)
+{
+ struct pm_qos_object *o = p;
+ struct dev_pm_qos_request *req = o->req;
+ dev_pm_qos_update_request(req, value);
+}
+
+static void pm_qos_cpu_remove(void *p)
+{
+ struct pm_qos_object *o = p;
+ struct dev_pm_qos_request *req = o->req;
+ dev_pm_qos_remove_request(req);
+ kfree(req);
+ o->req = 0;
+}
+
+static int __init cpufreq_pm_qos_init(void)
+{
+ int cpu;
+ int rc;
+
+ for_each_possible_cpu(cpu) {
+ struct device *dev = get_cpu_device(cpu);
+ if (!dev) {
+ pr_info("CPU%d: skipping\n", cpu);
+ continue;
+ }
+ rc = dev_pm_qos_constraints_allocate(dev);
+ BUG_ON(rc);
+ /* register interface for min freq */
+ sprintf(per_cpu(cf_pq, cpu).name_min, "cpu%d_freq_min", cpu);
+ per_cpu(cf_pq, cpu).obj_min.name
+ = per_cpu(cf_pq, cpu).name_min;
+ per_cpu(cf_pq, cpu).obj_min.constraints =
+ &dev->power.constraints[PM_QOS_CPU_FREQ_MIN];
+ per_cpu(cf_pq, cpu).obj_min.add = pm_qos_cpu_add;
+ per_cpu(cf_pq, cpu).obj_min.read = pm_qos_cpu_read;
+ per_cpu(cf_pq, cpu).obj_min.write = pm_qos_cpu_write;
+ per_cpu(cf_pq, cpu).obj_min.remove = pm_qos_cpu_remove;
+ per_cpu(cf_pq, cpu).obj_min.data = dev;
+ per_cpu(cf_pq, cpu).obj_min.pm_qos_dev_class
+ = PM_QOS_CPU_FREQ_MIN;
+ rc = pm_qos_register_misc(&per_cpu(cf_pq, cpu).obj_min);
+ BUG_ON(rc);
+ /* register interface for max freq */
+ sprintf(per_cpu(cf_pq, cpu).name_max, "cpu%d_freq_max", cpu);
+ per_cpu(cf_pq, cpu).obj_max.constraints =
+ &dev->power.constraints[PM_QOS_CPU_FREQ_MAX];
+ per_cpu(cf_pq, cpu).obj_max.name
+ = per_cpu(cf_pq, cpu).name_max;
+ per_cpu(cf_pq, cpu).obj_max.add = pm_qos_cpu_add;
+ per_cpu(cf_pq, cpu).obj_max.read = pm_qos_cpu_read;
+ per_cpu(cf_pq, cpu).obj_max.write = pm_qos_cpu_write;
+ per_cpu(cf_pq, cpu).obj_max.remove = pm_qos_cpu_remove;
+ per_cpu(cf_pq, cpu).obj_max.data = dev;
+ per_cpu(cf_pq, cpu).obj_max.pm_qos_dev_class
+ = PM_QOS_CPU_FREQ_MAX;
+ rc = pm_qos_register_misc(&per_cpu(cf_pq, cpu).obj_max);
+ BUG_ON(rc);
+ rc = dev_pm_qos_add_notifier(dev, PM_QOS_CPU_FREQ_MIN,
+ &min_freq_notifier);
+ BUG_ON(rc);
+ rc = dev_pm_qos_add_notifier(dev, PM_QOS_CPU_FREQ_MAX,
+ &max_freq_notifier);
+ BUG_ON(rc);
+ }
+
+ return 0;
+}
+late_initcall(cpufreq_pm_qos_init);
diff --git a/drivers/input/input-cfboost.c b/drivers/input/input-cfboost.c
index bef3ec5..52f0a38 100644
--- a/drivers/input/input-cfboost.c
+++ b/drivers/input/input-cfboost.c
@@ -25,6 +25,7 @@
#include <linux/input.h>
#include <linux/module.h>
#include <linux/pm_qos.h>
+#include <linux/cpu.h>
/* This module listens to input events and sets a temporary frequency
* floor upon input event detection. This is based on changes to
@@ -48,7 +49,7 @@ MODULE_DESCRIPTION("Input event CPU frequency booster");
MODULE_LICENSE("GPL v2");
-static struct pm_qos_request qos_req;
+static DEFINE_PER_CPU(struct dev_pm_qos_request, qos_req);
static struct work_struct boost;
static struct delayed_work unboost;
static unsigned int boost_freq; /* kHz */
@@ -59,14 +60,21 @@ static struct workqueue_struct *cfb_wq;
static void cfb_boost(struct work_struct *w)
{
+ int cpu;
cancel_delayed_work_sync(&unboost);
- pm_qos_update_request(&qos_req, boost_freq);
+ for_each_online_cpu(cpu) {
+ dev_pm_qos_update_request(&per_cpu(qos_req, cpu), boost_freq);
+ }
queue_delayed_work(cfb_wq, &unboost, msecs_to_jiffies(boost_time));
}
static void cfb_unboost(struct work_struct *w)
{
- pm_qos_update_request(&qos_req, PM_QOS_DEFAULT_VALUE);
+ int cpu;
+ for_each_online_cpu(cpu) {
+ dev_pm_qos_update_request(&per_cpu(qos_req, cpu),
+ PM_QOS_DEFAULT_VALUE);
+ }
}
static void cfb_input_event(struct input_handle *handle, unsigned int type,
@@ -142,6 +150,7 @@ static struct input_handler cfb_input_handler = {
static int __init cfboost_init(void)
{
int ret;
+ int cpu;
cfb_wq = create_workqueue("icfb-wq");
if (!cfb_wq)
@@ -153,13 +162,19 @@ static int __init cfboost_init(void)
destroy_workqueue(cfb_wq);
return ret;
}
- pm_qos_add_request(&qos_req, PM_QOS_CPU_FREQ_MIN,
- PM_QOS_DEFAULT_VALUE);
+ for_each_possible_cpu(cpu) {
+ struct device *dev = get_cpu_device(cpu);
+ dev_pm_qos_add_request(dev, &per_cpu(qos_req, cpu),
+ PM_QOS_CPU_FREQ_MIN,
+ PM_QOS_DEFAULT_VALUE);
+ }
return 0;
}
static void __exit cfboost_exit(void)
{
+ int cpu;
+
/* stop input events */
input_unregister_handler(&cfb_input_handler);
/* cancel pending work requests */
@@ -167,7 +182,9 @@ static void __exit cfboost_exit(void)
cancel_delayed_work_sync(&unboost);
/* clean up */
destroy_workqueue(cfb_wq);
- pm_qos_remove_request(&qos_req);
+ for_each_possible_cpu(cpu) {
+ dev_pm_qos_remove_request(&per_cpu(qos_req, cpu));
+ }
}
module_init(cfboost_init);
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 8825fe3..9f68c98 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -129,7 +129,8 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
} else if (!ts->low_latency_req.dev) {
/* First contact, request 100 us latency. */
dev_pm_qos_add_ancestor_request(&ts->client->dev,
- &ts->low_latency_req, 100);
+ &ts->low_latency_req,
+ PM_QOS_DEV_LATENCY, 100);
}
/* SYN_REPORT */
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index fedda35..843dc22 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -9,19 +9,27 @@
#include <linux/miscdevice.h>
#include <linux/device.h>
+/* Global */
enum {
PM_QOS_RESERVED = 0,
PM_QOS_CPU_DMA_LATENCY,
PM_QOS_NETWORK_LATENCY,
PM_QOS_NETWORK_THROUGHPUT,
- PM_QOS_CPU_FREQ_MIN,
- PM_QOS_CPU_FREQ_MAX,
/* insert new class ID */
PM_QOS_NUM_CLASSES,
};
+/* Per device */
+enum {
+ PM_QOS_DEV_LATENCY,
+ PM_QOS_CPU_FREQ_MIN,
+ PM_QOS_CPU_FREQ_MAX,
+
+ PM_QOS_DEV_NUM_CLASSES,
+};
+
#define PM_QOS_DEFAULT_VALUE -1
#define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
@@ -39,6 +47,8 @@ struct pm_qos_request {
struct dev_pm_qos_request {
struct plist_node node;
struct device *dev;
+ int dev_class;
+ int pm_qos_class;
};
enum pm_qos_type {
@@ -67,6 +77,20 @@ enum pm_qos_req_action {
PM_QOS_REMOVE_REQ /* Remove an existing request */
};
+struct pm_qos_object {
+ struct pm_qos_constraints *constraints;
+ struct miscdevice pm_qos_power_miscdev;
+ char *name;
+ int pm_qos_class;
+ int pm_qos_dev_class;
+ void *data;
+ void *req;
+ void *(*add)(void *);
+ s32 (*read)(void *);
+ void (*write)(void *, s32);
+ void (*remove)(void *);
+};
+
static inline int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
{
return req->dev != 0;
@@ -84,25 +108,28 @@ void pm_qos_remove_request(struct pm_qos_request *req);
int pm_qos_request(int pm_qos_class);
int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
+int pm_qos_register_misc(struct pm_qos_object *qos);
int pm_qos_request_active(struct pm_qos_request *req);
s32 pm_qos_read_value(struct pm_qos_constraints *c);
-s32 __dev_pm_qos_read_value(struct device *dev);
-s32 dev_pm_qos_read_value(struct device *dev);
+s32 __dev_pm_qos_read_value(struct device *dev, int id);
+s32 dev_pm_qos_read_value(struct device *dev, int id);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
- s32 value);
+ int id, s32 value);
int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
int dev_pm_qos_remove_request(struct dev_pm_qos_request *req);
-int dev_pm_qos_add_notifier(struct device *dev,
+int dev_pm_qos_add_notifier(struct device *dev, int id,
struct notifier_block *notifier);
-int dev_pm_qos_remove_notifier(struct device *dev,
+int dev_pm_qos_remove_notifier(struct device *dev, int id,
struct notifier_block *notifier);
-int dev_pm_qos_add_global_notifier(struct notifier_block *notifier);
-int dev_pm_qos_remove_global_notifier(struct notifier_block *notifier);
+int dev_pm_qos_add_global_notifier(struct notifier_block *notifier, int id);
+int dev_pm_qos_remove_global_notifier(struct notifier_block *notifier, int id);
+int dev_pm_qos_constraints_allocate(struct device *dev);
void dev_pm_qos_constraints_init(struct device *dev);
void dev_pm_qos_constraints_destroy(struct device *dev);
int dev_pm_qos_add_ancestor_request(struct device *dev,
- struct dev_pm_qos_request *req, s32 value);
+ struct dev_pm_qos_request *req,
+ int id, s32 value);
#else
static inline int pm_qos_update_target(struct pm_qos_constraints *c,
struct plist_node *node,
@@ -138,6 +165,8 @@ static inline int pm_qos_add_notifier(int pm_qos_class,
static inline int pm_qos_remove_notifier(int pm_qos_class,
struct notifier_block *notifier)
{ return 0; }
+static inline int pm_qos_register_misc(struct pm_qos_object *qos)
+ { return 0; }
static inline int pm_qos_request_active(struct pm_qos_request *req)
{ return 0; }
static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
@@ -168,6 +197,8 @@ static inline int dev_pm_qos_add_global_notifier(
static inline int dev_pm_qos_remove_global_notifier(
struct notifier_block *notifier)
{ return 0; }
+static inline int dev_pm_qos_constraints_allocate(struct device *dev)
+ { return 0; }
static inline void dev_pm_qos_constraints_init(struct device *dev)
{
dev->power.power_state = PMSG_ON;
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 04b744b..a3524ea 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -50,16 +50,22 @@
* or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
* held, taken with _irqsave. One lock to rule them all
*/
-struct pm_qos_object {
- struct pm_qos_constraints *constraints;
- struct miscdevice pm_qos_power_miscdev;
- char *name;
-};
static DEFINE_SPINLOCK(pm_qos_lock);
static struct pm_qos_object null_pm_qos;
+static void *pm_qos_global_add(void *);
+static s32 pm_qos_global_read(void *);
+static void pm_qos_global_write(void *, s32);
+static void pm_qos_global_remove(void *);
+
+#define PM_QOS_OBJ_INIT \
+ .add = pm_qos_global_add, \
+ .read = pm_qos_global_read, \
+ .write = pm_qos_global_write, \
+ .remove = pm_qos_global_remove
+
static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
static struct pm_qos_constraints cpu_dma_constraints = {
.list = PLIST_HEAD_INIT(cpu_dma_constraints.list),
@@ -71,6 +77,7 @@ static struct pm_qos_constraints cpu_dma_constraints = {
static struct pm_qos_object cpu_dma_pm_qos = {
.constraints = &cpu_dma_constraints,
.name = "cpu_dma_latency",
+ PM_QOS_OBJ_INIT,
};
static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
@@ -84,6 +91,7 @@ static struct pm_qos_constraints network_lat_constraints = {
static struct pm_qos_object network_lat_pm_qos = {
.constraints = &network_lat_constraints,
.name = "network_latency",
+ PM_QOS_OBJ_INIT,
};
@@ -98,44 +106,17 @@ static struct pm_qos_constraints network_tput_constraints = {
static struct pm_qos_object network_throughput_pm_qos = {
.constraints = &network_tput_constraints,
.name = "network_throughput",
+ PM_QOS_OBJ_INIT,
};
-static BLOCKING_NOTIFIER_HEAD(cpu_freq_min_notifier);
-static struct pm_qos_constraints cpu_freq_min_constraints = {
- .list = PLIST_HEAD_INIT(cpu_freq_min_constraints.list),
- .target_value = PM_QOS_CPU_FREQ_MIN_DEFAULT_VALUE,
- .default_value = PM_QOS_CPU_FREQ_MIN_DEFAULT_VALUE,
- .type = PM_QOS_MAX,
- .notifiers = &cpu_freq_min_notifier,
-};
-static struct pm_qos_object cpu_freq_min_pm_qos = {
- .constraints = &cpu_freq_min_constraints,
- .name = "cpu_freq_min",
-};
-
-
-static BLOCKING_NOTIFIER_HEAD(cpu_freq_max_notifier);
-static struct pm_qos_constraints cpu_freq_max_constraints = {
- .list = PLIST_HEAD_INIT(cpu_freq_max_constraints.list),
- .target_value = PM_QOS_CPU_FREQ_MAX_DEFAULT_VALUE,
- .default_value = PM_QOS_CPU_FREQ_MAX_DEFAULT_VALUE,
- .type = PM_QOS_MIN,
- .notifiers = &cpu_freq_max_notifier,
-};
-static struct pm_qos_object cpu_freq_max_pm_qos = {
- .constraints = &cpu_freq_max_constraints,
- .name = "cpu_freq_max",
-};
-
-
-static struct pm_qos_object *pm_qos_array[] = {
+#define PM_QOS_MAX_CLASSES (PM_QOS_NUM_CLASSES \
+ + NR_CPUS * PM_QOS_DEV_NUM_CLASSES)
+static struct pm_qos_object *pm_qos_array[PM_QOS_MAX_CLASSES] = {
&null_pm_qos,
&cpu_dma_pm_qos,
&network_lat_pm_qos,
&network_throughput_pm_qos,
- &cpu_freq_min_pm_qos,
- &cpu_freq_max_pm_qos,
};
static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
@@ -234,7 +215,7 @@ int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
if (prev_value != curr_value) {
blocking_notifier_call_chain(c->notifiers,
(unsigned long)curr_value,
- NULL);
+ node);
return 1;
} else {
return 0;
@@ -382,22 +363,30 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
}
EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
+static int pm_qos_ifcount = 1;
+
/* User space interface to PM QoS classes via misc devices */
-static int register_pm_qos_misc(struct pm_qos_object *qos)
+int pm_qos_register_misc(struct pm_qos_object *qos)
{
qos->pm_qos_power_miscdev.minor = MISC_DYNAMIC_MINOR;
qos->pm_qos_power_miscdev.name = qos->name;
qos->pm_qos_power_miscdev.fops = &pm_qos_power_fops;
-
+ if (pm_qos_array[pm_qos_ifcount] == 0)
+ pm_qos_array[pm_qos_ifcount] = qos;
+ else
+ BUG_ON(pm_qos_array[pm_qos_ifcount] != qos);
+ qos->pm_qos_class = pm_qos_ifcount;
+ ++pm_qos_ifcount;
return misc_register(&qos->pm_qos_power_miscdev);
}
+EXPORT_SYMBOL_GPL(pm_qos_register_misc);
static int find_pm_qos_object_by_minor(int minor)
{
int pm_qos_class;
for (pm_qos_class = 0;
- pm_qos_class < PM_QOS_NUM_CLASSES; pm_qos_class++) {
+ pm_qos_class < pm_qos_ifcount; pm_qos_class++) {
if (minor ==
pm_qos_array[pm_qos_class]->pm_qos_power_miscdev.minor)
return pm_qos_class;
@@ -411,12 +400,13 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
if (pm_qos_class >= 0) {
- struct pm_qos_request *req = kzalloc(sizeof(*req), GFP_KERNEL);
- if (!req)
+ struct pm_qos_object *o = pm_qos_array[pm_qos_class];
+ void *p = o->add(o);
+ if (!p)
return -ENOMEM;
- pm_qos_add_request(req, pm_qos_class, PM_QOS_DEFAULT_VALUE);
- filp->private_data = req;
+ o->req = p;
+ filp->private_data = o;
return 0;
}
@@ -425,12 +415,8 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
static int pm_qos_power_release(struct inode *inode, struct file *filp)
{
- struct pm_qos_request *req;
-
- req = filp->private_data;
- pm_qos_remove_request(req);
- kfree(req);
-
+ struct pm_qos_object *o = filp->private_data;
+ o->remove(o);
return 0;
}
@@ -439,18 +425,8 @@ static ssize_t pm_qos_power_read(struct file *filp, char __user *buf,
size_t count, loff_t *f_pos)
{
s32 value;
- unsigned long flags;
- struct pm_qos_request *req = filp->private_data;
-
- if (!req)
- return -EINVAL;
- if (!pm_qos_request_active(req))
- return -EINVAL;
-
- spin_lock_irqsave(&pm_qos_lock, flags);
- value = pm_qos_get_value(pm_qos_array[req->pm_qos_class]->constraints);
- spin_unlock_irqrestore(&pm_qos_lock, flags);
-
+ struct pm_qos_object *o = filp->private_data;
+ value = o->read(o);
return simple_read_from_buffer(buf, count, f_pos, &value, sizeof(s32));
}
@@ -458,7 +434,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
size_t count, loff_t *f_pos)
{
s32 value;
- struct pm_qos_request *req;
+ struct pm_qos_object *o;
if (count == sizeof(s32)) {
if (copy_from_user(&value, buf, sizeof(s32)))
@@ -489,22 +465,65 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
return -EINVAL;
}
- req = filp->private_data;
- pm_qos_update_request(req, value);
-
+ o = filp->private_data;
+ o->write(o, value);
return count;
}
+static void *pm_qos_global_add(void *p)
+{
+ struct pm_qos_object *o = p;
+ struct pm_qos_request *req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return 0;
+ pm_qos_add_request(req, o->pm_qos_class, PM_QOS_DEFAULT_VALUE);
+ return req;
+}
+
+static s32 pm_qos_global_read(void *p)
+{
+ struct pm_qos_object *o = p;
+ unsigned long flags;
+ struct pm_qos_request *req = o->req;
+ s32 value;
+
+ if (!req)
+ return -EINVAL;
+ if (!pm_qos_request_active(req))
+ return -EINVAL;
+
+ spin_lock_irqsave(&pm_qos_lock, flags);
+ value = pm_qos_get_value(pm_qos_array[req->pm_qos_class]->constraints);
+ spin_unlock_irqrestore(&pm_qos_lock, flags);
+ return value;
+}
+
+static void pm_qos_global_write(void *p, s32 value)
+{
+ struct pm_qos_object *o = p;
+ struct pm_qos_request *req = o->req;
+ pm_qos_update_request(req, value);
+}
+
+static void pm_qos_global_remove(void *p)
+{
+ struct pm_qos_object *o = p;
+ struct pm_qos_request *req = o->req;
+ pm_qos_remove_request(req);
+ kfree(req);
+ o->req = 0;
+}
+
static int __init pm_qos_power_init(void)
{
int ret = 0;
int i;
- BUILD_BUG_ON(ARRAY_SIZE(pm_qos_array) != PM_QOS_NUM_CLASSES);
+ BUILD_BUG_ON(ARRAY_SIZE(pm_qos_array) < PM_QOS_NUM_CLASSES);
for (i = 1; i < PM_QOS_NUM_CLASSES; i++) {
- ret = register_pm_qos_misc(pm_qos_array[i]);
+ ret = pm_qos_register_misc(pm_qos_array[i]);
if (ret < 0) {
printk(KERN_ERR "pm_qos_param: %s setup failed\n",
pm_qos_array[i]->name);
--
1.7.4.1
^ permalink raw reply related
* Re: [linux-pm] [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation
From: Eduardo Valentin @ 2012-02-07 19:34 UTC (permalink / raw)
To: Amit Kachhap
Cc: eduardo.valentin, linux-pm, linaro-dev, patches, linux-kernel,
linux-acpi
In-Reply-To: <CAK44p20Y5bLU-cUEvZqvzt300ZhtJ8z5KbRfLc8b2B=d3G=FsA@mail.gmail.com>
Hello Amit,
On Tue, Feb 07, 2012 at 10:21:15AM -0800, Amit Kachhap wrote:
> Hi eduardo,
>
> Again thanks for the review.
>
> On 7 February 2012 00:25, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> > Hello Amit,
> >
> > On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
> >> This patch adds support for generic cpu thermal cooling low level
> >> implementations using frequency scaling and cpuhotplugg currently.
> >> 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.
> >>
> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >> ---
> >> Documentation/thermal/cpu-cooling-api.txt | 52 +++++
> >> drivers/thermal/Kconfig | 11 +
> >> drivers/thermal/Makefile | 1 +
> >> drivers/thermal/cpu_cooling.c | 302 +++++++++++++++++++++++++++++
> >> include/linux/cpu_cooling.h | 45 +++++
> >> 5 files changed, 411 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..d30b4f2
> >> --- /dev/null
> >> +++ b/Documentation/thermal/cpu-cooling-api.txt
> >> @@ -0,0 +1,52 @@
> >> +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 types 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".
> >> +
> >> + tab_ptr: The table containing the percentage of frequency to be clipped for
> >> + each cooling state.
> >> + .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
> >> + cpu.
> >> + .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(void)
> >> +
> >> + This interface function unregisters the "thermal-cpufreq" cooling device.
> >> +
> >> +
> >> +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
> >> + "thermal-cpuhotplug".
> >> +
> >> + mask_val: all the allowed cpu's which can be hotplugged out.
> >> +
> >> +1.1.2 void cpuhotplug_cooling_unregister(void)
> >> +
> >> + This interface function unregisters the "thermal-cpuhotplug" cooling device.
> >> 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..cdd148c
> >> --- /dev/null
> >> +++ b/drivers/thermal/cpu_cooling.c
> >> @@ -0,0 +1,302 @@
> >> +/*
> >> + * 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 {
> >> + 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;
> >> +};
> >> +
> >> +static struct cpufreq_cooling_device *cpufreq_device;
> >> +
> >> +/*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(int cooling_state)
> >> +{
> >
> > Why do you need cooling_state to be signed? You used it always against
> > unsigned values. Besides, the thermal api imposes unsigned long.
> Yes, unsigned long is a better way.
> >
> >> + 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;
> >> +
> >> + 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 cpu = policy->cpu, th_pctg = 0, level;
> >> +
> >> + if (event != CPUFREQ_ADJUST)
> >> + return 0;
> >> +
> >> + level = cpufreq_device->cpufreq_state;
> >> +
> >> + if (level > 0) {
> >> + th_table =
> >> + &(cpufreq_device->tab_ptr[level - 1]);
> >> + th_pctg = th_table->freq_clip_pctg[cpu];
> >> + }
> >> +
> >> + max_freq =
> >> + (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
> >
> >
> > Might be interesting to extend this a bit. You could have the
> > above as default policy setup, but you could also allow people
> > to change this behavior. For instance, adding some callback option
> > while registering the cooling device (or a notifier), which would
> > determine the max_freq for the generic layer. The mechanism to
> > deploy max_freq into the system must be generic enough with cpufreq
> > APIs, but the decision to which max_freq to use, could be specific.
> > Don't you think?
> I agree that there will always some constraints on the system to cap
> the max frequency. But these thermal clippings is called for rare
> scenarios. Anyway, Your idea is worth considering. I will check if
> cpufreq has any generic way of achieving this.
Yeah, well, the idea was to have the max_freq value coming from some
registered callback in your code, not really on cpufreq.
> >
> >> +
> >> + 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)
> >> +{
> >> + *state = cpufreq_device->tab_size;
> >> + return 0;
> >> +}
> >> +
> >> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> >> + unsigned long *state)
> >> +{
> >> + *state = cpufreq_device->cpufreq_state;
> >> + return 0;
> >> +}
> >> +
> >> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> >> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> >> + unsigned long state)
> >> +{
> >> + cpufreq_apply_cooling(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;
> >> +
> >> + if (tab_ptr == NULL || tab_size == 0)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + cpufreq_device =
> >> + kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
> >> +
> >> + if (!cpufreq_device)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
> >> + &cpufreq_cooling_ops);
> >> + if (!cool_dev) {
> >> + kfree(cpufreq_device);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + cpufreq_device->tab_ptr = tab_ptr;
> >> + cpufreq_device->tab_size = tab_size;
> >> + cpufreq_device->cool_dev = cool_dev;
> >> + cpufreq_device->allowed_cpus = mask_val;
> >> +
> >> + cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> >> + CPUFREQ_POLICY_NOTIFIER);
> >> + return cool_dev;
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_register);
> >> +
> >> +void cpufreq_cooling_unregister(void)
> >> +{
> >> + cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> >> + CPUFREQ_POLICY_NOTIFIER);
> >> + thermal_cooling_device_unregister(cpufreq_device->cool_dev);
> >> + kfree(cpufreq_device);
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> >> +#else /*!CONFIG_CPU_FREQ*/
> >> +struct thermal_cooling_device *cpufreq_cooling_register(
> >> + struct freq_pctg_table *tab_ptr, unsigned int tab_size)
> >> +{
> >> + return NULL;
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_register);
> >> +void cpufreq_cooling_unregister(void)
> >> +{
> >> + return;
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> >> +#endif /*CONFIG_CPU_FREQ*/
> >> +
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> +
> >> +struct hotplug_cooling_device {
> >> + struct thermal_cooling_device *cool_dev;
> >> + unsigned int hotplug_state;
> >> + const struct cpumask *allowed_cpus;
> >> +};
> >> +static struct hotplug_cooling_device *hotplug_device;
> >> +
> >> +/*
> >> + * cpu hotplug cooling device callback functions
> >> + */
> >> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
> >> + unsigned long *state)
> >> +{
> >> + *state = 1;
> >> + return 0;
> >> +}
> >> +
> >> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
> >> + unsigned long *state)
> >> +{
> >> + /*This cooling device may be of type ACTIVE, so state field
> >> + can be 0 or 1*/
> >> + *state = hotplug_device->hotplug_state;
> >> + return 0;
> >> +}
> >> +
> >> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> >> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
> >> + unsigned long state)
> >> +{
> >> + int cpuid, this_cpu = smp_processor_id();
> >> +
> >> + if (hotplug_device->hotplug_state == state)
> >> + return 0;
> >> +
> >> + /*This cooling device may be of type ACTIVE, so state field
> >> + can be 0 or 1*/
> >
> > /*
> > * Small thing. Check the style for multi line comments.
> > * This is also an example.
> > */
> Ok.
> >
> >> + if (state == 1) {
> >> + for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> >> + if (cpu_online(cpuid) && (cpuid != this_cpu))
> >> + cpu_down(cpuid);
> >> + }
> >> + } else if (state == 0) {
> >> + for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> >> + if (!cpu_online(cpuid) && (cpuid != this_cpu))
> >> + cpu_up(cpuid);
> >> + }
> >> + } else
> >> + return -EINVAL;
> >> +
> >> + hotplug_device->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;
> >> +
> >> + hotplug_device =
> >> + kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
> >> +
> >> + if (!hotplug_device)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
> >> + &cpuhotplug_cooling_ops);
> >> + if (!cool_dev) {
> >> + kfree(hotplug_device);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + hotplug_device->cool_dev = cool_dev;
> >> + hotplug_device->hotplug_state = 0;
> >> + hotplug_device->allowed_cpus = mask_val;
> >> +
> >> + return cool_dev;
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> >> +
> >> +void cpuhotplug_cooling_unregister(void)
> >> +{
> >> + thermal_cooling_device_unregister(hotplug_device->cool_dev);
> >> + kfree(hotplug_device);
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> >> +#else /*!CONFIG_HOTPLUG_CPU*/
> >> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> >> + const struct cpumask *mask_val)
> >> +{
> >> + return NULL;
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> >> +void cpuhotplug_cooling_unregister(void)
> >> +{
> >> + return;
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> >> +#endif /*CONFIG_HOTPLUG_CPU*/
> >> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> >> new file mode 100644
> >> index 0000000..0c57375
> >> --- /dev/null
> >> +++ b/include/linux/cpu_cooling.h
> >> @@ -0,0 +1,45 @@
> >> +/*
> >> + * 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[NR_CPUS];
> >> + unsigned int polling_interval;
> >> +};
> >> +
> >> +extern struct thermal_cooling_device *cpufreq_cooling_register(
> >> + struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> >> + const struct cpumask *mask_val);
> >
> > I suppose your original idea was to have this function called only once right?
> > I'd suggest to add some documentation on this header file to specify this.
> >
> > Besides, the fact that you receive a cpumask may suggest you could register
> > different cooling device for subsets of your system cpus.
> >
> > In any case, if you call this function more than once, you may end with memory leaks
> > and the last call will be the one acting in the system.
> >
> yes these api's are meant to be called once. I am working on a new
> version where all these function will be multi-instance and hence
> different cpus can support different frequency clipping.
OK.
Another point, is about the fact that this implementation is clipping the frequency silently.
I mean, by changing the policy.max_freq, we may conflict with what the user defines as max freq.
> >
> >
> >> +
> >> +extern void cpufreq_cooling_unregister(void);
> >> +
> >> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
> >> + const struct cpumask *mask_val);
> >> +
> >> +extern void cpuhotplug_cooling_unregister(void);
> >> +
> >> +#endif /* __CPU_COOLING_H__ */
> >
> > Another thing, I have the impression that solving the config selection in the
> > header file instead of in .c files is cleaner. If you want to solve the config
> > selection in .c files, better to do so with Makefiles.
> >
> > If you keep the ifdefery ( THERMAL vs. HOTPLUG_CPU vs. CPU_FREQ ) here in the
> > header file, it is easier to read.
> Ok will move them in .h
> >
> >> --
> >> 1.7.1
> >>
> >> _______________________________________________
> >> linux-pm mailing list
> >> linux-pm@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [linux-pm] [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Eduardo Valentin @ 2012-02-07 19:29 UTC (permalink / raw)
To: Amit Kachhap
Cc: eduardo.valentin, linux-pm, linaro-dev, patches, linux-kernel,
linux-acpi, rob.lee
In-Reply-To: <CAK44p20-3+Ndn1pJmX7tT=UtYE+876_iNtqR+S0==wTh=iwvzA@mail.gmail.com>
Hello Amit,
On Tue, Feb 07, 2012 at 09:52:40AM -0800, Amit Kachhap wrote:
> Hi eduardo,
>
> Thanks for the detail review.
>
> On 6 February 2012 23:09, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> > Hello Amit,
> >
> > some comments embedded.
> >
> > On Wed, Jan 18, 2012 at 02:51:07PM +0530, Amit Daniel Kachhap wrote:
> >> Add a sysfs 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.
> >>
> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >> ---
> >> Documentation/thermal/sysfs-api.txt | 10 ++++
> >> drivers/thermal/thermal_sys.c | 96 +++++++++++++++++++++++++++++++++++
> >> include/linux/thermal.h | 8 +++
> >> 3 files changed, 114 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> >> index b61e46f..1db9a9d 100644
> >> --- a/Documentation/thermal/sysfs-api.txt
> >> +++ b/Documentation/thermal/sysfs-api.txt
> >> @@ -209,6 +209,13 @@ passive
> >> Valid values: 0 (disabled) or greater than 1000
> >> RW, Optional
> >>
> >> +trip_stats
> >> + This attribute presents the effective cooling generated from all the
> >> + cooling devices attached to a trip point. The output is a pair of value
> >> + for each trip point. Each pair represents
> >> + (trip index, cooling temperature difference in millidegree Celsius)
> >> + RO, Optional
> >> +
> >> *****************************
> >> * Cooling device attributes *
> >> *****************************
> >> @@ -261,6 +268,9 @@ method, the sys I/F structure will be built like this:
> >> |---cdev0_trip_point: 1 /* cdev0 can be used for passive */
> >> |---cdev1: --->/sys/class/thermal/cooling_device3
> >> |---cdev1_trip_point: 2 /* cdev1 can be used for active[0]*/
> >> + |---trip_stats 0 0
> >> + 1 8000 /*trip 1 causes 8 degree Celsius drop*/
> >> + 2 3000 /*trip 2 causes 3 degree Celsius drop*/
> >>
> >> |cooling_device0:
> >> |---type: Processor
> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> >> index dd9a574..47e7b6e 100644
> >> --- a/drivers/thermal/thermal_sys.c
> >> +++ b/drivers/thermal/thermal_sys.c
> >> @@ -92,6 +92,64 @@ static void release_idr(struct idr *idr, struct mutex *lock, int id)
> >> if (lock)
> >> mutex_unlock(lock);
> >> }
> >> +static void update_cooling_stats(struct thermal_zone_device *tz, long cur_temp)
> >> +{
> >> + int count, max_index, cur_interval;
> >> + long trip_temp, max_temp = 0, cool_temp;
> >> + static int last_trip_level = -1;
> >
> > I got confused here. Are you sure using a static variable here is safe? I suppose this function
> > is called for any thermal_zone_device, which in turns, may have different amount of trips, and
> > different update rate. You may be using last_trip_level as reference for a different tz. Meaning,
> > you would be screwing the stat buffers silently.
> >
> > If that is the case, I suggest you to move this to your stat structure.
>
> Agree. This looks a clear problem. Surely i will move last_trip_level
> inside structure tz.
>
> >
> >> +
> >> + 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) {
> >> + last_trip_level = -1;
> >> + return;
> >> + }
> >> +
> >> + cur_interval = tz->stat[max_index].interval_ptr;
> >> + cool_temp = tz->last_temperature - cur_temp;
> >> +
> >> + if (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;
> >> + last_trip_level = max_index;
> >> + } else {
> >> + tz->stat[max_index].cool_temp[cur_interval] += cool_temp;
> >
> > Why do you need to sum up here? Wouldn't this break your statistics? (I may completely missed your idea for last_trip_level).
> This will be summed up because after applying cooling action there is
> some cooling happening but not enough to change the trip level. So
> unless there is cooling enough to change the trip level I keep summing
> the temperature.
OK. You may want to add this explanation as a comment in the code.
> >
> >> + }
> >> +}
> >> +
> >> +static int calculate_cooling_temp_avg(struct thermal_zone_device *tz, int trip,
> >> + int *avg_cool)
> >> +{
> >> + int result = 0, count = 0, used_data = 0;
> >> +
> >> + if (trip > THERMAL_MAX_TRIPS)
> >
> > Shouldn't this be checked against tz->trips ? At least you allocate your *stat based on it.
> Correct.
> >
> >> + return -EINVAL;
> >> +
> >> + *avg_cool = 0;
> >> + for (count = 0; count < INTERVAL_HISTORY; count++) {
> >> + if (tz->stat[trip].cool_temp[count] > 0) {
> >> + *avg_cool += tz->stat[trip].cool_temp[count];
> >> + used_data++;
> >> + }
> >> + }
> >> + if (used_data > 1)
> >> + *avg_cool = (*avg_cool)/used_data;
> >
> > IIRC, the preferred operator style is (*avg_cool) / used_data
> OK.
> >
> >> + return result;
> >
> > result is used only to hold a 0 here?
> Ok This variable is not needed.
> >
> >> +}
> >>
> >> /* sys I/F for thermal zone */
> >>
> >> @@ -493,6 +551,26 @@ temp_crit_show(struct device *dev, struct device_attribute *attr,
> >> return sprintf(buf, "%ld\n", temperature);
> >> }
> >>
> >> +static ssize_t
> >> +thermal_cooling_trip_stats_show(struct device *dev,
> >> + struct device_attribute *attr,
> >> + char *buf)
> >> +{
> >> + struct thermal_zone_device *tz = to_thermal_zone(dev);
> >> + int avg_cool = 0, result, trip_index;
> >> + ssize_t len = 0;
> >> +
> >> + for (trip_index = 0; trip_index < tz->trips; trip_index++) {
> >> + result = calculate_cooling_temp_avg(tz,
> >> + trip_index, &avg_cool);
> >> + if (!result)
> >> + len += sprintf(buf + len, "%d %d\n",
> >> + trip_index, avg_cool);
> >> + }
> >> + return len;
> >> +}
> >> +static DEVICE_ATTR(trip_stats, 0444,
> >> + thermal_cooling_trip_stats_show, NULL);
> >>
> >> static struct thermal_hwmon_device *
> >> thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
> >> @@ -1049,6 +1127,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);
> >> @@ -1181,6 +1261,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);
> >
> > You never free this memory here.
> yes because memory allocated with devm_kzalloc is freed automatically
> when the device is freed.
OK. missed the devm_ on your code. My bad.
> >
> >> + if (!tz->stat)
> >> + goto unregister;
> >> +
> >> /* sys I/F */
> >> if (type) {
> >> result = device_create_file(&tz->device, &dev_attr_type);
> >> @@ -1207,6 +1294,12 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
> >> passive = 1;
> >> }
> >>
> >> + if (trips > 0) {
> >> + result = device_create_file(&tz->device, &dev_attr_trip_stats);
> >> + if (result)
> >> + goto unregister;
> >
> > The failing paths after your allocation point must consider freeing the memory you requested.
> >
> >> + }
> >> +
> >> if (!passive)
> >> result = device_create_file(&tz->device,
> >> &dev_attr_passive);
> >> @@ -1282,6 +1375,9 @@ 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 (tz->trips > 0)
> >> + device_remove_file(&tz->device, &dev_attr_trip_stats);
> >> +
> >
> > Amit,
> >
> > I think here it would be a good place to free the memory you allocated for *stat
> >
> >> thermal_remove_hwmon_sysfs(tz);
> >> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> >> idr_destroy(&tz->idr);
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index 47b4a27..47504c7 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -72,6 +72,13 @@ 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;
> >> +};
> >> +
> >> struct thermal_cooling_device {
> >> int id;
> >> char type[THERMAL_NAME_LENGTH];
> >> @@ -102,6 +109,7 @@ struct thermal_zone_device {
> >> struct list_head cooling_devices;
> >> struct idr idr;
> >> struct mutex lock; /* protect cooling devices list */
> >> + struct thermal_cooling_stats *stat;
> >> struct list_head node;
> >> struct delayed_work poll_queue;
> >> };
> >> --
> >> 1.7.1
> >>
> >> _______________________________________________
> >> linux-pm mailing list
> >> linux-pm@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Amit Kachhap @ 2012-02-07 18:26 UTC (permalink / raw)
To: Greg KH; +Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm, rob.lee
In-Reply-To: <20120207001818.GB30840@kroah.com>
On 6 February 2012 16:18, Greg KH <greg@kroah.com> wrote:
> On Mon, Feb 06, 2012 at 03:16:17PM -0800, Amit Kachhap wrote:
>> On 6 February 2012 09:03, Greg KH <greg@kroah.com> wrote:
>> > On Mon, Feb 06, 2012 at 05:56:58PM +0100, Pavel Machek wrote:
>> >> Hi!
>> >>
>> >> > Add a sysfs 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.
>> >>
>> >> /sys fs should be one-value-per-file, talk to gregkh.
>> >
>> > That's correct.
>> >
>> > Why not use debugfs for this instead?
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>> Thanks Greg/Pavel for looking into the patch.
>> Basically I checked the places where single sysfs entry is showing
>> more then 1 output. And
>> 1) /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state
>> 2) /sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table
>> are the places where the output is more than 1 value.
>
> Yes, those are two known-bad files, whose files will change soon and
> move to debugfs.
>
> Just because you found 2, instead of the thousands of other properly
> formatted files, does not mean you are allowed to create something like
> this.
>
>> Anyway I can enclose this sysfs inside CONFIG_THERMAL_COOLING_STATS macro.
>
> No, you can not create it at all in sysfs, if you really need such a
> large single file, use debugfs instead, that is what it is there for.
>
> Again, one-value-per-file is the sysfs rule, and has been for a decade
> now, please don't think this is a new thing.
>
> greg k-h
Ok I will follow one-value-per-file rule for sysfs file. Thanks
^ permalink raw reply
* Re: [linux-pm] [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation
From: Amit Kachhap @ 2012-02-07 18:21 UTC (permalink / raw)
To: eduardo.valentin; +Cc: linux-pm, linaro-dev, patches, linux-kernel, linux-acpi
In-Reply-To: <20120207082559.GA5558@besouro>
Hi eduardo,
Again thanks for the review.
On 7 February 2012 00:25, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> Hello Amit,
>
> On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling and cpuhotplugg currently.
>> 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.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>> Documentation/thermal/cpu-cooling-api.txt | 52 +++++
>> drivers/thermal/Kconfig | 11 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/cpu_cooling.c | 302 +++++++++++++++++++++++++++++
>> include/linux/cpu_cooling.h | 45 +++++
>> 5 files changed, 411 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..d30b4f2
>> --- /dev/null
>> +++ b/Documentation/thermal/cpu-cooling-api.txt
>> @@ -0,0 +1,52 @@
>> +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 types 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".
>> +
>> + tab_ptr: The table containing the percentage of frequency to be clipped for
>> + each cooling state.
>> + .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
>> + cpu.
>> + .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(void)
>> +
>> + This interface function unregisters the "thermal-cpufreq" cooling device.
>> +
>> +
>> +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
>> + "thermal-cpuhotplug".
>> +
>> + mask_val: all the allowed cpu's which can be hotplugged out.
>> +
>> +1.1.2 void cpuhotplug_cooling_unregister(void)
>> +
>> + This interface function unregisters the "thermal-cpuhotplug" cooling device.
>> 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..cdd148c
>> --- /dev/null
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -0,0 +1,302 @@
>> +/*
>> + * 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 {
>> + 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;
>> +};
>> +
>> +static struct cpufreq_cooling_device *cpufreq_device;
>> +
>> +/*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(int cooling_state)
>> +{
>
> Why do you need cooling_state to be signed? You used it always against
> unsigned values. Besides, the thermal api imposes unsigned long.
Yes, unsigned long is a better way.
>
>> + 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;
>> +
>> + 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 cpu = policy->cpu, th_pctg = 0, level;
>> +
>> + if (event != CPUFREQ_ADJUST)
>> + return 0;
>> +
>> + level = cpufreq_device->cpufreq_state;
>> +
>> + if (level > 0) {
>> + th_table =
>> + &(cpufreq_device->tab_ptr[level - 1]);
>> + th_pctg = th_table->freq_clip_pctg[cpu];
>> + }
>> +
>> + max_freq =
>> + (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
>
>
> Might be interesting to extend this a bit. You could have the
> above as default policy setup, but you could also allow people
> to change this behavior. For instance, adding some callback option
> while registering the cooling device (or a notifier), which would
> determine the max_freq for the generic layer. The mechanism to
> deploy max_freq into the system must be generic enough with cpufreq
> APIs, but the decision to which max_freq to use, could be specific.
> Don't you think?
I agree that there will always some constraints on the system to cap
the max frequency. But these thermal clippings is called for rare
scenarios. Anyway, Your idea is worth considering. I will check if
cpufreq has any generic way of achieving this.
>
>> +
>> + 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)
>> +{
>> + *state = cpufreq_device->tab_size;
>> + return 0;
>> +}
>> +
>> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long *state)
>> +{
>> + *state = cpufreq_device->cpufreq_state;
>> + return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
>> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long state)
>> +{
>> + cpufreq_apply_cooling(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;
>> +
>> + if (tab_ptr == NULL || tab_size == 0)
>> + return ERR_PTR(-EINVAL);
>> +
>> + cpufreq_device =
>> + kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
>> +
>> + if (!cpufreq_device)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
>> + &cpufreq_cooling_ops);
>> + if (!cool_dev) {
>> + kfree(cpufreq_device);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + cpufreq_device->tab_ptr = tab_ptr;
>> + cpufreq_device->tab_size = tab_size;
>> + cpufreq_device->cool_dev = cool_dev;
>> + cpufreq_device->allowed_cpus = mask_val;
>> +
>> + cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
>> + CPUFREQ_POLICY_NOTIFIER);
>> + return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +
>> +void cpufreq_cooling_unregister(void)
>> +{
>> + cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
>> + CPUFREQ_POLICY_NOTIFIER);
>> + thermal_cooling_device_unregister(cpufreq_device->cool_dev);
>> + kfree(cpufreq_device);
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#else /*!CONFIG_CPU_FREQ*/
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> + struct freq_pctg_table *tab_ptr, unsigned int tab_size)
>> +{
>> + return NULL;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +void cpufreq_cooling_unregister(void)
>> +{
>> + return;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#endif /*CONFIG_CPU_FREQ*/
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +
>> +struct hotplug_cooling_device {
>> + struct thermal_cooling_device *cool_dev;
>> + unsigned int hotplug_state;
>> + const struct cpumask *allowed_cpus;
>> +};
>> +static struct hotplug_cooling_device *hotplug_device;
>> +
>> +/*
>> + * cpu hotplug cooling device callback functions
>> + */
>> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
>> + unsigned long *state)
>> +{
>> + *state = 1;
>> + return 0;
>> +}
>> +
>> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long *state)
>> +{
>> + /*This cooling device may be of type ACTIVE, so state field
>> + can be 0 or 1*/
>> + *state = hotplug_device->hotplug_state;
>> + return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
>> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long state)
>> +{
>> + int cpuid, this_cpu = smp_processor_id();
>> +
>> + if (hotplug_device->hotplug_state == state)
>> + return 0;
>> +
>> + /*This cooling device may be of type ACTIVE, so state field
>> + can be 0 or 1*/
>
> /*
> * Small thing. Check the style for multi line comments.
> * This is also an example.
> */
Ok.
>
>> + if (state == 1) {
>> + for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
>> + if (cpu_online(cpuid) && (cpuid != this_cpu))
>> + cpu_down(cpuid);
>> + }
>> + } else if (state == 0) {
>> + for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
>> + if (!cpu_online(cpuid) && (cpuid != this_cpu))
>> + cpu_up(cpuid);
>> + }
>> + } else
>> + return -EINVAL;
>> +
>> + hotplug_device->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;
>> +
>> + hotplug_device =
>> + kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
>> +
>> + if (!hotplug_device)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
>> + &cpuhotplug_cooling_ops);
>> + if (!cool_dev) {
>> + kfree(hotplug_device);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + hotplug_device->cool_dev = cool_dev;
>> + hotplug_device->hotplug_state = 0;
>> + hotplug_device->allowed_cpus = mask_val;
>> +
>> + return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
>> +
>> +void cpuhotplug_cooling_unregister(void)
>> +{
>> + thermal_cooling_device_unregister(hotplug_device->cool_dev);
>> + kfree(hotplug_device);
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
>> +#else /*!CONFIG_HOTPLUG_CPU*/
>> +struct thermal_cooling_device *cpuhotplug_cooling_register(
>> + const struct cpumask *mask_val)
>> +{
>> + return NULL;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
>> +void cpuhotplug_cooling_unregister(void)
>> +{
>> + return;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
>> +#endif /*CONFIG_HOTPLUG_CPU*/
>> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
>> new file mode 100644
>> index 0000000..0c57375
>> --- /dev/null
>> +++ b/include/linux/cpu_cooling.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + * 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[NR_CPUS];
>> + unsigned int polling_interval;
>> +};
>> +
>> +extern struct thermal_cooling_device *cpufreq_cooling_register(
>> + struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> + const struct cpumask *mask_val);
>
> I suppose your original idea was to have this function called only once right?
> I'd suggest to add some documentation on this header file to specify this.
>
> Besides, the fact that you receive a cpumask may suggest you could register
> different cooling device for subsets of your system cpus.
>
> In any case, if you call this function more than once, you may end with memory leaks
> and the last call will be the one acting in the system.
>
yes these api's are meant to be called once. I am working on a new
version where all these function will be multi-instance and hence
different cpus can support different frequency clipping.
>
>
>> +
>> +extern void cpufreq_cooling_unregister(void);
>> +
>> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
>> + const struct cpumask *mask_val);
>> +
>> +extern void cpuhotplug_cooling_unregister(void);
>> +
>> +#endif /* __CPU_COOLING_H__ */
>
> Another thing, I have the impression that solving the config selection in the
> header file instead of in .c files is cleaner. If you want to solve the config
> selection in .c files, better to do so with Makefiles.
>
> If you keep the ifdefery ( THERMAL vs. HOTPLUG_CPU vs. CPU_FREQ ) here in the
> header file, it is easier to read.
Ok will move them in .h
>
>> --
>> 1.7.1
>>
>> _______________________________________________
>> linux-pm mailing list
>> linux-pm@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Amit Kachhap @ 2012-02-07 17:52 UTC (permalink / raw)
To: eduardo.valentin
Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm, rob.lee
In-Reply-To: <20120207070917.GA2616@besouro>
Hi eduardo,
Thanks for the detail review.
On 6 February 2012 23:09, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> Hello Amit,
>
> some comments embedded.
>
> On Wed, Jan 18, 2012 at 02:51:07PM +0530, Amit Daniel Kachhap wrote:
>> Add a sysfs 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.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>> Documentation/thermal/sysfs-api.txt | 10 ++++
>> drivers/thermal/thermal_sys.c | 96 +++++++++++++++++++++++++++++++++++
>> include/linux/thermal.h | 8 +++
>> 3 files changed, 114 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>> index b61e46f..1db9a9d 100644
>> --- a/Documentation/thermal/sysfs-api.txt
>> +++ b/Documentation/thermal/sysfs-api.txt
>> @@ -209,6 +209,13 @@ passive
>> Valid values: 0 (disabled) or greater than 1000
>> RW, Optional
>>
>> +trip_stats
>> + This attribute presents the effective cooling generated from all the
>> + cooling devices attached to a trip point. The output is a pair of value
>> + for each trip point. Each pair represents
>> + (trip index, cooling temperature difference in millidegree Celsius)
>> + RO, Optional
>> +
>> *****************************
>> * Cooling device attributes *
>> *****************************
>> @@ -261,6 +268,9 @@ method, the sys I/F structure will be built like this:
>> |---cdev0_trip_point: 1 /* cdev0 can be used for passive */
>> |---cdev1: --->/sys/class/thermal/cooling_device3
>> |---cdev1_trip_point: 2 /* cdev1 can be used for active[0]*/
>> + |---trip_stats 0 0
>> + 1 8000 /*trip 1 causes 8 degree Celsius drop*/
>> + 2 3000 /*trip 2 causes 3 degree Celsius drop*/
>>
>> |cooling_device0:
>> |---type: Processor
>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> index dd9a574..47e7b6e 100644
>> --- a/drivers/thermal/thermal_sys.c
>> +++ b/drivers/thermal/thermal_sys.c
>> @@ -92,6 +92,64 @@ static void release_idr(struct idr *idr, struct mutex *lock, int id)
>> if (lock)
>> mutex_unlock(lock);
>> }
>> +static void update_cooling_stats(struct thermal_zone_device *tz, long cur_temp)
>> +{
>> + int count, max_index, cur_interval;
>> + long trip_temp, max_temp = 0, cool_temp;
>> + static int last_trip_level = -1;
>
> I got confused here. Are you sure using a static variable here is safe? I suppose this function
> is called for any thermal_zone_device, which in turns, may have different amount of trips, and
> different update rate. You may be using last_trip_level as reference for a different tz. Meaning,
> you would be screwing the stat buffers silently.
>
> If that is the case, I suggest you to move this to your stat structure.
Agree. This looks a clear problem. Surely i will move last_trip_level
inside structure tz.
>
>> +
>> + 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) {
>> + last_trip_level = -1;
>> + return;
>> + }
>> +
>> + cur_interval = tz->stat[max_index].interval_ptr;
>> + cool_temp = tz->last_temperature - cur_temp;
>> +
>> + if (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;
>> + last_trip_level = max_index;
>> + } else {
>> + tz->stat[max_index].cool_temp[cur_interval] += cool_temp;
>
> Why do you need to sum up here? Wouldn't this break your statistics? (I may completely missed your idea for last_trip_level).
This will be summed up because after applying cooling action there is
some cooling happening but not enough to change the trip level. So
unless there is cooling enough to change the trip level I keep summing
the temperature.
>
>> + }
>> +}
>> +
>> +static int calculate_cooling_temp_avg(struct thermal_zone_device *tz, int trip,
>> + int *avg_cool)
>> +{
>> + int result = 0, count = 0, used_data = 0;
>> +
>> + if (trip > THERMAL_MAX_TRIPS)
>
> Shouldn't this be checked against tz->trips ? At least you allocate your *stat based on it.
Correct.
>
>> + return -EINVAL;
>> +
>> + *avg_cool = 0;
>> + for (count = 0; count < INTERVAL_HISTORY; count++) {
>> + if (tz->stat[trip].cool_temp[count] > 0) {
>> + *avg_cool += tz->stat[trip].cool_temp[count];
>> + used_data++;
>> + }
>> + }
>> + if (used_data > 1)
>> + *avg_cool = (*avg_cool)/used_data;
>
> IIRC, the preferred operator style is (*avg_cool) / used_data
OK.
>
>> + return result;
>
> result is used only to hold a 0 here?
Ok This variable is not needed.
>
>> +}
>>
>> /* sys I/F for thermal zone */
>>
>> @@ -493,6 +551,26 @@ temp_crit_show(struct device *dev, struct device_attribute *attr,
>> return sprintf(buf, "%ld\n", temperature);
>> }
>>
>> +static ssize_t
>> +thermal_cooling_trip_stats_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct thermal_zone_device *tz = to_thermal_zone(dev);
>> + int avg_cool = 0, result, trip_index;
>> + ssize_t len = 0;
>> +
>> + for (trip_index = 0; trip_index < tz->trips; trip_index++) {
>> + result = calculate_cooling_temp_avg(tz,
>> + trip_index, &avg_cool);
>> + if (!result)
>> + len += sprintf(buf + len, "%d %d\n",
>> + trip_index, avg_cool);
>> + }
>> + return len;
>> +}
>> +static DEVICE_ATTR(trip_stats, 0444,
>> + thermal_cooling_trip_stats_show, NULL);
>>
>> static struct thermal_hwmon_device *
>> thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
>> @@ -1049,6 +1127,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);
>> @@ -1181,6 +1261,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);
>
> You never free this memory here.
yes because memory allocated with devm_kzalloc is freed automatically
when the device is freed.
>
>> + if (!tz->stat)
>> + goto unregister;
>> +
>> /* sys I/F */
>> if (type) {
>> result = device_create_file(&tz->device, &dev_attr_type);
>> @@ -1207,6 +1294,12 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
>> passive = 1;
>> }
>>
>> + if (trips > 0) {
>> + result = device_create_file(&tz->device, &dev_attr_trip_stats);
>> + if (result)
>> + goto unregister;
>
> The failing paths after your allocation point must consider freeing the memory you requested.
>
>> + }
>> +
>> if (!passive)
>> result = device_create_file(&tz->device,
>> &dev_attr_passive);
>> @@ -1282,6 +1375,9 @@ 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 (tz->trips > 0)
>> + device_remove_file(&tz->device, &dev_attr_trip_stats);
>> +
>
> Amit,
>
> I think here it would be a good place to free the memory you allocated for *stat
>
>> thermal_remove_hwmon_sysfs(tz);
>> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>> idr_destroy(&tz->idr);
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 47b4a27..47504c7 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -72,6 +72,13 @@ 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;
>> +};
>> +
>> struct thermal_cooling_device {
>> int id;
>> char type[THERMAL_NAME_LENGTH];
>> @@ -102,6 +109,7 @@ struct thermal_zone_device {
>> struct list_head cooling_devices;
>> struct idr idr;
>> struct mutex lock; /* protect cooling devices list */
>> + struct thermal_cooling_stats *stat;
>> struct list_head node;
>> struct delayed_work poll_queue;
>> };
>> --
>> 1.7.1
>>
>> _______________________________________________
>> linux-pm mailing list
>> linux-pm@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Jean Pihet @ 2012-02-07 8:39 UTC (permalink / raw)
To: Rafael J. Wysocki, Venkatesh Pallipadi
Cc: Kevin Hilman, Linux PM mailing list, Pihet-XID, Jean,
linux-kernel, markgross
In-Reply-To: <201202062116.16744.rjw@sisk.pl>
On Mon, Feb 6, 2012 at 9:16 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, February 06, 2012, Jean Pihet wrote:
>> Rafael,
>>
>> On Mon, Feb 6, 2012 at 11:18 AM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
>> > Hi Rafael, Mark,
>> >
>> > On Sun, Feb 5, 2012 at 12:04 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> ...
>>
>> >> In kernel/power/Makefile:
>> >>
>> >> obj-$(CONFIG_PM) += main.o qos.o
>> >>
>> >> I guess that explains things. :-)
>> > Initially I thought we should have a way of disabling the feature on
>> > some (minimal) kernels and so thought CONFIG_PM was the option to use.
>> >
>> >> It's quite easy to make qos.o be independent of CONFIG_PM, in which case the
>> >> code added by Venki can be removed, so patches welcome (for 3.4, though).
>> > I am working on it, more to come soon.
>>
>> I have a couple of patches ready, to be applied on 3.3-rc1 (so without
>> Venki's patch applied).
>> The first one is on PM QoS, the second one on per-device PM QoS. Is
>> the latter needed?
>
> I'm not sure without looking. :-)
That makes sense ;p
Just sent out the patch set as '[PATCH 0/2] PM / QoS: unconditionally
build the feature'.
It has been compile tested only using the i386 defconfig, with and
without CONFIG_PM.
It requires some more testing on x86.
Venki, can you check it out if this fixes the initial problem?
>
> Thanks,
> Rafael
Regards,
Jean
^ permalink raw reply
* [PATCH 2/2] PM / QoS: unconditionally build the per-device constraints feature
From: Jean Pihet @ 2012-02-07 8:34 UTC (permalink / raw)
To: Linux PM mailing list, linux-kernel, Rafael J. Wysocki,
Mark Gross, Venkatesh Pallipadi
Cc: Jean Pihet
In-Reply-To: <1328603646-2714-1-git-send-email-j-pihet@ti.com>
The per-device PM QoS feature depends on CONFIG_PM which depends
on PM_SLEEP || PM_RUNTIME. This breaks CPU C-states with kernels
not having these CONFIGs.
This patch allows the feature in all cases.
Signed-off-by: Jean Pihet <j-pihet@ti.com>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Mark Gross <markgross@thegnar.org>
---
drivers/base/power/Makefile | 3 ++-
include/linux/pm_qos.h | 39 ---------------------------------------
2 files changed, 2 insertions(+), 40 deletions(-)
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index 2e58ebb..312eb65 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,4 +1,5 @@
-obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o
+obj-y += qos.o
+obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o
obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
obj-$(CONFIG_PM_RUNTIME) += runtime.o
obj-$(CONFIG_PM_TRACE_RTC) += trace.o
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 105be69..37b017a 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -77,7 +77,6 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_request_active(struct pm_qos_request *req);
s32 pm_qos_read_value(struct pm_qos_constraints *c);
-#ifdef CONFIG_PM
s32 __dev_pm_qos_read_value(struct device *dev);
s32 dev_pm_qos_read_value(struct device *dev);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
@@ -94,43 +93,5 @@ void dev_pm_qos_constraints_init(struct device *dev);
void dev_pm_qos_constraints_destroy(struct device *dev);
int dev_pm_qos_add_ancestor_request(struct device *dev,
struct dev_pm_qos_request *req, s32 value);
-#else
-static inline s32 __dev_pm_qos_read_value(struct device *dev)
- { return 0; }
-static inline s32 dev_pm_qos_read_value(struct device *dev)
- { return 0; }
-static inline int dev_pm_qos_add_request(struct device *dev,
- struct dev_pm_qos_request *req,
- s32 value)
- { return 0; }
-static inline int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
- s32 new_value)
- { return 0; }
-static inline int dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
- { return 0; }
-static inline int dev_pm_qos_add_notifier(struct device *dev,
- struct notifier_block *notifier)
- { return 0; }
-static inline int dev_pm_qos_remove_notifier(struct device *dev,
- struct notifier_block *notifier)
- { return 0; }
-static inline int dev_pm_qos_add_global_notifier(
- struct notifier_block *notifier)
- { return 0; }
-static inline int dev_pm_qos_remove_global_notifier(
- struct notifier_block *notifier)
- { return 0; }
-static inline void dev_pm_qos_constraints_init(struct device *dev)
-{
- dev->power.power_state = PMSG_ON;
-}
-static inline void dev_pm_qos_constraints_destroy(struct device *dev)
-{
- dev->power.power_state = PMSG_INVALID;
-}
-static inline int dev_pm_qos_add_ancestor_request(struct device *dev,
- struct dev_pm_qos_request *req, s32 value)
- { return 0; }
-#endif
#endif
--
1.7.5.4
^ permalink raw reply related
* [PATCH 1/2] PM / QoS: unconditionally build the feature
From: Jean Pihet @ 2012-02-07 8:34 UTC (permalink / raw)
To: Linux PM mailing list, linux-kernel, Rafael J. Wysocki,
Mark Gross, Venkatesh Pallipadi
Cc: Jean Pihet
In-Reply-To: <1328603646-2714-1-git-send-email-j-pihet@ti.com>
The PM QoS feature depends on CONFIG_PM which depends on
PM_SLEEP || PM_RUNTIME. This breaks CPU C-states with kernels
not having these CONFIGs.
This patch allows the feature in all cases.
Reported-by: Venkatesh Pallipadi <venki@google.com>
Signed-off-by: Jean Pihet <j-pihet@ti.com>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Mark Gross <markgross@thegnar.org>
---
include/linux/pm_qos.h | 29 +----------------------------
kernel/power/Makefile | 3 ++-
2 files changed, 3 insertions(+), 29 deletions(-)
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index e5bbcba..105be69 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -63,7 +63,6 @@ static inline int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
return req->dev != 0;
}
-#ifdef CONFIG_PM
int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
enum pm_qos_req_action action, int value);
void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
@@ -78,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_request_active(struct pm_qos_request *req);
s32 pm_qos_read_value(struct pm_qos_constraints *c);
+#ifdef CONFIG_PM
s32 __dev_pm_qos_read_value(struct device *dev);
s32 dev_pm_qos_read_value(struct device *dev);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
@@ -95,33 +95,6 @@ void dev_pm_qos_constraints_destroy(struct device *dev);
int dev_pm_qos_add_ancestor_request(struct device *dev,
struct dev_pm_qos_request *req, s32 value);
#else
-static inline int pm_qos_update_target(struct pm_qos_constraints *c,
- struct plist_node *node,
- enum pm_qos_req_action action,
- int value)
- { return 0; }
-static inline void pm_qos_add_request(struct pm_qos_request *req,
- int pm_qos_class, s32 value)
- { return; }
-static inline void pm_qos_update_request(struct pm_qos_request *req,
- s32 new_value)
- { return; }
-static inline void pm_qos_remove_request(struct pm_qos_request *req)
- { return; }
-
-static inline int pm_qos_request(int pm_qos_class)
- { return 0; }
-static inline int pm_qos_add_notifier(int pm_qos_class,
- struct notifier_block *notifier)
- { return 0; }
-static inline int pm_qos_remove_notifier(int pm_qos_class,
- struct notifier_block *notifier)
- { return 0; }
-static inline int pm_qos_request_active(struct pm_qos_request *req)
- { return 0; }
-static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
- { return 0; }
-
static inline s32 __dev_pm_qos_read_value(struct device *dev)
{ return 0; }
static inline s32 dev_pm_qos_read_value(struct device *dev)
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 07e0e28..66d808e 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -1,7 +1,8 @@
ccflags-$(CONFIG_PM_DEBUG) := -DDEBUG
-obj-$(CONFIG_PM) += main.o qos.o
+obj-y += qos.o
+obj-$(CONFIG_PM) += main.o
obj-$(CONFIG_VT_CONSOLE_SLEEP) += console.o
obj-$(CONFIG_FREEZER) += process.o
obj-$(CONFIG_SUSPEND) += suspend.o
--
1.7.5.4
^ permalink raw reply related
* [PATCH 0/2] PM / QoS: unconditionally build the feature
From: Jean Pihet @ 2012-02-07 8:34 UTC (permalink / raw)
To: Linux PM mailing list, linux-kernel, Rafael J. Wysocki,
Mark Gross, Venkatesh Pallipadi
Cc: Jean Pihet
The PM QoS features depend on CONFIG_PM which depends on
PM_SLEEP || PM_RUNTIME. This breaks CPU C-states with kernels
not having these CONFIGs.
Those patches allow the features in all cases.
Based on 3.3-rc1 from mainline.
Tested on OMAP3 Beagleboard, with CONFIG_PM.
Compile tested using the i386 defconfig, with and without CONFIG_PM.
Jean Pihet (2):
PM / QoS: unconditionally build the feature
PM / QoS: unconditionally build the per-device constraints feature
drivers/base/power/Makefile | 3 +-
include/linux/pm_qos.h | 66 -------------------------------------------
kernel/power/Makefile | 3 +-
3 files changed, 4 insertions(+), 68 deletions(-)
--
1.7.5.4
^ permalink raw reply
* Re: [linux-pm] [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation
From: Eduardo Valentin @ 2012-02-07 8:25 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-pm, linaro-dev, patches, linux-kernel, linux-acpi
In-Reply-To: <1323789196-4942-3-git-send-email-amit.kachhap@linaro.org>
Hello Amit,
On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling and cpuhotplugg currently.
> 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.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
> Documentation/thermal/cpu-cooling-api.txt | 52 +++++
> drivers/thermal/Kconfig | 11 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/cpu_cooling.c | 302 +++++++++++++++++++++++++++++
> include/linux/cpu_cooling.h | 45 +++++
> 5 files changed, 411 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..d30b4f2
> --- /dev/null
> +++ b/Documentation/thermal/cpu-cooling-api.txt
> @@ -0,0 +1,52 @@
> +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 types 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".
> +
> + tab_ptr: The table containing the percentage of frequency to be clipped for
> + each cooling state.
> + .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
> + cpu.
> + .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(void)
> +
> + This interface function unregisters the "thermal-cpufreq" cooling device.
> +
> +
> +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
> + "thermal-cpuhotplug".
> +
> + mask_val: all the allowed cpu's which can be hotplugged out.
> +
> +1.1.2 void cpuhotplug_cooling_unregister(void)
> +
> + This interface function unregisters the "thermal-cpuhotplug" cooling device.
> 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..cdd148c
> --- /dev/null
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -0,0 +1,302 @@
> +/*
> + * 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 {
> + 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;
> +};
> +
> +static struct cpufreq_cooling_device *cpufreq_device;
> +
> +/*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(int cooling_state)
> +{
Why do you need cooling_state to be signed? You used it always against
unsigned values. Besides, the thermal api imposes unsigned long.
> + 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;
> +
> + 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 cpu = policy->cpu, th_pctg = 0, level;
> +
> + if (event != CPUFREQ_ADJUST)
> + return 0;
> +
> + level = cpufreq_device->cpufreq_state;
> +
> + if (level > 0) {
> + th_table =
> + &(cpufreq_device->tab_ptr[level - 1]);
> + th_pctg = th_table->freq_clip_pctg[cpu];
> + }
> +
> + max_freq =
> + (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
Might be interesting to extend this a bit. You could have the
above as default policy setup, but you could also allow people
to change this behavior. For instance, adding some callback option
while registering the cooling device (or a notifier), which would
determine the max_freq for the generic layer. The mechanism to
deploy max_freq into the system must be generic enough with cpufreq
APIs, but the decision to which max_freq to use, could be specific.
Don't you think?
> +
> + 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)
> +{
> + *state = cpufreq_device->tab_size;
> + return 0;
> +}
> +
> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + *state = cpufreq_device->cpufreq_state;
> + return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long state)
> +{
> + cpufreq_apply_cooling(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;
> +
> + if (tab_ptr == NULL || tab_size == 0)
> + return ERR_PTR(-EINVAL);
> +
> + cpufreq_device =
> + kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
> +
> + if (!cpufreq_device)
> + return ERR_PTR(-ENOMEM);
> +
> + cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
> + &cpufreq_cooling_ops);
> + if (!cool_dev) {
> + kfree(cpufreq_device);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + cpufreq_device->tab_ptr = tab_ptr;
> + cpufreq_device->tab_size = tab_size;
> + cpufreq_device->cool_dev = cool_dev;
> + cpufreq_device->allowed_cpus = mask_val;
> +
> + cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> + CPUFREQ_POLICY_NOTIFIER);
> + return cool_dev;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +
> +void cpufreq_cooling_unregister(void)
> +{
> + cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> + CPUFREQ_POLICY_NOTIFIER);
> + thermal_cooling_device_unregister(cpufreq_device->cool_dev);
> + kfree(cpufreq_device);
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#else /*!CONFIG_CPU_FREQ*/
> +struct thermal_cooling_device *cpufreq_cooling_register(
> + struct freq_pctg_table *tab_ptr, unsigned int tab_size)
> +{
> + return NULL;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +void cpufreq_cooling_unregister(void)
> +{
> + return;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#endif /*CONFIG_CPU_FREQ*/
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +
> +struct hotplug_cooling_device {
> + struct thermal_cooling_device *cool_dev;
> + unsigned int hotplug_state;
> + const struct cpumask *allowed_cpus;
> +};
> +static struct hotplug_cooling_device *hotplug_device;
> +
> +/*
> + * cpu hotplug cooling device callback functions
> + */
> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + *state = 1;
> + return 0;
> +}
> +
> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + /*This cooling device may be of type ACTIVE, so state field
> + can be 0 or 1*/
> + *state = hotplug_device->hotplug_state;
> + return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long state)
> +{
> + int cpuid, this_cpu = smp_processor_id();
> +
> + if (hotplug_device->hotplug_state == state)
> + return 0;
> +
> + /*This cooling device may be of type ACTIVE, so state field
> + can be 0 or 1*/
/*
* Small thing. Check the style for multi line comments.
* This is also an example.
*/
> + if (state == 1) {
> + for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> + if (cpu_online(cpuid) && (cpuid != this_cpu))
> + cpu_down(cpuid);
> + }
> + } else if (state == 0) {
> + for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> + if (!cpu_online(cpuid) && (cpuid != this_cpu))
> + cpu_up(cpuid);
> + }
> + } else
> + return -EINVAL;
> +
> + hotplug_device->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;
> +
> + hotplug_device =
> + kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
> +
> + if (!hotplug_device)
> + return ERR_PTR(-ENOMEM);
> +
> + cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
> + &cpuhotplug_cooling_ops);
> + if (!cool_dev) {
> + kfree(hotplug_device);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + hotplug_device->cool_dev = cool_dev;
> + hotplug_device->hotplug_state = 0;
> + hotplug_device->allowed_cpus = mask_val;
> +
> + return cool_dev;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> +
> +void cpuhotplug_cooling_unregister(void)
> +{
> + thermal_cooling_device_unregister(hotplug_device->cool_dev);
> + kfree(hotplug_device);
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> +#else /*!CONFIG_HOTPLUG_CPU*/
> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> + const struct cpumask *mask_val)
> +{
> + return NULL;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> +void cpuhotplug_cooling_unregister(void)
> +{
> + return;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> +#endif /*CONFIG_HOTPLUG_CPU*/
> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> new file mode 100644
> index 0000000..0c57375
> --- /dev/null
> +++ b/include/linux/cpu_cooling.h
> @@ -0,0 +1,45 @@
> +/*
> + * 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[NR_CPUS];
> + unsigned int polling_interval;
> +};
> +
> +extern struct thermal_cooling_device *cpufreq_cooling_register(
> + struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> + const struct cpumask *mask_val);
I suppose your original idea was to have this function called only once right?
I'd suggest to add some documentation on this header file to specify this.
Besides, the fact that you receive a cpumask may suggest you could register
different cooling device for subsets of your system cpus.
In any case, if you call this function more than once, you may end with memory leaks
and the last call will be the one acting in the system.
> +
> +extern void cpufreq_cooling_unregister(void);
> +
> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
> + const struct cpumask *mask_val);
> +
> +extern void cpuhotplug_cooling_unregister(void);
> +
> +#endif /* __CPU_COOLING_H__ */
Another thing, I have the impression that solving the config selection in the
header file instead of in .c files is cleaner. If you want to solve the config
selection in .c files, better to do so with Makefiles.
If you keep the ifdefery ( THERMAL vs. HOTPLUG_CPU vs. CPU_FREQ ) here in the
header file, it is easier to read.
> --
> 1.7.1
>
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [linux-pm] [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Eduardo Valentin @ 2012-02-07 7:09 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-pm, linaro-dev, patches, linux-kernel, linux-acpi, rob.lee
In-Reply-To: <1326878467-17766-1-git-send-email-amit.kachhap@linaro.org>
Hello Amit,
some comments embedded.
On Wed, Jan 18, 2012 at 02:51:07PM +0530, Amit Daniel Kachhap wrote:
> Add a sysfs 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.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
> Documentation/thermal/sysfs-api.txt | 10 ++++
> drivers/thermal/thermal_sys.c | 96 +++++++++++++++++++++++++++++++++++
> include/linux/thermal.h | 8 +++
> 3 files changed, 114 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index b61e46f..1db9a9d 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -209,6 +209,13 @@ passive
> Valid values: 0 (disabled) or greater than 1000
> RW, Optional
>
> +trip_stats
> + This attribute presents the effective cooling generated from all the
> + cooling devices attached to a trip point. The output is a pair of value
> + for each trip point. Each pair represents
> + (trip index, cooling temperature difference in millidegree Celsius)
> + RO, Optional
> +
> *****************************
> * Cooling device attributes *
> *****************************
> @@ -261,6 +268,9 @@ method, the sys I/F structure will be built like this:
> |---cdev0_trip_point: 1 /* cdev0 can be used for passive */
> |---cdev1: --->/sys/class/thermal/cooling_device3
> |---cdev1_trip_point: 2 /* cdev1 can be used for active[0]*/
> + |---trip_stats 0 0
> + 1 8000 /*trip 1 causes 8 degree Celsius drop*/
> + 2 3000 /*trip 2 causes 3 degree Celsius drop*/
>
> |cooling_device0:
> |---type: Processor
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index dd9a574..47e7b6e 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -92,6 +92,64 @@ static void release_idr(struct idr *idr, struct mutex *lock, int id)
> if (lock)
> mutex_unlock(lock);
> }
> +static void update_cooling_stats(struct thermal_zone_device *tz, long cur_temp)
> +{
> + int count, max_index, cur_interval;
> + long trip_temp, max_temp = 0, cool_temp;
> + static int last_trip_level = -1;
I got confused here. Are you sure using a static variable here is safe? I suppose this function
is called for any thermal_zone_device, which in turns, may have different amount of trips, and
different update rate. You may be using last_trip_level as reference for a different tz. Meaning,
you would be screwing the stat buffers silently.
If that is the case, I suggest you to move this to your stat structure.
> +
> + 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) {
> + last_trip_level = -1;
> + return;
> + }
> +
> + cur_interval = tz->stat[max_index].interval_ptr;
> + cool_temp = tz->last_temperature - cur_temp;
> +
> + if (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;
> + last_trip_level = max_index;
> + } else {
> + tz->stat[max_index].cool_temp[cur_interval] += cool_temp;
Why do you need to sum up here? Wouldn't this break your statistics? (I may completely missed your idea for last_trip_level).
> + }
> +}
> +
> +static int calculate_cooling_temp_avg(struct thermal_zone_device *tz, int trip,
> + int *avg_cool)
> +{
> + int result = 0, count = 0, used_data = 0;
> +
> + if (trip > THERMAL_MAX_TRIPS)
Shouldn't this be checked against tz->trips ? At least you allocate your *stat based on it.
> + return -EINVAL;
> +
> + *avg_cool = 0;
> + for (count = 0; count < INTERVAL_HISTORY; count++) {
> + if (tz->stat[trip].cool_temp[count] > 0) {
> + *avg_cool += tz->stat[trip].cool_temp[count];
> + used_data++;
> + }
> + }
> + if (used_data > 1)
> + *avg_cool = (*avg_cool)/used_data;
IIRC, the preferred operator style is (*avg_cool) / used_data
> + return result;
result is used only to hold a 0 here?
> +}
>
> /* sys I/F for thermal zone */
>
> @@ -493,6 +551,26 @@ temp_crit_show(struct device *dev, struct device_attribute *attr,
> return sprintf(buf, "%ld\n", temperature);
> }
>
> +static ssize_t
> +thermal_cooling_trip_stats_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct thermal_zone_device *tz = to_thermal_zone(dev);
> + int avg_cool = 0, result, trip_index;
> + ssize_t len = 0;
> +
> + for (trip_index = 0; trip_index < tz->trips; trip_index++) {
> + result = calculate_cooling_temp_avg(tz,
> + trip_index, &avg_cool);
> + if (!result)
> + len += sprintf(buf + len, "%d %d\n",
> + trip_index, avg_cool);
> + }
> + return len;
> +}
> +static DEVICE_ATTR(trip_stats, 0444,
> + thermal_cooling_trip_stats_show, NULL);
>
> static struct thermal_hwmon_device *
> thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
> @@ -1049,6 +1127,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);
> @@ -1181,6 +1261,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);
You never free this memory here.
> + if (!tz->stat)
> + goto unregister;
> +
> /* sys I/F */
> if (type) {
> result = device_create_file(&tz->device, &dev_attr_type);
> @@ -1207,6 +1294,12 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
> passive = 1;
> }
>
> + if (trips > 0) {
> + result = device_create_file(&tz->device, &dev_attr_trip_stats);
> + if (result)
> + goto unregister;
The failing paths after your allocation point must consider freeing the memory you requested.
> + }
> +
> if (!passive)
> result = device_create_file(&tz->device,
> &dev_attr_passive);
> @@ -1282,6 +1375,9 @@ 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 (tz->trips > 0)
> + device_remove_file(&tz->device, &dev_attr_trip_stats);
> +
Amit,
I think here it would be a good place to free the memory you allocated for *stat
> thermal_remove_hwmon_sysfs(tz);
> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> idr_destroy(&tz->idr);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 47b4a27..47504c7 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -72,6 +72,13 @@ 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;
> +};
> +
> struct thermal_cooling_device {
> int id;
> char type[THERMAL_NAME_LENGTH];
> @@ -102,6 +109,7 @@ struct thermal_zone_device {
> struct list_head cooling_devices;
> struct idr idr;
> struct mutex lock; /* protect cooling devices list */
> + struct thermal_cooling_stats *stat;
> struct list_head node;
> struct delayed_work poll_queue;
> };
> --
> 1.7.1
>
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: Query: When will drv->poweroff() called
From: Viresh Kumar @ 2012-02-07 4:19 UTC (permalink / raw)
To: Rafael J. Wysocki, amit.kucheria
Cc: pavel@ucw.cz, len.brown@intel.com,
linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
Shiraz HASHIM, Armando VISCONTI, Vipin KUMAR, Deepak SIKRI,
Vipul Kumar SAMAR, Amit VIRDI, Rajeev KUMAR, Pratyush ANAND,
Mirko GARDI, Vincenzo FRASCINO, Bhupesh SHARMA,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <201202061307.40628.rjw@sisk.pl>
Thanks Rafael.
On 2/6/2012 5:37 PM, Rafael J. Wysocki wrote:
> On Monday, February 06, 2012, Viresh Kumar wrote:
>> >
>> > Hi Guys,
>> >
>> > Sorry for asking this silly question, but i couldn't locate much
>> > help for it in documentation, so asking it.
>> >
>> > We were testing hibernation for SPEAr13xx SoC family, based on
>> > ARM Cortex a9.
>> >
>> > I observed that poweroff() callback of individual drivers are not
>> > getting called at all, while we test hibernate.
> They should be called in the last phase of hibernation, after the image
> has been created and the system is going for "power off" (hance the
> callback name).
>
>> > I tried to go through the code to see what happened. It looked like
>> > there should be call to hibernation_set_ops() for platforms that are
>> > willing to get a call to poweroff() for their drivers.
> That's correct. The ->poweroff() callbacks are only executed if
> hibernation_mode is equal to HIBERNATION_PLATFORM, which is not the
> default.
>
>> > Otherwise shutdown of the busses gets called, which is a completely
>> > different path.
>> >
>> > There are many drivers today, that are registering poweroff() from dev_pm_ops
>> > but are not doing bus specific shutdown stuff.
>> >
>> > Even i tried to look for hibernation_set_ops() in kernel, and only acpi
>> > code is calling it.
> That's correct too.
>
>> > I didn't understood how other ARM Sub-Arch's are handling this.
> Well, they are supposed to call hibernation_set_ops() and set the operations
> appropriately. Those operations may be empty routines if they don't need
> to do anything, but they have to be defined.
Hi Amit,
Can you give some input on why none of the ARM platforms are calling
this routine currently.
--
viresh
^ permalink raw reply
* Re: [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Greg KH @ 2012-02-07 0:18 UTC (permalink / raw)
To: Amit Kachhap
Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm, rob.lee
In-Reply-To: <CAK44p22FWxiXyBA1zbiuO0cbDScTSMRSV9pFDDMxXsc43ebGpw@mail.gmail.com>
On Mon, Feb 06, 2012 at 03:16:17PM -0800, Amit Kachhap wrote:
> On 6 February 2012 09:03, Greg KH <greg@kroah.com> wrote:
> > On Mon, Feb 06, 2012 at 05:56:58PM +0100, Pavel Machek wrote:
> >> Hi!
> >>
> >> > Add a sysfs 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.
> >>
> >> /sys fs should be one-value-per-file, talk to gregkh.
> >
> > That's correct.
> >
> > Why not use debugfs for this instead?
> >
> > thanks,
> >
> > greg k-h
>
> Thanks Greg/Pavel for looking into the patch.
> Basically I checked the places where single sysfs entry is showing
> more then 1 output. And
> 1) /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state
> 2) /sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table
> are the places where the output is more than 1 value.
Yes, those are two known-bad files, whose files will change soon and
move to debugfs.
Just because you found 2, instead of the thousands of other properly
formatted files, does not mean you are allowed to create something like
this.
> Anyway I can enclose this sysfs inside CONFIG_THERMAL_COOLING_STATS macro.
No, you can not create it at all in sysfs, if you really need such a
large single file, use debugfs instead, that is what it is there for.
Again, one-value-per-file is the sysfs rule, and has been for a decade
now, please don't think this is a new thing.
greg k-h
^ permalink raw reply
* Re: [linux-pm] [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Amit Kachhap @ 2012-02-06 23:16 UTC (permalink / raw)
To: Greg KH
Cc: Pavel Machek, linux-pm, linaro-dev, patches, linux-kernel,
linux-acpi, rob.lee
In-Reply-To: <20120206170348.GA19749@kroah.com>
On 6 February 2012 09:03, Greg KH <greg@kroah.com> wrote:
> On Mon, Feb 06, 2012 at 05:56:58PM +0100, Pavel Machek wrote:
>> Hi!
>>
>> > Add a sysfs 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.
>>
>> /sys fs should be one-value-per-file, talk to gregkh.
>
> That's correct.
>
> Why not use debugfs for this instead?
>
> thanks,
>
> greg k-h
Thanks Greg/Pavel for looking into the patch.
Basically I checked the places where single sysfs entry is showing
more then 1 output. And
1) /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state
2) /sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table
are the places where the output is more than 1 value.
Anyway I can enclose this sysfs inside CONFIG_THERMAL_COOLING_STATS macro.
Thanks,
Amit Daniel
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Rafael J. Wysocki @ 2012-02-06 20:16 UTC (permalink / raw)
To: Jean Pihet
Cc: Kevin Hilman, Venkatesh Pallipadi, markgross, linux-kernel,
Linux PM mailing list, Pihet-XID, Jean
In-Reply-To: <CAORVsuW7mBCZdGqScX5jjaYDhijZH_H7vDKkJ4v6WWHn9rUsGw@mail.gmail.com>
On Monday, February 06, 2012, Jean Pihet wrote:
> Rafael,
>
> On Mon, Feb 6, 2012 at 11:18 AM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
> > Hi Rafael, Mark,
> >
> > On Sun, Feb 5, 2012 at 12:04 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> ...
>
> >> In kernel/power/Makefile:
> >>
> >> obj-$(CONFIG_PM) += main.o qos.o
> >>
> >> I guess that explains things. :-)
> > Initially I thought we should have a way of disabling the feature on
> > some (minimal) kernels and so thought CONFIG_PM was the option to use.
> >
> >> It's quite easy to make qos.o be independent of CONFIG_PM, in which case the
> >> code added by Venki can be removed, so patches welcome (for 3.4, though).
> > I am working on it, more to come soon.
>
> I have a couple of patches ready, to be applied on 3.3-rc1 (so without
> Venki's patch applied).
> The first one is on PM QoS, the second one on per-device PM QoS. Is
> the latter needed?
I'm not sure without looking. :-)
Thanks,
Rafael
^ permalink raw reply
* Re: [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Greg KH @ 2012-02-06 17:03 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm, rob.lee
In-Reply-To: <20120206165658.GA28687@atrey.karlin.mff.cuni.cz>
On Mon, Feb 06, 2012 at 05:56:58PM +0100, Pavel Machek wrote:
> Hi!
>
> > Add a sysfs 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.
>
> /sys fs should be one-value-per-file, talk to gregkh.
That's correct.
Why not use debugfs for this instead?
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC PATCH] thermal: Add support to report cooling statistics achieved by cooling devices
From: Pavel Machek @ 2012-02-06 16:56 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linaro-dev, patches, Greg KH, linux-kernel, linux-acpi, linux-pm,
rob.lee
In-Reply-To: <1326878467-17766-1-git-send-email-amit.kachhap@linaro.org>
Hi!
> Add a sysfs 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.
/sys fs should be one-value-per-file, talk to gregkh.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Jean Pihet @ 2012-02-06 16:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Kevin Hilman, Venkatesh Pallipadi, markgross, linux-kernel,
Linux PM mailing list, Pihet-XID, Jean
In-Reply-To: <CAORVsuVCYp+Fmfaje-UDnv1=x0PqApML6Jb-Z6EG0425SuREMg@mail.gmail.com>
Rafael,
On Mon, Feb 6, 2012 at 11:18 AM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
> Hi Rafael, Mark,
>
> On Sun, Feb 5, 2012 at 12:04 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
...
>> In kernel/power/Makefile:
>>
>> obj-$(CONFIG_PM) += main.o qos.o
>>
>> I guess that explains things. :-)
> Initially I thought we should have a way of disabling the feature on
> some (minimal) kernels and so thought CONFIG_PM was the option to use.
>
>> It's quite easy to make qos.o be independent of CONFIG_PM, in which case the
>> code added by Venki can be removed, so patches welcome (for 3.4, though).
> I am working on it, more to come soon.
I have a couple of patches ready, to be applied on 3.3-rc1 (so without
Venki's patch applied).
The first one is on PM QoS, the second one on per-device PM QoS. Is
the latter needed?
Please let me know and I will send them asap.
Regards,
Jean
>>
>> Thanks,
>> Rafael
^ permalink raw reply
* Re: Query: When will drv->poweroff() called
From: Rafael J. Wysocki @ 2012-02-06 12:07 UTC (permalink / raw)
To: Viresh Kumar
Cc: len.brown, Vipul Kumar SAMAR, Bhupesh SHARMA, Pratyush ANAND,
Armando VISCONTI, Mirko GARDI, linux-kernel@vger.kernel.org,
Vipin KUMAR, Shiraz HASHIM, Amit VIRDI, Rajeev KUMAR,
deepak sikri, linux-pm@lists.linux-foundation.org,
Vincenzo FRASCINO
In-Reply-To: <4F2F6B40.6070308@st.com>
Hi,
On Monday, February 06, 2012, Viresh Kumar wrote:
>
> Hi Guys,
>
> Sorry for asking this silly question, but i couldn't locate much
> help for it in documentation, so asking it.
>
> We were testing hibernation for SPEAr13xx SoC family, based on
> ARM Cortex a9.
>
> I observed that poweroff() callback of individual drivers are not
> getting called at all, while we test hibernate.
They should be called in the last phase of hibernation, after the image
has been created and the system is going for "power off" (hance the
callback name).
> I tried to go through the code to see what happened. It looked like
> there should be call to hibernation_set_ops() for platforms that are
> willing to get a call to poweroff() for their drivers.
That's correct. The ->poweroff() callbacks are only executed if
hibernation_mode is equal to HIBERNATION_PLATFORM, which is not the
default.
> Otherwise shutdown of the busses gets called, which is a completely
> different path.
>
> There are many drivers today, that are registering poweroff() from dev_pm_ops
> but are not doing bus specific shutdown stuff.
>
> Even i tried to look for hibernation_set_ops() in kernel, and only acpi
> code is calling it.
That's correct too.
> I didn't understood how other ARM Sub-Arch's are handling this.
Well, they are supposed to call hibernation_set_ops() and set the operations
appropriately. Those operations may be empty routines if they don't need
to do anything, but they have to be defined.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Jean Pihet @ 2012-02-06 10:18 UTC (permalink / raw)
To: Rafael J. Wysocki, markgross
Cc: Kevin Hilman, Venkatesh Pallipadi, Linux PM mailing list,
linux-kernel, Pihet-XID, Jean
In-Reply-To: <201202051204.38717.rjw@sisk.pl>
Hi Rafael, Mark,
On Sun, Feb 5, 2012 at 12:04 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, February 05, 2012, mark gross wrote:
>> On Fri, Feb 03, 2012 at 03:04:43PM +0100, Pihet-XID, Jean wrote:
>> > Looping in linux-pm
>> >
>> > On Fri, Feb 3, 2012 at 1:14 AM, Venkatesh Pallipadi <venki@google.com> wrote:
>> > > Looks like change "PM QoS: Move and rename the implementation files"
>> > > made pm_qos depend on CONFIG_PM which depends on
>> > > PM_SLEEP || PM_RUNTIME
>> > >
>> > > That breaks CPU C-states with kernels not having these CONFIGs, causing CPUs
>> > > to spend time in Polling loop idle instead of going into deep C-states,
>> > > consuming way way more power. This is with either acpi idle or intel idle
>> > > enabled.
>> > >
>> > > Either CONFIG_PM should be enabled with any pm_qos users or
>> > > the !CONFIG_PM pm_qos_request() should return sane defaults not to break
>> > > the existing users. Here's is the patch for the latter option.
>> > I think the real question is whether PM QoS should be functional in
>> > all cases (as is ACPI) or whether only if certain options are set
>> > (CONFIG_PM).
>> > In the current code if CONFIG_PM is not enabled, a dummy PM QoS API is
>> > provided as function stubs in order for the build to succeed.
>> >
>> > Rafael, Mark,
>> > What do you think? Should PM QoS be enabled in all cases? Are there
>> > any known dependencies with CONFIG_PM?
>>
>> Yes I do think pm_qos interfaces should be enabled all the time and be
>> independent of CONFIG_PM. Also, I still am not a fan of the renaming
>> patch but, as the argument for and against renaming cannot be based on
>> quantifiable things I've chosen not to let it bother me.
>>
>> I think Venki's change is a band aid and we should fix it right by not
>> having a dependency on config_pm for the interface to behave.
>>
>> I'll take a look at why there is now a dependency before I have more to
>> say.
>
> In kernel/power/Makefile:
>
> obj-$(CONFIG_PM) += main.o qos.o
>
> I guess that explains things. :-)
Initially I thought we should have a way of disabling the feature on
some (minimal) kernels and so thought CONFIG_PM was the option to use.
> It's quite easy to make qos.o be independent of CONFIG_PM, in which case the
> code added by Venki can be removed, so patches welcome (for 3.4, though).
I am working on it, more to come soon.
>
> Thanks,
> Rafael
Thanks,
Jean
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Query: When will drv->poweroff() called
From: Viresh Kumar @ 2012-02-06 5:55 UTC (permalink / raw)
To: pavel, Rafael J. Wysocki, len.brown
Cc: linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
Shiraz HASHIM, Armando VISCONTI, Vipin KUMAR, deepak sikri,
Vipul Kumar SAMAR, Amit VIRDI, Rajeev KUMAR, Pratyush ANAND,
Mirko GARDI, Vincenzo FRASCINO, Bhupesh SHARMA
Hi Guys,
Sorry for asking this silly question, but i couldn't locate much
help for it in documentation, so asking it.
We were testing hibernation for SPEAr13xx SoC family, based on
ARM Cortex a9.
I observed that poweroff() callback of individual drivers are not
getting called at all, while we test hibernate.
I tried to go through the code to see what happened. It looked like
there should be call to hibernation_set_ops() for platforms that are
willing to get a call to poweroff() for their drivers. Otherwise
shutdown of the busses gets called, which is a completely different path.
There are many drivers today, that are registering poweroff() from dev_pm_ops
but are not doing bus specific shutdown stuff.
Even i tried to look for hibernation_set_ops() in kernel, and only acpi
code is calling it. I didn't understood how other ARM Sub-Arch's are handling
this.
Thanks in advance :)
--
viresh
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Rafael J. Wysocki @ 2012-02-05 11:04 UTC (permalink / raw)
To: markgross
Cc: Pihet-XID, Jean, Venkatesh Pallipadi, Kevin Hilman, linux-kernel,
Len Brown, Linux PM mailing list
In-Reply-To: <20120205035057.GA4948@gs62>
On Sunday, February 05, 2012, mark gross wrote:
> On Fri, Feb 03, 2012 at 03:04:43PM +0100, Pihet-XID, Jean wrote:
> > Looping in linux-pm
> >
> > On Fri, Feb 3, 2012 at 1:14 AM, Venkatesh Pallipadi <venki@google.com> wrote:
> > > Looks like change "PM QoS: Move and rename the implementation files"
> > > made pm_qos depend on CONFIG_PM which depends on
> > > PM_SLEEP || PM_RUNTIME
> > >
> > > That breaks CPU C-states with kernels not having these CONFIGs, causing CPUs
> > > to spend time in Polling loop idle instead of going into deep C-states,
> > > consuming way way more power. This is with either acpi idle or intel idle
> > > enabled.
> > >
> > > Either CONFIG_PM should be enabled with any pm_qos users or
> > > the !CONFIG_PM pm_qos_request() should return sane defaults not to break
> > > the existing users. Here's is the patch for the latter option.
> > I think the real question is whether PM QoS should be functional in
> > all cases (as is ACPI) or whether only if certain options are set
> > (CONFIG_PM).
> > In the current code if CONFIG_PM is not enabled, a dummy PM QoS API is
> > provided as function stubs in order for the build to succeed.
> >
> > Rafael, Mark,
> > What do you think? Should PM QoS be enabled in all cases? Are there
> > any known dependencies with CONFIG_PM?
>
> Yes I do think pm_qos interfaces should be enabled all the time and be
> independent of CONFIG_PM. Also, I still am not a fan of the renaming
> patch but, as the argument for and against renaming cannot be based on
> quantifiable things I've chosen not to let it bother me.
>
> I think Venki's change is a band aid and we should fix it right by not
> having a dependency on config_pm for the interface to behave.
>
> I'll take a look at why there is now a dependency before I have more to
> say.
In kernel/power/Makefile:
obj-$(CONFIG_PM) += main.o qos.o
I guess that explains things. :-)
It's quite easy to make qos.o be independent of CONFIG_PM, in which case the
code added by Venki can be removed, so patches welcome (for 3.4, though).
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: mark gross @ 2012-02-05 3:50 UTC (permalink / raw)
To: Pihet-XID, Jean
Cc: Kevin Hilman, Venkatesh Pallipadi, markgross, linux-kernel,
Linux PM mailing list
In-Reply-To: <CAN5iSWo-n7OER4i1tPoeGBhY_RY-nX=7n4B3mFv1f0JK074bhQ@mail.gmail.com>
On Fri, Feb 03, 2012 at 03:04:43PM +0100, Pihet-XID, Jean wrote:
> Looping in linux-pm
>
> On Fri, Feb 3, 2012 at 1:14 AM, Venkatesh Pallipadi <venki@google.com> wrote:
> > Looks like change "PM QoS: Move and rename the implementation files"
> > made pm_qos depend on CONFIG_PM which depends on
> > PM_SLEEP || PM_RUNTIME
> >
> > That breaks CPU C-states with kernels not having these CONFIGs, causing CPUs
> > to spend time in Polling loop idle instead of going into deep C-states,
> > consuming way way more power. This is with either acpi idle or intel idle
> > enabled.
> >
> > Either CONFIG_PM should be enabled with any pm_qos users or
> > the !CONFIG_PM pm_qos_request() should return sane defaults not to break
> > the existing users. Here's is the patch for the latter option.
> I think the real question is whether PM QoS should be functional in
> all cases (as is ACPI) or whether only if certain options are set
> (CONFIG_PM).
> In the current code if CONFIG_PM is not enabled, a dummy PM QoS API is
> provided as function stubs in order for the build to succeed.
>
> Rafael, Mark,
> What do you think? Should PM QoS be enabled in all cases? Are there
> any known dependencies with CONFIG_PM?
Yes I do think pm_qos interfaces should be enabled all the time and be
independent of CONFIG_PM. Also, I still am not a fan of the renaming
patch but, as the argument for and against renaming cannot be based on
quantifiable things I've chosen not to let it bother me.
I think Venki's change is a band aid and we should fix it right by not
having a dependency on config_pm for the interface to behave.
I'll take a look at why there is now a dependency before I have more to
say.
--mark
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox