[PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Don't set the RK817_POWER_EN_SAVE0 or RK817_POWER_EN_SAVE1 bits. These
bits are not used by mainline Linux and actively setting them in U-boot
interferes with their function for BSP kernels, causing systems to
hang when regulators are enabled or disabled incorrectly. [1]

[1] https://gist.github.com/fr0z3n-cs/047b24a6fd430e816fe02a877ad7ab8c

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/power/pmic/rk8xx.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
index 95b71d2fe49..16b3dc7cb1c 100644
--- a/drivers/power/pmic/rk8xx.c
+++ b/drivers/power/pmic/rk8xx.c
@@ -251,8 +251,6 @@ static int rk8xx_probe(struct udevice *dev)
 	int ret = 0, i, show_variant;
 	u8 msb, lsb, id_msb, id_lsb;
 	u8 on_source = 0, off_source = 0;
-	u8 power_en0, power_en1, power_en2, power_en3;
-	u8 value;
 
 	/* read Chip variant */
 	if (device_is_compatible(dev, "rockchip,rk817") ||
@@ -293,15 +291,6 @@ static int rk8xx_probe(struct udevice *dev)
 		off_source = RK817_OFF_SOURCE;
 		init_data = rk817_init_reg;
 		init_data_num = ARRAY_SIZE(rk817_init_reg);
-		power_en0 = pmic_reg_read(dev, RK817_POWER_EN0);
-		power_en1 = pmic_reg_read(dev, RK817_POWER_EN1);
-		power_en2 = pmic_reg_read(dev, RK817_POWER_EN2);
-		power_en3 = pmic_reg_read(dev, RK817_POWER_EN3);
-
-		value = (power_en0 & 0x0f) | ((power_en1 & 0x0f) << 4);
-		pmic_reg_write(dev, RK817_POWER_EN_SAVE0, value);
-		value = (power_en2 & 0x0f) | ((power_en3 & 0x0f) << 4);
-		pmic_reg_write(dev, RK817_POWER_EN_SAVE1, value);
 		break;
 	case RK806_ID: {
 		u32 rst_fun = 2;
-- 
2.53.0

Re: [PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[Quentin Schulz]

Hi Chris,

On 11/5/26 4:10 PM, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Don't set the RK817_POWER_EN_SAVE0 or RK817_POWER_EN_SAVE1 bits. These
> bits are not used by mainline Linux and actively setting them in U-boot
> interferes with their function for BSP kernels, causing systems to
> hang when regulators are enabled or disabled incorrectly. [1]
> 

More information in the commit log would be welcome, in the event the 
gist ever disappears.

In essence, those two registers are used to store the current state of 
the regulators (why? don't know and not explained in the gist, do you 
have some info maybe Chris? Anbernic seems to be ignoring it anyway but 
we don't know if there are actual users of it). They aren't meant for 
that at all, but that's what they are used for. Downstream kernels then 
read those registers and configure the regulators at boot based on the 
saved values. In upstream U-Boot, these registers are set at PMIC driver 
probe time, which doesn't make a lot of sense since no regulators would 
have been configured by then (although, they may have if the PMIC driver 
was probed and used in xPL and U-Boot proper then save the state as 
configured by xPL).

I'm a bit worried other downstream kernels may actually be expecting 
these registers to be set. But considering we probably are 
misconfiguring the save registers anyway (by saving too early), I guess 
that's fine?

Please expand the commit log to contain summarized information from the 
gist, and that would be an:

Acked-by: Quentin Schulz <quentin.schulz@cherry.de>

Considering this has been the case forever, I'm tempted to get this into 
2027.04 rather than 2027.01 as it's not a new regression. That would 
leave enough time for people to complain about it (though I'm not sure 
may people are actually testing upstream -rc on Rockchip?). At the same 
time, we're still relatively early in the 2027.01 rcs so maybe that's 
acceptable too.

Thanks!
Quentin

> [1] https://gist.github.com/fr0z3n-cs/047b24a6fd430e816fe02a877ad7ab8c
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>   drivers/power/pmic/rk8xx.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> index 95b71d2fe49..16b3dc7cb1c 100644
> --- a/drivers/power/pmic/rk8xx.c
> +++ b/drivers/power/pmic/rk8xx.c
> @@ -251,8 +251,6 @@ static int rk8xx_probe(struct udevice *dev)
>   	int ret = 0, i, show_variant;
>   	u8 msb, lsb, id_msb, id_lsb;
>   	u8 on_source = 0, off_source = 0;
> -	u8 power_en0, power_en1, power_en2, power_en3;
> -	u8 value;
>   
>   	/* read Chip variant */
>   	if (device_is_compatible(dev, "rockchip,rk817") ||
> @@ -293,15 +291,6 @@ static int rk8xx_probe(struct udevice *dev)
>   		off_source = RK817_OFF_SOURCE;
>   		init_data = rk817_init_reg;
>   		init_data_num = ARRAY_SIZE(rk817_init_reg);
> -		power_en0 = pmic_reg_read(dev, RK817_POWER_EN0);
> -		power_en1 = pmic_reg_read(dev, RK817_POWER_EN1);
> -		power_en2 = pmic_reg_read(dev, RK817_POWER_EN2);
> -		power_en3 = pmic_reg_read(dev, RK817_POWER_EN3);
> -
> -		value = (power_en0 & 0x0f) | ((power_en1 & 0x0f) << 4);
> -		pmic_reg_write(dev, RK817_POWER_EN_SAVE0, value);
> -		value = (power_en2 & 0x0f) | ((power_en3 & 0x0f) << 4);
> -		pmic_reg_write(dev, RK817_POWER_EN_SAVE1, value);
>   		break;
>   	case RK806_ID: {
>   		u32 rst_fun = 2;

Re: [PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[Chris Morgan]

On Thu, Nov 05, 2026 at 05:13:48PM +0100, Quentin Schulz wrote:
> Hi Chris,
> 
> On 11/5/26 4:10 PM, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Don't set the RK817_POWER_EN_SAVE0 or RK817_POWER_EN_SAVE1 bits. These
> > bits are not used by mainline Linux and actively setting them in U-boot
> > interferes with their function for BSP kernels, causing systems to
> > hang when regulators are enabled or disabled incorrectly. [1]
> > 
> 
> More information in the commit log would be welcome, in the event the gist
> ever disappears.
> 
> In essence, those two registers are used to store the current state of the
> regulators (why? don't know and not explained in the gist, do you have some
> info maybe Chris? Anbernic seems to be ignoring it anyway but we don't know
> if there are actual users of it). They aren't meant for that at all, but
> that's what they are used for. Downstream kernels then read those registers
> and configure the regulators at boot based on the saved values. In upstream
> U-Boot, these registers are set at PMIC driver probe time, which doesn't
> make a lot of sense since no regulators would have been configured by then
> (although, they may have if the PMIC driver was probed and used in xPL and
> U-Boot proper then save the state as configured by xPL).
> 
> I'm a bit worried other downstream kernels may actually be expecting these
> registers to be set. But considering we probably are misconfiguring the save
> registers anyway (by saving too early), I guess that's fine?
> 
> Please expand the commit log to contain summarized information from the
> gist, and that would be an:
> 
> Acked-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Considering this has been the case forever, I'm tempted to get this into
> 2027.04 rather than 2027.01 as it's not a new regression. That would leave
> enough time for people to complain about it (though I'm not sure may people
> are actually testing upstream -rc on Rockchip?). At the same time, we're
> still relatively early in the 2027.01 rcs so maybe that's acceptable too.
> 
> Thanks!
> Quentin
>

I'll resubmit tomorrow with more information in the commit message, as
requested. I'll save the ack so you can confirm if you think it's
good.

I don't know that we have a good answer as to why those regulators are
stored. The BSP U-Boot driver (assuming it wasn't modified by Anbernic)
would check the device-tree for a "not-save-power-en" parameter and if
it was not set at 1 it would save the contents of registers
RK817_POWER_EN0 through RK817_POWER_EN3 and then save the contents to
RK817_POWER_EN_SAVE0 (0x99) and RK817_POWER_EN_SAVE1 (0xa4). These
values would be saved at probe time which would be very early in the
boot process. The BSP kernel driver would then read the
RK817_POWER_EN_SAVE0 register and if it was not 0 it would then read
RK817_POWER_EN_SAVE1 and write all of those saved values back to the
PMIC as part of the reboot-notifier.

So basically it looks like as part of the reboot/shutdown process for
BSP kernel and bootloader, the initial values for what is on/off is
getting written back to the regulators unless not-save-power-en is
set in U-Boot. This process breaks down for boards where the BSP
bootloader sets not-save-power-en.

Since mainline Linux doesn't use these registers at all and there is
no parameter to control their use in mainline U-Boot like there is in
BSP U-Boot, we should just drop them so they don't interfere with the
BSP kernel.

Thank you,
Chris
 
> > [1] https://gist.github.com/fr0z3n-cs/047b24a6fd430e816fe02a877ad7ab8c
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> >   drivers/power/pmic/rk8xx.c | 11 -----------
> >   1 file changed, 11 deletions(-)
> > 
> > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > index 95b71d2fe49..16b3dc7cb1c 100644
> > --- a/drivers/power/pmic/rk8xx.c
> > +++ b/drivers/power/pmic/rk8xx.c
> > @@ -251,8 +251,6 @@ static int rk8xx_probe(struct udevice *dev)
> >   	int ret = 0, i, show_variant;
> >   	u8 msb, lsb, id_msb, id_lsb;
> >   	u8 on_source = 0, off_source = 0;
> > -	u8 power_en0, power_en1, power_en2, power_en3;
> > -	u8 value;
> >   	/* read Chip variant */
> >   	if (device_is_compatible(dev, "rockchip,rk817") ||
> > @@ -293,15 +291,6 @@ static int rk8xx_probe(struct udevice *dev)
> >   		off_source = RK817_OFF_SOURCE;
> >   		init_data = rk817_init_reg;
> >   		init_data_num = ARRAY_SIZE(rk817_init_reg);
> > -		power_en0 = pmic_reg_read(dev, RK817_POWER_EN0);
> > -		power_en1 = pmic_reg_read(dev, RK817_POWER_EN1);
> > -		power_en2 = pmic_reg_read(dev, RK817_POWER_EN2);
> > -		power_en3 = pmic_reg_read(dev, RK817_POWER_EN3);
> > -
> > -		value = (power_en0 & 0x0f) | ((power_en1 & 0x0f) << 4);
> > -		pmic_reg_write(dev, RK817_POWER_EN_SAVE0, value);
> > -		value = (power_en2 & 0x0f) | ((power_en3 & 0x0f) << 4);
> > -		pmic_reg_write(dev, RK817_POWER_EN_SAVE1, value);
> >   		break;
> >   	case RK806_ID: {
> >   		u32 rst_fun = 2;
> 

Re: [PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[Joseph Chen]

Hi Quentin, Chris:

在 2026/11/6 1:32, Chris Morgan 写道:
> On Thu, Nov 05, 2026 at 05:13:48PM +0100, Quentin Schulz wrote:
>> Hi Chris,
>>
>> On 11/5/26 4:10 PM, Chris Morgan wrote:
>>> From: Chris Morgan<macromorgan@hotmail.com>
>>>
>>> Don't set the RK817_POWER_EN_SAVE0 or RK817_POWER_EN_SAVE1 bits. These
>>> bits are not used by mainline Linux and actively setting them in U-boot
>>> interferes with their function for BSP kernels, causing systems to
>>> hang when regulators are enabled or disabled incorrectly. [1]
>>>
>> More information in the commit log would be welcome, in the event the gist
>> ever disappears.
>>
>> In essence, those two registers are used to store the current state of the
>> regulators (why? don't know and not explained in the gist, do you have some
>> info maybe Chris? Anbernic seems to be ignoring it anyway but we don't know
>> if there are actual users of it). They aren't meant for that at all, but
>> that's what they are used for. Downstream kernels then read those registers
>> and configure the regulators at boot based on the saved values. In upstream
>> U-Boot, these registers are set at PMIC driver probe time, which doesn't
>> make a lot of sense since no regulators would have been configured by then
>> (although, they may have if the PMIC driver was probed and used in xPL and
>> U-Boot proper then save the state as configured by xPL).
>>
>> I'm a bit worried other downstream kernels may actually be expecting these
>> registers to be set. But considering we probably are misconfiguring the save
>> registers anyway (by saving too early), I guess that's fine?
>>
>> Please expand the commit log to contain summarized information from the
>> gist, and that would be an:
>>
>> Acked-by: Quentin Schulz<quentin.schulz@cherry.de>
>>
>> Considering this has been the case forever, I'm tempted to get this into
>> 2027.04 rather than 2027.01 as it's not a new regression. That would leave
>> enough time for people to complain about it (though I'm not sure may people
>> are actually testing upstream -rc on Rockchip?). At the same time, we're
>> still relatively early in the 2027.01 rcs so maybe that's acceptable too.
>>
>> Thanks!
>> Quentin
>>
> I'll resubmit tomorrow with more information in the commit message, as
> requested. I'll save the ack so you can confirm if you think it's
> good.
>
> I don't know that we have a good answer as to why those regulators are
> stored. The BSP U-Boot driver (assuming it wasn't modified by Anbernic)
> would check the device-tree for a "not-save-power-en" parameter and if
> it was not set at 1 it would save the contents of registers
> RK817_POWER_EN0 through RK817_POWER_EN3 and then save the contents to
> RK817_POWER_EN_SAVE0 (0x99) and RK817_POWER_EN_SAVE1 (0xa4). These
> values would be saved at probe time which would be very early in the
> boot process. The BSP kernel driver would then read the
> RK817_POWER_EN_SAVE0 register and if it was not 0 it would then read
> RK817_POWER_EN_SAVE1 and write all of those saved values back to the
> PMIC as part of the reboot-notifier.
>
> So basically it looks like as part of the reboot/shutdown process for
> BSP kernel and bootloader, the initial values for what is on/off is
> getting written back to the regulators unless not-save-power-en is
> set in U-Boot. This process breaks down for boards where the BSP
> bootloader sets not-save-power-en.
>
> Since mainline Linux doesn't use these registers at all and there is
> no parameter to control their use in mainline U-Boot like there is in
> BSP U-Boot, we should just drop them so they don't interfere with the
> BSP kernel.
>
> Thank you,
> Chris
>   

The purpose for POWER_EN_SAVE0/1 is that:

During a PMIC reset, voltage settings are restored before enable (EN) 
settings. For example:
a regulator that is disabled by the PMIC hardware at power-on, with a 
default voltage of 1.8V,
may be enabled by the user and configured to output 1.0V during operation.
When a reboot triggers a PMIC reset, the regulator may briefly output 
1.8V before being disabled,
posing an overvoltage risk.

To prevent this, U-Boot saves the initial power-on enable states in 
POWER_EN_SAVE0/1.
The kernel restores these states in the reboot callback, disabling such 
regulators
before the PMIC reset to avoid unintended voltage output.


>>> [1]https://gist.github.com/fr0z3n-cs/047b24a6fd430e816fe02a877ad7ab8c
>>>
>>> Signed-off-by: Chris Morgan<macromorgan@hotmail.com>
>>> ---
>>>    drivers/power/pmic/rk8xx.c | 11 -----------
>>>    1 file changed, 11 deletions(-)
>>>
>>> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
>>> index 95b71d2fe49..16b3dc7cb1c 100644
>>> --- a/drivers/power/pmic/rk8xx.c
>>> +++ b/drivers/power/pmic/rk8xx.c
>>> @@ -251,8 +251,6 @@ static int rk8xx_probe(struct udevice *dev)
>>>    	int ret = 0, i, show_variant;
>>>    	u8 msb, lsb, id_msb, id_lsb;
>>>    	u8 on_source = 0, off_source = 0;
>>> -	u8 power_en0, power_en1, power_en2, power_en3;
>>> -	u8 value;
>>>    	/* read Chip variant */
>>>    	if (device_is_compatible(dev, "rockchip,rk817") ||
>>> @@ -293,15 +291,6 @@ static int rk8xx_probe(struct udevice *dev)
>>>    		off_source = RK817_OFF_SOURCE;
>>>    		init_data = rk817_init_reg;
>>>    		init_data_num = ARRAY_SIZE(rk817_init_reg);
>>> -		power_en0 = pmic_reg_read(dev, RK817_POWER_EN0);
>>> -		power_en1 = pmic_reg_read(dev, RK817_POWER_EN1);
>>> -		power_en2 = pmic_reg_read(dev, RK817_POWER_EN2);
>>> -		power_en3 = pmic_reg_read(dev, RK817_POWER_EN3);
>>> -
>>> -		value = (power_en0 & 0x0f) | ((power_en1 & 0x0f) << 4);
>>> -		pmic_reg_write(dev, RK817_POWER_EN_SAVE0, value);
>>> -		value = (power_en2 & 0x0f) | ((power_en3 & 0x0f) << 4);
>>> -		pmic_reg_write(dev, RK817_POWER_EN_SAVE1, value);
>>>    		break;
>>>    	case RK806_ID: {
>>>    		u32 rst_fun = 2;

-- 
Best regards,

陈健洪 (Joseph Chen)
chenjh@rock-chips.com
福州瑞芯微电子股份有限公司
Fuzhou Rockchip Electronics Co.Ltd
福建省福州市铜盘路软件大道89号软件园A区21号楼 (350003)
No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC
TEL：0591-83991906/07-8573

Re: [PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[Chris Morgan]

On Fri, Nov 06, 2026 at 04:57:55PM +0800, Joseph Chen wrote:
> Hi Quentin, Chris:
> 
> 在 2026/11/6 1:32, Chris Morgan 写道:
> > On Thu, Nov 05, 2026 at 05:13:48PM +0100, Quentin Schulz wrote:
> > > Hi Chris,
> > > 
> > > On 11/5/26 4:10 PM, Chris Morgan wrote:
> > > > From: Chris Morgan<macromorgan@hotmail.com>
> > > > 
> > > > Don't set the RK817_POWER_EN_SAVE0 or RK817_POWER_EN_SAVE1 bits. These
> > > > bits are not used by mainline Linux and actively setting them in U-boot
> > > > interferes with their function for BSP kernels, causing systems to
> > > > hang when regulators are enabled or disabled incorrectly. [1]
> > > > 
> > > More information in the commit log would be welcome, in the event the gist
> > > ever disappears.
> > > 
> > > In essence, those two registers are used to store the current state of the
> > > regulators (why? don't know and not explained in the gist, do you have some
> > > info maybe Chris? Anbernic seems to be ignoring it anyway but we don't know
> > > if there are actual users of it). They aren't meant for that at all, but
> > > that's what they are used for. Downstream kernels then read those registers
> > > and configure the regulators at boot based on the saved values. In upstream
> > > U-Boot, these registers are set at PMIC driver probe time, which doesn't
> > > make a lot of sense since no regulators would have been configured by then
> > > (although, they may have if the PMIC driver was probed and used in xPL and
> > > U-Boot proper then save the state as configured by xPL).
> > > 
> > > I'm a bit worried other downstream kernels may actually be expecting these
> > > registers to be set. But considering we probably are misconfiguring the save
> > > registers anyway (by saving too early), I guess that's fine?
> > > 
> > > Please expand the commit log to contain summarized information from the
> > > gist, and that would be an:
> > > 
> > > Acked-by: Quentin Schulz<quentin.schulz@cherry.de>
> > > 
> > > Considering this has been the case forever, I'm tempted to get this into
> > > 2027.04 rather than 2027.01 as it's not a new regression. That would leave
> > > enough time for people to complain about it (though I'm not sure may people
> > > are actually testing upstream -rc on Rockchip?). At the same time, we're
> > > still relatively early in the 2027.01 rcs so maybe that's acceptable too.
> > > 
> > > Thanks!
> > > Quentin
> > > 
> > I'll resubmit tomorrow with more information in the commit message, as
> > requested. I'll save the ack so you can confirm if you think it's
> > good.
> > 
> > I don't know that we have a good answer as to why those regulators are
> > stored. The BSP U-Boot driver (assuming it wasn't modified by Anbernic)
> > would check the device-tree for a "not-save-power-en" parameter and if
> > it was not set at 1 it would save the contents of registers
> > RK817_POWER_EN0 through RK817_POWER_EN3 and then save the contents to
> > RK817_POWER_EN_SAVE0 (0x99) and RK817_POWER_EN_SAVE1 (0xa4). These
> > values would be saved at probe time which would be very early in the
> > boot process. The BSP kernel driver would then read the
> > RK817_POWER_EN_SAVE0 register and if it was not 0 it would then read
> > RK817_POWER_EN_SAVE1 and write all of those saved values back to the
> > PMIC as part of the reboot-notifier.
> > 
> > So basically it looks like as part of the reboot/shutdown process for
> > BSP kernel and bootloader, the initial values for what is on/off is
> > getting written back to the regulators unless not-save-power-en is
> > set in U-Boot. This process breaks down for boards where the BSP
> > bootloader sets not-save-power-en.
> > 
> > Since mainline Linux doesn't use these registers at all and there is
> > no parameter to control their use in mainline U-Boot like there is in
> > BSP U-Boot, we should just drop them so they don't interfere with the
> > BSP kernel.
> > 
> > Thank you,
> > Chris
> 
> The purpose for POWER_EN_SAVE0/1 is that:
> 
> During a PMIC reset, voltage settings are restored before enable (EN)
> settings. For example:
> a regulator that is disabled by the PMIC hardware at power-on, with a
> default voltage of 1.8V,
> may be enabled by the user and configured to output 1.0V during operation.
> When a reboot triggers a PMIC reset, the regulator may briefly output 1.8V
> before being disabled,
> posing an overvoltage risk.
> 
> To prevent this, U-Boot saves the initial power-on enable states in
> POWER_EN_SAVE0/1.
> The kernel restores these states in the reboot callback, disabling such
> regulators
> before the PMIC reset to avoid unintended voltage output.
> 

This behavior is not reflected in mainline Linux, should it be?
If it should, it seems to me the proper fix would be:

1) Add the functionality to set the regulators in Linux, like it is
done in the BSP, as part of the reset handler.

2) Gate both the Linux functionality and U-Boot functionality behind
a device-tree option. Unlike BSP, make it opt-in rather than opt-out
(to preserve backwards compatibility with the existing mainline
kernel).

Let me know what you think and I can start this process.

Thank you,
Chris

> 
> > > > [1]https://gist.github.com/fr0z3n-cs/047b24a6fd430e816fe02a877ad7ab8c
> > > > 
> > > > Signed-off-by: Chris Morgan<macromorgan@hotmail.com>
> > > > ---
> > > >    drivers/power/pmic/rk8xx.c | 11 -----------
> > > >    1 file changed, 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > > > index 95b71d2fe49..16b3dc7cb1c 100644
> > > > --- a/drivers/power/pmic/rk8xx.c
> > > > +++ b/drivers/power/pmic/rk8xx.c
> > > > @@ -251,8 +251,6 @@ static int rk8xx_probe(struct udevice *dev)
> > > >    	int ret = 0, i, show_variant;
> > > >    	u8 msb, lsb, id_msb, id_lsb;
> > > >    	u8 on_source = 0, off_source = 0;
> > > > -	u8 power_en0, power_en1, power_en2, power_en3;
> > > > -	u8 value;
> > > >    	/* read Chip variant */
> > > >    	if (device_is_compatible(dev, "rockchip,rk817") ||
> > > > @@ -293,15 +291,6 @@ static int rk8xx_probe(struct udevice *dev)
> > > >    		off_source = RK817_OFF_SOURCE;
> > > >    		init_data = rk817_init_reg;
> > > >    		init_data_num = ARRAY_SIZE(rk817_init_reg);
> > > > -		power_en0 = pmic_reg_read(dev, RK817_POWER_EN0);
> > > > -		power_en1 = pmic_reg_read(dev, RK817_POWER_EN1);
> > > > -		power_en2 = pmic_reg_read(dev, RK817_POWER_EN2);
> > > > -		power_en3 = pmic_reg_read(dev, RK817_POWER_EN3);
> > > > -
> > > > -		value = (power_en0 & 0x0f) | ((power_en1 & 0x0f) << 4);
> > > > -		pmic_reg_write(dev, RK817_POWER_EN_SAVE0, value);
> > > > -		value = (power_en2 & 0x0f) | ((power_en3 & 0x0f) << 4);
> > > > -		pmic_reg_write(dev, RK817_POWER_EN_SAVE1, value);
> > > >    		break;
> > > >    	case RK806_ID: {
> > > >    		u32 rst_fun = 2;
> 
> -- 
> Best regards,
> 
> 陈健洪 (Joseph Chen)
> chenjh@rock-chips.com
> 福州瑞芯微电子股份有限公司
> Fuzhou Rockchip Electronics Co.Ltd
> 福建省福州市铜盘路软件大道89号软件园A区21号楼 (350003)
> No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC
> TEL：0591-83991906/07-8573
> 

Re: [PATCH] power: pmic: rk8xx: Don't set RK817_POWER_EN_SAVE bits

[Joseph Chen]

Hi Chris,

在 2026-11-06 22:24, Chris Morgan 写道:
> On Fri, Nov 06, 2026 at 04:57:55PM +0800, Joseph Chen wrote:
>> Hi Quentin, Chris:
>>
>> 在 2026/11/6 1:32, Chris Morgan 写道:
>>> On Thu, Nov 05, 2026 at 05:13:48PM +0100, Quentin Schulz wrote:
>>>> Hi Chris,
>>>>
>>>> On 11/5/26 4:10 PM, Chris Morgan wrote:
>>>>> From: Chris Morgan<macromorgan@hotmail.com>
>>>>>
>>>>> Don't set the RK817_POWER_EN_SAVE0 or RK817_POWER_EN_SAVE1 bits. These
>>>>> bits are not used by mainline Linux and actively setting them in U-boot
>>>>> interferes with their function for BSP kernels, causing systems to
>>>>> hang when regulators are enabled or disabled incorrectly. [1]
>>>>>
>>>> More information in the commit log would be welcome, in the event the gist
>>>> ever disappears.
>>>>
>>>> In essence, those two registers are used to store the current state of the
>>>> regulators (why? don't know and not explained in the gist, do you have some
>>>> info maybe Chris? Anbernic seems to be ignoring it anyway but we don't know
>>>> if there are actual users of it). They aren't meant for that at all, but
>>>> that's what they are used for. Downstream kernels then read those registers
>>>> and configure the regulators at boot based on the saved values. In upstream
>>>> U-Boot, these registers are set at PMIC driver probe time, which doesn't
>>>> make a lot of sense since no regulators would have been configured by then
>>>> (although, they may have if the PMIC driver was probed and used in xPL and
>>>> U-Boot proper then save the state as configured by xPL).
>>>>
>>>> I'm a bit worried other downstream kernels may actually be expecting these
>>>> registers to be set. But considering we probably are misconfiguring the save
>>>> registers anyway (by saving too early), I guess that's fine?
>>>>
>>>> Please expand the commit log to contain summarized information from the
>>>> gist, and that would be an:
>>>>
>>>> Acked-by: Quentin Schulz<quentin.schulz@cherry.de>
>>>>
>>>> Considering this has been the case forever, I'm tempted to get this into
>>>> 2027.04 rather than 2027.01 as it's not a new regression. That would leave
>>>> enough time for people to complain about it (though I'm not sure may people
>>>> are actually testing upstream -rc on Rockchip?). At the same time, we're
>>>> still relatively early in the 2027.01 rcs so maybe that's acceptable too.
>>>>
>>>> Thanks!
>>>> Quentin
>>>>
>>> I'll resubmit tomorrow with more information in the commit message, as
>>> requested. I'll save the ack so you can confirm if you think it's
>>> good.
>>>
>>> I don't know that we have a good answer as to why those regulators are
>>> stored. The BSP U-Boot driver (assuming it wasn't modified by Anbernic)
>>> would check the device-tree for a "not-save-power-en" parameter and if
>>> it was not set at 1 it would save the contents of registers
>>> RK817_POWER_EN0 through RK817_POWER_EN3 and then save the contents to
>>> RK817_POWER_EN_SAVE0 (0x99) and RK817_POWER_EN_SAVE1 (0xa4). These
>>> values would be saved at probe time which would be very early in the
>>> boot process. The BSP kernel driver would then read the
>>> RK817_POWER_EN_SAVE0 register and if it was not 0 it would then read
>>> RK817_POWER_EN_SAVE1 and write all of those saved values back to the
>>> PMIC as part of the reboot-notifier.
>>>
>>> So basically it looks like as part of the reboot/shutdown process for
>>> BSP kernel and bootloader, the initial values for what is on/off is
>>> getting written back to the regulators unless not-save-power-en is
>>> set in U-Boot. This process breaks down for boards where the BSP
>>> bootloader sets not-save-power-en.
>>>
>>> Since mainline Linux doesn't use these registers at all and there is
>>> no parameter to control their use in mainline U-Boot like there is in
>>> BSP U-Boot, we should just drop them so they don't interfere with the
>>> BSP kernel.
>>>
>>> Thank you,
>>> Chris
>> The purpose for POWER_EN_SAVE0/1 is that:
>>
>> During a PMIC reset, voltage settings are restored before enable (EN)
>> settings. For example:
>> a regulator that is disabled by the PMIC hardware at power-on, with a
>> default voltage of 1.8V,
>> may be enabled by the user and configured to output 1.0V during operation.
>> When a reboot triggers a PMIC reset, the regulator may briefly output 1.8V
>> before being disabled,
>> posing an overvoltage risk.
>>
>> To prevent this, U-Boot saves the initial power-on enable states in
>> POWER_EN_SAVE0/1.
>> The kernel restores these states in the reboot callback, disabling such
>> regulators
>> before the PMIC reset to avoid unintended voltage output.
>>
> This behavior is not reflected in mainline Linux, should it be?
> If it should, it seems to me the proper fix would be:
>
> 1) Add the functionality to set the regulators in Linux, like it is
> done in the BSP, as part of the reset handler.
>
> 2) Gate both the Linux functionality and U-Boot functionality behind
> a device-tree option. Unlike BSP, make it opt-in rather than opt-out
> (to preserve backwards compatibility with the existing mainline
> kernel).
>
> Let me know what you think and I can start this process.
>
> Thank you,
> Chris

I have synced this with the rockchip RK817 owner, and we suggest the 
second approach "Gate both ...".
Additionally, the RK BSP kernel will add parsing for the 
`not-save-power-en` property to control whether POWER_EN_SAVE0/1 is used.

>>>>> [1]https://gist.github.com/fr0z3n-cs/047b24a6fd430e816fe02a877ad7ab8c
>>>>>
>>>>> Signed-off-by: Chris Morgan<macromorgan@hotmail.com>
>>>>> ---
>>>>>     drivers/power/pmic/rk8xx.c | 11 -----------
>>>>>     1 file changed, 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
>>>>> index 95b71d2fe49..16b3dc7cb1c 100644
>>>>> --- a/drivers/power/pmic/rk8xx.c
>>>>> +++ b/drivers/power/pmic/rk8xx.c
>>>>> @@ -251,8 +251,6 @@ static int rk8xx_probe(struct udevice *dev)
>>>>>     	int ret = 0, i, show_variant;
>>>>>     	u8 msb, lsb, id_msb, id_lsb;
>>>>>     	u8 on_source = 0, off_source = 0;
>>>>> -	u8 power_en0, power_en1, power_en2, power_en3;
>>>>> -	u8 value;
>>>>>     	/* read Chip variant */
>>>>>     	if (device_is_compatible(dev, "rockchip,rk817") ||
>>>>> @@ -293,15 +291,6 @@ static int rk8xx_probe(struct udevice *dev)
>>>>>     		off_source = RK817_OFF_SOURCE;
>>>>>     		init_data = rk817_init_reg;
>>>>>     		init_data_num = ARRAY_SIZE(rk817_init_reg);
>>>>> -		power_en0 = pmic_reg_read(dev, RK817_POWER_EN0);
>>>>> -		power_en1 = pmic_reg_read(dev, RK817_POWER_EN1);
>>>>> -		power_en2 = pmic_reg_read(dev, RK817_POWER_EN2);
>>>>> -		power_en3 = pmic_reg_read(dev, RK817_POWER_EN3);
>>>>> -
>>>>> -		value = (power_en0 & 0x0f) | ((power_en1 & 0x0f) << 4);
>>>>> -		pmic_reg_write(dev, RK817_POWER_EN_SAVE0, value);
>>>>> -		value = (power_en2 & 0x0f) | ((power_en3 & 0x0f) << 4);
>>>>> -		pmic_reg_write(dev, RK817_POWER_EN_SAVE1, value);
>>>>>     		break;
>>>>>     	case RK806_ID: {
>>>>>     		u32 rst_fun = 2;
>> -- 
>> Best regards,
>>
>> 陈健洪 (Joseph Chen)
>> chenjh@rock-chips.com
>> 福州瑞芯微电子股份有限公司
>> Fuzhou Rockchip Electronics Co.Ltd
>> 福建省福州市铜盘路软件大道89号软件园A区21号楼 (350003)
>> No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC
>> TEL：0591-83991906/07-8573
>>
-- 
Best regards,

陈健洪 (Joseph Chen)
chenjh@rock-chips.com
福州瑞芯微电子股份有限公司
Fuzhou Rockchip Electronics Co.Ltd
福建省福州市铜盘路软件大道89号软件园A区17号楼 (350003)
No. 17 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC
TEL：0591-83991906/07-8573