OMAP: Asynchronous clock disable

Hi

In N800 mmc driver we enable and disable mmc clock in a continous cycle with small delays.

I wonder if asynchronou clock disable has been suggested before?

I would like to be able to disable clocks asynchronously with a small delay so that when doing repetitive clk enable/disable cycle, the clock would just stay up and only shut down when the continuos stream of requests is over.

What I would like to do is:

clk_enable(clk):
... process the request
clk_disable_async(clk, 2);

This would leave clock running for 2 ticks after returning from clk_disable_async() and only then disable it. In mmc driver the delay could be close to 50 ms.

If clk_disable_async() would be reapplies, the latter call would stay in effect since it is done using mod_timer(). For example here the clock would be disabled 1 tick after the second call to clk_disable_async():

     clk_enable(clk);
     clk_disable_async(clk, HZ);
     clk_enable(clk);
     clk_disable_async(clk, 1);

If clk_disable() and clk_disable_async() would be mixed, clk_disable() would not cancel pending clk disable since the usecount would still be bigger than 0 after calling clk_disable():

     clk_enable(clk);
     clk_disable_async(clk, HZ);
     clk_enable(clk);
     clk_disable(clk);

For clearing up pending clock disables I use clk_flush_disable(clk) clears out any pending disable immediately.

     clk_enable(clk);
     clk_disable_async(clk, HZ);
     ... later
     clk_flush_disable(clk)

Jarkko Lavinen

From: Jarkko Lavinen <[email protected]>
Date: Fri, 5 Oct 2007 15:39:46 +0300
Subject: [PATCH] OMAP: Add asynchronous clock disable

Asynchronouse clock disable is useful for repetitive request processing, where clock is enabled and disabled continuosly.

---
 arch/arm/plat-omap/clock.c        |   56 +++++++++++++++++++++++++++++++++++--
 include/asm-arm/arch-omap/clock.h |    4 ++
 include/linux/clk.h               |   16 ++++++++++
 3 files changed, 73 insertions(+), 3 deletions(-)

index 70f2311..a94b122 100644
@@ -127,7 +127,7 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL(clk_enable);
 
-void clk_disable(struct clk *clk)
+void clk_disable_async(struct clk *clk, int ticks)
 {
 	unsigned long flags;
 

@@ -142,14 +142,61 @@ void clk_disable(struct clk *clk)
 		goto out;
 	}
 
-	if (arch_clock->clk_disable)
-		arch_clock->clk_disable(clk);
+	if (ticks > 0) {
+		if ((clk->flags & CLOCK_ASYNC_DISABLE) == 0) {
+			printk(KERN_ERR "Trying to disable clock %s "
+			       "asynchronousely without proper "
+			       "initialization\n", clk->name);
+			WARN_ON(1);
+		} else {
+			if (timer_pending(&clk->disable_timer))
+				clk->usecount--;
+
+			mod_timer(&clk->disable_timer, jiffies + ticks);
+		}
+	} else
+		if (arch_clock->clk_disable)
+			arch_clock->clk_disable(clk);
+out:
+	spin_unlock_irqrestore(&clockfw_lock, flags);
+}
+EXPORT_SYMBOL(clk_disable_async);
+
+void clk_flush_disable(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clockfw_lock, flags);
+	if ((clk->flags & CLOCK_ASYNC_DISABLE) == 0) {
+		printk(KERN_ERR "Trying to disable clock %s asynchronousely "
+		       "without timer initialization\n", clk->name);
+		WARN_ON(1);
+		goto out;
+	}
 
+	if (timer_pending(&clk->disable_timer)) {
+		del_timer(&clk->disable_timer);
+		if (arch_clock->clk_disable)
+			arch_clock->clk_disable(clk);
+	}
 out:
 	spin_unlock_irqrestore(&clockfw_lock, flags);
 }
+EXPORT_SYMBOL(clk_flush_disable);
+
+void clk_disable(struct clk *clk)
+{
+	clk_disable_async(clk, 0);
+}
 EXPORT_SYMBOL(clk_disable);
 
+static void clk_disable_timer(unsigned long data)
+{
+	struct clk *clk = (struct clk *) data;
+
+	clk_disable_async(clk, 0);
+}
+
 int clk_get_usecount(struct clk *clk)
 {
 	unsigned long flags;

@@ -336,6 +383,9 @@ int clk_register(struct clk *clk)
 
 	mutex_lock(&clocks_mutex);
 	list_add(&clk->node, &clocks);
+	if (clk->flags & CLOCK_ASYNC_DISABLE)
+		setup_timer(&clk->disable_timer, clk_disable_timer,
+			    (unsigned long) clk);
 	if (clk->init)
 		clk->init(clk);
 	mutex_unlock(&clocks_mutex);

index cb7a301..aa95aac 100644
@@ -13,6 +13,8 @@
 #ifndef __ARCH_ARM_OMAP_CLOCK_H
 #define __ARCH_ARM_OMAP_CLOCK_H
 
+#include <linux/timer.h>
+
 struct module;
 struct clk;
 

@@ -57,6 +59,7 @@ struct clk {
 	void __iomem		*enable_reg;
 	__u8			enable_bit;
 	__s8			usecount;
+	struct timer_list	disable_timer;
 	void			(*recalc)(struct clk *);
 	int			(*set_rate)(struct clk *, unsigned long);
 	long			(*round_rate)(struct clk *, unsigned long);

@@ -123,6 +126,7 @@ extern void clk_enable_init_clocks(void);
 #define CLOCK_IN_OMAP243X	(1 << 26)
 #define CLOCK_IN_OMAP343X	(1 << 27)
 #define PARENT_CONTROLS_CLOCK	(1 << 28)
+#define CLOCK_ASYNC_DISABLE	(1 << 29)
 
 /* Clksel_rate flags */
 #define DEFAULT_RATE            (1 << 0)

index 5ca8c6f..c922f79 100644
@@ -63,6 +63,22 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_disable_async - disable the clock after a delay
+ * @clk:   clock source
+ * @ticks: delay
+ *
+ * Asyncronous clock disable is implemented with a timer which runs
+ * clk_disable when the timer expires.
+ */
+void clk_disable_async(struct clk *clk, int ticks);
+
+/**
+ * clk_sync_disable - Finish pending clock disable immediately
+ * @clk: clock source
+ */
+void clk_flush_disable(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source

1.5.3.1

Hi,

This could be applied also to other cases:
-PLL locking/relocking (however this is quite fast) -external oscillator enabling/disabling; this can introduce significant latency, so the oscillator would beleft running and OMAP could still go to clock stop, rather than the current implementation which prevents retention

<snip>

This is ok but anyway i don't see it as being badly needed since in a practical use case it would be probably used after a long sequence:

do {
	clk_enable(clk);
	do_something_requiring_clk();
	clk_disable_async(clk, HZ);
} while (work_left());

clk_flush_disable(clk);

so i expect the improvement to be minimal to the case where the timer is left to expire (i assume the timer won't be too long, otherwise the clock would have been released for good every time)

Cheers, Igor

Igor Stoppa <[email protected]>
(Nokia Multimedia - CP - OSSO / Helsinki, Finland)

Generally I think something like this is good. Internally I've been calling this clock_disable_lazy().

For OMAP3 we have been trying to make drivers more aggressive in their FCLCOK handling. As much of this control happens in bottom layers of large stacks, we don't always know the call frequency on all entry points into a driver. This call can have local clock implications like you are seeing and also power domain and chip wide ones.

As for what is the right number for each driver we were floating ideas like if you have an active inner region of the driver, it should use a small number and for outer regions use a larger number.

* If you don't have the time or knowledge to instrument these drivers call in points as their may be many (or to be more future proof with less maintenance) it might be ok to set the time out to coincide with some global system wide or domain wide activity timers.

This might apply more to say power domains, but one point is, if the display is on and the user is interacting the touch screen, it is impossible to hit _chip_ OFF modes anyway. This will only happen after the screen has blanked. Thus for some timers having them approximately set to user input speeds might be ok. Once these major events have happened (screen blank and no input activity), it now becomes about optimizing duty cycle of chip low power modes. In this case shorter timer outs are better as they are more impacting in that system mode.

I'll try to look at actual code a bit more later on.

Regards,
Richard W.

Hi,

Sounds like this could also allow the same performance as keeping clocks on all the time for some drivers.

We just have to take care not to use it too much with timers going off all the time :)

Tony

Hopefully.

Yes. Too many timers would kill dynamic tick. However, the new timer coalescence APIs as presented at OLS2007 and probably lists might make that go away. That or maybe just per clock domain.

An API change might not even be needed. It could just activate timers based on some global setting. Its not like today you depend on the clock really being off when you say clock_disable() (because of use count effects).

I did like better Yuha's clk_use/unuse which were potentially delayed and an always synchronous/force enable/disable. No need for a flush.

Regards,
Richard W.