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

[Tom Hsieh <hungen3108@gmail.com>]

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