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

[Yao Zi <ziyao@disroot.org>]

On Mon, Jul 21, 2025 at 08:08:06PM +0800, Ze Huang wrote:
> 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.

This may be a little farsight: do you think it's better to make the
reset timing part of the of_match_data? This is more flexible and
reduces future burden when introducing a new platform that comes with a
different reset timing, which is a very likely case we'll face since
it's a "generic" driver.

> > > > +	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
> 

Regards,
Yao Zi