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.abbar...@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.
Thanks,
Michal
