Hi Nishant, On Fri, Jul 13, 2012 at 5:01 AM, Menon, Nishanth <nm@xxxxxx> wrote: > On Thu, Jun 14, 2012 at 9:53 AM, Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> wrote: > [..] >> --- a/arch/arm/mach-omap2/powerdomain-common.c >> +++ b/arch/arm/mach-omap2/powerdomain-common.c >> @@ -108,3 +108,74 @@ u32 omap2_pwrdm_get_mem_bank_stst_mask(u8 bank) >> return 0; >> } >> >> +/** >> + * omap2_pwrdm_func_to_pwrst - Convert functional (i.e. logical) to >> + * internal (i.e. registers) values for the power domains states. >> + * @struct powerdomain * to convert the values for >> + * @func_pwrst: functional power state >> + * >> + * Returns the internal power state value for the power domain, or >> + * -EINVAL in case of invalid parameters passed in. >> + */ >> +int omap2_pwrdm_func_to_pwrst(struct powerdomain *pwrdm, u8 func_pwrst) > > Unless we handle logicpwst and pwst here, it makes it hard for non > arch/arm/mach-omap2/powerdomain* files to use this properly. The logic and mem states support is coming in another patch in the series. I think I should merge the patches together since I got the same remarks a few times already > >> +{ >> + int ret; >> + >> + switch (func_pwrst) { >> + case PWRDM_FUNC_PWRST_ON: >> + ret = PWRDM_POWER_ON; >> + break; >> + case PWRDM_FUNC_PWRST_INACTIVE: >> + ret = PWRDM_POWER_INACTIVE; >> + break; >> + case PWRDM_FUNC_PWRST_CSWR: >> + case PWRDM_FUNC_PWRST_OSWR: >> + ret = PWRDM_POWER_RET; > > logic power state? > >> + break; >> + case PWRDM_FUNC_PWRST_OFF: >> + ret = PWRDM_POWER_OFF; >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> + >> +/** >> + * omap2_pwrdm_pwrst_to_func - Convert internal (i.e. registers) to >> + * functional (i.e. logical) values for the power domains states. >> + * @struct powerdomain * to convert the values for >> + * @pwrst: internal power state >> + * >> + * Returns the functional power state value for the power domain, or >> + * -EINVAL in case of invalid parameters passed in. >> + */ >> +int omap2_pwrdm_pwrst_to_func(struct powerdomain *pwrdm, u8 pwrst) > Same here as well.. >> +{ >> + int ret; >> + >> + switch (pwrst) { >> + case PWRDM_POWER_ON: >> + ret = PWRDM_FUNC_PWRST_ON; >> + break; >> + case PWRDM_POWER_INACTIVE: >> + ret = PWRDM_FUNC_PWRST_INACTIVE; >> + break; >> + case PWRDM_POWER_RET: >> + /* >> + * XXX warning: return OSWR in case of pd in RET and >> + * logic in OFF >> + */ > We need to be able to pass in logic pwst to be able to make that determination. > >> + ret = PWRDM_FUNC_PWRST_CSWR; >> + break; >> + case PWRDM_POWER_OFF: >> + ret = PWRDM_FUNC_PWRST_OFF; >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> + >> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c >> index 1641e72..e79b5ae 100644 >> --- a/arch/arm/mach-omap2/powerdomain.c >> +++ b/arch/arm/mach-omap2/powerdomain.c >> @@ -466,6 +466,135 @@ int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm) >> } >> >> /** >> + * pwrdm_func_to_pwrst - get the internal (i.e. registers) value mapped >> + * to the functional state >> + * @pwrdm: struct powerdomain * to query >> + * @func_pwrst: functional power state >> + * >> + * Convert the functional power state to the platform specific power >> + * domain state value. >> + * Returns the internal power domain state value or -EINVAL in case >> + * of invalid parameters passed in. >> + */ >> +int pwrdm_func_to_pwrst(struct powerdomain *pwrdm, u8 func_pwrst) > should take enum pwrdm_func_state func_pwrst Ok > >> +{ >> + int ret = func_pwrst; >> + >> + if ((!pwrdm) || (func_pwrst >= PWRDM_MAX_FUNC_PWRSTS)) >> + return -EINVAL; >> + >> + if (arch_pwrdm && arch_pwrdm->pwrdm_func_to_pwrst) >> + ret = arch_pwrdm->pwrdm_func_to_pwrst(pwrdm, func_pwrst); > > Should we not return an error if the function pointer is not available > for the arch? > on platforms without func_pwrst pointer converter routing hooked on, > we return func_pwrst back to caller, which expects pwrst! This is what the code does, cf. the first line of the function 'int ret = func_pwrst;'. Is that what you mean? > >> + >> + pr_debug("powerdomain: convert func %0x to pwrst %0x for %s\n", >> + func_pwrst, ret, pwrdm->name); >> + >> + return ret; >> +} >> + >> +/** >> + * pwrdm_pwrst_to_func - get the functional (i.e. logical) value mapped >> + * to the internal state >> + * @pwrdm: struct powerdomain * to query >> + * @pwrst: internal power state >> + * >> + * Convert the internal power state to the power domain functional value. >> + * Returns the functional power domain state value or -EINVAL in case >> + * of invalid parameters passed in. >> + */ >> +int pwrdm_pwrst_to_func(struct powerdomain *pwrdm, u8 pwrst) > should return enum pwrdm_func_state Ok > >> +{ >> + int ret = pwrst; >> + >> + if ((!pwrdm) || (pwrst >= PWRDM_MAX_PWRSTS)) >> + return -EINVAL; >> + >> + pr_debug("powerdomain: convert %0x pwrst to func for %s\n", >> + pwrst, pwrdm->name); >> + >> + if (arch_pwrdm && arch_pwrdm->pwrdm_pwrst_to_func) >> + ret = arch_pwrdm->pwrdm_pwrst_to_func(pwrdm, pwrst); > > same in reverse here. > >> + >> + return ret; >> +} >> + >> +/* Types of sleep_switch used in omap_set_pwrdm_state */ >> +#define FORCEWAKEUP_SWITCH 0 >> +#define LOWPOWERSTATE_SWITCH 1 >> + >> +/** >> + * omap_set_pwrdm_state - program next powerdomain power state >> + * @pwrdm: struct powerdomain * to set >> + * @func_pwrst: power domain functional state, one of the PWRDM_FUNC_* macros >> + * >> + * This programs the pwrdm next functional state, sets the dependencies >> + * and waits for the state to be applied. >> + */ >> +int omap_set_pwrdm_state(struct powerdomain *pwrdm, >> + enum pwrdm_func_state func_pwrst) >> +{ >> + u8 curr_pwrst, next_pwrst; >> + int pwrst = pwrdm_func_to_pwrst(pwrdm, func_pwrst); >> + int sleep_switch = -1, ret = 0, hwsup = 0; >> + >> + if (!pwrdm || IS_ERR(pwrdm) || (pwrst < 0)) { >> + pr_debug("%s: invalid params: pwrdm=%p, func_pwrst=%0x\n", >> + __func__, pwrdm, func_pwrst); >> + return -EINVAL; >> + } >> + >> + pr_debug("%s: set func_pwrst %0x to pwrdm %s\n", >> + __func__, func_pwrst, pwrdm->name); >> + >> + mutex_lock(&pwrdm->lock); > This should be a spinlock (previous patch i believe) - hurts cpuidle otherwise. Ok that had been reported by Ambresh. I have a fix ready for that. > >> + >> + while (!(pwrdm->pwrsts & (1 << pwrst))) { >> + if (pwrst == PWRDM_POWER_OFF) >> + goto out; >> + pwrst--; >> + } >> + > > if we like to handle a single API for OSWR and CSWR, then it needs to > handle logic power state as well here. > >> + next_pwrst = pwrdm_read_next_pwrst(pwrdm); >> + if (next_pwrst == pwrst) >> + goto out; >> + >> + curr_pwrst = pwrdm_read_pwrst(pwrdm); >> + if (curr_pwrst < PWRDM_POWER_ON) { >> + if ((curr_pwrst > pwrst) && >> + (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE)) { >> + sleep_switch = LOWPOWERSTATE_SWITCH; >> + } else { >> + hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]); >> + clkdm_wakeup(pwrdm->pwrdm_clkdms[0]); >> + sleep_switch = FORCEWAKEUP_SWITCH; >> + } >> + } >> + >> + ret = pwrdm_set_next_pwrst(pwrdm, pwrst); >> + if (ret) >> + pr_err("%s: unable to set power state of powerdomain: %s\n", >> + __func__, pwrdm->name); >> + >> + switch (sleep_switch) { >> + case FORCEWAKEUP_SWITCH: >> + if (hwsup) >> + clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]); >> + else >> + clkdm_sleep(pwrdm->pwrdm_clkdms[0]); >> + break; >> + case LOWPOWERSTATE_SWITCH: >> + pwrdm_set_lowpwrstchange(pwrdm); >> + pwrdm_wait_transition(pwrdm); >> + pwrdm_state_switch(pwrdm); >> + break; >> + } >> + >> +out: >> + mutex_unlock(&pwrdm->lock); >> + return ret; >> +} >> + >> +/** >> * pwrdm_set_next_pwrst - set next powerdomain power state >> * @pwrdm: struct powerdomain * to set >> * @pwrst: one of the PWRDM_POWER_* macros >> @@ -522,6 +651,21 @@ int pwrdm_read_next_pwrst(struct powerdomain *pwrdm) >> } >> >> /** >> + * pwrdm_read_next_func_pwrst - get next powerdomain functional power state >> + * @pwrdm: struct powerdomain * to get power state >> + * >> + * Return the powerdomain @pwrdm's next functional power state. >> + * Returns -EINVAL if the powerdomain pointer is null or returns >> + * the next power state upon success. >> + */ >> +int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm) > should return enum pwrdm_func_state >> +{ >> + int ret = pwrdm_read_next_pwrst(pwrdm); >> + >> + return pwrdm_pwrst_to_func(pwrdm, ret); >> +} >> + >> +/** >> * pwrdm_read_pwrst - get current powerdomain power state >> * @pwrdm: struct powerdomain * to get power state >> * >> @@ -543,6 +687,21 @@ int pwrdm_read_pwrst(struct powerdomain *pwrdm) >> } >> >> /** >> + * pwrdm_read_func_pwrst - get current powerdomain functional power state >> + * @pwrdm: struct powerdomain * to get power state >> + * >> + * Return the powerdomain @pwrdm's current functional power state. >> + * Returns -EINVAL if the powerdomain pointer is null or returns >> + * the current power state upon success. >> + */ >> +int pwrdm_read_func_pwrst(struct powerdomain *pwrdm) > should return enum pwrdm_func_state >> +{ >> + int ret = pwrdm_read_pwrst(pwrdm); >> + >> + return pwrdm_pwrst_to_func(pwrdm, ret); >> +} > > my Crib about the above apis are lack of logic power state handling :( > which forces code like cpuidle to use different apis for logic > power state and force them to use these apis just for pwrst. Please look at the rest of the series. >> + >> +/** >> * pwrdm_read_prev_pwrst - get previous powerdomain power state >> * @pwrdm: struct powerdomain * to get previous power state >> * >> @@ -564,6 +723,21 @@ int pwrdm_read_prev_pwrst(struct powerdomain *pwrdm) >> } >> >> /** >> + * pwrdm_read_prev_func_pwrst - get previous powerdomain functional power state >> + * @pwrdm: struct powerdomain * to get previous power state >> + * >> + * Return the powerdomain @pwrdm's previous functional power state. >> + * Returns -EINVAL if the powerdomain pointer is null or returns the >> + * previous power state upon success. >> + */ >> +int pwrdm_read_prev_func_pwrst(struct powerdomain *pwrdm) > should return enum pwrdm_func_state >> +{ >> + int ret = pwrdm_read_prev_pwrst(pwrdm); >> + >> + return pwrdm_pwrst_to_func(pwrdm, ret); >> +} > again here. > >> + >> +/** >> * pwrdm_set_logic_retst - set powerdomain logic power state upon retention >> * @pwrdm: struct powerdomain * to set >> * @pwrst: one of the PWRDM_POWER_* macros >> diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h >> index bab84fc..0404f9f 100644 >> --- a/arch/arm/mach-omap2/powerdomain.h >> +++ b/arch/arm/mach-omap2/powerdomain.h >> @@ -9,9 +9,6 @@ >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 as >> * published by the Free Software Foundation. >> - * >> - * XXX This should be moved to the mach-omap2/ directory at the earliest >> - * opportunity. >> */ >> >> #ifndef __ARCH_ARM_MACH_OMAP2_POWERDOMAIN_H >> @@ -26,7 +23,51 @@ >> >> #include "voltage.h" >> >> -/* Powerdomain basic power states */ >> +/*************************************************************** >> + * External API >> + ***************************************************************/ >> + >> +/* Powerdomain functional power states, used by the external API functions */ >> +enum pwrdm_func_state { >> + PWRDM_FUNC_PWRST_OFF = 0x0, >> + PWRDM_FUNC_PWRST_OSWR, >> + PWRDM_FUNC_PWRST_CSWR, >> + PWRDM_FUNC_PWRST_INACTIVE, >> + PWRDM_FUNC_PWRST_ON, >> + PWRDM_MAX_FUNC_PWRSTS /* Last value, used as the max value */ >> +}; >> + >> +struct clockdomain; >> +struct powerdomain; >> + >> +struct powerdomain *pwrdm_lookup(const char *name); >> + >> +int pwrdm_for_each(int (*fn)(struct powerdomain *pwrdm, void *user), >> + void *user); >> +int pwrdm_for_each_nolock(int (*fn)(struct powerdomain *pwrdm, void *user), >> + void *user); >> + >> +struct voltagedomain *pwrdm_get_voltdm(struct powerdomain *pwrdm); >> + >> +int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm); >> + >> +int pwrdm_read_prev_func_pwrst(struct powerdomain *pwrdm); >> +int pwrdm_read_func_pwrst(struct powerdomain *pwrdm); >> +int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm); >> +extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, >> + enum pwrdm_func_state func_pwrst); >> + >> +extern void omap242x_powerdomains_init(void); >> +extern void omap243x_powerdomains_init(void); >> +extern void omap3xxx_powerdomains_init(void); >> +extern void omap44xx_powerdomains_init(void); >> + >> + > > We should split the following out to a separate header which is not to > be used by anyone else other than powerdomain*.[ch] > >> +/*************************************************************** >> + * Internal code, only used in powerdomain*.[ch] >> + ***************************************************************/ >> + >> +/* Powerdomain internal power states, internal use only */ >> #define PWRDM_POWER_OFF 0x0 >> #define PWRDM_POWER_RET 0x1 >> #define PWRDM_POWER_INACTIVE 0x2 >> @@ -45,7 +86,6 @@ >> #define PWRSTS_RET_ON (PWRSTS_RET | PWRSTS_ON) >> #define PWRSTS_OFF_RET_ON (PWRSTS_OFF_RET | PWRSTS_ON) >> >> - >> /* Powerdomain flags */ >> #define PWRDM_HAS_HDWR_SAR (1 << 0) /* hardware save-and-restore support */ >> #define PWRDM_HAS_MPU_QUIRK (1 << 1) /* MPU pwr domain has MEM bank 0 bits >> @@ -74,9 +114,6 @@ >> /* XXX A completely arbitrary number. What is reasonable here? */ >> #define PWRDM_TRANSITION_BAILOUT 100000 >> >> -struct clockdomain; >> -struct powerdomain; >> - >> /** >> * struct powerdomain - OMAP powerdomain >> * @name: Powerdomain name >> @@ -130,6 +167,10 @@ struct powerdomain { >> >> /** >> * struct pwrdm_ops - Arch specific function implementations >> + * @pwrdm_func_to_pwrst: Convert the pd functional power state to >> + * the internal state >> + * @pwrdm_pwrst_to_func: Convert the pd internal power state to >> + * the functional state >> * @pwrdm_set_next_pwrst: Set the target power state for a pd >> * @pwrdm_read_next_pwrst: Read the target power state set for a pd >> * @pwrdm_read_pwrst: Read the current power state of a pd >> @@ -150,6 +191,8 @@ struct powerdomain { >> * @pwrdm_wait_transition: Wait for a pd state transition to complete >> */ >> struct pwrdm_ops { >> + int (*pwrdm_func_to_pwrst)(struct powerdomain *pwrdm, u8 func_pwrst); > should take in enum pwrdm_func_state >> + int (*pwrdm_pwrst_to_func)(struct powerdomain *pwrdm, u8 func_pwrst); > should return enum pwrdm_func_state > >> int (*pwrdm_set_next_pwrst)(struct powerdomain *pwrdm, u8 pwrst); >> int (*pwrdm_read_next_pwrst)(struct powerdomain *pwrdm); >> int (*pwrdm_read_pwrst)(struct powerdomain *pwrdm); >> @@ -174,21 +217,11 @@ int pwrdm_register_platform_funcs(struct pwrdm_ops *custom_funcs); >> int pwrdm_register_pwrdms(struct powerdomain **pwrdm_list); >> int pwrdm_complete_init(void); >> >> -struct powerdomain *pwrdm_lookup(const char *name); >> - >> -int pwrdm_for_each(int (*fn)(struct powerdomain *pwrdm, void *user), >> - void *user); >> -int pwrdm_for_each_nolock(int (*fn)(struct powerdomain *pwrdm, void *user), >> - void *user); >> - >> int pwrdm_add_clkdm(struct powerdomain *pwrdm, struct clockdomain *clkdm); >> int pwrdm_del_clkdm(struct powerdomain *pwrdm, struct clockdomain *clkdm); >> int pwrdm_for_each_clkdm(struct powerdomain *pwrdm, >> int (*fn)(struct powerdomain *pwrdm, >> struct clockdomain *clkdm)); >> -struct voltagedomain *pwrdm_get_voltdm(struct powerdomain *pwrdm); >> - >> -int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm); >> >> int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst); >> int pwrdm_read_next_pwrst(struct powerdomain *pwrdm); >> @@ -196,6 +229,9 @@ int pwrdm_read_pwrst(struct powerdomain *pwrdm); >> int pwrdm_read_prev_pwrst(struct powerdomain *pwrdm); >> int pwrdm_clear_all_prev_pwrst(struct powerdomain *pwrdm); >> >> +int omap2_pwrdm_func_to_pwrst(struct powerdomain *pwrdm, u8 func_pwrst); >> +int omap2_pwrdm_pwrst_to_func(struct powerdomain *pwrdm, u8 pwrst); >> + >> int pwrdm_set_logic_retst(struct powerdomain *pwrdm, u8 pwrst); >> int pwrdm_set_mem_onst(struct powerdomain *pwrdm, u8 bank, u8 pwrst); >> int pwrdm_set_mem_retst(struct powerdomain *pwrdm, u8 bank, u8 pwrst); >> @@ -220,11 +256,6 @@ int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm); >> int pwrdm_get_context_loss_count(struct powerdomain *pwrdm); >> bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm); >> >> -extern void omap242x_powerdomains_init(void); >> -extern void omap243x_powerdomains_init(void); >> -extern void omap3xxx_powerdomains_init(void); >> -extern void omap44xx_powerdomains_init(void); >> - >> extern struct pwrdm_ops omap2_pwrdm_operations; >> extern struct pwrdm_ops omap3_pwrdm_operations; >> extern struct pwrdm_ops omap4_pwrdm_operations; >> diff --git a/arch/arm/mach-omap2/powerdomain2xxx_3xxx.c b/arch/arm/mach-omap2/powerdomain2xxx_3xxx.c >> index 0f0a9f1..79c1293 100644 >> --- a/arch/arm/mach-omap2/powerdomain2xxx_3xxx.c >> +++ b/arch/arm/mach-omap2/powerdomain2xxx_3xxx.c >> @@ -26,6 +26,7 @@ >> >> >> /* Common functions across OMAP2 and OMAP3 */ >> + >> static int omap2_pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst) >> { >> omap2_prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK, >> @@ -210,6 +211,8 @@ static int omap3_pwrdm_disable_hdwr_sar(struct powerdomain *pwrdm) >> } >> >> struct pwrdm_ops omap2_pwrdm_operations = { >> + .pwrdm_func_to_pwrst = omap2_pwrdm_func_to_pwrst, >> + .pwrdm_pwrst_to_func = omap2_pwrdm_pwrst_to_func, >> .pwrdm_set_next_pwrst = omap2_pwrdm_set_next_pwrst, >> .pwrdm_read_next_pwrst = omap2_pwrdm_read_next_pwrst, >> .pwrdm_read_pwrst = omap2_pwrdm_read_pwrst, >> @@ -222,6 +225,8 @@ struct pwrdm_ops omap2_pwrdm_operations = { >> }; >> >> struct pwrdm_ops omap3_pwrdm_operations = { >> + .pwrdm_func_to_pwrst = omap2_pwrdm_func_to_pwrst, >> + .pwrdm_pwrst_to_func = omap2_pwrdm_pwrst_to_func, >> .pwrdm_set_next_pwrst = omap2_pwrdm_set_next_pwrst, >> .pwrdm_read_next_pwrst = omap2_pwrdm_read_next_pwrst, >> .pwrdm_read_pwrst = omap2_pwrdm_read_pwrst, >> diff --git a/arch/arm/mach-omap2/powerdomain44xx.c b/arch/arm/mach-omap2/powerdomain44xx.c >> index 601325b..538b528 100644 >> --- a/arch/arm/mach-omap2/powerdomain44xx.c >> +++ b/arch/arm/mach-omap2/powerdomain44xx.c >> @@ -209,6 +209,8 @@ static int omap4_pwrdm_wait_transition(struct powerdomain *pwrdm) >> } >> >> struct pwrdm_ops omap4_pwrdm_operations = { >> + .pwrdm_func_to_pwrst = omap2_pwrdm_func_to_pwrst, >> + .pwrdm_pwrst_to_func = omap2_pwrdm_pwrst_to_func, >> .pwrdm_set_next_pwrst = omap4_pwrdm_set_next_pwrst, >> .pwrdm_read_next_pwrst = omap4_pwrdm_read_next_pwrst, >> .pwrdm_read_pwrst = omap4_pwrdm_read_pwrst, > > otherwise, this is a nice concept Great! Thanks for the review. > -- > Regards, > Nishanth Menon Regards, Jean -- 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