[PATCH] Add idle wait support for 44x platforms

[PATCH] Add idle wait support for 44x platforms

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1207262487 18000
# Node ID 7226bef216680748a50327900572c2fbc3e762b0
# Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
Add idle wait support for 44x platforms

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.

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

2) The device tree will be checked for the "/hypervisor" node
   If this node is seen it will use "wait" for idle, so that
   the hypervisor can know when guest Linux kernel it is in
   an idle state.

This patch, unlike the last, isolates the code to 44x platforms.

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

diff --git a/arch/powerpc/platforms/44x/44x.h b/arch/powerpc/platforms/44x/44x.h
--- a/arch/powerpc/platforms/44x/44x.h
+++ b/arch/powerpc/platforms/44x/44x.h
@@ -5,4 +5,6 @@ extern void as1_writeb(u8 data, volatile
 extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
 extern void ppc44x_reset_system(char *cmd);
 
+extern int ppc44x_idle_init(void);
+
 #endif /* __POWERPC_PLATFORMS_44X_44X_H */
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,5 @@ obj-$(CONFIG_44x)	:= misc_44x.o
 obj-$(CONFIG_44x)	:= misc_44x.o
+obj-$(CONFIG_44x)	+= idle.o
 obj-$(CONFIG_EBONY)	+= ebony.o
 obj-$(CONFIG_TAISHAN)	+= taishan.o
 obj-$(CONFIG_BAMBOO)	+= bamboo.o
diff --git a/arch/powerpc/platforms/44x/bamboo.c b/arch/powerpc/platforms/44x/bamboo.c
--- a/arch/powerpc/platforms/44x/bamboo.c
+++ b/arch/powerpc/platforms/44x/bamboo.c
@@ -61,3 +61,5 @@ define_machine(bamboo) {
 	.restart			= ppc44x_reset_system,
 	.calibrate_decr 	= generic_calibrate_decr,
 };
+
+machine_late_initcall(bamboo, ppc44x_idle_init);
diff --git a/arch/powerpc/platforms/44x/canyonlands.c b/arch/powerpc/platforms/44x/canyonlands.c
--- a/arch/powerpc/platforms/44x/canyonlands.c
+++ b/arch/powerpc/platforms/44x/canyonlands.c
@@ -62,3 +62,5 @@ define_machine(canyonlands) {
 	.restart			= ppc44x_reset_system,
 	.calibrate_decr			= generic_calibrate_decr,
 };
+
+machine_late_initcall(canyonlands, ppc44x_idle_init);
diff --git a/arch/powerpc/platforms/44x/ebony.c b/arch/powerpc/platforms/44x/ebony.c
--- a/arch/powerpc/platforms/44x/ebony.c
+++ b/arch/powerpc/platforms/44x/ebony.c
@@ -69,3 +69,5 @@ define_machine(ebony) {
 	.restart		= ppc44x_reset_system,
 	.calibrate_decr		= generic_calibrate_decr,
 };
+
+machine_late_initcall(ebony, ppc44x_idle_init);
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,75 @@
+/*
+ * Copyright 2008 IBM Corp. 
+ *
+ * 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 <asm/machdep.h>
+
+static void ppc44x_idle(void);
+
+struct sleep_mode {
+	char *name;
+	void (*entry)(void);
+};
+
+static struct sleep_mode modes[] = {
+	{ .name = "spin", .entry = NULL },
+	{ .name = "wait", .entry = &ppc44x_idle },
+};
+
+static int current_mode = 0;
+
+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);
+	/* return to initial state */
+	mtmsr(msr_save);
+}
+
+int __init ppc44x_idle_init(void)
+{
+	if(of_find_node_by_path("/hypervisor") != NULL) {
+		/* if we find /hypervisor node is in device tree,
+		   set idle mode to wait */
+		current_mode = 1; /* wait mode */
+	}
+
+	ppc_md.power_save = modes[current_mode].entry;
+	return 0;
+}
+
+static int __init idle_param(char *p)
+{ 
+	int i;
+
+	for (i = 0; i < sizeof(modes)/sizeof(struct sleep_mode); i++) {
+		if (!strcmp(modes[i].name, p)) {
+			current_mode = i;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+early_param("idle", idle_param);
diff --git a/arch/powerpc/platforms/44x/katmai.c b/arch/powerpc/platforms/44x/katmai.c
--- a/arch/powerpc/platforms/44x/katmai.c
+++ b/arch/powerpc/platforms/44x/katmai.c
@@ -61,3 +61,5 @@ define_machine(katmai) {
 	.restart			= ppc44x_reset_system,
 	.calibrate_decr			= generic_calibrate_decr,
 };
+
+machine_late_initcall(katmai, ppc44x_idle_init);
diff --git a/arch/powerpc/platforms/44x/rainier.c b/arch/powerpc/platforms/44x/rainier.c
--- a/arch/powerpc/platforms/44x/rainier.c
+++ b/arch/powerpc/platforms/44x/rainier.c
@@ -60,3 +60,5 @@ define_machine(rainier) {
 	.restart			= ppc44x_reset_system,
 	.calibrate_decr			= generic_calibrate_decr,
 };
+
+machine_late_initcall(rainier, ppc44x_idle_init);
diff --git a/arch/powerpc/platforms/44x/sequoia.c b/arch/powerpc/platforms/44x/sequoia.c
--- a/arch/powerpc/platforms/44x/sequoia.c
+++ b/arch/powerpc/platforms/44x/sequoia.c
@@ -61,3 +61,5 @@ define_machine(sequoia) {
 	.restart			= ppc44x_reset_system,
 	.calibrate_decr			= generic_calibrate_decr,
 };
+
+machine_late_initcall(sequoia, ppc44x_idle_init);
diff --git a/arch/powerpc/platforms/44x/taishan.c b/arch/powerpc/platforms/44x/taishan.c
--- a/arch/powerpc/platforms/44x/taishan.c
+++ b/arch/powerpc/platforms/44x/taishan.c
@@ -71,3 +71,5 @@ define_machine(taishan) {
 	.restart		= ppc44x_reset_system,
 	.calibrate_decr		= generic_calibrate_decr,
 };
+
+machine_late_initcall(taishan, ppc44x_idle_init);

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

[Tony Breeds]

On Thu, Apr 03, 2008 at 05:43:02PM -0500, Jerone Young wrote:

Hi Jerone,
	A few minor nits.
> Add idle wait support for 44x platforms
> 
> 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.
> 
> 1) Command line
> 	idle=spin <-- CPU will spin (this is the default)
> 	idle=wait <-- set CPU into wait state when idle
> 
> 2) The device tree will be checked for the "/hypervisor" node
>    If this node is seen it will use "wait" for idle, so that
>    the hypervisor can know when guest Linux kernel it is in
>    an idle state.
> 
> This patch, unlike the last, isolates the code to 44x platforms.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

Can you include a diffstat in here?
 
> +static int current_mode = 0;

Leave this as: static int current_mode;, so it'll end up in the bss

> +int __init ppc44x_idle_init(void)
> +{
> +	if(of_find_node_by_path("/hypervisor") != NULL) {
          ^ space
> +		/* if we find /hypervisor node is in device tree,
> +		   set idle mode to wait */
> +		current_mode = 1; /* wait mode */
> +	}

You don't really need the braces {} here.

> +static int __init idle_param(char *p)
> +{ 
> +	int i;
> +
> +	for (i = 0; i < sizeof(modes)/sizeof(struct sleep_mode); i++) {

ARRAY_SIZE(modes)

Yours Tony

  linux.conf.au    http://www.marchsouth.org/
  Jan 19 - 24 2009 The Australian Linux Technical Conference!

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

[Hollis Blanchard]

On Thursday 03 April 2008 17:43:02 Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1207262487 18000
> # Node ID 7226bef216680748a50327900572c2fbc3e762b0
> # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> Add idle wait support for 44x platforms
>
> 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.
>
> 1) Command line
> 	idle=spin <-- CPU will spin (this is the default)
> 	idle=wait <-- set CPU into wait state when idle
>
> 2) The device tree will be checked for the "/hypervisor" node
>    If this node is seen it will use "wait" for idle, so that
>    the hypervisor can know when guest Linux kernel it is in
>    an idle state.
>
> This patch, unlike the last, isolates the code to 44x platforms.
>
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

Very nice.

> +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);
> +	/* return to initial state */
> +	mtmsr(msr_save);
> +}
> +
> +int __init ppc44x_idle_init(void)
> +{
> +	if(of_find_node_by_path("/hypervisor") != NULL) {
> +		/* if we find /hypervisor node is in device tree,
> +		   set idle mode to wait */
> +		current_mode = 1; /* wait mode */
> +	}
> +
> +	ppc_md.power_save = modes[current_mode].entry;
> +	return 0;
> +}

By the way, watch that space in "if(". Also, you need to call of_node_put().

The one thing I don't like is the hardcoded assumption that 1 means "wait". 
Instead, you could do something like this (not even compile-tested):

int __init ppc44x_idle_init(void)
{
	void *func = modes[current_mode].entry;
	struct device_node *node;

	node = of_find_node_by_path("/hypervisor") 
	if (node) {
		/* if we find /hypervisor node is in device tree,
		 * set idle mode to wait */
		func = &ppc44x_idle;
		of_node_put(node);
	}

	ppc_md.power_save = func;
	return 0;
}

Stuart, we're getting into ePAPR territory. Do you think we need to worry 
about a hypervisor not handling mtmsr(MSR_WE)? In that case, we'd need to be 
more specific than just testing for "/hypervisor". IMHO every hypervisor 
should implement it... but maybe /hypervisor/idle = "wait" would be more 
explicit and therefore better?

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Scott Wood]

On Thu, Apr 03, 2008 at 06:13:59PM -0500, Hollis Blanchard wrote:
> Stuart, we're getting into ePAPR territory. Do you think we need to worry 
> about a hypervisor not handling mtmsr(MSR_WE)? In that case, we'd need to be 
> more specific than just testing for "/hypervisor". IMHO every hypervisor 
> should implement it... but maybe /hypervisor/idle = "wait" would be more 
> explicit and therefore better?

We can't trap on MSR[WE] updates on hypervisor-enabled chips; the write
is silently ignored.

-Scott

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

[Josh Boyer]

On Fri, 4 Apr 2008 10:03:56 +1100
tony@bakeyournoodle.com (Tony Breeds) wrote:

> > +int __init ppc44x_idle_init(void)
> > +{
> > +	if(of_find_node_by_path("/hypervisor") != NULL) {
>           ^ space
> > +		/* if we find /hypervisor node is in device tree,
> > +		   set idle mode to wait */
> > +		current_mode = 1; /* wait mode */
> > +	}
> 
> You don't really need the braces {} here.

They are technically not required, but I like them there none the
less because of the multi-line comment.  I find it to be better style
personally.

josh

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

[Josh Boyer]

On Thu, 03 Apr 2008 17:43:02 -0500
Jerone Young <jyoung5@us.ibm.com> wrote:

> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1207262487 18000
> # Node ID 7226bef216680748a50327900572c2fbc3e762b0
> # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1

As a complete and unrelated side note to the actual patch, wtf is this
hg stuff?  I can't really tell what tree you're even basing this off of.

> Add idle wait support for 44x platforms
> 
> 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.

This huge single line needs fixing in the next version of the patch.

> 
> 1) Command line
> 	idle=spin <-- CPU will spin (this is the default)
> 	idle=wait <-- set CPU into wait state when idle
> 
> 2) The device tree will be checked for the "/hypervisor" node
>    If this node is seen it will use "wait" for idle, so that
>    the hypervisor can know when guest Linux kernel it is in
>    an idle state.
> 
> This patch, unlike the last, isolates the code to 44x platforms.

In addition to the comments Tony and Hollis made, I have a few of my
own.

> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/platforms/44x/44x.h b/arch/powerpc/platforms/44x/44x.h
> --- a/arch/powerpc/platforms/44x/44x.h
> +++ b/arch/powerpc/platforms/44x/44x.h
> @@ -5,4 +5,6 @@ extern void as1_writeb(u8 data, volatile
>  extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
>  extern void ppc44x_reset_system(char *cmd);
> 
> +extern int ppc44x_idle_init(void);
> +
>  #endif /* __POWERPC_PLATFORMS_44X_44X_H */

The changes to this file aren't needed.  See below.

> 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,5 @@ obj-$(CONFIG_44x)	:= misc_44x.o
>  obj-$(CONFIG_44x)	:= misc_44x.o
> +obj-$(CONFIG_44x)	+= idle.o

Just add this target to the already existing obj-(CONFIG_44x)

>  obj-$(CONFIG_EBONY)	+= ebony.o
>  obj-$(CONFIG_TAISHAN)	+= taishan.o
>  obj-$(CONFIG_BAMBOO)	+= bamboo.o
> diff --git a/arch/powerpc/platforms/44x/bamboo.c b/arch/powerpc/platforms/44x/bamboo.c
> --- a/arch/powerpc/platforms/44x/bamboo.c
> +++ b/arch/powerpc/platforms/44x/bamboo.c
> @@ -61,3 +61,5 @@ define_machine(bamboo) {
>  	.restart			= ppc44x_reset_system,
>  	.calibrate_decr 	= generic_calibrate_decr,
>  };
> +
> +machine_late_initcall(bamboo, ppc44x_idle_init);

Ugh.  Don't add an init call to every 4xx board like this.  It's not
needed.  See below.

> 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

If you're ever going to extend bare metal support for this to 40x, then
this is the wrong place for it.  It should reside in
arch/powerpc/sysdev/ppc4xx_soc.c in that case.

> +
> +#include <linux/of.h>
> +#include <asm/machdep.h>
> +
> +static void ppc44x_idle(void);

This isn't needed.  Move the structures below the function.

> +struct sleep_mode {
> +	char *name;
> +	void (*entry)(void);
> +};
> +
> +static struct sleep_mode modes[] = {
> +	{ .name = "spin", .entry = NULL },
> +	{ .name = "wait", .entry = &ppc44x_idle },
> +};

<snip>

> +int __init ppc44x_idle_init(void)
> +{
> +	if(of_find_node_by_path("/hypervisor") != NULL) {
> +		/* if we find /hypervisor node is in device tree,
> +		   set idle mode to wait */
> +		current_mode = 1; /* wait mode */
> +	}
> +
> +	ppc_md.power_save = modes[current_mode].entry;
> +	return 0;

I liked Hollis' method of assignment here.

> +}

Add an arch_initcall(ppc44x_idle_init); here and dispense with
changing every board .c file in the 44x directory.

josh

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

[Jerone Young]

On Thu, 2008-04-03 at 21:00 -0500, Josh Boyer wrote:
> On Thu, 03 Apr 2008 17:43:02 -0500
> Jerone Young <jyoung5@us.ibm.com> wrote:
> 
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1207262487 18000
> > # Node ID 7226bef216680748a50327900572c2fbc3e762b0
> > # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> 
> As a complete and unrelated side note to the actual patch, wtf is this
> hg stuff?  I can't really tell what tree you're even basing this off of.

hehe...I primary use hg as it's just much easier to deal with. Yes I do
go through the conversion. But it is worth it. The patches I send are in
git format.

> 
> > Add idle wait support for 44x platforms
> > 
> > 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.
> 
> This huge single line needs fixing in the next version of the patch.
> 
> > 
> > 1) Command line
> > 	idle=spin <-- CPU will spin (this is the default)
> > 	idle=wait <-- set CPU into wait state when idle
> > 
> > 2) The device tree will be checked for the "/hypervisor" node
> >    If this node is seen it will use "wait" for idle, so that
> >    the hypervisor can know when guest Linux kernel it is in
> >    an idle state.
> > 
> > This patch, unlike the last, isolates the code to 44x platforms.
> 
> In addition to the comments Tony and Hollis made, I have a few of my
> own.
> 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> > 
> > diff --git a/arch/powerpc/platforms/44x/44x.h b/arch/powerpc/platforms/44x/44x.h
> > --- a/arch/powerpc/platforms/44x/44x.h
> > +++ b/arch/powerpc/platforms/44x/44x.h
> > @@ -5,4 +5,6 @@ extern void as1_writeb(u8 data, volatile
> >  extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
> >  extern void ppc44x_reset_system(char *cmd);
> > 
> > +extern int ppc44x_idle_init(void);
> > +
> >  #endif /* __POWERPC_PLATFORMS_44X_44X_H */
> 
> The changes to this file aren't needed.  See below.
> 
> > 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,5 @@ obj-$(CONFIG_44x)	:= misc_44x.o
> >  obj-$(CONFIG_44x)	:= misc_44x.o
> > +obj-$(CONFIG_44x)	+= idle.o
> 
> Just add this target to the already existing obj-(CONFIG_44x)
> 
> >  obj-$(CONFIG_EBONY)	+= ebony.o
> >  obj-$(CONFIG_TAISHAN)	+= taishan.o
> >  obj-$(CONFIG_BAMBOO)	+= bamboo.o
> > diff --git a/arch/powerpc/platforms/44x/bamboo.c b/arch/powerpc/platforms/44x/bamboo.c
> > --- a/arch/powerpc/platforms/44x/bamboo.c
> > +++ b/arch/powerpc/platforms/44x/bamboo.c
> > @@ -61,3 +61,5 @@ define_machine(bamboo) {
> >  	.restart			= ppc44x_reset_system,
> >  	.calibrate_decr 	= generic_calibrate_decr,
> >  };
> > +
> > +machine_late_initcall(bamboo, ppc44x_idle_init);
> 
> Ugh.  Don't add an init call to every 4xx board like this.  It's not
> needed.  See below.
> 
> > 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
> 
> If you're ever going to extend bare metal support for this to 40x, then
> this is the wrong place for it.  It should reside in
> arch/powerpc/sysdev/ppc4xx_soc.c in that case.

So I did this at first after your suggestions but then I changed it
because:

- it depended on CONFIG_4xx_SOC .. which I could not figure out who
actually enabled this

- wanted to make a patch that could also go into earlier kernels 


> 
> > +
> > +#include <linux/of.h>
> > +#include <asm/machdep.h>
> > +
> > +static void ppc44x_idle(void);
> 
> This isn't needed.  Move the structures below the function.

This can be done
> 
> > +struct sleep_mode {
> > +	char *name;
> > +	void (*entry)(void);
> > +};
> > +
> > +static struct sleep_mode modes[] = {
> > +	{ .name = "spin", .entry = NULL },
> > +	{ .name = "wait", .entry = &ppc44x_idle },
> > +};
> 
> <snip>
> 
> > +int __init ppc44x_idle_init(void)
> > +{
> > +	if(of_find_node_by_path("/hypervisor") != NULL) {
> > +		/* if we find /hypervisor node is in device tree,
> > +		   set idle mode to wait */
> > +		current_mode = 1; /* wait mode */
> > +	}
> > +
> > +	ppc_md.power_save = modes[current_mode].entry;
> > +	return 0;
> 
> I liked Hollis' method of assignment here.
> 
> > +}
> 
> Add an arch_initcall(ppc44x_idle_init); here and dispense with
> changing every board .c file in the 44x directory.
ah didn't know about arch_initcall

> 
> josh

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

[Jerone Young]

On Fri, 2008-04-04 at 10:03 +1100, Tony Breeds wrote:
> On Thu, Apr 03, 2008 at 05:43:02PM -0500, Jerone Young wrote:
> 
> Hi Jerone,
> 	A few minor nits.
> > Add idle wait support for 44x platforms
> > 
> > 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.
> > 
> > 1) Command line
> > 	idle=spin <-- CPU will spin (this is the default)
> > 	idle=wait <-- set CPU into wait state when idle
> > 
> > 2) The device tree will be checked for the "/hypervisor" node
> >    If this node is seen it will use "wait" for idle, so that
> >    the hypervisor can know when guest Linux kernel it is in
> >    an idle state.
> > 
> > This patch, unlike the last, isolates the code to 44x platforms.
> > 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> Can you include a diffstat in here?

I think there is a way. I can see if I can do it. Though I may have to
do it out side of my normal hg tools.

> 
> > +static int current_mode = 0;
> 
> Leave this as: static int current_mode;, so it'll end up in the bss

The problem here is that this defines the default case. Is there really
a benefit having this in bss ?

> 
> > +int __init ppc44x_idle_init(void)
> > +{
> > +	if(of_find_node_by_path("/hypervisor") != NULL) {
>           ^ space
> > +		/* if we find /hypervisor node is in device tree,
> > +		   set idle mode to wait */
> > +		current_mode = 1; /* wait mode */
> > +	}
> 
> You don't really need the braces {} here.
> 
> > +static int __init idle_param(char *p)
> > +{ 
> > +	int i;
> > +
> > +	for (i = 0; i < sizeof(modes)/sizeof(struct sleep_mode); i++) {
> 
> ARRAY_SIZE(modes)

I'll do this.

> 
> Yours Tony
> 
>   linux.conf.au    http://www.marchsouth.org/
>   Jan 19 - 24 2009 The Australian Linux Technical Conference!
> 

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

[Jerone Young]

On Thu, 2008-04-03 at 18:13 -0500, Hollis Blanchard wrote:
> On Thursday 03 April 2008 17:43:02 Jerone Young wrote:
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1207262487 18000
> > # Node ID 7226bef216680748a50327900572c2fbc3e762b0
> > # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> > Add idle wait support for 44x platforms
> >
> > 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.
> >
> > 1) Command line
> > 	idle=spin <-- CPU will spin (this is the default)
> > 	idle=wait <-- set CPU into wait state when idle
> >
> > 2) The device tree will be checked for the "/hypervisor" node
> >    If this node is seen it will use "wait" for idle, so that
> >    the hypervisor can know when guest Linux kernel it is in
> >    an idle state.
> >
> > This patch, unlike the last, isolates the code to 44x platforms.
> >
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> Very nice.
> 
> > +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);
> > +	/* return to initial state */
> > +	mtmsr(msr_save);
> > +}
> > +
> > +int __init ppc44x_idle_init(void)
> > +{
> > +	if(of_find_node_by_path("/hypervisor") != NULL) {
> > +		/* if we find /hypervisor node is in device tree,
> > +		   set idle mode to wait */
> > +		current_mode = 1; /* wait mode */
> > +	}
> > +
> > +	ppc_md.power_save = modes[current_mode].entry;
> > +	return 0;
> > +}
> 
> By the way, watch that space in "if(". Also, you need to call of_node_put()
Got it.

> 
> The one thing I don't like is the hardcoded assumption that 1 means "wait". 
> Instead, you could do something like this (not even compile-tested):
> 
> int __init ppc44x_idle_init(void)
> {
> 	void *func = modes[current_mode].entry;
> 	struct device_node *node;
> 
> 	node = of_find_node_by_path("/hypervisor") 
> 	if (node) {
> 		/* if we find /hypervisor node is in device tree,
> 		 * set idle mode to wait */
> 		func = &ppc44x_idle;
> 		of_node_put(node);
> 	}
> 
> 	ppc_md.power_save = func;
> 	return 0;
> }
> 
Will change.

> Stuart, we're getting into ePAPR territory. Do you think we need to worry 
> about a hypervisor not handling mtmsr(MSR_WE)? In that case, we'd need to be 
> more specific than just testing for "/hypervisor". IMHO every hypervisor 
> should implement it... but maybe /hypervisor/idle = "wait" would be more 
> explicit and therefore better?

This is the problem with just looking for the /hypervisor node. I'll
submit another with all the comments from everyone. But I have a feeling
this is going to lead to a longer discussion.

> 

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

[Josh Boyer]

On Fri, 04 Apr 2008 01:12:38 -0500
Jerone Young <jyoung5@us.ibm.com> wrote: 
> > 
> > > +static int current_mode = 0;
> > 
> > Leave this as: static int current_mode;, so it'll end up in the bss
> 
> The problem here is that this defines the default case. Is there really
> a benefit having this in bss ?

It's still defined to 0 if it's in the BSS, as that is all initialized
to 0.

josh

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

[Hollis Blanchard]

On Friday 04 April 2008 00:59:59 Jerone Young wrote:
> On Thu, 2008-04-03 at 21:00 -0500, Josh Boyer wrote:
> > On Thu, 03 Apr 2008 17:43:02 -0500
> >
> > Jerone Young <jyoung5@us.ibm.com> wrote:
> > > # HG changeset patch
> > > # User Jerone Young <jyoung5@us.ibm.com>
> > > # Date 1207262487 18000
> > > # Node ID 7226bef216680748a50327900572c2fbc3e762b0
> > > # Parent =A0a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> >
> > As a complete and unrelated side note to the actual patch, wtf is this
> > hg stuff? =A0I can't really tell what tree you're even basing this off =
of.
>
> hehe...I primary use hg as it's just much easier to deal with. Yes I do
> go through the conversion. But it is worth it. The patches I send are in
> git format.

By the way, you can use "hg email --plain" to avoid confusing the=20
unenlightened. ;) The Mercurial changeset data won't be useful here=20
anyways...

In reply to an earlier unrelated comment, there is also "hg email --diffsta=
t".

=2D-=20
Hollis Blanchard
IBM Linux Technology Center

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

[Arnd Bergmann]

On Friday 04 April 2008, Josh Boyer wrote:
> On Fri, 04 Apr 2008 01:12:38 -0500
> Jerone Young <jyoung5@us.ibm.com> wrote: 
> > > 
> > > > +static int current_mode = 0;
> > > 
> > > Leave this as: static int current_mode;, so it'll end up in the bss
> > 
> > The problem here is that this defines the default case. Is there really
> > a benefit having this in bss ?
> 
> It's still defined to 0 if it's in the BSS, as that is all initialized
> to 0.

Actually, a static assignment to 0 has not caused the symbol to end up
in .data for many gcc versions, it always goes into .bss now unless you
assign it a value other than 0 or use explicit section attributes.

Whether or not you write the "= 0" is purely stylistic sugar and does
not have any impact the generated binary.

	Arnd <><

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

[Josh Boyer]

On Tue, 8 Apr 2008 04:17:36 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Friday 04 April 2008, Josh Boyer wrote:
> > On Fri, 04 Apr 2008 01:12:38 -0500
> > Jerone Young <jyoung5@us.ibm.com> wrote: 
> > > > 
> > > > > +static int current_mode = 0;
> > > > 
> > > > Leave this as: static int current_mode;, so it'll end up in the bss
> > > 
> > > The problem here is that this defines the default case. Is there really
> > > a benefit having this in bss ?
> > 
> > It's still defined to 0 if it's in the BSS, as that is all initialized
> > to 0.
> 
> Actually, a static assignment to 0 has not caused the symbol to end up
> in .data for many gcc versions, it always goes into .bss now unless you
> assign it a value other than 0 or use explicit section attributes.

IIRC, gcc 3.2 is still supported and it didn't do that.  Old toolchains
still exist.

> Whether or not you write the "= 0" is purely stylistic sugar and does
> not have any impact the generated binary.

Only if you're using a newer gcc version...

josh

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

[Arnd Bergmann]

On Tuesday 08 April 2008, Josh Boyer wrote:
> >=20
> > Actually, a static assignment to 0 has not caused the symbol to end up
> > in .data for many gcc versions, it always goes into .bss now unless you
> > assign it a value other than 0 or use explicit section attributes.
>=20
> IIRC, gcc 3.2 is still supported and it didn't do that. =A0Old toolchains
> still exist.

Ok, I thought it was before 3.2. The oldest version I had around was
gcc-3.3 and that had the new behaviour.

	Arnd <><

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

[Josh Boyer]

On Tue, 8 Apr 2008 04:41:28 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 08 April 2008, Josh Boyer wrote:
> > >=20
> > > Actually, a static assignment to 0 has not caused the symbol to end up
> > > in .data for many gcc versions, it always goes into .bss now unless y=
ou
> > > assign it a value other than 0 or use explicit section attributes.
> >=20
> > IIRC, gcc 3.2 is still supported and it didn't do that. =C2=A0Old toolc=
hains
> > still exist.
>=20
> Ok, I thought it was before 3.2. The oldest version I had around was
> gcc-3.3 and that had the new behaviour.

http://www.gnu.org/software/gcc/gcc-3.3/changes.html

gcc 3.3.1 it seems.  I thought I included that in the original reply,
but it's late and I suck :).

josh