[PATCH 00/04][RFC] PM: Runtime platform device PM

[PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

PM: Runtime platform device power management

[PATCH 01/04] Driver Core: Add platform device arch data
[PATCH 02/04] Driver Core: Add idle and wakeup functions
[PATCH 03/04] PM: Add platform bus runtime dev_pm_ops
[PATCH 04/04] sh: Runtime platform device PM mockup

These patches extend platform device and driver interfaces to
allow architectures to implement platform device runtime pm.

Upstream runtime power management needs to be improved to fully
make use of hardware power saving features found on embedded
platforms and in common SoCs.

For runtime power management we today have cpuidle, the clock
framework and qos. This allows the cpu core to enter various
forms of deep sleep, and for devices we may stop clocks to save
power. Modern SoCs however allow disabling of power to parts of
the chip, and we have no upstream interface to handle that today.

I propose adding the following simple platform device functions:
 - platform_device_wakeup()
 - platform_device_idle()

The idle function is used by the platform driver to let the 
architecture power management code know that from now on the
device is in idle state. When the device is marked as idle
the architecture specific runtime power management may decide
to do various levels of device power management, ranging from
stopping clocks to turning off power. The dev_pm_ops callbacks
may be invoked by the runtime pm code to save and restore state
whenever the device is marked as idle.

The wakeup function is used by the platform driver to notify the
architecture code that the driver wants to make use of the hardware
device. If the device has been put in sleep then it needs to be
woken up. This wakeup call may invoke dev_pm_ops callbacks.

Have a look at the last patch for a SuperH mockup that shows how
it all fits together. We need this or a similar interface to be able
to enter deep sleep states on SuperH. I believe other architectures
have similar requirements.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 arch/Kconfig                               |    9 +
 arch/sh/Kconfig                            |    3 
 arch/sh/boards/mach-migor/setup.c          |    9 +
 arch/sh/include/asm/device.h               |    6 
 arch/sh/include/cpu-sh4/cpu/sh7722.h       |   14 ++
 arch/sh/kernel/cpu/sh4a/setup-sh7722.c     |   31 ++++
 arch/sh/kernel/cpu/shmobile/pm.c           |  118 +++++++++++++++++
 drivers/base/platform.c                    |  193 +++++++++++++++++++++++++++-
 drivers/clocksource/sh_cmt.c               |    2 
 drivers/clocksource/sh_tmu.c               |    2 
 drivers/i2c/busses/i2c-sh_mobile.c         |    2 
 drivers/media/video/sh_mobile_ceu_camera.c |    2 
 drivers/video/sh_mobile_lcdcfb.c           |    4 
 include/linux/platform_device.h            |   21 +++
 14 files changed, 415 insertions(+), 1 deletion(-)

[PATCH 01/04] Driver Core: Add platform device arch data

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Allow architecture specific data in struct platform_device.
Platform device data is needed by the runtime pm code. A similar
struct already exists for struct device.

The architecture specific asm/device.h file needs to provide
struct pdev_archdata if CONFIG_HAVE_PLATFORM_DEVICE_DATA is set.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Optionally we can make use of struct device archdata instead,
 but since the runtime device pm is limited to platform devices
 it makes sense to make this data platform device specific imo.

 arch/Kconfig                    |    3 +++
 include/linux/platform_device.h |    5 +++++
 2 files changed, 8 insertions(+)

--- 0001/arch/Kconfig
+++ work/arch/Kconfig	2009-05-15 14:51:35.000000000 +0900
@@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
 
 config HAVE_DEFAULT_NO_SPIN_MUTEXES
 	bool
+
+config HAVE_PLATFORM_DEVICE_ARCHDATA
+	bool
--- 0001/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-05-15 16:59:01.000000000 +0900
@@ -23,6 +23,11 @@ struct platform_device {
 	void		*platform_data;
 
 	struct platform_device_id	*id_entry;
+
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_ARCHDATA
+	/* arch specific additions */
+	struct pdev_archdata	archdata;
+#endif
 };
 
 #define platform_get_device_id(pdev)	((pdev)->id_entry)

[PATCH 01/04] Driver Core: Add platform device arch data

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Allow architecture specific data in struct platform_device.
Platform device data is needed by the runtime pm code. A similar
struct already exists for struct device.

The architecture specific asm/device.h file needs to provide
struct pdev_archdata if CONFIG_HAVE_PLATFORM_DEVICE_DATA is set.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Optionally we can make use of struct device archdata instead,
 but since the runtime device pm is limited to platform devices
 it makes sense to make this data platform device specific imo.

 arch/Kconfig                    |    3 +++
 include/linux/platform_device.h |    5 +++++
 2 files changed, 8 insertions(+)

--- 0001/arch/Kconfig
+++ work/arch/Kconfig	2009-05-15 14:51:35.000000000 +0900
@@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
 
 config HAVE_DEFAULT_NO_SPIN_MUTEXES
 	bool
+
+config HAVE_PLATFORM_DEVICE_ARCHDATA
+	bool
--- 0001/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-05-15 16:59:01.000000000 +0900
@@ -23,6 +23,11 @@ struct platform_device {
 	void		*platform_data;
 
 	struct platform_device_id	*id_entry;
+
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_ARCHDATA
+	/* arch specific additions */
+	struct pdev_archdata	archdata;
+#endif
 };
 
 #define platform_get_device_id(pdev)	((pdev)->id_entry)

[PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Add platform_device_idle() and platform_device_wakeup()
and allow architectures to implement their own versions
of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Yes, this needs documentation. See [00/04] for now.

 arch/Kconfig                    |    3 +++
 include/linux/platform_device.h |    8 ++++++++
 2 files changed, 11 insertions(+)

--- 0002/arch/Kconfig
+++ work/arch/Kconfig	2009-05-26 12:31:06.000000000 +0900
@@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
 
 config HAVE_PLATFORM_DEVICE_ARCHDATA
 	bool
+
+config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
+	bool
--- 0002/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-05-26 12:32:40.000000000 +0900
@@ -57,6 +57,14 @@ extern int platform_device_add(struct pl
 extern void platform_device_del(struct platform_device *pdev);
 extern void platform_device_put(struct platform_device *pdev);
 
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
+void platform_device_idle(struct platform_device *pdev);
+void platform_device_wakeup(struct platform_device *pdev);
+#else
+static inline void platform_device_idle(struct platform_device *pdev) {}
+static inline void platform_device_wakeup(struct platform_device *pdev) {}
+#endif
+
 struct platform_driver {
 	int (*probe)(struct platform_device *);
 	int (*remove)(struct platform_device *);

[PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Add platform_device_idle() and platform_device_wakeup()
and allow architectures to implement their own versions
of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Yes, this needs documentation. See [00/04] for now.

 arch/Kconfig                    |    3 +++
 include/linux/platform_device.h |    8 ++++++++
 2 files changed, 11 insertions(+)

--- 0002/arch/Kconfig
+++ work/arch/Kconfig	2009-05-26 12:31:06.000000000 +0900
@@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
 
 config HAVE_PLATFORM_DEVICE_ARCHDATA
 	bool
+
+config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
+	bool
--- 0002/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-05-26 12:32:40.000000000 +0900
@@ -57,6 +57,14 @@ extern int platform_device_add(struct pl
 extern void platform_device_del(struct platform_device *pdev);
 extern void platform_device_put(struct platform_device *pdev);
 
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
+void platform_device_idle(struct platform_device *pdev);
+void platform_device_wakeup(struct platform_device *pdev);
+#else
+static inline void platform_device_idle(struct platform_device *pdev) {}
+static inline void platform_device_wakeup(struct platform_device *pdev) {}
+#endif
+
 struct platform_driver {
 	int (*probe)(struct platform_device *);
 	int (*remove)(struct platform_device *);

[PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Wrap the platform device bus dev_pm_ops to allow runtime
pm and regular suspend and resume to coexist.

Platform device data is extended with flags that allow
us to keep track of which dev_pm_ops that has been called.

Basically, if a device has been frozen by the runtime pm
code, don't call ->freeze() again when hibernating.

Architecture code can use platform_runtime_dev_pm_ops to
call driver dev_pm_ops associated with a certain device.

Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 This is a bit of a hack, any better way to wrap dev_pm_ops?

 arch/Kconfig                    |    3 
 drivers/base/platform.c         |  193 ++++++++++++++++++++++++++++++++++++++-
 include/linux/platform_device.h |    8 +
 3 files changed, 203 insertions(+), 1 deletion(-)

--- 0003/arch/Kconfig
+++ work/arch/Kconfig	2009-05-26 21:22:17.000000000 +0900
@@ -118,3 +118,6 @@ config HAVE_PLATFORM_DEVICE_ARCHDATA
 
 config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
 	bool
+
+config HAVE_PLATFORM_DEVICE_RUNTIME_PM
+	bool
--- 0001/drivers/base/platform.c
+++ work/drivers/base/platform.c	2009-05-27 17:31:06.000000000 +0900
@@ -962,12 +962,203 @@ static struct dev_pm_ops platform_dev_pm
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM
+
+#define DEV_PM_OP_PREPARE offsetof(struct dev_pm_ops, prepare)
+#define DEV_PM_OP_COMPLETE offsetof(struct dev_pm_ops, complete)
+#define DEV_PM_OP_SUSPEND offsetof(struct dev_pm_ops, suspend)
+#define DEV_PM_OP_RESUME offsetof(struct dev_pm_ops, resume)
+#define DEV_PM_OP_FREEZE offsetof(struct dev_pm_ops, freeze)
+#define DEV_PM_OP_THAW offsetof(struct dev_pm_ops, thaw)
+#define DEV_PM_OP_POWEROFF offsetof(struct dev_pm_ops, poweroff)
+#define DEV_PM_OP_RESTORE offsetof(struct dev_pm_ops, restore)
+#define DEV_PM_OP_SUSPEND_NOIRQ offsetof(struct dev_pm_ops, suspend_noirq)
+#define DEV_PM_OP_RESUME_NOIRQ offsetof(struct dev_pm_ops, resume_noirq)
+#define DEV_PM_OP_FREEZE_NOIRQ offsetof(struct dev_pm_ops, freeze_noirq)
+#define DEV_PM_OP_THAW_NOIRQ offsetof(struct dev_pm_ops, thaw_noirq)
+#define DEV_PM_OP_POWEROFF_NOIRQ offsetof(struct dev_pm_ops, poweroff_noirq)
+#define DEV_PM_OP_RESTORE_NOIRQ offsetof(struct dev_pm_ops, restore_noirq)
+
+static DEFINE_SPINLOCK(platform_runtime_lock);
+
+static int platform_runtime_call_op(struct device *dev, unsigned int op)
+{
+	struct dev_pm_ops *dev_pm_ops = PLATFORM_PM_OPS_PTR;
+	void **vp = (void **)dev_pm_ops;
+	int (*int_op)(struct device *);
+
+	if (dev_pm_ops) {
+		if (op = DEV_PM_OP_COMPLETE) {
+			if (dev_pm_ops->complete)
+				dev_pm_ops->complete(dev);
+		} else {
+			int_op = vp[op / sizeof(void *)];
+			if (int_op)
+				return int_op(dev);
+		}
+	}
+
+	return 0;
+}
+
+static int platform_runtime_call_once(struct device *dev,
+				      unsigned int op)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	unsigned long flags;
+	int bit, bit_op, is_suspend, ret;
+
+	ret = 0;
+	is_suspend = 0;
+
+	switch (op) {
+	case DEV_PM_OP_COMPLETE:
+		bit_op = DEV_PM_OP_PREPARE;
+		break;
+	case DEV_PM_OP_RESUME:
+		bit_op = DEV_PM_OP_SUSPEND;
+		break;
+	case DEV_PM_OP_THAW:
+		bit_op = DEV_PM_OP_FREEZE;
+		break;
+	case DEV_PM_OP_RESTORE:
+		bit_op = DEV_PM_OP_POWEROFF;
+		break;
+	case DEV_PM_OP_RESUME_NOIRQ:
+		bit_op = DEV_PM_OP_SUSPEND_NOIRQ;
+		break;
+	case DEV_PM_OP_THAW_NOIRQ:
+		bit_op = DEV_PM_OP_FREEZE_NOIRQ;
+		break;
+	case DEV_PM_OP_RESTORE_NOIRQ:
+		bit_op = DEV_PM_OP_POWEROFF_NOIRQ;
+		break;
+	default:
+		bit_op = op;
+		is_suspend = 1;
+	}
+
+	bit = bit_op / sizeof(void *);
+
+	spin_lock_irqsave(&platform_runtime_lock, flags);
+
+	if (test_bit(bit, &pdev->runtime_flags) != is_suspend) {
+		ret = platform_runtime_call_op(dev, op);
+
+		if (!ret) {
+			if (is_suspend)
+				__set_bit(bit, &pdev->runtime_flags);
+			else
+				__clear_bit(bit, &pdev->runtime_flags);
+		}
+	}
+
+	spin_unlock_irqrestore(&platform_runtime_lock, flags);
+
+	return ret;
+}
+
+static int platform_runtime_prepare(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_PREPARE);
+}
+
+static void platform_runtime_complete(struct device *dev)
+{
+	platform_runtime_call_once(dev, DEV_PM_OP_COMPLETE);
+}
+
+static int platform_runtime_suspend(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_SUSPEND);
+}
+
+static int platform_runtime_resume(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESUME);
+}
+
+static int platform_runtime_freeze(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_FREEZE);
+}
+
+static int platform_runtime_thaw(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_THAW);
+}
+
+static int platform_runtime_poweroff(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_POWEROFF);
+}
+
+static int platform_runtime_restore(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESTORE);
+}
+
+static int platform_runtime_suspend_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_SUSPEND_NOIRQ);
+}
+
+static int platform_runtime_resume_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESUME_NOIRQ);
+}
+
+static int platform_runtime_freeze_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_FREEZE_NOIRQ);
+}
+
+static int platform_runtime_thaw_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_THAW_NOIRQ);
+}
+
+static int platform_runtime_poweroff_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_POWEROFF_NOIRQ);
+}
+
+static int platform_runtime_restore_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESTORE_NOIRQ);
+}
+
+struct dev_pm_ops platform_runtime_dev_pm_ops = {
+	.prepare = platform_runtime_prepare,
+	.complete = platform_runtime_complete,
+	.suspend = platform_runtime_suspend,
+	.resume = platform_runtime_resume,
+	.freeze = platform_runtime_freeze,
+	.thaw = platform_runtime_thaw,
+	.poweroff = platform_runtime_poweroff,
+	.restore = platform_runtime_restore,
+	.suspend_noirq = platform_runtime_suspend_noirq,
+	.resume_noirq = platform_runtime_resume_noirq,
+	.freeze_noirq = platform_runtime_freeze_noirq,
+	.thaw_noirq = platform_runtime_thaw_noirq,
+	.poweroff_noirq = platform_runtime_poweroff_noirq,
+	.restore_noirq = platform_runtime_restore_noirq,
+};
+
+#define PLATFORM_RUNTIME_PM_OPS_PTR	(&platform_runtime_dev_pm_ops)
+
+#else /* !CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM */
+
+#define PLATFORM_RUNTIME_PM_OPS_PTR	PLATFORM_PM_OPS_PTR
+
+#endif /* !CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM */
+
 struct bus_type platform_bus_type = {
 	.name		= "platform",
 	.dev_attrs	= platform_dev_attrs,
 	.match		= platform_match,
 	.uevent		= platform_uevent,
-	.pm		= PLATFORM_PM_OPS_PTR,
+	.pm		= PLATFORM_RUNTIME_PM_OPS_PTR,
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
--- 0003/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-05-26 21:22:17.000000000 +0900
@@ -27,6 +27,10 @@ struct platform_device {
 	/* arch specific additions */
 	struct pdev_archdata	archdata;
 #endif
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM
+	unsigned long runtime_flags;
+#endif
+
 };
 
 #define platform_get_device_id(pdev)	((pdev)->id_entry)
@@ -65,6 +69,10 @@ static inline void platform_device_idle(
 static inline void platform_device_wakeup(struct platform_device *pdev) {}
 #endif
 
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM
+extern struct dev_pm_ops platform_runtime_dev_pm_ops;
+#endif
+
 struct platform_driver {
 	int (*probe)(struct platform_device *);
 	int (*remove)(struct platform_device *);

[PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Wrap the platform device bus dev_pm_ops to allow runtime
pm and regular suspend and resume to coexist.

Platform device data is extended with flags that allow
us to keep track of which dev_pm_ops that has been called.

Basically, if a device has been frozen by the runtime pm
code, don't call ->freeze() again when hibernating.

Architecture code can use platform_runtime_dev_pm_ops to
call driver dev_pm_ops associated with a certain device.

Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 This is a bit of a hack, any better way to wrap dev_pm_ops?

 arch/Kconfig                    |    3 
 drivers/base/platform.c         |  193 ++++++++++++++++++++++++++++++++++++++-
 include/linux/platform_device.h |    8 +
 3 files changed, 203 insertions(+), 1 deletion(-)

--- 0003/arch/Kconfig
+++ work/arch/Kconfig	2009-05-26 21:22:17.000000000 +0900
@@ -118,3 +118,6 @@ config HAVE_PLATFORM_DEVICE_ARCHDATA
 
 config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
 	bool
+
+config HAVE_PLATFORM_DEVICE_RUNTIME_PM
+	bool
--- 0001/drivers/base/platform.c
+++ work/drivers/base/platform.c	2009-05-27 17:31:06.000000000 +0900
@@ -962,12 +962,203 @@ static struct dev_pm_ops platform_dev_pm
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM
+
+#define DEV_PM_OP_PREPARE offsetof(struct dev_pm_ops, prepare)
+#define DEV_PM_OP_COMPLETE offsetof(struct dev_pm_ops, complete)
+#define DEV_PM_OP_SUSPEND offsetof(struct dev_pm_ops, suspend)
+#define DEV_PM_OP_RESUME offsetof(struct dev_pm_ops, resume)
+#define DEV_PM_OP_FREEZE offsetof(struct dev_pm_ops, freeze)
+#define DEV_PM_OP_THAW offsetof(struct dev_pm_ops, thaw)
+#define DEV_PM_OP_POWEROFF offsetof(struct dev_pm_ops, poweroff)
+#define DEV_PM_OP_RESTORE offsetof(struct dev_pm_ops, restore)
+#define DEV_PM_OP_SUSPEND_NOIRQ offsetof(struct dev_pm_ops, suspend_noirq)
+#define DEV_PM_OP_RESUME_NOIRQ offsetof(struct dev_pm_ops, resume_noirq)
+#define DEV_PM_OP_FREEZE_NOIRQ offsetof(struct dev_pm_ops, freeze_noirq)
+#define DEV_PM_OP_THAW_NOIRQ offsetof(struct dev_pm_ops, thaw_noirq)
+#define DEV_PM_OP_POWEROFF_NOIRQ offsetof(struct dev_pm_ops, poweroff_noirq)
+#define DEV_PM_OP_RESTORE_NOIRQ offsetof(struct dev_pm_ops, restore_noirq)
+
+static DEFINE_SPINLOCK(platform_runtime_lock);
+
+static int platform_runtime_call_op(struct device *dev, unsigned int op)
+{
+	struct dev_pm_ops *dev_pm_ops = PLATFORM_PM_OPS_PTR;
+	void **vp = (void **)dev_pm_ops;
+	int (*int_op)(struct device *);
+
+	if (dev_pm_ops) {
+		if (op == DEV_PM_OP_COMPLETE) {
+			if (dev_pm_ops->complete)
+				dev_pm_ops->complete(dev);
+		} else {
+			int_op = vp[op / sizeof(void *)];
+			if (int_op)
+				return int_op(dev);
+		}
+	}
+
+	return 0;
+}
+
+static int platform_runtime_call_once(struct device *dev,
+				      unsigned int op)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	unsigned long flags;
+	int bit, bit_op, is_suspend, ret;
+
+	ret = 0;
+	is_suspend = 0;
+
+	switch (op) {
+	case DEV_PM_OP_COMPLETE:
+		bit_op = DEV_PM_OP_PREPARE;
+		break;
+	case DEV_PM_OP_RESUME:
+		bit_op = DEV_PM_OP_SUSPEND;
+		break;
+	case DEV_PM_OP_THAW:
+		bit_op = DEV_PM_OP_FREEZE;
+		break;
+	case DEV_PM_OP_RESTORE:
+		bit_op = DEV_PM_OP_POWEROFF;
+		break;
+	case DEV_PM_OP_RESUME_NOIRQ:
+		bit_op = DEV_PM_OP_SUSPEND_NOIRQ;
+		break;
+	case DEV_PM_OP_THAW_NOIRQ:
+		bit_op = DEV_PM_OP_FREEZE_NOIRQ;
+		break;
+	case DEV_PM_OP_RESTORE_NOIRQ:
+		bit_op = DEV_PM_OP_POWEROFF_NOIRQ;
+		break;
+	default:
+		bit_op = op;
+		is_suspend = 1;
+	}
+
+	bit = bit_op / sizeof(void *);
+
+	spin_lock_irqsave(&platform_runtime_lock, flags);
+
+	if (test_bit(bit, &pdev->runtime_flags) != is_suspend) {
+		ret = platform_runtime_call_op(dev, op);
+
+		if (!ret) {
+			if (is_suspend)
+				__set_bit(bit, &pdev->runtime_flags);
+			else
+				__clear_bit(bit, &pdev->runtime_flags);
+		}
+	}
+
+	spin_unlock_irqrestore(&platform_runtime_lock, flags);
+
+	return ret;
+}
+
+static int platform_runtime_prepare(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_PREPARE);
+}
+
+static void platform_runtime_complete(struct device *dev)
+{
+	platform_runtime_call_once(dev, DEV_PM_OP_COMPLETE);
+}
+
+static int platform_runtime_suspend(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_SUSPEND);
+}
+
+static int platform_runtime_resume(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESUME);
+}
+
+static int platform_runtime_freeze(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_FREEZE);
+}
+
+static int platform_runtime_thaw(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_THAW);
+}
+
+static int platform_runtime_poweroff(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_POWEROFF);
+}
+
+static int platform_runtime_restore(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESTORE);
+}
+
+static int platform_runtime_suspend_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_SUSPEND_NOIRQ);
+}
+
+static int platform_runtime_resume_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESUME_NOIRQ);
+}
+
+static int platform_runtime_freeze_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_FREEZE_NOIRQ);
+}
+
+static int platform_runtime_thaw_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_THAW_NOIRQ);
+}
+
+static int platform_runtime_poweroff_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_POWEROFF_NOIRQ);
+}
+
+static int platform_runtime_restore_noirq(struct device *dev)
+{
+	return platform_runtime_call_once(dev, DEV_PM_OP_RESTORE_NOIRQ);
+}
+
+struct dev_pm_ops platform_runtime_dev_pm_ops = {
+	.prepare = platform_runtime_prepare,
+	.complete = platform_runtime_complete,
+	.suspend = platform_runtime_suspend,
+	.resume = platform_runtime_resume,
+	.freeze = platform_runtime_freeze,
+	.thaw = platform_runtime_thaw,
+	.poweroff = platform_runtime_poweroff,
+	.restore = platform_runtime_restore,
+	.suspend_noirq = platform_runtime_suspend_noirq,
+	.resume_noirq = platform_runtime_resume_noirq,
+	.freeze_noirq = platform_runtime_freeze_noirq,
+	.thaw_noirq = platform_runtime_thaw_noirq,
+	.poweroff_noirq = platform_runtime_poweroff_noirq,
+	.restore_noirq = platform_runtime_restore_noirq,
+};
+
+#define PLATFORM_RUNTIME_PM_OPS_PTR	(&platform_runtime_dev_pm_ops)
+
+#else /* !CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM */
+
+#define PLATFORM_RUNTIME_PM_OPS_PTR	PLATFORM_PM_OPS_PTR
+
+#endif /* !CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM */
+
 struct bus_type platform_bus_type = {
 	.name		= "platform",
 	.dev_attrs	= platform_dev_attrs,
 	.match		= platform_match,
 	.uevent		= platform_uevent,
-	.pm		= PLATFORM_PM_OPS_PTR,
+	.pm		= PLATFORM_RUNTIME_PM_OPS_PTR,
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
--- 0003/include/linux/platform_device.h
+++ work/include/linux/platform_device.h	2009-05-26 21:22:17.000000000 +0900
@@ -27,6 +27,10 @@ struct platform_device {
 	/* arch specific additions */
 	struct pdev_archdata	archdata;
 #endif
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM
+	unsigned long runtime_flags;
+#endif
+
 };
 
 #define platform_get_device_id(pdev)	((pdev)->id_entry)
@@ -65,6 +69,10 @@ static inline void platform_device_idle(
 static inline void platform_device_wakeup(struct platform_device *pdev) {}
 #endif
 
+#ifdef CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM
+extern struct dev_pm_ops platform_runtime_dev_pm_ops;
+#endif
+
 struct platform_driver {
 	int (*probe)(struct platform_device *);
 	int (*remove)(struct platform_device *);

[PATCH 04/04] sh: Runtime platform device PM mockup

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Make use of platform device runtime pm functions.

The patch contains changes in the following order:
1) Kconfig stuff, enables patch [01/04] to [03/04]
2) SuperH specific platform device data and harware block id
3) Mark platform devices with hardware block id
4) Change a few drivers to use idle and wakeup (todo: replace clocks)
5) Runtime PM mockup using delayed work to freeze devices.

The code does not really do anything useful apart from printing out
which devices to freeze and wakeup. The freezing may be driven from
cpuidle demand instead. Locking needs more work.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 arch/sh/Kconfig                            |    3 
 arch/sh/boards/mach-migor/setup.c          |    9 ++
 arch/sh/include/asm/device.h               |    6 +
 arch/sh/include/cpu-sh4/cpu/sh7722.h       |   14 +++
 arch/sh/kernel/cpu/sh4a/setup-sh7722.c     |   31 +++++++
 arch/sh/kernel/cpu/shmobile/pm.c           |  118 ++++++++++++++++++++++++++++
 drivers/clocksource/sh_cmt.c               |    2 
 drivers/clocksource/sh_tmu.c               |    2 
 drivers/i2c/busses/i2c-sh_mobile.c         |    2 
 drivers/media/video/sh_mobile_ceu_camera.c |    2 
 drivers/video/sh_mobile_lcdcfb.c           |    4 
 11 files changed, 193 insertions(+)

--- 0001/arch/sh/Kconfig
+++ work/arch/sh/Kconfig	2009-05-26 21:22:23.000000000 +0900
@@ -16,6 +16,7 @@ config SUPERH
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_DMA_API_DEBUG
 	select RTC_LIB
+	select HAVE_PLATFORM_DEVICE_ARCHDATA
 	help
 	  The SuperH is a RISC processor targeted for use in embedded systems
 	  and consumer electronics; it was also used in the Sega Dreamcast
@@ -201,6 +202,8 @@ config CPU_SHX3
 config ARCH_SHMOBILE
 	bool
 	select ARCH_SUSPEND_POSSIBLE
+	select HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
+	select HAVE_PLATFORM_DEVICE_RUNTIME_PM
 
 if SUPERH32
 
--- 0001/arch/sh/include/asm/device.h
+++ work/arch/sh/include/asm/device.h	2009-05-26 21:22:23.000000000 +0900
@@ -12,3 +12,9 @@ int platform_resource_setup_memory(struc
 
 void plat_early_device_setup(void);
 
+struct pdev_archdata {
+#ifdef CONFIG_ARCH_SHMOBILE
+	int hw_blk_id;
+	struct list_head entry;
+#endif
+};
--- 0001/arch/sh/include/cpu-sh4/cpu/sh7722.h
+++ work/arch/sh/include/cpu-sh4/cpu/sh7722.h	2009-05-26 21:31:31.000000000 +0900
@@ -207,4 +207,18 @@ enum {
 	GPIO_FN_KEYOUT3, GPIO_FN_KEYOUT4_IN6, GPIO_FN_KEYOUT5_IN5,
 };
 
+enum {
+	HW_BLK_UNKNOWN=0,
+	HW_BLK_URAM, HW_BLK_XYMEM,
+	HW_BLK_TMU0, HW_BLK_TMU1, HW_BLK_TMU2,
+	HW_BLK_CMT, HW_BLK_RWDT, HW_BLK_FLCTL,
+	HW_BLK_SCIF0, HW_BLK_SCIF1, HW_BLK_SCIF2,
+	HW_BLK_IIC, HW_BLK_RTC, HW_BLK_SDHI,
+	HW_BLK_KEYSC, HW_BLK_USBF, HW_BLK_2DG,
+	HW_BLK_SIU, HW_BLK_VOU, HW_BLK_JPU,
+	HW_BLK_BEU, HW_BLK_CEU, HW_BLK_VEU,
+	HW_BLK_VPU, HW_BLK_LCDC,
+	HW_BLK_NR,
+};
+
 #endif /* __ASM_SH7722_H__ */
--- 0001/arch/sh/boards/mach-migor/setup.c
+++ work/arch/sh/boards/mach-migor/setup.c	2009-05-26 21:22:34.000000000 +0900
@@ -98,6 +98,9 @@ static struct platform_device sh_keysc_d
 	.dev	= {
 		.platform_data	= &sh_keysc_info,
 	},
+	.archdata = {
+		.hw_blk_id = HW_BLK_KEYSC,
+	},
 };
 
 static struct mtd_partition migor_nor_flash_partitions[] =
@@ -292,6 +295,9 @@ static struct platform_device migor_lcdc
 	.dev	= {
 		.platform_data	= &sh_mobile_lcdc_info,
 	},
+	.archdata = {
+		.hw_blk_id = HW_BLK_LCDC,
+	},
 };
 
 static struct clk *camera_clk;
@@ -379,6 +385,9 @@ static struct platform_device migor_ceu_
 	.dev	= {
 		.platform_data	= &sh_mobile_ceu_info,
 	},
+	.archdata = {
+		.hw_blk_id = HW_BLK_CEU,
+	},
 };
 
 static struct ov772x_camera_info ov7725_info = {
--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7722.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7722.c	2009-05-26 21:22:23.000000000 +0900
@@ -16,6 +16,7 @@
 #include <linux/sh_timer.h>
 #include <asm/clock.h>
 #include <asm/mmzone.h>
+#include <cpu/sh7722.h>
 
 static struct resource rtc_resources[] = {
 	[0] = {
@@ -45,6 +46,9 @@ static struct platform_device rtc_device
 	.id		= -1,
 	.num_resources	= ARRAY_SIZE(rtc_resources),
 	.resource	= rtc_resources,
+	.archdata = {
+		.hw_blk_id = HW_BLK_RTC,
+	},
 };
 
 static struct resource usbf_resources[] = {
@@ -70,6 +74,9 @@ static struct platform_device usbf_devic
 	},
 	.num_resources	= ARRAY_SIZE(usbf_resources),
 	.resource	= usbf_resources,
+	.archdata = {
+		.hw_blk_id = HW_BLK_USBF,
+	},
 };
 
 static struct resource iic_resources[] = {
@@ -91,6 +98,9 @@ static struct platform_device iic_device
 	.id             = 0, /* "i2c0" clock */
 	.num_resources  = ARRAY_SIZE(iic_resources),
 	.resource       = iic_resources,
+	.archdata = {
+		.hw_blk_id = HW_BLK_IIC,
+	},
 };
 
 static struct uio_info vpu_platform_data = {
@@ -119,6 +129,9 @@ static struct platform_device vpu_device
 	},
 	.resource	= vpu_resources,
 	.num_resources	= ARRAY_SIZE(vpu_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_VPU,
+	},
 };
 
 static struct uio_info veu_platform_data = {
@@ -147,6 +160,9 @@ static struct platform_device veu_device
 	},
 	.resource	= veu_resources,
 	.num_resources	= ARRAY_SIZE(veu_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_VEU,
+	},
 };
 
 static struct uio_info jpu_platform_data = {
@@ -175,6 +191,9 @@ static struct platform_device jpu_device
 	},
 	.resource	= jpu_resources,
 	.num_resources	= ARRAY_SIZE(jpu_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_JPU,
+	},
 };
 
 static struct sh_timer_config cmt_platform_data = {
@@ -207,6 +226,9 @@ static struct platform_device cmt_device
 	},
 	.resource	= cmt_resources,
 	.num_resources	= ARRAY_SIZE(cmt_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_CMT,
+	},
 };
 
 static struct sh_timer_config tmu0_platform_data = {
@@ -238,6 +260,9 @@ static struct platform_device tmu0_devic
 	},
 	.resource	= tmu0_resources,
 	.num_resources	= ARRAY_SIZE(tmu0_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_TMU0,
+	},
 };
 
 static struct sh_timer_config tmu1_platform_data = {
@@ -269,6 +294,9 @@ static struct platform_device tmu1_devic
 	},
 	.resource	= tmu1_resources,
 	.num_resources	= ARRAY_SIZE(tmu1_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_TMU1,
+	},
 };
 
 static struct sh_timer_config tmu2_platform_data = {
@@ -299,6 +327,9 @@ static struct platform_device tmu2_devic
 	},
 	.resource	= tmu2_resources,
 	.num_resources	= ARRAY_SIZE(tmu2_resources),
+	.archdata = {
+		.hw_blk_id = HW_BLK_TMU2,
+	},
 };
 
 static struct plat_sci_port sci_platform_data[] = {
--- 0001/drivers/clocksource/sh_cmt.c
+++ work/drivers/clocksource/sh_cmt.c	2009-05-26 21:22:23.000000000 +0900
@@ -153,6 +153,7 @@ static int sh_cmt_enable(struct sh_cmt_p
 	int ret;
 
 	/* enable clock */
+	platform_device_wakeup(p->pdev);
 	ret = clk_enable(p->clk);
 	if (ret) {
 		pr_err("sh_cmt: cannot enable clock \"%s\"\n", cfg->clk);
@@ -186,6 +187,7 @@ static void sh_cmt_disable(struct sh_cmt
 
 	/* stop clock */
 	clk_disable(p->clk);
+	platform_device_idle(p->pdev);
 }
 
 /* private flags */
--- 0001/drivers/clocksource/sh_tmu.c
+++ work/drivers/clocksource/sh_tmu.c	2009-05-26 21:22:23.000000000 +0900
@@ -110,6 +110,7 @@ static int sh_tmu_enable(struct sh_tmu_p
 	int ret;
 
 	/* enable clock */
+	platform_device_wakeup(p->pdev);
 	ret = clk_enable(p->clk);
 	if (ret) {
 		pr_err("sh_tmu: cannot enable clock \"%s\"\n", cfg->clk);
@@ -140,6 +141,7 @@ static void sh_tmu_disable(struct sh_tmu
 
 	/* stop clock */
 	clk_disable(p->clk);
+	platform_device_idle(p->pdev);
 }
 
 static void sh_tmu_set_next(struct sh_tmu_priv *p, unsigned long delta,
--- 0001/drivers/i2c/busses/i2c-sh_mobile.c
+++ work/drivers/i2c/busses/i2c-sh_mobile.c	2009-05-26 21:30:56.000000000 +0900
@@ -166,6 +166,7 @@ static void activate_ch(struct sh_mobile
 	u_int32_t tmp;
 
 	/* Make sure the clock is enabled */
+	platform_device_wakeup(to_platform_device(pd->dev));
 	clk_enable(pd->clk);
 
 	/* Get clock rate after clock is enabled */
@@ -215,6 +216,7 @@ static void deactivate_ch(struct sh_mobi
 
 	/* Disable clock */
 	clk_disable(pd->clk);
+	platform_device_idle(to_platform_device(pd->dev));
 }
 
 static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
--- 0001/drivers/media/video/sh_mobile_ceu_camera.c
+++ work/drivers/media/video/sh_mobile_ceu_camera.c	2009-05-26 21:22:23.000000000 +0900
@@ -364,6 +364,7 @@ static int sh_mobile_ceu_add_device(stru
 	if (ret)
 		goto err;
 
+	platform_device_wakeup(to_platform_device(ici->dev));
 	clk_enable(pcdev->clk);
 
 	ceu_write(pcdev, CAPSR, 1 << 16); /* reset */
@@ -399,6 +400,7 @@ static void sh_mobile_ceu_remove_device(
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 
 	clk_disable(pcdev->clk);
+	platform_device_idle(to_platform_device(pcdev->ici.dev));
 
 	icd->ops->release(icd);
 
--- 0001/drivers/video/sh_mobile_lcdcfb.c
+++ work/drivers/video/sh_mobile_lcdcfb.c	2009-05-26 21:30:39.000000000 +0900
@@ -38,6 +38,7 @@ struct sh_mobile_lcdc_chan {
 };
 
 struct sh_mobile_lcdc_priv {
+	struct platform_device *pdev;
 	void __iomem *base;
 	int irq;
 #ifdef CONFIG_HAVE_CLK
@@ -185,6 +186,7 @@ struct sh_mobile_lcdc_sys_bus_ops sh_mob
 #ifdef CONFIG_HAVE_CLK
 static void sh_mobile_lcdc_clk_on(struct sh_mobile_lcdc_priv *priv)
 {
+	platform_device_wakeup(priv->pdev);
 	if (atomic_inc_and_test(&priv->clk_usecnt)) {
 		clk_enable(priv->clk);
 		if (priv->dot_clk)
@@ -199,6 +201,7 @@ static void sh_mobile_lcdc_clk_off(struc
 			clk_disable(priv->dot_clk);
 		clk_disable(priv->clk);
 	}
+	platform_device_idle(priv->pdev);
 }
 #else
 static void sh_mobile_lcdc_clk_on(struct sh_mobile_lcdc_priv *priv) {}
@@ -744,6 +747,7 @@ static int __init sh_mobile_lcdc_probe(s
 	}
 
 	priv->irq = i;
+	priv->pdev = pdev;
 	platform_set_drvdata(pdev, priv);
 	pdata = pdev->dev.platform_data;
 
--- 0001/arch/sh/kernel/cpu/shmobile/pm.c
+++ work/arch/sh/kernel/cpu/shmobile/pm.c	2009-05-26 21:22:23.000000000 +0900
@@ -13,8 +13,10 @@
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/suspend.h>
+#include <linux/platform_device.h>
 #include <asm/suspend.h>
 #include <asm/uaccess.h>
+#include <cpu/sh7722.h>
 
 /*
  * Sleep modes available on SuperH Mobile:
@@ -90,3 +92,119 @@ static int __init sh_pm_init(void)
 }
 
 late_initcall(sh_pm_init);
+
+static DECLARE_BITMAP(hw_blks_added, HW_BLK_NR);
+static DECLARE_BITMAP(hw_blks_idle, HW_BLK_NR);
+static DECLARE_BITMAP(hw_blks_frozen, HW_BLK_NR);
+
+static DEFINE_SPINLOCK(hw_blk_lock);
+static LIST_HEAD(hw_blk_list);
+
+#define PLATFORM_PM_IDLE_DELAY 1000 /* ms */
+
+static void platform_device_freezer(struct work_struct *work);
+static DECLARE_DELAYED_WORK(hw_blk_delayed_work, platform_device_freezer);
+
+static void platform_device_schedule_work(int id)
+{
+	schedule_delayed_work(&hw_blk_delayed_work, PLATFORM_PM_IDLE_DELAY);
+}
+
+static void platform_device_freezer(struct work_struct *work)
+{
+	struct platform_device *pdev;
+	int id;
+
+	spin_lock(&hw_blk_lock);
+	list_for_each_entry(pdev, &hw_blk_list, archdata.entry) {
+		id = pdev->archdata.hw_blk_id;
+
+		if (test_bit(id, hw_blks_idle) &&
+		    !test_bit(id, hw_blks_frozen)) {
+
+			printk("freezing idle device %d!\n",
+			       pdev->archdata.hw_blk_id);
+
+			platform_runtime_dev_pm_ops.freeze_noirq(&pdev->dev);
+			__set_bit(pdev->archdata.hw_blk_id, hw_blks_frozen);
+		}
+	}
+	spin_unlock(&hw_blk_lock);
+}
+
+void platform_device_wakeup(struct platform_device *pdev)
+{
+	int id = pdev->archdata.hw_blk_id;
+
+	/* ignore off-chip or already woken up platform devices */
+	if (!id || !test_bit(id, hw_blks_idle))
+		return;
+	
+	spin_lock(&hw_blk_lock);
+	if (test_bit(id, hw_blks_frozen)) {
+		printk("waking up frozen device %d!\n", id);
+
+		platform_runtime_dev_pm_ops.thaw_noirq(&pdev->dev);
+		__clear_bit(id, hw_blks_frozen);
+	}
+	spin_unlock(&hw_blk_lock);
+	clear_bit(id, hw_blks_idle);
+}
+
+void platform_device_idle(struct platform_device *pdev)
+{
+	int id = pdev->archdata.hw_blk_id;
+
+	/* ignore off-chip non-SoC platform devices */
+	if (!id)
+		return;
+
+	set_bit(id, hw_blks_idle);
+	if (test_bit(id, hw_blks_added))
+		platform_device_schedule_work(id);
+}
+
+static int __devinit platform_bus_notify(struct notifier_block *nb,
+					 unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct platform_device *pdev = to_platform_device(dev);
+	int id = pdev->archdata.hw_blk_id;
+
+	/* ignore off-chip non-SoC platform devices */
+	if (!id)
+		return 0;
+
+	switch(action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		INIT_LIST_HEAD(&pdev->archdata.entry);
+
+		spin_lock(&hw_blk_lock);
+		list_add_tail(&pdev->archdata.entry, &hw_blk_list);
+		__set_bit(id, hw_blks_added);
+		spin_unlock(&hw_blk_lock);
+
+		if (test_bit(id, hw_blks_idle))
+			platform_device_schedule_work(id);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		spin_lock(&hw_blk_lock);
+		list_del(&pdev->archdata.entry);
+		__clear_bit(id, hw_blks_added);
+		spin_unlock(&hw_blk_lock);
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block platform_bus_notifier = {
+	.notifier_call = platform_bus_notify
+};
+
+static int __init sh_pm_runtime_init(void)
+{
+	bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
+	return 0;
+}
+
+arch_initcall(sh_pm_runtime_init);

Re: [linux-pm] [PATCH 00/04][RFC] PM: Runtime platform device PM

[Mark Brown]

On Wed, May 27, 2009 at 07:06:25PM +0900, Magnus Damm wrote:

> These patches extend platform device and driver interfaces to
> allow architectures to implement platform device runtime pm.

> Have a look at the last patch for a SuperH mockup that shows how
> it all fits together. We need this or a similar interface to be able
> to enter deep sleep states on SuperH. I believe other architectures
> have similar requirements.

ARM definitely does.  The OMAP SoCs are probably furthest along in
software support for this sort of functionality but it's pretty much
standard for modern ARMs.  I'd CC in the ARM list but it's subscribers
only, though the OMAP folks are also over on linux-omap@vger.kernel.org
which isn't.

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Mark Brown]

On Wed, May 27, 2009 at 07:06:25PM +0900, Magnus Damm wrote:

> These patches extend platform device and driver interfaces to
> allow architectures to implement platform device runtime pm.

> Have a look at the last patch for a SuperH mockup that shows how
> it all fits together. We need this or a similar interface to be able
> to enter deep sleep states on SuperH. I believe other architectures
> have similar requirements.

ARM definitely does.  The OMAP SoCs are probably furthest along in
software support for this sort of functionality but it's pretty much
standard for modern ARMs.  I'd CC in the ARM list but it's subscribers
only, though the OMAP folks are also over on linux-omap@vger.kernel.org
which isn't.

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Wed, 27 May 2009, Magnus Damm wrote:

> PM: Runtime platform device power management
> 
> [PATCH 01/04] Driver Core: Add platform device arch data
> [PATCH 02/04] Driver Core: Add idle and wakeup functions
> [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops
> [PATCH 04/04] sh: Runtime platform device PM mockup
> 
> These patches extend platform device and driver interfaces to
> allow architectures to implement platform device runtime pm.
> 
> Upstream runtime power management needs to be improved to fully
> make use of hardware power saving features found on embedded
> platforms and in common SoCs.
> 
> For runtime power management we today have cpuidle, the clock
> framework and qos. This allows the cpu core to enter various
> forms of deep sleep, and for devices we may stop clocks to save
> power. Modern SoCs however allow disabling of power to parts of
> the chip, and we have no upstream interface to handle that today.
> 
> I propose adding the following simple platform device functions:
>  - platform_device_wakeup()
>  - platform_device_idle()
> 
> The idle function is used by the platform driver to let the 
> architecture power management code know that from now on the
> device is in idle state. When the device is marked as idle
> the architecture specific runtime power management may decide
> to do various levels of device power management, ranging from
> stopping clocks to turning off power. The dev_pm_ops callbacks
> may be invoked by the runtime pm code to save and restore state
> whenever the device is marked as idle.
> 
> The wakeup function is used by the platform driver to notify the
> architecture code that the driver wants to make use of the hardware
> device. If the device has been put in sleep then it needs to be
> woken up. This wakeup call may invoke dev_pm_ops callbacks.
> 
> Have a look at the last patch for a SuperH mockup that shows how
> it all fits together. We need this or a similar interface to be able
> to enter deep sleep states on SuperH. I believe other architectures
> have similar requirements.

Have you given any thought as to whether the platform_device_wakeup and 
platform_device_idle calls should be restricted to process context?

You should consider using a special workqueue for this stuff instead of 
relying on the default workqueue.  It can help avoid deadlocks, and it 
has the advantage that you can define the workqueue to be freezable.  
(Generally speaking, you don't want platform-level wakeup and idle 
calls to start running spontaneously in the middle of a system sleep 
transition.)

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Wed, 27 May 2009, Magnus Damm wrote:

> PM: Runtime platform device power management
> 
> [PATCH 01/04] Driver Core: Add platform device arch data
> [PATCH 02/04] Driver Core: Add idle and wakeup functions
> [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops
> [PATCH 04/04] sh: Runtime platform device PM mockup
> 
> These patches extend platform device and driver interfaces to
> allow architectures to implement platform device runtime pm.
> 
> Upstream runtime power management needs to be improved to fully
> make use of hardware power saving features found on embedded
> platforms and in common SoCs.
> 
> For runtime power management we today have cpuidle, the clock
> framework and qos. This allows the cpu core to enter various
> forms of deep sleep, and for devices we may stop clocks to save
> power. Modern SoCs however allow disabling of power to parts of
> the chip, and we have no upstream interface to handle that today.
> 
> I propose adding the following simple platform device functions:
>  - platform_device_wakeup()
>  - platform_device_idle()
> 
> The idle function is used by the platform driver to let the 
> architecture power management code know that from now on the
> device is in idle state. When the device is marked as idle
> the architecture specific runtime power management may decide
> to do various levels of device power management, ranging from
> stopping clocks to turning off power. The dev_pm_ops callbacks
> may be invoked by the runtime pm code to save and restore state
> whenever the device is marked as idle.
> 
> The wakeup function is used by the platform driver to notify the
> architecture code that the driver wants to make use of the hardware
> device. If the device has been put in sleep then it needs to be
> woken up. This wakeup call may invoke dev_pm_ops callbacks.
> 
> Have a look at the last patch for a SuperH mockup that shows how
> it all fits together. We need this or a similar interface to be able
> to enter deep sleep states on SuperH. I believe other architectures
> have similar requirements.

Have you given any thought as to whether the platform_device_wakeup and 
platform_device_idle calls should be restricted to process context?

You should consider using a special workqueue for this stuff instead of 
relying on the default workqueue.  It can help avoid deadlocks, and it 
has the advantage that you can define the workqueue to be freezable.  
(Generally speaking, you don't want platform-level wakeup and idle 
calls to start running spontaneously in the middle of a system sleep 
transition.)

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Kevin Hilman]

Magnus Damm <magnus.damm@gmail.com> writes:

> PM: Runtime platform device power management
>
> [PATCH 01/04] Driver Core: Add platform device arch data
> [PATCH 02/04] Driver Core: Add idle and wakeup functions
> [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops
> [PATCH 04/04] sh: Runtime platform device PM mockup
>
> These patches extend platform device and driver interfaces to
> allow architectures to implement platform device runtime pm.
>
> Upstream runtime power management needs to be improved to fully
> make use of hardware power saving features found on embedded
> platforms and in common SoCs.
>
> For runtime power management we today have cpuidle, the clock
> framework and qos. This allows the cpu core to enter various
> forms of deep sleep, and for devices we may stop clocks to save
> power. Modern SoCs however allow disabling of power to parts of
> the chip, and we have no upstream interface to handle that today.
>
> I propose adding the following simple platform device functions:
>  - platform_device_wakeup()
>  - platform_device_idle()
>
> The idle function is used by the platform driver to let the 
> architecture power management code know that from now on the
> device is in idle state. When the device is marked as idle
> the architecture specific runtime power management may decide
> to do various levels of device power management, ranging from
> stopping clocks to turning off power. The dev_pm_ops callbacks
> may be invoked by the runtime pm code to save and restore state
> whenever the device is marked as idle.
>
> The wakeup function is used by the platform driver to notify the
> architecture code that the driver wants to make use of the hardware
> device. If the device has been put in sleep then it needs to be
> woken up. This wakeup call may invoke dev_pm_ops callbacks.
>
> Have a look at the last patch for a SuperH mockup that shows how
> it all fits together. We need this or a similar interface to be able
> to enter deep sleep states on SuperH. I believe other architectures
> have similar requirements.
>
> Signed-off-by: Magnus Damm <damm@igel.co.jp>

Speaking on behalf of OMAP PM developers, and as maintainer of the
current OMAP PM infrastructure, this is a major step towards a generic
runtime PM that we would use on OMAP as well.

In the PM branch of the linux-omap tree, we currently have an ad-hoc
but working infrastructure for all of this, including device-specific
save/restore hooks for supporting some or all of the on-chip
powerdomains to go off during idle.

However, our current approach hides most of the details behind the
clock framework, custom save/restore hooks and in some cases custom
platform hooks passed using platform_data.

I like this proposed approach much better as it generalizes the
approach and allows us to share the infrastructure across arches
as well as decouple device idleness from clock management.

Kevin

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Kevin Hilman]

Magnus Damm <magnus.damm@gmail.com> writes:

> PM: Runtime platform device power management
>
> [PATCH 01/04] Driver Core: Add platform device arch data
> [PATCH 02/04] Driver Core: Add idle and wakeup functions
> [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops
> [PATCH 04/04] sh: Runtime platform device PM mockup
>
> These patches extend platform device and driver interfaces to
> allow architectures to implement platform device runtime pm.
>
> Upstream runtime power management needs to be improved to fully
> make use of hardware power saving features found on embedded
> platforms and in common SoCs.
>
> For runtime power management we today have cpuidle, the clock
> framework and qos. This allows the cpu core to enter various
> forms of deep sleep, and for devices we may stop clocks to save
> power. Modern SoCs however allow disabling of power to parts of
> the chip, and we have no upstream interface to handle that today.
>
> I propose adding the following simple platform device functions:
>  - platform_device_wakeup()
>  - platform_device_idle()
>
> The idle function is used by the platform driver to let the 
> architecture power management code know that from now on the
> device is in idle state. When the device is marked as idle
> the architecture specific runtime power management may decide
> to do various levels of device power management, ranging from
> stopping clocks to turning off power. The dev_pm_ops callbacks
> may be invoked by the runtime pm code to save and restore state
> whenever the device is marked as idle.
>
> The wakeup function is used by the platform driver to notify the
> architecture code that the driver wants to make use of the hardware
> device. If the device has been put in sleep then it needs to be
> woken up. This wakeup call may invoke dev_pm_ops callbacks.
>
> Have a look at the last patch for a SuperH mockup that shows how
> it all fits together. We need this or a similar interface to be able
> to enter deep sleep states on SuperH. I believe other architectures
> have similar requirements.
>
> Signed-off-by: Magnus Damm <damm@igel.co.jp>

Speaking on behalf of OMAP PM developers, and as maintainer of the
current OMAP PM infrastructure, this is a major step towards a generic
runtime PM that we would use on OMAP as well.

In the PM branch of the linux-omap tree, we currently have an ad-hoc
but working infrastructure for all of this, including device-specific
save/restore hooks for supporting some or all of the on-chip
powerdomains to go off during idle.

However, our current approach hides most of the details behind the
clock framework, custom save/restore hooks and in some cases custom
platform hooks passed using platform_data.

I like this proposed approach much better as it generalizes the
approach and allows us to share the infrastructure across arches
as well as decouple device idleness from clock management.

Kevin

Re: [linux-pm] [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Wed, May 27, 2009 at 9:10 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 27, 2009 at 07:06:25PM +0900, Magnus Damm wrote:
>
>> These patches extend platform device and driver interfaces to
>> allow architectures to implement platform device runtime pm.
>
>> Have a look at the last patch for a SuperH mockup that shows how
>> it all fits together. We need this or a similar interface to be able
>> to enter deep sleep states on SuperH. I believe other architectures
>> have similar requirements.
>
> ARM definitely does.  The OMAP SoCs are probably furthest along in
> software support for this sort of functionality but it's pretty much
> standard for modern ARMs.  I'd CC in the ARM list but it's subscribers
> only, though the OMAP folks are also over on linux-omap@vger.kernel.org
> which isn't.

I'll include linux-omap next time. Thanks for your suggestion!

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Wed, May 27, 2009 at 9:10 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 27, 2009 at 07:06:25PM +0900, Magnus Damm wrote:
>
>> These patches extend platform device and driver interfaces to
>> allow architectures to implement platform device runtime pm.
>
>> Have a look at the last patch for a SuperH mockup that shows how
>> it all fits together. We need this or a similar interface to be able
>> to enter deep sleep states on SuperH. I believe other architectures
>> have similar requirements.
>
> ARM definitely does.  The OMAP SoCs are probably furthest along in
> software support for this sort of functionality but it's pretty much
> standard for modern ARMs.  I'd CC in the ARM list but it's subscribers
> only, though the OMAP folks are also over on linux-omap@vger.kernel.org
> which isn't.

I'll include linux-omap next time. Thanks for your suggestion!

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 27 May 2009, Magnus Damm wrote:
>> PM: Runtime platform device power management

> Have you given any thought as to whether the platform_device_wakeup and
> platform_device_idle calls should be restricted to process context?

Good question. The first thing that pops into my mind is to have same
restrictions as clk_enable() and clk_disable(). To keep things simple
I'd say that it's unlikely that any embedded platform driver would
need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
only allow _noirq variants of dev_pm_ops, not sure. Any ideas?

> You should consider using a special workqueue for this stuff instead of
> relying on the default workqueue.  It can help avoid deadlocks, and it
> has the advantage that you can define the workqueue to be freezable.
> (Generally speaking, you don't want platform-level wakeup and idle
> calls to start running spontaneously in the middle of a system sleep
> transition.)

Yeah, the mockup code needs more work. =) For SuperH Mobile I suspect
that we don't need any workqueues since the freeze() and disabling of
power has to be done from cpuidle context. For SoCs with more complex
power domain layouts it may make sense handle each power domain
independently. And on top of this we probably want to have some QoS
information so the runtime pm code can chose which device sleep mode
to enter. Like cpuidle but for devices or power domains.

Thanks for your comments!

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 27 May 2009, Magnus Damm wrote:
>> PM: Runtime platform device power management

> Have you given any thought as to whether the platform_device_wakeup and
> platform_device_idle calls should be restricted to process context?

Good question. The first thing that pops into my mind is to have same
restrictions as clk_enable() and clk_disable(). To keep things simple
I'd say that it's unlikely that any embedded platform driver would
need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
only allow _noirq variants of dev_pm_ops, not sure. Any ideas?

> You should consider using a special workqueue for this stuff instead of
> relying on the default workqueue.  It can help avoid deadlocks, and it
> has the advantage that you can define the workqueue to be freezable.
> (Generally speaking, you don't want platform-level wakeup and idle
> calls to start running spontaneously in the middle of a system sleep
> transition.)

Yeah, the mockup code needs more work. =) For SuperH Mobile I suspect
that we don't need any workqueues since the freeze() and disabling of
power has to be done from cpuidle context. For SoCs with more complex
power domain layouts it may make sense handle each power domain
independently. And on top of this we probably want to have some QoS
information so the runtime pm code can chose which device sleep mode
to enter. Like cpuidle but for devices or power domains.

Thanks for your comments!

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Thursday 28 May 2009, Magnus Damm wrote:
> On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 27 May 2009, Magnus Damm wrote:
> >> PM: Runtime platform device power management
> 
> > Have you given any thought as to whether the platform_device_wakeup and
> > platform_device_idle calls should be restricted to process context?
> 
> Good question. The first thing that pops into my mind is to have same
> restrictions as clk_enable() and clk_disable(). To keep things simple
> I'd say that it's unlikely that any embedded platform driver would
> need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
> only allow _noirq variants of dev_pm_ops, not sure. Any ideas?
> 
> > You should consider using a special workqueue for this stuff instead of
> > relying on the default workqueue.  It can help avoid deadlocks, and it
> > has the advantage that you can define the workqueue to be freezable.
> > (Generally speaking, you don't want platform-level wakeup and idle
> > calls to start running spontaneously in the middle of a system sleep
> > transition.)
> 
> Yeah, the mockup code needs more work. =) For SuperH Mobile I suspect
> that we don't need any workqueues since the freeze() and disabling of
> power has to be done from cpuidle context. For SoCs with more complex
> power domain layouts it may make sense handle each power domain
> independently. And on top of this we probably want to have some QoS
> information so the runtime pm code can chose which device sleep mode
> to enter. Like cpuidle but for devices or power domains.

I think we'll need one separate workqueue for run-time PM in general, so that
bus types don't introduce their own workqueues for this purpose.  IMO one
system-wide run-time PM workqueue should be sufficient (it could also be
used for the suspend blockers BTW).

So, perhaps it makes sense to implement such a workqueue at the core level?
Thoughts?

Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Thursday 28 May 2009, Magnus Damm wrote:
> On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 27 May 2009, Magnus Damm wrote:
> >> PM: Runtime platform device power management
> 
> > Have you given any thought as to whether the platform_device_wakeup and
> > platform_device_idle calls should be restricted to process context?
> 
> Good question. The first thing that pops into my mind is to have same
> restrictions as clk_enable() and clk_disable(). To keep things simple
> I'd say that it's unlikely that any embedded platform driver would
> need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
> only allow _noirq variants of dev_pm_ops, not sure. Any ideas?
> 
> > You should consider using a special workqueue for this stuff instead of
> > relying on the default workqueue.  It can help avoid deadlocks, and it
> > has the advantage that you can define the workqueue to be freezable.
> > (Generally speaking, you don't want platform-level wakeup and idle
> > calls to start running spontaneously in the middle of a system sleep
> > transition.)
> 
> Yeah, the mockup code needs more work. =) For SuperH Mobile I suspect
> that we don't need any workqueues since the freeze() and disabling of
> power has to be done from cpuidle context. For SoCs with more complex
> power domain layouts it may make sense handle each power domain
> independently. And on top of this we probably want to have some QoS
> information so the runtime pm code can chose which device sleep mode
> to enter. Like cpuidle but for devices or power domains.

I think we'll need one separate workqueue for run-time PM in general, so that
bus types don't introduce their own workqueues for this purpose.  IMO one
system-wide run-time PM workqueue should be sufficient (it could also be
used for the suspend blockers BTW).

So, perhaps it makes sense to implement such a workqueue at the core level?
Thoughts?

Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Thu, 28 May 2009, Rafael J. Wysocki wrote:

> I think we'll need one separate workqueue for run-time PM in general, so that
> bus types don't introduce their own workqueues for this purpose.  IMO one
> system-wide run-time PM workqueue should be sufficient (it could also be
> used for the suspend blockers BTW).
> 
> So, perhaps it makes sense to implement such a workqueue at the core level?
> Thoughts?

That's fine with me.  This new workqueue can take over the job of the 
"ksuspend_usbd" workqueue.  But it would have to be marked as 
freezable.

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Thu, 28 May 2009, Rafael J. Wysocki wrote:

> I think we'll need one separate workqueue for run-time PM in general, so that
> bus types don't introduce their own workqueues for this purpose.  IMO one
> system-wide run-time PM workqueue should be sufficient (it could also be
> used for the suspend blockers BTW).
> 
> So, perhaps it makes sense to implement such a workqueue at the core level?
> Thoughts?

That's fine with me.  This new workqueue can take over the job of the 
"ksuspend_usbd" workqueue.  But it would have to be marked as 
freezable.

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Thu, 28 May 2009, Magnus Damm wrote:

> On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 27 May 2009, Magnus Damm wrote:
> >> PM: Runtime platform device power management
> 
> > Have you given any thought as to whether the platform_device_wakeup and
> > platform_device_idle calls should be restricted to process context?
> 
> Good question. The first thing that pops into my mind is to have same
> restrictions as clk_enable() and clk_disable(). To keep things simple
> I'd say that it's unlikely that any embedded platform driver would
> need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
> only allow _noirq variants of dev_pm_ops, not sure. Any ideas?

I don't know what would be appropriate for you.  USB power transitions
cannot be made in an atomic context, but the situation could well be
different for platform devices.  It's just something to be aware of.

(And by the way, the _noirq ops don't run in atomic context.  They run 
in process context with most interrupt deliveries disabled.  It's not 
the same thing -- they are allowed to sleep.)

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Thu, 28 May 2009, Magnus Damm wrote:

> On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 27 May 2009, Magnus Damm wrote:
> >> PM: Runtime platform device power management
> 
> > Have you given any thought as to whether the platform_device_wakeup and
> > platform_device_idle calls should be restricted to process context?
> 
> Good question. The first thing that pops into my mind is to have same
> restrictions as clk_enable() and clk_disable(). To keep things simple
> I'd say that it's unlikely that any embedded platform driver would
> need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
> only allow _noirq variants of dev_pm_ops, not sure. Any ideas?

I don't know what would be appropriate for you.  USB power transitions
cannot be made in an atomic context, but the situation could well be
different for platform devices.  It's just something to be aware of.

(And by the way, the _noirq ops don't run in atomic context.  They run 
in process context with most interrupt deliveries disabled.  It's not 
the same thing -- they are allowed to sleep.)

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Kevin Hilman]

Magnus Damm <magnus.damm@gmail.com> writes:

> For runtime power management we today have cpuidle, the clock
> framework and qos. This allows the cpu core to enter various
> forms of deep sleep, and for devices we may stop clocks to save
> power. Modern SoCs however allow disabling of power to parts of
> the chip, and we have no upstream interface to handle that today.
>
> I propose adding the following simple platform device functions:
>  - platform_device_wakeup()
>  - platform_device_idle()
>
> The idle function is used by the platform driver to let the 
> architecture power management code know that from now on the
> device is in idle state. When the device is marked as idle
> the architecture specific runtime power management may decide
> to do various levels of device power management, ranging from
> stopping clocks to turning off power. The dev_pm_ops callbacks
> may be invoked by the runtime pm code to save and restore state
> whenever the device is marked as idle.
>
> The wakeup function is used by the platform driver to notify the
> architecture code that the driver wants to make use of the hardware
> device. If the device has been put in sleep then it needs to be
> woken up. This wakeup call may invoke dev_pm_ops callbacks.

A couple comments/questions:

Wouldn't the 'wakeup' hook be better named platform_device_enable().
The _wakeup name seems to imply that the call will perform some sort
of wakeup, but all it's really doing is enabling it by turning it on
or taking it out of idle.

In the process of discussing a similar interface for OMAP, we dicussed
that having 3 states would be more useful.  Specifically, and
_enable(), _idle() and _disable() hook.  The _enable() and _idle()
hooks being exactly what you proposed above, but with the addition of
a _disable() hook which says not only can the device go idle, but that
the driver is really finished with the device.  In this case, more
aggresive PM measures could be taken, such as turning of regulators
that may have long latencies that may not be appropriate to turn off
in idle.

I'm interested in any thoughts there because the line between _idle()
and _disable() remains a little fuzzy to me still.  For exmaple, maybe
the _idle() call in combination with latency constraints in PM QoS
could do effectively the same thing as _disable().

Kevin

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Kevin Hilman]

Magnus Damm <magnus.damm@gmail.com> writes:

> For runtime power management we today have cpuidle, the clock
> framework and qos. This allows the cpu core to enter various
> forms of deep sleep, and for devices we may stop clocks to save
> power. Modern SoCs however allow disabling of power to parts of
> the chip, and we have no upstream interface to handle that today.
>
> I propose adding the following simple platform device functions:
>  - platform_device_wakeup()
>  - platform_device_idle()
>
> The idle function is used by the platform driver to let the 
> architecture power management code know that from now on the
> device is in idle state. When the device is marked as idle
> the architecture specific runtime power management may decide
> to do various levels of device power management, ranging from
> stopping clocks to turning off power. The dev_pm_ops callbacks
> may be invoked by the runtime pm code to save and restore state
> whenever the device is marked as idle.
>
> The wakeup function is used by the platform driver to notify the
> architecture code that the driver wants to make use of the hardware
> device. If the device has been put in sleep then it needs to be
> woken up. This wakeup call may invoke dev_pm_ops callbacks.

A couple comments/questions:

Wouldn't the 'wakeup' hook be better named platform_device_enable().
The _wakeup name seems to imply that the call will perform some sort
of wakeup, but all it's really doing is enabling it by turning it on
or taking it out of idle.

In the process of discussing a similar interface for OMAP, we dicussed
that having 3 states would be more useful.  Specifically, and
_enable(), _idle() and _disable() hook.  The _enable() and _idle()
hooks being exactly what you proposed above, but with the addition of
a _disable() hook which says not only can the device go idle, but that
the driver is really finished with the device.  In this case, more
aggresive PM measures could be taken, such as turning of regulators
that may have long latencies that may not be appropriate to turn off
in idle.

I'm interested in any thoughts there because the line between _idle()
and _disable() remains a little fuzzy to me still.  For exmaple, maybe
the _idle() call in combination with latency constraints in PM QoS
could do effectively the same thing as _disable().

Kevin

Re: [PATCH 01/04] Driver Core: Add platform device arch data

[Rafael J. Wysocki]

On Wednesday 27 May 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Allow architecture specific data in struct platform_device.
> Platform device data is needed by the runtime pm code. A similar
> struct already exists for struct device.
> 
> The architecture specific asm/device.h file needs to provide
> struct pdev_archdata if CONFIG_HAVE_PLATFORM_DEVICE_DATA is set.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Optionally we can make use of struct device archdata instead,
>  but since the runtime device pm is limited to platform devices
>  it makes sense to make this data platform device specific imo.
> 
>  arch/Kconfig                    |    3 +++
>  include/linux/platform_device.h |    5 +++++
>  2 files changed, 8 insertions(+)
> 
> --- 0001/arch/Kconfig
> +++ work/arch/Kconfig	2009-05-15 14:51:35.000000000 +0900
> @@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
>  
>  config HAVE_DEFAULT_NO_SPIN_MUTEXES
>  	bool
> +
> +config HAVE_PLATFORM_DEVICE_ARCHDATA

Hmm.  Do we really need yet another CONFIG_ option for that?

> +	bool
> --- 0001/include/linux/platform_device.h
> +++ work/include/linux/platform_device.h	2009-05-15 16:59:01.000000000 +0900
> @@ -23,6 +23,11 @@ struct platform_device {
>  	void		*platform_data;
>  
>  	struct platform_device_id	*id_entry;
> +
> +#ifdef CONFIG_HAVE_PLATFORM_DEVICE_ARCHDATA
> +	/* arch specific additions */
> +	struct pdev_archdata	archdata;
> +#endif
>  };
>  
>  #define platform_get_device_id(pdev)	((pdev)->id_entry)

Re: [PATCH 01/04] Driver Core: Add platform device arch data

[Rafael J. Wysocki]

On Wednesday 27 May 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Allow architecture specific data in struct platform_device.
> Platform device data is needed by the runtime pm code. A similar
> struct already exists for struct device.
> 
> The architecture specific asm/device.h file needs to provide
> struct pdev_archdata if CONFIG_HAVE_PLATFORM_DEVICE_DATA is set.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Optionally we can make use of struct device archdata instead,
>  but since the runtime device pm is limited to platform devices
>  it makes sense to make this data platform device specific imo.
> 
>  arch/Kconfig                    |    3 +++
>  include/linux/platform_device.h |    5 +++++
>  2 files changed, 8 insertions(+)
> 
> --- 0001/arch/Kconfig
> +++ work/arch/Kconfig	2009-05-15 14:51:35.000000000 +0900
> @@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
>  
>  config HAVE_DEFAULT_NO_SPIN_MUTEXES
>  	bool
> +
> +config HAVE_PLATFORM_DEVICE_ARCHDATA

Hmm.  Do we really need yet another CONFIG_ option for that?

> +	bool
> --- 0001/include/linux/platform_device.h
> +++ work/include/linux/platform_device.h	2009-05-15 16:59:01.000000000 +0900
> @@ -23,6 +23,11 @@ struct platform_device {
>  	void		*platform_data;
>  
>  	struct platform_device_id	*id_entry;
> +
> +#ifdef CONFIG_HAVE_PLATFORM_DEVICE_ARCHDATA
> +	/* arch specific additions */
> +	struct pdev_archdata	archdata;
> +#endif
>  };
>  
>  #define platform_get_device_id(pdev)	((pdev)->id_entry)

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Wednesday 27 May 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Add platform_device_idle() and platform_device_wakeup()
> and allow architectures to implement their own versions
> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Yes, this needs documentation. See [00/04] for now.
> 
>  arch/Kconfig                    |    3 +++
>  include/linux/platform_device.h |    8 ++++++++
>  2 files changed, 11 insertions(+)
> 
> --- 0002/arch/Kconfig
> +++ work/arch/Kconfig	2009-05-26 12:31:06.000000000 +0900
> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
>  
>  config HAVE_PLATFORM_DEVICE_ARCHDATA
>  	bool
> +
> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP

Again, I'm not sure if the new CONFIG_ option is really necessary.

> +	bool
> --- 0002/include/linux/platform_device.h
> +++ work/include/linux/platform_device.h	2009-05-26 12:32:40.000000000 +0900
> @@ -57,6 +57,14 @@ extern int platform_device_add(struct pl
>  extern void platform_device_del(struct platform_device *pdev);
>  extern void platform_device_put(struct platform_device *pdev);
>  
> +#ifdef CONFIG_HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
> +void platform_device_idle(struct platform_device *pdev);
> +void platform_device_wakeup(struct platform_device *pdev);
> +#else
> +static inline void platform_device_idle(struct platform_device *pdev) {}
> +static inline void platform_device_wakeup(struct platform_device *pdev) {}
> +#endif
> +
>  struct platform_driver {
>  	int (*probe)(struct platform_device *);
>  	int (*remove)(struct platform_device *);

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Wednesday 27 May 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Add platform_device_idle() and platform_device_wakeup()
> and allow architectures to implement their own versions
> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Yes, this needs documentation. See [00/04] for now.
> 
>  arch/Kconfig                    |    3 +++
>  include/linux/platform_device.h |    8 ++++++++
>  2 files changed, 11 insertions(+)
> 
> --- 0002/arch/Kconfig
> +++ work/arch/Kconfig	2009-05-26 12:31:06.000000000 +0900
> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
>  
>  config HAVE_PLATFORM_DEVICE_ARCHDATA
>  	bool
> +
> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP

Again, I'm not sure if the new CONFIG_ option is really necessary.

> +	bool
> --- 0002/include/linux/platform_device.h
> +++ work/include/linux/platform_device.h	2009-05-26 12:32:40.000000000 +0900
> @@ -57,6 +57,14 @@ extern int platform_device_add(struct pl
>  extern void platform_device_del(struct platform_device *pdev);
>  extern void platform_device_put(struct platform_device *pdev);
>  
> +#ifdef CONFIG_HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
> +void platform_device_idle(struct platform_device *pdev);
> +void platform_device_wakeup(struct platform_device *pdev);
> +#else
> +static inline void platform_device_idle(struct platform_device *pdev) {}
> +static inline void platform_device_wakeup(struct platform_device *pdev) {}
> +#endif
> +
>  struct platform_driver {
>  	int (*probe)(struct platform_device *);
>  	int (*remove)(struct platform_device *);

Re: [PATCH 01/04] Driver Core: Add platform device arch data

[Magnus Damm]

2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 27 May 2009, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Allow architecture specific data in struct platform_device.
>> Platform device data is needed by the runtime pm code. A similar
>> struct already exists for struct device.
>>
>> The architecture specific asm/device.h file needs to provide
>> struct pdev_archdata if CONFIG_HAVE_PLATFORM_DEVICE_DATA is set.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  Optionally we can make use of struct device archdata instead,
>>  but since the runtime device pm is limited to platform devices
>>  it makes sense to make this data platform device specific imo.
>>
>>  arch/Kconfig                    |    3 +++
>>  include/linux/platform_device.h |    5 +++++
>>  2 files changed, 8 insertions(+)
>>
>> --- 0001/arch/Kconfig
>> +++ work/arch/Kconfig 2009-05-15 14:51:35.000000000 +0900
>> @@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
>>
>>  config HAVE_DEFAULT_NO_SPIN_MUTEXES
>>       bool
>> +
>> +config HAVE_PLATFORM_DEVICE_ARCHDATA
>
> Hmm.  Do we really need yet another CONFIG_ option for that?

I don't think we need a total of 3 different config options for the
platform device pm stuff, but I do think it makes sense to handle the
platform archdata separately. Not sure which is the best way though,
the #ifdefery is not very pretty. Maybe the best solution is just to
drop the Kconfig stuff and add empty structures to all architecture
versions of asm/device.h.

Any suggestions?

Thanks!

/ magnus

Re: [PATCH 01/04] Driver Core: Add platform device arch data

[Magnus Damm]

2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 27 May 2009, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Allow architecture specific data in struct platform_device.
>> Platform device data is needed by the runtime pm code. A similar
>> struct already exists for struct device.
>>
>> The architecture specific asm/device.h file needs to provide
>> struct pdev_archdata if CONFIG_HAVE_PLATFORM_DEVICE_DATA is set.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  Optionally we can make use of struct device archdata instead,
>>  but since the runtime device pm is limited to platform devices
>>  it makes sense to make this data platform device specific imo.
>>
>>  arch/Kconfig                    |    3 +++
>>  include/linux/platform_device.h |    5 +++++
>>  2 files changed, 8 insertions(+)
>>
>> --- 0001/arch/Kconfig
>> +++ work/arch/Kconfig 2009-05-15 14:51:35.000000000 +0900
>> @@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
>>
>>  config HAVE_DEFAULT_NO_SPIN_MUTEXES
>>       bool
>> +
>> +config HAVE_PLATFORM_DEVICE_ARCHDATA
>
> Hmm.  Do we really need yet another CONFIG_ option for that?

I don't think we need a total of 3 different config options for the
platform device pm stuff, but I do think it makes sense to handle the
platform archdata separately. Not sure which is the best way though,
the #ifdefery is not very pretty. Maybe the best solution is just to
drop the Kconfig stuff and add empty structures to all architecture
versions of asm/device.h.

Any suggestions?

Thanks!

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 27 May 2009, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Add platform_device_idle() and platform_device_wakeup()
>> and allow architectures to implement their own versions
>> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  Yes, this needs documentation. See [00/04] for now.
>>
>>  arch/Kconfig                    |    3 +++
>>  include/linux/platform_device.h |    8 ++++++++
>>  2 files changed, 11 insertions(+)
>>
>> --- 0002/arch/Kconfig
>> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
>> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
>>
>>  config HAVE_PLATFORM_DEVICE_ARCHDATA
>>       bool
>> +
>> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
>
> Again, I'm not sure if the new CONFIG_ option is really necessary.

At least it could be combined into a single option for platform device
runtime pm. So no need to have separate kconfig options for [02/04]
and [03/04].

We could also drop the #ifdef stuff and go for weak symbols that can
be overriden by architecture code.

I'm open to suggestions. =)

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 27 May 2009, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Add platform_device_idle() and platform_device_wakeup()
>> and allow architectures to implement their own versions
>> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  Yes, this needs documentation. See [00/04] for now.
>>
>>  arch/Kconfig                    |    3 +++
>>  include/linux/platform_device.h |    8 ++++++++
>>  2 files changed, 11 insertions(+)
>>
>> --- 0002/arch/Kconfig
>> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
>> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
>>
>>  config HAVE_PLATFORM_DEVICE_ARCHDATA
>>       bool
>> +
>> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
>
> Again, I'm not sure if the new CONFIG_ option is really necessary.

At least it could be combined into a single option for platform device
runtime pm. So no need to have separate kconfig options for [02/04]
and [03/04].

We could also drop the #ifdef stuff and go for weak symbols that can
be overriden by architecture code.

I'm open to suggestions. =)

/ magnus

Re: [PATCH 01/04] Driver Core: Add platform device arch data

[Paul Mundt]

On Fri, May 29, 2009 at 02:07:35PM +0900, Magnus Damm wrote:
> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> --- 0001/arch/Kconfig
> >> +++ work/arch/Kconfig 2009-05-15 14:51:35.000000000 +0900
> >> @@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
> >>
> >> ?config HAVE_DEFAULT_NO_SPIN_MUTEXES
> >> ? ? ? bool
> >> +
> >> +config HAVE_PLATFORM_DEVICE_ARCHDATA
> >
> > Hmm. ?Do we really need yet another CONFIG_ option for that?
> 
> I don't think we need a total of 3 different config options for the
> platform device pm stuff, but I do think it makes sense to handle the
> platform archdata separately. Not sure which is the best way though,
> the #ifdefery is not very pretty. Maybe the best solution is just to
> drop the Kconfig stuff and add empty structures to all architecture
> versions of asm/device.h.
> 
A stub in asm-generic/device.h will cover the majority of cases. That is
much cleaner than adding to the Kconfig mess.

Re: [PATCH 01/04] Driver Core: Add platform device arch data

[Paul Mundt]

On Fri, May 29, 2009 at 02:07:35PM +0900, Magnus Damm wrote:
> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> --- 0001/arch/Kconfig
> >> +++ work/arch/Kconfig 2009-05-15 14:51:35.000000000 +0900
> >> @@ -112,3 +112,6 @@ config HAVE_DMA_API_DEBUG
> >>
> >> ?config HAVE_DEFAULT_NO_SPIN_MUTEXES
> >> ? ? ? bool
> >> +
> >> +config HAVE_PLATFORM_DEVICE_ARCHDATA
> >
> > Hmm. ?Do we really need yet another CONFIG_ option for that?
> 
> I don't think we need a total of 3 different config options for the
> platform device pm stuff, but I do think it makes sense to handle the
> platform archdata separately. Not sure which is the best way though,
> the #ifdefery is not very pretty. Maybe the best solution is just to
> drop the Kconfig stuff and add empty structures to all architecture
> versions of asm/device.h.
> 
A stub in asm-generic/device.h will cover the majority of cases. That is
much cleaner than adding to the Kconfig mess.

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Fri, May 29, 2009 at 12:33 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 28 May 2009, Magnus Damm wrote:
>
>> On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Wed, 27 May 2009, Magnus Damm wrote:
>> >> PM: Runtime platform device power management
>>
>> > Have you given any thought as to whether the platform_device_wakeup and
>> > platform_device_idle calls should be restricted to process context?
>>
>> Good question. The first thing that pops into my mind is to have same
>> restrictions as clk_enable() and clk_disable(). To keep things simple
>> I'd say that it's unlikely that any embedded platform driver would
>> need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
>> only allow _noirq variants of dev_pm_ops, not sure. Any ideas?
>
> I don't know what would be appropriate for you.  USB power transitions
> cannot be made in an atomic context, but the situation could well be
> different for platform devices.  It's just something to be aware of.

For our SuperH platform devices there should be no need to sleep.

> (And by the way, the _noirq ops don't run in atomic context.  They run
> in process context with most interrupt deliveries disabled.  It's not
> the same thing -- they are allowed to sleep.)

Huh, so if they are allowed to sleep then clock events are still
running. And probably some clocksource as well. I guess that's why you
say "most" interrupts disabled instead of all. Sharing the timer IRQ
is not allowed then?

I wonder if the PM code assumes that the clock event is a sys device?
We use platform drivers for clock events on SuperH...

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Fri, May 29, 2009 at 12:33 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 28 May 2009, Magnus Damm wrote:
>
>> On Wed, May 27, 2009 at 11:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Wed, 27 May 2009, Magnus Damm wrote:
>> >> PM: Runtime platform device power management
>>
>> > Have you given any thought as to whether the platform_device_wakeup and
>> > platform_device_idle calls should be restricted to process context?
>>
>> Good question. The first thing that pops into my mind is to have same
>> restrictions as clk_enable() and clk_disable(). To keep things simple
>> I'd say that it's unlikely that any embedded platform driver would
>> need to sleep during the dev_pm_ops callbacks. Maybe it makes sense to
>> only allow _noirq variants of dev_pm_ops, not sure. Any ideas?
>
> I don't know what would be appropriate for you.  USB power transitions
> cannot be made in an atomic context, but the situation could well be
> different for platform devices.  It's just something to be aware of.

For our SuperH platform devices there should be no need to sleep.

> (And by the way, the _noirq ops don't run in atomic context.  They run
> in process context with most interrupt deliveries disabled.  It's not
> the same thing -- they are allowed to sleep.)

Huh, so if they are allowed to sleep then clock events are still
running. And probably some clocksource as well. I guess that's why you
say "most" interrupts disabled instead of all. Sharing the timer IRQ
is not allowed then?

I wonder if the PM code assumes that the clock event is a sys device?
We use platform drivers for clock events on SuperH...

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Fri, May 29, 2009 at 2:14 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Magnus Damm <magnus.damm@gmail.com> writes:
>> I propose adding the following simple platform device functions:
>>  - platform_device_wakeup()
>>  - platform_device_idle()

> A couple comments/questions:
>
> Wouldn't the 'wakeup' hook be better named platform_device_enable().
> The _wakeup name seems to imply that the call will perform some sort
> of wakeup, but all it's really doing is enabling it by turning it on
> or taking it out of idle.

The names platform_device_enable() and platform_device_disable() were
my first pick. Similar to pci_enable_device() and
pci_disable_device(). People may mixup platform_device_wakeup() and
pci_enable_wake() so avoiding using wakeup as name may be a good plan.
Not sure what would be a better name though.

Now why I didn't go with enable() and disable() was that dev_pm_ops
callbacks may be called for a device that has been "disabled". This
seems a bit strange to me. Making the interface clear and show that
this is not a normal enable() and disable() interface may be a good
idea.

Or maybe it's perfectly acceptable that a "disabled" device may get
it's dev_pm_ops callbacks exercised?

> In the process of discussing a similar interface for OMAP, we dicussed
> that having 3 states would be more useful.  Specifically, and
> _enable(), _idle() and _disable() hook.  The _enable() and _idle()
> hooks being exactly what you proposed above, but with the addition of
> a _disable() hook which says not only can the device go idle, but that
> the driver is really finished with the device.  In this case, more
> aggresive PM measures could be taken, such as turning of regulators
> that may have long latencies that may not be appropriate to turn off
> in idle.

Hm.. I wonder when the driver is really finished with the device
though. Only when the module is unloaded? If so then we could deal
with the hard disable using platform bus notifiers (like in [04/04]).

Or is it common that the driver does disable() at some point and
depending on the work load may have to perform a enable() soon again?

disable() seems like a more heavy weight clk_disable()? Maybe we can
let i2c drivers use enable() and disable() in their ->master_xfer()
callback. So the natural state of the i2c master device is off, and
enable() is called before data transfer in ->master_xfer(), followed
by a disable(). I do exactly this but with the clock framework in
i2c-sh_mobile.c.

The problem with the i2c example above is that the driver writer has
no clue about the frequency of the enable() and disable() calls. So
maybe disable() should be replaced with idle()? But if so when is the
real use for disable(). I'd say that the runtime PM core should decide
which state to enter, and the driver can can help out by giving
latency requirements as input. That's it.

So I'm not really sure when we need this hard disable(). Do you have
any special case in mind?

> I'm interested in any thoughts there because the line between _idle()
> and _disable() remains a little fuzzy to me still.  For exmaple, maybe
> the _idle() call in combination with latency constraints in PM QoS
> could do effectively the same thing as _disable().

Aren't two functions (wakeup()/idle() or enable()/disable()) together
with PM QoS latency enough?

Yeah, the fuzzy line is interesting. =)

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Fri, May 29, 2009 at 2:14 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Magnus Damm <magnus.damm@gmail.com> writes:
>> I propose adding the following simple platform device functions:
>>  - platform_device_wakeup()
>>  - platform_device_idle()

> A couple comments/questions:
>
> Wouldn't the 'wakeup' hook be better named platform_device_enable().
> The _wakeup name seems to imply that the call will perform some sort
> of wakeup, but all it's really doing is enabling it by turning it on
> or taking it out of idle.

The names platform_device_enable() and platform_device_disable() were
my first pick. Similar to pci_enable_device() and
pci_disable_device(). People may mixup platform_device_wakeup() and
pci_enable_wake() so avoiding using wakeup as name may be a good plan.
Not sure what would be a better name though.

Now why I didn't go with enable() and disable() was that dev_pm_ops
callbacks may be called for a device that has been "disabled". This
seems a bit strange to me. Making the interface clear and show that
this is not a normal enable() and disable() interface may be a good
idea.

Or maybe it's perfectly acceptable that a "disabled" device may get
it's dev_pm_ops callbacks exercised?

> In the process of discussing a similar interface for OMAP, we dicussed
> that having 3 states would be more useful.  Specifically, and
> _enable(), _idle() and _disable() hook.  The _enable() and _idle()
> hooks being exactly what you proposed above, but with the addition of
> a _disable() hook which says not only can the device go idle, but that
> the driver is really finished with the device.  In this case, more
> aggresive PM measures could be taken, such as turning of regulators
> that may have long latencies that may not be appropriate to turn off
> in idle.

Hm.. I wonder when the driver is really finished with the device
though. Only when the module is unloaded? If so then we could deal
with the hard disable using platform bus notifiers (like in [04/04]).

Or is it common that the driver does disable() at some point and
depending on the work load may have to perform a enable() soon again?

disable() seems like a more heavy weight clk_disable()? Maybe we can
let i2c drivers use enable() and disable() in their ->master_xfer()
callback. So the natural state of the i2c master device is off, and
enable() is called before data transfer in ->master_xfer(), followed
by a disable(). I do exactly this but with the clock framework in
i2c-sh_mobile.c.

The problem with the i2c example above is that the driver writer has
no clue about the frequency of the enable() and disable() calls. So
maybe disable() should be replaced with idle()? But if so when is the
real use for disable(). I'd say that the runtime PM core should decide
which state to enter, and the driver can can help out by giving
latency requirements as input. That's it.

So I'm not really sure when we need this hard disable(). Do you have
any special case in mind?

> I'm interested in any thoughts there because the line between _idle()
> and _disable() remains a little fuzzy to me still.  For exmaple, maybe
> the _idle() call in combination with latency constraints in PM QoS
> could do effectively the same thing as _disable().

Aren't two functions (wakeup()/idle() or enable()/disable()) together
with PM QoS latency enough?

Yeah, the fuzzy line is interesting. =)

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Fri, 29 May 2009, Magnus Damm wrote:

> > (And by the way, the _noirq ops don't run in atomic context.  They run
> > in process context with most interrupt deliveries disabled.  It's not
> > the same thing -- they are allowed to sleep.)
> 
> Huh, so if they are allowed to sleep then clock events are still
> running. And probably some clocksource as well. I guess that's why you
> say "most" interrupts disabled instead of all. Sharing the timer IRQ
> is not allowed then?

Rafael can tell you.  But notice I didn't say the interrupts were
disabled -- I said that interrupt _delivery_ was disabled.  In general
the interrupts themselves _are_ enabled; the kernel fields them but
then does not call the drivers' interrupt handler routines.  (Except
perhaps for IRQs which are explicitly marked as wakeup sources...)

> I wonder if the PM code assumes that the clock event is a sys device?
> We use platform drivers for clock events on SuperH...

Ask Rafael.

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Fri, 29 May 2009, Magnus Damm wrote:

> > (And by the way, the _noirq ops don't run in atomic context.  They run
> > in process context with most interrupt deliveries disabled.  It's not
> > the same thing -- they are allowed to sleep.)
> 
> Huh, so if they are allowed to sleep then clock events are still
> running. And probably some clocksource as well. I guess that's why you
> say "most" interrupts disabled instead of all. Sharing the timer IRQ
> is not allowed then?

Rafael can tell you.  But notice I didn't say the interrupts were
disabled -- I said that interrupt _delivery_ was disabled.  In general
the interrupts themselves _are_ enabled; the kernel fields them but
then does not call the drivers' interrupt handler routines.  (Except
perhaps for IRQs which are explicitly marked as wakeup sources...)

> I wonder if the PM code assumes that the clock event is a sys device?
> We use platform drivers for clock events on SuperH...

Ask Rafael.

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Friday 29 May 2009, Alan Stern wrote:
> On Fri, 29 May 2009, Magnus Damm wrote:
> 
> > > (And by the way, the _noirq ops don't run in atomic context.  They run
> > > in process context with most interrupt deliveries disabled.  It's not
> > > the same thing -- they are allowed to sleep.)
> > 
> > Huh, so if they are allowed to sleep then clock events are still
> > running. And probably some clocksource as well. I guess that's why you
> > say "most" interrupts disabled instead of all. Sharing the timer IRQ
> > is not allowed then?
> 
> Rafael can tell you.  But notice I didn't say the interrupts were
> disabled -- I said that interrupt _delivery_ was disabled.  In general
> the interrupts themselves _are_ enabled; the kernel fields them but
> then does not call the drivers' interrupt handler routines.

That's correct, except for the platforms where __disable_irq() actually masks
the IRQ.  I'm not sure if SuperH is one of these.

> (Except perhaps for IRQs which are explicitly marked as wakeup sources...)

No, they are treated just like the other ones except that we check if any of
them are pending right at the beginning of sysdev_suspend().

> > I wonder if the PM code assumes that the clock event is a sys device?

Yes, it does.

> > We use platform drivers for clock events on SuperH...

Argh.  This is going to hurt, but I'm not sure how badly.  At least on
multiprocessor.

Best,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Friday 29 May 2009, Alan Stern wrote:
> On Fri, 29 May 2009, Magnus Damm wrote:
> 
> > > (And by the way, the _noirq ops don't run in atomic context.  They run
> > > in process context with most interrupt deliveries disabled.  It's not
> > > the same thing -- they are allowed to sleep.)
> > 
> > Huh, so if they are allowed to sleep then clock events are still
> > running. And probably some clocksource as well. I guess that's why you
> > say "most" interrupts disabled instead of all. Sharing the timer IRQ
> > is not allowed then?
> 
> Rafael can tell you.  But notice I didn't say the interrupts were
> disabled -- I said that interrupt _delivery_ was disabled.  In general
> the interrupts themselves _are_ enabled; the kernel fields them but
> then does not call the drivers' interrupt handler routines.

That's correct, except for the platforms where __disable_irq() actually masks
the IRQ.  I'm not sure if SuperH is one of these.

> (Except perhaps for IRQs which are explicitly marked as wakeup sources...)

No, they are treated just like the other ones except that we check if any of
them are pending right at the beginning of sysdev_suspend().

> > I wonder if the PM code assumes that the clock event is a sys device?

Yes, it does.

> > We use platform drivers for clock events on SuperH...

Argh.  This is going to hurt, but I'm not sure how badly.  At least on
multiprocessor.

Best,
Rafael

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Rafael J. Wysocki]

On Wednesday 27 May 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Wrap the platform device bus dev_pm_ops to allow runtime
> pm and regular suspend and resume to coexist.
> 
> Platform device data is extended with flags that allow
> us to keep track of which dev_pm_ops that has been called.
> 
> Basically, if a device has been frozen by the runtime pm
> code, don't call ->freeze() again when hibernating.
> 
> Architecture code can use platform_runtime_dev_pm_ops to
> call driver dev_pm_ops associated with a certain device.
> 
> Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  This is a bit of a hack, any better way to wrap dev_pm_ops?

I'm not really sure you need to wrap them at all.

There are a few choices:

(1) You can use the platform_pm_* functions directly for run-time PM, but
in that case you'll need to make sure that the "suspend" ones return 0
immediately when called during system-wide suspend or hibernation (there's the
question whether the "resume" ones should still put the device into the full
power state in that case).  For this purpose you can add a single flag to
struct platform_device and set it for devices that have already been
"suspended" (this flag, when set, will make all of the "suspend" callbacks
return 0 without doing anything until the device is "resumed").

(2) You can add separate platform callbacks for run-time PM that will execute
the drivers' dev_pm_ops callbacks and presumably do something else (I don't
know what that may be for platform devices, though).  In that case, again,
adding a flag to struct platform_device and making platform_pm_* check it
should be sufficient to prevent devices from being suspended twice in a row.

(3) You can add separate platform callbacks for run-time PM for both the
bus type and the drivers, in which dev_pm_ops will be totally separate from
these new callbacks, although of course you'll need provide some kind of
synchronization bettween them all.  That also may be done through a flag
in struct platform_device IMO.

Now, since other bus types will most probably also need a flag in their
_device structures, it may be worth putting it into struct device (we've
discsussed that already).

I'm not sure which of (1) - (3) are the most suitable for the platform bus
type.  For PCI I'd probably choose (2), because the current PCI bus type's
dev_pm_ops callbacks are tailored to system-wide power transitions.
Moreover, PCI devices can generally be put first into D1, then into D2 and
finally into D3, which only makes sense at run time, and some of them may
have to be put back into the full power state before a system-wide transition
(apparently, we'll need a separate flag to mark such devices).

Of course, if you decide to add separate run-time PM callbacks for the
platform bus type, you won't need to wrap its dev_pm_ops callbacks any more,
but you'll need to modify them to check the appropriate flag(s).  For example,
you may choose to use a two-bit pm_suspend_level field such that

* if pm_suspend_level = 1, platform_pm_prepare() will return immediately
* if pm_suspend_level = 2, platform_pm_prepare() and platform_pm_suspend()
  will return immediately
* if pm_suspend_level = 3, platform_pm_prepare(), platform_pm_suspend()
  and platform_pm_suspend_noirq() will return immediately

(and analogously for the hibernation callbacks) and make your run-time PM
callbacks set this field appropriately.

Thanks,
Rafael

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Rafael J. Wysocki]

On Wednesday 27 May 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Wrap the platform device bus dev_pm_ops to allow runtime
> pm and regular suspend and resume to coexist.
> 
> Platform device data is extended with flags that allow
> us to keep track of which dev_pm_ops that has been called.
> 
> Basically, if a device has been frozen by the runtime pm
> code, don't call ->freeze() again when hibernating.
> 
> Architecture code can use platform_runtime_dev_pm_ops to
> call driver dev_pm_ops associated with a certain device.
> 
> Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  This is a bit of a hack, any better way to wrap dev_pm_ops?

I'm not really sure you need to wrap them at all.

There are a few choices:

(1) You can use the platform_pm_* functions directly for run-time PM, but
in that case you'll need to make sure that the "suspend" ones return 0
immediately when called during system-wide suspend or hibernation (there's the
question whether the "resume" ones should still put the device into the full
power state in that case).  For this purpose you can add a single flag to
struct platform_device and set it for devices that have already been
"suspended" (this flag, when set, will make all of the "suspend" callbacks
return 0 without doing anything until the device is "resumed").

(2) You can add separate platform callbacks for run-time PM that will execute
the drivers' dev_pm_ops callbacks and presumably do something else (I don't
know what that may be for platform devices, though).  In that case, again,
adding a flag to struct platform_device and making platform_pm_* check it
should be sufficient to prevent devices from being suspended twice in a row.

(3) You can add separate platform callbacks for run-time PM for both the
bus type and the drivers, in which dev_pm_ops will be totally separate from
these new callbacks, although of course you'll need provide some kind of
synchronization bettween them all.  That also may be done through a flag
in struct platform_device IMO.

Now, since other bus types will most probably also need a flag in their
_device structures, it may be worth putting it into struct device (we've
discsussed that already).

I'm not sure which of (1) - (3) are the most suitable for the platform bus
type.  For PCI I'd probably choose (2), because the current PCI bus type's
dev_pm_ops callbacks are tailored to system-wide power transitions.
Moreover, PCI devices can generally be put first into D1, then into D2 and
finally into D3, which only makes sense at run time, and some of them may
have to be put back into the full power state before a system-wide transition
(apparently, we'll need a separate flag to mark such devices).

Of course, if you decide to add separate run-time PM callbacks for the
platform bus type, you won't need to wrap its dev_pm_ops callbacks any more,
but you'll need to modify them to check the appropriate flag(s).  For example,
you may choose to use a two-bit pm_suspend_level field such that

* if pm_suspend_level = 1, platform_pm_prepare() will return immediately
* if pm_suspend_level = 2, platform_pm_prepare() and platform_pm_suspend()
  will return immediately
* if pm_suspend_level = 3, platform_pm_prepare(), platform_pm_suspend()
  and platform_pm_suspend_noirq() will return immediately

(and analogously for the hibernation callbacks) and make your run-time PM
callbacks set this field appropriately.

Thanks,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Thursday 28 May 2009, Alan Stern wrote:
> On Thu, 28 May 2009, Rafael J. Wysocki wrote:
> 
> > I think we'll need one separate workqueue for run-time PM in general, so that
> > bus types don't introduce their own workqueues for this purpose.  IMO one
> > system-wide run-time PM workqueue should be sufficient (it could also be
> > used for the suspend blockers BTW).
> > 
> > So, perhaps it makes sense to implement such a workqueue at the core level?
> > Thoughts?
> 
> That's fine with me.  This new workqueue can take over the job of the 
> "ksuspend_usbd" workqueue.  But it would have to be marked as 
> freezable.

Having considered it for a while I'm not sure if freezing is the right approach
here.  Namely, the work items put into the workqueue need not be useful after
the resume any more, so perhaps we should deqeue them during suspend?

Best,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Thursday 28 May 2009, Alan Stern wrote:
> On Thu, 28 May 2009, Rafael J. Wysocki wrote:
> 
> > I think we'll need one separate workqueue for run-time PM in general, so that
> > bus types don't introduce their own workqueues for this purpose.  IMO one
> > system-wide run-time PM workqueue should be sufficient (it could also be
> > used for the suspend blockers BTW).
> > 
> > So, perhaps it makes sense to implement such a workqueue at the core level?
> > Thoughts?
> 
> That's fine with me.  This new workqueue can take over the job of the 
> "ksuspend_usbd" workqueue.  But it would have to be marked as 
> freezable.

Having considered it for a while I'm not sure if freezing is the right approach
here.  Namely, the work items put into the workqueue need not be useful after
the resume any more, so perhaps we should deqeue them during suspend?

Best,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:

> On Thursday 28 May 2009, Alan Stern wrote:
> > On Thu, 28 May 2009, Rafael J. Wysocki wrote:
> > 
> > > I think we'll need one separate workqueue for run-time PM in general, so that
> > > bus types don't introduce their own workqueues for this purpose.  IMO one
> > > system-wide run-time PM workqueue should be sufficient (it could also be
> > > used for the suspend blockers BTW).
> > > 
> > > So, perhaps it makes sense to implement such a workqueue at the core level?
> > > Thoughts?
> > 
> > That's fine with me.  This new workqueue can take over the job of the 
> > "ksuspend_usbd" workqueue.  But it would have to be marked as 
> > freezable.
> 
> Having considered it for a while I'm not sure if freezing is the right approach
> here.  Namely, the work items put into the workqueue need not be useful after
> the resume any more, so perhaps we should deqeue them during suspend?

With USB at least, this isn't true.  Some work items shouldn't be
dequeued, namely, various sorts of resume requests generated while the
driver is in interrupt context.  If they were dequeued during a system
suspend then the device wouldn't ever be woken up, because usbcore
doesn't pass system-resume messages to an autosuspended device.  The
idea is if a device was autosuspended before the system sleep then it
should remain autosuspended after the system wakes up.

Furthermore, even if the existing work items were dequeued, you'd still 
have to worry about new work items added on by drivers before they get 
suspended.  If the workqueue were allowed to run freely, we might find 
devices being autoresumed in the middle of the sleep transition!

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:

> On Thursday 28 May 2009, Alan Stern wrote:
> > On Thu, 28 May 2009, Rafael J. Wysocki wrote:
> > 
> > > I think we'll need one separate workqueue for run-time PM in general, so that
> > > bus types don't introduce their own workqueues for this purpose.  IMO one
> > > system-wide run-time PM workqueue should be sufficient (it could also be
> > > used for the suspend blockers BTW).
> > > 
> > > So, perhaps it makes sense to implement such a workqueue at the core level?
> > > Thoughts?
> > 
> > That's fine with me.  This new workqueue can take over the job of the 
> > "ksuspend_usbd" workqueue.  But it would have to be marked as 
> > freezable.
> 
> Having considered it for a while I'm not sure if freezing is the right approach
> here.  Namely, the work items put into the workqueue need not be useful after
> the resume any more, so perhaps we should deqeue them during suspend?

With USB at least, this isn't true.  Some work items shouldn't be
dequeued, namely, various sorts of resume requests generated while the
driver is in interrupt context.  If they were dequeued during a system
suspend then the device wouldn't ever be woken up, because usbcore
doesn't pass system-resume messages to an autosuspended device.  The
idea is if a device was autosuspended before the system sleep then it
should remain autosuspended after the system wakes up.

Furthermore, even if the existing work items were dequeued, you'd still 
have to worry about new work items added on by drivers before they get 
suspended.  If the workqueue were allowed to run freely, we might find 
devices being autoresumed in the middle of the sleep transition!

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Monday 01 June 2009, Alan Stern wrote:
> On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:
> 
> > On Thursday 28 May 2009, Alan Stern wrote:
> > > On Thu, 28 May 2009, Rafael J. Wysocki wrote:
> > > 
> > > > I think we'll need one separate workqueue for run-time PM in general, so that
> > > > bus types don't introduce their own workqueues for this purpose.  IMO one
> > > > system-wide run-time PM workqueue should be sufficient (it could also be
> > > > used for the suspend blockers BTW).
> > > > 
> > > > So, perhaps it makes sense to implement such a workqueue at the core level?
> > > > Thoughts?
> > > 
> > > That's fine with me.  This new workqueue can take over the job of the 
> > > "ksuspend_usbd" workqueue.  But it would have to be marked as 
> > > freezable.
> > 
> > Having considered it for a while I'm not sure if freezing is the right approach
> > here.  Namely, the work items put into the workqueue need not be useful after
> > the resume any more, so perhaps we should deqeue them during suspend?
> 
> With USB at least, this isn't true.  Some work items shouldn't be
> dequeued, namely, various sorts of resume requests generated while the
> driver is in interrupt context.  If they were dequeued during a system
> suspend then the device wouldn't ever be woken up, because usbcore
> doesn't pass system-resume messages to an autosuspended device.  The
> idea is if a device was autosuspended before the system sleep then it
> should remain autosuspended after the system wakes up.

Hmm, well, I'm not sure, really.  During a system-wide resume, does it really
matter if devices were autosuspended before the preceding suspend or they have
been suspended by the PM core?  Also, what happens if the platform firmware
resumes the autosuspended devices before passing control to the kernel during
system-wide resume?  How do we handle that?

Moreover, what if the autosuspended device is no longer present in the system
after a system-wide resume.  Are we still going to attempt to autoresume it?

> Furthermore, even if the existing work items were dequeued, you'd still 
> have to worry about new work items added on by drivers before they get 
> suspended.  If the workqueue were allowed to run freely, we might find 
> devices being autoresumed in the middle of the sleep transition!

Unless there's some other kind of synchronization between the workqueue and the
core suspend-resume code.

IMO, it would be convenient to treat every system-wide suspend (or hibernation)
as a cancellation point for all of the pending autosuspend and autoresume
requests and to treat devices autosuspended before that point as just
suspended.  Of course, the bus type and device driver suspend callbacks would
have to be prepared to handle this situation cleanly.

Best,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Monday 01 June 2009, Alan Stern wrote:
> On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:
> 
> > On Thursday 28 May 2009, Alan Stern wrote:
> > > On Thu, 28 May 2009, Rafael J. Wysocki wrote:
> > > 
> > > > I think we'll need one separate workqueue for run-time PM in general, so that
> > > > bus types don't introduce their own workqueues for this purpose.  IMO one
> > > > system-wide run-time PM workqueue should be sufficient (it could also be
> > > > used for the suspend blockers BTW).
> > > > 
> > > > So, perhaps it makes sense to implement such a workqueue at the core level?
> > > > Thoughts?
> > > 
> > > That's fine with me.  This new workqueue can take over the job of the 
> > > "ksuspend_usbd" workqueue.  But it would have to be marked as 
> > > freezable.
> > 
> > Having considered it for a while I'm not sure if freezing is the right approach
> > here.  Namely, the work items put into the workqueue need not be useful after
> > the resume any more, so perhaps we should deqeue them during suspend?
> 
> With USB at least, this isn't true.  Some work items shouldn't be
> dequeued, namely, various sorts of resume requests generated while the
> driver is in interrupt context.  If they were dequeued during a system
> suspend then the device wouldn't ever be woken up, because usbcore
> doesn't pass system-resume messages to an autosuspended device.  The
> idea is if a device was autosuspended before the system sleep then it
> should remain autosuspended after the system wakes up.

Hmm, well, I'm not sure, really.  During a system-wide resume, does it really
matter if devices were autosuspended before the preceding suspend or they have
been suspended by the PM core?  Also, what happens if the platform firmware
resumes the autosuspended devices before passing control to the kernel during
system-wide resume?  How do we handle that?

Moreover, what if the autosuspended device is no longer present in the system
after a system-wide resume.  Are we still going to attempt to autoresume it?

> Furthermore, even if the existing work items were dequeued, you'd still 
> have to worry about new work items added on by drivers before they get 
> suspended.  If the workqueue were allowed to run freely, we might find 
> devices being autoresumed in the middle of the sleep transition!

Unless there's some other kind of synchronization between the workqueue and the
core suspend-resume code.

IMO, it would be convenient to treat every system-wide suspend (or hibernation)
as a cancellation point for all of the pending autosuspend and autoresume
requests and to treat devices autosuspended before that point as just
suspended.  Of course, the bus type and device driver suspend callbacks would
have to be prepared to handle this situation cleanly.

Best,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:

> > With USB at least, this isn't true.  Some work items shouldn't be
> > dequeued, namely, various sorts of resume requests generated while the
> > driver is in interrupt context.  If they were dequeued during a system
> > suspend then the device wouldn't ever be woken up, because usbcore
> > doesn't pass system-resume messages to an autosuspended device.  The
> > idea is if a device was autosuspended before the system sleep then it
> > should remain autosuspended after the system wakes up.
> 
> Hmm, well, I'm not sure, really.  During a system-wide resume, does it really
> matter if devices were autosuspended before the preceding suspend or they have
> been suspended by the PM core?

It's partly a question of avoiding unnecessary activity.  Is there any
reason to resume a device if it's just going to be autosuspended again
a few seconds later?

It's also a matter of principle.  A system sleep should be as
transparent as possible (except for the passage of time, of course).  
When the sleep is over, devices should be in the same state as before
it began.

>  Also, what happens if the platform firmware
> resumes the autosuspended devices before passing control to the kernel during
> system-wide resume?  How do we handle that?

In the case of USB, it's not possible to resume a suspended device
without first resuming and using a host controller.  The host
controller driver will realize that the firmware has taken over the
controller and will reset the controller.

For devices that were not autosuspended, this action converts an
ordinary resume into a reset-resume.  A different driver method is
called, and drivers that don't support reset-resume are automatically
unbound from their devices (and then reprobed afterward).

For devices that were autosuspended, the controller reset will put them 
back into a suspended state.  They will remain that way until they are 
autoresumed -- and of course it will be an "auto-reset-resume".

What happens if the firmware is sufficiently sneaky that it manages to
fool the host controller driver?  In that case we end up with a device
that really is at full power even though the kernel thinks it is
autosuspended.  When we finally try to autoresume it, we will realize
that it wasn't suspended after all.

> Moreover, what if the autosuspended device is no longer present in the system
> after a system-wide resume.  Are we still going to attempt to autoresume it?

When the device or perhaps its parent hub is resumed, we will realize 
the device is gone.  An attempted autoresume will simply fail.  Until 
that happens, the kernel may believe the device is present when in fact 
it isn't.

> > Furthermore, even if the existing work items were dequeued, you'd still 
> > have to worry about new work items added on by drivers before they get 
> > suspended.  If the workqueue were allowed to run freely, we might find 
> > devices being autoresumed in the middle of the sleep transition!
> 
> Unless there's some other kind of synchronization between the workqueue and the
> core suspend-resume code.

So the work routine would somehow be aware when a system sleep is in
progress, at which point it would return immediately without doing
anything?  That seems racey.  Isn't it easier just to freeze the
workqueue?

> IMO, it would be convenient to treat every system-wide suspend (or hibernation)
> as a cancellation point for all of the pending autosuspend and autoresume
> requests and to treat devices autosuspended before that point as just
> suspended.  Of course, the bus type and device driver suspend callbacks would
> have to be prepared to handle this situation cleanly.

For pending autosuspend, okay.  The device was powered up before the
system sleep, it remains powered up afterward, and another autosuspend
will occur in due course.

For pending autoresume requests coming from user I/O calls, again okay.  
There can't be any of these because userspace is frozen, and besides,
they wouldn't go through the workqueue anyway.

For autoresume requests coming from an interrupt handler, I'm not so 
sure.  I don't know of any good examples -- in fact there don't seem to 
be any examples at the moment.  (The USB core supports autoresume 
requests in interrupt context, AFAICS no driver uses them.)  So maybe 
it's premature to worry about this case.

Lastly, we have resume requests coming from a device -- i.e., remote
wakeup requests.  This is a little tricky.  Suppose you have remote
wakeup enabled, and you press a key on your USB keyboard just as the
computer is going to sleep.  Should this cause the sleep to be aborted?  
Should it cause the computer to wake back up again right away?  Should
the keystroke be ignored?

More to the point, suppose you plug in a new USB device as the computer
is going to sleep.  When the computer wakes up, shouldn't it realize
that a new device has been connected?  This is the same problem that
other people have been discussing -- what should happen to wakeup IRQs
during the time the system is suspending?  Will they get delivered 
later on (and when drivers are ready to receive them)?

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:

> > With USB at least, this isn't true.  Some work items shouldn't be
> > dequeued, namely, various sorts of resume requests generated while the
> > driver is in interrupt context.  If they were dequeued during a system
> > suspend then the device wouldn't ever be woken up, because usbcore
> > doesn't pass system-resume messages to an autosuspended device.  The
> > idea is if a device was autosuspended before the system sleep then it
> > should remain autosuspended after the system wakes up.
> 
> Hmm, well, I'm not sure, really.  During a system-wide resume, does it really
> matter if devices were autosuspended before the preceding suspend or they have
> been suspended by the PM core?

It's partly a question of avoiding unnecessary activity.  Is there any
reason to resume a device if it's just going to be autosuspended again
a few seconds later?

It's also a matter of principle.  A system sleep should be as
transparent as possible (except for the passage of time, of course).  
When the sleep is over, devices should be in the same state as before
it began.

>  Also, what happens if the platform firmware
> resumes the autosuspended devices before passing control to the kernel during
> system-wide resume?  How do we handle that?

In the case of USB, it's not possible to resume a suspended device
without first resuming and using a host controller.  The host
controller driver will realize that the firmware has taken over the
controller and will reset the controller.

For devices that were not autosuspended, this action converts an
ordinary resume into a reset-resume.  A different driver method is
called, and drivers that don't support reset-resume are automatically
unbound from their devices (and then reprobed afterward).

For devices that were autosuspended, the controller reset will put them 
back into a suspended state.  They will remain that way until they are 
autoresumed -- and of course it will be an "auto-reset-resume".

What happens if the firmware is sufficiently sneaky that it manages to
fool the host controller driver?  In that case we end up with a device
that really is at full power even though the kernel thinks it is
autosuspended.  When we finally try to autoresume it, we will realize
that it wasn't suspended after all.

> Moreover, what if the autosuspended device is no longer present in the system
> after a system-wide resume.  Are we still going to attempt to autoresume it?

When the device or perhaps its parent hub is resumed, we will realize 
the device is gone.  An attempted autoresume will simply fail.  Until 
that happens, the kernel may believe the device is present when in fact 
it isn't.

> > Furthermore, even if the existing work items were dequeued, you'd still 
> > have to worry about new work items added on by drivers before they get 
> > suspended.  If the workqueue were allowed to run freely, we might find 
> > devices being autoresumed in the middle of the sleep transition!
> 
> Unless there's some other kind of synchronization between the workqueue and the
> core suspend-resume code.

So the work routine would somehow be aware when a system sleep is in
progress, at which point it would return immediately without doing
anything?  That seems racey.  Isn't it easier just to freeze the
workqueue?

> IMO, it would be convenient to treat every system-wide suspend (or hibernation)
> as a cancellation point for all of the pending autosuspend and autoresume
> requests and to treat devices autosuspended before that point as just
> suspended.  Of course, the bus type and device driver suspend callbacks would
> have to be prepared to handle this situation cleanly.

For pending autosuspend, okay.  The device was powered up before the
system sleep, it remains powered up afterward, and another autosuspend
will occur in due course.

For pending autoresume requests coming from user I/O calls, again okay.  
There can't be any of these because userspace is frozen, and besides,
they wouldn't go through the workqueue anyway.

For autoresume requests coming from an interrupt handler, I'm not so 
sure.  I don't know of any good examples -- in fact there don't seem to 
be any examples at the moment.  (The USB core supports autoresume 
requests in interrupt context, AFAICS no driver uses them.)  So maybe 
it's premature to worry about this case.

Lastly, we have resume requests coming from a device -- i.e., remote
wakeup requests.  This is a little tricky.  Suppose you have remote
wakeup enabled, and you press a key on your USB keyboard just as the
computer is going to sleep.  Should this cause the sleep to be aborted?  
Should it cause the computer to wake back up again right away?  Should
the keystroke be ignored?

More to the point, suppose you plug in a new USB device as the computer
is going to sleep.  When the computer wakes up, shouldn't it realize
that a new device has been connected?  This is the same problem that
other people have been discussing -- what should happen to wakeup IRQs
during the time the system is suspending?  Will they get delivered 
later on (and when drivers are ready to receive them)?

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Alan Stern wrote:
> On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:
> 
> > > With USB at least, this isn't true.  Some work items shouldn't be
> > > dequeued, namely, various sorts of resume requests generated while the
> > > driver is in interrupt context.  If they were dequeued during a system
> > > suspend then the device wouldn't ever be woken up, because usbcore
> > > doesn't pass system-resume messages to an autosuspended device.  The
> > > idea is if a device was autosuspended before the system sleep then it
> > > should remain autosuspended after the system wakes up.
> > 
> > Hmm, well, I'm not sure, really.  During a system-wide resume, does it really
> > matter if devices were autosuspended before the preceding suspend or they have
> > been suspended by the PM core?
> 
> It's partly a question of avoiding unnecessary activity.  Is there any
> reason to resume a device if it's just going to be autosuspended again
> a few seconds later?

Well, I'm not sure if we know in advance what's going to happen to the device
right after a system-wide resume.  It may be a totally different situation from
the device activity point of view.

> It's also a matter of principle.  A system sleep should be as
> transparent as possible (except for the passage of time, of course).  
> When the sleep is over, devices should be in the same state as before
> it began.

As a matter of fact, this need not be true at all.  For example, consider a
wireless network adapter that was assosicated with specific access point
before a system-wide suspend, but this access point is not present any more
after the subsequent system-wide resume.

I really don't think we can predict what's going to happen to a device after
a system-wide resume, so IMO we should assume that the device will be necessary
immediately after the resume and we should resume it.  It still can autosuspend
normally afterwards if it's not needed after all.

> >  Also, what happens if the platform firmware
> > resumes the autosuspended devices before passing control to the kernel during
> > system-wide resume?  How do we handle that?
> 
> In the case of USB, it's not possible to resume a suspended device
> without first resuming and using a host controller.  The host
> controller driver will realize that the firmware has taken over the
> controller and will reset the controller.
> 
> For devices that were not autosuspended, this action converts an
> ordinary resume into a reset-resume.  A different driver method is
> called, and drivers that don't support reset-resume are automatically
> unbound from their devices (and then reprobed afterward).
> 
> For devices that were autosuspended, the controller reset will put them 
> back into a suspended state.  They will remain that way until they are 
> autoresumed -- and of course it will be an "auto-reset-resume".

I'm not sure if this is doable so easily for PCI devices, for one example.
Most probably, they will come up in D0 after the system-wide suspend and
the PCI resume code will have to suspend them directly so that they remain
"autosuspended" after the resume.

> What happens if the firmware is sufficiently sneaky that it manages to
> fool the host controller driver?  In that case we end up with a device
> that really is at full power even though the kernel thinks it is
> autosuspended.  When we finally try to autoresume it, we will realize
> that it wasn't suspended after all.
> 
> > Moreover, what if the autosuspended device is no longer present in the system
> > after a system-wide resume.  Are we still going to attempt to autoresume it?
> 
> When the device or perhaps its parent hub is resumed, we will realize 
> the device is gone.  An attempted autoresume will simply fail.  Until 
> that happens, the kernel may believe the device is present when in fact 
> it isn't.
> 
> > > Furthermore, even if the existing work items were dequeued, you'd still 
> > > have to worry about new work items added on by drivers before they get 
> > > suspended.  If the workqueue were allowed to run freely, we might find 
> > > devices being autoresumed in the middle of the sleep transition!
> > 
> > Unless there's some other kind of synchronization between the workqueue and the
> > core suspend-resume code.
> 
> So the work routine would somehow be aware when a system sleep is in
> progress, at which point it would return immediately without doing
> anything?  That seems racey.  Isn't it easier just to freeze the
> workqueue?

Well, yes and no.  Namely, if we freeze the workqueue, all of the bus types
using it will have to check the vaildity of operations carried out by it,
because of the possibility that system-wide power transition took place in the
meantime and changed the state of the device.  Also, please take the PCI
example into consideration.

> > IMO, it would be convenient to treat every system-wide suspend (or hibernation)
> > as a cancellation point for all of the pending autosuspend and autoresume
> > requests and to treat devices autosuspended before that point as just
> > suspended.  Of course, the bus type and device driver suspend callbacks would
> > have to be prepared to handle this situation cleanly.
> 
> For pending autosuspend, okay.  The device was powered up before the
> system sleep, it remains powered up afterward, and another autosuspend
> will occur in due course.
> 
> For pending autoresume requests coming from user I/O calls, again okay.  
> There can't be any of these because userspace is frozen, and besides,
> they wouldn't go through the workqueue anyway.
> 
> For autoresume requests coming from an interrupt handler, I'm not so 
> sure.  I don't know of any good examples -- in fact there don't seem to 
> be any examples at the moment.  (The USB core supports autoresume 
> requests in interrupt context, AFAICS no driver uses them.)  So maybe 
> it's premature to worry about this case.

Hmm.  Can you please describe a theoretical situation in which this kind
of autoresume request will appear?  I'm not really sure what you mean here. :-)

> Lastly, we have resume requests coming from a device -- i.e., remote
> wakeup requests.  This is a little tricky.  Suppose you have remote
> wakeup enabled, and you press a key on your USB keyboard just as the
> computer is going to sleep.  Should this cause the sleep to be aborted?  
> Should it cause the computer to wake back up again right away?  Should
> the keystroke be ignored?

This is a difficult problem in general.  I _think_ we should ignore such
autoresume requests for devices that are not marked as wake-up ones
and we should abort the suspend if there's a request from a wake-up device.
 
> More to the point, suppose you plug in a new USB device as the computer
> is going to sleep.  When the computer wakes up, shouldn't it realize
> that a new device has been connected?  This is the same problem that
> other people have been discussing -- what should happen to wakeup IRQs
> during the time the system is suspending?  Will they get delivered 
> later on (and when drivers are ready to receive them)?

I think they should abort the suspend unless it's too late (eg. the suspend
sequence has reached sysdev_suspend()).

Anyway, below is a prototype implementation of a generic runtime PM workqueue,
just for discussion.

pm_schedule_suspend() and pm_schedule_resume() are to be called by bus types
that are requesting the autosuspend or autoresume, respectively, of a device.
The supposed control flow is like this:
* bus type calls pm_schedule_suspend() to schedule autosuspend of a device
* at the right time the PM workqueue executes the bus type's ->autosuspend()
  callback which is responsible for calling the device driver to suspend the
  device (the way in which this is done is totally bus type-specific)
* if there's a resume event for the device, it's delivered to the bus type
  driver (in a bus type-specific way) and the bus type schedules autoresume of
  the device by calling pm_schedule_resume()
* the PM workqueue executes the bus type's ->autoresume() as soon as it can
  and this callback is responsible for calling the device driver to resume the
  device (the way in which this is done is totally bus type-specific).

Thanks,
Rafael

---
 drivers/base/power/main.c |   11 ++++--
 include/linux/pm.h        |   16 +++++++++
 kernel/power/main.c       |   75 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/pm.h
=================================--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -22,6 +22,7 @@
 #define _LINUX_PM_H
 
 #include <linux/list.h>
+#include <linux/workqueue.h>
 
 /*
  * Callbacks for platform drivers to implement.
@@ -182,6 +183,8 @@ struct dev_pm_ops {
 	int (*thaw_noirq)(struct device *dev);
 	int (*poweroff_noirq)(struct device *dev);
 	int (*restore_noirq)(struct device *dev);
+	int (*autosuspend)(struct device *dev);
+	int (*autoresume)(struct device *dev);
 };
 
 /**
@@ -323,8 +326,21 @@ struct dev_pm_info {
 #ifdef	CONFIG_PM_SLEEP
 	struct list_head	entry;
 #endif
+	struct delayed_work	dwork;
 };
 
+extern void pm_schedule_suspend(struct device *dev, unsigned long delay);
+extern void pm_schedule_resume(struct device *dev);
+
+extern bool transition_started;
+
+static inline bool pm_transition_in_progress(void)
+{
+	return transition_started;
+}
+
+extern void pm_flush_workqueue(void);
+
 /*
  * The PM_EVENT_ messages are also used by drivers implementing the legacy
  * suspend framework, based on the ->suspend() and ->resume() callbacks common
Index: linux-2.6/kernel/power/main.c
=================================--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -22,6 +22,8 @@
 #include <linux/freezer.h>
 #include <linux/vmstat.h>
 #include <linux/syscalls.h>
+#include <linux/workqueue.h>
+#include <linux/device.h>
 
 #include "power.h"
 
@@ -578,12 +580,20 @@ static struct attribute_group attr_group
 	.attrs = g,
 };
 
+static int __init pm_start_workqueue(void);
 
 static int __init pm_init(void)
 {
+	int error;
+
+	error = pm_start_workqueue();
+	if (error)
+		return error;
+
 	power_kobj = kobject_create_and_add("power", NULL);
 	if (!power_kobj)
 		return -ENOMEM;
+
 	return sysfs_create_group(power_kobj, &attr_group);
 }
 
@@ -729,3 +739,68 @@ done:
 late_initcall(test_suspend);
 
 #endif /* CONFIG_PM_TEST_SUSPEND */
+
+
+static struct workqueue_struct *pm_wq;
+
+static int __init pm_start_workqueue(void)
+{
+	pm_wq = create_singlethread_workqueue("pm");
+
+	return pm_wq ? 0 : -ENOMEM;
+}
+
+void pm_flush_workqueue(void)
+{
+	flush_workqueue(pm_wq);
+}
+
+static inline struct device *pm_dwork_to_device(struct delayed_work *dwork)
+{
+	struct dev_pm_info *d = container_of(dwork, struct dev_pm_info, dwork);
+	return container_of(d, struct device, power);
+}
+
+static void pm_autosuspend(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct device *dev = pm_dwork_to_device(dwork);
+
+	device_pm_lock();
+	if (dev && dev->bus && dev->bus->pm && dev->bus->pm->autosuspend
+	    && !pm_transition_in_progress())
+		dev->bus->pm->autosuspend(dev);
+	device_pm_unlock();
+}
+
+void pm_schedule_suspend(struct device *dev, unsigned long delay)
+{
+	device_pm_lock();
+	if (!pm_transition_in_progress()) {
+		INIT_DELAYED_WORK(&dev->power.dwork, pm_autosuspend);
+		queue_delayed_work(pm_wq, &dev->power.dwork, delay);
+	}
+	device_pm_unlock();
+}
+
+static void pm_autoresume(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct device *dev = pm_dwork_to_device(dwork);
+
+	device_pm_lock();
+	if (dev && dev->bus && dev->bus->pm && dev->bus->pm->autoresume
+	    && !pm_transition_in_progress())
+		dev->bus->pm->autoresume(dev);
+	device_pm_unlock();
+}
+
+void pm_schedule_resume(struct device *dev)
+{
+	device_pm_lock();
+	if (!pm_transition_in_progress()) {
+		INIT_WORK(&dev->power.dwork.work, pm_autoresume);
+		queue_work(pm_wq, &dev->power.dwork.work);
+	}
+	device_pm_unlock();
+}
Index: linux-2.6/drivers/base/power/main.c
=================================--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -46,7 +46,7 @@ static DEFINE_MUTEX(dpm_list_mtx);
  * Set once the preparation of devices for a PM transition has started, reset
  * before starting to resume devices.  Protected by dpm_list_mtx.
  */
-static bool transition_started;
+bool transition_started;
 
 /**
  *	device_pm_lock - lock the list of active devices used by the PM core
@@ -436,7 +436,6 @@ static void dpm_resume(pm_message_t stat
 
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
-	transition_started = false;
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 
@@ -461,6 +460,7 @@ static void dpm_resume(pm_message_t stat
 		put_device(dev);
 	}
 	list_splice(&list, &dpm_list);
+	transition_started = false;
 	mutex_unlock(&dpm_list_mtx);
 }
 
@@ -752,9 +752,14 @@ static int dpm_prepare(pm_message_t stat
 	struct list_head list;
 	int error = 0;
 
-	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
 	transition_started = true;
+	mutex_unlock(&dpm_list_mtx);
+
+	pm_flush_workqueue();
+
+	INIT_LIST_HEAD(&list);
+	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Alan Stern wrote:
> On Mon, 1 Jun 2009, Rafael J. Wysocki wrote:
> 
> > > With USB at least, this isn't true.  Some work items shouldn't be
> > > dequeued, namely, various sorts of resume requests generated while the
> > > driver is in interrupt context.  If they were dequeued during a system
> > > suspend then the device wouldn't ever be woken up, because usbcore
> > > doesn't pass system-resume messages to an autosuspended device.  The
> > > idea is if a device was autosuspended before the system sleep then it
> > > should remain autosuspended after the system wakes up.
> > 
> > Hmm, well, I'm not sure, really.  During a system-wide resume, does it really
> > matter if devices were autosuspended before the preceding suspend or they have
> > been suspended by the PM core?
> 
> It's partly a question of avoiding unnecessary activity.  Is there any
> reason to resume a device if it's just going to be autosuspended again
> a few seconds later?

Well, I'm not sure if we know in advance what's going to happen to the device
right after a system-wide resume.  It may be a totally different situation from
the device activity point of view.

> It's also a matter of principle.  A system sleep should be as
> transparent as possible (except for the passage of time, of course).  
> When the sleep is over, devices should be in the same state as before
> it began.

As a matter of fact, this need not be true at all.  For example, consider a
wireless network adapter that was assosicated with specific access point
before a system-wide suspend, but this access point is not present any more
after the subsequent system-wide resume.

I really don't think we can predict what's going to happen to a device after
a system-wide resume, so IMO we should assume that the device will be necessary
immediately after the resume and we should resume it.  It still can autosuspend
normally afterwards if it's not needed after all.

> >  Also, what happens if the platform firmware
> > resumes the autosuspended devices before passing control to the kernel during
> > system-wide resume?  How do we handle that?
> 
> In the case of USB, it's not possible to resume a suspended device
> without first resuming and using a host controller.  The host
> controller driver will realize that the firmware has taken over the
> controller and will reset the controller.
> 
> For devices that were not autosuspended, this action converts an
> ordinary resume into a reset-resume.  A different driver method is
> called, and drivers that don't support reset-resume are automatically
> unbound from their devices (and then reprobed afterward).
> 
> For devices that were autosuspended, the controller reset will put them 
> back into a suspended state.  They will remain that way until they are 
> autoresumed -- and of course it will be an "auto-reset-resume".

I'm not sure if this is doable so easily for PCI devices, for one example.
Most probably, they will come up in D0 after the system-wide suspend and
the PCI resume code will have to suspend them directly so that they remain
"autosuspended" after the resume.

> What happens if the firmware is sufficiently sneaky that it manages to
> fool the host controller driver?  In that case we end up with a device
> that really is at full power even though the kernel thinks it is
> autosuspended.  When we finally try to autoresume it, we will realize
> that it wasn't suspended after all.
> 
> > Moreover, what if the autosuspended device is no longer present in the system
> > after a system-wide resume.  Are we still going to attempt to autoresume it?
> 
> When the device or perhaps its parent hub is resumed, we will realize 
> the device is gone.  An attempted autoresume will simply fail.  Until 
> that happens, the kernel may believe the device is present when in fact 
> it isn't.
> 
> > > Furthermore, even if the existing work items were dequeued, you'd still 
> > > have to worry about new work items added on by drivers before they get 
> > > suspended.  If the workqueue were allowed to run freely, we might find 
> > > devices being autoresumed in the middle of the sleep transition!
> > 
> > Unless there's some other kind of synchronization between the workqueue and the
> > core suspend-resume code.
> 
> So the work routine would somehow be aware when a system sleep is in
> progress, at which point it would return immediately without doing
> anything?  That seems racey.  Isn't it easier just to freeze the
> workqueue?

Well, yes and no.  Namely, if we freeze the workqueue, all of the bus types
using it will have to check the vaildity of operations carried out by it,
because of the possibility that system-wide power transition took place in the
meantime and changed the state of the device.  Also, please take the PCI
example into consideration.

> > IMO, it would be convenient to treat every system-wide suspend (or hibernation)
> > as a cancellation point for all of the pending autosuspend and autoresume
> > requests and to treat devices autosuspended before that point as just
> > suspended.  Of course, the bus type and device driver suspend callbacks would
> > have to be prepared to handle this situation cleanly.
> 
> For pending autosuspend, okay.  The device was powered up before the
> system sleep, it remains powered up afterward, and another autosuspend
> will occur in due course.
> 
> For pending autoresume requests coming from user I/O calls, again okay.  
> There can't be any of these because userspace is frozen, and besides,
> they wouldn't go through the workqueue anyway.
> 
> For autoresume requests coming from an interrupt handler, I'm not so 
> sure.  I don't know of any good examples -- in fact there don't seem to 
> be any examples at the moment.  (The USB core supports autoresume 
> requests in interrupt context, AFAICS no driver uses them.)  So maybe 
> it's premature to worry about this case.

Hmm.  Can you please describe a theoretical situation in which this kind
of autoresume request will appear?  I'm not really sure what you mean here. :-)

> Lastly, we have resume requests coming from a device -- i.e., remote
> wakeup requests.  This is a little tricky.  Suppose you have remote
> wakeup enabled, and you press a key on your USB keyboard just as the
> computer is going to sleep.  Should this cause the sleep to be aborted?  
> Should it cause the computer to wake back up again right away?  Should
> the keystroke be ignored?

This is a difficult problem in general.  I _think_ we should ignore such
autoresume requests for devices that are not marked as wake-up ones
and we should abort the suspend if there's a request from a wake-up device.
 
> More to the point, suppose you plug in a new USB device as the computer
> is going to sleep.  When the computer wakes up, shouldn't it realize
> that a new device has been connected?  This is the same problem that
> other people have been discussing -- what should happen to wakeup IRQs
> during the time the system is suspending?  Will they get delivered 
> later on (and when drivers are ready to receive them)?

I think they should abort the suspend unless it's too late (eg. the suspend
sequence has reached sysdev_suspend()).

Anyway, below is a prototype implementation of a generic runtime PM workqueue,
just for discussion.

pm_schedule_suspend() and pm_schedule_resume() are to be called by bus types
that are requesting the autosuspend or autoresume, respectively, of a device.
The supposed control flow is like this:
* bus type calls pm_schedule_suspend() to schedule autosuspend of a device
* at the right time the PM workqueue executes the bus type's ->autosuspend()
  callback which is responsible for calling the device driver to suspend the
  device (the way in which this is done is totally bus type-specific)
* if there's a resume event for the device, it's delivered to the bus type
  driver (in a bus type-specific way) and the bus type schedules autoresume of
  the device by calling pm_schedule_resume()
* the PM workqueue executes the bus type's ->autoresume() as soon as it can
  and this callback is responsible for calling the device driver to resume the
  device (the way in which this is done is totally bus type-specific).

Thanks,
Rafael

---
 drivers/base/power/main.c |   11 ++++--
 include/linux/pm.h        |   16 +++++++++
 kernel/power/main.c       |   75 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -22,6 +22,7 @@
 #define _LINUX_PM_H
 
 #include <linux/list.h>
+#include <linux/workqueue.h>
 
 /*
  * Callbacks for platform drivers to implement.
@@ -182,6 +183,8 @@ struct dev_pm_ops {
 	int (*thaw_noirq)(struct device *dev);
 	int (*poweroff_noirq)(struct device *dev);
 	int (*restore_noirq)(struct device *dev);
+	int (*autosuspend)(struct device *dev);
+	int (*autoresume)(struct device *dev);
 };
 
 /**
@@ -323,8 +326,21 @@ struct dev_pm_info {
 #ifdef	CONFIG_PM_SLEEP
 	struct list_head	entry;
 #endif
+	struct delayed_work	dwork;
 };
 
+extern void pm_schedule_suspend(struct device *dev, unsigned long delay);
+extern void pm_schedule_resume(struct device *dev);
+
+extern bool transition_started;
+
+static inline bool pm_transition_in_progress(void)
+{
+	return transition_started;
+}
+
+extern void pm_flush_workqueue(void);
+
 /*
  * The PM_EVENT_ messages are also used by drivers implementing the legacy
  * suspend framework, based on the ->suspend() and ->resume() callbacks common
Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -22,6 +22,8 @@
 #include <linux/freezer.h>
 #include <linux/vmstat.h>
 #include <linux/syscalls.h>
+#include <linux/workqueue.h>
+#include <linux/device.h>
 
 #include "power.h"
 
@@ -578,12 +580,20 @@ static struct attribute_group attr_group
 	.attrs = g,
 };
 
+static int __init pm_start_workqueue(void);
 
 static int __init pm_init(void)
 {
+	int error;
+
+	error = pm_start_workqueue();
+	if (error)
+		return error;
+
 	power_kobj = kobject_create_and_add("power", NULL);
 	if (!power_kobj)
 		return -ENOMEM;
+
 	return sysfs_create_group(power_kobj, &attr_group);
 }
 
@@ -729,3 +739,68 @@ done:
 late_initcall(test_suspend);
 
 #endif /* CONFIG_PM_TEST_SUSPEND */
+
+
+static struct workqueue_struct *pm_wq;
+
+static int __init pm_start_workqueue(void)
+{
+	pm_wq = create_singlethread_workqueue("pm");
+
+	return pm_wq ? 0 : -ENOMEM;
+}
+
+void pm_flush_workqueue(void)
+{
+	flush_workqueue(pm_wq);
+}
+
+static inline struct device *pm_dwork_to_device(struct delayed_work *dwork)
+{
+	struct dev_pm_info *d = container_of(dwork, struct dev_pm_info, dwork);
+	return container_of(d, struct device, power);
+}
+
+static void pm_autosuspend(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct device *dev = pm_dwork_to_device(dwork);
+
+	device_pm_lock();
+	if (dev && dev->bus && dev->bus->pm && dev->bus->pm->autosuspend
+	    && !pm_transition_in_progress())
+		dev->bus->pm->autosuspend(dev);
+	device_pm_unlock();
+}
+
+void pm_schedule_suspend(struct device *dev, unsigned long delay)
+{
+	device_pm_lock();
+	if (!pm_transition_in_progress()) {
+		INIT_DELAYED_WORK(&dev->power.dwork, pm_autosuspend);
+		queue_delayed_work(pm_wq, &dev->power.dwork, delay);
+	}
+	device_pm_unlock();
+}
+
+static void pm_autoresume(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct device *dev = pm_dwork_to_device(dwork);
+
+	device_pm_lock();
+	if (dev && dev->bus && dev->bus->pm && dev->bus->pm->autoresume
+	    && !pm_transition_in_progress())
+		dev->bus->pm->autoresume(dev);
+	device_pm_unlock();
+}
+
+void pm_schedule_resume(struct device *dev)
+{
+	device_pm_lock();
+	if (!pm_transition_in_progress()) {
+		INIT_WORK(&dev->power.dwork.work, pm_autoresume);
+		queue_work(pm_wq, &dev->power.dwork.work);
+	}
+	device_pm_unlock();
+}
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -46,7 +46,7 @@ static DEFINE_MUTEX(dpm_list_mtx);
  * Set once the preparation of devices for a PM transition has started, reset
  * before starting to resume devices.  Protected by dpm_list_mtx.
  */
-static bool transition_started;
+bool transition_started;
 
 /**
  *	device_pm_lock - lock the list of active devices used by the PM core
@@ -436,7 +436,6 @@ static void dpm_resume(pm_message_t stat
 
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
-	transition_started = false;
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 
@@ -461,6 +460,7 @@ static void dpm_resume(pm_message_t stat
 		put_device(dev);
 	}
 	list_splice(&list, &dpm_list);
+	transition_started = false;
 	mutex_unlock(&dpm_list_mtx);
 }
 
@@ -752,9 +752,14 @@ static int dpm_prepare(pm_message_t stat
 	struct list_head list;
 	int error = 0;
 
-	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
 	transition_started = true;
+	mutex_unlock(&dpm_list_mtx);
+
+	pm_flush_workqueue();
+
+	INIT_LIST_HEAD(&list);
+	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Magnus Damm]

Hi Rafael,

Thanks for your feedback!

2009/5/30 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 27 May 2009, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Wrap the platform device bus dev_pm_ops to allow runtime
>> pm and regular suspend and resume to coexist.
>>
>> Platform device data is extended with flags that allow
>> us to keep track of which dev_pm_ops that has been called.
>>
>> Basically, if a device has been frozen by the runtime pm
>> code, don't call ->freeze() again when hibernating.
>>
>> Architecture code can use platform_runtime_dev_pm_ops to
>> call driver dev_pm_ops associated with a certain device.
>>
>> Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  This is a bit of a hack, any better way to wrap dev_pm_ops?
>
> I'm not really sure you need to wrap them at all.

I realize that I didn't explain very well why I decided to wrap
dev_pm_ops. Sorry about that. Basically there are two reasons:
1) Wrap to always have the runtime dev_pm_ops functions regardless of kconfig.
2) Wrap to make sure runtime and system-wide dev_pm_ops can coexist.

> There are a few choices:
>
> (1) You can use the platform_pm_* functions directly for run-time PM, but
> in that case you'll need to make sure that the "suspend" ones return 0
> immediately when called during system-wide suspend or hibernation (there's the
> question whether the "resume" ones should still put the device into the full
> power state in that case).  For this purpose you can add a single flag to
> struct platform_device and set it for devices that have already been
> "suspended" (this flag, when set, will make all of the "suspend" callbacks
> return 0 without doing anything until the device is "resumed").

I have to make sure that the right Kconfig bits are enabled though,
otherwise some platform_pm_* functions will be missing. So one merit
for the wrapped functions is in this patch is that they are always
there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.

Is one flag really enough? Isn't it a bit strange from the driver
point of view to always get their ->prepare() callback executed, but
->suspend() gets filtered?

The bitmap in this patch is more fine-grained, so struct
platform_device remember which of each dev_pm_ops callback that have
been called. It may be overly complex though. Also, please note that
both runtime dev_pm_ops and system-wide dev_pm_ops go through the same
dev_pm_ops that keep track of which callbacks that have been called.

Using some kind of flag(s) for coexisting with system-wide
suspend-to-ram/disk sounds good.

> (2) You can add separate platform callbacks for run-time PM that will execute
> the drivers' dev_pm_ops callbacks and presumably do something else (I don't
> know what that may be for platform devices, though).  In that case, again,
> adding a flag to struct platform_device and making platform_pm_* check it
> should be sufficient to prevent devices from being suspended twice in a row.

We may need to do other things as well - not sure - but regardless we
still need flag(s) to avoid double suspend.

> (3) You can add separate platform callbacks for run-time PM for both the
> bus type and the drivers, in which dev_pm_ops will be totally separate from
> these new callbacks, although of course you'll need provide some kind of
> synchronization bettween them all.  That also may be done through a flag
> in struct platform_device IMO.

Yeah, that's also one way. I wonder if that helps us though, I feel
that we already have a pretty wide range of callbacks in dev_pm_ops.
I'm not sure if they cover all cases we need though, I guess future
experiments will tell.

> Now, since other bus types will most probably also need a flag in their
> _device structures, it may be worth putting it into struct device (we've
> discsussed that already).

Yeah, I wonder if a single flag is enough though? Aren't we coexisting
with CONFIG_SUSPEND, CONFIG_HIBERNATION and CONFIG_KEXEC_JUMP?

> I'm not sure which of (1) - (3) are the most suitable for the platform bus
> type.  For PCI I'd probably choose (2), because the current PCI bus type's
> dev_pm_ops callbacks are tailored to system-wide power transitions.
> Moreover, PCI devices can generally be put first into D1, then into D2 and
> finally into D3, which only makes sense at run time, and some of them may
> have to be put back into the full power state before a system-wide transition
> (apparently, we'll need a separate flag to mark such devices).

If I understand (1)-(3) correctly, then I think (1) is probably the
best choice for our platform devices. I guess (2) is not very far from
(1), so if we go with (1) to begin with then we can deal with SoC
specific things in our arch code to come closer to (2) over time if
needed.

> Of course, if you decide to add separate run-time PM callbacks for the
> platform bus type, you won't need to wrap its dev_pm_ops callbacks any more,
> but you'll need to modify them to check the appropriate flag(s).  For example,
> you may choose to use a two-bit pm_suspend_level field such that
>
> * if pm_suspend_level = 1, platform_pm_prepare() will return immediately
> * if pm_suspend_level = 2, platform_pm_prepare() and platform_pm_suspend()
>  will return immediately
> * if pm_suspend_level = 3, platform_pm_prepare(), platform_pm_suspend()
>  and platform_pm_suspend_noirq() will return immediately
>
> (and analogously for the hibernation callbacks) and make your run-time PM
> callbacks set this field appropriately.

That is true, but this checking is not needed for systems where
runtime PM is disabled. Or am I misunderstanding?

Maybe it's worth to discuss how to integrate this. I suspect that this
will only affect some selected architectures to begin with, and the
rest of the code base should be unaffected by this change as long as
the runtime kconfig is disabled.

So I decided to wrap dev_pm_ops to make the impact for non runtime PM
systems as small as possible. This while giving the runtime PM case
access to all dev_pm_ops regardless of suspend/hibernation kconfig.

Does it make sense?

Thank you!

/ magnus

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Magnus Damm]

Hi Rafael,

Thanks for your feedback!

2009/5/30 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 27 May 2009, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Wrap the platform device bus dev_pm_ops to allow runtime
>> pm and regular suspend and resume to coexist.
>>
>> Platform device data is extended with flags that allow
>> us to keep track of which dev_pm_ops that has been called.
>>
>> Basically, if a device has been frozen by the runtime pm
>> code, don't call ->freeze() again when hibernating.
>>
>> Architecture code can use platform_runtime_dev_pm_ops to
>> call driver dev_pm_ops associated with a certain device.
>>
>> Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  This is a bit of a hack, any better way to wrap dev_pm_ops?
>
> I'm not really sure you need to wrap them at all.

I realize that I didn't explain very well why I decided to wrap
dev_pm_ops. Sorry about that. Basically there are two reasons:
1) Wrap to always have the runtime dev_pm_ops functions regardless of kconfig.
2) Wrap to make sure runtime and system-wide dev_pm_ops can coexist.

> There are a few choices:
>
> (1) You can use the platform_pm_* functions directly for run-time PM, but
> in that case you'll need to make sure that the "suspend" ones return 0
> immediately when called during system-wide suspend or hibernation (there's the
> question whether the "resume" ones should still put the device into the full
> power state in that case).  For this purpose you can add a single flag to
> struct platform_device and set it for devices that have already been
> "suspended" (this flag, when set, will make all of the "suspend" callbacks
> return 0 without doing anything until the device is "resumed").

I have to make sure that the right Kconfig bits are enabled though,
otherwise some platform_pm_* functions will be missing. So one merit
for the wrapped functions is in this patch is that they are always
there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.

Is one flag really enough? Isn't it a bit strange from the driver
point of view to always get their ->prepare() callback executed, but
->suspend() gets filtered?

The bitmap in this patch is more fine-grained, so struct
platform_device remember which of each dev_pm_ops callback that have
been called. It may be overly complex though. Also, please note that
both runtime dev_pm_ops and system-wide dev_pm_ops go through the same
dev_pm_ops that keep track of which callbacks that have been called.

Using some kind of flag(s) for coexisting with system-wide
suspend-to-ram/disk sounds good.

> (2) You can add separate platform callbacks for run-time PM that will execute
> the drivers' dev_pm_ops callbacks and presumably do something else (I don't
> know what that may be for platform devices, though).  In that case, again,
> adding a flag to struct platform_device and making platform_pm_* check it
> should be sufficient to prevent devices from being suspended twice in a row.

We may need to do other things as well - not sure - but regardless we
still need flag(s) to avoid double suspend.

> (3) You can add separate platform callbacks for run-time PM for both the
> bus type and the drivers, in which dev_pm_ops will be totally separate from
> these new callbacks, although of course you'll need provide some kind of
> synchronization bettween them all.  That also may be done through a flag
> in struct platform_device IMO.

Yeah, that's also one way. I wonder if that helps us though, I feel
that we already have a pretty wide range of callbacks in dev_pm_ops.
I'm not sure if they cover all cases we need though, I guess future
experiments will tell.

> Now, since other bus types will most probably also need a flag in their
> _device structures, it may be worth putting it into struct device (we've
> discsussed that already).

Yeah, I wonder if a single flag is enough though? Aren't we coexisting
with CONFIG_SUSPEND, CONFIG_HIBERNATION and CONFIG_KEXEC_JUMP?

> I'm not sure which of (1) - (3) are the most suitable for the platform bus
> type.  For PCI I'd probably choose (2), because the current PCI bus type's
> dev_pm_ops callbacks are tailored to system-wide power transitions.
> Moreover, PCI devices can generally be put first into D1, then into D2 and
> finally into D3, which only makes sense at run time, and some of them may
> have to be put back into the full power state before a system-wide transition
> (apparently, we'll need a separate flag to mark such devices).

If I understand (1)-(3) correctly, then I think (1) is probably the
best choice for our platform devices. I guess (2) is not very far from
(1), so if we go with (1) to begin with then we can deal with SoC
specific things in our arch code to come closer to (2) over time if
needed.

> Of course, if you decide to add separate run-time PM callbacks for the
> platform bus type, you won't need to wrap its dev_pm_ops callbacks any more,
> but you'll need to modify them to check the appropriate flag(s).  For example,
> you may choose to use a two-bit pm_suspend_level field such that
>
> * if pm_suspend_level = 1, platform_pm_prepare() will return immediately
> * if pm_suspend_level = 2, platform_pm_prepare() and platform_pm_suspend()
>  will return immediately
> * if pm_suspend_level = 3, platform_pm_prepare(), platform_pm_suspend()
>  and platform_pm_suspend_noirq() will return immediately
>
> (and analogously for the hibernation callbacks) and make your run-time PM
> callbacks set this field appropriately.

That is true, but this checking is not needed for systems where
runtime PM is disabled. Or am I misunderstanding?

Maybe it's worth to discuss how to integrate this. I suspect that this
will only affect some selected architectures to begin with, and the
rest of the code base should be unaffected by this change as long as
the runtime kconfig is disabled.

So I decided to wrap dev_pm_ops to make the impact for non runtime PM
systems as small as possible. This while giving the runtime PM case
access to all dev_pm_ops regardless of suspend/hibernation kconfig.

Does it make sense?

Thank you!

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Sat, May 30, 2009 at 3:18 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday 29 May 2009, Alan Stern wrote:
>> On Fri, 29 May 2009, Magnus Damm wrote:
>>
>> > > (And by the way, the _noirq ops don't run in atomic context.  They run
>> > > in process context with most interrupt deliveries disabled.  It's not
>> > > the same thing -- they are allowed to sleep.)
>> >
>> > Huh, so if they are allowed to sleep then clock events are still
>> > running. And probably some clocksource as well. I guess that's why you
>> > say "most" interrupts disabled instead of all. Sharing the timer IRQ
>> > is not allowed then?
>>
>> Rafael can tell you.  But notice I didn't say the interrupts were
>> disabled -- I said that interrupt _delivery_ was disabled.  In general
>> the interrupts themselves _are_ enabled; the kernel fields them but
>> then does not call the drivers' interrupt handler routines.
>
> That's correct, except for the platforms where __disable_irq() actually masks
> the IRQ.  I'm not sure if SuperH is one of these.

We make use of CONFIG_GENERIC_HARDIRQS so the __disable_irq() suspend
logic should work for us as well.

>> (Except perhaps for IRQs which are explicitly marked as wakeup sources...)
>
> No, they are treated just like the other ones except that we check if any of
> them are pending right at the beginning of sysdev_suspend().
>
>> > I wonder if the PM code assumes that the clock event is a sys device?
>
> Yes, it does.

I note the IRQF_TIMER check in __disable_irq(). I wonder if we can
make a second not bus-specific round where we disable only IRQF_TIMER
interrupts.

>> > We use platform drivers for clock events on SuperH...
>
> Argh.  This is going to hurt, but I'm not sure how badly.  At least on
> multiprocessor.

Luckily most boards around here are still UP. =)

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

On Sat, May 30, 2009 at 3:18 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday 29 May 2009, Alan Stern wrote:
>> On Fri, 29 May 2009, Magnus Damm wrote:
>>
>> > > (And by the way, the _noirq ops don't run in atomic context.  They run
>> > > in process context with most interrupt deliveries disabled.  It's not
>> > > the same thing -- they are allowed to sleep.)
>> >
>> > Huh, so if they are allowed to sleep then clock events are still
>> > running. And probably some clocksource as well. I guess that's why you
>> > say "most" interrupts disabled instead of all. Sharing the timer IRQ
>> > is not allowed then?
>>
>> Rafael can tell you.  But notice I didn't say the interrupts were
>> disabled -- I said that interrupt _delivery_ was disabled.  In general
>> the interrupts themselves _are_ enabled; the kernel fields them but
>> then does not call the drivers' interrupt handler routines.
>
> That's correct, except for the platforms where __disable_irq() actually masks
> the IRQ.  I'm not sure if SuperH is one of these.

We make use of CONFIG_GENERIC_HARDIRQS so the __disable_irq() suspend
logic should work for us as well.

>> (Except perhaps for IRQs which are explicitly marked as wakeup sources...)
>
> No, they are treated just like the other ones except that we check if any of
> them are pending right at the beginning of sysdev_suspend().
>
>> > I wonder if the PM code assumes that the clock event is a sys device?
>
> Yes, it does.

I note the IRQF_TIMER check in __disable_irq(). I wonder if we can
make a second not bus-specific round where we disable only IRQF_TIMER
interrupts.

>> > We use platform drivers for clock events on SuperH...
>
> Argh.  This is going to hurt, but I'm not sure how badly.  At least on
> multiprocessor.

Luckily most boards around here are still UP. =)

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Tue, 2 Jun 2009, Rafael J. Wysocki wrote:

> > It's partly a question of avoiding unnecessary activity.  Is there any
> > reason to resume a device if it's just going to be autosuspended again
> > a few seconds later?
> 
> Well, I'm not sure if we know in advance what's going to happen to the device
> right after a system-wide resume.  It may be a totally different situation from
> the device activity point of view.

We _never_ know in advance what's going to happen.  An autosuspended
device may have to wake up at any time in order to carry out some work
-- that's true whether or not a system sleep has taken place.  IMO it
doesn't make sense to wake up an autosuspended device for no reason
other than that it _may_ need to do something; we should let it stay in
a low-power mode until we _know_ it has to wake up.

> > It's also a matter of principle.  A system sleep should be as
> > transparent as possible (except for the passage of time, of course).  
> > When the sleep is over, devices should be in the same state as before
> > it began.
> 
> As a matter of fact, this need not be true at all.  For example, consider a
> wireless network adapter that was assosicated with specific access point
> before a system-wide suspend, but this access point is not present any more
> after the subsequent system-wide resume.

This example doesn't prove your point.  Sure, conditions may have
changed while the system was asleep.  They might also change while the
system is awake.  In general, whether or not the system was asleep
shouldn't make any difference -- to as great an extent as possible we
should strive to pretend that the entire sleep took no time at all (or
simply didn't happen).

So: Suppose the system had been awake when the access point
disappeared.  What would the wireless adapter driver do then?  It
should try to do exactly the same thing if the access point disappears
while the system is asleep.

You see?  The sleep should be transparent.

> I really don't think we can predict what's going to happen to a device after
> a system-wide resume, so IMO we should assume that the device will be necessary
> immediately after the resume and we should resume it.  It still can autosuspend
> normally afterwards if it's not needed after all.

This is highly debatable.  I'd like to hear some thoughts from other 
people.


> > >  Also, what happens if the platform firmware
> > > resumes the autosuspended devices before passing control to the kernel during
> > > system-wide resume?  How do we handle that?
> > 
> > In the case of USB, it's not possible to resume a suspended device
> > without first resuming and using a host controller.  The host
> > controller driver will realize that the firmware has taken over the
> > controller and will reset the controller.
...

> I'm not sure if this is doable so easily for PCI devices, for one example.
> Most probably, they will come up in D0 after the system-wide suspend and
> the PCI resume code will have to suspend them directly so that they remain
> "autosuspended" after the resume.

Clearly the behavior will need to vary among buses.  What's appropriate
for USB won't be appropriate for PCI.  Furthermore, the overhead of
waking a PCI device unnecessarily might be a lot lower than the
overhead of waking a USB device, so it might make sense always to
resume autosuspended PCI devices during a system resume.


> > So the work routine would somehow be aware when a system sleep is in
> > progress, at which point it would return immediately without doing
> > anything?  That seems racey.  Isn't it easier just to freeze the
> > workqueue?
> 
> Well, yes and no.  Namely, if we freeze the workqueue, all of the bus types
> using it will have to check the vaildity of operations carried out by it,
> because of the possibility that system-wide power transition took place in the
> meantime and changed the state of the device.  Also, please take the PCI
> example into consideration.

Freezability doesn't matter; you can prevent items from being added to
the workqueue during a power transition just as easily regardless of
whether it is freezable or not.  In fact, it's easier if the workqueue
is freezable.  You don't have to prevent anything from being added; you
just have to cancel everything on the workqueue before unfreezing it.

One solution would be to have the bus types cancel their own pending
work items explicitly.  Another is to have two freezable PM workqueues,
one with automatic cancellation and the other without.


> > For autoresume requests coming from an interrupt handler, I'm not so 
> > sure.  I don't know of any good examples -- in fact there don't seem to 
> > be any examples at the moment.  (The USB core supports autoresume 
> > requests in interrupt context, AFAICS no driver uses them.)  So maybe 
> > it's premature to worry about this case.
> 
> Hmm.  Can you please describe a theoretical situation in which this kind
> of autoresume request will appear?  I'm not really sure what you mean here. :-)

Well, here's something people discussed but which hasn't been
implemented (and may never be implemented).  A suspended USB keyboard
doesn't have enough power to turn on any LEDs.  So suppose a system has
two keyboards, and kbd-A is autosuspended when the user engages
CapsLock on kbd-B.  The input layer wants to turn on the CapsLock LED
on all the keyboards, which means it has to autoresume kbd-A.  But all
this action takes place in interrupt context, so an autoresume request
would have to be queued.


> > Lastly, we have resume requests coming from a device -- i.e., remote
> > wakeup requests.  This is a little tricky.  Suppose you have remote
> > wakeup enabled, and you press a key on your USB keyboard just as the
> > computer is going to sleep.  Should this cause the sleep to be aborted?  
> > Should it cause the computer to wake back up again right away?  Should
> > the keystroke be ignored?
> 
> This is a difficult problem in general.  I _think_ we should ignore such
> autoresume requests for devices that are not marked as wake-up ones
> and we should abort the suspend if there's a request from a wake-up device.

It _is_ difficult.  Right now USB ignores such requests, because the
threads that handle them are all freezable.  But the information isn't
lost; it is stored in the hardware and when the system wakes up the
requests will be seen.

Except for wakeup requests from root hubs.  If they arrive at just the 
right time then they are not stored in the hardware -- instead they are 
stored as work items on the ksuspend_usbd workqueue!  In essence, the 
workqueue serves as a software mechanism for saving IRQs which were 
delivered but couldn't be acted on.

That's why I don't want to erase the work items; it would be logically 
equivalent to dropping interrupts.


> Anyway, below is a prototype implementation of a generic runtime PM workqueue,
> just for discussion.
> 
> pm_schedule_suspend() and pm_schedule_resume() are to be called by bus types
> that are requesting the autosuspend or autoresume, respectively, of a device.
> The supposed control flow is like this:
> * bus type calls pm_schedule_suspend() to schedule autosuspend of a device
> * at the right time the PM workqueue executes the bus type's ->autosuspend()
>   callback which is responsible for calling the device driver to suspend the
>   device (the way in which this is done is totally bus type-specific)
> * if there's a resume event for the device, it's delivered to the bus type
>   driver (in a bus type-specific way) and the bus type schedules autoresume of
>   the device by calling pm_schedule_resume()
> * the PM workqueue executes the bus type's ->autoresume() as soon as it can
>   and this callback is responsible for calling the device driver to resume the
>   device (the way in which this is done is totally bus type-specific).

It looks like the only benefits provided by this patch are the
workqueue itself and the embedded delayed_work structure.  The ability
to cancel the work items (or prevent them from being added) during a
system sleep is questionable, as discussed above.  There isn't any
value added by the new functions or methods.

In short, it would be just as useful simply to add the delayed_work 
structure and export two PM workqueues, both freezable, with automatic
cancellation for one of them.

Alan Stern

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Alan Stern]

On Tue, 2 Jun 2009, Rafael J. Wysocki wrote:

> > It's partly a question of avoiding unnecessary activity.  Is there any
> > reason to resume a device if it's just going to be autosuspended again
> > a few seconds later?
> 
> Well, I'm not sure if we know in advance what's going to happen to the device
> right after a system-wide resume.  It may be a totally different situation from
> the device activity point of view.

We _never_ know in advance what's going to happen.  An autosuspended
device may have to wake up at any time in order to carry out some work
-- that's true whether or not a system sleep has taken place.  IMO it
doesn't make sense to wake up an autosuspended device for no reason
other than that it _may_ need to do something; we should let it stay in
a low-power mode until we _know_ it has to wake up.

> > It's also a matter of principle.  A system sleep should be as
> > transparent as possible (except for the passage of time, of course).  
> > When the sleep is over, devices should be in the same state as before
> > it began.
> 
> As a matter of fact, this need not be true at all.  For example, consider a
> wireless network adapter that was assosicated with specific access point
> before a system-wide suspend, but this access point is not present any more
> after the subsequent system-wide resume.

This example doesn't prove your point.  Sure, conditions may have
changed while the system was asleep.  They might also change while the
system is awake.  In general, whether or not the system was asleep
shouldn't make any difference -- to as great an extent as possible we
should strive to pretend that the entire sleep took no time at all (or
simply didn't happen).

So: Suppose the system had been awake when the access point
disappeared.  What would the wireless adapter driver do then?  It
should try to do exactly the same thing if the access point disappears
while the system is asleep.

You see?  The sleep should be transparent.

> I really don't think we can predict what's going to happen to a device after
> a system-wide resume, so IMO we should assume that the device will be necessary
> immediately after the resume and we should resume it.  It still can autosuspend
> normally afterwards if it's not needed after all.

This is highly debatable.  I'd like to hear some thoughts from other 
people.


> > >  Also, what happens if the platform firmware
> > > resumes the autosuspended devices before passing control to the kernel during
> > > system-wide resume?  How do we handle that?
> > 
> > In the case of USB, it's not possible to resume a suspended device
> > without first resuming and using a host controller.  The host
> > controller driver will realize that the firmware has taken over the
> > controller and will reset the controller.
...

> I'm not sure if this is doable so easily for PCI devices, for one example.
> Most probably, they will come up in D0 after the system-wide suspend and
> the PCI resume code will have to suspend them directly so that they remain
> "autosuspended" after the resume.

Clearly the behavior will need to vary among buses.  What's appropriate
for USB won't be appropriate for PCI.  Furthermore, the overhead of
waking a PCI device unnecessarily might be a lot lower than the
overhead of waking a USB device, so it might make sense always to
resume autosuspended PCI devices during a system resume.


> > So the work routine would somehow be aware when a system sleep is in
> > progress, at which point it would return immediately without doing
> > anything?  That seems racey.  Isn't it easier just to freeze the
> > workqueue?
> 
> Well, yes and no.  Namely, if we freeze the workqueue, all of the bus types
> using it will have to check the vaildity of operations carried out by it,
> because of the possibility that system-wide power transition took place in the
> meantime and changed the state of the device.  Also, please take the PCI
> example into consideration.

Freezability doesn't matter; you can prevent items from being added to
the workqueue during a power transition just as easily regardless of
whether it is freezable or not.  In fact, it's easier if the workqueue
is freezable.  You don't have to prevent anything from being added; you
just have to cancel everything on the workqueue before unfreezing it.

One solution would be to have the bus types cancel their own pending
work items explicitly.  Another is to have two freezable PM workqueues,
one with automatic cancellation and the other without.


> > For autoresume requests coming from an interrupt handler, I'm not so 
> > sure.  I don't know of any good examples -- in fact there don't seem to 
> > be any examples at the moment.  (The USB core supports autoresume 
> > requests in interrupt context, AFAICS no driver uses them.)  So maybe 
> > it's premature to worry about this case.
> 
> Hmm.  Can you please describe a theoretical situation in which this kind
> of autoresume request will appear?  I'm not really sure what you mean here. :-)

Well, here's something people discussed but which hasn't been
implemented (and may never be implemented).  A suspended USB keyboard
doesn't have enough power to turn on any LEDs.  So suppose a system has
two keyboards, and kbd-A is autosuspended when the user engages
CapsLock on kbd-B.  The input layer wants to turn on the CapsLock LED
on all the keyboards, which means it has to autoresume kbd-A.  But all
this action takes place in interrupt context, so an autoresume request
would have to be queued.


> > Lastly, we have resume requests coming from a device -- i.e., remote
> > wakeup requests.  This is a little tricky.  Suppose you have remote
> > wakeup enabled, and you press a key on your USB keyboard just as the
> > computer is going to sleep.  Should this cause the sleep to be aborted?  
> > Should it cause the computer to wake back up again right away?  Should
> > the keystroke be ignored?
> 
> This is a difficult problem in general.  I _think_ we should ignore such
> autoresume requests for devices that are not marked as wake-up ones
> and we should abort the suspend if there's a request from a wake-up device.

It _is_ difficult.  Right now USB ignores such requests, because the
threads that handle them are all freezable.  But the information isn't
lost; it is stored in the hardware and when the system wakes up the
requests will be seen.

Except for wakeup requests from root hubs.  If they arrive at just the 
right time then they are not stored in the hardware -- instead they are 
stored as work items on the ksuspend_usbd workqueue!  In essence, the 
workqueue serves as a software mechanism for saving IRQs which were 
delivered but couldn't be acted on.

That's why I don't want to erase the work items; it would be logically 
equivalent to dropping interrupts.


> Anyway, below is a prototype implementation of a generic runtime PM workqueue,
> just for discussion.
> 
> pm_schedule_suspend() and pm_schedule_resume() are to be called by bus types
> that are requesting the autosuspend or autoresume, respectively, of a device.
> The supposed control flow is like this:
> * bus type calls pm_schedule_suspend() to schedule autosuspend of a device
> * at the right time the PM workqueue executes the bus type's ->autosuspend()
>   callback which is responsible for calling the device driver to suspend the
>   device (the way in which this is done is totally bus type-specific)
> * if there's a resume event for the device, it's delivered to the bus type
>   driver (in a bus type-specific way) and the bus type schedules autoresume of
>   the device by calling pm_schedule_resume()
> * the PM workqueue executes the bus type's ->autoresume() as soon as it can
>   and this callback is responsible for calling the device driver to resume the
>   device (the way in which this is done is totally bus type-specific).

It looks like the only benefits provided by this patch are the
workqueue itself and the embedded delayed_work structure.  The ability
to cancel the work items (or prevent them from being added) during a
system sleep is questionable, as discussed above.  There isn't any
value added by the new functions or methods.

In short, it would be just as useful simply to add the delayed_work 
structure and export two PM workqueues, both freezable, with automatic
cancellation for one of them.

Alan Stern

Re: [linux-pm] [PATCH 00/04][RFC] PM: Runtime platform device PM

[Pavel Machek]

Hi!


> > In the process of discussing a similar interface for OMAP, we dicussed
> > that having 3 states would be more useful.  Specifically, and
> > _enable(), _idle() and _disable() hook.  The _enable() and _idle()
> > hooks being exactly what you proposed above, but with the addition of
> > a _disable() hook which says not only can the device go idle, but that
> > the driver is really finished with the device.  In this case, more
> > aggresive PM measures could be taken, such as turning of regulators
> > that may have long latencies that may not be appropriate to turn off
> > in idle.
> 
> Hm.. I wonder when the driver is really finished with the device
> though. Only when the module is unloaded? If so then we could deal
> with the hard disable using platform bus notifiers (like in [04/04]).

I'd guess that they call disable() when /dev node is closed... and
idle() when device is opened but inactive.

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

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Pavel Machek]

Hi!


> > In the process of discussing a similar interface for OMAP, we dicussed
> > that having 3 states would be more useful.  Specifically, and
> > _enable(), _idle() and _disable() hook.  The _enable() and _idle()
> > hooks being exactly what you proposed above, but with the addition of
> > a _disable() hook which says not only can the device go idle, but that
> > the driver is really finished with the device.  In this case, more
> > aggresive PM measures could be taken, such as turning of regulators
> > that may have long latencies that may not be appropriate to turn off
> > in idle.
> 
> Hm.. I wonder when the driver is really finished with the device
> though. Only when the module is unloaded? If so then we could deal
> with the hard disable using platform bus notifiers (like in [04/04]).

I'd guess that they call disable() when /dev node is closed... and
idle() when device is opened but inactive.

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

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Friday 29 May 2009, Magnus Damm wrote:
> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> Add platform_device_idle() and platform_device_wakeup()
> >> and allow architectures to implement their own versions
> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >>
> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> ---
> >>
> >>  Yes, this needs documentation. See [00/04] for now.
> >>
> >>  arch/Kconfig                    |    3 +++
> >>  include/linux/platform_device.h |    8 ++++++++
> >>  2 files changed, 11 insertions(+)
> >>
> >> --- 0002/arch/Kconfig
> >> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
> >> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
> >>
> >>  config HAVE_PLATFORM_DEVICE_ARCHDATA
> >>       bool
> >> +
> >> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
> >
> > Again, I'm not sure if the new CONFIG_ option is really necessary.
> 
> At least it could be combined into a single option for platform device
> runtime pm. So no need to have separate kconfig options for [02/04]
> and [03/04].
> 
> We could also drop the #ifdef stuff and go for weak symbols that can
> be overriden by architecture code.

The weak symbols don't work really AFAICS.  People have been replacing them
with alternative things recently.

> I'm open to suggestions. =)

Well, that depends on what exactly you want to achieve. :-)

Best,
Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Friday 29 May 2009, Magnus Damm wrote:
> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> Add platform_device_idle() and platform_device_wakeup()
> >> and allow architectures to implement their own versions
> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >>
> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> ---
> >>
> >>  Yes, this needs documentation. See [00/04] for now.
> >>
> >>  arch/Kconfig                    |    3 +++
> >>  include/linux/platform_device.h |    8 ++++++++
> >>  2 files changed, 11 insertions(+)
> >>
> >> --- 0002/arch/Kconfig
> >> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
> >> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
> >>
> >>  config HAVE_PLATFORM_DEVICE_ARCHDATA
> >>       bool
> >> +
> >> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
> >
> > Again, I'm not sure if the new CONFIG_ option is really necessary.
> 
> At least it could be combined into a single option for platform device
> runtime pm. So no need to have separate kconfig options for [02/04]
> and [03/04].
> 
> We could also drop the #ifdef stuff and go for weak symbols that can
> be overriden by architecture code.

The weak symbols don't work really AFAICS.  People have been replacing them
with alternative things recently.

> I'm open to suggestions. =)

Well, that depends on what exactly you want to achieve. :-)

Best,
Rafael

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Rafael J. Wysocki]

[-- Attachment #1.1: Type: text/plain, Size: 8475 bytes --]

On Tuesday 02 June 2009, Magnus Damm wrote:
> Hi Rafael,
> 
> Thanks for your feedback!
> 
> 2009/5/30 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> Wrap the platform device bus dev_pm_ops to allow runtime
> >> pm and regular suspend and resume to coexist.
> >>
> >> Platform device data is extended with flags that allow
> >> us to keep track of which dev_pm_ops that has been called.
> >>
> >> Basically, if a device has been frozen by the runtime pm
> >> code, don't call ->freeze() again when hibernating.
> >>
> >> Architecture code can use platform_runtime_dev_pm_ops to
> >> call driver dev_pm_ops associated with a certain device.
> >>
> >> Enable with CONFIG_HAVE_PLATFORM_DEVICE_RUNTIME_PM.
> >>
> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> ---
> >>
> >>  This is a bit of a hack, any better way to wrap dev_pm_ops?
> >
> > I'm not really sure you need to wrap them at all.
> 
> I realize that I didn't explain very well why I decided to wrap
> dev_pm_ops. Sorry about that. Basically there are two reasons:
> 1) Wrap to always have the runtime dev_pm_ops functions regardless of kconfig.

Well, I'm not sure if this is the right approach (see below).

> 2) Wrap to make sure runtime and system-wide dev_pm_ops can coexist.

That's if you want to have a separate dev_pm_ops object for run-time PM, which
I don't think is necessary.

> > There are a few choices:
> >
> > (1) You can use the platform_pm_* functions directly for run-time PM, but
> > in that case you'll need to make sure that the "suspend" ones return 0
> > immediately when called during system-wide suspend or hibernation (there's the
> > question whether the "resume" ones should still put the device into the full
> > power state in that case).  For this purpose you can add a single flag to
> > struct platform_device and set it for devices that have already been
> > "suspended" (this flag, when set, will make all of the "suspend" callbacks
> > return 0 without doing anything until the device is "resumed").
> 
> I have to make sure that the right Kconfig bits are enabled though,
> otherwise some platform_pm_* functions will be missing. So one merit
> for the wrapped functions is in this patch is that they are always
> there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.

Make them depend on CONFIG_PM, then.  They are not _that_ much code anyway,
we can easily afford building them even on platforms that really don't use them,
but I expect them to be used by more and more platforms over time anyway.

> Is one flag really enough?

Not necessarily, although I think we'll end up using one flag only.

> Isn't it a bit strange from the driver point of view to always get their
> ->prepare() callback executed, but ->suspend() gets filtered?

In fact, from the run-time PM POV, ->prepare() and ->suspend() should
always be executed together and ->suspend_noirq() shouldn't be executed
at all.  Now, it may be necessary to execute some code from ->suspend_noirq()
for run-time PM too, but not by using this callback directly.

> The bitmap in this patch is more fine-grained, so struct
> platform_device remember which of each dev_pm_ops callback that have
> been called. It may be overly complex though. Also, please note that
> both runtime dev_pm_ops and system-wide dev_pm_ops go through the same
> dev_pm_ops that keep track of which callbacks that have been called.
> 
> Using some kind of flag(s) for coexisting with system-wide
> suspend-to-ram/disk sounds good.

Of course we should be avoiding double suspend, so some kind of sychronization
between the "sleep" suspend and run-time suspend is necessary and I agree that
it's probably most convenient to use some flags for this purpose.  The
questions are how many flags we're going to need and how exactly we're going
to use them.

> > (2) You can add separate platform callbacks for run-time PM that will execute
> > the drivers' dev_pm_ops callbacks and presumably do something else (I don't
> > know what that may be for platform devices, though).  In that case, again,
> > adding a flag to struct platform_device and making platform_pm_* check it
> > should be sufficient to prevent devices from being suspended twice in a row.
> 
> We may need to do other things as well - not sure - but regardless we
> still need flag(s) to avoid double suspend.

Agreed.

> > (3) You can add separate platform callbacks for run-time PM for both the
> > bus type and the drivers, in which dev_pm_ops will be totally separate from
> > these new callbacks, although of course you'll need provide some kind of
> > synchronization bettween them all.  That also may be done through a flag
> > in struct platform_device IMO.
> 
> Yeah, that's also one way. I wonder if that helps us though, I feel
> that we already have a pretty wide range of callbacks in dev_pm_ops.
> I'm not sure if they cover all cases we need though, I guess future
> experiments will tell.

Yes.  Still, we can anticipate that the run-time PM operations may be slightly
different from the 'sleep state' PM ones, in which case it makes sens to
introduce new callbacks, because that makes it clear(er) what the code is
supposed to do.

> > Now, since other bus types will most probably also need a flag in their
> > _device structures, it may be worth putting it into struct device (we've
> > discsussed that already).
> 
> Yeah, I wonder if a single flag is enough though? Aren't we coexisting
> with CONFIG_SUSPEND, CONFIG_HIBERNATION and CONFIG_KEXEC_JUMP?
> 
> > I'm not sure which of (1) - (3) are the most suitable for the platform bus
> > type.  For PCI I'd probably choose (2), because the current PCI bus type's
> > dev_pm_ops callbacks are tailored to system-wide power transitions.
> > Moreover, PCI devices can generally be put first into D1, then into D2 and
> > finally into D3, which only makes sense at run time, and some of them may
> > have to be put back into the full power state before a system-wide transition
> > (apparently, we'll need a separate flag to mark such devices).
> 
> If I understand (1)-(3) correctly, then I think (1) is probably the
> best choice for our platform devices.

OK, for _your_ platform devices it may be the best choice, but what about
the other platforms' platform devices?  Do you think (1) will be suitable for
them all and if so then why?

> I guess (2) is not very far from (1), so if we go with (1) to begin with
> then we can deal with SoC specific things in our arch code to come closer to
> (2) over time if needed.
> 
> > Of course, if you decide to add separate run-time PM callbacks for the
> > platform bus type, you won't need to wrap its dev_pm_ops callbacks any more,
> > but you'll need to modify them to check the appropriate flag(s).  For example,
> > you may choose to use a two-bit pm_suspend_level field such that
> >
> > * if pm_suspend_level = 1, platform_pm_prepare() will return immediately
> > * if pm_suspend_level = 2, platform_pm_prepare() and platform_pm_suspend()
> >  will return immediately
> > * if pm_suspend_level = 3, platform_pm_prepare(), platform_pm_suspend()
> >  and platform_pm_suspend_noirq() will return immediately
> >
> > (and analogously for the hibernation callbacks) and make your run-time PM
> > callbacks set this field appropriately.
> 
> That is true, but this checking is not needed for systems where
> runtime PM is disabled.

No, it is not, but it won't hurt really.  Also, if we do it intelligently, we
can make the compiler optimize out the checks.  I'm not really sure it's worth
the effort, however.

> Or am I misunderstanding?
>
> Maybe it's worth to discuss how to integrate this. I suspect that this
> will only affect some selected architectures to begin with, and the
> rest of the code base should be unaffected by this change as long as
> the runtime kconfig is disabled.
> 
> So I decided to wrap dev_pm_ops to make the impact for non runtime PM
> systems as small as possible. This while giving the runtime PM case
> access to all dev_pm_ops regardless of suspend/hibernation kconfig.
> 
> Does it make sense?

Well, as I said above, I don't really think that run-time PM is going to need
all of the callbacks from dev_pm_ops.  Moreover, I'm not sure if it makes
sense to have more than two (call it 'autosuspend' and 'autoresume' using the
already existing USB terminology) callbacks for run-time PM, at least at
the driver level.

Best,
Rafael

[-- Attachment #1.2: Type: text/html, Size: 14318 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

Hi Pavel!

On Wed, Jun 3, 2009 at 6:37 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > In the process of discussing a similar interface for OMAP, we dicussed
>> > that having 3 states would be more useful.  Specifically, and
>> > _enable(), _idle() and _disable() hook.  The _enable() and _idle()
>> > hooks being exactly what you proposed above, but with the addition of
>> > a _disable() hook which says not only can the device go idle, but that
>> > the driver is really finished with the device.  In this case, more
>> > aggresive PM measures could be taken, such as turning of regulators
>> > that may have long latencies that may not be appropriate to turn off
>> > in idle.
>>
>> Hm.. I wonder when the driver is really finished with the device
>> though. Only when the module is unloaded? If so then we could deal
>> with the hard disable using platform bus notifiers (like in [04/04]).
>
> I'd guess that they call disable() when /dev node is closed... and
> idle() when device is opened but inactive.

But the time from close() to next open() is work load dependent. So
the difference between _disable() and _idle() is pretty fuzzy in my
opinion. I'm trying to think of a case where hard _disable() is
needed, but nothing comes to mind.

In my mind a single disable()/idle() function together with latency
information is enough. It covers the entire range from _idle() to hard
_disable().

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Magnus Damm]

Hi Pavel!

On Wed, Jun 3, 2009 at 6:37 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > In the process of discussing a similar interface for OMAP, we dicussed
>> > that having 3 states would be more useful.  Specifically, and
>> > _enable(), _idle() and _disable() hook.  The _enable() and _idle()
>> > hooks being exactly what you proposed above, but with the addition of
>> > a _disable() hook which says not only can the device go idle, but that
>> > the driver is really finished with the device.  In this case, more
>> > aggresive PM measures could be taken, such as turning of regulators
>> > that may have long latencies that may not be appropriate to turn off
>> > in idle.
>>
>> Hm.. I wonder when the driver is really finished with the device
>> though. Only when the module is unloaded? If so then we could deal
>> with the hard disable using platform bus notifiers (like in [04/04]).
>
> I'd guess that they call disable() when /dev node is closed... and
> idle() when device is opened but inactive.

But the time from close() to next open() is work load dependent. So
the difference between _disable() and _idle() is pretty fuzzy in my
opinion. I'm trying to think of a case where hard _disable() is
needed, but nothing comes to mind.

In my mind a single disable()/idle() function together with latency
information is enough. It covers the entire range from _idle() to hard
_disable().

/ magnus

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Alan Stern wrote:
> On Tue, 2 Jun 2009, Rafael J. Wysocki wrote:
> 
> > > It's partly a question of avoiding unnecessary activity.  Is there any
> > > reason to resume a device if it's just going to be autosuspended again
> > > a few seconds later?
> > 
> > Well, I'm not sure if we know in advance what's going to happen to the device
> > right after a system-wide resume.  It may be a totally different situation from
> > the device activity point of view.
> 
> We _never_ know in advance what's going to happen.  An autosuspended
> device may have to wake up at any time in order to carry out some work
> -- that's true whether or not a system sleep has taken place.  IMO it
> doesn't make sense to wake up an autosuspended device for no reason
> other than that it _may_ need to do something; we should let it stay in
> a low-power mode until we _know_ it has to wake up.

OK, that's a valid point.

I also think that if the BIOS has already woke up the device during system-wide
resume, we shouldn't try to put it to a low power state again unless we know
that this is desirable.

> > > It's also a matter of principle.  A system sleep should be as
> > > transparent as possible (except for the passage of time, of course).  
> > > When the sleep is over, devices should be in the same state as before
> > > it began.
> > 
> > As a matter of fact, this need not be true at all.  For example, consider a
> > wireless network adapter that was assosicated with specific access point
> > before a system-wide suspend, but this access point is not present any more
> > after the subsequent system-wide resume.
> 
> This example doesn't prove your point.

No, it doesn't.  Still, it illustrates the problem I had in mind, that it may
not be desirable to restore the pre-suspend settings in all cases.

> Sure, conditions may have changed while the system was asleep.  They might
> also change while the system is awake.  In general, whether or not the system
> was asleep shouldn't make any difference -- to as great an extent as possible
> we should strive to pretend that the entire sleep took no time at all (or
> simply didn't happen).
> 
> So: Suppose the system had been awake when the access point
> disappeared.  What would the wireless adapter driver do then?  It
> should try to do exactly the same thing if the access point disappears
> while the system is asleep.

Not really.  Namely, when the access point _vanishes_ at run time, this is an
error condition from which we have to recover, while if it's _not_ _present_
after a resume, it's a normal situation and we shouldn't be recovering from
that (the user might have changed the physical location while suspended).

On my system the wireless always remembers the network it was associated with
before suspend and it tries to reassociate with it after the subsequent resume,
even if the network is not there any more, so it fails and I have to choose
another one manually.  This isn't a desirable behavior and I'd even say it's
not correct, because after a fresh boot it always scans for networks and
associates with one that's actually present.  I'd like it to do the same after
the resume.

> You see?  The sleep should be transparent.

In some cases it won't be, because the hardware state changes while sleeping.
For example, a laptop battery may be depleted while suspended, so that after
resume it's in the 'low' condition, while it was in the 'good' condition before
the preceding suspend.  Arguably, you can't drain a battery from 90% to 10%,
for example, momentarily.  Also, a CPU fan might be 100% on before suspend,
because the CPU was hot at that time, but that doesn't mean the fan should be
100% on after the subsequent resume, because it's likely that the CPU will be
cold (on some systems trying to make the fan spin too fast may lead to
general problems with thermal management afterwards).

> > I really don't think we can predict what's going to happen to a device after
> > a system-wide resume, so IMO we should assume that the device will be necessary
> > immediately after the resume and we should resume it.  It still can autosuspend
> > normally afterwards if it's not needed after all.
> 
> This is highly debatable.  I'd like to hear some thoughts from other 
> people.

Agreed, but no one has spoken so far. :-)

> > > >  Also, what happens if the platform firmware
> > > > resumes the autosuspended devices before passing control to the kernel during
> > > > system-wide resume?  How do we handle that?
> > > 
> > > In the case of USB, it's not possible to resume a suspended device
> > > without first resuming and using a host controller.  The host
> > > controller driver will realize that the firmware has taken over the
> > > controller and will reset the controller.
> ...
> 
> > I'm not sure if this is doable so easily for PCI devices, for one example.
> > Most probably, they will come up in D0 after the system-wide suspend and
> > the PCI resume code will have to suspend them directly so that they remain
> > "autosuspended" after the resume.
> 
> Clearly the behavior will need to vary among buses.

Agreed.

> What's appropriate for USB won't be appropriate for PCI.  Furthermore, the
> overhead of waking a PCI device unnecessarily might be a lot lower than the
> overhead of waking a USB device, so it might make sense always to
> resume autosuspended PCI devices during a system resume.

Sure.  Also, as I said earlier, the BIOS usually wakes up PCI devices (in fact
a bus reset causes them to wake up and the buses are ususally reset during
resume by the BIOS) and it doesn't really make sense to put them into low
power states just because they were in low power states before the suspend.

> > > So the work routine would somehow be aware when a system sleep is in
> > > progress, at which point it would return immediately without doing
> > > anything?  That seems racey.  Isn't it easier just to freeze the
> > > workqueue?
> > 
> > Well, yes and no.  Namely, if we freeze the workqueue, all of the bus types
> > using it will have to check the vaildity of operations carried out by it,
> > because of the possibility that system-wide power transition took place in the
> > meantime and changed the state of the device.  Also, please take the PCI
> > example into consideration.
> 
> Freezability doesn't matter; you can prevent items from being added to
> the workqueue during a power transition just as easily regardless of
> whether it is freezable or not.  In fact, it's easier if the workqueue
> is freezable.  You don't have to prevent anything from being added; you
> just have to cancel everything on the workqueue before unfreezing it.

Good idea.

> One solution would be to have the bus types cancel their own pending
> work items explicitly.  Another is to have two freezable PM workqueues,
> one with automatic cancellation and the other without.

What about having a flag in struct dev_pm_info (along with the delayed
work structure) indicating whether or not the work should be automatically
cancelled before unfreezing the queue?  [It seems, in that case we'd need
another flag to indicate what is to cancel, the delayed work or just the work.]

> > > For autoresume requests coming from an interrupt handler, I'm not so 
> > > sure.  I don't know of any good examples -- in fact there don't seem to 
> > > be any examples at the moment.  (The USB core supports autoresume 
> > > requests in interrupt context, AFAICS no driver uses them.)  So maybe 
> > > it's premature to worry about this case.
> > 
> > Hmm.  Can you please describe a theoretical situation in which this kind
> > of autoresume request will appear?  I'm not really sure what you mean here. :-)
> 
> Well, here's something people discussed but which hasn't been
> implemented (and may never be implemented).  A suspended USB keyboard
> doesn't have enough power to turn on any LEDs.  So suppose a system has
> two keyboards, and kbd-A is autosuspended when the user engages
> CapsLock on kbd-B.  The input layer wants to turn on the CapsLock LED
> on all the keyboards, which means it has to autoresume kbd-A.  But all
> this action takes place in interrupt context, so an autoresume request
> would have to be queued.

I see, but this is similar to the remote wake-up in that we have to queue
the autoresume request.

> > > Lastly, we have resume requests coming from a device -- i.e., remote
> > > wakeup requests.  This is a little tricky.  Suppose you have remote
> > > wakeup enabled, and you press a key on your USB keyboard just as the
> > > computer is going to sleep.  Should this cause the sleep to be aborted?  
> > > Should it cause the computer to wake back up again right away?  Should
> > > the keystroke be ignored?
> > 
> > This is a difficult problem in general.  I _think_ we should ignore such
> > autoresume requests for devices that are not marked as wake-up ones
> > and we should abort the suspend if there's a request from a wake-up device.
> 
> It _is_ difficult.  Right now USB ignores such requests, because the
> threads that handle them are all freezable.  But the information isn't
> lost; it is stored in the hardware and when the system wakes up the
> requests will be seen.
> 
> Except for wakeup requests from root hubs.  If they arrive at just the 
> right time then they are not stored in the hardware -- instead they are 
> stored as work items on the ksuspend_usbd workqueue!  In essence, the 
> workqueue serves as a software mechanism for saving IRQs which were 
> delivered but couldn't be acted on.
> 
> That's why I don't want to erase the work items; it would be logically 
> equivalent to dropping interrupts.

I see.

Still, we'll need to think about a mechanism allowing us to abort suspend
if there's a wake-up request from a wake-up device.

> > Anyway, below is a prototype implementation of a generic runtime PM workqueue,
> > just for discussion.
> > 
> > pm_schedule_suspend() and pm_schedule_resume() are to be called by bus types
> > that are requesting the autosuspend or autoresume, respectively, of a device.
> > The supposed control flow is like this:
> > * bus type calls pm_schedule_suspend() to schedule autosuspend of a device
> > * at the right time the PM workqueue executes the bus type's ->autosuspend()
> >   callback which is responsible for calling the device driver to suspend the
> >   device (the way in which this is done is totally bus type-specific)
> > * if there's a resume event for the device, it's delivered to the bus type
> >   driver (in a bus type-specific way) and the bus type schedules autoresume of
> >   the device by calling pm_schedule_resume()
> > * the PM workqueue executes the bus type's ->autoresume() as soon as it can
> >   and this callback is responsible for calling the device driver to resume the
> >   device (the way in which this is done is totally bus type-specific).
> 
> It looks like the only benefits provided by this patch are the
> workqueue itself and the embedded delayed_work structure.  The ability
> to cancel the work items (or prevent them from being added) during a
> system sleep is questionable, as discussed above.  There isn't any
> value added by the new functions or methods.

The value added is that since the delayed work member is to be used for
queueing work, each bus type will have to do the same casting from
'struct work_struct' to 'struct delayed_work' and next to 'struct device'
to get to the device object, which means quite a bit of code duplication. 

Also, IMO, if there's a set of functions for doing that, the probability
that someone gets things wrong is smaller (slightly).  Moreover, we can
add variants for the works that will be automatically cancelled during
system-wide resume, so that bus types don't have to worry about the details
related to that.

Thanks,
Rafael

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Rafael J. Wysocki]

On Tuesday 02 June 2009, Alan Stern wrote:
> On Tue, 2 Jun 2009, Rafael J. Wysocki wrote:
> 
> > > It's partly a question of avoiding unnecessary activity.  Is there any
> > > reason to resume a device if it's just going to be autosuspended again
> > > a few seconds later?
> > 
> > Well, I'm not sure if we know in advance what's going to happen to the device
> > right after a system-wide resume.  It may be a totally different situation from
> > the device activity point of view.
> 
> We _never_ know in advance what's going to happen.  An autosuspended
> device may have to wake up at any time in order to carry out some work
> -- that's true whether or not a system sleep has taken place.  IMO it
> doesn't make sense to wake up an autosuspended device for no reason
> other than that it _may_ need to do something; we should let it stay in
> a low-power mode until we _know_ it has to wake up.

OK, that's a valid point.

I also think that if the BIOS has already woke up the device during system-wide
resume, we shouldn't try to put it to a low power state again unless we know
that this is desirable.

> > > It's also a matter of principle.  A system sleep should be as
> > > transparent as possible (except for the passage of time, of course).  
> > > When the sleep is over, devices should be in the same state as before
> > > it began.
> > 
> > As a matter of fact, this need not be true at all.  For example, consider a
> > wireless network adapter that was assosicated with specific access point
> > before a system-wide suspend, but this access point is not present any more
> > after the subsequent system-wide resume.
> 
> This example doesn't prove your point.

No, it doesn't.  Still, it illustrates the problem I had in mind, that it may
not be desirable to restore the pre-suspend settings in all cases.

> Sure, conditions may have changed while the system was asleep.  They might
> also change while the system is awake.  In general, whether or not the system
> was asleep shouldn't make any difference -- to as great an extent as possible
> we should strive to pretend that the entire sleep took no time at all (or
> simply didn't happen).
> 
> So: Suppose the system had been awake when the access point
> disappeared.  What would the wireless adapter driver do then?  It
> should try to do exactly the same thing if the access point disappears
> while the system is asleep.

Not really.  Namely, when the access point _vanishes_ at run time, this is an
error condition from which we have to recover, while if it's _not_ _present_
after a resume, it's a normal situation and we shouldn't be recovering from
that (the user might have changed the physical location while suspended).

On my system the wireless always remembers the network it was associated with
before suspend and it tries to reassociate with it after the subsequent resume,
even if the network is not there any more, so it fails and I have to choose
another one manually.  This isn't a desirable behavior and I'd even say it's
not correct, because after a fresh boot it always scans for networks and
associates with one that's actually present.  I'd like it to do the same after
the resume.

> You see?  The sleep should be transparent.

In some cases it won't be, because the hardware state changes while sleeping.
For example, a laptop battery may be depleted while suspended, so that after
resume it's in the 'low' condition, while it was in the 'good' condition before
the preceding suspend.  Arguably, you can't drain a battery from 90% to 10%,
for example, momentarily.  Also, a CPU fan might be 100% on before suspend,
because the CPU was hot at that time, but that doesn't mean the fan should be
100% on after the subsequent resume, because it's likely that the CPU will be
cold (on some systems trying to make the fan spin too fast may lead to
general problems with thermal management afterwards).

> > I really don't think we can predict what's going to happen to a device after
> > a system-wide resume, so IMO we should assume that the device will be necessary
> > immediately after the resume and we should resume it.  It still can autosuspend
> > normally afterwards if it's not needed after all.
> 
> This is highly debatable.  I'd like to hear some thoughts from other 
> people.

Agreed, but no one has spoken so far. :-)

> > > >  Also, what happens if the platform firmware
> > > > resumes the autosuspended devices before passing control to the kernel during
> > > > system-wide resume?  How do we handle that?
> > > 
> > > In the case of USB, it's not possible to resume a suspended device
> > > without first resuming and using a host controller.  The host
> > > controller driver will realize that the firmware has taken over the
> > > controller and will reset the controller.
> ...
> 
> > I'm not sure if this is doable so easily for PCI devices, for one example.
> > Most probably, they will come up in D0 after the system-wide suspend and
> > the PCI resume code will have to suspend them directly so that they remain
> > "autosuspended" after the resume.
> 
> Clearly the behavior will need to vary among buses.

Agreed.

> What's appropriate for USB won't be appropriate for PCI.  Furthermore, the
> overhead of waking a PCI device unnecessarily might be a lot lower than the
> overhead of waking a USB device, so it might make sense always to
> resume autosuspended PCI devices during a system resume.

Sure.  Also, as I said earlier, the BIOS usually wakes up PCI devices (in fact
a bus reset causes them to wake up and the buses are ususally reset during
resume by the BIOS) and it doesn't really make sense to put them into low
power states just because they were in low power states before the suspend.

> > > So the work routine would somehow be aware when a system sleep is in
> > > progress, at which point it would return immediately without doing
> > > anything?  That seems racey.  Isn't it easier just to freeze the
> > > workqueue?
> > 
> > Well, yes and no.  Namely, if we freeze the workqueue, all of the bus types
> > using it will have to check the vaildity of operations carried out by it,
> > because of the possibility that system-wide power transition took place in the
> > meantime and changed the state of the device.  Also, please take the PCI
> > example into consideration.
> 
> Freezability doesn't matter; you can prevent items from being added to
> the workqueue during a power transition just as easily regardless of
> whether it is freezable or not.  In fact, it's easier if the workqueue
> is freezable.  You don't have to prevent anything from being added; you
> just have to cancel everything on the workqueue before unfreezing it.

Good idea.

> One solution would be to have the bus types cancel their own pending
> work items explicitly.  Another is to have two freezable PM workqueues,
> one with automatic cancellation and the other without.

What about having a flag in struct dev_pm_info (along with the delayed
work structure) indicating whether or not the work should be automatically
cancelled before unfreezing the queue?  [It seems, in that case we'd need
another flag to indicate what is to cancel, the delayed work or just the work.]

> > > For autoresume requests coming from an interrupt handler, I'm not so 
> > > sure.  I don't know of any good examples -- in fact there don't seem to 
> > > be any examples at the moment.  (The USB core supports autoresume 
> > > requests in interrupt context, AFAICS no driver uses them.)  So maybe 
> > > it's premature to worry about this case.
> > 
> > Hmm.  Can you please describe a theoretical situation in which this kind
> > of autoresume request will appear?  I'm not really sure what you mean here. :-)
> 
> Well, here's something people discussed but which hasn't been
> implemented (and may never be implemented).  A suspended USB keyboard
> doesn't have enough power to turn on any LEDs.  So suppose a system has
> two keyboards, and kbd-A is autosuspended when the user engages
> CapsLock on kbd-B.  The input layer wants to turn on the CapsLock LED
> on all the keyboards, which means it has to autoresume kbd-A.  But all
> this action takes place in interrupt context, so an autoresume request
> would have to be queued.

I see, but this is similar to the remote wake-up in that we have to queue
the autoresume request.

> > > Lastly, we have resume requests coming from a device -- i.e., remote
> > > wakeup requests.  This is a little tricky.  Suppose you have remote
> > > wakeup enabled, and you press a key on your USB keyboard just as the
> > > computer is going to sleep.  Should this cause the sleep to be aborted?  
> > > Should it cause the computer to wake back up again right away?  Should
> > > the keystroke be ignored?
> > 
> > This is a difficult problem in general.  I _think_ we should ignore such
> > autoresume requests for devices that are not marked as wake-up ones
> > and we should abort the suspend if there's a request from a wake-up device.
> 
> It _is_ difficult.  Right now USB ignores such requests, because the
> threads that handle them are all freezable.  But the information isn't
> lost; it is stored in the hardware and when the system wakes up the
> requests will be seen.
> 
> Except for wakeup requests from root hubs.  If they arrive at just the 
> right time then they are not stored in the hardware -- instead they are 
> stored as work items on the ksuspend_usbd workqueue!  In essence, the 
> workqueue serves as a software mechanism for saving IRQs which were 
> delivered but couldn't be acted on.
> 
> That's why I don't want to erase the work items; it would be logically 
> equivalent to dropping interrupts.

I see.

Still, we'll need to think about a mechanism allowing us to abort suspend
if there's a wake-up request from a wake-up device.

> > Anyway, below is a prototype implementation of a generic runtime PM workqueue,
> > just for discussion.
> > 
> > pm_schedule_suspend() and pm_schedule_resume() are to be called by bus types
> > that are requesting the autosuspend or autoresume, respectively, of a device.
> > The supposed control flow is like this:
> > * bus type calls pm_schedule_suspend() to schedule autosuspend of a device
> > * at the right time the PM workqueue executes the bus type's ->autosuspend()
> >   callback which is responsible for calling the device driver to suspend the
> >   device (the way in which this is done is totally bus type-specific)
> > * if there's a resume event for the device, it's delivered to the bus type
> >   driver (in a bus type-specific way) and the bus type schedules autoresume of
> >   the device by calling pm_schedule_resume()
> > * the PM workqueue executes the bus type's ->autoresume() as soon as it can
> >   and this callback is responsible for calling the device driver to resume the
> >   device (the way in which this is done is totally bus type-specific).
> 
> It looks like the only benefits provided by this patch are the
> workqueue itself and the embedded delayed_work structure.  The ability
> to cancel the work items (or prevent them from being added) during a
> system sleep is questionable, as discussed above.  There isn't any
> value added by the new functions or methods.

The value added is that since the delayed work member is to be used for
queueing work, each bus type will have to do the same casting from
'struct work_struct' to 'struct delayed_work' and next to 'struct device'
to get to the device object, which means quite a bit of code duplication. 

Also, IMO, if there's a set of functions for doing that, the probability
that someone gets things wrong is smaller (slightly).  Moreover, we can
add variants for the works that will be automatically cancelled during
system-wide resume, so that bus types don't have to worry about the details
related to that.

Thanks,
Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Friday 29 May 2009, Magnus Damm wrote:
>> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
>> > On Wednesday 27 May 2009, Magnus Damm wrote:
>> >> From: Magnus Damm <damm@igel.co.jp>
>> >>
>> >> Add platform_device_idle() and platform_device_wakeup()
>> >> and allow architectures to implement their own versions
>> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>> >>
>> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> ---
>> >>
>> >>  Yes, this needs documentation. See [00/04] for now.
>> >>
>> >>  arch/Kconfig                    |    3 +++
>> >>  include/linux/platform_device.h |    8 ++++++++
>> >>  2 files changed, 11 insertions(+)
>> >>
>> >> --- 0002/arch/Kconfig
>> >> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
>> >> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
>> >>
>> >>  config HAVE_PLATFORM_DEVICE_ARCHDATA
>> >>       bool
>> >> +
>> >> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
>> >
>> > Again, I'm not sure if the new CONFIG_ option is really necessary.
>>
>> At least it could be combined into a single option for platform device
>> runtime pm. So no need to have separate kconfig options for [02/04]
>> and [03/04].
>>
>> We could also drop the #ifdef stuff and go for weak symbols that can
>> be overriden by architecture code.
>
> The weak symbols don't work really AFAICS.  People have been replacing them
> with alternative things recently.
>
>> I'm open to suggestions. =)
>
> Well, that depends on what exactly you want to achieve. :-)

=)

I'd like an architecture independent interface upstream that makes it
possible for device drivers to notify the architecture code that they
are idle() and that need wakeup().

In the long term I hope we can share a great deal of runtime PM code
between architectures, but for now we just need some architecture
specific hooks to get started.

The wakeup()/idle() code in this patch is one way to solve it on a
platform device level. Another more generic way would be to add
->enable() and ->disable() callbacks to struct bus_type and introduce
device_enable() and device_disable() that takes struct device and
invokes the bus callbacks if set.

Any thoughts? Does it make sense?

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Friday 29 May 2009, Magnus Damm wrote:
>> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
>> > On Wednesday 27 May 2009, Magnus Damm wrote:
>> >> From: Magnus Damm <damm@igel.co.jp>
>> >>
>> >> Add platform_device_idle() and platform_device_wakeup()
>> >> and allow architectures to implement their own versions
>> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>> >>
>> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> ---
>> >>
>> >>  Yes, this needs documentation. See [00/04] for now.
>> >>
>> >>  arch/Kconfig                    |    3 +++
>> >>  include/linux/platform_device.h |    8 ++++++++
>> >>  2 files changed, 11 insertions(+)
>> >>
>> >> --- 0002/arch/Kconfig
>> >> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
>> >> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
>> >>
>> >>  config HAVE_PLATFORM_DEVICE_ARCHDATA
>> >>       bool
>> >> +
>> >> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
>> >
>> > Again, I'm not sure if the new CONFIG_ option is really necessary.
>>
>> At least it could be combined into a single option for platform device
>> runtime pm. So no need to have separate kconfig options for [02/04]
>> and [03/04].
>>
>> We could also drop the #ifdef stuff and go for weak symbols that can
>> be overriden by architecture code.
>
> The weak symbols don't work really AFAICS.  People have been replacing them
> with alternative things recently.
>
>> I'm open to suggestions. =)
>
> Well, that depends on what exactly you want to achieve. :-)

=)

I'd like an architecture independent interface upstream that makes it
possible for device drivers to notify the architecture code that they
are idle() and that need wakeup().

In the long term I hope we can share a great deal of runtime PM code
between architectures, but for now we just need some architecture
specific hooks to get started.

The wakeup()/idle() code in this patch is one way to solve it on a
platform device level. Another more generic way would be to add
->enable() and ->disable() callbacks to struct bus_type and introduce
device_enable() and device_disable() that takes struct device and
invokes the bus callbacks if set.

Any thoughts? Does it make sense?

/ magnus

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Magnus Damm]

Hi Rafael,

On Wed, Jun 3, 2009 at 6:47 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Tuesday 02 June 2009, Magnus Damm wrote:
>> I have to make sure that the right Kconfig bits are enabled though,
>> otherwise some platform_pm_* functions will be missing. So one merit
>> for the wrapped functions is in this patch is that they are always
>> there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.
>
> Make them depend on CONFIG_PM, then. They are not _that_ much code anyway,
> we can easily afford building them even on platforms that really don't use
> them,
> but I expect them to be used by more and more platforms over time anyway.

Sure, good idea!

>> Is one flag really enough?
>
> Not necessarily, although I think we'll end up using one flag only.
>
>> Isn't it a bit strange from the driver point of view to always get their
>> ->prepare() callback executed, but ->suspend() gets filtered?
>
> In fact, from the run-time PM POV, ->prepare() and ->suspend() should
> always be executed together and ->suspend_noirq() shouldn't be executed
> at all. Now, it may be necessary to execute some code from ->suspend_noirq()
> for run-time PM too, but not by using this callback directly.

Why shouldn't _noirq() be used? I understand that _noirq callbacks
assume that interrupts are not delivered.

It looks to me that most drivers seem to misuse _noirq to work around
ordering issues. They basically use suspend_late() or suspend_noirq()
because they want to be sure that they are suspended after other
drivers. For instance, i2c master controllers seem to want to suspend
using suspend_late() to suspend after the i2c devices on the bus.

Do you have any pointer to code that manages the device hierarchy? Or
is it up to the bus_type?

> Of course we should be avoiding double suspend, so some kind of
> sychronization
> between the "sleep" suspend and run-time suspend is necessary and I agree
> that
> it's probably most convenient to use some flags for this purpose. The
> questions are how many flags we're going to need and how exactly we're going
> to use them.

Exactly!

>> > (3) You can add separate platform callbacks for run-time PM for both the
>> > bus type and the drivers, in which dev_pm_ops will be totally separate
>> > from
>> > these new callbacks, although of course you'll need provide some kind of
>> > synchronization bettween them all.  That also may be done through a flag
>> > in struct platform_device IMO.
>>
>> Yeah, that's also one way. I wonder if that helps us though, I feel
>> that we already have a pretty wide range of callbacks in dev_pm_ops.
>> I'm not sure if they cover all cases we need though, I guess future
>> experiments will tell.
>
> Yes. Still, we can anticipate that the run-time PM operations may be
> slightly
> different from the 'sleep state' PM ones, in which case it makes sens to
> introduce new callbacks, because that makes it clear(er) what the code is
> supposed to do.

Sure, new callbacks are fine as well if you think that's better.

>> If I understand (1)-(3) correctly, then I think (1) is probably the
>> best choice for our platform devices.
>
> OK, for _your_ platform devices it may be the best choice, but what about
> the other platforms' platform devices? Do you think (1) will be suitable for
> them all and if so then why?

I can only speak for SuperH and a bit for ARM. I see a clear need for
for run-time state saving at least. And I think dev_pm_ops ->freeze()
and ->restore() seem like good matches for this, so yes, it's enough
to begin with.

>> I guess (2) is not very far from (1), so if we go with (1) to begin with
>> then we can deal with SoC specific things in our arch code to come closer
>> to
>> (2) over time if needed.

I think (1) is the simplest and does not require much changes to begin
with. People can build stuff on top of this which we later can break
out and make generic.

>> Maybe it's worth to discuss how to integrate this. I suspect that this
>> will only affect some selected architectures to begin with, and the
>> rest of the code base should be unaffected by this change as long as
>> the runtime kconfig is disabled.
>>
>> So I decided to wrap dev_pm_ops to make the impact for non runtime PM
>> systems as small as possible. This while giving the runtime PM case
>> access to all dev_pm_ops regardless of suspend/hibernation kconfig.
>>
>> Does it make sense?
>
> Well, as I said above, I don't really think that run-time PM is going to
> need
> all of the callbacks from dev_pm_ops. Moreover, I'm not sure if it makes
> sense to have more than two (call it 'autosuspend' and 'autoresume' using
> the
> already existing USB terminology) callbacks for run-time PM, at least at
> the driver level.

For SuperH two callbacks for state saving and restoring is enough to
begin with. Functionality wise this is very similar to dev_pm_ops
->freeze() and ->restore() so I think just using those should be fine.

Cheers,

/ magnus

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Magnus Damm]

Hi Rafael,

On Wed, Jun 3, 2009 at 6:47 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Tuesday 02 June 2009, Magnus Damm wrote:
>> I have to make sure that the right Kconfig bits are enabled though,
>> otherwise some platform_pm_* functions will be missing. So one merit
>> for the wrapped functions is in this patch is that they are always
>> there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.
>
> Make them depend on CONFIG_PM, then. They are not _that_ much code anyway,
> we can easily afford building them even on platforms that really don't use
> them,
> but I expect them to be used by more and more platforms over time anyway.

Sure, good idea!

>> Is one flag really enough?
>
> Not necessarily, although I think we'll end up using one flag only.
>
>> Isn't it a bit strange from the driver point of view to always get their
>> ->prepare() callback executed, but ->suspend() gets filtered?
>
> In fact, from the run-time PM POV, ->prepare() and ->suspend() should
> always be executed together and ->suspend_noirq() shouldn't be executed
> at all. Now, it may be necessary to execute some code from ->suspend_noirq()
> for run-time PM too, but not by using this callback directly.

Why shouldn't _noirq() be used? I understand that _noirq callbacks
assume that interrupts are not delivered.

It looks to me that most drivers seem to misuse _noirq to work around
ordering issues. They basically use suspend_late() or suspend_noirq()
because they want to be sure that they are suspended after other
drivers. For instance, i2c master controllers seem to want to suspend
using suspend_late() to suspend after the i2c devices on the bus.

Do you have any pointer to code that manages the device hierarchy? Or
is it up to the bus_type?

> Of course we should be avoiding double suspend, so some kind of
> sychronization
> between the "sleep" suspend and run-time suspend is necessary and I agree
> that
> it's probably most convenient to use some flags for this purpose. The
> questions are how many flags we're going to need and how exactly we're going
> to use them.

Exactly!

>> > (3) You can add separate platform callbacks for run-time PM for both the
>> > bus type and the drivers, in which dev_pm_ops will be totally separate
>> > from
>> > these new callbacks, although of course you'll need provide some kind of
>> > synchronization bettween them all.  That also may be done through a flag
>> > in struct platform_device IMO.
>>
>> Yeah, that's also one way. I wonder if that helps us though, I feel
>> that we already have a pretty wide range of callbacks in dev_pm_ops.
>> I'm not sure if they cover all cases we need though, I guess future
>> experiments will tell.
>
> Yes. Still, we can anticipate that the run-time PM operations may be
> slightly
> different from the 'sleep state' PM ones, in which case it makes sens to
> introduce new callbacks, because that makes it clear(er) what the code is
> supposed to do.

Sure, new callbacks are fine as well if you think that's better.

>> If I understand (1)-(3) correctly, then I think (1) is probably the
>> best choice for our platform devices.
>
> OK, for _your_ platform devices it may be the best choice, but what about
> the other platforms' platform devices? Do you think (1) will be suitable for
> them all and if so then why?

I can only speak for SuperH and a bit for ARM. I see a clear need for
for run-time state saving at least. And I think dev_pm_ops ->freeze()
and ->restore() seem like good matches for this, so yes, it's enough
to begin with.

>> I guess (2) is not very far from (1), so if we go with (1) to begin with
>> then we can deal with SoC specific things in our arch code to come closer
>> to
>> (2) over time if needed.

I think (1) is the simplest and does not require much changes to begin
with. People can build stuff on top of this which we later can break
out and make generic.

>> Maybe it's worth to discuss how to integrate this. I suspect that this
>> will only affect some selected architectures to begin with, and the
>> rest of the code base should be unaffected by this change as long as
>> the runtime kconfig is disabled.
>>
>> So I decided to wrap dev_pm_ops to make the impact for non runtime PM
>> systems as small as possible. This while giving the runtime PM case
>> access to all dev_pm_ops regardless of suspend/hibernation kconfig.
>>
>> Does it make sense?
>
> Well, as I said above, I don't really think that run-time PM is going to
> need
> all of the callbacks from dev_pm_ops. Moreover, I'm not sure if it makes
> sense to have more than two (call it 'autosuspend' and 'autoresume' using
> the
> already existing USB terminology) callbacks for run-time PM, at least at
> the driver level.

For SuperH two callbacks for state saving and restoring is enough to
begin with. Functionality wise this is very similar to dev_pm_ops
->freeze() and ->restore() so I think just using those should be fine.

Cheers,

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Friday 05 June 2009, Magnus Damm wrote:
> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Friday 29 May 2009, Magnus Damm wrote:
> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >>
> >> >> Add platform_device_idle() and platform_device_wakeup()
> >> >> and allow architectures to implement their own versions
> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >> >>
> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >> ---
> >> >>
> >> >>  Yes, this needs documentation. See [00/04] for now.
> >> >>
> >> >>  arch/Kconfig                    |    3 +++
> >> >>  include/linux/platform_device.h |    8 ++++++++
> >> >>  2 files changed, 11 insertions(+)
> >> >>
> >> >> --- 0002/arch/Kconfig
> >> >> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
> >> >> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
> >> >>
> >> >>  config HAVE_PLATFORM_DEVICE_ARCHDATA
> >> >>       bool
> >> >> +
> >> >> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
> >> >
> >> > Again, I'm not sure if the new CONFIG_ option is really necessary.
> >>
> >> At least it could be combined into a single option for platform device
> >> runtime pm. So no need to have separate kconfig options for [02/04]
> >> and [03/04].
> >>
> >> We could also drop the #ifdef stuff and go for weak symbols that can
> >> be overriden by architecture code.
> >
> > The weak symbols don't work really AFAICS.  People have been replacing them
> > with alternative things recently.
> >
> >> I'm open to suggestions. =)
> >
> > Well, that depends on what exactly you want to achieve. :-)
> 
> =)
> 
> I'd like an architecture independent interface upstream that makes it
> possible for device drivers to notify the architecture code that they
> are idle() and that need wakeup().
> 
> In the long term I hope we can share a great deal of runtime PM code
> between architectures, but for now we just need some architecture
> specific hooks to get started.
> 
> The wakeup()/idle() code in this patch is one way to solve it on a
> platform device level. Another more generic way would be to add
> ->enable() and ->disable() callbacks to struct bus_type and introduce
> device_enable() and device_disable() that takes struct device and
> invokes the bus callbacks if set.

So, you need a generic mechanism that drivers can use to notify the bus type
code that a device is idle and therefore it should schedule an autosuspend
request for the device.  Also, you want a mechanism by which drivers can notify
the platform code that there is a need to wake-up an autosuspended device.
Is that correct?

Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Friday 05 June 2009, Magnus Damm wrote:
> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Friday 29 May 2009, Magnus Damm wrote:
> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >>
> >> >> Add platform_device_idle() and platform_device_wakeup()
> >> >> and allow architectures to implement their own versions
> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >> >>
> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >> ---
> >> >>
> >> >>  Yes, this needs documentation. See [00/04] for now.
> >> >>
> >> >>  arch/Kconfig                    |    3 +++
> >> >>  include/linux/platform_device.h |    8 ++++++++
> >> >>  2 files changed, 11 insertions(+)
> >> >>
> >> >> --- 0002/arch/Kconfig
> >> >> +++ work/arch/Kconfig 2009-05-26 12:31:06.000000000 +0900
> >> >> @@ -115,3 +115,6 @@ config HAVE_DEFAULT_NO_SPIN_MUTEXES
> >> >>
> >> >>  config HAVE_PLATFORM_DEVICE_ARCHDATA
> >> >>       bool
> >> >> +
> >> >> +config HAVE_PLATFORM_DEVICE_IDLE_WAKEUP
> >> >
> >> > Again, I'm not sure if the new CONFIG_ option is really necessary.
> >>
> >> At least it could be combined into a single option for platform device
> >> runtime pm. So no need to have separate kconfig options for [02/04]
> >> and [03/04].
> >>
> >> We could also drop the #ifdef stuff and go for weak symbols that can
> >> be overriden by architecture code.
> >
> > The weak symbols don't work really AFAICS.  People have been replacing them
> > with alternative things recently.
> >
> >> I'm open to suggestions. =)
> >
> > Well, that depends on what exactly you want to achieve. :-)
> 
> =)
> 
> I'd like an architecture independent interface upstream that makes it
> possible for device drivers to notify the architecture code that they
> are idle() and that need wakeup().
> 
> In the long term I hope we can share a great deal of runtime PM code
> between architectures, but for now we just need some architecture
> specific hooks to get started.
> 
> The wakeup()/idle() code in this patch is one way to solve it on a
> platform device level. Another more generic way would be to add
> ->enable() and ->disable() callbacks to struct bus_type and introduce
> device_enable() and device_disable() that takes struct device and
> invokes the bus callbacks if set.

So, you need a generic mechanism that drivers can use to notify the bus type
code that a device is idle and therefore it should schedule an autosuspend
request for the device.  Also, you want a mechanism by which drivers can notify
the platform code that there is a need to wake-up an autosuspended device.
Is that correct?

Rafael

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Rafael J. Wysocki]

On Friday 05 June 2009, Magnus Damm wrote:
> Hi Rafael,
> 
> On Wed, Jun 3, 2009 at 6:47 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Tuesday 02 June 2009, Magnus Damm wrote:
> >> I have to make sure that the right Kconfig bits are enabled though,
> >> otherwise some platform_pm_* functions will be missing. So one merit
> >> for the wrapped functions is in this patch is that they are always
> >> there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.
> >
> > Make them depend on CONFIG_PM, then. They are not _that_ much code anyway,
> > we can easily afford building them even on platforms that really don't use
> > them,
> > but I expect them to be used by more and more platforms over time anyway.
> 
> Sure, good idea!
> 
> >> Is one flag really enough?
> >
> > Not necessarily, although I think we'll end up using one flag only.
> >
> >> Isn't it a bit strange from the driver point of view to always get their
> >> ->prepare() callback executed, but ->suspend() gets filtered?
> >
> > In fact, from the run-time PM POV, ->prepare() and ->suspend() should
> > always be executed together and ->suspend_noirq() shouldn't be executed
> > at all. Now, it may be necessary to execute some code from ->suspend_noirq()
> > for run-time PM too, but not by using this callback directly.
> 
> Why shouldn't _noirq() be used? I understand that _noirq callbacks
> assume that interrupts are not delivered.

Yes, the callbacks are exactly for this purpose.

> It looks to me that most drivers seem to misuse _noirq to work around
> ordering issues.

Yes, they do.

> They basically use suspend_late() or suspend_noirq()
> because they want to be sure that they are suspended after other
> drivers. For instance, i2c master controllers seem to want to suspend
> using suspend_late() to suspend after the i2c devices on the bus.
> 
> Do you have any pointer to code that manages the device hierarchy? Or
> is it up to the bus_type?

It should be up to the bus type.  In theory.

In fact we assume that the hierarchy is reflected by the ordering of dpm_list
(ie. "parent" devices should be put on the list before the "child" ones and
therefore they should be suspened later and resumed earlier).  This works
in the majority of cases, because bus controllers are usually registered
before the devices on the bus.  Still, there are other dependencies between
devices that we don't have any means to take into account at the moment.

> > Of course we should be avoiding double suspend, so some kind of
> > sychronization
> > between the "sleep" suspend and run-time suspend is necessary and I agree
> > that
> > it's probably most convenient to use some flags for this purpose. The
> > questions are how many flags we're going to need and how exactly we're going
> > to use them.
> 
> Exactly!
> 
> >> > (3) You can add separate platform callbacks for run-time PM for both the
> >> > bus type and the drivers, in which dev_pm_ops will be totally separate
> >> > from
> >> > these new callbacks, although of course you'll need provide some kind of
> >> > synchronization bettween them all.  That also may be done through a flag
> >> > in struct platform_device IMO.
> >>
> >> Yeah, that's also one way. I wonder if that helps us though, I feel
> >> that we already have a pretty wide range of callbacks in dev_pm_ops.
> >> I'm not sure if they cover all cases we need though, I guess future
> >> experiments will tell.
> >
> > Yes. Still, we can anticipate that the run-time PM operations may be
> > slightly
> > different from the 'sleep state' PM ones, in which case it makes sens to
> > introduce new callbacks, because that makes it clear(er) what the code is
> > supposed to do.
> 
> Sure, new callbacks are fine as well if you think that's better.

I really think so.

> >> If I understand (1)-(3) correctly, then I think (1) is probably the
> >> best choice for our platform devices.
> >
> > OK, for _your_ platform devices it may be the best choice, but what about
> > the other platforms' platform devices? Do you think (1) will be suitable for
> > them all and if so then why?
> 
> I can only speak for SuperH and a bit for ARM. I see a clear need for
> for run-time state saving at least. And I think dev_pm_ops ->freeze()
> and ->restore() seem like good matches for this, so yes, it's enough
> to begin with.
> 
> >> I guess (2) is not very far from (1), so if we go with (1) to begin with
> >> then we can deal with SoC specific things in our arch code to come closer
> >> to
> >> (2) over time if needed.
> 
> I think (1) is the simplest and does not require much changes to begin
> with. People can build stuff on top of this which we later can break
> out and make generic.

I'm not really comfortable with this approach, because it adds the constraint
that the 'system sleep' callbacks should _also_ be suitable for run-time PM.
That need not be the case in principle. 

> >> Maybe it's worth to discuss how to integrate this. I suspect that this
> >> will only affect some selected architectures to begin with, and the
> >> rest of the code base should be unaffected by this change as long as
> >> the runtime kconfig is disabled.
> >>
> >> So I decided to wrap dev_pm_ops to make the impact for non runtime PM
> >> systems as small as possible. This while giving the runtime PM case
> >> access to all dev_pm_ops regardless of suspend/hibernation kconfig.
> >>
> >> Does it make sense?
> >
> > Well, as I said above, I don't really think that run-time PM is going to
> > need
> > all of the callbacks from dev_pm_ops. Moreover, I'm not sure if it makes
> > sense to have more than two (call it 'autosuspend' and 'autoresume' using
> > the
> > already existing USB terminology) callbacks for run-time PM, at least at
> > the driver level.
> 
> For SuperH two callbacks for state saving and restoring is enough to
> begin with. Functionality wise this is very similar to dev_pm_ops
> ->freeze() and ->restore() so I think just using those should be fine.

I'd vote in favor of adding new callbacks.  If they happen to point to the same
code as the other dev_pm_ops callbacks in all cases, we can just drop them
later.  That should be easier than adding new callbacks when it appears they
are actually necessary at one point in future.

Best,
Rafael

Re: [PATCH 03/04] PM: Add platform bus runtime dev_pm_ops

[Rafael J. Wysocki]

On Friday 05 June 2009, Magnus Damm wrote:
> Hi Rafael,
> 
> On Wed, Jun 3, 2009 at 6:47 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Tuesday 02 June 2009, Magnus Damm wrote:
> >> I have to make sure that the right Kconfig bits are enabled though,
> >> otherwise some platform_pm_* functions will be missing. So one merit
> >> for the wrapped functions is in this patch is that they are always
> >> there regardless of CONFIG_SUSPEND and CONFIG_HIBERNATION.
> >
> > Make them depend on CONFIG_PM, then. They are not _that_ much code anyway,
> > we can easily afford building them even on platforms that really don't use
> > them,
> > but I expect them to be used by more and more platforms over time anyway.
> 
> Sure, good idea!
> 
> >> Is one flag really enough?
> >
> > Not necessarily, although I think we'll end up using one flag only.
> >
> >> Isn't it a bit strange from the driver point of view to always get their
> >> ->prepare() callback executed, but ->suspend() gets filtered?
> >
> > In fact, from the run-time PM POV, ->prepare() and ->suspend() should
> > always be executed together and ->suspend_noirq() shouldn't be executed
> > at all. Now, it may be necessary to execute some code from ->suspend_noirq()
> > for run-time PM too, but not by using this callback directly.
> 
> Why shouldn't _noirq() be used? I understand that _noirq callbacks
> assume that interrupts are not delivered.

Yes, the callbacks are exactly for this purpose.

> It looks to me that most drivers seem to misuse _noirq to work around
> ordering issues.

Yes, they do.

> They basically use suspend_late() or suspend_noirq()
> because they want to be sure that they are suspended after other
> drivers. For instance, i2c master controllers seem to want to suspend
> using suspend_late() to suspend after the i2c devices on the bus.
> 
> Do you have any pointer to code that manages the device hierarchy? Or
> is it up to the bus_type?

It should be up to the bus type.  In theory.

In fact we assume that the hierarchy is reflected by the ordering of dpm_list
(ie. "parent" devices should be put on the list before the "child" ones and
therefore they should be suspened later and resumed earlier).  This works
in the majority of cases, because bus controllers are usually registered
before the devices on the bus.  Still, there are other dependencies between
devices that we don't have any means to take into account at the moment.

> > Of course we should be avoiding double suspend, so some kind of
> > sychronization
> > between the "sleep" suspend and run-time suspend is necessary and I agree
> > that
> > it's probably most convenient to use some flags for this purpose. The
> > questions are how many flags we're going to need and how exactly we're going
> > to use them.
> 
> Exactly!
> 
> >> > (3) You can add separate platform callbacks for run-time PM for both the
> >> > bus type and the drivers, in which dev_pm_ops will be totally separate
> >> > from
> >> > these new callbacks, although of course you'll need provide some kind of
> >> > synchronization bettween them all.  That also may be done through a flag
> >> > in struct platform_device IMO.
> >>
> >> Yeah, that's also one way. I wonder if that helps us though, I feel
> >> that we already have a pretty wide range of callbacks in dev_pm_ops.
> >> I'm not sure if they cover all cases we need though, I guess future
> >> experiments will tell.
> >
> > Yes. Still, we can anticipate that the run-time PM operations may be
> > slightly
> > different from the 'sleep state' PM ones, in which case it makes sens to
> > introduce new callbacks, because that makes it clear(er) what the code is
> > supposed to do.
> 
> Sure, new callbacks are fine as well if you think that's better.

I really think so.

> >> If I understand (1)-(3) correctly, then I think (1) is probably the
> >> best choice for our platform devices.
> >
> > OK, for _your_ platform devices it may be the best choice, but what about
> > the other platforms' platform devices? Do you think (1) will be suitable for
> > them all and if so then why?
> 
> I can only speak for SuperH and a bit for ARM. I see a clear need for
> for run-time state saving at least. And I think dev_pm_ops ->freeze()
> and ->restore() seem like good matches for this, so yes, it's enough
> to begin with.
> 
> >> I guess (2) is not very far from (1), so if we go with (1) to begin with
> >> then we can deal with SoC specific things in our arch code to come closer
> >> to
> >> (2) over time if needed.
> 
> I think (1) is the simplest and does not require much changes to begin
> with. People can build stuff on top of this which we later can break
> out and make generic.

I'm not really comfortable with this approach, because it adds the constraint
that the 'system sleep' callbacks should _also_ be suitable for run-time PM.
That need not be the case in principle. 

> >> Maybe it's worth to discuss how to integrate this. I suspect that this
> >> will only affect some selected architectures to begin with, and the
> >> rest of the code base should be unaffected by this change as long as
> >> the runtime kconfig is disabled.
> >>
> >> So I decided to wrap dev_pm_ops to make the impact for non runtime PM
> >> systems as small as possible. This while giving the runtime PM case
> >> access to all dev_pm_ops regardless of suspend/hibernation kconfig.
> >>
> >> Does it make sense?
> >
> > Well, as I said above, I don't really think that run-time PM is going to
> > need
> > all of the callbacks from dev_pm_ops. Moreover, I'm not sure if it makes
> > sense to have more than two (call it 'autosuspend' and 'autoresume' using
> > the
> > already existing USB terminology) callbacks for run-time PM, at least at
> > the driver level.
> 
> For SuperH two callbacks for state saving and restoring is enough to
> begin with. Functionality wise this is very similar to dev_pm_ops
> ->freeze() and ->restore() so I think just using those should be fine.

I'd vote in favor of adding new callbacks.  If they happen to point to the same
code as the other dev_pm_ops callbacks in all cases, we can just drop them
later.  That should be easier than adding new callbacks when it appears they
are actually necessary at one point in future.

Best,
Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Friday 05 June 2009, Magnus Damm wrote:
>> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>> > On Friday 29 May 2009, Magnus Damm wrote:
>> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
>> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
>> >> >> From: Magnus Damm <damm@igel.co.jp>
>> >> >>
>> >> >> Add platform_device_idle() and platform_device_wakeup()
>> >> >> and allow architectures to implement their own versions
>> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>> >> >>
>> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> >> ---

>> The wakeup()/idle() code in this patch is one way to solve it on a
>> platform device level. Another more generic way would be to add
>> ->enable() and ->disable() callbacks to struct bus_type and introduce
>> device_enable() and device_disable() that takes struct device and
>> invokes the bus callbacks if set.
>
> So, you need a generic mechanism that drivers can use to notify the bus type
> code that a device is idle and therefore it should schedule an autosuspend
> request for the device.  Also, you want a mechanism by which drivers can notify
> the platform code that there is a need to wake-up an autosuspended device.
> Is that correct?

Yes, you are 100% correct that I want drivers to have some way to
notify the bus type that a certain device is idle or needs to be woken
up.

Exactly what should happen when the device is marked as idle is a
different question. I guess this is bus specific. Connecting the idle
notification directly to autosuspend is not a very good idea IMO since
the power management comes with latency restrictions.

If we zoom out a bit then I think that we should have something
similar to cpuidle but for devices. Maybe the driver should give a
list of suspend modes, their latencies and power savings. This per
driver (or per device) latency information is important, but even more
important IMO is latency information for the bus itself.

So for our on-chip SuperH SoC platform devices I'd like to keep track
of which devices that are idle, and if all devices within one power
domain are idle then i'd like to execute autosuspend() for those and
after that power off the bus/domain. But only if this doesn't break
any latency requirements.

Cheers,

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Friday 05 June 2009, Magnus Damm wrote:
>> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>> > On Friday 29 May 2009, Magnus Damm wrote:
>> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
>> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
>> >> >> From: Magnus Damm <damm@igel.co.jp>
>> >> >>
>> >> >> Add platform_device_idle() and platform_device_wakeup()
>> >> >> and allow architectures to implement their own versions
>> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>> >> >>
>> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> >> ---

>> The wakeup()/idle() code in this patch is one way to solve it on a
>> platform device level. Another more generic way would be to add
>> ->enable() and ->disable() callbacks to struct bus_type and introduce
>> device_enable() and device_disable() that takes struct device and
>> invokes the bus callbacks if set.
>
> So, you need a generic mechanism that drivers can use to notify the bus type
> code that a device is idle and therefore it should schedule an autosuspend
> request for the device.  Also, you want a mechanism by which drivers can notify
> the platform code that there is a need to wake-up an autosuspended device.
> Is that correct?

Yes, you are 100% correct that I want drivers to have some way to
notify the bus type that a certain device is idle or needs to be woken
up.

Exactly what should happen when the device is marked as idle is a
different question. I guess this is bus specific. Connecting the idle
notification directly to autosuspend is not a very good idea IMO since
the power management comes with latency restrictions.

If we zoom out a bit then I think that we should have something
similar to cpuidle but for devices. Maybe the driver should give a
list of suspend modes, their latencies and power savings. This per
driver (or per device) latency information is important, but even more
important IMO is latency information for the bus itself.

So for our on-chip SuperH SoC platform devices I'd like to keep track
of which devices that are idle, and if all devices within one power
domain are idle then i'd like to execute autosuspend() for those and
after that power off the bus/domain. But only if this doesn't break
any latency requirements.

Cheers,

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Tuesday 09 June 2009, Magnus Damm wrote:
> On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Friday 05 June 2009, Magnus Damm wrote:
> >> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> >> > On Friday 29 May 2009, Magnus Damm wrote:
> >> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> >> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >> >>
> >> >> >> Add platform_device_idle() and platform_device_wakeup()
> >> >> >> and allow architectures to implement their own versions
> >> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >> >> >>
> >> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >> >> ---
> 
> >> The wakeup()/idle() code in this patch is one way to solve it on a
> >> platform device level. Another more generic way would be to add
> >> ->enable() and ->disable() callbacks to struct bus_type and introduce
> >> device_enable() and device_disable() that takes struct device and
> >> invokes the bus callbacks if set.
> >
> > So, you need a generic mechanism that drivers can use to notify the bus type
> > code that a device is idle and therefore it should schedule an autosuspend
> > request for the device.  Also, you want a mechanism by which drivers can notify
> > the platform code that there is a need to wake-up an autosuspended device.
> > Is that correct?
> 
> Yes, you are 100% correct that I want drivers to have some way to
> notify the bus type that a certain device is idle or needs to be woken
> up.
> 
> Exactly what should happen when the device is marked as idle is a
> different question. I guess this is bus specific. Connecting the idle
> notification directly to autosuspend is not a very good idea IMO since
> the power management comes with latency restrictions.
> 
> If we zoom out a bit then I think that we should have something
> similar to cpuidle but for devices. Maybe the driver should give a
> list of suspend modes, their latencies and power savings. This per
> driver (or per device) latency information is important, but even more
> important IMO is latency information for the bus itself.
> 
> So for our on-chip SuperH SoC platform devices I'd like to keep track
> of which devices that are idle, and if all devices within one power
> domain are idle then i'd like to execute autosuspend() for those and
> after that power off the bus/domain. But only if this doesn't break
> any latency requirements.

OK, I think we can add ->idle() and ->wakeup() callbacks to struct bus_type
for this purpose.

BTW, I'm waiting for a new version of your patch adding the arch data to
struct platform_device with a better changelog.

Best,
Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Tuesday 09 June 2009, Magnus Damm wrote:
> On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Friday 05 June 2009, Magnus Damm wrote:
> >> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> >> > On Friday 29 May 2009, Magnus Damm wrote:
> >> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> >> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >> >>
> >> >> >> Add platform_device_idle() and platform_device_wakeup()
> >> >> >> and allow architectures to implement their own versions
> >> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >> >> >>
> >> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >> >> ---
> 
> >> The wakeup()/idle() code in this patch is one way to solve it on a
> >> platform device level. Another more generic way would be to add
> >> ->enable() and ->disable() callbacks to struct bus_type and introduce
> >> device_enable() and device_disable() that takes struct device and
> >> invokes the bus callbacks if set.
> >
> > So, you need a generic mechanism that drivers can use to notify the bus type
> > code that a device is idle and therefore it should schedule an autosuspend
> > request for the device.  Also, you want a mechanism by which drivers can notify
> > the platform code that there is a need to wake-up an autosuspended device.
> > Is that correct?
> 
> Yes, you are 100% correct that I want drivers to have some way to
> notify the bus type that a certain device is idle or needs to be woken
> up.
> 
> Exactly what should happen when the device is marked as idle is a
> different question. I guess this is bus specific. Connecting the idle
> notification directly to autosuspend is not a very good idea IMO since
> the power management comes with latency restrictions.
> 
> If we zoom out a bit then I think that we should have something
> similar to cpuidle but for devices. Maybe the driver should give a
> list of suspend modes, their latencies and power savings. This per
> driver (or per device) latency information is important, but even more
> important IMO is latency information for the bus itself.
> 
> So for our on-chip SuperH SoC platform devices I'd like to keep track
> of which devices that are idle, and if all devices within one power
> domain are idle then i'd like to execute autosuspend() for those and
> after that power off the bus/domain. But only if this doesn't break
> any latency requirements.

OK, I think we can add ->idle() and ->wakeup() callbacks to struct bus_type
for this purpose.

BTW, I'm waiting for a new version of your patch adding the arch data to
struct platform_device with a better changelog.

Best,
Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

On Wed, Jun 10, 2009 at 8:41 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Tuesday 09 June 2009, Magnus Damm wrote:
>> On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>> > On Friday 05 June 2009, Magnus Damm wrote:
>> >> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>> >> > On Friday 29 May 2009, Magnus Damm wrote:
>> >> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
>> >> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
>> >> >> >> From: Magnus Damm <damm@igel.co.jp>
>> >> >> >>
>> >> >> >> Add platform_device_idle() and platform_device_wakeup()
>> >> >> >> and allow architectures to implement their own versions
>> >> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>> >> >> >>
>> >> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> >> >> ---
>>
>> >> The wakeup()/idle() code in this patch is one way to solve it on a
>> >> platform device level. Another more generic way would be to add
>> >> ->enable() and ->disable() callbacks to struct bus_type and introduce
>> >> device_enable() and device_disable() that takes struct device and
>> >> invokes the bus callbacks if set.
>> >
>> > So, you need a generic mechanism that drivers can use to notify the bus type
>> > code that a device is idle and therefore it should schedule an autosuspend
>> > request for the device.  Also, you want a mechanism by which drivers can notify
>> > the platform code that there is a need to wake-up an autosuspended device.
>> > Is that correct?
>>
>> Yes, you are 100% correct that I want drivers to have some way to
>> notify the bus type that a certain device is idle or needs to be woken
>> up.
>>
>> Exactly what should happen when the device is marked as idle is a
>> different question. I guess this is bus specific. Connecting the idle
>> notification directly to autosuspend is not a very good idea IMO since
>> the power management comes with latency restrictions.
>>
>> If we zoom out a bit then I think that we should have something
>> similar to cpuidle but for devices. Maybe the driver should give a
>> list of suspend modes, their latencies and power savings. This per
>> driver (or per device) latency information is important, but even more
>> important IMO is latency information for the bus itself.
>>
>> So for our on-chip SuperH SoC platform devices I'd like to keep track
>> of which devices that are idle, and if all devices within one power
>> domain are idle then i'd like to execute autosuspend() for those and
>> after that power off the bus/domain. But only if this doesn't break
>> any latency requirements.
>
> OK, I think we can add ->idle() and ->wakeup() callbacks to struct bus_type
> for this purpose.

Sounds very good! So unless there are any objections I'll just post a
"Driver Core: Add idle and wakeup functions V2" which adds ->idle()
and ->wakeup() callbacks to struct bus_type together with inline asm
functions device_idle() and device_wakeup().

> BTW, I'm waiting for a new version of your patch adding the arch data to
> struct platform_device with a better changelog.

Yeah, sorry about the delay. I will post an updated version!

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Magnus Damm]

On Wed, Jun 10, 2009 at 8:41 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Tuesday 09 June 2009, Magnus Damm wrote:
>> On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>> > On Friday 05 June 2009, Magnus Damm wrote:
>> >> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>> >> > On Friday 29 May 2009, Magnus Damm wrote:
>> >> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
>> >> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
>> >> >> >> From: Magnus Damm <damm@igel.co.jp>
>> >> >> >>
>> >> >> >> Add platform_device_idle() and platform_device_wakeup()
>> >> >> >> and allow architectures to implement their own versions
>> >> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
>> >> >> >>
>> >> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> >> >> ---
>>
>> >> The wakeup()/idle() code in this patch is one way to solve it on a
>> >> platform device level. Another more generic way would be to add
>> >> ->enable() and ->disable() callbacks to struct bus_type and introduce
>> >> device_enable() and device_disable() that takes struct device and
>> >> invokes the bus callbacks if set.
>> >
>> > So, you need a generic mechanism that drivers can use to notify the bus type
>> > code that a device is idle and therefore it should schedule an autosuspend
>> > request for the device.  Also, you want a mechanism by which drivers can notify
>> > the platform code that there is a need to wake-up an autosuspended device.
>> > Is that correct?
>>
>> Yes, you are 100% correct that I want drivers to have some way to
>> notify the bus type that a certain device is idle or needs to be woken
>> up.
>>
>> Exactly what should happen when the device is marked as idle is a
>> different question. I guess this is bus specific. Connecting the idle
>> notification directly to autosuspend is not a very good idea IMO since
>> the power management comes with latency restrictions.
>>
>> If we zoom out a bit then I think that we should have something
>> similar to cpuidle but for devices. Maybe the driver should give a
>> list of suspend modes, their latencies and power savings. This per
>> driver (or per device) latency information is important, but even more
>> important IMO is latency information for the bus itself.
>>
>> So for our on-chip SuperH SoC platform devices I'd like to keep track
>> of which devices that are idle, and if all devices within one power
>> domain are idle then i'd like to execute autosuspend() for those and
>> after that power off the bus/domain. But only if this doesn't break
>> any latency requirements.
>
> OK, I think we can add ->idle() and ->wakeup() callbacks to struct bus_type
> for this purpose.

Sounds very good! So unless there are any objections I'll just post a
"Driver Core: Add idle and wakeup functions V2" which adds ->idle()
and ->wakeup() callbacks to struct bus_type together with inline asm
functions device_idle() and device_wakeup().

> BTW, I'm waiting for a new version of your patch adding the arch data to
> struct platform_device with a better changelog.

Yeah, sorry about the delay. I will post an updated version!

/ magnus

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Wednesday 10 June 2009, Magnus Damm wrote:
> On Wed, Jun 10, 2009 at 8:41 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Tuesday 09 June 2009, Magnus Damm wrote:
> >> On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> >> > On Friday 05 June 2009, Magnus Damm wrote:
> >> >> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> >> >> > On Friday 29 May 2009, Magnus Damm wrote:
> >> >> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> >> >> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> >> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >> >> >>
> >> >> >> >> Add platform_device_idle() and platform_device_wakeup()
> >> >> >> >> and allow architectures to implement their own versions
> >> >> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >> >> >> >>
> >> >> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >> >> >> ---
> >>
> >> >> The wakeup()/idle() code in this patch is one way to solve it on a
> >> >> platform device level. Another more generic way would be to add
> >> >> ->enable() and ->disable() callbacks to struct bus_type and introduce
> >> >> device_enable() and device_disable() that takes struct device and
> >> >> invokes the bus callbacks if set.
> >> >
> >> > So, you need a generic mechanism that drivers can use to notify the bus type
> >> > code that a device is idle and therefore it should schedule an autosuspend
> >> > request for the device.  Also, you want a mechanism by which drivers can notify
> >> > the platform code that there is a need to wake-up an autosuspended device.
> >> > Is that correct?
> >>
> >> Yes, you are 100% correct that I want drivers to have some way to
> >> notify the bus type that a certain device is idle or needs to be woken
> >> up.
> >>
> >> Exactly what should happen when the device is marked as idle is a
> >> different question. I guess this is bus specific. Connecting the idle
> >> notification directly to autosuspend is not a very good idea IMO since
> >> the power management comes with latency restrictions.
> >>
> >> If we zoom out a bit then I think that we should have something
> >> similar to cpuidle but for devices. Maybe the driver should give a
> >> list of suspend modes, their latencies and power savings. This per
> >> driver (or per device) latency information is important, but even more
> >> important IMO is latency information for the bus itself.
> >>
> >> So for our on-chip SuperH SoC platform devices I'd like to keep track
> >> of which devices that are idle, and if all devices within one power
> >> domain are idle then i'd like to execute autosuspend() for those and
> >> after that power off the bus/domain. But only if this doesn't break
> >> any latency requirements.
> >
> > OK, I think we can add ->idle() and ->wakeup() callbacks to struct bus_type
> > for this purpose.
> 
> Sounds very good! So unless there are any objections I'll just post a
> "Driver Core: Add idle and wakeup functions V2" which adds ->idle()
> and ->wakeup() callbacks to struct bus_type together with inline asm
> functions device_idle() and device_wakeup().

Well, I'd prefer to do that along with my run-time PM patch that is currently
being discussed with Alan and Oliver (I've sent a Cc of the first message
in that thread to you).

> > BTW, I'm waiting for a new version of your patch adding the arch data to
> > struct platform_device with a better changelog.
> 
> Yeah, sorry about the delay. I will post an updated version!

Great, thanks!

Best,
Rafael

Re: [PATCH 02/04] Driver Core: Add idle and wakeup functions

[Rafael J. Wysocki]

On Wednesday 10 June 2009, Magnus Damm wrote:
> On Wed, Jun 10, 2009 at 8:41 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Tuesday 09 June 2009, Magnus Damm wrote:
> >> On Sat, Jun 6, 2009 at 5:42 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> >> > On Friday 05 June 2009, Magnus Damm wrote:
> >> >> On Wed, Jun 3, 2009 at 6:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> >> >> > On Friday 29 May 2009, Magnus Damm wrote:
> >> >> >> 2009/5/29 Rafael J. Wysocki <rjw@sisk.pl>:
> >> >> >> > On Wednesday 27 May 2009, Magnus Damm wrote:
> >> >> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >> >> >>
> >> >> >> >> Add platform_device_idle() and platform_device_wakeup()
> >> >> >> >> and allow architectures to implement their own versions
> >> >> >> >> of these if CONFIG_HAVE_PLATFORM_IDLE_WAKEUP is set.
> >> >> >> >>
> >> >> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >> >> >> ---
> >>
> >> >> The wakeup()/idle() code in this patch is one way to solve it on a
> >> >> platform device level. Another more generic way would be to add
> >> >> ->enable() and ->disable() callbacks to struct bus_type and introduce
> >> >> device_enable() and device_disable() that takes struct device and
> >> >> invokes the bus callbacks if set.
> >> >
> >> > So, you need a generic mechanism that drivers can use to notify the bus type
> >> > code that a device is idle and therefore it should schedule an autosuspend
> >> > request for the device.  Also, you want a mechanism by which drivers can notify
> >> > the platform code that there is a need to wake-up an autosuspended device.
> >> > Is that correct?
> >>
> >> Yes, you are 100% correct that I want drivers to have some way to
> >> notify the bus type that a certain device is idle or needs to be woken
> >> up.
> >>
> >> Exactly what should happen when the device is marked as idle is a
> >> different question. I guess this is bus specific. Connecting the idle
> >> notification directly to autosuspend is not a very good idea IMO since
> >> the power management comes with latency restrictions.
> >>
> >> If we zoom out a bit then I think that we should have something
> >> similar to cpuidle but for devices. Maybe the driver should give a
> >> list of suspend modes, their latencies and power savings. This per
> >> driver (or per device) latency information is important, but even more
> >> important IMO is latency information for the bus itself.
> >>
> >> So for our on-chip SuperH SoC platform devices I'd like to keep track
> >> of which devices that are idle, and if all devices within one power
> >> domain are idle then i'd like to execute autosuspend() for those and
> >> after that power off the bus/domain. But only if this doesn't break
> >> any latency requirements.
> >
> > OK, I think we can add ->idle() and ->wakeup() callbacks to struct bus_type
> > for this purpose.
> 
> Sounds very good! So unless there are any objections I'll just post a
> "Driver Core: Add idle and wakeup functions V2" which adds ->idle()
> and ->wakeup() callbacks to struct bus_type together with inline asm
> functions device_idle() and device_wakeup().

Well, I'd prefer to do that along with my run-time PM patch that is currently
being discussed with Alan and Oliver (I've sent a Cc of the first message
in that thread to you).

> > BTW, I'm waiting for a new version of your patch adding the arch data to
> > struct platform_device with a better changelog.
> 
> Yeah, sorry about the delay. I will post an updated version!

Great, thanks!

Best,
Rafael

Re: [linux-pm] [PATCH 00/04][RFC] PM: Runtime platform device PM

[Pavel Machek]

Hi!

> > Sure, conditions may have changed while the system was asleep.  They might
> > also change while the system is awake.  In general, whether or not the system
> > was asleep shouldn't make any difference -- to as great an extent as possible
> > we should strive to pretend that the entire sleep took no time at all (or
> > simply didn't happen).
> > 
> > So: Suppose the system had been awake when the access point
> > disappeared.  What would the wireless adapter driver do then?  It
> > should try to do exactly the same thing if the access point disappears
> > while the system is asleep.
> 
> Not really.  Namely, when the access point _vanishes_ at run time, this is an
> error condition from which we have to recover, while if it's _not_ _present_
> after a resume, it's a normal situation and we shouldn't be recovering from
> that (the user might have changed the physical location while
> suspended).

I disagree here. User can just walk away with powered notebook.


> > You see?  The sleep should be transparent.
> 
> In some cases it won't be, because the hardware state changes while sleeping.
> For example, a laptop battery may be depleted while suspended, so that after
> resume it's in the 'low' condition, while it was in the 'good' condition before
> the preceding suspend.  Arguably, you can't drain a battery from 90% to 10%,
> for example, momentarily.  Also, a CPU fan might be 100% on before
> suspend,

Actually... going 90%->10% immediately is common behaviour of old
li-ion batteries.

> because the CPU was hot at that time, but that doesn't mean the fan should be
> 100% on after the subsequent resume, because it's likely that the CPU will be
> cold (on some systems trying to make the fan spin too fast may lead to
> general problems with thermal management afterwards).

100% is indeed sane default for fan. Yes you should slow the fan down
when you read the real temperature. And yes some systems play it safe
and do 100% fan during boot.

(Otherwise you may have heat problems if you suspend and immediately
resume).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 00/04][RFC] PM: Runtime platform device PM

[Pavel Machek]

Hi!

> > Sure, conditions may have changed while the system was asleep.  They might
> > also change while the system is awake.  In general, whether or not the system
> > was asleep shouldn't make any difference -- to as great an extent as possible
> > we should strive to pretend that the entire sleep took no time at all (or
> > simply didn't happen).
> > 
> > So: Suppose the system had been awake when the access point
> > disappeared.  What would the wireless adapter driver do then?  It
> > should try to do exactly the same thing if the access point disappears
> > while the system is asleep.
> 
> Not really.  Namely, when the access point _vanishes_ at run time, this is an
> error condition from which we have to recover, while if it's _not_ _present_
> after a resume, it's a normal situation and we shouldn't be recovering from
> that (the user might have changed the physical location while
> suspended).

I disagree here. User can just walk away with powered notebook.


> > You see?  The sleep should be transparent.
> 
> In some cases it won't be, because the hardware state changes while sleeping.
> For example, a laptop battery may be depleted while suspended, so that after
> resume it's in the 'low' condition, while it was in the 'good' condition before
> the preceding suspend.  Arguably, you can't drain a battery from 90% to 10%,
> for example, momentarily.  Also, a CPU fan might be 100% on before
> suspend,

Actually... going 90%->10% immediately is common behaviour of old
li-ion batteries.

> because the CPU was hot at that time, but that doesn't mean the fan should be
> 100% on after the subsequent resume, because it's likely that the CPU will be
> cold (on some systems trying to make the fan spin too fast may lead to
> general problems with thermal management afterwards).

100% is indeed sane default for fan. Yes you should slow the fan down
when you read the real temperature. And yes some systems play it safe
and do 100% fan during boot.

(Otherwise you may have heat problems if you suspend and immediately
resume).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html