public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/1] usb: dwc3: Support for USB3340x ULPI PHY
@ 2026-02-27 13:30 Ingo Rohloff
  2026-02-27 13:30 ` [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation Ingo Rohloff
  0 siblings, 1 reply; 5+ messages in thread
From: Ingo Rohloff @ 2026-02-27 13:30 UTC (permalink / raw)
  To: Thinh.Nguyen; +Cc: gregkh, linux-usb, Ingo Rohloff

The problem only pops up if you combine a DWC3 controller with a
Microchip USB3340 ULPI PHY.

This patch uses the USB Vendor/Product ID of the ULPI PHY
to detect the USB3340 model and then applies the necessary fix,
if this model is found.

Benefits of this approach
- Does not require any modification to existing device trees.
- Should work for all DWC3 IP / USB3340 combinations, not only
  for the Ultrascale+ ZyngMP DWC3 implementation, where this
  problem was originally found.
- Easy to extend in the future if a similar situation arises again.

Caveats:
- Replicates code from drivers/usb/common/ulpi.c

---
Changes in v3:
- Do not meantion what the DWC3_GUSB2PHYCFG_XCVRDLY does.
- Do not use device tree property to set enable_usb2_transceiver_delay.
  Instead autodetect if it's necessary to set enable_usb2_transceiver_delay.
- Link to v2: https://lore.kernel.org/linux-usb/20260225130323.24606-1-ingo.rohloff@lauterbach.com/

---
Changes in v2:
- Mention sources of information in commit message instead of code.
- Renamed property to "snps,enable-usb2-transceiver-delay".
- Renamed struct member to "enable_usb2_transceiver_delay".
- Describe dt-bindings in a second commit.
- Link to v1: https://lore.kernel.org/linux-usb/20260224141438.39524-1-ingo.rohloff@lauterbach.com/

---
Ingo Rohloff (1):
  usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.

 drivers/usb/dwc3/core.c | 16 +++++++++++++++
 drivers/usb/dwc3/core.h |  4 ++++
 drivers/usb/dwc3/ulpi.c | 43 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

--
2.52.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.
  2026-02-27 13:30 [PATCH v3 0/1] usb: dwc3: Support for USB3340x ULPI PHY Ingo Rohloff
@ 2026-02-27 13:30 ` Ingo Rohloff
  2026-02-28  0:59   ` Thinh Nguyen
  0 siblings, 1 reply; 5+ messages in thread
From: Ingo Rohloff @ 2026-02-27 13:30 UTC (permalink / raw)
  To: Thinh.Nguyen; +Cc: gregkh, linux-usb, Ingo Rohloff

The Microchip USB3340x ULPI PHY requires a delay when switching to the
high-speed transmitter. See:
    http://ww1.microchip.com/downloads/en/DeviceDoc/80000645A.pdf
    Module 2 "Device Enumeration Failure with Link IP Systems"

For details on the behavior and fix, refer to the AMD (formerly Xilinx)
forum post: "USB stuck in full speed mode with USB3340 ULPI PHY, ZynqMP."

This patch uses the USB PHY Vendor-ID and Product-ID to detect the
USB3340 PHY and then applies the necessary fix if this PHY is found.

Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.com>
---
 drivers/usb/dwc3/core.c | 16 +++++++++++++++
 drivers/usb/dwc3/core.h |  4 ++++
 drivers/usb/dwc3/ulpi.c | 43 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index cacc4ec9f7ce..cc1818635eb4 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -782,6 +782,20 @@ static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index)
 	return 0;
 }
 
+static void dwc3_hs_apply_ulpi_quirks(struct dwc3 *dwc)
+{
+	if (dwc->enable_usb2_transceiver_delay) {
+		int index;
+		u32 reg;
+
+		for (index = 0; index < dwc->num_usb2_ports; index++) {
+			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
+			reg |= DWC3_GUSB2PHYCFG_XCVRDLY;
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
+		}
+	}
+}
+
 /**
  * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -1361,6 +1375,8 @@ int dwc3_core_init(struct dwc3 *dwc)
 			return ret;
 		}
 		dwc->ulpi_ready = true;
+
+		dwc3_hs_apply_ulpi_quirks(dwc);
 	}
 
 	if (!dwc->phys_ready) {
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 67bcc8dccc89..94a2a55518c9 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -302,6 +302,7 @@
 #define DWC3_GUSB2PHYCFG_SUSPHY		BIT(6)
 #define DWC3_GUSB2PHYCFG_ULPI_UTMI	BIT(4)
 #define DWC3_GUSB2PHYCFG_ENBLSLPM	BIT(8)
+#define DWC3_GUSB2PHYCFG_XCVRDLY        BIT(9)
 #define DWC3_GUSB2PHYCFG_PHYIF(n)	(n << 3)
 #define DWC3_GUSB2PHYCFG_PHYIF_MASK	DWC3_GUSB2PHYCFG_PHYIF(1)
 #define DWC3_GUSB2PHYCFG_USBTRDTIM(n)	(n << 10)
@@ -1163,6 +1164,8 @@ struct dwc3_glue_ops {
  *	3	- Reserved
  * @dis_metastability_quirk: set to disable metastability quirk.
  * @dis_split_quirk: set to disable split boundary.
+ * @enable_usb2_transceiver_delay: Set to insert a delay before the
+ *                  assertion of the TxValid signal during a HS Chirp.
  * @sys_wakeup: set if the device may do system wakeup.
  * @wakeup_configured: set if the device is configured for remote wakeup.
  * @suspended: set to track suspend event due to U3/L2.
@@ -1406,6 +1409,7 @@ struct dwc3 {
 	unsigned		dis_metastability_quirk:1;
 
 	unsigned		dis_split_quirk:1;
+	unsigned		enable_usb2_transceiver_delay:1;
 	unsigned		async_callbacks:1;
 	unsigned		sys_wakeup:1;
 	unsigned		wakeup_configured:1;
diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
index 57daad15f502..e1a1a58f862c 100644
--- a/drivers/usb/dwc3/ulpi.c
+++ b/drivers/usb/dwc3/ulpi.c
@@ -14,6 +14,8 @@
 #include "core.h"
 #include "io.h"
 
+#define USB_VENDOR_MICROCHIP 0x0424
+
 #define DWC3_ULPI_ADDR(a) \
 		((a >= ULPI_EXT_VENDOR_SPECIFIC) ? \
 		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
@@ -83,6 +85,45 @@ static const struct ulpi_ops dwc3_ulpi_ops = {
 	.write = dwc3_ulpi_write,
 };
 
+static void dwc3_ulpi_detect_quirks(struct dwc3 *dwc)
+{
+	int ret;
+	u16 vendor_id, product_id;
+
+	/* Test the interface */
+	ret = dwc3_ulpi_write(dwc->dev, ULPI_SCRATCH, 0xaa);
+	if (ret < 0)
+		return;
+
+	ret = dwc3_ulpi_read(dwc->dev, ULPI_SCRATCH);
+	if (ret < 0)
+		return;
+
+	if (ret != 0xaa)
+		return;
+
+	vendor_id = dwc3_ulpi_read(dwc->dev, ULPI_VENDOR_ID_LOW);
+	vendor_id |= dwc3_ulpi_read(dwc->dev, ULPI_VENDOR_ID_HIGH) << 8;
+
+	product_id = dwc3_ulpi_read(dwc->dev, ULPI_PRODUCT_ID_LOW);
+	product_id |= dwc3_ulpi_read(dwc->dev, ULPI_PRODUCT_ID_HIGH) << 8;
+
+	pr_info(
+		"dwc3_ulpi: VendorID 0x%04x, ProductID 0x%04x\n",
+		vendor_id, product_id
+	);
+	switch (vendor_id) {
+	case USB_VENDOR_MICROCHIP:
+		switch (product_id) {
+		case 0x0009:
+			/* Microchip USB3340 ULPI PHY */
+			dwc->enable_usb2_transceiver_delay = true;
+			break;
+		}
+		break;
+	}
+}
+
 int dwc3_ulpi_init(struct dwc3 *dwc)
 {
 	/* Register the interface */
@@ -92,6 +133,8 @@ int dwc3_ulpi_init(struct dwc3 *dwc)
 		return PTR_ERR(dwc->ulpi);
 	}
 
+	dwc3_ulpi_detect_quirks(dwc);
+
 	return 0;
 }
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.
  2026-02-27 13:30 ` [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation Ingo Rohloff
@ 2026-02-28  0:59   ` Thinh Nguyen
  2026-03-02 11:53     ` Ingo Rohloff
  0 siblings, 1 reply; 5+ messages in thread
From: Thinh Nguyen @ 2026-02-28  0:59 UTC (permalink / raw)
  To: Ingo Rohloff
  Cc: Thinh Nguyen, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org

On Fri, Feb 27, 2026, Ingo Rohloff wrote:
> The Microchip USB3340x ULPI PHY requires a delay when switching to the
> high-speed transmitter. See:
>     https://urldefense.com/v3/__http://ww1.microchip.com/downloads/en/DeviceDoc/80000645A.pdf__;!!A4F2R9G_pg!cuAZjiUZyG51WLifZERoNCQsqZwqS5JqH_qsck0l8jAxBUuaarEH5CovWVC6WK7ltamHxavcQzHaeUxX2KS4IPnb0KcLs0U$ 
>     Module 2 "Device Enumeration Failure with Link IP Systems"
> 
> For details on the behavior and fix, refer to the AMD (formerly Xilinx)
> forum post: "USB stuck in full speed mode with USB3340 ULPI PHY, ZynqMP."
> 
> This patch uses the USB PHY Vendor-ID and Product-ID to detect the
> USB3340 PHY and then applies the necessary fix if this PHY is found.
> 
> Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.com>
> ---
>  drivers/usb/dwc3/core.c | 16 +++++++++++++++
>  drivers/usb/dwc3/core.h |  4 ++++
>  drivers/usb/dwc3/ulpi.c | 43 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index cacc4ec9f7ce..cc1818635eb4 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -782,6 +782,20 @@ static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index)
>  	return 0;
>  }
>  
> +static void dwc3_hs_apply_ulpi_quirks(struct dwc3 *dwc)
> +{
> +	if (dwc->enable_usb2_transceiver_delay) {
> +		int index;
> +		u32 reg;
> +
> +		for (index = 0; index < dwc->num_usb2_ports; index++) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
> +			reg |= DWC3_GUSB2PHYCFG_XCVRDLY;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
> +		}
> +	}

This should be placed in dwc3_ulpi_init() or dwc3_ulpi_detect_quirks().

> +}
> +
>  /**
>   * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
>   * @dwc: Pointer to our controller context structure
> @@ -1361,6 +1375,8 @@ int dwc3_core_init(struct dwc3 *dwc)
>  			return ret;
>  		}
>  		dwc->ulpi_ready = true;
> +
> +		dwc3_hs_apply_ulpi_quirks(dwc);
>  	}
>  
>  	if (!dwc->phys_ready) {
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 67bcc8dccc89..94a2a55518c9 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -302,6 +302,7 @@
>  #define DWC3_GUSB2PHYCFG_SUSPHY		BIT(6)
>  #define DWC3_GUSB2PHYCFG_ULPI_UTMI	BIT(4)
>  #define DWC3_GUSB2PHYCFG_ENBLSLPM	BIT(8)
> +#define DWC3_GUSB2PHYCFG_XCVRDLY        BIT(9)

Use tab for spacing.

>  #define DWC3_GUSB2PHYCFG_PHYIF(n)	(n << 3)
>  #define DWC3_GUSB2PHYCFG_PHYIF_MASK	DWC3_GUSB2PHYCFG_PHYIF(1)
>  #define DWC3_GUSB2PHYCFG_USBTRDTIM(n)	(n << 10)
> @@ -1163,6 +1164,8 @@ struct dwc3_glue_ops {
>   *	3	- Reserved
>   * @dis_metastability_quirk: set to disable metastability quirk.
>   * @dis_split_quirk: set to disable split boundary.
> + * @enable_usb2_transceiver_delay: Set to insert a delay before the
> + *                  assertion of the TxValid signal during a HS Chirp.
>   * @sys_wakeup: set if the device may do system wakeup.
>   * @wakeup_configured: set if the device is configured for remote wakeup.
>   * @suspended: set to track suspend event due to U3/L2.
> @@ -1406,6 +1409,7 @@ struct dwc3 {
>  	unsigned		dis_metastability_quirk:1;
>  
>  	unsigned		dis_split_quirk:1;
> +	unsigned		enable_usb2_transceiver_delay:1;
>  	unsigned		async_callbacks:1;
>  	unsigned		sys_wakeup:1;
>  	unsigned		wakeup_configured:1;
> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> index 57daad15f502..e1a1a58f862c 100644
> --- a/drivers/usb/dwc3/ulpi.c
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -14,6 +14,8 @@
>  #include "core.h"
>  #include "io.h"
>  
> +#define USB_VENDOR_MICROCHIP 0x0424
> +
>  #define DWC3_ULPI_ADDR(a) \
>  		((a >= ULPI_EXT_VENDOR_SPECIFIC) ? \
>  		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
> @@ -83,6 +85,45 @@ static const struct ulpi_ops dwc3_ulpi_ops = {
>  	.write = dwc3_ulpi_write,
>  };
>  
> +static void dwc3_ulpi_detect_quirks(struct dwc3 *dwc)

Can we rename this to dwc3_ulpi_setup() or dwc3_ulpi_config()?

I would avoid to use the term "quirk", it's to indicate something's
broken. In your case, I think it's more correct to note that it just
a config specific to your chip.

> +{
> +	int ret;
> +	u16 vendor_id, product_id;

Just minor formatting style request: Can we declare variables in
separate lines and keep them in reversed christmas tree style?

> +
> +	/* Test the interface */
> +	ret = dwc3_ulpi_write(dwc->dev, ULPI_SCRATCH, 0xaa);
> +	if (ret < 0)
> +		return;
> +
> +	ret = dwc3_ulpi_read(dwc->dev, ULPI_SCRATCH);
> +	if (ret < 0)
> +		return;
> +
> +	if (ret != 0xaa)
> +		return;
> +
> +	vendor_id = dwc3_ulpi_read(dwc->dev, ULPI_VENDOR_ID_LOW);
> +	vendor_id |= dwc3_ulpi_read(dwc->dev, ULPI_VENDOR_ID_HIGH) << 8;
> +
> +	product_id = dwc3_ulpi_read(dwc->dev, ULPI_PRODUCT_ID_LOW);
> +	product_id |= dwc3_ulpi_read(dwc->dev, ULPI_PRODUCT_ID_HIGH) << 8;
> +
> +	pr_info(

Use dev_debug(). Normal users probably don't need to know this info.

> +		"dwc3_ulpi: VendorID 0x%04x, ProductID 0x%04x\n",
> +		vendor_id, product_id
> +	);
> +	switch (vendor_id) {
> +	case USB_VENDOR_MICROCHIP:
> +		switch (product_id) {
> +		case 0x0009:
> +			/* Microchip USB3340 ULPI PHY */
> +			dwc->enable_usb2_transceiver_delay = true;

Do we really need this field enable_usb2_transceiver_delay anymore now
that we have this check base on the ProductID?

> +			break;
> +		}
> +		break;
> +	}
> +}
> +
>  int dwc3_ulpi_init(struct dwc3 *dwc)
>  {
>  	/* Register the interface */
> @@ -92,6 +133,8 @@ int dwc3_ulpi_init(struct dwc3 *dwc)
>  		return PTR_ERR(dwc->ulpi);
>  	}
>  
> +	dwc3_ulpi_detect_quirks(dwc);
> +
>  	return 0;
>  }
>  
> -- 
> 2.52.0
> 

This is a much better solution than before!

Thanks,
Thinh

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.
  2026-02-28  0:59   ` Thinh Nguyen
@ 2026-03-02 11:53     ` Ingo Rohloff
  2026-03-03  0:22       ` Thinh Nguyen
  0 siblings, 1 reply; 5+ messages in thread
From: Ingo Rohloff @ 2026-03-02 11:53 UTC (permalink / raw)
  To: Thinh Nguyen; +Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org

Hello Thinh,

I hope I didn't cut out too much of the discussion:

> > --- a/drivers/usb/dwc3/ulpi.c
> > +++ b/drivers/usb/dwc3/ulpi.c
> >
> > +static void dwc3_ulpi_detect_quirks(struct dwc3 *dwc) 
> > +... 
> > +	switch (vendor_id) {
> > +	case USB_VENDOR_MICROCHIP:
> > +		switch (product_id) {
> > +		case 0x0009:
> > +			/* Microchip USB3340 ULPI PHY */
> > +			dwc->enable_usb2_transceiver_delay = true; 
> > ...
>
> 
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> >
> > +static void dwc3_hs_apply_ulpi_quirks(struct dwc3 *dwc)
> > +{
> > +   if (dwc->enable_usb2_transceiver_delay) {
> > +	    int index;
> > +       u32 reg;
> > +
> > +	   for (index = 0; index < dwc->num_usb2_ports; index++) {
> > +          reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
> > +          reg |= DWC3_GUSB2PHYCFG_XCVRDLY;
> > +          dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
> > +      }
> > +  }  
> > ...
>
> This should be placed in dwc3_ulpi_init() or dwc3_ulpi_detect_quirks().
>
> ...
>
> Do we really need this field enable_usb2_transceiver_delay anymore now
> that we have this check base on the ProductID?

I have thought about this, but I think I have to do it like above.

I think modifying the GUSB2PHYCFG in drivers/usb/dwc3/ulpi.c is the wrong
place.
"ulpi.c" currently doesn't touch the DWC3 controller itself at all.

It seems all of the config for GUSB2PHYCFG is done via `dwc3_core_init()`.
This function is also called for resuming operation after a suspend:

dwc3_plat_runtime_resume
  dwc3_runtime_resume
    dwc3_resume_common
      dwc3_core_init_for_resume
        dwc3_core_init

So I think the right place to modify GUSB2PHYCFG according to the detected
UPLI PHY is in dwc3_core_init().

Note: dwc3_core_init() detects the ULPI PHY only once:

   if (!dwc->ulpi_ready) {
      ...
      dwc->ulpi_ready = true;
   }

My previous v3 patch has the bug to call `dwc3_hs_apply_ulpi_quirks`
(to be renamed in v4) ONLY ONCE. I think that's wrong:

The strategy is: When the ULPI PHY is detected, set
   dwc->enable_usb2_transceiver_delay
if necessary.
Then use this bit to modify GUSB2PHYCFG if necessary:
* Once after the ULPI PHY is detected initially,
  via dwc3_core_probe().
* Each time operation is resumed after a suspend.
  That's why `dwc3_hs_apply_ulpi_quirks` has to be called
  each time dwc3_core_init() is called.


**ALTERNATIVE**
Store the found ULPI Vendor/Product ID in `struct dwc3` instead of the
`enable_usb2_transceiver_delay` boolean and then directly use the
ULPI Vendor/Product ID to apply the necessary config of GUSB2PHYCFG in
`dwc3_hs_apply_ulpi_quirks`.

What do you think?



I will try to incorporate all other feedback you gave me.

with best regards
  Ingo


-- 


-------------------------------------------------------------------------
Dipl.-Inform.
Ingo ROHLOFF
Senior Staff Embedded Systems Engineering
phone +49 8102 9876-142 - ingo.rohloff@lauterbach.com

Lauterbach Engineering GmbH & Co. KG
Altlaufstrasse 40, 85635 Hoehenkirchen-Siegertsbrunn, GERMANY
www.lauterbach.com

Registered Office: Hoehenkirchen-Siegertsbrunn, Germany,
Local Court: Munich, HRA 87406, VAT ID: DE246770537,
Managing Directors: Lothar Lauterbach, Stephan Lauterbach, Dr. Thomas
Ullmann

-------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.
  2026-03-02 11:53     ` Ingo Rohloff
@ 2026-03-03  0:22       ` Thinh Nguyen
  0 siblings, 0 replies; 5+ messages in thread
From: Thinh Nguyen @ 2026-03-03  0:22 UTC (permalink / raw)
  To: Ingo Rohloff
  Cc: Thinh Nguyen, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org

On Mon, Mar 02, 2026, Ingo Rohloff wrote:
> Hello Thinh,
> 
> I hope I didn't cut out too much of the discussion:
> 
> > > --- a/drivers/usb/dwc3/ulpi.c
> > > +++ b/drivers/usb/dwc3/ulpi.c
> > >
> > > +static void dwc3_ulpi_detect_quirks(struct dwc3 *dwc) 
> > > +... 
> > > +	switch (vendor_id) {
> > > +	case USB_VENDOR_MICROCHIP:
> > > +		switch (product_id) {
> > > +		case 0x0009:
> > > +			/* Microchip USB3340 ULPI PHY */
> > > +			dwc->enable_usb2_transceiver_delay = true; 
> > > ...
> >
> > 
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > >
> > > +static void dwc3_hs_apply_ulpi_quirks(struct dwc3 *dwc)
> > > +{
> > > +   if (dwc->enable_usb2_transceiver_delay) {
> > > +	    int index;
> > > +       u32 reg;
> > > +
> > > +	   for (index = 0; index < dwc->num_usb2_ports; index++) {
> > > +          reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
> > > +          reg |= DWC3_GUSB2PHYCFG_XCVRDLY;
> > > +          dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
> > > +      }
> > > +  }  
> > > ...
> >
> > This should be placed in dwc3_ulpi_init() or dwc3_ulpi_detect_quirks().
> >
> > ...
> >
> > Do we really need this field enable_usb2_transceiver_delay anymore now
> > that we have this check base on the ProductID?
> 
> I have thought about this, but I think I have to do it like above.
> 
> I think modifying the GUSB2PHYCFG in drivers/usb/dwc3/ulpi.c is the wrong
> place.
> "ulpi.c" currently doesn't touch the DWC3 controller itself at all.
> 
> It seems all of the config for GUSB2PHYCFG is done via `dwc3_core_init()`.
> This function is also called for resuming operation after a suspend:
> 
> dwc3_plat_runtime_resume
>   dwc3_runtime_resume
>     dwc3_resume_common
>       dwc3_core_init_for_resume
>         dwc3_core_init
> 
> So I think the right place to modify GUSB2PHYCFG according to the detected
> UPLI PHY is in dwc3_core_init().
> 
> Note: dwc3_core_init() detects the ULPI PHY only once:
> 
>    if (!dwc->ulpi_ready) {
>       ...
>       dwc->ulpi_ready = true;
>    }
> 

Ah! I forgot that the ULPI init function is only executed once.

> My previous v3 patch has the bug to call `dwc3_hs_apply_ulpi_quirks`
> (to be renamed in v4) ONLY ONCE. I think that's wrong:
> 
> The strategy is: When the ULPI PHY is detected, set
>    dwc->enable_usb2_transceiver_delay
> if necessary.
> Then use this bit to modify GUSB2PHYCFG if necessary:
> * Once after the ULPI PHY is detected initially,
>   via dwc3_core_probe().
> * Each time operation is resumed after a suspend.
>   That's why `dwc3_hs_apply_ulpi_quirks` has to be called
>   each time dwc3_core_init() is called.
> 

Yes. Do this. If there's a poweroff and vcc reset is asserted, then the
GUSB2PHYCFG will need to be re-initialized.

> 
> **ALTERNATIVE**
> Store the found ULPI Vendor/Product ID in `struct dwc3` instead of the
> `enable_usb2_transceiver_delay` boolean and then directly use the
> ULPI Vendor/Product ID to apply the necessary config of GUSB2PHYCFG in
> `dwc3_hs_apply_ulpi_quirks`.
> 

Not this.

> What do you think?
> 

Sounds good. Thanks for double checking.

> 
> 
> I will try to incorporate all other feedback you gave me.
> 

Thanks!

BR,
Thinh

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-03-03  0:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-27 13:30 [PATCH v3 0/1] usb: dwc3: Support for USB3340x ULPI PHY Ingo Rohloff
2026-02-27 13:30 ` [PATCH v3 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation Ingo Rohloff
2026-02-28  0:59   ` Thinh Nguyen
2026-03-02 11:53     ` Ingo Rohloff
2026-03-03  0:22       ` Thinh Nguyen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox