diff mbox

usb: musb: fix context save over suspend.

Message ID 20130121202831.40a09bbc@notabene.brown (mailing list archive)
State New, archived
Headers show

Commit Message

NeilBrown Jan. 21, 2013, 9:28 a.m. UTC
The standard suspend sequence involves runtime_resuming
devices before suspending the system.
So just saving context in runtime_suspend and restoring it
in runtime resume isn't enough.  We  must also save in "suspend"
and restore in "resume".

Without this patch, and OMAP3 system with off_mode enabled will find
the musb port non-functional after suspend/resume.  With the patch it
works perfectly.

Signed-off-by: NeilBrown <neilb@suse.de>

Comments

Igor Grinberg Jan. 21, 2013, 11:38 a.m. UTC | #1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Neil,

On 01/21/13 11:28, NeilBrown wrote:
> 
> 
> The standard suspend sequence involves runtime_resuming
> devices before suspending the system.
> So just saving context in runtime_suspend and restoring it
> in runtime resume isn't enough.  We  must also save in "suspend"
> and restore in "resume".
> 
> Without this patch, and OMAP3 system with off_mode enabled will find
> the musb port non-functional after suspend/resume.  With the patch it
> works perfectly.

Hmmm... Some time ago, this has been removed in
5d193ce8 (usb: musb: PM: fix context save/restore in suspend/resume path)

Am I missing something? Or things changed and now this patch is correct?

> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index fd34867..b6ccc02 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2225,6 +2225,7 @@ static int musb_suspend(struct device *dev)
>  	}
>  
>  	spin_unlock_irqrestore(&musb->lock, flags);
> +	musb_save_context(musb);
>  	return 0;
>  }
>  
> @@ -2234,6 +2235,8 @@ static int musb_resume_noirq(struct device *dev)
>  	 * unless for some reason the whole soc powered down or the USB
>  	 * module got reset through the PSC (vs just being disabled).
>  	 */
> +	struct musb	*musb = dev_to_musb(dev);
> +	musb_restore_context(musb);
>  	return 0;
>  }
>  

- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJQ/SjTAAoJEBDE8YO64EfaHrsP/2bl4rP6L/tWLSZ+rNEdz6B+
Qo+HVOhnTVsOxgWbbd5VrfhE28jLoFGMslrLuI+geeCcJ1zgwNsahG9C11bygyfu
54hQgkmaxDJPDKAlalcy7VK9C6tOTgQV5iSbuRlemttK879dTrb+33zP6idn5+zK
kxptY38fpmyojnl8gJiVa6Plik/apQcVr+GIx8CMwj+YQC5vkdg7cUEWyngfyk2C
W0U4NceroS8NSjRbcFV3V6Q912TVjKzl+B2yxVD0OBaSK4BpHEncDBXiVx8APq87
4nDeBB5gDXi1rtN3YjcfDaFu0me5qzpYc3JFFidvdLTdXIdvxDzjHgMqsZB8ZBYC
R0e5PtIw/62I90d63JkXZXVRTB7JeZsGfZFY2R7MxBab9or8zz0OyYwGWoW63vzH
oFrwmAkWoD0IEKcfc8+dd99eicgZrmQL6FDWrEMsX+RS34LRtVU30SVAudRhY+CR
MhNCjKyFySwx7wqkGgJl1ECl0Y6U4ua0v4bv7kdE6eyrgbQIkiGliSJ7DhWBPcP6
iMIOTwC7+LwPYP/MX2uYR3DXDfI0XwiqdtyzhD9LJe4PRol8zjozS2j0Y7FriItw
jFqsgCgwDc9j8ufcpXf5ZynJYnlCG0iLuAPEUugZot83/CpxgU++A8cuHqUrOnhH
76L95rflUTkpiQ76ffP7
=jqXb
-----END PGP SIGNATURE-----
--
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
NeilBrown Jan. 21, 2013, 9:38 p.m. UTC | #2
LS0tLS1CRUdJTiBQR1AgU0lHTkVEIE1FU1NBR0UtLS0tLQ0KSGFzaDogU0hBMQ0KDQpPbiBNb24s
IDIxIEphbiAyMDEzIDEzOjM4OjU5ICswMjAwIElnb3IgR3JpbmJlcmcgPGdyaW5iZXJnQGNvbXB1
bGFiLmNvLmlsPg0Kd3JvdGU6DQoNCj4gLS0tLS1CRUdJTiBQR1AgU0lHTkVEIE1FU1NBR0UtLS0t
LQ0KPiBIYXNoOiBTSEExDQo+IA0KPiBIaSBOZWlsLA0KPiANCj4gT24gMDEvMjEvMTMgMTE6Mjgs
IE5laWxCcm93biB3cm90ZToNCj4gPiANCj4gPiANCj4gPiBUaGUgc3RhbmRhcmQgc3VzcGVuZCBz
ZXF1ZW5jZSBpbnZvbHZlcyBydW50aW1lX3Jlc3VtaW5nDQo+ID4gZGV2aWNlcyBiZWZvcmUgc3Vz
cGVuZGluZyB0aGUgc3lzdGVtLg0KPiA+IFNvIGp1c3Qgc2F2aW5nIGNvbnRleHQgaW4gcnVudGlt
ZV9zdXNwZW5kIGFuZCByZXN0b3JpbmcgaXQNCj4gPiBpbiBydW50aW1lIHJlc3VtZSBpc24ndCBl
bm91Z2guICBXZSAgbXVzdCBhbHNvIHNhdmUgaW4gInN1c3BlbmQiDQo+ID4gYW5kIHJlc3RvcmUg
aW4gInJlc3VtZSIuDQo+ID4gDQo+ID4gV2l0aG91dCB0aGlzIHBhdGNoLCBhbmQgT01BUDMgc3lz
dGVtIHdpdGggb2ZmX21vZGUgZW5hYmxlZCB3aWxsIGZpbmQNCj4gPiB0aGUgbXVzYiBwb3J0IG5v
bi1mdW5jdGlvbmFsIGFmdGVyIHN1c3BlbmQvcmVzdW1lLiAgV2l0aCB0aGUgcGF0Y2ggaXQNCj4g
PiB3b3JrcyBwZXJmZWN0bHkuDQo+IA0KPiBIbW1tLi4uIFNvbWUgdGltZSBhZ28sIHRoaXMgaGFz
IGJlZW4gcmVtb3ZlZCBpbg0KPiA1ZDE5M2NlOCAodXNiOiBtdXNiOiBQTTogZml4IGNvbnRleHQg
c2F2ZS9yZXN0b3JlIGluIHN1c3BlbmQvcmVzdW1lIHBhdGgpDQo+IA0KPiBBbSBJIG1pc3Npbmcg
c29tZXRoaW5nPyBPciB0aGluZ3MgY2hhbmdlZCBhbmQgbm93IHRoaXMgcGF0Y2ggaXMgY29ycmVj
dD8NCg0KSGkgSWdvciwNCiB0aGFua3MgZm9yIGFsZXJ0aW5nIG1lIHRvIHRoYXQgcGF0Y2ggLi4u
LiBkb2VzIGFueW9uZSBlbHNlIGdldCB0aGUgZmVlbGluZw0KIHRoYXQgcG93ZXIgbWFuYWdlbWVu
dCB0byB0b28gY29tcGxleCB0byBiZSB1bmRlcnN0b29kIGJ5IGEgbWVyZSBodW1hbj8NCg0KIFRo
YXQgY29tbWl0ICg1ZDE5M2NlOCkgc3VnZ2VzdHMgdGhhdCB0aGUgbXVzYi1oZHJjIGRldmljZSBp
cyBhbg0KICdvbWFwX2RldmljZScsIG9yIG1heWJlIGhhcyBhIFBNIGRvbWFpbiBzZXQgdG8gc29t
ZXRoaW5nIGVsc2UuDQogSG93ZXZlciBpdCBpc24ndC9kb2Vzbid0LiAgZGV2LT5wbV9kb21haW4g
aXMgTlVMTC4gIFNvIG5vIFBNIGRvbWFpbiBsYXllcg0KIHdpbGwgZXZlciBjYWxsIHRoZSBtdXNi
X2NvcmUgbXVzYl9ydW50aW1lX3N1c3BlbmQvcmVzdW1lLg0KDQogVGhlIHBhcmVudCBkZXZpY2Ug
LSBtdXNiLW9tYXAyNDMwIC0gaXMgYW4gb21hcCBkZXZpY2UsIGRvZXMgaGF2ZSBwbV9kb21haW4N
CiBzZXQsIGFuZCBkb2VzIGhhdmUgaXRzIG9tYXAyNDMwX3J1bnRpbWVfc3VzcGVuZC9yZXN1bWUg
Y2FsbGVkIGZvciBzeXN0ZW0NCiBzdXNwZW5kIGFuZCBzbyB0aGUgY29udGV4dCBmb3IgdGhhdCBk
ZXZpY2UgaXMgc2F2ZWQgYW5kIHJlc3RvcmVkLg0KIEhvd2V2ZXIgdGhhdCBkb2Vzbid0IGhlbHAg
dGhlIGNvbnRleHQgZm9yIG11c2ItaGRyYy4NCg0KIFdoZXRoZXIgbXVzYiBldmVyIHdhcyBhbiBv
bWFwX2RldmljZSBpcyBiZXlvbmQgbXkgYXJjaGFlb2xvZ2ljYWwgc2tpbGxzIHRvDQogZGV0ZXJt
aW5lLg0KDQogS2V2aW46ICBXYXMgbXVzYi1oZHJjIGV2ZXIgYSBkZXZpY2Ugd2l0aCBhIHBtX2Rv
bWFpbj8gb3Igd2FzIGl0IG9ubHkgZXZlcg0KICAgICB0aGUgdmFyaW91cyBwb3NzaWJsZSBwYXJl
bnRzIHRoYXQgaGFkIGRvbWFpbnM/DQogICAgIEFyZSB5b3UgYWJsZSB0byBkZWZlbmQgeW91ciBl
YXJsaWVyIHBhdGNoIGluIHRvZGF5J3Mga2VybmVsPyAgSXQNCiAgICAgY2VydGFpbmx5IGNhdXNl
cyBteSBkZXZpY2Ugbm90IHRvIHdvcmsgcHJvcGVybHkuDQoNClRoYW5rcywNCk5laWxCcm93bg0K
DQoNCg0KPiANCj4gPiANCj4gPiBTaWduZWQtb2ZmLWJ5OiBOZWlsQnJvd24gPG5laWxiQHN1c2Uu
ZGU+DQo+ID4gDQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL211c2IvbXVzYl9jb3JlLmMg
Yi9kcml2ZXJzL3VzYi9tdXNiL211c2JfY29yZS5jDQo+ID4gaW5kZXggZmQzNDg2Ny4uYjZjY2Mw
MiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL3VzYi9tdXNiL211c2JfY29yZS5jDQo+ID4gKysr
IGIvZHJpdmVycy91c2IvbXVzYi9tdXNiX2NvcmUuYw0KPiA+IEBAIC0yMjI1LDYgKzIyMjUsNyBA
QCBzdGF0aWMgaW50IG11c2Jfc3VzcGVuZChzdHJ1Y3QgZGV2aWNlICpkZXYpDQo+ID4gIAl9DQo+
ID4gIA0KPiA+ICAJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmbXVzYi0+bG9jaywgZmxhZ3MpOw0K
PiA+ICsJbXVzYl9zYXZlX2NvbnRleHQobXVzYik7DQo+ID4gIAlyZXR1cm4gMDsNCj4gPiAgfQ0K
PiA+ICANCj4gPiBAQCAtMjIzNCw2ICsyMjM1LDggQEAgc3RhdGljIGludCBtdXNiX3Jlc3VtZV9u
b2lycShzdHJ1Y3QgZGV2aWNlICpkZXYpDQo+ID4gIAkgKiB1bmxlc3MgZm9yIHNvbWUgcmVhc29u
IHRoZSB3aG9sZSBzb2MgcG93ZXJlZCBkb3duIG9yIHRoZSBVU0INCj4gPiAgCSAqIG1vZHVsZSBn
b3QgcmVzZXQgdGhyb3VnaCB0aGUgUFNDICh2cyBqdXN0IGJlaW5nIGRpc2FibGVkKS4NCj4gPiAg
CSAqLw0KPiA+ICsJc3RydWN0IG11c2IJKm11c2IgPSBkZXZfdG9fbXVzYihkZXYpOw0KPiA+ICsJ
bXVzYl9yZXN0b3JlX2NvbnRleHQobXVzYik7DQo+ID4gIAlyZXR1cm4gMDsNCj4gPiAgfQ0KPiA+
ICANCj4gDQo+IC0gLS0gDQo+IFJlZ2FyZHMsDQo+IElnb3IuDQo+IC0tLS0tQkVHSU4gUEdQIFNJ
R05BVFVSRS0tLS0tDQo+IFZlcnNpb246IEdudVBHIHYyLjAuMTcgKEdOVS9MaW51eCkNCj4gQ29t
bWVudDogVXNpbmcgR251UEcgd2l0aCBNb3ppbGxhIC0gaHR0cDovL2VuaWdtYWlsLm1vemRldi5v
cmcvDQo+IA0KPiBpUUljQkFFQkFnQUdCUUpRL1NqVEFBb0pFQkRFOFlPNjRFZmFIcnNQLzJibDRy
UDZML3RXTFNaK3JORWR6NkIrDQo+IFFvK0hWT2huVFZzT3hnV2JiZDVWcmZoRTI4akxvRkdNc2xy
THVJK2dlZUNjSjF6Z3dOc2FoRzlDMTFieWd5ZnUNCj4gNTRoUWdrbWF4REpQREtBbGFsY3k3Vks5
QzZ0T1RnUVY1aVNidVJsZW10dEs4NzlkVHJiKzMzelA2aWRuNSt6Sw0KPiBreHB0WTM4ZnBteW9q
bmw4Z0ppVmE2UGxpay9hcFFjVnIrR0l4OENNd2orWVFDNXZrZGc3Y1VFV3luZ2Z5azJDDQo+IFcw
VTROY2Vyb1M4TlNqUmJjRlYzVjZROTEyVFZqS3psK0IyeXhWRDBPQmFTSzRCcEhFbmNEQlhpVng4
QVBxODcNCj4gNG5EZUJCNWdEWGkxcnROM1lqY2ZEYUZ1MG1lNXF6cFljM0pGRmlkdmRMVGRYSWR2
eER6akhnTXFzWkI4WkJZQw0KPiBSMGU1UHRJdy82Mkk5MGQ2M0prWFpYVlJUQjdKZVpzR2ZaRlky
UjdNeEJhYjlvcjh6ejBPeVl3R1dvVzYzdnpIDQo+IG9GcndtQWtXb0QwSUVLY2ZjOCtkZDk5ZWlj
Z1pybVFMNkZEV3JFTXNYK1JTMzRMUnRWVTMwU1ZBdWRSaFkrQ1INCj4gTWhOQ2pLeUZ5U3d4N3dx
a0dnSmwxRUNsMFk2VTR1YTB2NGJ2N2tkRTZleXJnYlFJa2lHbGlTSjdEaFdCUGNQNg0KPiBpTUlP
VHdDNytMd1BZUC9NWDJ1WVIzRFhEZkkwWHdpcWR0eXpoRDlMSmU0UFJvbDh6am96UzJqMFk3RnJp
SXR3DQo+IGpGcXNnQ2d3RGM5ajh1ZmNwWGY1WnluSllubENHMGlMdUFQRVV1Z1pvdDgzL0NweGdV
KytBOGN1SHFVck9uaEgNCj4gNzZMOTVyZmxVVGtwaVE3NmZmUDcNCj4gPWpxWGINCj4gLS0tLS1F
TkQgUEdQIFNJR05BVFVSRS0tLS0tDQoNCi0tLS0tQkVHSU4gUEdQIFNJR05BVFVSRS0tLS0tDQpW
ZXJzaW9uOiBHbnVQRyB2Mi4wLjE5IChHTlUvTGludXgpDQoNCmlRSVZBd1VCVVAyMVdEbnNudDFX
WW9HNUFRSVFsUS8rTGxYaTFaUlhLdE8vb2ovdTRpcXM3REw4UE5jWlg2QzcNCmo3UnJ4OG54ZDJD
MTVwRW54dk9ZSXJvakxUclhxejNiZ0U5Tm1CdllPWVkrZGoyLytDYVM3UkpzVFIwZ1lKaTANCkFN
a0xUOWs2K2VkVUo3OUt0SVJtZXFiblBFbnVqT1dIa0JkZzJkTHJtNFBwWW1HVW1jOFZHZUUzK21o
M0xhOTcNCnNzWm00Z1liaXA2VFB6QjRNZHZUYmhreGJFWEppZE9jZ0JKcnNkVHZNWEI4SGtaMzBX
cTYvWUVMdFlvWU5wWm8NCnJuK0NiUW81RGQwYlViOGZRM0ZkNXVuZlhxeDVONXR4QnNsd0s4SmdT
QTNMN2w5NmQzK3E5VU9mQmcvUHNVSkoNCldybkZhaHYxMWx5cGFqSHRuQ25YYjN6K1RzR2dWNHY0
NmFVWjRZayt0a3dWdjgxbmJPUkhUUkdvYUxSZVJNRzINClNpaS94WWV1Z091aG5KSS8vMDd5V3Ju
TGZ1bkZiSkp5SG1mWkFSei82a0tOb0lQclp3UkRtVmVPMytJdS8zOVINCnptd3ZKRFRxT053ZW5Y
blpFbXhxck9OMEUvWThWNmhkTmxHWmRGWUl5cEpyL1ltMnJwK1IrcWNSeUV3UXhBWWkNCjdoMW1B
Q3ZYRTd0WUNDb0JpK2ZONWhGYUYyVlFlTjFRcUpNVGltUWpxbmtnYUkyalE0Wm03ek1DVUtSRWhH
UHUNCjNUVHZPd3VGR00rYndiMmVLc1crNHpTemViZGVwWGFOalNQQ21IU0tVKys1U1ZjT01OaVp5
S2xydm9XOHpuTTQNCi8xRWErM0Nta0hxQWhtamE1Rmx5NE5ZTDJmZHkvTmhmSXFaSTJ5SXJUQUc1
OGlJYW5rUWpCSHlzcUFjR3J2UXANClRwbEhqN29zTzQwPQ0KPUc5MUkNCi0tLS0tRU5EIFBHUCBT
SUdOQVRVUkUtLS0tLQ0K
--
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
Igor Grinberg Jan. 22, 2013, 9:12 a.m. UTC | #3
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

It looks like Kevin has a new address:
Kevin Hilman <khilman@deeprootsystems.com>

On 01/21/13 23:38, NeilBrown wrote:
> On Mon, 21 Jan 2013 13:38:59 +0200 Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> 
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
> 
>> Hi Neil,
> 
>> On 01/21/13 11:28, NeilBrown wrote:
>>>
>>>
>>> The standard suspend sequence involves runtime_resuming
>>> devices before suspending the system.
>>> So just saving context in runtime_suspend and restoring it
>>> in runtime resume isn't enough.  We  must also save in "suspend"
>>> and restore in "resume".
>>>
>>> Without this patch, and OMAP3 system with off_mode enabled will find
>>> the musb port non-functional after suspend/resume.  With the patch it
>>> works perfectly.
> 
>> Hmmm... Some time ago, this has been removed in
>> 5d193ce8 (usb: musb: PM: fix context save/restore in suspend/resume path)
> 
>> Am I missing something? Or things changed and now this patch is correct?
> 
> Hi Igor,
>  thanks for alerting me to that patch .... does anyone else get the feeling
>  that power management to too complex to be understood by a mere human?
> 
>  That commit (5d193ce8) suggests that the musb-hdrc device is an
>  'omap_device', or maybe has a PM domain set to something else.
>  However it isn't/doesn't.  dev->pm_domain is NULL.  So no PM domain layer
>  will ever call the musb_core musb_runtime_suspend/resume.
> 
>  The parent device - musb-omap2430 - is an omap device, does have pm_domain
>  set, and does have its omap2430_runtime_suspend/resume called for system
>  suspend and so the context for that device is saved and restored.
>  However that doesn't help the context for musb-hdrc.
> 
>  Whether musb ever was an omap_device is beyond my archaeological skills to
>  determine.
> 
>  Kevin:  Was musb-hdrc ever a device with a pm_domain? or was it only ever
>      the various possible parents that had domains?
>      Are you able to defend your earlier patch in today's kernel?  It
>      certainly causes my device not to work properly.
> 
> Thanks,
> NeilBrown
> 
> 
> 
> 
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>
>>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>>> index fd34867..b6ccc02 100644
>>> --- a/drivers/usb/musb/musb_core.c
>>> +++ b/drivers/usb/musb/musb_core.c
>>> @@ -2225,6 +2225,7 @@ static int musb_suspend(struct device *dev)
>>>  	}
>>>  
>>>  	spin_unlock_irqrestore(&musb->lock, flags);
>>> +	musb_save_context(musb);
>>>  	return 0;
>>>  }
>>>  
>>> @@ -2234,6 +2235,8 @@ static int musb_resume_noirq(struct device *dev)
>>>  	 * unless for some reason the whole soc powered down or the USB
>>>  	 * module got reset through the PSC (vs just being disabled).
>>>  	 */
>>> +	struct musb	*musb = dev_to_musb(dev);
>>> +	musb_restore_context(musb);
>>>  	return 0;
>>>  }
>>>  
> 
>> - -- 
>> Regards,
>> Igor.
>> 


- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJQ/lgWAAoJEBDE8YO64EfacIAP/3nyXjs8QQpcD6RcNuRSLp3O
veKU2grzsUOL/Eu/8TQMM7WDz5n8YlycQ6/THNGGYojjOlEimDC02wbsI4gc5j41
QC1/XGf62Nlxv6CzORzkGkUoKXtVWzgMYKddWKPEwsXMKPun/LH4ZGDp+7Rkfcmu
U0k7LM1Pv487iG9pF3Bq5BPYeMxyxyFJC0PiQEK1ZE65RKWbCvInibc7p1bvZ+XX
JQxf8Qx1p/kBhqWc6LLpcHT5Z3B/F+3rxEhvf8lSu5fdRPHFffcmuX7bIbC/GlTe
e6mODA114mtn5YbaKCmnYExvJcpz4Nziy+8fGLJ56aAyeKI1wsOJzhWDpSKwQiIF
CVtYulk5iIfaeUA4sAzvRqEu7dJuaVgm6mEeGHQjebPastYhK7vHjiEOJJ2+LQrs
698A9wMLckp4AQ75HiwXRgi5N0W528gD8piNoIA9Sh1LwyytIa5Wg7uYw14UjtLJ
QIerm0v6Ay+8FbVfmpm9k0v3HkYfv0ZVTSdtIXVAE30+WKIBTn0yszxWYo6JZtvj
p0NEFUNVuR3C9k/xyzkcqwJh8Q6DrleWJeHWL59xgWWKzfeDKjU/DMOuWmuVEkTO
aRdAlW32VDtUeWlWz3Jz3IOWZRJQKCW2o97n/MDyxwMoRiMWcHxYw6jti6se9S7a
IGZeEMCcf39fnH5o7i2q
=nwGe
-----END PGP SIGNATURE-----
--
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
Ruslan Bilovol Jan. 23, 2013, 11:15 a.m. UTC | #4
Hi,

I faced the same issue on OMAP4 and made similar fix a week ago: http://review.omapzoom.org/31700 but in this patch I also check is the MUSB is already suspended (so the context is already saved) in .suspend callback so reading/writing to MUSB register is more safe.
It is almost same solution as we had a long time ago.

--
Best regards,
Ruslan Bilovol
Kevin Hilman Feb. 12, 2013, 9:03 p.m. UTC | #5
NeilBrown <neilb@suse.de> writes:

> On Mon, 21 Jan 2013 13:38:59 +0200 Igor Grinberg <grinberg@compulab.co.il>
> wrote:
>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>> 
>> Hi Neil,
>> 
>> On 01/21/13 11:28, NeilBrown wrote:
>> > 
>> > 
>> > The standard suspend sequence involves runtime_resuming
>> > devices before suspending the system.
>> > So just saving context in runtime_suspend and restoring it
>> > in runtime resume isn't enough.  We  must also save in "suspend"
>> > and restore in "resume".
>> > 
>> > Without this patch, and OMAP3 system with off_mode enabled will find
>> > the musb port non-functional after suspend/resume.  With the patch it
>> > works perfectly.
>> 
>> Hmmm... Some time ago, this has been removed in
>> 5d193ce8 (usb: musb: PM: fix context save/restore in suspend/resume path)
>> 
>> Am I missing something? Or things changed and now this patch is correct?
>
> Hi Igor,
>  thanks for alerting me to that patch .... does anyone else get the feeling
>  that power management to too complex to be understood by a mere human?

Yes.  ;)

>  That commit (5d193ce8) suggests that the musb-hdrc device is an
>  'omap_device', or maybe has a PM domain set to something else.
>  However it isn't/doesn't.  dev->pm_domain is NULL.  So no PM domain layer
>  will ever call the musb_core musb_runtime_suspend/resume.
>
>  The parent device - musb-omap2430 - is an omap device, does have pm_domain
>  set, and does have its omap2430_runtime_suspend/resume called for system
>  suspend and so the context for that device is saved and restored.
>  However that doesn't help the context for musb-hdrc.
>
>  Whether musb ever was an omap_device is beyond my archaeological skills to
>  determine.
>
>  Kevin:  Was musb-hdrc ever a device with a pm_domain? or was it only ever
>      the various possible parents that had domains?
>      Are you able to defend your earlier patch in today's kernel?  It
>      certainly causes my device not to work properly.

Sorry for the delay here, I'm back to a place where I can test this on
real hardware.

My patch was fixing a real hang when musb was built-in (or loaded), in
host-mode (mini-A cable attached) but no devices attached.  I just tried
to reproduce this, and with your patch, the system hangs during suspend.

That being said, your description makes sense why this context
save/restore is needed.  Perhaps your patch needs to add a check whether
the device is runtime suspended (I gather this is what Ruslan's patch is
doing.)

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
diff mbox

Patch

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index fd34867..b6ccc02 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2225,6 +2225,7 @@  static int musb_suspend(struct device *dev)
 	}
 
 	spin_unlock_irqrestore(&musb->lock, flags);
+	musb_save_context(musb);
 	return 0;
 }
 
@@ -2234,6 +2235,8 @@  static int musb_resume_noirq(struct device *dev)
 	 * unless for some reason the whole soc powered down or the USB
 	 * module got reset through the PSC (vs just being disabled).
 	 */
+	struct musb	*musb = dev_to_musb(dev);
+	musb_restore_context(musb);
 	return 0;
 }