On Tue, Jul 4, 2023 at 9:19 AM Lukasz Majewski <lu...@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-lu...@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-lu...@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 <c...@londelec.com>
> > > Cc: Stefano Babic <sba...@denx.de>
> > > Cc: Marek Vasut <ma...@denx.de>
> > > Cc: Fabio Estevam <feste...@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: lu...@denx.de
