[PATCH 2/8] ARM: OMAP2+: PM: introduce power domains functional states

Hi Nishant,

On Fri, Jul 13, 2012 at 5:01 AM, Menon, Nishanth [off-list ref] wrote:

On Thu, Jun 14, 2012 at 9:53 AM, Jean Pihet [off-list ref] wrote:
[..]

quoted

--- 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

quoted

+{
+       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?

quoted

+               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..

quoted

+{
+       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.

quoted

+               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

quoted

+{
+       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?

quoted

+
+       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

quoted

+{
+       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.

quoted

+
+       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.

quoted

+
+       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.

quoted

+       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

quoted

+{
+       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

quoted

+{
+       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.

quoted

+
+/**
  * 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

quoted

+{
+       int ret = pwrdm_read_prev_pwrst(pwrdm);
+
+       return pwrdm_pwrst_to_func(pwrdm, ret);
+}

again here.

quoted

+
+/**
  * 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]

quoted

+/***************************************************************
+ * 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

quoted

+       int     (*pwrdm_pwrst_to_func)(struct powerdomain *pwrdm, u8 func_pwrst);

should return enum pwrdm_func_state

quoted

        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