| Message ID | 20220815213134.23783-7-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix controller halt and endxfer timeout issues | expand |
On 8/15/2022, Wesley Cheng wrote: > Since EP0 transactions need to be completed before the controller halt > sequence is finished, this may take some time depending on the host and the > enabled functions. Increase the controller halt timeout, so that we give > the controller sufficient time to handle EP0 transfers. Remove the need > for making dwc3_gadget_suspend() and dwc3_gadget_resume() to be called in > a spinlock. Sounds like the removal of the spin_lock and the increase of halt timeout are 2 separate change. It would be nice to separate this patch since the $subject only indicates 1 of them. > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > drivers/usb/dwc3/core.c | 4 ---- > drivers/usb/dwc3/gadget.c | 8 +++++++- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index c5c238ab3083..23e123a1ab5f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1976,9 +1976,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > case DWC3_GCTL_PRTCAP_DEVICE: > if (pm_runtime_suspended(dwc->dev)) > break; > - spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_suspend(dwc); > - spin_unlock_irqrestore(&dwc->lock, flags); > synchronize_irq(dwc->irq_gadget); > dwc3_core_exit(dwc); > break; > @@ -2039,9 +2037,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > return ret; > > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > - spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_resume(dwc); > - spin_unlock_irqrestore(&dwc->lock, flags); > break; > case DWC3_GCTL_PRTCAP_HOST: > if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) { > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 7b66a54250a0..b2668a83cc29 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2444,7 +2444,7 @@ static void __dwc3_gadget_set_speed(struct dwc3 *dwc) > static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) > { > u32 reg; > - u32 timeout = 500; > + u32 timeout = 100; > > if (pm_runtime_suspended(dwc->dev)) > return 0; > @@ -2477,6 +2477,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) > dwc3_gadget_dctl_write_safe(dwc, reg); > > do { > + msleep(20); Polling interval of 20ms seems a bit much. Can we use usleep_range() between 1-2ms? Thanks, Thinh > reg = dwc3_readl(dwc->regs, DWC3_DSTS); > reg &= DWC3_DSTS_DEVCTRLHLT; > } while (--timeout && !(!is_on ^ !reg)); > @@ -4520,12 +4521,17 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > > int dwc3_gadget_suspend(struct dwc3 *dwc) > { > + unsigned long flags; > + > if (!dwc->gadget_driver) > return 0; > > dwc3_gadget_run_stop(dwc, false, false); > + > + spin_lock_irqsave(&dwc->lock, flags); > dwc3_disconnect_gadget(dwc); > __dwc3_gadget_stop(dwc); > + spin_unlock_irqrestore(&dwc->lock, flags); > > return 0; > }
Hi Thinh, On 8/16/2022 5:34 PM, Thinh Nguyen wrote: > On 8/15/2022, Wesley Cheng wrote: >> Since EP0 transactions need to be completed before the controller halt >> sequence is finished, this may take some time depending on the host and the >> enabled functions. Increase the controller halt timeout, so that we give >> the controller sufficient time to handle EP0 transfers. Remove the need >> for making dwc3_gadget_suspend() and dwc3_gadget_resume() to be called in >> a spinlock. > > Sounds like the removal of the spin_lock and the increase of halt > timeout are 2 separate change. It would be nice to separate this patch > since the $subject only indicates 1 of them. > Sure, I'll split these up. >> >> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 4 ---- >> drivers/usb/dwc3/gadget.c | 8 +++++++- >> 2 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index c5c238ab3083..23e123a1ab5f 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1976,9 +1976,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> case DWC3_GCTL_PRTCAP_DEVICE: >> if (pm_runtime_suspended(dwc->dev)) >> break; >> - spin_lock_irqsave(&dwc->lock, flags); >> dwc3_gadget_suspend(dwc); >> - spin_unlock_irqrestore(&dwc->lock, flags); >> synchronize_irq(dwc->irq_gadget); >> dwc3_core_exit(dwc); >> break; >> @@ -2039,9 +2037,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> return ret; >> >> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >> - spin_lock_irqsave(&dwc->lock, flags); >> dwc3_gadget_resume(dwc); >> - spin_unlock_irqrestore(&dwc->lock, flags); >> break; >> case DWC3_GCTL_PRTCAP_HOST: >> if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) { >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 7b66a54250a0..b2668a83cc29 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2444,7 +2444,7 @@ static void __dwc3_gadget_set_speed(struct dwc3 *dwc) >> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) >> { >> u32 reg; >> - u32 timeout = 500; >> + u32 timeout = 100; >> >> if (pm_runtime_suspended(dwc->dev)) >> return 0; >> @@ -2477,6 +2477,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) >> dwc3_gadget_dctl_write_safe(dwc, reg); >> >> do { >> + msleep(20); > > Polling interval of 20ms seems a bit much. Can we use usleep_range() > between 1-2ms? > > Thanks, > Thinh Sounds good. Let me reduce the interval gap and resubmit. Thanks Wesley Cheng > >> reg = dwc3_readl(dwc->regs, DWC3_DSTS); >> reg &= DWC3_DSTS_DEVCTRLHLT; >> } while (--timeout && !(!is_on ^ !reg)); >> @@ -4520,12 +4521,17 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >> >> int dwc3_gadget_suspend(struct dwc3 *dwc) >> { >> + unsigned long flags; >> + >> if (!dwc->gadget_driver) >> return 0; >> >> dwc3_gadget_run_stop(dwc, false, false); >> + >> + spin_lock_irqsave(&dwc->lock, flags); >> dwc3_disconnect_gadget(dwc); >> __dwc3_gadget_stop(dwc); >> + spin_unlock_irqrestore(&dwc->lock, flags); >> >> return 0; >> } >
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index c5c238ab3083..23e123a1ab5f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1976,9 +1976,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) case DWC3_GCTL_PRTCAP_DEVICE: if (pm_runtime_suspended(dwc->dev)) break; - spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_suspend(dwc); - spin_unlock_irqrestore(&dwc->lock, flags); synchronize_irq(dwc->irq_gadget); dwc3_core_exit(dwc); break; @@ -2039,9 +2037,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) return ret; dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); - spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_resume(dwc); - spin_unlock_irqrestore(&dwc->lock, flags); break; case DWC3_GCTL_PRTCAP_HOST: if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) { diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 7b66a54250a0..b2668a83cc29 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2444,7 +2444,7 @@ static void __dwc3_gadget_set_speed(struct dwc3 *dwc) static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) { u32 reg; - u32 timeout = 500; + u32 timeout = 100; if (pm_runtime_suspended(dwc->dev)) return 0; @@ -2477,6 +2477,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) dwc3_gadget_dctl_write_safe(dwc, reg); do { + msleep(20); reg = dwc3_readl(dwc->regs, DWC3_DSTS); reg &= DWC3_DSTS_DEVCTRLHLT; } while (--timeout && !(!is_on ^ !reg)); @@ -4520,12 +4521,17 @@ void dwc3_gadget_exit(struct dwc3 *dwc) int dwc3_gadget_suspend(struct dwc3 *dwc) { + unsigned long flags; + if (!dwc->gadget_driver) return 0; dwc3_gadget_run_stop(dwc, false, false); + + spin_lock_irqsave(&dwc->lock, flags); dwc3_disconnect_gadget(dwc); __dwc3_gadget_stop(dwc); + spin_unlock_irqrestore(&dwc->lock, flags); return 0; }
Since EP0 transactions need to be completed before the controller halt sequence is finished, this may take some time depending on the host and the enabled functions. Increase the controller halt timeout, so that we give the controller sufficient time to handle EP0 transfers. Remove the need for making dwc3_gadget_suspend() and dwc3_gadget_resume() to be called in a spinlock. Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> --- drivers/usb/dwc3/core.c | 4 ---- drivers/usb/dwc3/gadget.c | 8 +++++++- 2 files changed, 7 insertions(+), 5 deletions(-)