Re: [PATCH 1/4] thermal: exynos: Add thermal interface support forlinux thermal layer
From: Amit Kachhap
Date: Tue Mar 13 2012 - 00:22:31 EST
Hi Durgadoss,
Thanks for the detailed review.
On 12 March 2012 16:21, R, Durgadoss <durgadoss.r@xxxxxxxxx> wrote:
> Hi Amit,
>
> Thanks for keeping this up. And Sorry for late reply.
>
>> -----Original Message-----
>> From: amit kachhap [mailto:amitdanielk@xxxxxxxxx] On Behalf Of Amit Daniel
>> Kachhap
>> Sent: Saturday, March 03, 2012 4:36 PM
>> To: linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx; mjg59@xxxxxxxxxxxxx; linux-
>> acpi@xxxxxxxxxxxxxxx; lenb@xxxxxxxxxx; linaro-dev@xxxxxxxxxxxxxxxx; lm-
>> sensors@xxxxxxxxxxxxxx; amit.kachhap@xxxxxxxxxx; eduardo.valentin@xxxxxx; R,
>> Durgadoss; patches@xxxxxxxxxx
>> Subject: [PATCH 1/4] thermal: exynos: Add thermal interface support for linux
>> thermal layer
>>
>> This codes uses the generic linux thermal layer and creates a bridge
>> between temperature sensors, linux thermal framework and cooling devices
>> for samsung exynos platform. This layer recieves or monitor the
>> temperature from the sensor and informs the generic thermal layer to take
>> the necessary cooling action.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@xxxxxxxxxx>
>> ---
>> drivers/thermal/Kconfig | 8 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/exynos_thermal.c | 272 ++++++++++++++++++++++++++++++++++++++
>> include/linux/exynos_thermal.h | 72 ++++++++++
>> 4 files changed, 353 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/thermal/exynos_thermal.c
>> create mode 100644 include/linux/exynos_thermal.h
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 298c1cd..4e8df56 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -29,3 +29,11 @@ config CPU_THERMAL
>> This will be useful for platforms using the generic thermal interface
>> and not the ACPI interface.
>> If you want this support, you should say Y or M here.
>> +
>> +config SAMSUNG_THERMAL_INTERFACE
>> + bool "Samsung Thermal interface support"
>> + depends on THERMAL && CPU_THERMAL
>> + help
>> + This is a samsung thermal interface which will be used as
>> + a link between sensors and cooling devices with linux thermal
>> + framework.
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 655cbc4..c67b6b2 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -4,3 +4,4 @@
>>
>> obj-$(CONFIG_THERMAL) += thermal_sys.o
>> obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
>> +obj-$(CONFIG_SAMSUNG_THERMAL_INTERFACE) += exynos_thermal.o
>> diff --git a/drivers/thermal/exynos_thermal.c
>> b/drivers/thermal/exynos_thermal.c
>> new file mode 100644
>> index 0000000..878d45c
>> --- /dev/null
>> +++ b/drivers/thermal/exynos_thermal.c
>> @@ -0,0 +1,272 @@
>> +/* linux/drivers/thermal/exynos_thermal.c
>> + *
>> + * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/cpu_cooling.h>
>> +#include <linux/exynos_thermal.h>
>> +
>> +#define MAX_COOLING_DEVICE 4
>> +struct exynos4_thermal_zone {
>> + unsigned int idle_interval;
>> + unsigned int active_interval;
>> + struct thermal_zone_device *therm_dev;
>> + struct thermal_cooling_device *cool_dev[MAX_COOLING_DEVICE];
>> + unsigned int cool_dev_size;
>> + struct platform_device *exynos4_dev;
>> + struct thermal_sensor_conf *sensor_conf;
>> +};
>> +
>> +static struct exynos4_thermal_zone *th_zone;
>> +
>> +static int exynos4_get_mode(struct thermal_zone_device *thermal,
>> + enum thermal_device_mode *mode)
>> +{
>> + if (th_zone->sensor_conf) {
>> + pr_info("Temperature sensor not initialised\n");
>> + *mode = THERMAL_DEVICE_DISABLED;
>> + } else
>> + *mode = THERMAL_DEVICE_ENABLED;
>> + return 0;
>> +}
>> +
>> +static int exynos4_set_mode(struct thermal_zone_device *thermal,
>> + enum thermal_device_mode mode)
>> +{
>> + if (!th_zone->therm_dev) {
>> + pr_notice("thermal zone not registered\n");
>> + return 0;
>> + }
>> + if (mode == THERMAL_DEVICE_ENABLED)
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->active_interval*1000;
>> + else
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->idle_interval*1000;
>
> If it is 'DISABLED' mode, shouldn't the polling delay be just 0 ?
Yes Ideally this should be zero. But I wanted thermal monitoring to
always happen with some long interval in case of error scenarios.
Anyway I will check this again.
>
>> +
>> + thermal_zone_device_update(th_zone->therm_dev);
>> + pr_info("thermal polling set for duration=%d sec\n",
>> + th_zone->therm_dev->polling_delay/1000);
>> + return 0;
>> +}
>> +
>> +/*This may be called from interrupt based temperature sensor*/
>> +void exynos4_report_trigger(void)
>> +{
>> + unsigned int monitor_temp;
>> +
>> + if (!th_zone || !th_zone->therm_dev)
>> + return;
>> +
>> + monitor_temp = th_zone->sensor_conf->trip_data.trip_val[0];
>
> Why are we checking only against the 0-th trip point ?
> Why not for other trip_val[i] also ?
Yes correct. Actually the trip temperatures are arranged in ascending order.
>
>> +
>> + thermal_zone_device_update(th_zone->therm_dev);
>> +
>> + mutex_lock(&th_zone->therm_dev->lock);
>> + if (th_zone->therm_dev->last_temperature > monitor_temp)
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->active_interval*1000;
>> + else
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->idle_interval*1000;
>> +
>> + kobject_uevent(&th_zone->therm_dev->device.kobj, KOBJ_CHANGE);
>
> Wouldn't it make more sense to pass the trip point id also as an 'env'
> parameter ? This way, the user space can easily figure out which trip
> point has been crossed.
Its a good suggestion. I will check if some uevent property allows that.
>
>> + mutex_unlock(&th_zone->therm_dev->lock);
>> +}
>> +
>> +static int exynos4_get_trip_type(struct thermal_zone_device *thermal, int
>> trip,
>> + enum thermal_trip_type *type)
>> +{
>> + if (trip == 0 || trip == 1)
>> + *type = THERMAL_TRIP_STATE_ACTIVE;
>> + else if (trip == 2)
>> + *type = THERMAL_TRIP_CRITICAL;
>> + else
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos4_get_trip_temp(struct thermal_zone_device *thermal, int
>> trip,
>> + unsigned long *temp)
>> +{
>> + /*Monitor zone*/
>> + if (trip == 0)
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[0];
>> + /*Warn zone*/
>> + else if (trip == 1)
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[1];
>> + /*Panic zone*/
>> + else if (trip == 2)
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[2];
>> + else
>> + return -EINVAL;
>> + /*convert the temperature into millicelsius*/
>> + *temp = *temp * 1000;
>> +
>> + return 0;
>> +}
>
> This could be:
>
> if (trip < 0 || trip >2) return -EINVAL;
> *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
> return 0;
Agreed. Will apply.
>
>> +
>> +static int exynos4_get_crit_temp(struct thermal_zone_device *thermal,
>> + unsigned long *temp)
>> +{
>> + /*Panic zone*/
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[2];
>> + /*convert the temperature into millicelsius*/
>> + *temp = *temp * 1000;
>> + return 0;
>> +}
>
> Why not make it, exynos4_get_trip_temp(thermal, 2, temp) ?
OK.
>
>> +
>> +static int exynos4_bind(struct thermal_zone_device *thermal,
>> + struct thermal_cooling_device *cdev)
>> +{
>> + /* 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\n");
>> + return -EINVAL;
>> + }
>> + if (thermal_zone_bind_cooling_device(thermal, 1, cdev)) {
>> + pr_err("error binding cooling dev\n");
>> + return -EINVAL;
>
> If we fail here, do you want to remove the earlier 'binding' also ?
Yes. Missed this error handling.
>
>> + }
>> +
>> + return 0;
>> +
>> +}
>> +
>> +static int exynos4_unbind(struct thermal_zone_device *thermal,
>> + struct thermal_cooling_device *cdev)
>> +{
>> + if (cdev != th_zone->cool_dev[0])
>> + return 0;
>> +
>> + if (thermal_zone_unbind_cooling_device(thermal, 0, cdev)) {
>> + pr_err("error unbinding cooling dev\n");
>> + return -EINVAL;
>
> I think we should still go ahead and try to 'unbind' the other one.
Yes. Missed this error handling.
>
>> + }
>> + if (thermal_zone_unbind_cooling_device(thermal, 1, cdev)) {
>> + pr_err("error unbinding cooling dev\n");
>> + return -EINVAL;
>> + }
>> + return 0;
>> +
>> +}
>> +
>> +static int exynos4_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;
>> +}
>> +
>> +/* bind callback functions to thermalzone */
>> +static struct thermal_zone_device_ops exynos4_dev_ops = {
>> + .bind = exynos4_bind,
>> + .unbind = exynos4_unbind,
>> + .get_temp = exynos4_get_temp,
>> + .get_mode = exynos4_get_mode,
>> + .set_mode = exynos4_set_mode,
>> + .get_trip_type = exynos4_get_trip_type,
>> + .get_trip_temp = exynos4_get_trip_temp,
>> + .get_crit_temp = exynos4_get_crit_temp,
>> +};
>> +
>> +int exynos4_register_thermal(struct thermal_sensor_conf *sensor_conf)
>> +{
>> + int ret, count, tab_size;
>> + struct freq_pctg_table *tab_ptr;
>> +
>> + if (!sensor_conf || !sensor_conf->read_temperature) {
>> + pr_err("Temperature sensor not initialised\n");
>> + return -EINVAL;
>> + }
>> +
>> + th_zone = kzalloc(sizeof(struct exynos4_thermal_zone), GFP_KERNEL);
>> + if (!th_zone) {
>> + ret = -ENOMEM;
>> + goto err_unregister;
>> + }
>> +
>> + th_zone->sensor_conf = sensor_conf;
>> +
>> + tab_ptr = (struct freq_pctg_table *)sensor_conf->cooling_data.freq_data;
>> + tab_size = sensor_conf->cooling_data.freq_pctg_count;
>> +
>> + /*Register the cpufreq cooling device*/
>> + th_zone->cool_dev_size = 1;
>> + count = 0;
>> + th_zone->cool_dev[count] = cpufreq_cooling_register(
>> + (struct freq_pctg_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, &exynos4_dev_ops, 0, 0, 0, 1000);
>> + if (IS_ERR(th_zone->therm_dev)) {
>> + pr_err("Failed to register thermal zone device\n");
>> + ret = -EINVAL;
>> + goto err_unregister;
>> + }
>> +
>> + th_zone->active_interval = 1;
>> + th_zone->idle_interval = 10;
>> +
>> + exynos4_set_mode(th_zone->therm_dev, THERMAL_DEVICE_DISABLED);
>> +
>> + pr_info("Exynos: Kernel Thermal management registered\n");
>> +
>> + return 0;
>> +
>> +err_unregister:
>> + exynos4_unregister_thermal();
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(exynos4_register_thermal);
>> +
>> +void exynos4_unregister_thermal(void)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < th_zone->cool_dev_size; i++) {
>> + if (th_zone && th_zone->cool_dev[i])
>> + cpufreq_cooling_unregister(th_zone->cool_dev[i]);
>> + }
>> +
>> + if (th_zone && th_zone->therm_dev)
>> + thermal_zone_device_unregister(th_zone->therm_dev);
>> +
>> + kfree(th_zone);
>> +
>> + pr_info("Exynos: Kernel Thermal management unregistered\n");
>> +}
>> +EXPORT_SYMBOL(exynos4_unregister_thermal);
>> diff --git a/include/linux/exynos_thermal.h b/include/linux/exynos_thermal.h
>> new file mode 100644
>> index 0000000..186e409
>> --- /dev/null
>> +++ b/include/linux/exynos_thermal.h
>> @@ -0,0 +1,72 @@
>> +/* linux/include/linux/exynos_thermal.h
>> + *
>> + * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#ifndef THERMAL_INTERFACE_H
>> +#define THERMAL_INTERFACE_H
>> +/* CPU Zone information */
>> +
>> +#define SENSOR_NAME_LEN 16
>> +#define MAX_TRIP_COUNT 8
>> +
>> +#define PANIC_ZONE 4
>> +#define WARN_ZONE 3
>> +#define MONITOR_ZONE 2
>> +#define SAFE_ZONE 1
>> +#define NO_ACTION 0
>
> I don't get why we need two separate SAFE and NO_ACTION
> zones..To me, both should be the same.
Yes not needed.
>
>> +
>> +
>> +struct thermal_trip_point_conf {
>> + int trip_val[MAX_TRIP_COUNT];
>> + int trip_count;
>> +};
>> +
>> +struct thermal_cooling_conf {
>> + struct freq_pctg_table freq_data[MAX_TRIP_COUNT];
>> + int freq_pctg_count;
>> +};
>> +
>> +/**
>> + * struct exynos4_tmu_platform_data
>> + * @name: name of the temperature sensor
>> + * @read_temperature: A function pointer to read temperature info
>> + * @private_data: Temperature sensor private data
>> + * @sensor_data: Sensor specific information like trigger temperature, level
>> + */
>> +struct thermal_sensor_conf {
>> + char name[SENSOR_NAME_LEN];
>> + int (*read_temperature)(void *data);
>> + struct thermal_trip_point_conf trip_data;
>> + struct thermal_cooling_conf cooling_data;
>
> Since both trip_count and freq_pctg_count are same at all
> times (please correct me if I am wrong) I think it's better
> to have a single 'count' variable inside this structure and move
> trip_val and freq_data to this structure directly.
No trip_count and pctg_count are different.
>
> One General Concern:
> Why do we even need this exynos_thermal layer between Thermal Framework
> and the Thermal Sensor Drivers ? I think the thermal sensor driver (in
> this case exynos_tmu.c) can directly register with the Thermal framework.
> This means, we will soon have platformXXX_thermal.c files for each
> platform, which is not really a good way to go IMHO.
> I also understand that the framework does not have alarm attributes,
> notification support etc..But We can add them if needed.
Yes even comments from Guenter Roeck and Mark Brown also point in the
same direction. Actually I wanted to separate the sensor h/w
implementation independent from thermal management algorithm so its
future modification may be clean. But looks like the agreement is to
merge them.
>
> I have reviewed your two sets of patches independently. My only
> request to you would be to post the next versions of both the patch
> sets at the same time, so that it becomes easier to understand and test.
Thanks even i also think this a good way.
>
> Thanks,
> Durga
>
>> + void *private_data;
>> +};
>> +
>> +/**
>> + * exynos4_register_thermal: Register to the exynos thermal interface.
>> + * @sensor_conf: Structure containing temperature sensor information
>> + *
>> + * returns zero on success, else negative errno.
>> + */
>> +int exynos4_register_thermal(struct thermal_sensor_conf *sensor_conf);
>> +
>> +/**
>> + * exynos4_unregister_thermal: Un-register from the exynos thermal interface.
>> + *
>> + * return not applicable.
>> + */
>> +void exynos4_unregister_thermal(void);
>> +
>> +/**
>> + * exynos4_report_trigger: Report any trigger level crossed in the
>> + * temperature sensor. This may be useful to take any cooling action.
>> + *
>> + * return not applicable.
>> + */
>> +extern void exynos4_report_trigger(void);
>> +#endif
>> --
>> 1.7.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/