Re: [PATCH] regulator: bd718x7: Ensure SNVS power state is used as requested

[Matti Vaittinen]

On 02/05/2025 09:46, Esben Haabendal wrote:
> "Matti Vaittinen" <mazziesaccount@xxxxxxxxx> writes:
> > On 01/05/2025 17:48, Esben Haabendal wrote:
> > > With the introduction of the rohm,reset-snvs-powered DT binding [2], the
> > > PMIC settings were only changed when the new property was not found.
> > >
> > > As mentioned in [1] the default for BD71387 and BD71847 is to switch to
> > > SNVS power state on watchdog reset.
> >
> > I suppose you mean READY, not SNVS? Commit seems to state:
> > "By default only wathcdog reset changes state from poweroff to ready."
>
> You are absolutely right. Sorry about that.
>
> > > So even with rohm,reset-snvs-powered added to DT, a watchdog reset causes
> > > transitions through READY instead of SNVS.
> >
> > The original idea of the rohm,reset-snvs-powered was not to configure
> > the SNVS to be the target.
>
> Makes sense.
>
> If we keep it that way, then I think we should change the description of
> the binding. "Transfer PMIC to SNVS state at reset" tricked at least me
> into believing it would actually make the kernel setup the PMIC to
> go to SNVS state at reset.
>
> Maybe someething like:
>
>      PMIC is configured to go to SNVS state on reset. Bootloader or
>      something else is responsible for configuring the PMIC to do this.
>      The driver will not change this configuration when this property is
>      present.

I am not objecting this. And adding your suggestion at the end of the 
mail should make things, well, not pretty but working.

> I guess back in 2019 when you introduced the rohm,reset-snvs-powered
> binding you had to keep the code for writing to TRANS_COND1 in the
> default case for backwards compatibility. But in hindsight, I think the
> asymetry caused by not doing the same when rohm,reset-snvs-powered is
> used is what caught me off guard.

Yes, I agree. It should have changed the reset target from the day 1, 
but as I didn't do it right back then ... We now may very well have it 
somewhere it shouldn't be, and changing this is somewhat risky.

> But that is water under the bridge...
>
> > The driver was mostly built to assume that the PMIC has been
> > configured by earlier stages like uboot, and configs in the driver
> > were mostly introduced to make power rail enable states controllable
> > by the software - without risking the rails to be left off. Thus,
> > AFAIR, the values set by boot (or other power manager MCUs) haven't
> > been overwritten is the "rohm,reset-snvs-powered" has been found.
>
> Got it.
>
> > Configuring for example the hardware watchdog related stuff at Linux
> > driver boot is somewhat late, since watchdog should probably be running
> > already - and hangs might happen prior the driver probe.
>
> Yes. But this specific configuration is not too late to do at driver
> probe time, although it is better to do it as early as possible.
>
> > > And with the default reboot
> > > method in mxc_restart() is to cause a watchdog reset, we ended up powering
> > > off the SNVS domains, and thus losing SNVS state such as SNVS RTC and
> > > LPGPR, on reboots.
> > >
> > > With this change, the rohm,reset-snvs-powered property results in the PMIC
> > > configuration being modified so POWEROFF transitions to SNVS for all reset
> > > types, including watchdog reset.
> >
> > As far as I can say, this change is, in principle, fine. The
> > "rohm,reset-snvs-powered" shouldn't be populated in the device-tree, if
> > SNVS is not meant to be used. My only worry is that the BD71837, 47 and
> > 50 have been on the field since 2018 - and I am not at all sure all the
> > device-trees are sane...
>
> Yes, there is no way to know that fore sure. Even verifying the sanity
> of the in-tree device-trees will require quite some work.
>
> > And if we configure the reset to use SNVS state, then the software
> > controlled regulators will not turn ON after the reset. Fail to mark
> > them in the device-tree and the device will be dead until battery is
> > drained or removed.
> >
> > Is there a way for you to set the "target state" at boot SW?
>
> As of right now, not really. I am currently stuck with the existing
> bootloader. I will replace it sometime later, and at that time, I can
> make it configure the PMIC properly.
>
> > I think that should work as the Linux driver won't touch the target
> > state if rohm,reset-snvs-powered is set(?)
>
> That should work, yes.
>
> > This is not NACK to the change, this is asking if we had a safer way,
> > both for other users and also for you (since I still think these configs
> > should be done prior Linux driver probe)...
>
> We could create another device-tree binding to make the driver override
> PMIC configuration to use SNVS state on reset. But, in order to maintain
> backwards compatibility with the rohm,reset-snvs-powered, I don't know
> what to call it without adding more confusion. Maybe something like
> rohm,force-reset-snvs-powered?

This should keep the existing users happy while also supporting your use 
case. Together with fix to the description of the 
rohm,reset-snvs-powered (as you suggest above), this should work even if 
it's not really pretty. I am not sure how the DT folks see this though.

Another option is to change the rohm,reset-snvs-powered to have a value. 
Something like:
rohm,reset-snvs-powered = "default"; or
rohm,reset-snvs-powered = "forced";

> But although I found the bidning confusing at first, and currently is
> not able to configure the PMIC before starting Linux, I agree that
> it is better to have bootloader or something else handle PMIC
> configuration, so it is setup as early as possible.

Probably yes.

Yours,
	-- Matti