[UBOOT PATCH 1/3] usb: dwc3: core: improve reset sequence

[Eugen Hristev]

On 6/19/23 19:07, Marek Vasut wrote:
> On 6/19/23 15:26, Eugen Hristev wrote:
>> On 5/8/23 06:00, Venkatesh Yadav Abbarapu wrote:
>>> [ Felipe: Ported from Linux kernel commit
>>>       f59dcab17629 ("usb: dwc3: core: improve reset sequence") ]
>>>
>>> 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).
>>>
>>> Without proper core reset, observing random issues like when the
>>> USB(DWC3) is in device mode, the host device is not able to detect the
>>> USB device.
>>>
>>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu at amd.com>
>>> ---
>>>   drivers/usb/dwc3/core.c | 50 +++++++++++++++--------------------------
>>>   1 file changed, 18 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 49f6a1900b..8702a54efa 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -60,42 +60,28 @@ 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);
>>> -
>>> -    /* Assert USB3 PHY reset */
>>> -    reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>> -    reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
>>> -    dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>>> -
>>> -    /* Assert USB2 PHY reset */
>>> -    reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>>> -    reg |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
>>> -    dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>> -
>>> -    mdelay(100);
>>> -
>>> -    /* Clear USB3 PHY reset */
>>> -    reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>> -    reg &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
>>> -    dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>>> -
>>> -    /* Clear USB2 PHY reset */
>>> -    reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>>> -    reg &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
>>> -    dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), 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;
>>> -    mdelay(100);
>>> +    reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>> +    reg |= DWC3_DCTL_CSFTRST;
>>> +    dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>>> -    /* 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);
>>> +    do {
>>> +        reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>> +        if (!(reg & DWC3_DCTL_CSFTRST))
>>> +            return 0;
>>> +        udelay(1);
>>> +    } while (--retries);
>>> -    return 0;
>>> +    return -ETIMEDOUT;
>>>   }
>>>   /*
>>
>> Hello Venkatesh, Michal,
>>
>> I randomly found this patch while testing the Dwc3 on rockchip RK3588 
>> , I noticed that you removed the code that resets the PHYs in this 
>> patch hence DWC3 is no longer starting in my case.
>> Was this intentional with this patch ?
> 
> All of these patches are NAK until the DWC3 is synchronized with Linux. 
> Picking random DWC3 patches and ignoring other fixes will turn the whole 
> driver into an unmaintainable mess and make the sync that much harder in 
> the future. I spent enormous amount of my spare time trying to explain 
> to Xilinx how to do that mostly automatically, but that was all ignored 
> or countered with reason after reason why this cannot be done, but as 
> far as I can tell, nobody in Xilinx actually tried the proposal.
> 
> Hence, NAK
> 
> Hence, no need to worry these patches will be applied in their current 
> state.

Hi Marek,

that's fine. No argument from my side.

What I wanted to actually tell/ask , is the fact that this patch 
actually helps in my case, just that it breaks something else, and I 
wanted to get more info from the patch authors.

I am experiencing some situations where dwc3 does not correctly start 
unless I add some manual delays here and there, and I am trying to see why.

Eugen