[PATCH] usb: musb: fix enumeration after resume

[PATCH] usb: musb: fix enumeration after resume

[Andreas Kemnade]

On dm3730 there are enumeration problems after resume.
Investigation led to the cause that the MUSB_POWER_SOFTCONN
bit is not set. If it was set before suspend (because it
was enabled via musb_pullup()), it is set in
musb_restore_context() so the pullup is enabled. But then
musb_start() is called which overwrites MUSB_POWER and
therefore disables MUSB_POWER_SOFTCONN, so no pullup is
enabled and the device is not enumerated.

So let's do a subset of what musb_start() does
in the same way as musb_suspend() does it. Platform-specific
stuff it still called as there might be some phy-related stuff
which needs to be enabled.
Also interrupts are enabled, as it was the original idea
of calling musb_start() in musb_resume() according to
Commit 6fc6f4b87cb3 ("usb: musb: Disable interrupts on suspend,
enable them on resume")

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/usb/musb/musb_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index ea5013aa69e2..255424aae513 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2709,7 +2709,8 @@ static int musb_resume(struct device *dev)
 	if ((devctl & mask) != (musb->context.devctl & mask))
 		musb->port1_status = 0;
 
-	musb_start(musb);
+	musb_enable_interrupts(musb);
+	musb_platform_enable(musb);
 
 	spin_lock_irqsave(&musb->lock, flags);
 	error = musb_run_resume_work(musb);
-- 
2.11.0

Re: [PATCH] usb: musb: fix enumeration after resume

[Bin Liu]

Hi,

On Sat, Jan 27, 2018 at 09:34:03AM +0100, Andreas Kemnade wrote:
> On dm3730 there are enumeration problems after resume.
> Investigation led to the cause that the MUSB_POWER_SOFTCONN
> bit is not set. If it was set before suspend (because it
> was enabled via musb_pullup()), it is set in
> musb_restore_context() so the pullup is enabled. But then
> musb_start() is called which overwrites MUSB_POWER and
> therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> enabled and the device is not enumerated.
 
Do you see the issue with the v4.15?

> So let's do a subset of what musb_start() does
> in the same way as musb_suspend() does it. Platform-specific
> stuff it still called as there might be some phy-related stuff
> which needs to be enabled.
> Also interrupts are enabled, as it was the original idea
> of calling musb_start() in musb_resume() according to
> Commit 6fc6f4b87cb3 ("usb: musb: Disable interrupts on suspend,
> enable them on resume")

The logic in the fix makes sense, and I do see the same problem with
AM335x on v4.9 kernel, but it doesn't happen on v4.15. I haven't checked
if there is anything after musb_start() which sets MUSB_POWER_SOFTCON
bit.

Regards,
-Bin.

Re: [PATCH] usb: musb: fix enumeration after resume

[Andreas Kemnade]

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

Hi,

On Tue, 6 Feb 2018 12:46:05 -0600
Bin Liu <b-liu@ti.com> wrote:

> Hi,
> 
> On Sat, Jan 27, 2018 at 09:34:03AM +0100, Andreas Kemnade wrote:
> > On dm3730 there are enumeration problems after resume.
> > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > bit is not set. If it was set before suspend (because it
> > was enabled via musb_pullup()), it is set in
> > musb_restore_context() so the pullup is enabled. But then
> > musb_start() is called which overwrites MUSB_POWER and
> > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > enabled and the device is not enumerated.  
>  
> Do you see the issue with the v4.15?
> 
Yes. Tested without other patches. 
It was also there in earlier kernels but I had not had motivation enough
to debug.

So maybe it deserves a CC: Stable


> > So let's do a subset of what musb_start() does
> > in the same way as musb_suspend() does it. Platform-specific
> > stuff it still called as there might be some phy-related stuff
> > which needs to be enabled.
> > Also interrupts are enabled, as it was the original idea
> > of calling musb_start() in musb_resume() according to
> > Commit 6fc6f4b87cb3 ("usb: musb: Disable interrupts on suspend,
> > enable them on resume")  
> 
> The logic in the fix makes sense, and I do see the same problem with
> AM335x on v4.9 kernel, but it doesn't happen on v4.15. I haven't checked
> if there is anything after musb_start() which sets MUSB_POWER_SOFTCON
> bit.
> 
Well reconfiguring gadget from userspace helps.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: musb: fix enumeration after resume

[Bin Liu]

On Tue, Feb 06, 2018 at 08:00:24PM +0100, Andreas Kemnade wrote:
> Hi,
> 
> On Tue, 6 Feb 2018 12:46:05 -0600
> Bin Liu <b-liu@ti.com> wrote:
> 
> > Hi,
> > 
> > On Sat, Jan 27, 2018 at 09:34:03AM +0100, Andreas Kemnade wrote:
> > > On dm3730 there are enumeration problems after resume.
> > > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > > bit is not set. If it was set before suspend (because it
> > > was enabled via musb_pullup()), it is set in
> > > musb_restore_context() so the pullup is enabled. But then
> > > musb_start() is called which overwrites MUSB_POWER and
> > > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > > enabled and the device is not enumerated.  
> >  
> > Do you see the issue with the v4.15?
> > 
> Yes. Tested without other patches. 
> It was also there in earlier kernels but I had not had motivation enough
> to debug.

Applied to my tree. Thanks.

> So maybe it deserves a CC: Stable

I would prefer it is first tested on each stable tree.

> > > So let's do a subset of what musb_start() does
> > > in the same way as musb_suspend() does it. Platform-specific
> > > stuff it still called as there might be some phy-related stuff
> > > which needs to be enabled.
> > > Also interrupts are enabled, as it was the original idea
> > > of calling musb_start() in musb_resume() according to
> > > Commit 6fc6f4b87cb3 ("usb: musb: Disable interrupts on suspend,
> > > enable them on resume")  
> > 
> > The logic in the fix makes sense, and I do see the same problem with
> > AM335x on v4.9 kernel, but it doesn't happen on v4.15. I haven't checked
> > if there is anything after musb_start() which sets MUSB_POWER_SOFTCON
> > bit.

On v4.15, the SESSION bit is set later which sets the SOFTCON bit. I
didn't check how come v4.9 doesn't invoke it though.

Regards,
-Bin.

Re: [PATCH] usb: musb: fix enumeration after resume

[Andreas Kemnade]

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

On Wed, 7 Feb 2018 14:37:00 -0600
Bin Liu <b-liu@ti.com> wrote:

> On Tue, Feb 06, 2018 at 08:00:24PM +0100, Andreas Kemnade wrote:
> > Hi,
> > 
> > On Tue, 6 Feb 2018 12:46:05 -0600
> > Bin Liu <b-liu@ti.com> wrote:
> >   
> > > Hi,
> > > 
> > > On Sat, Jan 27, 2018 at 09:34:03AM +0100, Andreas Kemnade wrote:  
> > > > On dm3730 there are enumeration problems after resume.
> > > > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > > > bit is not set. If it was set before suspend (because it
> > > > was enabled via musb_pullup()), it is set in
> > > > musb_restore_context() so the pullup is enabled. But then
> > > > musb_start() is called which overwrites MUSB_POWER and
> > > > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > > > enabled and the device is not enumerated.    
> > >  
> > > Do you see the issue with the v4.15?
> > >   
> > Yes. Tested without other patches. 
> > It was also there in earlier kernels but I had not had motivation enough
> > to debug.  
> 
> Applied to my tree. Thanks.
> 
> > So maybe it deserves a CC: Stable  
> 
> I would prefer it is first tested on each stable tree.
> 
Tested with 4.16-rc1 with and without that patch
The problem still exists and can be fixed by that patch.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: musb: fix enumeration after resume

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [180127 08:34]:
> On dm3730 there are enumeration problems after resume.
> Investigation led to the cause that the MUSB_POWER_SOFTCONN
> bit is not set. If it was set before suspend (because it
> was enabled via musb_pullup()), it is set in
> musb_restore_context() so the pullup is enabled. But then
> musb_start() is called which overwrites MUSB_POWER and
> therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> enabled and the device is not enumerated.

I just gave this patch a quick try and things seem to behave
for me from PM point of view:

Tested-by: Tony Lindgren <tony@atomide.com>

Unrelated to this patch, I also noticed that we now somehow
higher idle power consumption initially when musb modules are
loaded. It used to idle after that but now to get things to
idle I had to plug and unplug a USB device once to the musb
port.

Regards,

Tony

Re: [PATCH] usb: musb: fix enumeration after resume

[Andreas Kemnade]

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

Hi,

On Tue, 6 Feb 2018 10:47:25 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [180127 08:34]:
> > On dm3730 there are enumeration problems after resume.
> > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > bit is not set. If it was set before suspend (because it
> > was enabled via musb_pullup()), it is set in
> > musb_restore_context() so the pullup is enabled. But then
> > musb_start() is called which overwrites MUSB_POWER and
> > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > enabled and the device is not enumerated.  
> 
> I just gave this patch a quick try and things seem to behave
> for me from PM point of view:
> 
> Tested-by: Tony Lindgren <tony@atomide.com>
> 
> Unrelated to this patch, I also noticed that we now somehow
> higher idle power consumption initially when musb modules are
> loaded. It used to idle after that but now to get things to
> idle I had to plug and unplug a USB device once to the musb
> port.
> 
Hmm, I have seen this effect with some earlier kernels but not with
4.15. My observation is that current consumption went down again after
a modprobe g_ether and ifconfig usb0 up

I was loading modules piece by piece and waited 10s after each and then
measured.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: musb: fix enumeration after resume

[Bin Liu]

On Tue, Feb 06, 2018 at 10:47:25AM -0800, Tony Lindgren wrote:
> * Andreas Kemnade <andreas@kemnade.info> [180127 08:34]:
> > On dm3730 there are enumeration problems after resume.
> > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > bit is not set. If it was set before suspend (because it
> > was enabled via musb_pullup()), it is set in
> > musb_restore_context() so the pullup is enabled. But then
> > musb_start() is called which overwrites MUSB_POWER and
> > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > enabled and the device is not enumerated.
> 
> I just gave this patch a quick try and things seem to behave
> for me from PM point of view:
> 
> Tested-by: Tony Lindgren <tony@atomide.com>

Thanks for testing it.

Regards,
-Bin.