* RE: [PATCH v3 0/6] thermal: exynos: Add kernel thermal support for exynos platform
From: Zhang, Rui @ 2012-05-08 20:06 UTC (permalink / raw)
To: Amit Daniel Kachhap, akpm@linux-foundation.org,
linux-pm@lists.linux-foundation.org
Cc: R, Durgadoss, linux-acpi@vger.kernel.org, lenb@kernel.org,
linaro-dev@lists.linaro.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, patches@linaro.org
In-Reply-To: <1336493898-7039-1-git-send-email-amit.kachhap@linaro.org>
Hi, Amit,
Sorry for the late response as I'm in a travel recently.
I think the generic cpufreq cooling patches are good.
But about the THERMAL_TRIP_STATE_INSTANCE patch, what I'd like to see is that
1. from thermal zone point of view, when the temperature is higher than a trip point, either ACTIVE or PASSIVE, what we should do is to set device cooling state to cur_state+1, right?
The only difference is that if we should take passive cooling actions or active cooling actions based on the policy.
So my question would be if it is possible to combine these two kind of trip points together. Maybe something like this:
In thermal_zone_device_update(),
...
case THERMAL_TRIP_PASSIVE:
if (passive cooling not allowed)
continue;
if (tc1)
thermal_zone_device_passive();
else
thermal_set_cooling_state();
break;
case THERMAL_TRIP_ACTIVE:
if (active cooling not allowed)
continue;
thermal_set_cooling_state();
break;
...
and thermal_set_cooling_state() {
list_for_each_entry(instance, &tz->cooling_devices, node) {
if (instance->trip != count)
continue;
cdev = instance->cdev;
if (temp >= trip_temp)
cdev->ops->set_cur_state(cdev, 1);
else
cdev->ops->set_cur_state(cdev, 0);
}
}
2. use only one cooling_device instance for a thermal_zone, and introduce cdev->trips which shows the trip points this cooling device is bind to.
And we may use multiple cooling devices states for one active trip point.
Then, thermal_set_cooling_state() would be look like
list_for_each_entry(instance, &tz->cooling_devices, node) {
cdev = instance->cdev;
/* check whether this cooling device is bind to the trip point */
if (!(cdev->trips & 1<<count))
continue;
cdev->ops->get_max_state(cdev, &max_state);
cdev->ops->get_cur_state(cdev, &cur_state);
if (temp >= trip_temp) {
if (cur_state++ <= max_state))
cdev->ops->set_cur_state(cdev, cur_state);
} else if ((temp < trip_temp) && (cur_state-- >= 0))
cdev->ops->set_cur_state(cdev, cur_state);
}
}
With these two things, I think the WARN_ZONE AND MONITOR_ZONE can be registered as two PASSIVE trip points in the generic thermal layer, right?
Actually, this is one thing in my TODO list. And I'd glad to make it high priority if this solves the problem you have.
Thanks,
rui
-----Original Message-----
From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Amit Daniel Kachhap
Sent: Tuesday, May 08, 2012 9:18 AM
To: akpm@linux-foundation.org; linux-pm@lists.linux-foundation.org
Cc: R, Durgadoss; linux-acpi@vger.kernel.org; lenb@kernel.org; Zhang, Rui; amit.kachhap@linaro.org; linaro-dev@lists.linaro.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; patches@linaro.org
Subject: [PATCH v3 0/6] thermal: exynos: Add kernel thermal support for exynos platform
Importance: High
Hi Andrew,
This patchset introduces a new generic cooling device based on cpufreq that can be used on non-ACPI platforms. As a proof of concept, we have drivers for the following platforms using this mechanism now:
* TI OMAP (git://git.linaro.org/people/amitdanielk/linux.git omap4460_thermal)
* Samsung Exynos (Exynos4 and Exynos5) in the current patchset.
* Freescale i.MX (git://git.linaro.org/people/amitdanielk/linux.git imx6q_thermal)
These patches have been reviewed by Rui Zhang (https://lkml.org/lkml/2012/4/9/448)
who seems to agree with them in principle, but I haven't had any luck getting them merged, perhaps a lack of maintainer bandwidth.
ACPI platforms currently have such a mechanism but it is wrapped in ACPI'isms that we don't have on ARM platforms. If this is accepted, I'm proposing to convert over the ACPI thermal driver to use this common code too.
Can you please merge these patches for 3.5?
Thanks,
Amit Daniel
Changes since V2:
*Added Exynos5 TMU sensor support by enhancing the exynos4 tmu driver. Exynos5 TMU driver was internally developed by SangWook Ju <sw.ju@samsung.com>.
*Removed cpuhotplug cooling code in this patchset.
*Rebased the patches against 3.4-rc6 kernel.
Changes since V1:
*Moved the sensor driver to driver/thermal folder from driver/hwmon folder as suggested by Mark Brown and Guenter Roeck *Added notifier support to notify the registered drivers of any cpu cooling action. The driver can modify the default cooling behaviour(eg set different max clip frequency).
*The percentage based frequency replaced with absolute clipped frequency.
*Some more conditional checks when setting max frequency.
*Renamed the new trip type THERMAL_TRIP_STATE_ACTIVE to THERMAL_TRIP_STATE_INSTANCE *Many review comments from R, Durgadoss <durgadoss.r@intel.com> and eduardo.valentin@ti.com implemented.
*Removed cooling stats through debugfs patch *The V1 based can be found here,
https://lkml.org/lkml/2012/2/22/123
http://lkml.org/lkml/2012/3/3/32
Changes since RFC:
*Changed the cpu cooling registration/unregistration API's to instance based *Changed the STATE_ACTIVE trip type to pass correct instance id *Adding support to restore back the policy->max_freq after doing frequency
clipping.
*Moved the trip cooling stats from sysfs node to debugfs node as suggested
by Greg KH greg@kroah.com
*Incorporated several review comments from eduardo.valentin@ti.com *Moved the Temperature sensor driver from driver/hwmon/ to driver/mfd as discussed with Guenter Roeck <guenter.roeck@ericsson.com> and Donggeun Kim <dg77.kim@samsung.com> (https://lkml.org/lkml/2012/1/5/7)
*Some changes according to the changes in common cpu cooling APIs *The RFC based patches can be found here,
https://lkml.org/lkml/2011/12/13/186
https://lkml.org/lkml/2011/12/21/169
Brief Description:
1) The generic cooling devices code is placed inside driver/thermal/* as placing inside acpi folder will need un-necessary enabling of acpi code. This codes is architecture independent.
2) This patchset adds a new trip type THERMAL_TRIP_STATE_INSTANCE which passes cooling device instance number and may be helpful for cpufreq cooling devices to take the correct cooling action. This trip type avoids the temperature comparision check again inside the cooling handler.
3) This patchset adds generic cpu cooling low level implementation through frequency clipping and cpu hotplug. In future, other cpu related cooling devices may be added here. An ACPI version of this already exists (drivers/acpi/processor_thermal.c). But this will be useful for platforms like ARM using the generic thermal interface along with the generic cpu cooling devices. The cooling device registration API's return cooling device pointers which can be easily binded with the thermal zone trip points.
The important APIs exposed are,
a)struct thermal_cooling_device *cpufreq_cooling_register(
struct freq_clip_table *tab_ptr, unsigned int tab_size,
const struct cpumask *mask_val)
b)void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
4) Samsung exynos platform thermal implementation is done using the generic cpu cooling APIs and the new trip type. The temperature sensor driver present in the hwmon folder(registered as hwmon driver) is moved to thermal folder and registered as a thermal driver.
All this patchset is based on Kernel version 3.4-rc6
A simple data/control flow diagrams is shown below,
Core Linux thermal <-----> Exynos thermal interface <----- Temperature Sensor
| |
\|/ |
Cpufreq cooling device <---------------
TODO:
*Will send the DT enablement patches later after the driver is merged.
Amit Daniel Kachhap (6):
thermal: Add a new trip type to use cooling device instance number
thermal: Add generic cpufreq cooling implementation
hwmon: exynos4: Move thermal sensor driver to driver/thermal
directory
thermal: exynos5: Add exynos5 thermal sensor driver support
thermal: exynos: Register the tmu sensor with the kernel thermal
layer
ARM: exynos: Add thermal sensor driver platform data support
Documentation/hwmon/exynos4_tmu | 81 ---
Documentation/thermal/cpu-cooling-api.txt | 60 ++
Documentation/thermal/exynos_thermal | 52 ++
Documentation/thermal/sysfs-api.txt | 4 +-
drivers/hwmon/Kconfig | 10 -
drivers/hwmon/Makefile | 1 -
drivers/hwmon/exynos4_tmu.c | 514 --------------
drivers/thermal/Kconfig | 20 +
drivers/thermal/Makefile | 4 +-
drivers/thermal/cpu_cooling.c | 359 ++++++++++
drivers/thermal/exynos_thermal.c | 933 ++++++++++++++++++++++++++
drivers/thermal/thermal_sys.c | 62 ++-
include/linux/cpu_cooling.h | 62 ++
include/linux/platform_data/exynos4_tmu.h | 83 ---
include/linux/platform_data/exynos_thermal.h | 100 +++
include/linux/thermal.h | 1 +
16 files changed, 1651 insertions(+), 695 deletions(-) delete mode 100644 Documentation/hwmon/exynos4_tmu create mode 100644 Documentation/thermal/cpu-cooling-api.txt
create mode 100644 Documentation/thermal/exynos_thermal
delete mode 100644 drivers/hwmon/exynos4_tmu.c create mode 100644 drivers/thermal/cpu_cooling.c create mode 100644 drivers/thermal/exynos_thermal.c create mode 100644 include/linux/cpu_cooling.h delete mode 100644 include/linux/platform_data/exynos4_tmu.h
create mode 100644 include/linux/platform_data/exynos_thermal.h
--
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: [PATCH v3 0/6] thermal: exynos: Add kernel thermal support for exynos platform
From: Andrew Morton @ 2012-05-08 20:16 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, linux-arm-kernel
In-Reply-To: <1336493898-7039-1-git-send-email-amit.kachhap@linaro.org>
On Tue, 8 May 2012 21:48:12 +0530
Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> This patchset introduces a new generic cooling device based on cpufreq that
> can be used on non-ACPI platforms. As a proof of concept, we have drivers for
> the following platforms using this mechanism now:
>
> * TI OMAP (git://git.linaro.org/people/amitdanielk/linux.git omap4460_thermal)
> * Samsung Exynos (Exynos4 and Exynos5) in the current patchset.
> * Freescale i.MX (git://git.linaro.org/people/amitdanielk/linux.git imx6q_thermal)
>
> These patches have been reviewed by Rui Zhang (https://lkml.org/lkml/2012/4/9/448)
But we don't have explicit Reviewed-by:s for the changelogs?
> who seems to agree with them in principle, but I haven't had any luck getting them
> merged, perhaps a lack of maintainer bandwidth.
>
> ACPI platforms currently have such a mechanism but it is wrapped in ACPI'isms
> that we don't have on ARM platforms. If this is accepted, I'm proposing to
> convert over the ACPI thermal driver to use this common code too.
^ permalink raw reply
* Re: [PATCH v3 2/6] thermal: Add generic cpufreq cooling implementation
From: Andrew Morton @ 2012-05-08 20:16 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, linux-arm-kernel
In-Reply-To: <1336493898-7039-3-git-send-email-amit.kachhap@linaro.org>
On Tue, 8 May 2012 21:48:14 +0530
Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling up/down based on the registration
> parameters. 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 APIs return the
> cooling device pointer. The user of these APIs are responsible for
> passing clipping frequency . The drivers can also register to recieve
> notification about any cooling action called. Even the driver can effect
> the cooling action by modifying the default data such as freq_clip_max if
> needed.
>
>
> ...
>
> +struct cpufreq_cooling_device {
> + int id;
> + struct thermal_cooling_device *cool_dev;
> + struct freq_clip_table *tab_ptr;
> + unsigned int tab_size;
> + unsigned int cpufreq_state;
> + const struct cpumask *allowed_cpus;
> + struct list_head node;
> +};
It would be nice to document the fields. Especially id, tab_size,
cpufreq_state and node. For `node' we should describe the locking for
the list, and describe which list_head anchors this list.
> +static LIST_HEAD(cooling_cpufreq_list);
> +static DEFINE_MUTEX(cooling_cpufreq_lock);
> +static DEFINE_IDR(cpufreq_idr);
> +static DEFINE_PER_CPU(unsigned int, max_policy_freq);
> +static struct freq_clip_table *notify_table;
> +static int notify_state;
> +static BLOCKING_NOTIFIER_HEAD(cputherm_state_notifier_list);
> +
> +static int get_idr(struct idr *idr, struct mutex *lock, int *id)
> +{
> + int err;
> +again:
> + if (unlikely(idr_pre_get(idr, GFP_KERNEL) == 0))
> + return -ENOMEM;
> +
> + if (lock)
> + mutex_lock(lock);
The test for NULL `lock' is unneeded. In fact the `lock' argument
could be removed altogether - just use cooling_cpufreq_lock directly.
> + err = idr_get_new(idr, NULL, id);
> + if (lock)
> + mutex_unlock(lock);
> + if (unlikely(err == -EAGAIN))
> + goto again;
> + else if (unlikely(err))
> + return err;
> +
> + *id = *id & MAX_ID_MASK;
> + return 0;
> +}
> +
> +static void release_idr(struct idr *idr, struct mutex *lock, int id)
> +{
> + if (lock)
> + mutex_lock(lock);
Ditto.
> + idr_remove(idr, id);
> + if (lock)
> + mutex_unlock(lock);
> +}
> +
>
> ...
>
> +
> +/*Below codes defines functions to be used for cpufreq as cooling device*/
> +static bool is_cpufreq_valid(int cpu)
> +{
> + struct cpufreq_policy policy;
> + return !cpufreq_get_policy(&policy, cpu) ? true : false;
Can use
return !cpufreq_get_policy(&policy, cpu);
> +}
> +
> +static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
> + unsigned long cooling_state)
> +{
> + unsigned int event, cpuid;
> + struct freq_clip_table *th_table;
> +
> + if (cooling_state > cpufreq_device->tab_size)
> + return -EINVAL;
> +
> + cpufreq_device->cpufreq_state = cooling_state;
> +
> + /*cpufreq thermal notifier uses this cpufreq device pointer*/
This code looks like it was written by two people.
/* One who does this */
/*And one who does this*/
The first one was right. Please go through all the comments in all the
patches and get the layout consistent?
> + notify_state = cooling_state;
> +
> + if (notify_state > 0) {
> + th_table = &(cpufreq_device->tab_ptr[cooling_state - 1]);
> + memcpy(notify_table, th_table, sizeof(struct freq_clip_table));
> + event = CPUFREQ_COOLING_TYPE;
> + blocking_notifier_call_chain(&cputherm_state_notifier_list,
> + event, notify_table);
> + }
> +
> + for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
> + if (is_cpufreq_valid(cpuid))
> + cpufreq_update_policy(cpuid);
> + }
> +
> + notify_state = -1;
> +
> + return 0;
> +}
> +
> +static int cpufreq_thermal_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct cpufreq_policy *policy = data;
> + unsigned long max_freq = 0;
> +
> + if ((event != CPUFREQ_ADJUST) || (notify_state == -1))
Please document `notify_state', at its definition site. This reader
doesn't know what "notify_state == -1" *means*.
> + return 0;
> +
> + if (notify_state > 0) {
> + max_freq = notify_table->freq_clip_max;
> +
> + if (per_cpu(max_policy_freq, policy->cpu) == 0)
> + per_cpu(max_policy_freq, policy->cpu) = policy->max;
> + } else {
> + if (per_cpu(max_policy_freq, policy->cpu) != 0) {
> + max_freq = per_cpu(max_policy_freq, policy->cpu);
> + per_cpu(max_policy_freq, policy->cpu) = 0;
> + } else {
> + max_freq = policy->max;
> + }
> + }
> +
> + /* Never exceed user_policy.max*/
> + if (max_freq > policy->user_policy.max)
> + max_freq = policy->user_policy.max;
> +
> + if (policy->max != max_freq)
> + cpufreq_verify_within_limits(policy, 0, max_freq);
> +
> + return 0;
> +}
> +
>
> ...
>
> +/*This cooling may be as PASSIVE/ACTIVE type*/
> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long state)
> +{
> + int ret = -EINVAL;
> + struct cpufreq_cooling_device *cpufreq_device;
> +
> + mutex_lock(&cooling_cpufreq_lock);
> + list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node) {
> + if (cpufreq_device && cpufreq_device->cool_dev == cdev) {
> + ret = 0;
> + break;
> + }
> + }
> + mutex_unlock(&cooling_cpufreq_lock);
> +
> + if (!ret)
> + ret = cpufreq_apply_cooling(cpufreq_device, state);
Now that we've dropped the lock, what prevents *cpufreq_device from
getting freed, or undesirably altered?
> + return ret;
> +}
> +
> +/* bind cpufreq callbacks to cpufreq cooling device */
> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
Can it be made const?
> + .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 = cpufreq_thermal_notifier,
> +};
> +
> +struct thermal_cooling_device *cpufreq_cooling_register(
> + struct freq_clip_table *tab_ptr, unsigned int tab_size,
> + const struct cpumask *mask_val)
> +{
> + struct thermal_cooling_device *cool_dev;
> + struct cpufreq_cooling_device *cpufreq_dev = NULL;
> + unsigned int cpufreq_dev_count = 0;
> + char dev_name[THERMAL_NAME_LENGTH];
> + int ret = 0, id = 0, i;
> +
> + if (tab_ptr == NULL || tab_size == 0)
> + return ERR_PTR(-EINVAL);
> +
> + list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node)
> + cpufreq_dev_count++;
> +
> + cpufreq_dev =
> + kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
The 80-col contortions are ugly. Alternatives are
cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
GFP_KERNEL);
or, better,
cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);
> + if (!cpufreq_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + if (cpufreq_dev_count == 0) {
> + notify_table = kzalloc(sizeof(struct freq_clip_table),
> + GFP_KERNEL);
> + if (!notify_table) {
> + kfree(cpufreq_dev);
> + return ERR_PTR(-ENOMEM);
> + }
> + }
> +
> + cpufreq_dev->tab_ptr = tab_ptr;
> + cpufreq_dev->tab_size = tab_size;
> + cpufreq_dev->allowed_cpus = mask_val;
> +
> + /* Initialize all the tab_ptr->mask_val to the passed mask_val */
> + for (i = 0; i < tab_size; i++)
> + ((struct freq_clip_table *)&tab_ptr[i])->mask_val = mask_val;
> +
> + ret = get_idr(&cpufreq_idr, &cooling_cpufreq_lock, &cpufreq_dev->id);
hm, "get_idr" is a poor name. One would expect it to do a lookup, but
it actually does an installation. That's a result of the poorly-named
idr_get_new(), I expect.
> + if (ret) {
> + kfree(cpufreq_dev);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + sprintf(dev_name, "thermal-cpufreq-%d", cpufreq_dev->id);
> +
> + cool_dev = thermal_cooling_device_register(dev_name, cpufreq_dev,
> + &cpufreq_cooling_ops);
> + if (!cool_dev) {
> + release_idr(&cpufreq_idr, &cooling_cpufreq_lock,
> + cpufreq_dev->id);
> + kfree(cpufreq_dev);
> + return ERR_PTR(-EINVAL);
> + }
> + cpufreq_dev->id = id;
> + cpufreq_dev->cool_dev = cool_dev;
> + mutex_lock(&cooling_cpufreq_lock);
> + list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list);
> + mutex_unlock(&cooling_cpufreq_lock);
> +
> + /*Register the notifier for first cpufreq cooling device*/
> + if (cpufreq_dev_count == 0)
> + cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> + CPUFREQ_POLICY_NOTIFIER);
> + return cool_dev;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +
> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> +{
> + struct cpufreq_cooling_device *cpufreq_dev = NULL;
> + unsigned int cpufreq_dev_count = 0;
> +
> + mutex_lock(&cooling_cpufreq_lock);
> + list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
> + if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
> + break;
> + cpufreq_dev_count++;
> + }
> +
> + if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
> + mutex_unlock(&cooling_cpufreq_lock);
> + return;
> + }
> +
> + list_del(&cpufreq_dev->node);
> + mutex_unlock(&cooling_cpufreq_lock);
> +
> + /*Unregister the notifier for the last cpufreq cooling device*/
> + if (cpufreq_dev_count == 1) {
But we dropped the lock, so local variable cpufreq_dev_count is now
meaningless. What prevents a race here?
> + cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> + CPUFREQ_POLICY_NOTIFIER);
> + kfree(notify_table);
> + }
> +
> + thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
> + release_idr(&cpufreq_idr, &cooling_cpufreq_lock, cpufreq_dev->id);
> + kfree(cpufreq_dev);
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>
> ...
>
> +struct freq_clip_table {
> + unsigned int freq_clip_max;
> + unsigned int polling_interval;
> + unsigned int temp_level;
> + const struct cpumask *mask_val;
> +};
hm, what does this thing do. Needs a nice comment for the uninitiated,
please. Something which describes the overall roles, responsibilities
and general reasons for existence.
> +int cputherm_register_notifier(struct notifier_block *nb, unsigned int list);
> +int cputherm_unregister_notifier(struct notifier_block *nb, unsigned int list);
> +
> +#ifdef CONFIG_CPU_FREQ
> +struct thermal_cooling_device *cpufreq_cooling_register(
> + struct freq_clip_table *tab_ptr, unsigned int tab_size,
> + const struct cpumask *mask_val);
> +
> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
> +#else /*!CONFIG_CPU_FREQ*/
(more whacky comment layout)
>
> ...
>
^ permalink raw reply
* Re: [PATCH v3 3/6] hwmon: exynos4: Move thermal sensor driver to driver/thermal directory
From: Andrew Morton @ 2012-05-08 20:16 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, linux-arm-kernel
In-Reply-To: <1336493898-7039-4-git-send-email-amit.kachhap@linaro.org>
On Tue, 8 May 2012 21:48:15 +0530
Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> This movement is needed because the hwmon entries and corresponding
> sysfs interface is a duplicate of utilities already provided by
> driver/thermal/thermal_sys.c. The goal is to place it in thermal folder
> and add necessary functions to use the in-kernel thermal interfaces.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> Signed-off-by: Donggeun Kim <dg77.kim@samsung.com>
> ---
> Documentation/hwmon/exynos4_tmu | 81 ----
> Documentation/thermal/exynos_thermal | 52 +++
> drivers/hwmon/Kconfig | 10 -
> drivers/hwmon/Makefile | 1 -
> drivers/hwmon/exynos4_tmu.c | 514 --------------------------
Please cc the hwmon people? Jean Delvare <khali@linux-fr.org>, Guenter
Roeck <guenter.roeck@ericsson.com>, lm-sensors@lm-sensors.org
> drivers/thermal/Kconfig | 9 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/exynos_thermal.c | 409 ++++++++++++++++++++
> include/linux/platform_data/exynos4_tmu.h | 83 ----
> include/linux/platform_data/exynos_thermal.h | 83 ++++
^ permalink raw reply
* Re: [PATCH v3 5/6] thermal: exynos: Register the tmu sensor with the kernel thermal layer
From: Andrew Morton @ 2012-05-08 20:16 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, linux-arm-kernel
In-Reply-To: <1336493898-7039-6-git-send-email-amit.kachhap@linaro.org>
On Tue, 8 May 2012 21:48:17 +0530
Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> This code added creates a link between temperature sensors, linux thermal
> framework and cooling devices for samsung exynos platform. This layer
> monitors the temperature from the sensor and informs the generic thermal
> layer to take the necessary cooling action.
>
>
> ...
>
> +static void exynos_report_trigger(void)
> +{
> + unsigned int i;
> + char data[2];
> + char *envp[] = { data, NULL };
> +
> + if (!th_zone || !th_zone->therm_dev)
> + return;
> +
> + thermal_zone_device_update(th_zone->therm_dev);
> +
> + mutex_lock(&th_zone->therm_dev->lock);
> + /* Find the level for which trip happened */
> + for (i = 0; i < th_zone->sensor_conf->trip_data.trip_count; i++) {
> + if (th_zone->therm_dev->last_temperature <
> + th_zone->sensor_conf->trip_data.trip_val[i] * 1000)
> + break;
> + }
> +
> + if (th_zone->mode == THERMAL_DEVICE_ENABLED) {
> + if (i > 0)
> + th_zone->therm_dev->polling_delay = ACTIVE_INTERVAL;
> + else
> + th_zone->therm_dev->polling_delay = IDLE_INTERVAL;
> + }
> +
> + sprintf(data, "%u", i);
yikes, if `i' exceeds 9, we have a stack scribble. Please review this
and at least use snprintf(... sizeof(data)) to prevent accidents.
> + kobject_uevent_env(&th_zone->therm_dev->device.kobj, KOBJ_CHANGE, envp);
> + mutex_unlock(&th_zone->therm_dev->lock);
> +}
> +
>
> ...
>
> +/* Get trip temperature callback functions for thermal zone */
> +static int exynos_get_trip_temp(struct thermal_zone_device *thermal, int trip,
> + unsigned long *temp)
> +{
> + if (trip < 0 || trip > 2)
I don't know what `trip' does and I don't know the meaning of the
values 0, 1 and 2. Documentation, please.
> + return -EINVAL;
> +
> + *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
> + /* convert the temperature into millicelsius */
> + *temp = *temp * 1000;
> +
> + return 0;
> +}
> +
>
> ...
>
> +/* Bind callback functions for thermal zone */
> +static int exynos_bind(struct thermal_zone_device *thermal,
> + struct thermal_cooling_device *cdev)
> +{
> + int ret = 0;
> +
> + /* if the cooling device is the one from exynos4 bind it */
> + if (cdev != th_zone->cool_dev[0])
> + return 0;
> +
> + if (thermal_zone_bind_cooling_device(thermal, 0, cdev)) {
> + pr_err("error binding cooling dev inst 0\n");
> + return -EINVAL;
> + }
> + if (thermal_zone_bind_cooling_device(thermal, 1, cdev)) {
> + pr_err("error binding cooling dev inst 1\n");
> + ret = -EINVAL;
> + goto error_bind1;
> + }
There can never be more than two instances?
> + return ret;
> +error_bind1:
> + thermal_zone_unbind_cooling_device(thermal, 0, cdev);
> + return ret;
> +}
> +
>
> ...
>
> +/* Get temperature callback functions for thermal zone */
> +static int exynos_get_temp(struct thermal_zone_device *thermal,
> + unsigned long *temp)
> +{
> + void *data;
> +
> + if (!th_zone->sensor_conf) {
> + pr_info("Temperature sensor not initialised\n");
> + return -EINVAL;
> + }
> + data = th_zone->sensor_conf->private_data;
> + *temp = th_zone->sensor_conf->read_temperature(data);
> + /* convert the temperature into millicelsius */
> + *temp = *temp * 1000;
> + return 0;
> +}
> +
> +/* Operation callback functions for thermal zone */
> +static struct thermal_zone_device_ops exynos_dev_ops = {
Can it be const? That sometimes saves space, as the table doesn't need
to be moved into writeable storage at runtime.
> + .bind = exynos_bind,
> + .unbind = exynos_unbind,
> + .get_temp = exynos_get_temp,
> + .get_mode = exynos_get_mode,
> + .set_mode = exynos_set_mode,
> + .get_trip_type = exynos_get_trip_type,
> + .get_trip_temp = exynos_get_trip_temp,
> + .get_crit_temp = exynos_get_crit_temp,
> +};
> +
> +/* Register with the in-kernel thermal management */
> +static int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
> +{
> + int ret, count, tab_size;
> + struct freq_clip_table *tab_ptr;
> +
> + if (!sensor_conf || !sensor_conf->read_temperature) {
> + pr_err("Temperature sensor not initialised\n");
> + return -EINVAL;
> + }
> +
> + th_zone = kzalloc(sizeof(struct exynos_thermal_zone), GFP_KERNEL);
> + if (!th_zone) {
> + ret = -ENOMEM;
> + goto err_unregister;
This seems wrong? If we need to call exynos_unregister_thermal() on
this error path then we needed to call it on the predecing one?
Perhaps?
> + }
> +
> + th_zone->sensor_conf = sensor_conf;
> +
> + tab_ptr = (struct freq_clip_table *)sensor_conf->cooling_data.freq_data;
> + tab_size = sensor_conf->cooling_data.freq_clip_count;
> +
> + /* Register the cpufreq cooling device */
> + th_zone->cool_dev_size = 1;
> + count = 0;
> + th_zone->cool_dev[count] = cpufreq_cooling_register(
> + (struct freq_clip_table *)&(tab_ptr[count]),
> + tab_size, cpumask_of(0));
> +
> + if (IS_ERR(th_zone->cool_dev[count])) {
> + pr_err("Failed to register cpufreq cooling device\n");
> + ret = -EINVAL;
> + th_zone->cool_dev_size = 0;
> + goto err_unregister;
> + }
> +
> + th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
> + 3, NULL, &exynos_dev_ops, 0, 0, 0, IDLE_INTERVAL);
> +
> + if (IS_ERR(th_zone->therm_dev)) {
> + pr_err("Failed to register thermal zone device\n");
> + ret = -EINVAL;
> + goto err_unregister;
> + }
> + th_zone->mode = THERMAL_DEVICE_ENABLED;
> +
> + pr_info("Exynos: Kernel Thermal management registered\n");
> +
> + return 0;
> +
> +err_unregister:
> + exynos_unregister_thermal();
> + return ret;
> +}
> +
>
> ...
>
^ permalink raw reply
* Re: [PATCH v3 6/6] ARM: exynos: Add thermal sensor driver platform data support
From: Andrew Morton @ 2012-05-08 20:16 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, linux-arm-kernel
In-Reply-To: <1336493898-7039-7-git-send-email-amit.kachhap@linaro.org>
On Tue, 8 May 2012 21:48:18 +0530
Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> This patch adds necessary default platform data support needed for TMU driver.
> This dt/non-dt values are tested for origen exynos4210 and smdk exynos5250 platforms.
>
>
> ...
>
> --- a/drivers/thermal/exynos_thermal.c
> +++ b/drivers/thermal/exynos_thermal.c
> @@ -646,14 +646,117 @@ static irqreturn_t exynos_tmu_irq(int irq, void *id)
> static struct thermal_sensor_conf exynos_sensor_conf = {
> .name = "exynos-therm",
> .read_temperature = (int (*)(void *))exynos_tmu_read,
> +};
> +
> +#if defined(CONFIG_CPU_EXYNOS4210)
> +static struct exynos_tmu_platform_data exynos4_default_tmu_data = {
Again, make it const if possible.
> + .threshold = 80,
> + .trigger_levels[0] = 5,
> + .trigger_levels[1] = 20,
> + .trigger_levels[2] = 30,
> + .trigger_level0_en = 1,
> + .trigger_level1_en = 1,
> + .trigger_level2_en = 1,
> + .trigger_level3_en = 0,
> + .gain = 15,
> + .reference_voltage = 7,
> + .cal_type = TYPE_ONE_POINT_TRIMMING,
> + .freq_tab[0] = {
> + .freq_clip_max = 800 * 1000,
> + },
> + .freq_tab[1] = {
> + .freq_clip_max = 200 * 1000,
> + },
> + .freq_tab_count = 2,
> + .type = SOC_ARCH_EXYNOS4,
> +};
> +#define EXYNOS4_TMU_DRV_DATA ((kernel_ulong_t)&exynos4_default_tmu_data)
> +#else
> +#define EXYNOS4_TMU_DRV_DATA ((kernel_ulong_t)NULL)
> +#endif
See below.
> +#if defined(CONFIG_SOC_EXYNOS5250)
> +static struct exynos_tmu_platform_data exynos5_default_tmu_data = {
> + .trigger_levels[0] = 85,
> + .trigger_levels[1] = 103,
> + .trigger_levels[2] = 110,
> + .trigger_level0_en = 1,
> + .trigger_level1_en = 1,
> + .trigger_level2_en = 1,
> + .trigger_level3_en = 0,
> + .gain = 8,
> + .reference_voltage = 16,
> + .noise_cancel_mode = 4,
> + .cal_type = TYPE_ONE_POINT_TRIMMING,
> + .efuse_value = 55,
> + .freq_tab[0] = {
> + .freq_clip_max = 800 * 1000,
> + },
> + .freq_tab[1] = {
> + .freq_clip_max = 200 * 1000,
> + },
> + .freq_tab_count = 2,
> + .type = SOC_ARCH_EXYNOS5,
> +};
> +#define EXYNOS5_TMU_DRV_DATA ((kernel_ulong_t)&exynos5_default_tmu_data)
The use of kernel_ulong_t is unexpected.
I suspect you could remove this cast altogether. Or make it void*.
> +#else
> +#define EXYNOS5_TMU_DRV_DATA ((kernel_ulong_t)NULL)
And remove this cast too. Rely upon void* magic.
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id exynos_tmu_match[] = {
> + {
> + .compatible = "samsung,exynos4-tmu",
> + .data = (void *)EXYNOS4_TMU_DRV_DATA,
No cast is needed if EXYNOS4_TMU_DRV_DATA has a pointer type.
> + },
> + {
> + .compatible = "samsung,exynos5-tmu",
> + .data = (void *)EXYNOS5_TMU_DRV_DATA,
No cast is needed if EXYNOS4_TMU_DRV_DATA has a pointer type.
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_tmu_match);
> +#else
> +#define exynos_tmu_match NULL
> +#endif
> +
>
> ...
>
^ permalink raw reply
* Re: [PATCH V3 00/10] PM: Create the AVS(Adaptive Voltage Scaling)
From: Woodruff, Richard @ 2012-05-08 20:38 UTC (permalink / raw)
To: AnilKumar, Chimata, Hilman, Kevin
Cc: J, KEERTHY, Mark Brown, linux-kernel@vger.kernel.org,
linux-pm@lists.linux-foundation.org, linux-omap@vger.kernel.org,
Pihet-XID, Jean, linux-arm-kernel@lists.infradead.org
In-Reply-To: <331ABD5ECB02734CA317220B2BBEABC13E9B6681@DBDE01.ent.ti.com>
> > >> The only thing the higher-level layers might potentially need to do
> > >> is to enable/disable AVS around transitions (e.g. when changing OPP, > > >> AVS is disabled before changing OPP and only re-enabled when the new > >> >> nominal voltage has been acheived.)
Getting clean baseline in place is huge step but actual production interfaces will need to comprehend some OPP to AVS dependencies beyond on/off.
AVS as used in many OMAP designs (mostly > OMAP4430) do have some per OPP dependent details:
- Multiple PMICs are in use. In some current designs the AVS step size is different per voltage range. At OPP change time you have to reconfigure several AVS parameters before enable.
- ABB-ldo sequencing and parameters in tightly coupled with OPP and AVS enables.
- Good power savings can be had in future 1.5/3.5 by adjusting nominal to calibrated-plus-margin voltage.
Regards,
Richard W.
^ permalink raw reply
* Re: [linux-pm] [PATCH V3 00/10] PM: Create the AVS(Adaptive Voltage Scaling)
From: Kevin Hilman @ 2012-05-08 22:16 UTC (permalink / raw)
To: Woodruff, Richard
Cc: AnilKumar, Chimata, J, KEERTHY, Mark Brown,
linux-kernel@vger.kernel.org, linux-pm@lists.linux-foundation.org,
linux-omap@vger.kernel.org, Pihet-XID, Jean,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <EF62F09C0797D947AD4180A1043C0DF73C40BCE1@DLEE10.ent.ti.com>
"Woodruff, Richard" <r-woodruff2@ti.com> writes:
>> > >> The only thing the higher-level layers might potentially need to
>> > >> do is to enable/disable AVS around transitions (e.g. when
>> > >> changing OPP, > > >> AVS is disabled before changing OPP and
>> > >> only re-enabled when the new > >> >> nominal voltage has been
>> > >> acheived.)
>
> Getting clean baseline in place is huge step but actual production
> interfaces will need to comprehend some OPP to AVS dependencies beyond
> on/off.
A basic OMAP AVS driver has been in mainline for a long time, yet we
have not seen support submitted for all of these features.
When folks are motivated to propose such changes upstream, we will be
happy to discuss them and add support for them to the AVS driver.
Until then, the primary purpose of this series is to do some minimal
cleanup of an *existing* driver so it can be moved into drivers/*. New
features can be added there as easily as they could've been added when
it was a driver under arch/arm.
Kevin
^ permalink raw reply
* Re: [PATCH V3 00/10] PM: Create the AVS(Adaptive Voltage Scaling)
From: Woodruff, Richard @ 2012-05-09 0:39 UTC (permalink / raw)
To: Hilman, Kevin
Cc: J, KEERTHY, Mark Brown, linux-kernel@vger.kernel.org,
AnilKumar, Chimata, linux-pm@lists.linux-foundation.org,
linux-omap@vger.kernel.org, Pihet-XID, Jean,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <87lil2jp2z.fsf@ti.com>
> From: Hilman, Kevin
> Sent: Tuesday, May 08, 2012 5:17 PM
> A basic OMAP AVS driver has been in mainline for a long time, yet we
> have not seen support submitted for all of these features.
1.5/3.5 is a feature.
ABB is requirement for a production useable driver. Higher speed rated OMAP4 and all OMAP5 added these to be useable. Yes this is effort. Point of mentioning is to raise awareness of need.
Yet to be added feature has different meaning than functional gap.
Regards,
Richard W.
^ permalink raw reply
* Re: [linux-pm] [PATCH V3 00/10] PM: Create the AVS(Adaptive Voltage Scaling)
From: Koen Kooi @ 2012-05-09 8:19 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Hilman, Kevin, AnilKumar, Chimata, J, KEERTHY, Mark Brown,
linux-kernel@vger.kernel.org, linux-pm@lists.linux-foundation.org,
linux-omap@vger.kernel.org, Pihet-XID, Jean,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <EF62F09C0797D947AD4180A1043C0DF73C40C247@DLEE10.ent.ti.com>
Op 9 mei 2012, om 02:39 heeft Woodruff, Richard het volgende geschreven:
>> From: Hilman, Kevin
>> Sent: Tuesday, May 08, 2012 5:17 PM
>
>> A basic OMAP AVS driver has been in mainline for a long time, yet we
>> have not seen support submitted for all of these features.
>
> 1.5/3.5 is a feature.
>
> ABB is requirement for a production useable driver.
ABB/FBB is also needed for 1GHz support on omap3 based SoCs like AM37xx and DM37xx.
regards,
Koen
^ permalink raw reply
* Re: [PATCH v3 2/6] thermal: Add generic cpufreq cooling implementation
From: Amit Kachhap @ 2012-05-09 8:27 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, linux-arm-kernel
In-Reply-To: <20120508131626.ffe262f6.akpm@linux-foundation.org>
On 9 May 2012 01:46, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 8 May 2012 21:48:14 +0530
> Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling up/down based on the registration
>> parameters. 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 APIs return the
>> cooling device pointer. The user of these APIs are responsible for
>> passing clipping frequency . The drivers can also register to recieve
>> notification about any cooling action called. Even the driver can effect
>> the cooling action by modifying the default data such as freq_clip_max if
>> needed.
>>
>>
>> ...
>>
>> +struct cpufreq_cooling_device {
>> + int id;
>> + struct thermal_cooling_device *cool_dev;
>> + struct freq_clip_table *tab_ptr;
>> + unsigned int tab_size;
>> + unsigned int cpufreq_state;
>> + const struct cpumask *allowed_cpus;
>> + struct list_head node;
>> +};
>
> It would be nice to document the fields. Especially id, tab_size,
> cpufreq_state and node. For `node' we should describe the locking for
> the list, and describe which list_head anchors this list.
Thanks Andrew for the detailed review. I will add more documentation
and post the next version shortly.
>
>> +static LIST_HEAD(cooling_cpufreq_list);
>> +static DEFINE_MUTEX(cooling_cpufreq_lock);
>> +static DEFINE_IDR(cpufreq_idr);
>> +static DEFINE_PER_CPU(unsigned int, max_policy_freq);
>> +static struct freq_clip_table *notify_table;
>> +static int notify_state;
>> +static BLOCKING_NOTIFIER_HEAD(cputherm_state_notifier_list);
>> +
>> +static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>> +{
>> + int err;
>> +again:
>> + if (unlikely(idr_pre_get(idr, GFP_KERNEL) == 0))
>> + return -ENOMEM;
>> +
>> + if (lock)
>> + mutex_lock(lock);
>
> The test for NULL `lock' is unneeded. In fact the `lock' argument
> could be removed altogether - just use cooling_cpufreq_lock directly.
Agreed
>
>> + err = idr_get_new(idr, NULL, id);
>> + if (lock)
>> + mutex_unlock(lock);
>> + if (unlikely(err == -EAGAIN))
>> + goto again;
>> + else if (unlikely(err))
>> + return err;
>> +
>> + *id = *id & MAX_ID_MASK;
>> + return 0;
>> +}
>> +
>> +static void release_idr(struct idr *idr, struct mutex *lock, int id)
>> +{
>> + if (lock)
>> + mutex_lock(lock);
>
> Ditto.
>
>> + idr_remove(idr, id);
>> + if (lock)
>> + mutex_unlock(lock);
>> +}
>> +
>>
>> ...
>>
>> +
>> +/*Below codes defines functions to be used for cpufreq as cooling device*/
>> +static bool is_cpufreq_valid(int cpu)
>> +{
>> + struct cpufreq_policy policy;
>> + return !cpufreq_get_policy(&policy, cpu) ? true : false;
>
> Can use
Ok
>
> return !cpufreq_get_policy(&policy, cpu);
>
>> +}
>> +
>> +static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
>> + unsigned long cooling_state)
>> +{
>> + unsigned int event, cpuid;
>> + struct freq_clip_table *th_table;
>> +
>> + if (cooling_state > cpufreq_device->tab_size)
>> + return -EINVAL;
>> +
>> + cpufreq_device->cpufreq_state = cooling_state;
>> +
>> + /*cpufreq thermal notifier uses this cpufreq device pointer*/
>
> This code looks like it was written by two people.
>
> /* One who does this */
> /*And one who does this*/
>
> The first one was right. Please go through all the comments in all the
> patches and get the layout consistent?
Sure will add more details.
>
>
>> + notify_state = cooling_state;
>> +
>> + if (notify_state > 0) {
>> + th_table = &(cpufreq_device->tab_ptr[cooling_state - 1]);
>> + memcpy(notify_table, th_table, sizeof(struct freq_clip_table));
>> + event = CPUFREQ_COOLING_TYPE;
>> + blocking_notifier_call_chain(&cputherm_state_notifier_list,
>> + event, notify_table);
>> + }
>> +
>> + for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
>> + if (is_cpufreq_valid(cpuid))
>> + cpufreq_update_policy(cpuid);
>> + }
>> +
>> + notify_state = -1;
>> +
>> + return 0;
>> +}
>> +
>> +static int cpufreq_thermal_notifier(struct notifier_block *nb,
>> + unsigned long event, void *data)
>> +{
>> + struct cpufreq_policy *policy = data;
>> + unsigned long max_freq = 0;
>> +
>> + if ((event != CPUFREQ_ADJUST) || (notify_state == -1))
>
> Please document `notify_state', at its definition site. This reader
> doesn't know what "notify_state == -1" *means*.
>
>> + return 0;
>> +
>> + if (notify_state > 0) {
>> + max_freq = notify_table->freq_clip_max;
>> +
>> + if (per_cpu(max_policy_freq, policy->cpu) == 0)
>> + per_cpu(max_policy_freq, policy->cpu) = policy->max;
>> + } else {
>> + if (per_cpu(max_policy_freq, policy->cpu) != 0) {
>> + max_freq = per_cpu(max_policy_freq, policy->cpu);
>> + per_cpu(max_policy_freq, policy->cpu) = 0;
>> + } else {
>> + max_freq = policy->max;
>> + }
>> + }
>> +
>> + /* Never exceed user_policy.max*/
>> + if (max_freq > policy->user_policy.max)
>> + max_freq = policy->user_policy.max;
>> +
>> + if (policy->max != max_freq)
>> + cpufreq_verify_within_limits(policy, 0, max_freq);
>> +
>> + return 0;
>> +}
>> +
>>
>> ...
>>
>> +/*This cooling may be as PASSIVE/ACTIVE type*/
>> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long state)
>> +{
>> + int ret = -EINVAL;
>> + struct cpufreq_cooling_device *cpufreq_device;
>> +
>> + mutex_lock(&cooling_cpufreq_lock);
>> + list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node) {
>> + if (cpufreq_device && cpufreq_device->cool_dev == cdev) {
>> + ret = 0;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&cooling_cpufreq_lock);
>> +
>> + if (!ret)
>> + ret = cpufreq_apply_cooling(cpufreq_device, state);
>
> Now that we've dropped the lock, what prevents *cpufreq_device from
> getting freed, or undesirably altered?
Agreed the lock can be put over the entire funtion.
>
>> + return ret;
>> +}
>> +
>> +/* bind cpufreq callbacks to cpufreq cooling device */
>> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
>
> Can it be made const?
Yes it can be made const as it is unmodified.
>
>> + .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 = cpufreq_thermal_notifier,
>> +};
>> +
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> + struct freq_clip_table *tab_ptr, unsigned int tab_size,
>> + const struct cpumask *mask_val)
>> +{
>> + struct thermal_cooling_device *cool_dev;
>> + struct cpufreq_cooling_device *cpufreq_dev = NULL;
>> + unsigned int cpufreq_dev_count = 0;
>> + char dev_name[THERMAL_NAME_LENGTH];
>> + int ret = 0, id = 0, i;
>> +
>> + if (tab_ptr == NULL || tab_size == 0)
>> + return ERR_PTR(-EINVAL);
>> +
>> + list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node)
>> + cpufreq_dev_count++;
>> +
>> + cpufreq_dev =
>> + kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
>
> The 80-col contortions are ugly. Alternatives are
>
> cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
> GFP_KERNEL);
>
> or, better,
>
> cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);
Ok will use shorter variables.
>
>
>> + if (!cpufreq_dev)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + if (cpufreq_dev_count == 0) {
>> + notify_table = kzalloc(sizeof(struct freq_clip_table),
>> + GFP_KERNEL);
>> + if (!notify_table) {
>> + kfree(cpufreq_dev);
>> + return ERR_PTR(-ENOMEM);
>> + }
>> + }
>> +
>> + cpufreq_dev->tab_ptr = tab_ptr;
>> + cpufreq_dev->tab_size = tab_size;
>> + cpufreq_dev->allowed_cpus = mask_val;
>> +
>> + /* Initialize all the tab_ptr->mask_val to the passed mask_val */
>> + for (i = 0; i < tab_size; i++)
>> + ((struct freq_clip_table *)&tab_ptr[i])->mask_val = mask_val;
>> +
>> + ret = get_idr(&cpufreq_idr, &cooling_cpufreq_lock, &cpufreq_dev->id);
>
> hm, "get_idr" is a poor name. One would expect it to do a lookup, but
> it actually does an installation. That's a result of the poorly-named
> idr_get_new(), I expect.
>
>
>> + if (ret) {
>> + kfree(cpufreq_dev);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + sprintf(dev_name, "thermal-cpufreq-%d", cpufreq_dev->id);
>> +
>> + cool_dev = thermal_cooling_device_register(dev_name, cpufreq_dev,
>> + &cpufreq_cooling_ops);
>> + if (!cool_dev) {
>> + release_idr(&cpufreq_idr, &cooling_cpufreq_lock,
>> + cpufreq_dev->id);
>> + kfree(cpufreq_dev);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + cpufreq_dev->id = id;
>> + cpufreq_dev->cool_dev = cool_dev;
>> + mutex_lock(&cooling_cpufreq_lock);
>> + list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list);
>> + mutex_unlock(&cooling_cpufreq_lock);
>> +
>> + /*Register the notifier for first cpufreq cooling device*/
>> + if (cpufreq_dev_count == 0)
>> + cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
>> + CPUFREQ_POLICY_NOTIFIER);
>> + return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +
>> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>> +{
>> + struct cpufreq_cooling_device *cpufreq_dev = NULL;
>> + unsigned int cpufreq_dev_count = 0;
>> +
>> + mutex_lock(&cooling_cpufreq_lock);
>> + list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
>> + if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
>> + break;
>> + cpufreq_dev_count++;
>> + }
>> +
>> + if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
>> + mutex_unlock(&cooling_cpufreq_lock);
>> + return;
>> + }
>> +
>> + list_del(&cpufreq_dev->node);
>> + mutex_unlock(&cooling_cpufreq_lock);
>> +
>> + /*Unregister the notifier for the last cpufreq cooling device*/
>> + if (cpufreq_dev_count == 1) {
>
> But we dropped the lock, so local variable cpufreq_dev_count is now
> meaningless. What prevents a race here?
Yes lock can be extended to include it.
>
>> + cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
>> + CPUFREQ_POLICY_NOTIFIER);
>> + kfree(notify_table);
>> + }
>> +
>> + thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
>> + release_idr(&cpufreq_idr, &cooling_cpufreq_lock, cpufreq_dev->id);
>> + kfree(cpufreq_dev);
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>>
>> ...
>>
>> +struct freq_clip_table {
>> + unsigned int freq_clip_max;
>> + unsigned int polling_interval;
>> + unsigned int temp_level;
>> + const struct cpumask *mask_val;
>> +};
>
> hm, what does this thing do. Needs a nice comment for the uninitiated,
> please. Something which describes the overall roles, responsibilities
> and general reasons for existence.
Ok
>
>> +int cputherm_register_notifier(struct notifier_block *nb, unsigned int list);
>> +int cputherm_unregister_notifier(struct notifier_block *nb, unsigned int list);
>> +
>> +#ifdef CONFIG_CPU_FREQ
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> + struct freq_clip_table *tab_ptr, unsigned int tab_size,
>> + const struct cpumask *mask_val);
>> +
>> +void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
>> +#else /*!CONFIG_CPU_FREQ*/
>
> (more whacky comment layout)
>
>>
>> ...
>>
>
^ permalink raw reply
* Re: [PATCH v3 5/6] thermal: exynos: Register the tmu sensor with the kernel thermal layer
From: Amit Kachhap @ 2012-05-09 9:26 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-pm, durgadoss.r, linux-acpi, lenb, rui.zhang, linaro-dev,
linux-kernel, linux-arm-kernel, linux-samsung-soc, patches
In-Reply-To: <20120508131637.a2f59904.akpm@linux-foundation.org>
On 9 May 2012 01:46, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 8 May 2012 21:48:17 +0530
> Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>
>> This code added creates a link between temperature sensors, linux thermal
>> framework and cooling devices for samsung exynos platform. This layer
>> monitors the temperature from the sensor and informs the generic thermal
>> layer to take the necessary cooling action.
>>
>>
>> ...
>>
>> +static void exynos_report_trigger(void)
>> +{
>> + unsigned int i;
>> + char data[2];
>> + char *envp[] = { data, NULL };
>> +
>> + if (!th_zone || !th_zone->therm_dev)
>> + return;
>> +
>> + thermal_zone_device_update(th_zone->therm_dev);
>> +
>> + mutex_lock(&th_zone->therm_dev->lock);
>> + /* Find the level for which trip happened */
>> + for (i = 0; i < th_zone->sensor_conf->trip_data.trip_count; i++) {
>> + if (th_zone->therm_dev->last_temperature <
>> + th_zone->sensor_conf->trip_data.trip_val[i] * 1000)
>> + break;
>> + }
>> +
>> + if (th_zone->mode == THERMAL_DEVICE_ENABLED) {
>> + if (i > 0)
>> + th_zone->therm_dev->polling_delay = ACTIVE_INTERVAL;
>> + else
>> + th_zone->therm_dev->polling_delay = IDLE_INTERVAL;
>> + }
>> +
>> + sprintf(data, "%u", i);
>
> yikes, if `i' exceeds 9, we have a stack scribble. Please review this
> and at least use snprintf(... sizeof(data)) to prevent accidents.
Ok
>
>
>> + kobject_uevent_env(&th_zone->therm_dev->device.kobj, KOBJ_CHANGE, envp);
>> + mutex_unlock(&th_zone->therm_dev->lock);
>> +}
>> +
>>
>> ...
>>
>> +/* Get trip temperature callback functions for thermal zone */
>> +static int exynos_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>> + unsigned long *temp)
>> +{
>> + if (trip < 0 || trip > 2)
>
> I don't know what `trip' does and I don't know the meaning of the
> values 0, 1 and 2. Documentation, please.
Agreed will put their description.
>
>> + return -EINVAL;
>> +
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
>> + /* convert the temperature into millicelsius */
>> + *temp = *temp * 1000;
>> +
>> + return 0;
>> +}
>> +
>>
>> ...
>>
>> +/* Bind callback functions for thermal zone */
>> +static int exynos_bind(struct thermal_zone_device *thermal,
>> + struct thermal_cooling_device *cdev)
>> +{
>> + int ret = 0;
>> +
>> + /* if the cooling device is the one from exynos4 bind it */
>> + if (cdev != th_zone->cool_dev[0])
>> + return 0;
>> +
>> + if (thermal_zone_bind_cooling_device(thermal, 0, cdev)) {
>> + pr_err("error binding cooling dev inst 0\n");
>> + return -EINVAL;
>> + }
>> + if (thermal_zone_bind_cooling_device(thermal, 1, cdev)) {
>> + pr_err("error binding cooling dev inst 1\n");
>> + ret = -EINVAL;
>> + goto error_bind1;
>> + }
>
> There can never be more than two instances?
As of now it is fixed as we register only 2 zones
>
>> + return ret;
>> +error_bind1:
>> + thermal_zone_unbind_cooling_device(thermal, 0, cdev);
>> + return ret;
>> +}
>> +
>>
>> ...
>>
>> +/* Get temperature callback functions for thermal zone */
>> +static int exynos_get_temp(struct thermal_zone_device *thermal,
>> + unsigned long *temp)
>> +{
>> + void *data;
>> +
>> + if (!th_zone->sensor_conf) {
>> + pr_info("Temperature sensor not initialised\n");
>> + return -EINVAL;
>> + }
>> + data = th_zone->sensor_conf->private_data;
>> + *temp = th_zone->sensor_conf->read_temperature(data);
>> + /* convert the temperature into millicelsius */
>> + *temp = *temp * 1000;
>> + return 0;
>> +}
>> +
>> +/* Operation callback functions for thermal zone */
>> +static struct thermal_zone_device_ops exynos_dev_ops = {
>
> Can it be const? That sometimes saves space, as the table doesn't need
> to be moved into writeable storage at runtime.
Yes it can be made const
>
>> + .bind = exynos_bind,
>> + .unbind = exynos_unbind,
>> + .get_temp = exynos_get_temp,
>> + .get_mode = exynos_get_mode,
>> + .set_mode = exynos_set_mode,
>> + .get_trip_type = exynos_get_trip_type,
>> + .get_trip_temp = exynos_get_trip_temp,
>> + .get_crit_temp = exynos_get_crit_temp,
>> +};
>> +
>> +/* Register with the in-kernel thermal management */
>> +static int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
>> +{
>> + int ret, count, tab_size;
>> + struct freq_clip_table *tab_ptr;
>> +
>> + if (!sensor_conf || !sensor_conf->read_temperature) {
>> + pr_err("Temperature sensor not initialised\n");
>> + return -EINVAL;
>> + }
>> +
>> + th_zone = kzalloc(sizeof(struct exynos_thermal_zone), GFP_KERNEL);
>> + if (!th_zone) {
>> + ret = -ENOMEM;
>> + goto err_unregister;
>
> This seems wrong? If we need to call exynos_unregister_thermal() on
> this error path then we needed to call it on the predecing one?
> Perhaps?
ok my fault.
>
>> + }
>> +
>> + th_zone->sensor_conf = sensor_conf;
>> +
>> + tab_ptr = (struct freq_clip_table *)sensor_conf->cooling_data.freq_data;
>> + tab_size = sensor_conf->cooling_data.freq_clip_count;
>> +
>> + /* Register the cpufreq cooling device */
>> + th_zone->cool_dev_size = 1;
>> + count = 0;
>> + th_zone->cool_dev[count] = cpufreq_cooling_register(
>> + (struct freq_clip_table *)&(tab_ptr[count]),
>> + tab_size, cpumask_of(0));
>> +
>> + if (IS_ERR(th_zone->cool_dev[count])) {
>> + pr_err("Failed to register cpufreq cooling device\n");
>> + ret = -EINVAL;
>> + th_zone->cool_dev_size = 0;
>> + goto err_unregister;
>> + }
>> +
>> + th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
>> + 3, NULL, &exynos_dev_ops, 0, 0, 0, IDLE_INTERVAL);
>> +
>> + if (IS_ERR(th_zone->therm_dev)) {
>> + pr_err("Failed to register thermal zone device\n");
>> + ret = -EINVAL;
>> + goto err_unregister;
>> + }
>> + th_zone->mode = THERMAL_DEVICE_ENABLED;
>> +
>> + pr_info("Exynos: Kernel Thermal management registered\n");
>> +
>> + return 0;
>> +
>> +err_unregister:
>> + exynos_unregister_thermal();
>> + return ret;
>> +}
>> +
>>
>> ...
>>
>
--
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: [PATCH v3 0/6] thermal: exynos: Add kernel thermal support for exynos platform
From: Amit Kachhap @ 2012-05-09 12:44 UTC (permalink / raw)
To: Zhang, Rui
Cc: akpm@linux-foundation.org, linux-pm@lists.linux-foundation.org,
R, Durgadoss, linux-acpi@vger.kernel.org, lenb@kernel.org,
linaro-dev@lists.linaro.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, patches@linaro.org
In-Reply-To: <744357E9AAD1214791ACBA4B0B90926310025F@SHSMSX101.ccr.corp.intel.com>
On 9 May 2012 01:36, Zhang, Rui <rui.zhang@intel.com> wrote:
> Hi, Amit,
>
> Sorry for the late response as I'm in a travel recently.
>
> I think the generic cpufreq cooling patches are good.
>
> But about the THERMAL_TRIP_STATE_INSTANCE patch, what I'd like to see is that
> 1. from thermal zone point of view, when the temperature is higher than a trip point, either ACTIVE or PASSIVE, what we should do is to set device cooling state to cur_state+1, right?
> The only difference is that if we should take passive cooling actions or active cooling actions based on the policy.
> So my question would be if it is possible to combine these two kind of trip points together. Maybe something like this:
>
> In thermal_zone_device_update(),
>
> ...
> case THERMAL_TRIP_PASSIVE:
> if (passive cooling not allowed)
> continue;
> if (tc1)
> thermal_zone_device_passive();
> else
> thermal_set_cooling_state();
> break;
> case THERMAL_TRIP_ACTIVE:
> if (active cooling not allowed)
> continue;
> thermal_set_cooling_state();
> break;
> ...
>
> and thermal_set_cooling_state() {
> list_for_each_entry(instance, &tz->cooling_devices, node) {
> if (instance->trip != count)
> continue;
>
> cdev = instance->cdev;
>
> if (temp >= trip_temp)
> cdev->ops->set_cur_state(cdev, 1);
> else
> cdev->ops->set_cur_state(cdev, 0);
> }
> }
>
> 2. use only one cooling_device instance for a thermal_zone, and introduce cdev->trips which shows the trip points this cooling device is bind to.
> And we may use multiple cooling devices states for one active trip point.
>
> Then, thermal_set_cooling_state() would be look like
>
> list_for_each_entry(instance, &tz->cooling_devices, node) {
> cdev = instance->cdev;
> /* check whether this cooling device is bind to the trip point */
> if (!(cdev->trips & 1<<count))
> continue;
> cdev->ops->get_max_state(cdev, &max_state);
> cdev->ops->get_cur_state(cdev, &cur_state);
>
> if (temp >= trip_temp) {
> if (cur_state++ <= max_state))
> cdev->ops->set_cur_state(cdev, cur_state);
> } else if ((temp < trip_temp) && (cur_state-- >= 0))
> cdev->ops->set_cur_state(cdev, cur_state);
> }
> }
Hi Rui,
The above implementation also cools instance based cooling devices
like passive trip type. I need some changes on top of your
implementation such as,
thermal_set_cooling_state() {
list_for_each_entry(instance, &tz->cooling_devices,
node) {
cdev = instance->cdev;
if (!cdev->trips & 1<<count)
continue;
inst_id = 0;
for_each_bit_set(i, cdev->trips, count)
inst_id++;
cdev->ops->get_max_state(cdev, &max_state);
if ((temp >= trip_temp) && (inst_id <= max_state))
cdev->ops->set_cur_state(cdev, inst_id);
else if ((temp < trip_temp) && (--inst_id >= 0))
cdev->ops->set_cur_state(cdev, inst_id);
}
}
I agree with you that the instance based trip types violates the
concept like reading the cur_state and do cur_state++/cur_state--
depending upon threshold increase or decrease because it needs the
state_id/inst_id.
I am actually thinking of dropping this new trip type and use the
existing THERMAL_TRIP_ACTIVE because there is so much logic in
calculating the state_id. The only flip side of using TRIP_ACTIVE is
that I need to create so many cooling devices.
Thanks,
Amit D
>
> With these two things, I think the WARN_ZONE AND MONITOR_ZONE can be registered as two PASSIVE trip points in the generic thermal layer, right?
> Actually, this is one thing in my TODO list. And I'd glad to make it high priority if this solves the problem you have.
>
> Thanks,
> rui
>
> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Amit Daniel Kachhap
> Sent: Tuesday, May 08, 2012 9:18 AM
> To: akpm@linux-foundation.org; linux-pm@lists.linux-foundation.org
> Cc: R, Durgadoss; linux-acpi@vger.kernel.org; lenb@kernel.org; Zhang, Rui; amit.kachhap@linaro.org; linaro-dev@lists.linaro.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; patches@linaro.org
> Subject: [PATCH v3 0/6] thermal: exynos: Add kernel thermal support for exynos platform
> Importance: High
>
> Hi Andrew,
>
> This patchset introduces a new generic cooling device based on cpufreq that can be used on non-ACPI platforms. As a proof of concept, we have drivers for the following platforms using this mechanism now:
>
> * TI OMAP (git://git.linaro.org/people/amitdanielk/linux.git omap4460_thermal)
> * Samsung Exynos (Exynos4 and Exynos5) in the current patchset.
> * Freescale i.MX (git://git.linaro.org/people/amitdanielk/linux.git imx6q_thermal)
>
> These patches have been reviewed by Rui Zhang (https://lkml.org/lkml/2012/4/9/448)
> who seems to agree with them in principle, but I haven't had any luck getting them merged, perhaps a lack of maintainer bandwidth.
>
> ACPI platforms currently have such a mechanism but it is wrapped in ACPI'isms that we don't have on ARM platforms. If this is accepted, I'm proposing to convert over the ACPI thermal driver to use this common code too.
>
> Can you please merge these patches for 3.5?
>
> Thanks,
> Amit Daniel
>
>
> Changes since V2:
> *Added Exynos5 TMU sensor support by enhancing the exynos4 tmu driver. Exynos5 TMU driver was internally developed by SangWook Ju <sw.ju@samsung.com>.
> *Removed cpuhotplug cooling code in this patchset.
> *Rebased the patches against 3.4-rc6 kernel.
>
> Changes since V1:
> *Moved the sensor driver to driver/thermal folder from driver/hwmon folder as suggested by Mark Brown and Guenter Roeck *Added notifier support to notify the registered drivers of any cpu cooling action. The driver can modify the default cooling behaviour(eg set different max clip frequency).
> *The percentage based frequency replaced with absolute clipped frequency.
> *Some more conditional checks when setting max frequency.
> *Renamed the new trip type THERMAL_TRIP_STATE_ACTIVE to THERMAL_TRIP_STATE_INSTANCE *Many review comments from R, Durgadoss <durgadoss.r@intel.com> and eduardo.valentin@ti.com implemented.
> *Removed cooling stats through debugfs patch *The V1 based can be found here,
> https://lkml.org/lkml/2012/2/22/123
> http://lkml.org/lkml/2012/3/3/32
>
> Changes since RFC:
> *Changed the cpu cooling registration/unregistration API's to instance based *Changed the STATE_ACTIVE trip type to pass correct instance id *Adding support to restore back the policy->max_freq after doing frequency
> clipping.
> *Moved the trip cooling stats from sysfs node to debugfs node as suggested
> by Greg KH greg@kroah.com
> *Incorporated several review comments from eduardo.valentin@ti.com *Moved the Temperature sensor driver from driver/hwmon/ to driver/mfd as discussed with Guenter Roeck <guenter.roeck@ericsson.com> and Donggeun Kim <dg77.kim@samsung.com> (https://lkml.org/lkml/2012/1/5/7)
> *Some changes according to the changes in common cpu cooling APIs *The RFC based patches can be found here,
> https://lkml.org/lkml/2011/12/13/186
> https://lkml.org/lkml/2011/12/21/169
>
>
> Brief Description:
>
> 1) The generic cooling devices code is placed inside driver/thermal/* as placing inside acpi folder will need un-necessary enabling of acpi code. This codes is architecture independent.
>
> 2) This patchset adds a new trip type THERMAL_TRIP_STATE_INSTANCE which passes cooling device instance number and may be helpful for cpufreq cooling devices to take the correct cooling action. This trip type avoids the temperature comparision check again inside the cooling handler.
>
> 3) This patchset adds generic cpu cooling low level implementation through frequency clipping and cpu hotplug. In future, other cpu related cooling devices may be added here. An ACPI version of this already exists (drivers/acpi/processor_thermal.c). But this will be useful for platforms like ARM using the generic thermal interface along with the generic cpu cooling devices. The cooling device registration API's return cooling device pointers which can be easily binded with the thermal zone trip points.
> The important APIs exposed are,
> a)struct thermal_cooling_device *cpufreq_cooling_register(
> struct freq_clip_table *tab_ptr, unsigned int tab_size,
> const struct cpumask *mask_val)
> b)void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>
> 4) Samsung exynos platform thermal implementation is done using the generic cpu cooling APIs and the new trip type. The temperature sensor driver present in the hwmon folder(registered as hwmon driver) is moved to thermal folder and registered as a thermal driver.
>
> All this patchset is based on Kernel version 3.4-rc6
>
> A simple data/control flow diagrams is shown below,
>
> Core Linux thermal <-----> Exynos thermal interface <----- Temperature Sensor
> | |
> \|/ |
> Cpufreq cooling device <---------------
>
> TODO:
> *Will send the DT enablement patches later after the driver is merged.
>
>
> Amit Daniel Kachhap (6):
> thermal: Add a new trip type to use cooling device instance number
> thermal: Add generic cpufreq cooling implementation
> hwmon: exynos4: Move thermal sensor driver to driver/thermal
> directory
> thermal: exynos5: Add exynos5 thermal sensor driver support
> thermal: exynos: Register the tmu sensor with the kernel thermal
> layer
> ARM: exynos: Add thermal sensor driver platform data support
>
> Documentation/hwmon/exynos4_tmu | 81 ---
> Documentation/thermal/cpu-cooling-api.txt | 60 ++
> Documentation/thermal/exynos_thermal | 52 ++
> Documentation/thermal/sysfs-api.txt | 4 +-
> drivers/hwmon/Kconfig | 10 -
> drivers/hwmon/Makefile | 1 -
> drivers/hwmon/exynos4_tmu.c | 514 --------------
> drivers/thermal/Kconfig | 20 +
> drivers/thermal/Makefile | 4 +-
> drivers/thermal/cpu_cooling.c | 359 ++++++++++
> drivers/thermal/exynos_thermal.c | 933 ++++++++++++++++++++++++++
> drivers/thermal/thermal_sys.c | 62 ++-
> include/linux/cpu_cooling.h | 62 ++
> include/linux/platform_data/exynos4_tmu.h | 83 ---
> include/linux/platform_data/exynos_thermal.h | 100 +++
> include/linux/thermal.h | 1 +
> 16 files changed, 1651 insertions(+), 695 deletions(-) delete mode 100644 Documentation/hwmon/exynos4_tmu create mode 100644 Documentation/thermal/cpu-cooling-api.txt
> create mode 100644 Documentation/thermal/exynos_thermal
> delete mode 100644 drivers/hwmon/exynos4_tmu.c create mode 100644 drivers/thermal/cpu_cooling.c create mode 100644 drivers/thermal/exynos_thermal.c create mode 100644 include/linux/cpu_cooling.h delete mode 100644 include/linux/platform_data/exynos4_tmu.h
> create mode 100644 include/linux/platform_data/exynos_thermal.h
>
> --
> 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] [PATCH V3 00/10] PM: Create the AVS(Adaptive Voltage Scaling)
From: Kevin Hilman @ 2012-05-09 18:29 UTC (permalink / raw)
To: Woodruff, Richard
Cc: J, KEERTHY, Mark Brown, linux-kernel@vger.kernel.org,
AnilKumar, Chimata, linux-pm@lists.linux-foundation.org,
linux-omap@vger.kernel.org, Pihet-XID, Jean,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <EF62F09C0797D947AD4180A1043C0DF73C40C247@DLEE10.ent.ti.com>
"Woodruff, Richard" <r-woodruff2@ti.com> writes:
>> From: Hilman, Kevin
>> Sent: Tuesday, May 08, 2012 5:17 PM
>
>> A basic OMAP AVS driver has been in mainline for a long time, yet we
>> have not seen support submitted for all of these features.
>
> 1.5/3.5 is a feature.
And I'm still waiting for it to be submitted upstream.
> ABB is requirement for a production useable driver. Higher speed rated
> OMAP4 and all OMAP5 added these to be useable.
ditto
> Yes this is effort. Point of mentioning is to raise awareness of need.
I'm well aware of the need.
> Yet to be added feature has different meaning than functional gap.
And both need to be submitted upstream.
Kevin
^ permalink raw reply
* Re: Plumbers: PM constraints micro-conf RFP
From: Kevin Hilman @ 2012-05-09 18:35 UTC (permalink / raw)
To: markgross
Cc: Agarwal, Ramesh, paulmck, Praveen Chidambaram, Amit Kucheria,
John Stultz, linux-pm, Antti P Miettinen, jeen.pihet
In-Reply-To: <20120502143607.GA21640@G62>
Hi Mark,
mark gross <markgross@thegnar.org> writes:
> This is a Request For Participation in a micro-conference at this years
> Linux plumbers event. For this micro conference to happen we need to
> reach a certain critical mass WRT participants as measured by submitted
> talks associated to Power Management Constraints.
>
> The To: list is populated with folks that I've had interactions with
> over extending pm-qos or constraint based PM over the past year.
>
> If you are working on problems related to constraining the power /
> performance of devices I am inviting your participation and request you
> submit a proposal for presenting your problem space (preferred) and or
> solution to a group of developers looking for a good solution to push
> upstream. The talks will be about 20 min long as I want to get into
> some design and implementation discussions after the requirements
> definition is mostly finished.
Thanks for organizing this.
I won't be at Plumbers this year (I'll be on vacation), but will
certainly participate by email as I have an interest in these topics.
I've added Jean Pihet to Cc as he is also working on QoS/constraints
related topics.
Kevin
^ permalink raw reply
* Re: Plumbers: PM constraints micro-conf RFP
From: Kevin Hilman @ 2012-05-09 18:54 UTC (permalink / raw)
To: markgross
Cc: Agarwal, Ramesh, paulmck, Praveen Chidambaram, Amit Kucheria,
John Stultz, linux-pm, Antti P Miettinen, jeen.pihet
In-Reply-To: <20120502143607.GA21640@G62>
Hi Mark,
mark gross <markgross@thegnar.org> writes:
> This is a Request For Participation in a micro-conference at this years
> Linux plumbers event. For this micro conference to happen we need to
> reach a certain critical mass WRT participants as measured by submitted
> talks associated to Power Management Constraints.
>
> The To: list is populated with folks that I've had interactions with
> over extending pm-qos or constraint based PM over the past year.
>
> If you are working on problems related to constraining the power /
> performance of devices I am inviting your participation and request you
> submit a proposal for presenting your problem space (preferred) and or
> solution to a group of developers looking for a good solution to push
> upstream. The talks will be about 20 min long as I want to get into
> some design and implementation discussions after the requirements
> definition is mostly finished.
Thanks for organizing this.
I won't be at Plumbers this year (I'll be on vacation), but will
certainly participate by email as I have an interest in these topics.
I've added Jean Pihet to Cc as he is also working on QoS/constraints
related topics.
Kevin
^ permalink raw reply
* Re: [PATCHv4 1/4] cpuidle: refactor out cpuidle_enter_state
From: Rafael J. Wysocki @ 2012-05-09 21:03 UTC (permalink / raw)
To: Colin Cross
Cc: Kevin Hilman, Len Brown, Russell King, Greg Kroah-Hartman,
Kay Sievers, linux-kernel, Amit Kucheria, linux-pm,
Arjan van de Ven, Arnd Bergmann, linux-arm-kernel
In-Reply-To: <1336438662-10484-2-git-send-email-ccross@android.com>
On Tuesday, May 08, 2012, Colin Cross wrote:
> Split the code to enter a state and update the stats into a helper
> function, cpuidle_enter_state, and export it. This function will
> be called by the coupled state code to handle entering the safe
> state and the final coupled state.
>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Colin Cross <ccross@android.com>
I've said
Reviewed-by: Rafael J. Wysocki <rjw@sisk.pl>
to this already. Any chance to update the tags?
Rafael
> ---
> drivers/cpuidle/cpuidle.c | 42 +++++++++++++++++++++++++++++-------------
> drivers/cpuidle/cpuidle.h | 2 ++
> 2 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 2f0083a..3e3e3e4 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -103,6 +103,34 @@ int cpuidle_play_dead(void)
> }
>
> /**
> + * cpuidle_enter_state - enter the state and update stats
> + * @dev: cpuidle device for this cpu
> + * @drv: cpuidle driver for this cpu
> + * @next_state: index into drv->states of the state to enter
> + */
> +int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> + int next_state)
> +{
> + int entered_state;
> +
> + entered_state = cpuidle_enter_ops(dev, drv, next_state);
> +
> + if (entered_state >= 0) {
> + /* Update cpuidle counters */
> + /* This can be moved to within driver enter routine
> + * but that results in multiple copies of same code.
> + */
> + dev->states_usage[entered_state].time +=
> + (unsigned long long)dev->last_residency;
> + dev->states_usage[entered_state].usage++;
> + } else {
> + dev->last_residency = 0;
> + }
> +
> + return entered_state;
> +}
> +
> +/**
> * cpuidle_idle_call - the main idle loop
> *
> * NOTE: no locks or semaphores should be used here
> @@ -143,23 +171,11 @@ int cpuidle_idle_call(void)
> trace_power_start_rcuidle(POWER_CSTATE, next_state, dev->cpu);
> trace_cpu_idle_rcuidle(next_state, dev->cpu);
>
> - entered_state = cpuidle_enter_ops(dev, drv, next_state);
> + entered_state = cpuidle_enter_state(dev, drv, next_state);
>
> trace_power_end_rcuidle(dev->cpu);
> trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
>
> - if (entered_state >= 0) {
> - /* Update cpuidle counters */
> - /* This can be moved to within driver enter routine
> - * but that results in multiple copies of same code.
> - */
> - dev->states_usage[entered_state].time +=
> - (unsigned long long)dev->last_residency;
> - dev->states_usage[entered_state].usage++;
> - } else {
> - dev->last_residency = 0;
> - }
> -
> /* give the governor an opportunity to reflect on the outcome */
> if (cpuidle_curr_governor->reflect)
> cpuidle_curr_governor->reflect(dev, entered_state);
> diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
> index 7db1866..d8a3ccc 100644
> --- a/drivers/cpuidle/cpuidle.h
> +++ b/drivers/cpuidle/cpuidle.h
> @@ -14,6 +14,8 @@
> extern struct mutex cpuidle_lock;
> extern spinlock_t cpuidle_driver_lock;
> extern int cpuidle_disabled(void);
> +extern int cpuidle_enter_state(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int next_state);
>
> /* idle loop */
> extern void cpuidle_install_idle_handler(void);
>
^ permalink raw reply
* Re: [PATCHv4 2/4] cpuidle: fix error handling in __cpuidle_register_device
From: Rafael J. Wysocki @ 2012-05-09 21:04 UTC (permalink / raw)
To: Colin Cross
Cc: Kevin Hilman, Len Brown, Russell King, Greg Kroah-Hartman,
Kay Sievers, linux-kernel, Amit Kucheria, linux-pm,
Arjan van de Ven, Arnd Bergmann, linux-arm-kernel
In-Reply-To: <1336438662-10484-3-git-send-email-ccross@android.com>
On Tuesday, May 08, 2012, Colin Cross wrote:
> Fix the error handling in __cpuidle_register_device to include
> the missing list_del. Move it to a label, which will simplify
> the error handling when coupled states are added.
>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Colin Cross <ccross@android.com>
I've said
Reviewed-by: Rafael J. Wysocki <rjw@sisk.pl>
to this already too.
Rafael
> ---
> drivers/cpuidle/cpuidle.c | 13 +++++++++----
> 1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 3e3e3e4..4540672 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -403,13 +403,18 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
>
> per_cpu(cpuidle_devices, dev->cpu) = dev;
> list_add(&dev->device_list, &cpuidle_detected_devices);
> - if ((ret = cpuidle_add_sysfs(cpu_dev))) {
> - module_put(cpuidle_driver->owner);
> - return ret;
> - }
> + ret = cpuidle_add_sysfs(cpu_dev);
> + if (ret)
> + goto err_sysfs;
>
> dev->registered = 1;
> return 0;
> +
> +err_sysfs:
> + list_del(&dev->device_list);
> + per_cpu(cpuidle_devices, dev->cpu) = NULL;
> + module_put(cpuidle_driver->owner);
> + return ret;
> }
>
> /**
>
^ permalink raw reply
* Re: [PATCHv4 3/4] cpuidle: add support for states that affect multiple cpus
From: Rafael J. Wysocki @ 2012-05-09 21:19 UTC (permalink / raw)
To: Colin Cross
Cc: Kevin Hilman, Len Brown, Russell King, Greg Kroah-Hartman,
Kay Sievers, linux-kernel, Amit Kucheria, linux-pm,
Arjan van de Ven, Arnd Bergmann, linux-arm-kernel
In-Reply-To: <1336438662-10484-4-git-send-email-ccross@android.com>
On Tuesday, May 08, 2012, Colin Cross wrote:
> On some ARM SMP SoCs (OMAP4460, Tegra 2, and probably more), the
> cpus cannot be independently powered down, either due to
> sequencing restrictions (on Tegra 2, cpu 0 must be the last to
> power down), or due to HW bugs (on OMAP4460, a cpu powering up
> will corrupt the gic state unless the other cpu runs a work
> around). Each cpu has a power state that it can enter without
> coordinating with the other cpu (usually Wait For Interrupt, or
> WFI), and one or more "coupled" power states that affect blocks
> shared between the cpus (L2 cache, interrupt controller, and
> sometimes the whole SoC). Entering a coupled power state must
> be tightly controlled on both cpus.
>
> The easiest solution to implementing coupled cpu power states is
> to hotplug all but one cpu whenever possible, usually using a
> cpufreq governor that looks at cpu load to determine when to
> enable the secondary cpus. This causes problems, as hotplug is an
> expensive operation, so the number of hotplug transitions must be
> minimized, leading to very slow response to loads, often on the
> order of seconds.
>
> This file implements an alternative solution, where each cpu will
> wait in the WFI state until all cpus are ready to enter a coupled
> state, at which point the coupled state function will be called
> on all cpus at approximately the same time.
>
> Once all cpus are ready to enter idle, they are woken by an smp
> cross call. At this point, there is a chance that one of the
> cpus will find work to do, and choose not to enter idle. A
> final pass is needed to guarantee that all cpus will call the
> power state enter function at the same time. During this pass,
> each cpu will increment the ready counter, and continue once the
> ready counter matches the number of online coupled cpus. If any
> cpu exits idle, the other cpus will decrement their counter and
> retry.
>
> To use coupled cpuidle states, a cpuidle driver must:
>
> Set struct cpuidle_device.coupled_cpus to the mask of all
> coupled cpus, usually the same as cpu_possible_mask if all cpus
> are part of the same cluster. The coupled_cpus mask must be
> set in the struct cpuidle_device for each cpu.
>
> Set struct cpuidle_device.safe_state to a state that is not a
> coupled state. This is usually WFI.
>
> Set CPUIDLE_FLAG_COUPLED in struct cpuidle_state.flags for each
> state that affects multiple cpus.
>
> Provide a struct cpuidle_state.enter function for each state
> that affects multiple cpus. This function is guaranteed to be
> called on all cpus at approximately the same time. The driver
> should ensure that the cpus all abort together if any cpu tries
> to abort once the function is called.
>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Trinabh Gupta <g.trinabh@gmail.com>
> Cc: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Colin Cross <ccross@android.com>
This one looks good to me at first glance, so
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
I'll try to find some more time to review it more in depth later this week.
Thanks,
Rafael
> ---
> drivers/cpuidle/Kconfig | 3 +
> drivers/cpuidle/Makefile | 1 +
> drivers/cpuidle/coupled.c | 655 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/cpuidle/cpuidle.c | 15 +-
> drivers/cpuidle/cpuidle.h | 30 ++
> include/linux/cpuidle.h | 7 +
> 6 files changed, 710 insertions(+), 1 deletions(-)
> create mode 100644 drivers/cpuidle/coupled.c
>
> v2:
> * removed the coupled lock, replacing it with atomic counters
> * added a check for outstanding pokes before beginning the
> final transition to avoid extra wakeups
> * made the cpuidle_coupled struct completely private
> * fixed kerneldoc comment formatting
>
> v3:
> * fixed decrement in cpuidle_coupled_cpu_set_alive
> * added kerneldoc annotation to the description
>
> v4:
> * removed BUG_ONs
> * converted ready and waiting counts to a single atomic
> * prevent coupled idle during hotplug, simplifying alive_count
>
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index 78a666d..a76b689 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -18,3 +18,6 @@ config CPU_IDLE_GOV_MENU
> bool
> depends on CPU_IDLE && NO_HZ
> default y
> +
> +config ARCH_NEEDS_CPU_IDLE_COUPLED
> + def_bool n
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 5634f88..38c8f69 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -3,3 +3,4 @@
> #
>
> obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
> +obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> new file mode 100644
> index 0000000..93101fb
> --- /dev/null
> +++ b/drivers/cpuidle/coupled.c
> @@ -0,0 +1,655 @@
> +/*
> + * coupled.c - helper functions to enter the same idle state on multiple cpus
> + *
> + * Copyright (c) 2011 Google, Inc.
> + *
> + * Author: Colin Cross <ccross@android.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/cpu.h>
> +#include <linux/cpuidle.h>
> +#include <linux/mutex.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "cpuidle.h"
> +
> +/**
> + * DOC: Coupled cpuidle states
> + *
> + * On some ARM SMP SoCs (OMAP4460, Tegra 2, and probably more), the
> + * cpus cannot be independently powered down, either due to
> + * sequencing restrictions (on Tegra 2, cpu 0 must be the last to
> + * power down), or due to HW bugs (on OMAP4460, a cpu powering up
> + * will corrupt the gic state unless the other cpu runs a work
> + * around). Each cpu has a power state that it can enter without
> + * coordinating with the other cpu (usually Wait For Interrupt, or
> + * WFI), and one or more "coupled" power states that affect blocks
> + * shared between the cpus (L2 cache, interrupt controller, and
> + * sometimes the whole SoC). Entering a coupled power state must
> + * be tightly controlled on both cpus.
> + *
> + * This file implements a solution, where each cpu will wait in the
> + * WFI state until all cpus are ready to enter a coupled state, at
> + * which point the coupled state function will be called on all
> + * cpus at approximately the same time.
> + *
> + * Once all cpus are ready to enter idle, they are woken by an smp
> + * cross call. At this point, there is a chance that one of the
> + * cpus will find work to do, and choose not to enter idle. A
> + * final pass is needed to guarantee that all cpus will call the
> + * power state enter function at the same time. During this pass,
> + * each cpu will increment the ready counter, and continue once the
> + * ready counter matches the number of online coupled cpus. If any
> + * cpu exits idle, the other cpus will decrement their counter and
> + * retry.
> + *
> + * requested_state stores the deepest coupled idle state each cpu
> + * is ready for. It is assumed that the states are indexed from
> + * shallowest (highest power, lowest exit latency) to deepest
> + * (lowest power, highest exit latency). The requested_state
> + * variable is not locked. It is only written from the cpu that
> + * it stores (or by the on/offlining cpu if that cpu is offline),
> + * and only read after all the cpus are ready for the coupled idle
> + * state are are no longer updating it.
> + *
> + * Three atomic counters are used. alive_count tracks the number
> + * of cpus in the coupled set that are currently or soon will be
> + * online. waiting_count tracks the number of cpus that are in
> + * the waiting loop, in the ready loop, or in the coupled idle state.
> + * ready_count tracks the number of cpus that are in the ready loop
> + * or in the coupled idle state.
> + *
> + * To use coupled cpuidle states, a cpuidle driver must:
> + *
> + * Set struct cpuidle_device.coupled_cpus to the mask of all
> + * coupled cpus, usually the same as cpu_possible_mask if all cpus
> + * are part of the same cluster. The coupled_cpus mask must be
> + * set in the struct cpuidle_device for each cpu.
> + *
> + * Set struct cpuidle_device.safe_state to a state that is not a
> + * coupled state. This is usually WFI.
> + *
> + * Set CPUIDLE_FLAG_COUPLED in struct cpuidle_state.flags for each
> + * state that affects multiple cpus.
> + *
> + * Provide a struct cpuidle_state.enter function for each state
> + * that affects multiple cpus. This function is guaranteed to be
> + * called on all cpus at approximately the same time. The driver
> + * should ensure that the cpus all abort together if any cpu tries
> + * to abort once the function is called. The function should return
> + * with interrupts still disabled.
> + */
> +
> +/**
> + * struct cpuidle_coupled - data for set of cpus that share a coupled idle state
> + * @coupled_cpus: mask of cpus that are part of the coupled set
> + * @requested_state: array of requested states for cpus in the coupled set
> + * @ready_waiting_counts: combined count of cpus in ready or waiting loops
> + * @online_count: count of cpus that are online
> + * @refcnt: reference count of cpuidle devices that are using this struct
> + * @prevent: flag to prevent coupled idle while a cpu is hotplugging
> + */
> +struct cpuidle_coupled {
> + cpumask_t coupled_cpus;
> + int requested_state[NR_CPUS];
> + atomic_t ready_waiting_counts;
> + int online_count;
> + int refcnt;
> + int prevent;
> +};
> +
> +#define WAITING_BITS 16
> +#define MAX_WAITING_CPUS (1 << WAITING_BITS)
> +#define WAITING_MASK (MAX_WAITING_CPUS - 1)
> +#define READY_MASK (~WAITING_MASK)
> +
> +#define CPUIDLE_COUPLED_NOT_IDLE (-1)
> +
> +static DEFINE_MUTEX(cpuidle_coupled_lock);
> +static DEFINE_PER_CPU(struct call_single_data, cpuidle_coupled_poke_cb);
> +
> +/*
> + * The cpuidle_coupled_poked_mask mask is used to avoid calling
> + * __smp_call_function_single with the per cpu call_single_data struct already
> + * in use. This prevents a deadlock where two cpus are waiting for each others
> + * call_single_data struct to be available
> + */
> +static cpumask_t cpuidle_coupled_poked_mask;
> +
> +/**
> + * cpuidle_state_is_coupled - check if a state is part of a coupled set
> + * @dev: struct cpuidle_device for the current cpu
> + * @drv: struct cpuidle_driver for the platform
> + * @state: index of the target state in drv->states
> + *
> + * Returns true if the target state is coupled with cpus besides this one
> + */
> +bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int state)
> +{
> + return drv->states[state].flags & CPUIDLE_FLAG_COUPLED;
> +}
> +
> +/**
> + * cpuidle_coupled_set_ready - mark a cpu as ready
> + * @coupled: the struct coupled that contains the current cpu
> + */
> +static inline void cpuidle_coupled_set_ready(struct cpuidle_coupled *coupled)
> +{
> + atomic_add(MAX_WAITING_CPUS, &coupled->ready_waiting_counts);
> +}
> +
> +/**
> + * cpuidle_coupled_set_not_ready - mark a cpu as not ready
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Decrements the ready counter, unless the ready (and thus the waiting) counter
> + * is equal to the number of online cpus. Prevents a race where one cpu
> + * decrements the waiting counter and then re-increments it just before another
> + * cpu has decremented its ready counter, leading to the ready counter going
> + * down from the number of online cpus without going through the coupled idle
> + * state.
> + *
> + * Returns 0 if the counter was decremented successfully, -EINVAL if the ready
> + * counter was equal to the number of online cpus.
> + */
> +static
> +inline int cpuidle_coupled_set_not_ready(struct cpuidle_coupled *coupled)
> +{
> + int all;
> + int ret;
> +
> + all = coupled->online_count || (coupled->online_count << WAITING_BITS);
> + ret = atomic_add_unless(&coupled->ready_waiting_counts,
> + -MAX_WAITING_CPUS, all);
> +
> + return ret ? 0 : -EINVAL;
> +}
> +
> +/**
> + * cpuidle_coupled_no_cpus_ready - check if no cpus in a coupled set are ready
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Returns true if all of the cpus in a coupled set are out of the ready loop.
> + */
> +static inline int cpuidle_coupled_no_cpus_ready(struct cpuidle_coupled *coupled)
> +{
> + int r = atomic_read(&coupled->ready_waiting_counts) >> WAITING_BITS;
> + return r == 0;
> +}
> +
> +/**
> + * cpuidle_coupled_cpus_ready - check if all cpus in a coupled set are ready
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Returns true if all cpus coupled to this target state are in the ready loop
> + */
> +static inline bool cpuidle_coupled_cpus_ready(struct cpuidle_coupled *coupled)
> +{
> + int r = atomic_read(&coupled->ready_waiting_counts) >> WAITING_BITS;
> + return r == coupled->online_count;
> +}
> +
> +/**
> + * cpuidle_coupled_cpus_waiting - check if all cpus in a coupled set are waiting
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Returns true if all cpus coupled to this target state are in the wait loop
> + */
> +static inline bool cpuidle_coupled_cpus_waiting(struct cpuidle_coupled *coupled)
> +{
> + int w = atomic_read(&coupled->ready_waiting_counts) & WAITING_MASK;
> + return w == coupled->online_count;
> +}
> +
> +/**
> + * cpuidle_coupled_no_cpus_waiting - check if no cpus in coupled set are waiting
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Returns true if all of the cpus in a coupled set are out of the waiting loop.
> + */
> +static inline int cpuidle_coupled_no_cpus_waiting(struct cpuidle_coupled *coupled)
> +{
> + int w = atomic_read(&coupled->ready_waiting_counts) & WAITING_MASK;
> + return w == 0;
> +}
> +
> +/**
> + * cpuidle_coupled_get_state - determine the deepest idle state
> + * @dev: struct cpuidle_device for this cpu
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Returns the deepest idle state that all coupled cpus can enter
> + */
> +static inline int cpuidle_coupled_get_state(struct cpuidle_device *dev,
> + struct cpuidle_coupled *coupled)
> +{
> + int i;
> + int state = INT_MAX;
> +
> + /*
> + * Read barrier ensures that read of requested_state is ordered after
> + * reads of ready_count. Matches the write barriers
> + * cpuidle_set_state_waiting.
> + */
> + smp_rmb();
> +
> + for_each_cpu_mask(i, coupled->coupled_cpus)
> + if (cpu_online(i) && coupled->requested_state[i] < state)
> + state = coupled->requested_state[i];
> +
> + return state;
> +}
> +
> +static void cpuidle_coupled_poked(void *info)
> +{
> + int cpu = (unsigned long)info;
> + cpumask_clear_cpu(cpu, &cpuidle_coupled_poked_mask);
> +}
> +
> +/**
> + * cpuidle_coupled_poke - wake up a cpu that may be waiting
> + * @cpu: target cpu
> + *
> + * Ensures that the target cpu exits it's waiting idle state (if it is in it)
> + * and will see updates to waiting_count before it re-enters it's waiting idle
> + * state.
> + *
> + * If cpuidle_coupled_poked_mask is already set for the target cpu, that cpu
> + * either has or will soon have a pending IPI that will wake it out of idle,
> + * or it is currently processing the IPI and is not in idle.
> + */
> +static void cpuidle_coupled_poke(int cpu)
> +{
> + struct call_single_data *csd = &per_cpu(cpuidle_coupled_poke_cb, cpu);
> +
> + if (!cpumask_test_and_set_cpu(cpu, &cpuidle_coupled_poked_mask))
> + __smp_call_function_single(cpu, csd, 0);
> +}
> +
> +/**
> + * cpuidle_coupled_poke_others - wake up all other cpus that may be waiting
> + * @dev: struct cpuidle_device for this cpu
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Calls cpuidle_coupled_poke on all other online cpus.
> + */
> +static void cpuidle_coupled_poke_others(int this_cpu,
> + struct cpuidle_coupled *coupled)
> +{
> + int cpu;
> +
> + for_each_cpu_mask(cpu, coupled->coupled_cpus)
> + if (cpu != this_cpu && cpu_online(cpu))
> + cpuidle_coupled_poke(cpu);
> +}
> +
> +/**
> + * cpuidle_coupled_set_waiting - mark this cpu as in the wait loop
> + * @dev: struct cpuidle_device for this cpu
> + * @coupled: the struct coupled that contains the current cpu
> + * @next_state: the index in drv->states of the requested state for this cpu
> + *
> + * Updates the requested idle state for the specified cpuidle device,
> + * poking all coupled cpus out of idle if necessary to let them see the new
> + * state.
> + */
> +static void cpuidle_coupled_set_waiting(int cpu,
> + struct cpuidle_coupled *coupled, int next_state)
> +{
> + int w;
> +
> + coupled->requested_state[cpu] = next_state;
> +
> + /*
> + * If this is the last cpu to enter the waiting state, poke
> + * all the other cpus out of their waiting state so they can
> + * enter a deeper state. This can race with one of the cpus
> + * exiting the waiting state due to an interrupt and
> + * decrementing waiting_count, see comment below.
> + *
> + * The atomic_inc_return provides a write barrier to order the write
> + * to requested_state with the later write that increments ready_count.
> + */
> + w = atomic_inc_return(&coupled->ready_waiting_counts) & WAITING_MASK;
> + if (w == coupled->online_count)
> + cpuidle_coupled_poke_others(cpu, coupled);
> +}
> +
> +/**
> + * cpuidle_coupled_set_not_waiting - mark this cpu as leaving the wait loop
> + * @dev: struct cpuidle_device for this cpu
> + * @coupled: the struct coupled that contains the current cpu
> + *
> + * Removes the requested idle state for the specified cpuidle device.
> + */
> +static void cpuidle_coupled_set_not_waiting(int cpu,
> + struct cpuidle_coupled *coupled)
> +{
> + /*
> + * Decrementing waiting count can race with incrementing it in
> + * cpuidle_coupled_set_waiting, but that's OK. Worst case, some
> + * cpus will increment ready_count and then spin until they
> + * notice that this cpu has cleared it's requested_state.
> + */
> + atomic_dec(&coupled->ready_waiting_counts);
> +
> + coupled->requested_state[cpu] = CPUIDLE_COUPLED_NOT_IDLE;
> +}
> +
> +/**
> + * cpuidle_coupled_clear_pokes - spin until the poke interrupt is processed
> + * @cpu - this cpu
> + *
> + * Turns on interrupts and spins until any outstanding poke interrupts have
> + * been processed and the poke bit has been cleared.
> + *
> + * Other interrupts may also be processed while interrupts are enabled, so
> + * need_resched() must be tested after turning interrupts off again to make sure
> + * the interrupt didn't schedule work that should take the cpu out of idle.
> + *
> + * Returns 0 if need_resched was false, -EINTR if need_resched was true.
> + */
> +static int cpuidle_coupled_clear_pokes(int cpu)
> +{
> + local_irq_enable();
> + while (cpumask_test_cpu(cpu, &cpuidle_coupled_poked_mask))
> + cpu_relax();
> + local_irq_disable();
> +
> + return need_resched() ? -EINTR : 0;
> +}
> +
> +/**
> + * cpuidle_enter_state_coupled - attempt to enter a state with coupled cpus
> + * @dev: struct cpuidle_device for the current cpu
> + * @drv: struct cpuidle_driver for the platform
> + * @next_state: index of the requested state in drv->states
> + *
> + * Coordinate with coupled cpus to enter the target state. This is a two
> + * stage process. In the first stage, the cpus are operating independently,
> + * and may call into cpuidle_enter_state_coupled at completely different times.
> + * To save as much power as possible, the first cpus to call this function will
> + * go to an intermediate state (the cpuidle_device's safe state), and wait for
> + * all the other cpus to call this function. Once all coupled cpus are idle,
> + * the second stage will start. Each coupled cpu will spin until all cpus have
> + * guaranteed that they will call the target_state.
> + *
> + * This function must be called with interrupts disabled. It may enable
> + * interrupts while preparing for idle, and it will always return with
> + * interrupts enabled.
> + */
> +int cpuidle_enter_state_coupled(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int next_state)
> +{
> + int entered_state = -1;
> + struct cpuidle_coupled *coupled = dev->coupled;
> +
> + if (!coupled)
> + return -EINVAL;
> +
> + while (coupled->prevent) {
> + if (cpuidle_coupled_clear_pokes(dev->cpu)) {
> + local_irq_enable();
> + return entered_state;
> + }
> + entered_state = cpuidle_enter_state(dev, drv,
> + dev->safe_state_index);
> + }
> +
> + /* Read barrier ensures online_count is read after prevent is cleared */
> + smp_rmb();
> +
> + cpuidle_coupled_set_waiting(dev->cpu, coupled, next_state);
> +
> +retry:
> + /*
> + * Wait for all coupled cpus to be idle, using the deepest state
> + * allowed for a single cpu.
> + */
> + while (!cpuidle_coupled_cpus_waiting(coupled)) {
> + if (cpuidle_coupled_clear_pokes(dev->cpu)) {
> + cpuidle_coupled_set_not_waiting(dev->cpu, coupled);
> + goto out;
> + }
> +
> + if (coupled->prevent) {
> + cpuidle_coupled_set_not_waiting(dev->cpu, coupled);
> + goto out;
> + }
> +
> + entered_state = cpuidle_enter_state(dev, drv,
> + dev->safe_state_index);
> + }
> +
> + if (cpuidle_coupled_clear_pokes(dev->cpu)) {
> + cpuidle_coupled_set_not_waiting(dev->cpu, coupled);
> + goto out;
> + }
> +
> + /*
> + * All coupled cpus are probably idle. There is a small chance that
> + * one of the other cpus just became active. Increment the ready count,
> + * and spin until all coupled cpus have incremented the counter. Once a
> + * cpu has incremented the ready counter, it cannot abort idle and must
> + * spin until either all cpus have incremented the ready counter, or
> + * another cpu leaves idle and decrements the waiting counter.
> + */
> +
> + cpuidle_coupled_set_ready(coupled);
> + while (!cpuidle_coupled_cpus_ready(coupled)) {
> + /* Check if any other cpus bailed out of idle. */
> + if (!cpuidle_coupled_cpus_waiting(coupled))
> + if (!cpuidle_coupled_set_not_ready(coupled))
> + goto retry;
> +
> + cpu_relax();
> + }
> +
> + /* all cpus have acked the coupled state */
> + next_state = cpuidle_coupled_get_state(dev, coupled);
> +
> + entered_state = cpuidle_enter_state(dev, drv, next_state);
> +
> + cpuidle_coupled_set_not_waiting(dev->cpu, coupled);
> + cpuidle_coupled_set_not_ready(coupled);
> +
> +out:
> + /*
> + * Normal cpuidle states are expected to return with irqs enabled.
> + * That leads to an inefficiency where a cpu receiving an interrupt
> + * that brings it out of idle will process that interrupt before
> + * exiting the idle enter function and decrementing ready_count. All
> + * other cpus will need to spin waiting for the cpu that is processing
> + * the interrupt. If the driver returns with interrupts disabled,
> + * all other cpus will loop back into the safe idle state instead of
> + * spinning, saving power.
> + *
> + * Calling local_irq_enable here allows coupled states to return with
> + * interrupts disabled, but won't cause problems for drivers that
> + * exit with interrupts enabled.
> + */
> + local_irq_enable();
> +
> + /*
> + * Wait until all coupled cpus have exited idle. There is no risk that
> + * a cpu exits and re-enters the ready state because this cpu has
> + * already decremented its waiting_count.
> + */
> + while (!cpuidle_coupled_no_cpus_ready(coupled))
> + cpu_relax();
> +
> + return entered_state;
> +}
> +
> +/**
> + * cpuidle_coupled_register_device - register a coupled cpuidle device
> + * @dev: struct cpuidle_device for the current cpu
> + *
> + * Called from cpuidle_register_device to handle coupled idle init. Finds the
> + * cpuidle_coupled struct for this set of coupled cpus, or creates one if none
> + * exists yet.
> + */
> +int cpuidle_coupled_register_device(struct cpuidle_device *dev)
> +{
> + int cpu;
> + struct cpuidle_device *other_dev;
> + struct call_single_data *csd;
> + struct cpuidle_coupled *coupled;
> +
> + if (cpumask_empty(&dev->coupled_cpus))
> + return 0;
> +
> + for_each_cpu_mask(cpu, dev->coupled_cpus) {
> + other_dev = per_cpu(cpuidle_devices, cpu);
> + if (other_dev && other_dev->coupled) {
> + coupled = other_dev->coupled;
> + goto have_coupled;
> + }
> + }
> +
> + /* No existing coupled info found, create a new one */
> + coupled = kzalloc(sizeof(struct cpuidle_coupled), GFP_KERNEL);
> + if (!coupled)
> + return -ENOMEM;
> +
> + coupled->coupled_cpus = dev->coupled_cpus;
> +
> +have_coupled:
> + dev->coupled = coupled;
> + if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
> + coupled->prevent++;
> +
> + coupled->refcnt++;
> +
> + csd = &per_cpu(cpuidle_coupled_poke_cb, dev->cpu);
> + csd->func = cpuidle_coupled_poked;
> + csd->info = (void *)(unsigned long)dev->cpu;
> +
> + return 0;
> +}
> +
> +/**
> + * cpuidle_coupled_unregister_device - unregister a coupled cpuidle device
> + * @dev: struct cpuidle_device for the current cpu
> + *
> + * Called from cpuidle_unregister_device to tear down coupled idle. Removes the
> + * cpu from the coupled idle set, and frees the cpuidle_coupled_info struct if
> + * this was the last cpu in the set.
> + */
> +void cpuidle_coupled_unregister_device(struct cpuidle_device *dev)
> +{
> + struct cpuidle_coupled *coupled = dev->coupled;
> +
> + if (cpumask_empty(&dev->coupled_cpus))
> + return;
> +
> + if (--coupled->refcnt)
> + kfree(coupled);
> + dev->coupled = NULL;
> +}
> +
> +/**
> + * cpuidle_coupled_prevent_idle - prevent cpus from entering a coupled state
> + * @coupled: the struct coupled that contains the cpu that is changing state
> + *
> + * Disables coupled cpuidle on a coupled set of cpus. Used to ensure that
> + * cpu_online_mask doesn't change while cpus are coordinating coupled idle.
> + */
> +static void cpuidle_coupled_prevent_idle(struct cpuidle_coupled *coupled)
> +{
> + int cpu = get_cpu();
> +
> + /* Force all cpus out of the waiting loop. */
> + coupled->prevent++;
> + cpuidle_coupled_poke_others(cpu, coupled);
> + put_cpu();
> + while (!cpuidle_coupled_no_cpus_waiting(coupled))
> + cpu_relax();
> +}
> +
> +/**
> + * cpuidle_coupled_allow_idle - allows cpus to enter a coupled state
> + * @coupled: the struct coupled that contains the cpu that is changing state
> + *
> + * Enables coupled cpuidle on a coupled set of cpus. Used to ensure that
> + * cpu_online_mask doesn't change while cpus are coordinating coupled idle.
> + */
> +static void cpuidle_coupled_allow_idle(struct cpuidle_coupled *coupled)
> +{
> + int cpu = get_cpu();
> +
> + /*
> + * Write barrier ensures readers see the new online_count when they
> + * see prevent == 0.
> + */
> + smp_wmb();
> + coupled->prevent--;
> + /* Force cpus out of the prevent loop. */
> + cpuidle_coupled_poke_others(cpu, coupled);
> + put_cpu();
> +}
> +
> +/**
> + * cpuidle_coupled_cpu_notify - notifier called during hotplug transitions
> + * @nb: notifier block
> + * @action: hotplug transition
> + * @hcpu: target cpu number
> + *
> + * Called when a cpu is brought on or offline using hotplug. Updates the
> + * coupled cpu set appropriately
> + */
> +static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
> + unsigned long action, void *hcpu)
> +{
> + int cpu = (unsigned long)hcpu;
> + struct cpuidle_device *dev;
> +
> + mutex_lock(&cpuidle_lock);
> +
> + dev = per_cpu(cpuidle_devices, cpu);
> + if (!dev->coupled)
> + goto out;
> +
> + switch (action & ~CPU_TASKS_FROZEN) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_PREPARE:
> + cpuidle_coupled_prevent_idle(dev->coupled);
> + break;
> + case CPU_ONLINE:
> + case CPU_DEAD:
> + dev->coupled->online_count = num_online_cpus();
> + /* Fall through */
> + case CPU_UP_CANCELED:
> + case CPU_DOWN_FAILED:
> + cpuidle_coupled_allow_idle(dev->coupled);
> + break;
> + }
> +
> +out:
> + mutex_unlock(&cpuidle_lock);
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block cpuidle_coupled_cpu_notifier = {
> + .notifier_call = cpuidle_coupled_cpu_notify,
> +};
> +
> +static int __init cpuidle_coupled_init(void)
> +{
> + return register_cpu_notifier(&cpuidle_coupled_cpu_notifier);
> +}
> +core_initcall(cpuidle_coupled_init);
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 4540672..e81cfda 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -171,7 +171,11 @@ int cpuidle_idle_call(void)
> trace_power_start_rcuidle(POWER_CSTATE, next_state, dev->cpu);
> trace_cpu_idle_rcuidle(next_state, dev->cpu);
>
> - entered_state = cpuidle_enter_state(dev, drv, next_state);
> + if (cpuidle_state_is_coupled(dev, drv, next_state))
> + entered_state = cpuidle_enter_state_coupled(dev, drv,
> + next_state);
> + else
> + entered_state = cpuidle_enter_state(dev, drv, next_state);
>
> trace_power_end_rcuidle(dev->cpu);
> trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> @@ -407,9 +411,16 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
> if (ret)
> goto err_sysfs;
>
> + ret = cpuidle_coupled_register_device(dev);
> + if (ret)
> + goto err_coupled;
> +
> dev->registered = 1;
> return 0;
>
> +err_coupled:
> + cpuidle_remove_sysfs(cpu_dev);
> + wait_for_completion(&dev->kobj_unregister);
> err_sysfs:
> list_del(&dev->device_list);
> per_cpu(cpuidle_devices, dev->cpu) = NULL;
> @@ -464,6 +475,8 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
> wait_for_completion(&dev->kobj_unregister);
> per_cpu(cpuidle_devices, dev->cpu) = NULL;
>
> + cpuidle_coupled_unregister_device(dev);
> +
> cpuidle_resume_and_unlock();
>
> module_put(cpuidle_driver->owner);
> diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
> index d8a3ccc..76e7f69 100644
> --- a/drivers/cpuidle/cpuidle.h
> +++ b/drivers/cpuidle/cpuidle.h
> @@ -32,4 +32,34 @@ extern int cpuidle_enter_state(struct cpuidle_device *dev,
> extern int cpuidle_add_sysfs(struct device *dev);
> extern void cpuidle_remove_sysfs(struct device *dev);
>
> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> +bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int state);
> +int cpuidle_enter_state_coupled(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int next_state);
> +int cpuidle_coupled_register_device(struct cpuidle_device *dev);
> +void cpuidle_coupled_unregister_device(struct cpuidle_device *dev);
> +#else
> +static inline bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int state)
> +{
> + return false;
> +}
> +
> +static inline int cpuidle_enter_state_coupled(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int next_state)
> +{
> + return -1;
> +}
> +
> +static inline int cpuidle_coupled_register_device(struct cpuidle_device *dev)
> +{
> + return 0;
> +}
> +
> +static inline void cpuidle_coupled_unregister_device(struct cpuidle_device *dev)
> +{
> +}
> +#endif
> +
> #endif /* __DRIVER_CPUIDLE_H */
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 6c26a3d..6038448 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -57,6 +57,7 @@ struct cpuidle_state {
>
> /* Idle State Flags */
> #define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */
> +#define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */
>
> #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>
> @@ -100,6 +101,12 @@ struct cpuidle_device {
> struct list_head device_list;
> struct kobject kobj;
> struct completion kobj_unregister;
> +
> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> + int safe_state_index;
> + cpumask_t coupled_cpus;
> + struct cpuidle_coupled *coupled;
> +#endif
> };
>
> DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
>
^ permalink raw reply
* Re: [PATCHv4 4/4] cpuidle: coupled: add parallel barrier function
From: Rafael J. Wysocki @ 2012-05-09 21:31 UTC (permalink / raw)
To: Colin Cross
Cc: Kevin Hilman, Len Brown, Russell King, Greg Kroah-Hartman,
Kay Sievers, linux-kernel, Amit Kucheria, linux-pm,
Arjan van de Ven, Arnd Bergmann, linux-arm-kernel
In-Reply-To: <1336438662-10484-5-git-send-email-ccross@android.com>
On Tuesday, May 08, 2012, Colin Cross wrote:
> Adds cpuidle_coupled_parallel_barrier, which can be used by coupled
> cpuidle state enter functions to handle resynchronization after
> determining if any cpu needs to abort. The normal use case will
> be:
>
> static bool abort_flag;
> static atomic_t abort_barrier;
>
> int arch_cpuidle_enter(struct cpuidle_device *dev, ...)
> {
> if (arch_turn_off_irq_controller()) {
> /* returns an error if an irq is pending and would be lost
> if idle continued and turned off power */
> abort_flag = true;
> }
>
> cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
>
> if (abort_flag) {
> /* One of the cpus didn't turn off it's irq controller */
> arch_turn_on_irq_controller();
> return -EINTR;
> }
>
> /* continue with idle */
> ...
> }
>
> This will cause all cpus to abort idle together if one of them needs
> to abort.
>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
> drivers/cpuidle/coupled.c | 37 +++++++++++++++++++++++++++++++++++++
> include/linux/cpuidle.h | 4 ++++
> 2 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index 93101fb..3e65de1 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -130,6 +130,43 @@ struct cpuidle_coupled {
> static cpumask_t cpuidle_coupled_poked_mask;
>
> /**
> + * cpuidle_coupled_parallel_barrier - synchronize all online coupled cpus
> + * @dev: cpuidle_device of the calling cpu
> + * @a: atomic variable to hold the barrier
> + *
> + * No caller to this function will return from this function until all online
> + * cpus in the same coupled group have called this function. Once any caller
> + * has returned from this function, the barrier is immediately available for
> + * reuse.
> + *
> + * The atomic variable a must be initialized to 0 before any cpu calls
> + * this function, will be reset to 0 before any cpu returns from this function.
> + *
> + * Must only be called from within a coupled idle state handler
> + * (state.enter when state.flags has CPUIDLE_FLAG_COUPLED set).
> + *
> + * Provides full smp barrier semantics before and after calling.
> + */
> +void cpuidle_coupled_parallel_barrier(struct cpuidle_device *dev, atomic_t *a)
> +{
> + int n = dev->coupled->online_count;
> +
> + smp_mb__before_atomic_inc();
> + atomic_inc(a);
> +
> + while (atomic_read(a) < n)
> + cpu_relax();
> +
> + if (atomic_inc_return(a) == n * 2) {
> + atomic_set(a, 0);
> + return;
> + }
> +
> + while (atomic_read(a) > n)
> + cpu_relax();
> +}
Well, this looks like "wait until all CPUs execute this code". Don't we have
anything like this already somewhere?
> +
> +/**
> * cpuidle_state_is_coupled - check if a state is part of a coupled set
> * @dev: struct cpuidle_device for the current cpu
> * @drv: struct cpuidle_driver for the platform
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 6038448..5ab7183 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -183,6 +183,10 @@ static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
>
> #endif
>
> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> +void cpuidle_coupled_parallel_barrier(struct cpuidle_device *dev, atomic_t *a);
> +#endif
Why exactly is the extra Kconfig option necessary?
> +
> /******************************
> * CPUIDLE GOVERNOR INTERFACE *
> ******************************/
Rafael
^ permalink raw reply
* Re: [PATCH V3 05/10] ARM: OMAP2+: SmartReflex: introduce a busy loop condition test macro
From: J, KEERTHY @ 2012-05-10 6:19 UTC (permalink / raw)
To: AnilKumar, Chimata
Cc: linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Hilman, Kevin, rjw@sisk.pl, linux-kernel@vger.kernel.org,
linux-pm@lists.linux-foundation.org, Pihet-XID, Jean
In-Reply-To: <331ABD5ECB02734CA317220B2BBEABC13E9B667C@DBDE01.ent.ti.com>
On Tue, May 8, 2012 at 3:47 PM, AnilKumar, Chimata <anilkumar@ti.com> wrote:
> On Mon, May 07, 2012 at 10:51:53, J, KEERTHY wrote:
>> On Fri, May 4, 2012 at 2:42 PM, AnilKumar, Chimata <anilkumar@ti.com> wrote:
>> > On Thu, Apr 26, 2012 at 23:10:36, J, KEERTHY wrote:
>> >> From: Jean Pihet <j-pihet@ti.com>
>> >>
>> >> Now that omap_test_timeout is only accessible from mach-omap2/,
>> >> introduce a similar function for SR.
>> >>
>> >> This change makes the SmartReflex implementation ready for the move
>> >> to drivers/.
>> >>
>> >> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> >> Signed-off-by: J Keerthy <j-keerthy@ti.com>
>> >> ---
>> >> arch/arm/mach-omap2/smartreflex.c | 12 ++++++------
>> >> include/linux/power/smartreflex.h | 23 ++++++++++++++++++++++-
>> >> 2 files changed, 28 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>> >> index d859277..acef08d 100644
>> >> --- a/arch/arm/mach-omap2/smartreflex.c
>> >> +++ b/arch/arm/mach-omap2/smartreflex.c
>> >> @@ -289,9 +289,9 @@ static void sr_v1_disable(struct omap_sr *sr)
>> >> * Wait for SR to be disabled.
>> >> * wait until ERRCONFIG.MCUDISACKINTST = 1. Typical latency is 1us.
>> >> */
>> >> - omap_test_timeout((sr_read_reg(sr, ERRCONFIG_V1) &
>> >> - ERRCONFIG_MCUDISACKINTST), SR_DISABLE_TIMEOUT,
>> >> - timeout);
>> >> + sr_test_cond_timeout((sr_read_reg(sr, ERRCONFIG_V1) &
>> >> + ERRCONFIG_MCUDISACKINTST), SR_DISABLE_TIMEOUT,
>> >> + timeout);
>> >>
>> >> if (timeout >= SR_DISABLE_TIMEOUT)
>> >> dev_warn(&sr->pdev->dev, "%s: Smartreflex disable timedout\n",
>> >> @@ -334,9 +334,9 @@ static void sr_v2_disable(struct omap_sr *sr)
>> >> * Wait for SR to be disabled.
>> >> * wait until IRQSTATUS.MCUDISACKINTST = 1. Typical latency is 1us.
>> >> */
>> >> - omap_test_timeout((sr_read_reg(sr, IRQSTATUS) &
>> >> - IRQSTATUS_MCUDISABLEACKINT), SR_DISABLE_TIMEOUT,
>> >> - timeout);
>> >> + sr_test_cond_timeout((sr_read_reg(sr, IRQSTATUS) &
>> >> + IRQSTATUS_MCUDISABLEACKINT), SR_DISABLE_TIMEOUT,
>> >> + timeout);
>> >>
>> >> if (timeout >= SR_DISABLE_TIMEOUT)
>> >> dev_warn(&sr->pdev->dev, "%s: Smartreflex disable timedout\n",
>> >> diff --git a/include/linux/power/smartreflex.h b/include/linux/power/smartreflex.h
>> >> index 884eaee..78b795e 100644
>> >> --- a/include/linux/power/smartreflex.h
>> >> +++ b/include/linux/power/smartreflex.h
>> >> @@ -22,7 +22,7 @@
>> >>
>> >> #include <linux/types.h>
>> >> #include <linux/platform_device.h>
>> >> -
>> >> +#include <linux/delay.h>
>> >> #include <plat/voltage.h>
>> >>
>> >> /*
>> >> @@ -168,6 +168,27 @@ struct omap_sr {
>> >> };
>> >>
>> >> /**
>> >> + * test_cond_timeout - busy-loop, testing a condition
>> >> + * @cond: condition to test until it evaluates to true
>> >> + * @timeout: maximum number of microseconds in the timeout
>> >> + * @index: loop index (integer)
>> >> + *
>> >> + * Loop waiting for @cond to become true or until at least @timeout
>> >> + * microseconds have passed. To use, define some integer @index in the
>> >> + * calling code. After running, if @index == @timeout, then the loop has
>> >> + * timed out.
>> >> + *
>> >> + * Copied from omap_test_timeout */
>> >> +#define sr_test_cond_timeout(cond, timeout, index) \
>> >> +({ \
>> >> + for (index = 0; index < timeout; index++) { \
>> >> + if (cond) \
>> >> + break; \
>> >> + udelay(1); \
>> >> + } \
>> >> +})
>> >
>> > I think we can use time_after()/time_before() APIs for timeout and cpu_relax() for
>> > tight loops instead of udelay().
>>
>> cpu_relax() changes the priority everytime to low and will yield to
>> another thread.
>> Considering that we are checking the condition every microsecond does it make
>> some saving using cpu_relax().
>>
>
> cpu_relax() does not involve any priority changes or scheduling AFAICS.
> Have a look at this thread:
>
> http://www.spinics.net/lists/netdev/msg151699.html
Thanks. My bad. I meant yielding to another thread with in a space
of 1uS.
>
> Regards
> AnilKumar
>
--
Regards and Thanks,
Keerthy
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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
* [PATCH 0/2] ARM: DAVINCI: cpuidle - cleanups
From: Daniel Lezcano @ 2012-05-10 8:44 UTC (permalink / raw)
To: nsekhar, khilman, lenb
Cc: davinci-linux-open-source, linaro-dev, linux-pm, patches
These couple of patches use the new cpuidle core api to refactor
some part of the code. The first one removes the state count initialization
as it is done from the cpuidle core and the second one use the new
API and removes the ops.
The patchset is based on Lenb's tree on top of Robert Lee cpuidle consolidation
work.
I don't have this board, I was not able to test these patches.
Daniel Lezcano (2):
ARM: DAVINCI: cpuidle - remove useless state count initialization
ARM: DAVINCI: cpuidle - remove ops
arch/arm/mach-davinci/cpuidle.c | 83 +++++++++++++--------------------------
1 files changed, 27 insertions(+), 56 deletions(-)
--
1.7.5.4
^ permalink raw reply
* [PATCH 1/2] ARM: DAVINCI: cpuidle - remove useless state count initialization
From: Daniel Lezcano @ 2012-05-10 8:44 UTC (permalink / raw)
To: nsekhar, khilman, lenb
Cc: davinci-linux-open-source, linaro-dev, linux-pm, patches
In-Reply-To: <1336639485-26955-1-git-send-email-daniel.lezcano@linaro.org>
The state count is initialized in the driver structure, the cpuidle
core uses it to initialize the device state count.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
arch/arm/mach-davinci/cpuidle.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index 9107691..f0f179c 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -128,8 +128,6 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
cpuidle_set_statedata(&device->states_usage[1], &davinci_states[1]);
- device->state_count = DAVINCI_CPUIDLE_MAX_STATES;
-
ret = cpuidle_register_driver(&davinci_idle_driver);
if (ret) {
dev_err(&pdev->dev, "failed to register driver\n");
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/2] ARM: DAVINCI: cpuidle - remove ops
From: Daniel Lezcano @ 2012-05-10 8:44 UTC (permalink / raw)
To: nsekhar-l0cyMroinI0, khilman-l0cyMroinI0,
lenb-DgEjT+Ai2ygdnm+yROfE0A
Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
linaro-dev-cunTk1MwBs8s++Sfvej+rw,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
patches-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <1336639485-26955-1-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
This patch removes the ops usage because we have the index
passed as parameter to the idle function and we can determine
if we do WFI or memory retention.
The benefit of this cleanup is the removal of:
* the ops
* the statedata usage because we want to get rid of it in all the drivers
* extra structure definition
* extra functions definition
* pointless macro definition BIT(0)
It also benefits the readability.
Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
arch/arm/mach-davinci/cpuidle.c | 81 +++++++++++++--------------------------
1 files changed, 27 insertions(+), 54 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index f0f179c..61f4e52 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -25,35 +25,46 @@
#define DAVINCI_CPUIDLE_MAX_STATES 2
-struct davinci_ops {
- void (*enter) (u32 flags);
- void (*exit) (u32 flags);
- u32 flags;
-};
+static bool ddr2_pwdn = false;
+
+static void __iomem *ddr2_reg_base;
+
+static void davinci_save_ddr_power(int enter, bool pdown)
+{
+ u32 val;
+
+ val = __raw_readl(ddr2_reg_base + DDR2_SDRCR_OFFSET);
+
+ if (enter) {
+ if (pdown)
+ val |= DDR2_SRPD_BIT;
+ else
+ val &= ~DDR2_SRPD_BIT;
+ val |= DDR2_LPMODEN_BIT;
+ } else {
+ val &= ~(DDR2_SRPD_BIT | DDR2_LPMODEN_BIT);
+ }
+
+ __raw_writel(val, ddr2_reg_base + DDR2_SDRCR_OFFSET);
+}
/* Actual code that puts the SoC in different idle states */
static int davinci_enter_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
- struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
- struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
-
- if (ops && ops->enter)
- ops->enter(ops->flags);
+ if (index)
+ davinci_save_ddr_power(1, ddr2_pwdn);
index = cpuidle_wrap_enter(dev, drv, index,
arm_cpuidle_simple_enter);
- if (ops && ops->exit)
- ops->exit(ops->flags);
+ if (index)
+ davinci_save_ddr_power(0, ddr2_pwdn);
return index;
}
-/* fields in davinci_ops.flags */
-#define DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN BIT(0)
-
static struct cpuidle_driver davinci_idle_driver = {
.name = "cpuidle-davinci",
.owner = THIS_MODULE,
@@ -71,43 +82,6 @@ static struct cpuidle_driver davinci_idle_driver = {
};
static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
-static void __iomem *ddr2_reg_base;
-
-static void davinci_save_ddr_power(int enter, bool pdown)
-{
- u32 val;
-
- val = __raw_readl(ddr2_reg_base + DDR2_SDRCR_OFFSET);
-
- if (enter) {
- if (pdown)
- val |= DDR2_SRPD_BIT;
- else
- val &= ~DDR2_SRPD_BIT;
- val |= DDR2_LPMODEN_BIT;
- } else {
- val &= ~(DDR2_SRPD_BIT | DDR2_LPMODEN_BIT);
- }
-
- __raw_writel(val, ddr2_reg_base + DDR2_SDRCR_OFFSET);
-}
-
-static void davinci_c2state_enter(u32 flags)
-{
- davinci_save_ddr_power(1, !!(flags & DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN));
-}
-
-static void davinci_c2state_exit(u32 flags)
-{
- davinci_save_ddr_power(0, !!(flags & DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN));
-}
-
-static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = {
- [1] = {
- .enter = davinci_c2state_enter,
- .exit = davinci_c2state_exit,
- },
-};
static int __init davinci_cpuidle_probe(struct platform_device *pdev)
{
@@ -125,8 +99,7 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
ddr2_reg_base = pdata->ddr2_ctlr_base;
if (pdata->ddr2_pdown)
- davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
- cpuidle_set_statedata(&device->states_usage[1], &davinci_states[1]);
+ ddr2_pwdn = true;
ret = cpuidle_register_driver(&davinci_idle_driver);
if (ret) {
--
1.7.5.4
^ permalink raw reply related
* RE: [PATCH V3 07/10] ARM: OMAP2+: SmartReflex: Use per-OPP data structure
From: Guyotte, Greg @ 2012-05-10 19:11 UTC (permalink / raw)
To: J, KEERTHY, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Hilman, Kevin, rjw@sisk.pl,
linux-kernel@vger.kernel.org, linux-pm@lists.linux-foundation.org
Cc: Pihet-XID, Jean, Paul Walmsley, Gopinath, Thara, Menon, Nishanth
In-Reply-To: <1335462041-4949-8-git-send-email-j-keerthy@ti.com>
Hi Jean,
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of J, KEERTHY
> Sent: Thursday, April 26, 2012 12:41 PM
> To: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> Hilman, Kevin; rjw@sisk.pl; linux-kernel@vger.kernel.org; linux-
> pm@lists.linux-foundation.org
> Cc: Pihet-XID, Jean; J, KEERTHY; Paul Walmsley; Gopinath, Thara; Menon,
> Nishanth
> Subject: [PATCH V3 07/10] ARM: OMAP2+: SmartReflex: Use per-OPP data
> structure
>
> From: Jean Pihet <j-pihet@ti.com>
>
> The SmartReflex driver incorrectly treats some per-OPP data as data
> common to all OPPs (e.g., ERRMINLIMIT). Move this data into a per-OPP
> data structure.
>
> Furthermore, in order to make the SmartReflex implementation ready for
> the move to drivers/, remove the dependency from the SR driver code
> to the voltage layer by querying the data tables only from the SR device
> init code.
>
> Based on Paul's original code for the SmartReflex driver conversion.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> Signed-off-by: J Keerthy <j-keerthy@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Thara Gopinath <thara@ti.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
> arch/arm/mach-omap2/smartreflex.c | 38 +++++++++++++++++---------------
> ----
> arch/arm/mach-omap2/sr_device.c | 36 +++++++++++++++++++++++++++++---
> --
> include/linux/power/smartreflex.h | 8 +++++-
> 3 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
> omap2/smartreflex.c
> index acef08d..20075de 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -347,22 +347,23 @@ static void sr_v2_disable(struct omap_sr *sr)
> sr_write_reg(sr, IRQSTATUS, IRQSTATUS_MCUDISABLEACKINT);
> }
>
> -static u32 sr_retrieve_nvalue(struct omap_sr *sr, u32 efuse_offs)
> +static struct omap_sr_nvalue_table *sr_retrieve_nvalue_row(
> + struct omap_sr *sr, u32 efuse_offs)
> {
> int i;
>
> if (!sr->nvalue_table) {
> dev_warn(&sr->pdev->dev, "%s: Missing ntarget value table\n",
> __func__);
> - return 0;
> + return NULL;
> }
>
> for (i = 0; i < sr->nvalue_count; i++) {
> if (sr->nvalue_table[i].efuse_offs == efuse_offs)
> - return sr->nvalue_table[i].nvalue;
> + return &sr->nvalue_table[i];
> }
>
> - return 0;
> + return NULL;
> }
>
> /* Public Functions */
> @@ -586,7 +587,7 @@ int sr_enable(struct voltagedomain *voltdm, unsigned
> long volt)
> {
> struct omap_volt_data *volt_data;
> struct omap_sr *sr = _sr_lookup(voltdm);
> - u32 nvalue_reciprocal;
> + struct omap_sr_nvalue_table *nvalue_row;
> int ret;
>
> if (IS_ERR(sr)) {
> @@ -602,16 +603,16 @@ int sr_enable(struct voltagedomain *voltdm, unsigned
> long volt)
> return PTR_ERR(volt_data);
> }
>
> - nvalue_reciprocal = sr_retrieve_nvalue(sr, volt_data-
> >sr_efuse_offs);
> + nvalue_row = sr_retrieve_nvalue_row(sr, volt_data->sr_efuse_offs);
>
> - if (!nvalue_reciprocal) {
> - dev_warn(&sr->pdev->dev, "%s: NVALUE = 0 at voltage %ld\n",
> - __func__, volt);
> + if (!nvalue_row) {
> + dev_warn(&sr->pdev->dev, "%s: failure getting SR data for this
> voltage %ld\n",
> + __func__, volt);
> return -ENODATA;
> }
>
> /* errminlimit is opp dependent and hence linked to voltage */
> - sr->err_minlimit = volt_data->sr_errminlimit;
> + sr->err_minlimit = nvalue_row->errminlimit;
>
> pm_runtime_get_sync(&sr->pdev->dev);
>
> @@ -624,7 +625,7 @@ int sr_enable(struct voltagedomain *voltdm, unsigned
> long volt)
> if (ret)
> return ret;
>
> - sr_write_reg(sr, NVALUERECIPROCAL, nvalue_reciprocal);
> + sr_write_reg(sr, NVALUERECIPROCAL, nvalue_row->nvalue);
>
> /* SRCONFIG - enable SR */
> sr_modify_reg(sr, SRCONFIG, SRCONFIG_SRENABLE, SRCONFIG_SRENABLE);
> @@ -872,7 +873,6 @@ static int __init omap_sr_probe(struct platform_device
> *pdev)
> struct omap_sr_data *pdata = pdev->dev.platform_data;
> struct resource *mem, *irq;
> struct dentry *nvalue_dir;
> - struct omap_volt_data *volt_data;
> int i, ret = 0;
>
> sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
> @@ -990,12 +990,10 @@ static int __init omap_sr_probe(struct
> platform_device *pdev)
> goto err_debugfs;
> }
>
> - omap_voltage_get_volttable(sr_info->voltdm, &volt_data);
> - if (!volt_data) {
> - dev_warn(&pdev->dev, "%s: %s: No Voltage table for the"
> - " corresponding vdd. Cannot create debugfs"
> - "entries for n-values\n",
> - __func__, sr_info->name);
> + if (sr_info->nvalue_count == 0 || !sr_info->nvalue_table) {
> + dev_warn(&pdev->dev, "%s: %s: No Voltage table for the
> corresponding vdd. Cannot create debugfs entries for n-values\n",
> + __func__, sr_info->name);
> +
> ret = -ENODATA;
> goto err_debugfs;
> }
> @@ -1003,8 +1001,8 @@ static int __init omap_sr_probe(struct
> platform_device *pdev)
> for (i = 0; i < sr_info->nvalue_count; i++) {
> char name[NVALUE_NAME_LEN + 1];
>
> - snprintf(name, sizeof(name), "volt_%d",
> - volt_data[i].volt_nominal);
> + snprintf(name, sizeof(name), "volt_%lu",
> + sr_info->nvalue_table[i].volt_nominal);
> (void) debugfs_create_x32(name, S_IRUGO | S_IWUSR, nvalue_dir,
> &(sr_info->nvalue_table[i].nvalue));
> }
> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-
> omap2/sr_device.c
> index e081174..e107e39 100644
> --- a/arch/arm/mach-omap2/sr_device.c
> +++ b/arch/arm/mach-omap2/sr_device.c
> @@ -36,7 +36,10 @@ static void __init sr_set_nvalues(struct omap_volt_data
> *volt_data,
> struct omap_sr_data *sr_data)
> {
> struct omap_sr_nvalue_table *nvalue_table;
> - int i, count = 0;
> + int i, j, count = 0;
> +
> + sr_data->nvalue_count = 0;
> + sr_data->nvalue_table = NULL;
>
> while (volt_data[count].volt_nominal)
> count++;
> @@ -44,8 +47,14 @@ static void __init sr_set_nvalues(struct omap_volt_data
> *volt_data,
> nvalue_table = kzalloc(sizeof(struct omap_sr_nvalue_table)*count,
> GFP_KERNEL);
>
> - for (i = 0; i < count; i++) {
> + if (!nvalue_table) {
> + pr_err("OMAP: SmartReflex: cannot allocate memory for n-value
> table\n");
> + return;
> + }
> +
> + for (i = 0, j = 0; i < count; i++) {
> u32 v;
> +
> /*
> * In OMAP4 the efuse registers are 24 bit aligned.
> * A __raw_readl will fail for non-32 bit aligned address
> @@ -58,15 +67,30 @@ static void __init sr_set_nvalues(struct
> omap_volt_data *volt_data,
> omap_ctrl_readb(offset + 1) << 8 |
> omap_ctrl_readb(offset + 2) << 16;
> } else {
> - v = omap_ctrl_readl(volt_data[i].sr_efuse_offs);
> + v = omap_ctrl_readl(volt_data[i].sr_efuse_offs);
> }
>
> - nvalue_table[i].efuse_offs = volt_data[i].sr_efuse_offs;
> - nvalue_table[i].nvalue = v;
> + /*
> + * Many OMAP SoCs don't have the eFuse values set.
> + * For example, pretty much all OMAP3xxx before
> + * ES3.something.
> + *
> + * XXX There needs to be some way for board files or
> + * userspace to add these in.
> + */
> + if (v == 0)
> + continue;
> +
> + nvalue_table[j].nvalue = v;
> + nvalue_table[j].efuse_offs = volt_data[i].sr_efuse_offs;
> + nvalue_table[j].errminlimit = volt_data[i].sr_errminlimit;
> + nvalue_table[j].volt_nominal = volt_data[i].volt_nominal;
> +
> + j++;
> }
>
> sr_data->nvalue_table = nvalue_table;
> - sr_data->nvalue_count = count;
> + sr_data->nvalue_count = j;
> }
>
> static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
> diff --git a/include/linux/power/smartreflex.h
> b/include/linux/power/smartreflex.h
> index 78b795e..222f901 100644
> --- a/include/linux/power/smartreflex.h
> +++ b/include/linux/power/smartreflex.h
> @@ -243,12 +243,16 @@ struct omap_sr_class_data {
> /**
> * struct omap_sr_nvalue_table - Smartreflex n-target value info
> *
> - * @efuse_offs: The offset of the efuse where n-target values are stored.
> - * @nvalue: The n-target value.
> + * @efuse_offs: The offset of the efuse where n-target values are
> stored.
> + * @nvalue: The n-target value.
> + * @errminlimit: The value of the ERRMINLIMIT bitfield for this n-target
> + * @volt_nominal: microvolts DC that the VDD is initially programmed to
> */
> struct omap_sr_nvalue_table {
> u32 efuse_offs;
> u32 nvalue;
> + u32 errminlimit;
> + unsigned long volt_nominal;
[GG] I would suggest that we also need to add errmaxlimit and errweight fields to this structure. These are very much possible to change per OPP, though it hasn't been common in implementations up to now.
> };
>
> /**
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" 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
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