Re: [PATCH 2/2] usb: dwc3: core: improve reset sequence

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

On Thu, Jan 15, 2026 at 16:05, Chris Morgan <macromorgan@hotmail.com> wrote:

> On Wed, Jan 14, 2026 at 12:15:56PM +0100, Mattijs Korpershoek wrote:
>> Hi Chris,
>> 
>> Thank you for the patch and sorry for the review delay.
>> 
>> On Tue, Dec 23, 2025 at 17:13, Chris Morgan <macroalpha82@gmail.com> wrote:
>> 
>> > From: Chris Morgan <macromorgan@hotmail.com>
>> >
>> > According to Synopsys Databook, we shouldn't be
>> > relying on GCTL.CORESOFTRESET bit as that's only for
>> > debugging purposes. Instead, let's use DCTL.CSFTRST
>> > if we're OTG or PERIPHERAL mode.
>> >
>> > Host side block will be reset by XHCI driver if
>> > necessary. Note that this reduces amount of time
>> > spent on dwc3_probe() by a long margin.
>> >
>> > We're still gonna wait for reset to finish for a
>> > long time (default to 1ms max), but tests show that
>> > the reset polling loop executed at most 19 times
>> > (modprobe dwc3 && modprobe -r dwc3 executed 1000
>> > times in a row).
>> >
>> > Note that this patch was submitted to Linux in 2016 [1], however I can
>> > confirm it is needed to support gadget mode in U-Boot on my device.
>> >
>> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/patch/drivers/usb/dwc3?id=f59dcab176293b646e1358144c93c58c3cda2813
>> >
>> > Suggested-by: Mian Yousaf Kaukab <yousaf.kaukab@intel.com>
>> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>> > ---
>> >  drivers/usb/dwc3/core.c | 31 +++++++++++++++++++++----------
>> >  1 file changed, 21 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> > index 847fa1f82c3..b4de2e95a0d 100644
>> > --- a/drivers/usb/dwc3/core.c
>> > +++ b/drivers/usb/dwc3/core.c
>> > @@ -59,11 +59,20 @@ static void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>> >  static int dwc3_core_soft_reset(struct dwc3 *dwc)
>> >  {
>> >  	u32		reg;
>> > +	int		retries = 1000;
>> >  
>> > -	/* Before Resetting PHY, put Core in Reset */
>> > -	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> > -	reg |= DWC3_GCTL_CORESOFTRESET;
>> > -	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> > +	/*
>> > +	 * We're resetting only the device side because, if we're in host mode,
>> > +	 * XHCI driver will reset the host block. If dwc3 was configured for
>> > +	 * host-only mode, then we can return early.
>> > +	 */
>> > +	if (dwc->dr_mode == USB_DR_MODE_HOST)
>> > +		return 0;
>> > +
>> > +	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> > +	reg |= DWC3_DCTL_CSFTRST;
>> > +	reg &= ~DWC3_DCTL_RUN_STOP;
>> > +	dwc3_gadget_dctl_write_safe(dwc, reg);
>> >  
>> >  	/* Assert USB3 PHY reset */
>> >  	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>> 
>> Looking more closely at [1], I can see that /* Assert USB3 PHY reset */
>> and  /* Assert USB2 PHY reset */  have also been removed in that patch
>> (in linux)
>> 
>> Can you please explain why it has not been removed in the U-Boot
>> version?
>> Please add details of that in the commit message.
>
> I don't remember honestly, I think I kept that there because I wasn't
> entirely sure if it was still necessary by other boards or not. For v2
> what I'm going to do instead is basically port the
> dwc3_core_soft_reset() function as it exists today in the mainline
> v6.19-rc5 branch and implement that here instead. So at least this
> function will be identical to Linux. I can confirm doing so does work
> on my board and continues to resolve the problems I had previously.

Thanks, looking forward to v2!

>
>> 
>> > @@ -87,12 +96,14 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>> >  	reg &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
>> >  	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> >  
>> > -	mdelay(100);
>> > +	do {
>> > +		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> > +		if (!(reg & DWC3_DCTL_CSFTRST))
>> > +			return 0;
>> > +		udelay(1);
>> > +	} while (--retries);
>> >  
>> > -	/* After PHYs are stable we can take Core out of reset state */
>> > -	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> > -	reg &= ~DWC3_GCTL_CORESOFTRESET;
>> > -	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> > +	return -ETIMEDOUT;
>> >  
>> >  	return 0;
>> >  }
>> > @@ -137,7 +148,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>> >  
>> >  	if (dwc->ref_clk) {
>> >  		rate = clk_get_rate(dwc->ref_clk);
>> > -		if (!rate)
>> > +		if (!rate || (long)rate < 0)
>> 
>> Is this a spurious change? it does not appear in the linux version of
>> the patch and also seems unrelated to soft reset.
>> 
>
> Spurious change, the device works fine without it as far as I can tell.
>
>> >  			return;
>> >  		period = NSEC_PER_SEC / rate;
>> >  	} else {
>> > -- 
>> > 2.43.0
>
> Thank you,
> Chris