[PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[Kevin Hilman]

The MUSB driver does not currently implment suspend/resume callbacks,
seemingly because it does not support cutting power when devices are
connected in host mode.

Without a proper suspend/resume implementation, the OMAP PM domain
layer which automatically gates non runtime-suspended devices will
cause problems in suspend by the drivers ->runtime_suspend() callback
which will try to cut power.

Fix this by disabling PM domain auto suspending for MUSB until the
driver can be fixed to properly implement suspend/resume.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/usb-musb.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
index 2679750..e19eb0a 100644
--- a/arch/arm/mach-omap2/usb-musb.c
+++ b/arch/arm/mach-omap2/usb-musb.c
@@ -110,6 +110,7 @@ void __init usb_musb_init(struct omap_musb_board_data *musb_board_data)
 						name, oh_name);
 		return;
 	}
+	omap_device_disable_idle_on_suspend(pdev);
 
 	dev = &pdev->dev;
 	get_device(dev);
-- 
1.7.6

Re: [PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[Kevin Hilman]

Kevin Hilman <khilman@ti.com> writes:

> The MUSB driver does not currently implment suspend/resume callbacks,
> seemingly because it does not support cutting power when devices are
> connected in host mode.
>
> Without a proper suspend/resume implementation, the OMAP PM domain
> layer which automatically gates non runtime-suspended devices will
> cause problems in suspend by the drivers ->runtime_suspend() callback
> which will try to cut power.
>
> Fix this by disabling PM domain auto suspending for MUSB until the
> driver can be fixed to properly implement suspend/resume.
>
> Signed-off-by: Kevin Hilman <khilman@ti.com>

I guess it's obvious, but should be stated (I'll update the changelog):

Also note that this prevents the CORE powerdomain from hitting retention
(or off), and thus prevents full-chip retention/off whenever USB devices
are attatched, so this should not be considered as a real fix, but a
workaround that at least prevents driver failure.

Kevin

Re: [PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

Hi,

On Mon, Nov 28, 2011 at 04:56:30PM -0800, Kevin Hilman wrote:
> The MUSB driver does not currently implment suspend/resume callbacks,

this is not entirelly true, actually. Such methods are missing for
omap2430 glue layer, not for MUSB itself. And the fact is that it's only
missing because we failed to use UNIVERSAL_DEV_PM_OPS for declaring
dev_pm_ops structure.

Can you see if this patch helps:

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index fd5dd46..d89caec 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -525,10 +525,8 @@ static int omap2430_runtime_resume(struct device *dev)
 	return 0;
 }
 
-static struct dev_pm_ops omap2430_pm_ops = {
-	.runtime_suspend = omap2430_runtime_suspend,
-	.runtime_resume = omap2430_runtime_resume,
-};
+static UNIVERSAL_DEV_PM_OPS(omap2430_pm_ops, omap2430_runtime_suspend,
+		omap2430_runtime_resume, NULL);
 
 #define DEV_PM_OPS	(&omap2430_pm_ops)
 #else

You also have to remember that for host side PM to work, you depend on
your class driver supporting autosuspend feature. If it doesn't, then
its interface will never call usb_autopm_put_interface() and thus pm
counter will never decrease. Be sure to use a device whose driver
supports autosuspend feature.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Mon, Nov 28, 2011 at 04:56:30PM -0800, Kevin Hilman wrote:
>> The MUSB driver does not currently implment suspend/resume callbacks,
>
> this is not entirelly true, actually. Such methods are missing for
> omap2430 glue layer, not for MUSB itself. And the fact is that it's only
> missing because we failed to use UNIVERSAL_DEV_PM_OPS for declaring
> dev_pm_ops structure.

I guess that also means that nobody has tested MUSB host suspend/resume
with devices attached.

> Can you see if this patch helps:

Sure.

That patch makes sense, and seems necessary, but doesn't fix the problem.

The root of the problem is that the PM domain code will call the
driver's runtime PM methods late in the suspend if the device is not
already runtime suspended.  

In your patch, you make the driver's suspend/resume methods call the
runtime methods, but, the PM core doesn't know that that the device is now
runtime suspended, so the OMAP PM domain code will still call the
driver's runtime PM methods to try and suspend the device.

In the case of this glue layer, the runtime PM methods call some PHY
code which is trying to use I2C.  When this happens late in the suspend
process, I2C may already be suspended, so you get a bunch of I2C
timeouts.

Kevin

Re: [PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[Grazvydas Ignotas]

Hello,

I've been trying to get suspend working with musb compiled in on 3.2.
It seems both patches from this thread are needed, Kevin's patch stops
omap2430_runtime_suspend() from being called at inappropriate time
(when i2c is suspended [1]) and Felipe's patch brings that call back
at better time (when i2c is still active). Perhaps they can be applied
now, as current state crashes for me (see [1])?

However there is a new issue with these patches applied, core_pwrdm no
longer enters low power state, but I think I've found why. After
omap_device_disable_idle_on_suspend() call, _od_suspend_noirq()
function in omap_device.c no longer calls omap_device_idle(), which
means things (clocks?) are not properly stopped before entering
suspend, preventing core_pwrdm from going down.

So I guess the question is, can it be done that
omap2430_runtime_suspend() could access i2c and omap_device_idle()
would be called?

[1] For some reason I get data abort on i2c register access if it's
suspended, not i2c timeouts that some people report..


On Wed, Dec 7, 2011 at 9:37 PM, Kevin Hilman <khilman@ti.com> wrote:
> Felipe Balbi <balbi@ti.com> writes:
>
>> Hi,
>>
>> On Mon, Nov 28, 2011 at 04:56:30PM -0800, Kevin Hilman wrote:
>>> The MUSB driver does not currently implment suspend/resume callbacks,
>>
>> this is not entirelly true, actually. Such methods are missing for
>> omap2430 glue layer, not for MUSB itself. And the fact is that it's only
>> missing because we failed to use UNIVERSAL_DEV_PM_OPS for declaring
>> dev_pm_ops structure.
>
> I guess that also means that nobody has tested MUSB host suspend/resume
> with devices attached.
>
>> Can you see if this patch helps:
>
> Sure.
>
> That patch makes sense, and seems necessary, but doesn't fix the problem.
>
> The root of the problem is that the PM domain code will call the
> driver's runtime PM methods late in the suspend if the device is not
> already runtime suspended.
>
> In your patch, you make the driver's suspend/resume methods call the
> runtime methods, but, the PM core doesn't know that that the device is now
> runtime suspended, so the OMAP PM domain code will still call the
> driver's runtime PM methods to try and suspend the device.
>
> In the case of this glue layer, the runtime PM methods call some PHY
> code which is trying to use I2C.  When this happens late in the suspend
> process, I2C may already be suspended, so you get a bunch of I2C
> timeouts.
>
> Kevin
> --
> 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

-- 
Gražvydas
--
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

Re: [PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

On Mon, 30 Jan 2012 00:57:13 +0200 Grazvydas Ignotas <notasas@gmail.com>
wrote:

> Hello,
> 
> I've been trying to get suspend working with musb compiled in on 3.2.
> It seems both patches from this thread are needed, Kevin's patch stops
> omap2430_runtime_suspend() from being called at inappropriate time
> (when i2c is suspended [1]) and Felipe's patch brings that call back
> at better time (when i2c is still active). Perhaps they can be applied
> now, as current state crashes for me (see [1])?
> 
> However there is a new issue with these patches applied, core_pwrdm no
> longer enters low power state, but I think I've found why. After
> omap_device_disable_idle_on_suspend() call, _od_suspend_noirq()
> function in omap_device.c no longer calls omap_device_idle(), which
> means things (clocks?) are not properly stopped before entering
> suspend, preventing core_pwrdm from going down.
> 
> So I guess the question is, can it be done that
> omap2430_runtime_suspend() could access i2c and omap_device_idle()
> would be called?
> 
> [1] For some reason I get data abort on i2c register access if it's
> suspended, not i2c timeouts that some people report..

1/ I don't know what your problem might be, but I have suspend working
 quite well with musb compiled in.  CORE goes to RET.

 The code is at git://neil.brown.name/gta04 in the 3.2-gta04 branch.
 There are various additions and hacks in there, I don't know which
 ones might be important.

2/ If you see i2c register access abort, I think that is because the iclk is
  being turned off.  Timeouts are because the interrupt is disabled or
  ignored.

good luck,
NeilBrown



> 
> 
> On Wed, Dec 7, 2011 at 9:37 PM, Kevin Hilman <khilman@ti.com> wrote:
> > Felipe Balbi <balbi@ti.com> writes:
> >
> >> Hi,
> >>
> >> On Mon, Nov 28, 2011 at 04:56:30PM -0800, Kevin Hilman wrote:
> >>> The MUSB driver does not currently implment suspend/resume callbacks,
> >>
> >> this is not entirelly true, actually. Such methods are missing for
> >> omap2430 glue layer, not for MUSB itself. And the fact is that it's only
> >> missing because we failed to use UNIVERSAL_DEV_PM_OPS for declaring
> >> dev_pm_ops structure.
> >
> > I guess that also means that nobody has tested MUSB host suspend/resume
> > with devices attached.
> >
> >> Can you see if this patch helps:
> >
> > Sure.
> >
> > That patch makes sense, and seems necessary, but doesn't fix the problem.
> >
> > The root of the problem is that the PM domain code will call the
> > driver's runtime PM methods late in the suspend if the device is not
> > already runtime suspended.
> >
> > In your patch, you make the driver's suspend/resume methods call the
> > runtime methods, but, the PM core doesn't know that that the device is now
> > runtime suspended, so the OMAP PM domain code will still call the
> > driver's runtime PM methods to try and suspend the device.
> >
> > In the case of this glue layer, the runtime PM methods call some PHY
> > code which is trying to use I2C.  When this happens late in the suspend
> > process, I2C may already be suspended, so you get a bunch of I2C
> > timeouts.
> >
> > Kevin
> > --
> > 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
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH/RFC] ARM: OMAP: MUSB: disable omap_device auto-suspend

[Grazvydas Ignotas]

On Tue, Jan 31, 2012 at 12:37 AM, NeilBrown <neilb@suse.de> wrote:
>
> 1/ I don't know what your problem might be, but I have suspend working
>  quite well with musb compiled in.  CORE goes to RET.
>
>  The code is at git://neil.brown.name/gta04 in the 3.2-gta04 branch.
>  There are various additions and hacks in there, I don't know which
>  ones might be important.

ok, I see you have USB gadged compiled as module, I had it compiled in
and that seems to be the problem. With gadget as a module it now
suspends fine for me too, thanks.


-- 
Gražvydas
--
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