[RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework

[RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework

[Amit Daniel Kachhap]

PATCH 1)  [thermal: Add a new trip type to use cooling device instance number]
This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
cooling device instance number and may be helpful for cpufreq cooling devices
to take the correct cooling action.

PATCH 2)  [thermal: Add generic cpu cooling implementation]
This patch 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.


Amit Daniel Kachhap (2):
  thermal: Add a new trip type to use cooling device instance number
  thermal: Add generic cpu cooling implementation

 Documentation/thermal/cpu-cooling-api.txt |   52 +++++
 Documentation/thermal/sysfs-api.txt       |    4 +-
 drivers/thermal/Kconfig                   |   11 +
 drivers/thermal/Makefile                  |    1 +
 drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
 drivers/thermal/thermal_sys.c             |   27 +++-
 include/linux/cpu_cooling.h               |   45 +++++
 include/linux/thermal.h                   |    1 +
 8 files changed, 440 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/thermal/cpu-cooling-api.txt
 create mode 100644 drivers/thermal/cpu_cooling.c
 create mode 100644 include/linux/cpu_cooling.h

[RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Amit Daniel Kachhap]

This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
device instance number. This helps the cooling device registered as
different instances to perform appropriate cooling action decision in
the set_cur_state call back function.

Also since the trip temperature's are in ascending order so some logic
is put in place to skip the un-necessary checks.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
---
 Documentation/thermal/sysfs-api.txt |    4 ++--
 drivers/thermal/thermal_sys.c       |   27 ++++++++++++++++++++++++++-
 include/linux/thermal.h             |    1 +
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index b61e46f..5c1d44e 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -184,8 +184,8 @@ trip_point_[0-*]_temp
 
 trip_point_[0-*]_type
 	Strings which indicate the type of the trip point.
-	E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
-	thermal zone.
+	E.g. it can be one of critical, hot, passive, active[0-1],
+	state-active[0-*] for ACPI thermal zone.
 	RO, Optional
 
 cdev[0-*]
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index dd9a574..72b1ab3 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
 		return sprintf(buf, "passive\n");
 	case THERMAL_TRIP_ACTIVE:
 		return sprintf(buf, "active\n");
+	case THERMAL_TRIP_STATE_ACTIVE:
+		return sprintf(buf, "state-active\n");
 	default:
 		return sprintf(buf, "unknown\n");
 	}
@@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
 void thermal_zone_device_update(struct thermal_zone_device *tz)
 {
 	int count, ret = 0;
-	long temp, trip_temp;
+	long temp, trip_temp, max_state, last_trip_change = 0;
 	enum thermal_trip_type trip_type;
 	struct thermal_cooling_device_instance *instance;
 	struct thermal_cooling_device *cdev;
@@ -1086,6 +1088,29 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 					cdev->ops->set_cur_state(cdev, 0);
 			}
 			break;
+		case THERMAL_TRIP_STATE_ACTIVE:
+			list_for_each_entry(instance, &tz->cooling_devices,
+					    node) {
+				if (instance->trip != count)
+					continue;
+
+				if (temp <= last_trip_change)
+					continue;
+
+				cdev = instance->cdev;
+				cdev->ops->get_max_state(cdev, &max_state);
+
+				if ((temp >= trip_temp) &&
+						((count + 1) <= max_state))
+					cdev->ops->set_cur_state(cdev,
+								count + 1);
+				else if ((temp < trip_temp) &&
+							(count <= max_state))
+					cdev->ops->set_cur_state(cdev, count);
+
+				last_trip_change = trip_temp;
+			}
+			break;
 		case THERMAL_TRIP_PASSIVE:
 			if (temp >= trip_temp || tz->passive)
 				thermal_zone_device_passive(tz, temp,
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 47b4a27..d7d0a27 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -42,6 +42,7 @@ enum thermal_trip_type {
 	THERMAL_TRIP_PASSIVE,
 	THERMAL_TRIP_HOT,
 	THERMAL_TRIP_CRITICAL,
+	THERMAL_TRIP_STATE_ACTIVE,
 };
 
 struct thermal_zone_device_ops {
-- 
1.7.1

Re: [linux-pm] [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Vincent Guittot]

Hi Amit,

I'm not sure that using the trip index for setting the state of a
cooling device is a generic solution because you are adding a
dependency between the trip index and the cooling device state that
might be difficult to handle. This dependency implies that a cooling
device like cpufreq_cooling_device must be registered in the 1st trips
of a thermal_zone which is not possible when we want to register 2
cpufreq_cooling_devices in the same thermal_zone.
You should only rely on the current and last temperatures to detect if
a trip_temp has been crossed and you should increase or decrease the
current state of the cooling device accordingly.

something like below should work with cpufreq_cooling_device and will
not add any constraint on the trip index. The state of a cooling
device is increased/decreased once for each trip

+		case THERMAL_TRIP_STATE_ACTIVE:
+			list_for_each_entry(instance, &tz->cooling_devices,
+					    node) {
+				if (instance->trip != count)
+					continue;
+
+				cdev = instance->cdev;
+
+				if ((temp >= trip_temp)
+					&& (trip_temp > tz->last_temperature)) {
+					cdev->ops->get_max_state(cdev,
+							&max_state);
+					cdev->ops->get_cur_state(cdev,
+							&current_state);
+					if (++current_state <= max_state)
+						cdev->ops->set_cur_state(cdev,
+								current_state);
+				}
+				else if ((temp < trip_temp)
+					&& (trip_temp <= tz->last_temperature)) {
+					cdev->ops->get_cur_state(cdev,
+							&current_state);
+					if (current_state > 0)
+						cdev->ops->set_cur_state(cdev,
+								--current_state);
+			}
+			break;

Regards,
Vincent

On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
> device instance number. This helps the cooling device registered as
> different instances to perform appropriate cooling action decision in
> the set_cur_state call back function.
>
> Also since the trip temperature's are in ascending order so some logic
> is put in place to skip the un-necessary checks.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
>  Documentation/thermal/sysfs-api.txt |    4 ++--
>  drivers/thermal/thermal_sys.c       |   27 ++++++++++++++++++++++++++-
>  include/linux/thermal.h             |    1 +
>  3 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index b61e46f..5c1d44e 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
>
>  trip_point_[0-*]_type
>        Strings which indicate the type of the trip point.
> -       E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
> -       thermal zone.
> +       E.g. it can be one of critical, hot, passive, active[0-1],
> +       state-active[0-*] for ACPI thermal zone.
>        RO, Optional
>
>  cdev[0-*]
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index dd9a574..72b1ab3 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
>                return sprintf(buf, "passive\n");
>        case THERMAL_TRIP_ACTIVE:
>                return sprintf(buf, "active\n");
> +       case THERMAL_TRIP_STATE_ACTIVE:
> +               return sprintf(buf, "state-active\n");
>        default:
>                return sprintf(buf, "unknown\n");
>        }
> @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>  void thermal_zone_device_update(struct thermal_zone_device *tz)
>  {
>        int count, ret = 0;
> -       long temp, trip_temp;
> +       long temp, trip_temp, max_state, last_trip_change = 0;
>        enum thermal_trip_type trip_type;
>        struct thermal_cooling_device_instance *instance;
>        struct thermal_cooling_device *cdev;
> @@ -1086,6 +1088,29 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>                                        cdev->ops->set_cur_state(cdev, 0);
>                        }
>                        break;
> +               case THERMAL_TRIP_STATE_ACTIVE:
> +                       list_for_each_entry(instance, &tz->cooling_devices,
> +                                           node) {
> +                               if (instance->trip != count)
> +                                       continue;
> +
> +                               if (temp <= last_trip_change)
> +                                       continue;
> +
> +                               cdev = instance->cdev;
> +                               cdev->ops->get_max_state(cdev, &max_state);
> +
> +                               if ((temp >= trip_temp) &&
> +                                               ((count + 1) <= max_state))
> +                                       cdev->ops->set_cur_state(cdev,
> +                                                               count + 1);
> +                               else if ((temp < trip_temp) &&
> +                                                       (count <= max_state))
> +                                       cdev->ops->set_cur_state(cdev, count);
> +
> +                               last_trip_change = trip_temp;
> +                       }
> +                       break;
>                case THERMAL_TRIP_PASSIVE:
>                        if (temp >= trip_temp || tz->passive)
>                                thermal_zone_device_passive(tz, temp,
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 47b4a27..d7d0a27 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -42,6 +42,7 @@ enum thermal_trip_type {
>        THERMAL_TRIP_PASSIVE,
>        THERMAL_TRIP_HOT,
>        THERMAL_TRIP_CRITICAL,
> +       THERMAL_TRIP_STATE_ACTIVE,
>  };
>
>  struct thermal_zone_device_ops {
> --
> 1.7.1
>
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [linux-pm] [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Amit Kachhap]

 Hi Vincent,

Thanks for the review.
Well actually your are correct that current temperature and last
temperature can be used to increase or decrease the cpu frequency. But
this has to be done again in cooling devices so to make the cooling
devices generic and to avoid the temperature comparison again this new
trip type passes the cooling device instance id.
Also about your queries that this may add dependency between trip
index and cooling state. This is actually needed and this dependency
is created when the cooling device is binded with trip points(For
cpufreq type cooling device just the instance of cooling device is
associated with trip points). More over the existing PASSIVE cooling
trip type does the same thing and iterates across all the cooling
state.

 Thanks,
 Amit Daniel
>
> On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>
>> Hi Amit,
>>
>> I'm not sure that using the trip index for setting the state of a
>> cooling device is a generic solution because you are adding a
>> dependency between the trip index and the cooling device state that
>> might be difficult to handle. This dependency implies that a cooling
>> device like cpufreq_cooling_device must be registered in the 1st trips
>> of a thermal_zone which is not possible when we want to register 2
>> cpufreq_cooling_devices in the same thermal_zone.
>> You should only rely on the current and last temperatures to detect if
>> a trip_temp has been crossed and you should increase or decrease the
>> current state of the cooling device accordingly.
>>
>> something like below should work with cpufreq_cooling_device and will
>> not add any constraint on the trip index. The state of a cooling
>> device is increased/decreased once for each trip
>>
>> +               case THERMAL_TRIP_STATE_ACTIVE:
>> +                       list_for_each_entry(instance, &tz->cooling_devices,
>> +                                           node) {
>> +                               if (instance->trip != count)
>> +                                       continue;
>> +
>> +                               cdev = instance->cdev;
>> +
>> +                               if ((temp >= trip_temp)
>> +                                       && (trip_temp > tz->last_temperature)) {
>> +                                       cdev->ops->get_max_state(cdev,
>> +                                                       &max_state);
>> +                                       cdev->ops->get_cur_state(cdev,
>> +                                                       &current_state);
>> +                                       if (++current_state <= max_state)
>> +                                               cdev->ops->set_cur_state(cdev,
>> +                                                               current_state);
>> +                               }
>> +                               else if ((temp < trip_temp)
>> +                                       && (trip_temp <= tz->last_temperature)) {
>> +                                       cdev->ops->get_cur_state(cdev,
>> +                                                       &current_state);
>> +                                       if (current_state > 0)
>> +                                               cdev->ops->set_cur_state(cdev,
>> +                                                               --current_state);
>> +                       }
>> +                       break;
>>
>> Regards,
>> Vincent
>>
>> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
>> > trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
>> > device instance number. This helps the cooling device registered as
>> > different instances to perform appropriate cooling action decision in
>> > the set_cur_state call back function.
>> >
>> > Also since the trip temperature's are in ascending order so some logic
>> > is put in place to skip the un-necessary checks.
>> >
>> > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> > ---
>> >  Documentation/thermal/sysfs-api.txt |    4 ++--
>> >  drivers/thermal/thermal_sys.c       |   27 ++++++++++++++++++++++++++-
>> >  include/linux/thermal.h             |    1 +
>> >  3 files changed, 29 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>> > index b61e46f..5c1d44e 100644
>> > --- a/Documentation/thermal/sysfs-api.txt
>> > +++ b/Documentation/thermal/sysfs-api.txt
>> > @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
>> >
>> >  trip_point_[0-*]_type
>> >        Strings which indicate the type of the trip point.
>> > -       E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
>> > -       thermal zone.
>> > +       E.g. it can be one of critical, hot, passive, active[0-1],
>> > +       state-active[0-*] for ACPI thermal zone.
>> >        RO, Optional
>> >
>> >  cdev[0-*]
>> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> > index dd9a574..72b1ab3 100644
>> > --- a/drivers/thermal/thermal_sys.c
>> > +++ b/drivers/thermal/thermal_sys.c
>> > @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
>> >                return sprintf(buf, "passive\n");
>> >        case THERMAL_TRIP_ACTIVE:
>> >                return sprintf(buf, "active\n");
>> > +       case THERMAL_TRIP_STATE_ACTIVE:
>> > +               return sprintf(buf, "state-active\n");
>> >        default:
>> >                return sprintf(buf, "unknown\n");
>> >        }
>> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
>> >  {
>> >        int count, ret = 0;
>> > -       long temp, trip_temp;
>> > +       long temp, trip_temp, max_state, last_trip_change = 0;
>> >        enum thermal_trip_type trip_type;
>> >        struct thermal_cooling_device_instance *instance;
>> >        struct thermal_cooling_device *cdev;
>> > @@ -1086,6 +1088,29 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>> >                                        cdev->ops->set_cur_state(cdev, 0);
>> >                        }
>> >                        break;
>> > +               case THERMAL_TRIP_STATE_ACTIVE:
>> > +                       list_for_each_entry(instance, &tz->cooling_devices,
>> > +                                           node) {
>> > +                               if (instance->trip != count)
>> > +                                       continue;
>> > +
>> > +                               if (temp <= last_trip_change)
>> > +                                       continue;
>> > +
>> > +                               cdev = instance->cdev;
>> > +                               cdev->ops->get_max_state(cdev, &max_state);
>> > +
>> > +                               if ((temp >= trip_temp) &&
>> > +                                               ((count + 1) <= max_state))
>> > +                                       cdev->ops->set_cur_state(cdev,
>> > +                                                               count + 1);
>> > +                               else if ((temp < trip_temp) &&
>> > +                                                       (count <= max_state))
>> > +                                       cdev->ops->set_cur_state(cdev, count);
>> > +
>> > +                               last_trip_change = trip_temp;
>> > +                       }
>> > +                       break;
>> >                case THERMAL_TRIP_PASSIVE:
>> >                        if (temp >= trip_temp || tz->passive)
>> >                                thermal_zone_device_passive(tz, temp,
>> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> > index 47b4a27..d7d0a27 100644
>> > --- a/include/linux/thermal.h
>> > +++ b/include/linux/thermal.h
>> > @@ -42,6 +42,7 @@ enum thermal_trip_type {
>> >        THERMAL_TRIP_PASSIVE,
>> >        THERMAL_TRIP_HOT,
>> >        THERMAL_TRIP_CRITICAL,
>> > +       THERMAL_TRIP_STATE_ACTIVE,
>> >  };
>> >
>> >  struct thermal_zone_device_ops {
>> > --
>> > 1.7.1
>> >
>> > _______________________________________________
>> > linux-pm mailing list
>> > linux-pm@lists.linux-foundation.org
>> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>
>

Re: [linux-pm] [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Rob Lee]

Hey Amit/Vincent,

It appears that with this implementation the STATE_ACTIVE trip number
used will also be the index of the cool_freq_tab used.  If that is
true, then perhaps a common structure would be beneficial that links
each STATE_ACTIVE trip point with its corresponding cooling data.

BR,
Rob

On Tue, Dec 20, 2011 at 11:11 PM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>  Hi Vincent,
>
> Thanks for the review.
> Well actually your are correct that current temperature and last
> temperature can be used to increase or decrease the cpu frequency. But
> this has to be done again in cooling devices so to make the cooling
> devices generic and to avoid the temperature comparison again this new
> trip type passes the cooling device instance id.
> Also about your queries that this may add dependency between trip
> index and cooling state. This is actually needed and this dependency
> is created when the cooling device is binded with trip points(For
> cpufreq type cooling device just the instance of cooling device is
> associated with trip points). More over the existing PASSIVE cooling
> trip type does the same thing and iterates across all the cooling
> state.
>
>  Thanks,
>  Amit Daniel
>>
>> On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>>
>>> Hi Amit,
>>>
>>> I'm not sure that using the trip index for setting the state of a
>>> cooling device is a generic solution because you are adding a
>>> dependency between the trip index and the cooling device state that
>>> might be difficult to handle. This dependency implies that a cooling
>>> device like cpufreq_cooling_device must be registered in the 1st trips
>>> of a thermal_zone which is not possible when we want to register 2
>>> cpufreq_cooling_devices in the same thermal_zone.
>>> You should only rely on the current and last temperatures to detect if
>>> a trip_temp has been crossed and you should increase or decrease the
>>> current state of the cooling device accordingly.
>>>
>>> something like below should work with cpufreq_cooling_device and will
>>> not add any constraint on the trip index. The state of a cooling
>>> device is increased/decreased once for each trip
>>>
>>> +               case THERMAL_TRIP_STATE_ACTIVE:
>>> +                       list_for_each_entry(instance, &tz->cooling_devices,
>>> +                                           node) {
>>> +                               if (instance->trip != count)
>>> +                                       continue;
>>> +
>>> +                               cdev = instance->cdev;
>>> +
>>> +                               if ((temp >= trip_temp)
>>> +                                       && (trip_temp > tz->last_temperature)) {
>>> +                                       cdev->ops->get_max_state(cdev,
>>> +                                                       &max_state);
>>> +                                       cdev->ops->get_cur_state(cdev,
>>> +                                                       &current_state);
>>> +                                       if (++current_state <= max_state)
>>> +                                               cdev->ops->set_cur_state(cdev,
>>> +                                                               current_state);
>>> +                               }
>>> +                               else if ((temp < trip_temp)
>>> +                                       && (trip_temp <= tz->last_temperature)) {
>>> +                                       cdev->ops->get_cur_state(cdev,
>>> +                                                       &current_state);
>>> +                                       if (current_state > 0)
>>> +                                               cdev->ops->set_cur_state(cdev,
>>> +                                                               --current_state);
>>> +                       }
>>> +                       break;
>>>
>>> Regards,
>>> Vincent
>>>
>>> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>>> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
>>> > trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
>>> > device instance number. This helps the cooling device registered as
>>> > different instances to perform appropriate cooling action decision in
>>> > the set_cur_state call back function.
>>> >
>>> > Also since the trip temperature's are in ascending order so some logic
>>> > is put in place to skip the un-necessary checks.
>>> >
>>> > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>>> > ---
>>> >  Documentation/thermal/sysfs-api.txt |    4 ++--
>>> >  drivers/thermal/thermal_sys.c       |   27 ++++++++++++++++++++++++++-
>>> >  include/linux/thermal.h             |    1 +
>>> >  3 files changed, 29 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>>> > index b61e46f..5c1d44e 100644
>>> > --- a/Documentation/thermal/sysfs-api.txt
>>> > +++ b/Documentation/thermal/sysfs-api.txt
>>> > @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
>>> >
>>> >  trip_point_[0-*]_type
>>> >        Strings which indicate the type of the trip point.
>>> > -       E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
>>> > -       thermal zone.
>>> > +       E.g. it can be one of critical, hot, passive, active[0-1],
>>> > +       state-active[0-*] for ACPI thermal zone.
>>> >        RO, Optional
>>> >
>>> >  cdev[0-*]
>>> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>>> > index dd9a574..72b1ab3 100644
>>> > --- a/drivers/thermal/thermal_sys.c
>>> > +++ b/drivers/thermal/thermal_sys.c
>>> > @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
>>> >                return sprintf(buf, "passive\n");
>>> >        case THERMAL_TRIP_ACTIVE:
>>> >                return sprintf(buf, "active\n");
>>> > +       case THERMAL_TRIP_STATE_ACTIVE:
>>> > +               return sprintf(buf, "state-active\n");
>>> >        default:
>>> >                return sprintf(buf, "unknown\n");
>>> >        }
>>> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>>> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
>>> >  {
>>> >        int count, ret = 0;
>>> > -       long temp, trip_temp;
>>> > +       long temp, trip_temp, max_state, last_trip_change = 0;
>>> >        enum thermal_trip_type trip_type;
>>> >        struct thermal_cooling_device_instance *instance;
>>> >        struct thermal_cooling_device *cdev;
>>> > @@ -1086,6 +1088,29 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>>> >                                        cdev->ops->set_cur_state(cdev, 0);
>>> >                        }
>>> >                        break;
>>> > +               case THERMAL_TRIP_STATE_ACTIVE:
>>> > +                       list_for_each_entry(instance, &tz->cooling_devices,
>>> > +                                           node) {
>>> > +                               if (instance->trip != count)
>>> > +                                       continue;
>>> > +
>>> > +                               if (temp <= last_trip_change)
>>> > +                                       continue;
>>> > +
>>> > +                               cdev = instance->cdev;
>>> > +                               cdev->ops->get_max_state(cdev, &max_state);
>>> > +
>>> > +                               if ((temp >= trip_temp) &&
>>> > +                                               ((count + 1) <= max_state))
>>> > +                                       cdev->ops->set_cur_state(cdev,
>>> > +                                                               count + 1);
>>> > +                               else if ((temp < trip_temp) &&
>>> > +                                                       (count <= max_state))
>>> > +                                       cdev->ops->set_cur_state(cdev, count);
>>> > +
>>> > +                               last_trip_change = trip_temp;
>>> > +                       }
>>> > +                       break;
>>> >                case THERMAL_TRIP_PASSIVE:
>>> >                        if (temp >= trip_temp || tz->passive)
>>> >                                thermal_zone_device_passive(tz, temp,
>>> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> > index 47b4a27..d7d0a27 100644
>>> > --- a/include/linux/thermal.h
>>> > +++ b/include/linux/thermal.h
>>> > @@ -42,6 +42,7 @@ enum thermal_trip_type {
>>> >        THERMAL_TRIP_PASSIVE,
>>> >        THERMAL_TRIP_HOT,
>>> >        THERMAL_TRIP_CRITICAL,
>>> > +       THERMAL_TRIP_STATE_ACTIVE,
>>> >  };
>>> >
>>> >  struct thermal_zone_device_ops {
>>> > --
>>> > 1.7.1
>>> >
>>> > _______________________________________________
>>> > linux-pm mailing list
>>> > linux-pm@lists.linux-foundation.org
>>> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-pm] [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Amit Kachhap]

Hi Rob,

I got your point. The main idea of doing like this is to keep the
cooling implementation independent from thermal zone
algorithms(thermal_sys.c). But binding freq_tab index to the trip
numbers may be not be bad idea. I will give more thought into it in
the next patchset.

Regards,
Amit D

On 11 January 2012 13:42, Rob Lee <rob.lee@linaro.org> wrote:
> Hey Amit/Vincent,
>
> It appears that with this implementation the STATE_ACTIVE trip number
> used will also be the index of the cool_freq_tab used.  If that is
> true, then perhaps a common structure would be beneficial that links
> each STATE_ACTIVE trip point with its corresponding cooling data.
>
> BR,
> Rob
>
> On Tue, Dec 20, 2011 at 11:11 PM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>>  Hi Vincent,
>>
>> Thanks for the review.
>> Well actually your are correct that current temperature and last
>> temperature can be used to increase or decrease the cpu frequency. But
>> this has to be done again in cooling devices so to make the cooling
>> devices generic and to avoid the temperature comparison again this new
>> trip type passes the cooling device instance id.
>> Also about your queries that this may add dependency between trip
>> index and cooling state. This is actually needed and this dependency
>> is created when the cooling device is binded with trip points(For
>> cpufreq type cooling device just the instance of cooling device is
>> associated with trip points). More over the existing PASSIVE cooling
>> trip type does the same thing and iterates across all the cooling
>> state.
>>
>>  Thanks,
>>  Amit Daniel
>>>
>>> On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>>>
>>>> Hi Amit,
>>>>
>>>> I'm not sure that using the trip index for setting the state of a
>>>> cooling device is a generic solution because you are adding a
>>>> dependency between the trip index and the cooling device state that
>>>> might be difficult to handle. This dependency implies that a cooling
>>>> device like cpufreq_cooling_device must be registered in the 1st trips
>>>> of a thermal_zone which is not possible when we want to register 2
>>>> cpufreq_cooling_devices in the same thermal_zone.
>>>> You should only rely on the current and last temperatures to detect if
>>>> a trip_temp has been crossed and you should increase or decrease the
>>>> current state of the cooling device accordingly.
>>>>
>>>> something like below should work with cpufreq_cooling_device and will
>>>> not add any constraint on the trip index. The state of a cooling
>>>> device is increased/decreased once for each trip
>>>>
>>>> +               case THERMAL_TRIP_STATE_ACTIVE:
>>>> +                       list_for_each_entry(instance, &tz->cooling_devices,
>>>> +                                           node) {
>>>> +                               if (instance->trip != count)
>>>> +                                       continue;
>>>> +
>>>> +                               cdev = instance->cdev;
>>>> +
>>>> +                               if ((temp >= trip_temp)
>>>> +                                       && (trip_temp > tz->last_temperature)) {
>>>> +                                       cdev->ops->get_max_state(cdev,
>>>> +                                                       &max_state);
>>>> +                                       cdev->ops->get_cur_state(cdev,
>>>> +                                                       &current_state);
>>>> +                                       if (++current_state <= max_state)
>>>> +                                               cdev->ops->set_cur_state(cdev,
>>>> +                                                               current_state);
>>>> +                               }
>>>> +                               else if ((temp < trip_temp)
>>>> +                                       && (trip_temp <= tz->last_temperature)) {
>>>> +                                       cdev->ops->get_cur_state(cdev,
>>>> +                                                       &current_state);
>>>> +                                       if (current_state > 0)
>>>> +                                               cdev->ops->set_cur_state(cdev,
>>>> +                                                               --current_state);
>>>> +                       }
>>>> +                       break;
>>>>
>>>> Regards,
>>>> Vincent
>>>>
>>>> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>>>> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
>>>> > trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
>>>> > device instance number. This helps the cooling device registered as
>>>> > different instances to perform appropriate cooling action decision in
>>>> > the set_cur_state call back function.
>>>> >
>>>> > Also since the trip temperature's are in ascending order so some logic
>>>> > is put in place to skip the un-necessary checks.
>>>> >
>>>> > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>>>> > ---
>>>> >  Documentation/thermal/sysfs-api.txt |    4 ++--
>>>> >  drivers/thermal/thermal_sys.c       |   27 ++++++++++++++++++++++++++-
>>>> >  include/linux/thermal.h             |    1 +
>>>> >  3 files changed, 29 insertions(+), 3 deletions(-)
>>>> >
>>>> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>>>> > index b61e46f..5c1d44e 100644
>>>> > --- a/Documentation/thermal/sysfs-api.txt
>>>> > +++ b/Documentation/thermal/sysfs-api.txt
>>>> > @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
>>>> >
>>>> >  trip_point_[0-*]_type
>>>> >        Strings which indicate the type of the trip point.
>>>> > -       E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
>>>> > -       thermal zone.
>>>> > +       E.g. it can be one of critical, hot, passive, active[0-1],
>>>> > +       state-active[0-*] for ACPI thermal zone.
>>>> >        RO, Optional
>>>> >
>>>> >  cdev[0-*]
>>>> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>>>> > index dd9a574..72b1ab3 100644
>>>> > --- a/drivers/thermal/thermal_sys.c
>>>> > +++ b/drivers/thermal/thermal_sys.c
>>>> > @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
>>>> >                return sprintf(buf, "passive\n");
>>>> >        case THERMAL_TRIP_ACTIVE:
>>>> >                return sprintf(buf, "active\n");
>>>> > +       case THERMAL_TRIP_STATE_ACTIVE:
>>>> > +               return sprintf(buf, "state-active\n");
>>>> >        default:
>>>> >                return sprintf(buf, "unknown\n");
>>>> >        }
>>>> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>>>> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
>>>> >  {
>>>> >        int count, ret = 0;
>>>> > -       long temp, trip_temp;
>>>> > +       long temp, trip_temp, max_state, last_trip_change = 0;
>>>> >        enum thermal_trip_type trip_type;
>>>> >        struct thermal_cooling_device_instance *instance;
>>>> >        struct thermal_cooling_device *cdev;
>>>> > @@ -1086,6 +1088,29 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>>>> >                                        cdev->ops->set_cur_state(cdev, 0);
>>>> >                        }
>>>> >                        break;
>>>> > +               case THERMAL_TRIP_STATE_ACTIVE:
>>>> > +                       list_for_each_entry(instance, &tz->cooling_devices,
>>>> > +                                           node) {
>>>> > +                               if (instance->trip != count)
>>>> > +                                       continue;
>>>> > +
>>>> > +                               if (temp <= last_trip_change)
>>>> > +                                       continue;
>>>> > +
>>>> > +                               cdev = instance->cdev;
>>>> > +                               cdev->ops->get_max_state(cdev, &max_state);
>>>> > +
>>>> > +                               if ((temp >= trip_temp) &&
>>>> > +                                               ((count + 1) <= max_state))
>>>> > +                                       cdev->ops->set_cur_state(cdev,
>>>> > +                                                               count + 1);
>>>> > +                               else if ((temp < trip_temp) &&
>>>> > +                                                       (count <= max_state))
>>>> > +                                       cdev->ops->set_cur_state(cdev, count);
>>>> > +
>>>> > +                               last_trip_change = trip_temp;
>>>> > +                       }
>>>> > +                       break;
>>>> >                case THERMAL_TRIP_PASSIVE:
>>>> >                        if (temp >= trip_temp || tz->passive)
>>>> >                                thermal_zone_device_passive(tz, temp,
>>>> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> > index 47b4a27..d7d0a27 100644
>>>> > --- a/include/linux/thermal.h
>>>> > +++ b/include/linux/thermal.h
>>>> > @@ -42,6 +42,7 @@ enum thermal_trip_type {
>>>> >        THERMAL_TRIP_PASSIVE,
>>>> >        THERMAL_TRIP_HOT,
>>>> >        THERMAL_TRIP_CRITICAL,
>>>> > +       THERMAL_TRIP_STATE_ACTIVE,
>>>> >  };
>>>> >
>>>> >  struct thermal_zone_device_ops {
>>>> > --
>>>> > 1.7.1
>>>> >
>>>> > _______________________________________________
>>>> > linux-pm mailing list
>>>> > linux-pm@lists.linux-foundation.org
>>>> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Matthew Garrett]

I'm not really a fan of this as it stands - the name isn't very 
intuitive and the code's pretty difficult to read. Would the following 
(incomplete and obviously untested) not have the effect you want? Then 
you register multiple trip points with the same cooling device but 
different private values, and the state set does whatever you want it 
to. Or am I misunderstanding the problem you're trying to solve?

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 220ce7e..817f2ba 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -50,6 +50,7 @@ struct thermal_cooling_device_instance {
 	char attr_name[THERMAL_NAME_LENGTH];
 	struct device_attribute attr;
 	struct list_head node;
+	unsigned long private;
 };
 
 static DEFINE_IDR(thermal_tz_idr);
@@ -909,7 +910,8 @@ static struct class thermal_class = {
  * @ops:		standard thermal cooling devices callbacks.
  */
 struct thermal_cooling_device *thermal_cooling_device_register(
-     char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
+	char *type, void *devdata, const struct thermal_cooling_device_ops *ops,
+	unsigned long private)
 {
 	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *pos;
@@ -936,6 +938,7 @@ struct thermal_cooling_device *thermal_cooling_device_register(
 	cdev->ops = ops;
 	cdev->device.class = &thermal_class;
 	cdev->devdata = devdata;
+	cdev->private = private;
 	dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
 	result = device_register(&cdev->device);
 	if (result) {
@@ -1079,11 +1082,14 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 					continue;
 
 				cdev = instance->cdev;
-
-				if (temp >= trip_temp)
-					cdev->ops->set_cur_state(cdev, 1);
-				else
-					cdev->ops->set_cur_state(cdev, 0);
+				if (cdev->private) {
+					cdev->ops->set_cur_state(cdev, cdev->private);
+				} else {
+					if (temp >= trip_temp)
+						cdev->ops->set_cur_state(cdev, 1);
+					else
+						cdev->ops->set_cur_state(cdev, 0);
+				}
 			}
 			break;
 		case THERMAL_TRIP_PASSIVE:
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 796f1ff..04aac09 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -148,7 +148,7 @@ int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
 				       struct thermal_cooling_device *);
 void thermal_zone_device_update(struct thermal_zone_device *);
 struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
-		const struct thermal_cooling_device_ops *);
+		     const struct thermal_cooling_device_ops *, unsigned long private);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
 
 #ifdef CONFIG_NET


-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Amit Kachhap]

On 1 February 2012 20:19, Matthew Garrett <mjg@redhat.com> wrote:
>
> I'm not really a fan of this as it stands - the name isn't very
> intuitive and the code's pretty difficult to read. Would the following
> (incomplete and obviously untested) not have the effect you want? Then
> you register multiple trip points with the same cooling device but
> different private values, and the state set does whatever you want it
> to. Or am I misunderstanding the problem you're trying to solve?

Thanks for the detailed review of the patch. Actually i tried to merge
the benefits of trip type ACTIVE and PASSIVE into one so this name.
This new trip type is just like ACTIVE but instead of OFF(0)/ON(1)
all values greater then 0 is on. Anyways I looked at your
implementation below but this will not solve the purpose as
thermal_cooling_device_register() need to be be called only once to
register a cooling device such cpu frequency based. However the same
cooling device may be binded many times to different trips.
Say,
1) thermal_zone_bind_cooling_device(tz_dev, 0, cdev);
2) thermal_zone_bind_cooling_device(tz_dev, 1, cdev);
3) thermal_zone_bind_cooling_device(tz_dev, 2, cdev);

0,1, 2 are nothing but trip points so the set_cur_state should be called like
set_cur_state(cdev, 0)
set_cur_state(cdev, 1)
set_cur_state(cdev, 2) when the trip point threshold are crossed.

Yeah I agree that implementation logic looks complex but this to
prevent the lower temp trip points cooling handlers to be called. I
will surely make this better in next version.

>
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 220ce7e..817f2ba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -50,6 +50,7 @@ struct thermal_cooling_device_instance {
>        char attr_name[THERMAL_NAME_LENGTH];
>        struct device_attribute attr;
>        struct list_head node;
> +       unsigned long private;
>  };
>
>  static DEFINE_IDR(thermal_tz_idr);
> @@ -909,7 +910,8 @@ static struct class thermal_class = {
>  * @ops:               standard thermal cooling devices callbacks.
>  */
>  struct thermal_cooling_device *thermal_cooling_device_register(
> -     char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
> +       char *type, void *devdata, const struct thermal_cooling_device_ops *ops,
> +       unsigned long private)
>  {
>        struct thermal_cooling_device *cdev;
>        struct thermal_zone_device *pos;
> @@ -936,6 +938,7 @@ struct thermal_cooling_device *thermal_cooling_device_register(
>        cdev->ops = ops;
>        cdev->device.class = &thermal_class;
>        cdev->devdata = devdata;
> +       cdev->private = private;
>        dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
>        result = device_register(&cdev->device);
>        if (result) {
> @@ -1079,11 +1082,14 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>                                        continue;
>
>                                cdev = instance->cdev;
> -
> -                               if (temp >= trip_temp)
> -                                       cdev->ops->set_cur_state(cdev, 1);
> -                               else
> -                                       cdev->ops->set_cur_state(cdev, 0);
> +                               if (cdev->private) {
> +                                       cdev->ops->set_cur_state(cdev, cdev->private);
> +                               } else {
> +                                       if (temp >= trip_temp)
> +                                               cdev->ops->set_cur_state(cdev, 1);
> +                                       else
> +                                               cdev->ops->set_cur_state(cdev, 0);
> +                               }
>                        }
>                        break;
>                case THERMAL_TRIP_PASSIVE:
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 796f1ff..04aac09 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -148,7 +148,7 @@ int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
>                                       struct thermal_cooling_device *);
>  void thermal_zone_device_update(struct thermal_zone_device *);
>  struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
> -               const struct thermal_cooling_device_ops *);
> +                    const struct thermal_cooling_device_ops *, unsigned long private);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>
>  #ifdef CONFIG_NET
>
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org

[RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Amit Daniel Kachhap]

This patch adds support for generic cpu thermal cooling low level
implementations using frequency scaling and cpuhotplugg currently.
Different cpu related cooling devices can be registered by the
user and the binding of these cooling devices to the corresponding
trip points can be easily done as the registration API's return the
cooling device pointer.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
---
 Documentation/thermal/cpu-cooling-api.txt |   52 +++++
 drivers/thermal/Kconfig                   |   11 +
 drivers/thermal/Makefile                  |    1 +
 drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
 include/linux/cpu_cooling.h               |   45 +++++
 5 files changed, 411 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/thermal/cpu-cooling-api.txt
 create mode 100644 drivers/thermal/cpu_cooling.c
 create mode 100644 include/linux/cpu_cooling.h

diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
new file mode 100644
index 0000000..d30b4f2
--- /dev/null
+++ b/Documentation/thermal/cpu-cooling-api.txt
@@ -0,0 +1,52 @@
+CPU cooling api's How To
+===================================
+
+Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
+
+Updated: 13 Dec 2011
+
+Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
+
+0. Introduction
+
+The generic cpu cooling(freq clipping, cpuhotplug) provides
+registration/unregistration api's to the user. The binding of the cooling
+devices to the trip types is left for the user. The registration api's returns
+the cooling device pointer.
+
+1. cpufreq cooling api's
+
+1.1 cpufreq registration api's
+1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val)
+
+    This interface function registers the cpufreq cooling device with the name
+	"thermal-cpufreq".
+
+    tab_ptr: The table containing the percentage of frequency to be clipped for
+    each cooling state.
+	.freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
+	 cpu.
+	.polling_interval: polling interval for this cooling state.
+    tab_size: the total number of cooling state.
+    mask_val: all the allowed cpu's where frequency clipping can happen.
+
+1.1.2 void cpufreq_cooling_unregister(void)
+
+    This interface function unregisters the "thermal-cpufreq" cooling device.
+
+
+1.2 cpuhotplug registration api's
+
+1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val)
+
+    This interface function registers the cpuhotplug cooling device with the name
+	"thermal-cpuhotplug".
+
+    mask_val: all the allowed cpu's which can be hotplugged out.
+
+1.1.2 void cpuhotplug_cooling_unregister(void)
+
+    This interface function unregisters the "thermal-cpuhotplug" cooling device.
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index f7f71b2..298c1cd 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -18,3 +18,14 @@ config THERMAL_HWMON
 	depends on THERMAL
 	depends on HWMON=y || HWMON=THERMAL
 	default y
+
+config CPU_THERMAL
+	bool "generic cpu cooling support"
+	depends on THERMAL
+	help
+	  This implements the generic cpu cooling mechanism through frequency
+	  reduction, cpu hotplug and any other ways of reducing temperature. An
+	  ACPI version of this already exists(drivers/acpi/processor_thermal.c).
+	  This will be useful for platforms using the generic thermal interface
+	  and not the ACPI interface.
+	  If you want this support, you should say Y or M here.
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 31108a0..655cbc4 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
+obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
new file mode 100644
index 0000000..cdd148c
--- /dev/null
+++ b/drivers/thermal/cpu_cooling.c
@@ -0,0 +1,302 @@
+/*
+ *  linux/drivers/thermal/cpu_cooling.c
+ *
+ *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
+ *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+#include <linux/cpufreq.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/cpu.h>
+#include <linux/cpu_cooling.h>
+
+#ifdef CONFIG_CPU_FREQ
+struct cpufreq_cooling_device {
+	struct thermal_cooling_device *cool_dev;
+	struct freq_pctg_table *tab_ptr;
+	unsigned int tab_size;
+	unsigned int cpufreq_state;
+	const struct cpumask *allowed_cpus;
+};
+
+static struct cpufreq_cooling_device *cpufreq_device;
+
+/*Below codes defines functions to be used for cpufreq as cooling device*/
+static bool is_cpufreq_valid(int cpu)
+{
+	struct cpufreq_policy policy;
+	if (!cpufreq_get_policy(&policy, cpu))
+		return true;
+	return false;
+}
+
+static int cpufreq_apply_cooling(int cooling_state)
+{
+	int cpuid, this_cpu = smp_processor_id();
+
+	if (!is_cpufreq_valid(this_cpu))
+		return 0;
+
+	if (cooling_state > cpufreq_device->tab_size)
+		return -EINVAL;
+
+	/*Check if last cooling level is same as current cooling level*/
+	if (cpufreq_device->cpufreq_state == cooling_state)
+		return 0;
+
+	cpufreq_device->cpufreq_state = cooling_state;
+
+	for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
+		if (is_cpufreq_valid(cpuid))
+			cpufreq_update_policy(cpuid);
+	}
+
+	return 0;
+}
+
+static int thermal_cpufreq_notifier(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct cpufreq_policy *policy = data;
+	struct freq_pctg_table *th_table;
+	unsigned long max_freq = 0;
+	unsigned int cpu = policy->cpu, th_pctg = 0, level;
+
+	if (event != CPUFREQ_ADJUST)
+		return 0;
+
+	level = cpufreq_device->cpufreq_state;
+
+	if (level > 0) {
+		th_table =
+			&(cpufreq_device->tab_ptr[level - 1]);
+		th_pctg = th_table->freq_clip_pctg[cpu];
+	}
+
+	max_freq =
+		(policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
+
+	cpufreq_verify_within_limits(policy, 0, max_freq);
+
+	return 0;
+}
+
+/*
+ * cpufreq cooling device callback functions
+ */
+static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	*state = cpufreq_device->tab_size;
+	return 0;
+}
+
+static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	*state = cpufreq_device->cpufreq_state;
+	return 0;
+}
+
+/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
+static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long state)
+{
+	cpufreq_apply_cooling(state);
+	return 0;
+}
+
+/* bind cpufreq callbacks to cpufreq cooling device */
+static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
+	.get_max_state = cpufreq_get_max_state,
+	.get_cur_state = cpufreq_get_cur_state,
+	.set_cur_state = cpufreq_set_cur_state,
+};
+
+static struct notifier_block thermal_cpufreq_notifier_block = {
+	.notifier_call = thermal_cpufreq_notifier,
+};
+
+struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val)
+{
+	struct thermal_cooling_device *cool_dev;
+
+	if (tab_ptr == NULL || tab_size == 0)
+		return ERR_PTR(-EINVAL);
+
+	cpufreq_device =
+		kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
+
+	if (!cpufreq_device)
+		return ERR_PTR(-ENOMEM);
+
+	cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
+						&cpufreq_cooling_ops);
+	if (!cool_dev) {
+		kfree(cpufreq_device);
+		return ERR_PTR(-EINVAL);
+	}
+
+	cpufreq_device->tab_ptr = tab_ptr;
+	cpufreq_device->tab_size = tab_size;
+	cpufreq_device->cool_dev = cool_dev;
+	cpufreq_device->allowed_cpus = mask_val;
+
+	cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
+						CPUFREQ_POLICY_NOTIFIER);
+	return cool_dev;
+}
+EXPORT_SYMBOL(cpufreq_cooling_register);
+
+void cpufreq_cooling_unregister(void)
+{
+	cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
+						CPUFREQ_POLICY_NOTIFIER);
+	thermal_cooling_device_unregister(cpufreq_device->cool_dev);
+	kfree(cpufreq_device);
+}
+EXPORT_SYMBOL(cpufreq_cooling_unregister);
+#else /*!CONFIG_CPU_FREQ*/
+struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size)
+{
+	return NULL;
+}
+EXPORT_SYMBOL(cpufreq_cooling_register);
+void cpufreq_cooling_unregister(void)
+{
+	return;
+}
+EXPORT_SYMBOL(cpufreq_cooling_unregister);
+#endif /*CONFIG_CPU_FREQ*/
+
+#ifdef CONFIG_HOTPLUG_CPU
+
+struct hotplug_cooling_device {
+	struct thermal_cooling_device *cool_dev;
+	unsigned int hotplug_state;
+	const struct cpumask *allowed_cpus;
+};
+static struct hotplug_cooling_device *hotplug_device;
+
+/*
+ * cpu hotplug cooling device callback functions
+ */
+static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	*state = 1;
+	return 0;
+}
+
+static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	/*This cooling device may be of type ACTIVE, so state field
+	can be 0 or 1*/
+	*state = hotplug_device->hotplug_state;
+	return 0;
+}
+
+/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
+static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long state)
+{
+	int cpuid, this_cpu = smp_processor_id();
+
+	if (hotplug_device->hotplug_state == state)
+		return 0;
+
+	/*This cooling device may be of type ACTIVE, so state field
+	can be 0 or 1*/
+	if (state == 1) {
+		for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
+			if (cpu_online(cpuid) && (cpuid != this_cpu))
+				cpu_down(cpuid);
+		}
+	} else if (state == 0) {
+		for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
+			if (!cpu_online(cpuid) && (cpuid != this_cpu))
+				cpu_up(cpuid);
+		}
+	} else
+		return -EINVAL;
+
+	hotplug_device->hotplug_state = state;
+
+	return 0;
+}
+/* bind hotplug callbacks to cpu hotplug cooling device */
+static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
+	.get_max_state = cpuhotplug_get_max_state,
+	.get_cur_state = cpuhotplug_get_cur_state,
+	.set_cur_state = cpuhotplug_set_cur_state,
+};
+
+struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val)
+{
+	struct thermal_cooling_device *cool_dev;
+
+	hotplug_device =
+		kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
+
+	if (!hotplug_device)
+		return ERR_PTR(-ENOMEM);
+
+	cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
+						&cpuhotplug_cooling_ops);
+	if (!cool_dev) {
+		kfree(hotplug_device);
+		return ERR_PTR(-EINVAL);
+	}
+
+	hotplug_device->cool_dev = cool_dev;
+	hotplug_device->hotplug_state = 0;
+	hotplug_device->allowed_cpus = mask_val;
+
+	return cool_dev;
+}
+EXPORT_SYMBOL(cpuhotplug_cooling_register);
+
+void cpuhotplug_cooling_unregister(void)
+{
+	thermal_cooling_device_unregister(hotplug_device->cool_dev);
+	kfree(hotplug_device);
+}
+EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
+#else /*!CONFIG_HOTPLUG_CPU*/
+struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val)
+{
+	return NULL;
+}
+EXPORT_SYMBOL(cpuhotplug_cooling_register);
+void cpuhotplug_cooling_unregister(void)
+{
+	return;
+}
+EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
+#endif /*CONFIG_HOTPLUG_CPU*/
diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
new file mode 100644
index 0000000..0c57375
--- /dev/null
+++ b/include/linux/cpu_cooling.h
@@ -0,0 +1,45 @@
+/*
+ *  linux/include/linux/cpu_cooling.h
+ *
+ *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
+ *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef __CPU_COOLING_H__
+#define __CPU_COOLING_H__
+
+#include <linux/thermal.h>
+
+struct freq_pctg_table {
+	unsigned int freq_clip_pctg[NR_CPUS];
+	unsigned int polling_interval;
+};
+
+extern struct thermal_cooling_device *cpufreq_cooling_register(
+	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
+	const struct cpumask *mask_val);
+
+extern void cpufreq_cooling_unregister(void);
+
+extern struct thermal_cooling_device *cpuhotplug_cooling_register(
+	const struct cpumask *mask_val);
+
+extern void cpuhotplug_cooling_unregister(void);
+
+#endif /* __CPU_COOLING_H__ */
-- 
1.7.1

Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Rob Lee]

Hey Amit, I was able to use your code on an i.MX6Q thermal
implementation and it seemed to work pretty well.  Thanks for adding
this.  A couple of comments below.

On Tue, Dec 13, 2011 at 9:13 AM, Amit Daniel Kachhap
<amit.kachhap@linaro.org> wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling and cpuhotplugg currently.
> Different cpu related cooling devices can be registered by the
> user and the binding of these cooling devices to the corresponding
> trip points can be easily done as the registration API's return the
> cooling device pointer.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
>  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
>  drivers/thermal/Kconfig                   |   11 +
>  drivers/thermal/Makefile                  |    1 +
>  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
>  include/linux/cpu_cooling.h               |   45 +++++
>  5 files changed, 411 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>  create mode 100644 drivers/thermal/cpu_cooling.c
>  create mode 100644 include/linux/cpu_cooling.h
>
> diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
> new file mode 100644
> index 0000000..d30b4f2
> --- /dev/null
> +++ b/Documentation/thermal/cpu-cooling-api.txt
> @@ -0,0 +1,52 @@
> +CPU cooling api's How To
> +===================================
> +
> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
> +
> +Updated: 13 Dec 2011
> +
> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
> +
> +0. Introduction
> +
> +The generic cpu cooling(freq clipping, cpuhotplug) provides
> +registration/unregistration api's to the user. The binding of the cooling
> +devices to the trip types is left for the user. The registration api's returns
> +the cooling device pointer.
> +
> +1. cpufreq cooling api's
> +
> +1.1 cpufreq registration api's
> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +       const struct cpumask *mask_val)
> +
> +    This interface function registers the cpufreq cooling device with the name
> +       "thermal-cpufreq".
> +
> +    tab_ptr: The table containing the percentage of frequency to be clipped for
> +    each cooling state.
> +       .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
> +        cpu.
> +       .polling_interval: polling interval for this cooling state.
> +    tab_size: the total number of cooling state.
> +    mask_val: all the allowed cpu's where frequency clipping can happen.
> +
> +1.1.2 void cpufreq_cooling_unregister(void)
> +
> +    This interface function unregisters the "thermal-cpufreq" cooling device.
> +
> +
> +1.2 cpuhotplug registration api's
> +
> +1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
> +       const struct cpumask *mask_val)
> +
> +    This interface function registers the cpuhotplug cooling device with the name
> +       "thermal-cpuhotplug".
> +
> +    mask_val: all the allowed cpu's which can be hotplugged out.
> +
> +1.1.2 void cpuhotplug_cooling_unregister(void)
> +
> +    This interface function unregisters the "thermal-cpuhotplug" cooling device.
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f7f71b2..298c1cd 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -18,3 +18,14 @@ config THERMAL_HWMON
>        depends on THERMAL
>        depends on HWMON=y || HWMON=THERMAL
>        default y
> +
> +config CPU_THERMAL
> +       bool "generic cpu cooling support"
> +       depends on THERMAL
> +       help
> +         This implements the generic cpu cooling mechanism through frequency
> +         reduction, cpu hotplug and any other ways of reducing temperature. An
> +         ACPI version of this already exists(drivers/acpi/processor_thermal.c).
> +         This will be useful for platforms using the generic thermal interface
> +         and not the ACPI interface.
> +         If you want this support, you should say Y or M here.
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 31108a0..655cbc4 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -3,3 +3,4 @@
>  #
>
>  obj-$(CONFIG_THERMAL)          += thermal_sys.o
> +obj-$(CONFIG_CPU_THERMAL)      += cpu_cooling.o
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> new file mode 100644
> index 0000000..cdd148c
> --- /dev/null
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -0,0 +1,302 @@
> +/*
> + *  linux/drivers/thermal/cpu_cooling.c
> + *
> + *  Copyright (C) 2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/cpu.h>
> +#include <linux/cpu_cooling.h>
> +
> +#ifdef CONFIG_CPU_FREQ
> +struct cpufreq_cooling_device {
> +       struct thermal_cooling_device *cool_dev;
> +       struct freq_pctg_table *tab_ptr;
> +       unsigned int tab_size;
> +       unsigned int cpufreq_state;
> +       const struct cpumask *allowed_cpus;
> +};
> +
> +static struct cpufreq_cooling_device *cpufreq_device;
> +
> +/*Below codes defines functions to be used for cpufreq as cooling device*/
> +static bool is_cpufreq_valid(int cpu)
> +{
> +       struct cpufreq_policy policy;
> +       if (!cpufreq_get_policy(&policy, cpu))
> +               return true;
> +       return false;
> +}
> +
> +static int cpufreq_apply_cooling(int cooling_state)
> +{
> +       int cpuid, this_cpu = smp_processor_id();
> +
> +       if (!is_cpufreq_valid(this_cpu))
> +               return 0;
> +
> +       if (cooling_state > cpufreq_device->tab_size)
> +               return -EINVAL;
> +
> +       /*Check if last cooling level is same as current cooling level*/
> +       if (cpufreq_device->cpufreq_state == cooling_state)
> +               return 0;
> +
> +       cpufreq_device->cpufreq_state = cooling_state;
> +
> +       for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
> +               if (is_cpufreq_valid(cpuid))
> +                       cpufreq_update_policy(cpuid);
> +       }
> +
> +       return 0;
> +}
> +
> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
> +                                       unsigned long event, void *data)
> +{
> +       struct cpufreq_policy *policy = data;
> +       struct freq_pctg_table *th_table;
> +       unsigned long max_freq = 0;
> +       unsigned int cpu = policy->cpu, th_pctg = 0, level;
> +
> +       if (event != CPUFREQ_ADJUST)
> +               return 0;
> +
> +       level = cpufreq_device->cpufreq_state;
> +
> +       if (level > 0) {
> +               th_table =
> +                       &(cpufreq_device->tab_ptr[level - 1]);
> +               th_pctg = th_table->freq_clip_pctg[cpu];
> +       }
> +
> +       max_freq =
> +               (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
> +
> +       cpufreq_verify_within_limits(policy, 0, max_freq);
> +
> +       return 0;
> +}
> +
> +/*
> + * cpufreq cooling device callback functions
> + */
> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
> +                                unsigned long *state)
> +{
> +       *state = cpufreq_device->tab_size;
> +       return 0;
> +}
> +
> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> +                                unsigned long *state)
> +{
> +       *state = cpufreq_device->cpufreq_state;
> +       return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> +                                unsigned long state)
> +{
> +       cpufreq_apply_cooling(state);
> +       return 0;
> +}
> +
> +/* bind cpufreq callbacks to cpufreq cooling device */
> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> +       .get_max_state = cpufreq_get_max_state,
> +       .get_cur_state = cpufreq_get_cur_state,
> +       .set_cur_state = cpufreq_set_cur_state,
> +};
> +
> +static struct notifier_block thermal_cpufreq_notifier_block = {
> +       .notifier_call = thermal_cpufreq_notifier,
> +};
> +
> +struct thermal_cooling_device *cpufreq_cooling_register(
> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +       const struct cpumask *mask_val)
> +{
> +       struct thermal_cooling_device *cool_dev;
> +
> +       if (tab_ptr == NULL || tab_size == 0)
> +               return ERR_PTR(-EINVAL);
> +
> +       cpufreq_device =
> +               kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
> +
> +       if (!cpufreq_device)
> +               return ERR_PTR(-ENOMEM);
> +
> +       cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
> +                                               &cpufreq_cooling_ops);
> +       if (!cool_dev) {
> +               kfree(cpufreq_device);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       cpufreq_device->tab_ptr = tab_ptr;
> +       cpufreq_device->tab_size = tab_size;
> +       cpufreq_device->cool_dev = cool_dev;
> +       cpufreq_device->allowed_cpus = mask_val;
> +
> +       cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> +                                               CPUFREQ_POLICY_NOTIFIER);
> +       return cool_dev;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +
> +void cpufreq_cooling_unregister(void)
> +{
> +       cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> +                                               CPUFREQ_POLICY_NOTIFIER);
> +       thermal_cooling_device_unregister(cpufreq_device->cool_dev);
> +       kfree(cpufreq_device);
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#else /*!CONFIG_CPU_FREQ*/
> +struct thermal_cooling_device *cpufreq_cooling_register(
> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size)

Need to add the "const struct cpumask *mask_val" parameter
to this function like it's used above and prototyped in the heard
or else it causes a build error when building
without CONFIG_CPU_FREQ defined.

> +{
> +       return NULL;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +void cpufreq_cooling_unregister(void)
> +{
> +       return;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#endif /*CONFIG_CPU_FREQ*/
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +
> +struct hotplug_cooling_device {
> +       struct thermal_cooling_device *cool_dev;
> +       unsigned int hotplug_state;
> +       const struct cpumask *allowed_cpus;
> +};
> +static struct hotplug_cooling_device *hotplug_device;
> +
> +/*
> + * cpu hotplug cooling device callback functions
> + */
> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
> +                                unsigned long *state)
> +{
> +       *state = 1;
> +       return 0;
> +}
> +
> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
> +                                unsigned long *state)
> +{
> +       /*This cooling device may be of type ACTIVE, so state field
> +       can be 0 or 1*/
> +       *state = hotplug_device->hotplug_state;
> +       return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
> +                                unsigned long state)
> +{
> +       int cpuid, this_cpu = smp_processor_id();
> +
> +       if (hotplug_device->hotplug_state == state)
> +               return 0;
> +
> +       /*This cooling device may be of type ACTIVE, so state field
> +       can be 0 or 1*/
> +       if (state == 1) {
> +               for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> +                       if (cpu_online(cpuid) && (cpuid != this_cpu))
> +                               cpu_down(cpuid);
> +               }
> +       } else if (state == 0) {
> +               for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> +                       if (!cpu_online(cpuid) && (cpuid != this_cpu))
> +                               cpu_up(cpuid);
> +               }
> +       } else
> +               return -EINVAL;
> +

I don't like how ACTIVE does not have available notification callbacks
like HOT and CRITICAL do.  Perhaps I fail to grasp why they aren't there
but besides just applying a cooling device, one might want to do something
else as well upon hitting these trip points.  So that said, it might be nice
to add a notification to STATE_ACTIVE as well.

> +       hotplug_device->hotplug_state = state;
> +
> +       return 0;
> +}
> +/* bind hotplug callbacks to cpu hotplug cooling device */
> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
> +       .get_max_state = cpuhotplug_get_max_state,
> +       .get_cur_state = cpuhotplug_get_cur_state,
> +       .set_cur_state = cpuhotplug_set_cur_state,
> +};
> +
> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> +       const struct cpumask *mask_val)
> +{
> +       struct thermal_cooling_device *cool_dev;
> +
> +       hotplug_device =
> +               kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
> +
> +       if (!hotplug_device)
> +               return ERR_PTR(-ENOMEM);
> +
> +       cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
> +                                               &cpuhotplug_cooling_ops);
> +       if (!cool_dev) {
> +               kfree(hotplug_device);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       hotplug_device->cool_dev = cool_dev;
> +       hotplug_device->hotplug_state = 0;
> +       hotplug_device->allowed_cpus = mask_val;
> +
> +       return cool_dev;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> +
> +void cpuhotplug_cooling_unregister(void)
> +{
> +       thermal_cooling_device_unregister(hotplug_device->cool_dev);
> +       kfree(hotplug_device);
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> +#else /*!CONFIG_HOTPLUG_CPU*/
> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> +       const struct cpumask *mask_val)
> +{
> +       return NULL;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> +void cpuhotplug_cooling_unregister(void)
> +{
> +       return;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> +#endif /*CONFIG_HOTPLUG_CPU*/
> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> new file mode 100644
> index 0000000..0c57375
> --- /dev/null
> +++ b/include/linux/cpu_cooling.h
> @@ -0,0 +1,45 @@
> +/*
> + *  linux/include/linux/cpu_cooling.h
> + *
> + *  Copyright (C) 2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef __CPU_COOLING_H__
> +#define __CPU_COOLING_H__
> +
> +#include <linux/thermal.h>
> +
> +struct freq_pctg_table {
> +       unsigned int freq_clip_pctg[NR_CPUS];
> +       unsigned int polling_interval;
> +};
> +
> +extern struct thermal_cooling_device *cpufreq_cooling_register(
> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +       const struct cpumask *mask_val);
> +
> +extern void cpufreq_cooling_unregister(void);
> +
> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
> +       const struct cpumask *mask_val);
> +
> +extern void cpuhotplug_cooling_unregister(void);
> +
> +#endif /* __CPU_COOLING_H__ */
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Amit Kachhap]

On 11 January 2012 13:32, Rob Lee <rob.lee@linaro.org> wrote:
> Hey Amit, I was able to use your code on an i.MX6Q thermal
> implementation and it seemed to work pretty well.  Thanks for adding
> this.  A couple of comments below.
Thanks for testing and reviewing the code.
>
> On Tue, Dec 13, 2011 at 9:13 AM, Amit Daniel Kachhap
> <amit.kachhap@linaro.org> wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling and cpuhotplugg currently.
>> Different cpu related cooling devices can be registered by the
>> user and the binding of these cooling devices to the corresponding
>> trip points can be easily done as the registration API's return the
>> cooling device pointer.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>>  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
>>  drivers/thermal/Kconfig                   |   11 +
>>  drivers/thermal/Makefile                  |    1 +
>>  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
>>  include/linux/cpu_cooling.h               |   45 +++++
>>  5 files changed, 411 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>>  create mode 100644 drivers/thermal/cpu_cooling.c
>>  create mode 100644 include/linux/cpu_cooling.h
>>
>> diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
>> new file mode 100644
>> index 0000000..d30b4f2
>> --- /dev/null
>> +++ b/Documentation/thermal/cpu-cooling-api.txt
>> @@ -0,0 +1,52 @@
>> +CPU cooling api's How To
>> +===================================
>> +
>> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> +
>> +Updated: 13 Dec 2011
>> +
>> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
>> +
>> +0. Introduction
>> +
>> +The generic cpu cooling(freq clipping, cpuhotplug) provides
>> +registration/unregistration api's to the user. The binding of the cooling
>> +devices to the trip types is left for the user. The registration api's returns
>> +the cooling device pointer.
>> +
>> +1. cpufreq cooling api's
>> +
>> +1.1 cpufreq registration api's
>> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
>> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +       const struct cpumask *mask_val)
>> +
>> +    This interface function registers the cpufreq cooling device with the name
>> +       "thermal-cpufreq".
>> +
>> +    tab_ptr: The table containing the percentage of frequency to be clipped for
>> +    each cooling state.
>> +       .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
>> +        cpu.
>> +       .polling_interval: polling interval for this cooling state.
>> +    tab_size: the total number of cooling state.
>> +    mask_val: all the allowed cpu's where frequency clipping can happen.
>> +
>> +1.1.2 void cpufreq_cooling_unregister(void)
>> +
>> +    This interface function unregisters the "thermal-cpufreq" cooling device.
>> +
>> +
>> +1.2 cpuhotplug registration api's
>> +
>> +1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +       const struct cpumask *mask_val)
>> +
>> +    This interface function registers the cpuhotplug cooling device with the name
>> +       "thermal-cpuhotplug".
>> +
>> +    mask_val: all the allowed cpu's which can be hotplugged out.
>> +
>> +1.1.2 void cpuhotplug_cooling_unregister(void)
>> +
>> +    This interface function unregisters the "thermal-cpuhotplug" cooling device.
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index f7f71b2..298c1cd 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -18,3 +18,14 @@ config THERMAL_HWMON
>>        depends on THERMAL
>>        depends on HWMON=y || HWMON=THERMAL
>>        default y
>> +
>> +config CPU_THERMAL
>> +       bool "generic cpu cooling support"
>> +       depends on THERMAL
>> +       help
>> +         This implements the generic cpu cooling mechanism through frequency
>> +         reduction, cpu hotplug and any other ways of reducing temperature. An
>> +         ACPI version of this already exists(drivers/acpi/processor_thermal.c).
>> +         This will be useful for platforms using the generic thermal interface
>> +         and not the ACPI interface.
>> +         If you want this support, you should say Y or M here.
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 31108a0..655cbc4 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -3,3 +3,4 @@
>>  #
>>
>>  obj-$(CONFIG_THERMAL)          += thermal_sys.o
>> +obj-$(CONFIG_CPU_THERMAL)      += cpu_cooling.o
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> new file mode 100644
>> index 0000000..cdd148c
>> --- /dev/null
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -0,0 +1,302 @@
>> +/*
>> + *  linux/drivers/thermal/cpu_cooling.c
>> + *
>> + *  Copyright (C) 2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
>> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful, but
>> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpu_cooling.h>
>> +
>> +#ifdef CONFIG_CPU_FREQ
>> +struct cpufreq_cooling_device {
>> +       struct thermal_cooling_device *cool_dev;
>> +       struct freq_pctg_table *tab_ptr;
>> +       unsigned int tab_size;
>> +       unsigned int cpufreq_state;
>> +       const struct cpumask *allowed_cpus;
>> +};
>> +
>> +static struct cpufreq_cooling_device *cpufreq_device;
>> +
>> +/*Below codes defines functions to be used for cpufreq as cooling device*/
>> +static bool is_cpufreq_valid(int cpu)
>> +{
>> +       struct cpufreq_policy policy;
>> +       if (!cpufreq_get_policy(&policy, cpu))
>> +               return true;
>> +       return false;
>> +}
>> +
>> +static int cpufreq_apply_cooling(int cooling_state)
>> +{
>> +       int cpuid, this_cpu = smp_processor_id();
>> +
>> +       if (!is_cpufreq_valid(this_cpu))
>> +               return 0;
>> +
>> +       if (cooling_state > cpufreq_device->tab_size)
>> +               return -EINVAL;
>> +
>> +       /*Check if last cooling level is same as current cooling level*/
>> +       if (cpufreq_device->cpufreq_state == cooling_state)
>> +               return 0;
>> +
>> +       cpufreq_device->cpufreq_state = cooling_state;
>> +
>> +       for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
>> +               if (is_cpufreq_valid(cpuid))
>> +                       cpufreq_update_policy(cpuid);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
>> +                                       unsigned long event, void *data)
>> +{
>> +       struct cpufreq_policy *policy = data;
>> +       struct freq_pctg_table *th_table;
>> +       unsigned long max_freq = 0;
>> +       unsigned int cpu = policy->cpu, th_pctg = 0, level;
>> +
>> +       if (event != CPUFREQ_ADJUST)
>> +               return 0;
>> +
>> +       level = cpufreq_device->cpufreq_state;
>> +
>> +       if (level > 0) {
>> +               th_table =
>> +                       &(cpufreq_device->tab_ptr[level - 1]);
>> +               th_pctg = th_table->freq_clip_pctg[cpu];
>> +       }
>> +
>> +       max_freq =
>> +               (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
>> +
>> +       cpufreq_verify_within_limits(policy, 0, max_freq);
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * cpufreq cooling device callback functions
>> + */
>> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long *state)
>> +{
>> +       *state = cpufreq_device->tab_size;
>> +       return 0;
>> +}
>> +
>> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long *state)
>> +{
>> +       *state = cpufreq_device->cpufreq_state;
>> +       return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
>> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long state)
>> +{
>> +       cpufreq_apply_cooling(state);
>> +       return 0;
>> +}
>> +
>> +/* bind cpufreq callbacks to cpufreq cooling device */
>> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
>> +       .get_max_state = cpufreq_get_max_state,
>> +       .get_cur_state = cpufreq_get_cur_state,
>> +       .set_cur_state = cpufreq_set_cur_state,
>> +};
>> +
>> +static struct notifier_block thermal_cpufreq_notifier_block = {
>> +       .notifier_call = thermal_cpufreq_notifier,
>> +};
>> +
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +       const struct cpumask *mask_val)
>> +{
>> +       struct thermal_cooling_device *cool_dev;
>> +
>> +       if (tab_ptr == NULL || tab_size == 0)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       cpufreq_device =
>> +               kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
>> +
>> +       if (!cpufreq_device)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
>> +                                               &cpufreq_cooling_ops);
>> +       if (!cool_dev) {
>> +               kfree(cpufreq_device);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>> +
>> +       cpufreq_device->tab_ptr = tab_ptr;
>> +       cpufreq_device->tab_size = tab_size;
>> +       cpufreq_device->cool_dev = cool_dev;
>> +       cpufreq_device->allowed_cpus = mask_val;
>> +
>> +       cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
>> +                                               CPUFREQ_POLICY_NOTIFIER);
>> +       return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +
>> +void cpufreq_cooling_unregister(void)
>> +{
>> +       cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
>> +                                               CPUFREQ_POLICY_NOTIFIER);
>> +       thermal_cooling_device_unregister(cpufreq_device->cool_dev);
>> +       kfree(cpufreq_device);
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#else /*!CONFIG_CPU_FREQ*/
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size)
>
> Need to add the "const struct cpumask *mask_val" parameter
> to this function like it's used above and prototyped in the heard
> or else it causes a build error when building
> without CONFIG_CPU_FREQ defined.
Yes sure somehow missed adding this. Thanks for pointing this out.
>
>> +{
>> +       return NULL;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +void cpufreq_cooling_unregister(void)
>> +{
>> +       return;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#endif /*CONFIG_CPU_FREQ*/
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +
>> +struct hotplug_cooling_device {
>> +       struct thermal_cooling_device *cool_dev;
>> +       unsigned int hotplug_state;
>> +       const struct cpumask *allowed_cpus;
>> +};
>> +static struct hotplug_cooling_device *hotplug_device;
>> +
>> +/*
>> + * cpu hotplug cooling device callback functions
>> + */
>> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long *state)
>> +{
>> +       *state = 1;
>> +       return 0;
>> +}
>> +
>> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long *state)
>> +{
>> +       /*This cooling device may be of type ACTIVE, so state field
>> +       can be 0 or 1*/
>> +       *state = hotplug_device->hotplug_state;
>> +       return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
>> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
>> +                                unsigned long state)
>> +{
>> +       int cpuid, this_cpu = smp_processor_id();
>> +
>> +       if (hotplug_device->hotplug_state == state)
>> +               return 0;
>> +
>> +       /*This cooling device may be of type ACTIVE, so state field
>> +       can be 0 or 1*/
>> +       if (state == 1) {
>> +               for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
>> +                       if (cpu_online(cpuid) && (cpuid != this_cpu))
>> +                               cpu_down(cpuid);
>> +               }
>> +       } else if (state == 0) {
>> +               for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
>> +                       if (!cpu_online(cpuid) && (cpuid != this_cpu))
>> +                               cpu_up(cpuid);
>> +               }
>> +       } else
>> +               return -EINVAL;
>> +
>
> I don't like how ACTIVE does not have available notification callbacks
> like HOT and CRITICAL do.  Perhaps I fail to grasp why they aren't there
> but besides just applying a cooling device, one might want to do something
> else as well upon hitting these trip points.  So that said, it might be nice
> to add a notification to STATE_ACTIVE as well.
Well I added ACTIVE_STATE trip type as just some additional features
over trip type ACTIVE. Also each thermal zone can have more then one
trip types so HOT notifications can still be used. But I like your
suggestion of putting notfications in all the trip types. Lets see if
the maintainers agree to this idea.
>
>> +       hotplug_device->hotplug_state = state;
>> +
>> +       return 0;
>> +}
>> +/* bind hotplug callbacks to cpu hotplug cooling device */
>> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
>> +       .get_max_state = cpuhotplug_get_max_state,
>> +       .get_cur_state = cpuhotplug_get_cur_state,
>> +       .set_cur_state = cpuhotplug_set_cur_state,
>> +};
>> +
>> +struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +       const struct cpumask *mask_val)
>> +{
>> +       struct thermal_cooling_device *cool_dev;
>> +
>> +       hotplug_device =
>> +               kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
>> +
>> +       if (!hotplug_device)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
>> +                                               &cpuhotplug_cooling_ops);
>> +       if (!cool_dev) {
>> +               kfree(hotplug_device);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>> +
>> +       hotplug_device->cool_dev = cool_dev;
>> +       hotplug_device->hotplug_state = 0;
>> +       hotplug_device->allowed_cpus = mask_val;
>> +
>> +       return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
>> +
>> +void cpuhotplug_cooling_unregister(void)
>> +{
>> +       thermal_cooling_device_unregister(hotplug_device->cool_dev);
>> +       kfree(hotplug_device);
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
>> +#else /*!CONFIG_HOTPLUG_CPU*/
>> +struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +       const struct cpumask *mask_val)
>> +{
>> +       return NULL;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
>> +void cpuhotplug_cooling_unregister(void)
>> +{
>> +       return;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
>> +#endif /*CONFIG_HOTPLUG_CPU*/
>> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
>> new file mode 100644
>> index 0000000..0c57375
>> --- /dev/null
>> +++ b/include/linux/cpu_cooling.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + *  linux/include/linux/cpu_cooling.h
>> + *
>> + *  Copyright (C) 2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
>> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful, but
>> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +
>> +#ifndef __CPU_COOLING_H__
>> +#define __CPU_COOLING_H__
>> +
>> +#include <linux/thermal.h>
>> +
>> +struct freq_pctg_table {
>> +       unsigned int freq_clip_pctg[NR_CPUS];
>> +       unsigned int polling_interval;
>> +};
>> +
>> +extern struct thermal_cooling_device *cpufreq_cooling_register(
>> +       struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +       const struct cpumask *mask_val);
>> +
>> +extern void cpufreq_cooling_unregister(void);
>> +
>> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +       const struct cpumask *mask_val);
>> +
>> +extern void cpuhotplug_cooling_unregister(void);
>> +
>> +#endif /* __CPU_COOLING_H__ */
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Rob Lee]

>> I don't like how ACTIVE does not have available notification callbacks
>> like HOT and CRITICAL do.  Perhaps I fail to grasp why they aren't there
>> but besides just applying a cooling device, one might want to do something
>> else as well upon hitting these trip points.  So that said, it might be nice
>> to add a notification to STATE_ACTIVE as well.
> Well I added ACTIVE_STATE trip type as just some additional features
> over trip type ACTIVE. Also each thermal zone can have more then one
> trip types so HOT notifications can still be used. But I like your
> suggestion of putting notfications in all the trip types. Lets see if
> the maintainers agree to this idea.
Thinking about it a bit more, I didn't consider HOT because it only
notifies when you cross above this trip point and not below and the
use case I had in mind would ideally want to be notified in both
cases.  Also, if ACTIVE/ACTIVE_STATE trip types will always have
cooling devices bound to them, then the notification could be put in
the cooling device code if desired.  I think these features are
separate from your main effort and can be addressed in separate future
patch.

Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Matthew Garrett]

On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling and cpuhotplugg currently.

We've been over this kind of thing in the past. cpu hotplug is a 
relatively expensive operation, so people have previously been 
enthusiastic about using the scheduler to simply avoid running anything 
on CPUs if they're overheating. Has any general consensus been reached 
on this?

I'm also not entirely thrilled at now having two ways to manage the cpu 
through the thermal layer. ACPI already plugs in via the passive trip 
points. If we're going to do this then I'd like to see the ACPI code 
merged in with the generic cpu cooling code.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Amit Kachhap]

On 1 February 2012 20:27, Matthew Garrett <mjg@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling and cpuhotplugg currently.
>
> We've been over this kind of thing in the past. cpu hotplug is a
> relatively expensive operation, so people have previously been
> enthusiastic about using the scheduler to simply avoid running anything
> on CPUs if they're overheating. Has any general consensus been reached
> on this?
yes you are right that cpuhotplug is an expensive process and it may
further heat up the system before turning off so the ideal way would
be to reduce the capacity of the cpu gradually. Anyway these patches
are only exporting those API's and the actual use of them depends on
the user. Although my bigger focus is on cpufreq as cooling devices so
I might remove cpuhotplug in the next version.
>
> I'm also not entirely thrilled at now having two ways to manage the cpu
> through the thermal layer. ACPI already plugs in via the passive trip
> points. If we're going to do this then I'd like to see the ACPI code
> merged in with the generic cpu cooling code.
Yeah I also agree that there is a kind of repetition and not entirely
sure where to place these codes. I will try adding them inside acpi.
Thanks for the suggestion.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org

Re: [linux-pm] [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Eduardo Valentin]

Hello Amit,

On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling and cpuhotplugg currently.
> Different cpu related cooling devices can be registered by the
> user and the binding of these cooling devices to the corresponding
> trip points can be easily done as the registration API's return the
> cooling device pointer.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> ---
>  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
>  drivers/thermal/Kconfig                   |   11 +
>  drivers/thermal/Makefile                  |    1 +
>  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
>  include/linux/cpu_cooling.h               |   45 +++++
>  5 files changed, 411 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>  create mode 100644 drivers/thermal/cpu_cooling.c
>  create mode 100644 include/linux/cpu_cooling.h
> 
> diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
> new file mode 100644
> index 0000000..d30b4f2
> --- /dev/null
> +++ b/Documentation/thermal/cpu-cooling-api.txt
> @@ -0,0 +1,52 @@
> +CPU cooling api's How To
> +===================================
> +
> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
> +
> +Updated: 13 Dec 2011
> +
> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
> +
> +0. Introduction
> +
> +The generic cpu cooling(freq clipping, cpuhotplug) provides
> +registration/unregistration api's to the user. The binding of the cooling
> +devices to the trip types is left for the user. The registration api's returns
> +the cooling device pointer.
> +
> +1. cpufreq cooling api's
> +
> +1.1 cpufreq registration api's
> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val)
> +
> +    This interface function registers the cpufreq cooling device with the name
> +	"thermal-cpufreq".
> +
> +    tab_ptr: The table containing the percentage of frequency to be clipped for
> +    each cooling state.
> +	.freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
> +	 cpu.
> +	.polling_interval: polling interval for this cooling state.
> +    tab_size: the total number of cooling state.
> +    mask_val: all the allowed cpu's where frequency clipping can happen.
> +
> +1.1.2 void cpufreq_cooling_unregister(void)
> +
> +    This interface function unregisters the "thermal-cpufreq" cooling device.
> +
> +
> +1.2 cpuhotplug registration api's
> +
> +1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
> +	const struct cpumask *mask_val)
> +
> +    This interface function registers the cpuhotplug cooling device with the name
> +	"thermal-cpuhotplug".
> +
> +    mask_val: all the allowed cpu's which can be hotplugged out.
> +
> +1.1.2 void cpuhotplug_cooling_unregister(void)
> +
> +    This interface function unregisters the "thermal-cpuhotplug" cooling device.
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f7f71b2..298c1cd 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -18,3 +18,14 @@ config THERMAL_HWMON
>  	depends on THERMAL
>  	depends on HWMON=y || HWMON=THERMAL
>  	default y
> +
> +config CPU_THERMAL
> +	bool "generic cpu cooling support"
> +	depends on THERMAL
> +	help
> +	  This implements the generic cpu cooling mechanism through frequency
> +	  reduction, cpu hotplug and any other ways of reducing temperature. An
> +	  ACPI version of this already exists(drivers/acpi/processor_thermal.c).
> +	  This will be useful for platforms using the generic thermal interface
> +	  and not the ACPI interface.
> +	  If you want this support, you should say Y or M here.
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 31108a0..655cbc4 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -3,3 +3,4 @@
>  #
>  
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
> +obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> new file mode 100644
> index 0000000..cdd148c
> --- /dev/null
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -0,0 +1,302 @@
> +/*
> + *  linux/drivers/thermal/cpu_cooling.c
> + *
> + *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/cpu.h>
> +#include <linux/cpu_cooling.h>
> +
> +#ifdef CONFIG_CPU_FREQ
> +struct cpufreq_cooling_device {
> +	struct thermal_cooling_device *cool_dev;
> +	struct freq_pctg_table *tab_ptr;
> +	unsigned int tab_size;
> +	unsigned int cpufreq_state;
> +	const struct cpumask *allowed_cpus;
> +};
> +
> +static struct cpufreq_cooling_device *cpufreq_device;
> +
> +/*Below codes defines functions to be used for cpufreq as cooling device*/
> +static bool is_cpufreq_valid(int cpu)
> +{
> +	struct cpufreq_policy policy;
> +	if (!cpufreq_get_policy(&policy, cpu))
> +		return true;
> +	return false;
> +}
> +
> +static int cpufreq_apply_cooling(int cooling_state)
> +{

Why do you need cooling_state to be signed? You used it always against
unsigned values. Besides, the thermal api imposes unsigned long.

> +	int cpuid, this_cpu = smp_processor_id();
> +
> +	if (!is_cpufreq_valid(this_cpu))
> +		return 0;
> +
> +	if (cooling_state > cpufreq_device->tab_size)
> +		return -EINVAL;
> +
> +	/*Check if last cooling level is same as current cooling level*/
> +	if (cpufreq_device->cpufreq_state == cooling_state)
> +		return 0;
> +
> +	cpufreq_device->cpufreq_state = cooling_state;
> +
> +	for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
> +		if (is_cpufreq_valid(cpuid))
> +			cpufreq_update_policy(cpuid);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct cpufreq_policy *policy = data;
> +	struct freq_pctg_table *th_table;
> +	unsigned long max_freq = 0;
> +	unsigned int cpu = policy->cpu, th_pctg = 0, level;
> +
> +	if (event != CPUFREQ_ADJUST)
> +		return 0;
> +
> +	level = cpufreq_device->cpufreq_state;
> +
> +	if (level > 0) {
> +		th_table =
> +			&(cpufreq_device->tab_ptr[level - 1]);
> +		th_pctg = th_table->freq_clip_pctg[cpu];
> +	}
> +
> +	max_freq =
> +		(policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;


Might be interesting to extend this a bit. You could have the
above as default policy setup, but you could also allow people
to change this behavior. For instance, adding some callback option
while registering the cooling device (or a notifier), which would
determine the max_freq for the generic layer. The mechanism to
deploy max_freq into the system must be generic enough with cpufreq
APIs, but the decision to which max_freq to use, could be specific.
Don't you think?

> +
> +	cpufreq_verify_within_limits(policy, 0, max_freq);
> +
> +	return 0;
> +}
> +
> +/*
> + * cpufreq cooling device callback functions
> + */
> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	*state = cpufreq_device->tab_size;
> +	return 0;
> +}
> +
> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	*state = cpufreq_device->cpufreq_state;
> +	return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long state)
> +{
> +	cpufreq_apply_cooling(state);
> +	return 0;
> +}
> +
> +/* bind cpufreq callbacks to cpufreq cooling device */
> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> +	.get_max_state = cpufreq_get_max_state,
> +	.get_cur_state = cpufreq_get_cur_state,
> +	.set_cur_state = cpufreq_set_cur_state,
> +};
> +
> +static struct notifier_block thermal_cpufreq_notifier_block = {
> +	.notifier_call = thermal_cpufreq_notifier,
> +};
> +
> +struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val)
> +{
> +	struct thermal_cooling_device *cool_dev;
> +
> +	if (tab_ptr == NULL || tab_size == 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	cpufreq_device =
> +		kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
> +
> +	if (!cpufreq_device)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
> +						&cpufreq_cooling_ops);
> +	if (!cool_dev) {
> +		kfree(cpufreq_device);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	cpufreq_device->tab_ptr = tab_ptr;
> +	cpufreq_device->tab_size = tab_size;
> +	cpufreq_device->cool_dev = cool_dev;
> +	cpufreq_device->allowed_cpus = mask_val;
> +
> +	cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> +						CPUFREQ_POLICY_NOTIFIER);
> +	return cool_dev;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +
> +void cpufreq_cooling_unregister(void)
> +{
> +	cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> +						CPUFREQ_POLICY_NOTIFIER);
> +	thermal_cooling_device_unregister(cpufreq_device->cool_dev);
> +	kfree(cpufreq_device);
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#else /*!CONFIG_CPU_FREQ*/
> +struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size)
> +{
> +	return NULL;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_register);
> +void cpufreq_cooling_unregister(void)
> +{
> +	return;
> +}
> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> +#endif /*CONFIG_CPU_FREQ*/
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +
> +struct hotplug_cooling_device {
> +	struct thermal_cooling_device *cool_dev;
> +	unsigned int hotplug_state;
> +	const struct cpumask *allowed_cpus;
> +};
> +static struct hotplug_cooling_device *hotplug_device;
> +
> +/*
> + * cpu hotplug cooling device callback functions
> + */
> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	*state = 1;
> +	return 0;
> +}
> +
> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	/*This cooling device may be of type ACTIVE, so state field
> +	can be 0 or 1*/
> +	*state = hotplug_device->hotplug_state;
> +	return 0;
> +}
> +
> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long state)
> +{
> +	int cpuid, this_cpu = smp_processor_id();
> +
> +	if (hotplug_device->hotplug_state == state)
> +		return 0;
> +
> +	/*This cooling device may be of type ACTIVE, so state field
> +	can be 0 or 1*/

/*
 * Small thing. Check the style for multi line comments.
 * This is also an example.
 */

> +	if (state == 1) {
> +		for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> +			if (cpu_online(cpuid) && (cpuid != this_cpu))
> +				cpu_down(cpuid);
> +		}
> +	} else if (state == 0) {
> +		for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> +			if (!cpu_online(cpuid) && (cpuid != this_cpu))
> +				cpu_up(cpuid);
> +		}
> +	} else
> +		return -EINVAL;
> +
> +	hotplug_device->hotplug_state = state;
> +
> +	return 0;
> +}
> +/* bind hotplug callbacks to cpu hotplug cooling device */
> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
> +	.get_max_state = cpuhotplug_get_max_state,
> +	.get_cur_state = cpuhotplug_get_cur_state,
> +	.set_cur_state = cpuhotplug_set_cur_state,
> +};
> +
> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> +	const struct cpumask *mask_val)
> +{
> +	struct thermal_cooling_device *cool_dev;
> +
> +	hotplug_device =
> +		kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
> +
> +	if (!hotplug_device)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
> +						&cpuhotplug_cooling_ops);
> +	if (!cool_dev) {
> +		kfree(hotplug_device);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	hotplug_device->cool_dev = cool_dev;
> +	hotplug_device->hotplug_state = 0;
> +	hotplug_device->allowed_cpus = mask_val;
> +
> +	return cool_dev;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> +
> +void cpuhotplug_cooling_unregister(void)
> +{
> +	thermal_cooling_device_unregister(hotplug_device->cool_dev);
> +	kfree(hotplug_device);
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> +#else /*!CONFIG_HOTPLUG_CPU*/
> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> +	const struct cpumask *mask_val)
> +{
> +	return NULL;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> +void cpuhotplug_cooling_unregister(void)
> +{
> +	return;
> +}
> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> +#endif /*CONFIG_HOTPLUG_CPU*/
> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> new file mode 100644
> index 0000000..0c57375
> --- /dev/null
> +++ b/include/linux/cpu_cooling.h
> @@ -0,0 +1,45 @@
> +/*
> + *  linux/include/linux/cpu_cooling.h
> + *
> + *  Copyright (C) 2011	Samsung Electronics Co., Ltd(http://www.samsung.com)
> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef __CPU_COOLING_H__
> +#define __CPU_COOLING_H__
> +
> +#include <linux/thermal.h>
> +
> +struct freq_pctg_table {
> +	unsigned int freq_clip_pctg[NR_CPUS];
> +	unsigned int polling_interval;
> +};
> +
> +extern struct thermal_cooling_device *cpufreq_cooling_register(
> +	struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> +	const struct cpumask *mask_val);

I suppose your original idea was to have this function called only once right?
I'd suggest to add some documentation on this header file to specify this.

Besides, the fact that you receive a cpumask may suggest you could register
different cooling device for subsets of your system cpus.

In any case, if you call this function more than once, you may end with memory leaks
and the last call will be the one acting in the system.



> +
> +extern void cpufreq_cooling_unregister(void);
> +
> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
> +	const struct cpumask *mask_val);
> +
> +extern void cpuhotplug_cooling_unregister(void);
> +
> +#endif /* __CPU_COOLING_H__ */

Another thing, I have the impression that solving the config selection in the
header file instead of in .c files is cleaner. If you want to solve the config
selection in .c files, better to do so with Makefiles.

If you keep the ifdefery ( THERMAL vs. HOTPLUG_CPU vs. CPU_FREQ ) here in the
header file, it is easier to read.

> -- 
> 1.7.1
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [linux-pm] [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Amit Kachhap]

Hi eduardo,

Again thanks for the review.

On 7 February 2012 00:25, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> Hello Amit,
>
> On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling and cpuhotplugg currently.
>> Different cpu related cooling devices can be registered by the
>> user and the binding of these cooling devices to the corresponding
>> trip points can be easily done as the registration API's return the
>> cooling device pointer.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>>  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
>>  drivers/thermal/Kconfig                   |   11 +
>>  drivers/thermal/Makefile                  |    1 +
>>  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
>>  include/linux/cpu_cooling.h               |   45 +++++
>>  5 files changed, 411 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>>  create mode 100644 drivers/thermal/cpu_cooling.c
>>  create mode 100644 include/linux/cpu_cooling.h
>>
>> diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
>> new file mode 100644
>> index 0000000..d30b4f2
>> --- /dev/null
>> +++ b/Documentation/thermal/cpu-cooling-api.txt
>> @@ -0,0 +1,52 @@
>> +CPU cooling api's How To
>> +===================================
>> +
>> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> +
>> +Updated: 13 Dec 2011
>> +
>> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
>> +
>> +0. Introduction
>> +
>> +The generic cpu cooling(freq clipping, cpuhotplug) provides
>> +registration/unregistration api's to the user. The binding of the cooling
>> +devices to the trip types is left for the user. The registration api's returns
>> +the cooling device pointer.
>> +
>> +1. cpufreq cooling api's
>> +
>> +1.1 cpufreq registration api's
>> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val)
>> +
>> +    This interface function registers the cpufreq cooling device with the name
>> +     "thermal-cpufreq".
>> +
>> +    tab_ptr: The table containing the percentage of frequency to be clipped for
>> +    each cooling state.
>> +     .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
>> +      cpu.
>> +     .polling_interval: polling interval for this cooling state.
>> +    tab_size: the total number of cooling state.
>> +    mask_val: all the allowed cpu's where frequency clipping can happen.
>> +
>> +1.1.2 void cpufreq_cooling_unregister(void)
>> +
>> +    This interface function unregisters the "thermal-cpufreq" cooling device.
>> +
>> +
>> +1.2 cpuhotplug registration api's
>> +
>> +1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +     const struct cpumask *mask_val)
>> +
>> +    This interface function registers the cpuhotplug cooling device with the name
>> +     "thermal-cpuhotplug".
>> +
>> +    mask_val: all the allowed cpu's which can be hotplugged out.
>> +
>> +1.1.2 void cpuhotplug_cooling_unregister(void)
>> +
>> +    This interface function unregisters the "thermal-cpuhotplug" cooling device.
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index f7f71b2..298c1cd 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -18,3 +18,14 @@ config THERMAL_HWMON
>>       depends on THERMAL
>>       depends on HWMON=y || HWMON=THERMAL
>>       default y
>> +
>> +config CPU_THERMAL
>> +     bool "generic cpu cooling support"
>> +     depends on THERMAL
>> +     help
>> +       This implements the generic cpu cooling mechanism through frequency
>> +       reduction, cpu hotplug and any other ways of reducing temperature. An
>> +       ACPI version of this already exists(drivers/acpi/processor_thermal.c).
>> +       This will be useful for platforms using the generic thermal interface
>> +       and not the ACPI interface.
>> +       If you want this support, you should say Y or M here.
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 31108a0..655cbc4 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -3,3 +3,4 @@
>>  #
>>
>>  obj-$(CONFIG_THERMAL)                += thermal_sys.o
>> +obj-$(CONFIG_CPU_THERMAL)    += cpu_cooling.o
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> new file mode 100644
>> index 0000000..cdd148c
>> --- /dev/null
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -0,0 +1,302 @@
>> +/*
>> + *  linux/drivers/thermal/cpu_cooling.c
>> + *
>> + *  Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
>> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful, but
>> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpu_cooling.h>
>> +
>> +#ifdef CONFIG_CPU_FREQ
>> +struct cpufreq_cooling_device {
>> +     struct thermal_cooling_device *cool_dev;
>> +     struct freq_pctg_table *tab_ptr;
>> +     unsigned int tab_size;
>> +     unsigned int cpufreq_state;
>> +     const struct cpumask *allowed_cpus;
>> +};
>> +
>> +static struct cpufreq_cooling_device *cpufreq_device;
>> +
>> +/*Below codes defines functions to be used for cpufreq as cooling device*/
>> +static bool is_cpufreq_valid(int cpu)
>> +{
>> +     struct cpufreq_policy policy;
>> +     if (!cpufreq_get_policy(&policy, cpu))
>> +             return true;
>> +     return false;
>> +}
>> +
>> +static int cpufreq_apply_cooling(int cooling_state)
>> +{
>
> Why do you need cooling_state to be signed? You used it always against
> unsigned values. Besides, the thermal api imposes unsigned long.
Yes, unsigned long is a better way.
>
>> +     int cpuid, this_cpu = smp_processor_id();
>> +
>> +     if (!is_cpufreq_valid(this_cpu))
>> +             return 0;
>> +
>> +     if (cooling_state > cpufreq_device->tab_size)
>> +             return -EINVAL;
>> +
>> +     /*Check if last cooling level is same as current cooling level*/
>> +     if (cpufreq_device->cpufreq_state == cooling_state)
>> +             return 0;
>> +
>> +     cpufreq_device->cpufreq_state = cooling_state;
>> +
>> +     for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
>> +             if (is_cpufreq_valid(cpuid))
>> +                     cpufreq_update_policy(cpuid);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
>> +                                     unsigned long event, void *data)
>> +{
>> +     struct cpufreq_policy *policy = data;
>> +     struct freq_pctg_table *th_table;
>> +     unsigned long max_freq = 0;
>> +     unsigned int cpu = policy->cpu, th_pctg = 0, level;
>> +
>> +     if (event != CPUFREQ_ADJUST)
>> +             return 0;
>> +
>> +     level = cpufreq_device->cpufreq_state;
>> +
>> +     if (level > 0) {
>> +             th_table =
>> +                     &(cpufreq_device->tab_ptr[level - 1]);
>> +             th_pctg = th_table->freq_clip_pctg[cpu];
>> +     }
>> +
>> +     max_freq =
>> +             (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
>
>
> Might be interesting to extend this a bit. You could have the
> above as default policy setup, but you could also allow people
> to change this behavior. For instance, adding some callback option
> while registering the cooling device (or a notifier), which would
> determine the max_freq for the generic layer. The mechanism to
> deploy max_freq into the system must be generic enough with cpufreq
> APIs, but the decision to which max_freq to use, could be specific.
> Don't you think?
I agree that there will always some constraints on the system to cap
the max frequency. But these thermal clippings is called for rare
scenarios. Anyway, Your idea is worth considering. I will check if
cpufreq has any generic way of achieving this.
>
>> +
>> +     cpufreq_verify_within_limits(policy, 0, max_freq);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * cpufreq cooling device callback functions
>> + */
>> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long *state)
>> +{
>> +     *state = cpufreq_device->tab_size;
>> +     return 0;
>> +}
>> +
>> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long *state)
>> +{
>> +     *state = cpufreq_device->cpufreq_state;
>> +     return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
>> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long state)
>> +{
>> +     cpufreq_apply_cooling(state);
>> +     return 0;
>> +}
>> +
>> +/* bind cpufreq callbacks to cpufreq cooling device */
>> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
>> +     .get_max_state = cpufreq_get_max_state,
>> +     .get_cur_state = cpufreq_get_cur_state,
>> +     .set_cur_state = cpufreq_set_cur_state,
>> +};
>> +
>> +static struct notifier_block thermal_cpufreq_notifier_block = {
>> +     .notifier_call = thermal_cpufreq_notifier,
>> +};
>> +
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val)
>> +{
>> +     struct thermal_cooling_device *cool_dev;
>> +
>> +     if (tab_ptr == NULL || tab_size == 0)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     cpufreq_device =
>> +             kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
>> +
>> +     if (!cpufreq_device)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
>> +                                             &cpufreq_cooling_ops);
>> +     if (!cool_dev) {
>> +             kfree(cpufreq_device);
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     cpufreq_device->tab_ptr = tab_ptr;
>> +     cpufreq_device->tab_size = tab_size;
>> +     cpufreq_device->cool_dev = cool_dev;
>> +     cpufreq_device->allowed_cpus = mask_val;
>> +
>> +     cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
>> +                                             CPUFREQ_POLICY_NOTIFIER);
>> +     return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +
>> +void cpufreq_cooling_unregister(void)
>> +{
>> +     cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
>> +                                             CPUFREQ_POLICY_NOTIFIER);
>> +     thermal_cooling_device_unregister(cpufreq_device->cool_dev);
>> +     kfree(cpufreq_device);
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#else /*!CONFIG_CPU_FREQ*/
>> +struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size)
>> +{
>> +     return NULL;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_register);
>> +void cpufreq_cooling_unregister(void)
>> +{
>> +     return;
>> +}
>> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
>> +#endif /*CONFIG_CPU_FREQ*/
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +
>> +struct hotplug_cooling_device {
>> +     struct thermal_cooling_device *cool_dev;
>> +     unsigned int hotplug_state;
>> +     const struct cpumask *allowed_cpus;
>> +};
>> +static struct hotplug_cooling_device *hotplug_device;
>> +
>> +/*
>> + * cpu hotplug cooling device callback functions
>> + */
>> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long *state)
>> +{
>> +     *state = 1;
>> +     return 0;
>> +}
>> +
>> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long *state)
>> +{
>> +     /*This cooling device may be of type ACTIVE, so state field
>> +     can be 0 or 1*/
>> +     *state = hotplug_device->hotplug_state;
>> +     return 0;
>> +}
>> +
>> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
>> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
>> +                              unsigned long state)
>> +{
>> +     int cpuid, this_cpu = smp_processor_id();
>> +
>> +     if (hotplug_device->hotplug_state == state)
>> +             return 0;
>> +
>> +     /*This cooling device may be of type ACTIVE, so state field
>> +     can be 0 or 1*/
>
> /*
>  * Small thing. Check the style for multi line comments.
>  * This is also an example.
>  */
Ok.
>
>> +     if (state == 1) {
>> +             for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
>> +                     if (cpu_online(cpuid) && (cpuid != this_cpu))
>> +                             cpu_down(cpuid);
>> +             }
>> +     } else if (state == 0) {
>> +             for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
>> +                     if (!cpu_online(cpuid) && (cpuid != this_cpu))
>> +                             cpu_up(cpuid);
>> +             }
>> +     } else
>> +             return -EINVAL;
>> +
>> +     hotplug_device->hotplug_state = state;
>> +
>> +     return 0;
>> +}
>> +/* bind hotplug callbacks to cpu hotplug cooling device */
>> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
>> +     .get_max_state = cpuhotplug_get_max_state,
>> +     .get_cur_state = cpuhotplug_get_cur_state,
>> +     .set_cur_state = cpuhotplug_set_cur_state,
>> +};
>> +
>> +struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +     const struct cpumask *mask_val)
>> +{
>> +     struct thermal_cooling_device *cool_dev;
>> +
>> +     hotplug_device =
>> +             kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
>> +
>> +     if (!hotplug_device)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
>> +                                             &cpuhotplug_cooling_ops);
>> +     if (!cool_dev) {
>> +             kfree(hotplug_device);
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     hotplug_device->cool_dev = cool_dev;
>> +     hotplug_device->hotplug_state = 0;
>> +     hotplug_device->allowed_cpus = mask_val;
>> +
>> +     return cool_dev;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
>> +
>> +void cpuhotplug_cooling_unregister(void)
>> +{
>> +     thermal_cooling_device_unregister(hotplug_device->cool_dev);
>> +     kfree(hotplug_device);
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
>> +#else /*!CONFIG_HOTPLUG_CPU*/
>> +struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +     const struct cpumask *mask_val)
>> +{
>> +     return NULL;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
>> +void cpuhotplug_cooling_unregister(void)
>> +{
>> +     return;
>> +}
>> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
>> +#endif /*CONFIG_HOTPLUG_CPU*/
>> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
>> new file mode 100644
>> index 0000000..0c57375
>> --- /dev/null
>> +++ b/include/linux/cpu_cooling.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + *  linux/include/linux/cpu_cooling.h
>> + *
>> + *  Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
>> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful, but
>> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +
>> +#ifndef __CPU_COOLING_H__
>> +#define __CPU_COOLING_H__
>> +
>> +#include <linux/thermal.h>
>> +
>> +struct freq_pctg_table {
>> +     unsigned int freq_clip_pctg[NR_CPUS];
>> +     unsigned int polling_interval;
>> +};
>> +
>> +extern struct thermal_cooling_device *cpufreq_cooling_register(
>> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
>> +     const struct cpumask *mask_val);
>
> I suppose your original idea was to have this function called only once right?
> I'd suggest to add some documentation on this header file to specify this.
>
> Besides, the fact that you receive a cpumask may suggest you could register
> different cooling device for subsets of your system cpus.
>
> In any case, if you call this function more than once, you may end with memory leaks
> and the last call will be the one acting in the system.
>
yes these api's are meant to be called once. I am working on a new
version where all these function will be multi-instance and hence
different cpus can support different frequency clipping.
>
>
>> +
>> +extern void cpufreq_cooling_unregister(void);
>> +
>> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
>> +     const struct cpumask *mask_val);
>> +
>> +extern void cpuhotplug_cooling_unregister(void);
>> +
>> +#endif /* __CPU_COOLING_H__ */
>
> Another thing, I have the impression that solving the config selection in the
> header file instead of in .c files is cleaner. If you want to solve the config
> selection in .c files, better to do so with Makefiles.
>
> If you keep the ifdefery ( THERMAL vs. HOTPLUG_CPU vs. CPU_FREQ ) here in the
> header file, it is easier to read.
Ok will move them in .h
>
>> --
>> 1.7.1
>>
>> _______________________________________________
>> linux-pm mailing list
>> linux-pm@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [linux-pm] [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation

[Eduardo Valentin]

Hello Amit,

On Tue, Feb 07, 2012 at 10:21:15AM -0800, Amit Kachhap wrote:
> Hi eduardo,
> 
> Again thanks for the review.
> 
> On 7 February 2012 00:25, Eduardo Valentin <eduardo.valentin@ti.com> wrote:
> > Hello Amit,
> >
> > On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
> >> This patch adds support for generic cpu thermal cooling low level
> >> implementations using frequency scaling and cpuhotplugg currently.
> >> Different cpu related cooling devices can be registered by the
> >> user and the binding of these cooling devices to the corresponding
> >> trip points can be easily done as the registration API's return the
> >> cooling device pointer.
> >>
> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >> ---
> >>  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
> >>  drivers/thermal/Kconfig                   |   11 +
> >>  drivers/thermal/Makefile                  |    1 +
> >>  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
> >>  include/linux/cpu_cooling.h               |   45 +++++
> >>  5 files changed, 411 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
> >>  create mode 100644 drivers/thermal/cpu_cooling.c
> >>  create mode 100644 include/linux/cpu_cooling.h
> >>
> >> diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt
> >> new file mode 100644
> >> index 0000000..d30b4f2
> >> --- /dev/null
> >> +++ b/Documentation/thermal/cpu-cooling-api.txt
> >> @@ -0,0 +1,52 @@
> >> +CPU cooling api's How To
> >> +===================================
> >> +
> >> +Written by Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >> +
> >> +Updated: 13 Dec 2011
> >> +
> >> +Copyright (c)  2011 Samsung Electronics Co., Ltd(http://www.samsung.com)
> >> +
> >> +0. Introduction
> >> +
> >> +The generic cpu cooling(freq clipping, cpuhotplug) provides
> >> +registration/unregistration api's to the user. The binding of the cooling
> >> +devices to the trip types is left for the user. The registration api's returns
> >> +the cooling device pointer.
> >> +
> >> +1. cpufreq cooling api's
> >> +
> >> +1.1 cpufreq registration api's
> >> +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register(
> >> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> >> +     const struct cpumask *mask_val)
> >> +
> >> +    This interface function registers the cpufreq cooling device with the name
> >> +     "thermal-cpufreq".
> >> +
> >> +    tab_ptr: The table containing the percentage of frequency to be clipped for
> >> +    each cooling state.
> >> +     .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each
> >> +      cpu.
> >> +     .polling_interval: polling interval for this cooling state.
> >> +    tab_size: the total number of cooling state.
> >> +    mask_val: all the allowed cpu's where frequency clipping can happen.
> >> +
> >> +1.1.2 void cpufreq_cooling_unregister(void)
> >> +
> >> +    This interface function unregisters the "thermal-cpufreq" cooling device.
> >> +
> >> +
> >> +1.2 cpuhotplug registration api's
> >> +
> >> +1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register(
> >> +     const struct cpumask *mask_val)
> >> +
> >> +    This interface function registers the cpuhotplug cooling device with the name
> >> +     "thermal-cpuhotplug".
> >> +
> >> +    mask_val: all the allowed cpu's which can be hotplugged out.
> >> +
> >> +1.1.2 void cpuhotplug_cooling_unregister(void)
> >> +
> >> +    This interface function unregisters the "thermal-cpuhotplug" cooling device.
> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> >> index f7f71b2..298c1cd 100644
> >> --- a/drivers/thermal/Kconfig
> >> +++ b/drivers/thermal/Kconfig
> >> @@ -18,3 +18,14 @@ config THERMAL_HWMON
> >>       depends on THERMAL
> >>       depends on HWMON=y || HWMON=THERMAL
> >>       default y
> >> +
> >> +config CPU_THERMAL
> >> +     bool "generic cpu cooling support"
> >> +     depends on THERMAL
> >> +     help
> >> +       This implements the generic cpu cooling mechanism through frequency
> >> +       reduction, cpu hotplug and any other ways of reducing temperature. An
> >> +       ACPI version of this already exists(drivers/acpi/processor_thermal.c).
> >> +       This will be useful for platforms using the generic thermal interface
> >> +       and not the ACPI interface.
> >> +       If you want this support, you should say Y or M here.
> >> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> >> index 31108a0..655cbc4 100644
> >> --- a/drivers/thermal/Makefile
> >> +++ b/drivers/thermal/Makefile
> >> @@ -3,3 +3,4 @@
> >>  #
> >>
> >>  obj-$(CONFIG_THERMAL)                += thermal_sys.o
> >> +obj-$(CONFIG_CPU_THERMAL)    += cpu_cooling.o
> >> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> >> new file mode 100644
> >> index 0000000..cdd148c
> >> --- /dev/null
> >> +++ b/drivers/thermal/cpu_cooling.c
> >> @@ -0,0 +1,302 @@
> >> +/*
> >> + *  linux/drivers/thermal/cpu_cooling.c
> >> + *
> >> + *  Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
> >> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> >> + *
> >> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> + *  This program is free software; you can redistribute it and/or modify
> >> + *  it under the terms of the GNU General Public License as published by
> >> + *  the Free Software Foundation; version 2 of the License.
> >> + *
> >> + *  This program is distributed in the hope that it will be useful, but
> >> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + *  General Public License for more details.
> >> + *
> >> + *  You should have received a copy of the GNU General Public License along
> >> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> >> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> >> + *
> >> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> + */
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/thermal.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/cpufreq.h>
> >> +#include <linux/err.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/cpu.h>
> >> +#include <linux/cpu_cooling.h>
> >> +
> >> +#ifdef CONFIG_CPU_FREQ
> >> +struct cpufreq_cooling_device {
> >> +     struct thermal_cooling_device *cool_dev;
> >> +     struct freq_pctg_table *tab_ptr;
> >> +     unsigned int tab_size;
> >> +     unsigned int cpufreq_state;
> >> +     const struct cpumask *allowed_cpus;
> >> +};
> >> +
> >> +static struct cpufreq_cooling_device *cpufreq_device;
> >> +
> >> +/*Below codes defines functions to be used for cpufreq as cooling device*/
> >> +static bool is_cpufreq_valid(int cpu)
> >> +{
> >> +     struct cpufreq_policy policy;
> >> +     if (!cpufreq_get_policy(&policy, cpu))
> >> +             return true;
> >> +     return false;
> >> +}
> >> +
> >> +static int cpufreq_apply_cooling(int cooling_state)
> >> +{
> >
> > Why do you need cooling_state to be signed? You used it always against
> > unsigned values. Besides, the thermal api imposes unsigned long.
> Yes, unsigned long is a better way.
> >
> >> +     int cpuid, this_cpu = smp_processor_id();
> >> +
> >> +     if (!is_cpufreq_valid(this_cpu))
> >> +             return 0;
> >> +
> >> +     if (cooling_state > cpufreq_device->tab_size)
> >> +             return -EINVAL;
> >> +
> >> +     /*Check if last cooling level is same as current cooling level*/
> >> +     if (cpufreq_device->cpufreq_state == cooling_state)
> >> +             return 0;
> >> +
> >> +     cpufreq_device->cpufreq_state = cooling_state;
> >> +
> >> +     for_each_cpu(cpuid, cpufreq_device->allowed_cpus) {
> >> +             if (is_cpufreq_valid(cpuid))
> >> +                     cpufreq_update_policy(cpuid);
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int thermal_cpufreq_notifier(struct notifier_block *nb,
> >> +                                     unsigned long event, void *data)
> >> +{
> >> +     struct cpufreq_policy *policy = data;
> >> +     struct freq_pctg_table *th_table;
> >> +     unsigned long max_freq = 0;
> >> +     unsigned int cpu = policy->cpu, th_pctg = 0, level;
> >> +
> >> +     if (event != CPUFREQ_ADJUST)
> >> +             return 0;
> >> +
> >> +     level = cpufreq_device->cpufreq_state;
> >> +
> >> +     if (level > 0) {
> >> +             th_table =
> >> +                     &(cpufreq_device->tab_ptr[level - 1]);
> >> +             th_pctg = th_table->freq_clip_pctg[cpu];
> >> +     }
> >> +
> >> +     max_freq =
> >> +             (policy->cpuinfo.max_freq * (100 - th_pctg)) / 100;
> >
> >
> > Might be interesting to extend this a bit. You could have the
> > above as default policy setup, but you could also allow people
> > to change this behavior. For instance, adding some callback option
> > while registering the cooling device (or a notifier), which would
> > determine the max_freq for the generic layer. The mechanism to
> > deploy max_freq into the system must be generic enough with cpufreq
> > APIs, but the decision to which max_freq to use, could be specific.
> > Don't you think?
> I agree that there will always some constraints on the system to cap
> the max frequency. But these thermal clippings is called for rare
> scenarios. Anyway, Your idea is worth considering. I will check if
> cpufreq has any generic way of achieving this.

Yeah, well, the idea was to have the max_freq value coming from some
registered callback in your code, not really on cpufreq.

> >
> >> +
> >> +     cpufreq_verify_within_limits(policy, 0, max_freq);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +/*
> >> + * cpufreq cooling device callback functions
> >> + */
> >> +static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
> >> +                              unsigned long *state)
> >> +{
> >> +     *state = cpufreq_device->tab_size;
> >> +     return 0;
> >> +}
> >> +
> >> +static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> >> +                              unsigned long *state)
> >> +{
> >> +     *state = cpufreq_device->cpufreq_state;
> >> +     return 0;
> >> +}
> >> +
> >> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> >> +static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
> >> +                              unsigned long state)
> >> +{
> >> +     cpufreq_apply_cooling(state);
> >> +     return 0;
> >> +}
> >> +
> >> +/* bind cpufreq callbacks to cpufreq cooling device */
> >> +static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> >> +     .get_max_state = cpufreq_get_max_state,
> >> +     .get_cur_state = cpufreq_get_cur_state,
> >> +     .set_cur_state = cpufreq_set_cur_state,
> >> +};
> >> +
> >> +static struct notifier_block thermal_cpufreq_notifier_block = {
> >> +     .notifier_call = thermal_cpufreq_notifier,
> >> +};
> >> +
> >> +struct thermal_cooling_device *cpufreq_cooling_register(
> >> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> >> +     const struct cpumask *mask_val)
> >> +{
> >> +     struct thermal_cooling_device *cool_dev;
> >> +
> >> +     if (tab_ptr == NULL || tab_size == 0)
> >> +             return ERR_PTR(-EINVAL);
> >> +
> >> +     cpufreq_device =
> >> +             kzalloc(sizeof(struct cpufreq_cooling_device), GFP_KERNEL);
> >> +
> >> +     if (!cpufreq_device)
> >> +             return ERR_PTR(-ENOMEM);
> >> +
> >> +     cool_dev = thermal_cooling_device_register("thermal-cpufreq", NULL,
> >> +                                             &cpufreq_cooling_ops);
> >> +     if (!cool_dev) {
> >> +             kfree(cpufreq_device);
> >> +             return ERR_PTR(-EINVAL);
> >> +     }
> >> +
> >> +     cpufreq_device->tab_ptr = tab_ptr;
> >> +     cpufreq_device->tab_size = tab_size;
> >> +     cpufreq_device->cool_dev = cool_dev;
> >> +     cpufreq_device->allowed_cpus = mask_val;
> >> +
> >> +     cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> >> +                                             CPUFREQ_POLICY_NOTIFIER);
> >> +     return cool_dev;
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_register);
> >> +
> >> +void cpufreq_cooling_unregister(void)
> >> +{
> >> +     cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
> >> +                                             CPUFREQ_POLICY_NOTIFIER);
> >> +     thermal_cooling_device_unregister(cpufreq_device->cool_dev);
> >> +     kfree(cpufreq_device);
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> >> +#else /*!CONFIG_CPU_FREQ*/
> >> +struct thermal_cooling_device *cpufreq_cooling_register(
> >> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size)
> >> +{
> >> +     return NULL;
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_register);
> >> +void cpufreq_cooling_unregister(void)
> >> +{
> >> +     return;
> >> +}
> >> +EXPORT_SYMBOL(cpufreq_cooling_unregister);
> >> +#endif /*CONFIG_CPU_FREQ*/
> >> +
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> +
> >> +struct hotplug_cooling_device {
> >> +     struct thermal_cooling_device *cool_dev;
> >> +     unsigned int hotplug_state;
> >> +     const struct cpumask *allowed_cpus;
> >> +};
> >> +static struct hotplug_cooling_device *hotplug_device;
> >> +
> >> +/*
> >> + * cpu hotplug cooling device callback functions
> >> + */
> >> +static int cpuhotplug_get_max_state(struct thermal_cooling_device *cdev,
> >> +                              unsigned long *state)
> >> +{
> >> +     *state = 1;
> >> +     return 0;
> >> +}
> >> +
> >> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
> >> +                              unsigned long *state)
> >> +{
> >> +     /*This cooling device may be of type ACTIVE, so state field
> >> +     can be 0 or 1*/
> >> +     *state = hotplug_device->hotplug_state;
> >> +     return 0;
> >> +}
> >> +
> >> +/*This cooling may be as PASSIVE/STATE_ACTIVE type*/
> >> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
> >> +                              unsigned long state)
> >> +{
> >> +     int cpuid, this_cpu = smp_processor_id();
> >> +
> >> +     if (hotplug_device->hotplug_state == state)
> >> +             return 0;
> >> +
> >> +     /*This cooling device may be of type ACTIVE, so state field
> >> +     can be 0 or 1*/
> >
> > /*
> >  * Small thing. Check the style for multi line comments.
> >  * This is also an example.
> >  */
> Ok.
> >
> >> +     if (state == 1) {
> >> +             for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> >> +                     if (cpu_online(cpuid) && (cpuid != this_cpu))
> >> +                             cpu_down(cpuid);
> >> +             }
> >> +     } else if (state == 0) {
> >> +             for_each_cpu(cpuid, hotplug_device->allowed_cpus) {
> >> +                     if (!cpu_online(cpuid) && (cpuid != this_cpu))
> >> +                             cpu_up(cpuid);
> >> +             }
> >> +     } else
> >> +             return -EINVAL;
> >> +
> >> +     hotplug_device->hotplug_state = state;
> >> +
> >> +     return 0;
> >> +}
> >> +/* bind hotplug callbacks to cpu hotplug cooling device */
> >> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
> >> +     .get_max_state = cpuhotplug_get_max_state,
> >> +     .get_cur_state = cpuhotplug_get_cur_state,
> >> +     .set_cur_state = cpuhotplug_set_cur_state,
> >> +};
> >> +
> >> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> >> +     const struct cpumask *mask_val)
> >> +{
> >> +     struct thermal_cooling_device *cool_dev;
> >> +
> >> +     hotplug_device =
> >> +             kzalloc(sizeof(struct hotplug_cooling_device), GFP_KERNEL);
> >> +
> >> +     if (!hotplug_device)
> >> +             return ERR_PTR(-ENOMEM);
> >> +
> >> +     cool_dev = thermal_cooling_device_register("thermal-cpuhotplug", NULL,
> >> +                                             &cpuhotplug_cooling_ops);
> >> +     if (!cool_dev) {
> >> +             kfree(hotplug_device);
> >> +             return ERR_PTR(-EINVAL);
> >> +     }
> >> +
> >> +     hotplug_device->cool_dev = cool_dev;
> >> +     hotplug_device->hotplug_state = 0;
> >> +     hotplug_device->allowed_cpus = mask_val;
> >> +
> >> +     return cool_dev;
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> >> +
> >> +void cpuhotplug_cooling_unregister(void)
> >> +{
> >> +     thermal_cooling_device_unregister(hotplug_device->cool_dev);
> >> +     kfree(hotplug_device);
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> >> +#else /*!CONFIG_HOTPLUG_CPU*/
> >> +struct thermal_cooling_device *cpuhotplug_cooling_register(
> >> +     const struct cpumask *mask_val)
> >> +{
> >> +     return NULL;
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_register);
> >> +void cpuhotplug_cooling_unregister(void)
> >> +{
> >> +     return;
> >> +}
> >> +EXPORT_SYMBOL(cpuhotplug_cooling_unregister);
> >> +#endif /*CONFIG_HOTPLUG_CPU*/
> >> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> >> new file mode 100644
> >> index 0000000..0c57375
> >> --- /dev/null
> >> +++ b/include/linux/cpu_cooling.h
> >> @@ -0,0 +1,45 @@
> >> +/*
> >> + *  linux/include/linux/cpu_cooling.h
> >> + *
> >> + *  Copyright (C) 2011       Samsung Electronics Co., Ltd(http://www.samsung.com)
> >> + *  Copyright (C) 2011  Amit Daniel <amit.kachhap@linaro.org>
> >> + *
> >> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> + *  This program is free software; you can redistribute it and/or modify
> >> + *  it under the terms of the GNU General Public License as published by
> >> + *  the Free Software Foundation; version 2 of the License.
> >> + *
> >> + *  This program is distributed in the hope that it will be useful, but
> >> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + *  General Public License for more details.
> >> + *
> >> + *  You should have received a copy of the GNU General Public License along
> >> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> >> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> >> + *
> >> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> + */
> >> +
> >> +#ifndef __CPU_COOLING_H__
> >> +#define __CPU_COOLING_H__
> >> +
> >> +#include <linux/thermal.h>
> >> +
> >> +struct freq_pctg_table {
> >> +     unsigned int freq_clip_pctg[NR_CPUS];
> >> +     unsigned int polling_interval;
> >> +};
> >> +
> >> +extern struct thermal_cooling_device *cpufreq_cooling_register(
> >> +     struct freq_pctg_table *tab_ptr, unsigned int tab_size,
> >> +     const struct cpumask *mask_val);
> >
> > I suppose your original idea was to have this function called only once right?
> > I'd suggest to add some documentation on this header file to specify this.
> >
> > Besides, the fact that you receive a cpumask may suggest you could register
> > different cooling device for subsets of your system cpus.
> >
> > In any case, if you call this function more than once, you may end with memory leaks
> > and the last call will be the one acting in the system.
> >
> yes these api's are meant to be called once. I am working on a new
> version where all these function will be multi-instance and hence
> different cpus can support different frequency clipping.


OK. 

Another point, is about the fact that this implementation is clipping the frequency silently.
I mean, by changing the policy.max_freq, we may conflict with what the user defines as max freq.

> >
> >
> >> +
> >> +extern void cpufreq_cooling_unregister(void);
> >> +
> >> +extern struct thermal_cooling_device *cpuhotplug_cooling_register(
> >> +     const struct cpumask *mask_val);
> >> +
> >> +extern void cpuhotplug_cooling_unregister(void);
> >> +
> >> +#endif /* __CPU_COOLING_H__ */
> >
> > Another thing, I have the impression that solving the config selection in the
> > header file instead of in .c files is cleaner. If you want to solve the config
> > selection in .c files, better to do so with Makefiles.
> >
> > If you keep the ifdefery ( THERMAL vs. HOTPLUG_CPU vs. CPU_FREQ ) here in the
> > header file, it is easier to read.
> Ok will move them in .h
> >
> >> --
> >> 1.7.1
> >>
> >> _______________________________________________
> >> linux-pm mailing list
> >> linux-pm@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework

[Amit Kachhap]

On 13 December 2011 20:43, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> PATCH 1)  [thermal: Add a new trip type to use cooling device instance number]
> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
> cooling device instance number and may be helpful for cpufreq cooling devices
> to take the correct cooling action.
>
> PATCH 2)  [thermal: Add generic cpu cooling implementation]
> This patch 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.
>

Any comments on these patches? I submitted them quite long back.

Regards,
Amit D

>
> Amit Daniel Kachhap (2):
>  thermal: Add a new trip type to use cooling device instance number
>  thermal: Add generic cpu cooling implementation
>
>  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
>  Documentation/thermal/sysfs-api.txt       |    4 +-
>  drivers/thermal/Kconfig                   |   11 +
>  drivers/thermal/Makefile                  |    1 +
>  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
>  drivers/thermal/thermal_sys.c             |   27 +++-
>  include/linux/cpu_cooling.h               |   45 +++++
>  include/linux/thermal.h                   |    1 +
>  8 files changed, 440 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>  create mode 100644 drivers/thermal/cpu_cooling.c
>  create mode 100644 include/linux/cpu_cooling.h
>

Re: [RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework

[Zhang Rui]

Hi, sorry for the late response.

On 四, 2012-01-19 at 14:47 +0530, Amit Kachhap wrote:
> On 13 December 2011 20:43, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> > PATCH 1)  [thermal: Add a new trip type to use cooling device instance number]
> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
> > cooling device instance number and may be helpful for cpufreq cooling devices
> > to take the correct cooling action.
> >
Sorry, I'm still not quite clear about how this will be used.
Say, processor has P0~P3, then we need to register a thermal zone with
three STATE_ACTIVE trip points, like the picture shown below?

processor in full speed, i.e. P0
---------------------------- 
state active trip point 1
----------------------------
processor in P1
----------------------------
state active trip point 2
----------------------------
processor in P2
----------------------------
state active trip point 3
----------------------------
processor in P3

> > PATCH 2)  [thermal: Add generic cpu cooling implementation]
> > This patch 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.
> >
It seems that we can convert the ACPI processor thermal driver to follow
this generic cpu cooling implementation, right?

thanks,
rui
> 
> Any comments on these patches? I submitted them quite long back.
> 
> Regards,
> Amit D
> 
> >
> > Amit Daniel Kachhap (2):
> >  thermal: Add a new trip type to use cooling device instance number
> >  thermal: Add generic cpu cooling implementation
> >
> >  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
> >  Documentation/thermal/sysfs-api.txt       |    4 +-
> >  drivers/thermal/Kconfig                   |   11 +
> >  drivers/thermal/Makefile                  |    1 +
> >  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
> >  drivers/thermal/thermal_sys.c             |   27 +++-
> >  include/linux/cpu_cooling.h               |   45 +++++
> >  include/linux/thermal.h                   |    1 +
> >  8 files changed, 440 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
> >  create mode 100644 drivers/thermal/cpu_cooling.c
> >  create mode 100644 include/linux/cpu_cooling.h
> >

Re: [RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework

[Amit Kachhap]

On 3 February 2012 12:42, Zhang Rui <rui.zhang@intel.com> wrote:
> Hi, sorry for the late response.
>
> On 四, 2012-01-19 at 14:47 +0530, Amit Kachhap wrote:
>> On 13 December 2011 20:43, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>> > PATCH 1)  [thermal: Add a new trip type to use cooling device instance number]
>> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
>> > cooling device instance number and may be helpful for cpufreq cooling devices
>> > to take the correct cooling action.
>> >
> Sorry, I'm still not quite clear about how this will be used.
> Say, processor has P0~P3, then we need to register a thermal zone with
> three STATE_ACTIVE trip points, like the picture shown below?
>
> processor in full speed, i.e. P0
> ----------------------------
> state active trip point 1
> ----------------------------
> processor in P1
> ----------------------------
> state active trip point 2
> ----------------------------
> processor in P2
> ----------------------------
> state active trip point 3
> ----------------------------
> processor in P3
>

Thanks for the review. Your representation of the 3 PSTATES and trip
points are correct. Also remember the case that we have registered one
cpufreq cooling device and binded 3 instance of it with the 3 trip
points. So in the current patch set_cur_state will pass the instance
id also which will help in moving to the current P state.

This same behaviour is achieved through PASSIVE state but it iterates
through all the P states. so the current patch avoids the iteration
and goes to a single state directly.

>> > PATCH 2)  [thermal: Add generic cpu cooling implementation]
>> > This patch 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.
>> >
> It seems that we can convert the ACPI processor thermal driver to follow
> this generic cpu cooling implementation, right?
I am not sure if we can convert the processor thermal driver because
it performs other cpu cooling like throttling, idle etc and then ACPI
abstraction on top of it. I am thinking of exporting the generic low
level cooling implementation like this patch is doing from
processor_thermal.c file so that it can be usable with non-acpi
interface.
What is your opinion about it?

Thanks,
Amit Daniel
>
> thanks,
> rui
>>
>> Any comments on these patches? I submitted them quite long back.
>>
>> Regards,
>> Amit D
>>
>> >
>> > Amit Daniel Kachhap (2):
>> >  thermal: Add a new trip type to use cooling device instance number
>> >  thermal: Add generic cpu cooling implementation
>> >
>> >  Documentation/thermal/cpu-cooling-api.txt |   52 +++++
>> >  Documentation/thermal/sysfs-api.txt       |    4 +-
>> >  drivers/thermal/Kconfig                   |   11 +
>> >  drivers/thermal/Makefile                  |    1 +
>> >  drivers/thermal/cpu_cooling.c             |  302 +++++++++++++++++++++++++++++
>> >  drivers/thermal/thermal_sys.c             |   27 +++-
>> >  include/linux/cpu_cooling.h               |   45 +++++
>> >  include/linux/thermal.h                   |    1 +
>> >  8 files changed, 440 insertions(+), 3 deletions(-)
>> >  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>> >  create mode 100644 drivers/thermal/cpu_cooling.c
>> >  create mode 100644 include/linux/cpu_cooling.h
>> >
>
>