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

On 8/24/26 8:29 PM, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <[email protected]>
> 
> 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 resolving the index against the child node's clk and ensure to
> enable the clock before using it.
> 
> Fixes: c6583354b7c1 ("usb: dwc3: Look up reference clock DT phandle in both controller DT nodes")
> Signed-off-by: Stefan Eichenberger <[email protected]>
> ---
>   drivers/usb/dwc3/dwc3-generic.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
> index 2356b3bc0aa..e16d236bd9d 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 {
> @@ -76,9 +77,28 @@ static int dwc3_generic_probe(struct udevice *dev,
>   		index = ofnode_stringlist_search(node, "clock-names", "ref");
>   		if (index < 0)
>   			index = ofnode_stringlist_search(node, "clock-names", "ref_clk");
> -	}
> -	if (index >= 0)
> +		/*
> +		 * glue->clks was populated from the parent glue node's own
> +		 * clock-names list, which can differ in length/order from
> +		 * this child node's list (e.g. i.MX95: glue has "hsio",
> +		 * "suspend", the dwc3 child has "bus_early", "ref",
> +		 * "suspend"). Reusing this index against glue->clks would
> +		 * silently resolve to the wrong clock, so fetch it directly
> +		 * from this node instead.
> +		 */
> +		if (index >= 0) {
> +			if (!clk_get_by_index(dev, index, &priv->ref_clk)) {

Would it be possible to call clk_get_by_name(dev, ...) and 
clk_get_by_name(dev->parent, ...) instead, until one of them succeeds ? 
That could make the code simpler.

> +				clk_enable(&priv->ref_clk);
> +				dwc3->ref_clk = &priv->ref_clk;
> +			} else {
> +				debug("%s: failed to get ref_clk from DT\n", dev->name);

dev_dbg()

> +			}
> +		} else {
> +			debug("%s: no ref_clk found in DT\n", dev->name);

dev_dbg()

> +		}
> +	} else {
>   		dwc3->ref_clk = &glue->clks.clks[index];
> +	}
>   #endif
>   
>   	/*