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

[Marek Vasut]

On 6/20/23 08:36, Michal Simek wrote:
> 
> 
> On 6/19/23 19:04, Eugen Hristev wrote:
>> 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.
> 
> It is not just about dwc3. There are other parts which are ported from 
> Linux kernel and are out of sync from Linux kernel for quite a long time.
> Another example is xhci - kernel 3.4.
> I am little bit worried that the whole usb stack is out of sync.

AMD/Xilinx had the opportunity and means to fix that worry, mostly in an 
automated manner, but chose to ignore all input and be unhelpful. Too bad.