diff mbox

[v2,6/7] OMAP2+: clockdomain: Add 2 APIs to control clockdomain from hwmod framework

Message ID 1308917193-16912-7-git-send-email-b-cousson@ti.com (mailing list archive)
State New, archived
Delegated to: Benoit Cousson
Headers show

Commit Message

Benoit Cousson June 24, 2011, 12:06 p.m. UTC
Duplicate the existing API for clockdomain enable from clock to enable
a clock domain from hwmod framework.
This will be needed when the hwmod framework will move from the current
clock centric approach to the module based approach.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/clockdomain.c |  150 +++++++++++++++++++++++++++++-------
 arch/arm/mach-omap2/clockdomain.h |    3 +
 2 files changed, 124 insertions(+), 29 deletions(-)

Comments

Todd Poynor June 26, 2011, 8:07 p.m. UTC | #1
On Fri, Jun 24, 2011 at 02:06:32PM +0200, Benoit Cousson wrote:
> Duplicate the existing API for clockdomain enable from clock to enable
> a clock domain from hwmod framework.
> This will be needed when the hwmod framework will move from the current
> clock centric approach to the module based approach.
> 
...
> +static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm)
> +{
> +	if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_disable)
> +		return -EINVAL;
> +
> +#ifdef DEBUG
> +	if (atomic_read(&clkdm->usecount) == 0) {
> +		WARN_ON(1); /* underflow */
> +		return -ERANGE;
> +	}
> +#endif
> +
> +	if (atomic_dec_return(&clkdm->usecount) > 0)
> +		return 0;

Suggest taking the error return out of the #ifdef DEBUG code (mainly
to lessen the chance that enabling debugging features makes problems
appear or disappear, although the WARN_ON should be a prominent clue
in this case), and maybe just have the function always handle
underflow.

...
> @@ -877,35 +911,93 @@ int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
>   */
>  int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk)
>  {
> +	int ret = 0;
> +
>  	/*
>  	 * XXX Rewrite this code to maintain a list of enabled
>  	 * downstream clocks for debugging purposes?
>  	 */
>  
> -	if (!clkdm || !clk)
> +	if (!clk)
>  		return -EINVAL;
>  
> -	if (!arch_clkdm || !arch_clkdm->clkdm_clk_disable)
> +	ret = _clkdm_clk_hwmod_disable(clkdm);
> +
> +	/* All downstream clocks of this clockdomain are now disabled */
> +	pr_debug("clockdomain: clkdm %s: clk %s now disabled\n", clkdm->name,
> +		 clk->name);

The pr_debug should be printed only if ret == 0.

The comment seems true only if the clock domain's usecount went to zero.

The terminology here may be a bit confusing: the function does not actually
disable the named downstream clock in the usual sense
(as in clk_disable(clk), or any action to individually gate that
clock)  Similar comments for clkdm_clk_enable). 

Similar comments for clkdm_hwmod_disable.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benoit Cousson June 27, 2011, 9:18 a.m. UTC | #2
Hi Todd,

On 6/26/2011 10:07 PM, Todd Poynor wrote:
> On Fri, Jun 24, 2011 at 02:06:32PM +0200, Benoit Cousson wrote:
>> Duplicate the existing API for clockdomain enable from clock to enable
>> a clock domain from hwmod framework.
>> This will be needed when the hwmod framework will move from the current
>> clock centric approach to the module based approach.
>>
> ...
>> +static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm)
>> +{
>> +	if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_disable)
>> +		return -EINVAL;
>> +
>> +#ifdef DEBUG
>> +	if (atomic_read(&clkdm->usecount) == 0) {
>> +		WARN_ON(1); /* underflow */
>> +		return -ERANGE;
>> +	}
>> +#endif
>> +
>> +	if (atomic_dec_return(&clkdm->usecount)>  0)
>> +		return 0;
>
> Suggest taking the error return out of the #ifdef DEBUG code (mainly
> to lessen the chance that enabling debugging features makes problems
> appear or disappear, although the WARN_ON should be a prominent clue
> in this case), and maybe just have the function always handle
> underflow.

If your final point is that we'd better remove the #ifdef, I do agree.

> ...
>> @@ -877,35 +911,93 @@ int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
>>    */
>>   int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk)
>>   {
>> +	int ret = 0;
>> +
>>   	/*
>>   	 * XXX Rewrite this code to maintain a list of enabled
>>   	 * downstream clocks for debugging purposes?
>>   	 */
>>
>> -	if (!clkdm || !clk)
>> +	if (!clk)
>>   		return -EINVAL;
>>
>> -	if (!arch_clkdm || !arch_clkdm->clkdm_clk_disable)
>> +	ret = _clkdm_clk_hwmod_disable(clkdm);
>> +
>> +	/* All downstream clocks of this clockdomain are now disabled */
>> +	pr_debug("clockdomain: clkdm %s: clk %s now disabled\n", clkdm->name,
>> +		 clk->name);
>
> The pr_debug should be printed only if ret == 0.

That's not even enough, because the function is returning 0 even if the 
domain is already enabled.

> The comment seems true only if the clock domain's usecount went to zero.
>
> The terminology here may be a bit confusing: the function does not actually
> disable the named downstream clock in the usual sense
> (as in clk_disable(clk), or any action to individually gate that
> clock)  Similar comments for clkdm_clk_enable).

Yeah, I kept that from the original code, but in fact, I'll probably get 
rid of the pr_debug here and put it in the two helper functions.

Benoit

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c
index 2ab3686..7add49e 100644
--- a/arch/arm/mach-omap2/clockdomain.c
+++ b/arch/arm/mach-omap2/clockdomain.c
@@ -817,7 +817,48 @@  int clkdm_is_idle(struct clockdomain *clkdm)
 }
 
 
-/* Clockdomain-to-clock framework interface code */
+/* Clockdomain-to-clock/hwmod framework interface code */
+
+static int _clkdm_clk_hwmod_enable(struct clockdomain *clkdm)
+{
+	if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_enable)
+		return -EINVAL;
+
+	/*
+	 * For arch's with no autodeps, clkcm_clk_enable
+	 * 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)
+		return 0;
+
+	arch_clkdm->clkdm_clk_enable(clkdm);
+	pwrdm_wait_transition(clkdm->pwrdm.ptr);
+	pwrdm_clkdm_state_switch(clkdm);
+
+	return 0;
+}
+
+static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm)
+{
+	if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_disable)
+		return -EINVAL;
+
+#ifdef DEBUG
+	if (atomic_read(&clkdm->usecount) == 0) {
+		WARN_ON(1); /* underflow */
+		return -ERANGE;
+	}
+#endif
+
+	if (atomic_dec_return(&clkdm->usecount) > 0)
+		return 0;
+
+	arch_clkdm->clkdm_clk_disable(clkdm);
+	pwrdm_clkdm_state_switch(clkdm);
+
+	return 0;
+}
 
 /**
  * clkdm_clk_enable - add an enabled downstream clock to this clkdm
@@ -835,30 +876,23 @@  int clkdm_is_idle(struct clockdomain *clkdm)
  */
 int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
 {
+	int ret = 0;
+
 	/*
 	 * XXX Rewrite this code to maintain a list of enabled
 	 * downstream clocks for debugging purposes?
 	 */
 
-	if (!clkdm || !clk)
+	if (!clk)
 		return -EINVAL;
 
-	if (!arch_clkdm || !arch_clkdm->clkdm_clk_enable)
-		return -EINVAL;
-
-	if (atomic_inc_return(&clkdm->usecount) > 1)
-		return 0;
+	ret = _clkdm_clk_hwmod_enable(clkdm);
 
 	/* Clockdomain now has one enabled downstream clock */
-
 	pr_debug("clockdomain: clkdm %s: clk %s now enabled\n", clkdm->name,
 		 clk->name);
 
-	arch_clkdm->clkdm_clk_enable(clkdm);
-	pwrdm_wait_transition(clkdm->pwrdm.ptr);
-	pwrdm_clkdm_state_switch(clkdm);
-
-	return 0;
+	return ret;
 }
 
 /**
@@ -877,35 +911,93 @@  int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
  */
 int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk)
 {
+	int ret = 0;
+
 	/*
 	 * XXX Rewrite this code to maintain a list of enabled
 	 * downstream clocks for debugging purposes?
 	 */
 
-	if (!clkdm || !clk)
+	if (!clk)
 		return -EINVAL;
 
-	if (!arch_clkdm || !arch_clkdm->clkdm_clk_disable)
+	ret = _clkdm_clk_hwmod_disable(clkdm);
+
+	/* All downstream clocks of this clockdomain are now disabled */
+	pr_debug("clockdomain: clkdm %s: clk %s now disabled\n", clkdm->name,
+		 clk->name);
+
+	return ret;
+}
+
+/**
+ * clkdm_hwmod_enable - add an enabled downstream hwmod to this clkdm
+ * @clkdm: struct clockdomain *
+ * @oh: struct omap_hwmod * of the enabled downstream hwmod
+ *
+ * Increment the usecount of the clockdomain @clkdm and ensure that it
+ * is awake before @oh is enabled. Intended to be called by
+ * module_enable() code.
+ * If the clockdomain is in software-supervised idle mode, force the
+ * clockdomain to wake.  If the clockdomain is in hardware-supervised idle
+ * mode, add clkdm-pwrdm autodependencies, to ensure that devices in the
+ * clockdomain can be read from/written to by on-chip processors.
+ * Returns -EINVAL if passed null pointers;
+ * returns 0 upon success or if the clockdomain is in hwsup idle mode.
+ */
+int clkdm_hwmod_enable(struct clockdomain *clkdm, struct omap_hwmod *oh)
+{
+	int ret = 0;
+
+	/*
+	 * XXX Rewrite this code to maintain a list of enabled
+	 * downstream hwmods for debugging purposes?
+	 */
+
+	if (!oh)
 		return -EINVAL;
 
-#ifdef DEBUG
-	if (atomic_read(&clkdm->usecount) == 0) {
-		WARN_ON(1); /* underflow */
-		return -ERANGE;
-	}
-#endif
+	ret = _clkdm_clk_hwmod_enable(clkdm);
 
-	if (atomic_dec_return(&clkdm->usecount) > 0)
-		return 0;
+	/* Clockdomain now has one enabled downstream hwmod */
+	pr_debug("clockdomain: clkdm %s: oh %s now enabled\n", clkdm->name,
+		 oh->name);
 
-	/* All downstream clocks of this clockdomain are now disabled */
+	return ret;
+}
 
-	pr_debug("clockdomain: clkdm %s: clk %s now disabled\n", clkdm->name,
-		 clk->name);
+/**
+ * clkdm_hwmod_disable - remove an enabled downstream hwmod from this clkdm
+ * @clkdm: struct clockdomain *
+ * @oh: struct omap_hwmod * of the disabled downstream hwmod
+ *
+ * Decrement the usecount of this clockdomain @clkdm when @oh is
+ * disabled. Intended to be called by module_disable() code.
+ * If the clockdomain usecount goes to 0, put the clockdomain to sleep
+ * (software-supervised mode) or remove the clkdm autodependencies
+ * (hardware-supervised mode).
+ * Returns -EINVAL if passed null pointers; -ERANGE if the @clkdm usecount
+ * underflows and debugging is enabled; or returns 0 upon success or if the
+ * clockdomain is in hwsup idle mode.
+ */
+int clkdm_hwmod_disable(struct clockdomain *clkdm, struct omap_hwmod *oh)
+{
+	int ret = 0;
 
-	arch_clkdm->clkdm_clk_disable(clkdm);
-	pwrdm_clkdm_state_switch(clkdm);
+	/*
+	 * XXX Rewrite this code to maintain a list of enabled
+	 * downstream hwmods for debugging purposes?
+	 */
 
-	return 0;
+	if (!oh)
+		return -EINVAL;
+
+	ret = _clkdm_clk_hwmod_disable(clkdm);
+
+	/* All downstream hwmods of this clockdomain are now disabled */
+	pr_debug("clockdomain: clkdm %s: oh %s now disabled\n", clkdm->name,
+		 oh->name);
+
+	return ret;
 }
 
diff --git a/arch/arm/mach-omap2/clockdomain.h b/arch/arm/mach-omap2/clockdomain.h
index 085ed82..77244b3 100644
--- a/arch/arm/mach-omap2/clockdomain.h
+++ b/arch/arm/mach-omap2/clockdomain.h
@@ -20,6 +20,7 @@ 
 
 #include "powerdomain.h"
 #include <plat/clock.h>
+#include <plat/omap_hwmod.h>
 #include <plat/cpu.h>
 
 /*
@@ -186,6 +187,8 @@  int clkdm_sleep(struct clockdomain *clkdm);
 
 int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk);
 int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk);
+int clkdm_hwmod_enable(struct clockdomain *clkdm, struct omap_hwmod *oh);
+int clkdm_hwmod_disable(struct clockdomain *clkdm, struct omap_hwmod *oh);
 
 extern void __init omap2xxx_clockdomains_init(void);
 extern void __init omap3xxx_clockdomains_init(void);