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

On 8/26/26 9:18 AM, Stefan Eichenberger wrote:
> On Tue, Aug 25, 2026 at 01:40:44AM +0200, Marek Vasut wrote:
>> 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.
>>
> 
> You are right, I try to rework that in the next version. Thanks for the
> suggestion. I will also remove/replace the debug() calls.
Excellent, thank you !