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

[PATCH v1] 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.

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

This patch was originally part of this series:
https://lore.kernel.org/all/20260914074314.12522-1-eichest@gmail.com/
Marek asked to split it out, as it is a separate bug fix.

---
 drivers/usb/dwc3/dwc3-generic.c | 44 +++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 2356b3bc0aa..2982a91e3ad 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -31,6 +31,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 +48,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 +63,29 @@ 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);
+	if (rc)
+		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 +152,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 v1] usb: dwc3-generic: fix ref clock lookup indexing wrong clk_bulk array

[Marek Vasut]

On 9/16/26 8:16 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);
> +	if (rc)
> +		rc = clk_get_by_name(dev->parent, "ref", &clk);

One small nitpick -- would it make sense to validate whether the parent 
node has "dwc3" or "snps" in its compatible string, so this wouldn't 
accidentally attempt to grab clock of some completely unrelated bus node ?

> +	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
The rest is good, thank you !

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

[Stefan Eichenberger]

Hi Marek,

On Wed, Sep 16, 2026 at 10:43:50PM +0200, Marek Vasut wrote:
> On 9/16/26 8:16 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);
> > +	if (rc)
> > +		rc = clk_get_by_name(dev->parent, "ref", &clk);
> 
> One small nitpick -- would it make sense to validate whether the parent node
> has "dwc3" or "snps" in its compatible string, so this wouldn't accidentally
> attempt to grab clock of some completely unrelated bus node ?
> 

I'm not sure if this is necessary, or indeed if it would make things
worse. For example, ti,am62-usb would not match even if it relied on the
fallback for the ref clock. The same applies to samsung,exynos990.dtsi,
which uses "samsung,exynos850-dwusb3". This would mean we would have to
list all the possible compatible strings, which would be difficult to
maintain.

Regards,
Stefan

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

[Marek Vasut]

On 9/17/26 4:24 PM, Stefan Eichenberger wrote:

Hello Stefan,

> On Wed, Sep 16, 2026 at 10:43:50PM +0200, Marek Vasut wrote:
>> On 9/16/26 8:16 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);
>>> +	if (rc)
>>> +		rc = clk_get_by_name(dev->parent, "ref", &clk);
>>
>> One small nitpick -- would it make sense to validate whether the parent node
>> has "dwc3" or "snps" in its compatible string, so this wouldn't accidentally
>> attempt to grab clock of some completely unrelated bus node ?
>>
> 
> I'm not sure if this is necessary

Necessary, I would argue yes, because if we don't do that, we might pick 
up clock from some sort of unrelated device and that is simply a failure 
to validate input.

> , or indeed if it would make things
> worse. For example, ti,am62-usb would not match even if it relied on the
> fallback for the ref clock. The same applies to samsung,exynos990.dtsi,
> which uses "samsung,exynos850-dwusb3". This would mean we would have to
> list all the possible compatible strings, which would be difficult to
> maintain.

Isn't that what Linux does as well ?

-- 
Best regards,
Marek Vasut

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

[Tom Hsieh]

Hi Marek, Stefan,

I hit this independently on an i.MX95 board (Advantech AOM-2521, with a
GL3523 hub on the DWC3 root port), so here is some data for the archive
and an attempt at the open question.

What the hardware shows. U-Boot's `clk dump` reports scmi-1 at 32768 Hz
and scmi-2 at 24000000 Hz. "ref" is index 1 of the dwc3 subnode's
clock-names, and index 1 of the glue node's bulk is scmi-1, so
dwc3_ref_clk_period() does its arithmetic on 32768:

                          before    after
  GUCTL.REFCLKPER            821       41
  GFLADJ.REFCLK_FLADJ          2     2032
  GFLADJ.240MHZDECR           28       10

Both columns are register reads. Once the clock is resolved correctly
GFLADJ reads 0x0a87f000, bit for bit what Linux programs on the same
board, and `usb start` goes from

  Device not responding to set address.
  USB device not accepting new address (error=80000000)

on the USB 2.0 half of the hub to both halves enumerating. SuperSpeed
is unaffected here, consistent with it taking its clock from the PIPE
interface rather than from these registers. Those numbers come from a
local change of my own rather than from the patch as posted, so no
Tested-by from me yet - I will send one once I have booted your version.

> > This would mean we would have to list all the possible compatible
> > strings, which would be difficult to maintain.
>
> Isn't that what Linux does as well ?

Not for clocks, no. Linux never looks at the parent for a clock at all.
dwc3_get_clocks() in drivers/usb/dwc3/core.c works only on its own node:

	static int dwc3_get_clocks(struct dwc3 *dwc)
	{
		struct device *dev = dwc->dev;
		...
		dwc->ref_clk = devm_clk_get_optional(dev, "ref");

so there is no parent-compatible check there to copy. The one place
dwc3 core does test the parent's compatible is an unrelated
register-offset quirk:

	if (of_device_is_compatible(parent, "realtek,rtd-dwc3")) {
		dwc_res.start -= DWC3_GLOBALS_REGS_START;
		dwc_res.start += DWC3_RTK_RTD_GLOBALS_REGS_START;
	}

The parent lookup is a U-Boot-only construct, added by c6583354b7c1 to
cover the DTs that keep the clock in the glue node.

On whether dev->parent can be an unrelated bus node: I do not think it
can, and the reason is structural rather than a property of any one DT.
A dwc3-generic device exists only because dwc3_glue_bind() bound it,
either from a subnode of the glue node or, for the flattened-DT glues
that go through dwc3_flat_dt_get_ctrl_dev(), from the glue node itself.
Four drivers use that .bind - dwc3-generic-wrapper, uniphier-dwc3,
dwc3-sti and dwc3-am62 - so dev->parent is always one of those four,
never an arbitrary bus.

As for what the check would cost today, the current list is short enough
to state exactly. Of the 27 compatibles across those four glue drivers'
of_match tables, three would fail a "dwc3"/"snps" substring test:

	ti,am62-usb                 drivers/usb/dwc3/dwc3-am62.c
	samsung,exynos7870-dwusb3   drivers/usb/dwc3/dwc3-generic.c
	samsung,exynos850-dwusb3    drivers/usb/dwc3/dwc3-generic.c

Stefan named the first and third; exynos7870 is the one that completes
the set. So the check would need three exceptions on the day it lands,
and a fourth the next time a glue is added that does not spell "dwc3"
in its compatible.

If the intent is to validate that the parent really is a DWC3 glue
rather than to enumerate SoCs, the binding above is the thing that
actually expresses it, and it does not need a list.

Either way, please do not let this hold the fix up on my account - the
wrong reference clock is a real failure on shipping hardware.

Thanks,
Tom

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

[Stefan Eichenberger]

Hi Marek and Tom,

On Mon, Sep 21, 2026 at 01:44:32PM +0800, Tom Hsieh wrote:
> Hi Marek, Stefan,
> 
> I hit this independently on an i.MX95 board (Advantech AOM-2521, with a
> GL3523 hub on the DWC3 root port), so here is some data for the archive
> and an attempt at the open question.
> 
> What the hardware shows. U-Boot's `clk dump` reports scmi-1 at 32768 Hz
> and scmi-2 at 24000000 Hz. "ref" is index 1 of the dwc3 subnode's
> clock-names, and index 1 of the glue node's bulk is scmi-1, so
> dwc3_ref_clk_period() does its arithmetic on 32768:
> 
>                           before    after
>   GUCTL.REFCLKPER            821       41
>   GFLADJ.REFCLK_FLADJ          2     2032
>   GFLADJ.240MHZDECR           28       10
> 
> Both columns are register reads. Once the clock is resolved correctly
> GFLADJ reads 0x0a87f000, bit for bit what Linux programs on the same
> board, and `usb start` goes from
> 
>   Device not responding to set address.
>   USB device not accepting new address (error=80000000)
> 
> on the USB 2.0 half of the hub to both halves enumerating. SuperSpeed
> is unaffected here, consistent with it taking its clock from the PIPE
> interface rather than from these registers. Those numbers come from a
> local change of my own rather than from the patch as posted, so no
> Tested-by from me yet - I will send one once I have booted your version.
> 
> > > This would mean we would have to list all the possible compatible
> > > strings, which would be difficult to maintain.
> >
> > Isn't that what Linux does as well ?
> 
> Not for clocks, no. Linux never looks at the parent for a clock at all.
> dwc3_get_clocks() in drivers/usb/dwc3/core.c works only on its own node:
> 
> 	static int dwc3_get_clocks(struct dwc3 *dwc)
> 	{
> 		struct device *dev = dwc->dev;
> 		...
> 		dwc->ref_clk = devm_clk_get_optional(dev, "ref");
> 
> so there is no parent-compatible check there to copy. The one place
> dwc3 core does test the parent's compatible is an unrelated
> register-offset quirk:
> 
> 	if (of_device_is_compatible(parent, "realtek,rtd-dwc3")) {
> 		dwc_res.start -= DWC3_GLOBALS_REGS_START;
> 		dwc_res.start += DWC3_RTK_RTD_GLOBALS_REGS_START;
> 	}
> 
> The parent lookup is a U-Boot-only construct, added by c6583354b7c1 to
> cover the DTs that keep the clock in the glue node.

I came to a similar conclusion. In Linux the dwc3 doesn't have to know
about the ref_clk and it is therefore only managed if it has its own
ref_clk clock set.

> 
> On whether dev->parent can be an unrelated bus node: I do not think it
> can, and the reason is structural rather than a property of any one DT.
> A dwc3-generic device exists only because dwc3_glue_bind() bound it,
> either from a subnode of the glue node or, for the flattened-DT glues
> that go through dwc3_flat_dt_get_ctrl_dev(), from the glue node itself.
> Four drivers use that .bind - dwc3-generic-wrapper, uniphier-dwc3,
> dwc3-sti and dwc3-am62 - so dev->parent is always one of those four,
> never an arbitrary bus.
> 

I think it would be possible to define a "snps,dwc3" compatible node
without a wrapper node and then probe is called. Because it relies on a
wrapper, the ref_clk lookup won't work. So one thing that would
currently work is to just test the driver name. Something like this: 
if (rc && dev->parent && strstr(dev->parent->driver->name, "dwc3"))

Every wrapper driver currently has dwc3 in its name:
- uniphier-dwc3
- dwc3-sti
- dwc3-am62
- dwc3-generic-wrapper
However, if we have to handle this corner case, wouldn't it be better
that probe fails entirely when dwc3_generic_probe is not called by a
dwc3 wrapper driver? Or asking differently, is it supposed to work that
the "snps,dwc3" node can be used without a wrapper driver?

If that's not supposed to work, I would propose something like this at
the top of dwc3_generic_probe:
if (!strstr(dev->parent->driver->name, "dwc3"))
	return -EINVAL;

Regards,
Stefan

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

[Marek Vasut]

On 9/21/26 11:10 AM, Stefan Eichenberger wrote:

Hello everyone,

> However, if we have to handle this corner case, wouldn't it be better
> that probe fails entirely when dwc3_generic_probe is not called by a
> dwc3 wrapper driver? Or asking differently, is it supposed to work that
> the "snps,dwc3" node can be used without a wrapper driver?

arch/arm/boot/dts/nxp/ls/ls1021a.dtsi

792                 usb3: usb@3100000 {
793                         compatible = "snps,dwc3";

It seems snps,dwc3 can be used without a wrapper ?

> If that's not supposed to work, I would propose something like this at
> the top of dwc3_generic_probe:
> if (!strstr(dev->parent->driver->name, "dwc3"))
> 	return -EINVAL;

The parent driver name check sounds good !

-- 
Best regards,
Marek Vasut

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

[Stefan Eichenberger]

Hi Marek and Tom,

Thanks a lot for the feedback and explanations.

On Mon, Sep 21, 2026 at 11:40:15AM +0200, Marek Vasut wrote:
> On 9/21/26 11:10 AM, Stefan Eichenberger wrote:
> 
> Hello everyone,
> 
> > However, if we have to handle this corner case, wouldn't it be better
> > that probe fails entirely when dwc3_generic_probe is not called by a
> > dwc3 wrapper driver? Or asking differently, is it supposed to work that
> > the "snps,dwc3" node can be used without a wrapper driver?
> 
> arch/arm/boot/dts/nxp/ls/ls1021a.dtsi
> 
> 792                 usb3: usb@3100000 {
> 793                         compatible = "snps,dwc3";
> 
> It seems snps,dwc3 can be used without a wrapper ?
> 
> > If that's not supposed to work, I would propose something like this at
> > the top of dwc3_generic_probe:
> > if (!strstr(dev->parent->driver->name, "dwc3"))
> > 	return -EINVAL;
> 
> The parent driver name check sounds good !

Perfect, then I will add this check to v2.

Regards,
Stefan

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

[Tom Hsieh]

Hi Stefan,

Thanks for looking into this. There are examples of standalone
"snps,dwc3" nodes, such as rp1_usb0 and rp1_usb1 in
dts/upstream/src/arm64/broadcom/rp1-common.dtsi.

What I am still missing is the binding path from such a node to
dwc3_generic_probe(). Neither dwc3-generic-host nor
dwc3-generic-peripheral has an of_match table. The direct "snps,dwc3"
match is provided by xhci-dwc3, when that driver is enabled, and uses a
separate probe implementation.

The platform binding path I found for the two generic drivers is
dwc3_glue_bind_common(), called by dwc3_glue_bind():

	device_bind_driver_to_node(parent, driver, name, node, &dev);

Here, parent is the glue device. This also holds for the flattened-DT
case: the glue and controller devices share an ofnode, but the controller
device is still bound as a child of the glue device.

So my understanding is that merely describing a standalone "snps,dwc3"
node would not make dwc3_generic_probe() run with an unrelated bus device
as its parent. On the current normal in-tree binding paths, the proposed
driver-name test would pass.

Is there another binding path you had in mind?

An explicit check could still serve as a defensive guard. My concern with
strstr() is that it checks a naming convention rather than the parent's
role. If documenting the existing assumption is sufficient, a comment
explaining that dev->parent is the glue device which bound this controller
might help.

I am happy to leave the choice of guard to you and Marek - this is just
to clarify the binding path.

Regards,
Tom

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

[Tom Hsieh]

Hi Stefan,

I tested your patch on i.MX95 (Advantech AOM-2521), with a GL3523 hub
connected to the DWC3 root port.

The board is not supported in mainline, so testing was done on NXP BSP
u-boot-imx 2026.04 (lf_v2026.04). Compared with your patch's mainline
pre-image, the BSP dwc3-generic.c differs only in board_usb_init() /
board_usb_cleanup() hooks and glue ops linkage and const qualifiers,
all outside the ref-clock code.

The ref-clock hunks applied exactly; only the dwc3_generic_remove()
hunk needed a six-line offset and fuzz 1. I replaced my local ref-clock
fix with your patch, keeping the other five BSP patches unchanged.

After "usb start":

  GUCTL  0x4c10c12c = 0x0a404010    REFCLKPER    = 41
  GFLADJ 0x4c10c630 = 0x0a87f000    REFCLK_FLADJ = 2032, 240MHZDECR = 10

These fields match the expected values for a 24 MHz reference.
GFLADJ also matches the value Linux programs on this board. Earlier
testing of an unfixed build gave 821 / 2 / 28 when the 32.768 kHz clock
was selected.

Both halves of the GL3523 now enumerate, the USB 3.2 half at 5 Gb/s and
the USB 2.1 half at 480 Mb/s. The 480 Mb/s half previously failed with
"Device not responding to set address"; that error is absent with your
patch.

The added clk_enable() also works for the SCMI reference clock on this
board. There is no "failed to enable ref clock" message, and "clk dump"
shows the 24 MHz clock's usecount increasing from 0 to 1:

  before "usb start":   24000000   0   scmi-2
  after  "usb start":   24000000   1   osc24m

Testing covered boot-time hub enumeration; no USB storage device was
attached, and the remove path was not exercised.

Tested-by: Tom Hsieh <Tom.Hsieh@advantech.com>

Regards,
Tom