[UBOOT PATCH 0/3] Port the usb reset patches from linux

[UBOOT PATCH 0/3] Port the usb reset patches from linux

[Venkatesh Yadav Abbarapu]

Port the usb reset patches from linux kernel.

Venkatesh Yadav Abbarapu (3):
  usb: dwc3: core: improve reset sequence
  usb: dwc3: gadget: Don't send unintended link state change
  usb: dwc3: core: Only handle soft-reset in DCTL

 drivers/usb/dwc3/core.c   | 51 +++++++++++++++------------------------
 drivers/usb/dwc3/gadget.c | 16 ++++++------
 drivers/usb/dwc3/gadget.h | 14 +++++++++++
 3 files changed, 40 insertions(+), 41 deletions(-)

-- 
2.17.1

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

[Venkatesh Yadav Abbarapu]

[ 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@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;
 }
 
 /*
-- 
2.17.1

[UBOOT PATCH 2/3] usb: dwc3: gadget: Don't send unintended link state change

[Venkatesh Yadav Abbarapu]

[ Nguyen/Felipe/Greg: Ported from Linux kernel commit
	5b738211fb59 ("usb: dwc3: gadget: Don't send
				unintended link state change") ]

DCTL.ULSTCHNGREQ is a write-only field. When doing a read-modify-write
to DCTL, the driver must make sure that there's no unintended link state
change request from whatever is read from DCTL.ULSTCHNGREQ. Set link
state change to no-action when the driver writes to DCTL.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
---
 drivers/usb/dwc3/gadget.c | 16 +++++++---------
 drivers/usb/dwc3/gadget.h | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index eb416b832a..24a2c455b0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -62,7 +62,7 @@ int dwc3_gadget_set_test_mode(struct dwc3 *dwc, int mode)
 		return -EINVAL;
 	}
 
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+	dwc3_gadget_dctl_write_safe(dwc, reg);
 
 	return 0;
 }
@@ -1382,7 +1382,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 		dwc->pullups_connected = false;
 	}
 
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+	dwc3_gadget_dctl_write_safe(dwc, reg);
 
 	do {
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
@@ -2047,10 +2047,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	reg &= ~DWC3_DCTL_INITU1ENA;
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
-
 	reg &= ~DWC3_DCTL_INITU2ENA;
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+	dwc3_gadget_dctl_write_safe(dwc, reg);
 
 	dwc3_disconnect_gadget(dwc);
 	dwc->start_config_issued = false;
@@ -2099,7 +2097,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	reg &= ~DWC3_DCTL_TSTCTRL_MASK;
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+	dwc3_gadget_dctl_write_safe(dwc, reg);
 	dwc->test_mode = false;
 
 	dwc3_stop_active_transfers(dwc);
@@ -2215,11 +2213,11 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 		if (dwc->has_lpm_erratum && dwc->revision >= DWC3_REVISION_240A)
 			reg |= DWC3_DCTL_LPM_ERRATA(dwc->lpm_nyet_threshold);
 
-		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+		dwc3_gadget_dctl_write_safe(dwc, reg);
 	} else {
 		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 		reg &= ~DWC3_DCTL_HIRD_THRES_MASK;
-		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+		dwc3_gadget_dctl_write_safe(dwc, reg);
 	}
 
 	dep = dwc->eps[0];
@@ -2327,7 +2325,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
 
 				reg &= ~u1u2;
 
-				dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+				dwc3_gadget_dctl_write_safe(dwc, reg);
 				break;
 			default:
 				/* do nothing */
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 7806ce59a2..b48ec6b237 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -104,4 +104,18 @@ static inline u32 dwc3_gadget_ep_get_transfer_index(struct dwc3 *dwc, u8 number)
 	return DWC3_DEPCMD_GET_RSC_IDX(res_id);
 }
 
+/**
+ * dwc3_gadget_dctl_write_safe - write to DCTL safe from link state change
+ * @dwc: pointer to our context structure
+ * @value: value to write to DCTL
+ *
+ * Use this function when doing read-modify-write to DCTL. It will not
+ * send link state change request.
+ */
+static inline void dwc3_gadget_dctl_write_safe(struct dwc3 *dwc, u32 value)
+{
+	value &= ~DWC3_DCTL_ULSTCHNGREQ_MASK;
+	dwc3_writel(dwc->regs, DWC3_DCTL, value);
+}
+
 #endif /* __DRIVERS_USB_DWC3_GADGET_H */
-- 
2.17.1

[UBOOT PATCH 3/3] usb: dwc3: core: Only handle soft-reset in DCTL

[Venkatesh Yadav Abbarapu]

[ Nguyen/Greg: Ported from Linux kernel commit
	f4fd84ae0765a ("usb: dwc3: core: Only handle soft-reset in DCTL") ]

Make sure not to set run_stop bit or link state change request while
initiating soft-reset. Register read-modify-write operation may
unintentionally start the controller before the initialization completes
with its previous DCTL value, which can cause initialization failure.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
---
 drivers/usb/dwc3/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8702a54efa..c80f2d0a53 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -72,7 +72,8 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	reg |= DWC3_DCTL_CSFTRST;
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+	reg &= ~DWC3_DCTL_RUN_STOP;
+	dwc3_gadget_dctl_write_safe(dwc, reg);
 
 	do {
 		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-- 
2.17.1

Re: [UBOOT PATCH 0/3] Port the usb reset patches from linux

[Marek Vasut]

On 5/8/23 05:00, Venkatesh Yadav Abbarapu wrote:
> Port the usb reset patches from linux kernel.

What kind of patches are these ?
What sort of problem are those patches attempting to address ?

> Venkatesh Yadav Abbarapu (3):
>    usb: dwc3: core: improve reset sequence
>    usb: dwc3: gadget: Don't send unintended link state change
>    usb: dwc3: core: Only handle soft-reset in DCTL

These seem to be randomly picked patches from Linux 4.7, 5.5 ... but 
there seem to be a huge amount of backports missing inbetween, which 
would create a tremendous maintenance burden of the DWC3 driver.

Can you please instead pick ALL the missing patches from Linux, so that 
the DWC3 driver is instead synchronized with Linux, rather than 
diverging and growing partial backports ?

It shouldn't be difficult, one approach I can think of is roughly this:
- figure out the original merge base from which the DWC3 driver was 
imported to U-Boot
- in U-Boot, revert all dwc3 patches on top of that import patch
- pick all Linux kernel dwc3 patches from that merge base and apply on 
top of this U-Boot with reverts
- Run rebase and drop the reverts, let git drop duplicate patches

Thank you

Re: [UBOOT PATCH 0/3] Port the usb reset patches from linux

[Michal Simek]

Hi,

On 5/8/23 13:56, Marek Vasut wrote:
> On 5/8/23 05:00, Venkatesh Yadav Abbarapu wrote:
>> Port the usb reset patches from linux kernel.
> 
> What kind of patches are these ?
> What sort of problem are those patches attempting to address ?
> 
>> Venkatesh Yadav Abbarapu (3):
>>    usb: dwc3: core: improve reset sequence
>>    usb: dwc3: gadget: Don't send unintended link state change
>>    usb: dwc3: core: Only handle soft-reset in DCTL
> 
> These seem to be randomly picked patches from Linux 4.7, 5.5 ... but there seem 
> to be a huge amount of backports missing inbetween, which would create a 
> tremendous maintenance burden of the DWC3 driver.
> 
> Can you please instead pick ALL the missing patches from Linux, so that the DWC3 
> driver is instead synchronized with Linux, rather than diverging and growing 
> partial backports ?
> 
> It shouldn't be difficult, one approach I can think of is roughly this:
> - figure out the original merge base from which the DWC3 driver was imported to 
> U-Boot
> - in U-Boot, revert all dwc3 patches on top of that import patch
> - pick all Linux kernel dwc3 patches from that merge base and apply on top of 
> this U-Boot with reverts
> - Run rebase and drop the reverts, let git drop duplicate patches

Based on internal discussion decision was made to keep these patches only in soc 
vendor tree. We are not happy with it but we are not going to invest our time 
and take responsibility for the driver synchronization work at this point.
That's why if you insist on full synchronization with Linux version please 
ignore this patchset and also serarate dwc3 clock patch.

Thanks,
Michal

Re: [UBOOT PATCH 0/3] Port the usb reset patches from linux

[Marek Vasut]

On 5/17/23 08:37, Michal Simek wrote:
> Hi,

Hi,

> On 5/8/23 13:56, Marek Vasut wrote:
>> On 5/8/23 05:00, Venkatesh Yadav Abbarapu wrote:
>>> Port the usb reset patches from linux kernel.
>>
>> What kind of patches are these ?
>> What sort of problem are those patches attempting to address ?
>>
>>> Venkatesh Yadav Abbarapu (3):
>>>    usb: dwc3: core: improve reset sequence
>>>    usb: dwc3: gadget: Don't send unintended link state change
>>>    usb: dwc3: core: Only handle soft-reset in DCTL
>>
>> These seem to be randomly picked patches from Linux 4.7, 5.5 ... but 
>> there seem to be a huge amount of backports missing inbetween, which 
>> would create a tremendous maintenance burden of the DWC3 driver.
>>
>> Can you please instead pick ALL the missing patches from Linux, so 
>> that the DWC3 driver is instead synchronized with Linux, rather than 
>> diverging and growing partial backports ?
>>
>> It shouldn't be difficult, one approach I can think of is roughly this:
>> - figure out the original merge base from which the DWC3 driver was 
>> imported to U-Boot
>> - in U-Boot, revert all dwc3 patches on top of that import patch
>> - pick all Linux kernel dwc3 patches from that merge base and apply on 
>> top of this U-Boot with reverts
>> - Run rebase and drop the reverts, let git drop duplicate patches
> 
> Based on internal discussion decision was made to keep these patches 
> only in soc vendor tree. We are not happy with it but we are not going 
> to invest our time and take responsibility for the driver 
> synchronization work at this point.

That's unfortunate, as I spent considerable amount of time explaining 
and re-explaining how to perform this synchronization in very much 
automated manner. It would be nice to know whether AMD attempted this 
approach and get a report back regarding any difficulties with that 
approach.

> That's why if you insist on full synchronization with Linux version 
> please ignore this patchset and also serarate dwc3 clock patch.

I already tried to explain this multiple times before, picking random 
patches into the DWC3 driver would make the full synchronization more 
difficult later, and it would make maintaining the driver harder too.

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

[Eugen Hristev]

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@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 ?

Thanks,
Eugen

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

[Marek Vasut]

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@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.

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

[Eugen Hristev]

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@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.

Eugen

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

[Marek Vasut]

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@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.

OK

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

[Michal Simek]

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@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.

Thanks,
Michal

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

[Marek Vasut]

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@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.

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

[Michal Simek]

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@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. 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.

Thanks,
Michal

Re: [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@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.

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

[Michal Simek]

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@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

Re: [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@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.

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

[Michal Simek]

On 6/20/23 16:13, Marek Vasut wrote:
> 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@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.

Not random. Just from beginning that's important difference.

> 
>> 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.

That's just your opinion. We shared these backports for others to use and Eugen 
confirms that "is the fact that this patch actually helps in my case" and that 
"just that it breaks something else" doesn't mean that this patch is wrong. It 
maybe points to another problem which that platform has.


>> 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.

That's fine. I can live with it.

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6631 bytes --]

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@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michal Simek]

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@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

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

[Marek Vasut]

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@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.