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: <Zxif6vmh8BE_C-_n@hovoldconsulting.com>
Date: Wed, 23 Oct 2024 09:04:10 +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 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).

That is, in this case, you should not deassert reset before making sure
the supplies are enabled.

> > > +	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. That applies also to the
reset line as I also mentioned above.

What does the datasheet say?

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

Johan