[RFC/NOT FOR MERGING 0/5] OMAP PM patches

[RFC/NOT FOR MERGING 0/5] OMAP PM patches

[Felipe Balbi]

Hi guys,

this series is actually *REALLY* far from ready, but I wanted
to ask if I should continue down this track because it really
looks (to me at least) that OMAP's PM layer took a few uneecessary
shortcuts.

I'm trying to understand the reasoning behind that, so bear with
me for a while.

At least patches 1 and 4 look like they could go upstream, but
please give it a very good review. I will continue to work on
these if the rest of the community thinks it's valid, otherwise
I would like to get some explanation for the way OMAP PM layer
is implemented today.

cheers

Felipe Balbi (5):
  arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
  arm: omap: don't forcefully runtime suspend a device
  arm: omap: introduce other PM methods
  i2c: omap: don't re-enable IRQs after masking them
  i2c: omap: introduce suspend/resume methods

 arch/arm/plat-omap/omap_device.c | 162 +++++++++++++++++++++++++++++++++++++--
 drivers/i2c/busses/i2c-omap.c    |  70 ++++++++++++-----
 2 files changed, 207 insertions(+), 25 deletions(-)

-- 
1.8.0.rc0

[RFC/NOT FOR MERGING 3/5] arm: omap: introduce other PM methods

[Felipe Balbi]

current omap_device PM implementation defines
omap-specific *_noirq methods but uses the
generic versions for all other PM methods.

As it turns out, if a device decides to implement
non-runtime PM callbacks, we might fall into a
situation where the hwmod is still idled which
will generate an abort exception when we try
to access device's address space while clocks
are still gated.

In order to solve that, we implement all other
methods taking into account that devices might
not implement those, in which case we return
early.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

notice here that it would be far better to avoid the code duplication
and have another function (e.g. _od_generic_suspend/resume) which would
receive pm_message_t and omap_device as arguments so it can decide
what to do.

That can be done on a later version, though.

 arch/arm/plat-omap/omap_device.c | 145 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 1 deletion(-)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index cd84eac..60ce750 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -843,16 +843,159 @@ static int _od_resume_noirq(struct device *dev)
 
 	return pm_generic_resume_noirq(dev);
 }
+
+static int _od_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret;
+
+	if (!pm || !pm->suspend)
+		return 0;
+
+	/* Don't attempt suspend on a driver that is not bound */
+	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
+		return 0;
+
+	ret = pm_generic_suspend(dev);
+	if (ret)
+		return ret;
+
+	if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+		omap_device_idle(pdev);
+	od->flags |= OMAP_DEVICE_SUSPENDED;
+
+	return 0;
+}
+
+static int _od_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (!pm || !pm->resume)
+		return 0;
+
+	if (od->flags & OMAP_DEVICE_SUSPENDED) {
+		od->flags &= ~OMAP_DEVICE_SUSPENDED;
+
+		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+			omap_device_enable(pdev);
+	}
+
+	return pm_generic_resume(dev);
+}
+
+static int _od_freeze(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret;
+
+	if (!pm || !pm->freeze)
+		return 0;
+
+	/* Don't attempt late suspend on a driver that is not bound */
+	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
+		return 0;
+
+	ret = pm_generic_freeze(dev);
+	if (ret)
+		return ret;
+
+	if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+		omap_device_idle(pdev);
+	od->flags |= OMAP_DEVICE_SUSPENDED;
+
+	return 0;
+}
+
+static int _od_thaw(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (!pm || !pm->thaw)
+		return 0;
+
+	if (od->flags & OMAP_DEVICE_SUSPENDED) {
+		od->flags &= ~OMAP_DEVICE_SUSPENDED;
+
+		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+			omap_device_enable(pdev);
+	}
+
+	return pm_generic_thaw(dev);
+}
+
+static int _od_poweroff(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret;
+
+	if (!pm || !pm->poweroff)
+		return 0;
+
+	/* Don't attempt late suspend on a driver that is not bound */
+	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
+		return 0;
+
+	ret = pm_generic_poweroff(dev);
+	if (ret)
+		return ret;
+
+	if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+		omap_device_idle(pdev);
+	od->flags |= OMAP_DEVICE_SUSPENDED;
+
+	return 0;
+}
+
+static int _od_restore(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (!pm || !pm->restore)
+		return 0;
+
+	if (od->flags & OMAP_DEVICE_SUSPENDED) {
+		od->flags &= ~OMAP_DEVICE_SUSPENDED;
+
+		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+			omap_device_enable(pdev);
+	}
+
+	return pm_generic_restore(dev);
+}
 #else
 #define _od_suspend_noirq NULL
 #define _od_resume_noirq NULL
+#define _od_suspend NULL
+#define _od_resume NULL
+#define _od_freeze NULL
+#define _od_thaw NULL
+#define _od_poweroff NULL
+#define _od_restore NULL
 #endif
 
 struct dev_pm_domain omap_device_pm_domain = {
 	.ops = {
 		SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
 				   _od_runtime_idle)
-		USE_PLATFORM_PM_SLEEP_OPS
+		.suspend = _od_suspend,
+		.resume = _od_resume,
+		.freeze = _od_freeze,
+		.thaw = _od_thaw,
+		.poweroff = _od_poweroff,
+		.restore = _od_restore,
 		.suspend_noirq = _od_suspend_noirq,
 		.resume_noirq = _od_resume_noirq,
 	}
-- 
1.8.0.rc0

[RFC/NOT FOR MERGING 3/5] arm: omap: introduce other PM methods

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> current omap_device PM implementation defines
> omap-specific *_noirq methods but uses the
> generic versions for all other PM methods.
>
> As it turns out, if a device decides to implement
> non-runtime PM callbacks, we might fall into a
> situation where the hwmod is still idled which
> will generate an abort exception when we try
> to access device's address space while clocks
> are still gated.

Please explain in more detail how this could happen.

Kevin

> In order to solve that, we implement all other
> methods taking into account that devices might
> not implement those, in which case we return
> early.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>

[RFC/NOT FOR MERGING 3/5] arm: omap: introduce other PM methods

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 10:07:31AM -0700, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > current omap_device PM implementation defines
> > omap-specific *_noirq methods but uses the
> > generic versions for all other PM methods.
> >
> > As it turns out, if a device decides to implement
> > non-runtime PM callbacks, we might fall into a
> > situation where the hwmod is still idled which
> > will generate an abort exception when we try
> > to access device's address space while clocks
> > are still gated.
> 
> Please explain in more detail how this could happen.

just follow the patchset. If I implement system suspend and I'm not
focefully calling runtime_* methods, who will be there to call
omap_device_enable() ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/eecb1070/attachment.sig>

[RFC/NOT FOR MERGING 3/5] arm: omap: introduce other PM methods

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 08:06:40PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Oct 18, 2012 at 10:07:31AM -0700, Kevin Hilman wrote:
> > Felipe Balbi <balbi@ti.com> writes:
> > 
> > > current omap_device PM implementation defines
> > > omap-specific *_noirq methods but uses the
> > > generic versions for all other PM methods.
> > >
> > > As it turns out, if a device decides to implement
> > > non-runtime PM callbacks, we might fall into a
> > > situation where the hwmod is still idled which
> > > will generate an abort exception when we try
> > > to access device's address space while clocks
> > > are still gated.
> > 
> > Please explain in more detail how this could happen.
> 
> just follow the patchset. If I implement system suspend and I'm not
> focefully calling runtime_* methods, who will be there to call
> omap_device_enable() ?

let me rephrase this. If we exit _noirq right in the beginning with the
extra check I've added, who will call omap_device_enable() ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/dca978dd/attachment.sig>

[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 <balbi@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

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

[Kevin Hilman]

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);

[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 <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 ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/30dddd42/attachment-0001.sig>

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Felipe Balbi]

current implementation doesn't take care about
drivers which don't provide *_noirq methods and we
could fall into a situation where we would suspend/resume
devices even though they haven't asked for it.

One such case happened with the I2C driver which
was getting suspended during suspend_noirq() just
to be resume right after by any other device doing
an I2C transfer on its suspend method.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 arch/arm/plat-omap/omap_device.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index 7a7d1f2..935f416 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int ret;
 
+	if (!pm || !pm->suspend_noirq)
+		return 0;
+
 	/* Don't attempt late suspend on a driver that is not bound */
 	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
 		return 0;
@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (!pm || !pm->resume_noirq)
+		return 0;
 
 	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
 	    !pm_runtime_status_suspended(dev)) {
-- 
1.8.0.rc0

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> current implementation doesn't take care about
> drivers which don't provide *_noirq methods 

The generic ops handle this.  See below.

> and we could fall into a situation where we would suspend/resume
> devices even though they haven't asked for it.

The following explanation doesn't explain this, so I dont' follow
the "even though they haven't asked for it" part.

> One such case happened with the I2C driver which
> was getting suspended during suspend_noirq() just
> to be resume right after by any other device doing
> an I2C transfer on its suspend method.

In order to be I2C to be runtime resumed after its noirq method has been
called, that means the other device is doing an I2C xfer in its noirq
method.  That is a bug.

> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  arch/arm/plat-omap/omap_device.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index 7a7d1f2..935f416 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct omap_device *od = to_omap_device(pdev);
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  	int ret;
>  
> +	if (!pm || !pm->suspend_noirq)
> +		return 0;
> +

you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)

>  	/* Don't attempt late suspend on a driver that is not bound */
>  	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
>  		return 0;
> @@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct omap_device *od = to_omap_device(pdev);
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
> +	if (!pm || !pm->resume_noirq)
> +		return 0;

and this is basically pm_generic_resume_noirq()

>  
>  	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>  	    !pm_runtime_status_suspended(dev)) {

Kevin

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 09:34:15AM -0700, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > current implementation doesn't take care about
> > drivers which don't provide *_noirq methods 
> 
> The generic ops handle this.  See below.
> 
> > and we could fall into a situation where we would suspend/resume
> > devices even though they haven't asked for it.
> 
> The following explanation doesn't explain this, so I dont' follow
> the "even though they haven't asked for it" part.
> 
> > One such case happened with the I2C driver which
> > was getting suspended during suspend_noirq() just
> > to be resume right after by any other device doing
> > an I2C transfer on its suspend method.
> 
> In order to be I2C to be runtime resumed after its noirq method has been
> called, that means the other device is doing an I2C xfer in its noirq
> method.  That is a bug.
> 
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  arch/arm/plat-omap/omap_device.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> > index 7a7d1f2..935f416 100644
> > --- a/arch/arm/plat-omap/omap_device.c
> > +++ b/arch/arm/plat-omap/omap_device.c
> > @@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct omap_device *od = to_omap_device(pdev);
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >  	int ret;
> >  
> > +	if (!pm || !pm->suspend_noirq)
> > +		return 0;
> > +
> 
> you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)
> 
> >  	/* Don't attempt late suspend on a driver that is not bound */
> >  	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
> >  		return 0;
> > @@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct omap_device *od = to_omap_device(pdev);
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (!pm || !pm->resume_noirq)
> > +		return 0;
> 
> and this is basically pm_generic_resume_noirq()

not true, there is a slight different. Note that you only call
pm_generic_resume_noirq() after calling pm_runtime_resume()ing the
device:

> static int _od_resume_noirq(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) &&
> 	    !pm_runtime_status_suspended(dev)) {
> 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
> 		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
> 			omap_device_enable(pdev);
> 		pm_generic_runtime_resume(dev);

right here. IMHO this is a bug, if the define doesn't implement
resume_noirq, it's telling you that it has nothing to do at that time,
so you shouldn't forcefully resume the device.

If the device dies, then it means that it needs to implement
resume_noirq. What you have here, is a "workaround" for badly written
device drivers IMHO. Note also, that you could runtime resume devices
which don't implement resume_noirq().

the same is valid for suspend_noirq.

> 	}
> 
> 	return pm_generic_resume_noirq(dev);
> }



-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/1798106e/attachment.sig>

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Thu, Oct 18, 2012 at 09:34:15AM -0700, Kevin Hilman wrote:
>> Felipe Balbi <balbi@ti.com> writes:
>> 
>> > current implementation doesn't take care about
>> > drivers which don't provide *_noirq methods 
>> 
>> The generic ops handle this.  See below.
>> 
>> > and we could fall into a situation where we would suspend/resume
>> > devices even though they haven't asked for it.
>> 
>> The following explanation doesn't explain this, so I dont' follow
>> the "even though they haven't asked for it" part.
>> 
>> > One such case happened with the I2C driver which
>> > was getting suspended during suspend_noirq() just
>> > to be resume right after by any other device doing
>> > an I2C transfer on its suspend method.
>> 
>> In order to be I2C to be runtime resumed after its noirq method has been
>> called, that means the other device is doing an I2C xfer in its noirq
>> method.  That is a bug.
>> 
>> > Signed-off-by: Felipe Balbi <balbi@ti.com>
>> > ---
>> >  arch/arm/plat-omap/omap_device.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> > index 7a7d1f2..935f416 100644
>> > --- a/arch/arm/plat-omap/omap_device.c
>> > +++ b/arch/arm/plat-omap/omap_device.c
>> > @@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
>> >  {
>> >  	struct platform_device *pdev = to_platform_device(dev);
>> >  	struct omap_device *od = to_omap_device(pdev);
>> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> >  	int ret;
>> >  
>> > +	if (!pm || !pm->suspend_noirq)
>> > +		return 0;
>> > +
>> 
>> you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)
>> 
>> >  	/* Don't attempt late suspend on a driver that is not bound */
>> >  	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
>> >  		return 0;
>> > @@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
>> >  {
>> >  	struct platform_device *pdev = to_platform_device(dev);
>> >  	struct omap_device *od = to_omap_device(pdev);
>> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> > +
>> > +	if (!pm || !pm->resume_noirq)
>> > +		return 0;
>> 
>> and this is basically pm_generic_resume_noirq()
>
> not true, there is a slight different. Note that you only call
> pm_generic_resume_noirq() after calling pm_runtime_resume()ing the
> device:

>> static int _od_resume_noirq(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) &&
>> 	    !pm_runtime_status_suspended(dev)) {
>> 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
>> 		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
>> 			omap_device_enable(pdev);
>> 		pm_generic_runtime_resume(dev);
>
> right here. IMHO this is a bug, if the define doesn't implement
> resume_noirq, it's telling you that it has nothing to do at that time,

This is where the misunderstanding is.  I suggest you have a look
at commit c03f007a8bf0e092caeb6856a5c8a850df10b974 where this feature
was added, but I'll try to summarize:

The goal is simply this:

If, by the time .suspend_noirq has run, the driver is not already
runtime suspended, then forcefully runtime suspend it.

That's it.

Again, this is required because system suspend disables runtime PM
transitions at a certain point, if the device is used after that point,
there is *no other way* for the driver to properly manage the idle
transition (or, if there is, please explain it to me.)

> so you shouldn't forcefully resume the device.

Note it's only forcefully resumed *if* it was forcefully suspended by
suspend_noirq.

> If the device dies, then it means that it needs to implement
> resume_noirq. What you have here, is a "workaround" for badly written
> device drivers IMHO. 

That's one way of looking at it.  The other way of looking at it is that
by handling this at the PM domain level, the drivers can be much simpler,
and thus less likely to get wrong.

But even then, with your proposed changes, I don't think the device will
be properly idled (including the omap_device_idle) in these important cases:

1) I2C is used by some other device *after* its ->suspend method has
   been called.
2) I2C runtime PM is disabled from userspace
   (echo off > /sys/devices/.../power/control)

but I'll take this up in the I2C driver patch itself.

> Note also, that you could runtime resume devices which don't implement
> resume_noirq().

Again, this is by design.  

It doesn't matter if the driver has noirq methods.  If it is not yet
runtime suspended, it is forceably runtime suspended.   The generic
noirq calls are just there in case the driver has noirq callbacks.

Kevin

> the same is valid for suspend_noirq.
>
>> 	}
>> 
>> 	return pm_generic_resume_noirq(dev);
>> }

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 10:42:37AM -0700, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > Hi,
> >
> > On Thu, Oct 18, 2012 at 09:34:15AM -0700, Kevin Hilman wrote:
> >> Felipe Balbi <balbi@ti.com> writes:
> >> 
> >> > current implementation doesn't take care about
> >> > drivers which don't provide *_noirq methods 
> >> 
> >> The generic ops handle this.  See below.
> >> 
> >> > and we could fall into a situation where we would suspend/resume
> >> > devices even though they haven't asked for it.
> >> 
> >> The following explanation doesn't explain this, so I dont' follow
> >> the "even though they haven't asked for it" part.
> >> 
> >> > One such case happened with the I2C driver which
> >> > was getting suspended during suspend_noirq() just
> >> > to be resume right after by any other device doing
> >> > an I2C transfer on its suspend method.
> >> 
> >> In order to be I2C to be runtime resumed after its noirq method has been
> >> called, that means the other device is doing an I2C xfer in its noirq
> >> method.  That is a bug.
> >> 
> >> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> > ---
> >> >  arch/arm/plat-omap/omap_device.c | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> >> > index 7a7d1f2..935f416 100644
> >> > --- a/arch/arm/plat-omap/omap_device.c
> >> > +++ b/arch/arm/plat-omap/omap_device.c
> >> > @@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
> >> >  {
> >> >  	struct platform_device *pdev = to_platform_device(dev);
> >> >  	struct omap_device *od = to_omap_device(pdev);
> >> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >> >  	int ret;
> >> >  
> >> > +	if (!pm || !pm->suspend_noirq)
> >> > +		return 0;
> >> > +
> >> 
> >> you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)
> >> 
> >> >  	/* Don't attempt late suspend on a driver that is not bound */
> >> >  	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
> >> >  		return 0;
> >> > @@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
> >> >  {
> >> >  	struct platform_device *pdev = to_platform_device(dev);
> >> >  	struct omap_device *od = to_omap_device(pdev);
> >> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >> > +
> >> > +	if (!pm || !pm->resume_noirq)
> >> > +		return 0;
> >> 
> >> and this is basically pm_generic_resume_noirq()
> >
> > not true, there is a slight different. Note that you only call
> > pm_generic_resume_noirq() after calling pm_runtime_resume()ing the
> > device:
> 
> >> static int _od_resume_noirq(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) &&
> >> 	    !pm_runtime_status_suspended(dev)) {
> >> 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
> >> 		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
> >> 			omap_device_enable(pdev);
> >> 		pm_generic_runtime_resume(dev);
> >
> > right here. IMHO this is a bug, if the define doesn't implement
> > resume_noirq, it's telling you that it has nothing to do at that time,
> 
> This is where the misunderstanding is.  I suggest you have a look
> at commit c03f007a8bf0e092caeb6856a5c8a850df10b974 where this feature
> was added, but I'll try to summarize:
> 
> The goal is simply this:
> 
> If, by the time .suspend_noirq has run, the driver is not already
> runtime suspended, then forcefully runtime suspend it.
> 
> That's it.

Yes, I got the intention, but to me it looks wrong.

> Again, this is required because system suspend disables runtime PM
> transitions at a certain point, if the device is used after that point,
> there is *no other way* for the driver to properly manage the idle
> transition (or, if there is, please explain it to me.)

Can you please let me know of any situation where e.g. I2C/SPI would be
needed after all its children are suspended ?

> > so you shouldn't forcefully resume the device.
> 
> Note it's only forcefully resumed *if* it was forcefully suspended by
> suspend_noirq.

likewise, you shouldn't forcefully runtime suspend a device. The device
driver needs to be required to provide suspend/resume functions if it
cares.

> > If the device dies, then it means that it needs to implement
> > resume_noirq. What you have here, is a "workaround" for badly written
> > device drivers IMHO. 
> 
> That's one way of looking at it.  The other way of looking at it is that
> by handling this at the PM domain level, the drivers can be much simpler,
> and thus less likely to get wrong.

You're still bypassing what the PM framework wants us to do, no ? What
if Rafael decides to drastically change the way system and runtime PM is
done and it ends up breaking what we have on OMAP ? If we stick to the
standard, the it's less likely to brea.

> But even then, with your proposed changes, I don't think the device will
> be properly idled (including the omap_device_idle) in these important cases:
> 
> 1) I2C is used by some other device *after* its ->suspend method has
>    been called.

how can that happen ? I2C's ->suspend method will only be called after
all its children are suspended.

> 2) I2C runtime PM is disabled from userspace
>    (echo off > /sys/devices/.../power/control)

that should not make a difference if you omap_device_idle() is written
as it should. Maybe what you need is a refactor to provide
omap_device_idle() and omap_device_runtime_idle(), the latter taking
care of the case where runtime pm can be disabled from userspace while
the former idling whenever it's called.

> but I'll take this up in the I2C driver patch itself.

sure :-)

> > Note also, that you could runtime resume devices which don't implement
> > resume_noirq().
> 
> Again, this is by design.

a very weird design IMHO. Either that or I'm really missing something
here.

> It doesn't matter if the driver has noirq methods.  If it is not yet
> runtime suspended, it is forceably runtime suspended.   The generic

if it's not yet runtime suspended, you need to call the driver's
->suspend_* method (depending on the suspend phase you are right now),
but "reusing" the runtime_suspend method sounds to me like another
"special OMAP requirement".

> noirq calls are just there in case the driver has noirq callbacks.

I get that, but why would you suspend a device which doesn't want to be
suspended in suspend_noirq(), but using its runtime_suspend method ?

If I2C dies on a suspend/resume transition, it's a bug in the I2C
driver, no ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/687f60a5/attachment.sig>

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Thu, Oct 18, 2012 at 10:42:37AM -0700, Kevin Hilman wrote:
>> Felipe Balbi <balbi@ti.com> writes:
>> 
>> > Hi,
>> >
>> > On Thu, Oct 18, 2012 at 09:34:15AM -0700, Kevin Hilman wrote:
>> >> Felipe Balbi <balbi@ti.com> writes:
>> >> 
>> >> > current implementation doesn't take care about
>> >> > drivers which don't provide *_noirq methods 
>> >> 
>> >> The generic ops handle this.  See below.
>> >> 
>> >> > and we could fall into a situation where we would suspend/resume
>> >> > devices even though they haven't asked for it.
>> >> 
>> >> The following explanation doesn't explain this, so I dont' follow
>> >> the "even though they haven't asked for it" part.
>> >> 
>> >> > One such case happened with the I2C driver which
>> >> > was getting suspended during suspend_noirq() just
>> >> > to be resume right after by any other device doing
>> >> > an I2C transfer on its suspend method.
>> >> 
>> >> In order to be I2C to be runtime resumed after its noirq method has been
>> >> called, that means the other device is doing an I2C xfer in its noirq
>> >> method.  That is a bug.
>> >> 
>> >> > Signed-off-by: Felipe Balbi <balbi@ti.com>
>> >> > ---
>> >> >  arch/arm/plat-omap/omap_device.c | 8 ++++++++
>> >> >  1 file changed, 8 insertions(+)
>> >> >
>> >> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> >> > index 7a7d1f2..935f416 100644
>> >> > --- a/arch/arm/plat-omap/omap_device.c
>> >> > +++ b/arch/arm/plat-omap/omap_device.c
>> >> > @@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
>> >> >  {
>> >> >  	struct platform_device *pdev = to_platform_device(dev);
>> >> >  	struct omap_device *od = to_omap_device(pdev);
>> >> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> >> >  	int ret;
>> >> >  
>> >> > +	if (!pm || !pm->suspend_noirq)
>> >> > +		return 0;
>> >> > +
>> >> 
>> >> you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)
>> >> 
>> >> >  	/* Don't attempt late suspend on a driver that is not bound */
>> >> >  	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
>> >> >  		return 0;
>> >> > @@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
>> >> >  {
>> >> >  	struct platform_device *pdev = to_platform_device(dev);
>> >> >  	struct omap_device *od = to_omap_device(pdev);
>> >> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> >> > +
>> >> > +	if (!pm || !pm->resume_noirq)
>> >> > +		return 0;
>> >> 
>> >> and this is basically pm_generic_resume_noirq()
>> >
>> > not true, there is a slight different. Note that you only call
>> > pm_generic_resume_noirq() after calling pm_runtime_resume()ing the
>> > device:
>> 
>> >> static int _od_resume_noirq(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) &&
>> >> 	    !pm_runtime_status_suspended(dev)) {
>> >> 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
>> >> 		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
>> >> 			omap_device_enable(pdev);
>> >> 		pm_generic_runtime_resume(dev);
>> >
>> > right here. IMHO this is a bug, if the define doesn't implement
>> > resume_noirq, it's telling you that it has nothing to do at that time,
>> 
>> This is where the misunderstanding is.  I suggest you have a look
>> at commit c03f007a8bf0e092caeb6856a5c8a850df10b974 where this feature
>> was added, but I'll try to summarize:
>> 
>> The goal is simply this:
>> 
>> If, by the time .suspend_noirq has run, the driver is not already
>> runtime suspended, then forcefully runtime suspend it.
>> 
>> That's it.
>
> Yes, I got the intention, but to me it looks wrong.
>
>> Again, this is required because system suspend disables runtime PM
>> transitions at a certain point, if the device is used after that point,
>> there is *no other way* for the driver to properly manage the idle
>> transition (or, if there is, please explain it to me.)
>
> Can you please let me know of any situation where e.g. I2C/SPI would be
> needed after all its children are suspended ?

There lots of I2C users who are not children of the I2C bus device.  Any
users of regulators on the I2C-connected PMIC for example (MMC comes to
mind, and is not a child of the I2C bus.).  Also, enable/disable of GPIO
IRQ lines on I2C GPIO expanders.  The devices using those GPIO IRQs are
not children of the I2C bus. 

Stated differently, I2C devices themselves (PMIC, GPIO expanders, etc.)
are children of the I2C bus device.  But *users* of those devices will
be other drivers who have no parent/child relationship with the I2C bus
device in the driver model.

>> > so you shouldn't forcefully resume the device.
>> 
>> Note it's only forcefully resumed *if* it was forcefully suspended by
>> suspend_noirq.
>
> likewise, you shouldn't forcefully runtime suspend a device. The device
> driver needs to be required to provide suspend/resume functions if it
> cares.

I don't think you've fully understood the problem being solved here.

This is for devices who designed for use throughout the suspend process
(IOW, *after* their suspend callbacks have been called.)

>> > If the device dies, then it means that it needs to implement
>> > resume_noirq. What you have here, is a "workaround" for badly written
>> > device drivers IMHO. 
>> 
>> That's one way of looking at it.  The other way of looking at it is that
>> by handling this at the PM domain level, the drivers can be much simpler,
>> and thus less likely to get wrong.
>
> You're still bypassing what the PM framework wants us to do, no ? What
> if Rafael decides to drastically change the way system and runtime PM is
> done and it ends up breaking what we have on OMAP ? If we stick to the
> standard, the it's less likely to brea.

What I have done on OMAP was done in direct collaboration with the PM
core mantainers, and designed specifically for limitations in the PM
core.  If those assumptions change, I will have to change.  I have no
problems dealing with that if/when the time comes.

>> But even then, with your proposed changes, I don't think the device will
>> be properly idled (including the omap_device_idle) in these important cases:
>> 
>> 1) I2C is used by some other device *after* its ->suspend method has
>>    been called.
>
> how can that happen ? I2C's ->suspend method will only be called after
> all its children are suspended.

Unfortunately, drivers which use I2C are not all children of the I2C bus
device in the driver model (see above.)

This gets to a much bigger problem with the linux driver model today.
There are *many* power relationships/dependencies between devices that 
are not modeled by driver model device hierarchy.

>> 2) I2C runtime PM is disabled from userspace
>>    (echo off > /sys/devices/.../power/control)
>
> that should not make a difference if you omap_device_idle() is written
> as it should. Maybe what you need is a refactor to provide
> omap_device_idle() and omap_device_runtime_idle(), the latter taking
> care of the case where runtime pm can be disabled from userspace while
> the former idling whenever it's called.

No thanks.

>> but I'll take this up in the I2C driver patch itself.
>
> sure :-)
>
>> > Note also, that you could runtime resume devices which don't implement
>> > resume_noirq().
>> 
>> Again, this is by design.
>
> a very weird design IMHO. Either that or I'm really missing something
> here.

I agree it's wierd.  Unfortunately, it's necessary.

>> It doesn't matter if the driver has noirq methods.  If it is not yet
>> runtime suspended, it is forceably runtime suspended.   The generic
>
> if it's not yet runtime suspended, you need to call the driver's
> ->suspend_* method (depending on the suspend phase you are right now),

You're confusing system suspend and runtime suspend, and forgetting that
we might want to use runtime PM *during* system suspend/resume.

Again, the reason a device may not yet runtime suspended is because it
was in use during suspend, and runtime PM has been disabled by the PM
core during system PM.  If the PM core did not disable runtime PM, the
drivers runtime PM methods would be called, and we would not need to
implement this in the PM domain layer.

> but "reusing" the runtime_suspend method sounds to me like another
> "special OMAP requirement".

There's nothing special about OMAP here.  Any devices that want to use
runtime PM *throughout* the suspend path has this requirment, and the
implementation used in our PM domain layer was implemented based on the
recommendation of Rafael.

>> noirq calls are just there in case the driver has noirq callbacks.
>
> I get that, but why would you suspend a device which doesn't want to be
> suspended in suspend_noirq(), but using its runtime_suspend method ?

What do you mean "doesn't want to be suspended"?  If a device doesn't
want to be suspended, it needs to return an error from one of its
suspend methods.   Barring that, the system *will* suspend, and in order
to hit low power states, we have to properly idle each device.

The underlying assumption is that by the time the noirq phase runs, the
device should already be runtime suspended.  If it is not, then one of
the following ha happened:

- device was used during suspend, but after runtime PM was disabled by
  the PM core (during system suspend)

- runtime PM disabled from userspace

In both cases, *somebody* has to properly idle the device and "finish"
the runtime suspend late in the suspend phase.  As recommended by
Rafael, we handle this at the PM domain level.

> If I2C dies on a suspend/resume transition, it's a bug in the I2C
> driver, no ?

Sure, but this has nothing to do with I2C dying.

Kevin

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 11:42:33AM -0700, Kevin Hilman wrote:
> >> Again, this is required because system suspend disables runtime PM
> >> transitions at a certain point, if the device is used after that point,
> >> there is *no other way* for the driver to properly manage the idle
> >> transition (or, if there is, please explain it to me.)
> >
> > Can you please let me know of any situation where e.g. I2C/SPI would be
> > needed after all its children are suspended ?
> 
> There lots of I2C users who are not children of the I2C bus device.  Any
> users of regulators on the I2C-connected PMIC for example (MMC comes to
> mind, and is not a child of the I2C bus.).  Also, enable/disable of GPIO
> IRQ lines on I2C GPIO expanders.  The devices using those GPIO IRQs are
> not children of the I2C bus. 

MMC isn't a child of I2C, indeed, but the regulator is a child of TWL
which is a child of the I2C bus.

> Stated differently, I2C devices themselves (PMIC, GPIO expanders, etc.)
> are children of the I2C bus device.  But *users* of those devices will
> be other drivers who have no parent/child relationship with the I2C bus
> device in the driver model.

correct, but MMC is not suspended as late as suspend_noirq() is it ?
Looks like only I2C can fall into this category because of exactly
regulators and I/O expanders. So wouldn't implementing
omap_i2c_suspend_noirq() and omap_i2c_resume_noirq() solve the same
problem ?

> >> > so you shouldn't forcefully resume the device.
> >> 
> >> Note it's only forcefully resumed *if* it was forcefully suspended by
> >> suspend_noirq.
> >
> > likewise, you shouldn't forcefully runtime suspend a device. The device
> > driver needs to be required to provide suspend/resume functions if it
> > cares.
> 
> I don't think you've fully understood the problem being solved here.
> 
> This is for devices who designed for use throughout the suspend process
> (IOW, *after* their suspend callbacks have been called.)

is there no way for a device to tell PM core that it doesn't want to be
suspended during ->suspend_early or ->suspend but only on
->suspend_noirq ? Isn't the fact that we don't implement ->suspend_early
and ->suspend enough to keep the device running ? I think that will be
the case with the patch which adds the extra check on
_od_suspend_noirq(), no ?

And before you reply, if the device implements none of the suspend
methods, then you shouldn't suspend it at all, because you'd be masking
a bug in the driver, IMHO.

> >> But even then, with your proposed changes, I don't think the device will
> >> be properly idled (including the omap_device_idle) in these important cases:
> >> 
> >> 1) I2C is used by some other device *after* its ->suspend method has
> >>    been called.
> >
> > how can that happen ? I2C's ->suspend method will only be called after
> > all its children are suspended.
> 
> Unfortunately, drivers which use I2C are not all children of the I2C bus
> device in the driver model (see above.)
> 
> This gets to a much bigger problem with the linux driver model today.
> There are *many* power relationships/dependencies between devices that 
> are not modeled by driver model device hierarchy.

Ok. Now I see a good point :-) You're correct, thanks

> >> It doesn't matter if the driver has noirq methods.  If it is not yet
> >> runtime suspended, it is forceably runtime suspended.   The generic
> >
> > if it's not yet runtime suspended, you need to call the driver's
> > ->suspend_* method (depending on the suspend phase you are right now),
> 
> You're confusing system suspend and runtime suspend, and forgetting that
> we might want to use runtime PM *during* system suspend/resume.

Maybe I wasn't clear enough. What I meant to say was that if the device
isn't runtime_suspended by the time you should be calling its
->suspend_noirq method, then calling the device's runtime_suspend()
method looks very weird to me.

It really seems, to me, that implementing suspend_noirq() on e.g. I2C
controller driver should do the trick provided you call
->suspend_noirq() instead of ->runtime_suspend(). I mean, if we do exact
the same thing on runtime_suspend() and suspend_noirq() (namely disable
IRQs, save context, etc), then it should make no difference which method
you call, right ?

> Again, the reason a device may not yet runtime suspended is because it
> was in use during suspend, and runtime PM has been disabled by the PM
> core during system PM.  If the PM core did not disable runtime PM, the
> drivers runtime PM methods would be called, and we would not need to
> implement this in the PM domain layer.

fair enough, but what I still don't get is why you need to call exactly
->runtime_suspend. What's the problem with calling ->suspend_noirq() ?

Again, look at the last patch. Note that I do the same thing on both
->suspend (should be ->suspend_noirq()) and ->runtime_suspend(), with a
slight difference:

on ->suspend(_noirq) if the device is already runtime_suspended, I just
return 0, otherwise I follow the same procedure and (due to the other
patches) omap_device_idle() will be called the same way. The device will
be suspended, but (let's assume) PM runtime doesn't know about it. No
problem, during resume you will call omap_device_resume() and later call
pm_generic_resume(_noirq) which will call omap_i2c_resume(_noirq) which
will:

reenable_irqs();
restore_context();

pm_runtime_disable(dev);
pm_runtime_set_active(dev); /* we have already resumed the device */
pm_runtime_enable(dev);

return 0;

> >> noirq calls are just there in case the driver has noirq callbacks.
> >
> > I get that, but why would you suspend a device which doesn't want to be
> > suspended in suspend_noirq(), but using its runtime_suspend method ?
> 
> What do you mean "doesn't want to be suspended"?  If a device doesn't

by "doesn't want to be suspended" I mean that it hasn't implemented a
noirq method.

> want to be suspended, it needs to return an error from one of its
> suspend methods.   Barring that, the system *will* suspend, and in order
> to hit low power states, we have to properly idle each device.
> 
> The underlying assumption is that by the time the noirq phase runs, the
> device should already be runtime suspended.  If it is not, then one of
> the following ha happened:
> 
> - device was used during suspend, but after runtime PM was disabled by
>   the PM core (during system suspend)
> 
> - runtime PM disabled from userspace
> 
> In both cases, *somebody* has to properly idle the device and "finish"

yes, and that somebody is omap_device_idle() right ? Now my question
remains: why is it so important to call device's ->runtime_suspend
method. Why couldn't you call device's ->suspend_noirq method instead ?

> the runtime suspend late in the suspend phase.  As recommended by
> Rafael, we handle this at the PM domain level.
> 
> > If I2C dies on a suspend/resume transition, it's a bug in the I2C
> > driver, no ?
> 
> Sure, but this has nothing to do with I2C dying.

Imagine that instead of calling ->runtime_suspend we were to call
->suspend_noirq, but the device doesn't implement that. What would
happen ? no context save, no context restore, I2C dead.

ps: I'll continue reading the code and further test my series to see
if I can understand what you say here.

cheers

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/7af66528/attachment.sig>

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

[...]

> if the device implements none of the suspend methods, then you
> shouldn't suspend it at all, because you'd be masking a bug in the
> driver, IMHO.

OK, let's start over here, because here's the fundamental difference.  I
don't think missing suspend methods means a bug in the driver at all.

For a moment, let's ignore the limitations/restrictions that exist today
betwen system PM and runtime PM and assume runtime PM can be used
anytime, including anywhere in the suspend/resume path.

In that ideal world, many drivers would not need system suspend methods
at all.  They would just runtime PM to enable/disable themselves based
on driver/device activity.

Stated differently, since the driver is using runtime PM, when system
suspend happens, the device is already idle, and runtime suspended, so
there is nothing for the system PM methods to do.  Therefore, no system
PM callbacks are needed.  No bug in driver.

(yes, this is oversimplified to illustrate the goal/point.  Some drivers
may want to have a ->suspend callback to wait/stop any current
processing, but that is just so runtime PM can kick in.)

This is the "runtime PM centric" view of things that we have been
driving towards with OMAP drivers (we've been doing this with aggressive
clock gating even before runtime PM framework.)  

In other words, if drivers are written with this "runtime PM centric"
view, there should be almost nothing to do in the suspend method, except
quiesce the hardware so runtime PM kicks in.

This runtime PM centric view is the mindset/philosophy behind the PM
domain implementation, and driver PM adaptations that we've been doing
for some time.

In converting/reviewing/testing *many* drivers that have been PM adapted
(for system PM and runtime PM), most folks do not get this right.
Therfore, if driver writers can concentrate on runtime PM, and just use
system PM as a "quiesce HW, let runtime PM take over" method, that is
where I want to go.

Kevin

[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

[...]

> ps: I'll continue reading the code and further test my series to see
> if I can understand what you say here.

OK.  And please get it working in cases where drivers are using I2C in
their suspend/resume (and even late/early) paths, and also where device
runtime PM is disabled from userspace.  All of that works with today's
code and will not work with your proposed code.

Kevin

[RFC/NOT FOR MERGING 5/5] i2c: omap: introduce suspend/resume methods

[Felipe Balbi]

during system-wide (static) suspend, we also
want to save our context and restore it when
waking up.

Let's introduce new suspend/resume methods so
that we can survive a suspend-to-ram transition.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c | 60 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 7eeae11..ceab138 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1236,13 +1236,8 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-#ifdef CONFIG_PM_RUNTIME
-static int omap_i2c_runtime_suspend(struct device *dev)
+static int omap_i2c_low_level_suspend(struct omap_i2c_dev *_dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
-	u16 iv;
-
 	_dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
 
 	omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
@@ -1253,11 +1248,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int omap_i2c_runtime_resume(struct device *dev)
+static int omap_i2c_low_level_resume(struct omap_i2c_dev *_dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
-
 	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
 		omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
@@ -1278,9 +1270,57 @@ static int omap_i2c_runtime_resume(struct device *dev)
 
 	return 0;
 }
+
+static int omap_i2c_suspend(struct device *dev)
+{
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
+
+	if (pm_runtime_suspended(dev))
+		return 0;
+
+	return omap_i2c_low_level_suspend(_dev);
+}
+
+static int omap_i2c_resume(struct device *dev)
+{
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
+	int r;
+
+	r = omap_i2c_low_level_resume(_dev);
+	if (r)
+		return r;
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_RUNTIME
+static int omap_i2c_runtime_suspend(struct device *dev)
+{
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
+
+	return omap_i2c_low_level_suspend(_dev);
+}
+
+static int omap_i2c_runtime_resume(struct device *dev)
+{
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
+
+	return omap_i2c_low_level_resume(_dev);
+}
+#else
+#define omap_i2c_runtime_suspend NULL
+#define omap_i2c_runtime_resume NULL
 #endif /* CONFIG_PM_RUNTIME */
 
 static struct dev_pm_ops omap_i2c_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend,
+			omap_i2c_resume)
 	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
 			   omap_i2c_runtime_resume, NULL)
 };
-- 
1.8.0.rc0

[RFC/NOT FOR MERGING 4/5] i2c: omap: don't re-enable IRQs after masking them

[Felipe Balbi]

OMAP I2C driver will re-enable IRQs right after
masking them during suspend.

That's not what we want. We want to keep IRQs
masked until our resume method is called.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..7eeae11 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1247,14 +1247,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 
 	omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
 
-	if (_dev->rev < OMAP_I2C_OMAP1_REV_2) {
-		iv = omap_i2c_read_reg(_dev, OMAP_I2C_IV_REG); /* Read clears */
-	} else {
-		omap_i2c_write_reg(_dev, OMAP_I2C_STAT_REG, _dev->iestate);
-
-		/* Flush posted write */
-		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
-	}
+	/* Flush posted write */
+	omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
 
 	return 0;
 }
-- 
1.8.0.rc0

[RFC/NOT FOR MERGING 4/5] i2c: omap: don't re-enable IRQs after masking them

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> OMAP I2C driver will re-enable IRQs right after
> masking them during suspend.
>
> That's not what we want. We want to keep IRQs
> masked until our resume method is called.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..7eeae11 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1247,14 +1247,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>  
>  	omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
>  
> -	if (_dev->rev < OMAP_I2C_OMAP1_REV_2) {
> -		iv = omap_i2c_read_reg(_dev, OMAP_I2C_IV_REG); /* Read clears */
> -	} else {
> -		omap_i2c_write_reg(_dev, OMAP_I2C_STAT_REG, _dev->iestate);
> -
> -		/* Flush posted write */
> -		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> -	}

Are you sure this re-enables the interrupt?  It looks to me like
it's meant to clear the interrupt.    

> +	/* Flush posted write */
> +	omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);

Assuming the above is correct, should this be IE_REG?

>  	return 0;
>  }

Kevin

[RFC/NOT FOR MERGING 4/5] i2c: omap: don't re-enable IRQs after masking them

[Felipe Balbi]

On Thu, Oct 18, 2012 at 10:10:06AM -0700, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > OMAP I2C driver will re-enable IRQs right after
> > masking them during suspend.
> >
> > That's not what we want. We want to keep IRQs
> > masked until our resume method is called.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index db31eae..7eeae11 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1247,14 +1247,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
> >  
> >  	omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
> >  
> > -	if (_dev->rev < OMAP_I2C_OMAP1_REV_2) {
> > -		iv = omap_i2c_read_reg(_dev, OMAP_I2C_IV_REG); /* Read clears */
> > -	} else {
> > -		omap_i2c_write_reg(_dev, OMAP_I2C_STAT_REG, _dev->iestate);
> > -
> > -		/* Flush posted write */
> > -		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> > -	}
> 
> Are you sure this re-enables the interrupt?  It looks to me like
> it's meant to clear the interrupt.    
> 
> > +	/* Flush posted write */
> > +	omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> 
> Assuming the above is correct, should this be IE_REG?

indeed. My eyes failed. This patch can be thrown away. My bad.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/b4a51840/attachment.sig>