by Amit Kachhap

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] thermal: exynos: Register the tmu sensor with the kernel thermal layer

On 9 May 2012 01:46, Andrew Morton <[email protected]> wrote:
> On Tue, ?8 May 2012 21:48:17 +0530
> Amit Daniel Kachhap <[email protected]> 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;
>> +}
>> +
>>
>> ...
>>
>

2012-05-09 12:44:55

by Amit Kachhap

[permalink] [raw]

Subject: Re: [PATCH v3 0/6] thermal: exynos: Add kernel thermal support for exynos platform

On 9 May 2012 01:36, Zhang, Rui <[email protected]> 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: [email protected] [mailto:[email protected]] On Behalf Of Amit Daniel Kachhap
> Sent: Tuesday, May 08, 2012 9:18 AM
> To: [email protected]; [email protected]
> Cc: R, Durgadoss; [email protected]; [email protected]; Zhang, Rui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> 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 <[email protected]>.
> *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 <[email protected]> and [email protected] 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 [email protected]
> *Incorporated several review comments from [email protected] *Moved the Temperature sensor driver from driver/hwmon/ to driver/mfd ?as discussed with Guenter Roeck <[email protected]> and ?Donggeun Kim <[email protected]> (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 [email protected] More majordomo info at ?http://vger.kernel.org/majordomo-info.html