[v7,1/9] cpuidle: Add common time keeping and irq enabling

Make necessary changes to implement time keeping and irq enabling
in the core cpuidle code.  This will allow the removal of these
functionalities from various platform cpuidle implementations whose
timekeeping and irq enabling follows the form in this common code.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/include/asm/cpuidle.h |   22 +++++++++++
 arch/arm/kernel/Makefile       |    2 +-
 arch/arm/kernel/cpuidle.c      |   21 +++++++++++
 drivers/cpuidle/cpuidle.c      |   79 ++++++++++++++++++++++++++++++++--------
 include/linux/cpuidle.h        |   13 ++++++-
 5 files changed, 119 insertions(+), 18 deletions(-)
 create mode 100644 arch/arm/include/asm/cpuidle.h
 create mode 100644 arch/arm/kernel/cpuidle.c

Hi Rob,

On 03/01/2012 06:12 AM, Robert Lee wrote:

> Make necessary changes to implement time keeping and irq enabling
> in the core cpuidle code.  This will allow the removal of these
> functionalities from various platform cpuidle implementations whose
> timekeeping and irq enabling follows the form in this common code.


The generic cpuidle changes look good, but is there a reason as
to why these changes are enabled only for ARM and not other
archs ?

> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/include/asm/cpuidle.h |   22 +++++++++++
>  arch/arm/kernel/Makefile       |    2 +-
>  arch/arm/kernel/cpuidle.c      |   21 +++++++++++
>  drivers/cpuidle/cpuidle.c      |   79 ++++++++++++++++++++++++++++++++--------
>  include/linux/cpuidle.h        |   13 ++++++-
>  5 files changed, 119 insertions(+), 18 deletions(-)
>  create mode 100644 arch/arm/include/asm/cpuidle.h
>  create mode 100644 arch/arm/kernel/cpuidle.c
> 
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> new file mode 100644
> index 0000000..165676e
> --- /dev/null
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -0,0 +1,22 @@
> +#ifndef __ASM_ARM_CPUIDLE_H
> +#define __ASM_ARM_CPUIDLE_H
> +
> +#ifdef CONFIG_CPU_IDLE
> +extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index);
> +#else
> +static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index) { return -ENODEV; }
> +#endif
> +
> +/* Common ARM WFI state */
> +#define ARM_CPUIDLE_WFI_STATE {\
> +	.enter                  = arm_cpuidle_simple_enter,\
> +	.exit_latency           = 1,\
> +	.target_residency       = 1,\
> +	.flags                  = CPUIDLE_FLAG_TIME_VALID,\
> +	.name                   = "WFI",\
> +	.desc                   = "ARM WFI",\
> +}
> +
> +#endif
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 43b740d..940c27f 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -21,7 +21,7 @@ obj-$(CONFIG_DEPRECATED_PARAM_STRUCT) += compat.o
> 
>  obj-$(CONFIG_LEDS)		+= leds.o
>  obj-$(CONFIG_OC_ETM)		+= etm.o
> -
> +obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
>  obj-$(CONFIG_ISA_DMA_API)	+= dma.o
>  obj-$(CONFIG_ARCH_ACORN)	+= ecard.o 
>  obj-$(CONFIG_FIQ)		+= fiq.o fiqasm.o
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> new file mode 100644
> index 0000000..89545f6
> --- /dev/null
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/cpuidle.h>
> +#include <asm/proc-fns.h>
> +
> +int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index)
> +{
> +	cpu_do_idle();
> +
> +	return index;
> +}
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 59f4261..56de5f7 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -53,6 +53,24 @@ static void cpuidle_kick_cpus(void) {}
> 
>  static int __cpuidle_register_device(struct cpuidle_device *dev);
> 
> +static inline int cpuidle_enter(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int index)
> +{
> +	struct cpuidle_state *target_state = &drv->states[index];
> +	return target_state->enter(dev, drv, index);
> +}
> +
> +static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	return cpuidle_wrap_enter(dev, drv, index, cpuidle_enter);
> +}
> +
> +typedef int (*cpuidle_enter_t)(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index);
> +
> +static cpuidle_enter_t cpuidle_enter_ops;
> +
>  /**
>   * cpuidle_idle_call - the main idle loop
>   *
> @@ -63,7 +81,6 @@ int cpuidle_idle_call(void)
>  {
>  	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>  	struct cpuidle_driver *drv = cpuidle_get_driver();
> -	struct cpuidle_state *target_state;
>  	int next_state, entered_state;
> 
>  	if (off)
> @@ -92,12 +109,10 @@ int cpuidle_idle_call(void)
>  		return 0;
>  	}
> 
> -	target_state = &drv->states[next_state];
> -
>  	trace_power_start(POWER_CSTATE, next_state, dev->cpu);
>  	trace_cpu_idle(next_state, dev->cpu);
> 
> -	entered_state = target_state->enter(dev, drv, next_state);
> +	entered_state = cpuidle_enter_ops(dev, drv, next_state);
> 
>  	trace_power_end(dev->cpu);
>  	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
> @@ -110,6 +125,8 @@ int cpuidle_idle_call(void)
>  		dev->states_usage[entered_state].time +=
>  				(unsigned long long)dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
> +	} else {
> +		dev->last_residency = 0;
>  	}
> 
>  	/* give the governor an opportunity to reflect on the outcome */
> @@ -164,20 +181,29 @@ void cpuidle_resume_and_unlock(void)
> 
>  EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
> 
> -#ifdef CONFIG_ARCH_HAS_CPU_RELAX
> -static int poll_idle(struct cpuidle_device *dev,
> -		struct cpuidle_driver *drv, int index)
> +/**
> + * cpuidle_wrap_enter - performs timekeeping and irqen around enter function
> + * @dev: pointer to a valid cpuidle_device object
> + * @drv: pointer to a valid cpuidle_driver object
> + * @index: index of the target cpuidle state.
> + */
> +int cpuidle_wrap_enter(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int index,
> +				int (*enter)(struct cpuidle_device *dev,
> +					struct cpuidle_driver *drv, int index))
>  {
> -	ktime_t	t1, t2;
> +	ktime_t time_start, time_end;
>  	s64 diff;
> 
> -	t1 = ktime_get();
> +	time_start = ktime_get();
> +
> +	index = enter(dev, drv, index);
> +
> +	time_end = ktime_get();
> +
>  	local_irq_enable();
> -	while (!need_resched())
> -		cpu_relax();
> 
> -	t2 = ktime_get();
> -	diff = ktime_to_us(ktime_sub(t2, t1));
> +	diff = ktime_to_us(ktime_sub(time_end, time_start));
>  	if (diff > INT_MAX)
>  		diff = INT_MAX;
> 
> @@ -186,6 +212,23 @@ static int poll_idle(struct cpuidle_device *dev,
>  	return index;
>  }
> 
> +#ifdef CONFIG_ARCH_HAS_CPU_RELAX
> +static inline int __poll_idle(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index)
> +{
> +	while (!need_resched())
> +		cpu_relax();
> +
> +	return index;
> +}
> +
> +static int poll_idle(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index)
> +{
> +	return cpuidle_wrap_enter(dev,	drv, index,
> +				__poll_idle);
> +}
> +
>  static void poll_idle_init(struct cpuidle_driver *drv)
>  {
>  	struct cpuidle_state *state = &drv->states[0];
> @@ -212,10 +255,11 @@ static void poll_idle_init(struct cpuidle_driver *drv) {}
>  int cpuidle_enable_device(struct cpuidle_device *dev)
>  {
>  	int ret, i;
> +	struct cpuidle_driver *drv = cpuidle_get_driver();
> 
>  	if (dev->enabled)
>  		return 0;
> -	if (!cpuidle_get_driver() || !cpuidle_curr_governor)
> +	if (!drv || !cpuidle_curr_governor)
>  		return -EIO;
>  	if (!dev->state_count)
>  		return -EINVAL;
> @@ -226,13 +270,16 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
>  			return ret;
>  	}
> 
> -	poll_idle_init(cpuidle_get_driver());
> +	cpuidle_enter_ops = drv->en_core_tk_irqen ?
> +		cpuidle_enter_tk : cpuidle_enter;
> +
> +	poll_idle_init(drv);
> 
>  	if ((ret = cpuidle_add_state_sysfs(dev)))
>  		return ret;
> 
>  	if (cpuidle_curr_governor->enable &&
> -	    (ret = cpuidle_curr_governor->enable(cpuidle_get_driver(), dev)))
> +	    (ret = cpuidle_curr_governor->enable(drv, dev)))
>  		goto fail_sysfs;
> 
>  	for (i = 0; i < dev->state_count; i++) {
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..927db28 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -15,6 +15,7 @@
>  #include <linux/list.h>
>  #include <linux/kobject.h>
>  #include <linux/completion.h>
> +#include <linux/hrtimer.h>
> 
>  #define CPUIDLE_STATE_MAX	8
>  #define CPUIDLE_NAME_LEN	16
> @@ -122,6 +123,8 @@ struct cpuidle_driver {
>  	struct module 		*owner;
> 
>  	unsigned int		power_specified:1;
> +	/* set to 1 to use the core cpuidle time keeping (for all states). */
> +	unsigned int		en_core_tk_irqen:1;
>  	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
>  	int			state_count;
>  	int			safe_state_index;
> @@ -140,7 +143,10 @@ extern void cpuidle_pause_and_lock(void);
>  extern void cpuidle_resume_and_unlock(void);
>  extern int cpuidle_enable_device(struct cpuidle_device *dev);
>  extern void cpuidle_disable_device(struct cpuidle_device *dev);
> -
> +extern int cpuidle_wrap_enter(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int index,
> +				int (*enter)(struct cpuidle_device *dev,
> +					struct cpuidle_driver *drv, int index));
>  #else
>  static inline void disable_cpuidle(void) { }
>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
> @@ -157,6 +163,11 @@ static inline void cpuidle_resume_and_unlock(void) { }
>  static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>  {return -ENODEV; }
>  static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
> +static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int index,
> +				int (*enter)(struct cpuidle_device *dev,
> +					struct cpuidle_driver *drv, int index))
> +{ return -ENODEV; }
> 
>  #endif
> 


For the generic cpuidle changes
Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>

Hello Deepthi,

On Wed, Feb 29, 2012 at 10:15 PM, Deepthi Dharwar
<deepthi@linux.vnet.ibm.com> wrote:
> Hi Rob,
>
> On 03/01/2012 06:12 AM, Robert Lee wrote:
>
>> Make necessary changes to implement time keeping and irq enabling
>> in the core cpuidle code.  This will allow the removal of these
>> functionalities from various platform cpuidle implementations whose
>> timekeeping and irq enabling follows the form in this common code.
>
>
> The generic cpuidle changes look good, but is there a reason as
> to why these changes are enabled only for ARM and not other
> archs ?
>

Besides ARM, this patchset also enables some of this new consolidation
functionality on arch/SH and for archs that use the
CONFIG_ARCH_HAS_CPU_RELAX (maybe x86 uses this?).

For the powerpc P-series, it could probably could be modified to use
the consolidated timekeeping but I didn't feel comfortable making that
change myself for a couple of reasons.  First, the common wrapper also
includes the local_irq_enable() call, but the p-series cpuidle code
doesn't include this call, as instead, it relies on the
local_irq_enable() call in the cpu_idle() function in
arch/powerpc/kernel/idle.c.  Is it OK to remove this
local_irq_enable() once the wrapper is used?  Second, is there any
special coordination needed with the timekeeping functions and the
mfspr() calls?

Looking at the intel and acpi cpuidle implementations, their current
organization does seem to be able to use the common time keeping / irq
enabling wrapper.  Upon first glance, it appears that there are
special timer/timekeeping requirements for x86 that aren't required by
other platforms.  But that may not be correct.

If you look back at v4 of this patch series, you'll see an attempt at
a common timekeeping that could be used by x86 and acpi , but it
causes other compromises that to me aren't worth the extra gain from a
100% common timekeeping / irq enable solution.  I requested
feedback/opinions on this issue after v4 but didn't hear anything
about changes made to the intel or acpi implementations.  So I
continued on with the common wrapper direction from v3 when making v5.

Ultimately, even if the consolidated code only can be used by most and
not all arch or platform cpuidle implementations, it still reduces
some platform cpuidle fragmentation and duplicated code and hopefully
improves the maintainability of the core cpuidle.