ARM: OMAP2+: second set of PM fixes and cleanup for 3.9

Fix the following sparse warnings in the OMAP3/4 CPUIdle code:

arch/arm/mach-omap2/cpuidle34xx.c:272:1: warning: symbol 'omap3_idle_dev' was not declared. Should it be static? arch/arm/mach-omap2/cpuidle34xx.c:274:23: warning: symbol 'omap3_idle_driver' was not declared. Should it be static? arch/arm/mach-omap2/cpuidle44xx.c:164:1: warning: symbol 'omap4_idle_dev' was not declared. Should it be static? arch/arm/mach-omap2/cpuidle44xx.c:166:23: warning: symbol 'omap4_idle_driver' was not declared. Should it be static?

Also fix the following checkpatch warnings:

WARNING: please, no space before tabs
#44: FILE: arch/arm/mach-omap2/cpuidle34xx.c:105:
+^I.name = ^I"omap3_idle",$

WARNING: please, no space before tabs
#45: FILE: arch/arm/mach-omap2/cpuidle34xx.c:106:
+^I.owner = ^ITHIS_MODULE,$

ERROR: code indent should use tabs where possible
#211: FILE: arch/arm/mach-omap2/cpuidle44xx.c:74:
+ /* C2 - CPU0 OFF + CPU1 OFF + MPU CSWR */$

I am using your "paul-linux-pwrdm_post_fpwrst_devel_a_3.9" branch, where all the patches which you have posted are present (I believe so) and I am getting following sparse warning -

Quoted 3 lines · Vaibhav Hiremath

I am using your "paul-linux-pwrdm_post_fpwrst_devel_a_3.9" branch, where all the patches which you have posted are present (I believe so) and I am getting following sparse warning -

The branch name to use is:

"TEST_pwrdm_post_fpwrst_devel_a_3.9"

- Paul

Quoted 4 lines · Paul Walmsley

The branch name to use is:

"TEST_pwrdm_post_fpwrst_devel_a_3.9"

If I am correct, it only includes one additional patch (merge commit), right???

commit d94831e0005fee743cefd28f4c20b7c435c71236
Merge: 3e885c6 80ab3b2
Author: Paul Walmsley <[email protected]>
Date: Sun Dec 9 13:06:51 2012 -0700

build fixes

Does this also fix sparse warnings?

Quoted 18 lines · Vaibhav

Quoted 4 lines · Paul Walmsley

The branch name to use is:

"TEST_pwrdm_post_fpwrst_devel_a_3.9"

If I am correct, it only includes one additional patch (merge commit), right???

commit d94831e0005fee743cefd28f4c20b7c435c71236
Merge: 3e885c6 80ab3b2
Author: Paul Walmsley <[email protected]>
Date: Sun Dec 9 13:06:51 2012 -0700

build fixes

Quoted 1 line

Does this also fix sparse warnings?

Just ran a quick sparse check on mach-omap2 at 3e885c6 and d94831e with 'make -j4 C=2 arch/arm/mach-omap2', and no warnings showed up. There were some sparse issues that got fixed at an earlier point, though, so perhaps you have an older copy of the branches somehow?

Fix the following sparse warnings in the OMAP3/4 CPUIdle code:

arch/arm/mach-omap2/cpuidle34xx.c:272:1: warning: symbol 'omap3_idle_dev' was not declared. Should it be static? arch/arm/mach-omap2/cpuidle34xx.c:274:23: warning: symbol 'omap3_idle_driver' was not declared. Should it be static? arch/arm/mach-omap2/cpuidle44xx.c:164:1: warning: symbol 'omap4_idle_dev' was not declared. Should it be static? arch/arm/mach-omap2/cpuidle44xx.c:166:23: warning: symbol 'omap4_idle_driver' was not declared. Should it be static?

Also fix the following checkpatch warnings:

WARNING: please, no space before tabs
#44: FILE: arch/arm/mach-omap2/cpuidle34xx.c:105:
+^I.name = ^I"omap3_idle",$

WARNING: please, no space before tabs
#45: FILE: arch/arm/mach-omap2/cpuidle34xx.c:106:
+^I.owner = ^ITHIS_MODULE,$

ERROR: code indent should use tabs where possible
#211: FILE: arch/arm/mach-omap2/cpuidle44xx.c:74:
+ /* C2 - CPU0 OFF + CPU1 OFF + MPU CSWR */$

Signed-off-by Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

---

Move omap_set_pwrdm_state() from the PM code to the powerdomain code, and refactor it to split it up into several functions. A subsequent patch will rename it to conform with the existing powerdomain function names.

Signed-off-by Paul Walmsley <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/pm.c          |   61 --------------------
 arch/arm/mach-omap2/pm.h          |    1
 arch/arm/mach-omap2/powerdomain.c |  112
+++++++++++++++++++++++++++----------
 arch/arm/mach-omap2/powerdomain.h |    3 +
 4 files changed, 85 insertions(+), 92 deletions(-)

arch/arm/mach-omap2/pm.c +0 -61

index cc8ed0f..2e2a897 100644
@@ -76,10 +76,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) &&

@@ -92,63 +88,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;
-
-       while (!(pwrdm->pwrsts & (1 << pwrst))) {
-               if (pwrst == PWRDM_POWER_OFF)
-                       return ret;
-               pwrst--;
-       }
-
-       next_pwrst = pwrdm_read_next_pwrst(pwrdm);
-       if (next_pwrst == pwrst)
-               return ret;
-
-       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_state_switch(pwrdm);
-               break;
-       }
-
-       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

arch/arm/mach-omap2/pm.h +0 -1

index 686137d..707e9cb 100644
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)
 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 omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
 extern int (*omap_pm_suspend)(void);
 

arch/arm/mach-omap2/powerdomain.c +0 -1

index 97b3881..05f00660 100644
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
        return (pwrdm && pwrdm->flags & PWRDM_HAS_HDWR_SAR) ? 1 : 0;
 }
 
-/**

arch/arm/mach-omap2/powerdomain.c +0 -18

-{
-       int ret = -EINVAL;
-
-       if (!pwrdm)
-               return -EINVAL;
-
-       if (!(pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE))
-               return -EINVAL;
-
-       pr_debug("powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n",
-                pwrdm->name);
-
-       if (arch_pwrdm && arch_pwrdm->pwrdm_set_lowpwrstchange)
-               ret = arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);
-
-       return ret;
-}
-
 int pwrdm_state_switch(struct powerdomain *pwrdm)
 {
        int ret;

arch/arm/mach-omap2/powerdomain.c +11 -0

+       u8 curr_pwrst, sleep_switch;
+
+       curr_pwrst = pwrdm_read_pwrst(pwrdm);
+       if (curr_pwrst < PWRDM_POWER_ON) {
+               if (curr_pwrst > pwrst &&
+                   pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
+                   arch_pwrdm->pwrdm_set_lowpwrstchange) {
+                       sleep_switch = LOWPOWERSTATE_SWITCH;
+               } else {
+                       *hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]);
+                       clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);

arch/arm/mach-omap2/powerdomain.c +14 -0

+static void _pwrdm_restore_clkdm_state(struct powerdomain *pwrdm,
+                                      u8 sleep_switch, bool hwsup)
+{
+       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:
+               if (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
+                   arch_pwrdm->pwrdm_set_lowpwrstchange)
+                       arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);

arch/arm/mach-omap2/powerdomain.c +25 -0

+int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
+{
+       u8 next_pwrst, sleep_switch;
+       int ret = 0;
+       bool hwsup = false;
+
+       if (!pwrdm || IS_ERR(pwrdm))
+               return -EINVAL;
+
+       while (!(pwrdm->pwrsts & (1 << pwrst))) {
+               if (pwrst == PWRDM_POWER_OFF)
+                       return ret;
+               pwrst--;
+       }
+
+       next_pwrst = pwrdm_read_next_pwrst(pwrdm);
+       if (next_pwrst == pwrst)
+               return ret;
+
+       sleep_switch = _pwrdm_save_clkdm_state_and_activate(pwrdm, pwrst,
+                                                           &hwsup);
+
+       ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
+       if (ret)
+               pr_err("%s: unable to set power state of powerdomain:

%s\n",

arch/arm/mach-omap2/powerdomain.c +7 -0

+                      __func__, pwrdm->name);
+
+       _pwrdm_restore_clkdm_state(pwrdm, sleep_switch, hwsup);
+
+       return ret;
+}
+
 /**
  * pwrdm_get_context_loss_count - get powerdomain's context loss count
  * @pwrdm: struct powerdomain * to wait for

arch/arm/mach-omap2/powerdomain.h +0 -0

index 7c1534b..1edb3b7 100644
@@ -228,10 +228,11 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
 int pwrdm_state_switch(struct powerdomain *pwrdm);
 int pwrdm_pre_transition(struct powerdomain *pwrdm);

Quoted 11 lines · Paul Walmsley

-/**
- * pwrdm_set_lowpwrstchange - Request a low power state change
- * @pwrdm: struct powerdomain *
- *
- * Allows a powerdomain to transtion to a lower power sleep state
- * from an existing sleep state without waking up the powerdomain.
- * Returns -EINVAL if the powerdomain pointer is null or if the
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
- * upon success.
- */

Can this kerneldoc be reused in the new code?

Quoted 4 lines

(quoted diff) +0 -0

@@ -984,6 +955,89 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)
        return 0;
 }
 

Review int pwrdm_post_transition(struct powerdomain *pwrdm) · line 962

Quoted 5 lines

(quoted diff) +4 -0

+/* Types of sleep_switch used in omap_set_pwrdm_state */
+#define ALREADYACTIVE_SWITCH   0
+#define FORCEWAKEUP_SWITCH     1
+#define LOWPOWERSTATE_SWITCH   2
 

Could you add some more documentation here?
What is the goal of the function, what does it return etc. ?

Review int pwrdm_post_transition(struct powerdomain *pwrdm) · line 965

Quoted 3 lines

(quoted diff) +3 -0

+
+static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
+                                              u8 pwrst, bool *hwsup)

Same here

Review int pwrdm_post_transition(struct powerdomain *pwrdm) · line 967

Quoted 2 lines

(quoted diff) +2 -0

+static void _pwrdm_restore_clkdm_state(struct powerdomain *pwrdm,
+                                      u8 sleep_switch, bool hwsup)

Same here.

Review int pwrdm_post_transition(struct powerdomain *pwrdm) · line 972

Quoted 5 lines

(quoted diff) +4 -0

+/*
+ * This sets pwrdm state (other than mpu & core. Currently only ON &
+ * RET are supported.
+ */
 

Is this one correct?

Move omap_set_pwrdm_state() from the PM code to the powerdomain code, and refactor it to split it up into several functions. A subsequent patch will rename it to conform with the existing powerdomain function names.

Signed-off-by Paul Walmsley <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/pm.c          |   61 --------------------
 arch/arm/mach-omap2/pm.h          |    1 
 arch/arm/mach-omap2/powerdomain.c |  112 +++++++++++++++++++++++++++----------
 arch/arm/mach-omap2/powerdomain.h |    3 +
 4 files changed, 85 insertions(+), 92 deletions(-)

arch/arm/mach-omap2/pm.c +0 -4

index cc8ed0f..2e2a897 100644
@@ -76,10 +76,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) &&

@@ -92,63 +88,6 @@ int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
 }
 

arch/arm/mach-omap2/pm.c +0 -48

-		return -EINVAL;
-
-	while (!(pwrdm->pwrsts & (1 << pwrst))) {
-		if (pwrst == PWRDM_POWER_OFF)
-			return ret;
-		pwrst--;
-	}
-
-	next_pwrst = pwrdm_read_next_pwrst(pwrdm);
-	if (next_pwrst == pwrst)
-		return ret;
-
-	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_state_switch(pwrdm);
-		break;
-	}
-
-	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

arch/arm/mach-omap2/pm.h +0 -1

index 686137d..707e9cb 100644
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)
 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 omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
 extern int (*omap_pm_suspend)(void);
 

arch/arm/mach-omap2/powerdomain.c +83 -29

index 97b3881..05f00660 100644
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
 	return (pwrdm && pwrdm->flags & PWRDM_HAS_HDWR_SAR) ? 1 : 0;
 }
 
-/**
- * pwrdm_set_lowpwrstchange - Request a low power state change
- * @pwrdm: struct powerdomain *
- *
- * Allows a powerdomain to transtion to a lower power sleep state
- * from an existing sleep state without waking up the powerdomain.
- * Returns -EINVAL if the powerdomain pointer is null or if the
- * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
- * upon success.
- */
-int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm)
-{
-	int ret = -EINVAL;
-
-	if (!pwrdm)
-		return -EINVAL;
-
-	if (!(pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE))
-		return -EINVAL;
-
-	pr_debug("powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n",
-		 pwrdm->name);
-
-	if (arch_pwrdm && arch_pwrdm->pwrdm_set_lowpwrstchange)
-		ret = arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);
-
-	return ret;
-}
-
 int pwrdm_state_switch(struct powerdomain *pwrdm)
 {
 	int ret;

@@ -984,6 +955,89 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)
 	return 0;
 }
 
+/* Types of sleep_switch used in omap_set_pwrdm_state */
+#define ALREADYACTIVE_SWITCH	0
+#define FORCEWAKEUP_SWITCH	1
+#define LOWPOWERSTATE_SWITCH	2
+
+static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
+					       u8 pwrst, bool *hwsup)
+{
+	u8 curr_pwrst, sleep_switch;
+
+	curr_pwrst = pwrdm_read_pwrst(pwrdm);
+	if (curr_pwrst < PWRDM_POWER_ON) {
+		if (curr_pwrst > pwrst &&
+		    pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
+		    arch_pwrdm->pwrdm_set_lowpwrstchange) {
+			sleep_switch = LOWPOWERSTATE_SWITCH;
+		} else {
+			*hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]);
+			clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
+			sleep_switch = FORCEWAKEUP_SWITCH;
+		}
+	} else {
+		sleep_switch = ALREADYACTIVE_SWITCH;
+	}
+
+	return sleep_switch;
+}
+
+static void _pwrdm_restore_clkdm_state(struct powerdomain *pwrdm,
+				       u8 sleep_switch, bool hwsup)
+{
+	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:
+		if (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
+		    arch_pwrdm->pwrdm_set_lowpwrstchange)
+			arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);
+		pwrdm_state_switch(pwrdm);
+		break;
+	}
+}
+
+/*
+ * This sets pwrdm state (other than mpu & core. Currently only ON &
+ * RET are supported.
+ */
+int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
+{
+	u8 next_pwrst, sleep_switch;
+	int ret = 0;
+	bool hwsup = false;
+
+	if (!pwrdm || IS_ERR(pwrdm))
+		return -EINVAL;
+
+	while (!(pwrdm->pwrsts & (1 << pwrst))) {
+		if (pwrst == PWRDM_POWER_OFF)
+			return ret;
+		pwrst--;
+	}
+
+	next_pwrst = pwrdm_read_next_pwrst(pwrdm);
+	if (next_pwrst == pwrst)
+		return ret;
+
+	sleep_switch = _pwrdm_save_clkdm_state_and_activate(pwrdm, pwrst,
+							    &hwsup);
+
+	ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
+	if (ret)
+		pr_err("%s: unable to set power state of powerdomain: %s\n",
+		       __func__, pwrdm->name);
+
+	_pwrdm_restore_clkdm_state(pwrdm, sleep_switch, hwsup);
+
+	return ret;
+}
+
 /**
  * pwrdm_get_context_loss_count - get powerdomain's context loss count
  * @pwrdm: struct powerdomain * to wait for

arch/arm/mach-omap2/powerdomain.h +2 -1

index 7c1534b..1edb3b7 100644
@@ -228,10 +228,11 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
 int pwrdm_state_switch(struct powerdomain *pwrdm);
 int pwrdm_pre_transition(struct powerdomain *pwrdm);
 int pwrdm_post_transition(struct powerdomain *pwrdm);
-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 int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 state);
+
 extern void omap242x_powerdomains_init(void);
 extern void omap243x_powerdomains_init(void);
 extern void omap3xxx_powerdomains_init(void);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

arch/arm/mach-omap2/pm.c int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused) · line 103

Quoted 6 lines

arch/arm/mach-omap2/pm.c +0 -6

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

You can use IS_ERR_OR_NULL here.

Move omap_set_pwrdm_state() from the PM code to the powerdomain code, and refactor it to split it up into several functions. A subsequent patch will rename it to conform with the existing powerdomain function names.

Signed-off-by Paul Walmsley <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/pm.c          |   61 --------------------
 arch/arm/mach-omap2/pm.h          |    1
 arch/arm/mach-omap2/powerdomain.c |  112 +++++++++++++++++++++++++++----------
 arch/arm/mach-omap2/powerdomain.h |    3 +
 4 files changed, 85 insertions(+), 92 deletions(-)

arch/arm/mach-omap2/pm.c +0 -61

index cc8ed0f..2e2a897 100644
@@ -76,10 +76,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) &&

@@ -92,63 +88,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;
-
-       while (!(pwrdm->pwrsts & (1 << pwrst))) {
-               if (pwrst == PWRDM_POWER_OFF)
-                       return ret;
-               pwrst--;
-       }
-
-       next_pwrst = pwrdm_read_next_pwrst(pwrdm);
-       if (next_pwrst == pwrst)
-               return ret;
-
-       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_state_switch(pwrdm);
-               break;
-       }
-
-       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

arch/arm/mach-omap2/pm.h +0 -1

index 686137d..707e9cb 100644
@@ -33,7 +33,6 @@ static inline int omap4_idle_init(void)
 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 omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
 extern int (*omap_pm_suspend)(void);
 

arch/arm/mach-omap2/powerdomain.c +0 -0

index 97b3881..05f00660 100644
@@ -921,35 +921,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
        return (pwrdm && pwrdm->flags & PWRDM_HAS_HDWR_SAR) ? 1 : 0;
 }
 

arch/arm/mach-omap2/powerdomain.c +14 -0

+
+static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
+                                              u8 pwrst, bool *hwsup)
 
+{
+       u8 curr_pwrst, sleep_switch;
+
+       curr_pwrst = pwrdm_read_pwrst(pwrdm);
+       if (curr_pwrst < PWRDM_POWER_ON) {
+               if (curr_pwrst > pwrst &&
+                   pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
+                   arch_pwrdm->pwrdm_set_lowpwrstchange) {
+                       sleep_switch = LOWPOWERSTATE_SWITCH;
+               } else {
+                       *hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]);

arch/arm/mach-omap2/powerdomain.c +13 -0

+static void _pwrdm_restore_clkdm_state(struct powerdomain *pwrdm,
+                                      u8 sleep_switch, bool hwsup)
+{
+       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:
+               if (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
+                   arch_pwrdm->pwrdm_set_lowpwrstchange)

activate(pwrdm, pwrst,

arch/arm/mach-omap2/powerdomain.c +12 -0

+                                                           &hwsup);
+
+       ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
+       if (ret)
+               pr_err("%s: unable to set power state of powerdomain: %s\n",
+                      __func__, pwrdm->name);
+
+       _pwrdm_restore_clkdm_state(pwrdm, sleep_switch, hwsup);
+
+       return ret;
+}
+
 /**
  * pwrdm_get_context_loss_count - get powerdomain's context loss count
  * @pwrdm: struct powerdomain * to wait for

arch/arm/mach-omap2/powerdomain.h +2 -1

index 7c1534b..1edb3b7 100644
@@ -228,10 +228,11 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
 int pwrdm_state_switch(struct powerdomain *pwrdm);
 int pwrdm_pre_transition(struct powerdomain *pwrdm);
 int pwrdm_post_transition(struct powerdomain *pwrdm);
-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 int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 state);
+
 extern void omap242x_powerdomains_init(void);
 extern void omap243x_powerdomains_init(void);
 extern void omap3xxx_powerdomains_init(void);

Add a per-powerdomain spinlock. Use that instead of the clockdomain spinlock. Add pwrdm_lock()/pwrdm_unlock() functions to allow other code to acquire or release the powerdomain spinlock without reaching directly into the struct powerdomain.

Signed-off-by Paul Walmsley <[email protected]>

Cc Jean Pihet <[email protected]>

---
 arch/arm/mach-omap2/clockdomain-powerdomain.h |   22 +++
 arch/arm/mach-omap2/clockdomain.c             |  161
+++++++++++++++++--------
 arch/arm/mach-omap2/clockdomain.h             |    6 +
 arch/arm/mach-omap2/powerdomain-clockdomain.h |   27 ++++
 arch/arm/mach-omap2/powerdomain.c             |   56 ++++++++-
 arch/arm/mach-omap2/powerdomain.h             |   11 +-
 6 files changed, 219 insertions(+), 64 deletions(-)
 create mode 100644 arch/arm/mach-omap2/clockdomain-powerdomain.h
 create mode 100644 arch/arm/mach-omap2/powerdomain-clockdomain.h

arch/arm/mach-omap2/clockdomain-powerdomain.h new file +2 -0

new file mode 100644
index 0000000..8beee2d
@@ -0,0 +1,22 @@
+/*
+ * OMAP clockdomain framework functions for use only by the powerdomain

code
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ * Paul Walmsley
+ *
+ * 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.
+ */

arch/arm/mach-omap2/clockdomain-powerdomain.h new file +12 -0

+
+#ifndef __ARCH_ARM_MACH_OMAP2_CLOCKDOMAIN_POWERDOMAIN_H
+#define __ARCH_ARM_MACH_OMAP2_CLOCKDOMAIN_POWERDOMAIN_H
+
+#include "clockdomain.h"
+
+extern void clkdm_allow_idle_nolock(struct clockdomain *clkdm);
+extern void clkdm_deny_idle_nolock(struct clockdomain *clkdm);
+extern int clkdm_wakeup_nolock(struct clockdomain *clkdm);
+extern int clkdm_sleep_nolock(struct clockdomain *clkdm);
+
+#endif

arch/arm/mach-omap2/clockdomain.c +6 -8

index 18f65fd..2142dab 100644
@@ -30,6 +30,7 @@
 #include "soc.h"
 #include "clock.h"
 #include "clockdomain.h"
+#include "powerdomain-clockdomain.h"
 
 /* clkdm_list contains all registered struct clockdomains */
 static LIST_HEAD(clkdm_list);

@@ -91,8 +92,6 @@ static int _clkdm_register(struct clockdomain *clkdm)
 
        pwrdm_add_clkdm(pwrdm, clkdm);
 
-       spin_lock_init(&clkdm->lock);
-
        pr_debug("clockdomain: registered %s\n", clkdm->name);
 
        return 0;

@@ -733,18 +732,17 @@ int clkdm_clear_all_sleepdeps(struct clockdomain
 *clkdm)
 }
 
 /**
- * clkdm_sleep - force clockdomain sleep transition
+ * clkdm_sleep_nolock - force clockdomain sleep transition (lockless)
  * @clkdm: struct clockdomain *
  *
  * Instruct the CM to force a sleep transition on the specified
- * clockdomain @clkdm.  Returns -EINVAL if @clkdm is NULL or if
- * clockdomain does not support software-initiated sleep; 0 upon
- * success.
+ * clockdomain @clkdm.  Only for use by the powerdomain code.  Returns
+ * -EINVAL if @clkdm is NULL or if clockdomain does not support
+ * software-initiated sleep; 0 upon success.
  */
-int clkdm_sleep(struct clockdomain *clkdm)
+int clkdm_sleep_nolock(struct clockdomain *clkdm)
 {
        int ret;
-       unsigned long flags;
 
        if (!clkdm)

                return -EINVAL;

arch/arm/mach-omap2/clockdomain.c +73 -22

@@ -760,27 +758,45 @@ int clkdm_sleep(struct clockdomain *clkdm)
 
        pr_debug("clockdomain: forcing sleep on %s\n", clkdm->name);
 
-       spin_lock_irqsave(&clkdm->lock, flags);
        clkdm->_flags &= ~_CLKDM_FLAG_HWSUP_ENABLED;
        ret = arch_clkdm->clkdm_sleep(clkdm);
-       ret |= pwrdm_state_switch(clkdm->pwrdm.ptr);
-       spin_unlock_irqrestore(&clkdm->lock, flags);
+       ret |= pwrdm_state_switch_nolock(clkdm->pwrdm.ptr);
+
        return ret;
 }
 
 /**
- * clkdm_wakeup - force clockdomain wakeup transition
+ * clkdm_sleep - force clockdomain sleep transition
  * @clkdm: struct clockdomain *
  *
- * Instruct the CM to force a wakeup transition on the specified
- * clockdomain @clkdm.  Returns -EINVAL if @clkdm is NULL or if the
- * clockdomain does not support software-controlled wakeup; 0 upon
+ * Instruct the CM to force a sleep transition on the specified
+ * clockdomain @clkdm.  Returns -EINVAL if @clkdm is NULL or if
+ * clockdomain does not support software-initiated sleep; 0 upon
  * success.
  */
-int clkdm_wakeup(struct clockdomain *clkdm)
+int clkdm_sleep(struct clockdomain *clkdm)
+{
+       int ret;
+
+       pwrdm_lock(clkdm->pwrdm.ptr);
+       ret = clkdm_sleep_nolock(clkdm);
+       pwrdm_unlock(clkdm->pwrdm.ptr);
+
+       return ret;
+}
+
+/**
+ * clkdm_wakeup_nolock - force clockdomain wakeup transition (lockless)
+ * @clkdm: struct clockdomain *
+ *
+ * Instruct the CM to force a wakeup transition on the specified
+ * clockdomain @clkdm.  Only for use by the powerdomain code.  Returns
+ * -EINVAL if @clkdm is NULL or if the clockdomain does not support
+ * software-controlled wakeup; 0 upon success.
+ */
+int clkdm_wakeup_nolock(struct clockdomain *clkdm)
 {
        int ret;
-       unsigned long flags;
 
        if (!clkdm)
                return -EINVAL;

@@ -796,28 +812,46 @@ int clkdm_wakeup(struct clockdomain *clkdm)
 
        pr_debug("clockdomain: forcing wakeup on %s\n", clkdm->name);
 
-       spin_lock_irqsave(&clkdm->lock, flags);
        clkdm->_flags &= ~_CLKDM_FLAG_HWSUP_ENABLED;
        ret = arch_clkdm->clkdm_wakeup(clkdm);
-       ret |= pwrdm_state_switch(clkdm->pwrdm.ptr);
-       spin_unlock_irqrestore(&clkdm->lock, flags);
+       ret |= pwrdm_state_switch_nolock(clkdm->pwrdm.ptr);
+
        return ret;
 }
 
 /**
- * clkdm_allow_idle - enable hwsup idle transitions for clkdm
+ * clkdm_wakeup - force clockdomain wakeup transition
  * @clkdm: struct clockdomain *
  *
- * Allow the hardware to automatically switch the clockdomain @clkdm into
- * active or idle states, as needed by downstream clocks.  If the
+ * Instruct the CM to force a wakeup transition on the specified
+ * clockdomain @clkdm.  Returns -EINVAL if @clkdm is NULL or if the
+ * clockdomain does not support software-controlled wakeup; 0 upon
+ * success.
+ */
+int clkdm_wakeup(struct clockdomain *clkdm)
+{
+       int ret;
+
+       pwrdm_lock(clkdm->pwrdm.ptr);
+       ret = clkdm_wakeup_nolock(clkdm);
+       pwrdm_unlock(clkdm->pwrdm.ptr);
+
+       return ret;
+}
+
+/**
+ * clkdm_allow_idle_nolock - enable hwsup idle transitions for clkdm
+ * @clkdm: struct clockdomain *
+ *
+ * Allow the hardware to automatically switch the clockdomain @clkdm
+ * into active or idle states, as needed by downstream clocks.  If the
  * clockdomain has any downstream clocks enabled in the clock
  * framework, wkdep/sleepdep autodependencies are added; this is so
- * device drivers can read and write to the device.  No return value.
+ * device drivers can read and write to the device.  Only for use by
+ * the powerdomain code.  No return value.
  */
-void clkdm_allow_idle(struct clockdomain *clkdm)
+void clkdm_allow_idle_nolock(struct clockdomain *clkdm)
 {
-       unsigned long flags;
-
        if (!clkdm)
                return;
 

@@ -833,11 +867,26 @@ void clkdm_allow_idle(struct clockdomain *clkdm)
        pr_debug("clockdomain: enabling automatic idle transitions for
 %s\n",
                 clkdm->name);
 
-       spin_lock_irqsave(&clkdm->lock, flags);
        clkdm->_flags |= _CLKDM_FLAG_HWSUP_ENABLED;
        arch_clkdm->clkdm_allow_idle(clkdm);
-       pwrdm_state_switch(clkdm->pwrdm.ptr);
-       spin_unlock_irqrestore(&clkdm->lock, flags);
+       pwrdm_state_switch_nolock(clkdm->pwrdm.ptr);
+}
+
+/**
+ * clkdm_allow_idle - enable hwsup idle transitions for clkdm
+ * @clkdm: struct clockdomain *
+ *
+ * Allow the hardware to automatically switch the clockdomain @clkdm into
+ * active or idle states, as needed by downstream clocks.  If the
+ * clockdomain has any downstream clocks enabled in the clock
+ * framework, wkdep/sleepdep autodependencies are added; this is so
+ * device drivers can read and write to the device.  No return value.
+ */
+void clkdm_allow_idle(struct clockdomain *clkdm)
+{
+       pwrdm_lock(clkdm->pwrdm.ptr);
+       clkdm_allow_idle_nolock(clkdm);
+       pwrdm_unlock(clkdm->pwrdm.ptr);
 }
 

/**

arch/arm/mach-omap2/clockdomain.c +20 -7

@@ -847,12 +896,11 @@ void clkdm_allow_idle(struct clockdomain *clkdm)
  * Prevent the hardware from automatically switching the clockdomain
  * @clkdm into inactive or idle states.  If the clockdomain has
  * downstream clocks enabled in the clock framework, wkdep/sleepdep
- * autodependencies are removed.  No return value.
+ * autodependencies are removed.  Only for use by the powerdomain
+ * code.  No return value.
  */
-void clkdm_deny_idle(struct clockdomain *clkdm)
+void clkdm_deny_idle_nolock(struct clockdomain *clkdm)
 {
-       unsigned long flags;
-
        if (!clkdm)
                return;
 

@@ -868,11 +916,25 @@ void clkdm_deny_idle(struct clockdomain *clkdm)
        pr_debug("clockdomain: disabling automatic idle transitions for
 %s\n",
                 clkdm->name);
 
-       spin_lock_irqsave(&clkdm->lock, flags);
        clkdm->_flags &= ~_CLKDM_FLAG_HWSUP_ENABLED;
        arch_clkdm->clkdm_deny_idle(clkdm);
-       pwrdm_state_switch(clkdm->pwrdm.ptr);
-       spin_unlock_irqrestore(&clkdm->lock, flags);
+       pwrdm_state_switch_nolock(clkdm->pwrdm.ptr);
+}
+
+/**
+ * clkdm_deny_idle - disable hwsup idle transitions for clkdm
+ * @clkdm: struct clockdomain *
+ *
+ * Prevent the hardware from automatically switching the clockdomain
+ * @clkdm into inactive or idle states.  If the clockdomain has
+ * downstream clocks enabled in the clock framework, wkdep/sleepdep
+ * autodependencies are removed.  No return value.
+ */
+void clkdm_deny_idle(struct clockdomain *clkdm)
+{
+       pwrdm_lock(clkdm->pwrdm.ptr);
+       clkdm_deny_idle_nolock(clkdm);
+       pwrdm_unlock(clkdm->pwrdm.ptr);
 }
 

/**

arch/arm/mach-omap2/clockdomain.c +1 -6

@@ -889,14 +951,11 @@ void clkdm_deny_idle(struct clockdomain *clkdm)
 bool clkdm_in_hwsup(struct clockdomain *clkdm)
 {
        bool ret;
-       unsigned long flags;
 
        if (!clkdm)
                return false;
 
-       spin_lock_irqsave(&clkdm->lock, flags);
        ret = (clkdm->_flags & _CLKDM_FLAG_HWSUP_ENABLED) ? true : false;
-       spin_unlock_irqrestore(&clkdm->lock, flags);
 
        return ret;
 }

@@ -922,12 +981,10 @@ bool clkdm_missing_idle_reporting(struct clockdomain
 *clkdm)
 
 static int _clkdm_clk_hwmod_enable(struct clockdomain *clkdm)
 {
-       unsigned long flags;
-
        if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_enable)
                return -EINVAL;
 
-       spin_lock_irqsave(&clkdm->lock, flags);
+       pwrdm_lock(clkdm->pwrdm.ptr);
 
        /*

* For arch's with no autodeps, clkcm_clk_enable

arch/arm/mach-omap2/clockdomain.c +8 -10

@@ -935,13 +992,13 @@ static int _clkdm_clk_hwmod_enable(struct
 clockdomain *clkdm)
         * enabled, so the clkdm can be force woken up.
         */
        if ((atomic_inc_return(&clkdm->usecount) > 1) && autodeps) {
-               spin_unlock_irqrestore(&clkdm->lock, flags);
+               pwrdm_unlock(clkdm->pwrdm.ptr);
                return 0;
        }
 
        arch_clkdm->clkdm_clk_enable(clkdm);
-       pwrdm_state_switch(clkdm->pwrdm.ptr);
-       spin_unlock_irqrestore(&clkdm->lock, flags);
+       pwrdm_state_switch_nolock(clkdm->pwrdm.ptr);
+       pwrdm_unlock(clkdm->pwrdm.ptr);
 
        pr_debug("clockdomain: %s: enabled\n", clkdm->name);

@@ -950,27 +1007,25 @@ static int _clkdm_clk_hwmod_enable(struct
 clockdomain *clkdm)
 
 static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm)
 {
-       unsigned long flags;
-
        if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_disable)
                return -EINVAL;
 
-       spin_lock_irqsave(&clkdm->lock, flags);
+       pwrdm_lock(clkdm->pwrdm.ptr);
 
        if (atomic_read(&clkdm->usecount) == 0) {
-               spin_unlock_irqrestore(&clkdm->lock, flags);
+               pwrdm_unlock(clkdm->pwrdm.ptr);
                WARN_ON(1); /* underflow */
                return -ERANGE;
        }
 
        if (atomic_dec_return(&clkdm->usecount) > 0) {
-               spin_unlock_irqrestore(&clkdm->lock, flags);
+               pwrdm_unlock(clkdm->pwrdm.ptr);
                return 0;
        }
 
        arch_clkdm->clkdm_clk_disable(clkdm);
-       pwrdm_state_switch(clkdm->pwrdm.ptr);
-       spin_unlock_irqrestore(&clkdm->lock, flags);
+       pwrdm_state_switch_nolock(clkdm->pwrdm.ptr);
+       pwrdm_unlock(clkdm->pwrdm.ptr);
 
        pr_debug("clockdomain: %s: disabled\n", clkdm->name);

arch/arm/mach-omap2/clockdomain.h +4 -2

index bc42446..e7f1b4b 100644
@@ -15,7 +15,6 @@
 #define __ARCH_ARM_MACH_OMAP2_CLOCKDOMAIN_H
 
 #include <linux/init.h>
-#include <linux/spinlock.h>
 
 #include "powerdomain.h"
 #include "clock.h"

@@ -139,7 +138,6 @@ struct clockdomain {
        struct clkdm_dep *sleepdep_srcs;
        atomic_t usecount;
        struct list_head node;
-       spinlock_t lock;
 };
 
 /**

@@ -196,12 +194,16 @@ int clkdm_del_sleepdep(struct clockdomain *clkdm1,
 struct clockdomain *clkdm2);
 int clkdm_read_sleepdep(struct clockdomain *clkdm1, struct clockdomain
 *clkdm2);
 int clkdm_clear_all_sleepdeps(struct clockdomain *clkdm);
 
+void clkdm_allow_idle_nolock(struct clockdomain *clkdm);
 void clkdm_allow_idle(struct clockdomain *clkdm);
+void clkdm_deny_idle_nolock(struct clockdomain *clkdm);
 void clkdm_deny_idle(struct clockdomain *clkdm);
 bool clkdm_in_hwsup(struct clockdomain *clkdm);
 bool clkdm_missing_idle_reporting(struct clockdomain *clkdm);
 
+int clkdm_wakeup_nolock(struct clockdomain *clkdm);
 int clkdm_wakeup(struct clockdomain *clkdm);
+int clkdm_sleep_nolock(struct clockdomain *clkdm);
 int clkdm_sleep(struct clockdomain *clkdm);

int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk);

arch/arm/mach-omap2/powerdomain-clockdomain.h new file +27 -0

new file mode 100644
index 0000000..e404eec
@@ -0,0 +1,27 @@
+/*
+ * OMAP powerdomain functions only for use by the OMAP clockdomain code
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ * Paul Walmsley
+ *
+ * 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.
+ *
+ * The point of this file is to try to prevent other code outside of
+ * clockdomain.c from calling the functions listed herein.  Yes, this
+ * means you.
+ */
+
+#ifndef __ARCH_ARM_MACH_OMAP2_POWERDOMAIN_CLOCKDOMAIN_H
+#define __ARCH_ARM_MACH_OMAP2_POWERDOMAIN_CLOCKDOMAIN_H
+
+#include <linux/types.h>
+
+#include "powerdomain.h"
+
+extern void pwrdm_lock(struct powerdomain *pwrdm);
+extern void pwrdm_unlock(struct powerdomain *pwrdm);
+extern int pwrdm_state_switch_nolock(struct powerdomain *pwrdm);
+
+#endif

arch/arm/mach-omap2/powerdomain.c +40 -2

index 05f00660..2a5f15b 100644
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/errno.h>
 #include <linux/string.h>
+#include <linux/spinlock.h>
 #include <trace/events/power.h>
 
 #include "cm2xxx_3xxx.h"

@@ -31,6 +32,7 @@
 
 #include "powerdomain.h"
 #include "clockdomain.h"
+#include "clockdomain-powerdomain.h"
 
 #include "soc.h"
 #include "pm.h"

@@ -101,6 +103,7 @@ static int _pwrdm_register(struct powerdomain *pwrdm)
        pwrdm->voltdm.ptr = voltdm;
        INIT_LIST_HEAD(&pwrdm->voltdm_node);
        voltdm_add_pwrdm(voltdm, pwrdm);
+       spin_lock_init(&pwrdm->_lock);
 
        list_add(&pwrdm->node, &pwrdm_list);
 

@@ -276,6 +279,30 @@ int pwrdm_complete_init(void)
 }
 
 /**
+ * pwrdm_lock - acquire a Linux spinlock on a powerdomain
+ * @pwrdm: struct powerdomain * to lock
+ *
+ * Acquire the powerdomain spinlock on @pwrdm.  No return value.
+ */
+void pwrdm_lock(struct powerdomain *pwrdm)
+       __acquires(&pwrdm->_lock)
+{
+       spin_lock_irqsave(&pwrdm->_lock, pwrdm->_lock_flags);
+}
+
+/**
+ * pwrdm_unlock - release a Linux spinlock on a powerdomain
+ * @pwrdm: struct powerdomain * to unlock
+ *
+ * Release the powerdomain spinlock on @pwrdm.  No return value.
+ */
+void pwrdm_unlock(struct powerdomain *pwrdm)
+       __releases(&pwrdm->_lock)
+{
+       spin_unlock_irqrestore(&pwrdm->_lock, pwrdm->_lock_flags);
+}
+
+/**
  * pwrdm_lookup - look up a powerdomain by name, return a pointer
  * @name: name of powerdomain
  *

@@ -921,7 +948,7 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
        return (pwrdm && pwrdm->flags & PWRDM_HAS_HDWR_SAR) ? 1 : 0;
 }
 
-int pwrdm_state_switch(struct powerdomain *pwrdm)
+int pwrdm_state_switch_nolock(struct powerdomain *pwrdm)
 {
        int ret;
 

@@ -935,6 +962,17 @@ int pwrdm_state_switch(struct powerdomain *pwrdm)
        return ret;
 }
 
+int __deprecated pwrdm_state_switch(struct powerdomain *pwrdm)
+{
+       int ret;
+
+       pwrdm_lock(pwrdm);
+       ret = pwrdm_state_switch_nolock(pwrdm);
+       pwrdm_unlock(pwrdm);
+
+       return ret;
+}
+
 int pwrdm_pre_transition(struct powerdomain *pwrdm)
 {
        if (pwrdm)

@@ -973,7 +1011,7 @@ static u8 _pwrdm_save_clkdm_state_and_activate(struct
 powerdomain *pwrdm,
                        sleep_switch = LOWPOWERSTATE_SWITCH;
                } else {
                        *hwsup = clkdm_in_hwsup(pwrdm->pwrdm_clkdms[0]);
-                       clkdm_wakeup(pwrdm->pwrdm_clkdms[0]);
+                       clkdm_wakeup_nolock(pwrdm->pwrdm_clkdms[0]);
                        sleep_switch = FORCEWAKEUP_SWITCH;
                }

        } else {

arch/arm/mach-omap2/powerdomain.c +3 -3

@@ -989,15 +1027,15 @@ static void _pwrdm_restore_clkdm_state(struct
 powerdomain *pwrdm,
        switch (sleep_switch) {
        case FORCEWAKEUP_SWITCH:
                if (hwsup)
-                       clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]);
+                       clkdm_allow_idle_nolock(pwrdm->pwrdm_clkdms[0]);
                else
-                       clkdm_sleep(pwrdm->pwrdm_clkdms[0]);
+                       clkdm_sleep_nolock(pwrdm->pwrdm_clkdms[0]);
                break;
        case LOWPOWERSTATE_SWITCH:
                if (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
                    arch_pwrdm->pwrdm_set_lowpwrstchange)
                        arch_pwrdm->pwrdm_set_lowpwrstchange(pwrdm);
-               pwrdm_state_switch(pwrdm);
+               pwrdm_state_switch_nolock(pwrdm);
                break;
        }

}

arch/arm/mach-omap2/powerdomain.c +5 -1

@@ -1021,9 +1059,13 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm,
 u8 pwrst)
                pwrst--;
        }
 
+       pwrdm_lock(pwrdm);
+
        next_pwrst = pwrdm_read_next_pwrst(pwrdm);
-       if (next_pwrst == pwrst)
+       if (next_pwrst == pwrst) {
+               pwrdm_unlock(pwrdm);
                return ret;
+       }
 
        sleep_switch = _pwrdm_save_clkdm_state_and_activate(pwrdm, pwrst,

                                                            &hwsup);

arch/arm/mach-omap2/powerdomain.c +2 -0

@@ -1035,6 +1077,8 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm,
 u8 pwrst)
 
        _pwrdm_restore_clkdm_state(pwrdm, sleep_switch, hwsup);
 
+       pwrdm_unlock(pwrdm);
+
        return ret;
 }

arch/arm/mach-omap2/powerdomain.h +6 -3

index 1edb3b7..83b4892 100644
@@ -19,8 +19,7 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
-
-#include <linux/atomic.h>
+#include <linux/spinlock.h>
 
 #include "voltage.h"
 

@@ -103,6 +102,8 @@ struct powerdomain;
  * @state_counter:
  * @timer:
  * @state_timer:
+ * @_lock: spinlock used to serialize powerdomain and some clockdomain ops
+ * @_lock_flags: stored flags when @_lock is taken
  *
  * @prcm_partition possible values are defined in mach-omap2/prcm44xx.h.
  */

@@ -127,7 +128,8 @@ struct powerdomain {
        unsigned state_counter[PWRDM_MAX_PWRSTS];
        unsigned ret_logic_off_counter;
        unsigned ret_mem_off_counter[PWRDM_MAX_MEM_BANKS];
-
+       spinlock_t _lock;
+       unsigned long _lock_flags;
        const u8 pwrstctrl_offs;
        const u8 pwrstst_offs;
        const u32 logicretstate_mask;

@@ -225,6 +227,7 @@ int pwrdm_enable_hdwr_sar(struct powerdomain *pwrdm);
 int pwrdm_disable_hdwr_sar(struct powerdomain *pwrdm);
 bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
 
+int pwrdm_state_switch_nolock(struct powerdomain *pwrdm);
 int pwrdm_state_switch(struct powerdomain *pwrdm);
 int pwrdm_pre_transition(struct powerdomain *pwrdm);
 int pwrdm_post_transition(struct powerdomain *pwrdm);

Quoted 5 lines · Paul Walmsley

Add a per-powerdomain spinlock. Use that instead of the clockdomain spinlock. Add pwrdm_lock()/pwrdm_unlock() functions to allow other code to acquire or release the powerdomain spinlock without reaching directly into the struct powerdomain.

Since clockdomains are part of powerdomains it seems weird for the clockdomain code to take a powerdoamin lock.

Is there a reason why the powerdomain could not take the lock before calling the clockdomain functions?

Also, are the lock and nolock version the clockdomain function needed?

Add a per-powerdomain spinlock. Use that instead of the clockdomain spinlock. Add pwrdm_lock()/pwrdm_unlock() functions to allow other code to acquire or release the powerdomain spinlock without reaching directly into the struct powerdomain.

The atomic usecounts seem to be confusing, and are no longer needed since the operations that they are attached to really should take place under lock. Replace the atomic counters with simple integers, protected by the enclosing powerdomain spinlock.

Signed-off-by Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/clockdomain.c  |   88 +++++++++++++++++++++++++++++++-----
 arch/arm/mach-omap2/clockdomain.h  |    6 +-
 arch/arm/mach-omap2/cm3xxx.c       |    6 +-
 arch/arm/mach-omap2/cminst44xx.c   |    2 -
 arch/arm/mach-omap2/pm-debug.c     |    6 +-
 arch/arm/mach-omap2/pm.c           |    3 +
 arch/arm/mach-omap2/prm2xxx_3xxx.c |    3 +
 7 files changed, 88 insertions(+), 26 deletions(-)

arch/arm/mach-omap2/clockdomain.c +75 -13

index 9d5b69f..ed8ac2f 100644
@@ -210,7 +210,8 @@ static int _clkdm_add_wkdep(struct clockdomain *clkdm1,
 		return ret;
 	}
 
-	if (atomic_inc_return(&cd->wkdep_usecount) == 1) {
+	cd->wkdep_usecount++;
+	if (cd->wkdep_usecount == 1) {
 		pr_debug("clockdomain: hardware will wake up %s when %s wakes up\n",
 			 clkdm1->name, clkdm2->name);
 

@@ -252,7 +253,8 @@ static int _clkdm_del_wkdep(struct clockdomain *clkdm1,
 		return ret;
 	}
 
-	if (atomic_dec_return(&cd->wkdep_usecount) == 0) {
+	cd->wkdep_usecount--;
+	if (cd->wkdep_usecount == 0) {
 		pr_debug("clockdomain: hardware will no longer wake up %s after %s wakes up\n",
 			 clkdm1->name, clkdm2->name);
 

@@ -296,7 +298,8 @@ static int _clkdm_add_sleepdep(struct clockdomain *clkdm1,
 		return ret;
 	}
 
-	if (atomic_inc_return(&cd->sleepdep_usecount) == 1) {
+	cd->sleepdep_usecount++;
+	if (cd->sleepdep_usecount == 1) {
 		pr_debug("clockdomain: will prevent %s from sleeping if %s is active\n",
 			 clkdm1->name, clkdm2->name);
 

@@ -340,7 +343,8 @@ static int _clkdm_del_sleepdep(struct clockdomain *clkdm1,
 		return ret;
 	}
 
-	if (atomic_dec_return(&cd->sleepdep_usecount) == 0) {
+	cd->sleepdep_usecount--;
+	if (cd->sleepdep_usecount == 0) {
 		pr_debug("clockdomain: will no longer prevent %s from sleeping if %s is active\n",
 			 clkdm1->name, clkdm2->name);
 

@@ -567,7 +571,21 @@ struct powerdomain *clkdm_get_pwrdm(struct clockdomain *clkdm)
  */
 int clkdm_add_wkdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
 {
-	return _clkdm_add_wkdep(clkdm1, clkdm2);
+	struct clkdm_dep *cd;
+	int ret;
+
+	if (!clkdm1 || !clkdm2)
+		return -EINVAL;
+
+	cd = _clkdm_deps_lookup(clkdm2, clkdm1->wkdep_srcs);
+	if (IS_ERR(cd))
+		return PTR_ERR(cd);
+
+	pwrdm_lock(cd->clkdm->pwrdm.ptr);
+	ret = _clkdm_add_wkdep(clkdm1, clkdm2);
+	pwrdm_unlock(cd->clkdm->pwrdm.ptr);
+
+	return ret;
 }
 
 /**

@@ -582,7 +600,21 @@ int clkdm_add_wkdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
  */
 int clkdm_del_wkdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
 {
-	return _clkdm_del_wkdep(clkdm1, clkdm2);
+	struct clkdm_dep *cd;
+	int ret;
+
+	if (!clkdm1 || !clkdm2)
+		return -EINVAL;
+
+	cd = _clkdm_deps_lookup(clkdm2, clkdm1->wkdep_srcs);
+	if (IS_ERR(cd))
+		return PTR_ERR(cd);
+
+	pwrdm_lock(cd->clkdm->pwrdm.ptr);
+	ret = _clkdm_del_wkdep(clkdm1, clkdm2);
+	pwrdm_unlock(cd->clkdm->pwrdm.ptr);
+
+	return ret;
 }
 
 /**

@@ -620,7 +652,7 @@ int clkdm_read_wkdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
 		return ret;
 	}
 
-	/* XXX It's faster to return the atomic wkdep_usecount */
+	/* XXX It's faster to return the wkdep_usecount */
 	return arch_clkdm->clkdm_read_wkdep(clkdm1, clkdm2);
 }
 

@@ -659,7 +691,21 @@ int clkdm_clear_all_wkdeps(struct clockdomain *clkdm)
  */
 int clkdm_add_sleepdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
 {
-	return _clkdm_add_sleepdep(clkdm1, clkdm2);
+	struct clkdm_dep *cd;
+	int ret;
+
+	if (!clkdm1 || !clkdm2)
+		return -EINVAL;
+
+	cd = _clkdm_deps_lookup(clkdm2, clkdm1->wkdep_srcs);
+	if (IS_ERR(cd))
+		return PTR_ERR(cd);
+
+	pwrdm_lock(cd->clkdm->pwrdm.ptr);
+	ret = _clkdm_add_sleepdep(clkdm1, clkdm2);
+	pwrdm_unlock(cd->clkdm->pwrdm.ptr);
+
+	return ret;
 }
 
 /**

@@ -676,7 +722,21 @@ int clkdm_add_sleepdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
  */
 int clkdm_del_sleepdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
 {
-	return _clkdm_del_sleepdep(clkdm1, clkdm2);
+	struct clkdm_dep *cd;
+	int ret;
+
+	if (!clkdm1 || !clkdm2)
+		return -EINVAL;
+
+	cd = _clkdm_deps_lookup(clkdm2, clkdm1->wkdep_srcs);
+	if (IS_ERR(cd))
+		return PTR_ERR(cd);
+
+	pwrdm_lock(cd->clkdm->pwrdm.ptr);
+	ret = _clkdm_del_sleepdep(clkdm1, clkdm2);
+	pwrdm_unlock(cd->clkdm->pwrdm.ptr);
+
+	return ret;
 }
 
 /**

@@ -716,7 +776,7 @@ int clkdm_read_sleepdep(struct clockdomain *clkdm1, struct clockdomain *clkdm2)
 		return ret;
 	}
 
-	/* XXX It's faster to return the atomic sleepdep_usecount */
+	/* XXX It's faster to return the sleepdep_usecount */
 	return arch_clkdm->clkdm_read_sleepdep(clkdm1, clkdm2);
 }
 

@@ -1063,7 +1123,8 @@ static int _clkdm_clk_hwmod_enable(struct clockdomain *clkdm)
 	 * should be called for every clock instance or hwmod that is
 	 * enabled, so the clkdm can be force woken up.
 	 */
-	if ((atomic_inc_return(&clkdm->usecount) > 1) && autodeps) {
+	clkdm->usecount++;
+	if (clkdm->usecount > 1 && autodeps) {
 		pwrdm_unlock(clkdm->pwrdm.ptr);
 		return 0;
 	}

@@ -1084,13 +1145,14 @@ static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm)
 
 	pwrdm_lock(clkdm->pwrdm.ptr);
 
-	if (atomic_read(&clkdm->usecount) == 0) {
+	if (clkdm->usecount == 0) {
 		pwrdm_unlock(clkdm->pwrdm.ptr);
 		WARN_ON(1); /* underflow */
 		return -ERANGE;
 	}
 
-	if (atomic_dec_return(&clkdm->usecount) > 0) {
+	clkdm->usecount--;
+	if (clkdm->usecount > 0) {
 		pwrdm_unlock(clkdm->pwrdm.ptr);
 		return 0;
 	}

arch/arm/mach-omap2/clockdomain.h +3 -3

index 50c3cd8..2da3765 100644
@@ -91,8 +91,8 @@ struct clkdm_autodep {
 struct clkdm_dep {
 	const char *clkdm_name;
 	struct clockdomain *clkdm;
-	atomic_t wkdep_usecount;
-	atomic_t sleepdep_usecount;
+	s16 wkdep_usecount;
+	s16 sleepdep_usecount;
 };
 
 /* Possible flags for struct clockdomain._flags */

@@ -136,7 +136,7 @@ struct clockdomain {
 	const u16 clkdm_offs;
 	struct clkdm_dep *wkdep_srcs;
 	struct clkdm_dep *sleepdep_srcs;
-	atomic_t usecount;
+	int usecount;
 	struct list_head node;
 };
 

arch/arm/mach-omap2/cm3xxx.c +3 -3

index b94af4c..9061c30 100644
@@ -186,7 +186,7 @@ static int omap3xxx_clkdm_clear_all_sleepdeps(struct clockdomain *clkdm)
 			continue; /* only happens if data is erroneous */
 
 		mask |= 1 << cd->clkdm->dep_bit;
-		atomic_set(&cd->sleepdep_usecount, 0);
+		cd->sleepdep_usecount = 0;
 	}
 	omap2_cm_clear_mod_reg_bits(mask, clkdm->pwrdm.ptr->prcm_offs,
 				    OMAP3430_CM_SLEEPDEP);

@@ -209,7 +209,7 @@ static int omap3xxx_clkdm_wakeup(struct clockdomain *clkdm)
 
 static void omap3xxx_clkdm_allow_idle(struct clockdomain *clkdm)
 {
-	if (atomic_read(&clkdm->usecount) > 0)
+	if (clkdm->usecount > 0)
 		clkdm_add_autodeps(clkdm);
 
 	omap3xxx_cm_clkdm_enable_hwsup(clkdm->pwrdm.ptr->prcm_offs,

@@ -221,7 +221,7 @@ static void omap3xxx_clkdm_deny_idle(struct clockdomain *clkdm)
 	omap3xxx_cm_clkdm_disable_hwsup(clkdm->pwrdm.ptr->prcm_offs,
 					clkdm->clktrctrl_mask);
 
-	if (atomic_read(&clkdm->usecount) > 0)
+	if (clkdm->usecount > 0)
 		clkdm_del_autodeps(clkdm);
 }
 

arch/arm/mach-omap2/cminst44xx.c +1 -1

index 7f9a464..f0290f5 100644
@@ -393,7 +393,7 @@ static int omap4_clkdm_clear_all_wkup_sleep_deps(struct clockdomain *clkdm)
 			continue; /* only happens if data is erroneous */
 
 		mask |= 1 << cd->clkdm->dep_bit;
-		atomic_set(&cd->wkdep_usecount, 0);
+		cd->wkdep_usecount = 0;
 	}
 
 	omap4_cminst_clear_inst_reg_bits(mask, clkdm->prcm_partition,

arch/arm/mach-omap2/pm-debug.c +2 -4

index 3cf4fdf..806a06b 100644
@@ -84,10 +84,8 @@ static int clkdm_dbg_show_counter(struct clockdomain *clkdm, void *user)
 		strncmp(clkdm->name, "dpll", 4) == 0)
 		return 0;
 
-	seq_printf(s, "%s->%s (%d)", clkdm->name,
-			clkdm->pwrdm.ptr->name,
-			atomic_read(&clkdm->usecount));
-	seq_printf(s, "\n");
+	seq_printf(s, "%s->%s (%d)\n", clkdm->name, clkdm->pwrdm.ptr->name,
+		   clkdm->usecount);
 
 	return 0;
 }

arch/arm/mach-omap2/pm.c +2 -1

index 2e2a897..6b7cb7c 100644
@@ -78,11 +78,12 @@ static void __init omap2_init_processor_devices(void)
 
 int __init omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
 {
+	/* XXX The usecount test is racy */
 	if ((clkdm->flags & CLKDM_CAN_ENABLE_AUTO) &&
 	    !(clkdm->flags & CLKDM_MISSING_IDLE_REPORTING))
 		clkdm_allow_idle(clkdm);
 	else if (clkdm->flags & CLKDM_CAN_FORCE_SLEEP &&
-		 atomic_read(&clkdm->usecount) == 0)
+		 clkdm->usecount == 0)
 		clkdm_sleep(clkdm);
 	return 0;
 }

arch/arm/mach-omap2/prm2xxx_3xxx.c +0 -0

index a3e121f..947f6ad 100644