| Message ID | 20191126034151.38154-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: musb: fix idling for suspend after disconnect interrupt | expand |
Hello! On 26.11.2019 6:41, Tony Lindgren wrote: > When disconnected as USB B-device, we sometimes get a suspend interrupt > after disconnect interrupt. In that case we have devctl set to 99 with > VBUS still valid and musb_pm_runtime_check_session() wrongly things we Thinks? > have an active session. We have no other interrupts after disconnect > coming in this case at least with the omap2430 glue. > > Let's fix the issue by checking the interrupt status again with > delayed work for the devctl 99 case. In the suspend after disconnect > case the devctl session bit has cleared by then and musb can idle. > For a typical USB B-device connect case we just continue with normal > interrupts. > > Cc: Merlijn Wajer <merlijn@wizzup.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> [...] MBR, Sergei
* Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [191126 10:21]: > Hello! > > On 26.11.2019 6:41, Tony Lindgren wrote: > > > When disconnected as USB B-device, we sometimes get a suspend interrupt > > after disconnect interrupt. In that case we have devctl set to 99 with > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > > Thinks? Thanks will fix. Tony
Hi Tony, Sorry for my late response. On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote: > When disconnected as USB B-device, we sometimes get a suspend interrupt > after disconnect interrupt. In that case we have devctl set to 99 with > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > have an active session. We have no other interrupts after disconnect > coming in this case at least with the omap2430 glue. I don't have an omap2430 platform to test, but its musb doesn't generate DISCONNECT interrupt at all when disconnected from USB host in any case? It is a surprise, the musb core driver expects the DISCONNECT interrupt after VBUS is lost and relies on it to tear down the gadget driver and the state machine. I am wondering how its USB is functional without the DISCONNECT event... -Bin. > > Let's fix the issue by checking the interrupt status again with > delayed work for the devctl 99 case. In the suspend after disconnect > case the devctl session bit has cleared by then and musb can idle. > For a typical USB B-device connect case we just continue with normal > interrupts. > > Cc: Merlijn Wajer <merlijn@wizzup.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/usb/musb/musb_core.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > --- a/drivers/usb/musb/musb_core.c > +++ b/drivers/usb/musb/musb_core.c > @@ -1943,6 +1943,9 @@ ATTRIBUTE_GROUPS(musb); > #define MUSB_QUIRK_B_INVALID_VBUS_91 (MUSB_DEVCTL_BDEVICE | \ > (2 << MUSB_DEVCTL_VBUS_SHIFT) | \ > MUSB_DEVCTL_SESSION) > +#define MUSB_QUIRK_B_DISCONNECT_99 (MUSB_DEVCTL_BDEVICE | \ > + (3 << MUSB_DEVCTL_VBUS_SHIFT) | \ > + MUSB_DEVCTL_SESSION) > #define MUSB_QUIRK_A_DISCONNECT_19 ((3 << MUSB_DEVCTL_VBUS_SHIFT) | \ > MUSB_DEVCTL_SESSION) > > @@ -1965,6 +1968,11 @@ static void musb_pm_runtime_check_session(struct musb *musb) > s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV | > MUSB_DEVCTL_HR; > switch (devctl & ~s) { > + case MUSB_QUIRK_B_DISCONNECT_99: > + musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); > + schedule_delayed_work(&musb->irq_work, > + msecs_to_jiffies(1000)); > + break; > case MUSB_QUIRK_B_INVALID_VBUS_91: > if (musb->quirk_retries && !musb->flush_irq_work) { > musb_dbg(musb, > -- > 2.24.0
Hi, * Bin Liu <b-liu@ti.com> [191212 16:02]: > Hi Tony, > > Sorry for my late response. No worries. > On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote: > > When disconnected as USB B-device, we sometimes get a suspend interrupt > > after disconnect interrupt. In that case we have devctl set to 99 with > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > > have an active session. We have no other interrupts after disconnect > > coming in this case at least with the omap2430 glue. > > I don't have an omap2430 platform to test, but its musb doesn't generate > DISCONNECT interrupt at all when disconnected from USB host in any case? > It is a surprise, the musb core driver expects the DISCONNECT interrupt > after VBUS is lost and relies on it to tear down the gadget driver and > the state machine. I am wondering how its USB is functional without the > DISCONNECT event... We do get DISCONNECT, but we can then get a SUSPEND after DISCONNECT has already happened.. That will wake up musb waiting for further interrupts thinking it's connected. But after that there are no more interrupts as the cable is disconnected so we need to poll the status again. If we see SUSPEND before DISCONNECT, then things idle fine. Regards, Tony
On Thu, Dec 12, 2019 at 08:09:46AM -0800, Tony Lindgren wrote: > Hi, > > * Bin Liu <b-liu@ti.com> [191212 16:02]: > > Hi Tony, > > > > Sorry for my late response. > > No worries. > > > On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote: > > > When disconnected as USB B-device, we sometimes get a suspend interrupt > > > after disconnect interrupt. In that case we have devctl set to 99 with > > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > > > have an active session. We have no other interrupts after disconnect > > > coming in this case at least with the omap2430 glue. > > > > I don't have an omap2430 platform to test, but its musb doesn't generate > > DISCONNECT interrupt at all when disconnected from USB host in any case? > > It is a surprise, the musb core driver expects the DISCONNECT interrupt > > after VBUS is lost and relies on it to tear down the gadget driver and > > the state machine. I am wondering how its USB is functional without the > > DISCONNECT event... > > We do get DISCONNECT, but we can then get a SUSPEND after DISCONNECT > has already happened.. Ahh, I missed the first sentence in the commit log... Now I understand the issue. Thanks. > > That will wake up musb waiting for further interrupts thinking it's > connected. But after that there are no more interrupts as the cable > is disconnected so we need to poll the status again. > > If we see SUSPEND before DISCONNECT, then things idle fine. Does SUSPEND always comes after DISCONNECT on omap2430, or just sometimes? I guess the USB connector has some issue - when DP/DM pins are disconnected, SUSPEND interrupt is generated; when VBUS/GND pins are disconnected, DISCONNECT interrupt is generated. Because DP/DM pins are shorter than VBUS/GND, SUSPEND should come before DISCONNECT. -Bin.
* Bin Liu <b-liu@ti.com> [191212 17:09]: > On Thu, Dec 12, 2019 at 08:09:46AM -0800, Tony Lindgren wrote: > > That will wake up musb waiting for further interrupts thinking it's > > connected. But after that there are no more interrupts as the cable > > is disconnected so we need to poll the status again. > > > > If we see SUSPEND before DISCONNECT, then things idle fine. > > Does SUSPEND always comes after DISCONNECT on omap2430, or just > sometimes? I guess the USB connector has some issue - when DP/DM pins > are disconnected, SUSPEND interrupt is generated; when VBUS/GND pins are > disconnected, DISCONNECT interrupt is generated. Because DP/DM pins are > shorter than VBUS/GND, SUSPEND should come before DISCONNECT. No this does not always happen, but happens annoyingly often for me to notice. Your explanation sounds good though, and also sounds that can be affected by the wear on a micro-USB connector. Regards, Tony
On Tue, Nov 26, 2019 at 09:45:15AM -0800, Tony Lindgren wrote: > * Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [191126 10:21]: > > Hello! > > > > On 26.11.2019 6:41, Tony Lindgren wrote: > > > > > When disconnected as USB B-device, we sometimes get a suspend interrupt > > > after disconnect interrupt. In that case we have devctl set to 99 with > > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > > > > Thinks? > > Thanks will fix. Is this the only update needed? I can fix it locally when queuing the patch. -Bin.
* Bin Liu <b-liu@ti.com> [191212 17:23]: > On Tue, Nov 26, 2019 at 09:45:15AM -0800, Tony Lindgren wrote: > > * Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [191126 10:21]: > > > Hello! > > > > > > On 26.11.2019 6:41, Tony Lindgren wrote: > > > > > > > When disconnected as USB B-device, we sometimes get a suspend interrupt > > > > after disconnect interrupt. In that case we have devctl set to 99 with > > > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > > > > > > Thinks? > > > > Thanks will fix. > > Is this the only update needed? I can fix it locally when queuing the > patch. So it seems. Maybe also add your explanation for why the wrong ordering of interrupts can happen while at it. Regards, Tony
Hi Tony, On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote: > When disconnected as USB B-device, we sometimes get a suspend interrupt > after disconnect interrupt. In that case we have devctl set to 99 with > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > have an active session. We have no other interrupts after disconnect > coming in this case at least with the omap2430 glue. So I had been debugging a issue with musb_hrdc driver preventing a suspend on the pinephone, which is Allwinner A64 platform. Namely, if I have USB connected, and I try to suspend, it would hang until USB is disconnected. After enabling tracing, I realized that is hanging after this commit. Reverting it makes device suspend and resume correctly. Some more of debugging notes can be found at, https://gitlab.com/postmarketOS/pmaports/-/issues/839 I wonder what would be right solution here? Disable this quirk somehow for device? Regards > Let's fix the issue by checking the interrupt status again with > delayed work for the devctl 99 case. In the suspend after disconnect > case the devctl session bit has cleared by then and musb can idle. > For a typical USB B-device connect case we just continue with normal > interrupts. > > Cc: Merlijn Wajer <merlijn@wizzup.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/usb/musb/musb_core.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > --- a/drivers/usb/musb/musb_core.c > +++ b/drivers/usb/musb/musb_core.c > @@ -1943,6 +1943,9 @@ ATTRIBUTE_GROUPS(musb); > #define MUSB_QUIRK_B_INVALID_VBUS_91 (MUSB_DEVCTL_BDEVICE | \ > (2 << MUSB_DEVCTL_VBUS_SHIFT) | \ > MUSB_DEVCTL_SESSION) > +#define MUSB_QUIRK_B_DISCONNECT_99 (MUSB_DEVCTL_BDEVICE | \ > + (3 << MUSB_DEVCTL_VBUS_SHIFT) | \ > + MUSB_DEVCTL_SESSION) > #define MUSB_QUIRK_A_DISCONNECT_19 ((3 << MUSB_DEVCTL_VBUS_SHIFT) | \ > MUSB_DEVCTL_SESSION) > > @@ -1965,6 +1968,11 @@ static void musb_pm_runtime_check_session(struct musb *musb) > s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV | > MUSB_DEVCTL_HR; > switch (devctl & ~s) { > + case MUSB_QUIRK_B_DISCONNECT_99: > + musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); > + schedule_delayed_work(&musb->irq_work, > + msecs_to_jiffies(1000)); > + break; > case MUSB_QUIRK_B_INVALID_VBUS_91: > if (musb->quirk_retries && !musb->flush_irq_work) { > musb_dbg(musb, > -- > 2.24.0
Hi, * Bhushan Shah <bshah@kde.org> [201027 04:55]: > Hi Tony, > > On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote: > > When disconnected as USB B-device, we sometimes get a suspend interrupt > > after disconnect interrupt. In that case we have devctl set to 99 with > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we > > have an active session. We have no other interrupts after disconnect > > coming in this case at least with the omap2430 glue. > > So I had been debugging a issue with musb_hrdc driver preventing a > suspend on the pinephone, which is Allwinner A64 platform. Great. After that you might want to take a look and make sure that musb also gets idled properly during runtime with no devices connected :) > Namely, if I have USB connected, and I try to suspend, it would hang > until USB is disconnected. After enabling tracing, I realized that is > hanging after this commit. Reverting it makes device suspend and resume > correctly. > > Some more of debugging notes can be found at, > > https://gitlab.com/postmarketOS/pmaports/-/issues/839 > > I wonder what would be right solution here? Disable this quirk somehow > for device? Hmm maybe we're just missing the check for suspend here. Maybe give the following untested patch a try? I'll give it a try here too but it might be few days. Seems like we might be able to eventually simplify the suspend and quirk check stuff, but let's fix the $subject issue first. Regards, Tony 8< ---------------------- diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -2005,10 +2005,14 @@ static void musb_pm_runtime_check_session(struct musb *musb) MUSB_DEVCTL_HR; switch (devctl & ~s) { case MUSB_QUIRK_B_DISCONNECT_99: - musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); - schedule_delayed_work(&musb->irq_work, - msecs_to_jiffies(1000)); - break; + if (musb->quirk_retries && !musb->flush_irq_work) { + musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); + schedule_delayed_work(&musb->irq_work, + msecs_to_jiffies(1000)); + musb->quirk_retries--; + break; + } + /* fall through */ case MUSB_QUIRK_B_INVALID_VBUS_91: if (musb->quirk_retries && !musb->flush_irq_work) { musb_dbg(musb,
Hello! On Tue, Oct 27, 2020 at 08:17:41AM +0200, Tony Lindgren wrote: > Hmm maybe we're just missing the check for suspend here. Maybe > give the following untested patch a try? > > I'll give it a try here too but it might be few days. Thanks for quick patch! I tested this on my device and I can confirm that it fixes issue for me. So from my side, Tested-by: Bhushan Shah <bshah@kde.org> Thanks! > Seems like we might be able to eventually simplify the suspend and > quirk check stuff, but let's fix the $subject issue first. > > Regards, > > Tony > > 8< ---------------------- > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > --- a/drivers/usb/musb/musb_core.c > +++ b/drivers/usb/musb/musb_core.c > @@ -2005,10 +2005,14 @@ static void musb_pm_runtime_check_session(struct musb *musb) > MUSB_DEVCTL_HR; > switch (devctl & ~s) { > case MUSB_QUIRK_B_DISCONNECT_99: > - musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); > - schedule_delayed_work(&musb->irq_work, > - msecs_to_jiffies(1000)); > - break; > + if (musb->quirk_retries && !musb->flush_irq_work) { > + musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); > + schedule_delayed_work(&musb->irq_work, > + msecs_to_jiffies(1000)); > + musb->quirk_retries--; > + break; > + } > + /* fall through */ > case MUSB_QUIRK_B_INVALID_VBUS_91: > if (musb->quirk_retries && !musb->flush_irq_work) { > musb_dbg(musb,
* Bhushan Shah <bshah@kde.org> [201027 07:59]: > Hello! > > On Tue, Oct 27, 2020 at 08:17:41AM +0200, Tony Lindgren wrote: > > Hmm maybe we're just missing the check for suspend here. Maybe > > give the following untested patch a try? > > > > I'll give it a try here too but it might be few days. > > Thanks for quick patch! I tested this on my device and I can confirm > that it fixes issue for me. > > So from my side, > > Tested-by: Bhushan Shah <bshah@kde.org> OK good to hear :) Will post a proper patch after testing here too. Regards, Tony > > Seems like we might be able to eventually simplify the suspend and > > quirk check stuff, but let's fix the $subject issue first. > > > > Regards, > > > > Tony > > > > 8< ---------------------- > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > > --- a/drivers/usb/musb/musb_core.c > > +++ b/drivers/usb/musb/musb_core.c > > @@ -2005,10 +2005,14 @@ static void musb_pm_runtime_check_session(struct musb *musb) > > MUSB_DEVCTL_HR; > > switch (devctl & ~s) { > > case MUSB_QUIRK_B_DISCONNECT_99: > > - musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); > > - schedule_delayed_work(&musb->irq_work, > > - msecs_to_jiffies(1000)); > > - break; > > + if (musb->quirk_retries && !musb->flush_irq_work) { > > + musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); > > + schedule_delayed_work(&musb->irq_work, > > + msecs_to_jiffies(1000)); > > + musb->quirk_retries--; > > + break; > > + } > > + /* fall through */ > > case MUSB_QUIRK_B_INVALID_VBUS_91: > > if (musb->quirk_retries && !musb->flush_irq_work) { > > musb_dbg(musb, > > -- > Bhushan Shah > http://blog.bshah.in > IRC Nick : bshah on Freenode > GPG key fingerprint : 0AAC 775B B643 7A8D 9AF7 A3AC FE07 8411 7FBC E11D
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -1943,6 +1943,9 @@ ATTRIBUTE_GROUPS(musb); #define MUSB_QUIRK_B_INVALID_VBUS_91 (MUSB_DEVCTL_BDEVICE | \ (2 << MUSB_DEVCTL_VBUS_SHIFT) | \ MUSB_DEVCTL_SESSION) +#define MUSB_QUIRK_B_DISCONNECT_99 (MUSB_DEVCTL_BDEVICE | \ + (3 << MUSB_DEVCTL_VBUS_SHIFT) | \ + MUSB_DEVCTL_SESSION) #define MUSB_QUIRK_A_DISCONNECT_19 ((3 << MUSB_DEVCTL_VBUS_SHIFT) | \ MUSB_DEVCTL_SESSION) @@ -1965,6 +1968,11 @@ static void musb_pm_runtime_check_session(struct musb *musb) s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV | MUSB_DEVCTL_HR; switch (devctl & ~s) { + case MUSB_QUIRK_B_DISCONNECT_99: + musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n"); + schedule_delayed_work(&musb->irq_work, + msecs_to_jiffies(1000)); + break; case MUSB_QUIRK_B_INVALID_VBUS_91: if (musb->quirk_retries && !musb->flush_irq_work) { musb_dbg(musb,
When disconnected as USB B-device, we sometimes get a suspend interrupt after disconnect interrupt. In that case we have devctl set to 99 with VBUS still valid and musb_pm_runtime_check_session() wrongly things we have an active session. We have no other interrupts after disconnect coming in this case at least with the omap2430 glue. Let's fix the issue by checking the interrupt status again with delayed work for the devctl 99 case. In the suspend after disconnect case the devctl session bit has cleared by then and musb can idle. For a typical USB B-device connect case we just continue with normal interrupts. Cc: Merlijn Wajer <merlijn@wizzup.org> Cc: Pavel Machek <pavel@ucw.cz> Cc: Sebastian Reichel <sre@kernel.org> Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/usb/musb/musb_core.c | 8 ++++++++ 1 file changed, 8 insertions(+)