RE: [PATCH 09/10] ARM: OMAP2+: clockdomain: convert existing atomic usecounts into spinlock-protected shorts/ints

2012-12-25 Thread Bedia, Vaibhav 
Hi Paul,

On Sun, Dec 09, 2012 at 06:53:43, Paul Walmsley wrote:
 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.
[...]
  
 @@ -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;
   }

This is not directly related to this patch but something I noticed when I 
enabled
the various debug prints.

if the clkdm-usecount is  1, there is still a call to 
arch_clkdm-clkdm_clk_enable().
Won't the usecount 1 guarantee that the clkdm is in the right state and the 
PRCM
access can be skipped?

Regards,
Vaibhav
--
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 09/10] ARM: OMAP2+: clockdomain: convert existing atomic usecounts into spinlock-protected shorts/ints

2012-12-12 Thread Vaibhav Hiremath 


On 12/9/2012 6:53 AM, Paul Walmsley wrote:
 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 p...@pwsan.com
 Cc: Kevin Hilman khil...@deeprootsystems.com
 ---
  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(-)
 
 diff --git a/arch/arm/mach-omap2/clockdomain.c 
 b/arch/arm/mach-omap2/clockdomain.c
 index 9d5b69f..ed8ac2f 100644
 --- a/arch/arm/mach-omap2/clockdomain.c
 +++ b/arch/arm/mach-omap2/clockdomain.c
 @@ -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

[PATCH 09/10] ARM: OMAP2+: clockdomain: convert existing atomic usecounts into spinlock-protected shorts/ints

2012-12-08 Thread Paul Walmsley 
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 p...@pwsan.com
Cc: Kevin Hilman khil...@deeprootsystems.com
---
 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(-)

diff --git a/arch/arm/mach-omap2/clockdomain.c 
b/arch/arm/mach-omap2/clockdomain.c
index 9d5b69f..ed8ac2f 100644
--- a/arch/arm/mach-omap2/clockdomain.c
+++ b/arch/arm/mach-omap2/clockdomain.c
@@ -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;
+
+