[PATCH, RFC] linkstation: implement standby

[PATCH, RFC] linkstation: implement standby

[Guennadi Liakhovetski]

Implement suspend/resume for "mpc10x" compatible fsl host PCI controllers, 
use it for linkstation standby.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

This requires patches from Scott Wood from this thread: 
http://ozlabs.org/pipermail/linuxppc-dev/2007-July/039109.html and my 
recent patch to implement wakeup from serial ports.

Thanks
Guennadi

 arch/powerpc/platforms/embedded6xx/linkstation.c |   57 +++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.c                    |   44 +++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.h                    |    6 ++
 include/asm-powerpc/mpc6xx.h                     |    2
 4 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/embedded6xx/linkstation.c b/arch/powerpc/platforms/embedded6xx/linkstation.c
index ab9e3f9..8ee0f0b 100644
--- a/arch/powerpc/platforms/embedded6xx/linkstation.c
+++ b/arch/powerpc/platforms/embedded6xx/linkstation.c
@@ -197,3 +197,60 @@ define_machine(linkstation){
 	.halt	 		= linkstation_halt,
 	.calibrate_decr 	= generic_calibrate_decr,
 };
+
+#ifdef CONFIG_PM
+
+#include <sysdev/fsl_soc.h>
+#include <asm/mpc6xx.h>
+
+static int ls_pm_valid(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static int ls_pm_enter(suspend_state_t state)
+{
+	char ier;
+	int ret = 0;
+	u64 tb;
+
+	if ((ret = mpc10x_suspend(state)) < 0)
+		return ret;
+
+	/* Get timebase */
+	tb = get_tb();
+
+	/* put CPU to sleep, re-enabling interrupts */
+	mpc6xx_enter_sleep();
+
+	local_irq_disable();
+
+	set_tb(tb >> 32, tb & 0xfffffffful);
+
+fail:
+	mpc10x_resume(state);
+
+	return ret;
+}
+
+static struct pm_ops ls_pm_ops = {
+	.valid		= ls_pm_valid,
+	.enter		= ls_pm_enter,
+};
+
+static int __init ls_pm_init(void)
+{
+	if (!machine_is(linkstation))
+		return 0;
+
+	pm_set_ops(&ls_pm_ops);
+	return 0;
+}
+
+device_initcall(ls_pm_init);
+#endif
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..c0d66df 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -1186,3 +1186,47 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+#ifdef CONFIG_PM
+#include <linux/pci.h>
+#include <asm/pci-bridge.h>
+
+#define	MPC10X_LP_REF_EN	(1<<12)
+#define	MPC10X_PM		(1<<7)
+#define MPC10X_DOZE		(1<<5)
+#define	MPC10X_NAP		(1<<4)
+#define	MPC10X_SLEEP		(1<<3)
+
+int mpc10x_suspend(suspend_state_t state)
+{
+	struct pci_dev *bridge;
+	u16 pmcr1;
+
+	bridge = pci_find_slot(0, 0);
+	if (!bridge)
+		return -ENODEV;
+
+	pci_read_config_word(bridge, 0x70, &pmcr1);
+	pmcr1 &= ~(MPC10X_DOZE | MPC10X_NAP);
+	pmcr1 |= MPC10X_PM | MPC10X_SLEEP | MPC10X_LP_REF_EN;
+	pci_write_config_word(bridge, 0x70, pmcr1);
+
+	return 0;
+}
+
+int mpc10x_resume(suspend_state_t state)
+{
+	struct pci_dev *bridge;
+	u16 pmcr1;
+
+	bridge = pci_find_slot(0, 0);
+	if (!bridge)
+		return -ENODEV;
+
+	pci_read_config_word(bridge, 0x70, &pmcr1);
+	pmcr1 &= ~(MPC10X_PM | MPC10X_DOZE | MPC10X_SLEEP | MPC10X_NAP | MPC10X_LP_REF_EN);
+	pci_write_config_word(bridge, 0x70, pmcr1);
+
+	return 0;
+}
+#endif
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 04e145b..bcb96cf 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -8,5 +8,11 @@ extern phys_addr_t get_immrbase(void);
 extern u32 get_brgfreq(void);
 extern u32 get_baudrate(void);
 
+#ifdef CONFIG_PM
+#include <linux/pm.h>
+extern int mpc10x_suspend(suspend_state_t state);
+extern int mpc10x_resume(suspend_state_t state);
+#endif
+
 #endif
 #endif
diff --git a/include/asm-powerpc/mpc6xx.h b/include/asm-powerpc/mpc6xx.h
index 01a33ed..be86967 100644
--- a/include/asm-powerpc/mpc6xx.h
+++ b/include/asm-powerpc/mpc6xx.h
@@ -1,6 +1,6 @@
 #ifndef __ASM_POWERPC_MPC6xx_H
 #define __ASM_POWERPC_MPC6xx_H
 
-void mpc6xx_enter_sleep(void);
+extern void mpc6xx_enter_sleep(void);
 
 #endif

Re: [PATCH, RFC] linkstation: implement standby

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Sun, 26 Aug 2007 02:03:49 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>
> +static int ls_pm_enter(suspend_state_t state)
> +{
> +	char ier;
> +	int ret = 0;
> +	u64 tb;
> +
> +	if ((ret = mpc10x_suspend(state)) < 0)
> +		return ret;
> +
> +	/* Get timebase */
> +	tb = get_tb();
> +
> +	/* put CPU to sleep, re-enabling interrupts */
> +	mpc6xx_enter_sleep();
> +
> +	local_irq_disable();
> +
> +	set_tb(tb >> 32, tb & 0xfffffffful);
> +
> +fail:

Unused label?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH, RFC] linkstation: implement standby

[Guennadi Liakhovetski]

On Sun, 26 Aug 2007, Stephen Rothwell wrote:

> On Sun, 26 Aug 2007 02:03:49 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> >
> > +static int ls_pm_enter(suspend_state_t state)
> > +{
> > +	char ier;
> > +	int ret = 0;
> > +	u64 tb;
> > +
> > +	if ((ret = mpc10x_suspend(state)) < 0)
> > +		return ret;
> > +
> > +	/* Get timebase */
> > +	tb = get_tb();
> > +
> > +	/* put CPU to sleep, re-enabling interrupts */
> > +	mpc6xx_enter_sleep();
> > +
> > +	local_irq_disable();
> > +
> > +	set_tb(tb >> 32, tb & 0xfffffffful);
> > +
> > +fail:
> 
> Unused label?

Right, will fix in the next version, thanks.

Guennadi
---
Guennadi Liakhovetski

Re: [PATCH, RFC] linkstation: implement standby

[Tony Breeds]

On Sun, Aug 26, 2007 at 02:03:49AM +0200, Guennadi Liakhovetski wrote:
> Implement suspend/resume for "mpc10x" compatible fsl host PCI controllers, 
> use it for linkstation standby.

Hi Guennadi

<snip>

> +static int __init ls_pm_init(void)
> +{
> +	if (!machine_is(linkstation))
> +		return 0;

This should probably be -ENODEV.

<snip>

> +int mpc10x_suspend(suspend_state_t state)

You only seem to call this from ls_pm_enter() where you don't check the
return value, perhaps this should be void ?

> +{
> +	struct pci_dev *bridge;
> +	u16 pmcr1;
> +
> +	bridge = pci_find_slot(0, 0);
> +	if (!bridge)
> +		return -ENODEV;

I think you want pci_get_bus_and_slot() here instead of pci_find_slot(),
in fact I think it'd be cleaner, if you located the bridge in
ls_pm_enter() and passed it in.

> +int mpc10x_resume(suspend_state_t state)
> +{
> +	struct pci_dev *bridge;
> +	u16 pmcr1;
> +
> +	bridge = pci_find_slot(0, 0);
> +	if (!bridge)
> +		return -ENODEV;

Same comments ass mpc10x_suspend();

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [linux-pm] [PATCH, RFC] linkstation: implement standby

[Pavel Machek]

On Sun 2007-08-26 02:03:49, Guennadi Liakhovetski wrote:
> Implement suspend/resume for "mpc10x" compatible fsl host PCI controllers, 
> use it for linkstation standby.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Looks okay to me. I'm glad to see "standby" is present on non-i386
machines, too.
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH, RFC] linkstation: implement standby

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 158 bytes --]

On Sun, 2007-08-26 at 02:03 +0200, Guennadi Liakhovetski wrote:

> +#ifdef CONFIG_PM

some of those probably want to be CONFIG_PM_SLEEP now.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH, RFC] linkstation: implement standby

[Guennadi Liakhovetski]

Implement suspend/resume for "mpc10x" compatible fsl host PCI controllers, 
use it for linkstation standby. This is version 2, taking into account 
comments to the previous version and re-implementing MPC10x suspend in a 
separate pci driver.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

On Sun, 26 Aug 2007, Stephen Rothwell wrote:

> On Sun, 26 Aug 2007 02:03:49 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> >
> > +fail:
> 
> Unused label?

Removed.

On Mon, 27 Aug 2007, Tony Breeds wrote:

> On Sun, Aug 26, 2007 at 02:03:49AM +0200, Guennadi Liakhovetski wrote:
> 
> > +static int __init ls_pm_init(void)
> > +{
> > +	if (!machine_is(linkstation))
> > +		return 0;
> 
> This should probably be -ENODEV.

Fixed. The rest your comments are no longer relevant for this version, 
since mpc10x_{suspend,resume} functions are not called directly from 
linkstation.c, rather using late_suspend / early_resume (pci) hooks.

On Mon, 27 Aug 2007, Johannes Berg wrote:

> On Sun, 2007-08-26 at 02:03 +0200, Guennadi Liakhovetski wrote:
> 
> > +#ifdef CONFIG_PM
> 
> some of those probably want to be CONFIG_PM_SLEEP now.

Well, I wasn't sure when PM can be used not meaning PM_SLEEP, so, I left 
PM for now. Can certainly change, if it really matters.

A couple more thing:

1. the driver is called mpc10x.c, but the comment says it is for MPC8241. 
I don't know what PCI IDs other fsl chips have and which of them are 
compatible with 824[15].

2. the probe function, that just returns 0 seems to be redundant, but 
without it device-driver linking is incomplete and suspend/resume are not 
called.

3. the patch(es) from Scott Wood that are required for this one are still 
not merged, and I cannot remember having seen any comments / objections...

Thanks
Guennadi

diff --git a/arch/powerpc/platforms/embedded6xx/linkstation.c b/arch/powerpc/platforms/embedded6xx/linkstation.c
index ab9e3f9..eb214d0 100644
--- a/arch/powerpc/platforms/embedded6xx/linkstation.c
+++ b/arch/powerpc/platforms/embedded6xx/linkstation.c
@@ -196,3 +196,53 @@ define_machine(linkstation){
 	.halt	 		= linkstation_halt,
 	.calibrate_decr 	= generic_calibrate_decr,
 };
+
+#ifdef CONFIG_PM
+
+#include <linux/serial_reg.h>
+#include <sysdev/fsl_soc.h>
+#include <asm/mpc6xx.h>
+
+static int ls_pm_valid(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static int ls_pm_enter(suspend_state_t state)
+{
+	u64 tb;
+
+	/* Get timebase */
+	tb = get_tb();
+
+	/* put CPU to sleep, re-enabling interrupts */
+	mpc6xx_enter_sleep();
+
+	local_irq_disable();
+
+	set_tb(tb >> 32, tb & 0xfffffffful);
+
+	return 0;
+}
+
+static struct pm_ops ls_pm_ops = {
+	.valid		= ls_pm_valid,
+	.enter		= ls_pm_enter,
+};
+
+static int __init ls_pm_init(void)
+{
+	if (!machine_is(linkstation))
+		return -ENODEV;
+
+	pm_set_ops(&ls_pm_ops);
+	return 0;
+}
+
+device_initcall(ls_pm_init);
+#endif
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 7a1d6d5..4676cc8 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -39,3 +39,6 @@ config HT_IRQ
 	   This allows native hypertransport devices to use interrupts.
 
 	   If unsure say Y.
+
+config MPC10X_PM
+	def_bool FSL_SOC && PCI && PM
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 006054a..161e36b 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -43,6 +43,8 @@ obj-$(CONFIG_HOTPLUG) += setup-bus.o
 
 obj-$(CONFIG_PCI_SYSCALL) += syscall.o
 
+obj-$(CONFIG_MPC10X_PM) += mpc10x.o
+
 ifeq ($(CONFIG_PCI_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
 endif
diff --git a/drivers/pci/mpc10x.c b/drivers/pci/mpc10x.c
index e9891df..6bd773b 100644
--- a/drivers/pci/mpc10x.c
+++ b/drivers/pci/mpc10x.c
@@ -0,0 +1,73 @@
+/*
+ * Power-management driver for MPC8241 host-PCI controller
+ *
+ * Copyright (C) 2007 G. Liakhovetski (g.liakhovetski@gmx.de)
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of
+ * any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/pci.h>
+#include <linux/pm.h>
+
+#include <asm/pci-bridge.h>
+
+#define	MPC10X_LP_REF_EN	(1<<12)
+#define	MPC10X_PM		(1<<7)
+#define MPC10X_DOZE		(1<<5)
+#define	MPC10X_NAP		(1<<4)
+#define	MPC10X_SLEEP		(1<<3)
+
+static int __devinit mpc10x_pm_probe(struct pci_dev *pdev,
+				     const struct pci_device_id *id)
+{
+	return 0;
+}
+
+static int mpc10x_pm_suspend_late(struct pci_dev *dev, pm_message_t state)
+{
+	u16 pmcr1;
+
+	pci_read_config_word(dev, 0x70, &pmcr1);
+	pmcr1 &= ~(MPC10X_DOZE | MPC10X_NAP);
+	pmcr1 |= MPC10X_PM | MPC10X_SLEEP | MPC10X_LP_REF_EN;
+	pci_write_config_word(dev, 0x70, pmcr1);
+
+	return 0;
+}
+
+static int mpc10x_pm_resume_early(struct pci_dev *dev)
+{
+	u16 pmcr1;
+
+	pci_read_config_word(dev, 0x70, &pmcr1);
+	pmcr1 &= ~(MPC10X_PM | MPC10X_DOZE | MPC10X_SLEEP | MPC10X_NAP | MPC10X_LP_REF_EN);
+	pci_write_config_word(dev, 0x70, pmcr1);
+
+	return 0;
+}
+
+static struct pci_device_id mpc10x_pm_pci_tbl[] = {
+	{ PCI_VENDOR_ID_MOTOROLA, PCI_DEVICE_ID_MOTOROLA_MPC8241,
+	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, mpc10x_pm_pci_tbl);
+
+static struct pci_driver mpc10x_pm_driver = {
+	.name		= "mpc10x-pm",
+	.id_table	= mpc10x_pm_pci_tbl,
+	.resume_early	= mpc10x_pm_resume_early,
+	.suspend_late	= mpc10x_pm_suspend_late,
+	.probe		= mpc10x_pm_probe,
+};
+
+static int __init mpc10x_pm_init(void)
+{
+	return pci_register_driver(&mpc10x_pm_driver);
+}
+
+device_initcall(mpc10x_pm_init);
diff --git a/include/asm-powerpc/mpc6xx.h b/include/asm-powerpc/mpc6xx.h
index 01a33ed..be86967 100644
--- a/include/asm-powerpc/mpc6xx.h
+++ b/include/asm-powerpc/mpc6xx.h
@@ -1,6 +1,6 @@
 #ifndef __ASM_POWERPC_MPC6xx_H
 #define __ASM_POWERPC_MPC6xx_H
 
-void mpc6xx_enter_sleep(void);
+extern void mpc6xx_enter_sleep(void);
 
 #endif
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index b15c649..dfe9bd2 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -802,6 +802,7 @@
 #define PCI_DEVICE_ID_MOTOROLA_MPC105	0x0001
 #define PCI_DEVICE_ID_MOTOROLA_MPC106	0x0002
 #define PCI_DEVICE_ID_MOTOROLA_MPC107	0x0004
+#define PCI_DEVICE_ID_MOTOROLA_MPC8241	0x0006
 #define PCI_DEVICE_ID_MOTOROLA_RAVEN	0x4801
 #define PCI_DEVICE_ID_MOTOROLA_FALCON	0x4802
 #define PCI_DEVICE_ID_MOTOROLA_HAWK	0x4803

Re: [PATCH, RFC] linkstation: implement standby

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

On Wed, 2007-08-29 at 00:30 +0200, Guennadi Liakhovetski wrote:

> > > +#ifdef CONFIG_PM
> > 
> > some of those probably want to be CONFIG_PM_SLEEP now.
> 
> Well, I wasn't sure when PM can be used not meaning PM_SLEEP, so, I left 
> PM for now. Can certainly change, if it really matters.

You end up compiling more code than necessary if PM_SLEEP is disabled
but PM is enabled (i.e. runtime powermanagement enabled but no sleep
states)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]