Re: platform/i2c busses: pm runtime and system sleep

On Fri, Dec 17, 2010 at 05:39, Rafael J. Wysocki [off-list ref] wrote:

On Thursday, December 16, 2010, Rabin Vincent wrote:

quoted

There seem to be some differences between the generic ops and the i2c
and platform busses' implementations of the interaction between runtime
PM and system sleep:

? (1) The platform bus does not implement the
? ? ? don't-call-pm->suspend()-if pm_runtime_suspended()-returns-true
? ? ? functionality implemented by the generic ops and i2c.

? (2) Both I2C and platform do not set the device as active when a
? ? ? pm->resume callback exists and it succeeds.

...

quoted

Are these divergences from the generic ops to be considered as bugs?

I think so. ?I'm not sure about (1), because someone may already depend on
that behavior, but (2) looks like a bug to me.

Revisiting these points again.  (2) has since been corrected for i2c,
but platform does not do (1) and (2).

I've submitted a patch today to convert the AMBA bus to support pm-ops,
and it was convenient to just use the GENERIC_SUBSYS_PM_OPS.  But some
ARM SoCs have a combination of AMBA and platform devices for the on-chip
devices so having different behaviour between the interaction of
runtime-pm and system suspend callbacks does not seem like an ideal
situation, and would only serve to confuse driver writers.  So, should I
just not use GENERIC_SUBSYS_PM_OPS in the AMBA bus but instead open-code
the rountines to make it work like platform?

This will solve the platform vs AMBA bus, but shouldn't we really be
aiming for consistent behaviour between these and the other busses such
as I2C and SPI, which are also usually commonly used on the same
platforms and are using GENERIC_PM_OPS?

Should we be auditing all platform drivers and then switch platform to
the GENERIC_PM_OPS?

Or should the two points (1) and (2) be not handled in the bus at all
and be left to individual drivers (in which case we should audit i2c and
spi and change GENERIC_PM_OPS)?

On Thu, Feb 17, 2011 at 20:55, Rabin Vincent [off-list ref] wrote:

This will solve the platform vs AMBA bus, but shouldn't we really be
aiming for consistent behaviour between these and the other busses such
as I2C and SPI, which are also usually commonly used on the same
platforms and are using GENERIC_PM_OPS?

Should we be auditing all platform drivers and then switch platform to
the GENERIC_PM_OPS?

Or should the two points (1) and (2) be not handled in the bus at all
and be left to individual drivers (in which case we should audit i2c and
spi and change GENERIC_PM_OPS)?

How about something like the below?  If we have something like this, we
can just switch platform to GENERIC_PM_OPS and add the
pm_runtime_want_interaction() (or something better named) call to the
i2c and spi drivers using runtime PM.

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 42f97f9..c2a3b63 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c

@@ -87,7 +87,10 @@ static int __pm_generic_call(struct device *dev, int event)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int (*callback)(struct device *);

-	if (!pm || pm_runtime_suspended(dev))
+	if (!pm)
+		return 0;
+
+	if (device_want_interaction(dev) && pm_runtime_suspended(dev))
 		return 0;

 	switch (event) {

@@ -185,7 +188,7 @@ static int __pm_generic_resume(struct device *dev,

int event)
 		return 0;

 	ret = callback(dev);
-	if (!ret && pm_runtime_enabled(dev)) {
+	if (!ret && device_want_interaction(dev) && pm_runtime_enabled(dev)) {
 		pm_runtime_disable(dev);
 		pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 42615b4..2b8a099 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c

@@ -1069,6 +1069,30 @@ void pm_runtime_allow(struct device *dev)
 EXPORT_SYMBOL_GPL(pm_runtime_allow);

 /**
+ * pm_runtime_want_interaction - Enable interaction between system sleep
+ *				 and runtime PM callbacks at the bus/subsystem
+ *				 level.
+ * @dev: Device to handle
+ *
+ * Set the power.want_interaction flage, which tells the generic PM subsystem
+ * ops that the following actions should be done during system suspend/resume:
+ *
+ * - If the device has been runtime suspended, the driver's
+ *   suspend() handler will not be invoked.
+ *
+ * - If the device has a resume() pm callback, and the resume()
+ *   callback returns success on system resume, the device's
+ *   runtime PM status will be set to active.
+ */
+void pm_runtime_want_interaction(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.want_interaction = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_want_interaction);
+
+/**
  * pm_runtime_no_callbacks - Ignore run-time PM callbacks for a device.
  * @dev: Device to handle.
  *

diff --git a/include/linux/pm.h b/include/linux/pm.h
index dd9c7ab..b9bcfb9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h

@@ -450,6 +450,7 @@ struct dev_pm_info {
 	unsigned int		irq_safe:1;
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
+	unsigned int		want_interaction:1;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
 	int			runtime_error;

diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index d34f067..a0e081b 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h

@@ -44,6 +44,7 @@ extern void pm_runtime_irq_safe(struct device *dev);
 extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
 extern void pm_runtime_set_autosuspend_delay(struct device *dev, int delay);
 extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
+static void pm_runtime_want_system_sleep_interaction(struct device *dev);

 static inline bool pm_children_suspended(struct device *dev)
 {

@@ -66,6 +67,11 @@ static inline void pm_runtime_put_noidle(struct device *dev)
 	atomic_add_unless(&dev->power.usage_count, -1, 0);
 }

+static inline bool device_want_interaction(struct device *dev)
+{
+	return dev->power.want_interaction;
+}
+
 static inline bool device_run_wake(struct device *dev)
 {
 	return dev->power.run_wake;

@@ -122,6 +128,7 @@ static inline bool pm_children_suspended(struct

device *dev) { return false; }
 static inline void pm_suspend_ignore_children(struct device *dev, bool en) {}
 static inline void pm_runtime_get_noresume(struct device *dev) {}
 static inline void pm_runtime_put_noidle(struct device *dev) {}
+static inline bool device_want_interaction(struct device *dev) {
return false; }
 static inline bool device_run_wake(struct device *dev) { return false; }
 static inline void device_set_run_wake(struct device *dev, bool enable) {}
 static inline bool pm_runtime_suspended(struct device *dev) { return false; }

@@ -132,6 +139,7 @@ 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 void pm_runtime_no_callbacks(struct device *dev) {}
 static inline void pm_runtime_irq_safe(struct device *dev) {}
+static inline void pm_runtime_want_system_sleep_interaction(struct
device *dev) {}

 static inline void pm_runtime_mark_last_busy(struct device *dev) {}
 static inline void __pm_runtime_use_autosuspend(struct device *dev,

On Fri, 18 Feb 2011, Rabin Vincent wrote:

How about something like the below?  If we have something like this, we
can just switch platform to GENERIC_PM_OPS and add the
pm_runtime_want_interaction() (or something better named)

Yes, please find a better name!  Maybe something starting with 
"generic_" to indicate that this applies only to devices using the 
GENERIC_PM_OPS.

 call to the
i2c and spi drivers using runtime PM.

This last part is normally true for all devices.  If you don't want it 
to hold when want_interaction isn't set, you should add a good 
explanation to sections 6 and 7 in Documentation/power/runtime.txt.

+void pm_runtime_want_interaction(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.want_interaction = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_want_interaction);
+
+/**
  * pm_runtime_no_callbacks - Ignore run-time PM callbacks for a device.
  * @dev: Device to handle.
  *

Don't forget that you also need to describe these things in 
Documentation/power/runtime.txt.

Alan Stern

On Friday, February 18, 2011, Rabin Vincent wrote:

On Thu, Feb 17, 2011 at 20:55, Rabin Vincent [off-list ref] wrote:

quoted

This will solve the platform vs AMBA bus, but shouldn't we really be
aiming for consistent behaviour between these and the other busses such
as I2C and SPI, which are also usually commonly used on the same
platforms and are using GENERIC_PM_OPS?

Should we be auditing all platform drivers and then switch platform to
the GENERIC_PM_OPS?

Or should the two points (1) and (2) be not handled in the bus at all
and be left to individual drivers (in which case we should audit i2c and
spi and change GENERIC_PM_OPS)?

How about something like the below?  If we have something like this, we
can just switch platform to GENERIC_PM_OPS and add the
pm_runtime_want_interaction() (or something better named) call to the
i2c and spi drivers using runtime PM.

Why don't we make platform_bus_type behave along the lines of generic ops
instead?

Rafael

On Fri, Feb 18, 2011 at 23:58, Rafael J. Wysocki [off-list ref] wrote:

On Friday, February 18, 2011, Rabin Vincent wrote:

quoted

On Thu, Feb 17, 2011 at 20:55, Rabin Vincent [off-list ref] wrote:

quoted

This will solve the platform vs AMBA bus, but shouldn't we really be
aiming for consistent behaviour between these and the other busses such
as I2C and SPI, which are also usually commonly used on the same
platforms and are using GENERIC_PM_OPS?

Should we be auditing all platform drivers and then switch platform to
the GENERIC_PM_OPS?

Or should the two points (1) and (2) be not handled in the bus at all
and be left to individual drivers (in which case we should audit i2c and
spi and change GENERIC_PM_OPS)?

How about something like the below? ?If we have something like this, we
can just switch platform to GENERIC_PM_OPS and add the
pm_runtime_want_interaction() (or something better named) call to the
i2c and spi drivers using runtime PM.

Why don't we make platform_bus_type behave along the lines of generic ops
instead?

At least drivers/spi/omap2_mcspi.c, drivers/video/sh_mobile_lcdcfb.c and
drivers/watchdog/omap_wdt.c are some pm_runtime-using drivers which seem
to do different things in their runtime vs normal suspend/resume
routines, so forcing platform into the active-on-resume behaviour of the
generic ops may make some use cases impossible.  Conversion of more OMAP
drivers to runtime pm appears to be ongoing so I'd imagine we'd be
seeing more of this.  Perhaps Kevin or Magnus will have a comment here.
The same thing applies to AMBA drivers.

Looking at the i2c drivers using runtime pm in comparison, they all seem
to be using straightforward UNIVERSAL_PM_OPS-style code with the runtime
and the system sleep doing the same things.  So maybe we do need to
treat platform/AMBA different from the I2C/SPI group?

On Friday, February 18, 2011, Rabin Vincent wrote:

On Fri, Feb 18, 2011 at 23:58, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Friday, February 18, 2011, Rabin Vincent wrote:

quoted

On Thu, Feb 17, 2011 at 20:55, Rabin Vincent [off-list ref] wrote:

quoted

This will solve the platform vs AMBA bus, but shouldn't we really be
aiming for consistent behaviour between these and the other busses such
as I2C and SPI, which are also usually commonly used on the same
platforms and are using GENERIC_PM_OPS?

Should we be auditing all platform drivers and then switch platform to
the GENERIC_PM_OPS?

Or should the two points (1) and (2) be not handled in the bus at all
and be left to individual drivers (in which case we should audit i2c and
spi and change GENERIC_PM_OPS)?

How about something like the below?  If we have something like this, we
can just switch platform to GENERIC_PM_OPS and add the
pm_runtime_want_interaction() (or something better named) call to the
i2c and spi drivers using runtime PM.

Why don't we make platform_bus_type behave along the lines of generic ops
instead?

At least drivers/spi/omap2_mcspi.c, drivers/video/sh_mobile_lcdcfb.c and
drivers/watchdog/omap_wdt.c are some pm_runtime-using drivers which seem
to do different things in their runtime vs normal suspend/resume
routines, so forcing platform into the active-on-resume behaviour of the
generic ops may make some use cases impossible.  Conversion of more OMAP
drivers to runtime pm appears to be ongoing so I'd imagine we'd be
seeing more of this.  Perhaps Kevin or Magnus will have a comment here.
The same thing applies to AMBA drivers.

I see.

Looking at the i2c drivers using runtime pm in comparison, they all seem
to be using straightforward UNIVERSAL_PM_OPS-style code with the runtime
and the system sleep doing the same things.  So maybe we do need to
treat platform/AMBA different from the I2C/SPI group?

We probably do.

Thanks,
Rafael

On Fri, Feb 18, 2011 at 09:20:29PM +0100, Rafael J. Wysocki wrote:

On Friday, February 18, 2011, Rabin Vincent wrote:

quoted

Looking at the i2c drivers using runtime pm in comparison, they all seem
to be using straightforward UNIVERSAL_PM_OPS-style code with the runtime
and the system sleep doing the same things.  So maybe we do need to
treat platform/AMBA different from the I2C/SPI group?

We probably do.

Do we have any pressing need to convert AMBA stuff?  I haven't heard any
reason yet to convert them to runtime PM - they don't even make any
runtime PM calls.

Maybe Linus can comment on the PM stuff as he has SoCs with these in.
As my boards don't have any sensible PM support, I don't have any
visibility of what PM facilities would be required.

On Fri, Feb 18, 2011 at 08:27:44PM +0000, Russell King - ARM Linux wrote:

Do we have any pressing need to convert AMBA stuff?  I haven't heard any
reason yet to convert them to runtime PM - they don't even make any
runtime PM calls.

There's a bit of a chicken and egg problem in that it's not possible for
devices to make use of runtime PM unless the bus has runtime PM support
implemented - the bus implementation is mandatory, there's no default.

On Sat, Feb 19, 2011 at 01:57, Russell King - ARM Linux
[off-list ref] wrote:

Do we have any pressing need to convert AMBA stuff? ?I haven't heard any
reason yet to convert them to runtime PM - they don't even make any
runtime PM calls.

Maybe Linus can comment on the PM stuff as he has SoCs with these in.
As my boards don't have any sensible PM support, I don't have any
visibility of what PM facilities would be required.

The rationale for runtime power control is the same as that for
65500fa94aaeb3 "ARM: 6467/1: amba: optional PrimeCell core voltage
switch".

As compared to the regulator API which that patch is using, the runtime
pm usage is more flexible (for example allowing certain power control
APIs to be called from atomic context), provides callbacks for
asynchronous turnoff with callbacks back to the driver to save/restore
state (runtime_suspend()/runtime_resume()), and provides core support
for things like "autosuspend" which allows delaying suspend until some
time after last inactivity.  Using runtime PM also allows use of the new
device-level power domain support ("PM: Add support for device power
domains", in -next) to easily implement SoC-specific handling.

We need to first add bus support for this to allow drivers to use this
API.  It is possible to make the AMBA patch smaller and touch only the
AMBA bus code by implementing support for the legacy bus-specific
suspend/resume calls, and drivers can be later converted to pm-ops as
needed.

2011/2/18 Russell King - ARM Linux [off-list ref]:

Do we have any pressing need to convert AMBA stuff? ?I haven't heard any
reason yet to convert them to runtime PM - they don't even make any
runtime PM calls.

Maybe Linus can comment on the PM stuff as he has SoCs with these in.
As my boards don't have any sensible PM support, I don't have any
visibility of what PM facilities would be required.

Sure, basically I ACK Rabins patch and his reasoning for it. (BTW
Rabin spends most of his days working on the Ux500 SoCs too.)

The runtime PM we need for Ux500 is to switch off silicon core
voltage first and foremost. The call I've added to switch of a core
voltage regulator will need to be called when the silicon is idle.

In spi/amba-pl022.c I take the most brutal approach with a recent
patch: hammer off this core switch (and clock) whenever the hardware
is not used. This is simple in this driver since it has no state to preserve
across transfers, it is written such that the core is loaded with the
appropriate state for each message.

Continuing this approach we run into two problems with this
and other drivers:

-  Hammering off/on the clock+voltage is causing delays in HW
   so what you want is some hysteresis (usually, wait a few us/ms
   then switch off) - sort of a takeoff/landing effect.

-  Modelling voltage domains as regulators is nice, but require
   us to switch on/off from process context, so we cannot do this
   from interrupt handlers.

Both of these problems are solved by elegance if we use runtime
PM, since it will provide a hysteresis timeout that can be triggered
from interrupt context and call the idling hooks in process context.

Yours,
Linus Walleij

On Sat, Feb 19, 2011 at 10:54:57AM +0100, Linus Walleij wrote:

2011/2/18 Russell King - ARM Linux [off-list ref]:

quoted

Do we have any pressing need to convert AMBA stuff? ?I haven't heard any
reason yet to convert them to runtime PM - they don't even make any
runtime PM calls.

Maybe Linus can comment on the PM stuff as he has SoCs with these in.
As my boards don't have any sensible PM support, I don't have any
visibility of what PM facilities would be required.

Sure, basically I ACK Rabins patch and his reasoning for it. (BTW
Rabin spends most of his days working on the Ux500 SoCs too.)

The runtime PM we need for Ux500 is to switch off silicon core
voltage first and foremost. The call I've added to switch of a core
voltage regulator will need to be called when the silicon is idle.

In spi/amba-pl022.c I take the most brutal approach with a recent
patch: hammer off this core switch (and clock) whenever the hardware
is not used. This is simple in this driver since it has no state to preserve
across transfers, it is written such that the core is loaded with the
appropriate state for each message.

Continuing this approach we run into two problems with this
and other drivers:

-  Hammering off/on the clock+voltage is causing delays in HW
   so what you want is some hysteresis (usually, wait a few us/ms
   then switch off) - sort of a takeoff/landing effect.

-  Modelling voltage domains as regulators is nice, but require
   us to switch on/off from process context, so we cannot do this
   from interrupt handlers.

Both of these problems are solved by elegance if we use runtime
PM, since it will provide a hysteresis timeout that can be triggered
from interrupt context and call the idling hooks in process context.

So what's the interdependence with the platform bus that was being talked
about earlier in this thread?

2011/2/19 Russell King - ARM Linux [off-list ref]:

[Me]

quoted

Both of these problems are solved by elegance if we use runtime
PM, since it will provide a hysteresis timeout that can be triggered
from interrupt context and call the idling hooks in process context.

So what's the interdependence with the platform bus that was being talked
about earlier in this thread?

That's about consistency of runtime PM semantics across
different buses as I understand it.

We have both platform bus and AMBA bus devices in the system,
so it is desireable if the semantics of their runtime PM are identical.

If I understand it, the difference is that the platform bus will call
runtime_suspend() on the device even if it was already in suspended
state, so the question is about whether the AMBA runtime PM
should do this too since it is similar to the platform bus, or if it should
go for the more intutive approach of not suspending suspended
hardware.

I think the current patch from Rabin as it stands does the latter, and
is good as it stands. It's the other buses and their drivers that
need patching.

Yours,
Linus Walleij