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

[Johan Hovold <johan@kernel.org>]

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