| Message ID | 1350488043-5053-3-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Felipe Balbi <balbi@ti.com> writes: > device drivers should be smart enough to provide > ->suspend/->resume callbacks when needed and they > should take care of doing whatever needs to be > done in order to allow a device to be suspended. > > Calling pm_runtime_* from system suspend isn't > the right way to achieve that, as it creates a > situation where OMAP's PM has different requirements > and semantics than all other architectures. > > Signed-off-by: Felipe Balbi <balbi@ti.com> NAK This support is required to handle some restrictions placed on runtime PM and system PM interactions. Basically, runtime PM transitions are disabled part way through system PM (that itself was a much debated topic last year, but that's how it works today.) Because of this limitation, drivers that are active during the suspend phase (commonly becasue they are used by [late] suspend methods of other devices) may have multiple runtime PM transitions during static suspend/resume. These drivers have the problem that after runtime PM has been disabled, even when they pm_runtime_put*, they will not actually be transitioned (and their runtime PM callbacks will not be called.) So these devices are in a "ready to runtime suspend" state, but they will not transition because runtime PM is disabled. After your patch, they will still be idled (omap_device_idle), but the driver will have no notification that this has happened because you removed the calling of the runtime PM callbacks. In the changelog, you seem to be implying that anything the driver should be doing should be done in its suspend/resume callbacks instead of the runtime suspend/resume callbacks (but don't give your reasoning.) Using the current approach (which was actually suggested by Rafael), it means many transiactional drivers (like I2C) can be implemented as runtime PM only, and don't need to provide suspend/resume callbacks at all. It also means they can be used throughout the suspend/resume path (well until noirq methods.) The approach in $SUBJECT patch would mean that drivers should not be used after their suspend method has been called. That places some severe limitations on drivers like I2C, SPI, HSI, UART etc. that are often used by the suspend/resume methods of other drivers. Kevin > --- > arch/arm/plat-omap/omap_device.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c > index 935f416..cd84eac 100644 > --- a/arch/arm/plat-omap/omap_device.c > +++ b/arch/arm/plat-omap/omap_device.c > @@ -817,11 +817,9 @@ static int _od_suspend_noirq(struct device *dev) > ret = pm_generic_suspend_noirq(dev); > > if (!ret && !pm_runtime_status_suspended(dev)) { > - if (pm_generic_runtime_suspend(dev) == 0) { > - if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) > - omap_device_idle(pdev); > - od->flags |= OMAP_DEVICE_SUSPENDED; > - } > + if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) > + omap_device_idle(pdev); > + od->flags |= OMAP_DEVICE_SUSPENDED; > } > > return ret; > @@ -841,7 +839,6 @@ static int _od_resume_noirq(struct device *dev) > od->flags &= ~OMAP_DEVICE_SUSPENDED; > if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) > omap_device_enable(pdev); > - pm_generic_runtime_resume(dev); > } > > return pm_generic_resume_noirq(dev); -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Oct 18, 2012 at 10:01:00AM -0700, Kevin Hilman wrote: > Felipe Balbi <balbi@ti.com> writes: > > > device drivers should be smart enough to provide > > ->suspend/->resume callbacks when needed and they > > should take care of doing whatever needs to be > > done in order to allow a device to be suspended. > > > > Calling pm_runtime_* from system suspend isn't > > the right way to achieve that, as it creates a > > situation where OMAP's PM has different requirements > > and semantics than all other architectures. > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > NAK > > This support is required to handle some restrictions placed on runtime > PM and system PM interactions. Basically, runtime PM transitions are > disabled part way through system PM (that itself was a much debated > topic last year, but that's how it works today.) > > Because of this limitation, drivers that are active during the suspend > phase (commonly becasue they are used by [late] suspend methods of other > devices) may have multiple runtime PM transitions during static > suspend/resume. These drivers have the problem that after runtime PM > has been disabled, even when they pm_runtime_put*, they will not > actually be transitioned (and their runtime PM callbacks will not be > called.) > > So these devices are in a "ready to runtime suspend" state, but they > will not transition because runtime PM is disabled. > > After your patch, they will still be idled (omap_device_idle), but the > driver will have no notification that this has happened because you > removed the calling of the runtime PM callbacks. to me this only seems like the driver misses some static PM callbacks and some pm_runtime_disable; pm_runtime_set_active; pm_runtime_enable; on their system resume methods. > In the changelog, you seem to be implying that anything the driver > should be doing should be done in its suspend/resume callbacks instead > of the runtime suspend/resume callbacks (but don't give your reasoning.) that's not what I implied at all. What I imply is that if driver needs to do something during system suspend/resume, then it needs to implement those methods. Calling runtime_* methods from system PM looks like a workaround for incomplete drivers. > Using the current approach (which was actually suggested by Rafael), it > means many transiactional drivers (like I2C) can be implemented as > runtime PM only, and don't need to provide suspend/resume callbacks at to me that sounds bogus :-s what's the problem of using the same function for system suspend and runtime suspend ? (see the last patch of the series where I implement I2C system suspend and resume) > all. It also means they can be used throughout the suspend/resume path > (well until noirq methods.) fair enough... > The approach in $SUBJECT patch would mean that drivers should not be > used after their suspend method has been called. That places some > severe limitations on drivers like I2C, SPI, HSI, UART etc. that are > often used by the suspend/resume methods of other drivers. I can't see the limitation you talk about. Let's use I2C as an example. In order to suspend RTC we need I2C resume, but driver core guarantees that parent will only suspend after all children are suspended. If that's as far as _noirq, then I2C needs to implement _noirq callbacks. I don't see the issue because RTC is a child of I2C and will suspend before its parent. No ?
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index 935f416..cd84eac 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c @@ -817,11 +817,9 @@ static int _od_suspend_noirq(struct device *dev) ret = pm_generic_suspend_noirq(dev); if (!ret && !pm_runtime_status_suspended(dev)) { - if (pm_generic_runtime_suspend(dev) == 0) { - if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) - omap_device_idle(pdev); - od->flags |= OMAP_DEVICE_SUSPENDED; - } + if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) + omap_device_idle(pdev); + od->flags |= OMAP_DEVICE_SUSPENDED; } return ret; @@ -841,7 +839,6 @@ static int _od_resume_noirq(struct device *dev) od->flags &= ~OMAP_DEVICE_SUSPENDED; if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) omap_device_enable(pdev); - pm_generic_runtime_resume(dev); } return pm_generic_resume_noirq(dev);
device drivers should be smart enough to provide ->suspend/->resume callbacks when needed and they should take care of doing whatever needs to be done in order to allow a device to be suspended. Calling pm_runtime_* from system suspend isn't the right way to achieve that, as it creates a situation where OMAP's PM has different requirements and semantics than all other architectures. Signed-off-by: Felipe Balbi <balbi@ti.com> --- arch/arm/plat-omap/omap_device.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)