platform/i2c busses: pm runtime and system sleep

platform/i2c busses: pm runtime and system sleep

[Rabin Vincent]

Hello,

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.

      This seems to have been done in i2c until recently, but has been
      removed by 753419f59e ("i2c: Fix for suspend/resume issue").  It
      seems to me that this removal is incorrect, and instead the real
      problem with the implementation was that it set the device as
      active even if no resume callback existed, whereas it should only
      do so when it exists and returns zero, like the generic ops.

Are these divergences from the generic ops to be considered as bugs?
Atleast (2) will cause devices which use UNIVERSAL_DEV_PM_OPS to have
incorrect runtime pm state after a resume from system sleep.

If so, before I send patches to fix them: can it be assumed that only
drivers using dev_pm_ops (and not the legacy ops of these busses) will
need the interactions between runtime PM and system sleep as done in the
generic ops?  This would mean that simple busses could simply use the
generic ops like below instead of duplicating their behaviour:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6b4cc56..46117e0 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -212,7 +212,7 @@ static int i2c_device_pm_resume(struct device *dev)
 	int ret;

 	if (pm)
-		ret = pm->resume ? pm->resume(dev) : 0;
+		ret = pm_generic_resume(dev);
 	else
 		ret = i2c_legacy_resume(dev);

 thanks,
 Rabin

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

[Rafael J. Wysocki]

On Thursday, December 16, 2010, Rabin Vincent wrote:
> Hello,
> 
> 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.
> 
>       This seems to have been done in i2c until recently, but has been
>       removed by 753419f59e ("i2c: Fix for suspend/resume issue").  It
>       seems to me that this removal is incorrect, and instead the real
>       problem with the implementation was that it set the device as
>       active even if no resume callback existed, whereas it should only
>       do so when it exists and returns zero, like the generic ops.
> 
> 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.

> Atleast (2) will cause devices which use UNIVERSAL_DEV_PM_OPS to have
> incorrect runtime pm state after a resume from system sleep.
> 
> If so, before I send patches to fix them: can it be assumed that only
> drivers using dev_pm_ops (and not the legacy ops of these busses) will
> need the interactions between runtime PM and system sleep as done in the
> generic ops?

Yes, you can make this assumption safely.  The drivers that don't use
dev_pm_ops can't support runtime PM at all.

> This would mean that simple busses could simply use the
> generic ops like below instead of duplicating their behaviour:
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 6b4cc56..46117e0 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -212,7 +212,7 @@ static int i2c_device_pm_resume(struct device *dev)
>  	int ret;
> 
>  	if (pm)
> -		ret = pm->resume ? pm->resume(dev) : 0;
> +		ret = pm_generic_resume(dev);
>  	else
>  		ret = i2c_legacy_resume(dev);

Thanks,
Rafael

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

[Rabin Vincent]

On Fri, Dec 17, 2010 at 05:39, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, December 16, 2010, Rabin Vincent wrote:
>> 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.
...
>> 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)?

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

[Rabin Vincent]

On Thu, Feb 17, 2011 at 20:55, Rabin Vincent <rabin-66gdRtMMWGc@public.gmane.org> 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,

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

[Alan Stern]

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.

> 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.
> + */

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

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

[Rafael J. Wysocki]

On Friday, February 18, 2011, Rabin Vincent wrote:
> On Thu, Feb 17, 2011 at 20:55, Rabin Vincent <rabin-66gdRtMMWGc@public.gmane.org> 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.

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

Rafael

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

[Rabin Vincent]

On Fri, Feb 18, 2011 at 23:58, Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org> wrote:
> On Friday, February 18, 2011, Rabin Vincent wrote:
>> On Thu, Feb 17, 2011 at 20:55, Rabin Vincent <rabin-66gdRtMMWGc@public.gmane.org> 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.
>
> 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?

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

[Rafael J. Wysocki]

On Friday, February 18, 2011, Rabin Vincent wrote:
> On Fri, Feb 18, 2011 at 23:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, February 18, 2011, Rabin Vincent wrote:
> >> On Thu, Feb 17, 2011 at 20:55, Rabin Vincent <rabin@rab.in> 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.
> >
> > 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

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

[Russell King - ARM Linux]

On Fri, Feb 18, 2011 at 09:20:29PM +0100, Rafael J. Wysocki wrote:
> On Friday, February 18, 2011, Rabin Vincent wrote:
> > 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.

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

[Rabin Vincent]

On Sat, Feb 19, 2011 at 01:57, Russell King - ARM Linux
<linux@arm.linux.org.uk> 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.

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

[Mark Brown]

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.

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

[Linus Walleij]

2011/2/18 Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>:

> 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

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

[Russell King - ARM Linux]

On Sat, Feb 19, 2011 at 10:54:57AM +0100, Linus Walleij wrote:
> 2011/2/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> 
> > 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?

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

[Linus Walleij]

2011/2/19 Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>:
> [Me]
>> 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

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

[Mark Brown]

On Thu, Dec 16, 2010 at 11:56:57PM +0530, Rabin Vincent wrote:

> 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.

This is platform lagging behind I2C in implementation - both originally 
did what platform does and then I2C was updated and platform wasn't.

It'd be really good if this could all be factored out into the PM core,
we're going to have to do the same thing for at least SPI as well and
possibly some other buses :/

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

[Rafael J. Wysocki]

On Friday, December 17, 2010, Mark Brown wrote:
> On Thu, Dec 16, 2010 at 11:56:57PM +0530, Rabin Vincent wrote:
> 
> > 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.
> 
> This is platform lagging behind I2C in implementation - both originally 
> did what platform does and then I2C was updated and platform wasn't.
> 
> It'd be really good if this could all be factored out into the PM core,
> we're going to have to do the same thing for at least SPI as well and
> possibly some other buses :/

So how exactly the PM core is supposed to include those things?

There certainly are other buses that don't want to do them.

Thanks,
Rafael

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

[Mark Brown]

On Fri, Dec 17, 2010 at 02:25:07PM +0100, Rafael J. Wysocki wrote:
> On Friday, December 17, 2010, Mark Brown wrote:

> > It'd be really good if this could all be factored out into the PM core,
> > we're going to have to do the same thing for at least SPI as well and
> > possibly some other buses :/

> So how exactly the PM core is supposed to include those things?

> There certainly are other buses that don't want to do them.

By, for example, providing default implementations which the buses can
use if they choose to.

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

[Rafael J. Wysocki]

On Friday, December 17, 2010, Mark Brown wrote:
> On Fri, Dec 17, 2010 at 02:25:07PM +0100, Rafael J. Wysocki wrote:
> > On Friday, December 17, 2010, Mark Brown wrote:
> 
> > > It'd be really good if this could all be factored out into the PM core,
> > > we're going to have to do the same thing for at least SPI as well and
> > > possibly some other buses :/
> 
> > So how exactly the PM core is supposed to include those things?
> 
> > There certainly are other buses that don't want to do them.
> 
> By, for example, providing default implementations which the buses can
> use if they choose to.

OK, so we have generic_subsys_pm_ops.  Do we need anything beyond that?

Rafael

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

[Mark Brown]

On Fri, Dec 17, 2010 at 02:49:25PM +0100, Rafael J. Wysocki wrote:
> On Friday, December 17, 2010, Mark Brown wrote:

> > By, for example, providing default implementations which the buses can
> > use if they choose to.

> OK, so we have generic_subsys_pm_ops.  Do we need anything beyond that?

Hrm.  Possibly just some fiddling with those or alternative versions.
For example, looking at the I2C bus suspend it's this:

static int i2c_device_pm_suspend(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

	if (pm) {
		if (pm_runtime_suspended(dev))
			return 0;
		else
			return pm->suspend ? pm->suspend(dev) : 0;
	}

	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
}

Ideally the if (pm) block could just be factored out into the pm core as
there's nothing I2C-specific about that at all.  Possibly even the whole
logic surrounding fall back to legacy, though that smells a bit.  The
generic suspend operation doesn't fit here:

int pm_generic_suspend(struct device *dev)
{
	return __pm_generic_call(dev, PM_EVENT_SUSPEND);
}
EXPORT_SYMBOL_GPL(pm_generic_suspend);

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

[Rafael J. Wysocki]

On Friday, December 17, 2010, Mark Brown wrote:
> On Fri, Dec 17, 2010 at 02:49:25PM +0100, Rafael J. Wysocki wrote:
> > On Friday, December 17, 2010, Mark Brown wrote:
> 
> > > By, for example, providing default implementations which the buses can
> > > use if they choose to.
> 
> > OK, so we have generic_subsys_pm_ops.  Do we need anything beyond that?
> 
> Hrm.  Possibly just some fiddling with those or alternative versions.
> For example, looking at the I2C bus suspend it's this:
> 
> static int i2c_device_pm_suspend(struct device *dev)
> {
> 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> 
> 	if (pm) {
> 		if (pm_runtime_suspended(dev))
> 			return 0;
> 		else
> 			return pm->suspend ? pm->suspend(dev) : 0;
> 	}
> 
> 	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> }
> 
> Ideally the if (pm) block could just be factored out into the pm core as
> there's nothing I2C-specific about that at all.  Possibly even the whole
> logic surrounding fall back to legacy, though that smells a bit.

No, the legacy is i2c-specific.

> The generic suspend operation doesn't fit here:
> 
> int pm_generic_suspend(struct device *dev)
> {
> 	return __pm_generic_call(dev, PM_EVENT_SUSPEND);
> }
> EXPORT_SYMBOL_GPL(pm_generic_suspend);

Well, looking at __pm_generic_call(), I think it does.

It appears to do exactly what the pm block above does, so it should be possible
to have something like this:

static int i2c_device_pm_suspend(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
	return pm ? pm_generic_suspend(dev) : i2c_legacy_suspend(dev, PMSG_SUSPEND);
}

if I'm not missing anything.

Thanks,
Rafael

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

[Mark Brown]

On Sat, Dec 18, 2010 at 12:01:25AM +0100, Rafael J. Wysocki wrote:
> On Friday, December 17, 2010, Mark Brown wrote:

> > 	if (pm) {
> > 		if (pm_runtime_suspended(dev))
> > 			return 0;
> > 		else
> > 			return pm->suspend ? pm->suspend(dev) : 0;
> > 	}

> > 	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > }

> > Ideally the if (pm) block could just be factored out into the pm core as
> > there's nothing I2C-specific about that at all.  Possibly even the whole
> > logic surrounding fall back to legacy, though that smells a bit.

> No, the legacy is i2c-specific.

SPI and platform (the first two buses I looked at) both seem to have
legacy suspend operations too?  Clearly the bus would need to provide an
op to invoke the legacy call but the logic which prioritises the pm_ops
over the legacy operation is generic.

> > The generic suspend operation doesn't fit here:

> > int pm_generic_suspend(struct device *dev)
> > {
> > 	return __pm_generic_call(dev, PM_EVENT_SUSPEND);
> > }
> > EXPORT_SYMBOL_GPL(pm_generic_suspend);

> Well, looking at __pm_generic_call(), I think it does.

> It appears to do exactly what the pm block above does, so it should be possible
> to have something like this:

Oh, so it does.

> static int i2c_device_pm_suspend(struct device *dev)
> {
> 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> 	return pm ? pm_generic_suspend(dev) : i2c_legacy_suspend(dev, PMSG_SUSPEND);
> }

> if I'm not missing anything.

Yes, I think that should work - thanks.  A similar thing would work for
the default platform bus implementation, too, and bring it into line
with I2C here.  I'll have a play.  Similarly for SPI.

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

[Rafael J. Wysocki]

On Saturday, December 18, 2010, Mark Brown wrote:
> On Sat, Dec 18, 2010 at 12:01:25AM +0100, Rafael J. Wysocki wrote:
> > On Friday, December 17, 2010, Mark Brown wrote:
> 
> > > 	if (pm) {
> > > 		if (pm_runtime_suspended(dev))
> > > 			return 0;
> > > 		else
> > > 			return pm->suspend ? pm->suspend(dev) : 0;
> > > 	}
> 
> > > 	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > }
> 
> > > Ideally the if (pm) block could just be factored out into the pm core as
> > > there's nothing I2C-specific about that at all.  Possibly even the whole
> > > logic surrounding fall back to legacy, though that smells a bit.
> 
> > No, the legacy is i2c-specific.
> 
> SPI and platform (the first two buses I looked at) both seem to have
> legacy suspend operations too?  Clearly the bus would need to provide an
> op to invoke the legacy call but the logic which prioritises the pm_ops
> over the legacy operation is generic.

Well, the problem with that is the driver would need to tell the generic call
what the legacy routine is and there's no, er, generic way to do that.

In the i2c case, for example, there is struct i2c_driver that contains the
->suspend() and ->resume() pointers, so the bus type driver _knows_ how to
get there, but the PM core doesn't have this information.

Thanks,
Rafael

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

[Mark Brown]

On Sat, Dec 18, 2010 at 01:54:57PM +0100, Rafael J. Wysocki wrote:
> On Saturday, December 18, 2010, Mark Brown wrote:

> > SPI and platform (the first two buses I looked at) both seem to have
> > legacy suspend operations too?  Clearly the bus would need to provide an
> > op to invoke the legacy call but the logic which prioritises the pm_ops
> > over the legacy operation is generic.

> Well, the problem with that is the driver would need to tell the generic call
> what the legacy routine is and there's no, er, generic way to do that.

> In the i2c case, for example, there is struct i2c_driver that contains the
> ->suspend() and ->resume() pointers, so the bus type driver _knows_ how to
> get there, but the PM core doesn't have this information.

Sure, but this could be readily accomplished by providing bus
legacy_suspend() and legacy_resume() operations that the generic code
could use to do the actual call.  This would save them all implmeneting
essentially the same decision making code for all the various different 
PM operations - the only bit that differs between buses is going to be
the actual process for calling the legacy API.

Like I say, I'm not sure if it's actually worth it.

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

[Rafael J. Wysocki]

On Saturday, December 18, 2010, Mark Brown wrote:
> On Sat, Dec 18, 2010 at 01:54:57PM +0100, Rafael J. Wysocki wrote:
> > On Saturday, December 18, 2010, Mark Brown wrote:
> 
> > > SPI and platform (the first two buses I looked at) both seem to have
> > > legacy suspend operations too?  Clearly the bus would need to provide an
> > > op to invoke the legacy call but the logic which prioritises the pm_ops
> > > over the legacy operation is generic.
> 
> > Well, the problem with that is the driver would need to tell the generic call
> > what the legacy routine is and there's no, er, generic way to do that.
> 
> > In the i2c case, for example, there is struct i2c_driver that contains the
> > ->suspend() and ->resume() pointers, so the bus type driver _knows_ how to
> > get there, but the PM core doesn't have this information.
> 
> Sure, but this could be readily accomplished by providing bus
> legacy_suspend() and legacy_resume() operations that the generic code
> could use to do the actual call.

First, there already are ->suspend() and ->resume() callbacks in
struct bus_type which are regarded as "legacy".  The PM core uses those as
appropriate in drivers/base/power/main.c .

Second, the situation at hand is that the bus type implements dev_pm_ops,
but the driver doesn't.  Now, pm_generic_suspend() is called with a struct
device pointer, so it would have to go back to dev->bus, find the
->legacy_suspend() callback (as opposed to ->suspend(), which also is legacy,
but is called by the PM core instead).  May I call that confusing?

> This would save them all implmeneting
> essentially the same decision making code for all the various different 
> PM operations - the only bit that differs between buses is going to be
> the actual process for calling the legacy API.
> 
> Like I say, I'm not sure if it's actually worth it.

Well, I don't really think so.

Thanks,
Rafael

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

[Mark Brown]

On Sat, Dec 18, 2010 at 03:59:50PM +0100, Rafael J. Wysocki wrote:

> Second, the situation at hand is that the bus type implements dev_pm_ops,
> but the driver doesn't.  Now, pm_generic_suspend() is called with a struct
> device pointer, so it would have to go back to dev->bus, find the
> ->legacy_suspend() callback (as opposed to ->suspend(), which also is legacy,
> but is called by the PM core instead).  May I call that confusing?

Well, the trouble is that the whole situation is already pretty
confusing for what should be very simple buses, each one needs to write
a bunch of not really bus specific code in order to get basic behaviour
which allows the drivers to make use of runtime PM, requiring more
thought and care per bus than I'd expect given that they've nothing
really to contribute.  This leads to the sort of random variations
between buses that Rabin is reporting, and means that updates keep
having to get done in multiple different places.

The overall effect is that from the point of view of trying to use
runtime PM in drivers which work with these simple buses everything
feels like it's much harder work than it should be.  Moving all the
decision making out of the buses and into the PM core seems like a win
here.

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

[Rafael J. Wysocki]

On Monday, December 20, 2010, Mark Brown wrote:
> On Sat, Dec 18, 2010 at 03:59:50PM +0100, Rafael J. Wysocki wrote:
> 
> > Second, the situation at hand is that the bus type implements dev_pm_ops,
> > but the driver doesn't.  Now, pm_generic_suspend() is called with a struct
> > device pointer, so it would have to go back to dev->bus, find the
> > ->legacy_suspend() callback (as opposed to ->suspend(), which also is legacy,
> > but is called by the PM core instead).  May I call that confusing?
> 
> Well, the trouble is that the whole situation is already pretty
> confusing for what should be very simple buses, each one needs to write
> a bunch of not really bus specific code in order to get basic behaviour
> which allows the drivers to make use of runtime PM, requiring more
> thought and care per bus than I'd expect given that they've nothing
> really to contribute.  This leads to the sort of random variations
> between buses that Rabin is reporting, and means that updates keep
> having to get done in multiple different places.
> 
> The overall effect is that from the point of view of trying to use
> runtime PM in drivers which work with these simple buses everything
> feels like it's much harder work than it should be.  Moving all the
> decision making out of the buses and into the PM core seems like a win
> here.

Well, the _solution_ is to get rid of the legacy stuff from those buses in the
first place.  Then, they'll just need to use the generic ops without any
trouble.

What you're proposing is a workaround that people will use as an excuse for
not doing the right thing forever.

Thanks,
Rafael

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

[Mark Brown]

On Mon, Dec 20, 2010 at 10:13:53PM +0100, Rafael J. Wysocki wrote:

> Well, the _solution_ is to get rid of the legacy stuff from those buses in the
> first place.  Then, they'll just need to use the generic ops without any
> trouble.

> What you're proposing is a workaround that people will use as an excuse for
> not doing the right thing forever.

I hadn't actually realised that there was any urgency about removing the
old APIs - I'd not really seen much work in that direction.  If the
intention is to actively push people over to dev_pm_ops that does make a
bit more sense.

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

[Rafael J. Wysocki]

On Wednesday, December 22, 2010, Mark Brown wrote:
> On Mon, Dec 20, 2010 at 10:13:53PM +0100, Rafael J. Wysocki wrote:
> 
> > Well, the _solution_ is to get rid of the legacy stuff from those buses in the
> > first place.  Then, they'll just need to use the generic ops without any
> > trouble.
> 
> > What you're proposing is a workaround that people will use as an excuse for
> > not doing the right thing forever.
> 
> I hadn't actually realised that there was any urgency about removing the
> old APIs - I'd not really seen much work in that direction.  If the
> intention is to actively push people over to dev_pm_ops that does make a
> bit more sense.

There's no urgency strictly speaking, but there's a preference. :-)

I think it probably is the time to actually start pushing people into that,
because the legacy stuff has been causing too much pain recently.

Thanks,
Rafael