[PATCH] usb: musb: fix context save over suspend.

[PATCH] usb: musb: fix context save over suspend.

[NeilBrown]

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



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>

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

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

Re: [PATCH] usb: musb: fix context save over suspend.

[Igor Grinberg]

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

Re: [PATCH] usb: musb: fix context save over suspend.

[NeilBrown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=US-ASCII, Size: 4671 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.
> -----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-----

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIVAwUBUP21WDnsnt1WYoG5AQIQlQ/+LlXi1ZRXKtO/oj/u4iqs7DL8PNcZX6C7
j7Rrx8nxd2C15pEnxvOYIrojLTrXqz3bgE9NmBvYOYY+dj2/+CaS7RJsTR0gYJi0
AMkLT9k6+edUJ79KtIRmeqbnPEnujOWHkBdg2dLrm4PpYmGUmc8VGeE3+mh3La97
ssZm4gYbip6TPzB4MdvTbhkxbEXJidOcgBJrsdTvMXB8HkZ30Wq6/YELtYoYNpZo
rn+CbQo5Dd0bUb8fQ3Fd5unfXqx5N5txBslwK8JgSA3L7l96d3+q9UOfBg/PsUJJ
WrnFahv11lypajHtnCnXb3z+TsGgV4v46aUZ4Yk+tkwVv81nbORHTRGoaLReRMG2
Sii/xYeugOuhnJI//07yWrnLfunFbJJyHmfZARz/6kKNoIPrZwRDmVeO3+Iu/39R
zmwvJDTqONwenXnZEmxqrON0E/Y8V6hdNlGZdFYIypJr/Ym2rp+R+qcRyEwQxAYi
7h1mACvXE7tYCCoBi+fN5hFaF2VQeN1QqJMTimQjqnkgaI2jQ4Zm7zMCUKREhGPu
3TTvOwuFGM+bwb2eKsW+4zSzebdepXaNjSPCmHSKU++5SVcOMNiZyKlrvoW8znM4
/1Ea+3CmkHqAhmja5Fly4NYL2fdy/NhfIqZI2yIrTAG58iIankQjBHysqAcGrvQp
TplHj7osO40=
=G91I
-----END PGP SIGNATURE-----
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] usb: musb: fix context save over suspend.

[NeilBrown]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.
> -----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-----

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIVAwUBUP21WDnsnt1WYoG5AQIQlQ/+LlXi1ZRXKtO/oj/u4iqs7DL8PNcZX6C7
j7Rrx8nxd2C15pEnxvOYIrojLTrXqz3bgE9NmBvYOYY+dj2/+CaS7RJsTR0gYJi0
AMkLT9k6+edUJ79KtIRmeqbnPEnujOWHkBdg2dLrm4PpYmGUmc8VGeE3+mh3La97
ssZm4gYbip6TPzB4MdvTbhkxbEXJidOcgBJrsdTvMXB8HkZ30Wq6/YELtYoYNpZo
rn+CbQo5Dd0bUb8fQ3Fd5unfXqx5N5txBslwK8JgSA3L7l96d3+q9UOfBg/PsUJJ
WrnFahv11lypajHtnCnXb3z+TsGgV4v46aUZ4Yk+tkwVv81nbORHTRGoaLReRMG2
Sii/xYeugOuhnJI//07yWrnLfunFbJJyHmfZARz/6kKNoIPrZwRDmVeO3+Iu/39R
zmwvJDTqONwenXnZEmxqrON0E/Y8V6hdNlGZdFYIypJr/Ym2rp+R+qcRyEwQxAYi
7h1mACvXE7tYCCoBi+fN5hFaF2VQeN1QqJMTimQjqnkgaI2jQ4Zm7zMCUKREhGPu
3TTvOwuFGM+bwb2eKsW+4zSzebdepXaNjSPCmHSKU++5SVcOMNiZyKlrvoW8znM4
/1Ea+3CmkHqAhmja5Fly4NYL2fdy/NhfIqZI2yIrTAG58iIankQjBHysqAcGrvQp
TplHj7osO40=
=G91I
-----END PGP SIGNATURE-----

Re: [PATCH] usb: musb: fix context save over suspend.

[Igor Grinberg]

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

RE: [PATCH] usb: musb: fix context save over suspend.

[Bilovol, Ruslan]

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

________________________________________
From: linux-usb-owner@vger.kernel.org [linux-usb-owner@vger.kernel.org] on behalf of Igor Grinberg [grinberg@compulab.co.il]
Sent: Tuesday, January 22, 2013 11:12 AM
To: NeilBrown
Cc: Balbi, Felipe; Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org; Kevin Hilman
Subject: Re: [PATCH] usb: musb: fix context save over suspend.

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

Re: [PATCH] usb: musb: fix context save over suspend.

[Kevin Hilman]

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

Re: [PATCH] usb: musb: fix context save over suspend.

[NeilBrown]

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

On Tue, 12 Feb 2013 13:03:36 -0800 Kevin Hilman <khilman@linaro.org> wrote:

> 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.
> 

Odd.  I plug in a mini-A cable, note that the 'mode' file holds
'a_idle' (sometimes just plugging in the cable isn't enough to trigger that,
but sometimes it is....) and suspend/resume work perfectly.  Though after
resume it is back to b_idle.

unplug/replug and it is back to  a_idle.  suspend/resume and back to b_idle.

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

I'm not sure it is possible for the device to be runtime suspended at this
point.  Certainly my device never is, even if it was just before the suspend
sequence started. Something is waking it up...
(instruments the code).

Ahh - usb_suspend() calls choose_wakeup() which might call
pm_runtime_resume() if the could be a need to reprogram the wakeup setting.
As that is a 'might', the device might not be runtime-awake when 'suspend'
runs.

Can you see if this, on top of my previous patch, does any better on your
hardware?

Thanks,
NeilBrown


diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 956db0e..00deb94 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2278,7 +2278,8 @@ static int musb_suspend(struct device *dev)
 	}
 
 	spin_unlock_irqrestore(&musb->lock, flags);
-	musb_save_context(musb);
+	if (!pm_runtime_status_suspended(dev))
+		musb_save_context(musb);
 	return 0;
 }
 
@@ -2289,7 +2290,8 @@ static int musb_resume_noirq(struct device *dev)
 	 * module got reset through the PSC (vs just being disabled).
 	 */
 	struct musb	*musb = dev_to_musb(dev);
-	musb_restore_context(musb);
+	if (!pm_runtime_status_suspended(dev))
+		musb_restore_context(musb);
 	return 0;
 }
 

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

Re: [PATCH] usb: musb: fix context save over suspend.

[NeilBrown]

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

On Tue, 12 Feb 2013 13:03:36 -0800 Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> writes:
> 
> > On Mon, 21 Jan 2013 13:38:59 +0200 Igor Grinberg <grinberg-UTxiZqZC01TGNzoMY4ZM7g@public.gmane.orgil>
> > 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.
> 

Odd.  I plug in a mini-A cable, note that the 'mode' file holds
'a_idle' (sometimes just plugging in the cable isn't enough to trigger that,
but sometimes it is....) and suspend/resume work perfectly.  Though after
resume it is back to b_idle.

unplug/replug and it is back to  a_idle.  suspend/resume and back to b_idle.

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

I'm not sure it is possible for the device to be runtime suspended at this
point.  Certainly my device never is, even if it was just before the suspend
sequence started. Something is waking it up...
(instruments the code).

Ahh - usb_suspend() calls choose_wakeup() which might call
pm_runtime_resume() if the could be a need to reprogram the wakeup setting.
As that is a 'might', the device might not be runtime-awake when 'suspend'
runs.

Can you see if this, on top of my previous patch, does any better on your
hardware?

Thanks,
NeilBrown


diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 956db0e..00deb94 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2278,7 +2278,8 @@ static int musb_suspend(struct device *dev)
 	}
 
 	spin_unlock_irqrestore(&musb->lock, flags);
-	musb_save_context(musb);
+	if (!pm_runtime_status_suspended(dev))
+		musb_save_context(musb);
 	return 0;
 }
 
@@ -2289,7 +2290,8 @@ static int musb_resume_noirq(struct device *dev)
 	 * module got reset through the PSC (vs just being disabled).
 	 */
 	struct musb	*musb = dev_to_musb(dev);
-	musb_restore_context(musb);
+	if (!pm_runtime_status_suspended(dev))
+		musb_restore_context(musb);
 	return 0;
 }
 

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

Re: [PATCH] usb: musb: fix context save over suspend.

[Kevin Hilman]

NeilBrown <neilb@suse.de> writes:

> On Tue, 12 Feb 2013 13:03:36 -0800 Kevin Hilman <khilman@linaro.org> wrote:
>
>> NeilBrown <neilb@suse.de> writes:
>> 

[...]

>> 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.
>> 
>
> Odd.  I plug in a mini-A cable, note that the 'mode' file holds
> 'a_idle' (sometimes just plugging in the cable isn't enough to trigger that,
> but sometimes it is....) and suspend/resume work perfectly.  Though after
> resume it is back to b_idle.
>
> unplug/replug and it is back to  a_idle.  suspend/resume and back to b_idle.
>
>> 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.)
>
> I'm not sure it is possible for the device to be runtime suspended at this
> point.  Certainly my device never is, even if it was just before the suspend
> sequence started. Something is waking it up...
> (instruments the code).
>
> Ahh - usb_suspend() calls choose_wakeup() which might call
> pm_runtime_resume() if the could be a need to reprogram the wakeup setting.
> As that is a 'might', the device might not be runtime-awake when 'suspend'
> runs.
>
> Can you see if this, on top of my previous patch, does any better on your
> hardware?

Yes, this patch adding the check on top of your previous one makes
things work just fine on my hardware (3530/Overo).

Kevin

> Thanks,
> NeilBrown
>
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 956db0e..00deb94 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2278,7 +2278,8 @@ static int musb_suspend(struct device *dev)
>  	}
>  
>  	spin_unlock_irqrestore(&musb->lock, flags);
> -	musb_save_context(musb);
> +	if (!pm_runtime_status_suspended(dev))
> +		musb_save_context(musb);
>  	return 0;
>  }
>  
> @@ -2289,7 +2290,8 @@ static int musb_resume_noirq(struct device *dev)
>  	 * module got reset through the PSC (vs just being disabled).
>  	 */
>  	struct musb	*musb = dev_to_musb(dev);
> -	musb_restore_context(musb);
> +	if (!pm_runtime_status_suspended(dev))
> +		musb_restore_context(musb);
>  	return 0;
>  }
>