[PATCH] [v5] Add idle wait support for 44x platforms

[PATCH] [v5] Add idle wait support for 44x platforms

[Jerone Young]

2 files changed, 77 insertions(+), 1 deletion(-)
arch/powerpc/platforms/44x/Makefile |    2 
arch/powerpc/platforms/44x/idle.c   |   76 +++++++++++++++++++++++++++++++++++


Updates: Now setting MSR_WE is now default
         Tested on hardware platforms bamboo & sequioa and appears
         things are working fine on actually hardware!

This patch adds the ability for the CPU to go into wait state while in cpu_idle loop. This helps virtulization solutions know when the guest Linux kernel is in an idle state. There are two ways to do it.

Command line
	idle=spin <-- CPU will spin
	idle=wait <-- set CPU into wait state when idle (default)


Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/platforms/44x/Makefile b/arch/powerpc/platforms/44x/Makefile
--- a/arch/powerpc/platforms/44x/Makefile
+++ b/arch/powerpc/platforms/44x/Makefile
@@ -1,4 +1,4 @@ obj-$(CONFIG_44x)	:= misc_44x.o
-obj-$(CONFIG_44x)	:= misc_44x.o
+obj-$(CONFIG_44x)	:= misc_44x.o idle.o
 obj-$(CONFIG_EBONY)	+= ebony.o
 obj-$(CONFIG_TAISHAN)	+= taishan.o
 obj-$(CONFIG_BAMBOO)	+= bamboo.o
diff --git a/arch/powerpc/platforms/44x/idle.c b/arch/powerpc/platforms/44x/idle.c
new file mode 100644
--- /dev/null
+++ b/arch/powerpc/platforms/44x/idle.c
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2008 IBM Corp. 
+ *
+ * Based on arch/powerpc/platforms/pasemi/idle.c: 
+ * Copyright (C) 2006-2007 PA Semi, Inc
+ *
+ * Added by: Jerone Young <jyoung5@us.ibm.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <asm/machdep.h>
+
+static int current_mode;
+
+struct sleep_mode {
+	char *name;
+	void (*entry)(void);
+};
+
+static void ppc44x_idle(void)
+{
+	unsigned long msr_save;
+
+	msr_save = mfmsr();
+	/* set wait state MSR */
+	mtmsr(msr_save|MSR_WE|MSR_EE|MSR_CE|MSR_DE);
+	isync();
+	/* return to initial state */
+	mtmsr(msr_save);
+	isync();
+}
+
+static struct sleep_mode modes[] = {
+	{ .name = "wait", .entry = &ppc44x_idle },
+	{ .name = "spin", .entry = NULL },
+};
+
+int __init ppc44x_idle_init(void)
+{
+	void *func = modes[current_mode].entry;
+	ppc_md.power_save = func;
+	return 0;
+}
+
+arch_initcall(ppc44x_idle_init);
+
+static int __init idle_param(char *p)
+{ 
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(modes); i++) {
+		if (!strcmp(modes[i].name, p)) {
+			current_mode = i;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+early_param("idle", idle_param);

Re: [kvm-ppc-devel] [PATCH] [v5] Add idle wait support for 44x platforms

[Hollis Blanchard]

On Tuesday 08 April 2008 11:49:14 Jerone Young wrote:
> 2 files changed, 77 insertions(+), 1 deletion(-)
> arch/powerpc/platforms/44x/Makefile |    2
> arch/powerpc/platforms/44x/idle.c   |   76
> +++++++++++++++++++++++++++++++++++
>
>
> Updates: Now setting MSR_WE is now default
>          Tested on hardware platforms bamboo & sequioa and appears
>          things are working fine on actually hardware!
>
> This patch adds the ability for the CPU to go into wait state while in
> cpu_idle loop. This helps virtulization solutions know when the guest Linux
> kernel is in an idle state. There are two ways to do it.
>
> Command line
> 	idle=spin <-- CPU will spin
> 	idle=wait <-- set CPU into wait state when idle (default)
>
>
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] [v5] Add idle wait support for 44x platforms

[Josh Boyer]

On Tue, 08 Apr 2008 11:49:14 -0500
Jerone Young <jyoung5@us.ibm.com> wrote:

> 2 files changed, 77 insertions(+), 1 deletion(-)
> arch/powerpc/platforms/44x/Makefile |    2 
> arch/powerpc/platforms/44x/idle.c   |   76 +++++++++++++++++++++++++++++++++++
> 
> 
> Updates: Now setting MSR_WE is now default
>          Tested on hardware platforms bamboo & sequioa and appears
>          things are working fine on actually hardware!
> 
> This patch adds the ability for the CPU to go into wait state while in cpu_idle loop. This helps virtulization solutions know when the guest Linux kernel is in an idle state. There are two ways to do it.
> 
> Command line
> 	idle=spin <-- CPU will spin
> 	idle=wait <-- set CPU into wait state when idle (default)
> 
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

Looks great.  I'll add it to my tree shortly.

josh

Re: [PATCH] [v5] Add idle wait support for 44x platforms

[Arnd Bergmann]

On Tuesday 08 April 2008, Jerone Young wrote:
> +static struct sleep_mode modes[] =3D {
> +=A0=A0=A0=A0=A0=A0=A0{ .name =3D "wait", .entry =3D &ppc44x_idle },
> +=A0=A0=A0=A0=A0=A0=A0{ .name =3D "spin", .entry =3D NULL },
> +};
> +
> +int __init ppc44x_idle_init(void)
> +{
> +=A0=A0=A0=A0=A0=A0=A0void *func =3D modes[current_mode].entry;
> +=A0=A0=A0=A0=A0=A0=A0ppc_md.power_save =3D func;
> +=A0=A0=A0=A0=A0=A0=A0return 0;
> +}
> +
> +arch_initcall(ppc44x_idle_init);
> +
> +static int __init idle_param(char *p)
> +{=20
> +=A0=A0=A0=A0=A0=A0=A0int i;
> +
> +=A0=A0=A0=A0=A0=A0=A0for (i =3D 0; i < ARRAY_SIZE(modes); i++) {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (!strcmp(modes[i].name, =
p)) {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0cur=
rent_mode =3D i;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0bre=
ak;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0}
> +=A0=A0=A0=A0=A0=A0=A0}
> +
> +=A0=A0=A0=A0=A0=A0=A0return 0;
> +}
> +
> +early_param("idle", idle_param);

ok, sorry to steal the show again, now that everyone seems to be happy
with the current code, but isn't this equivalent to the simpler

static int __init idle_param(char *p)
{
	if (!strcmp(modes[i].name, "spin"))
		ppc_md.power_save =3D NULL;
}
early_param("idle", idle_param);

if you statically initialize the ppc_md.power_save function to ppc44x_idle
in the platform setup files?

	Arnd <><

Re: [PATCH] [v5] Add idle wait support for 44x platforms

[Jerone Young]

On Thu, 2008-04-10 at 15:44 +0200, Arnd Bergmann wrote:
> On Tuesday 08 April 2008, Jerone Young wrote:
> > +static struct sleep_mode modes[] = {
> > +       { .name = "wait", .entry = &ppc44x_idle },
> > +       { .name = "spin", .entry = NULL },
> > +};
> > +
> > +int __init ppc44x_idle_init(void)
> > +{
> > +       void *func = modes[current_mode].entry;
> > +       ppc_md.power_save = func;
> > +       return 0;
> > +}
> > +
> > +arch_initcall(ppc44x_idle_init);
> > +
> > +static int __init idle_param(char *p)
> > +{ 
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(modes); i++) {
> > +               if (!strcmp(modes[i].name, p)) {
> > +                       current_mode = i;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +early_param("idle", idle_param);
> 
> ok, sorry to steal the show again, now that everyone seems to be happy
> with the current code, but isn't this equivalent to the simple

Well it could be this simple. But the current code leaves a lot more
room to add different type waits or spins if need be (if they are ever
needed ... though none off the top of my head at the moment)...but it
does allow you to create another wait state for whatever reason a lot
easier.

So I really don't think this needs to change. Unless everyone really
feels that it just has to be.

> 
> static int __init idle_param(char *p)
> {
> 	if (!strcmp(modes[i].name, "spin"))
> 		ppc_md.power_save = NULL;
> }
> early_param("idle", idle_param);
> 
> if you statically initialize the ppc_md.power_save function to ppc44x_idle
> in the platform setup files?

The idea is to not statically initialize ppc_md.power_save to
ppc44x_idle in each platform setup file.

> 
> 	Arnd <><

Re: [PATCH] [v5] Add idle wait support for 44x platforms

[Arnd Bergmann]

On Thursday 10 April 2008, Jerone Young wrote:
> Well it could be this simple. But the current code leaves a lot more
> room to add different type waits or spins if need be (if they are ever
> needed ... though none off the top of my head at the moment)...but it
> does allow you to create another wait state for whatever reason a lot
> easier.
>=20
> So I really don't think this needs to change. Unless everyone really
> feels that it just has to be.

No, it doesn't need to change, the current patch is entirely correct,
just a little bit more complicated than it needs to be, and I try
not to have code in anticipation of something getting more complicated
in the future, you can always add the complexity at the point where you
need it.

> >=20
> > static int __init idle_param(char *p)
> > {
> > =A0=A0=A0=A0=A0=A0if (!strcmp(modes[i].name, "spin"))
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ppc_md.power_save =3D NULL;
> > }
> > early_param("idle", idle_param);
> >=20
> > if you statically initialize the ppc_md.power_save function to ppc44x_i=
dle
> > in the platform setup files?
>=20
> The idea is to not statically initialize ppc_md.power_save to
> ppc44x_idle in each platform setup file.
>=20

Why not? Unlike the platform_initcall, it wouldn't cost anything and your
current code has the same effect in the end, but in a less obvious way.

	Arnd <><

Re: [PATCH] [v5] Add idle wait support for 44x platforms

[Josh Boyer]

On Fri, 2008-04-11 at 02:18 +0200, Arnd Bergmann wrote:
> > > static int __init idle_param(char *p)
> > > {
> > >       if (!strcmp(modes[i].name, "spin"))
> > >               ppc_md.power_save = NULL;
> > > }
> > > early_param("idle", idle_param);
> > > 
> > > if you statically initialize the ppc_md.power_save function to ppc44x_idle
> > > in the platform setup files?
> > 
> > The idea is to not statically initialize ppc_md.power_save to
> > ppc44x_idle in each platform setup file.
> > 
> 
> Why not? Unlike the platform_initcall, it wouldn't cost anything and your
> current code has the same effect in the end, but in a less obvious way.

This is likely something that has evolved from the original patch which
added a machine_late_initcall(<machine>, ppc44x_idle_init) to every
platform.c file.  It made the original patch overly complicated as
opposed to adding a single arch_initcall in a common file.  That was
also before we decided to make "wait" the default, which made things
much easier to deal with.

I'll take the blame for the way the minorly suboptimal state the patch
is, as Jerone had to jump through several hoops already at my request.
I agree it's cleaner to do as you suggest in the long run, and may well
make some changes along those lines myself.

josh

Re: [PATCH] [v5] Add idle wait support for 44x platforms

[Olof Johansson]

On Fri, Apr 11, 2008 at 02:18:22AM +0200, Arnd Bergmann wrote:
> On Thursday 10 April 2008, Jerone Young wrote:
> > Well it could be this simple. But the current code leaves a lot more
> > room to add different type waits or spins if need be (if they are ever
> > needed ... though none off the top of my head at the moment)...but it
> > does allow you to create another wait state for whatever reason a lot
> > easier.
> > 
> > So I really don't think this needs to change. Unless everyone really
> > feels that it just has to be.
> 
> No, it doesn't need to change, the current patch is entirely correct,
> just a little bit more complicated than it needs to be, and I try
> not to have code in anticipation of something getting more complicated
> in the future, you can always add the complexity at the point where you
> need it.

That piece of code came across from the platforms/pasemi version,
where we for a while had more than two kinds of idle loops
(doze/nap/sleep/rvwinkle). Turns out doze does well enough that the
others weren't really needed so I never submitted support for the deeper
sleep modes.


-Olof