[v2] usb: dwc3: core: fix a issue about clear connect state

According to Synopsys Programming Guide chapter 2.2 Register Resets,
it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
reset, if DWC3 controller as a slave device and stay connected with a usb
host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
slave device when the DWC3 controller did not power off. because the
connection status is incorrect, so we also need to clear DCTL.RUN_STOP
bit for disabling connect when doing core soft reset.

Fixes: f59dcab176293b6 ("usb: dwc3: core: improve reset sequence")
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v1 -> v2:
	* modify some commit messages by Sergei's suggest, Thanks
	  very much for Sergei's help!

 drivers/usb/dwc3/core.c | 1 +
 1 file changed, 1 insertion(+)

Dejin Zheng wrote:
> According to Synopsys Programming Guide chapter 2.2 Register Resets,

> it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft

> reset, if DWC3 controller as a slave device and stay connected with a usb

> host, then, while rebooting linux, it will fail to reinitialize dwc3 as a

> slave device when the DWC3 controller did not power off.


If you reboot the OS, wouldn't it go through the driver tear-down
sequence and clear the run_stop bit anyway?
However, I can see how this can be an issue.

>  because the

> connection status is incorrect, so we also need to clear DCTL.RUN_STOP

> bit for disabling connect when doing core soft reset.

>

> Fixes: f59dcab176293b6 ("usb: dwc3: core: improve reset sequence")

> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>

> ---

> v1 -> v2:

> 	* modify some commit messages by Sergei's suggest, Thanks

> 	  very much for Sergei's help!

>

>  drivers/usb/dwc3/core.c | 1 +

>  1 file changed, 1 insertion(+)

>

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

> index 2eb34c8b4065..239636c454c2 100644

> --- a/drivers/usb/dwc3/core.c

> +++ b/drivers/usb/dwc3/core.c

> @@ -256,6 +256,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)

>  

>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);

>  	reg |= DWC3_DCTL_CSFTRST;

> +	reg &= ~DWC3_DCTL_RUN_STOP;

>  	dwc3_writel(dwc->regs, DWC3_DCTL, reg);

>  

>  	/*


There will still be other stale configuration in DCTL if you do this. I
think it's better to reset the other fields of DCTL to the default
(should be all 0s) instead of doing register read-modify-write as what
we're doing here. If not, at least we should use
dwc3_gadget_dctl_write_safe().

Thanks,
Thinh

On Mon, Oct 19, 2020 at 10:04:41PM +0000, Thinh Nguyen wrote:
> Dejin Zheng wrote:

Hi Thinh:
> > According to Synopsys Programming Guide chapter 2.2 Register Resets,
> > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
> > reset, if DWC3 controller as a slave device and stay connected with a usb
> > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
> > slave device when the DWC3 controller did not power off.
> 
> If you reboot the OS, wouldn't it go through the driver tear-down
> sequence and clear the run_stop bit anyway?
Yes, you are right, this is a point worth checking. and I think it might
still be necessary to reset it.

> However, I can see how this can be an issue.
>
> >  because the
> > connection status is incorrect, so we also need to clear DCTL.RUN_STOP
> > bit for disabling connect when doing core soft reset.
> >
> > Fixes: f59dcab176293b6 ("usb: dwc3: core: improve reset sequence")
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> > v1 -> v2:
> > 	* modify some commit messages by Sergei's suggest, Thanks
> > 	  very much for Sergei's help!
> >
> >  drivers/usb/dwc3/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 2eb34c8b4065..239636c454c2 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -256,6 +256,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
> >  
> >  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> >  	reg |= DWC3_DCTL_CSFTRST;
> > +	reg &= ~DWC3_DCTL_RUN_STOP;
> >  	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> >  
> >  	/*
> 
> There will still be other stale configuration in DCTL if you do this. I
> think it's better to reset the other fields of DCTL to the default
> (should be all 0s) instead of doing register read-modify-write as what
> we're doing here. If not, at least we should use
> dwc3_gadget_dctl_write_safe().
>
Thinh, thanks very much for your suggestion, I think it might be better
to reset all areas of DCTL register. I tested it on my SOC platform and
it worked.

BR,
Dejin

> Thanks,
> Thinh
>