>-----Original Message-----
>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com]
>Sent: 12 January, 2010 20:25
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv2 1/6] OMAP: Powerdomains: Add support for
>INACTIVE state on pwrdm level
>
>Tero Kristo <tero.kri...@nokia.com> writes:
>
>> From: Tero Kristo <tero.kri...@nokia.com>
>>
>> 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.kri...@nokia.com>
>
>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 majord...@vger.kernel.org
>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
