[PATCH] OMAP: Asynchronous clock disable

[PATCH] OMAP: Asynchronous clock disable

[Jarkko Lavinen]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

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

[-- Attachment #2: 0001-OMAP-Add-asynchronous-clock-disable.patch --]
[-- Type: text/x-diff, Size: 4550 bytes --]

>From 557e94d0a43efbc7e312ab7c1d8a568d65a9310a Mon Sep 17 00:00:00 2001
From: Jarkko Lavinen <jarkko.lavinen@nokia.com>
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.

Signed-off-by: Jarkko Lavinen <jarkko.lavinen@nokia.com>
---
 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(-)

diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index 70f2311..a94b122 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -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);
diff --git a/include/asm-arm/arch-omap/clock.h b/include/asm-arm/arch-omap/clock.h
index cb7a301..aa95aac 100644
--- a/include/asm-arm/arch-omap/clock.h
+++ b/include/asm-arm/arch-omap/clock.h
@@ -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)
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 5ca8c6f..c922f79 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] OMAP: Asynchronous clock disable

[Igor Stoppa]

Hi,

On Fri, 2007-10-05 at 15:50 +0300, ext Jarkko Lavinen wrote:
> 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.

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

> What I would like to do is:

<snip>

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

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 <igor.stoppa@nokia.com>
(Nokia Multimedia - CP - OSSO / Helsinki, Finland)

RE: [PATCH] OMAP: Asynchronous clock disable

[Woodruff, Richard]

> Subject: [PATCH] OMAP: Asynchronous clock disable

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.

Re: [PATCH] OMAP: Asynchronous clock disable

[Tony Lindgren]

Hi,

* Woodruff, Richard <r-woodruff2@ti.com> [071005 06:29]:
> 
> > Subject: [PATCH] OMAP: Asynchronous clock disable
> 
> 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.

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

RE: [PATCH] OMAP: Asynchronous clock disable

[Woodruff, Richard]

> From: Tony Lindgren
> Sent: Sunday, October 07, 2007 10:51 PM
> > 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.
> 
> Sounds like this could also allow the same performance as keeping clocks
> on all the time for some drivers.

Hopefully.

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

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.