Re: [PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states
Hi Nishant,
On Fri, Jul 13, 2012 at 5:01 AM, Menon, Nishanth n...@ti.com wrote:
On Thu, Jun 14, 2012 at 9:53 AM, Jean Pihet jean.pi...@newoldbits.com 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
Re: [PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states
On Fri, Jul 13, 2012 at 2:07 AM, Jean Pihet jean.pi...@newoldbits.com wrote: 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. Thanks for doing these, but with https://patchwork.kernel.org/patch/1160431/ in context, my comments on this patch is redundant and i think we should go with Rajendra's patch instead of this. I am currently dropping this patch in my internal tree. Will comment further as i get to next set of patches in this series. Regards, Nishanth Menon -- 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
Re: [PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states
Hi! On Fri, Jul 13, 2012 at 7:29 AM, Menon, Nishanth n...@ti.com wrote: On Fri, Jul 13, 2012 at 12:26 AM, Rajendra Nayak rna...@ti.com wrote: On Friday 13 July 2012 08:31 AM, Menon, Nishanth wrote: 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. Have you looked at an alternate approach that was proposed here.. https://patchwork.kernel.org/patch/1160431/ Santosh pointed me to the thread offline. This is indeed a much better approach IMHO than having 3 conflicting options inside powerdomain framework. After looking at the code and having sent my comments, I like it ... mainly because it is really similar to my proposal ;-p Can you elaborate more on 'having 3 conflicting options inside powerdomain framework'? Here are the main differences in the implementation: - the RFC code provides a _private header file, which forces the external users (cpuidle, pm.c etc.), - the RFC code still uses the same function names while my code renames them to '*_func_*'. This makes the code look more complicated than it really is. Regards, Nishanth Menon We are having a discussion on the best way to have the feature in. More to come! Thanks! Jean -- 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
Re: [PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states
On Fri, Jul 13, 2012 at 2:18 AM, Jean Pihet jean.pi...@newoldbits.com wrote: [..] Santosh pointed me to the thread offline. This is indeed a much better approach IMHO than having 3 conflicting options inside powerdomain framework. After looking at the code and having sent my comments, I like it ... mainly because it is really similar to my proposal ;-p Can you elaborate more on 'having 3 conflicting options inside powerdomain framework'? Current code has: a) PWRDM_POWER_XYZ - describe power state b) PWRSTS_XYZ -meant to describe supported states for each pwrdm this patch introduces: a) PWRDM_POWER_XYZ - describe power state (only for powerdomain*.[ch]) b) PWRSTS_XYZ -meant to describe supported states for each pwrdm c) PWRDM_FUNC_XYZ - for files other than powerdomain*.[ch] https://patchwork.kernel.org/patch/1160431/ maintains a) PWRDM_POWER_XYZ - describe power state b) PWRSTS_XYZ -meant to describe supported states for each pwrdm i) reduces code churn ii) supports logic pwr handling within existing framework iii) no conflicting usage beyond known definition and usage. (though personally, I;d like to see PWRSTS_XYZ dissappear into private header.. Here are the main differences in the implementation: - the RFC code provides a _private header file, which forces the external users (cpuidle, pm.c etc.), That is one of the reasons I like it. I need to have a code which will be maintained beyond the original code creators - reduced ability to mess up the code by not-well-informed developers is paramount importance for me. - the RFC code still uses the same function names while my code renames them to '*_func_*'. This makes the code look more complicated than it really is. True. But, it paves the way to move all functions that is not intended to be used beyond powerdomain files to a private power domain header - which achieves the same objective without code churn. Regards, Nishanth Menon -- 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
Re: [PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states
On Fri, Jul 13, 2012 at 12:26 AM, Rajendra Nayak rna...@ti.com wrote: On Friday 13 July 2012 08:31 AM, Menon, Nishanth wrote: 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. Have you looked at an alternate approach that was proposed here.. https://patchwork.kernel.org/patch/1160431/ Santosh pointed me to the thread offline. This is indeed a much better approach IMHO than having 3 conflicting options inside powerdomain framework. Regards, Nishanth Menon -- 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
Re: [PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states
Hi,
Here are some remarks I got after an internal review. I think those
points need to be discussed with a broader audience.
On Thu, Jun 14, 2012 at 4:53 PM, Jean Pihet jean.pi...@newoldbits.com wrote:
Introduce functional (or logical) states for power domains and the
API functions to read the power domains settings and to convert
between the functional (i.e. logical) and the internal (or registers)
values.
OMAP2, OMAP3 and OMAP4 platforms are defining a conversion routine.
In the new API only the function omap_set_pwrdm_state shall be used
to change a power domain target state, along with the associated
PWRDM_FUNC_* macros as the state parameters.
While at it the function is moved to the power domains code.
The power domain API in powerdomain.h is split in the external and the
internal parts; only the external API functions and defines shall be used
by external code, the internal API is only to be used in powerdomain*.[ch]
files.
The memory and logic states are not using the functional states, this
comes as a subsequent patch.
Signed-off-by: Jean Pihet j-pi...@ti.com
---
arch/arm/mach-omap2/pm.c | 66 ---
arch/arm/mach-omap2/pm.h | 1 -
arch/arm/mach-omap2/powerdomain-common.c | 71 +++
arch/arm/mach-omap2/powerdomain.c | 174
arch/arm/mach-omap2/powerdomain.h | 77 +
arch/arm/mach-omap2/powerdomain2xxx_3xxx.c | 5 +
arch/arm/mach-omap2/powerdomain44xx.c | 2 +
7 files changed, 306 insertions(+), 90 deletions(-)
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index a05f00c..dfe702b 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -74,10 +74,6 @@ static void __init omap2_init_processor_devices(void)
}
}
-/* Types of sleep_switch used in omap_set_pwrdm_state */
-#define FORCEWAKEUP_SWITCH 0
-#define LOWPOWERSTATE_SWITCH 1
-
int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
{
if (clkdm-flags CLKDM_CAN_ENABLE_AUTO)
@@ -89,68 +85,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm,
void *unused)
}
/*
- * This sets pwrdm state (other than mpu core. Currently only ON
- * RET are supported.
- */
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
-{
- u8 curr_pwrst, next_pwrst;
- int sleep_switch = -1, ret = 0, hwsup = 0;
-
- if (!pwrdm || IS_ERR(pwrdm))
- return -EINVAL;
-
- mutex_lock(pwrdm-lock);
-
- while (!(pwrdm-pwrsts (1 pwrst))) {
- if (pwrst == PWRDM_POWER_OFF)
- goto out;
- pwrst--;
- }
-
- 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;
-}
-
-
-
-/*
* This API is to be called during init to set the various voltage
* domains to the voltage as per the opp table. Typically we boot up
* at the nominal voltage. So this function finds out the rate of
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 7856489..5bc0848 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -18,7 +18,6 @@
extern void *omap3_secure_ram_storage;
extern void omap3_pm_off_mode_enable(int);
extern void omap_sram_idle(void);
-extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
extern int omap3_idle_init(void);
extern int omap4_idle_init(void);
extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
diff --git a/arch/arm/mach-omap2/powerdomain-common.c
b/arch/arm/mach-omap2/powerdomain-common.c
index
