Message-ID: <2996270f-d03e-45f7-9b6b-c5675c39515b@gmail.com>
Date: Fri, 2 May 2025 11:20:32 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Esben Haabendal <esben@...nix.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

On 02/05/2025 09:46, Esben Haabendal wrote:
> "Matti Vaittinen" <mazziesaccount@...il.com> 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

Powered by blists - more mailing lists