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

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

[Esben Haabendal]

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.

So even with rohm,reset-snvs-powered added to DT, a watchdog reset causes
transitions through READY instead of SNVS. 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.

[1] commit e770b18bbbae ("regulator: bd718x7: Change next state after poweroff to ready")
[2] commit 049369d46428 ("regulator: bd718x7: Support SNVS low power state")

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/regulator/bd718x7-regulator.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index 1bb048de3ecd5a8df1087a48afc728a64623a024..d8c7fb2e73986a39066c9e8a114dd8d733bc8a33 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -1715,23 +1715,24 @@ static int bd718xx_probe(struct platform_device *pdev)
 					 "rohm,reset-snvs-powered");
 
 	/*
-	 * Change the next stage from poweroff to be READY instead of SNVS
-	 * for all reset types because OTP loading at READY will clear SEL
-	 * bit allowing HW defaults for power rails to be used
+	 * Set next power state from poweroff to be either READY or SNVS for all
+	 * reset types. The default is READY state because OTP loading at READY
+	 * will clear SEL bit allowing HW defaults for power rails to be used.
+	 * Using SNVS power state instead allows SNVS state, such as RTC and
+	 * LPGPR to be persisted over reboots.
 	 */
-	if (!use_snvs) {
-		err = regmap_update_bits(regmap, BD718XX_REG_TRANS_COND1,
-					 BD718XX_ON_REQ_POWEROFF_MASK |
-					 BD718XX_SWRESET_POWEROFF_MASK |
-					 BD718XX_WDOG_POWEROFF_MASK |
-					 BD718XX_KEY_L_POWEROFF_MASK,
-					 BD718XX_POWOFF_TO_RDY);
-		if (err)
-			return dev_err_probe(&pdev->dev, err,
-					     "Failed to change reset target\n");
-
-		dev_dbg(&pdev->dev, "Changed all resets from SVNS to READY\n");
-	}
+	err = regmap_update_bits(regmap, BD718XX_REG_TRANS_COND1,
+				 BD718XX_ON_REQ_POWEROFF_MASK |
+				 BD718XX_SWRESET_POWEROFF_MASK |
+				 BD718XX_WDOG_POWEROFF_MASK |
+				 BD718XX_KEY_L_POWEROFF_MASK,
+				 use_snvs ? 0 : BD718XX_POWOFF_TO_RDY);
+	if (err)
+		return dev_err_probe(&pdev->dev, err,
+				     "Failed to change reset target\n");
+
+	dev_dbg(&pdev->dev, "Changed all resets from %s to %s\n",
+		use_snvs ? "READY" : "SNVS", use_snvs ? "SNVS" : "READY");
 
 	config.dev = pdev->dev.parent;
 	config.regmap = regmap;

---
base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
change-id: 20250501-bd718x7-snvs-reset-186d934b37c1

Best regards,
-- 
Esben Haabendal <esben@geanix.com>

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

[Matti Vaittinen]

Hi Esben,

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

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

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

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.

> 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... 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? I think 
that should work as the Linux driver won't touch the target state if 
rohm,reset-snvs-powered is set(?)

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

Yours,
	-- Matti

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

[Esben Haabendal]

"Matti Vaittinen" <mazziesaccount@gmail.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

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@gmail.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

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

[Esben Haabendal]

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

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

Exactly.

>> 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";

I suspect that device-tree maintainers will not accept changing an
existing binding to a different type, as it would break existing
device-trees. In this case changing an already defined boolean property
to an enum type. But I had the same idea :)

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

I will start working on bootloader support for these devices next week,
so for what I am concerned, I can live with doing something like
proposed here as a temporary out-of-tree solution.

What do you think, should we leave it in the current state, or can we
figure out a way to allow the driver to actively change the watchdog
reset to go to SNVS power state?

/Esben

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

[Matti Vaittinen]

On 03/05/2025 12:23, Esben Haabendal wrote:
> 
> What do you think, should we leave it in the current state, or can we
> figure out a way to allow the driver to actively change the watchdog
> reset to go to SNVS power state?

I can't really think of a safe way other than adding a new property.

The IC is oldish, and AFACS, quite widely used. People seem to have 
found a way to make it work without (in-tree) feature for changing the 
reset target to the SNVS. I wouldn't try to implement this unless we 
have a use-case really requiring it. So, I'd suggest you to do it in 
boot. Still, if you have a valid case for this to be done in the driver 
- then I have no reason to object adding a new binding + support in the 
driver.

Yours,
	-- Matti

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

[Esben Haabendal]

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

> On 03/05/2025 12:23, Esben Haabendal wrote:
>>
>> What do you think, should we leave it in the current state, or can we
>> figure out a way to allow the driver to actively change the watchdog
>> reset to go to SNVS power state?
>
> I can't really think of a safe way other than adding a new property.
>
> The IC is oldish, and AFACS, quite widely used. People seem to have
> found a way to make it work without (in-tree) feature for changing the
> reset target to the SNVS. I wouldn't try to implement this unless we
> have a use-case really requiring it. So, I'd suggest you to do it in
> boot. Still, if you have a valid case for this to be done in the driver
> - then I have no reason to object adding a new binding + support in the
> driver.

I have now implemted the PMIC setup in U-Boot, so am fine with dropping
the patch for now.

For anyone else finding this thread later on, the advice is to setup the
PMIC in bootloader or by some other low-level method. But if that for
some reason is not feasible, you can continue the work / discussion we
have had here in order to add a new binding + support in the driver for
it.

/Esben