Re: [PATCH v3 6/8] usb: dwc3: Increase DWC3 controller halt timeout

[Wesley Cheng]

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@xxxxxxxxxxxx>
> > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > ---
> >   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;
> >   }