[PATCH v2] usb: dwc3-generic: fix ref clock lookup indexing wrong clk_bulk array

[PATCH v2] usb: dwc3-generic: fix ref clock lookup indexing wrong clk_bulk array

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

On Verdin iMX95, with a USB hub permanently attached to the DWC3 host
port the hub itself enumerates fine but a USB stick plugged into it
fails intermittently:

  Verdin iMX95 # usb start
  starting USB...
  USB XHCI 1.10
  Device not responding to set address.

        USB device not accepting new address (error=80000000)
  Bus usb@4c100000: 2 USB Device(s) found
         scanning usb for storage devices... 0 Storage Device(s) found
  Verdin iMX95 # usb reset
  resetting USB...
  USB XHCI 1.10
  WARN endpoint is halted
  WARN endpoint is halted
  unable to get device descriptor (error=-22)

Commit c6583354b7c1 ("usb: dwc3: Look up reference clock DT phandle in
both controller DT nodes") added a fallback that searches the DWC3 child
node's "clock-names" for "ref"/"ref_clk" when it isn't found on the
parent glue node, but always resolves the found index against
glue->clks, a clk_bulk populated only from the parent glue node's own
clock-names list. This is wrong since the child node's clock-names list
is not guaranteed to be in the same order as the parent's, and indeed on
Verdin iMX95 the child node has a different order than the parent node.

Fix it by looking the reference clock up by name with clk_get_by_name(),
trying the dwc3 device and then its parent glue device, so the clock is
always resolved through the device that owns the node its name was found
in. Also enable the clock before use, and disable it again in
dwc3_generic_remove() to keep the enable balanced.

Because we moved to clk_get_by_name() for receiving the reference clock,
also ensure that the parent driver is a DWC3 glue driver before looking
up the reference clock, since the parent could be a bus or root device
with unrelated clocks.

Fixes: c6583354b7c1 ("usb: dwc3: Look up reference clock DT phandle in both controller DT nodes")
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
Tested-by: Tom Hsieh <Tom.Hsieh@advantech.com>

---
Changes in v2:
- Check if the parent driver is a DWC3 glue driver before looking up the
  reference clock. I use driver->name as this should work with all
  implementations used today and I hope this is more generic. (Marek)
- Add Tested-by from Tom
---
 drivers/usb/dwc3/dwc3-generic.c | 52 +++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 2356b3bc0aa..3c3ff4eb590 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -13,6 +13,7 @@
 #include <dm/device_compat.h>
 #include <dm/lists.h>
 #include <linux/delay.h>
+#include <linux/string.h>
 #include <linux/usb/gadget.h>
 #include <power/regulator.h>
 #include <usb/xhci.h>
@@ -31,6 +32,7 @@ struct dwc3_generic_priv {
 	struct dwc3 dwc3;
 	struct phy_bulk phys;
 	struct gpio_desc *ulpi_reset;
+	struct clk ref_clk;
 };
 
 struct dwc3_generic_host_priv {
@@ -47,8 +49,7 @@ static int dwc3_generic_probe(struct udevice *dev,
 	struct dwc3_generic_plat *plat = dev_get_plat(dev);
 	struct dwc3 *dwc3 = &priv->dwc3;
 	struct dwc3_glue_data *glue = dev_get_plat(dev->parent);
-	int __maybe_unused index;
-	ofnode __maybe_unused node;
+	struct clk __maybe_unused clk;
 
 	dwc3->dev = dev;
 	dwc3->maximum_speed = plat->maximum_speed;
@@ -63,22 +64,36 @@ static int dwc3_generic_probe(struct udevice *dev,
 	 * - in top level generic node, with no subnode (i.MX8MQ)
 	 * - in generic subnode, with other clock in top level node (i.MX8MP)
 	 * - in both top level node and generic subnode (Rockchip)
-	 * Cover all the possibilities here by looking into both nodes, start
-	 * with the top level node as that seems to be used in majority of DTs
-	 * to reference the clock.
+	 * Cover all the possibilities here by looking into both nodes, the
+	 * generic node first and the top level glue node second. The clock
+	 * must be looked up on the device that owns the node it is described
+	 * in, the clock-names lists of the two nodes are unrelated (e.g. on
+	 * i.MX95 the glue node has "hsio", "suspend" while the generic
+	 * subnode has "bus_early", "ref", "suspend").
 	 */
-	node = dev_ofnode(dev->parent);
-	index = ofnode_stringlist_search(node, "clock-names", "ref");
-	if (index < 0)
-		index = ofnode_stringlist_search(node, "clock-names", "ref_clk");
-	if (index < 0) {
-		node = dev_ofnode(dev);
-		index = ofnode_stringlist_search(node, "clock-names", "ref");
-		if (index < 0)
-			index = ofnode_stringlist_search(node, "clock-names", "ref_clk");
+	rc = clk_get_by_name(dev, "ref", &clk);
+	if (rc)
+		rc = clk_get_by_name(dev, "ref_clk", &clk);
+	/*
+	 * Only fall back to the parent node's clocks if the parent is a
+	 * dwc3 glue wrapper. When the generic node is a top level node,
+	 * the parent is not the glue device but a bus or the root device,
+	 * and its clocks are unrelated to the controller.
+	 */
+	if (rc && dev->parent && strstr(dev->parent->driver->name, "dwc3")) {
+		rc = clk_get_by_name(dev->parent, "ref", &clk);
+		if (rc)
+			rc = clk_get_by_name(dev->parent, "ref_clk", &clk);
+	}
+	if (!rc) {
+		priv->ref_clk = clk;
+
+		rc = clk_enable(&priv->ref_clk);
+		if (rc && rc != -ENOSYS)
+			dev_warn(dev, "failed to enable ref clock: %d\n", rc);
+
+		dwc3->ref_clk = &priv->ref_clk;
 	}
-	if (index >= 0)
-		dwc3->ref_clk = &glue->clks.clks[index];
 #endif
 
 	/*
@@ -145,6 +160,11 @@ static int dwc3_generic_remove(struct udevice *dev,
 
 	dwc3_remove(dwc3);
 	dwc3_shutdown_phy(dev, &priv->phys);
+
+	/* Balance the clk_enable() from dwc3_generic_probe() */
+	if (dwc3->ref_clk)
+		clk_disable(dwc3->ref_clk);
+
 	unmap_physmem(dwc3->regs, MAP_NOCACHE);
 
 	return 0;
-- 
2.55.0

Re: [PATCH v2] usb: dwc3-generic: fix ref clock lookup indexing wrong clk_bulk array

[Marek Vasut]

On 9/22/26 10:42 PM, Stefan Eichenberger wrote:

Hello Stefan,

> +	rc = clk_get_by_name(dev, "ref", &clk);
> +	if (rc)
> +		rc = clk_get_by_name(dev, "ref_clk", &clk);
> +	/*
> +	 * Only fall back to the parent node's clocks if the parent is a
> +	 * dwc3 glue wrapper. When the generic node is a top level node,
> +	 * the parent is not the glue device but a bus or the root device,
> +	 * and its clocks are unrelated to the controller.
> +	 */
> +	if (rc && dev->parent && strstr(dev->parent->driver->name, "dwc3")) {

Could dev->parent->driver be NULL ?

> +		rc = clk_get_by_name(dev->parent, "ref", &clk);
> +		if (rc)
> +			rc = clk_get_by_name(dev->parent, "ref_clk", &clk);
> +	}
> +	if (!rc) {
> +		priv->ref_clk = clk;
> +
> +		rc = clk_enable(&priv->ref_clk);
> +		if (rc && rc != -ENOSYS)
> +			dev_warn(dev, "failed to enable ref clock: %d\n", rc);

Shouldn't the driver fail to probe if clock cannot be enabled ?

[...]

Re: [PATCH v2] usb: dwc3-generic: fix ref clock lookup indexing wrong clk_bulk array

[Stefan Eichenberger]

Hi Marek,

On Wed, Sep 23, 2026 at 03:30:44PM +0200, Marek Vasut wrote:
> On 9/22/26 10:42 PM, Stefan Eichenberger wrote:
> 
> Hello Stefan,
> 
> > +	rc = clk_get_by_name(dev, "ref", &clk);
> > +	if (rc)
> > +		rc = clk_get_by_name(dev, "ref_clk", &clk);
> > +	/*
> > +	 * Only fall back to the parent node's clocks if the parent is a
> > +	 * dwc3 glue wrapper. When the generic node is a top level node,
> > +	 * the parent is not the glue device but a bus or the root device,
> > +	 * and its clocks are unrelated to the controller.
> > +	 */
> > +	if (rc && dev->parent && strstr(dev->parent->driver->name, "dwc3")) {
> 
> Could dev->parent->driver be NULL ?
> 

It should never be NULL. This could only happen when we define it in the
root node but then it should never bind. However, I guess it's better to
be safe and check for NULL anyways in case something is really wrong. I
will add the check in the next version.

> > +		rc = clk_get_by_name(dev->parent, "ref", &clk);
> > +		if (rc)
> > +			rc = clk_get_by_name(dev->parent, "ref_clk", &clk);
> > +	}
> > +	if (!rc) {
> > +		priv->ref_clk = clk;
> > +
> > +		rc = clk_enable(&priv->ref_clk);
> > +		if (rc && rc != -ENOSYS)
> > +			dev_warn(dev, "failed to enable ref clock: %d\n", rc);
> 
> Shouldn't the driver fail to probe if clock cannot be enabled ?

Yes that makes sense, I will change that in the next version.

Regards,
Stefan

Re: [PATCH v2] usb: dwc3-generic: fix ref clock lookup indexing wrong clk_bulk array

[Marek Vasut]

On 9/24/26 2:51 PM, Stefan Eichenberger wrote:

Hello Stefan,

> On Wed, Sep 23, 2026 at 03:30:44PM +0200, Marek Vasut wrote:
>> On 9/22/26 10:42 PM, Stefan Eichenberger wrote:
>>
>> Hello Stefan,
>>
>>> +	rc = clk_get_by_name(dev, "ref", &clk);
>>> +	if (rc)
>>> +		rc = clk_get_by_name(dev, "ref_clk", &clk);
>>> +	/*
>>> +	 * Only fall back to the parent node's clocks if the parent is a
>>> +	 * dwc3 glue wrapper. When the generic node is a top level node,
>>> +	 * the parent is not the glue device but a bus or the root device,
>>> +	 * and its clocks are unrelated to the controller.
>>> +	 */
>>> +	if (rc && dev->parent && strstr(dev->parent->driver->name, "dwc3")) {
>>
>> Could dev->parent->driver be NULL ?
>>
> 
> It should never be NULL. This could only happen when we define it in the
> root node but then it should never bind. However, I guess it's better to
> be safe and check for NULL anyways in case something is really wrong. I
> will add the check in the next version.
> 
>>> +		rc = clk_get_by_name(dev->parent, "ref", &clk);
>>> +		if (rc)
>>> +			rc = clk_get_by_name(dev->parent, "ref_clk", &clk);
>>> +	}
>>> +	if (!rc) {
>>> +		priv->ref_clk = clk;
>>> +
>>> +		rc = clk_enable(&priv->ref_clk);
>>> +		if (rc && rc != -ENOSYS)
>>> +			dev_warn(dev, "failed to enable ref clock: %d\n", rc);
>>
>> Shouldn't the driver fail to probe if clock cannot be enabled ?
> 
> Yes that makes sense, I will change that in the next version.

Thank you!

-- 
Best regards,
Marek Vasut