>-----Original Message----- >From: ext Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >Sent: 12 January, 2010 20:25 >To: Kristo Tero (Nokia-D/Tampere) >Cc: linux-omap@xxxxxxxxxxxxxxx >Subject: Re: [PATCHv2 1/6] OMAP: Powerdomains: Add support for >INACTIVE state on pwrdm level > >Tero Kristo <tero.kristo@xxxxxxxxx> writes: > >> From: Tero Kristo <tero.kristo@xxxxxxxxx> >> >> Currently only ON, RET and OFF are supported, and ON is >arguably broken as it >> allows the powerdomain to enter INACTIVE state unless idle >is prevented. >> Now, pwrdm code prevents idle if ON is selected, and also >adds support for >> INACTIVE. This simplifies the control needed inside sleep code. >> >> Signed-off-by: Tero Kristo <tero.kristo@xxxxxxxxx> > >Hi Tero, > >apologies for the long delay in reviewing this updated series. No problem, I have actually been on a holiday myself so I did not really miss review comments that much. Thanks for taking the time to look at this. >I really like this idea as it indeed simplifies the control code >inside the idle path. This also needs a review by Paul and should >merge via his powerdomain updates for 2.6.34. > >The changelog should also describe that the powerdomain struct >now caches the next_state. Yeah, I can add this comment. >One minor comment. I think the introduction of signed compares in >certain cases leads to possible confusion and readability problems. > >I'm not sure I realy follow the need for the invalid state. Instead >of setting next_state to -1 in pwrdm_register, why not read the >current HW value and use that as the starting value? I guess this invalid stuff comes from my old lazy initialization habits. :) But yes, I can change this into a format where we just read the current value from the register. >If the invalid state is really needed, instead of using -1 and having >to change to using signed comparisons in certain cases, it seems like >we could just define a new invalid state as zero, and move the others >up a notch. This is probably not good, as it would break the direct SW to HW value mapping, so I will go with the previous one. > >Then, use something like this to check for a valid state: > >static inline bool pwrdm_is_valid_state(struct powerdomain *pwrdm) { > return (pwrdm->state > PWRDM_POWER_INVALID) ? true : false; >} > >Kevin > >> --- >> arch/arm/mach-omap2/powerdomain.c | 32 >+++++++++++++++++++++---- >> arch/arm/mach-omap2/powerdomains34xx.h | 26 >++++++++++---------- >> arch/arm/plat-omap/include/plat/powerdomain.h | 6 ++++- >> 3 files changed, 45 insertions(+), 19 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/powerdomain.c >b/arch/arm/mach-omap2/powerdomain.c >> index b6990e3..1237717 100644 >> --- a/arch/arm/mach-omap2/powerdomain.c >> +++ b/arch/arm/mach-omap2/powerdomain.c >> @@ -112,8 +112,8 @@ static struct powerdomain >*_pwrdm_deps_lookup(struct powerdomain *pwrdm, >> static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag) >> { >> >> - int prev; >> - int state; >> + u8 prev; >> + u8 state; >> >> if (pwrdm == NULL) >> return -EINVAL; >> @@ -220,7 +220,7 @@ int pwrdm_register(struct powerdomain *pwrdm) >> >> pr_debug("powerdomain: registered %s\n", pwrdm->name); >> ret = 0; >> - >> + pwrdm->next_state = -1; > >> pr_unlock: >> write_unlock_irqrestore(&pwrdm_rwlock, flags); >> >> @@ -701,19 +701,38 @@ int pwrdm_get_mem_bank_count(struct >powerdomain *pwrdm) >> */ >> int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst) >> { >> + u8 prg_pwrst; >> + >> if (!pwrdm) >> return -EINVAL; >> >> + if (pwrdm->next_state == pwrst) >> + return 0; >> + >> if (!(pwrdm->pwrsts & (1 << pwrst))) >> return -EINVAL; >> >> pr_debug("powerdomain: setting next powerstate for %s to %0x\n", >> pwrdm->name, pwrst); >> >> + /* INACTIVE is reserved, so we program pwrdm as ON */ >> + if (pwrst == PWRDM_POWER_INACTIVE) >> + prg_pwrst = PWRDM_POWER_ON; >> + else >> + prg_pwrst = pwrst; >> + >> prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK, >> - (pwrst << OMAP_POWERSTATE_SHIFT), >> + (prg_pwrst << OMAP_POWERSTATE_SHIFT), >> pwrdm->prcm_offs, PM_PWSTCTRL); >> >> + /* If next state is ON, prevent idle */ >> + if (pwrst == PWRDM_POWER_ON) >> + omap2_clkdm_deny_idle(pwrdm->pwrdm_clkdms[0]); >> + else >> + omap2_clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]); >> + >> + pwrdm->next_state = pwrst; >> + >> return 0; >> } >> >> @@ -730,8 +749,11 @@ int pwrdm_read_next_pwrst(struct >powerdomain *pwrdm) >> if (!pwrdm) >> return -EINVAL; >> >> + if (pwrdm->next_state > -1) >> + return pwrdm->next_state; >> + >> return prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTCTRL, >> - OMAP_POWERSTATE_MASK); >> + OMAP_POWERSTATE_MASK); >> } >> >> /** >> diff --git a/arch/arm/mach-omap2/powerdomains34xx.h >b/arch/arm/mach-omap2/powerdomains34xx.h >> index fd09b08..9eb2dc5 100644 >> --- a/arch/arm/mach-omap2/powerdomains34xx.h >> +++ b/arch/arm/mach-omap2/powerdomains34xx.h >> @@ -165,7 +165,7 @@ static struct powerdomain iva2_pwrdm = { >> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430), >> .dep_bit = OMAP3430_PM_WKDEP_MPU_EN_IVA2_SHIFT, >> .wkdep_srcs = iva2_wkdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRSTS_OFF_RET, >> .banks = 4, >> .pwrsts_mem_ret = { >> @@ -188,7 +188,7 @@ static struct powerdomain mpu_34xx_pwrdm = { >> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430), >> .dep_bit = OMAP3430_EN_MPU_SHIFT, >> .wkdep_srcs = mpu_34xx_wkdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRSTS_OFF_RET, >> .banks = 1, >> .pwrsts_mem_ret = { >> @@ -206,7 +206,7 @@ static struct powerdomain >core_34xx_pre_es3_1_pwrdm = { >> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430ES1 | >> CHIP_IS_OMAP3430ES2 | >> CHIP_IS_OMAP3430ES3_0), >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .dep_bit = OMAP3430_EN_CORE_SHIFT, >> .banks = 2, >> .pwrsts_mem_ret = { >> @@ -214,8 +214,8 @@ static struct powerdomain >core_34xx_pre_es3_1_pwrdm = { >> [1] = PWRSTS_OFF_RET, /* MEM2RETSTATE */ >> }, >> .pwrsts_mem_on = { >> - [0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */ >> - [1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */ >> + [0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */ >> + [1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */ >> }, >> }; >> >> @@ -224,7 +224,7 @@ static struct powerdomain >core_34xx_es3_1_pwrdm = { >> .name = "core_pwrdm", >> .prcm_offs = CORE_MOD, >> .omap_chip = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES3_1), >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .dep_bit = OMAP3430_EN_CORE_SHIFT, >> .flags = PWRDM_HAS_HDWR_SAR, /* for USBTLL only */ >> .banks = 2, >> @@ -233,8 +233,8 @@ static struct powerdomain >core_34xx_es3_1_pwrdm = { >> [1] = PWRSTS_OFF_RET, /* MEM2RETSTATE */ >> }, >> .pwrsts_mem_on = { >> - [0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */ >> - [1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */ >> + [0] = PWRSTS_OFF_RET_INA_ON, /* MEM1ONSTATE */ >> + [1] = PWRSTS_OFF_RET_INA_ON, /* MEM2ONSTATE */ >> }, >> }; >> >> @@ -246,7 +246,7 @@ static struct powerdomain dss_pwrdm = { >> .dep_bit = OMAP3430_PM_WKDEP_MPU_EN_DSS_SHIFT, >> .wkdep_srcs = cam_dss_wkdeps, >> .sleepdep_srcs = dss_per_usbhost_sleepdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRDM_POWER_RET, >> .banks = 1, >> .pwrsts_mem_ret = { >> @@ -286,7 +286,7 @@ static struct powerdomain cam_pwrdm = { >> .prcm_offs = OMAP3430_CAM_MOD, >> .wkdep_srcs = cam_dss_wkdeps, >> .sleepdep_srcs = cam_gfx_sleepdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRDM_POWER_RET, >> .banks = 1, >> .pwrsts_mem_ret = { >> @@ -304,7 +304,7 @@ static struct powerdomain per_pwrdm = { >> .dep_bit = OMAP3430_EN_PER_SHIFT, >> .wkdep_srcs = per_usbhost_wkdeps, >> .sleepdep_srcs = dss_per_usbhost_sleepdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRSTS_OFF_RET, >> .banks = 1, >> .pwrsts_mem_ret = { >> @@ -326,7 +326,7 @@ static struct powerdomain neon_pwrdm = { >> .prcm_offs = OMAP3430_NEON_MOD, >> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430), >> .wkdep_srcs = neon_wkdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRDM_POWER_RET, >> }; >> >> @@ -336,7 +336,7 @@ static struct powerdomain usbhost_pwrdm = { >> .omap_chip = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2), >> .wkdep_srcs = per_usbhost_wkdeps, >> .sleepdep_srcs = dss_per_usbhost_sleepdeps, >> - .pwrsts = PWRSTS_OFF_RET_ON, >> + .pwrsts = PWRSTS_OFF_RET_INA_ON, >> .pwrsts_logic_ret = PWRDM_POWER_RET, >> /* >> * REVISIT: Enabling usb host save and restore >mechanism seems to >> diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h >b/arch/arm/plat-omap/include/plat/powerdomain.h >> index 3d45ee1..55350d0 100644 >> --- a/arch/arm/plat-omap/include/plat/powerdomain.h >> +++ b/arch/arm/plat-omap/include/plat/powerdomain.h >> @@ -37,6 +37,9 @@ >> >> #define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON)) >> >> +#define PWRSTS_OFF_RET_INA_ON (PWRSTS_OFF_RET_ON | \ >> + (1 << PWRDM_POWER_INACTIVE)) >> + >> >> /* Powerdomain flags */ >> #define PWRDM_HAS_HDWR_SAR (1 << 0) /* hardware >save-and-restore support */ >> @@ -117,7 +120,8 @@ struct powerdomain { >> >> struct list_head node; >> >> - int state; >> + u8 state; >> + s8 next_state; >> unsigned state_counter[4]; >> >> #ifdef CONFIG_PM_DEBUG >> -- >> 1.5.4.3 >> >> -- >> 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 >-- 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
