Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
[...]
>>>You have ignored a few very important points:
>>>
>>>Firstly, system suspend is supposed to work even when runtime PM is
>>>not configured.
>>>
>>>Secondly, the user can disable runtime PM via sysfs at any time.
>>>This shouldn't mess up system suspend.
>>>
>>>Basically, it's a bad idea to mix up system suspend with runtime PM.
>>
>> Your observations are correct but this is a generic limitation and
>> Kevin is working on this problem in parallel.
>>
>> As of now, all OMAP drivers are mandated to use ONLY runtime pm framework
>> for enabling/disabling clocks. I will let Kevin comment further.
>
> Okay, let's see what Kevin says.
While I did design the OMAP PM core to be runtime PM centric, and we
implemented several drivers based on runtime PM alone, after some long
discussions on linux-pm[1] with Alan and Rafael (PM maintainer) over the
last couple weeks, I'm now convinced I had the wrong design/approach.
Rafael and Alan have been patient with my stubborness, but now I've been
pursuaded. Rafael has detailed on linux-pm the various
problems/limitations/races between runtime PM and system PM[2], so I
don't plan debating them again here.
That being said, today we have several drivers that use runtime PM calls
in their suspend/resume path and our PM domain implementation (inside
omap_device) deals with most of the limitations fine. However, there
are 2 main problems/limitation with this that we've chosen to live with
(for now):
1) synchronous calls must be used in the suspend/resume path (because
PM workqueue is frozen during suspend/resumea)
2) disabling runtime PM from userspace will prevent that device
from hitting its low-power state during suspend.
For v3.1 (and before), we've lived with these limitations, and I'm OK
with merging other drivers for v3.1 with these limitations. After 3.1,
this will be changing (more on this below.)
So, while I've been OK with merging drivers with the above limitations
for one more merge window, $SUBJECT patch adds a new twist by forcibly
managing the parent device from the child device. Personally, I really
don't like that approach and it serves as a good illustration of yet
another reason why system PM and runtime PM need understood as
conceptually very different.
For v3.2, the PM core will change[2] to futher limit/protect
interactions between runtime PM and system PM, and I will be reworking
our PM domain (omap_device) implementation accordingly.
Basically, what that will mean is that our PM domain layer (omap_device)
will also call omap_device_idle() in the suspend path, but only if the
device is *not* already idle (from previous runtime suspend.) The PM
domain layer will then omap_device_enable() the device in the system
resume path if it was suspended in the system suspend path. A minimally
tested patch to do this is below.
So, the driver still does not have to care about it's specific clocks
etc. (which should address Felipe's concern), clocks and other
IP-specific PM details will all continue to be handled by omap_device,
just like it is with runtime PM.
The primary change to the driver, is that whatever needs to be done to
prepare for both runtime PM and system PM (including context
save/restore etc.) will have to be done in a common function(s) that
will be called by *both* of its ->runtime_suspend() and ->suspend()
callbacks, and similar for ->runtime_resume() and ->resume().
Some drivers will have additional work to do for system PM though. This
is mainly because system PM can happen at *any* time, including in the
middle of ongoing activity, whereas runtime PM transitions happen when
the device is known to be idle. What that means is that for example, a
drivers ->suspend() method might need to wait for (or forcibly stop) any
ongoing activity in order to be sure the device is ready to be suspended.
Frankly, this is not a very big change for the drivers, as the
device-specific idle work will still be handled by the PM domain layer.
Hope that helps clarify the background.
As for this particular patch, since it is rather late in the development
cycle for v3.1, I would recommend that it wait until the omap_device
changes, and then let the PM core (for system PM and runtime PM) handle
the parent/child relationships as they are designed to. But that is up
to Felipe and USB maintainers to decide.
Kevin
[1] https://lists.linux-foundation.org/pipermail/linux-pm/2011-June/031559.html
[2] https://lists.linux-foundation.org/pipermail/linux-pm/2011-June/031977.html
>From 6696e9a2b106ca9c9936e5c2ad89650010120e10 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@xxxxxx>
Date: Tue, 7 Jun 2011 16:07:28 -0700
Subject: [PATCH] OMAP: PM: omap_device: add system PM methods for PM domain handling
Using PM domain callbacks, use omap_device idle/enable to
automatically suspend/resume devices. Also use pm_generic_* routines
to ensure driver's callbacks are correctly called.
Driver ->suspend callback is needed to ensure the driver is in a state
that it can be suspended.
If device is already idle (typically because of previous runtime PM
activity), there's nothing extra to do.
KJH: The omap_device_* calls should probably actually be done in the
_noirq() methods.
Not-yet-Signed-off-by: Kevin Hilman <khilman@xxxxxx>
---
arch/arm/plat-omap/include/plat/omap_device.h | 4 +++
arch/arm/plat-omap/omap_device.c | 32 +++++++++++++++++++++++++
2 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index e4c349f..bc36d05 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -44,6 +44,9 @@ extern struct device omap_device_parent;
#define OMAP_DEVICE_STATE_IDLE 2
#define OMAP_DEVICE_STATE_SHUTDOWN 3
+/* omap_device.flags values */
+#define OMAP_DEVICE_SUSPENDED BIT(0)
+
/**
* struct omap_device - omap_device wrapper for platform_devices
* @pdev: platform_device
@@ -73,6 +76,7 @@ struct omap_device {
s8 pm_lat_level;
u8 hwmods_cnt;
u8 _state;
+ u8 flags;
};
/* Device driver interface (call via platform_data fn ptrs) */
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index 49fc0df..f2711c3 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -564,12 +564,44 @@ static int _od_runtime_resume(struct device *dev)
return pm_generic_runtime_resume(dev);
}
+static int _od_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_device *od = to_omap_device(pdev);
+ int ret;
+
+ ret = pm_generic_suspend(dev);
+
+ od->flags &= ~OMAP_DEVICE_SUSPENDED;
+
+ if (od->_state == OMAP_DEVICE_STATE_ENABLED) {
+ omap_device_idle(pdev);
+ od->flags |= OMAP_DEVICE_SUSPENDED;
+ }
+
+ return ret;
+}
+
+static int _od_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_device *od = to_omap_device(pdev);
+
+ if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
+ (od->_state == OMAP_DEVICE_STATE_IDLE))
+ omap_device_enable(pdev);
+
+ return pm_generic_resume(dev);
+}
+
static struct dev_power_domain omap_device_power_domain = {
.ops = {
.runtime_suspend = _od_runtime_suspend,
.runtime_idle = _od_runtime_idle,
.runtime_resume = _od_runtime_resume,
USE_PLATFORM_PM_SLEEP_OPS
+ .suspend = _od_suspend,
+ .resume = _od_resume,
}
};
--
1.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
