Message-ID: <744357E9AAD1214791ACBA4B0B90926301192A50@SHSMSX101.ccr.corp.intel.com>
Date:	Thu, 17 Oct 2013 15:09:05 +0000
From:	"Zhang, Rui" <rui.zhang@...el.com>
To:	Lukasz Majewski <l.majewski@...sung.com>
CC:	Viresh Kumar <viresh.kumar@...aro.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Eduardo Valentin <eduardo.valentin@...com>,
	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Jonghwa Lee <jonghwa3.lee@...sung.com>,
	"Lukasz Majewski" <l.majewski@...ess.pl>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Myungjoo Ham <myungjoo.ham@...sung.com>,
	"R, Durgadoss" <durgadoss.r@...el.com>
Subject: RE: [PATCH v9 3/7] thermal:boost: Automatic enable/disable of BOOST
 feature



> -----Original Message-----
> From: Lukasz Majewski [mailto:l.majewski@...sung.com]
> Sent: Tuesday, October 15, 2013 11:43 PM
> To: Zhang, Rui
> Cc: Viresh Kumar; Rafael J. Wysocki; Eduardo Valentin;
> cpufreq@...r.kernel.org; Linux PM list; Jonghwa Lee; Lukasz Majewski;
> linux-kernel; Bartlomiej Zolnierkiewicz; Myungjoo Ham; R, Durgadoss
> Subject: Re: [PATCH v9 3/7] thermal:boost: Automatic enable/disable of
> BOOST feature
> Importance: High
> 
> Hi Zhang,
> 
> > Hi, Lukasz,
> >
> > thanks for the patch, sorry that I didn't look into this one earlier.
> 
> Yes, I would _really_ appreciate _earlier_ feedback from thermal
> maintainers :-)
> 
> >
> > On Mon, 2013-10-14 at 14:17 +0200, Lukasz Majewski wrote:
> > > This patch provides auto disable/enable operation for boost. When
> > > any defined trip point is passed, the boost is disabled.
> >
> > Do you mean boost is disabled if the system is in a overheating state?
> 
> In short - Yes.
> 
> 
> To be more precise - the thermal here is a "safe" valve.
> 
> Its role is to provide hysteresis similar to the one available for
> Intel processors.
> 
> Intel does it in HW. Here I'm trying to do the same with SW for ARM.
> 
> >
> > > In that moment thermal monitor workqueue is woken up and it
> monitors
> > > if the device temperature drops below 75% of the smallest trip
> > > point.
> >
> > Just FYI, the smallest trip point does not equal the trip point with
> > lowest temperature value.
> 
> Thermal processors to which I've looked (exynos 4/5, ste-snowball) had
> trip points defined monotonically with smallest value defined first.
> 
> This was the rationale for choosing thermal trip point 0.
> 
But this is not a hard rule for all thermal drivers, thus you can't make this assumption.
 
> 
> >
> > Say, here is a platform with an active trip point at 40C, and an
> > critical trip point at 100C, you want to enable boost only if the
> > temperature is under 30C, right?
> 
> In short: no (please read below explanation).
> 
> 
> The boost rough idea:
> 1. I enable boost from cpufreq (no matter what is the state of thermal)
> 2. If temperature is too high, then thermal interrupt would trigger and
> disable boost 3. If device cools down - I enable the boost again
> 
> 
> >
> > > When device cools down, the boost is enabled again.
> > >
> > > Signed-off-by: Lukasz Majewski <l.majewski@...sung.com>
> > > Signed-off-by: Myungjoo Ham <myungjoo.ham@...sung.com>
> > >
> > > ---
> > > Changes for v9:
> > > - None
> > >
> > > Changes for v8:
> > > - Move cpufreq_boost_* stub functions definition (needed when
> > > cpufreq is not compiled in) to cpufreq.h at cpufreq core support
> > > commit
> > >
> > > Changes for v7:
> > > - None
> > >
> > > Changes for v6:
> > > - Disable boost only when supported and enabled
> > > - Protect boost related thermal_zone_device struct fields with
> mutex
> > > - Evaluate temperature trend during boost enable decision
> > > - Create separate methods to handle boost enable/disable
> > >   (thermal_boost_{enable|disable}) operations
> > > - Boost is disabled at any trip point passage (not only the non
> > > critical one)
> > > - Add stub definitions for cpufreq boost functions used when
> > >   CONFIG_CPU_FREQ is NOT defined.
> > >
> > > Changes for v5:
> > > - Move boost disable code from cpu_cooling.c to thermal_core.c
> > >   (to handle_non_critical_trips)
> > > - Extent struct thermal_zone_device by adding overheated bool flag
> > > - Implement auto enable of boost after device cools down
> > > - Introduce boost_polling flag, which indicates if thermal uses
> it's
> > > predefined pool delay or has woken up thermal workqueue only to
> wait
> > > until device cools down.
> > >
> > > Changes for v4:
> > > - New patch
> > >
> > >  drivers/thermal/thermal_core.c |   55
> > > ++++++++++++++++++++++++++++++++++++++++
> > > include/linux/thermal.h        |    2 ++ 2 files changed, 57
> > > insertions(+)
> > >
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c index 4962a6a..a167ab9 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -34,6 +34,7 @@
> > >  #include <linux/thermal.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/string.h>
> > > +#include <linux/cpufreq.h>
> >
> > Actually, I do not like to see this as thermal_core.c.
> > Because it is the platform thermal driver that owns the thermal
> > policy, e.g. it tells the thermal core to take what action at what
> > temperature.
> > And this cpufreq boost support should be part of the thermal policy.
> 
> 
> Boost is defined as policy independent at cpufreq.


> So I believe that
> it shall be also thermal policy independent.

Boost mode support itself is policy independent, but when to use it is kind of a policy, right?
Say, if you introduce boost support in cpufreq cooling code, either as a cooling device or as a special cooling state, it is thermal policy independent, but when to use this cooling device/state is surely part of thermal policy.

> In the end thermal shall
> help cpufreq to not burn the device.
> 
> 
> >
> > For example, here is a platform that supports boost. And it has a
> > passive trip point at 40C, which means the platform driver wants to
> > reduce the processor frequency when the temperature at 40C.
> > And what you're trying to add in this patch is to turn on boost mode
> > when the temperature is under 30C, right?
> 
> In short: yes.
> 
> I want to add code which would disable boost when detected temperature
> is more than 40C.
> 
> First, boost must be enabled at cpufreq. Only then it can be disabled
> (if temp > 40C) at thermal.
> 
> During boost disablement I also setup the thermal zone for
> polling (if we already poll it - no settings are changed).
> 
> The boost is re-enabled only when temperature drops to 30C AND the
> tz->overheated is set (which means that we are at overheated state
> caused by boost).
> 
> 
> > If yes, then I'd prefer to
> > 1. introduce a separate cpu cooling device that just has two cooling
> > state, 0 means boost mode enabled, and 1 means boost mode disabled.
> > 2. For any platform thermal driver that wants this support, introduce
> > a new trip point (30C) to the platform thermal driver,
> > and bind the
> > cpufreq boost cooling device to this trip point.
> 
> >
> > And IMO, Step 1 can be an enhancement of cpufreq cooling feature. You
> > just need to introduce two new APIs for registering/unregistering an
> > cpu boost cooling device, without changing the current cpufreq
> > cooling code.
> >
> > Further more, cpufreq_boost_trigger_state(1) just make it possible to
> > enter boost mode, it does not mean the cpu will be put into boost
> mode
> > immediately, right?
> 
> Yes, correct.
> 
> > can we make it transparent to thermal core, say,
> > always enable it when the cpu is in cooling state 0 (p0)?
> 
> Thanks for presenting possible solution.
>
No problem. 

Thanks,
rui
> I will investigate it for boost.
> 
Thanks,
rui
> >
> > thanks,
> > rui
> > >  #include <net/netlink.h>
> > >  #include <net/genetlink.h>
> > >
> > > @@ -366,9 +367,59 @@ static void handle_critical_trips(struct
> > > thermal_zone_device *tz, }
> > >  }
> > >
> > > +static int thermal_boost_enable(struct thermal_zone_device *tz)
> > > +{
> > > +	enum thermal_trend trend = get_tz_trend(tz, 0);
> > > +	long trip_temp;
> > > +
> > > +	if (!tz->ops->get_trip_temp || !tz->overheated)
> > > +		return -EPERM;
> > > +	if (trend == THERMAL_TREND_RAISING || trend ==
> > > THERMAL_TREND_RAISE_FULL)
> > > +		return -EBUSY;
> > > +
> > > +	tz->ops->get_trip_temp(tz, 0, &trip_temp);
> > > +	/*
> > > +	 * Enable boost again only when current temperature is less
> > > +	 * than 75% of trip_temp[0]
> > > +	 */
> > > +	if ((tz->temperature + (trip_temp >> 2)) < trip_temp) {
> > > +		mutex_lock(&tz->lock);
> > > +		tz->overheated = false;
> > > +		if (tz->boost_polling) {
> > > +			tz->boost_polling = false;
> > > +			tz->polling_delay = 0;
> > > +		}
> > > +		mutex_unlock(&tz->lock);
> > > +		cpufreq_boost_trigger_state(1);
> > > +		return 0;
> > > +	}
> > > +	return -EBUSY;
> > > +}
> > > +
> > > +static void thermal_boost_disable(struct thermal_zone_device *tz)
> > > +{
> > > +	cpufreq_boost_trigger_state(0);
> > > +
> > > +	/*
> > > +	 * If no workqueue for monitoring is running - start one
> > > with
> > > +	 * 1000 ms monitoring period
> > > +	 * If workqueue already running - do not change its period
> > > and only
> > > +	 * test if target CPU has cooled down
> > > +	 */
> > > +	mutex_lock(&tz->lock);
> > > +	if (!tz->polling_delay) {
> > > +		tz->boost_polling = true;
> > > +		tz->polling_delay = 1000;
> > > +	}
> > > +	tz->overheated = true;
> > > +	mutex_unlock(&tz->lock);
> > > +}
> > > +
> > >  static void handle_thermal_trip(struct thermal_zone_device *tz,
> > > int trip) {
> > >  	enum thermal_trip_type type;
> > > +	if (cpufreq_boost_supported() && cpufreq_boost_enabled())
> > > +		thermal_boost_disable(tz);
> > >
> > >  	tz->ops->get_trip_type(tz, trip, &type);
> > >
> > > @@ -467,6 +518,10 @@ static void thermal_zone_device_check(struct
> > > work_struct *work) struct thermal_zone_device *tz =
> > > container_of(work, struct thermal_zone_device,
> > >  						      poll_queue.work);
> > > +	if (cpufreq_boost_supported())
> > > +		if (!thermal_boost_enable(tz))
> > > +			return;
> > > +
> > >  	thermal_zone_device_update(tz);
> > >  }
> > >
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index b268d3c..b316bdf 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -172,6 +172,8 @@ struct thermal_zone_device {
> > >  	int emul_temperature;
> > >  	int passive;
> > >  	unsigned int forced_passive;
> > > +	bool overheated;
> > > +	bool boost_polling;
> > >  	const struct thermal_zone_device_ops *ops;
> > >  	const struct thermal_zone_params *tzp;
> > >  	struct thermal_governor *governor;
> >
> >
> 
> 
> 
> --
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists