Re: [RFC/NOT FOR MERGING 2/5] arm: omap: don't forcefully runtime suspend a device

[Kevin Hilman]

Felipe Balbi ba...@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 ba...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/NOT FOR MERGING 2/5] arm: omap: don't forcefully runtime suspend a device

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 10:01:00AM -0700, Kevin Hilman wrote:
 Felipe Balbi ba...@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 ba...@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 ?

-- 
balbi


signature.asc
Description: Digital signature

[RFC/NOT FOR MERGING 2/5] arm: omap: don't forcefully runtime suspend a device

[Felipe Balbi]

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 ba...@ti.com
---
 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);
-- 
1.8.0.rc0

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html