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

[Marek Vasut]

On 6/20/23 15:36, Michal Simek wrote:
> 
> 
> On 6/20/23 11:54, Marek Vasut wrote:
>> 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 that's exactly what these 3 patches are doing. All of them are 
> backports from Linux kernel. They are steps in that direction you wanted 
> to go.

No, they just pick random subset of fixes from the kernel and leave the 
rest of the issues still open. They only make subsequent proper 
backporting more difficult by leaving out patches between these three 
patches. This only makes the history a mess.

> Venkatesh will never get a time to port all of them and send tens of 
> patches to be fully in sync with the Linux kernel. Also it will take a 
> lot of time to get all of them tested on all SOCs.

This is AMD/Xilinx decision to be unhelpful, too bad.

> Last but not least if there is a problem as Eugen reported then let's 
> fix it.

The problem Eugen reported is triggered by these patches here as far as 
understand the discussion, all the more reason for NAK.