[UBOOT PATCH 1/3] usb: dwc3: core: improve reset sequence
Michal Simek
michal.simek at amd.com
Tue Jun 20 15:36:28 CEST 2023
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.
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.
Last but not least if there is a problem as Eugen reported then let's fix it.
Thanks,
Michal
More information about the U-Boot
mailing list