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
