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 <[email protected]>
---
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
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.
* Andreas Kemnade <[email protected]> [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 <[email protected]>
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
Hi,
On Tue, 6 Feb 2018 12:46:05 -0600
Bin Liu <[email protected]> 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
Hi,
On Tue, 6 Feb 2018 10:47:25 -0800
Tony Lindgren <[email protected]> wrote:
> * Andreas Kemnade <[email protected]> [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 <[email protected]>
>
> 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
On Tue, Feb 06, 2018 at 10:47:25AM -0800, Tony Lindgren wrote:
> * Andreas Kemnade <[email protected]> [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 <[email protected]>
Thanks for testing it.
Regards,
-Bin.
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 <[email protected]> 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.
On Wed, 7 Feb 2018 14:37:00 -0600
Bin Liu <[email protected]> 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 <[email protected]> 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