[PATCH] rework pm_ops pm_disk_modes foo

[PATCH] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1: 011-pm-ops-diskmode.patch --]
[-- Type: text/plain, Size: 12601 bytes --]

The pm_ops.pm_disk_mode is used in totally bogus ways since
nobody really seems to understand what it actually does.

This patch clarifies what pm_disk_mode is actually for by
splitting it up into default_pm_disk_mode and a bitmask of
pm_disk_modes.

It also removes all the arm and sh users that think they can veto
suspend to disk via pm_ops; not so since the user can always
do echo shutdown > /sys/power/disk, you need to find a better
way involving Kconfig or such.

ACPI is the only user left with a non-zero pm_disk_modes bitmask.

The patch also sets the default mode to shutdown again, but
when a new pm_ops is registered its default mode is honoured.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Andriy Skulysh <askulsyh@gmail.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Cliff Brake <cbrake@accelent.com>
Cc: Dirk Behme <dirk.behme@de.bosch.com>
Cc: Nicolas Pitre <nico@cam.org>
Cc: David Singleton <daviado@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Cc: Ben Dooks <ben@simtec.co.uk>
Cc: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
Cc: David Shaohua Li <shaohua.li@intel.com>
Cc: Patrick Mochel <mochel@osdl.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.osdl.org

---
 arch/arm/common/sharpsl_pm.c |    7 ++---
 arch/arm/mach-at91/pm.c      |    1 
 arch/arm/mach-omap1/pm.c     |    1 
 arch/arm/mach-omap2/pm.c     |    1 
 arch/arm/mach-pxa/pm.c       |    4 ---
 arch/arm/mach-sa1100/pm.c    |    9 -------
 arch/arm/plat-s3c24xx/pm.c   |    9 -------
 arch/sh/boards/hp6xx/pm.c    |    7 -----
 drivers/acpi/sleep/main.c    |   10 +++++---
 include/linux/pm.h           |   18 ++++++++++-----
 kernel/power/disk.c          |   51 ++++++++++++++++++++++++++++++-------------
 kernel/power/main.c          |    7 +++++
 12 files changed, 65 insertions(+), 60 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 02:18:54.830252495 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 02:26:32.270252495 +0100
@@ -137,24 +137,30 @@ typedef int __bitwise suspend_disk_metho
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
- * @pm_disk_mode: Set to the disk method that the user should be able to
- *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
- *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
- *	are always allowed, currently only %PM_DISK_PLATFORM
- *	makes sense. If the user then choses %PM_DISK_PLATFORM,
+ * @pm_disk_modes: Set to a bitfield of the available shutdown methods
+ *	the user should be able to configure for suspend-to-disk, use
+ *	1<<%PM_DISK_PLATFORM etc. %PM_DISK_SHUTDOWN, %PM_DISK_REBOOT,
+ *	%PM_DISK_TEST and %PM_DISK_TESTPROC are always allowed regardless
+ *	of whether they are included in this mask.
+ *	If the user then choses any one other than those four,
  *	the @prepare call will be called before suspending to disk
  *	(if present), the @enter call should be present and will
  *	be called after all state has been saved and the machine
  *	is ready to be shut down/suspended/..., and the @finish
  *	callback is called after state has been restored. All
  *	these calls are called with %PM_SUSPEND_DISK as the state.
+ *	Note that despite all this, you absolutely must turn off
+ *	the machine and not just suspend it.
+ *
+ * @default_pm_disk_mode: Set to the default suspend to disk method.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
-	suspend_disk_method_t pm_disk_mode;
+	unsigned long pm_disk_modes;
+	suspend_disk_method_t default_pm_disk_mode;
 };
 
 /**
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 02:18:54.960252495 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 02:19:12.750252495 +0100
@@ -39,7 +39,13 @@ static inline int platform_prepare(void)
 {
 	int error = 0;
 
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->prepare)
 			error = pm_ops->prepare(PM_SUSPEND_DISK);
 	}
@@ -56,23 +62,28 @@ static inline int platform_prepare(void)
  *	there ain't no turning back.
  */
 
-static void power_down(suspend_disk_method_t mode)
+static void power_down(void)
 {
 	disable_nonboot_cpus();
-	switch(mode) {
-	case PM_DISK_PLATFORM:
-		if (pm_ops && pm_ops->enter) {
-			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-			pm_ops->enter(PM_SUSPEND_DISK);
-			break;
-		}
+
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+		break;
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;
 	case PM_DISK_REBOOT:
 		kernel_restart(NULL);
 		break;
+	default:
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	}
+
 	kernel_halt();
 	/* Valid image is on the disk, if we continue we risk serious data corruption
 	   after resume. */
@@ -82,7 +93,13 @@ static void power_down(suspend_disk_meth
 
 static inline void platform_finish(void)
 {
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->finish)
 			pm_ops->finish(PM_SUSPEND_DISK);
 	}
@@ -167,7 +184,7 @@ int pm_suspend_disk(void)
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
 		if (!error)
-			power_down(pm_disk_mode);
+			power_down();
 		else {
 			swsusp_free();
 			goto Thaw;
@@ -347,12 +364,16 @@ static ssize_t disk_store(struct subsyst
 		}
 	}
 	if (mode) {
-		if (mode == PM_DISK_SHUTDOWN || mode == PM_DISK_REBOOT ||
-		     mode == PM_DISK_TEST || mode == PM_DISK_TESTPROC) {
+		switch (mode) {
+		case PM_DISK_SHUTDOWN:
+		case PM_DISK_REBOOT:
+		case PM_DISK_TEST:
+		case PM_DISK_TESTPROC:
 			pm_disk_mode = mode;
-		} else {
+			break;
+		default:
 			if (pm_ops && pm_ops->enter &&
-			    (mode == pm_ops->pm_disk_mode))
+			    ((1<<mode) & pm_ops->pm_disk_modes))
 				pm_disk_mode = mode;
 			else
 				error = -EINVAL;
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 02:18:54.850252495 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 02:57:38.400252495 +0100
@@ -30,7 +30,7 @@
 DEFINE_MUTEX(pm_mutex);
 
 struct pm_ops *pm_ops;
-suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
+suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
  *	pm_set_ops - Set the global power method table. 
@@ -41,6 +41,11 @@ void pm_set_ops(struct pm_ops * ops)
 {
 	mutex_lock(&pm_mutex);
 	pm_ops = ops;
+	if (ops && ops->default_pm_disk_mode) {
+		WARN_ON(!( (1<<ops->default_pm_disk_mode) & ops->pm_disk_modes));
+		pm_disk_mode = ops->default_pm_disk_mode;
+	} else
+		pm_disk_mode = PM_DISK_SHUTDOWN;
 	mutex_unlock(&pm_mutex);
 }
 
--- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 02:18:55.080252495 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 02:19:12.750252495 +0100
@@ -766,10 +766,9 @@ static void sharpsl_apm_get_power_status
 }
 
 static struct pm_ops sharpsl_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
-	.prepare	= pxa_pm_prepare,
-	.enter		= corgi_pxa_pm_enter,
-	.finish		= pxa_pm_finish,
+	.prepare		= pxa_pm_prepare,
+	.enter			= corgi_pxa_pm_enter,
+	.finish			= pxa_pm_finish,
 };
 
 static int __init sharpsl_pm_probe(struct platform_device *pdev)
--- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 02:18:55.290252495 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 02:19:12.750252495 +0100
@@ -201,7 +201,6 @@ error:
 
 
 static struct pm_ops at91_pm_ops ={
-	.pm_disk_mode	= 0,
 	.valid		= at91_pm_valid_state,
 	.prepare	= at91_pm_prepare,
 	.enter		= at91_pm_enter,
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 02:18:55.180252495 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -698,7 +698,6 @@ static struct irqaction omap_wakeup_irq 
 
 
 static struct pm_ops omap_pm_ops ={
-	.pm_disk_mode	= 0,
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 02:18:55.250252495 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -370,7 +370,6 @@ static int omap2_pm_finish(suspend_state
 }
 
 static struct pm_ops omap_pm_ops = {
-	.pm_disk_mode	= 0,
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 02:18:55.350252495 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -223,11 +223,7 @@ int pxa_pm_finish(suspend_state_t state)
 
 EXPORT_SYMBOL_GPL(pxa_pm_finish);
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops pxa_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 02:18:55.450252495 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -59,9 +59,6 @@ static int sa11x0_pm_enter(suspend_state
 	unsigned long gpio, sleep_save[SLEEP_SAVE_SIZE];
 	struct timespec delta, rtc;
 
-	if (state != PM_SUSPEND_MEM)
-		return -EINVAL;
-
 	/* preserve current time */
 	rtc.tv_sec = RCNR;
 	rtc.tv_nsec = 0;
@@ -134,12 +131,8 @@ unsigned long sleep_phys_sp(void *sp)
 	return virt_to_phys(sp);
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops sa11x0_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
-	.enter		= sa11x0_pm_enter,
+	.enter			= sa11x0_pm_enter,
 };
 
 static int __init sa11x0_pm_init(void)
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:18:55.520252495 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:19:12.770252495 +0100
@@ -511,11 +511,6 @@ static int s3c2410_pm_enter(suspend_stat
 		return -EINVAL;
 	}
 
-	if (state != PM_SUSPEND_MEM) {
-		printk(KERN_ERR PFX "error: only PM_SUSPEND_MEM supported\n");
-		return -EINVAL;
-	}
-
 	/* check if we have anything to wake-up with... bad things seem
 	 * to happen if you suspend with no wakeup (system will often
 	 * require a full power-cycle)
@@ -633,11 +628,7 @@ static int s3c2410_pm_finish(suspend_sta
 	return 0;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops s3c2410_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
 	.finish		= s3c2410_pm_finish,
--- linux-2.6.orig/drivers/acpi/sleep/main.c	2007-03-20 02:18:55.050252495 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 02:19:12.770252495 +0100
@@ -95,7 +95,7 @@ static int acpi_pm_enter(suspend_state_t
 		break;
 
 	case PM_SUSPEND_DISK:
-		if (acpi_pm_ops.pm_disk_mode == PM_DISK_PLATFORM)
+		if (acpi_pm_ops.default_pm_disk_mode == PM_DISK_PLATFORM)
 			status = acpi_enter_sleep_state(acpi_state);
 		break;
 	case PM_SUSPEND_MAX:
@@ -219,8 +219,12 @@ int __init acpi_sleep_init(void)
 			printk(" S%d", i);
 		}
 		if (i == ACPI_STATE_S4) {
-			if (sleep_states[i])
-				acpi_pm_ops.pm_disk_mode = PM_DISK_PLATFORM;
+			if (sleep_states[i]) {
+				acpi_pm_ops.default_pm_disk_mode =
+					PM_DISK_PLATFORM;
+				acpi_pm_ops.pm_disk_modes =
+					1<<PM_DISK_PLATFORM;
+			}
 		}
 	}
 	printk(")\n");
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:18:55.600252495 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:19:12.770252495 +0100
@@ -27,9 +27,6 @@ static int hp6x0_pm_enter(suspend_state_
 	u16 hd64461_stbcr;
 #endif
 
-	if (state != PM_SUSPEND_MEM)
-		return -EINVAL;
-
 #ifdef CONFIG_HD64461_ENABLER
 	outb(0, HD64461_PCC1CSCIER);
 
@@ -70,11 +67,7 @@ static int hp6x0_pm_enter(suspend_state_
 	return 0;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops hp6x0_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.enter		= hp6x0_pm_enter,
 };
 

--

[PATCH v2] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1: 011-pm-ops-diskmode.patch --]
[-- Type: text/plain, Size: 11564 bytes --]

The pm_ops.pm_disk_mode is used in totally bogus ways since
nobody really seems to understand what it actually does.

This patch clarifies the pm_disk_mode description.

It also removes all the arm and sh users that think they can veto
suspend to disk via pm_ops; not so since the user can always
do echo shutdown > /sys/power/disk, you need to find a better
way involving Kconfig or such.

ACPI is the only user left with a non-zero pm_disk_mode.

The patch also sets the default mode to shutdown again, but
when a new pm_ops is registered its default mode is honoured.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Cliff Brake <cbrake@accelent.com>
Cc: Dirk Behme <dirk.behme@de.bosch.com>
Cc: Nicolas Pitre <nico@cam.org>
Cc: David Singleton <daviado@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Cc: Ben Dooks <ben@simtec.co.uk>
Cc: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
Cc: David Shaohua Li <shaohua.li@intel.com>
Cc: Patrick Mochel <mochelp@infinity.powertie.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.osdl.org

---
 arch/arm/common/sharpsl_pm.c |    1 
 arch/arm/mach-at91/pm.c      |    1 
 arch/arm/mach-omap1/pm.c     |    1 
 arch/arm/mach-omap2/pm.c     |    1 
 arch/arm/mach-pxa/pm.c       |    4 ---
 arch/arm/mach-sa1100/pm.c    |    7 ------
 arch/arm/plat-s3c24xx/pm.c   |    9 -------
 arch/sh/boards/hp6xx/pm.c    |    7 ------
 include/linux/pm.h           |   25 ++++++++++++---------
 kernel/power/disk.c          |   49 ++++++++++++++++++++++++++++++-------------
 kernel/power/main.c          |    6 ++++-
 11 files changed, 54 insertions(+), 57 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 02:18:54.830252495 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 09:45:16.760886698 +0100
@@ -112,6 +112,8 @@ typedef int __bitwise suspend_state_t;
 
 typedef int __bitwise suspend_disk_method_t;
 
+/* invalid must be 0 so struct pm_ops initialisers can leave it out */
+#define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
 #define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
 #define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
 #define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
@@ -137,17 +139,18 @@ typedef int __bitwise suspend_disk_metho
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
- * @pm_disk_mode: Set to the disk method that the user should be able to
- *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
- *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
- *	are always allowed, currently only %PM_DISK_PLATFORM
- *	makes sense. If the user then choses %PM_DISK_PLATFORM,
- *	the @prepare call will be called before suspending to disk
- *	(if present), the @enter call should be present and will
- *	be called after all state has been saved and the machine
- *	is ready to be shut down/suspended/..., and the @finish
- *	callback is called after state has been restored. All
- *	these calls are called with %PM_SUSPEND_DISK as the state.
+ * @pm_disk_mode: The generic code always allows one of the shutdown methods
+ *	%PM_DISK_SHUTDOWN, %PM_DISK_REBOOT, %PM_DISK_TEST and
+ *	%PM_DISK_TESTPROC. If this variable is set, the mode it is set
+ *	to is allowed in addition to those modes and is also made default.
+ *	When this mode is sent selected, the @prepare call will be called
+ *	before suspending to disk (if present), the @enter call should be
+ *	present and will be called after all state has been saved and the
+ *	machine is ready to be powered off; the @finish callback is called
+ *	after state has been restored. All these calls are called with
+ *	%PM_SUSPEND_DISK as the state.
+ *
+ * @default_pm_disk_mode: Set to the default suspend to disk method.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 02:18:54.960252495 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 09:40:54.120886698 +0100
@@ -39,7 +39,13 @@ static inline int platform_prepare(void)
 {
 	int error = 0;
 
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->prepare)
 			error = pm_ops->prepare(PM_SUSPEND_DISK);
 	}
@@ -56,23 +62,28 @@ static inline int platform_prepare(void)
  *	there ain't no turning back.
  */
 
-static void power_down(suspend_disk_method_t mode)
+static void power_down(void)
 {
 	disable_nonboot_cpus();
-	switch(mode) {
-	case PM_DISK_PLATFORM:
-		if (pm_ops && pm_ops->enter) {
-			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-			pm_ops->enter(PM_SUSPEND_DISK);
-			break;
-		}
+
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+		break;
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;
 	case PM_DISK_REBOOT:
 		kernel_restart(NULL);
 		break;
+	default:
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	}
+
 	kernel_halt();
 	/* Valid image is on the disk, if we continue we risk serious data corruption
 	   after resume. */
@@ -82,7 +93,13 @@ static void power_down(suspend_disk_meth
 
 static inline void platform_finish(void)
 {
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->finish)
 			pm_ops->finish(PM_SUSPEND_DISK);
 	}
@@ -167,7 +184,7 @@ int pm_suspend_disk(void)
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
 		if (!error)
-			power_down(pm_disk_mode);
+			power_down();
 		else {
 			swsusp_free();
 			goto Thaw;
@@ -347,10 +364,14 @@ static ssize_t disk_store(struct subsyst
 		}
 	}
 	if (mode) {
-		if (mode == PM_DISK_SHUTDOWN || mode == PM_DISK_REBOOT ||
-		     mode == PM_DISK_TEST || mode == PM_DISK_TESTPROC) {
+		switch (mode) {
+		case PM_DISK_SHUTDOWN:
+		case PM_DISK_REBOOT:
+		case PM_DISK_TEST:
+		case PM_DISK_TESTPROC:
 			pm_disk_mode = mode;
-		} else {
+			break;
+		default:
 			if (pm_ops && pm_ops->enter &&
 			    (mode == pm_ops->pm_disk_mode))
 				pm_disk_mode = mode;
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 02:18:54.850252495 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 09:44:44.610886698 +0100
@@ -30,7 +30,7 @@
 DEFINE_MUTEX(pm_mutex);
 
 struct pm_ops *pm_ops;
-suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
+suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
  *	pm_set_ops - Set the global power method table. 
@@ -41,6 +41,10 @@ void pm_set_ops(struct pm_ops * ops)
 {
 	mutex_lock(&pm_mutex);
 	pm_ops = ops;
+	if (ops && ops->pm_disk_mode != PM_DISK_INVALID) {
+		pm_disk_mode = ops->pm_disk_mode;
+	} else
+		pm_disk_mode = PM_DISK_SHUTDOWN;
 	mutex_unlock(&pm_mutex);
 }
 
--- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 02:18:55.080252495 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 09:42:09.100886698 +0100
@@ -766,7 +766,6 @@ static void sharpsl_apm_get_power_status
 }
 
 static struct pm_ops sharpsl_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= pxa_pm_prepare,
 	.enter		= corgi_pxa_pm_enter,
 	.finish		= pxa_pm_finish,
--- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 02:18:55.290252495 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 02:19:12.750252495 +0100
@@ -201,7 +201,6 @@ error:
 
 
 static struct pm_ops at91_pm_ops ={
-	.pm_disk_mode	= 0,
 	.valid		= at91_pm_valid_state,
 	.prepare	= at91_pm_prepare,
 	.enter		= at91_pm_enter,
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 02:18:55.180252495 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -698,7 +698,6 @@ static struct irqaction omap_wakeup_irq 
 
 
 static struct pm_ops omap_pm_ops ={
-	.pm_disk_mode	= 0,
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 02:18:55.250252495 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -370,7 +370,6 @@ static int omap2_pm_finish(suspend_state
 }
 
 static struct pm_ops omap_pm_ops = {
-	.pm_disk_mode	= 0,
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 02:18:55.350252495 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 02:19:12.760252495 +0100
@@ -223,11 +223,7 @@ int pxa_pm_finish(suspend_state_t state)
 
 EXPORT_SYMBOL_GPL(pxa_pm_finish);
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops pxa_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 02:18:55.450252495 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 09:42:32.170886698 +0100
@@ -59,9 +59,6 @@ static int sa11x0_pm_enter(suspend_state
 	unsigned long gpio, sleep_save[SLEEP_SAVE_SIZE];
 	struct timespec delta, rtc;
 
-	if (state != PM_SUSPEND_MEM)
-		return -EINVAL;
-
 	/* preserve current time */
 	rtc.tv_sec = RCNR;
 	rtc.tv_nsec = 0;
@@ -134,11 +131,7 @@ unsigned long sleep_phys_sp(void *sp)
 	return virt_to_phys(sp);
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops sa11x0_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.enter		= sa11x0_pm_enter,
 };
 
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:18:55.520252495 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:19:12.770252495 +0100
@@ -511,11 +511,6 @@ static int s3c2410_pm_enter(suspend_stat
 		return -EINVAL;
 	}
 
-	if (state != PM_SUSPEND_MEM) {
-		printk(KERN_ERR PFX "error: only PM_SUSPEND_MEM supported\n");
-		return -EINVAL;
-	}
-
 	/* check if we have anything to wake-up with... bad things seem
 	 * to happen if you suspend with no wakeup (system will often
 	 * require a full power-cycle)
@@ -633,11 +628,7 @@ static int s3c2410_pm_finish(suspend_sta
 	return 0;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops s3c2410_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
 	.finish		= s3c2410_pm_finish,
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:18:55.600252495 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:19:12.770252495 +0100
@@ -27,9 +27,6 @@ static int hp6x0_pm_enter(suspend_state_
 	u16 hd64461_stbcr;
 #endif
 
-	if (state != PM_SUSPEND_MEM)
-		return -EINVAL;
-
 #ifdef CONFIG_HD64461_ENABLER
 	outb(0, HD64461_PCC1CSCIER);
 
@@ -70,11 +67,7 @@ static int hp6x0_pm_enter(suspend_state_
 	return 0;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops hp6x0_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.enter		= hp6x0_pm_enter,
 };
 

--

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Pavel Machek]

Hi!

> --- linux-2.6.orig/include/linux/pm.h	2007-03-20 02:18:54.830252495 +0100
> +++ linux-2.6/include/linux/pm.h	2007-03-20 09:45:16.760886698 +0100
> @@ -112,6 +112,8 @@ typedef int __bitwise suspend_state_t;
>  
>  typedef int __bitwise suspend_disk_method_t;
>  
> +/* invalid must be 0 so struct pm_ops initialisers can leave it out */
> +#define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
>  #define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
>  #define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
>  #define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
> @@ -137,17 +139,18 @@ typedef int __bitwise suspend_disk_metho
>   * @finish: Called when the system has left the given state and all devices
>   *	are resumed. The return value is ignored.
>   *
> - * @pm_disk_mode: Set to the disk method that the user should be able to
> - *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
> - *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
> - *	are always allowed, currently only %PM_DISK_PLATFORM
> - *	makes sense. If the user then choses %PM_DISK_PLATFORM,
> - *	the @prepare call will be called before suspending to disk
> - *	(if present), the @enter call should be present and will
> - *	be called after all state has been saved and the machine
> - *	is ready to be shut down/suspended/..., and the @finish
> - *	callback is called after state has been restored. All
> - *	these calls are called with %PM_SUSPEND_DISK as the state.
> + * @pm_disk_mode: The generic code always allows one of the shutdown methods
> + *	%PM_DISK_SHUTDOWN, %PM_DISK_REBOOT, %PM_DISK_TEST and
> + *	%PM_DISK_TESTPROC. If this variable is set, the mode it is set
> + *	to is allowed in addition to those modes and is also made default.
> + *	When this mode is sent selected, the @prepare call will be called
> + *	before suspending to disk (if present), the @enter call should be
> + *	present and will be called after all state has been saved and the
> + *	machine is ready to be powered off; the @finish callback is called
> + *	after state has been restored. All these calls are called with
> + *	%PM_SUSPEND_DISK as the state.

Is the pm_disk_mode still bitmask? If yes, say so.

...no, it does not appear so.

> --- linux-2.6.orig/kernel/power/main.c	2007-03-20 02:18:54.850252495 +0100
> +++ linux-2.6/kernel/power/main.c	2007-03-20 09:44:44.610886698 +0100
> @@ -30,7 +30,7 @@
>  DEFINE_MUTEX(pm_mutex);
>  
>  struct pm_ops *pm_ops;
> -suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
> +suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;

Please don't do this. We want to keep the "use platform if available"
behaviour. [Changing platform->shutdown is really _big_ change,
independend from any cleanups, and it needs to go separate at the very
least. It will break some machines.]

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

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Johannes Berg]

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

On Tue, 2007-03-20 at 10:31 +0100, Pavel Machek wrote:

> Is the pm_disk_mode still bitmask? If yes, say so.
> 
> ...no, it does not appear so.

No, I should have explained. Since the prepare/enter/finish callbacks
aren't told what the chosen pm_disk_mode is, there is no point in
allowing multiple since they can't differentiate. Changing that is
something that could be done, but doesn't seem necessary since ACPI is
the only user.

> Please don't do this. We want to keep the "use platform if available"
> behaviour. [Changing platform->shutdown is really _big_ change,
> independend from any cleanups, and it needs to go separate at the very
> least. It will break some machines.]

As far as I can tell "use platform if available" means "use platform
when ACPI pm_ops are present" since that's the only pm_ops that has
platform. And notice that I change the default when pm_ops are
registered.

If, of course, my assumption about ACPI here is wrong, then some other
pm_ops implementations will need to be changed to have .pm_disk_mode =
PM_DISK_PLATFORM.

However, using PM_DISK_PLATFORM by default w/o pm_ops support is *wrong*
as I explained previously since it leads to user-interface
inconsistencies, the user can switch *away* from platform but *not back*
to platform.

johannes

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

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

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Pavel Machek]

Hi!

> > Is the pm_disk_mode still bitmask? If yes, say so.
> > 
> > ...no, it does not appear so.
> 
> No, I should have explained. Since the prepare/enter/finish callbacks
> aren't told what the chosen pm_disk_mode is, there is no point in
> allowing multiple since they can't differentiate. Changing that is
> something that could be done, but doesn't seem necessary since ACPI is
> the only user.

I guess we'll have more such users in future.

> > Please don't do this. We want to keep the "use platform if available"
> > behaviour. [Changing platform->shutdown is really _big_ change,
> > independend from any cleanups, and it needs to go separate at the very
> > least. It will break some machines.]
> 
> As far as I can tell "use platform if available" means "use platform
> when ACPI pm_ops are present" since that's the only pm_ops that has
> platform. And notice that I change the default when pm_ops are
> registered.

i did not look _that_ closely. As long as platform is still default on
acpi systems, we are okay.

> However, using PM_DISK_PLATFORM by default w/o pm_ops support is *wrong*
> as I explained previously since it leads to user-interface
> inconsistencies, the user can switch *away* from platform but *not back*
> to platform.

No argument about that.



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

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Johannes Berg]

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

Hi,

> I guess we'll have more such users in future.

Right, but then the question remains whether multiple disk-shutdown
modes make sense. I doubt it.

> i did not look _that_ closely. As long as platform is still default on
> acpi systems, we are okay.

It is. And there are no other users of pm_ops that can do this.

johannes

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

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

[PATCH] add firmware disk state and clean up

[Johannes Berg]

This patch adds a new state PM_SUSPEND_FIRMWAREDISK state and cleans up
all pm_ops users to include a .valid callback which in most cases allows
only mem suspend since except ACPI they all don't differentiate between
standby/mem which are currently allowed.

This new state has no users yet but I think APM could be converted to
it. My old laptop used to be able to suspend to disk from firmware, but
it died so I couldn't test such a conversion.

In any case, having a .valid callback will be essential if more suspend
modes are added, and this makes it clearer by example.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 arch/arm/common/sharpsl_pm.c |    6 ++++++
 arch/arm/mach-at91/pm.c      |    5 +++++
 arch/arm/mach-omap1/pm.c     |    6 +++++-
 arch/arm/mach-omap2/pm.c     |    6 ++++++
 arch/arm/mach-pnx4008/pm.c   |   34 +++-------------------------------
 arch/arm/mach-pxa/pm.c       |    6 ++++++
 arch/arm/mach-sa1100/pm.c    |    6 ++++++
 arch/arm/plat-s3c24xx/pm.c   |   18 +++---------------
 arch/sh/boards/hp6xx/pm.c    |    6 ++++++
 drivers/acpi/sleep/main.c    |   13 +++++++++++--
 include/linux/pm.h           |   16 ++++++++--------
 kernel/power/disk.c          |    3 +--
 kernel/power/main.c          |    9 ++++++---
 13 files changed, 72 insertions(+), 62 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 10:47:36.543214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 10:48:06.643214909 +0100
@@ -108,19 +108,19 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_STANDBY	((__force suspend_state_t) 1)
 #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
 #define PM_SUSPEND_DISK		((__force suspend_state_t) 4)
-#define PM_SUSPEND_MAX		((__force suspend_state_t) 5)
+#define PM_SUSPEND_FIRMWAREDISK	((__force suspend_state_t) 5)
+#define PM_SUSPEND_MAX		((__force suspend_state_t) 6)
 
 typedef int __bitwise suspend_disk_method_t;
 
 /* invalid must be 0 so struct pm_ops initialisers can leave it out */
 #define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
-#define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
-#define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
-#define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
-#define	PM_DISK_REBOOT		((__force suspend_disk_method_t) 4)
-#define	PM_DISK_TEST		((__force suspend_disk_method_t) 5)
-#define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 6)
-#define	PM_DISK_MAX		((__force suspend_disk_method_t) 7)
+#define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 1)
+#define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 2)
+#define	PM_DISK_REBOOT		((__force suspend_disk_method_t) 3)
+#define	PM_DISK_TEST		((__force suspend_disk_method_t) 4)
+#define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 5)
+#define	PM_DISK_MAX		((__force suspend_disk_method_t) 6)
 
 /**
  * struct pm_ops - Callbacks for managing platform dependent suspend states.
--- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:02.053214909 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:58.963214909 +0100
@@ -765,10 +765,16 @@ static void sharpsl_apm_get_power_status
 	info->battery_life = sharpsl_pm.battstat.mainbat_percent;
 }
 
+static int pxa_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops sharpsl_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= corgi_pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pxa_pm_valid,
 };
 
 static int __init sharpsl_pm_probe(struct platform_device *pdev)
--- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 10:55:35.433214909 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 10:56:02.243214909 +0100
@@ -199,11 +199,16 @@ error:
 	return 0;
 }
 
+static int at91_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
 
 static struct pm_ops at91_pm_ops ={
 	.valid		= at91_pm_valid_state,
 	.prepare	= at91_pm_prepare,
 	.enter		= at91_pm_enter,
+	.valid		= at91_pm_valid,
 };
 
 static int __init at91_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 10:52:17.403214909 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 10:52:51.483214909 +0100
@@ -695,12 +695,16 @@ static struct irqaction omap_wakeup_irq 
 	.handler	= omap_wakeup_interrupt
 };
 
-
+static int omap_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
 
 static struct pm_ops omap_pm_ops ={
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
+	.valid		= omap_pm_valid,
 };
 
 static int __init omap_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 10:52:56.603214909 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 10:53:22.353214909 +0100
@@ -369,10 +369,16 @@ static int omap2_pm_finish(suspend_state
 	return 0;
 }
 
+static int omap2_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops omap_pm_ops = {
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
+	.valid		= omap2_pm_valid,
 };
 
 int __init omap2_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pnx4008/pm.c	2007-03-20 10:53:29.803214909 +0100
+++ linux-2.6/arch/arm/mach-pnx4008/pm.c	2007-03-20 11:03:50.133214909 +0100
@@ -115,42 +115,14 @@ static int pnx4008_pm_enter(suspend_stat
 	return 0;
 }
 
-/*
- * Called after processes are frozen, but before we shut down devices.
- */
-static int pnx4008_pm_prepare(suspend_state_t state)
+static int pnx4008_pm_valid(suspend_state_t state)
 {
-	switch (state) {
-	case PM_SUSPEND_STANDBY:
-	case PM_SUSPEND_MEM:
-		break;
-
-	case PM_SUSPEND_DISK:
-		return -ENOTSUPP;
-		break;
-
-	default:
-		return -EINVAL;
-		break;
-	}
-	return 0;
-}
-
-/*
- * Called after devices are re-setup, but before processes are thawed.
- */
-static int pnx4008_pm_finish(suspend_state_t state)
-{
-	return 0;
+	return state == PM_SUSPEND_MEM;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops pnx4008_pm_ops = {
-	.prepare = pnx4008_pm_prepare,
 	.enter = pnx4008_pm_enter,
-	.finish = pnx4008_pm_finish,
+	.valid = pnx4008_pm_valid,
 };
 
 static int __init pnx4008_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 10:54:29.943214909 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 10:54:59.263214909 +0100
@@ -223,10 +223,16 @@ int pxa_pm_finish(suspend_state_t state)
 
 EXPORT_SYMBOL_GPL(pxa_pm_finish);
 
+static int pxa_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops pxa_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pxa_pm_valid,
 };
 
 static int __init pxa_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 10:55:05.173214909 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 10:55:30.583214909 +0100
@@ -131,8 +131,14 @@ unsigned long sleep_phys_sp(void *sp)
 	return virt_to_phys(sp);
 }
 
+static int sa11x0_pm_valid(suspend_state_t state)
+{
+	return state == PM_STATE_MEM;
+}
+
 static struct pm_ops sa11x0_pm_ops = {
 	.enter		= sa11x0_pm_enter,
+	.valid		= sa11x0_pm_valid,
 };
 
 static int __init sa11x0_pm_init(void)
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 10:56:06.093214909 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 10:56:47.963214909 +0100
@@ -612,26 +612,14 @@ static int s3c2410_pm_enter(suspend_stat
 	return 0;
 }
 
-/*
- * Called after processes are frozen, but before we shut down devices.
- */
-static int s3c2410_pm_prepare(suspend_state_t state)
+static int s3c2410_pm_valid(suspend_state_t state)
 {
-	return 0;
-}
-
-/*
- * Called after devices are re-setup, but before processes are thawed.
- */
-static int s3c2410_pm_finish(suspend_state_t state)
-{
-	return 0;
+	return state == PM_SUSPEND_MEM;
 }
 
 static struct pm_ops s3c2410_pm_ops = {
-	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
-	.finish		= s3c2410_pm_finish,
+	.valid		= s3c2410_pm_valid,
 };
 
 /* s3c2410_pm_init
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 10:57:05.693214909 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 10:58:04.543214909 +0100
@@ -67,8 +67,14 @@ static int hp6x0_pm_enter(suspend_state_
 	return 0;
 }
 
+static int hp6x0_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops hp6x0_pm_ops = {
 	.enter		= hp6x0_pm_enter,
+	.valid		= hp6x0_pm_valid,
 };
 
 static int __init hp6x0_pm_init(void)
--- linux-2.6.orig/drivers/acpi/sleep/main.c	2007-03-20 10:58:09.733214909 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 11:02:27.133214909 +0100
@@ -168,9 +168,18 @@ int acpi_suspend(u32 acpi_state)
 
 static int acpi_pm_state_valid(suspend_state_t pm_state)
 {
-	u32 acpi_state = acpi_suspend_states[pm_state];
+	u32 acpi_state;
 
-	return sleep_states[acpi_state];
+	switch (pm_state) {
+	case PM_SUSPEND_ON:
+	case PM_SUSPEND_STANDBY:
+	case PM_SUSPEND_MEM:
+		acpi_state = acpi_suspend_states[pm_state];
+
+		return sleep_states[acpi_state];
+	default:
+		return 0;
+	}
 }
 
 static struct pm_ops acpi_pm_ops = {
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 11:11:57.793214909 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 11:12:13.683214909 +0100
@@ -304,7 +304,6 @@ late_initcall(software_resume);
 
 
 static const char * const pm_disk_modes[] = {
-	[PM_DISK_FIRMWARE]	= "firmware",
 	[PM_DISK_PLATFORM]	= "platform",
 	[PM_DISK_SHUTDOWN]	= "shutdown",
 	[PM_DISK_REBOOT]	= "reboot",
@@ -357,7 +356,7 @@ static ssize_t disk_store(struct subsyst
 	len = p ? p - buf : n;
 
 	mutex_lock(&pm_mutex);
-	for (i = PM_DISK_FIRMWARE; i < PM_DISK_MAX; i++) {
+	for (i = PM_DISK_PLATFORM; i < PM_DISK_MAX; i++) {
 		if (!strncmp(buf, pm_disk_modes[i], len)) {
 			mode = i;
 			break;
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 11:13:31.173214909 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 11:15:30.153214909 +0100
@@ -157,11 +157,14 @@ static void suspend_finish(suspend_state
 
 
 static const char * const pm_states[PM_SUSPEND_MAX] = {
-	[PM_SUSPEND_STANDBY]	= "standby",
-	[PM_SUSPEND_MEM]	= "mem",
+	"standby",
+	"mem",
 #ifdef CONFIG_SOFTWARE_SUSPEND
-	[PM_SUSPEND_DISK]	= "disk",
+	"disk",
+#else
+	NULL,
 #endif
+	"firmware-disk",
 };
 
 static inline int valid_state(suspend_state_t state)

Re: [PATCH] add firmware disk state and clean up

[Pavel Machek]

Hi!

> This patch adds a new state PM_SUSPEND_FIRMWAREDISK state and cleans up
> all pm_ops users to include a .valid callback which in most cases allows
> only mem suspend since except ACPI they all don't differentiate between
> standby/mem which are currently allowed.

> This new state has no users yet but I think APM could be converted

The person doing the conversion should add the state,
sorry... ... actually it makes sense, but I'd prefer not to have it,
yet.

> --- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:02.053214909 +0100
> +++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:58.963214909 +0100
> @@ -765,10 +765,16 @@ static void sharpsl_apm_get_power_status
>  	info->battery_life = sharpsl_pm.battstat.mainbat_percent;
>  }
>  
> +static int pxa_pm_valid(suspend_state_t state)
> +{
> +	return state == PM_SUSPEND_MEM;
> +}

create static inline pm_valid_only_mem() somewhere in header, then
share it around the code?

> --- linux-2.6.orig/kernel/power/main.c	2007-03-20 11:13:31.173214909 +0100
> +++ linux-2.6/kernel/power/main.c	2007-03-20 11:15:30.153214909 +0100
> @@ -157,11 +157,14 @@ static void suspend_finish(suspend_state
>  
>  
>  static const char * const pm_states[PM_SUSPEND_MAX] = {
> -	[PM_SUSPEND_STANDBY]	= "standby",
> -	[PM_SUSPEND_MEM]	= "mem",
> +	"standby",
> +	"mem",
>  #ifdef CONFIG_SOFTWARE_SUSPEND
> -	[PM_SUSPEND_DISK]	= "disk",
> +	"disk",
> +#else
> +	NULL,
>  #endif
> +	"firmware-disk",
>  };

I'd keep the c99 syntax here. (And we do not need firmware-disk just
yet).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] add firmware disk state and clean up

[Johannes Berg]

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

On Tue, 2007-03-20 at 11:25 +0100, Pavel Machek wrote:

> The person doing the conversion should add the state,
> sorry... ... actually it makes sense, but I'd prefer not to have it,
> yet.

That's ok. The patch is here for that person to implement it ;)

> create static inline pm_valid_only_mem() somewhere in header, then
> share it around the code?

Actually, it can't be inline if it is to be assigned to a function
pointer, but it an be an exported function. Will do that.


> I'd keep the c99 syntax here.

I prefer the c99 syntax as well, but at least this gives compile errors
when somebody adds a new state. Then again, I suppose when they add a
new state they'll want to test it and quickly notice that they need to
do something to make it accessible.

johannes

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

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

[PATCH] remove firmware disk mode

[Johannes Berg]

This patch removes the firmware disk suspend mode which is the wrong
approach, it is supposed to be used for implementing firmware-based disk
suspend but cannot actually be used for that.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 include/linux/pm.h  |   13 ++++++-------
 kernel/power/disk.c |    3 +--
 2 files changed, 7 insertions(+), 9 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 11:55:22.663214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 11:56:09.783214909 +0100
@@ -114,13 +114,12 @@ typedef int __bitwise suspend_disk_metho
 
 /* invalid must be 0 so struct pm_ops initialisers can leave it out */
 #define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
-#define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
-#define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
-#define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
-#define	PM_DISK_REBOOT		((__force suspend_disk_method_t) 4)
-#define	PM_DISK_TEST		((__force suspend_disk_method_t) 5)
-#define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 6)
-#define	PM_DISK_MAX		((__force suspend_disk_method_t) 7)
+#define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 1)
+#define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 2)
+#define	PM_DISK_REBOOT		((__force suspend_disk_method_t) 3)
+#define	PM_DISK_TEST		((__force suspend_disk_method_t) 4)
+#define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 5)
+#define	PM_DISK_MAX		((__force suspend_disk_method_t) 6)
 
 /**
  * struct pm_ops - Callbacks for managing platform dependent suspend states.
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 11:55:23.353214909 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 11:56:09.783214909 +0100
@@ -304,7 +304,6 @@ late_initcall(software_resume);
 
 
 static const char * const pm_disk_modes[] = {
-	[PM_DISK_FIRMWARE]	= "firmware",
 	[PM_DISK_PLATFORM]	= "platform",
 	[PM_DISK_SHUTDOWN]	= "shutdown",
 	[PM_DISK_REBOOT]	= "reboot",
@@ -357,7 +356,7 @@ static ssize_t disk_store(struct subsyst
 	len = p ? p - buf : n;
 
 	mutex_lock(&pm_mutex);
-	for (i = PM_DISK_FIRMWARE; i < PM_DISK_MAX; i++) {
+	for (i = PM_DISK_PLATFORM; i < PM_DISK_MAX; i++) {
 		if (!strncmp(buf, pm_disk_modes[i], len)) {
 			mode = i;
 			break;

Re: [PATCH] remove firmware disk mode

[Pavel Machek]

Hi!

> This patch removes the firmware disk suspend mode which is the wrong
> approach, it is supposed to be used for implementing firmware-based disk
> suspend but cannot actually be used for that.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

ACK. (You may want to send it to akpm).
								Pavel

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

[PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

Almost all users of pm_ops only support mem sleep, don't check in .valid
and don't reject any others in .prepare so users can be confused if they
check /sys/power/state, especially when new states are ever added.

This patch implements a generic pm_valid_only_mem function that is then
exported for users as well as using it in almost all pm_ops.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 arch/arm/common/sharpsl_pm.c |    1 +
 arch/arm/mach-omap1/pm.c     |    1 +
 arch/arm/mach-omap2/pm.c     |    1 +
 arch/arm/mach-pnx4008/pm.c   |   35 +----------------------------------
 arch/arm/mach-pxa/pm.c       |    1 +
 arch/arm/mach-sa1100/pm.c    |    1 +
 arch/arm/plat-s3c24xx/pm.c   |   19 +------------------
 arch/sh/boards/hp6xx/pm.c    |    1 +
 drivers/acpi/sleep/main.c    |   13 +++++++++++--
 include/linux/pm.h           |    4 ++++
 kernel/power/main.c          |   13 +++++++++++++
 11 files changed, 36 insertions(+), 54 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 11:56:09.783214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 12:01:46.583214909 +0100
@@ -128,6 +128,9 @@ typedef int __bitwise suspend_disk_metho
  *	always valid and never passed to this call.
  *	If not assigned, all suspend states are advertised as valid
  *	in /sys/power/state (but can still be rejected by prepare or enter.)
+ *	Since new states can be added for other platforms, you should
+ *	assign this callback. There is a %pm_valid_only_mem function
+ *	available if you only implemented mem sleep.
  *
  * @prepare: Prepare the platform for the given suspend state. Can return a
  *	negative error code if necessary.
@@ -167,6 +170,7 @@ extern void pm_set_ops(struct pm_ops *pm
 extern struct pm_ops *pm_ops;
 extern int pm_suspend(suspend_state_t state);
 
+extern int pm_valid_only_mem(suspend_state_t state);
 
 /*
  * Device power management
--- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 11:55:22.693214909 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 11:56:34.173214909 +0100
@@ -769,6 +769,7 @@ static struct pm_ops sharpsl_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= corgi_pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init sharpsl_pm_probe(struct platform_device *pdev)
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 11:55:22.793214909 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 11:56:34.173214909 +0100
@@ -701,6 +701,7 @@ static struct pm_ops omap_pm_ops ={
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init omap_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 11:55:22.823214909 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 11:56:34.183214909 +0100
@@ -373,6 +373,7 @@ static struct pm_ops omap_pm_ops = {
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 int __init omap2_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pnx4008/pm.c	2007-03-20 11:55:22.883214909 +0100
+++ linux-2.6/arch/arm/mach-pnx4008/pm.c	2007-03-20 11:56:34.183214909 +0100
@@ -115,42 +115,9 @@ static int pnx4008_pm_enter(suspend_stat
 	return 0;
 }
 
-/*
- * Called after processes are frozen, but before we shut down devices.
- */
-static int pnx4008_pm_prepare(suspend_state_t state)
-{
-	switch (state) {
-	case PM_SUSPEND_STANDBY:
-	case PM_SUSPEND_MEM:
-		break;
-
-	case PM_SUSPEND_DISK:
-		return -ENOTSUPP;
-		break;
-
-	default:
-		return -EINVAL;
-		break;
-	}
-	return 0;
-}
-
-/*
- * Called after devices are re-setup, but before processes are thawed.
- */
-static int pnx4008_pm_finish(suspend_state_t state)
-{
-	return 0;
-}
-
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops pnx4008_pm_ops = {
-	.prepare = pnx4008_pm_prepare,
 	.enter = pnx4008_pm_enter,
-	.finish = pnx4008_pm_finish,
+	.valid = pm_valid_only_mem,
 };
 
 static int __init pnx4008_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 11:55:22.983214909 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 11:56:34.183214909 +0100
@@ -227,6 +227,7 @@ static struct pm_ops pxa_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init pxa_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 11:55:23.013214909 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 11:56:34.183214909 +0100
@@ -133,6 +133,7 @@ unsigned long sleep_phys_sp(void *sp)
 
 static struct pm_ops sa11x0_pm_ops = {
 	.enter		= sa11x0_pm_enter,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init sa11x0_pm_init(void)
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 11:55:23.143214909 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 11:56:34.223214909 +0100
@@ -612,26 +612,9 @@ static int s3c2410_pm_enter(suspend_stat
 	return 0;
 }
 
-/*
- * Called after processes are frozen, but before we shut down devices.
- */
-static int s3c2410_pm_prepare(suspend_state_t state)
-{
-	return 0;
-}
-
-/*
- * Called after devices are re-setup, but before processes are thawed.
- */
-static int s3c2410_pm_finish(suspend_state_t state)
-{
-	return 0;
-}
-
 static struct pm_ops s3c2410_pm_ops = {
-	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
-	.finish		= s3c2410_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 /* s3c2410_pm_init
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 11:55:23.253214909 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 11:56:34.223214909 +0100
@@ -69,6 +69,7 @@ static int hp6x0_pm_enter(suspend_state_
 
 static struct pm_ops hp6x0_pm_ops = {
 	.enter		= hp6x0_pm_enter,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init hp6x0_pm_init(void)
--- linux-2.6.orig/drivers/acpi/sleep/main.c	2007-03-20 11:55:23.293214909 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 11:56:34.223214909 +0100
@@ -168,9 +168,18 @@ int acpi_suspend(u32 acpi_state)
 
 static int acpi_pm_state_valid(suspend_state_t pm_state)
 {
-	u32 acpi_state = acpi_suspend_states[pm_state];
+	u32 acpi_state;
 
-	return sleep_states[acpi_state];
+	switch (pm_state) {
+	case PM_SUSPEND_ON:
+	case PM_SUSPEND_STANDBY:
+	case PM_SUSPEND_MEM:
+		acpi_state = acpi_suspend_states[pm_state];
+
+		return sleep_states[acpi_state];
+	default:
+		return 0;
+	}
 }
 
 static struct pm_ops acpi_pm_ops = {
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 11:55:23.403214909 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 11:56:34.233214909 +0100
@@ -48,6 +48,19 @@ void pm_set_ops(struct pm_ops * ops)
 	mutex_unlock(&pm_mutex);
 }
 
+/**
+ * pm_valid_only_mem - generic memory-only valid callback
+ *
+ * pm_ops drivers that implement mem suspend only and only need
+ * to check for that in their .valid callback can use this instead
+ * of rolling their own .valid callback.
+ */
+int pm_valid_only_mem(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
+
 static inline void pm_finish(suspend_state_t state)
 {
 	if (pm_ops->finish)

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> Almost all users of pm_ops only support mem sleep, don't check in .valid
> and don't reject any others in .prepare so users can be confused if they
> check /sys/power/state, especially when new states are ever added.
> 
> This patch implements a generic pm_valid_only_mem function that is then
> exported for users as well as using it in almost all pm_ops.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

ACK. (You may want to send it to akpm).
									Pavel

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Tuesday 20 March 2007 4:06 am, Johannes Berg wrote:
> Almost all users of pm_ops only support mem sleep, don't check in .valid
> and don't reject any others in .prepare so users can be confused if they
> check /sys/power/state, especially when new states are ever added.

By the way ... as a note to implementors, it should be trivial to
implement a basic "standby" state that suspends drivers, disables
many clocks, and probably puts DRAM into self-refresh mode, but
uses only the wait-for-interrupt CPU lowpower mode.

A key difference between that and STR would then be that STR does
extra magic, like switching the CPU to a slow clock and then turning
off all the clocks that drive the chip "fast".  Also, that because
it disables so many clocks, the SOC probably can't support as many
types of wakeup events in STR.


I mention this because implementing such a "standby" mode means
that all the platform drivers can start to make their suspend()
and resume() code behave, and userspace tools can be put into
place, before all that tricky/painful STR work gets done.  Also,
because driver wakeup events in such a "standby" mode tend to be
a lot more powerful ... pretty much how a driver using runtime
PM models would work (instead of user-visible "goto sleep").

That is, support for such a "standby" mode is the easiest way
to seed all the PM work on a given platform.  Even though it
doesn't save as much power, it's a useful step towards deeper
power saving modes.

And thus endeth the lesson for today.  I look forward to seeing
a lot more platforms supporting at least minimal PM, now.  ;)

- Dave



 

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> > Almost all users of pm_ops only support mem sleep, don't check in .valid
> > and don't reject any others in .prepare so users can be confused if they
> > check /sys/power/state, especially when new states are ever added.
> 
> By the way ... as a note to implementors, it should be trivial to
> implement a basic "standby" state that suspends drivers, disables
> many clocks, and probably puts DRAM into self-refresh mode, but
> uses only the wait-for-interrupt CPU lowpower mode.
> 
> A key difference between that and STR would then be that STR does
> extra magic, like switching the CPU to a slow clock and then turning
> off all the clocks that drive the chip "fast".  Also, that because
> it disables so many clocks, the SOC probably can't support as many
> types of wakeup events in STR.
> 
> I mention this because implementing such a "standby" mode means
> that all the platform drivers can start to make their suspend()
> and resume() code behave, and userspace tools can be put into
> place, before all that tricky/painful STR work gets done.  Also,
> because driver wakeup events in such a "standby" mode tend to be
> a lot more powerful ... pretty much how a driver using runtime
> PM models would work (instead of user-visible "goto sleep").

Actually, I second that. "standby" that spins down disks to protect
them for transport is very useful feature, even if it does not save
much power. (Okay, that's notebooks and high-end-zauruses with
spinning disks, but...)
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Tue, 20 Mar 2007, David Brownell wrote:

> On Tuesday 20 March 2007 4:06 am, Johannes Berg wrote:
> > Almost all users of pm_ops only support mem sleep, don't check in .valid
> > and don't reject any others in .prepare so users can be confused if they
> > check /sys/power/state, especially when new states are ever added.
> 
> By the way ... as a note to implementors, it should be trivial to
> implement a basic "standby" state that suspends drivers, disables
> many clocks, and probably puts DRAM into self-refresh mode, but
> uses only the wait-for-interrupt CPU lowpower mode.
> 
> A key difference between that and STR would then be that STR does
> extra magic, like switching the CPU to a slow clock and then turning
> off all the clocks that drive the chip "fast".  Also, that because
> it disables so many clocks, the SOC probably can't support as many
> types of wakeup events in STR.

Hm, interesting. What you described above is very similar to what I've 
just implemented for a 8241 based system (linkstation: 
http://ozlabs.org/pipermail/linuxppc-dev/2007-March/thread.html#33203).
But Paul Mackerras suggested to consider it a StR, whereas Johannes Berg 
proposed to call it a standby, which is also what seems to be more logical 
to me. May we agree on some "simple" criteria, like "CPU power on, i.e., 
CPU registers preserved"? If yes - standby, CPU off, registers lost - StR? 
I can imagine CPUs with multiple power sources allowing to switch some of 
them on and off respectively losing / keeping some register sets...

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Wednesday 21 March 2007 2:01 pm, Guennadi Liakhovetski wrote:
> On Tue, 20 Mar 2007, David Brownell wrote:
> > 
> > By the way ... as a note to implementors, it should be trivial to
> > implement a basic "standby" state that suspends drivers, disables
> > many clocks, and probably puts DRAM into self-refresh mode, but
> > uses only the wait-for-interrupt CPU lowpower mode.
> > 
> > A key difference between that and STR would then be that STR does
> > extra magic, like switching the CPU to a slow clock and then turning
> > off all the clocks that drive the chip "fast".  Also, that because
> > it disables so many clocks, the SOC probably can't support as many
> > types of wakeup events in STR.
> 
> Hm, interesting. What you described above is very similar to what I've 
> just implemented for a 8241 based system (linkstation: 
> http://ozlabs.org/pipermail/linuxppc-dev/2007-March/thread.html#33203).

Just the other day.  :)


> But Paul Mackerras suggested to consider it a StR, whereas Johannes Berg 
> proposed to call it a standby, which is also what seems to be more logical 
> to me. 

Seems more like a "standby" to me too -- at least by comparison to
APM and ACPI definitions of "standby", vs what STR involves.


> May we agree on some "simple" criteria, like "CPU power on, i.e.,  
> CPU registers preserved"? If yes - standby, CPU off, registers lost - StR? 

The ACPI spec has some verbiage on those things, which uses
roughly that distinction.

Which is very much an indication of how weak ACPI is.  It
doesn't contemplate typical SOC behavior, which have a wide
variety of system sleep states that leave the CPU on ... and
which may not even *have* (or need!) a "cpu off" state.

My own definition would be more like:  the minimal RAM-based
power-saving system state is "standby".  If the system
implements a deeper RAM-based system sleep state, that's "STR".

If Linux eventually allows more system sleep states than
just "standby", "mem", and "disk", then most of the new
states will probably fit between "standby" (ACPI S1) and
"mem" (ACPI S3).


> I can imagine CPUs with multiple power sources allowing to switch some of 
> them on and off respectively losing / keeping some register sets...

I can see that more readily with SOC designs that have multiple
power domains.  Consider:  CPU, Peripherals-1, Peripherals-2.
That implies several sleep states:

  - all domains powered
  - only peripherals-1 off
  - only peripherals-2 off
  - both peripheral domains off
  - ...
  - all three domains off

Now, I'm not sure that it would be useful to expose all those
states to userspace, but surely an implementor might find it
useful to implement more than one.  In which case, one would
be called "standby", the next "STR", and ... well, Linux PM
can't handle anything else yet.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Wed, 21 Mar 2007, David Brownell wrote:

> On Wednesday 21 March 2007 2:01 pm, Guennadi Liakhovetski wrote:
> 
> > May we agree on some "simple" criteria, like "CPU power on, i.e.,  
> > CPU registers preserved"? If yes - standby, CPU off, registers lost - StR? 
> 
> The ACPI spec has some verbiage on those things, which uses
> roughly that distinction.
> 
> Which is very much an indication of how weak ACPI is.  It
> doesn't contemplate typical SOC behavior, which have a wide
> variety of system sleep states that leave the CPU on ... and
> which may not even *have* (or need!) a "cpu off" state.
> 
> My own definition would be more like:  the minimal RAM-based
> power-saving system state is "standby".  If the system
> implements a deeper RAM-based system sleep state, that's "STR".

Hmmm, this leaves the decision how to call each state COMPLETELY to the 
implementor, doesn't it? For example, in my suspend above, I do put the 
peripheral controller to sleep too, whereas a "minimal RAM-based" suspend 
could leave it on. So, does it mean I already qualify for StR?:-) I am 
sure you have more experience with power management and a better 
understanding of it too, maybe that's the reason why your definition makes 
a good sense to you and not to me. But "suspend to RAM" - I think it would 
be logical to interpret it as "only system RAM contents is preserved. All 
other volatile storage (CPU registers, peripheral internal registers and 
RAM, etc.) contents is lost."

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> > Which is very much an indication of how weak ACPI is.  It
> > doesn't contemplate typical SOC behavior, which have a wide
> > variety of system sleep states that leave the CPU on ... and
> > which may not even *have* (or need!) a "cpu off" state.
> > 
> > My own definition would be more like:  the minimal RAM-based
> > power-saving system state is "standby".  If the system
> > implements a deeper RAM-based system sleep state, that's "STR".
> 
> Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> implementor, doesn't it?

Is that a problem? If someone is clever enough to implement suspend, I
think we can trust them to name their states right.

(And trust me, we can flame them if not).

(Anyway, my definition would be "mem" == RAM is powered, everything
else is down, except for devices needed for wakeup; "standby" ==
something is powered that can be powered down, we'll fix that in next version).
								Pavel

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

[ removed crosspost to linux-arm, members-only ]

On Wednesday 21 March 2007 3:57 pm, Pavel Machek wrote:
> Hi!
> 
> > > Which is very much an indication of how weak ACPI is.  It
> > > doesn't contemplate typical SOC behavior, which have a wide
> > > variety of system sleep states that leave the CPU on ... and
> > > which may not even *have* (or need!) a "cpu off" state.
> > > 
> > > My own definition would be more like:  the minimal RAM-based
> > > power-saving system state is "standby".  If the system
> > > implements a deeper RAM-based system sleep state, that's "STR".
> > 
> > Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> > implementor, doesn't it?

Not really.  Standby is not as deep a sleep state as STR, so it
would be wrong to have it save more power than STR.

And remember that the implementor must make various decisions in
any case, since the SOC probably has half a dozen distinct
low power states, but Linux can only name two of them.


> Is that a problem? If someone is clever enough to implement suspend, I
> think we can trust them to name their states right.

Modulo my earlier comment, showing that you **don't** need to be
especially clever to implement a "standby" on most systems ... !

 
> (And trust me, we can flame them if not).
> 
> (Anyway, my definition would be "mem" == RAM is powered, everything
> else is down, except for devices needed for wakeup; "standby" ==
> something is powered that can be powered down, we'll fix that in next version).

That implies that standby is less desirable, and wouldn't be used much.

That's a false implication.  Among other things, it may be more important
to have various wakeup event sources at moderate power, than to go
without them at lower power.  Simple math:  N hours at 75% power savings,
which lets the system become fully operational at any time; versus those
same hours at 0% power savings (full power), because STR (90% savings)
doesn't support some essential wakeup events.


Moreover, that assumes "powered" is the relevant issue, rather than for
example "clocked".  As in:  power is always applied everywhere, registers
are always preserved, but more clocks are gated off in deeper sleep states.

That "CPU and peripheral state is discarded" notion is NOT generally
applicable, so that it shouldn't be hard-coded into a definition of
what distinguishes the states given the "standby" and "mem" names.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> > (And trust me, we can flame them if not).
> > 
> > (Anyway, my definition would be "mem" == RAM is powered, everything
> > else is down, except for devices needed for wakeup; "standby" ==
> > something is powered that can be powered down, we'll fix that in next version).
> 
> That implies that standby is less desirable, and wouldn't be used much.
> 
> That's a false implication.  Among other things, it may be more important
> to have various wakeup event sources at moderate power, than to go
> without them at lower power.  Simple math:  N hours at 75% power savings,
> which lets the system become fully operational at any time; versus those
> same hours at 0% power savings (full power), because STR (90% savings)
> doesn't support some essential wakeup events.

I guess we are violently agreeing. See my "except for devices needed
for wakeup". I should not have added the "next version" note, as
"standby" is useful, too.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

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

On Wed, 2007-03-21 at 16:25 -0700, David Brownell wrote:
> [ removed crosspost to linux-arm, members-only ]

My mistake, sorry about that.

> And remember that the implementor must make various decisions in
> any case, since the SOC probably has half a dozen distinct
> low power states, but Linux can only name two of them.

Actually, now that we have everybody using .valid we can add an
arbitrary amount of them if we wish :>

johannes

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

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 3:03 am, Johannes Berg wrote:

> > And remember that the implementor must make various decisions in
> > any case, since the SOC probably has half a dozen distinct
> > low power states, but Linux can only name two of them.
> 
> Actually, now that we have everybody using .valid

Seems kind of temporary, so long as pm_set_ops() doesn't
enforce that as a requirement ...


> we can add an 
> arbitrary amount of them if we wish :>

For as many as can be #defined, yes.  But such #defines will
be rare; adding a new one would mean updating pm_states[]
plus maybe other code, and there are other structural issues
with that notion ... especially the way suspend() methods
have no way to determine the semantics of the target state.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

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

On Thu, 2007-03-22 at 10:10 -0700, David Brownell wrote:

> Seems kind of temporary, so long as pm_set_ops() doesn't
> enforce that as a requirement ...

I just sent a patch to akpm to do exactly that :)

> > we can add an 
> > arbitrary amount of them if we wish :>
> 
> For as many as can be #defined, yes.  But such #defines will
> be rare; adding a new one would mean updating pm_states[]
> plus maybe other code, and there are other structural issues
> with that notion ... 

True. I think pm_states is the only thing that would need to be changed,
but I don't really advocate changing it and introducing dozens of new
states.

> especially the way suspend() methods
> have no way to determine the semantics of the target state.

I can't parse that.

johannes

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

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 10:18 am, Johannes Berg wrote:
> On Thu, 2007-03-22 at 10:10 -0700, David Brownell wrote:
> 
> > especially the way suspend() methods
> > have no way to determine the semantics of the target state.
> 
> I can't parse that.

Look at docs for most any SOC processor and you'll see that
not only does it have a variety of lowpower states, but that
each of them has various rules about what works there and
what doesn't.

So long as driver suspend() methods have no some way to see
how far they "must" shut down for a given target sleep state,
there is only limited utility to defining such states.

That's because the state would only really apply to the cpu
specific code ... drivers must assume the worst case, precluding
the primary reasons to use those less-deep system states.

I just reposted my clk_must_disable() patches; I think it's time
to merge them.  They address that issue for one of the most
common type of rule for system sleep states ... at least, in
terms of interface.  Platform support will still be an issue,
but at least the issue won't be "crippled by bad design".

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

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

On Thu, 2007-03-22 at 11:13 -0700, David Brownell wrote:

> Look at docs for most any SOC processor and you'll see that
> not only does it have a variety of lowpower states, but that
> each of them has various rules about what works there and
> what doesn't.
> 
> So long as driver suspend() methods have no some way to see
> how far they "must" shut down for a given target sleep state,
> there is only limited utility to defining such states.

Oh ok, yeah, I see.

> That's because the state would only really apply to the cpu
> specific code ... drivers must assume the worst case, precluding
> the primary reasons to use those less-deep system states.

Right. That's a thing we'd have to fix in the interface to drivers to
tell them which state we're entering. But that can also get hairy since
the same device driver might support devices that are built into
different systems and might support different things there by being
hooked up differently....

> I just reposted my clk_must_disable() patches; I think it's time
> to merge them.  They address that issue for one of the most
> common type of rule for system sleep states ... at least, in
> terms of interface.  Platform support will still be an issue,
> but at least the issue won't be "crippled by bad design".

I can't speak for any of that, I haven't ever worked with PM on anything
not looking like a big desktop or at least laptop.

In any case, I didn't intend to fix up all that or even make it
possible, so far all I wanted was to clean up the mess surrounding
pm_ops even as it is currently defined (though it wasn't really
well-documented which probably led to the mess, I had to read the code
to understand it, I had no part in writing it.)

johannes

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

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

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Wednesday, 21 March 2007 23:57, Pavel Machek wrote:
> Hi!
> 
> > > Which is very much an indication of how weak ACPI is.  It
> > > doesn't contemplate typical SOC behavior, which have a wide
> > > variety of system sleep states that leave the CPU on ... and
> > > which may not even *have* (or need!) a "cpu off" state.
> > > 
> > > My own definition would be more like:  the minimal RAM-based
> > > power-saving system state is "standby".  If the system
> > > implements a deeper RAM-based system sleep state, that's "STR".
> > 
> > Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> > implementor, doesn't it?
> 
> Is that a problem? If someone is clever enough to implement suspend, I
> think we can trust them to name their states right.
> 
> (And trust me, we can flame them if not).
> 
> (Anyway, my definition would be "mem" == RAM is powered, everything
> else is down, except for devices needed for wakeup; "standby" ==
> something is powered that can be powered down, we'll fix that in next version).

I think we can define "standby" a bit more precisely.  Something like:
- processes are frozen,
- devices are suspended,
- nonboot CPUs are down (and in low powered states, if possible),
- "system" devices may or may not be suspended, depending on the platform,
- the boot CPU may or may not be in a low power state, depending on the platform,
- RAM is powered
- wake up need not be BIOS-driven (main difference from "mem")

Greetings,
Rafael

Re: [PATCH] add firmware disk state and clean up

[David Brownell]

On Tuesday 20 March 2007 3:17 am, Johannes Berg wrote:

> --- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 10:55:35.433214909 +0100
> +++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 10:56:02.243214909 +0100
> @@ -199,11 +199,16 @@ error:
>  	return 0;
>  }
>  
> +static int at91_pm_valid(suspend_state_t state)
> +{
> +	return state == PM_SUSPEND_MEM;
> +}
>  
>  static struct pm_ops at91_pm_ops ={
>  	.valid		= at91_pm_valid_state,
>  	.prepare	= at91_pm_prepare,
>  	.enter		= at91_pm_enter,
> +	.valid		= at91_pm_valid,
>  };
>  
>  static int __init at91_pm_init(void)

Clearly bogus.  The STR support isn't upstream yet, true, but
you shouldn't modify that code at all.

- Dave

Re: [PATCH] add firmware disk state and clean up

[Pavel Machek]

Hi!

> > +static int at91_pm_valid(suspend_state_t state)
> > +{
> > +	return state == PM_SUSPEND_MEM;
> > +}
> >  
> >  static struct pm_ops at91_pm_ops ={
> >  	.valid		= at91_pm_valid_state,
> >  	.prepare	= at91_pm_prepare,
> >  	.enter		= at91_pm_enter,
> > +	.valid		= at91_pm_valid,
> >  };
> >  
> >  static int __init at91_pm_init(void)
> 
> Clearly bogus.  The STR support isn't upstream yet, true, but

No... look again. This tries to disable suspend-to-disk on ARM, so
that only suspend-to-RAM is enabled. I'd not call it bogus.

The series is needed to stop pm from reporting "platform" on ARM;
"platform" only makes sense on ACPI systems.

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

Re: [PATCH] add firmware disk state and clean up

[David Brownell]

On Tuesday 20 March 2007 3:09 pm, Pavel Machek wrote:
> Hi!
> 
> > > +static int at91_pm_valid(suspend_state_t state)
> > > +{
> > > +	return state == PM_SUSPEND_MEM;
> > > +}
> > >  
> > >  static struct pm_ops at91_pm_ops ={
> > >  	.valid		= at91_pm_valid_state,
         ^^^^^

> > >  	.prepare	= at91_pm_prepare,
> > >  	.enter		= at91_pm_enter,
> > > +	.valid		= at91_pm_valid,
         ^^^^^

> > >  };
> > >  
> > >  static int __init at91_pm_init(void)
> > 
> > Clearly bogus.  The STR support isn't upstream yet, true, but
> 
> No... look again. 

No, YOU look again.  If that compiles, it's a GCC bug.


> This tries to disable suspend-to-disk on ARM, so 
> that only suspend-to-RAM is enabled. I'd not call it bogus.
> 
> The series is needed to stop pm from reporting "platform" on ARM;
> "platform" only makes sense on ACPI systems.
> 
> 								Pavel

Re: [PATCH] rework pm_ops pm_disk_modes foo

[Johannes Berg]

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

I'll repost these three patches for inclusion once I've gotten some more
feedback, ok?

The three patches currently are:
 * rework pm_ops pm_disk_modes foo
   The pm_ops.pm_disk_mode is used in totally bogus ways, this fixes it,
   clarifies the description and kills all the users that wrongly think
   they can veto suspend to disk that way.
 * remove firmware disk mode
 * implement pm_ops.valid for everybody

After that I can go back to actually making it work on powermac...

johannes

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

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

Re: [PATCH] implement pm_ops.valid for everybody

[Scott E. Preece]

| From: "Rafael J. Wysocki" <rjw@sisk.pl>
| 
| On Wednesday, 21 March 2007 23:57, Pavel Machek wrote:
| > Hi!
| > 
| > > > Which is very much an indication of how weak ACPI is.  It
| > > > doesn't contemplate typical SOC behavior, which have a wide
| > > > variety of system sleep states that leave the CPU on ... and
| > > > which may not even *have* (or need!) a "cpu off" state.
| > > > 
| > > > My own definition would be more like:  the minimal RAM-based
| > > > power-saving system state is "standby".  If the system
| > > > implements a deeper RAM-based system sleep state, that's "STR".
| > > 
| > > Hmmm, this leaves the decision how to call each state COMPLETELY to the 
| > > implementor, doesn't it?
| > 
| > Is that a problem? If someone is clever enough to implement suspend, I
| > think we can trust them to name their states right.
| > 
| > (And trust me, we can flame them if not).
| > 
| > (Anyway, my definition would be "mem" == RAM is powered, everything
| > else is down, except for devices needed for wakeup; "standby" ==
| > something is powered that can be powered down, we'll fix that in next version).
| 
| I think we can define "standby" a bit more precisely.  Something like:
| - processes are frozen,
| - devices are suspended,
| - nonboot CPUs are down (and in low powered states, if possible),
| - "system" devices may or may not be suspended, depending on the platform,
| - the boot CPU may or may not be in a low power state, depending on the platform,
| - RAM is powered
| - wake up need not be BIOS-driven (main difference from "mem")
---

I would be tempted to say that that last bullet is the distinguishing
characteristic - that you come back from standby by just continuing
where you left off, but you come back from StR by something akin to
booting.

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 6:44 am, Scott E. Preece wrote:
> | From: "Rafael J. Wysocki" <rjw@sisk.pl>
> | 
> | I think we can define "standby" a bit more precisely.  Something like:
> | - processes are frozen,
> | - devices are suspended,

True of all sleep states, although one wants "suspended" to
allow different levels of device functionality.  (Maybe it
can wake up, maybe its firmware is monitoring the WLAN, etc.)


> | - nonboot CPUs are down (and in low powered states, if possible),
> | - the boot CPU may or may not be in a low power state, depending on the platform,

Both seem platform-specific.  One might want the DSP to be fully
active while the control CPU is in a lowpower state, etc.


> | - "system" devices may or may not be suspended, depending on the platform,

... so this "restriction" is meaningless ...


> | - RAM is powered

I think that's assumed by all states except suspend-to-disk.  Modulo
the potential for powering off the unused banks, of course.


> | - wake up need not be BIOS-driven (main difference from "mem")
> ---
> 
> I would be tempted to say that that last bullet is the distinguishing
> characteristic - that you come back from standby by just continuing
> where you left off, but you come back from StR by something akin to
> booting.

I was going to say that even thinking about a "BIOS" is an x86-ism,
so this would be an unusably non-generic rule.  :)

SOC systems often need to drive their deepest sleep states from code
living in on-chip SRAM (maybe 16 KB).  Sometimes that's also done in
conjunction with code from an on-chip mask ROM, especially if the CPU
itself gets powered off.  That's not BIOS...


... but I guess I don't see why one would want to try to nail down
a definition of either "standby" or "STR".  The implementation will
necessarily be highly platform-specific, to the extent that almost
any rule will need to be broken somewhere.  So, why bother trying
to fit every system into the mold of APM/ACPI terminology?

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Tony Lindgren]

Hi,

* David Brownell <david-b@pacbell.net> [070322 14:30]:
> On Thursday 22 March 2007 6:44 am, Scott E. Preece wrote:
> > | From: "Rafael J. Wysocki" <rjw@sisk.pl>
> > | 
> > | I think we can define "standby" a bit more precisely.  Something like:
> > | - processes are frozen,
> > | - devices are suspended,
> 
> True of all sleep states, although one wants "suspended" to
> allow different levels of device functionality.  (Maybe it
> can wake up, maybe its firmware is monitoring the WLAN, etc.)

In addition to offering wakeup events for individual devices,
the device suspend states should be something like retention
and suspend, where:

Retention is where clocks are off for a device, but power is on.
In this case the device registers are maintained in hardware.

Suspend is where clocks and power are off. In this state the
device registers are maintained in software.

Laptops mostly have suspend, while socs allow both retention
and suspend in many cases.

Regards,

Tony

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 12:26 pm, Tony Lindgren wrote:
> 
> In addition to offering wakeup events for individual devices,
> the device suspend states should be something like retention
> and suspend, where:

Maybe ... worth discussing.  Most PCI drivers don't make
that distinction, although they could (see below).

What they do instead is assume the lowest device power state
("suspend"), and re-initialize in resume().  If Linux starts to
support standby and STR modes properly ... then it'd make sense
to teach more PCI drivers to try using "retention" states.  But
those drivers would still need to be prepared to re-init.


> Retention is where clocks are off for a device, but power is on.
> In this case the device registers are maintained in hardware.

Analagous to PCI D1 or D2.

> Suspend is where clocks and power are off. In this state the
> device registers are maintained in software.

Analagous to PCI D3, especially D3cold ... although PCI D3
certainly allows the Vaux "power well" to power some parts
of the device, so that not all register values get reset.


> Laptops mostly have suspend, while socs allow both retention
> and suspend in many cases.

Not quite true, as noted above.  There are differences in how
things are factored, but those mechanisms exist in both x86
and SOC worlds.  One key difference from a Linux perspective
is probably that without ACPI in the way, a SOC design can
make much better use of the hardware PM capabilities.

Very few non-USB drivers address "retention" modes on laptops;
USB host controller drivers need it to handle "remote wakeup",
which one expects to work from "standby" and suspend-to-RAM.
(Plus potentialy suspend-to-disk, but that's uncommon.)

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[tony]

* David Brownell <david-b@pacbell.net> [070322 17:29]:
> On Thursday 22 March 2007 12:26 pm, Tony Lindgren wrote:
> > 
> > In addition to offering wakeup events for individual devices,
> > the device suspend states should be something like retention
> > and suspend, where:
> 
> Maybe ... worth discussing.  Most PCI drivers don't make
> that distinction, although they could (see below).
> 
> What they do instead is assume the lowest device power state
> ("suspend"), and re-initialize in resume().  If Linux starts to
> support standby and STR modes properly ... then it'd make sense
> to teach more PCI drivers to try using "retention" states.  But
> those drivers would still need to be prepared to re-init.

I agree, in general we should start taking advantage of the
device power states. 
 
> > Retention is where clocks are off for a device, but power is on.
> > In this case the device registers are maintained in hardware.
> 
> Analagous to PCI D1 or D2.

Hmmm, I think with PCI it's just numbering where the power
consumption decreases as the nuber increases except for D3hot
and D3cold.
 
> > Suspend is where clocks and power are off. In this state the
> > device registers are maintained in software.
> 
> Analagous to PCI D3, especially D3cold ... although PCI D3
> certainly allows the Vaux "power well" to power some parts
> of the device, so that not all register values get reset.

Maybe actually D3hot = retention and D3cold = suspend? 

PCI	SOCs		CLOCKS	POWER
D3hot	retention	off	on
D3cold	suspend		off	off
 
> > Laptops mostly have suspend, while socs allow both retention
> > and suspend in many cases.
> 
> Not quite true, as noted above.  There are differences in how
> things are factored, but those mechanisms exist in both x86
> and SOC worlds.  One key difference from a Linux perspective
> is probably that without ACPI in the way, a SOC design can
> make much better use of the hardware PM capabilities.
> 
> Very few non-USB drivers address "retention" modes on laptops;
> USB host controller drivers need it to handle "remote wakeup",
> which one expects to work from "standby" and suspend-to-RAM.
> (Plus potentialy suspend-to-disk, but that's uncommon.)

Yeah, OK.

Tony

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Friday 23 March 2007 6:15 am, tony@atomide.com wrote:
> * David Brownell <david-b@pacbell.net> [070322 17:29]:
> > On Thursday 22 March 2007 12:26 pm, Tony Lindgren wrote:

> > > Retention is where clocks are off for a device, but power is on.
> > > In this case the device registers are maintained in hardware.
> > 
> > Analagous to PCI D1 or D2.
> 
> Hmmm, I think with PCI it's just numbering where the power
> consumption decreases as the nuber increases except for D3hot
> and D3cold.

There are additional constraints ... as the number increases,
more device state can be discarded (significant!), and there
are longer latencies to return to D0 (almost noise).


> > > Suspend is where clocks and power are off. In this state the
> > > device registers are maintained in software.
> > 
> > Analagous to PCI D3, especially D3cold ... although PCI D3
> > certainly allows the Vaux "power well" to power some parts
> > of the device, so that not all register values get reset.
> 
> Maybe actually D3hot = retention and D3cold = suspend? 
> 
> PCI       SOCs        CLOCKS	POWER
> D3hot     retention   off	on
> D3cold    suspend     off	off

That's why I said "especially", but there's other funkiness
beyond the fact that the PCI spec leaves out "clocks" and
other such implementation details.

In particular, ISTR transition D3hot->D0 can optionally add
some level of device reset, so it's not quite as direct a
mapping as D3cold.  Plus there's Vaux letting PCI devices
live in two overlapping power domains.

- Dave


> > > Laptops mostly have suspend, while socs allow both retention
> > > and suspend in many cases.
> > 
> > Not quite true, as noted above.  There are differences in how
> > things are factored, but those mechanisms exist in both x86
> > and SOC worlds.  One key difference from a Linux perspective
> > is probably that without ACPI in the way, a SOC design can
> > make much better use of the hardware PM capabilities.
> > 
> > Very few non-USB drivers address "retention" modes on laptops;
> > USB host controller drivers need it to handle "remote wakeup",
> > which one expects to work from "standby" and suspend-to-RAM.
> > (Plus potentialy suspend-to-disk, but that's uncommon.)
> 
> Yeah, OK.
> 
> Tony
> 

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Thursday, 22 March 2007 20:26, Tony Lindgren wrote:
> Hi,
> 
> * David Brownell <david-b@pacbell.net> [070322 14:30]:
> > On Thursday 22 March 2007 6:44 am, Scott E. Preece wrote:
> > > | From: "Rafael J. Wysocki" <rjw@sisk.pl>
> > > | 
> > > | I think we can define "standby" a bit more precisely.  Something like:
> > > | - processes are frozen,
> > > | - devices are suspended,
> > 
> > True of all sleep states, although one wants "suspended" to
> > allow different levels of device functionality.  (Maybe it
> > can wake up, maybe its firmware is monitoring the WLAN, etc.)
> 
> In addition to offering wakeup events for individual devices,
> the device suspend states should be something like retention
> and suspend, where:
> 
> Retention is where clocks are off for a device, but power is on.
> In this case the device registers are maintained in hardware.
> 
> Suspend is where clocks and power are off. In this state the
> device registers are maintained in software.
> 
> Laptops mostly have suspend, while socs allow both retention
> and suspend in many cases.

I think there can be more low-power states that just "standby",
"STR", "STD", etc., but for example "standby" should be at least comparable
between different platforms.

Greetings,
Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Thursday, 22 March 2007 19:29, David Brownell wrote:
> On Thursday 22 March 2007 6:44 am, Scott E. Preece wrote:
> > | From: "Rafael J. Wysocki" <rjw@sisk.pl>
> > | 
> > | I think we can define "standby" a bit more precisely.  Something like:
> > | - processes are frozen,
> > | - devices are suspended,
> 
> True of all sleep states, although one wants "suspended" to
> allow different levels of device functionality.  (Maybe it
> can wake up, maybe its firmware is monitoring the WLAN, etc.)
> 
> 
> > | - nonboot CPUs are down (and in low powered states, if possible),
> > | - the boot CPU may or may not be in a low power state, depending on the platform,
> 
> Both seem platform-specific.  One might want the DSP to be fully
> active while the control CPU is in a lowpower state, etc.
> 
> 
> > | - "system" devices may or may not be suspended, depending on the platform,
> 
> ... so this "restriction" is meaningless ...
> 
> 
> > | - RAM is powered
> 
> I think that's assumed by all states except suspend-to-disk.  Modulo
> the potential for powering off the unused banks, of course.
> 
> 
> > | - wake up need not be BIOS-driven (main difference from "mem")
> > ---
> > 
> > I would be tempted to say that that last bullet is the distinguishing
> > characteristic - that you come back from standby by just continuing
> > where you left off, but you come back from StR by something akin to
> > booting.
> 
> I was going to say that even thinking about a "BIOS" is an x86-ism,
> so this would be an unusably non-generic rule.  :)
> 
> SOC systems often need to drive their deepest sleep states from code
> living in on-chip SRAM (maybe 16 KB).  Sometimes that's also done in
> conjunction with code from an on-chip mask ROM, especially if the CPU
> itself gets powered off.  That's not BIOS...
> 
> 
> ... but I guess I don't see why one would want to try to nail down
> a definition of either "standby" or "STR".

So that the meaning of "standby" and "STR" is known, more or less.

If you say "I'd like platforms to implement standby", you should say what
you mean by "standby", IMHO.

> The implementation will necessarily be highly platform-specific, to the
> extent that almost any rule will need to be broken somewhere.  So, why
> bother trying to fit every system into the mold of APM/ACPI terminology?

Actually, I don't care what the definition will be.  Still, I'd like to have
a definition.

Greetings,
Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 2:27 pm, Rafael J. Wysocki wrote:
> On Thursday, 22 March 2007 19:29, David Brownell wrote:
> > 
> > ... but I guess I don't see why one would want to try to nail down
> > a definition of either "standby" or "STR".
> 
> So that the meaning of "standby" and "STR" is known, more or less.

But "more or less" != "nailed down (so tightly it's not always appicable)"


> If you say "I'd like platforms to implement standby", you should say what
> you mean by "standby", IMHO.

I thought my original note described that, as well as describing how
it differs from STR.

STR shuts down a lot more.  Not necessarily powering down the CPU
(which is what would cause the need for boot/BIOS code to have the
"this is really a resume" cases, and isn't always possible), but at
least being more agressive about powering down clocks and such.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Thu, 22 Mar 2007, David Brownell wrote:

> STR shuts down a lot more.  Not necessarily powering down the CPU
> (which is what would cause the need for boot/BIOS code to have the
> "this is really a resume" cases, and isn't always possible), but at
> least being more agressive about powering down clocks and such.

Dave, sorry, maybe I am being dense the whole time, but - "more" than 
what??? If you implement several modes, ok, you can compare them. But if 
you only implement one suspend mode for whatever reason - what are you 
comparing it to? 

Thanks
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 2:56 pm, Guennadi Liakhovetski wrote:
> On Thu, 22 Mar 2007, David Brownell wrote:
> 
> > STR shuts down a lot more.  Not necessarily powering down the CPU
> > (which is what would cause the need for boot/BIOS code to have the
> > "this is really a resume" cases, and isn't always possible), but at
> > least being more agressive about powering down clocks and such.
> 
> Dave, sorry, maybe I am being dense the whole time, but - "more" than 
> what??? If you implement several modes, ok, you can compare them. But if 
> you only implement one suspend mode for whatever reason - what are you 
> comparing it to? 

How about ... that canonical trivial "standby" mode I described,
where the CPU doesn't do much more than what a power-naive idle loop
might be doing?  Almost any system can do that much.

On ARM that'd be a wait-for-interrupt instruction; some other CPUs
may have analagous instructions that explicitly try to save some power,
which might not be good for an idle loop.  And some systems may have
power saving to wrap around that WFI; the idle loop might even use them.

Take it from first principles, and assume today's typical case where
drivers don't (can't!! sigh) do anything different in standby vs STR.
Then the *only* difference will be what pm_enter/pm_exit can do ...
and that's going to be primarly "what does the CPU do".

So if pm_enter is doing fancy stuff to the CPU, like re-clocking it
or powering it off ... that's more of an STR than a "standby".  As
Rafael put it, the resume then requires a "boot-like procedure".

Of course this is all points on a spectrum, and different systems
can work very differently.  Which is why I don't like the idea of
trying to place hard requirements on what STR or standby must be;
a platform could very easily support two useful power states, where
neither fits such "hard" labels.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Thursday, 22 March 2007 22:43, David Brownell wrote:
> On Thursday 22 March 2007 2:27 pm, Rafael J. Wysocki wrote:
> > On Thursday, 22 March 2007 19:29, David Brownell wrote:
> > > 
> > > ... but I guess I don't see why one would want to try to nail down
> > > a definition of either "standby" or "STR".
> > 
> > So that the meaning of "standby" and "STR" is known, more or less.
> 
> But "more or less" != "nailed down (so tightly it's not always appicable)"
> 
> 
> > If you say "I'd like platforms to implement standby", you should say what
> > you mean by "standby", IMHO.
> 
> I thought my original note described that, as well as describing how
> it differs from STR.
> 
> STR shuts down a lot more.  Not necessarily powering down the CPU
> (which is what would cause the need for boot/BIOS code to have the
> "this is really a resume" cases, and isn't always possible), but at
> least being more agressive about powering down clocks and such.

Well, I think the only clear distinction between the STR and "standby" is the
necessity to go through a boot-like procedure in order to resume from the
former.  So, I'd tend to think the STR is when the CPU(s) is(are) powered
down and if some platforms don't support that, they just don't support the
STR.

Also, I'd like to define "standby" and STR as system-wide low power states
which involve the freezing of processes before entering them.

Greetings,
Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 3:10 pm, Rafael J. Wysocki wrote:

> Well, I think the only clear distinction between the STR and "standby" is the
> necessity to go through a boot-like procedure in order to resume from the
> former.

So what's a "boot-like procedure"?   Ten instructions?  A hundred?
A thousand?  Ten thousand?  Does it take a certain amount of time?
Does it perform certain operations?  Does it involve going through
ACPI (or APM)?  If so, what about the fact that ACPI (or APM) are
involved in "standby" resumes too (on platforms using them)?

And why wouldn't a standby mode be able to do any or all of those?


> So, I'd tend to think the STR is when the CPU(s) is(are) powered 
> down and if some platforms don't support that, they just don't support the
> STR.

That seems like a counterproductive restriction.  The only reason to
adopt it is if you care so much about ACPI that you insist on using
their state definitions even on systems that will never use ACPI.

For a system that supports several power saving modes but doesn't
have the ability to turn the CPU off, what conceivable value would
there be in saying it's not OK to use the "STR" label for any of
those states?

And thus, to say that the system is only **allowed** to expose one
of those power saving modes to Linux ... and that it must always
be called "standby"?  Even if, from an external perspective, it
acts just like an STR would act?

My answer:  there is NO value to such an arbitrary restriction.


> Also, I'd like to define "standby" and STR as system-wide low power states
> which involve the freezing of processes before entering them.

That should go without saying, since "echo NAME > /sys/power/state"
does that regardless of NAME.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Thursday, 22 March 2007 23:56, David Brownell wrote:
> On Thursday 22 March 2007 3:10 pm, Rafael J. Wysocki wrote:
> 
> > Well, I think the only clear distinction between the STR and "standby" is the
> > necessity to go through a boot-like procedure in order to resume from the
> > former.
> 
> So what's a "boot-like procedure"?   Ten instructions?  A hundred?
> A thousand?  Ten thousand?  Does it take a certain amount of time?
> Does it perform certain operations?  Does it involve going through
> ACPI (or APM)?  If so, what about the fact that ACPI (or APM) are
> involved in "standby" resumes too (on platforms using them)?
> 
> And why wouldn't a standby mode be able to do any or all of those?
> 
> 
> > So, I'd tend to think the STR is when the CPU(s) is(are) powered 
> > down and if some platforms don't support that, they just don't support the
> > STR.
> 
> That seems like a counterproductive restriction.  The only reason to
> adopt it is if you care so much about ACPI that you insist on using
> their state definitions even on systems that will never use ACPI.
> 
> For a system that supports several power saving modes but doesn't
> have the ability to turn the CPU off, what conceivable value would
> there be in saying it's not OK to use the "STR" label for any of
> those states?
> 
> And thus, to say that the system is only **allowed** to expose one
> of those power saving modes to Linux ... and that it must always
> be called "standby"?  Even if, from an external perspective, it
> acts just like an STR would act?
> 
> My answer:  there is NO value to such an arbitrary restriction.

I'm not talking on restrictions.  I'm talking on being able to define
_anything_ more precisely then just a low-power system-wide state.

And let's start from just something, please.  Like STR and "stadndby" to begin
with?  At least on ACPI systems we can distinguish one from the other quite
clearly, so why can't we start from that and _then_ generalize?

Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:

> > My answer:  there is NO value to such an arbitrary restriction.
> 
> I'm not talking on restrictions.

You most certainly did talk about them.  You said that if the 
hardware doesn't support a "turn CPU off" mode, then you'd
define that as being incapable of implementing suspend-to-RAM.
That's a restriction ... a very arbitrary one.


> I'm talking on being able to define 
> _anything_ more precisely then just a low-power system-wide state.

Me too.  And I'm trying to convey to you the results of the
investigations I did on that topic.  You don't seem to like
those results though ...


> And let's start from just something, please.  Like STR and "standby" to begin
> with?  At least on ACPI systems we can distinguish one from the other quite
> clearly, so why can't we start from that and _then_ generalize?

That's exactly what I did.  Looked also at APM, and several
different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).

The generalization I came up with is what I've described.
Namely, that coming up with one definition of those states
that can usefully be mapped all platforms is impractical.
They're just labels.  The platform implementor can choose
two states to implement, but non-x86 hardware states rarely
match the expectations of ACPI.

So the fundamental definition needs to be in relative terms,
because platform-specific differences otherwise make trouble.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Matthew Locke]

On Mar 22, 2007, at 4:55 PM, David Brownell wrote:

> On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
>
>>> My answer:  there is NO value to such an arbitrary restriction.
>>
>> I'm not talking on restrictions.
>
> You most certainly did talk about them.  You said that if the
> hardware doesn't support a "turn CPU off" mode, then you'd
> define that as being incapable of implementing suspend-to-RAM.
> That's a restriction ... a very arbitrary one.
>
>
>> I'm talking on being able to define
>> _anything_ more precisely then just a low-power system-wide state.
>
> Me too.  And I'm trying to convey to you the results of the
> investigations I did on that topic.  You don't seem to like
> those results though ...
>
>
>> And let's start from just something, please.  Like STR and  
>> "standby" to begin
>> with?  At least on ACPI systems we can distinguish one from the  
>> other quite
>> clearly, so why can't we start from that and _then_ generalize?
>
> That's exactly what I did.  Looked also at APM, and several
> different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
>
> The generalization I came up with is what I've described.
> Namely, that coming up with one definition of those states
> that can usefully be mapped all platforms is impractical.
> They're just labels.  The platform implementor can choose
> two states to implement, but non-x86 hardware states rarely
> match the expectations of ACPI.
>
> So the fundamental definition needs to be in relative terms,
> because platform-specific differences otherwise make trouble.

The problem is that a 1:1 mapping between system low power state and  
a processor low power state is trying to be forced on every  
platform.  As Dave pointed out, embedded SoC's provide multiple low  
power states that qualify for the suspend-to-ram definition.  The  
only reasonable platform independent definition is that in STR memory  
is powered and contents preserved.  The rest is platform specific.

I think the right answer is that a mechanism for mapping platform  
specific states to the system states is needed. Platforms define  
their low power states and define the default for each system  
state .  On x86 platforms, the default just works and is probably  
never changed.  On embedded platforms, a policy manager can change  
the other low power states according to its latency and operational  
requirements.


>
> - Dave
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] implement pm_ops.valid for everybody

[tony]

* Matthew Locke <matt@nomadgs.com> [070322 21:15]:
> 
> On Mar 22, 2007, at 4:55 PM, David Brownell wrote:
> 
> > On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
> >
> >>> My answer:  there is NO value to such an arbitrary restriction.
> >>
> >> I'm not talking on restrictions.
> >
> > You most certainly did talk about them.  You said that if the
> > hardware doesn't support a "turn CPU off" mode, then you'd
> > define that as being incapable of implementing suspend-to-RAM.
> > That's a restriction ... a very arbitrary one.
> >
> >
> >> I'm talking on being able to define
> >> _anything_ more precisely then just a low-power system-wide state.
> >
> > Me too.  And I'm trying to convey to you the results of the
> > investigations I did on that topic.  You don't seem to like
> > those results though ...
> >
> >
> >> And let's start from just something, please.  Like STR and  
> >> "standby" to begin
> >> with?  At least on ACPI systems we can distinguish one from the  
> >> other quite
> >> clearly, so why can't we start from that and _then_ generalize?
> >
> > That's exactly what I did.  Looked also at APM, and several
> > different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
> >
> > The generalization I came up with is what I've described.
> > Namely, that coming up with one definition of those states
> > that can usefully be mapped all platforms is impractical.
> > They're just labels.  The platform implementor can choose
> > two states to implement, but non-x86 hardware states rarely
> > match the expectations of ACPI.
> >
> > So the fundamental definition needs to be in relative terms,
> > because platform-specific differences otherwise make trouble.
> 
> The problem is that a 1:1 mapping between system low power state and  
> a processor low power state is trying to be forced on every  
> platform.  As Dave pointed out, embedded SoC's provide multiple low  
> power states that qualify for the suspend-to-ram definition.  The  
> only reasonable platform independent definition is that in STR memory  
> is powered and contents preserved.  The rest is platform specific.
> 
> I think the right answer is that a mechanism for mapping platform  
> specific states to the system states is needed. Platforms define  
> their low power states and define the default for each system  
> state .  On x86 platforms, the default just works and is probably  
> never changed.  On embedded platforms, a policy manager can change  
> the other low power states according to its latency and operational  
> requirements.

Plus the states should be distributed. Trying to force the whole
system into certain state turns things messy.

Some devices may be active while some are in retention or suspend.

Basically everything should idle itself automatically whenever
possible based on a idle timer or some other policy, such as
suspending a device from user space via sysfs.

Regards,

Tony 

Re: [PATCH] implement pm_ops.valid for everybody

[Igor Stoppa]

On Fri, 2007-03-23 at 09:17 -0400, ext
linux-pm-bounces@lists.linux-foundation.org wrote:
> * Matthew Locke <matt@nomadgs.com> [070322 21:15]:
> > 
> > On Mar 22, 2007, at 4:55 PM, David Brownell wrote:
> > 
> > > On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
> > >
> > >>> My answer:  there is NO value to such an arbitrary restriction.
> > >>
> > >> I'm not talking on restrictions.
> > >
> > > You most certainly did talk about them.  You said that if the
> > > hardware doesn't support a "turn CPU off" mode, then you'd
> > > define that as being incapable of implementing suspend-to-RAM.
> > > That's a restriction ... a very arbitrary one.
> > >
> > >
> > >> I'm talking on being able to define
> > >> _anything_ more precisely then just a low-power system-wide state.
> > >
> > > Me too.  And I'm trying to convey to you the results of the
> > > investigations I did on that topic.  You don't seem to like
> > > those results though ...
> > >
> > >
> > >> And let's start from just something, please.  Like STR and  
> > >> "standby" to begin
> > >> with?  At least on ACPI systems we can distinguish one from the  
> > >> other quite
> > >> clearly, so why can't we start from that and _then_ generalize?
> > >
> > > That's exactly what I did.  Looked also at APM, and several
> > > different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
> > >
> > > The generalization I came up with is what I've described.
> > > Namely, that coming up with one definition of those states
> > > that can usefully be mapped all platforms is impractical.
> > > They're just labels.  The platform implementor can choose
> > > two states to implement, but non-x86 hardware states rarely
> > > match the expectations of ACPI.
> > >
> > > So the fundamental definition needs to be in relative terms,
> > > because platform-specific differences otherwise make trouble.
> > 
> > The problem is that a 1:1 mapping between system low power state and  
> > a processor low power state is trying to be forced on every  
> > platform.  As Dave pointed out, embedded SoC's provide multiple low  
> > power states that qualify for the suspend-to-ram definition.  The  
> > only reasonable platform independent definition is that in STR memory  
> > is powered and contents preserved.  The rest is platform specific.
> > 
> > I think the right answer is that a mechanism for mapping platform  
> > specific states to the system states is needed. Platforms define  
> > their low power states and define the default for each system  
> > state .  On x86 platforms, the default just works and is probably  
> > never changed.  On embedded platforms, a policy manager can change  
> > the other low power states according to its latency and operational  
> > requirements.
> 
> Plus the states should be distributed. Trying to force the whole
> system into certain state turns things messy.
> 
> Some devices may be active while some are in retention or suspend.
> 
> Basically everything should idle itself automatically whenever
> possible based on a idle timer or some other policy, such as
> suspending a device from user space via sysfs.

The timer sound like a reasonable idea, as long as there is one timer
for each shared resource, not user.

Example:

Devices A & B share the same voltage domain.

Device A has timeout period Timeout(A)
Device B has timeout period Timeout(B)

One timer is associated to the voltage regulator/switch and will expire
at t=TIM

Every time the device d (either A or B) performs some activity, then 

TIM = max(TIM, now + Timeout(d))

When t=TIM (timer expired), then the suspend() function for each device
is called.


-- 
Cheers, Igor

Igor Stoppa <igor.stoppa@nokia.com>
(Nokia Multimedia - CP - OSSO / Helsinki, Finland)

Re: [PATCH] implement pm_ops.valid for everybody

[tony]

* Igor Stoppa <igor.stoppa@nokia.com> [070323 09:37]:
> On Fri, 2007-03-23 at 09:17 -0400, ext
> linux-pm-bounces@lists.linux-foundation.org wrote:
> > * Matthew Locke <matt@nomadgs.com> [070322 21:15]:
> > > 
> > > On Mar 22, 2007, at 4:55 PM, David Brownell wrote:
> > > 
> > > > On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
> > > >
> > > >>> My answer:  there is NO value to such an arbitrary restriction.
> > > >>
> > > >> I'm not talking on restrictions.
> > > >
> > > > You most certainly did talk about them.  You said that if the
> > > > hardware doesn't support a "turn CPU off" mode, then you'd
> > > > define that as being incapable of implementing suspend-to-RAM.
> > > > That's a restriction ... a very arbitrary one.
> > > >
> > > >
> > > >> I'm talking on being able to define
> > > >> _anything_ more precisely then just a low-power system-wide state.
> > > >
> > > > Me too.  And I'm trying to convey to you the results of the
> > > > investigations I did on that topic.  You don't seem to like
> > > > those results though ...
> > > >
> > > >
> > > >> And let's start from just something, please.  Like STR and  
> > > >> "standby" to begin
> > > >> with?  At least on ACPI systems we can distinguish one from the  
> > > >> other quite
> > > >> clearly, so why can't we start from that and _then_ generalize?
> > > >
> > > > That's exactly what I did.  Looked also at APM, and several
> > > > different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
> > > >
> > > > The generalization I came up with is what I've described.
> > > > Namely, that coming up with one definition of those states
> > > > that can usefully be mapped all platforms is impractical.
> > > > They're just labels.  The platform implementor can choose
> > > > two states to implement, but non-x86 hardware states rarely
> > > > match the expectations of ACPI.
> > > >
> > > > So the fundamental definition needs to be in relative terms,
> > > > because platform-specific differences otherwise make trouble.
> > > 
> > > The problem is that a 1:1 mapping between system low power state and  
> > > a processor low power state is trying to be forced on every  
> > > platform.  As Dave pointed out, embedded SoC's provide multiple low  
> > > power states that qualify for the suspend-to-ram definition.  The  
> > > only reasonable platform independent definition is that in STR memory  
> > > is powered and contents preserved.  The rest is platform specific.
> > > 
> > > I think the right answer is that a mechanism for mapping platform  
> > > specific states to the system states is needed. Platforms define  
> > > their low power states and define the default for each system  
> > > state .  On x86 platforms, the default just works and is probably  
> > > never changed.  On embedded platforms, a policy manager can change  
> > > the other low power states according to its latency and operational  
> > > requirements.
> > 
> > Plus the states should be distributed. Trying to force the whole
> > system into certain state turns things messy.
> > 
> > Some devices may be active while some are in retention or suspend.
> > 
> > Basically everything should idle itself automatically whenever
> > possible based on a idle timer or some other policy, such as
> > suspending a device from user space via sysfs.
> 
> The timer sound like a reasonable idea, as long as there is one timer
> for each shared resource, not user.
> 
> Example:
> 
> Devices A & B share the same voltage domain.
> 
> Device A has timeout period Timeout(A)
> Device B has timeout period Timeout(B)
> 
> One timer is associated to the voltage regulator/switch and will expire
> at t=TIM
> 
> Every time the device d (either A or B) performs some activity, then 
> 
> TIM = max(TIM, now + Timeout(d))
> 
> When t=TIM (timer expired), then the suspend() function for each device
> is called.

What problem do you see with with device specific idle timers?

For example, what's wrong with the following:

When the device specific idle timer expires, the driver's suspend
function would get called, and the device would release it's clock
and voltage.

Then when a shared voltage domain has 0 users, that voltage domain
can be shut off. Same thing with clock domains.

Regards,

Tony

Re: [PATCH] implement pm_ops.valid for everybody

[Igor Stoppa]

On Fri, 2007-03-23 at 10:52 -0400, ext tony@atomide.com wrote:
> * Igor Stoppa <igor.stoppa@nokia.com> [070323 09:37]:
> > On Fri, 2007-03-23 at 09:17 -0400, ext
> > linux-pm-bounces@lists.linux-foundation.org wrote:
> > > * Matthew Locke <matt@nomadgs.com> [070322 21:15]:
> > > > 
> > > > On Mar 22, 2007, at 4:55 PM, David Brownell wrote:
> > > > 
> > > > > On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
> > > > >
> > > > >>> My answer:  there is NO value to such an arbitrary restriction.
> > > > >>
> > > > >> I'm not talking on restrictions.
> > > > >
> > > > > You most certainly did talk about them.  You said that if the
> > > > > hardware doesn't support a "turn CPU off" mode, then you'd
> > > > > define that as being incapable of implementing suspend-to-RAM.
> > > > > That's a restriction ... a very arbitrary one.
> > > > >
> > > > >
> > > > >> I'm talking on being able to define
> > > > >> _anything_ more precisely then just a low-power system-wide state.
> > > > >
> > > > > Me too.  And I'm trying to convey to you the results of the
> > > > > investigations I did on that topic.  You don't seem to like
> > > > > those results though ...
> > > > >
> > > > >
> > > > >> And let's start from just something, please.  Like STR and  
> > > > >> "standby" to begin
> > > > >> with?  At least on ACPI systems we can distinguish one from the  
> > > > >> other quite
> > > > >> clearly, so why can't we start from that and _then_ generalize?
> > > > >
> > > > > That's exactly what I did.  Looked also at APM, and several
> > > > > different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
> > > > >
> > > > > The generalization I came up with is what I've described.
> > > > > Namely, that coming up with one definition of those states
> > > > > that can usefully be mapped all platforms is impractical.
> > > > > They're just labels.  The platform implementor can choose
> > > > > two states to implement, but non-x86 hardware states rarely
> > > > > match the expectations of ACPI.
> > > > >
> > > > > So the fundamental definition needs to be in relative terms,
> > > > > because platform-specific differences otherwise make trouble.
> > > > 
> > > > The problem is that a 1:1 mapping between system low power state and  
> > > > a processor low power state is trying to be forced on every  
> > > > platform.  As Dave pointed out, embedded SoC's provide multiple low  
> > > > power states that qualify for the suspend-to-ram definition.  The  
> > > > only reasonable platform independent definition is that in STR memory  
> > > > is powered and contents preserved.  The rest is platform specific.
> > > > 
> > > > I think the right answer is that a mechanism for mapping platform  
> > > > specific states to the system states is needed. Platforms define  
> > > > their low power states and define the default for each system  
> > > > state .  On x86 platforms, the default just works and is probably  
> > > > never changed.  On embedded platforms, a policy manager can change  
> > > > the other low power states according to its latency and operational  
> > > > requirements.
> > > 
> > > Plus the states should be distributed. Trying to force the whole
> > > system into certain state turns things messy.
> > > 
> > > Some devices may be active while some are in retention or suspend.
> > > 
> > > Basically everything should idle itself automatically whenever
> > > possible based on a idle timer or some other policy, such as
> > > suspending a device from user space via sysfs.
> > 
> > The timer sound like a reasonable idea, as long as there is one timer
> > for each shared resource, not user.
> > 
> > Example:
> > 
> > Devices A & B share the same voltage domain.
> > 
> > Device A has timeout period Timeout(A)
> > Device B has timeout period Timeout(B)
> > 
> > One timer is associated to the voltage regulator/switch and will expire
> > at t=TIM
> > 
> > Every time the device d (either A or B) performs some activity, then 
> > 
> > TIM = max(TIM, now + Timeout(d))
> > 
> > When t=TIM (timer expired), then the suspend() function for each device
> > is called.
> 
> What problem do you see with with device specific idle timers?

That the number of idle timers grows linearly with the number of
resources consumers rather than _providers_

See also my comment below.

> For example, what's wrong with the following:
> 
> When the device specific idle timer expires, the driver's suspend
> function would get called, and the device would release it's clock
> and voltage.

We might end up doing extra useless activity by saving the state of a
device that is re-enabled without even going off.

Of course the restoring can be optimised so that it doesn't happen
unless the voltage has actually been removed (this implies that the
state saving happens in such a way that doesn't compromise the current
settings of the device).

> Then when a shared voltage domain has 0 users, that voltage domain
> can be shut off. 

Both your and my approaches have drawbacks: in your case the system will
probably end up doing extra state saving, but will be ready to perform
immediately the transition to off; in my case there will be the overhead
of saving the state of the peripherals.

However saving state in a preemptive way is decoupled by having idle
timers associated to resources providers rather than consumers.

> Same thing with clock domains.

Clocks is fine, since no saving/restoring is needed, albeit we might
consider PLL relock time to fall in this "costy" class of activities.

-- 
Cheers, Igor

Igor Stoppa <igor.stoppa@nokia.com>
(Nokia Multimedia - CP - OSSO / Helsinki, Finland)

Re: [PATCH] implement pm_ops.valid for everybody

[Matthew Locke]

On Mar 23, 2007, at 8:17 AM, Igor Stoppa wrote:

> On Fri, 2007-03-23 at 10:52 -0400, ext tony@atomide.com wrote:
>> * Igor Stoppa <igor.stoppa@nokia.com> [070323 09:37]:
>>> On Fri, 2007-03-23 at 09:17 -0400, ext
>>> linux-pm-bounces@lists.linux-foundation.org wrote:
>>>> * Matthew Locke <matt@nomadgs.com> [070322 21:15]:
>>>>>
>>>>> On Mar 22, 2007, at 4:55 PM, David Brownell wrote:
>>>>>
>>>>>> On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
>>>>>>
>>>>>>>> My answer:  there is NO value to such an arbitrary restriction.
>>>>>>>
>>>>>>> I'm not talking on restrictions.
>>>>>>
>>>>>> You most certainly did talk about them.  You said that if the
>>>>>> hardware doesn't support a "turn CPU off" mode, then you'd
>>>>>> define that as being incapable of implementing suspend-to-RAM.
>>>>>> That's a restriction ... a very arbitrary one.
>>>>>>
>>>>>>
>>>>>>> I'm talking on being able to define
>>>>>>> _anything_ more precisely then just a low-power system-wide  
>>>>>>> state.
>>>>>>
>>>>>> Me too.  And I'm trying to convey to you the results of the
>>>>>> investigations I did on that topic.  You don't seem to like
>>>>>> those results though ...
>>>>>>
>>>>>>
>>>>>>> And let's start from just something, please.  Like STR and
>>>>>>> "standby" to begin
>>>>>>> with?  At least on ACPI systems we can distinguish one from the
>>>>>>> other quite
>>>>>>> clearly, so why can't we start from that and _then_ generalize?
>>>>>>
>>>>>> That's exactly what I did.  Looked also at APM, and several
>>>>>> different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
>>>>>>
>>>>>> The generalization I came up with is what I've described.
>>>>>> Namely, that coming up with one definition of those states
>>>>>> that can usefully be mapped all platforms is impractical.
>>>>>> They're just labels.  The platform implementor can choose
>>>>>> two states to implement, but non-x86 hardware states rarely
>>>>>> match the expectations of ACPI.
>>>>>>
>>>>>> So the fundamental definition needs to be in relative terms,
>>>>>> because platform-specific differences otherwise make trouble.
>>>>>
>>>>> The problem is that a 1:1 mapping between system low power  
>>>>> state and
>>>>> a processor low power state is trying to be forced on every
>>>>> platform.  As Dave pointed out, embedded SoC's provide multiple  
>>>>> low
>>>>> power states that qualify for the suspend-to-ram definition.  The
>>>>> only reasonable platform independent definition is that in STR  
>>>>> memory
>>>>> is powered and contents preserved.  The rest is platform specific.
>>>>>
>>>>> I think the right answer is that a mechanism for mapping platform
>>>>> specific states to the system states is needed. Platforms define
>>>>> their low power states and define the default for each system
>>>>> state .  On x86 platforms, the default just works and is probably
>>>>> never changed.  On embedded platforms, a policy manager can change
>>>>> the other low power states according to its latency and  
>>>>> operational
>>>>> requirements.
>>>>
>>>> Plus the states should be distributed. Trying to force the whole
>>>> system into certain state turns things messy.
>>>>
>>>> Some devices may be active while some are in retention or suspend.
>>>>
>>>> Basically everything should idle itself automatically whenever
>>>> possible based on a idle timer or some other policy, such as
>>>> suspending a device from user space via sysfs.
>>>
>>> The timer sound like a reasonable idea, as long as there is one  
>>> timer
>>> for each shared resource, not user.
>>>
>>> Example:
>>>
>>> Devices A & B share the same voltage domain.
>>>
>>> Device A has timeout period Timeout(A)
>>> Device B has timeout period Timeout(B)
>>>
>>> One timer is associated to the voltage regulator/switch and will  
>>> expire
>>> at t=TIM
>>>
>>> Every time the device d (either A or B) performs some activity, then
>>>
>>> TIM = max(TIM, now + Timeout(d))
>>>
>>> When t=TIM (timer expired), then the suspend() function for each  
>>> device
>>> is called.
>>
>> What problem do you see with with device specific idle timers?
>
> That the number of idle timers grows linearly with the number of
> resources consumers rather than _providers_
>
> See also my comment below.
>
>> For example, what's wrong with the following:
>>
>> When the device specific idle timer expires, the driver's suspend
>> function would get called, and the device would release it's clock
>> and voltage.
>
> We might end up doing extra useless activity by saving the state of a
> device that is re-enabled without even going off.
>
> Of course the restoring can be optimised so that it doesn't happen
> unless the voltage has actually been removed (this implies that the
> state saving happens in such a way that doesn't compromise the current
> settings of the device).
>
>> Then when a shared voltage domain has 0 users, that voltage domain
>> can be shut off.
>
> Both your and my approaches have drawbacks: in your case the system  
> will
> probably end up doing extra state saving, but will be ready to perform
> immediately the transition to off; in my case there will be the  
> overhead
> of saving the state of the peripherals.

I think we again have the problem that the behavior is very device  
specific.  Some i/o devices have internal low power states that may  
or may not require saving state.  Others have no notion of low power  
states. And on some platforms register contents are preserved even  
when a device is "off".

How about something like:

  - When idle timer pops,  driver releases pm resources (clock,  
voltage, whatever).
  - Then driver does device specific stuff which may or may not  
include going into a low power state and saving state.
  - If a pm resource (clock, voltage, whatever) reference count goes  
to zero, something will decide to turn it off.
  - Users of the pm resource are told they are going to lose power.
  - Then driver does device specific stuff.  If the device driver  
already saved state, then ignore.  Otherwise do what the device needs  
done.

>
> However saving state in a preemptive way is decoupled by having idle
> timers associated to resources providers rather than consumers.
>
>> Same thing with clock domains.
>
> Clocks is fine, since no saving/restoring is needed, albeit we might
> consider PLL relock time to fall in this "costy" class of activities.
>
> -- 
> Cheers, Igor
>
> Igor Stoppa <igor.stoppa@nokia.com>
> (Nokia Multimedia - CP - OSSO / Helsinki, Finland)
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] implement pm_ops.valid for everybody

[Igor Stoppa]

On Fri, 2007-03-23 at 11:51 -0700, ext Matthew Locke wrote:
> On Mar 23, 2007, at 8:17 AM, Igor Stoppa wrote:
> 
> > On Fri, 2007-03-23 at 10:52 -0400, ext tony@atomide.com wrote:
> >> * Igor Stoppa <igor.stoppa@nokia.com> [070323 09:37]:
> >>> On Fri, 2007-03-23 at 09:17 -0400, ext
> >>> linux-pm-bounces@lists.linux-foundation.org wrote:
> >>>> * Matthew Locke <matt@nomadgs.com> [070322 21:15]:
> >>>>>
> >>>>> On Mar 22, 2007, at 4:55 PM, David Brownell wrote:
> >>>>>
> >>>>>> On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
> >>>>>>
> >>>>>>>> My answer:  there is NO value to such an arbitrary restriction.
> >>>>>>>
> >>>>>>> I'm not talking on restrictions.
> >>>>>>
> >>>>>> You most certainly did talk about them.  You said that if the
> >>>>>> hardware doesn't support a "turn CPU off" mode, then you'd
> >>>>>> define that as being incapable of implementing suspend-to-RAM.
> >>>>>> That's a restriction ... a very arbitrary one.
> >>>>>>
> >>>>>>
> >>>>>>> I'm talking on being able to define
> >>>>>>> _anything_ more precisely then just a low-power system-wide  
> >>>>>>> state.
> >>>>>>
> >>>>>> Me too.  And I'm trying to convey to you the results of the
> >>>>>> investigations I did on that topic.  You don't seem to like
> >>>>>> those results though ...
> >>>>>>
> >>>>>>
> >>>>>>> And let's start from just something, please.  Like STR and
> >>>>>>> "standby" to begin
> >>>>>>> with?  At least on ACPI systems we can distinguish one from the
> >>>>>>> other quite
> >>>>>>> clearly, so why can't we start from that and _then_ generalize?
> >>>>>>
> >>>>>> That's exactly what I did.  Looked also at APM, and several
> >>>>>> different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
> >>>>>>
> >>>>>> The generalization I came up with is what I've described.
> >>>>>> Namely, that coming up with one definition of those states
> >>>>>> that can usefully be mapped all platforms is impractical.
> >>>>>> They're just labels.  The platform implementor can choose
> >>>>>> two states to implement, but non-x86 hardware states rarely
> >>>>>> match the expectations of ACPI.
> >>>>>>
> >>>>>> So the fundamental definition needs to be in relative terms,
> >>>>>> because platform-specific differences otherwise make trouble.
> >>>>>
> >>>>> The problem is that a 1:1 mapping between system low power  
> >>>>> state and
> >>>>> a processor low power state is trying to be forced on every
> >>>>> platform.  As Dave pointed out, embedded SoC's provide multiple  
> >>>>> low
> >>>>> power states that qualify for the suspend-to-ram definition.  The
> >>>>> only reasonable platform independent definition is that in STR  
> >>>>> memory
> >>>>> is powered and contents preserved.  The rest is platform specific.
> >>>>>
> >>>>> I think the right answer is that a mechanism for mapping platform
> >>>>> specific states to the system states is needed. Platforms define
> >>>>> their low power states and define the default for each system
> >>>>> state .  On x86 platforms, the default just works and is probably
> >>>>> never changed.  On embedded platforms, a policy manager can change
> >>>>> the other low power states according to its latency and  
> >>>>> operational
> >>>>> requirements.
> >>>>
> >>>> Plus the states should be distributed. Trying to force the whole
> >>>> system into certain state turns things messy.
> >>>>
> >>>> Some devices may be active while some are in retention or suspend.
> >>>>
> >>>> Basically everything should idle itself automatically whenever
> >>>> possible based on a idle timer or some other policy, such as
> >>>> suspending a device from user space via sysfs.
> >>>
> >>> The timer sound like a reasonable idea, as long as there is one  
> >>> timer
> >>> for each shared resource, not user.
> >>>
> >>> Example:
> >>>
> >>> Devices A & B share the same voltage domain.
> >>>
> >>> Device A has timeout period Timeout(A)
> >>> Device B has timeout period Timeout(B)
> >>>
> >>> One timer is associated to the voltage regulator/switch and will  
> >>> expire
> >>> at t=TIM
> >>>
> >>> Every time the device d (either A or B) performs some activity, then
> >>>
> >>> TIM = max(TIM, now + Timeout(d))
> >>>
> >>> When t=TIM (timer expired), then the suspend() function for each  
> >>> device
> >>> is called.
> >>
> >> What problem do you see with with device specific idle timers?
> >
> > That the number of idle timers grows linearly with the number of
> > resources consumers rather than _providers_
> >
> > See also my comment below.
> >
> >> For example, what's wrong with the following:
> >>
> >> When the device specific idle timer expires, the driver's suspend
> >> function would get called, and the device would release it's clock
> >> and voltage.
> >
> > We might end up doing extra useless activity by saving the state of a
> > device that is re-enabled without even going off.
> >
> > Of course the restoring can be optimised so that it doesn't happen
> > unless the voltage has actually been removed (this implies that the
> > state saving happens in such a way that doesn't compromise the current
> > settings of the device).
> >
> >> Then when a shared voltage domain has 0 users, that voltage domain
> >> can be shut off.
> >
> > Both your and my approaches have drawbacks: in your case the system  
> > will
> > probably end up doing extra state saving, but will be ready to perform
> > immediately the transition to off; in my case there will be the  
> > overhead
> > of saving the state of the peripherals.
> 
> I think we again have the problem that the behavior is very device  
> specific.  

It's not really a problem.

> Some i/o devices have internal low power states that may  
> or may not require saving state.  Others have no notion of low power  
> states. And on some platforms register contents are preserved even  
> when a device is "off".

So???? Each ofthem will take care of itself, that's expected.

> How about something like:
> 
>   - When idle timer pops,  driver releases pm resources (clock,  
> voltage, whatever).
No: it's still using one timer for each driver.

>   - Then driver does device specific stuff which may or may not  
> include going into a low power state and saving state.
Sure, that's the device-specific part.

>   - If a pm resource (clock, voltage, whatever) reference count goes  
> to zero, something will decide to turn it off.

Something? I see a manager lurking here.

Why can't the corresponding framework (clock or voltage, let's drop the
whatever_framework till it gets integrated) switch off the resource when
the usecount drops to zero?

>   - Users of the pm resource are told they are going to lose power.

ok

>   - Then driver does device specific stuff.  If the device driver  
> already saved state, then ignore.  Otherwise do what the device needs  
> done.

I'd rather do everything here, but this is probably a decision that
depends on the specific device. So maybe a device-based behavior is the
best option: sort of PRE and POST.

> >
> > However saving state in a preemptive way is decoupled by having idle
> > timers associated to resources providers rather than consumers.
> >
> >> Same thing with clock domains.
> >
> > Clocks is fine, since no saving/restoring is needed, albeit we might
> > consider PLL relock time to fall in this "costy" class of activities.

-- 
Cheers, Igor

Igor Stoppa <igor.stoppa@nokia.com>
(Nokia Multimedia - CP - OSSO / Helsinki, Finland)

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 6:14 pm, Matthew Locke wrote:
> >
> > So the fundamental definition needs to be in relative terms,
> > because platform-specific differences otherwise make trouble.
> 
> The problem is that a 1:1 mapping between system low power state and  
> a processor low power state is trying to be forced on every  
> platform.

Sort of.  There are only two labels available for "system" states,
and the platform-specific code entering those states will probably
kick in a processor low power state.  But there'd be no point in
preventing that code from kicking in deeper power save states.

Consider the PXA 255, which has an "idle" mode that's natural for
the idle loop, a "33 MHz idle" mode that saves more power but
means most peripherals can't be clocked, and a "sleep" mode that
turns the CPU off.  A "standby" might normally use "idle", but
might likewise use "33 MHz idle" if all the peripheral clocks
happen to be gated off after the drivers suspend().  That wouldn't
be a one-to-one mapping ... and smarter hardware might do very
similar stuff automatically, too.


> As Dave pointed out, embedded SoC's provide multiple low   
> power states that qualify for the suspend-to-ram definition.  The  
> only reasonable platform independent definition is that in STR memory  
> is powered and contents preserved.  The rest is platform specific.

That definition also applies to "standby" of course ... there may
be a LOT of states where the "standby" label can usefully apply.


> I think the right answer is that a mechanism for mapping platform  
> specific states to the system states is needed. 

That could be done, but in practice not all platform states will
necessarily be implemented by the platform code.


> Platforms define   
> their low power states and define the default for each system  
> state .  On x86 platforms, the default just works and is probably  
> never changed. 

That's the way it works today on all Linux platforms.  The x86
ones actually don't "just work" in my observation ... when either
"standby" or "STR" actually work, I've been quite surprised; the
basic issue seems to be that ACPI resume paths often misbehave.


> On embedded platforms, a policy manager can change   
> the other low power states according to its latency and operational  
> requirements.

That's not yet a proposal to change things; no details.  :)

If I were to want to change things in that area, I'd likely want
to let platforms define their own states and names (*), but in
fact I think it's probably a lot more imporant for embedded cases
to support top notch *runtime* PM rather than /sys/power/state
kinds of transitions.

- Dave

(*) For example, predefine more suspend_state_t values -- maybe
    PM_SUSPEND_PLATFORM_{0-9}, to start with -- and then add a
    label-that-state callback to "struct pm_ops".  Then make
    /sys/power/state do the obvious stuff to read and write
    additional states ... voila, platforms can now add new low
    power states with their own names.  Their clk_must_disable()
    could let drivers see some of the state differences; there
    might need to be similar mechanisms for other PM resources.

Re: [PATCH] implement pm_ops.valid for everybody

[Matthew Locke]

On Mar 23, 2007, at 11:29 AM, David Brownell wrote:

> On Thursday 22 March 2007 6:14 pm, Matthew Locke wrote:
>>>
>>> So the fundamental definition needs to be in relative terms,
>>> because platform-specific differences otherwise make trouble.
>>
>> The problem is that a 1:1 mapping between system low power state and
>> a processor low power state is trying to be forced on every
>> platform.
>
> Sort of.  There are only two labels available for "system" states,
> and the platform-specific code entering those states will probably
> kick in a processor low power state.  But there'd be no point in
> preventing that code from kicking in deeper power save states.

Unless I've misread something, it is exactly a 1:1 mapping between  
the system state and the cpu state.  There is no code to support  
mapping multiple cpu states to the system state and then selecting  
the cpu state that is entered for a system state at runtime.

>
> Consider the PXA 255, which has an "idle" mode that's natural for
> the idle loop, a "33 MHz idle" mode that saves more power but
> means most peripherals can't be clocked, and a "sleep" mode that
> turns the CPU off.  A "standby" might normally use "idle", but
> might likewise use "33 MHz idle" if all the peripheral clocks
> happen to be gated off after the drivers suspend().  That wouldn't
> be a one-to-one mapping ... and smarter hardware might do very
> similar stuff automatically, too.
>


You are saying existing code handles this case?

>
>> As Dave pointed out, embedded SoC's provide multiple low
>> power states that qualify for the suspend-to-ram definition.  The
>> only reasonable platform independent definition is that in STR memory
>> is powered and contents preserved.  The rest is platform specific.
>
> That definition also applies to "standby" of course ... there may
> be a LOT of states where the "standby" label can usefully apply.

Sure, but its easier to start with one thing at time.  Frankly I  
don't know what do with standby, but if have this ability to change  
the mapping to the platform state at runtime, then it doesn't really  
matter.

>
>
>> I think the right answer is that a mechanism for mapping platform
>> specific states to the system states is needed.
>
> That could be done, but in practice not all platform states will
> necessarily be implemented by the platform code.

Ok, does this matter?  Certainly more than two will be implemented.   
The latest pxa's have much more than that.

>
>
>> Platforms define
>> their low power states and define the default for each system
>> state .  On x86 platforms, the default just works and is probably
>> never changed.
>
> That's the way it works today on all Linux platforms.  The x86
> ones actually don't "just work" in my observation ... when either
> "standby" or "STR" actually work, I've been quite surprised; the
> basic issue seems to be that ACPI resume paths often misbehave.

That is a different problem.  My point is that platforms that don't  
need to change mappings don't have to do anything different.   Of  
course there is no  magic that will make them work if they didn't  
already.

>
>
>> On embedded platforms, a policy manager can change
>> the other low power states according to its latency and operational
>> requirements.
>
> That's not yet a proposal to change things; no details.  :)

Funny.   The details on changing the mapping should be simple enough.

>
> If I were to want to change things in that area, I'd likely want
> to let platforms define their own states and names (*),

Sure.  Much of the behavior is platform specific.  It really depends  
on who is going to use /sys/power/state.  Will it be a general pm  
daemon that can be used on every platform or will it be specific to  
the platform.

> but in
> fact I think it's probably a lot more imporant for embedded cases
> to support top notch *runtime* PM rather than /sys/power/state
> kinds of transitions.

Then why bother contributing to this thread.   Since someone is  
changing code in this area already and discussing definitions, its  
good to let everyone what works for other platforms.

>
> - Dave
>
> (*) For example, predefine more suspend_state_t values -- maybe
>     PM_SUSPEND_PLATFORM_{0-9}, to start with -- and then add a
>     label-that-state callback to "struct pm_ops".  Then make
>     /sys/power/state do the obvious stuff to read and write
>     additional states ... voila, platforms can now add new low
>     power states with their own names.  Their clk_must_disable()
>     could let drivers see some of the state differences; there
>     might need to be similar mechanisms for other PM resources.
>
>

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Friday 23 March 2007 12:21 pm, Matthew Locke wrote:
> 
> Unless I've misread something, it is exactly a 1:1 mapping between  
> the system state and the cpu state.  There is no code to support  
> mapping multiple cpu states to the system state and then selecting  
> the cpu state that is entered for a system state at runtime.

Platform hardware and software can do whatever it wants when
it gets the pm_ops.enter() request.

There's no formal notion of CPU state (or current need for one!),
so of course there's no code doing what you sketched.


> > Consider the PXA 255, which has an "idle" mode that's natural for
> > the idle loop, a "33 MHz idle" mode that saves more power but
> > means most peripherals can't be clocked, and a "sleep" mode that
> > turns the CPU off.  A "standby" might normally use "idle", but
> > might likewise use "33 MHz idle" if all the peripheral clocks
> > happen to be gated off after the drivers suspend().  That wouldn't
> > be a one-to-one mapping ... and smarter hardware might do very
> > similar stuff automatically, too.
> 
> You are saying existing code handles this case?

Not at all.  It only handles an STR mode.  I'm pointing out
that would be a valid implementation of a "standby" mode,
with lower resume latency (other than device resume) than
the current STR.


> >> I think the right answer is that a mechanism for mapping platform
> >> specific states to the system states is needed.
> >
> > That could be done, but in practice not all platform states will
> > necessarily be implemented by the platform code.
> 
> Ok, does this matter?  Certainly more than two will be implemented.   
> The latest pxa's have much more than that.

It matters in the pragmatic sense that if you're assuming every
possible hardware PM state documented in the chip manual will be
implemented, tested, and used in Linux ... I'm highly skeptical.



> 	 My point is that platforms that don't  
> need to change mappings don't have to do anything different. 

But ... we don't currently have a need for "mappings", or to
change them.  What problem would be solved by adding "mappings"?
Especially since the $SUBJECT patches highlighted the fact that
most platforms only support one of the N possible state?  :)


> > If I were to want to change things in that area, I'd likely want
> > to let platforms define their own states and names (*),
> 
> Sure.  Much of the behavior is platform specific.  It really depends  
> on who is going to use /sys/power/state.  Will it be a general pm  
> daemon that can be used on every platform or will it be specific to  
> the platform.

I don't think platforms should gratuitously break the code that knows
how to use /sys/power/state ... and by "code" I include not just the
tools and their GUIs, but also documentation and the user training.


> > but in
> > fact I think it's probably a lot more imporant for embedded cases
> > to support top notch *runtime* PM rather than /sys/power/state
> > kinds of transitions.
> 
> Then why bother contributing to this thread.   Since someone is  
> changing code in this area already and discussing definitions, its  
> good to let everyone what works for other platforms.

Because I believe bad ideas should not prosper ... and accordingly
that pm_ops should make as much sense as practical.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Thu, 22 Mar 2007, David Brownell wrote:

> So the fundamental definition needs to be in relative terms,
> because platform-specific differences otherwise make trouble.

What if we define system-wide and per device power states in terms of 
time? Time needed for the system / device to become fully operational 
again? And you'd need a user-space calibration daemon...

Thanks
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 11:46 pm, Guennadi Liakhovetski wrote:
> On Thu, 22 Mar 2007, David Brownell wrote:
> 
> > So the fundamental definition needs to be in relative terms,
> > because platform-specific differences otherwise make trouble.
> 
> What if we define system-wide and per device power states in terms of 
> time? Time needed for the system / device to become fully operational 
> again? And you'd need a user-space calibration daemon...

So if you take two otherwise identical systems and clock one slower
than the other, then "standby" may become "suspend-to-RAM"?

Yuck.  No.  Speed is only a factor in that STR is likely slower.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Fri, 23 Mar 2007, David Brownell wrote:

> On Thursday 22 March 2007 11:46 pm, Guennadi Liakhovetski wrote:
> > On Thu, 22 Mar 2007, David Brownell wrote:
> > 
> > > So the fundamental definition needs to be in relative terms,
> > > because platform-specific differences otherwise make trouble.
> > 
> > What if we define system-wide and per device power states in terms of 
> > time? Time needed for the system / device to become fully operational 
> > again? And you'd need a user-space calibration daemon...
> 
> So if you take two otherwise identical systems and clock one slower
> than the other, then "standby" may become "suspend-to-RAM"?
> 
> Yuck.  No.  Speed is only a factor in that STR is likely slower.

I'm just trying to think about it from the user perspective. As a user I 
want - save the system save some power but be ready again when I need it 
_not_ _later_ than, 10 sec after wakeup. I don't care how it is called. I 
want to save as much power as possible and I want a certain wakeup time. 
Yes, if one system in your example is clocked slower, then yes, saying "I 
want it back up and running 5 seconds after wakeup" will for one of them 
mean "standby" for another one "str". As a user I don't care whatsoever 
what PCI state you drive my eth card into. I want it back in 5 seconds.

Similarly for single devices: I don't need wlan now, but when I need it I 
want it to be available in less than 2 seconds.

And if I say 10 minutes hibernate it.

If I say "don't care about time, save maximum power" - similarly.

And I may say "I want it wakeable from eth" or "from modem" take that into 
account too.

Those would be policies to be implemented in the user space. The kernel 
might just present a single numerical per-device parameter "wakeup 
responsiveness", and a way to do this system-wide.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Friday 23 March 2007 2:08 pm, Guennadi Liakhovetski wrote:
> On Fri, 23 Mar 2007, David Brownell wrote:
> > 
> > Yuck.  No.  Speed is only a factor in that STR is likely slower.
> 
> I'm just trying to think about it from the user perspective. As a user I 
> want - save the system save some power but be ready again when I need it 
> _not_ _later_ than, 10 sec after wakeup. I don't care how it is called. I 
> want to save as much power as possible and I want a certain wakeup time. 

That's not really a kernel issue; and it's highly dependent on what
devices are hooked up.  What if certain devices take a second to
re-activate ... and you have a dozen of them hooked up?


> Yes, if one system in your example is clocked slower, then yes, saying "I 
> want it back up and running 5 seconds after wakeup" will for one of them 
> mean "standby" for another one "str". As a user I don't care whatsoever 
> what PCI state you drive my eth card into. I want it back in 5 seconds.

I don't think any of the current tools address such guarantees.

And it seems to me you're looking at this more from a tools
perspective than from "what does pm_ops.enter(STATE) do".
So this is a change-of-topic.


> Similarly for single devices: I don't need wlan now, but when I need it I 
> want it to be available in less than 2 seconds.

That's a driver-specific issue.


> And if I say 10 minutes hibernate it.

The tools I've seen give a "hibernate" (suspend-to-disk) option,
but not a "10 minutes" option ... and on Linux, there isn't even
any kind of "enter system state X if idle for M minutes" facility.


> If I say "don't care about time, save maximum power" - similarly.

Again, that's an issue about what some to-be-written userspace
tool might do.


> And I may say "I want it wakeable from eth" or "from modem" take that into 
> account too.

All those are driver configuration issues, although there's also
a huge dose of "ACPI wake events rarely work".  For ethernet
devices there's "ethtool".  For serial lines and other devices,
there's /sys/devices/.../power/wakeup configuration.

 
> Those would be policies to be implemented in the user space. The kernel 
> might just present a single numerical per-device parameter "wakeup 
> responsiveness", and a way to do this system-wide.

Today, if you want to associate a set of wakeup events with some
particular sleep state (and the hardware supports them), just update
the /sys/devices/.../power/wakeup values before writing /sys/power/state
and voila!

That could be done with a shell script.

- Dave


> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
> 

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Friday, 23 March 2007 00:55, David Brownell wrote:
> On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
> 
> > > My answer:  there is NO value to such an arbitrary restriction.
> > 
> > I'm not talking on restrictions.
> 
> You most certainly did talk about them.

No, sorry.  Apparently, I should have said more precisely what I meant. :-)

> You said that if the hardware doesn't support a "turn CPU off" mode, then
> you'd define that as being incapable of implementing suspend-to-RAM.

That's _if_ the suspend-to-RAM is defined as the state in which the CPU
is off, which I _think_ would be a reasonable definition.  I don't mean the
platforms incapable of doing this should be restricted from entering any
system-wide low-power states, but perhaps we can call these states
differently.

> That's a restriction ... a very arbitrary one.
> 
> 
> > I'm talking on being able to define 
> > _anything_ more precisely then just a low-power system-wide state.
> 
> Me too.  And I'm trying to convey to you the results of the
> investigations I did on that topic.  You don't seem to like
> those results though ...
> 
> 
> > And let's start from just something, please.  Like STR and "standby" to begin
> > with?  At least on ACPI systems we can distinguish one from the other quite
> > clearly, so why can't we start from that and _then_ generalize?
> 
> That's exactly what I did.  Looked also at APM, and several
> different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
> 
> The generalization I came up with is what I've described.
> Namely, that coming up with one definition of those states
> that can usefully be mapped all platforms is impractical.
> They're just labels.

Yes.

> The platform implementor can choose two states to implement, but
> non-x86 hardware states rarely match the expectations of ACPI.

Yes.  I which case I think the states shouldn't be _labeled_ in the same
way as the "ACPI" states that they are not.

My point is that _if_ we use lables like "standby", "STR", "STD", etc.,
then they shouldn't mean different things on different platforms.  So, either
we don't use labels at all, or we should know what they mean regardless
of what platform we're talking about.

> So the fundamental definition needs to be in relative terms,
> because platform-specific differences otherwise make trouble.

If your point is that lables are impractical for identifying different
low-power states of different systems, then I agree.

Greetings,
Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Friday 23 March 2007 6:43 am, Rafael J. Wysocki wrote:
> On Friday, 23 March 2007 00:55, David Brownell wrote:

> > You said that if the hardware doesn't support a "turn CPU off" mode, then
> > you'd define that as being incapable of implementing suspend-to-RAM.
> 
> That's _if_ the suspend-to-RAM is defined as the state in which the CPU
> is off, which I _think_ would be a reasonable definition.  

I disagree.


> I don't mean the 
> platforms incapable of doing this should be restricted from entering any
> system-wide low-power states, but perhaps we can call these states
> differently.

Well, we have ** ONLY TWO LABELS TO APPLY ** and you're saying that
one of them should be restricted to systems where the CPU can go into
an "off" state.  


> > That's a restriction ... a very arbitrary one.
> > 
> > 
> > ...
>
> My point is that _if_ we use lables like "standby", "STR", "STD", etc.,

That is, the strings in /sys/power/state.  That's a given for now...


> then they shouldn't mean different things on different platforms. 

Unreasonable.  The platforms are different.  And moreover the
specifics DO NOT MATTER to userspace.  Plus, they can differ
even on two x86 systems:  different D-states, different wakeup
events.  So nobody has any valid expectation that STR on one
box has exactly the same behavior on a different box.  And if
users are trained to expect anything, it's that platforms will
differ in those details.


> So, either 
> we don't use labels at all, or we should know what they mean regardless
> of what platform we're talking about.

That's a false choice, when you "mean" anything more than
fairly broad behavioral expectations:  STR saves more power
than "standby", and transitions to/from STR take more
time than to/from "standby".

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Friday, 23 March 2007 18:57, David Brownell wrote:
> On Friday 23 March 2007 6:43 am, Rafael J. Wysocki wrote:
> > On Friday, 23 March 2007 00:55, David Brownell wrote:
> 
> > > You said that if the hardware doesn't support a "turn CPU off" mode, then
> > > you'd define that as being incapable of implementing suspend-to-RAM.
> > 
> > That's _if_ the suspend-to-RAM is defined as the state in which the CPU
> > is off, which I _think_ would be a reasonable definition.  
> 
> I disagree.

Fine, this only is a matter of opinion. :-)
 
> > I don't mean the 
> > platforms incapable of doing this should be restricted from entering any
> > system-wide low-power states, but perhaps we can call these states
> > differently.
> 
> Well, we have ** ONLY TWO LABELS TO APPLY ** and you're saying that
> one of them should be restricted to systems where the CPU can go into
> an "off" state.  

As long as you insist on using two lables only and these two labels in
particular, then fine.  [Well, in such a case I'd change them to something
different (like "suspend level 1", "suspend level 2").]

Still, technically it doesn't really matter.  What matters is that we need to
tell drivers what we expect them to do with the devices when we're
transitioning from one state to another system-wide.  And that, IMO, should
be defined.

> > > That's a restriction ... a very arbitrary one.
> > > 
> > My point is that _if_ we use lables like "standby", "STR", "STD", etc.,
> 
> That is, the strings in /sys/power/state.  That's a given for now...

Okay, I see your point now.
 
> > then they shouldn't mean different things on different platforms. 
> 
> Unreasonable.  The platforms are different.  And moreover the
> specifics DO NOT MATTER to userspace.  Plus, they can differ
> even on two x86 systems:  different D-states, different wakeup
> events.  So nobody has any valid expectation that STR on one
> box has exactly the same behavior on a different box.  And if
> users are trained to expect anything, it's that platforms will
> differ in those details.
> 
> > So, either 
> > we don't use labels at all, or we should know what they mean regardless
> > of what platform we're talking about.
> 
> That's a false choice, when you "mean" anything more than
> fairly broad behavioral expectations:  STR saves more power
> than "standby", and transitions to/from STR take more
> time than to/from "standby".

So be it.

Assume that the user does 'echo standby > /sys/power/state'.  I think he can
expect that in such a case we'll freeze tasks and put devices into low-power
states and when he wakes up the system (BTW, I think the method of waking
up can be treated as a differentiating factor) he should be able to continue
from where he stopped after a little time.  Fine.

Now, we have to make that happen.  After we have frozen tasks, we need to
call something like device_suspend(some_argument) where the argument should
tell drivers what to do.  Say we use something like PMSG_STANDBY and now
do you think the meaning of it can change from one platform to another?  And if
it can, how can the drivers figure out the meaning?

Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

(I do not want to get into this flamewar).

> > That's a false choice, when you "mean" anything more than
> > fairly broad behavioral expectations:  STR saves more power
> > than "standby", and transitions to/from STR take more
> > time than to/from "standby".
> 
> So be it.
> 
> Assume that the user does 'echo standby > /sys/power/state'.  I think he can
> expect that in such a case we'll freeze tasks and put devices into low-power
> states and when he wakes up the system (BTW, I think the method of waking
> up can be treated as a differentiating factor) he should be able to continue
> from where he stopped after a little time.  Fine.
> 
> Now, we have to make that happen.  After we have frozen tasks, we need to
> call something like device_suspend(some_argument) where the argument should
> tell drivers what to do.  Say we use something like PMSG_STANDBY and now

We would add another field to that struct, distingushing "mem" and
"standby". And meaning for the drivers would be "try to save a lot of
power, but keep the latency low" for standby, vs. "save as much power
as possible" for mem.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Friday 23 March 2007 5:01 pm, Pavel Machek wrote:

> > tell drivers what to do.  Say we use something like PMSG_STANDBY and now
> 
> We would add another field to that struct, distingushing "mem" and
> "standby". 

Just for the record:  I continue to oppose growing pm_message_t.

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Saturday, 24 March 2007 01:01, Pavel Machek wrote:
> Hi!
> 
> (I do not want to get into this flamewar).

I'd rather call it a misunderstanding.

> > > That's a false choice, when you "mean" anything more than
> > > fairly broad behavioral expectations:  STR saves more power
> > > than "standby", and transitions to/from STR take more
> > > time than to/from "standby".
> > 
> > So be it.
> > 
> > Assume that the user does 'echo standby > /sys/power/state'.  I think he can
> > expect that in such a case we'll freeze tasks and put devices into low-power
> > states and when he wakes up the system (BTW, I think the method of waking
> > up can be treated as a differentiating factor) he should be able to continue
> > from where he stopped after a little time.  Fine.
> > 
> > Now, we have to make that happen.  After we have frozen tasks, we need to
> > call something like device_suspend(some_argument) where the argument should
> > tell drivers what to do.  Say we use something like PMSG_STANDBY and now
> 
> We would add another field to that struct, distingushing "mem" and
> "standby". And meaning for the drivers would be "try to save a lot of
> power, but keep the latency low" for standby, vs. "save as much power
> as possible" for mem.

Well, I think we can add more PMSG_* constants just fine.  Still, for a driver,
the meaning of PMSG_WHATEVER should be clear.

I agree that whatever is done in pm_ops->enter(some_state) has to depend on
the platform for quite obvious reasons, but the PMSG_* have some meaning
beyond the platform-specific code.

Greetings,
Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[Dmitry Krivoschekov]

Rafael J. Wysocki wrote:
> On Friday, 23 March 2007 18:57, David Brownell wrote:
>> On Friday 23 March 2007 6:43 am, Rafael J. Wysocki wrote:
>>> On Friday, 23 March 2007 00:55, David Brownell wrote:
>>>> You said that if the hardware doesn't support a "turn CPU off" mode, then
>>>> you'd define that as being incapable of implementing suspend-to-RAM.
>>> That's _if_ the suspend-to-RAM is defined as the state in which the CPU
>>> is off, which I _think_ would be a reasonable definition.  
>> I disagree.
>
> Fine, this only is a matter of opinion. :-)
>  
>>> I don't mean the 
>>> platforms incapable of doing this should be restricted from entering any
>>> system-wide low-power states, but perhaps we can call these states
>>> differently.
>> Well, we have ** ONLY TWO LABELS TO APPLY ** and you're saying that
>> one of them should be restricted to systems where the CPU can go into
>> an "off" state.  
>
> As long as you insist on using two lables only and these two labels in
> particular, then fine.  [Well, in such a case I'd change them to something
> different (like "suspend level 1", "suspend level 2").]
>
> Still, technically it doesn't really matter.  What matters is that we need to
> tell drivers what we expect them to do with the devices when we're
> transitioning from one state to another system-wide.  And that, IMO, should
> be defined.

just my 2 cents...

The following system-wide low power states seem reasonable to me,
at least from embedded system perspective, at least as a reasonable
compromise for now...


Wait          -  CPU is inactive (idle, WFI or whatever lowpower state
                 it can leave immediately (the fastest from supported
mods)),
                 all drivers are active to be immediately ready
                 after CPU gets run

Doze          - The same as Wait mode but devices that pre-configured
                as "must_disable" should  be switched off
                (flag must_disable slould be added to dev_pm_info,
                and maybe appropriate sysfs interface as well)

Sleep         - CPU is in low power state that do not require
                restoring of registers, RAM is
                in self-refresh mode only devices configured
                via "may_wakeup" stay capable to wakeup
                (on various system it depends,
                i.e. the devices should be left active, or may
                be capable to wakeup even in lowpower state ),
                other devices are switched off.


DeepSleep     - the same as sleep but CPU is in the lowest of supported
                power states, RAM is switched off data should be saved
                (to disk,to flash)


Considering system-wide states, I don't think we should take care
of saying to devices which lowpower state they should go to, perhaps
it should be the lowest state (or in "DeepSleep" and "Sleep"
it is the lowest but in "Doze" it is medium).
This system wide states is pretty rough way to control system power,
if fine-grained PM is required then  we should think of another
ways to control it, /sys/power/state seems not the best interface
for this.



Regards,
Dmitry

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Friday 23 March 2007 1:39 pm, Rafael J. Wysocki wrote:

> 	After we have frozen tasks, we need to
> call something like device_suspend(some_argument) where the argument should
> tell drivers what to do. 

That parameter can't suffice, since the exact details depend on
system-dependent context.  Example, on one system a given sleep
state will allow a given device to issue wakeups ... on another,
it won't.

You seem to have overlooked the clk_must_disable() patches I
recently re-sent.  In conjunction with the driver model wakeup
flags, that can solve the problem on every SOC platform I've
had a reason to look at ... see how it works for AT91 USB.

(SOC "power management" documentatation invariably dwells on
clock gating ... and as previously discussed, those platforms
with "power domains", to help manage leakage current not just
clocking, can generally couple them to clocks.  So software
clock gating would normally put controllers into "retention"
mode like PCI D2, or in a few cases into a suspend" mode with
power off like PCI D3cold.  And if the state doesn't require
a given clock to be gated off, then the driver wakeup config
may direct that it's active enough to be a wakeup source.)

That is, we don't need a new method or complexified parameter
to the existing driver->suspend()...


> Say we use something like PMSG_STANDBY and now 
> do you think the meaning of it can change from one platform to another? 

No, let's not complexify that pm_message_t mistake any further.


> And if it can, how can the drivers figure out the meaning?

See summary above.  PCI devices have pci_choose_state(), which
is currently broken but ISTR is sort of fixable -- same notion,
once it does sane things.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

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

On Sat, 2007-03-24 at 13:49 -0700, David Brownell wrote:

> You seem to have overlooked the clk_must_disable() patches I
> recently re-sent.  In conjunction with the driver model wakeup
> flags, that can solve the problem on every SOC platform I've
> had a reason to look at ... see how it works for AT91 USB.

In fact, I looked at those, and it's actually useful on powermac laptops
(powerbooks/ibooks) as well, there are some sound clocks that could be
managed with it.

> No, let's not complexify that pm_message_t mistake any further.

I second that. Drivers can't really know what the precise semantics of
any state are across platforms, especially drivers that are shared
between different platforms but where the hw could be wired up
differently.

johannes

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

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Saturday 24 March 2007 2:01 pm, Johannes Berg wrote:
> On Sat, 2007-03-24 at 13:49 -0700, David Brownell wrote:
> 
> > You seem to have overlooked the clk_must_disable() patches I
> > recently re-sent.  In conjunction with the driver model wakeup
> > flags, that can solve the problem on every SOC platform I've
> > had a reason to look at ... see how it works for AT91 USB.
> 
> In fact, I looked at those, and it's actually useful on powermac laptops
> (powerbooks/ibooks) as well, there are some sound clocks that could be
> managed with it.

Interesting.  I've sort of assumed <linux/clk.h> didn't get much
use outside the embedded arch code (ARM, AVR32, SH pending, ...)
so far ... there's no good technical reason for that of course!


> > No, let's not complexify that pm_message_t mistake any further.
> 
> I second that. Drivers can't really know what the precise semantics of
> any state are across platforms, especially drivers that are shared
> between different platforms but where the hw could be wired up
> differently.

Even within one platform there can be board-specific diferences,
and different chip revisions may work differently.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Saturday, 24 March 2007 21:49, David Brownell wrote:
> On Friday 23 March 2007 1:39 pm, Rafael J. Wysocki wrote:
> 
> > 	After we have frozen tasks, we need to
> > call something like device_suspend(some_argument) where the argument should
> > tell drivers what to do. 
> 
> That parameter can't suffice, since the exact details depend on
> system-dependent context.  Example, on one system a given sleep
> state will allow a given device to issue wakeups ... on another,
> it won't.
> 
> You seem to have overlooked the clk_must_disable() patches I
> recently re-sent.  In conjunction with the driver model wakeup
> flags, that can solve the problem on every SOC platform I've
> had a reason to look at ... see how it works for AT91 USB.

Do I understand correctly that the idea is to introduce some functions
(eg. clk_must_disable()) that each platform can define differently and
that can be used by device drivers to obtain the additional information
needed to figure out what they are expected to do in .suspend()?

Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Saturday 24 March 2007 2:36 pm, Rafael J. Wysocki wrote:
> On Saturday, 24 March 2007 21:49, David Brownell wrote:

> > You seem to have overlooked the clk_must_disable() patches I
> > recently re-sent.  In conjunction with the driver model wakeup
> > flags, that can solve the problem on every SOC platform I've
> > had a reason to look at ... see how it works for AT91 USB.
> 
> Do I understand correctly that the idea is to introduce some functions
> (eg. clk_must_disable()) that each platform can define differently 

The definition is the same, it's the behavior that differs.  :)

That's no change from the rest of the clock framework, of course;
essentially every chip has a unique clock tree, but the interface
to that tree is defined the same on all systems.


> and 
> that can be used by device drivers to obtain the additional information
> needed to figure out what they are expected to do in .suspend()?

Yes.  See how it works for AT91 USB.  In sleep states where the
48 MHz PLL is off, suspend() fully disables host or peripheral
controllers and resume() re-initializes them from scratch.  But
otherwise the controller will often stay partially active, so
that it can be a wakeup event source and/or maintain any current
sessions.

The UART is a funky case; it usually can't adjust its internal
clock divisor (maybe at 1200 baud it could?), but some systems
reprogram RXD as a wakeup GPIO.  That particular patch doesn't
do either of those things -- but that could be done.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Dmitry Krivoschekov]

David Brownell wrote:
> On Friday 23 March 2007 1:39 pm, Rafael J. Wysocki wrote:
>
>> 	After we have frozen tasks, we need to
>> call something like device_suspend(some_argument) where the argument should
>> tell drivers what to do. 
>
> That parameter can't suffice, since the exact details depend on
> system-dependent context.  Example, on one system a given sleep
> state will allow a given device to issue wakeups ... on another,
> it won't.
>
What do you mean? Capability of h/w to be a wakeup source or
design decisions, when wakeup capability of a given device
is disabled intentionally for some reason?

System may have a number of devices that all are able to wakeup
the system from *all* sleep (low power) states the system
supports.Normally, such devices should be marked as "can_wakeup"
to demonstrate the capability, then, user may select these
devices as a wakeup source(s) from a given sleep state.
He(she) sets "should_wakeup" flag for this. For example,
I may want to wakeup the system via USB, MMC and keypad,
but when I put  the system into deeper sleep state I may want
to wakeup it only via keypad and do not bother it via MMC
and USB.


So, it is reasonable idea to permit some devices to be
wakeup sources for one system-wide state but restrict
the wakeup ability for another system-wide state.
But, it seems not quite reasonable to hardcode this
in platform-specific code, unless your platform is
designed for very specific needs. In general,
every wakeup source (which is capable to wakeup
at any system state) should be available via sysfs
(../power/wakeup interface of device)

> You seem to have overlooked the clk_must_disable() patches I
> recently re-sent.  In conjunction with the driver model wakeup
> flags, that can solve the problem on every SOC platform I've
> had a reason to look at ... see how it works for AT91 USB.

As I pointed above, user may want to choose between wakeup
sources and he(she) must be sure the choice won't be ignored,
but your changes for atmel_serial.c and at91_udc.c,
seems restrict user with that.



Thanks,
Dmitry

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Sunday 25 March 2007 3:26 am, Dmitry Krivoschekov wrote:
> David Brownell wrote:
> > On Friday 23 March 2007 1:39 pm, Rafael J. Wysocki wrote:
> >
> >> 	After we have frozen tasks, we need to
> >> call something like device_suspend(some_argument) where the argument should
> >> tell drivers what to do. 
> >
> > That parameter can't suffice, since the exact details depend on
> > system-dependent context.  Example, on one system a given sleep
> > state will allow a given device to issue wakeups ... on another,
> > it won't.
> >
> What do you mean? Capability of h/w to be a wakeup source or
> design decisions, when wakeup capability of a given device
> is disabled intentionally for some reason?

Both ways; device_may_wakeup(dev) is there to flag whether
the driver suspend() method should try to kick in wakeup
machinery, whether the restriction is from hardware or from
software (userspace) policy.


> System may have a number of devices that all are able to wakeup
> the system from *all* sleep (low power) states the system
> supports.Normally, such devices should be marked as "can_wakeup"
> to demonstrate the capability,

That flag doesn't indicate wakeup from "all" states, but
instead from "any" state.  There's no micromanagement
going on... just a simple way for userspace to say whether
they'd like to try using that device's wakeup machinery.

If the device can't wake from the target system state,
the driver won't be able to kick in that machinery.
Hardware trumps software, as it were.


> So, it is reasonable idea to permit some devices to be
> wakeup sources for one system-wide state but restrict
> the wakeup ability for another system-wide state.

That restriction being done in hardware or, if for some
reason userspace wants, by updating the sysfs "wakeup"
attribute for the relevant device before writing to the
/sys/power/state file.


> But, it seems not quite reasonable to hardcode this
> in platform-specific code, unless your platform is
> designed for very specific needs. In general,
> every wakeup source (which is capable to wakeup
> at any system state) should be available via sysfs
> (../power/wakeup interface of device)

Right, which is why I never suggested hardwiring any
of that in platform-specific code.  The only thing
that platform-specific code should handle is whether
the device can be a wakeup event source *at all*, which
is hardware-controlled.


> > You seem to have overlooked the clk_must_disable() patches I
> > recently re-sent.  In conjunction with the driver model wakeup
> > flags, that can solve the problem on every SOC platform I've
> > had a reason to look at ... see how it works for AT91 USB.
> 
> As I pointed above, user may want to choose between wakeup
> sources and he(she) must be sure the choice won't be ignored,

If the user sets a policy the hardware can't implement in
that particular context, then of course that choice can't
apply.  It's not "ignored", just irrelevant.


> but your changes for atmel_serial.c and at91_udc.c,
> seems restrict user with that.

The restriction is from hardware, not software.  And that
patch doesn't change that behavior; it couldn't!  :)

If userspace wants the USB host to be able to wake the
system from sleep, then the simple rule is:  don't use
the suspend-to-RAM mode.  Nothing software does can ever
change that restriction; "slow clock mode" doesn't run
the 48 MHz clock, wakeup doesn't work without that clock.

By forcing the system into suspend-to-RAM mode, rather
than using runtime PM to minimize power usage, userspace
has already said that being fully functional isn't the
top issue just then.

Keep in mind that most users are already trained in that
model.  Even if it didn't already make sense, that's what
MS-Windows has done forever.  Those little checkboxes in
each driver's GUI properties, saying "let it wake up the
system"?  They're boolean, and only reflect the details
of the ACPI tables in so far not having the checkbox if
that device is known to ACPI and isn't in that table.
It's a simple model, easily understood and generalized.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Jim Gettys]

Just to prove that all generalizations have exceptions:

On the OLPC system, we want the wireless (which, unfortunately, is USB
based), to be able to wake the system from suspend to RAM.

Of course, we have arranged there be an out of band signal from the USB
wireless to wake up the system.  So we don't use USB wakeup for that.
			- Jim

On Sun, 2007-03-25 at 08:20 -0700, David Brownell wrote:
> 
> If userspace wants the USB host to be able to wake the
> system from sleep, then the simple rule is:  don't use
> the suspend-to-RAM mode.  Nothing software does can ever
> change that restriction; "slow clock mode" doesn't run
> the 48 MHz clock, wakeup doesn't work without that clock.
> 
> By forcing the system into suspend-to-RAM mode, rather
> than using runtime PM to minimize power usage, userspace
> has already said that being fully functional isn't the
> top issue just then.
> 
> Keep in mind that most users are already trained in that
> model.  Even if it didn't already make sense, that's what
> MS-Windows has done forever.  Those little checkboxes in
> each driver's GUI properties, saying "let it wake up the
> system"?  They're boolean, and only reflect the details
> of the ACPI tables in so far not having the checkbox if
> that device is known to ACPI and isn't in that table.
> It's a simple model, easily understood and generalized.
> 
> - Dave
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
-- 
Jim Gettys
One Laptop Per Child

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Sunday 25 March 2007 9:23 am, Jim Gettys wrote:
> Just to prove that all generalizations have exceptions:
> 
> On the OLPC system, we want the wireless (which, unfortunately, is USB
> based), to be able to wake the system from suspend to RAM.
> 
> Of course, we have arranged there be an out of band signal from the USB
> wireless to wake up the system.  So we don't use USB wakeup for that.

That would be a case of a USB _peripheral_ waking the system,
albeit through agency of the host controller.

You imply that on OLPC, USB remote wakeup doesn't work from
STR ... which isn't the most common setup, though I've seen
it on systems that are particularly aggressive about power
savings.  Turning off VBUS power on USB ports shrinks the
power budget by at least the 0.5 mA per port most suspended
devices may draw, on top of whatever the transceivers draw
and whatever the system needs to monitor the relevant
transceiver state change events ... GPIOs might suffice.

Doesn't seem like much of an exception to me.  The USB stack
still follows USB rules.  The driver for that WLAN adapter can
have platform-specific extensions, no problem; if that's the
usual sort of wakeup irq signal, it's just a case of that
suspend() method having a bit more work to do.

And of course, it's not running on an AT91 system, so those
AT91-specific rules couldn't apply in any case! :)

- Dave


> 			- Jim
> 
> On Sun, 2007-03-25 at 08:20 -0700, David Brownell wrote:
> > 
> > If userspace wants the USB host to be able to wake the
> > system from sleep, then the simple rule is:  don't use
> > the suspend-to-RAM mode.  Nothing software does can ever
> > change that restriction; "slow clock mode" doesn't run
> > the 48 MHz clock, wakeup doesn't work without that clock.

That was with reference to the at91_udc driver, although the
same rule also applies to its USB host (OHCI) too.

Most x86 systems don't have a USB peripheral mode, so they
couldn't be woken up by an external USB host in any case.
The x86 systems generally have a USB host controller, and
it's usually wired so it can be used as a wakeup source by
an external USB peripheral.


> > By forcing the system into suspend-to-RAM mode, rather
> > than using runtime PM to minimize power usage, userspace
> > has already said that being fully functional isn't the
> > top issue just then.
> > 
> > Keep in mind that most users are already trained in that
> > model.  Even if it didn't already make sense, that's what
> > MS-Windows has done forever.  Those little checkboxes in
> > each driver's GUI properties, saying "let it wake up the
> > system"?  They're boolean, and only reflect the details
> > of the ACPI tables in so far not having the checkbox if
> > that device is known to ACPI and isn't in that table.
> > It's a simple model, easily understood and generalized.
> > 
> > - Dave
> > _______________________________________________
> > linux-pm mailing list
> > linux-pm@lists.linux-foundation.org
> > https://lists.linux-foundation.org/mailman/listinfo/linux-pm
> -- 
> Jim Gettys
> One Laptop Per Child
> 
> 

Re: [PATCH] implement pm_ops.valid for everybody

[Matthew Locke]

On Mar 23, 2007, at 6:43 AM, Rafael J. Wysocki wrote:

> On Friday, 23 March 2007 00:55, David Brownell wrote:
>> On Thursday 22 March 2007 4:21 pm, Rafael J. Wysocki wrote:
>>
>>>> My answer:  there is NO value to such an arbitrary restriction.
>>>
>>> I'm not talking on restrictions.
>>
>> You most certainly did talk about them.
>
> No, sorry.  Apparently, I should have said more precisely what I  
> meant. :-)
>
>> You said that if the hardware doesn't support a "turn CPU off"  
>> mode, then
>> you'd define that as being incapable of implementing suspend-to-RAM.
>
> That's _if_ the suspend-to-RAM is defined as the state in which the  
> CPU
> is off, which I _think_ would be a reasonable definition.  I don't  
> mean the
> platforms incapable of doing this should be restricted from  
> entering any
> system-wide low-power states, but perhaps we can call these states
> differently.

Again,  the only reasonable platform independent defintion for STR is  
that memory is powered in some way and contents preserved.  The state  
of the CPU and devices is platform independent.  See my previous  
email for an idea on how to handle it.


>
>> That's a restriction ... a very arbitrary one.
>>
>>
>>> I'm talking on being able to define
>>> _anything_ more precisely then just a low-power system-wide state.
>>
>> Me too.  And I'm trying to convey to you the results of the
>> investigations I did on that topic.  You don't seem to like
>> those results though ...
>>
>>
>>> And let's start from just something, please.  Like STR and  
>>> "standby" to begin
>>> with?  At least on ACPI systems we can distinguish one from the  
>>> other quite
>>> clearly, so why can't we start from that and _then_ generalize?
>>
>> That's exactly what I did.  Looked also at APM, and several
>> different SOC designs (AT91, OMAP1, PXA25x, SA1100, more).
>>
>> The generalization I came up with is what I've described.
>> Namely, that coming up with one definition of those states
>> that can usefully be mapped all platforms is impractical.
>> They're just labels.
>
> Yes.
>
>> The platform implementor can choose two states to implement, but
>> non-x86 hardware states rarely match the expectations of ACPI.
>
> Yes.  I which case I think the states shouldn't be _labeled_ in the  
> same
> way as the "ACPI" states that they are not.
>
> My point is that _if_ we use lables like "standby", "STR", "STD",  
> etc.,
> then they shouldn't mean different things on different platforms.   
> So, either
> we don't use labels at all, or we should know what they mean  
> regardless
> of what platform we're talking about.
>
>> So the fundamental definition needs to be in relative terms,
>> because platform-specific differences otherwise make trouble.
>
> If your point is that lables are impractical for identifying different
> low-power states of different systems, then I agree.
>
> Greetings,
> Rafael
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] implement pm_ops.valid for everybody

[Paul Mackerras]

Rafael J. Wysocki writes:

> > You said that if the hardware doesn't support a "turn CPU off" mode, then
> > you'd define that as being incapable of implementing suspend-to-RAM.
> 
> That's _if_ the suspend-to-RAM is defined as the state in which the CPU
> is off, which I _think_ would be a reasonable definition.  I don't mean the
> platforms incapable of doing this should be restricted from entering any
> system-wide low-power states, but perhaps we can call these states
> differently.

My old powerbook 3400 has a "sleep" mode where the CPU is in sleep
mode, consuming very little power (and I suspect its clock is switched
off), the RAM is kept refreshed, most of the peripherals are switched
off (except that the video chip keeps its register settings), and
wakeup is under the control of the PMU (power management unit).

My G4 powerbook has a "sleep" mode where the CPU is switched off, the
RAM is kept refreshed, most of the peripherals including the video
chip are switched off, and wakeup is under the control of the PMU.

As far as a user is concerned, both machines are doing the same thing
- they're asleep.  Tell me why we should draw a distinction at a
user-visible level between what these machines are doing, when there
is no user-visible difference in behaviour?

Paul.

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Saturday, 24 March 2007 04:08, Paul Mackerras wrote:
> Rafael J. Wysocki writes:
> 
> > > You said that if the hardware doesn't support a "turn CPU off" mode, then
> > > you'd define that as being incapable of implementing suspend-to-RAM.
> > 
> > That's _if_ the suspend-to-RAM is defined as the state in which the CPU
> > is off, which I _think_ would be a reasonable definition.  I don't mean the
> > platforms incapable of doing this should be restricted from entering any
> > system-wide low-power states, but perhaps we can call these states
> > differently.
> 
> My old powerbook 3400 has a "sleep" mode where the CPU is in sleep
> mode, consuming very little power (and I suspect its clock is switched
> off), the RAM is kept refreshed, most of the peripherals are switched
> off (except that the video chip keeps its register settings), and
> wakeup is under the control of the PMU (power management unit).
> 
> My G4 powerbook has a "sleep" mode where the CPU is switched off, the
> RAM is kept refreshed, most of the peripherals including the video
> chip are switched off, and wakeup is under the control of the PMU.
> 
> As far as a user is concerned, both machines are doing the same thing
> - they're asleep.  Tell me why we should draw a distinction at a
> user-visible level between what these machines are doing, when there
> is no user-visible difference in behaviour?

What I have in mind is the level at which we pass some argument (eg.
PMSG_SUSPEND) to a driver's .suspend() routine and this argument should
depend on the low-power state we want to enter and the meaning of it for
the driver should be exatly known.

Greetings,
Rafael

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Thu, 22 Mar 2007, David Brownell wrote:

> On Thursday 22 March 2007 3:10 pm, Rafael J. Wysocki wrote:
> 
> > Well, I think the only clear distinction between the STR and "standby" is the
> > necessity to go through a boot-like procedure in order to resume from the
> > former.
> 
> So what's a "boot-like procedure"?   Ten instructions?  A hundred?
> A thousand?  Ten thousand?  Does it take a certain amount of time?
> Does it perform certain operations?  Does it involve going through
> ACPI (or APM)?  If so, what about the fact that ACPI (or APM) are
> involved in "standby" resumes too (on platforms using them)?

Ok, on ARM / (at least embedded) ppc I would say - going through reset 
vector / toggling reset pin. Do these terms still make sense on 
hyper-threaded-cored-celled CPUs?

Thanks
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 4:29 pm, Guennadi Liakhovetski wrote:
> On Thu, 22 Mar 2007, David Brownell wrote:
> 
> > On Thursday 22 March 2007 3:10 pm, Rafael J. Wysocki wrote:
> > 
> > > Well, I think the only clear distinction between the STR and "standby" is the
> > > necessity to go through a boot-like procedure in order to resume from the
> > > former.
> > 
> > So what's a "boot-like procedure"?   Ten instructions?  A hundred?
> > A thousand?  Ten thousand?  Does it take a certain amount of time?
> > Does it perform certain operations?  Does it involve going through
> > ACPI (or APM)?  If so, what about the fact that ACPI (or APM) are
> > involved in "standby" resumes too (on platforms using them)?
> 
> Ok, on ARM / (at least embedded) ppc I would say - going through reset 
> vector / toggling reset pin.

However, I would also say that it makes plenty of sense to talk about
an STR state that doesn't involve such a reset.


> Do these terms still make sense on  hyper-threaded-cored-celled CPUs?

"Reset", sure ... though there are lots of types of reset.
And "boot-like procedure" is inherently vague though.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Fri, 23 Mar 2007, Guennadi Liakhovetski wrote:

> On Thu, 22 Mar 2007, David Brownell wrote:
> 
> > On Thursday 22 March 2007 3:10 pm, Rafael J. Wysocki wrote:
> > 
> > > Well, I think the only clear distinction between the STR and "standby" is the
> > > necessity to go through a boot-like procedure in order to resume from the
> > > former.
> > 
> > So what's a "boot-like procedure"?   Ten instructions?  A hundred?
> > A thousand?  Ten thousand?  Does it take a certain amount of time?
> > Does it perform certain operations?  Does it involve going through
> > ACPI (or APM)?  If so, what about the fact that ACPI (or APM) are
> > involved in "standby" resumes too (on platforms using them)?
> 
> Ok, on ARM / (at least embedded) ppc I would say - going through reset 
> vector / toggling reset pin. Do these terms still make sense on 
> hyper-threaded-cored-celled CPUs?

Maybe "taking a reset exception" is a slightly better wording for this.

Thanks
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Thursday, 22 March 2007 14:44, Scott E. Preece wrote:
> 
> | From: "Rafael J. Wysocki" <rjw@sisk.pl>
> | 
> | On Wednesday, 21 March 2007 23:57, Pavel Machek wrote:
> | > Hi!
> | > 
> | > > > Which is very much an indication of how weak ACPI is.  It
> | > > > doesn't contemplate typical SOC behavior, which have a wide
> | > > > variety of system sleep states that leave the CPU on ... and
> | > > > which may not even *have* (or need!) a "cpu off" state.
> | > > > 
> | > > > My own definition would be more like:  the minimal RAM-based
> | > > > power-saving system state is "standby".  If the system
> | > > > implements a deeper RAM-based system sleep state, that's "STR".
> | > > 
> | > > Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> | > > implementor, doesn't it?
> | > 
> | > Is that a problem? If someone is clever enough to implement suspend, I
> | > think we can trust them to name their states right.
> | > 
> | > (And trust me, we can flame them if not).
> | > 
> | > (Anyway, my definition would be "mem" == RAM is powered, everything
> | > else is down, except for devices needed for wakeup; "standby" ==
> | > something is powered that can be powered down, we'll fix that in next version).
> | 
> | I think we can define "standby" a bit more precisely.  Something like:
> | - processes are frozen,
> | - devices are suspended,
> | - nonboot CPUs are down (and in low powered states, if possible),
> | - "system" devices may or may not be suspended, depending on the platform,
> | - the boot CPU may or may not be in a low power state, depending on the platform,
> | - RAM is powered
> | - wake up need not be BIOS-driven (main difference from "mem")
> ---
> 
> I would be tempted to say that that last bullet is the distinguishing
> characteristic - that you come back from standby by just continuing
> where you left off, but you come back from StR by something akin to
> booting.

Yes, that's what I meant.

Greetings,
Rafael