[PATCH 0/6] usb: dwc3: gadget: avoid EP command timeout

[PATCH 0/6] usb: dwc3: gadget: avoid EP command timeout

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

While there are happy users who successfully have been using DFU on TI
AM62x [1][2], there are also others who were not so lucky up to now [3].

I felt into latter category and was wondering why I observe this:

--
U-Boot SPL 2024.04-61876f393762 (Apr 11 2024 - 09:27:15 +0000)
...
Trying to boot from DFU
dwc3-generic-peripheral usb@31000000: found 16 IN and 16 OUT endpoints
dwc3-generic-peripheral usb@31000000: Event buf 81e0a200 dma 00000000x length -2115984896
...
dwc3-generic-peripheral usb@31000000: initializing ep0out
...
dwc3-generic-peripheral usb@31000000: initializing ep15in
registering UDC driver []
dwc3-generic-peripheral usb@31000000: gadget no-function data soft-connect
dwc3-generic-peripheral usb@31000000: Enabling ep0out
dwc3-generic-peripheral usb@31000000: Command Complete --> 0
dwc3-generic-peripheral usb@31000000: failed to enable ep0out
failed to start : -110
g_dnl_register: failed!, error: -110
g_dnl_register failedSPL: failed to boot from all boot devices
### ERROR ### Please RESET the board ###
--

It turned out the timeout happened in dwc3_send_gadget_ep_cmd() on
DWC3_DEPCMD_SETEPCONFIG command and while the "timeout" has been increased
in Linux from 500 to 1000 and later 5000, in my case it took more than
77000 1us cycles to successfully complete.

It turns out that Linux DWC3 gadget code has been improved in the meanwhile
to follow DWC3 programming guide more precisely. Porting the following
Linux patches solved the timeout issue in U-Boot SPL on TI AM6232.

Strictly speaking
"usb: dwc3: gadget: Disable GUSB2PHYCFG.SUSPHY for End Transfer" and
"usb: dwc3: gadget: properly check ep cmd" are not required to tackle
the observed timeout, but it keeps the whole code block in sync with
Linux v6.8.

Felipe Balbi (4):
  usb: dwc3: gadget: combine return points into a single one
  usb: dwc3: gadget: clear SUSPHY bit before ep cmds
  usb: dwc3: gadget: only resume USB2 PHY in <=HIGHSPEED
  usb: dwc3: gadget: properly check ep cmd

Thinh Nguyen (2):
  usb: dwc3: gadget: Check ENBLSLPM before sending ep command
  usb: dwc3: gadget: Disable GUSB2PHYCFG.SUSPHY for End Transfer

 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/gadget.c | 47 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

1: https://git.ti.com/cgit/ti-u-boot/ti-u-boot/tree/configs/am62x_evm_r5_usbdfu_defconfig?h=09.02.00.009
2: https://lore.kernel.org/all/20240112085317.1866449-1-sjoerd@collabora.com/T/
3: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1162644/sk-am62-am62-android-fastboot-0-issue?tisearch=e2e-sitesearch&keymatch=SK-AM62

-- 
2.44.0

[PATCH 1/6] usb: dwc3: gadget: combine return points into a single one

[A. Sverdlin]

From: Felipe Balbi <felipe.balbi@linux.intel.com>

Upstream Linux commit c0ca324d09a0.

dwc3_send_gadget_ep_cmd() had three return
points. That becomes a pain to track when we need to
debug something or if we need to add more code
before returning.

Let's combine all three return points into a single
one just by introducing a local 'ret' variable.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/usb/dwc3/gadget.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 69d9fe40e2f87..17c19285f1c24 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -302,6 +302,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 {
 	u32			timeout = 500;
 	u32			reg;
+	int			ret = -EINVAL;
 
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR1(ep), params->param1);
@@ -313,7 +314,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 		if (!(reg & DWC3_DEPCMD_CMDACT)) {
 			dev_vdbg(dwc->dev, "Command Complete --> %d\n",
 					DWC3_DEPCMD_STATUS(reg));
-			return 0;
+			ret = 0;
+			break;
 		}
 
 		/*
@@ -321,11 +323,15 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 		 * interrupt context.
 		 */
 		timeout--;
-		if (!timeout)
-			return -ETIMEDOUT;
+		if (!timeout) {
+			ret = -ETIMEDOUT;
+			break;
+		}
 
 		udelay(1);
 	} while (1);
+
+	return ret;
 }
 
 static dma_addr_t dwc3_trb_dma_offset(struct dwc3_ep *dep,
-- 
2.44.0

Re: [PATCH 1/6] usb: dwc3: gadget: combine return points into a single one

[Mattijs Korpershoek]

Hi Alexander,

Thank you for the patch.

On ven., avril 12, 2024 at 22:26, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> Upstream Linux commit c0ca324d09a0.
>
> dwc3_send_gadget_ep_cmd() had three return
> points. That becomes a pain to track when we need to
> debug something or if we need to add more code
> before returning.
>
> Let's combine all three return points into a single
> one just by introducing a local 'ret' variable.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/dwc3/gadget.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 69d9fe40e2f87..17c19285f1c24 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -302,6 +302,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  {
>  	u32			timeout = 500;
>  	u32			reg;
> +	int			ret = -EINVAL;
>  
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR1(ep), params->param1);
> @@ -313,7 +314,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  		if (!(reg & DWC3_DEPCMD_CMDACT)) {
>  			dev_vdbg(dwc->dev, "Command Complete --> %d\n",
>  					DWC3_DEPCMD_STATUS(reg));
> -			return 0;
> +			ret = 0;
> +			break;
>  		}
>  
>  		/*
> @@ -321,11 +323,15 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  		 * interrupt context.
>  		 */
>  		timeout--;
> -		if (!timeout)
> -			return -ETIMEDOUT;
> +		if (!timeout) {
> +			ret = -ETIMEDOUT;
> +			break;
> +		}
>  
>  		udelay(1);
>  	} while (1);
> +
> +	return ret;
>  }
>  
>  static dma_addr_t dwc3_trb_dma_offset(struct dwc3_ep *dep,
> -- 
> 2.44.0

[PATCH 2/6] usb: dwc3: gadget: clear SUSPHY bit before ep cmds

[A. Sverdlin]

From: Felipe Balbi <felipe.balbi@linux.intel.com>

Upstream Linux commit 2b0f11df84bb.

Synopsys Databook 2.60a has a note that if we're
sending an endpoint command we _must_ make sure that
DWC3_GUSB2PHY(n).SUSPHY bit is cleared.

This patch implements that particular detail.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/usb/dwc3/gadget.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 17c19285f1c24..8f6513752f085 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -302,8 +302,25 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 {
 	u32			timeout = 500;
 	u32			reg;
+
+	int			susphy = false;
 	int			ret = -EINVAL;
 
+	/*
+	 * Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
+	 * we're issuing an endpoint command, we must check if
+	 * GUSB2PHYCFG.SUSPHY bit is set. If it is, then we need to clear it.
+	 *
+	 * We will also set SUSPHY bit to what it was before returning as stated
+	 * by the same section on Synopsys databook.
+	 */
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+	if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
+		susphy = true;
+		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+	}
+
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR1(ep), params->param1);
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR2(ep), params->param2);
@@ -331,6 +348,12 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 		udelay(1);
 	} while (1);
 
+	if (unlikely(susphy)) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+	}
+
 	return ret;
 }
 
-- 
2.44.0

Re: [PATCH 2/6] usb: dwc3: gadget: clear SUSPHY bit before ep cmds

[Mattijs Korpershoek]

Hi Alexander,

Thank you for the patch.

On ven., avril 12, 2024 at 22:26, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> Upstream Linux commit 2b0f11df84bb.
>
> Synopsys Databook 2.60a has a note that if we're
> sending an endpoint command we _must_ make sure that
> DWC3_GUSB2PHY(n).SUSPHY bit is cleared.
>
> This patch implements that particular detail.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/dwc3/gadget.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 17c19285f1c24..8f6513752f085 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -302,8 +302,25 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  {
>  	u32			timeout = 500;
>  	u32			reg;
> +
> +	int			susphy = false;
>  	int			ret = -EINVAL;
>  
> +	/*
> +	 * Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
> +	 * we're issuing an endpoint command, we must check if
> +	 * GUSB2PHYCFG.SUSPHY bit is set. If it is, then we need to clear it.
> +	 *
> +	 * We will also set SUSPHY bit to what it was before returning as stated
> +	 * by the same section on Synopsys databook.
> +	 */
> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +	if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
> +		susphy = true;
> +		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +	}
> +
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR1(ep), params->param1);
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR2(ep), params->param2);
> @@ -331,6 +348,12 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  		udelay(1);
>  	} while (1);
>  
> +	if (unlikely(susphy)) {
> +		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +	}
> +
>  	return ret;
>  }
>  
> -- 
> 2.44.0

[PATCH 3/6] usb: dwc3: gadget: only resume USB2 PHY in <=HIGHSPEED

[A. Sverdlin]

From: Felipe Balbi <felipe.balbi@linux.intel.com>

Upstream Linux commit ab2a92e7a608.

As a micro-power optimization, let's only resume the
USB2 PHY if we're working on <=HIGHSPEED. If we're
gonna work on SUPERSPEED or SUPERSPEED+, there's no
point in resuming the USB2 PHY.

Fixes: 2b0f11df84bb ("usb: dwc3: gadget: clear SUSPHY bit before ep cmds")
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/usb/dwc3/gadget.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8f6513752f085..00845dbadd27a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -314,11 +314,13 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 	 * We will also set SUSPHY bit to what it was before returning as stated
 	 * by the same section on Synopsys databook.
 	 */
-	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-	if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
-		susphy = true;
-		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
+			susphy = true;
+			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+		}
 	}
 
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
-- 
2.44.0

Re: [PATCH 3/6] usb: dwc3: gadget: only resume USB2 PHY in <=HIGHSPEED

[Mattijs Korpershoek]

Hi Alexander,

Thank you for the patch.

On ven., avril 12, 2024 at 22:26, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> Upstream Linux commit ab2a92e7a608.
>
> As a micro-power optimization, let's only resume the
> USB2 PHY if we're working on <=HIGHSPEED. If we're
> gonna work on SUPERSPEED or SUPERSPEED+, there's no
> point in resuming the USB2 PHY.
>
> Fixes: 2b0f11df84bb ("usb: dwc3: gadget: clear SUSPHY bit before ep cmds")
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/dwc3/gadget.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8f6513752f085..00845dbadd27a 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -314,11 +314,13 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  	 * We will also set SUSPHY bit to what it was before returning as stated
>  	 * by the same section on Synopsys databook.
>  	 */
> -	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -	if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
> -		susphy = true;
> -		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
> +		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
> +			susphy = true;
> +			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +		}
>  	}
>  
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
> -- 
> 2.44.0

[PATCH 4/6] usb: dwc3: gadget: Check ENBLSLPM before sending ep command

[A. Sverdlin]

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Upstream Linux commit 87dd96111b0b.

When operating in USB 2.0 speeds (HS/FS), if GUSB2PHYCFG.ENBLSLPM or
GUSB2PHYCFG.SUSPHY is set, it must be cleared before issuing an endpoint
command.

Current implementation only save and restore GUSB2PHYCFG.SUSPHY
configuration. We must save and clear both GUSB2PHYCFG.ENBLSLPM and
GUSB2PHYCFG.SUSPHY settings. Restore them after the command is
completed.

DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 00845dbadd27a..c14d7870b9461 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -301,26 +301,35 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 		unsigned cmd, struct dwc3_gadget_ep_cmd_params *params)
 {
 	u32			timeout = 500;
+	u32			saved_config = 0;
 	u32			reg;
 
-	int			susphy = false;
 	int			ret = -EINVAL;
 
 	/*
-	 * Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
-	 * we're issuing an endpoint command, we must check if
-	 * GUSB2PHYCFG.SUSPHY bit is set. If it is, then we need to clear it.
+	 * When operating in USB 2.0 speeds (HS/FS), if GUSB2PHYCFG.ENBLSLPM or
+	 * GUSB2PHYCFG.SUSPHY is set, it must be cleared before issuing an
+	 * endpoint command.
 	 *
-	 * We will also set SUSPHY bit to what it was before returning as stated
-	 * by the same section on Synopsys databook.
+	 * Save and clear both GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY
+	 * settings. Restore them after the command is completed.
+	 *
+	 * DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2
 	 */
 	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
-			susphy = true;
+			saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
 			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 		}
+
+		if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) {
+			saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM;
+			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
+		}
+
+		if (saved_config)
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 	}
 
 	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
@@ -350,9 +359,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 		udelay(1);
 	} while (1);
 
-	if (unlikely(susphy)) {
+	if (saved_config) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+		reg |= saved_config;
 		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 	}
 
-- 
2.44.0

Re: [PATCH 4/6] usb: dwc3: gadget: Check ENBLSLPM before sending ep command

[Mattijs Korpershoek]

Hi Alexander,

Thank you for the patch.

On ven., avril 12, 2024 at 22:26, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>
> Upstream Linux commit 87dd96111b0b.
>
> When operating in USB 2.0 speeds (HS/FS), if GUSB2PHYCFG.ENBLSLPM or
> GUSB2PHYCFG.SUSPHY is set, it must be cleared before issuing an endpoint
> command.
>
> Current implementation only save and restore GUSB2PHYCFG.SUSPHY
> configuration. We must save and clear both GUSB2PHYCFG.ENBLSLPM and
> GUSB2PHYCFG.SUSPHY settings. Restore them after the command is
> completed.
>
> DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 00845dbadd27a..c14d7870b9461 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -301,26 +301,35 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  		unsigned cmd, struct dwc3_gadget_ep_cmd_params *params)
>  {
>  	u32			timeout = 500;
> +	u32			saved_config = 0;
>  	u32			reg;
>  
> -	int			susphy = false;
>  	int			ret = -EINVAL;
>  
>  	/*
> -	 * Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
> -	 * we're issuing an endpoint command, we must check if
> -	 * GUSB2PHYCFG.SUSPHY bit is set. If it is, then we need to clear it.
> +	 * When operating in USB 2.0 speeds (HS/FS), if GUSB2PHYCFG.ENBLSLPM or
> +	 * GUSB2PHYCFG.SUSPHY is set, it must be cleared before issuing an
> +	 * endpoint command.
>  	 *
> -	 * We will also set SUSPHY bit to what it was before returning as stated
> -	 * by the same section on Synopsys databook.
> +	 * Save and clear both GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY
> +	 * settings. Restore them after the command is completed.
> +	 *
> +	 * DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2
>  	 */
>  	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
>  		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>  		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
> -			susphy = true;
> +			saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
>  			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>  		}
> +
> +		if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) {
> +			saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM;
> +			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
> +		}
> +
> +		if (saved_config)
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>  	}
>  
>  	dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
> @@ -350,9 +359,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  		udelay(1);
>  	} while (1);
>  
> -	if (unlikely(susphy)) {
> +	if (saved_config) {
>  		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +		reg |= saved_config;
>  		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>  	}
>  
> -- 
> 2.44.0

[PATCH 5/6] usb: dwc3: gadget: properly check ep cmd

[A. Sverdlin]

From: Felipe Balbi <felipe.balbi@linux.intel.com>

Upstream Linux commit 5999914f227b.

The cmd argument we pass to
dwc3_send_gadget_ep_cmd() could contain extra
arguments embedded. When checking for StartTransfer
command, we need to make sure to match only lower 4
bits which contain the actual command and ignore the
rest.

Reported-by: Janusz Dziedzic <januszx.dziedzic@intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
[A. Sverdlin: cherry-picked only DWC3_DEPCMD_CMD() define]
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/usb/dwc3/core.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1e7eda89a34c9..7709ab793f36d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -405,6 +405,8 @@
 #define DWC3_DEPCMD_SETTRANSFRESOURCE	(0x02 << 0)
 #define DWC3_DEPCMD_SETEPCONFIG		(0x01 << 0)
 
+#define DWC3_DEPCMD_CMD(x)		((x) & 0xf)
+
 /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */
 #define DWC3_DALEPENA_EP(n)		(1 << n)
 
-- 
2.44.0

Re: [PATCH 5/6] usb: dwc3: gadget: properly check ep cmd

[Mattijs Korpershoek]

Hi Alexander,

Thank you for the patch.

On ven., avril 12, 2024 at 22:26, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> Upstream Linux commit 5999914f227b.
>
> The cmd argument we pass to
> dwc3_send_gadget_ep_cmd() could contain extra
> arguments embedded. When checking for StartTransfer
> command, we need to make sure to match only lower 4
> bits which contain the actual command and ignore the
> rest.
>
> Reported-by: Janusz Dziedzic <januszx.dziedzic@intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> [A. Sverdlin: cherry-picked only DWC3_DEPCMD_CMD() define]
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/dwc3/core.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 1e7eda89a34c9..7709ab793f36d 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -405,6 +405,8 @@
>  #define DWC3_DEPCMD_SETTRANSFRESOURCE	(0x02 << 0)
>  #define DWC3_DEPCMD_SETEPCONFIG		(0x01 << 0)
>  
> +#define DWC3_DEPCMD_CMD(x)		((x) & 0xf)
> +
>  /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */
>  #define DWC3_DALEPENA_EP(n)		(1 << n)
>  
> -- 
> 2.44.0

[PATCH 6/6] usb: dwc3: gadget: Disable GUSB2PHYCFG.SUSPHY for End Transfer

[A. Sverdlin]

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Upstream Linux commit 3aa07f72894d.

If there's a disconnection while operating in eSS, there may be a delay
in VBUS drop response from the connector. In that case, the internal
link state may drop to operate in usb2 speed while the controller thinks
the VBUS is still high. The driver must make sure to disable
GUSB2PHYCFG.SUSPHY when sending endpoint command while in usb2 speed.
The End Transfer command may be called, and only that command needs to
go through at this point. Let's keep it simple and unconditionally
disable GUSB2PHYCFG.SUSPHY whenever we issue the command.

This scenario is not seen in real hardware. In a rare case, our
prototype type-c controller/interface may have a slow response
triggerring this issue.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Link: https://lore.kernel.org/r/5651117207803c26e2f22ddf4e5ce9e865dcf7c7.1668045468.git.Thinh.Nguyen@synopsys.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/usb/dwc3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c14d7870b9461..debfd4d6781db 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -316,7 +316,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
 	 *
 	 * DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2
 	 */
-	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
+	if (dwc->gadget.speed <= USB_SPEED_HIGH ||
+	    DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
 			saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
-- 
2.44.0

Re: [PATCH 6/6] usb: dwc3: gadget: Disable GUSB2PHYCFG.SUSPHY for End Transfer

[Greg Kroah-Hartman]

On Fri, Apr 12, 2024 at 10:26:06PM +0200, A. Sverdlin wrote:
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> Upstream Linux commit 3aa07f72894d.
> 
> If there's a disconnection while operating in eSS, there may be a delay
> in VBUS drop response from the connector. In that case, the internal
> link state may drop to operate in usb2 speed while the controller thinks
> the VBUS is still high. The driver must make sure to disable
> GUSB2PHYCFG.SUSPHY when sending endpoint command while in usb2 speed.
> The End Transfer command may be called, and only that command needs to
> go through at this point. Let's keep it simple and unconditionally
> disable GUSB2PHYCFG.SUSPHY whenever we issue the command.
> 
> This scenario is not seen in real hardware. In a rare case, our
> prototype type-c controller/interface may have a slow response
> triggerring this issue.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Link: https://lore.kernel.org/r/5651117207803c26e2f22ddf4e5ce9e865dcf7c7.1668045468.git.Thinh.Nguyen@synopsys.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Confused, why did you resend this to us?

Re: [PATCH 6/6] usb: dwc3: gadget: Disable GUSB2PHYCFG.SUSPHY for End Transfer

[Mattijs Korpershoek]

Hi Alexander,

Thank you for the patch.

On ven., avril 12, 2024 at 22:26, "A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>
> Upstream Linux commit 3aa07f72894d.
>
> If there's a disconnection while operating in eSS, there may be a delay
> in VBUS drop response from the connector. In that case, the internal
> link state may drop to operate in usb2 speed while the controller thinks
> the VBUS is still high. The driver must make sure to disable
> GUSB2PHYCFG.SUSPHY when sending endpoint command while in usb2 speed.
> The End Transfer command may be called, and only that command needs to
> go through at this point. Let's keep it simple and unconditionally
> disable GUSB2PHYCFG.SUSPHY whenever we issue the command.
>
> This scenario is not seen in real hardware. In a rare case, our
> prototype type-c controller/interface may have a slow response
> triggerring this issue.
>
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Link: https://lore.kernel.org/r/5651117207803c26e2f22ddf4e5ce9e865dcf7c7.1668045468.git.Thinh.Nguyen@synopsys.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

I've dropped Greg from the cc list as I understand by [1] that he
prefers to not receives responses on this.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

[1] https://lore.kernel.org/r/all/2024041354-exciting-suggest-b896@gregkh/

> ---
>  drivers/usb/dwc3/gadget.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index c14d7870b9461..debfd4d6781db 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -316,7 +316,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>  	 *
>  	 * DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2
>  	 */
> -	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
> +	if (dwc->gadget.speed <= USB_SPEED_HIGH ||
> +	    DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER) {
>  		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>  		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
>  			saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
> -- 
> 2.44.0

Re: [PATCH 0/6] usb: dwc3: gadget: avoid EP command timeout

[Mattijs Korpershoek]

Hi,

On Fri, 12 Apr 2024 22:26:00 +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> While there are happy users who successfully have been using DFU on TI
> AM62x [1][2], there are also others who were not so lucky up to now [3].
> 
> I felt into latter category and was wondering why I observe this:
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/6] usb: dwc3: gadget: combine return points into a single one
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/13395507ca1f48de25e70f4d27f709b1a4fa0a82
[2/6] usb: dwc3: gadget: clear SUSPHY bit before ep cmds
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/967b31c3ccdc09284a4447ebc4577bb7ef06d500
[3/6] usb: dwc3: gadget: only resume USB2 PHY in <=HIGHSPEED
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/d107a5319e20d5e5be3bd8fa298aeca9ef4990a0
[4/6] usb: dwc3: gadget: Check ENBLSLPM before sending ep command
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/db11351a887e20ff86e3a4c1f466d3d8dd42754a
[5/6] usb: dwc3: gadget: properly check ep cmd
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/95b4d655a44626f888bf823a0561a175d456d33a
[6/6] usb: dwc3: gadget: Disable GUSB2PHYCFG.SUSPHY for End Transfer
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/30f39de786ff3a87006461903e41a48c811ee764

--
Mattijs