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: <ZximeTNi7huc95te@linaro.org>
Date: Wed, 23 Oct 2024 10:32:09 +0300
From: Abel Vesa <abel.vesa@...aro.org>
To: Johan Hovold <johan@...nel.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 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.

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

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.

> 
> > > > +	ret = clk_prepare_enable(retimer->xo_clk);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > > > +		goto err_retimer_unregister;
> > > > +	}
> > > 
> > > Should you really enable the clock before the regulators?
> > 
> > So maybe in this case it might not really matter. But in principle,
> > the HW might be affected by clock glitches and such when IP block
> > is powered up but unclocked. Even more so if the clock enabling
> > (prepare, to be more exact) involves switching to a new PLL.
> > 
> > So clock first, then power up. At least that's my understanding of HW
> > in general.
> 
> I think you got that backwards as inputs are typically rated for some
> maximum voltage based on the supply voltage. 

Yes, but that's done at board design stage.

> That applies also to the
> reset line as I also mentioned above.
> 
> What does the datasheet say?

As mentioned above, datasheet shows reset asserted before the supplies
are being enabled.

> 
> > > > +
> > > > +	ret = ps8830_enable_vregs(retimer);
> > > > +	if (ret)
> > > > +		goto err_clk_disable;
> 
> Johan