[UBOOT PATCH 1/3] usb: dwc3: core: improve reset sequence
Marek Vasut
marex at denx.de
Thu Jun 22 17:00:21 CEST 2023
On 6/22/23 15:33, Michal Simek wrote:
>
>
> On 6/20/23 22:41, Tom Rini wrote:
>> On Tue, Jun 20, 2023 at 08:36:11AM +0200, 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.
>>
>> Yes, various parts of the stack could use some re-syncing with upstream.
>> And I forget if xhci is one of the parts that's done in such a way that
>> it should be straight-forward to do so. But dwc3 is. And while I can
>> see that there would be concerns about getting changes tested
>> sufficiently for something broad like that, that's what the merge window
>> is for. So why can't AMD re-sync things, test on your platforms and
>> have the community test on the rest? It should also come out fairly
>> quickly if there's more broad changes needed, and that in turn would be
>> good to know so we know what's next here.
>
> Marek is saying that it is going to be easy task. You are saying fairly
> quickly.
>
> The commit 85d5e7075f33e97079886027104591ff53d6363b is saying that patch
> taken from 3.19-rc1 (97bf6af1f9)
>
> Just simple git log shows that from that time 1160 patches has been
> applied there.
> $ git log --oneline --no-merges 97bf6af1f9..linux-next/master
> drivers/usb/dwc3/ | wc -l
> 1160
>
> U-Boot logs from the initial commit on this folder is showing addition
> 159 patches
> $ git log --oneline --no-merges drivers/usb/dwc3/ | wc -l
> 159
>
> It means diff is 1000 patches. I expect a lot of patches can be ignored,
> some of them are backports already but I wouldn't consider it as an easy
> task for non USB expects.
So:
- Revert the 159 patches
- Cherry-pick the 1160 patches
- Run git rebase -i, drop the 159 reverts
- Let git drop the already applied patches
I feel I am repeating myself.
I also feel AMD/Xilinx is looking for reasons to not even try, and I am
disappointed and tired by that.
More information about the U-Boot
mailing list