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
