linux-kernel - Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZxirM9HJELXGWVqv@hovoldconsulting.com>
Date: Wed, 23 Oct 2024 09:52:19 +0200
From: Johan Hovold <johan@...nel.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Rajendra Nayak <quic_rjendra@...cinc.com>,
	Sibi Sankar <quic_sibis@...cinc.com>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Trilok Soni <quic_tsoni@...cinc.com>, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C
 Retimer

On Wed, Oct 23, 2024 at 10:32:09AM +0300, Abel Vesa wrote:
> On 24-10-23 09:04:10, Johan Hovold wrote:
> > On Tue, Oct 22, 2024 at 12:01:14PM +0300, Abel Vesa wrote:
> > > On 24-10-15 15:03:15, Johan Hovold wrote:
> > > > On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:
> > 
> > > > > +	ret = ps8830_get_vregs(retimer);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > > > > +	if (IS_ERR(retimer->xo_clk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > > > > +				     "failed to get xo clock\n");
> > > > > +
> > > > > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > > 
> > > > The reset line is active low and should be described as such in DT. So
> > > > here you want to request it as logically low if you want to deassert
> > > > reset.
> > > 
> > > This is being reworked in v3 as we need to support cases where the
> > > retimer has been left enabled and initialized by bootloader and we want
> > > to keep that state until unplug event for the cold-plug orientation
> > > to work properly.
> > > 
> > > On top of that, we don't want to deassert the reset here. We do that
> > > via gpiod_set_value() call below, after the clocks and regulators have
> > > been enabled.
> > 
> > Ok, but you should generally not drive an input high before powering on
> > the device as that can damage the IC (more below).
> 
> This is just not true, generally. Think of top level XTALs which feed in
> clocks (and can't be disabled) before ICs are enabled.

I'm talking about an I/O pin here, you must generally not drive those
high before powering on the IC.

And AFAIU the same applies to clocks even though the risk of damage
there is lower.

> > That is, in this case, you should not deassert reset before making sure
> > the supplies are enabled.
> 
> Wrong. Even the data sheet of this retimer shows in the timigs plot the
> reset as being asserted before the supplies are enabled.

Reset *asserted*, yes (i.e. pull to ground). Not *deasserted* (i.e.
drive high) as you are doing here.

> And generally speaking, the reset needs to be asserted before the
> supplies are up, so that the IC doesn't start doing any work until
> the SW decides it needs to.

Again, the problem is that you are *deasserting* reset before enabling
the supplies.

Johan