[PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[Ulf Hansson]

Patch 1 -> 2:
These patches provides two new runtime PM helper functions which intend to be
used from system suspend/resume callbacks, to make sure devices are put into low
power state during system suspend and brought back to full power at system
resume.

The prerequisite is to have all levels of a device's runtime PM callbacks to be
defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available
for CONFIG_PM.

By using the new runtime PM helper functions especially the two scenarios below
will be addressed.

1) The PM core prevents .runtime_suspend callbacks from being invoked during
system suspend. That means even for a runtime PM centric subsystem and driver,
the device needs to be put into low power state from a system suspend callback.
Otherwise it may very well be left in full power state (runtime resumed) while
the system is suspended. By using the new helper functions, we make sure to walk
the hierarchy of a device's power domain, subsystem and driver.

2) Subsystems and drivers need to cope with all the combinations of
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME. The two new helper functions smothly
addresses this.

Patch 3 -> 4:
Fixup system suspend for spi-pl022 driver, by using the new runtime PM helper
functions.

These patches is based upon the latest next-branch for spi-pl022 driver. They
will not apply to Rafael's linux-pm tree, but are included in this patchset for
reference and to receive comments/acks.

Patch 5:
Fixup system suspend for i2c-nomadik driver, by using the new runtime PM helper
functions.

This patch is based upon the latest next-branch for the i2c-nomadik driver. They
will not apply to Rafael's linux-pm tree, but are included in this patchset for
reference and to receive comments/acks.

Patch 6 -> 8:
Fixup system suspend for mmci driver, by using the new runtime PM helper
functions.


A final note, the amba bus has converted to use the SET_PM_RUNTIME_PM_OPS
which is needed for these patches to work with only CONFIG_PM_SLEEP and not
CONFIG_PM_RUNTIME. Anyway, the patches for the amba bus have recently been
applied by Russell and resides in his ARM tree.


Ulf Hansson (8):
  PM / Runtime: Fetch runtime PM callbacks using a macro
  PM / Sleep / Runtime: Add pm_runtime_suspend|resume_force functions
  spi: pl022: Let runtime PM callbacks be available for CONFIG_PM
  spi: pl022: Don't ignore power domain and amba bus at system suspend
  i2c: nomadik: Fixup system suspend
  mmc: mmci: Mask IRQs for all variants during runtime suspend
  mmc: mmci: Let runtime PM callbacks be available for CONFIG_PM
  mmc: mmci: Put the device into low power state at system suspend

 drivers/base/power/Makefile      |    3 +-
 drivers/base/power/runtime.c     |  143 +++++++++++++++++++++++++++-----------
 drivers/i2c/busses/i2c-nomadik.c |   14 ++--
 drivers/mmc/host/mmci.c          |   60 ++++------------
 drivers/spi/spi-pl022.c          |   58 ++++++----------
 include/linux/pm_runtime.h       |    4 ++
 6 files changed, 151 insertions(+), 131 deletions(-)

-- 
1.7.9.5

[PATCH 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

While fetching the proper runtime PM callback, we walk the hierarchy of
device's power domains, subsystems and drivers.

This is common for rpm_suspend(), rpm_idle() and rpm_resume(). Let's
clean up the code by using a macro that handles this.

Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Alessandro Rubini <rubini@unipv.it>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/runtime.c |   59 ++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 72e00e6..dedbd64 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -13,6 +13,23 @@
 #include <trace/events/rpm.h>
 #include "power.h"
 
+#define RPM_GET_CALLBACK(dev, cb)				\
+({								\
+	if (dev->pm_domain)					\
+		callback = dev->pm_domain->ops.cb;		\
+	else if (dev->type && dev->type->pm)			\
+		callback = dev->type->pm->cb;			\
+	else if (dev->class && dev->class->pm)			\
+		callback = dev->class->pm->cb;			\
+	else if (dev->bus && dev->bus->pm)			\
+		callback = dev->bus->pm->cb;			\
+	else							\
+		callback = NULL;				\
+								\
+	if (!callback && dev->driver && dev->driver->pm)	\
+		callback = dev->driver->pm->cb;			\
+})
+
 static int rpm_resume(struct device *dev, int rpmflags);
 static int rpm_suspend(struct device *dev, int rpmflags);
 
@@ -310,19 +327,7 @@ static int rpm_idle(struct device *dev, int rpmflags)
 
 	dev->power.idle_notification = true;
 
-	if (dev->pm_domain)
-		callback = dev->pm_domain->ops.runtime_idle;
-	else if (dev->type && dev->type->pm)
-		callback = dev->type->pm->runtime_idle;
-	else if (dev->class && dev->class->pm)
-		callback = dev->class->pm->runtime_idle;
-	else if (dev->bus && dev->bus->pm)
-		callback = dev->bus->pm->runtime_idle;
-	else
-		callback = NULL;
-
-	if (!callback && dev->driver && dev->driver->pm)
-		callback = dev->driver->pm->runtime_idle;
+	RPM_GET_CALLBACK(dev, runtime_idle);
 
 	if (callback)
 		retval = __rpm_callback(callback, dev);
@@ -492,19 +497,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
-	if (dev->pm_domain)
-		callback = dev->pm_domain->ops.runtime_suspend;
-	else if (dev->type && dev->type->pm)
-		callback = dev->type->pm->runtime_suspend;
-	else if (dev->class && dev->class->pm)
-		callback = dev->class->pm->runtime_suspend;
-	else if (dev->bus && dev->bus->pm)
-		callback = dev->bus->pm->runtime_suspend;
-	else
-		callback = NULL;
-
-	if (!callback && dev->driver && dev->driver->pm)
-		callback = dev->driver->pm->runtime_suspend;
+	RPM_GET_CALLBACK(dev, runtime_suspend);
 
 	retval = rpm_callback(callback, dev);
 	if (retval)
@@ -724,19 +717,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
 
 	__update_runtime_status(dev, RPM_RESUMING);
 
-	if (dev->pm_domain)
-		callback = dev->pm_domain->ops.runtime_resume;
-	else if (dev->type && dev->type->pm)
-		callback = dev->type->pm->runtime_resume;
-	else if (dev->class && dev->class->pm)
-		callback = dev->class->pm->runtime_resume;
-	else if (dev->bus && dev->bus->pm)
-		callback = dev->bus->pm->runtime_resume;
-	else
-		callback = NULL;
-
-	if (!callback && dev->driver && dev->driver->pm)
-		callback = dev->driver->pm->runtime_resume;
+	RPM_GET_CALLBACK(dev, runtime_resume);
 
 	retval = rpm_callback(callback, dev);
 	if (retval) {
-- 
1.7.9.5

Re: [PATCH 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Josh Cartwright]

On Thu, Feb 20, 2014 at 04:31:13PM +0100, Ulf Hansson wrote:
> While fetching the proper runtime PM callback, we walk the hierarchy of
> device's power domains, subsystems and drivers.
> 
> This is common for rpm_suspend(), rpm_idle() and rpm_resume(). Let's
> clean up the code by using a macro that handles this.
> 
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Alessandro Rubini <rubini@unipv.it>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/runtime.c |   59 ++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 72e00e6..dedbd64 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -13,6 +13,23 @@
>  #include <trace/events/rpm.h>
>  #include "power.h"
>  
> +#define RPM_GET_CALLBACK(dev, cb)				\
> +({								\
> +	if (dev->pm_domain)					\
> +		callback = dev->pm_domain->ops.cb;		\
> +	else if (dev->type && dev->type->pm)			\
> +		callback = dev->type->pm->cb;			\
> +	else if (dev->class && dev->class->pm)			\
> +		callback = dev->class->pm->cb;			\
> +	else if (dev->bus && dev->bus->pm)			\
> +		callback = dev->bus->pm->cb;			\
> +	else							\
> +		callback = NULL;				\
> +								\
> +	if (!callback && dev->driver && dev->driver->pm)	\
> +		callback = dev->driver->pm->cb;			\

Just a matter of style, but toying with the caller's 'callback' variable
is a bit ugly, I'm wondering if it would be cleaner to rework the
statement expression to "return" the callback, which would be used like:

	callback = rpm_get_callback(dev, runtime_idle);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> While fetching the proper runtime PM callback, we walk the hierarchy of
> device's power domains, subsystems and drivers.
>
> This is common for rpm_suspend(), rpm_idle() and rpm_resume(). Let's
> clean up the code by using a macro that handles this.
>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Alessandro Rubini <rubini@unipv.it>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/runtime.c |   59 ++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 72e00e6..dedbd64 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -13,6 +13,23 @@
>  #include <trace/events/rpm.h>
>  #include "power.h"
>  
> +#define RPM_GET_CALLBACK(dev, cb)				\
> +({								\
> +	if (dev->pm_domain)					\
> +		callback = dev->pm_domain->ops.cb;		\
> +	else if (dev->type && dev->type->pm)			\
> +		callback = dev->type->pm->cb;			\
> +	else if (dev->class && dev->class->pm)			\
> +		callback = dev->class->pm->cb;			\
> +	else if (dev->bus && dev->bus->pm)			\
> +		callback = dev->bus->pm->cb;			\
> +	else							\
> +		callback = NULL;				\
> +								\
> +	if (!callback && dev->driver && dev->driver->pm)	\
> +		callback = dev->driver->pm->cb;			\
> +})
> +
>  static int rpm_resume(struct device *dev, int rpmflags);
>  static int rpm_suspend(struct device *dev, int rpmflags);
>  
> @@ -310,19 +327,7 @@ static int rpm_idle(struct device *dev, int rpmflags)
>  
>  	dev->power.idle_notification = true;
>  
> -	if (dev->pm_domain)
> -		callback = dev->pm_domain->ops.runtime_idle;
> -	else if (dev->type && dev->type->pm)
> -		callback = dev->type->pm->runtime_idle;
> -	else if (dev->class && dev->class->pm)
> -		callback = dev->class->pm->runtime_idle;
> -	else if (dev->bus && dev->bus->pm)
> -		callback = dev->bus->pm->runtime_idle;
> -	else
> -		callback = NULL;
> -
> -	if (!callback && dev->driver && dev->driver->pm)
> -		callback = dev->driver->pm->runtime_idle;
> +	RPM_GET_CALLBACK(dev, runtime_idle);

This macro sets the local 'callback' variable, but it's not at all
obvious when reading the code.  macros with side-effects like this are a
major readability problem.  Just use a function.

Kevin

Re: [PATCH 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

On 26 February 2014 16:50, Kevin Hilman <khilman@linaro.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> While fetching the proper runtime PM callback, we walk the hierarchy of
>> device's power domains, subsystems and drivers.
>>
>> This is common for rpm_suspend(), rpm_idle() and rpm_resume(). Let's
>> clean up the code by using a macro that handles this.
>>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: Alessandro Rubini <rubini@unipv.it>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/runtime.c |   59 ++++++++++++++----------------------------
>>  1 file changed, 20 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 72e00e6..dedbd64 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -13,6 +13,23 @@
>>  #include <trace/events/rpm.h>
>>  #include "power.h"
>>
>> +#define RPM_GET_CALLBACK(dev, cb)                            \
>> +({                                                           \
>> +     if (dev->pm_domain)                                     \
>> +             callback = dev->pm_domain->ops.cb;              \
>> +     else if (dev->type && dev->type->pm)                    \
>> +             callback = dev->type->pm->cb;                   \
>> +     else if (dev->class && dev->class->pm)                  \
>> +             callback = dev->class->pm->cb;                  \
>> +     else if (dev->bus && dev->bus->pm)                      \
>> +             callback = dev->bus->pm->cb;                    \
>> +     else                                                    \
>> +             callback = NULL;                                \
>> +                                                             \
>> +     if (!callback && dev->driver && dev->driver->pm)        \
>> +             callback = dev->driver->pm->cb;                 \
>> +})
>> +
>>  static int rpm_resume(struct device *dev, int rpmflags);
>>  static int rpm_suspend(struct device *dev, int rpmflags);
>>
>> @@ -310,19 +327,7 @@ static int rpm_idle(struct device *dev, int rpmflags)
>>
>>       dev->power.idle_notification = true;
>>
>> -     if (dev->pm_domain)
>> -             callback = dev->pm_domain->ops.runtime_idle;
>> -     else if (dev->type && dev->type->pm)
>> -             callback = dev->type->pm->runtime_idle;
>> -     else if (dev->class && dev->class->pm)
>> -             callback = dev->class->pm->runtime_idle;
>> -     else if (dev->bus && dev->bus->pm)
>> -             callback = dev->bus->pm->runtime_idle;
>> -     else
>> -             callback = NULL;
>> -
>> -     if (!callback && dev->driver && dev->driver->pm)
>> -             callback = dev->driver->pm->runtime_idle;
>> +     RPM_GET_CALLBACK(dev, runtime_idle);
>
> This macro sets the local 'callback' variable, but it's not at all
> obvious when reading the code.  macros with side-effects like this are a
> major readability problem.  Just use a function.
>
> Kevin

Thanks for reviewing Kevin. I got the same comment from Josh - I
totally agree with you guys.

There are a v2 submitted of this patch, please have look.

Kind regards
Uffe

[PATCH 2/8] PM / Sleep / Runtime: Add pm_runtime_suspend|resume_force functions

[Ulf Hansson]

This patch provides two new runtime PM helper functions which intend to
be used from system suspend/resume callbacks, to make sure devices are
put into low power state during system suspend and brought back to full
power at system resume.

The prerequisite is to have all levels of a device's runtime PM
callbacks to be defined through the SET_PM_RUNTIME_PM_OPS macro, which
means these are available for CONFIG_PM.

By using the new runtime PM helper functions especially the two
scenarios below will be addressed.

1) The PM core prevents .runtime_suspend callbacks from being invoked
during system suspend. That means even for a runtime PM centric
subsystem and driver, the device needs to be put into low power state
from a system suspend callback. Otherwise it may very well be left in
full power state (runtime resumed) while the system is suspended. By
using the new helper functions, we make sure to walk the hierarchy of
a device's power domain, subsystem and driver.

2) Subsystems and drivers need to cope with all the combinations of
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME. The two new helper functions
smothly addresses this.

Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Alessandro Rubini <rubini@unipv.it>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/Makefile  |    3 +-
 drivers/base/power/runtime.c |   84 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_runtime.h   |    4 ++
 3 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index 2e58ebb..1cb8544 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,5 @@
-obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o
+obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
-obj-$(CONFIG_PM_RUNTIME)	+= runtime.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_OPP)	+= opp.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index dedbd64..7363225 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -30,6 +30,7 @@
 		callback = dev->driver->pm->cb;			\
 })
 
+#ifdef CONFIG_PM_RUNTIME
 static int rpm_resume(struct device *dev, int rpmflags);
 static int rpm_suspend(struct device *dev, int rpmflags);
 
@@ -1382,3 +1383,86 @@ void pm_runtime_remove(struct device *dev)
 	if (dev->power.irq_safe && dev->parent)
 		pm_runtime_put(dev->parent);
 }
+#endif
+
+/**
+ * pm_runtime_force_suspend - Force a device into suspend state if needed.
+ * @dev: Device to suspend.
+ *
+ * Disable runtime PM so we safely can check the device's runtime PM status and
+ * if it is active, invoke it's .runtime_suspend callback to bring it into
+ * suspend state. Keep runtime PM disabled to preserve the state unless we
+ * encounter errors.
+ *
+ * Typically this function may be invoked from a system suspend callback to make
+ * sure the device is put into low power state.
+ */
+int pm_runtime_force_suspend(struct device *dev)
+{
+	int (*callback)(struct device *);
+	int ret = 0;
+
+	pm_runtime_disable(dev);
+
+	/*
+	 * Note that pm_runtime_status_suspended() returns false while
+	 * !CONFIG_PM_RUNTIME, which means the device will be put into low
+	 * power state.
+	 */
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	RPM_GET_CALLBACK(dev, runtime_suspend);
+
+	if (!callback) {
+		ret = -ENOSYS;
+		goto err;
+	}
+
+	ret = callback(dev);
+	if (ret)
+		goto err;
+
+	pm_runtime_set_suspended(dev);
+	return 0;
+err:
+	pm_runtime_enable(dev);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
+
+/**
+ * pm_runtime_force_resume - Force a device into resume state.
+ * @dev: Device to resume.
+ *
+ * Prior invoking this function we expect the user to have brought the device
+ * into low power state by a call to pm_runtime_force_suspend(). Here we reverse
+ * those actions and brings the device into full power. We update the runtime PM
+ * status and re-enables runtime PM.
+ *
+ * Typically this function may be invoked from a system resume callback to make
+ * sure the device is put into full power state.
+ */
+int pm_runtime_force_resume(struct device *dev)
+{
+	int (*callback)(struct device *);
+	int ret = 0;
+
+	RPM_GET_CALLBACK(dev, runtime_resume);
+
+	if (!callback) {
+		ret = -ENOSYS;
+		goto out;
+	}
+
+	ret = callback(dev);
+	if (ret)
+		goto out;
+
+	pm_runtime_set_active(dev);
+	pm_runtime_mark_last_busy(dev);
+out:
+	pm_runtime_enable(dev);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_force_resume);
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 16c9a62..2a5897a 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -26,9 +26,13 @@
 #ifdef CONFIG_PM
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
+extern int pm_runtime_force_suspend(struct device *dev);
+extern int pm_runtime_force_resume(struct device *dev);
 #else
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
+static inline int pm_runtime_force_suspend(struct device *dev) { return 0; }
+static inline int pm_runtime_force_resume(struct device *dev) { return 0; }
 #endif
 
 #ifdef CONFIG_PM_RUNTIME
-- 
1.7.9.5

[PATCH 3/8] spi: pl022: Let runtime PM callbacks be available for CONFIG_PM

[Ulf Hansson]

Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
callbacks. This means the callbacks becomes available for both
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed to handle the
combinations of these scenarios.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi-pl022.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 74a0729..6dfcabf 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2277,7 +2277,7 @@ pl022_remove(struct amba_device *adev)
 	return 0;
 }
 
-#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
+#ifdef CONFIG_PM
 /*
  * These two functions are used from both suspend/resume and
  * the runtime counterparts to handle external resources like
@@ -2343,7 +2343,7 @@ static int pl022_resume(struct device *dev)
 }
 #endif	/* CONFIG_PM */
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 static int pl022_runtime_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
@@ -2363,7 +2363,7 @@ static int pl022_runtime_resume(struct device *dev)
 
 static const struct dev_pm_ops pl022_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
-	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
+	SET_PM_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
 };
 
 static struct vendor_data vendor_arm = {
-- 
1.7.9.5

Re: [PATCH 3/8] spi: pl022: Let runtime PM callbacks be available for CONFIG_PM

[Josh Cartwright]

(comments below only marginally related to this change)

On Thu, Feb 20, 2014 at 04:31:15PM +0100, Ulf Hansson wrote:
> Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
> callbacks. This means the callbacks becomes available for both
> CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed to handle the
> combinations of these scenarios.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/spi/spi-pl022.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index 74a0729..6dfcabf 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -2277,7 +2277,7 @@ pl022_remove(struct amba_device *adev)
>  	return 0;
>  }
>  
> -#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
> +#ifdef CONFIG_PM

I wish we could get rid of PM-related #ifdefery in drivers...

For what it's worth, while working on another project, I discovered that
using a ternary operator in initializers might actually be able to help
out a bit:

	/*
	 * Intended for use in static object initializers,
	 * assign_if(const_expr, function) evaluates to 'function' if
	 * 'const_expr', otherwise NULL.
	 *
	 * The type of the assign_if() expression is typeof(function),
	 * and therefore can provide typechecking regardless of
	 * 'const_expr'.
	 *
	 * gcc considers 'function' to be used and will not generate a
	 * 'defined but not used' when not 'const_expr', however, gcc is
	 * smart enough to eliminate 'function' if assign_if() is the
	 * only reference.
	 */
	#define assign_if(const_expr, function)			\
		((const_expr) ? function : NULL)

	#define assign_if_enabled(option, function)		\
		assign_if(IS_ENABLED(option), function)

	#define assign_if_rpm(function)				\
		assign_if_enabled(CONFIG_PM_RUNTIME, function)

	static int foo_runtime_suspend(struct device *dev)
	{
		return 0;
	}

	static struct dev_pm_ops foo_pm_ops = {
		.runtime_suspend	= assign_if_rpm(foo_runtime_suspend),
	};

So, in this example, wrapping the definition of foo_runtime_suspend in
#ifdef CONFIG_PM_RUNTIME is unnecessary, as gcc will not emit it if
!CONFIG_PM_RUNTIME is not set (at least from my experiments).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 4/8] spi: pl022: Don't ignore power domain and amba bus at system suspend

[Ulf Hansson]

Previously only the resources controlled by the driver were put into
low power state at system suspend. Both the amba bus and a potential
power domain were ignored.

Moreover, while putting the device into low power state we first
brought it back to full power, but for no particular reason.

To handle both issues above, use pm_runtime_force_suspend|resume() from
the system suspend|resume callbacks.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi-pl022.c |   54 ++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 6dfcabf..d37e840 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2277,35 +2277,7 @@ pl022_remove(struct amba_device *adev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-/*
- * These two functions are used from both suspend/resume and
- * the runtime counterparts to handle external resources like
- * clocks, pins and regulators when going to sleep.
- */
-static void pl022_suspend_resources(struct pl022 *pl022, bool runtime)
-{
-	clk_disable_unprepare(pl022->clk);
-
-	if (runtime)
-		pinctrl_pm_select_idle_state(&pl022->adev->dev);
-	else
-		pinctrl_pm_select_sleep_state(&pl022->adev->dev);
-}
-
-static void pl022_resume_resources(struct pl022 *pl022, bool runtime)
-{
-	/* First go to the default state */
-	pinctrl_pm_select_default_state(&pl022->adev->dev);
-	if (!runtime)
-		/* Then let's idle the pins until the next transfer happens */
-		pinctrl_pm_select_idle_state(&pl022->adev->dev);
-
-	clk_prepare_enable(pl022->clk);
-}
-#endif
-
-#ifdef CONFIG_SUSPEND
+#ifdef CONFIG_PM_SLEEP
 static int pl022_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
@@ -2317,8 +2289,13 @@ static int pl022_suspend(struct device *dev)
 		return ret;
 	}
 
-	pm_runtime_get_sync(dev);
-	pl022_suspend_resources(pl022, false);
+	ret = pm_runtime_force_suspend(dev);
+	if (ret) {
+		spi_master_resume(pl022->master);
+		return ret;
+	}
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	dev_dbg(dev, "suspended\n");
 	return 0;
@@ -2329,8 +2306,9 @@ static int pl022_resume(struct device *dev)
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 	int ret;
 
-	pl022_resume_resources(pl022, false);
-	pm_runtime_put(dev);
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		dev_err(dev, "problem resuming\n");
 
 	/* Start the queue running */
 	ret = spi_master_resume(pl022->master);
@@ -2341,14 +2319,16 @@ static int pl022_resume(struct device *dev)
 
 	return ret;
 }
-#endif	/* CONFIG_PM */
+#endif
 
 #ifdef CONFIG_PM
 static int pl022_runtime_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
-	pl022_suspend_resources(pl022, true);
+	clk_disable_unprepare(pl022->clk);
+	pinctrl_pm_select_idle_state(dev);
+
 	return 0;
 }
 
@@ -2356,7 +2336,9 @@ static int pl022_runtime_resume(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
-	pl022_resume_resources(pl022, true);
+	pinctrl_pm_select_default_state(dev);
+	clk_prepare_enable(pl022->clk);
+
 	return 0;
 }
 #endif
-- 
1.7.9.5

[PATCH 5/8] i2c: nomadik: Fixup system suspend

[Ulf Hansson]

For !CONFIG_PM_RUNTIME, the device were never put back into active
state while resuming.

For CONFIG_PM_RUNTIME, we blindly trusted the device to be inactive
while we were about to handle it at suspend late, which is just too
optimistic.

Even if the driver uses pm_runtime_put_sync() after each tranfer to
return it's runtime PM resources, there are no guarantees this will
actually mean the device will inactivated. The reason is that the PM
core will prevent runtime suspend during system suspend, and thus when
a transfer occurs during the early phases of system suspend the device
will be kept active after the transfer.

To handle both issues above, use pm_runtime_force_suspend|resume() from
the system suspend|resume callbacks.

Cc: Alessandro Rubini <rubini@unipv.it>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 8082f5c..519f5b8 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -879,19 +879,19 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 #ifdef CONFIG_PM_SLEEP
 static int nmk_i2c_suspend_late(struct device *dev)
 {
-	pinctrl_pm_select_sleep_state(dev);
+	int ret;
 
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
 	return 0;
 }
 
 static int nmk_i2c_resume_early(struct device *dev)
 {
-	/* First go to the default state */
-	pinctrl_pm_select_default_state(dev);
-	/* Then let's idle the pins until the next transfer happens */
-	pinctrl_pm_select_idle_state(dev);
-
-	return 0;
+	return pm_runtime_force_resume(dev);
 }
 #endif
 
-- 
1.7.9.5

[PATCH 6/8] mmc: mmci: Mask IRQs for all variants during runtime suspend

[Ulf Hansson]

In runtime suspended state, we are not expecting IRQs and thus we can
safely mask them, not only for pwrreg_nopower variants but for all.

Obviously we then also need to make sure we restore the IRQ mask while
becoming runtime resumed.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b931226..178868a 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1758,35 +1758,34 @@ static void mmci_save(struct mmci_host *host)
 {
 	unsigned long flags;
 
-	if (host->variant->pwrreg_nopower) {
-		spin_lock_irqsave(&host->lock, flags);
+	spin_lock_irqsave(&host->lock, flags);
 
-		writel(0, host->base + MMCIMASK0);
+	writel(0, host->base + MMCIMASK0);
+	if (host->variant->pwrreg_nopower) {
 		writel(0, host->base + MMCIDATACTRL);
 		writel(0, host->base + MMCIPOWER);
 		writel(0, host->base + MMCICLOCK);
-		mmci_reg_delay(host);
-
-		spin_unlock_irqrestore(&host->lock, flags);
 	}
+	mmci_reg_delay(host);
 
+	spin_unlock_irqrestore(&host->lock, flags);
 }
 
 static void mmci_restore(struct mmci_host *host)
 {
 	unsigned long flags;
 
-	if (host->variant->pwrreg_nopower) {
-		spin_lock_irqsave(&host->lock, flags);
+	spin_lock_irqsave(&host->lock, flags);
 
+	if (host->variant->pwrreg_nopower) {
 		writel(host->clk_reg, host->base + MMCICLOCK);
 		writel(host->datactrl_reg, host->base + MMCIDATACTRL);
 		writel(host->pwr_reg, host->base + MMCIPOWER);
-		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
-		mmci_reg_delay(host);
-
-		spin_unlock_irqrestore(&host->lock, flags);
 	}
+	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+	mmci_reg_delay(host);
+
+	spin_unlock_irqrestore(&host->lock, flags);
 }
 
 static int mmci_runtime_suspend(struct device *dev)
-- 
1.7.9.5

[PATCH 7/8] mmc: mmci: Let runtime PM callbacks be available for CONFIG_PM

[Ulf Hansson]

Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
callbacks. This means the callbacks becomes available for both
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed to handle the
combinations of these scenarios.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 178868a..c88da1c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1753,7 +1753,7 @@ static int mmci_resume(struct device *dev)
 }
 #endif
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 static void mmci_save(struct mmci_host *host)
 {
 	unsigned long flags;
@@ -1821,7 +1821,7 @@ static int mmci_runtime_resume(struct device *dev)
 
 static const struct dev_pm_ops mmci_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume)
-	SET_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
+	SET_PM_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
 };
 
 static struct amba_id mmci_ids[] = {
-- 
1.7.9.5

[PATCH 8/8] mmc: mmci: Put the device into low power state at system suspend

[Ulf Hansson]

For CONFIG_PM_SLEEP, the device were always left in full power state
after system suspend.

We solely relied on a power domain to put it into low power state,
which is an unreasonable requirement to put on SOCs to implement.
Especially for those SOCs not supporting power domains at all.

Use pm_runtime_force_suspend|resume() as the system suspend callbacks,
to resolve the issue.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |   33 ++-------------------------------
 1 file changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c88da1c..1432ae2 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1723,36 +1723,6 @@ static int mmci_remove(struct amba_device *dev)
 	return 0;
 }
 
-#ifdef CONFIG_SUSPEND
-static int mmci_suspend(struct device *dev)
-{
-	struct amba_device *adev = to_amba_device(dev);
-	struct mmc_host *mmc = amba_get_drvdata(adev);
-
-	if (mmc) {
-		struct mmci_host *host = mmc_priv(mmc);
-		pm_runtime_get_sync(dev);
-		writel(0, host->base + MMCIMASK0);
-	}
-
-	return 0;
-}
-
-static int mmci_resume(struct device *dev)
-{
-	struct amba_device *adev = to_amba_device(dev);
-	struct mmc_host *mmc = amba_get_drvdata(adev);
-
-	if (mmc) {
-		struct mmci_host *host = mmc_priv(mmc);
-		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
-		pm_runtime_put(dev);
-	}
-
-	return 0;
-}
-#endif
-
 #ifdef CONFIG_PM
 static void mmci_save(struct mmci_host *host)
 {
@@ -1820,7 +1790,8 @@ static int mmci_runtime_resume(struct device *dev)
 #endif
 
 static const struct dev_pm_ops mmci_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
 	SET_PM_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
 };
 
-- 
1.7.9.5

Re: [PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> Patch 1 -> 2:
> These patches provides two new runtime PM helper functions which intend to be
> used from system suspend/resume callbacks, to make sure devices are put into low
> power state during system suspend and brought back to full power at system
> resume.
>
> The prerequisite is to have all levels of a device's runtime PM callbacks to be
> defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available
> for CONFIG_PM.
>
> By using the new runtime PM helper functions especially the two scenarios below
> will be addressed.
>
> 1) The PM core prevents .runtime_suspend callbacks from being invoked during
> system suspend. That means even for a runtime PM centric subsystem and driver,
> the device needs to be put into low power state from a system suspend callback.
> Otherwise it may very well be left in full power state (runtime resumed) while
> the system is suspended. By using the new helper functions, we make sure to walk
> the hierarchy of a device's power domain, subsystem and driver.

I thought it was the case that runtime PM was only disabled during the
'late' phase now.  Isn't that enough to allow runtime callbacks in the
normal suspend/resume hooks now?   /me looks.

oh, wait.  Ee still have the _get_noresume() in device_prepare().  hmm

Either way, I'm not not a big fan of new functions.  Personally, I think
subsystems/busses/pm_domains should be able to opt out of the PM core
behavior that blocks runtime PM transitions during system suspend.
Something like the (untested) hack below.  That way, we could avoid the
need for new helper functions.

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1b41fca3d65a..e0770009ba8e 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -832,7 +832,8 @@ static void device_complete(struct device *dev, pm_message_t state)
 
 	device_unlock(dev);
 
-	pm_runtime_put(dev);
+	if (dev->power.block_rpm_during_suspend)
+		pm_runtime_put(dev);
 }
 
 /**
@@ -1318,7 +1319,8 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	 * block runtime suspend here, during the prepare phase, and allow
 	 * it again during the complete phase.
 	 */
-	pm_runtime_get_noresume(dev);
+	if (dev->power.block_rpm_during_suspend)
+		pm_runtime_get_noresume(dev);
 
 	device_lock(dev);
 
@@ -1350,7 +1352,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
 
 	device_unlock(dev);
 
-	if (error)
+	if (error && dev->power.block_rpm_during_suspend)
 		pm_runtime_put(dev);
 
 	return error;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 8c6583a53a06..692cd543b71d 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -551,6 +551,7 @@ struct dev_pm_info {
 	struct wakeup_source	*wakeup;
 	bool			wakeup_path:1;
 	bool			syscore:1;
+	unsigned int            block_rpm_during_suspend:1;
 #else
 	unsigned int		should_wakeup:1;
 #endif

Kevin

Re: [PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[Ulf Hansson]

On 26 February 2014 17:30, Kevin Hilman <khilman@linaro.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> Patch 1 -> 2:
>> These patches provides two new runtime PM helper functions which intend to be
>> used from system suspend/resume callbacks, to make sure devices are put into low
>> power state during system suspend and brought back to full power at system
>> resume.
>>
>> The prerequisite is to have all levels of a device's runtime PM callbacks to be
>> defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available
>> for CONFIG_PM.
>>
>> By using the new runtime PM helper functions especially the two scenarios below
>> will be addressed.
>>
>> 1) The PM core prevents .runtime_suspend callbacks from being invoked during
>> system suspend. That means even for a runtime PM centric subsystem and driver,
>> the device needs to be put into low power state from a system suspend callback.
>> Otherwise it may very well be left in full power state (runtime resumed) while
>> the system is suspended. By using the new helper functions, we make sure to walk
>> the hierarchy of a device's power domain, subsystem and driver.
>
> I thought it was the case that runtime PM was only disabled during the
> 'late' phase now.  Isn't that enough to allow runtime callbacks in the
> normal suspend/resume hooks now?   /me looks.

I am not sure, I get your point here.

The PM core disables runtime PM at suspend_late. That is somewhat not
related to this patch, since the helper functions are supposed to work
standalone. Subsystem/drivers will in some cases need to invoke the
helper functions in an earlier phase than suspend_late, thus those can
not rely on runtime PM to be disabled, but need to handle that
themselves.

>
> oh, wait.  Ee still have the _get_noresume() in device_prepare().  hmm
>
> Either way, I'm not not a big fan of new functions.  Personally, I think
> subsystems/busses/pm_domains should be able to opt out of the PM core
> behavior that blocks runtime PM transitions during system suspend.
> Something like the (untested) hack below.  That way, we could avoid the
> need for new helper functions.
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 1b41fca3d65a..e0770009ba8e 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -832,7 +832,8 @@ static void device_complete(struct device *dev, pm_message_t state)
>
>         device_unlock(dev);
>
> -       pm_runtime_put(dev);
> +       if (dev->power.block_rpm_during_suspend)
> +               pm_runtime_put(dev);
>  }
>
>  /**
> @@ -1318,7 +1319,8 @@ static int device_prepare(struct device *dev, pm_message_t state)
>          * block runtime suspend here, during the prepare phase, and allow
>          * it again during the complete phase.
>          */
> -       pm_runtime_get_noresume(dev);
> +       if (dev->power.block_rpm_during_suspend)
> +               pm_runtime_get_noresume(dev);
>
>         device_lock(dev);
>
> @@ -1350,7 +1352,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
>
>         device_unlock(dev);
>
> -       if (error)
> +       if (error && dev->power.block_rpm_during_suspend)
>                 pm_runtime_put(dev);
>
>         return error;
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 8c6583a53a06..692cd543b71d 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -551,6 +551,7 @@ struct dev_pm_info {
>         struct wakeup_source    *wakeup;
>         bool                    wakeup_path:1;
>         bool                    syscore:1;
> +       unsigned int            block_rpm_during_suspend:1;
>  #else
>         unsigned int            should_wakeup:1;
>  #endif

This approach is what I initially started with earlier this autumn -
primarily to kick off the discussion. :-)

This is not the way to go, there are several reasons. Alan and Rafael,
should be given cred for being so patient with me while the pointed
out the reasons. Please have look at the discussion below.

http://marc.info/?t=138436024400005&r=1&w=2

Kind regards
Uffe

>
> Kevin

Re: [PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[Rafael J. Wysocki]

On Wednesday, February 26, 2014 08:30:07 AM Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
> > Patch 1 -> 2:
> > These patches provides two new runtime PM helper functions which intend to be
> > used from system suspend/resume callbacks, to make sure devices are put into low
> > power state during system suspend and brought back to full power at system
> > resume.
> >
> > The prerequisite is to have all levels of a device's runtime PM callbacks to be
> > defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available
> > for CONFIG_PM.
> >
> > By using the new runtime PM helper functions especially the two scenarios below
> > will be addressed.
> >
> > 1) The PM core prevents .runtime_suspend callbacks from being invoked during
> > system suspend. That means even for a runtime PM centric subsystem and driver,
> > the device needs to be put into low power state from a system suspend callback.
> > Otherwise it may very well be left in full power state (runtime resumed) while
> > the system is suspended. By using the new helper functions, we make sure to walk
> > the hierarchy of a device's power domain, subsystem and driver.
> 
> I thought it was the case that runtime PM was only disabled during the
> 'late' phase now.  Isn't that enough to allow runtime callbacks in the
> normal suspend/resume hooks now?   /me looks.
> 
> oh, wait.  Ee still have the _get_noresume() in device_prepare().  hmm
> 
> Either way, I'm not not a big fan of new functions.  Personally, I think
> subsystems/busses/pm_domains should be able to opt out of the PM core
> behavior that blocks runtime PM transitions during system suspend.
> Something like the (untested) hack below.  That way, we could avoid the
> need for new helper functions.

And if one of the subsystems in question is the platform bus type, then
adding the flag doesn't really make sense, because that means that on
many system it will be set for the majority of devices. :-)

That said I'm tired of this stuff already.  If you really really want to
remove the bumping up and dropping of the usage counter during system
suspend/resume by the core, please feel free to submit a patch for that
and I'll apply it.  However, if it causes any regressions to happen
anywhere, it will be dropped and we'll never talk about this again.

Deal? 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[Ulf Hansson]

On 27 February 2014 02:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, February 26, 2014 08:30:07 AM Kevin Hilman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>> > Patch 1 -> 2:
>> > These patches provides two new runtime PM helper functions which intend to be
>> > used from system suspend/resume callbacks, to make sure devices are put into low
>> > power state during system suspend and brought back to full power at system
>> > resume.
>> >
>> > The prerequisite is to have all levels of a device's runtime PM callbacks to be
>> > defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available
>> > for CONFIG_PM.
>> >
>> > By using the new runtime PM helper functions especially the two scenarios below
>> > will be addressed.
>> >
>> > 1) The PM core prevents .runtime_suspend callbacks from being invoked during
>> > system suspend. That means even for a runtime PM centric subsystem and driver,
>> > the device needs to be put into low power state from a system suspend callback.
>> > Otherwise it may very well be left in full power state (runtime resumed) while
>> > the system is suspended. By using the new helper functions, we make sure to walk
>> > the hierarchy of a device's power domain, subsystem and driver.
>>
>> I thought it was the case that runtime PM was only disabled during the
>> 'late' phase now.  Isn't that enough to allow runtime callbacks in the
>> normal suspend/resume hooks now?   /me looks.
>>
>> oh, wait.  Ee still have the _get_noresume() in device_prepare().  hmm
>>
>> Either way, I'm not not a big fan of new functions.  Personally, I think
>> subsystems/busses/pm_domains should be able to opt out of the PM core
>> behavior that blocks runtime PM transitions during system suspend.
>> Something like the (untested) hack below.  That way, we could avoid the
>> need for new helper functions.
>
> And if one of the subsystems in question is the platform bus type, then
> adding the flag doesn't really make sense, because that means that on
> many system it will be set for the majority of devices. :-)
>
> That said I'm tired of this stuff already.  If you really really want to
> remove the bumping up and dropping of the usage counter during system
> suspend/resume by the core, please feel free to submit a patch for that
> and I'll apply it.  However, if it causes any regressions to happen
> anywhere, it will be dropped and we'll never talk about this again.
>
> Deal?

No deal. I prefer the runtime PM helpers, I think that is the right
approach. I also believe this is what Alan Stern also would prefer,
right!? So let's convince Kevin instead. :-)

Let's just be clear of why I don't think Kevin suggested solution is
the way to go:

1. The runtime PM sysfs interface. Userspace may prevent
->runtime_suspend callbacks from being invoked anyway. So, it doesn't
matter if PM core does it too.

2. Currently we have CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP. Subsystem
and driver, must cope with both. I know there were a discussion about
combining them as one define, but we rejected that - at least for now.
Anyway, my point is, subsystem/driver must not rely on PM runtime core
(like pm_runtime_put_sync) from the system suspend callback to put
their devices into low power state.

Kind regards
Uffe

>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/8] PM / Sleep / Runtime: Fixup some driver's system suspend

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 27 February 2014 02:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, February 26, 2014 08:30:07 AM Kevin Hilman wrote:
>>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>>
>>> > Patch 1 -> 2:
>>> > These patches provides two new runtime PM helper functions which intend to be
>>> > used from system suspend/resume callbacks, to make sure devices are put into low
>>> > power state during system suspend and brought back to full power at system
>>> > resume.
>>> >
>>> > The prerequisite is to have all levels of a device's runtime PM callbacks to be
>>> > defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available
>>> > for CONFIG_PM.
>>> >
>>> > By using the new runtime PM helper functions especially the two scenarios below
>>> > will be addressed.
>>> >
>>> > 1) The PM core prevents .runtime_suspend callbacks from being invoked during
>>> > system suspend. That means even for a runtime PM centric subsystem and driver,
>>> > the device needs to be put into low power state from a system suspend callback.
>>> > Otherwise it may very well be left in full power state (runtime resumed) while
>>> > the system is suspended. By using the new helper functions, we make sure to walk
>>> > the hierarchy of a device's power domain, subsystem and driver.
>>>
>>> I thought it was the case that runtime PM was only disabled during the
>>> 'late' phase now.  Isn't that enough to allow runtime callbacks in the
>>> normal suspend/resume hooks now?   /me looks.
>>>
>>> oh, wait.  Ee still have the _get_noresume() in device_prepare().  hmm
>>>
>>> Either way, I'm not not a big fan of new functions.  Personally, I think
>>> subsystems/busses/pm_domains should be able to opt out of the PM core
>>> behavior that blocks runtime PM transitions during system suspend.
>>> Something like the (untested) hack below.  That way, we could avoid the
>>> need for new helper functions.
>>
>> And if one of the subsystems in question is the platform bus type, then
>> adding the flag doesn't really make sense, because that means that on
>> many system it will be set for the majority of devices. :-)
>>
>> That said I'm tired of this stuff already.  If you really really want to
>> remove the bumping up and dropping of the usage counter during system
>> suspend/resume by the core, please feel free to submit a patch for that
>> and I'll apply it.  However, if it causes any regressions to happen
>> anywhere, it will be dropped and we'll never talk about this again.
>>
>> Deal?
>
> No deal. I prefer the runtime PM helpers, I think that is the right
> approach. I also believe this is what Alan Stern also would prefer,
> right!? So let's convince Kevin instead. :-)

I don't need much convincing.  

My preference is for fewer functions/helpers because runtime PM is
already complicated enough for drivers.  However, I'm not going to
object to the helpers because they will allow us to simplify many
drivers/subsystems that are trying to handle the various combinations of
suspend/resume and runtime PM, so it's a step in the right direction.

Kevin