Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Ze Huang <huang.ze@linux.dev>]

On Sun, Jul 20, 2025 at 02:34:07PM +0800, Ze Huang wrote:
> On Tue, Jul 15, 2025 at 03:50:54PM -0500, Alex Elder wrote:
> > On 7/12/25 2:49 AM, Ze Huang wrote:
> > > To support flattened dwc3 dt model and drop the glue layer, introduce the
> > > `dwc3-generic` driver. This enables direct binding of the DWC3 core driver
> > > and offers an alternative to the existing glue driver `dwc3-of-simple`.
> > 
> > I'm not familiar with dwc-of-simple.c, and won't comment on
> > how this differs from that (or does not).
> > 
> > Given you're implementing an alternative though, can you explain
> > in a little more detail what's different between the two?  Why
> > would someone choose to use this driver rather than the other one?
> 
> They are basically the same.
> 
> dwc-generic use a plain dt node while dwc-of-simple will nest the dwc3
> node as its child.
> 
> Both will use dwc3_core_probe() to finish the probe process. But now we
> can simplify the process by just calling it, instead of calling
> of_platform_populate() and create another snps,dwc3 device driver.

[...]

> > > +	ret = reset_control_assert(dwc3->resets);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to assert resets\n");
> > > +
> > > +	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
> > > +	if (ret)
> > > +		return ret;
> > 
> > The re-assert shouldn't be set up unless the deassert below
> > succeeds.
> > 
> 
> Will move behind the deassert.
> 
> > > +	usleep_range(10, 1000);
> > 
> > This seems like a large range.  You could just do msleep(1);
> > Also, can you add a comment explaining why a delay is needed,
> > and why 1 millisecond is the right amount of time to sleep?
> > 
> 
> I will check the range with spacemit and reply soon.
> 

the resets are asynchronous with no strict timing. But to be safe, each
reset should stay active for at least 1 µs. I’ll switch to a udelay(2)
and add comment accordingly.

> > > +	ret = reset_control_deassert(dwc3->resets);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to deassert resets\n");
> > > +
> > > +	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> > 
> > Call devm_clk_bulk_get_all_enabled() instead of doing the two
> > steps separately here.
> > 
> 
> Will do, thanks.
> 
> > 					-Alex