diff mbox

[U-Boot] mxs: Possible in bug in mxs_enable_4p2_dcdc_input

Message ID 978811302.20941.1431496941951.JavaMail.open-xchange@oxbsltgw06.schlund.de
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Stefan Wahren May 13, 2015, 6:02 a.m. UTC 
Hi,

during the study of DC-DC handling in u-boot i noticed a possible bug.

In case the DC-DC is already enabled the function returns without reenabling
brown out detection. I don't think that's okay.
A possible fix would be to do the check if DC-DC is already enabled before
disabling brown out detection. This idea is
attached as a draft. If it's acceptable i will send a proper patch.

Best regards
Stefan

-------------------%<-------------------------------------------------------------------------

Comments

Stefan Wahren May 13, 2015, 8:14 a.m. UTC | #1 
Hi Marek,

Am 13.05.2015 um 09:13 schrieb Marek Vasut:
> On Wednesday, May 13, 2015 at 08:02:21 AM, Stefan Wahren wrote:
>> Hi,
> Hi,
>
>> during the study of DC-DC handling in u-boot i noticed a possible bug.
>>
>> In case the DC-DC is already enabled the function returns without
>> reenabling brown out detection. I don't think that's okay.
>> A possible fix would be to do the check if DC-DC is already enabled before
>> disabling brown out detection. This idea is
>> attached as a draft. If it's acceptable i will send a proper patch.
> What exactly would happen if you enable DCDC converter while BO detection
> is already enabled ? It is possible, that enabling the DCDC would cause a
> dip on the power rail and cause in turn a BO, no ? IIRC, that is the reason
> why BO detection is disabled when enabling DCDC.

i think we're talking about different things. The patch won't touch this
necessary behaviour.

No, i think it's faulty to leave the function with BO detection disabled
in the case the DC-DC is already enabled:

if (xfer && (readl(&power_regs->hw_power_5vctrl) &
                POWER_5VCTRL_ENABLE_DCDC)) {
         return;
}

>
> Best regards,
> Marek Vasut

Stefan
Marek Vasut May 13, 2015, 8:03 p.m. UTC | #2 
On Wednesday, May 13, 2015 at 10:14:19 AM, Stefan Wahren wrote:
> Hi Marek,
> 
> Am 13.05.2015 um 09:13 schrieb Marek Vasut:
> > On Wednesday, May 13, 2015 at 08:02:21 AM, Stefan Wahren wrote:
> >> Hi,
> > 
> > Hi,
> > 
> >> during the study of DC-DC handling in u-boot i noticed a possible bug.
> >> 
> >> In case the DC-DC is already enabled the function returns without
> >> reenabling brown out detection. I don't think that's okay.
> >> A possible fix would be to do the check if DC-DC is already enabled
> >> before disabling brown out detection. This idea is
> >> attached as a draft. If it's acceptable i will send a proper patch.
> > 
> > What exactly would happen if you enable DCDC converter while BO detection
> > is already enabled ? It is possible, that enabling the DCDC would cause a
> > dip on the power rail and cause in turn a BO, no ? IIRC, that is the
> > reason why BO detection is disabled when enabling DCDC.
> 
> i think we're talking about different things. The patch won't touch this
> necessary behaviour.
> 
> No, i think it's faulty to leave the function with BO detection disabled
> in the case the DC-DC is already enabled:
> 
> if (xfer && (readl(&power_regs->hw_power_5vctrl) &
>                 POWER_5VCTRL_ENABLE_DCDC)) {
>          return;
> }

Uh uh, I'm an idiot and I was probably half asleep when I blurted this last
mail, sorry.

Obviously, enabling the BO properly makes sense.

Best regards,
Marek Vasut
Stefano Babic May 15, 2015, 11:14 a.m. UTC | #3 
Hi Stefan,

On 13/05/2015 08:02, Stefan Wahren wrote:
> Hi,
> 
> during the study of DC-DC handling in u-boot i noticed a possible bug.
> 
> In case the DC-DC is already enabled the function returns without reenabling
> brown out detection. I don't think that's okay.
> A possible fix would be to do the check if DC-DC is already enabled before
> disabling brown out detection. This idea is
> attached as a draft. If it's acceptable i will send a proper patch.
> 

It looks like we agree on your changes. Can you send then a proper patch
with your Signed-off-by ?

Thanks !

Best regards,
Stefano Babic
diff mbox

Patch

diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
index c342217..42f3df2 100644
--- a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
+++ b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
@@ -332,6 +332,11 @@  static void mxs_enable_4p2_dcdc_input(int xfer)
 
 	debug("SPL: %s 4P2 DC-DC Input\n", xfer ? "Enabling" : "Disabling");
 
+	if (xfer && (readl(&power_regs->hw_power_5vctrl) &
+			POWER_5VCTRL_ENABLE_DCDC)) {
+		return;
+	}
+
 	prev_5v_brnout = readl(&power_regs->hw_power_5vctrl) &
 				POWER_5VCTRL_PWDN_5VBRNOUT;
 	prev_5v_droop = readl(&power_regs->hw_power_ctrl) &
@@ -343,11 +348,6 @@  static void mxs_enable_4p2_dcdc_input(int xfer)
 
 	clrbits_le32(&power_regs->hw_power_ctrl, POWER_CTRL_ENIRQ_VDD5V_DROOP);
 
-	if (xfer && (readl(&power_regs->hw_power_5vctrl) &
-			POWER_5VCTRL_ENABLE_DCDC)) {
-		return;
-	}
-
 	/*
 	 * Recording orignal values that will be modified temporarlily
 	 * to handle a chip bug. See chip errata for CQ ENGR00115837