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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aH-0d0OyLIo9tlkd@monica.localdomain>
Date: Tue, 22 Jul 2025 23:55:46 +0800
From: Ze Huang <huang.ze@...ux.dev>
To: Yao Zi <ziyao@...root.org>, Ze Huang <huang.ze@...ux.dev>,
	Alex Elder <elder@...e.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
	Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
	Philipp Zabel <p.zabel@...gutronix.de>
Cc: linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

On Tue, Jul 22, 2025 at 12:34:46AM +0000, Yao Zi wrote:
> 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.
> 

Hi Zi Yao, thanks for the suggestion.

The delay is only for safety. I think there will not be much device require
this setting. I'd prefer to keep the logic simple and just cover the
currently known compatible devices.

If we encounter future platforms that require different timing constraints,
we can revisit this and introduce of_match_data as needed.

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