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

[Marek Vasut]

On 6/20/23 11:42, Michal Simek wrote:
> 
> 
> On 6/20/23 11:23, Marek Vasut wrote:
>> 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.
> 
> It is not about ignoring input. It is about long term strategy about 
> code taken and accepted to U-Boot from Linux kernel. None sync dwc3/xhci 
> (and very likely other parts) for quite a long time and it looks like 
> there is no strategy for it that's why it is not done on regular basis.

AMD/Xilinx has the opportunity to improve that situation.

> And definitely we are not USB experts here and not going to take 
> responsibility for this task.
> But we continue to support and maintain code for AMD/Xilinx specific IPs 
> to making sure they are in a good share.

As I recall, ZynqMP does integrate DWC3 xHCI controller.