[PATCH v2 1/6] davinci: add CPU idle driver

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/6] davinci: add CPU idle driver

Kevin Hilman
Sekhar Nori <[hidden email]> writes:

> The patch adds support for DaVinci cpu idle driver.
>
> Two idle states are defined:
> 1. Wait for interrupt
> 2. Wait for interrupt and DDR self-refresh (or power down)
>
> Some DaVinci SoCs support putting DDR in self-refresh (eg Dm644x, DM6467)
> while others support putting DDR in self-refresh and power down (eg DM35x,
> DA8xx).
>
> Putting DDR (or mDDR) in power down saves more power than self-refresh.
>
> The patch has been tested on DA850/OMAP-L138 EVM.
>
> Signed-off-by: Sekhar Nori <[hidden email]>

Looks pretty good, some minor comments below.

> ---
>  arch/arm/mach-davinci/Makefile               |    1 +
>  arch/arm/mach-davinci/cpuidle.c              |  189 ++++++++++++++++++++++++++
>  arch/arm/mach-davinci/include/mach/cpuidle.h |   17 +++
>  3 files changed, 207 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-davinci/cpuidle.c
>  create mode 100644 arch/arm/mach-davinci/include/mach/cpuidle.h
>
> diff --git a/arch/arm/mach-davinci/Makefile b/arch/arm/mach-davinci/Makefile
> index be629c5..5eae1a9 100644
> --- a/arch/arm/mach-davinci/Makefile
> +++ b/arch/arm/mach-davinci/Makefile
> @@ -32,3 +32,4 @@ obj-$(CONFIG_MACH_DAVINCI_DA850_EVM) += board-da850-evm.o
>  
>  # Power Management
>  obj-$(CONFIG_CPU_FREQ) += cpufreq.o
> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o
> diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
> new file mode 100644
> index 0000000..a14307f
> --- /dev/null
> +++ b/arch/arm/mach-davinci/cpuidle.c
> @@ -0,0 +1,189 @@
> +/*
> + * CPU idle for DaVinci SoCs
> + *
> + * Copyright (C) 2009 Texas Instruments Incorporated. http://www.ti.com/
> + *
> + * Derived from Marvell Kirkwood CPU idle code.

Can you add a file refernce.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpuidle.h>
> +#include <linux/io.h>
> +#include <asm/proc-fns.h>
> +
> +#include <mach/cpuidle.h>
> +
> +#define DAVINCI_CPUIDLE_MAX_STATES 2
> +
> +struct davinci_ops {
> + void (*enter) (void);
> + void (*exit) (void);
> +};
> +
> +static struct cpuidle_driver davinci_idle_driver = {
> + .name = "cpuidle-davinci",
> + .owner = THIS_MODULE,
> +};
> +
> +static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
> +static void __iomem *ddr2_reg_base;
> +static int ddr2_pdown;

Hmm, not crazy about this global.  I think it should probably be a per
C-state flag/bool instead.

Maybe adding a 'u32 flags' field to davinci_states where this could be
one of the options.

Kevin

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/6] davinci: add CPU idle driver

Nori, Sekhar
On Tue, Nov 03, 2009 at 00:51:51, Kevin Hilman wrote:
> Sekhar Nori <[hidden email]> writes:
>

[...]

> > diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
> > new file mode 100644
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/io.h>
> > +#include <asm/proc-fns.h>
> > +
> > +#include <mach/cpuidle.h>
> > +
> > +#define DAVINCI_CPUIDLE_MAX_STATES 2
> > +
> > +struct davinci_ops {
> > +   void (*enter) (void);
> > +   void (*exit) (void);
> > +};
> > +
> > +static struct cpuidle_driver davinci_idle_driver = {
> > +   .name   = "cpuidle-davinci",
> > +   .owner  = THIS_MODULE,
> > +};
> > +
> > +static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
> > +static void __iomem *ddr2_reg_base;
> > +static int ddr2_pdown;
>
> Hmm, not crazy about this global.  I think it should probably be a per
> C-state flag/bool instead.
>
> Maybe adding a 'u32 flags' field to davinci_states where this could be
> one of the options.

Kevin,

Power down support as such is platform specific, not state
specific. OTOH, having a per-state flag enables extension
to doing self-refresh in one state and power down in another
if so desired.

I guess it makes some sense both ways and I have gone ahead
and added the flags to davinci_ops as you asked for.

Thanks,
Sekhar