Add idle power save for ppc 4xx

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

Signed-off-by Jerone Young <[email protected]>

arch/powerpc/kernel/Makefile +1 -0

@@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
 obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
+obj-$(CONFIG_4xx)		+= idle_4xx.o
 obj-$(CONFIG_TAU)		+= tau_6xx.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
 				   swsusp_$(CONFIG_WORD_SIZE).o

arch/powerpc/kernel/idle_4xx.c new file +5 -0

new file mode 100644
@@ -0,0 +1,24 @@
+/*
+ * This file contains the power_save function for 4xx CPUs
+ *
+ * added by Jerone Young <[email protected]>
+ *

Quoted 37 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

Signed-off-by Jerone Young <[email protected]>

arch/powerpc/kernel/Makefile +1 -0

@@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
 obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
+obj-$(CONFIG_4xx)		+= idle_4xx.o
 obj-$(CONFIG_TAU)		+= tau_6xx.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
 				   swsusp_$(CONFIG_WORD_SIZE).o

arch/powerpc/kernel/idle_4xx.c new file +5 -0

new file mode 100644
@@ -0,0 +1,24 @@
+/*
+ * This file contains the power_save function for 4xx CPUs
+ *
+ * added by Jerone Young <[email protected]>
+ *

arch/powerpc/kernel/idle_4xx.c line 15

Quoted 10 lines

arch/powerpc/kernel/idle_4xx.c new file +10 -0

+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/processor.h>
+#include <asm/machdep.h>
+
+void ppc4xx_idle()

void ppc4xx_idle(void)

arch/powerpc/kernel/idle_4xx.c line 22

Quoted 7 lines

arch/powerpc/kernel/idle_4xx.c new file +7 -0

+{
+	unsigned long msr_save;
+
+	/* set wait state MSR */
+	local_irq_enable();
+	msr_save = mfmsr();
+	mtmsr(msr_save|MSR_WE);

Why don't you |MSR_WE|MSR_EE at the same time?

arch/powerpc/kernel/idle_4xx.c line 24

Quoted 3 lines

arch/powerpc/kernel/idle_4xx.c new file +2 -0

+	local_irq_disable();
+}

None of the other power_save() implementations need this. In fact many of them don't even seem to return; they just loop around mtmsr.

Quoted 14 lines

arch/powerpc/kernel/setup_32.c +4 -0

@@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
 	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
 	    cpu_has_feature(CPU_FTR_CAN_NAP))
 		ppc_md.power_save = ppc6xx_idle;
+#endif
+
+#ifdef CONFIG_4xx
+	ppc_md.power_save = ppc4xx_idle;
 #endif
 

arch/powerpc/kernel/setup_32.c void __init machine_init(unsigned long d · line 141

Quoted 1 line

arch/powerpc/kernel/setup_32.c +0 -0

 	if (ppc_md.progress)

This belongs in the platform setup code.

Quoted 11 lines

void ppc4xx_idle(void)

arch/powerpc/kernel/idle_4xx.c line 22

Quoted 7 lines

arch/powerpc/kernel/idle_4xx.c new file +7 -0

+{
+	unsigned long msr_save;
+
+	/* set wait state MSR */
+	local_irq_enable();
+	msr_save = mfmsr();
+	mtmsr(msr_save|MSR_WE);

Why don't you |MSR_WE|MSR_EE at the same time?

You technically can do this. But the question is do all 4xx cpus use MSR_EE to enable interrupts? I can assume they do (from what I know),

Quoted 10 lines

arch/powerpc/kernel/idle_4xx.c line 22

Quoted 8 lines

arch/powerpc/kernel/idle_4xx.c new file +7 -0

+{
+   unsigned long msr_save;
+
+   /* set wait state MSR */
+   local_irq_enable();
+   msr_save = mfmsr();
+   mtmsr(msr_save|MSR_WE);

Why don't you |MSR_WE|MSR_EE at the same time?

You technically can do this. But the question is do all 4xx cpus use MSR_EE to enable interrupts? I can assume they do (from what I know), but figured it would be safer to make the local_irq_enable() call. I can change it to just set the MSR_EE bit though, since all 4xx cpus (as far as I know) use it.

Quoted 4 lines

Quoted 3 lines

+   local_irq_disable();
+}

Quoted 2 lines

None of the other power_save() implementations need this. In fact many of them don't even seem to return; they just loop around mtmsr.

Sure it can be removed. Though with the comment in the mach_dep structure about power_save. It specifically says that interrupts are off when it is called. So I was following it here mainly. But I can remove the disabling of interrupts, since mtmsr is the only used under power_save.

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

Quoted 10 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Quoted 17 lines · On Mon, 31 Mar 2008 12:07:17 -0500 Josh Boyer <[email protected]> wrote:

Quoted 10 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

Quoted 3 lines

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

josh

Quoted 24 lines · Josh Boyer

Quoted 20 lines · On Mon, 31 Mar 2008 12:07:17 -0500 Josh Boyer <[email protected]> wrote:

Quoted 10 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Quoted 3 lines

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

I'll go with the Kconfig option.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

Quoted 1 line

I'll go with the Kconfig option.

Go with a device-tree check. The pseries kernel supports both bare-metal and hypervisor in the same kernel image, and it works out which it's running on by looking at the device-tree. This seems equivalent to me?

Quoted 10 lines · Michael Ellerman

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

I'll go with the Kconfig option.

Quoted 3 lines

Go with a device-tree check. The pseries kernel supports both bare-metal and hypervisor in the same kernel image, and it works out which it's running on by looking at the device-tree. This seems equivalent to me?

After some back and forth on IRC, we decided on that as well. I love being right, then wrong, then right again.

/me needs more sleep.

Quoted 13 lines · Michael Ellerman

Quoted 12 lines

Quoted 10 lines

Quoted 6 lines

I'm assuming you pass a dtb to the virtual guest when you start it up.
Could you define a property in the CPU node there that can be parsed to
use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

I'll go with the Kconfig option.

Quoted 5 lines

Go with a device-tree check. The pseries kernel supports both bare- metal
and hypervisor in the same kernel image, and it works out which it's running on by looking at the device-tree. This seems equivalent to me?

After some back and forth on IRC, we decided on that as well. I love being right, then wrong, then right again.

Quoted 20 lines · Josh Boyer

Quoted 18 lines · Michael Ellerman

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

I'm assuming you pass a dtb to the virtual guest when you start it up.
Could you define a property in the CPU node there that can be parsed to
use the power_save function instead of always making it the default?

Quoted 5 lines

Quoted 3 lines

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

I'll go with the Kconfig option.

Go with a device-tree check. The pseries kernel supports both bare- metal
and hypervisor in the same kernel image, and it works out which it's running on by looking at the device-tree. This seems equivalent to me?

Quoted 2 lines

After some back and forth on IRC, we decided on that as well. I love being right, then wrong, then right again.

Awesome, can you summarize for the rest of us?

Quoted 12 lines · On Mon, 31 Mar 2008 12:07:17 -0500 Josh Boyer <[email protected]> wrote:

Quoted 11 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]> # Date 1206969060 18000 # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

Quoted 6 lines

Quoted 1 line

I have tested this on hardware & KVM virtual guest.

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

Quoted 3 lines

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device tree.
It doesn't describe hardware. A Kconfig option might be warranted
though.

Quoted 17 lines · Josh Boyer

Quoted 14 lines · On Mon, 31 Mar 2008 12:07:17 -0500 Josh Boyer <[email protected]> wrote:

Quoted 12 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]> # Date 1206969060 18000 # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

Quoted 4 lines

Quoted 4 lines

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

So it slightly helps hardware, and it helps virtualization a *lot*. What's the problem?

Quoted 4 lines

Quoted 3 lines

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Quoted 3 lines

Actually, you probably don't want this as a property in the device tree.
It doesn't describe hardware. A Kconfig option might be warranted
though.

There will be a device tree binding for hypervisor properties, so if it's not always enabled, having a hypervisor node (for any hypervisor) in the device tree would be an indicator. Far better than a Kconfig option, at any rate.

Quoted 19 lines · Hollis Blanchard

Quoted 17 lines · Josh Boyer

Quoted 14 lines · On Mon, 31 Mar 2008 12:07:17 -0500 Josh Boyer <[email protected]> wrote:

Quoted 12 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]> # Date 1206969060 18000 # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

So it slightly helps hardware, and it helps virtualization a *lot*. What's the problem?

There's 0 publicly available documentation on exactly what "Wait State Enable" means other than the description for the MSR register bit in the 4xx UM. I'm a very paranoid person.

Explain to me what it really provides with some kind of concrete numbers on real hardware and I'll think about it as the default. Until then, I think a Kconfig option (or DT property) is acceptable for now. I didn't say "no", I just said "make it optional."

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device tree.
It doesn't describe hardware. A Kconfig option might be warranted
though.

Quoted 4 lines

There will be a device tree binding for hypervisor properties, so if it's not always enabled, having a hypervisor node (for any hypervisor) in the device tree would be an indicator. Far better than a Kconfig option, at any rate.

But you want this in the guests, right? Not the hypervisor...

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

Quoted 3 lines · Josh Boyer

Actually, you probably don't want this as a property in the device tree. It doesn't describe hardware. A Kconfig option might be warranted though.

In general it is valid to have properties in the device tree that describe the hypervisor or the kernel's interface to it, since they are aspects of the platform, i.e. the virtual "hardware" that the kernel is running on.

Quoted 10 lines · Jerone Young

# HG changeset patch
# User Jerone Young <[email protected]>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

I'm not overly thrilled with adding this to all of 4xx. It doesn't actually save much power at all (1% on a project that actually measured it with an amp meter recently) and there's really no other benefit to doing it outside of the virtual guest case.

I'm assuming you pass a dtb to the virtual guest when you start it up. Could you define a property in the CPU node there that can be parsed to use the power_save function instead of always making it the default?

Quoted 11 lines

Signed-off-by Jerone Young <[email protected]>

arch/powerpc/kernel/Makefile +1 -0

@@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
 obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
+obj-$(CONFIG_4xx)		+= idle_4xx.o
 obj-$(CONFIG_TAU)		+= tau_6xx.o

arch/powerpc/kernel/idle_4xx.c

Quoted 6 lines

arch/powerpc/kernel/Makefile +0 -0

 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
 				   swsusp_$(CONFIG_WORD_SIZE).o

arch/powerpc/kernel/idle_4xx.c new file +0 -0

new file mode 100644

Can this be added to sysdev/ppc4xx_soc.c instead?

Quoted 4 lines

arch/powerpc/kernel/idle_4xx.c new file +3 -0

+#include <asm/processor.h>
+#include <asm/machdep.h>
+

arch/powerpc/kernel/idle_4xx.c line 24

Quoted 10 lines

arch/powerpc/kernel/idle_4xx.c new file +10 -0

+void ppc4xx_idle()
+{
+	unsigned long msr_save;
+
+	/* set wait state MSR */
+	local_irq_enable();
+	msr_save = mfmsr();
+	mtmsr(msr_save|MSR_WE);
+	local_irq_disable();
+}

I agree with Hollis on both the MSR_WE|MSR_EE and removing the local_irq_disable changes.

arch/powerpc/kernel/setup_32.c void __init machine_init(unsigned long d · line 139

Quoted 13 lines

arch/powerpc/kernel/setup_32.c +4 -0

@@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
 	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
 	    cpu_has_feature(CPU_FTR_CAN_NAP))
 		ppc_md.power_save = ppc6xx_idle;
+#endif
+
+#ifdef CONFIG_4xx
+	ppc_md.power_save = ppc4xx_idle;
 #endif

I agree this belongs in platform setup code. Finding the right spot for it might be a bit of a challenge.

josh

Quoted 6 lines

arch/powerpc/kernel/idle_4xx.c

Quoted 4 lines

arch/powerpc/kernel/idle_4xx.c new file +0 -0

new file mode 100644

Can this be added to sysdev/ppc4xx_soc.c instead?

Probably. Though the other platforms have there power_save code in idle_<platform>.S files, as they are in assembly.

Also I don't appear to have ppc4xx_soc.c in my source (using 2.6.25-rc6).