On 04/21/2015 08:00 PM, Tony Lindgren wrote: > * Nishanth Menon <nm@xxxxxx> [150421 15:13]: >> On 14:47-20150421, Tony Lindgren wrote: >>> * Tony Lindgren <tony@xxxxxxxxxxx> [150421 09:24]: >>>> If we use a combination of VMODE and I2C4 for retention modes, >>>> eventually the off idle power consumption will creep up by about >>>> 23mW, even during off mode with I2C4 always staying enabled. >>>> >>>> Looks like the only way to fix the extra power consumption is to >>>> disable I2C4 usage by setting SEL_VMODE for both off idle and >>>> retention idle. Let's also update the comments accordingly. >>> >>> Thanks Nishanth for pointing me to the related SmartReflex I2C4 >>> erratum. No need to disable I2C4 with the SREN workaround :) >>> >>> Better patch below, presumably this only affects omap3? >> >> SREN applies to both OMAP3 and 4 - so, the patch looks right. > > Oh OK, I was missing setting the clear bit for omap4. I've added > it now. > >>> If we use a combination of VMODE and I2C4 for retention modes, >>> eventually the off idle power consumption will creep up by about >>> 23mW, even during off mode with I2C4 always staying enabled. >>> >>> Turns out this is because of erratum 2.11 "Extra Power Consumed >> I suggest using the unique ID >> >> i531 - that remains constant across SoCs. > > OK thanks, updated patch below. > > Regards, > > Tony > > 8< ------------------ > From: Tony Lindgren <tony@xxxxxxxxxxx> > Date: Tue, 21 Apr 2015 14:31:23 -0700 > Subject: [PATCH] ARM: OMAP2+: Fix omap off idle power consumption creeping up > > If we use a combination of VMODE and I2C4 for retention modes, > eventually the off idle power consumption will creep up by about > 23mW, even during off mode with I2C4 always staying enabled. > > Turns out this is because of erratum i531 "Extra Power Consumed > When Repeated Start Operation Mode Is Enabled on I2C Interface > Dedicated for Smart Reflex (I2C4)" as pointed out by Nishanth > Menon <nm@xxxxxx>. > > Let's fix the issue by adding i2c_cfg_clear_mask for the bits > to clear when initializing the I2C4 adapter so we can clear > SREN bit that drives the I2C4 lines low otherwise when there > is no traffic. > > Fixes: 3b8c4ebb7630 ("ARM: OMAP3: Fix idle mode signaling for > sys_clkreq and sys_off_mode") > Cc: Kevin Hilman <khilman@xxxxxxxxxx> > Cc: Nishanth Menon <nm@xxxxxx> > Cc: Tero Kristo <t-kristo@xxxxxx> > Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx> > > --- a/arch/arm/mach-omap2/prm-regbits-34xx.h > +++ b/arch/arm/mach-omap2/prm-regbits-34xx.h > @@ -112,6 +112,7 @@ > #define OMAP3430_VC_CMD_ONLP_SHIFT 16 > #define OMAP3430_VC_CMD_RET_SHIFT 8 > #define OMAP3430_VC_CMD_OFF_SHIFT 0 > +#define OMAP3430_SREN_MASK (1 << 4) > #define OMAP3430_HSEN_MASK (1 << 3) > #define OMAP3430_MCODE_MASK (0x7 << 0) > #define OMAP3430_VALID_MASK (1 << 24) > --- a/arch/arm/mach-omap2/prm-regbits-44xx.h > +++ b/arch/arm/mach-omap2/prm-regbits-44xx.h > @@ -35,6 +35,7 @@ > #define OMAP4430_GLOBAL_WARM_SW_RST_SHIFT 1 > #define OMAP4430_GLOBAL_WUEN_MASK (1 << 16) > #define OMAP4430_HSMCODE_MASK (0x7 << 0) > +#define OMAP4430_SRMODEEN_MASK (1 << 4) > #define OMAP4430_HSMODEEN_MASK (1 << 3) > #define OMAP4430_HSSCLL_SHIFT 24 > #define OMAP4430_ICEPICK_RST_SHIFT 9 > --- a/arch/arm/mach-omap2/vc.c > +++ b/arch/arm/mach-omap2/vc.c > @@ -316,7 +316,8 @@ static void __init omap3_vc_init_pmic_signaling(struct voltagedomain *voltdm) > * idle. And we can also scale voltages to zero for off-idle. > * Note that no actual voltage scaling during off-idle will > * happen unless the board specific twl4030 PMIC scripts are > - * loaded. > + * loaded. See also omap_vc_i2c_init for comments regarding > + * erratum i531. > */ > val = voltdm->read(OMAP3_PRM_VOLTCTRL_OFFSET); > if (!(val & OMAP3430_PRM_VOLTCTRL_SEL_OFF)) { > @@ -704,9 +705,16 @@ static void __init omap_vc_i2c_init(struct voltagedomain *voltdm) > return; > } > > + /* > + * Note that for omap3 OMAP3430_SREN_MASK clears SREN to work around > + * erratum i531 "Extra Power Consumed When Repeated Start Operation > + * Mode Is Enabled on I2C Interface Dedicated for Smart Reflex (I2C4)". > + * Otherwise I2C4 eventually leads into about 23mW extran power being nitpick: extran ;) > + * consumed even during off idle using VMODE. > + */ > i2c_high_speed = voltdm->pmic->i2c_high_speed; > if (i2c_high_speed) > - voltdm->rmw(vc->common->i2c_cfg_hsen_mask, > + voltdm->rmw(vc->common->i2c_cfg_clear_mask, > vc->common->i2c_cfg_hsen_mask, > vc->common->i2c_cfg_reg); > > --- a/arch/arm/mach-omap2/vc.h > +++ b/arch/arm/mach-omap2/vc.h > @@ -34,6 +34,7 @@ struct voltagedomain; > * @cmd_ret_shift: RET field shift in PRM_VC_CMD_VAL_* register > * @cmd_off_shift: OFF field shift in PRM_VC_CMD_VAL_* register > * @i2c_cfg_reg: I2C configuration register offset > + * @i2c_cfg_clear_mask: high-speed mode bit clear mask in I2C config register > * @i2c_cfg_hsen_mask: high-speed mode bit field mask in I2C config register > * @i2c_mcode_mask: MCODE field mask for I2C config register > * > @@ -52,6 +53,7 @@ struct omap_vc_common { > u8 cmd_ret_shift; > u8 cmd_off_shift; > u8 i2c_cfg_reg; > + u8 i2c_cfg_clear_mask; > u8 i2c_cfg_hsen_mask; > u8 i2c_mcode_mask; > }; > --- a/arch/arm/mach-omap2/vc3xxx_data.c > +++ b/arch/arm/mach-omap2/vc3xxx_data.c > @@ -40,6 +40,7 @@ static struct omap_vc_common omap3_vc_common = { > .cmd_onlp_shift = OMAP3430_VC_CMD_ONLP_SHIFT, > .cmd_ret_shift = OMAP3430_VC_CMD_RET_SHIFT, > .cmd_off_shift = OMAP3430_VC_CMD_OFF_SHIFT, > + .i2c_cfg_clear_mask = OMAP3430_SREN_MASK, I know that the prminst rmw current code does: v &= ~mask; v |= bits; and not v &= ~mask; v |= (bits & mask); but to maintain legacy behavior in case the PRCM cleanup sequences change anything.. .i2c_cfg_clear_mask = OMAP3430_SREN_MASK | OMAP3430_HSEN_MASK, ? or voltdm->rmw(vc->common->i2c_cfg_clear_mask | vc->common->i2c_cfg_hsen_mask, vc->common->i2c_cfg_hsen_mask, vc->common->i2c_cfg_reg); no strong feelings though... > .i2c_cfg_hsen_mask = OMAP3430_HSEN_MASK, > .i2c_cfg_reg = OMAP3_PRM_VC_I2C_CFG_OFFSET, > .i2c_mcode_mask = OMAP3430_MCODE_MASK, > --- a/arch/arm/mach-omap2/vc44xx_data.c > +++ b/arch/arm/mach-omap2/vc44xx_data.c > @@ -42,6 +42,7 @@ static const struct omap_vc_common omap4_vc_common = { > .cmd_ret_shift = OMAP4430_RET_SHIFT, > .cmd_off_shift = OMAP4430_OFF_SHIFT, > .i2c_cfg_reg = OMAP4_PRM_VC_CFG_I2C_MODE_OFFSET, > + .i2c_cfg_clear_mask = OMAP4430_SRMODEEN_MASK, > .i2c_cfg_hsen_mask = OMAP4430_HSMODEEN_MASK, > .i2c_mcode_mask = OMAP4430_HSMCODE_MASK, > }; > otherwise, please feel to add: Reviewed-by: Nishanth Menon <nm@xxxxxx> -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html