| Message ID | 1344120280-7469-1-git-send-email-otavio@ossystems.com.br |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Otavio Salvador, > Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> > --- > Changes in v2: > - add comments > - fix when we have vbus OR vdd5v > - improve patch short description > > Changes in v3: > - change short-description prefix > > arch/arm/cpu/arm926ejs/mx28/spl_power_init.c | 26 > +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 > deletions(-) > > diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c > b/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c index 4b09b0c..fdf810c > 100644 > --- a/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c > +++ b/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c > @@ -564,6 +564,15 @@ void mx28_batt_boot(void) > 0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET); > } > > +static int mx28_valid_vbus(void) > +{ > + struct mx28_power_regs *power_regs = > + (struct mx28_power_regs *)MXS_POWER_BASE; > + > + /* iMX23 uses POWER_STS_VBUSVALID_STATUS at same offset */ > + return readl(&power_regs->hw_power_sts) & POWER_STS_VBUSVALID0_STATUS; > +} > + > void mx28_handle_5v_conflict(void) > { > struct mx28_power_regs *power_regs = > @@ -577,14 +586,19 @@ void mx28_handle_5v_conflict(void) > tmp = readl(&power_regs->hw_power_sts); > > if (tmp & POWER_STS_VDDIO_BO) { > + /* > + * VDDIO has a brownout, then the VDD5V_GT_VDDIO becomes > + * unreliable > + */ These should go into separate patch. > mx28_powerdown(); > break; > } > > - if (tmp & POWER_STS_VDD5V_GT_VDDIO) { > + if (mx28_valid_vbus() || (tmp & POWER_STS_VDD5V_GT_VDDIO)) { And if VDD5V_GT_VDDIO isn't true, this will crash. > mx28_boot_valid_5v(); > break; > } else { > + /* Neither 5v sees 5v so we power down */ > mx28_powerdown(); > break; > } > @@ -601,17 +615,15 @@ void mx28_5v_boot(void) > struct mx28_power_regs *power_regs = > (struct mx28_power_regs *)MXS_POWER_BASE; > > - /* > - * NOTE: In original IMX-Bootlets, this also checks for VBUSVALID, > - * but their implementation always returns 1 so we omit it here. > - */ > - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { > + if (mx28_valid_vbus() && And again ... you unconditionally add something that will break other boards that aren't supplied from 5V. This part isn't present in mx28 bootlets if I'm right, yes? > + (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO)) { > mx28_boot_valid_5v(); > return; > } > > early_delay(1000); > - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { > + if (mx28_valid_vbus() && > + (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO)) { > mx28_boot_valid_5v(); > return; > } Best regards, Marek Vasut
On Sat, Aug 4, 2012 at 7:49 PM, Marek Vasut <marex@denx.de> wrote: > Dear Otavio Salvador, > >> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> >> --- >> Changes in v2: >> - add comments >> - fix when we have vbus OR vdd5v >> - improve patch short description >> >> Changes in v3: >> - change short-description prefix >> >> arch/arm/cpu/arm926ejs/mx28/spl_power_init.c | 26 >> +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 >> deletions(-) >> >> diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c >> b/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c index 4b09b0c..fdf810c >> 100644 >> --- a/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c >> +++ b/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c >> @@ -564,6 +564,15 @@ void mx28_batt_boot(void) >> 0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET); >> } >> >> +static int mx28_valid_vbus(void) >> +{ >> + struct mx28_power_regs *power_regs = >> + (struct mx28_power_regs *)MXS_POWER_BASE; >> + >> + /* iMX23 uses POWER_STS_VBUSVALID_STATUS at same offset */ >> + return readl(&power_regs->hw_power_sts) & POWER_STS_VBUSVALID0_STATUS; >> +} >> + >> void mx28_handle_5v_conflict(void) >> { >> struct mx28_power_regs *power_regs = >> @@ -577,14 +586,19 @@ void mx28_handle_5v_conflict(void) >> tmp = readl(&power_regs->hw_power_sts); >> >> if (tmp & POWER_STS_VDDIO_BO) { >> + /* >> + * VDDIO has a brownout, then the VDD5V_GT_VDDIO becomes >> + * unreliable >> + */ > > These should go into separate patch. Ok; will split it. >> mx28_powerdown(); >> break; >> } >> >> - if (tmp & POWER_STS_VDD5V_GT_VDDIO) { >> + if (mx28_valid_vbus() || (tmp & POWER_STS_VDD5V_GT_VDDIO)) { > > And if VDD5V_GT_VDDIO isn't true, this will crash. On bootlets mx28_valid_vbus() just returns 1 but the code of mx23 does implement it and on the datasheet of mx28 it seems a valid check. >> mx28_boot_valid_5v(); >> break; >> } else { >> + /* Neither 5v sees 5v so we power down */ >> mx28_powerdown(); >> break; >> } >> @@ -601,17 +615,15 @@ void mx28_5v_boot(void) >> struct mx28_power_regs *power_regs = >> (struct mx28_power_regs *)MXS_POWER_BASE; >> >> - /* >> - * NOTE: In original IMX-Bootlets, this also checks for VBUSVALID, >> - * but their implementation always returns 1 so we omit it here. >> - */ >> - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { >> + if (mx28_valid_vbus() && > > And again ... you unconditionally add something that will break other boards > that aren't supplied from 5V. This part isn't present in mx28 bootlets if I'm > right, yes? Yes; this check is there too. But the comment about the difference between mx23 and mx28 code is applied here too. >> + (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO)) { >> mx28_boot_valid_5v(); >> return; >> } >> >> early_delay(1000); >> - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { >> + if (mx28_valid_vbus() && >> + (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO)) { >> mx28_boot_valid_5v(); >> return; >> } Regards,
Dear Otavio Salvador, > On Sat, Aug 4, 2012 at 7:49 PM, Marek Vasut <marex@denx.de> wrote: > > Dear Otavio Salvador, > > > >> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> > >> --- [...] > >> - /* > >> - * NOTE: In original IMX-Bootlets, this also checks for VBUSVALID, > >> - * but their implementation always returns 1 so we omit it here. > >> - */ > >> - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { > >> + if (mx28_valid_vbus() && > > > > And again ... you unconditionally add something that will break other > > boards that aren't supplied from 5V. This part isn't present in mx28 > > bootlets if I'm right, yes? > > Yes; this check is there too. But the comment about the difference > between mx23 and mx28 code is applied here too. According to 5VCTRL register (mx28 11.12.2) bit 4 (VBUSVALID_5VDETECT), this check is even redundant. Actually, if you don't use the VBUSVALID comparator, this check might fail I think. > >> + (readl(&power_regs->hw_power_sts) & > >> POWER_STS_VDD5V_GT_VDDIO)) { > >> > >> mx28_boot_valid_5v(); > >> return; > >> > >> } > >> > >> early_delay(1000); > >> > >> - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { > >> + if (mx28_valid_vbus() && > >> + (readl(&power_regs->hw_power_sts) & > >> POWER_STS_VDD5V_GT_VDDIO)) { > >> > >> mx28_boot_valid_5v(); > >> return; > >> > >> } > > Regards, Best regards, Marek Vasut
diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c b/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c index 4b09b0c..fdf810c 100644 --- a/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c +++ b/arch/arm/cpu/arm926ejs/mx28/spl_power_init.c @@ -564,6 +564,15 @@ void mx28_batt_boot(void) 0x8 << POWER_5VCTRL_CHARGE_4P2_ILIMIT_OFFSET); } +static int mx28_valid_vbus(void) +{ + struct mx28_power_regs *power_regs = + (struct mx28_power_regs *)MXS_POWER_BASE; + + /* iMX23 uses POWER_STS_VBUSVALID_STATUS at same offset */ + return readl(&power_regs->hw_power_sts) & POWER_STS_VBUSVALID0_STATUS; +} + void mx28_handle_5v_conflict(void) { struct mx28_power_regs *power_regs = @@ -577,14 +586,19 @@ void mx28_handle_5v_conflict(void) tmp = readl(&power_regs->hw_power_sts); if (tmp & POWER_STS_VDDIO_BO) { + /* + * VDDIO has a brownout, then the VDD5V_GT_VDDIO becomes + * unreliable + */ mx28_powerdown(); break; } - if (tmp & POWER_STS_VDD5V_GT_VDDIO) { + if (mx28_valid_vbus() || (tmp & POWER_STS_VDD5V_GT_VDDIO)) { mx28_boot_valid_5v(); break; } else { + /* Neither 5v sees 5v so we power down */ mx28_powerdown(); break; } @@ -601,17 +615,15 @@ void mx28_5v_boot(void) struct mx28_power_regs *power_regs = (struct mx28_power_regs *)MXS_POWER_BASE; - /* - * NOTE: In original IMX-Bootlets, this also checks for VBUSVALID, - * but their implementation always returns 1 so we omit it here. - */ - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { + if (mx28_valid_vbus() && + (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO)) { mx28_boot_valid_5v(); return; } early_delay(1000); - if (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO) { + if (mx28_valid_vbus() && + (readl(&power_regs->hw_power_sts) & POWER_STS_VDD5V_GT_VDDIO)) { mx28_boot_valid_5v(); return; }
Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> --- Changes in v2: - add comments - fix when we have vbus OR vdd5v - improve patch short description Changes in v3: - change short-description prefix arch/arm/cpu/arm926ejs/mx28/spl_power_init.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)