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 <[email protected]> wrote:
>
> > From: Chris Morgan <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Felipe Balbi <[email protected]>
> > Signed-off-by: Chris Morgan <[email protected]>
> > ---
> > 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.
>
> > @@ -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
