diff mbox

[09/16] Thermal: Introduce thermal_zone_trip_update()

Message ID 1342679480-5336-10-git-send-email-rui.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Zhang Rui July 19, 2012, 6:31 a.m. UTC
This function is used to update the cooling state of
all the cooling devices that are binded to an active trip point.

This will be used for passive cooling as well, in the future patches.
as both active and passive cooling can share the same algorithm,
which is

1. if the temperature is higher than a trip point,
   a. if the trend is THERMAL_TREND_RAISING, use higher cooling
      state for this trip point
   b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
      state for this trip point

2. if the temperature is lower than a trip point, use lower
   cooling state for this trip point.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/thermal.c        |    7 +++-
 drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
 2 files changed, 71 insertions(+), 27 deletions(-)

Comments

Rafael Wysocki July 19, 2012, 9:19 p.m. UTC | #1
On Thursday, July 19, 2012, Zhang Rui wrote:
> This function is used to update the cooling state of
> all the cooling devices that are binded to an active trip point.

s/binded/bound/

> This will be used for passive cooling as well, in the future patches.
> as both active and passive cooling can share the same algorithm,
> which is
> 
> 1. if the temperature is higher than a trip point,
>    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
>       state for this trip point
>    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
>       state for this trip point
> 
> 2. if the temperature is lower than a trip point, use lower
>    cooling state for this trip point.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/thermal.c        |    7 +++-
>  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
>  2 files changed, 71 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 73e335f..14c4879 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
>  	if (thermal_get_trip_type(thermal, trip, &type))
>  		return -EINVAL;
>  
> -	/* Only PASSIVE trip points need TREND */
> +	if (type == THERMAL_TRIP_ACTIVE) {
> +		/* aggressive active cooling */
> +		*trend = THERMAL_TREND_RAISING;
> +		return 0;

Please move that into thermal_zone_trip_update() directly, unless you
need it elsewhere.

> +	}
> +
>  	if (type != THERMAL_TRIP_PASSIVE)
>  		return -EINVAL;
>  
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 59af3b8..011faba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1076,6 +1076,70 @@ void thermal_cooling_device_unregister(struct
>  }
>  EXPORT_SYMBOL(thermal_cooling_device_unregister);
>  
> +/*
> + * Cooling algorithm for active trip points
> + *
> + * 1. if the temperature is higher than a trip point,
> + *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> + *       state for this trip point
> + *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> + *       state for this trip point
> + *
> + * 2. if the temperature is lower than a trip point, use lower
> + *    cooling state for this trip point
> + *
> + * Note that this behaves the same as the previous passive cooling
> + * algorithm.
> + */
> +
> +static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> +				     int trip, long temp)
> +{
> +	struct thermal_cooling_device_instance *instance;
> +	struct thermal_cooling_device *cdev = NULL;
> +	unsigned long cur_state, max_state;
> +	long trip_temp;
> +	enum thermal_trend trend;
> +
> +	tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +
> +	if (temp >= trip_temp) {
> +		thermal_get_trend(tz, trip, &trend);
> +
> +		list_for_each_entry(instance, &tz->cooling_devices, node) {
> +			if (instance->trip != trip)
> +				continue;
> +
> +			cdev = instance->cdev;
> +
> +			cdev->ops->get_cur_state(cdev, &cur_state);
> +			cdev->ops->get_max_state(cdev, &max_state);
> +
> +			if (trend == THERMAL_TREND_RAISING) {
> +				cur_state = cur_state < instance->upper ?
> +					    (cur_state + 1) : instance->upper;
> +			} else if (trend == THERMAL_TREND_DROPPING) {
> +				cur_state = cur_state > instance->lower ?
> +				    (cur_state - 1) : instance->lower;
> +			}
> +			cdev->ops->set_cur_state(cdev, cur_state);
> +		}
> +	} else {	/* below trip */
> +		list_for_each_entry(instance, &tz->cooling_devices, node) {
> +			if (instance->trip != trip)
> +				continue;
> +
> +			cdev = instance->cdev;
> +			cdev->ops->get_cur_state(cdev, &cur_state);
> +
> +			cur_state = cur_state > instance->lower ?
> +				    (cur_state - 1) : instance->lower;
> +			cdev->ops->set_cur_state(cdev, cur_state);
> +		}
> +	}
> +
> +	return;
> +}
>  /**
>   * thermal_zone_device_update - force an update of a thermal zone's state
>   * @ttz:	the thermal zone to update
> @@ -1086,9 +1150,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  	int count, ret = 0;
>  	long temp, trip_temp;
>  	enum thermal_trip_type trip_type;
> -	struct thermal_cooling_device_instance *instance;
> -	struct thermal_cooling_device *cdev;
> -	unsigned long cur_state, max_state;
>  
>  	mutex_lock(&tz->lock);
>  
> @@ -1124,29 +1185,7 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
>  					tz->ops->notify(tz, count, trip_type);
>  			break;
>  		case THERMAL_TRIP_ACTIVE:
> -			list_for_each_entry(instance, &tz->cooling_devices,
> -					    node) {
> -				if (instance->trip != count)
> -					continue;
> -
> -				cdev = instance->cdev;
> -
> -				cdev->ops->get_cur_state(cdev, &cur_state);
> -				cdev->ops->get_max_state(cdev, &max_state);
> -
> -				if (temp >= trip_temp)
> -					cur_state =
> -						cur_state < instance->upper ?
> -						(cur_state + 1) :
> -						instance->upper;
> -				else
> -					cur_state =
> -						cur_state > instance->lower ?
> -						(cur_state - 1) :
> -						instance->lower;
> -
> -				cdev->ops->set_cur_state(cdev, cur_state);
> -			}
> +			thermal_zone_trip_update(tz, count, temp);
>  			break;
>  		case THERMAL_TRIP_PASSIVE:
>  			if (temp >= trip_temp || tz->passive)
> 

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui July 24, 2012, 1:47 a.m. UTC | #2
On ?, 2012-07-19 at 23:19 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 19, 2012, Zhang Rui wrote:
> > This function is used to update the cooling state of
> > all the cooling devices that are binded to an active trip point.
> 
> s/binded/bound/
> 
got it.

> > This will be used for passive cooling as well, in the future patches.
> > as both active and passive cooling can share the same algorithm,
> > which is
> > 
> > 1. if the temperature is higher than a trip point,
> >    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> >       state for this trip point
> >    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> >       state for this trip point
> > 
> > 2. if the temperature is lower than a trip point, use lower
> >    cooling state for this trip point.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/thermal.c        |    7 +++-
> >  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
> >  2 files changed, 71 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index 73e335f..14c4879 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
> >  	if (thermal_get_trip_type(thermal, trip, &type))
> >  		return -EINVAL;
> >  
> > -	/* Only PASSIVE trip points need TREND */
> > +	if (type == THERMAL_TRIP_ACTIVE) {
> > +		/* aggressive active cooling */
> > +		*trend = THERMAL_TREND_RAISING;
> > +		return 0;
> 
> Please move that into thermal_zone_trip_update() directly, unless you
> need it elsewhere.
> 
IMO, I can say that ACPI thermal wants aggressive active cooling, as it
will never spin down the fan when the temperature is higher than the
trip point.

But I'm not sure if this is true for other platforms.
say if a fan have 5 speeds.
it may want to run at the maximum speed when the temperature is raising,
but if the temperature begins to drop, it may want to spin down the fan
a little for some reason, say, power, noise, etc.

so I want the platform thermal drivers to have the maximum flexibility.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui July 24, 2012, 7:11 a.m. UTC | #3
On ?, 2012-07-24 at 12:27 +0530, Amit Kachhap wrote:
> 
> 
> On 19 July 2012 12:01, Zhang Rui <rui.zhang@intel.com> wrote:
>         This function is used to update the cooling state of
>         all the cooling devices that are binded to an active trip
>         point.
>         
>         This will be used for passive cooling as well, in the future
>         patches.
>         as both active and passive cooling can share the same
>         algorithm,
>         which is
>         
>         1. if the temperature is higher than a trip point,
>            a. if the trend is THERMAL_TREND_RAISING, use higher
>         cooling
>               state for this trip point
>            b. if the trend is THERMAL_TREND_DROPPING, use lower
>         cooling
>               state for this trip point
>         
>         2. if the temperature is lower than a trip point, use lower
>            cooling state for this trip point.
> 
> Hi Rui,
> 
> Your patches looks useful.
> For my platform I need to have get_trend called even in the case when
> current temp is less than the trip point and then use
> THERMAL_TREND_DROPPING to actually lower the cooling state.
> 
hmm, why?
in the current cooling algorithm, when the temperature is lower than the
trip point, the cooling state is decreased by 1 every time,
unconditionally, isn't this what you want?

thanks,
rui

> Thanks,
> Amit
>  
>         
>         Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>         ---
>          drivers/acpi/thermal.c        |    7 +++-
>          drivers/thermal/thermal_sys.c |   91
>         +++++++++++++++++++++++++++++------------
>          2 files changed, 71 insertions(+), 27 deletions(-)
>         
>         diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>         index 73e335f..14c4879 100644
>         --- a/drivers/acpi/thermal.c
>         +++ b/drivers/acpi/thermal.c
>         @@ -715,7 +715,12 @@ static int thermal_get_trend(struct
>         thermal_zone_device *thermal,
>                 if (thermal_get_trip_type(thermal, trip, &type))
>                         return -EINVAL;
>         
>         -       /* Only PASSIVE trip points need TREND */
>         +       if (type == THERMAL_TRIP_ACTIVE) {
>         +               /* aggressive active cooling */
>         +               *trend = THERMAL_TREND_RAISING;
>         +               return 0;
>         +       }
>         +
>                 if (type != THERMAL_TRIP_PASSIVE)
>                         return -EINVAL;
>         
>         diff --git a/drivers/thermal/thermal_sys.c
>         b/drivers/thermal/thermal_sys.c
>         index 59af3b8..011faba 100644
>         --- a/drivers/thermal/thermal_sys.c
>         +++ b/drivers/thermal/thermal_sys.c
>         @@ -1076,6 +1076,70 @@ void
>         thermal_cooling_device_unregister(struct
>          }
>          EXPORT_SYMBOL(thermal_cooling_device_unregister);
>         
>         +/*
>         + * Cooling algorithm for active trip points
>         + *
>         + * 1. if the temperature is higher than a trip point,
>         + *    a. if the trend is THERMAL_TREND_RAISING, use higher
>         cooling
>         + *       state for this trip point
>         + *    b. if the trend is THERMAL_TREND_DROPPING, use lower
>         cooling
>         + *       state for this trip point
>         + *
>         + * 2. if the temperature is lower than a trip point, use
>         lower
>         + *    cooling state for this trip point
>         + *
>         + * Note that this behaves the same as the previous passive
>         cooling
>         + * algorithm.
>         + */
>         +
>         +static void thermal_zone_trip_update(struct
>         thermal_zone_device *tz,
>         +                                    int trip, long temp)
>         +{
>         +       struct thermal_cooling_device_instance *instance;
>         +       struct thermal_cooling_device *cdev = NULL;
>         +       unsigned long cur_state, max_state;
>         +       long trip_temp;
>         +       enum thermal_trend trend;
>         +
>         +       tz->ops->get_trip_temp(tz, trip, &trip_temp);
>         +
>         +       if (temp >= trip_temp) {
>         +               thermal_get_trend(tz, trip, &trend);
>         +
>         +               list_for_each_entry(instance,
>         &tz->cooling_devices, node) {
>         +                       if (instance->trip != trip)
>         +                               continue;
>         +
>         +                       cdev = instance->cdev;
>         +
>         +                       cdev->ops->get_cur_state(cdev,
>         &cur_state);
>         +                       cdev->ops->get_max_state(cdev,
>         &max_state);
>         +
>         +                       if (trend == THERMAL_TREND_RAISING) {
>         +                               cur_state = cur_state <
>         instance->upper ?
>         +                                           (cur_state + 1) :
>         instance->upper;
>         +                       } else if (trend ==
>         THERMAL_TREND_DROPPING) {
>         +                               cur_state = cur_state >
>         instance->lower ?
>         +                                   (cur_state - 1) :
>         instance->lower;
>         +                       }
>         +                       cdev->ops->set_cur_state(cdev,
>         cur_state);
>         +               }
>         +       } else {        /* below trip */
>         +               list_for_each_entry(instance,
>         &tz->cooling_devices, node) {
>         +                       if (instance->trip != trip)
>         +                               continue;
>         +
>         +                       cdev = instance->cdev;
>         +                       cdev->ops->get_cur_state(cdev,
>         &cur_state);
>         +
>         +                       cur_state = cur_state >
>         instance->lower ?
>         +                                   (cur_state - 1) :
>         instance->lower;
>         +                       cdev->ops->set_cur_state(cdev,
>         cur_state);
>         +               }
>         +       }
>         +
>         +       return;
>         +}
>          /**
>           * thermal_zone_device_update - force an update of a thermal
>         zone's state
>           * @ttz:       the thermal zone to update
>         @@ -1086,9 +1150,6 @@ void thermal_zone_device_update(struct
>         thermal_zone_device *tz)
>                 int count, ret = 0;
>                 long temp, trip_temp;
>                 enum thermal_trip_type trip_type;
>         -       struct thermal_cooling_device_instance *instance;
>         -       struct thermal_cooling_device *cdev;
>         -       unsigned long cur_state, max_state;
>         
>                 mutex_lock(&tz->lock);
>         
>         @@ -1124,29 +1185,7 @@ void thermal_zone_device_update(struct
>         thermal_zone_device *tz)
>                                                 tz->ops->notify(tz,
>         count, trip_type);
>                                 break;
>                         case THERMAL_TRIP_ACTIVE:
>         -                       list_for_each_entry(instance,
>         &tz->cooling_devices,
>         -                                           node) {
>         -                               if (instance->trip != count)
>         -                                       continue;
>         -
>         -                               cdev = instance->cdev;
>         -
>         -                               cdev->ops->get_cur_state(cdev,
>         &cur_state);
>         -                               cdev->ops->get_max_state(cdev,
>         &max_state);
>         -
>         -                               if (temp >= trip_temp)
>         -                                       cur_state =
>         -                                               cur_state <
>         instance->upper ?
>         -                                               (cur_state +
>         1) :
>         -
>         instance->upper;
>         -                               else
>         -                                       cur_state =
>         -                                               cur_state >
>         instance->lower ?
>         -                                               (cur_state -
>         1) :
>         -
>         instance->lower;
>         -
>         -                               cdev->ops->set_cur_state(cdev,
>         cur_state);
>         -                       }
>         +                       thermal_zone_trip_update(tz, count,
>         temp);
>                                 break;
>                         case THERMAL_TRIP_PASSIVE:
>                                 if (temp >= trip_temp || tz->passive)
>         --
>         1.7.9.5
>         
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Kachhap July 24, 2012, 7:57 a.m. UTC | #4
On 19 July 2012 12:01, Zhang Rui <rui.zhang@intel.com> wrote:
>
> This function is used to update the cooling state of
> all the cooling devices that are binded to an active trip point.
>
> This will be used for passive cooling as well, in the future patches.
> as both active and passive cooling can share the same algorithm,
> which is
>
> 1. if the temperature is higher than a trip point,
>    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
>       state for this trip point
>    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
>       state for this trip point
>
> 2. if the temperature is lower than a trip point, use lower
>    cooling state for this trip point.

Hi Rui,

Your patches looks useful.
For my platform I need to have get_trend called even in the case when
current temp is less than the trip point and then use
THERMAL_TREND_DROPPING to actually lower the cooling state.

Thanks,
Amit
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/thermal.c        |    7 +++-
>  drivers/thermal/thermal_sys.c |   91
> +++++++++++++++++++++++++++++------------
>  2 files changed, 71 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 73e335f..14c4879 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -715,7 +715,12 @@ static int thermal_get_trend(struct
> thermal_zone_device *thermal,
>         if (thermal_get_trip_type(thermal, trip, &type))
>                 return -EINVAL;
>
> -       /* Only PASSIVE trip points need TREND */
> +       if (type == THERMAL_TRIP_ACTIVE) {
> +               /* aggressive active cooling */
> +               *trend = THERMAL_TREND_RAISING;
> +               return 0;
> +       }
> +
>         if (type != THERMAL_TRIP_PASSIVE)
>                 return -EINVAL;
>
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 59af3b8..011faba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1076,6 +1076,70 @@ void thermal_cooling_device_unregister(struct
>  }
>  EXPORT_SYMBOL(thermal_cooling_device_unregister);
>
> +/*
> + * Cooling algorithm for active trip points
> + *
> + * 1. if the temperature is higher than a trip point,
> + *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> + *       state for this trip point
> + *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> + *       state for this trip point
> + *
> + * 2. if the temperature is lower than a trip point, use lower
> + *    cooling state for this trip point
> + *
> + * Note that this behaves the same as the previous passive cooling
> + * algorithm.
> + */
> +
> +static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> +                                    int trip, long temp)
> +{
> +       struct thermal_cooling_device_instance *instance;
> +       struct thermal_cooling_device *cdev = NULL;
> +       unsigned long cur_state, max_state;
> +       long trip_temp;
> +       enum thermal_trend trend;
> +
> +       tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +
> +       if (temp >= trip_temp) {
> +               thermal_get_trend(tz, trip, &trend);
> +
> +               list_for_each_entry(instance, &tz->cooling_devices, node)
> {
> +                       if (instance->trip != trip)
> +                               continue;
> +
> +                       cdev = instance->cdev;
> +
> +                       cdev->ops->get_cur_state(cdev, &cur_state);
> +                       cdev->ops->get_max_state(cdev, &max_state);
> +
> +                       if (trend == THERMAL_TREND_RAISING) {
> +                               cur_state = cur_state < instance->upper ?
> +                                           (cur_state + 1) :
> instance->upper;
> +                       } else if (trend == THERMAL_TREND_DROPPING) {
> +                               cur_state = cur_state > instance->lower ?
> +                                   (cur_state - 1) : instance->lower;
> +                       }
> +                       cdev->ops->set_cur_state(cdev, cur_state);
> +               }
> +       } else {        /* below trip */
> +               list_for_each_entry(instance, &tz->cooling_devices, node)
> {
> +                       if (instance->trip != trip)
> +                               continue;
> +
> +                       cdev = instance->cdev;
> +                       cdev->ops->get_cur_state(cdev, &cur_state);
> +
> +                       cur_state = cur_state > instance->lower ?
> +                                   (cur_state - 1) : instance->lower;
> +                       cdev->ops->set_cur_state(cdev, cur_state);
> +               }
> +       }
> +
> +       return;
> +}
>  /**
>   * thermal_zone_device_update - force an update of a thermal zone's state
>   * @ttz:       the thermal zone to update
> @@ -1086,9 +1150,6 @@ void thermal_zone_device_update(struct
> thermal_zone_device *tz)
>         int count, ret = 0;
>         long temp, trip_temp;
>         enum thermal_trip_type trip_type;
> -       struct thermal_cooling_device_instance *instance;
> -       struct thermal_cooling_device *cdev;
> -       unsigned long cur_state, max_state;
>
>         mutex_lock(&tz->lock);
>
> @@ -1124,29 +1185,7 @@ void thermal_zone_device_update(struct
> thermal_zone_device *tz)
>                                         tz->ops->notify(tz, count,
> trip_type);
>                         break;
>                 case THERMAL_TRIP_ACTIVE:
> -                       list_for_each_entry(instance,
> &tz->cooling_devices,
> -                                           node) {
> -                               if (instance->trip != count)
> -                                       continue;
> -
> -                               cdev = instance->cdev;
> -
> -                               cdev->ops->get_cur_state(cdev,
> &cur_state);
> -                               cdev->ops->get_max_state(cdev,
> &max_state);
> -
> -                               if (temp >= trip_temp)
> -                                       cur_state =
> -                                               cur_state <
> instance->upper ?
> -                                               (cur_state + 1) :
> -                                               instance->upper;
> -                               else
> -                                       cur_state =
> -                                               cur_state >
> instance->lower ?
> -                                               (cur_state - 1) :
> -                                               instance->lower;
> -
> -                               cdev->ops->set_cur_state(cdev, cur_state);
> -                       }
> +                       thermal_zone_trip_update(tz, count, temp);
>                         break;
>                 case THERMAL_TRIP_PASSIVE:
>                         if (temp >= trip_temp || tz->passive)
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Kachhap July 24, 2012, 8:06 a.m. UTC | #5
On 24 July 2012 12:41, Zhang Rui <rui.zhang@intel.com> wrote:
> On ?, 2012-07-24 at 12:27 +0530, Amit Kachhap wrote:
>>
>>
>> On 19 July 2012 12:01, Zhang Rui <rui.zhang@intel.com> wrote:
>>         This function is used to update the cooling state of
>>         all the cooling devices that are binded to an active trip
>>         point.
>>
>>         This will be used for passive cooling as well, in the future
>>         patches.
>>         as both active and passive cooling can share the same
>>         algorithm,
>>         which is
>>
>>         1. if the temperature is higher than a trip point,
>>            a. if the trend is THERMAL_TREND_RAISING, use higher
>>         cooling
>>               state for this trip point
>>            b. if the trend is THERMAL_TREND_DROPPING, use lower
>>         cooling
>>               state for this trip point
>>
>>         2. if the temperature is lower than a trip point, use lower
>>            cooling state for this trip point.
>>
>> Hi Rui,
>>
>> Your patches looks useful.
>> For my platform I need to have get_trend called even in the case when
>> current temp is less than the trip point and then use
>> THERMAL_TREND_DROPPING to actually lower the cooling state.
>>
> hmm, why?
> in the current cooling algorithm, when the temperature is lower than the
> trip point, the cooling state is decreased by 1 every time,
> unconditionally, isn't this what you want?

Basically the requirement is that I do not want to remove the cooling
device when the temp just reaches below the trip point because this
causes many throttling in this trip point so say I will remove cooling
device when the temp is < (trip - 5) which can be done inside the new
platform get_trend call.

Thanks,
Amit D
>
> thanks,
> rui
>
>> Thanks,
>> Amit
>>
>>
>>         Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>>         ---
>>          drivers/acpi/thermal.c        |    7 +++-
>>          drivers/thermal/thermal_sys.c |   91
>>         +++++++++++++++++++++++++++++------------
>>          2 files changed, 71 insertions(+), 27 deletions(-)
>>
>>         diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>>         index 73e335f..14c4879 100644
>>         --- a/drivers/acpi/thermal.c
>>         +++ b/drivers/acpi/thermal.c
>>         @@ -715,7 +715,12 @@ static int thermal_get_trend(struct
>>         thermal_zone_device *thermal,
>>                 if (thermal_get_trip_type(thermal, trip, &type))
>>                         return -EINVAL;
>>
>>         -       /* Only PASSIVE trip points need TREND */
>>         +       if (type == THERMAL_TRIP_ACTIVE) {
>>         +               /* aggressive active cooling */
>>         +               *trend = THERMAL_TREND_RAISING;
>>         +               return 0;
>>         +       }
>>         +
>>                 if (type != THERMAL_TRIP_PASSIVE)
>>                         return -EINVAL;
>>
>>         diff --git a/drivers/thermal/thermal_sys.c
>>         b/drivers/thermal/thermal_sys.c
>>         index 59af3b8..011faba 100644
>>         --- a/drivers/thermal/thermal_sys.c
>>         +++ b/drivers/thermal/thermal_sys.c
>>         @@ -1076,6 +1076,70 @@ void
>>         thermal_cooling_device_unregister(struct
>>          }
>>          EXPORT_SYMBOL(thermal_cooling_device_unregister);
>>
>>         +/*
>>         + * Cooling algorithm for active trip points
>>         + *
>>         + * 1. if the temperature is higher than a trip point,
>>         + *    a. if the trend is THERMAL_TREND_RAISING, use higher
>>         cooling
>>         + *       state for this trip point
>>         + *    b. if the trend is THERMAL_TREND_DROPPING, use lower
>>         cooling
>>         + *       state for this trip point
>>         + *
>>         + * 2. if the temperature is lower than a trip point, use
>>         lower
>>         + *    cooling state for this trip point
>>         + *
>>         + * Note that this behaves the same as the previous passive
>>         cooling
>>         + * algorithm.
>>         + */
>>         +
>>         +static void thermal_zone_trip_update(struct
>>         thermal_zone_device *tz,
>>         +                                    int trip, long temp)
>>         +{
>>         +       struct thermal_cooling_device_instance *instance;
>>         +       struct thermal_cooling_device *cdev = NULL;
>>         +       unsigned long cur_state, max_state;
>>         +       long trip_temp;
>>         +       enum thermal_trend trend;
>>         +
>>         +       tz->ops->get_trip_temp(tz, trip, &trip_temp);
>>         +
>>         +       if (temp >= trip_temp) {
>>         +               thermal_get_trend(tz, trip, &trend);
>>         +
>>         +               list_for_each_entry(instance,
>>         &tz->cooling_devices, node) {
>>         +                       if (instance->trip != trip)
>>         +                               continue;
>>         +
>>         +                       cdev = instance->cdev;
>>         +
>>         +                       cdev->ops->get_cur_state(cdev,
>>         &cur_state);
>>         +                       cdev->ops->get_max_state(cdev,
>>         &max_state);
>>         +
>>         +                       if (trend == THERMAL_TREND_RAISING) {
>>         +                               cur_state = cur_state <
>>         instance->upper ?
>>         +                                           (cur_state + 1) :
>>         instance->upper;
>>         +                       } else if (trend ==
>>         THERMAL_TREND_DROPPING) {
>>         +                               cur_state = cur_state >
>>         instance->lower ?
>>         +                                   (cur_state - 1) :
>>         instance->lower;
>>         +                       }
>>         +                       cdev->ops->set_cur_state(cdev,
>>         cur_state);
>>         +               }
>>         +       } else {        /* below trip */
>>         +               list_for_each_entry(instance,
>>         &tz->cooling_devices, node) {
>>         +                       if (instance->trip != trip)
>>         +                               continue;
>>         +
>>         +                       cdev = instance->cdev;
>>         +                       cdev->ops->get_cur_state(cdev,
>>         &cur_state);
>>         +
>>         +                       cur_state = cur_state >
>>         instance->lower ?
>>         +                                   (cur_state - 1) :
>>         instance->lower;
>>         +                       cdev->ops->set_cur_state(cdev,
>>         cur_state);
>>         +               }
>>         +       }
>>         +
>>         +       return;
>>         +}
>>          /**
>>           * thermal_zone_device_update - force an update of a thermal
>>         zone's state
>>           * @ttz:       the thermal zone to update
>>         @@ -1086,9 +1150,6 @@ void thermal_zone_device_update(struct
>>         thermal_zone_device *tz)
>>                 int count, ret = 0;
>>                 long temp, trip_temp;
>>                 enum thermal_trip_type trip_type;
>>         -       struct thermal_cooling_device_instance *instance;
>>         -       struct thermal_cooling_device *cdev;
>>         -       unsigned long cur_state, max_state;
>>
>>                 mutex_lock(&tz->lock);
>>
>>         @@ -1124,29 +1185,7 @@ void thermal_zone_device_update(struct
>>         thermal_zone_device *tz)
>>                                                 tz->ops->notify(tz,
>>         count, trip_type);
>>                                 break;
>>                         case THERMAL_TRIP_ACTIVE:
>>         -                       list_for_each_entry(instance,
>>         &tz->cooling_devices,
>>         -                                           node) {
>>         -                               if (instance->trip != count)
>>         -                                       continue;
>>         -
>>         -                               cdev = instance->cdev;
>>         -
>>         -                               cdev->ops->get_cur_state(cdev,
>>         &cur_state);
>>         -                               cdev->ops->get_max_state(cdev,
>>         &max_state);
>>         -
>>         -                               if (temp >= trip_temp)
>>         -                                       cur_state =
>>         -                                               cur_state <
>>         instance->upper ?
>>         -                                               (cur_state +
>>         1) :
>>         -
>>         instance->upper;
>>         -                               else
>>         -                                       cur_state =
>>         -                                               cur_state >
>>         instance->lower ?
>>         -                                               (cur_state -
>>         1) :
>>         -
>>         instance->lower;
>>         -
>>         -                               cdev->ops->set_cur_state(cdev,
>>         cur_state);
>>         -                       }
>>         +                       thermal_zone_trip_update(tz, count,
>>         temp);
>>                                 break;
>>                         case THERMAL_TRIP_PASSIVE:
>>                                 if (temp >= trip_temp || tz->passive)
>>         --
>>         1.7.9.5
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki July 24, 2012, 9:27 a.m. UTC | #6
On Tuesday, July 24, 2012, Zhang Rui wrote:
> On ?, 2012-07-19 at 23:19 +0200, Rafael J. Wysocki wrote:
> > On Thursday, July 19, 2012, Zhang Rui wrote:
> > > This function is used to update the cooling state of
> > > all the cooling devices that are binded to an active trip point.
> > 
> > s/binded/bound/
> > 
> got it.
> 
> > > This will be used for passive cooling as well, in the future patches.
> > > as both active and passive cooling can share the same algorithm,
> > > which is
> > > 
> > > 1. if the temperature is higher than a trip point,
> > >    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> > >       state for this trip point
> > >    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> > >       state for this trip point
> > > 
> > > 2. if the temperature is lower than a trip point, use lower
> > >    cooling state for this trip point.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/thermal.c        |    7 +++-
> > >  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
> > >  2 files changed, 71 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > index 73e335f..14c4879 100644
> > > --- a/drivers/acpi/thermal.c
> > > +++ b/drivers/acpi/thermal.c
> > > @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
> > >  	if (thermal_get_trip_type(thermal, trip, &type))
> > >  		return -EINVAL;
> > >  
> > > -	/* Only PASSIVE trip points need TREND */
> > > +	if (type == THERMAL_TRIP_ACTIVE) {
> > > +		/* aggressive active cooling */
> > > +		*trend = THERMAL_TREND_RAISING;
> > > +		return 0;
> > 
> > Please move that into thermal_zone_trip_update() directly, unless you
> > need it elsewhere.
> > 
> IMO, I can say that ACPI thermal wants aggressive active cooling, as it
> will never spin down the fan when the temperature is higher than the
> trip point.

I meant the code organization, not the functionality. :-)

Since thermal_get_trend() is static, it is not used outside of this file,
so you can move the "if (type == THERMAL_TRIP_ACTIVE)" conditional from it
directly into the caller (unless there are more callers, which I'm not sure
about without reading the whole code again).

That would make the code cleaner, because right now the condition is
hidden in thermal_get_trend() and it may not be clear to the reader of
thermal_zone_trip_update() that it is actually checked.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui July 25, 2012, 1:38 a.m. UTC | #7
On ?, 2012-07-24 at 11:27 +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 24, 2012, Zhang Rui wrote:
> > On ?, 2012-07-19 at 23:19 +0200, Rafael J. Wysocki wrote:
> > > On Thursday, July 19, 2012, Zhang Rui wrote:
> > > > This function is used to update the cooling state of
> > > > all the cooling devices that are binded to an active trip point.
> > > 
> > > s/binded/bound/
> > > 
> > got it.
> > 
> > > > This will be used for passive cooling as well, in the future patches.
> > > > as both active and passive cooling can share the same algorithm,
> > > > which is
> > > > 
> > > > 1. if the temperature is higher than a trip point,
> > > >    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> > > >       state for this trip point
> > > >    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> > > >       state for this trip point
> > > > 
> > > > 2. if the temperature is lower than a trip point, use lower
> > > >    cooling state for this trip point.
> > > > 
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > ---
> > > >  drivers/acpi/thermal.c        |    7 +++-
> > > >  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
> > > >  2 files changed, 71 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > > index 73e335f..14c4879 100644
> > > > --- a/drivers/acpi/thermal.c
> > > > +++ b/drivers/acpi/thermal.c
> > > > @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
> > > >  	if (thermal_get_trip_type(thermal, trip, &type))
> > > >  		return -EINVAL;
> > > >  
> > > > -	/* Only PASSIVE trip points need TREND */
> > > > +	if (type == THERMAL_TRIP_ACTIVE) {
> > > > +		/* aggressive active cooling */
> > > > +		*trend = THERMAL_TREND_RAISING;
> > > > +		return 0;
> > > 
> > > Please move that into thermal_zone_trip_update() directly, unless you
> > > need it elsewhere.
> > > 
> > IMO, I can say that ACPI thermal wants aggressive active cooling, as it
> > will never spin down the fan when the temperature is higher than the
> > trip point.
> 
> I meant the code organization, not the functionality. :-)
> 
sure.

> Since thermal_get_trend() is static, it is not used outside of this file,
> so you can move the "if (type == THERMAL_TRIP_ACTIVE)" conditional from it
> directly into the caller (unless there are more callers, which I'm not sure
> about without reading the whole code again).
> 
sorry I still do not get it.
the generic thermal layer is the caller of this callback.
so you mean in thermal_zone_trip_update(),
when updating an active trip point, we set the trend to
THERMAL_TREND_RAISING explicitly?
in this way, all the platform thermal drivers will spin up the fan step
by step when the temperature is higher than the trip point, even if the
temperature is dropping. (Note that, this is not a problem for ACPI, as
there is only two state for ACPI FAN, on/off, it must be always on when
the temperature is higher than the active trip point. but other platform
may want to spin down the fan a little even if the system is still warm)

> That would make the code cleaner, because right now the condition is
> hidden in thermal_get_trend() and it may not be clear to the reader of
> thermal_zone_trip_update() that it is actually checked.
> 
why?
Readers should trust the value returned by
drivers/thermal/thermal_sys.c:thermal_get_trend().
And platform thermal drivers should provide the proper trend for active
trip points as well. If they don't, we will use the default trend by
comparing the current temperature and last temperature.

thanks,
rui

> Thanks,
> Rafael


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki July 25, 2012, 11:07 a.m. UTC | #8
On Wednesday, July 25, 2012, Zhang Rui wrote:
> On ?, 2012-07-24 at 11:27 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 24, 2012, Zhang Rui wrote:
> > > On ?, 2012-07-19 at 23:19 +0200, Rafael J. Wysocki wrote:
> > > > On Thursday, July 19, 2012, Zhang Rui wrote:
> > > > > This function is used to update the cooling state of
> > > > > all the cooling devices that are binded to an active trip point.
> > > > 
> > > > s/binded/bound/
> > > > 
> > > got it.
> > > 
> > > > > This will be used for passive cooling as well, in the future patches.
> > > > > as both active and passive cooling can share the same algorithm,
> > > > > which is
> > > > > 
> > > > > 1. if the temperature is higher than a trip point,
> > > > >    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> > > > >       state for this trip point
> > > > >    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> > > > >       state for this trip point
> > > > > 
> > > > > 2. if the temperature is lower than a trip point, use lower
> > > > >    cooling state for this trip point.
> > > > > 
> > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > > ---
> > > > >  drivers/acpi/thermal.c        |    7 +++-
> > > > >  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
> > > > >  2 files changed, 71 insertions(+), 27 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > > > index 73e335f..14c4879 100644
> > > > > --- a/drivers/acpi/thermal.c
> > > > > +++ b/drivers/acpi/thermal.c
> > > > > @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
> > > > >  	if (thermal_get_trip_type(thermal, trip, &type))
> > > > >  		return -EINVAL;
> > > > >  
> > > > > -	/* Only PASSIVE trip points need TREND */
> > > > > +	if (type == THERMAL_TRIP_ACTIVE) {
> > > > > +		/* aggressive active cooling */
> > > > > +		*trend = THERMAL_TREND_RAISING;
> > > > > +		return 0;
> > > > 
> > > > Please move that into thermal_zone_trip_update() directly, unless you
> > > > need it elsewhere.
> > > > 
> > > IMO, I can say that ACPI thermal wants aggressive active cooling, as it
> > > will never spin down the fan when the temperature is higher than the
> > > trip point.
> > 
> > I meant the code organization, not the functionality. :-)
> > 
> sure.
> 
> > Since thermal_get_trend() is static, it is not used outside of this file,
> > so you can move the "if (type == THERMAL_TRIP_ACTIVE)" conditional from it
> > directly into the caller (unless there are more callers, which I'm not sure
> > about without reading the whole code again).
> > 
> sorry I still do not get it.
> the generic thermal layer is the caller of this callback.

I'm not talking about the callback, but of the static function with the
same name you've introduced into drivers/thermal/thermal_sys.c in the
previous patch. :-)

The only caller of this function seems to be thermal_zone_device_passive()
and my point is that it would be better to simply put the code from that
function into thermal_zone_device_passive() directly, unless there are
more callers added by the subsequent patches, which I'm not sure about.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui July 26, 2012, 12:49 a.m. UTC | #9
On ?, 2012-07-25 at 13:07 +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 25, 2012, Zhang Rui wrote:
> > On ?, 2012-07-24 at 11:27 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 24, 2012, Zhang Rui wrote:
> > > > On ?, 2012-07-19 at 23:19 +0200, Rafael J. Wysocki wrote:
> > > > > On Thursday, July 19, 2012, Zhang Rui wrote:
> > > > > > This function is used to update the cooling state of
> > > > > > all the cooling devices that are binded to an active trip point.
> > > > > 
> > > > > s/binded/bound/
> > > > > 
> > > > got it.
> > > > 
> > > > > > This will be used for passive cooling as well, in the future patches.
> > > > > > as both active and passive cooling can share the same algorithm,
> > > > > > which is
> > > > > > 
> > > > > > 1. if the temperature is higher than a trip point,
> > > > > >    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> > > > > >       state for this trip point
> > > > > >    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> > > > > >       state for this trip point
> > > > > > 
> > > > > > 2. if the temperature is lower than a trip point, use lower
> > > > > >    cooling state for this trip point.
> > > > > > 
> > > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > > > ---
> > > > > >  drivers/acpi/thermal.c        |    7 +++-
> > > > > >  drivers/thermal/thermal_sys.c |   91 +++++++++++++++++++++++++++++------------
> > > > > >  2 files changed, 71 insertions(+), 27 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > > > > index 73e335f..14c4879 100644
> > > > > > --- a/drivers/acpi/thermal.c
> > > > > > +++ b/drivers/acpi/thermal.c
> > > > > > @@ -715,7 +715,12 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
> > > > > >  	if (thermal_get_trip_type(thermal, trip, &type))
> > > > > >  		return -EINVAL;
> > > > > >  
> > > > > > -	/* Only PASSIVE trip points need TREND */
> > > > > > +	if (type == THERMAL_TRIP_ACTIVE) {
> > > > > > +		/* aggressive active cooling */
> > > > > > +		*trend = THERMAL_TREND_RAISING;
> > > > > > +		return 0;
> > > > > 
> > > > > Please move that into thermal_zone_trip_update() directly, unless you
> > > > > need it elsewhere.
> > > > > 
> > > > IMO, I can say that ACPI thermal wants aggressive active cooling, as it
> > > > will never spin down the fan when the temperature is higher than the
> > > > trip point.
> > > 
> > > I meant the code organization, not the functionality. :-)
> > > 
> > sure.
> > 
> > > Since thermal_get_trend() is static, it is not used outside of this file,
> > > so you can move the "if (type == THERMAL_TRIP_ACTIVE)" conditional from it
> > > directly into the caller (unless there are more callers, which I'm not sure
> > > about without reading the whole code again).
> > > 
> > sorry I still do not get it.
> > the generic thermal layer is the caller of this callback.
> 
> I'm not talking about the callback, but of the static function with the
> same name you've introduced into drivers/thermal/thermal_sys.c in the
> previous patch. :-)
> 
> The only caller of this function seems to be thermal_zone_device_passive()
> and my point is that it would be better to simply put the code from that
> function into thermal_zone_device_passive() directly, unless there are
> more callers added by the subsequent patches, which I'm not sure about.

hah, I understand.
yes, I can move thermal_sys.c thermal_get_trend() in to
thermal_trip_update.
and there will be no such confusion (two functions share the same name)
any more. :)

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui July 26, 2012, 5:08 a.m. UTC | #10
On ?, 2012-07-24 at 13:36 +0530, Amit Kachhap wrote:
> On 24 July 2012 12:41, Zhang Rui <rui.zhang@intel.com> wrote:
> > On ?, 2012-07-24 at 12:27 +0530, Amit Kachhap wrote:
> >>
> >>
> >> On 19 July 2012 12:01, Zhang Rui <rui.zhang@intel.com> wrote:
> >>         This function is used to update the cooling state of
> >>         all the cooling devices that are binded to an active trip
> >>         point.
> >>
> >>         This will be used for passive cooling as well, in the future
> >>         patches.
> >>         as both active and passive cooling can share the same
> >>         algorithm,
> >>         which is
> >>
> >>         1. if the temperature is higher than a trip point,
> >>            a. if the trend is THERMAL_TREND_RAISING, use higher
> >>         cooling
> >>               state for this trip point
> >>            b. if the trend is THERMAL_TREND_DROPPING, use lower
> >>         cooling
> >>               state for this trip point
> >>
> >>         2. if the temperature is lower than a trip point, use lower
> >>            cooling state for this trip point.
> >>
> >> Hi Rui,
> >>
> >> Your patches looks useful.
> >> For my platform I need to have get_trend called even in the case when
> >> current temp is less than the trip point and then use
> >> THERMAL_TREND_DROPPING to actually lower the cooling state.
> >>
> > hmm, why?
> > in the current cooling algorithm, when the temperature is lower than the
> > trip point, the cooling state is decreased by 1 every time,
> > unconditionally, isn't this what you want?
> 
> Basically the requirement is that I do not want to remove the cooling
> device when the temp just reaches below the trip point because this
> causes many throttling in this trip point so say I will remove cooling
> device when the temp is < (trip - 5) which can be done inside the new
> platform get_trend call.
> 
As Durga is working on the thermal governor patches, I think that one
can handle this, right?

Currently, as I do not want to change the behavior of the current
drivers, ACPI passive cooling, etc, I did not use trend when the
temperature is lower  than the trip point.
But anyway, if really needed, I can generate an incremental patch.
what do you think?

thanks,
rui

> Thanks,
> Amit D
> >
> > thanks,
> > rui
> >
> >> Thanks,
> >> Amit
> >>
> >>
> >>         Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >>         ---
> >>          drivers/acpi/thermal.c        |    7 +++-
> >>          drivers/thermal/thermal_sys.c |   91
> >>         +++++++++++++++++++++++++++++------------
> >>          2 files changed, 71 insertions(+), 27 deletions(-)
> >>
> >>         diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> >>         index 73e335f..14c4879 100644
> >>         --- a/drivers/acpi/thermal.c
> >>         +++ b/drivers/acpi/thermal.c
> >>         @@ -715,7 +715,12 @@ static int thermal_get_trend(struct
> >>         thermal_zone_device *thermal,
> >>                 if (thermal_get_trip_type(thermal, trip, &type))
> >>                         return -EINVAL;
> >>
> >>         -       /* Only PASSIVE trip points need TREND */
> >>         +       if (type == THERMAL_TRIP_ACTIVE) {
> >>         +               /* aggressive active cooling */
> >>         +               *trend = THERMAL_TREND_RAISING;
> >>         +               return 0;
> >>         +       }
> >>         +
> >>                 if (type != THERMAL_TRIP_PASSIVE)
> >>                         return -EINVAL;
> >>
> >>         diff --git a/drivers/thermal/thermal_sys.c
> >>         b/drivers/thermal/thermal_sys.c
> >>         index 59af3b8..011faba 100644
> >>         --- a/drivers/thermal/thermal_sys.c
> >>         +++ b/drivers/thermal/thermal_sys.c
> >>         @@ -1076,6 +1076,70 @@ void
> >>         thermal_cooling_device_unregister(struct
> >>          }
> >>          EXPORT_SYMBOL(thermal_cooling_device_unregister);
> >>
> >>         +/*
> >>         + * Cooling algorithm for active trip points
> >>         + *
> >>         + * 1. if the temperature is higher than a trip point,
> >>         + *    a. if the trend is THERMAL_TREND_RAISING, use higher
> >>         cooling
> >>         + *       state for this trip point
> >>         + *    b. if the trend is THERMAL_TREND_DROPPING, use lower
> >>         cooling
> >>         + *       state for this trip point
> >>         + *
> >>         + * 2. if the temperature is lower than a trip point, use
> >>         lower
> >>         + *    cooling state for this trip point
> >>         + *
> >>         + * Note that this behaves the same as the previous passive
> >>         cooling
> >>         + * algorithm.
> >>         + */
> >>         +
> >>         +static void thermal_zone_trip_update(struct
> >>         thermal_zone_device *tz,
> >>         +                                    int trip, long temp)
> >>         +{
> >>         +       struct thermal_cooling_device_instance *instance;
> >>         +       struct thermal_cooling_device *cdev = NULL;
> >>         +       unsigned long cur_state, max_state;
> >>         +       long trip_temp;
> >>         +       enum thermal_trend trend;
> >>         +
> >>         +       tz->ops->get_trip_temp(tz, trip, &trip_temp);
> >>         +
> >>         +       if (temp >= trip_temp) {
> >>         +               thermal_get_trend(tz, trip, &trend);
> >>         +
> >>         +               list_for_each_entry(instance,
> >>         &tz->cooling_devices, node) {
> >>         +                       if (instance->trip != trip)
> >>         +                               continue;
> >>         +
> >>         +                       cdev = instance->cdev;
> >>         +
> >>         +                       cdev->ops->get_cur_state(cdev,
> >>         &cur_state);
> >>         +                       cdev->ops->get_max_state(cdev,
> >>         &max_state);
> >>         +
> >>         +                       if (trend == THERMAL_TREND_RAISING) {
> >>         +                               cur_state = cur_state <
> >>         instance->upper ?
> >>         +                                           (cur_state + 1) :
> >>         instance->upper;
> >>         +                       } else if (trend ==
> >>         THERMAL_TREND_DROPPING) {
> >>         +                               cur_state = cur_state >
> >>         instance->lower ?
> >>         +                                   (cur_state - 1) :
> >>         instance->lower;
> >>         +                       }
> >>         +                       cdev->ops->set_cur_state(cdev,
> >>         cur_state);
> >>         +               }
> >>         +       } else {        /* below trip */
> >>         +               list_for_each_entry(instance,
> >>         &tz->cooling_devices, node) {
> >>         +                       if (instance->trip != trip)
> >>         +                               continue;
> >>         +
> >>         +                       cdev = instance->cdev;
> >>         +                       cdev->ops->get_cur_state(cdev,
> >>         &cur_state);
> >>         +
> >>         +                       cur_state = cur_state >
> >>         instance->lower ?
> >>         +                                   (cur_state - 1) :
> >>         instance->lower;
> >>         +                       cdev->ops->set_cur_state(cdev,
> >>         cur_state);
> >>         +               }
> >>         +       }
> >>         +
> >>         +       return;
> >>         +}
> >>          /**
> >>           * thermal_zone_device_update - force an update of a thermal
> >>         zone's state
> >>           * @ttz:       the thermal zone to update
> >>         @@ -1086,9 +1150,6 @@ void thermal_zone_device_update(struct
> >>         thermal_zone_device *tz)
> >>                 int count, ret = 0;
> >>                 long temp, trip_temp;
> >>                 enum thermal_trip_type trip_type;
> >>         -       struct thermal_cooling_device_instance *instance;
> >>         -       struct thermal_cooling_device *cdev;
> >>         -       unsigned long cur_state, max_state;
> >>
> >>                 mutex_lock(&tz->lock);
> >>
> >>         @@ -1124,29 +1185,7 @@ void thermal_zone_device_update(struct
> >>         thermal_zone_device *tz)
> >>                                                 tz->ops->notify(tz,
> >>         count, trip_type);
> >>                                 break;
> >>                         case THERMAL_TRIP_ACTIVE:
> >>         -                       list_for_each_entry(instance,
> >>         &tz->cooling_devices,
> >>         -                                           node) {
> >>         -                               if (instance->trip != count)
> >>         -                                       continue;
> >>         -
> >>         -                               cdev = instance->cdev;
> >>         -
> >>         -                               cdev->ops->get_cur_state(cdev,
> >>         &cur_state);
> >>         -                               cdev->ops->get_max_state(cdev,
> >>         &max_state);
> >>         -
> >>         -                               if (temp >= trip_temp)
> >>         -                                       cur_state =
> >>         -                                               cur_state <
> >>         instance->upper ?
> >>         -                                               (cur_state +
> >>         1) :
> >>         -
> >>         instance->upper;
> >>         -                               else
> >>         -                                       cur_state =
> >>         -                                               cur_state >
> >>         instance->lower ?
> >>         -                                               (cur_state -
> >>         1) :
> >>         -
> >>         instance->lower;
> >>         -
> >>         -                               cdev->ops->set_cur_state(cdev,
> >>         cur_state);
> >>         -                       }
> >>         +                       thermal_zone_trip_update(tz, count,
> >>         temp);
> >>                                 break;
> >>                         case THERMAL_TRIP_PASSIVE:
> >>                                 if (temp >= trip_temp || tz->passive)
> >>         --
> >>         1.7.9.5
> >>
> >>
> >
> >


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
durgadoss.r@intel.com July 26, 2012, 6:01 a.m. UTC | #11
Hi Rui/Amit,


> -----Original Message-----

> From: Zhang, Rui

> Sent: Thursday, July 26, 2012 10:38 AM

> To: Amit Kachhap

> Cc: linux-acpi@vger.kernel.org; linux-pm@vger.kernel.org; Rafael J. Wysocki;

> Matthew Garrett; Len Brown; R, Durgadoss; Eduardo Valentin; Wei Ni

> Subject: Re: [PATCH 09/16] Thermal: Introduce thermal_zone_trip_update()

> 

> On ?, 2012-07-24 at 13:36 +0530, Amit Kachhap wrote:

> > On 24 July 2012 12:41, Zhang Rui <rui.zhang@intel.com> wrote:

> > > On ?, 2012-07-24 at 12:27 +0530, Amit Kachhap wrote:

> > >>

> > >>

> > >> On 19 July 2012 12:01, Zhang Rui <rui.zhang@intel.com> wrote:

> > >>         This function is used to update the cooling state of

> > >>         all the cooling devices that are binded to an active trip

> > >>         point.

> > >>

> > >>         This will be used for passive cooling as well, in the future

> > >>         patches.

> > >>         as both active and passive cooling can share the same

> > >>         algorithm,

> > >>         which is

> > >>

> > >>         1. if the temperature is higher than a trip point,

> > >>            a. if the trend is THERMAL_TREND_RAISING, use higher

> > >>         cooling

> > >>               state for this trip point

> > >>            b. if the trend is THERMAL_TREND_DROPPING, use lower

> > >>         cooling

> > >>               state for this trip point

> > >>

> > >>         2. if the temperature is lower than a trip point, use lower

> > >>            cooling state for this trip point.

> > >>

> > >> Hi Rui,

> > >>

> > >> Your patches looks useful.

> > >> For my platform I need to have get_trend called even in the case when

> > >> current temp is less than the trip point and then use

> > >> THERMAL_TREND_DROPPING to actually lower the cooling state.

> > >>

> > > hmm, why?

> > > in the current cooling algorithm, when the temperature is lower than the

> > > trip point, the cooling state is decreased by 1 every time,

> > > unconditionally, isn't this what you want?

> >

> > Basically the requirement is that I do not want to remove the cooling

> > device when the temp just reaches below the trip point because this

> > causes many throttling in this trip point so say I will remove cooling

> > device when the temp is < (trip - 5) which can be done inside the new

> > platform get_trend call.

> >

> As Durga is working on the thermal governor patches, I think that one

> can handle this, right?


I am fine with this..Since we anyway want all throttling logic to be moved to
the governor, I think it makes sense to put this there.

So, I will pick it up, when I submit the governor patches.
Amit, Please feel free to point out if I don’t pick this one in my patches.

Thanks,
Durga
> 

> Currently, as I do not want to change the behavior of the current

> drivers, ACPI passive cooling, etc, I did not use trend when the

> temperature is lower  than the trip point.

> But anyway, if really needed, I can generate an incremental patch.

> what do you think?

> 

> thanks,

> rui

> 

> > Thanks,

> > Amit D

> > >

> > > thanks,

> > > rui

> > >

> > >> Thanks,

> > >> Amit

> > >>

> > >>

> > >>         Signed-off-by: Zhang Rui <rui.zhang@intel.com>

> > >>         ---

> > >>          drivers/acpi/thermal.c        |    7 +++-

> > >>          drivers/thermal/thermal_sys.c |   91

> > >>         +++++++++++++++++++++++++++++------------

> > >>          2 files changed, 71 insertions(+), 27 deletions(-)

> > >>

> > >>         diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c

> > >>         index 73e335f..14c4879 100644

> > >>         --- a/drivers/acpi/thermal.c

> > >>         +++ b/drivers/acpi/thermal.c

> > >>         @@ -715,7 +715,12 @@ static int thermal_get_trend(struct

> > >>         thermal_zone_device *thermal,

> > >>                 if (thermal_get_trip_type(thermal, trip, &type))

> > >>                         return -EINVAL;

> > >>

> > >>         -       /* Only PASSIVE trip points need TREND */

> > >>         +       if (type == THERMAL_TRIP_ACTIVE) {

> > >>         +               /* aggressive active cooling */

> > >>         +               *trend = THERMAL_TREND_RAISING;

> > >>         +               return 0;

> > >>         +       }

> > >>         +

> > >>                 if (type != THERMAL_TRIP_PASSIVE)

> > >>                         return -EINVAL;

> > >>

> > >>         diff --git a/drivers/thermal/thermal_sys.c

> > >>         b/drivers/thermal/thermal_sys.c

> > >>         index 59af3b8..011faba 100644

> > >>         --- a/drivers/thermal/thermal_sys.c

> > >>         +++ b/drivers/thermal/thermal_sys.c

> > >>         @@ -1076,6 +1076,70 @@ void

> > >>         thermal_cooling_device_unregister(struct

> > >>          }

> > >>          EXPORT_SYMBOL(thermal_cooling_device_unregister);

> > >>

> > >>         +/*

> > >>         + * Cooling algorithm for active trip points

> > >>         + *

> > >>         + * 1. if the temperature is higher than a trip point,

> > >>         + *    a. if the trend is THERMAL_TREND_RAISING, use higher

> > >>         cooling

> > >>         + *       state for this trip point

> > >>         + *    b. if the trend is THERMAL_TREND_DROPPING, use lower

> > >>         cooling

> > >>         + *       state for this trip point

> > >>         + *

> > >>         + * 2. if the temperature is lower than a trip point, use

> > >>         lower

> > >>         + *    cooling state for this trip point

> > >>         + *

> > >>         + * Note that this behaves the same as the previous passive

> > >>         cooling

> > >>         + * algorithm.

> > >>         + */

> > >>         +

> > >>         +static void thermal_zone_trip_update(struct

> > >>         thermal_zone_device *tz,

> > >>         +                                    int trip, long temp)

> > >>         +{

> > >>         +       struct thermal_cooling_device_instance *instance;

> > >>         +       struct thermal_cooling_device *cdev = NULL;

> > >>         +       unsigned long cur_state, max_state;

> > >>         +       long trip_temp;

> > >>         +       enum thermal_trend trend;

> > >>         +

> > >>         +       tz->ops->get_trip_temp(tz, trip, &trip_temp);

> > >>         +

> > >>         +       if (temp >= trip_temp) {

> > >>         +               thermal_get_trend(tz, trip, &trend);

> > >>         +

> > >>         +               list_for_each_entry(instance,

> > >>         &tz->cooling_devices, node) {

> > >>         +                       if (instance->trip != trip)

> > >>         +                               continue;

> > >>         +

> > >>         +                       cdev = instance->cdev;

> > >>         +

> > >>         +                       cdev->ops->get_cur_state(cdev,

> > >>         &cur_state);

> > >>         +                       cdev->ops->get_max_state(cdev,

> > >>         &max_state);

> > >>         +

> > >>         +                       if (trend == THERMAL_TREND_RAISING) {

> > >>         +                               cur_state = cur_state <

> > >>         instance->upper ?

> > >>         +                                           (cur_state + 1) :

> > >>         instance->upper;

> > >>         +                       } else if (trend ==

> > >>         THERMAL_TREND_DROPPING) {

> > >>         +                               cur_state = cur_state >

> > >>         instance->lower ?

> > >>         +                                   (cur_state - 1) :

> > >>         instance->lower;

> > >>         +                       }

> > >>         +                       cdev->ops->set_cur_state(cdev,

> > >>         cur_state);

> > >>         +               }

> > >>         +       } else {        /* below trip */

> > >>         +               list_for_each_entry(instance,

> > >>         &tz->cooling_devices, node) {

> > >>         +                       if (instance->trip != trip)

> > >>         +                               continue;

> > >>         +

> > >>         +                       cdev = instance->cdev;

> > >>         +                       cdev->ops->get_cur_state(cdev,

> > >>         &cur_state);

> > >>         +

> > >>         +                       cur_state = cur_state >

> > >>         instance->lower ?

> > >>         +                                   (cur_state - 1) :

> > >>         instance->lower;

> > >>         +                       cdev->ops->set_cur_state(cdev,

> > >>         cur_state);

> > >>         +               }

> > >>         +       }

> > >>         +

> > >>         +       return;

> > >>         +}

> > >>          /**

> > >>           * thermal_zone_device_update - force an update of a thermal

> > >>         zone's state

> > >>           * @ttz:       the thermal zone to update

> > >>         @@ -1086,9 +1150,6 @@ void thermal_zone_device_update(struct

> > >>         thermal_zone_device *tz)

> > >>                 int count, ret = 0;

> > >>                 long temp, trip_temp;

> > >>                 enum thermal_trip_type trip_type;

> > >>         -       struct thermal_cooling_device_instance *instance;

> > >>         -       struct thermal_cooling_device *cdev;

> > >>         -       unsigned long cur_state, max_state;

> > >>

> > >>                 mutex_lock(&tz->lock);

> > >>

> > >>         @@ -1124,29 +1185,7 @@ void thermal_zone_device_update(struct

> > >>         thermal_zone_device *tz)

> > >>                                                 tz->ops->notify(tz,

> > >>         count, trip_type);

> > >>                                 break;

> > >>                         case THERMAL_TRIP_ACTIVE:

> > >>         -                       list_for_each_entry(instance,

> > >>         &tz->cooling_devices,

> > >>         -                                           node) {

> > >>         -                               if (instance->trip != count)

> > >>         -                                       continue;

> > >>         -

> > >>         -                               cdev = instance->cdev;

> > >>         -

> > >>         -                               cdev->ops->get_cur_state(cdev,

> > >>         &cur_state);

> > >>         -                               cdev->ops->get_max_state(cdev,

> > >>         &max_state);

> > >>         -

> > >>         -                               if (temp >= trip_temp)

> > >>         -                                       cur_state =

> > >>         -                                               cur_state <

> > >>         instance->upper ?

> > >>         -                                               (cur_state +

> > >>         1) :

> > >>         -

> > >>         instance->upper;

> > >>         -                               else

> > >>         -                                       cur_state =

> > >>         -                                               cur_state >

> > >>         instance->lower ?

> > >>         -                                               (cur_state -

> > >>         1) :

> > >>         -

> > >>         instance->lower;

> > >>         -

> > >>         -                               cdev->ops->set_cur_state(cdev,

> > >>         cur_state);

> > >>         -                       }

> > >>         +                       thermal_zone_trip_update(tz, count,

> > >>         temp);

> > >>                                 break;

> > >>                         case THERMAL_TRIP_PASSIVE:

> > >>                                 if (temp >= trip_temp || tz->passive)

> > >>         --

> > >>         1.7.9.5

> > >>

> > >>

> > >

> > >

>
diff mbox

Patch

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 73e335f..14c4879 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -715,7 +715,12 @@  static int thermal_get_trend(struct thermal_zone_device *thermal,
 	if (thermal_get_trip_type(thermal, trip, &type))
 		return -EINVAL;
 
-	/* Only PASSIVE trip points need TREND */
+	if (type == THERMAL_TRIP_ACTIVE) {
+		/* aggressive active cooling */
+		*trend = THERMAL_TREND_RAISING;
+		return 0;
+	}
+
 	if (type != THERMAL_TRIP_PASSIVE)
 		return -EINVAL;
 
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 59af3b8..011faba 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1076,6 +1076,70 @@  void thermal_cooling_device_unregister(struct
 }
 EXPORT_SYMBOL(thermal_cooling_device_unregister);
 
+/*
+ * Cooling algorithm for active trip points
+ *
+ * 1. if the temperature is higher than a trip point,
+ *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
+ *       state for this trip point
+ *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
+ *       state for this trip point
+ *
+ * 2. if the temperature is lower than a trip point, use lower
+ *    cooling state for this trip point
+ *
+ * Note that this behaves the same as the previous passive cooling
+ * algorithm.
+ */
+
+static void thermal_zone_trip_update(struct thermal_zone_device *tz,
+				     int trip, long temp)
+{
+	struct thermal_cooling_device_instance *instance;
+	struct thermal_cooling_device *cdev = NULL;
+	unsigned long cur_state, max_state;
+	long trip_temp;
+	enum thermal_trend trend;
+
+	tz->ops->get_trip_temp(tz, trip, &trip_temp);
+
+	if (temp >= trip_temp) {
+		thermal_get_trend(tz, trip, &trend);
+
+		list_for_each_entry(instance, &tz->cooling_devices, node) {
+			if (instance->trip != trip)
+				continue;
+
+			cdev = instance->cdev;
+
+			cdev->ops->get_cur_state(cdev, &cur_state);
+			cdev->ops->get_max_state(cdev, &max_state);
+
+			if (trend == THERMAL_TREND_RAISING) {
+				cur_state = cur_state < instance->upper ?
+					    (cur_state + 1) : instance->upper;
+			} else if (trend == THERMAL_TREND_DROPPING) {
+				cur_state = cur_state > instance->lower ?
+				    (cur_state - 1) : instance->lower;
+			}
+			cdev->ops->set_cur_state(cdev, cur_state);
+		}
+	} else {	/* below trip */
+		list_for_each_entry(instance, &tz->cooling_devices, node) {
+			if (instance->trip != trip)
+				continue;
+
+			cdev = instance->cdev;
+			cdev->ops->get_cur_state(cdev, &cur_state);
+
+			cur_state = cur_state > instance->lower ?
+				    (cur_state - 1) : instance->lower;
+			cdev->ops->set_cur_state(cdev, cur_state);
+		}
+	}
+
+	return;
+}
 /**
  * thermal_zone_device_update - force an update of a thermal zone's state
  * @ttz:	the thermal zone to update
@@ -1086,9 +1150,6 @@  void thermal_zone_device_update(struct thermal_zone_device *tz)
 	int count, ret = 0;
 	long temp, trip_temp;
 	enum thermal_trip_type trip_type;
-	struct thermal_cooling_device_instance *instance;
-	struct thermal_cooling_device *cdev;
-	unsigned long cur_state, max_state;
 
 	mutex_lock(&tz->lock);
 
@@ -1124,29 +1185,7 @@  void thermal_zone_device_update(struct thermal_zone_device *tz)
 					tz->ops->notify(tz, count, trip_type);
 			break;
 		case THERMAL_TRIP_ACTIVE:
-			list_for_each_entry(instance, &tz->cooling_devices,
-					    node) {
-				if (instance->trip != count)
-					continue;
-
-				cdev = instance->cdev;
-
-				cdev->ops->get_cur_state(cdev, &cur_state);
-				cdev->ops->get_max_state(cdev, &max_state);
-
-				if (temp >= trip_temp)
-					cur_state =
-						cur_state < instance->upper ?
-						(cur_state + 1) :
-						instance->upper;
-				else
-					cur_state =
-						cur_state > instance->lower ?
-						(cur_state - 1) :
-						instance->lower;
-
-				cdev->ops->set_cur_state(cdev, cur_state);
-			}
+			thermal_zone_trip_update(tz, count, temp);
 			break;
 		case THERMAL_TRIP_PASSIVE:
 			if (temp >= trip_temp || tz->passive)