Re: [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup

[Rohith Kollalsi]

On 7/14/2022 6:36 AM, Thinh Nguyen wrote:
> On 7/12/2022, Rohith Kollalsi wrote:
> > According to the programming guide, it is recommended to
> > perform a GCTL_CORE_SOFTRESET only when switching the mode
> > from device to host or host to device. However, it is found
> > that during bootup when __dwc3_set_mode() is called for the
> > first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17)
> > of DWC3_GUSB3PIPECTL set. This some times leads to issues
> > like controller going into bad state and controller registers
> > reading value zero. Until GCTL_CORESOFTRESET is done and
> > run/stop bit is set core initialization is not complete.
> > Setting suspendable bit of DWC3_GUSB3PIPECTL and then
> > performing GCTL_CORESOFTRESET is therefore not recommended.
> > Avoid this by only performing the reset if current_dr_role is set,
> > that is, when doing subsequent role switching.
> >
> > Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode")
> > Signed-off-by: Rohith Kollalsi <quic_rkollals@xxxxxxxxxxx>
> > ---
> >    drivers/usb/dwc3/core.c | 10 +++++++---
> >    1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 050b2ba5986d..7ce0e033d755 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work)
> >    		break;
> >    	}
> >    
> > -	/* For DRD host or device mode only */
> > -	if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
> > -	    dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
> > +	/*
> > +	 * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET.
>
> The line above doesn't really explain why. Can you rephrase it to
> something like this or the equivalent:
> "When current_dr_role is not set, there's no role switching. Only
> perform GCTL.CoreSoftReset when there's DRD role switching."
>
> Thanks,
> Thinh

Thanks for the review Thinh, I have updated the next version of the 
patch with this suggestion.

Thanks,
Rohith

> > +	 * Perform it only for DRD host or device mode.
> > +	 */
> > +	if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
> > +			DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
> > +			dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
> >    		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> >    		reg |= DWC3_GCTL_CORESOFTRESET;
> >    		dwc3_writel(dwc->regs, DWC3_GCTL, reg);