Re: [RFC PATCH] thermal: powerclamp: Add support for ARM machine to powerclamp driver.

[Eduardo Valentin <eduardo.valentin@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 8279 bytes --]

Hello Jonghwa,

On 25-07-2013 06:22, Jonghwa Lee wrote:
> This patch modifies intel_powerclamp driver can be used in not only x86
> but also ARM. Any ARM system can use intel_powerclamp driver for managing
> thermal problem through injecting intentional idle cycle. The required
> modification is only that it replaces x86's instruction, mwait, with arm
> specific one, wfi and changes the way of calculation of idle rate of last
> window. Other algorithm and codes can be shared without any amendment.
> 

First of all, thanks for taking this up. It was on my TODO list, but I
still could not manage to start doing it.

I have had a quick look on your patch. I promise to review it properly
as soon as possible. Two major concerns are (1) I think the ARM part of
it is not really specific to ARM (more like non-x86), and could be
reused to other archs (ok, there are couple of points that may need to
be checked); and (2) I would rather split the code into two layers, one
that talks to thermal fw and another that is specific to arch, this way
we could reduce ifdefery.



> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
> ---
>  drivers/thermal/Kconfig            |    3 +-
>  drivers/thermal/intel_powerclamp.c |   94 ++++++++++++++++++++++++++----------
>  2 files changed, 70 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index e988c81..912a788 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -162,8 +162,7 @@ config DB8500_CPUFREQ_COOLING
>  config INTEL_POWERCLAMP
>  	tristate "Intel PowerClamp idle injection driver"
>  	depends on THERMAL
> -	depends on X86
> -	depends on CPU_SUP_INTEL
> +	depends on (X86 && CPU_SUP_INTEL) || ARM
>  	help
>  	  Enable this to enable Intel PowerClamp idle injection driver. This
>  	  enforce idle time which results in more package C-state residency. The
> diff --git a/drivers/thermal/intel_powerclamp.c b/drivers/thermal/intel_powerclamp.c
> index b40b37c..3ff350b 100644
> --- a/drivers/thermal/intel_powerclamp.c
> +++ b/drivers/thermal/intel_powerclamp.c
> @@ -52,12 +52,18 @@
>  #include <linux/seq_file.h>
>  #include <linux/sched/rt.h>
>  
> +#include <asm/hardirq.h>
> +#ifdef CONFIG_X86
>  #include <asm/nmi.h>
>  #include <asm/msr.h>
>  #include <asm/mwait.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/idle.h>
> -#include <asm/hardirq.h>
> +
> +static unsigned int target_mwait;
> +#elif CONFIG_ARM
> +#include <asm/proc-fns.h>

I don't think this is specific to ARM.

> +#endif
>  
>  #define MAX_TARGET_RATIO (50U)
>  /* For each undisturbed clamping period (no extra wake ups during idle time),
> @@ -71,7 +77,6 @@
>   */
>  #define DEFAULT_DURATION_JIFFIES (6)
>  
> -static unsigned int target_mwait;
>  static struct dentry *debug_dir;
>  
>  /* user selected target */
> @@ -178,6 +183,7 @@ MODULE_PARM_DESC(window_size, "sliding window in number of clamping cycles\n"
>  	"\twindow size results in slower response time but more smooth\n"
>  	"\tclamping results. default to 2.");
>  
> +#ifdef CONFIG_X86
>  static void find_target_mwait(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
> @@ -246,6 +252,7 @@ static u64 pkg_state_counter(void)
>  
>  	return count;
>  }
> +#endif
>  
>  static void noop_timer(unsigned long foo)
>  {
> @@ -316,6 +323,25 @@ static void adjust_compensation(int target_ratio, unsigned int win)
>  	}
>  }
>  
> +#ifdef CONFIG_X86
> +static void inline powerclamp_get_cstate_inform(u64 *msr_now, u64 *tsc_now)
> +{
> +	*msr_now = pkg_state_counter();
> +	rdtscll(*tsc_now);
> +}
> +#elif CONFIG_ARM
> +static void inline powerclamp_get_cstate_inform(u64 *idle, u64 *wall)
> +{
> +	u64 _wall;
> +	int cpu;
> +
> +	for_each_online_cpu(cpu) {
> +		*idle += get_cpu_idle_time_us(cpu, &_wall);
> +		*wall += _wall;
> +	}
> +}
> +#endif
> +
>  static bool powerclamp_adjust_controls(unsigned int target_ratio,
>  				unsigned int guard, unsigned int win)
>  {
> @@ -324,8 +350,7 @@ static bool powerclamp_adjust_controls(unsigned int target_ratio,
>  	u64 val64;
>  
>  	/* check result for the last window */
> -	msr_now = pkg_state_counter();
> -	rdtscll(tsc_now);
> +	powerclamp_get_cstate_inform(&msr_now, &tsc_now);
>  
>  	/* calculate pkg cstate vs tsc ratio */
>  	if (!msr_last || !tsc_last)
> @@ -353,6 +378,34 @@ static bool powerclamp_adjust_controls(unsigned int target_ratio,
>  	return set_target_ratio + guard <= current_ratio;
>  }
>  
> +#ifdef CONFIG_X86
> +static void powerclamp_enter_idle(void)
> +{
> +	unsigned long ecx = 1;
> +	unsigned long eax = target_mwait;
> +
> +	/*
> +	 * REVISIT: may call enter_idle() to notify drivers who
> +	 * can save power during cpu idle. same for exit_idle()
> +	 */
> +	local_touch_nmi();
> +	stop_critical_timings();
> +	__monitor((void *)&current_thread_info()->flags, 0, 0);
> +	cpu_relax(); /* allow HT sibling to run */
> +	__mwait(eax, ecx);
> +	start_critical_timings();
> +	atomic_inc(&idle_wakeup_counter);
> +}
> +#elif CONFIG_ARM
> +static void  powerclamp_enter_idle(void)
> +{
> +	stop_critical_timings();
> +	cpu_do_idle();
> +	start_critical_timings();
> +	atomic_inc(&idle_wakeup_counter);
> +}
> +#endif
> +
>  static int clamp_thread(void *arg)
>  {
>  	int cpunr = (unsigned long)arg;
> @@ -428,22 +481,9 @@ static int clamp_thread(void *arg)
>  		preempt_disable();
>  		tick_nohz_idle_enter();
>  		/* mwait until target jiffies is reached */
> -		while (time_before(jiffies, target_jiffies)) {
> -			unsigned long ecx = 1;
> -			unsigned long eax = target_mwait;
> -
> -			/*
> -			 * REVISIT: may call enter_idle() to notify drivers who
> -			 * can save power during cpu idle. same for exit_idle()
> -			 */
> -			local_touch_nmi();
> -			stop_critical_timings();
> -			__monitor((void *)&current_thread_info()->flags, 0, 0);
> -			cpu_relax(); /* allow HT sibling to run */
> -			__mwait(eax, ecx);
> -			start_critical_timings();
> -			atomic_inc(&idle_wakeup_counter);
> -		}
> +		while (time_before(jiffies, target_jiffies))
> +			powerclamp_enter_idle();
> +
>  		tick_nohz_idle_exit();
>  		preempt_enable_no_resched();
>  	}
> @@ -465,13 +505,12 @@ static void poll_pkg_cstate(struct work_struct *dummy)
>  	static u64 tsc_last;
>  	static unsigned long jiffies_last;
>  
> -	u64 msr_now;
> +	u64 msr_now = 0;
>  	unsigned long jiffies_now;
> -	u64 tsc_now;
> +	u64 tsc_now = 0;
>  	u64 val64;
>  
> -	msr_now = pkg_state_counter();
> -	rdtscll(tsc_now);
> +	powerclamp_get_cstate_inform(&msr_now, &tsc_now);
>  	jiffies_now = jiffies;
>  
>  	/* calculate pkg cstate vs tsc ratio */
> @@ -499,11 +538,13 @@ static int start_power_clamp(void)
>  	unsigned long cpu;
>  	struct task_struct *thread;
>  
> +#ifdef CONFIG_X86
>  	/* check if pkg cstate counter is completely 0, abort in this case */
>  	if (!pkg_state_counter()) {
>  		pr_err("pkg cstate counter not functional, abort\n");
>  		return -EINVAL;
>  	}
> +#endif
>  
>  	set_target_ratio = clamp(set_target_ratio, 0U, MAX_TARGET_RATIO - 1);
>  	/* prevent cpu hotplug */
> @@ -661,6 +702,7 @@ static struct thermal_cooling_device_ops powerclamp_cooling_ops = {
>  	.set_cur_state = powerclamp_set_cur_state,
>  };
>  
> +#ifdef CONFIG_X86
>  /* runs on Nehalem and later */
>  static const struct x86_cpu_id intel_powerclamp_ids[] = {
>  	{ X86_VENDOR_INTEL, 6, 0x1a},
> @@ -678,9 +720,11 @@ static const struct x86_cpu_id intel_powerclamp_ids[] = {
>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, intel_powerclamp_ids);
> +#endif
>  
>  static int powerclamp_probe(void)
>  {
> +#ifdef CONFIG_X86
>  	if (!x86_match_cpu(intel_powerclamp_ids)) {
>  		pr_err("Intel powerclamp does not run on family %d model %d\n",
>  				boot_cpu_data.x86, boot_cpu_data.x86_model);
> @@ -694,7 +738,7 @@ static int powerclamp_probe(void)
>  
>  	/* find the deepest mwait value */
>  	find_target_mwait();
> -
> +#endif
>  	return 0;
>  }
>  
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]