Message-ID: <87tt638q7t.fsf@geanix.com>
Date: Fri, 02 May 2025 08:46:30 +0200
From: Esben Haabendal <esben@...nix.com>
To: "Matti Vaittinen" <mazziesaccount@...il.com>
Cc: "Liam Girdwood" <lgirdwood@...il.com>,  "Mark Brown"
 <broonie@...nel.org>,  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: bd718x7: Ensure SNVS power state is used as
 requested

"Matti Vaittinen" <mazziesaccount@...il.com> writes:

> Hi Esben,
>
> Oh, it has been a while since I've heard anything from these PMICs :)

Still alive and kicking :)

> 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 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.
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?

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.

/Esben

Powered by blists - more mailing lists