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

[Stefan Eichenberger <eichest@gmail.com>]

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