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

From: [email-masked] (Vaibhav Hiremath)
Date: Wed, 12 Dec 2012 15:51:54 +0530

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 <[email-masked]>
> Cc: Kevin Hilman <[email-masked]>
> ---
>  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;
> +
> +	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;
>  	}
> diff --git a/arch/arm/mach-omap2/clockdomain.h b/arch/arm/mach-omap2/clockdomain.h
> index 50c3cd8..2da3765 100644
> --- a/arch/arm/mach-omap2/clockdomain.h
> +++ b/arch/arm/mach-omap2/clockdomain.h
> @@ -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;
>  };
>  
> diff --git a/arch/arm/mach-omap2/cm3xxx.c b/arch/arm/mach-omap2/cm3xxx.c
> index b94af4c..9061c30 100644
> --- a/arch/arm/mach-omap2/cm3xxx.c
> +++ b/arch/arm/mach-omap2/cm3xxx.c
> @@ -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);
>  }
>  
> diff --git a/arch/arm/mach-omap2/cminst44xx.c b/arch/arm/mach-omap2/cminst44xx.c
> index 7f9a464..f0290f5 100644
> --- a/arch/arm/mach-omap2/cminst44xx.c
> +++ b/arch/arm/mach-omap2/cminst44xx.c
> @@ -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,
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 3cf4fdf..806a06b 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -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;
>  }
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index 2e2a897..6b7cb7c 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -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;
>  }
> diff --git a/arch/arm/mach-omap2/prm2xxx_3xxx.c b/arch/arm/mach-omap2/prm2xxx_3xxx.c
> index a3e121f..947f6ad 100644
> --- a/arch/arm/mach-omap2/prm2xxx_3xxx.c
> +++ b/arch/arm/mach-omap2/prm2xxx_3xxx.c
> @@ -210,6 +210,7 @@ int omap2_clkdm_read_wkdep(struct clockdomain *clkdm1,
>  					     PM_WKDEP, (1 << clkdm2->dep_bit));
>  }
>  
> +/* XXX Caller must hold the clkdm's powerdomain lock */

Isn't this comment applicable for all the internal api's

omap2_clkdm_clear_all_wkdeps,
omap3xxx_clkdm_add_sleepdep,
omap3xxx_clkdm_del_sleepdep,
omap3xxx_clkdm_read_sleepdep,
omap3xxx_clkdm_clear_all_sleepdeps,

>  int omap2_clkdm_clear_all_wkdeps(struct clockdomain *clkdm)

Why this function is non-static? typo??? May be I am missing something.

It was static before, but api became public by below commit -

ARM: OMAP2/3: clockdomain/PRM/CM: move the low-level clockdomain
functions into PRM/CM

Thanks,
Vaibhav

>  {
>  	struct clkdm_dep *cd;
> @@ -221,7 +222,7 @@ int omap2_clkdm_clear_all_wkdeps(struct clockdomain *clkdm)
>  
>  		/* PRM accesses are slow, so minimize them */
>  		mask |= 1 << cd->clkdm->dep_bit;
> -		atomic_set(&cd->wkdep_usecount, 0);
> +		cd->wkdep_usecount = 0;
>  	}
>  
>  	omap2_prm_clear_mod_reg_bits(mask, clkdm->pwrdm.ptr->prcm_offs,
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>