Re: [PATCH v3 2/2] reset: rzg2l-usbphy-ctrl: Add suspend/resume support

[Claudiu Beznea]

Hi, Philipp,

On 1/8/26 15:09, Philipp Zabel wrote:
> Hi Claudiu,
>
> On Do, 2026-01-08 at 13:44 +0200, Claudiu Beznea wrote:
> [...]
> > > > +	ret = reset_control_deassert(priv->rstc);
> > > > +	if (ret)
> > > > +		goto pwrrdy_off;
> > >
> > > Do I understand correctly that this reset clears PHY_RESET_PORT[12]
> > > bits in the RESET register such that rzg2l_usbphy_ctrl_init() must be
> > > called below?
> >
> > No, this reset is the reset of this HW block, controlled by another HW
> > block (the clock controller).
> >
> > Bits in PHY_RESET_PORT and other registers specific to this driver could
> > be cleared due to the fact the power to this USB PHY CTRL HW block is
> > turned off in suspend.
> >
> > The Renesas RZ/G3S SoC, that uses this HW block, has a power saving mode
> > where power to most of the SoC components, including USB PHY CTRL, is
> > turned off.
> >
> > Due to this, we need to restore the previous settings. priv->rstc need
> > to also be restored as power to the clock controller is also lost.
>
> Ok, thank you for the explanation.
>
> > > > +	ret = pm_runtime_resume_and_get(dev);
> > > > +	if (ret)
> > > > +		goto reset_assert;
> > > > +
> > > > +	rzg2l_usbphy_ctrl_init(priv);
> > >
> > > This assumes that consumers requested PHY_RESET_PORT[12] resets to be
> > > asserted in their suspend function.
> >
> > That's right!
> >
> > > I think you should warn if that is
> > > not the case during suspend.
> >
> > AFAICT, that could be done by adding extra logic in this driver to store
> > the state of the de-asserted bits. We can't interrogate directly the
> > registers as there might be the case where these resets are used by
> > previous bootloaders (that might let them in the de-assert state) but
> > not by Linux.
>
> Isn't the RESET register initialized during rzg2l_usbphy_ctrl_probe()
> before rzg2l_usbphy_ctrl_suspend() can ever be called? It seems to me
> that read-back in suspend should never return a value left behind by a
> previous bootloader.

Ah, you're right, I overlooked that.

> [...]
> > I can add those extra software cache for the hw registers but this is
> > what I've tried to avoid.
> >
> > Please let me know how do you want me to proceed and I'll update.
>
> If reading back the RESET register during suspend can be used, please
> add a warning.

OK, I'll update it like this.

Thank you,
Claudiu