| Message ID | 20180427130005.6711-1-dg@emlix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Apr 27, 2018 at 03:00:05PM +0200, Daniel Glöckner wrote: > It has been observed that writing 0xF2 to the power register while it > reads as 0xF4 results in the register having the value 0xF0, i.e. clearing > RESUME and setting SUSPENDM in one go does not work. It might also violate > the USB spec to transition directly from resume to suspend, especially > when not taking T_DRSMDN into account. But this is what happens when a > remote wakeup occurs between SetPortFeature USB_PORT_FEAT_SUSPEND on the > root hub and musb_bus_suspend being called. > > This commit returns -EBUSY when musb_bus_suspend is called while remote > wakeup is signalled and thus avoids to reset the RESUME bit. Remember that > resume can be signalled only when the bus was already suspended, so it is > safe to skip the following steps even when musb_hub_control ignores the what do you mean by "skip the following steps"? > error returned by musb_port_suspend. > > The resume part of musb_port_suspend is modified to also accept a pending > remote wakeup, to bring it to the end after T_DRSMDN has passed. Can you please explain more here? I am not sure when musb_port_suspend() is involved in resume by remote wakeup, and what case is a 'pending' remote wakeup? > > Signed-off-by: Daniel Glöckner <dg@emlix.com> > --- > drivers/usb/musb/musb_host.c | 5 ++++- > drivers/usb/musb/musb_host.h | 7 +++++-- > drivers/usb/musb/musb_virthub.c | 27 ++++++++++++++++----------- > 3 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c > index 3a8451a..d05cb03 100644 > --- a/drivers/usb/musb/musb_host.c > +++ b/drivers/usb/musb/musb_host.c > @@ -2522,8 +2522,11 @@ static int musb_bus_suspend(struct usb_hcd *hcd) > { > struct musb *musb = hcd_to_musb(hcd); > u8 devctl; > + int ret; > > - musb_port_suspend(musb, true); > + ret = musb_port_suspend(musb, true); > + if (ret) > + return ret; > > if (!is_host_active(musb)) > return 0; > diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h > index 72392bb..2999845 100644 > --- a/drivers/usb/musb/musb_host.h > +++ b/drivers/usb/musb/musb_host.h > @@ -67,7 +67,7 @@ extern void musb_host_rx(struct musb *, u8); > extern void musb_root_disconnect(struct musb *musb); > extern void musb_host_resume_root_hub(struct musb *musb); > extern void musb_host_poke_root_hub(struct musb *musb); > -extern void musb_port_suspend(struct musb *musb, bool do_suspend); > +extern int musb_port_suspend(struct musb *musb, bool do_suspend); > extern void musb_port_reset(struct musb *musb, bool do_reset); > extern void musb_host_finish_resume(struct work_struct *work); > #else > @@ -99,7 +99,10 @@ static inline void musb_root_disconnect(struct musb *musb) {} > static inline void musb_host_resume_root_hub(struct musb *musb) {} > static inline void musb_host_poll_rh_status(struct musb *musb) {} > static inline void musb_host_poke_root_hub(struct musb *musb) {} > -static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {} > +static inline int musb_port_suspend(struct musb *musb, bool do_suspend) > +{ > + return 0; > +} > static inline void musb_port_reset(struct musb *musb, bool do_reset) {} > static inline void musb_host_finish_resume(struct work_struct *work) {} > #endif > diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c > index 5165d2b..41b44ce 100644 > --- a/drivers/usb/musb/musb_virthub.c > +++ b/drivers/usb/musb/musb_virthub.c > @@ -48,14 +48,14 @@ void musb_host_finish_resume(struct work_struct *work) > spin_unlock_irqrestore(&musb->lock, flags); > } > > -void musb_port_suspend(struct musb *musb, bool do_suspend) > +int musb_port_suspend(struct musb *musb, bool do_suspend) > { > struct usb_otg *otg = musb->xceiv->otg; > u8 power; > void __iomem *mbase = musb->mregs; > > if (!is_host_active(musb)) > - return; > + return 0; > > /* NOTE: this doesn't necessarily put PHY into low power mode, > * turning off its clock; that's a function of PHY integration and > @@ -66,16 +66,20 @@ void musb_port_suspend(struct musb *musb, bool do_suspend) > if (do_suspend) { > int retries = 10000; > > - power &= ~MUSB_POWER_RESUME; > - power |= MUSB_POWER_SUSPENDM; > - musb_writeb(mbase, MUSB_POWER, power); > + if (power & MUSB_POWER_RESUME) > + return -EBUSY; > > - /* Needed for OPT A tests */ > - power = musb_readb(mbase, MUSB_POWER); > - while (power & MUSB_POWER_SUSPENDM) { > + if (!(power & MUSB_POWER_SUSPENDM)) { > + power |= MUSB_POWER_SUSPENDM; > + musb_writeb(mbase, MUSB_POWER, power); > + > + /* Needed for OPT A tests */ > power = musb_readb(mbase, MUSB_POWER); > - if (retries-- < 1) > - break; > + while (power & MUSB_POWER_SUSPENDM) { > + power = musb_readb(mbase, MUSB_POWER); > + if (retries-- < 1) > + break; > + } > } > > musb_dbg(musb, "Root port suspended, power %02x", power); > @@ -100,7 +104,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend) > musb_dbg(musb, "bogus rh suspend? %s", > usb_otg_state_string(musb->xceiv->otg->state)); > } > - } else if (power & MUSB_POWER_SUSPENDM) { > + } else if (power & (MUSB_POWER_SUSPENDM | MUSB_POWER_RESUME)) { > power &= ~MUSB_POWER_SUSPENDM; > power |= MUSB_POWER_RESUME; > musb_writeb(mbase, MUSB_POWER, power); > @@ -111,6 +115,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend) > schedule_delayed_work(&musb->finish_resume_work, > msecs_to_jiffies(USB_RESUME_TIMEOUT)); > } > + return 0; > } > > void musb_port_reset(struct musb *musb, bool do_reset) Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, May 01, 2018 at 08:48:11AM -0500, Bin Liu wrote: > On Fri, Apr 27, 2018 at 03:00:05PM +0200, Daniel Glöckner wrote: > > It has been observed that writing 0xF2 to the power register while it > > reads as 0xF4 results in the register having the value 0xF0, i.e. clearing > > RESUME and setting SUSPENDM in one go does not work. It might also violate > > the USB spec to transition directly from resume to suspend, especially > > when not taking T_DRSMDN into account. But this is what happens when a > > remote wakeup occurs between SetPortFeature USB_PORT_FEAT_SUSPEND on the > > root hub and musb_bus_suspend being called. > > > > This commit returns -EBUSY when musb_bus_suspend is called while remote > > wakeup is signalled and thus avoids to reset the RESUME bit. Remember that > > resume can be signalled only when the bus was already suspended, so it is > > safe to skip the following steps even when musb_hub_control ignores the > > what do you mean by "skip the following steps"? Setting USB_PORT_STAT_SUSPEND in musb->port1_status, changing musb->xceiv->otg->state, setting musb->is_active, etc. > > error returned by musb_port_suspend. > > > > The resume part of musb_port_suspend is modified to also accept a pending > > remote wakeup, to bring it to the end after T_DRSMDN has passed. > > Can you please explain more here? I am not sure when musb_port_suspend() > is involved in resume by remote wakeup, and what case is a 'pending' > remote wakeup? With 'pending' I was referring to the situation when MUSB_POWER_RESUME has been set by the controller in the power register as a result of of a detected remote wakeup. I see... finish_resume_work is already scheduled by musb_stage0_irq. So the condition of the if statement probably does not need to be changed. I'll test without that part and make a v2 patch if it works. Btw., do you know why that 10000 iterations while loop is needed in musb_port_suspend to pass the OTG Protocol Test as indicated by the comment? Best regards, Daniel
On Wed, May 02, 2018 at 03:12:16AM +0200, Daniel Glöckner wrote: > Hi, > > On Tue, May 01, 2018 at 08:48:11AM -0500, Bin Liu wrote: > > On Fri, Apr 27, 2018 at 03:00:05PM +0200, Daniel Glöckner wrote: > > > It has been observed that writing 0xF2 to the power register while it > > > reads as 0xF4 results in the register having the value 0xF0, i.e. clearing > > > RESUME and setting SUSPENDM in one go does not work. It might also violate > > > the USB spec to transition directly from resume to suspend, especially > > > when not taking T_DRSMDN into account. But this is what happens when a > > > remote wakeup occurs between SetPortFeature USB_PORT_FEAT_SUSPEND on the > > > root hub and musb_bus_suspend being called. > > > > > > This commit returns -EBUSY when musb_bus_suspend is called while remote > > > wakeup is signalled and thus avoids to reset the RESUME bit. Remember that > > > resume can be signalled only when the bus was already suspended, so it is > > > safe to skip the following steps even when musb_hub_control ignores the > > > > what do you mean by "skip the following steps"? > > Setting USB_PORT_STAT_SUSPEND in musb->port1_status, changing > musb->xceiv->otg->state, setting musb->is_active, etc. Okay, but I am not sure how "even musb_hub_control ignores the error..." is relevant in this statement. The steps are safely skipped in musb_port_suspend() when MUSB_POWER_RESUME bit is set, no matter how musb_hub_control() to deal with the return code from musb_port_suspend(). Did I misunderstand anything? > > > > error returned by musb_port_suspend. > > > > > > The resume part of musb_port_suspend is modified to also accept a pending > > > remote wakeup, to bring it to the end after T_DRSMDN has passed. > > > > Can you please explain more here? I am not sure when musb_port_suspend() > > is involved in resume by remote wakeup, and what case is a 'pending' > > remote wakeup? > > With 'pending' I was referring to the situation when MUSB_POWER_RESUME > has been set by the controller in the power register as a result of > of a detected remote wakeup. Okay. > > I see... finish_resume_work is already scheduled by musb_stage0_irq. Yes. > So the condition of the if statement probably does not need to be I think so too. BTY, I don't think this pending remote wakeup situation will trigger ClearPortFeature, which is the only case that invokes that if statement you changed. > changed. I'll test without that part and make a v2 patch if it works. > > Btw., do you know why that 10000 iterations while loop is needed in > musb_port_suspend to pass the OTG Protocol Test as indicated by the > comment? Unfortunately I don't. There are many mysteries in the musb drivers :) Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Am 05/02/18 um 14:40 schrieb Bin Liu: > On Wed, May 02, 2018 at 03:12:16AM +0200, Daniel Glöckner wrote: >> On Tue, May 01, 2018 at 08:48:11AM -0500, Bin Liu wrote: >>> On Fri, Apr 27, 2018 at 03:00:05PM +0200, Daniel Glöckner wrote: >>>> This commit returns -EBUSY when musb_bus_suspend is called while remote >>>> wakeup is signalled and thus avoids to reset the RESUME bit. Remember that >>>> resume can be signalled only when the bus was already suspended, so it is >>>> safe to skip the following steps even when musb_hub_control ignores the >>> >>> what do you mean by "skip the following steps"? >> >> Setting USB_PORT_STAT_SUSPEND in musb->port1_status, changing >> musb->xceiv->otg->state, setting musb->is_active, etc. > > Okay, but I am not sure how "even musb_hub_control ignores the error..." > is relevant in this statement. The steps are safely skipped in > musb_port_suspend() when MUSB_POWER_RESUME bit is set, no matter how > musb_hub_control() to deal with the return code from > musb_port_suspend(). Did I misunderstand anything? No, I just wanted to point out that musb_hub_control does not need to clean up after the error. >> I'll test without that part and make a v2 patch if it works. It works without changing that line. v2 follows. Best regards, Daniel
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c index 3a8451a..d05cb03 100644 --- a/drivers/usb/musb/musb_host.c +++ b/drivers/usb/musb/musb_host.c @@ -2522,8 +2522,11 @@ static int musb_bus_suspend(struct usb_hcd *hcd) { struct musb *musb = hcd_to_musb(hcd); u8 devctl; + int ret; - musb_port_suspend(musb, true); + ret = musb_port_suspend(musb, true); + if (ret) + return ret; if (!is_host_active(musb)) return 0; diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h index 72392bb..2999845 100644 --- a/drivers/usb/musb/musb_host.h +++ b/drivers/usb/musb/musb_host.h @@ -67,7 +67,7 @@ extern void musb_host_rx(struct musb *, u8); extern void musb_root_disconnect(struct musb *musb); extern void musb_host_resume_root_hub(struct musb *musb); extern void musb_host_poke_root_hub(struct musb *musb); -extern void musb_port_suspend(struct musb *musb, bool do_suspend); +extern int musb_port_suspend(struct musb *musb, bool do_suspend); extern void musb_port_reset(struct musb *musb, bool do_reset); extern void musb_host_finish_resume(struct work_struct *work); #else @@ -99,7 +99,10 @@ static inline void musb_root_disconnect(struct musb *musb) {} static inline void musb_host_resume_root_hub(struct musb *musb) {} static inline void musb_host_poll_rh_status(struct musb *musb) {} static inline void musb_host_poke_root_hub(struct musb *musb) {} -static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {} +static inline int musb_port_suspend(struct musb *musb, bool do_suspend) +{ + return 0; +} static inline void musb_port_reset(struct musb *musb, bool do_reset) {} static inline void musb_host_finish_resume(struct work_struct *work) {} #endif diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c index 5165d2b..41b44ce 100644 --- a/drivers/usb/musb/musb_virthub.c +++ b/drivers/usb/musb/musb_virthub.c @@ -48,14 +48,14 @@ void musb_host_finish_resume(struct work_struct *work) spin_unlock_irqrestore(&musb->lock, flags); } -void musb_port_suspend(struct musb *musb, bool do_suspend) +int musb_port_suspend(struct musb *musb, bool do_suspend) { struct usb_otg *otg = musb->xceiv->otg; u8 power; void __iomem *mbase = musb->mregs; if (!is_host_active(musb)) - return; + return 0; /* NOTE: this doesn't necessarily put PHY into low power mode, * turning off its clock; that's a function of PHY integration and @@ -66,16 +66,20 @@ void musb_port_suspend(struct musb *musb, bool do_suspend) if (do_suspend) { int retries = 10000; - power &= ~MUSB_POWER_RESUME; - power |= MUSB_POWER_SUSPENDM; - musb_writeb(mbase, MUSB_POWER, power); + if (power & MUSB_POWER_RESUME) + return -EBUSY; - /* Needed for OPT A tests */ - power = musb_readb(mbase, MUSB_POWER); - while (power & MUSB_POWER_SUSPENDM) { + if (!(power & MUSB_POWER_SUSPENDM)) { + power |= MUSB_POWER_SUSPENDM; + musb_writeb(mbase, MUSB_POWER, power); + + /* Needed for OPT A tests */ power = musb_readb(mbase, MUSB_POWER); - if (retries-- < 1) - break; + while (power & MUSB_POWER_SUSPENDM) { + power = musb_readb(mbase, MUSB_POWER); + if (retries-- < 1) + break; + } } musb_dbg(musb, "Root port suspended, power %02x", power); @@ -100,7 +104,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend) musb_dbg(musb, "bogus rh suspend? %s", usb_otg_state_string(musb->xceiv->otg->state)); } - } else if (power & MUSB_POWER_SUSPENDM) { + } else if (power & (MUSB_POWER_SUSPENDM | MUSB_POWER_RESUME)) { power &= ~MUSB_POWER_SUSPENDM; power |= MUSB_POWER_RESUME; musb_writeb(mbase, MUSB_POWER, power); @@ -111,6 +115,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend) schedule_delayed_work(&musb->finish_resume_work, msecs_to_jiffies(USB_RESUME_TIMEOUT)); } + return 0; } void musb_port_reset(struct musb *musb, bool do_reset)
It has been observed that writing 0xF2 to the power register while it reads as 0xF4 results in the register having the value 0xF0, i.e. clearing RESUME and setting SUSPENDM in one go does not work. It might also violate the USB spec to transition directly from resume to suspend, especially when not taking T_DRSMDN into account. But this is what happens when a remote wakeup occurs between SetPortFeature USB_PORT_FEAT_SUSPEND on the root hub and musb_bus_suspend being called. This commit returns -EBUSY when musb_bus_suspend is called while remote wakeup is signalled and thus avoids to reset the RESUME bit. Remember that resume can be signalled only when the bus was already suspended, so it is safe to skip the following steps even when musb_hub_control ignores the error returned by musb_port_suspend. The resume part of musb_port_suspend is modified to also accept a pending remote wakeup, to bring it to the end after T_DRSMDN has passed. Signed-off-by: Daniel Glöckner <dg@emlix.com> --- drivers/usb/musb/musb_host.c | 5 ++++- drivers/usb/musb/musb_host.h | 7 +++++-- drivers/usb/musb/musb_virthub.c | 27 ++++++++++++++++----------- 3 files changed, 25 insertions(+), 14 deletions(-)