[PATCH 2/2] mxs: Don't enable 4P2 reg if MXS is powered only from DCDC_BATT

[PATCH 2/2] mxs: Don't enable 4P2 reg if MXS is powered only from DCDC_BATT

[Cody Green]

'mxs_power_enable_4p2()' function call was added to 'mxs_batt_boot()'
in 'commit a0f97610757d' to enable DCDC converter when board is powered
from 5V and has detected sufficient battery voltage.
This involves enabling 4P2 regulator and there is a code
in 'mxs_power_enable_4p2()' that disables VDDIO, VDDA, VDDD outputs of
the DCDC converter and enables BO for each power rail:

  setbits_le32(&power_regs->hw_power_vddioctrl,
    POWER_VDDIOCTRL_DISABLE_FET | POWER_VDDIOCTRL_PWDN_BRNOUT);

There is no issue if the MXS is powered from the 5V source and linear
regulators are supplying power to the VDDIO, VDDA, VDDD rails.
However, if the MXS is powered only from the DCDC_BATT without 5V,
disabling the DCDC converter outputs causes VDDIO, VDDA, VDDD rails to
lose power.

The proposed solution is not to call the 'mxs_power_enable_4p2()'
function if the MXS is powered only by the DCDC_BATT, because there is
no reason to enable 4P2 regulator in this case. Also 5V brownout should
not be enabled in 'mxs_power_init()' and linear regulator checks should
be disabled in 'mxs_power_set_vddx()'.

Signed-off-by: Cody Green <cody@londelec.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
---
 arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
index 33b76533e4..72172705f2 100644
--- a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
+++ b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
@@ -752,7 +752,9 @@ static void mxs_batt_boot(void)
 		POWER_5VCTRL_CHARGE_4P2_ILIMIT_MASK,
 		0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET);
 
+#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
 	mxs_power_enable_4p2();
+#endif
 }
 
 /**
@@ -1137,8 +1139,11 @@ static void mxs_power_set_vddx(const struct mxs_vddx_cfg *cfg,
 	cur_target += cfg->lowest_mV;
 
 	adjust_up = new_target > cur_target;
+
+#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
 	if (cfg->powered_by_linreg)
 		powered_by_linreg = cfg->powered_by_linreg();
+#endif
 
 	if (adjust_up && cfg->bo_irq) {
 		if (powered_by_linreg) {
@@ -1269,7 +1274,9 @@ void mxs_power_init(void)
 		POWER_CTRL_VBUS_VALID_IRQ | POWER_CTRL_BATT_BO_IRQ |
 		POWER_CTRL_DCDC4P2_BO_IRQ, &power_regs->hw_power_ctrl_clr);
 
+#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
 	writel(POWER_5VCTRL_PWDN_5VBRNOUT, &power_regs->hw_power_5vctrl_set);
+#endif
 
 	early_delay(1000);
 }
-- 
2.25.1

Re: [PATCH 2/2] mxs: Don't enable 4P2 reg if MXS is powered only from DCDC_BATT

[Marek Vasut]

On 7/3/23 18:33, Cody Green wrote:
> 'mxs_power_enable_4p2()' function call was added to 'mxs_batt_boot()'
> in 'commit a0f97610757d' to enable DCDC converter when board is powered
> from 5V and has detected sufficient battery voltage.
> This involves enabling 4P2 regulator and there is a code
> in 'mxs_power_enable_4p2()' that disables VDDIO, VDDA, VDDD outputs of
> the DCDC converter and enables BO for each power rail:
> 
>    setbits_le32(&power_regs->hw_power_vddioctrl,
>      POWER_VDDIOCTRL_DISABLE_FET | POWER_VDDIOCTRL_PWDN_BRNOUT);
> 
> There is no issue if the MXS is powered from the 5V source and linear
> regulators are supplying power to the VDDIO, VDDA, VDDD rails.
> However, if the MXS is powered only from the DCDC_BATT without 5V,
> disabling the DCDC converter outputs causes VDDIO, VDDA, VDDD rails to
> lose power.
> 
> The proposed solution is not to call the 'mxs_power_enable_4p2()'
> function if the MXS is powered only by the DCDC_BATT, because there is
> no reason to enable 4P2 regulator in this case. Also 5V brownout should
> not be enabled in 'mxs_power_init()' and linear regulator checks should
> be disabled in 'mxs_power_set_vddx()'.
> 
> Signed-off-by: Cody Green <cody@londelec.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> ---
>   arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
> index 33b76533e4..72172705f2 100644
> --- a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
> +++ b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
> @@ -752,7 +752,9 @@ static void mxs_batt_boot(void)
>   		POWER_5VCTRL_CHARGE_4P2_ILIMIT_MASK,
>   		0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET);
>   
> +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
>   	mxs_power_enable_4p2();
> +#endif
>   }
>   
>   /**
> @@ -1137,8 +1139,11 @@ static void mxs_power_set_vddx(const struct mxs_vddx_cfg *cfg,
>   	cur_target += cfg->lowest_mV;
>   
>   	adjust_up = new_target > cur_target;
> +
> +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
>   	if (cfg->powered_by_linreg)
>   		powered_by_linreg = cfg->powered_by_linreg();
> +#endif
>   
>   	if (adjust_up && cfg->bo_irq) {
>   		if (powered_by_linreg) {
> @@ -1269,7 +1274,9 @@ void mxs_power_init(void)
>   		POWER_CTRL_VBUS_VALID_IRQ | POWER_CTRL_BATT_BO_IRQ |
>   		POWER_CTRL_DCDC4P2_BO_IRQ, &power_regs->hw_power_ctrl_clr);
>   
> +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
>   	writel(POWER_5VCTRL_PWDN_5VBRNOUT, &power_regs->hw_power_5vctrl_set);
> +#endif
>   
>   	early_delay(1000);
>   }

+CC Lukasz

Re: [PATCH 2/2] mxs: Don't enable 4P2 reg if MXS is powered only from DCDC_BATT

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3899 bytes --]

Hi Marek, Cody,

> On 7/3/23 18:33, Cody Green wrote:
> > 'mxs_power_enable_4p2()' function call was added to
> > 'mxs_batt_boot()' in 'commit a0f97610757d' to enable DCDC converter
> > when board is powered from 5V and has detected sufficient battery
> > voltage. This involves enabling 4P2 regulator and there is a code
> > in 'mxs_power_enable_4p2()' that disables VDDIO, VDDA, VDDD outputs
> > of the DCDC converter and enables BO for each power rail:
> > 
> >    setbits_le32(&power_regs->hw_power_vddioctrl,
> >      POWER_VDDIOCTRL_DISABLE_FET | POWER_VDDIOCTRL_PWDN_BRNOUT);
> > 
> > There is no issue if the MXS is powered from the 5V source and
> > linear regulators are supplying power to the VDDIO, VDDA, VDDD
> > rails. However, if the MXS is powered only from the DCDC_BATT
> > without 5V, disabling the DCDC converter outputs causes VDDIO,
> > VDDA, VDDD rails to lose power.

I think that I've also hit the same issue with the XEA board
(upstreamed).

I've prepared a set of patches:
https://patchwork.ozlabs.org/project/uboot/patch/20230509143243.1523791-5-lukma@denx.de/

to fix this problem.

Those patches are now for some time on the mailing list for review - and
I do hope that Stefano will pull them with next PR for u-boot-imx
repository.

> > 
> > The proposed solution is not to call the 'mxs_power_enable_4p2()'
> > function if the MXS is powered only by the DCDC_BATT, because there
> > is no reason to enable 4P2 regulator in this case.

I think this patch:
https://patchwork.ozlabs.org/project/uboot/patch/20230509143243.1523791-3-lukma@denx.de/

address exactly this issue.

> > Also 5V brownout
> > should not be enabled in 'mxs_power_init()' and linear regulator
> > checks should be disabled in 'mxs_power_set_vddx()'.
> > 

I've also added some code to limit the VDD5V current when we intend to
use DCDC_BATT input as the _primary_ source of power (AN4199 advises
this one for industrial applications as the most reliable).


> > Signed-off-by: Cody Green <cody@londelec.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > ---
> >   arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
> > b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c index
> > 33b76533e4..72172705f2 100644 ---
> > a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +++
> > b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c @@ -752,7 +752,9 @@
> > static void mxs_batt_boot(void) POWER_5VCTRL_CHARGE_4P2_ILIMIT_MASK,
> >   		0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET);
> >   
> > +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
> >   	mxs_power_enable_4p2();
> > +#endif
> >   }
> >   
> >   /**
> > @@ -1137,8 +1139,11 @@ static void mxs_power_set_vddx(const struct
> > mxs_vddx_cfg *cfg, cur_target += cfg->lowest_mV;
> >   
> >   	adjust_up = new_target > cur_target;
> > +
> > +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
> >   	if (cfg->powered_by_linreg)
> >   		powered_by_linreg = cfg->powered_by_linreg();
> > +#endif
> >   
> >   	if (adjust_up && cfg->bo_irq) {
> >   		if (powered_by_linreg) {
> > @@ -1269,7 +1274,9 @@ void mxs_power_init(void)
> >   		POWER_CTRL_VBUS_VALID_IRQ |
> > POWER_CTRL_BATT_BO_IRQ | POWER_CTRL_DCDC4P2_BO_IRQ,
> > &power_regs->hw_power_ctrl_clr); 
> > +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
> >   	writel(POWER_5VCTRL_PWDN_5VBRNOUT,
> > &power_regs->hw_power_5vctrl_set); +#endif
> >   
> >   	early_delay(1000);
> >   }  
> 
> +CC Lukasz




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] mxs: Don't enable 4P2 reg if MXS is powered only from DCDC_BATT

[Cody Green]

On Tue, Jul 4, 2023 at 9:19 AM Lukasz Majewski <lukma@denx.de> wrote:
Hi Lukasz,
>
> Hi Marek, Cody,
>
> > On 7/3/23 18:33, Cody Green wrote:
> > > 'mxs_power_enable_4p2()' function call was added to
> > > 'mxs_batt_boot()' in 'commit a0f97610757d' to enable DCDC converter
> > > when board is powered from 5V and has detected sufficient battery
> > > voltage. This involves enabling 4P2 regulator and there is a code
> > > in 'mxs_power_enable_4p2()' that disables VDDIO, VDDA, VDDD outputs
> > > of the DCDC converter and enables BO for each power rail:
> > >
> > >    setbits_le32(&power_regs->hw_power_vddioctrl,
> > >      POWER_VDDIOCTRL_DISABLE_FET | POWER_VDDIOCTRL_PWDN_BRNOUT);
> > >
> > > There is no issue if the MXS is powered from the 5V source and
> > > linear regulators are supplying power to the VDDIO, VDDA, VDDD
> > > rails. However, if the MXS is powered only from the DCDC_BATT
> > > without 5V, disabling the DCDC converter outputs causes VDDIO,
> > > VDDA, VDDD rails to lose power.
>
> I think that I've also hit the same issue with the XEA board
> (upstreamed).
>
> I've prepared a set of patches:
> https://patchwork.ozlabs.org/project/uboot/patch/20230509143243.1523791-5-lukma@denx.de/
>
> to fix this problem.
>
> Those patches are now for some time on the mailing list for review - and
> I do hope that Stefano will pull them with next PR for u-boot-imx
> repository.
>
> > >
> > > The proposed solution is not to call the 'mxs_power_enable_4p2()'
> > > function if the MXS is powered only by the DCDC_BATT, because there
> > > is no reason to enable 4P2 regulator in this case.
>
> I think this patch:
> https://patchwork.ozlabs.org/project/uboot/patch/20230509143243.1523791-3-lukma@denx.de/
>
> address exactly this issue.

Yes, your patch [3/5] addresses the issue perfectly!
I was considering to add the Kconfig option, but decided against it,
as I thought nobody else is running the MXS only from DCDC_BATT without 5V.
In my opinion Kconfig option is better solution than my patch and
I am happy to add 'Tested-by' tag to your patch [3/5], if you wish.

>
> > > Also 5V brownout
> > > should not be enabled in 'mxs_power_init()' and linear regulator
> > > checks should be disabled in 'mxs_power_set_vddx()'.
> > >
>
> I've also added some code to limit the VDD5V current when we intend to
> use DCDC_BATT input as the _primary_ source of power (AN4199 advises
> this one for industrial applications as the most reliable).
>

I checked other patches in your set and as far as I can tell
the rest of them apply if the board is powered from both 
DCDC_BATT and 5V source. This is all good stuff, but what shall
we do about enabling 5V brownout:

 	writel(POWER_5VCTRL_PWDN_5VBRNOUT, &power_regs->hw_power_5vctrl_set);
in 'void mxs_power_init()'?

Would you consider using the new 'MXS_PMU_ENABLE_4P2_LINEAR_REGULATOR'
option to not enable 5V brownout as well? e.g.

	if (CONFIG_IS_ENABLED(MXS_PMU_ENABLE_4P2_LINEAR_REGULATOR))
	 	writel(POWER_5VCTRL_PWDN_5VBRNOUT, &power_regs->hw_power_5vctrl_set);

My reasoning is the 4P2 regulator would be disabled only in the
scenario if the board is running without 5V hence there is no need
for a 5V brownout.

There is a slightly different problem with:

	if (cfg->powered_by_linreg)
 		powered_by_linreg = cfg->powered_by_linreg();
in 'mxs_power_set_vddx()'.

I noticed that function 'mxs_get_vddd_power_source_off()'
returns '1' (i.e. powered_by_linreg) because of the following code:

	if (!(tmp & POWER_VDDDCTRL_ENABLE_LINREG)) {
		if ((tmp & POWER_VDDDCTRL_LINREG_OFFSET_MASK) ==
			POWER_VDDDCTRL_LINREG_OFFSET_1STEPS_BELOW) {
			return 1;
		}
	}

I am not sure why this piece of code even exists, but rather than 
risking to brake something by removing it, I decided that we can simply
disable calling 'mxs_get_vddd_power_source_off()' if the board is
powered from DCDC_BATT only. So would you consider the following as well?

	if (CONFIG_IS_ENABLED(MXS_PMU_ENABLE_4P2_LINEAR_REGULATOR))
 		if (cfg->powered_by_linreg)
 			powered_by_linreg = cfg->powered_by_linreg();

>
> > > Signed-off-by: Cody Green <cody@londelec.com>
> > > Cc: Stefano Babic <sbabic@denx.de>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > ---
> > >   arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
> > > b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c index
> > > 33b76533e4..72172705f2 100644 ---
> > > a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +++
> > > b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c @@ -752,7 +752,9 @@
> > > static void mxs_batt_boot(void) POWER_5VCTRL_CHARGE_4P2_ILIMIT_MASK,
> > >             0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET);
> > >   
> > > +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
> > >     mxs_power_enable_4p2();
> > > +#endif
> > >   }
> > >   
> > >   /**
> > > @@ -1137,8 +1139,11 @@ static void mxs_power_set_vddx(const struct
> > > mxs_vddx_cfg *cfg, cur_target += cfg->lowest_mV;
> > >   
> > >     adjust_up = new_target > cur_target;
> > > +
> > > +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
> > >     if (cfg->powered_by_linreg)
> > >             powered_by_linreg = cfg->powered_by_linreg();
> > > +#endif
> > >   
> > >     if (adjust_up && cfg->bo_irq) {
> > >             if (powered_by_linreg) {
> > > @@ -1269,7 +1274,9 @@ void mxs_power_init(void)
> > >             POWER_CTRL_VBUS_VALID_IRQ |
> > > POWER_CTRL_BATT_BO_IRQ | POWER_CTRL_DCDC4P2_BO_IRQ,
> > > &power_regs->hw_power_ctrl_clr);
> > > +#ifndef CFG_SPL_MXS_NO_VDD5V_SOURCE
> > >     writel(POWER_5VCTRL_PWDN_5VBRNOUT,
> > > &power_regs->hw_power_5vctrl_set); +#endif
> > >   
> > >     early_delay(1000);
> > >   } 
> >
> > +CC Lukasz
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de