timer_inc: timer helper code, clockevent only

From: Magnus Damm <[email protected]>

This patch adds clock source/event helper code for incrementing timers.

Normally tickless operation requires two timers, one for the clock source and another one for a oneshot timer event. The helper code makes it possible to use a single timer for both clock source and clock event, ie tickless with a single incrementing timer. Useful on for instance processors where all timers except one needs to be disabled for deep sleep.

The clock source is calculated from the running hardware timer plus a software controlled counter which together form a 32 bit value. High accuracy of the clock source is kept by never reprogramming the hardware counter, only the hardware match value.

Clock events are handled by reading out current hardware counter value and reprogramming the match register. Both periodic and oneshot modes are supported.

The helper code only supports incrementing timer hardware which at matching time resets and generates an interrupt but keeps on counting. Timers counting down are not supported.

This patch only implements clock event code to begin with. The code gets updated with full clock source support later in the series.

Signed-off-by Magnus Damm <[email protected]>

---

 drivers/clocksource/timer_inc.c |  349 +++++++++++++++++++++++++++++++++++++++
 include/linux/timer_inc.h       |   44 ++++
 2 files changed, 393 insertions(+)

drivers/clocksource/timer_inc.c new file +349 -0

@@ -0,0 +1,349 @@
+/*
+ * timer_inc.c - Clock source/event helper code for incrementing timers.
+ *
+ *  Copyright (C) 2008 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/spinlock.h>
+#include <linux/timer_inc.h>
+
+/*
+ * Written for increment-match-wrap timer hardware. The timer is constantly
+ * enabled and a software cycle counter is updated at interrupt time.
+ * The hardware timer counter is never modified to allow high clock source
+ * quality. The match register is rewritten to support oneshot and periodic
+ * clock events.
+ */
+
+/* private flags */
+#define FLAG_CLOCKEVENT (1 << 0)
+#define FLAG_REPROGRAM (1 << 2)
+#define FLAG_SKIPEVENT (1 << 3)
+#define FLAG_IRQCONTEXT (1 << 4)
+
+static void timer_inc_clock_event_program_verify(struct timer_inc *tip,
+						 int absolute)
+{
+	unsigned long new_match;
+	unsigned long value = tip->next_match_value;
+	unsigned long delay = 0;
+	unsigned long now = 0;
+	int has_wrapped;
+
+	now = tip->ops->get_counter(tip->priv, &has_wrapped);
+	tip->flags |= FLAG_REPROGRAM; /* force reprogram */
+
+	if (has_wrapped) {
+		/* we're competing with the interrupt handler.
+		 *  -> let the interrupt handler reprogram the timer.
+		 *  -> interrupt number two handles the event.
+		 */
+		tip->flags |= FLAG_SKIPEVENT;
+		return;
+	}
+
+	if (absolute)
+		now = 0;
+
+	do {
+		/* reprogram the timer hardware,
+		 * but don't save the new match value yet.
+		 */
+		new_match = now + value + delay;
+		if (new_match > tip->max_match_value)
+			new_match = tip->max_match_value;
+
+		tip->ops->set_match(tip->priv, new_match);
+
+		now = tip->ops->get_counter(tip->priv, &has_wrapped);
+		if (has_wrapped && (new_match > tip->match_value)) {
+			/* we are changing to a greater match value,
+			 * so this wrap must be caused by the counter
+			 * matching the old value.
+			 * -> first interrupt reprograms the timer.
+			 * -> interrupt number two handles the event.
+			 */
+			tip->flags |= FLAG_SKIPEVENT;
+			break;
+		}
+
+		if (has_wrapped) {
+			/* we are changing to a smaller match value,
+			 * so the wrap must be caused by the counter
+			 * matching the new value.
+			 * -> save programmed match value.
+			 * -> let isr handle the event.
+			 */
+			tip->match_value = new_match;
+			break;
+		}
+
+		/* be safe: verify hardware settings */
+		if (now < new_match) {
+			/* timer value is below match value, all good.
+			 * this makes sure we won't miss any match events.
+			 * -> save programmed match value.
+			 * -> let isr handle the event.
+			 */
+			tip->match_value = new_match;
+			break;
+		}
+
+		/* the counter has reached a value greater
+		 * than our new match value. and since the
+		 * has_wrapped flag isn't set we must have
+		 * programmed a too close event.
+		 * -> increase delay and retry.
+		 */
+		if (delay)
+			delay <<= 1;
+		else
+			delay = 1;
+
+		if (!delay)
+			pr_warning("timer_inc: too long delay\n");
+
+	} while (delay);
+}
+
+static void timer_inc_set_next(struct timer_inc *tip, unsigned long delta)
+{
+	unsigned long flags;
+
+	if (delta > tip->max_match_value)
+		pr_warning("timer_inc: delta out of range\n");
+
+	spin_lock_irqsave(&tip->lock, flags);
+	tip->next_match_value = delta;
+	timer_inc_clock_event_program_verify(tip, 0);
+	spin_unlock_irqrestore(&tip->lock, flags);
+}
+
+void timer_inc_interrupt(struct timer_inc *tip)
+{
+	if (!(tip->flags & FLAG_REPROGRAM))
+		tip->next_match_value = tip->max_match_value;
+
+	tip->flags |= FLAG_IRQCONTEXT;
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+	if (tip->flags & FLAG_CLOCKEVENT) {
+		if (!(tip->flags & FLAG_SKIPEVENT)) {
+			if (tip->ced.mode = CLOCK_EVT_MODE_ONESHOT) {
+				tip->next_match_value = tip->max_match_value;
+				tip->flags |= FLAG_REPROGRAM;
+			}
+
+			tip->ced.event_handler(&tip->ced);
+		}
+	}
+#endif
+	tip->flags &= ~FLAG_SKIPEVENT;
+
+	if (tip->flags & FLAG_REPROGRAM) {
+		tip->flags &= ~FLAG_REPROGRAM;
+		timer_inc_clock_event_program_verify(tip, 1);
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+		if (tip->flags & FLAG_CLOCKEVENT)
+			if ((tip->ced.mode = CLOCK_EVT_MODE_SHUTDOWN)
+			    || (tip->match_value = tip->next_match_value))
+				tip->flags &= ~FLAG_REPROGRAM;
+#endif
+	}
+
+	tip->flags &= ~FLAG_IRQCONTEXT;
+}
+
+static int timer_inc_start(struct timer_inc *tip, unsigned long flag)
+{
+	int ret = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tip->lock, flags);
+
+	if (!(tip->flags & FLAG_CLOCKEVENT))
+		ret = tip->ops->enable(tip->priv, &tip->rate);
+
+	if (!ret)
+		tip->flags |= flag;
+
+	spin_unlock_irqrestore(&tip->lock, flags);
+
+	return ret;
+}
+
+static void timer_inc_stop(struct timer_inc *tip, unsigned long flag)
+{
+	unsigned long flags;
+	unsigned long f;
+
+	spin_lock_irqsave(&tip->lock, flags);
+
+	f = tip->flags & FLAG_CLOCKEVENT;
+	tip->flags &= ~flag;
+
+	if (f && !(tip->flags & FLAG_CLOCKEVENT))
+		tip->ops->disable(tip->priv);
+
+	spin_unlock_irqrestore(&tip->lock, flags);
+}
+
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+
+static struct timer_inc *ced_to_timer_inc(struct clock_event_device *ced)
+{
+	return container_of(ced, struct timer_inc, ced);
+}
+
+static void timer_inc_clock_event_start(struct timer_inc *tip, int periodic)
+{
+	struct clock_event_device *ced = &tip->ced;
+
+	timer_inc_start(tip, FLAG_CLOCKEVENT);
+
+	/* TODO: calculate good shift from rate and counter bit width */
+
+	ced->shift = 32;
+	ced->mult = div_sc(tip->rate, NSEC_PER_SEC, ced->shift);
+	ced->max_delta_ns = clockevent_delta2ns(tip->max_match_value, ced);
+	ced->min_delta_ns = clockevent_delta2ns(0x1f, ced);
+
+	if (periodic)
+		timer_inc_set_next(tip, (tip->rate + HZ/2) / HZ);
+	else
+		timer_inc_set_next(tip, tip->max_match_value);
+}
+
+static void timer_inc_clock_event_mode(enum clock_event_mode mode,
+				       struct clock_event_device *ced)
+{
+	struct timer_inc *tip = ced_to_timer_inc(ced);
+
+	/* deal with old setting first */
+	switch (ced->mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+	case CLOCK_EVT_MODE_ONESHOT:
+		timer_inc_stop(tip, FLAG_CLOCKEVENT);
+		break;
+	default:
+		break;
+	}
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		pr_info("timer_inc: %s used for periodic clock events\n",
+			ced->name);
+		timer_inc_clock_event_start(tip, 1);
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		pr_info("timer_inc: %s used for oneshot clock events\n",
+			ced->name);
+		timer_inc_clock_event_start(tip, 0);
+		break;
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+		timer_inc_stop(tip, FLAG_CLOCKEVENT);
+		break;
+	default:
+		break;
+	}
+}
+
+static int timer_inc_clock_event_next(unsigned long delta,
+				      struct clock_event_device *ced)
+{
+	struct timer_inc *tip = ced_to_timer_inc(ced);
+
+	BUG_ON(ced->mode != CLOCK_EVT_MODE_ONESHOT);
+	if (likely(tip->flags & FLAG_IRQCONTEXT))
+		tip->next_match_value = delta;
+	else
+		timer_inc_set_next(tip, delta);
+
+	return 0;
+}
+
+static void timer_inc_register_clockevent(struct timer_inc *tip,
+					  char *name,
+					  unsigned long rating)
+{
+	struct clock_event_device *ced = &tip->ced;
+
+	memset(ced, 0, sizeof(*ced));
+
+	ced->name = name;
+	ced->features = CLOCK_EVT_FEAT_PERIODIC;
+	ced->features |= CLOCK_EVT_FEAT_ONESHOT;
+	ced->rating = rating;
+	ced->cpumask = CPU_MASK_CPU0;
+	ced->set_next_event = timer_inc_clock_event_next;
+	ced->set_mode = timer_inc_clock_event_mode;
+
+	pr_info("timer_inc: %s used for clock events\n", ced->name);
+	clockevents_register_device(ced);
+}
+
+static void timer_inc_unregister_clockevent(struct timer_inc *tip)
+{
+	struct clock_event_device *ced = &tip->ced;
+
+	if (ced->features)
+		clockevents_unregister_device(ced);
+}
+
+#else /* CONFIG_GENERIC_CLOCKEVENTS */
+static inline void timer_inc_register_clockevent(struct timer_inc *tip,
+						 char *name,
+						 unsigned long rating) {}
+static inline void timer_inc_unregister_clockevent(struct timer_inc *tip) {}
+#endif /* CONFIG_GENERIC_CLOCKEVENTS */
+
+int timer_inc_register(struct timer_inc *tip,
+		       char *name, void *priv,
+		       unsigned long bits,
+		       struct timer_inc_ops *ops,
+		       unsigned long clockevent_rating,
+		       unsigned long clocksource_rating)
+{
+	memset(tip, 0, sizeof(*tip));
+
+	if (bits = (sizeof(tip->max_match_value) * 8))
+		tip->max_match_value = ~0;
+	else
+		tip->max_match_value = (1 << bits) - 1;
+
+	tip->match_value = tip->max_match_value;
+	tip->priv = priv;
+	tip->ops = ops;
+	spin_lock_init(&tip->lock);
+
+	if (clockevent_rating)
+		timer_inc_register_clockevent(tip, name, clockevent_rating);
+
+	return 0;
+}
+
+void timer_inc_unregister(struct timer_inc *tip)
+{
+	timer_inc_unregister_clockevent(tip);
+}

include/linux/timer_inc.h new file +44 -0

@@ -0,0 +1,44 @@
+#ifndef __TIMER_INC_H__
+#define __TIMER_INC_H__
+
+/* Clock source/event helper code for incrementing timers. */
+
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/spinlock.h>
+
+struct timer_inc;
+
+struct timer_inc_ops {
+	void (*set_match)(void *priv, unsigned long value);
+	unsigned long (*get_counter)(void *priv, int *has_wrapped);
+	int (*enable)(void *priv, unsigned long *rate);
+	void (*disable)(void *priv);
+};
+
+struct timer_inc {
+	struct timer_inc_ops *ops;
+	void *priv;
+	unsigned long flags;
+	unsigned long match_value;
+	unsigned long next_match_value;
+	unsigned long max_match_value;
+	unsigned long rate;
+	spinlock_t lock;
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+	struct clock_event_device ced;
+#endif
+};
+
+void timer_inc_interrupt(struct timer_inc *t);
+
+int timer_inc_register(struct timer_inc *tip,
+		       char *name, void *priv,
+		       unsigned long bits,
+		       struct timer_inc_ops *ops,
+		       unsigned long clockevent_rating,
+		       unsigned long clocksource_rating);
+
+void timer_inc_unregister(struct timer_inc *tip);
+
+#endif /* __TIMER_INC_H__ */

Quoted 19 lines · Magnus Damm

From: Magnus Damm <[email protected]>

This patch adds clock source/event helper code for incrementing timers.

Normally tickless operation requires two timers, one for the clock source and another one for a oneshot timer event. The helper code makes it possible to use a single timer for both clock source and clock event, ie tickless with a single incrementing timer. Useful on for instance processors where all timers except one needs to be disabled for deep sleep.

The clock source is calculated from the running hardware timer plus a software controlled counter which together form a 32 bit value. High accuracy of the clock source is kept by never reprogramming the hardware counter, only the hardware match value.

Clock events are handled by reading out current hardware counter value and reprogramming the match register. Both periodic and oneshot modes are supported.

Quoted 3 lines

The helper code only supports incrementing timer hardware which at matching time resets and generates an interrupt but keeps on counting. Timers counting down are not supported.

Hey Magnus,
Forgive me again for not understanding all this immediately. It just
seems like a fair amount of generic infrastructure, so I want to make sure it really is reusable.

I'm not sure your initial assertion that tickless operation requires two timers is correct. Can you explain why this timer_inc structure is necessary, in comparison to just having separate clockevent and clocksource interfaces for one bit of hardware, like what is done with the similar count-up HPET on x86?

I just worry that we're adding extra structures and interfaces where it might be better to just re-factor the clocksource and clockevent structures themselves to handle the needs you have.

Quoted 20 lines · john stultz

Quoted 19 lines · Magnus Damm

From: Magnus Damm <[email protected]>

This patch adds clock source/event helper code for incrementing timers.

Normally tickless operation requires two timers, one for the clock source and another one for a oneshot timer event. The helper code makes it possible to use a single timer for both clock source and clock event, ie tickless with a single incrementing timer. Useful on for instance processors where all timers except one needs to be disabled for deep sleep.

The clock source is calculated from the running hardware timer plus a software controlled counter which together form a 32 bit value. High accuracy of the clock source is kept by never reprogramming the hardware counter, only the hardware match value.

Clock events are handled by reading out current hardware counter value and reprogramming the match register. Both periodic and oneshot modes are supported.

Quoted 8 lines

Quoted 3 lines

The helper code only supports incrementing timer hardware which at matching time resets and generates an interrupt but keeps on counting. Timers counting down are not supported.

Hey Magnus,
Forgive me again for not understanding all this immediately. It just
seems like a fair amount of generic infrastructure, so I want to make sure it really is reusable.

Hi John,

Thanks for checking my code, and sorry for not giving you enough background.

Quoted 5 lines

I'm not sure your initial assertion that tickless operation requires two timers is correct. Can you explain why this timer_inc structure is necessary, in comparison to just having separate clockevent and clocksource interfaces for one bit of hardware, like what is done with the similar count-up HPET on x86?

The HPET hardware seems to come with a single counter and several match channels. The clocksource is connected to the counter and the clockevents use the first match channel to generate events. The clocksource counter keeps on counting and the clockevents are reprogrammed as they should. All fine, and the existing clocksource and clockevent infrastructure fits very well for this. The CMT hardware otoh - which the timer_inc code is designed around - automatically clears the counter on match...

I'm currently writing code for the SuperH architecture where timer hardware available varies depending on processor model. In most cases each processor comes with say two or three different types of timer hardware blocks, and each block comes with somewhere between one and three channels. Most timers are 32-bit these days, but some are 16-bit only. Some count up-only, others down only and some do both directions. Each timer channel usually comes with it's own timer counter. The clock driving the counter and dividing capabilities varies greatly.

The most straight-forward way to add support for this wide range of hardware is to dedicate one 32-bit timer channel for clocksource and another one for clockevents. For this approach the existing clocksource and clockevent infrastructure fits well, but the downside is that we need to write two drivers for the same type of hardware - one using the clocksource interface and one for clockevents.

What I'm trying to do is to use a single timer channel for both clocksource and clockevents. This is because I want to power down as much hardware as possible and only use a single CMT timer channel which is driven by a separate 32Khz clock. If the rest of the system is idle then we can deep sleep by stopping the main clock and power off most of the processor, but still have a working clocksource and single shot clockevent and let them keep track of time and wake us up from deep sleep.

So when I hacked up my first shared timer prototype I saw that this is something that should be broken out. Mainly since the shared timer logic has nothing to do with the actual timer hardware details. And I know at least three types of different timer hardware types on SuperH that can reuse this code.

After breaking out the timer_inc code I realized that I managed to remove the dependency on if the timer will be used as clocksource or clockevent. So all of a sudden I can have a single driver and let the user decide how it should be used.

Quoted 3 lines

I just worry that we're adding extra structures and interfaces where it might be better to just re-factor the clocksource and clockevent structures themselves to handle the needs you have.

Yeah, especially the programming-and-retrying part of the timer_inc code looks very similar to the clock event code. I don't mind modifying existing structures at all. My not-so-qualified guess is that other architectures also may come with similar increment-match-clear timer hardware.

Quoted 40 lines · Magnus Damm

Quoted 20 lines · john stultz

Quoted 19 lines · Magnus Damm

From: Magnus Damm <[email protected]>

This patch adds clock source/event helper code for incrementing timers.

Normally tickless operation requires two timers, one for the clock source and another one for a oneshot timer event. The helper code makes it possible to use a single timer for both clock source and clock event, ie tickless with a single incrementing timer. Useful on for instance processors where all timers except one needs to be disabled for deep sleep.

The clock source is calculated from the running hardware timer plus a software controlled counter which together form a 32 bit value. High accuracy of the clock source is kept by never reprogramming the hardware counter, only the hardware match value.

Clock events are handled by reading out current hardware counter value and reprogramming the match register. Both periodic and oneshot modes are supported.

Quoted 8 lines

Quoted 3 lines

The helper code only supports incrementing timer hardware which at matching time resets and generates an interrupt but keeps on counting. Timers counting down are not supported.

Hey Magnus,
Forgive me again for not understanding all this immediately. It just
seems like a fair amount of generic infrastructure, so I want to make sure it really is reusable.

Hi John,

Thanks for checking my code, and sorry for not giving you enough background.

Quoted 5 lines

I'm not sure your initial assertion that tickless operation requires two timers is correct. Can you explain why this timer_inc structure is necessary, in comparison to just having separate clockevent and clocksource interfaces for one bit of hardware, like what is done with the similar count-up HPET on x86?

Quoted 8 lines

The HPET hardware seems to come with a single counter and several match channels. The clocksource is connected to the counter and the clockevents use the first match channel to generate events. The clocksource counter keeps on counting and the clockevents are reprogrammed as they should. All fine, and the existing clocksource and clockevent infrastructure fits very well for this. The CMT hardware otoh - which the timer_inc code is designed around - automatically clears the counter on match...

So this sounds similar to how the PIT clocksource is implemented. Where half is a hardware counter and the upper bits are faked via software. Is this right, or am I misunderstanding? I'm a little cautious of these software hacks since they can cause some trouble depending on how long interrupt processing might be deferred. For example, while the hardware seems to have an overflow bit(which is critical), should interrupts be starved for too long, like what used to happen on i386 w/ SMIs, or from other IRQ processing, I worry you might run in to trouble from double overflow. This would be further problematic in SMP configs, but I'm not sure if that's something the sh architecture deals with.

Quoted 10 lines

I'm currently writing code for the SuperH architecture where timer hardware available varies depending on processor model. In most cases each processor comes with say two or three different types of timer hardware blocks, and each block comes with somewhere between one and three channels. Most timers are 32-bit these days, but some are 16-bit only. Some count up-only, others down only and some do both directions. Each timer channel usually comes with it's own timer counter. The clock driving the counter and dividing capabilities varies greatly.

Quoted 6 lines

The most straight-forward way to add support for this wide range of hardware is to dedicate one 32-bit timer channel for clocksource and another one for clockevents. For this approach the existing clocksource and clockevent infrastructure fits well, but the downside is that we need to write two drivers for the same type of hardware - one using the clocksource interface and one for clockevents.

I'm not sure if this is a major downside. :) They're just interface hooks, and given the wide array of hardware that can be used for one or both interfaces the division seems pretty reasonable to me.

Quoted 8 lines

What I'm trying to do is to use a single timer channel for both clocksource and clockevents. This is because I want to power down as much hardware as possible and only use a single CMT timer channel which is driven by a separate 32Khz clock. If the rest of the system is idle then we can deep sleep by stopping the main clock and power off most of the processor, but still have a working clocksource and single shot clockevent and let them keep track of time and wake us up from deep sleep.

This seems like a fine goal.

Quoted 10 lines

So when I hacked up my first shared timer prototype I saw that this is something that should be broken out. Mainly since the shared timer logic has nothing to do with the actual timer hardware details. And I know at least three types of different timer hardware types on SuperH that can reuse this code.

After breaking out the timer_inc code I realized that I managed to remove the dependency on if the timer will be used as clocksource or clockevent. So all of a sudden I can have a single driver and let the user decide how it should be used.

I guess what I'm most concerned about is that you've created yet another interface that we have to manage and maintain, and its similar enough to the existing interfaces that developers might be confused as to which they should write to for their hardware.

Further I'm not sure the amount of duplication you'd end up with to create just clocksource and clockevent drivers for the different hardware would be more then the amount of code needed in the registration and management, along with the clocksource and clockevent integration of the timer_inc infrastructure.

Looking at the code again, you seem to have to indirect through a number of interfaces:
1) cmt_device structure (actual hardware def)
2) cmt_driver and setup
3) timer_inc structure
4) clockevents and clocksource structures

It seems the cmd_driver could probably skip the timer_inc bits without too much trouble, and just create a clockevent or clocksource structure directly, no?

I'm still not sure I see what necessary function the timer_inc structure provides.

Quoted 9 lines

Quoted 3 lines

I just worry that we're adding extra structures and interfaces where it might be better to just re-factor the clocksource and clockevent structures themselves to handle the needs you have.

Yeah, especially the programming-and-retrying part of the timer_inc code looks very similar to the clock event code. I don't mind modifying existing structures at all. My not-so-qualified guess is that other architectures also may come with similar increment-match-clear timer hardware.

I definitely am interesting in trying to figure out if the clockevent and clocksource interfaces need refinement to better adapt to hardware if they are not sufficient. But I'm very hesitant to recommend creating super-interfaces that try to span both.

It just seems to me that most of the timer_inc code could be pushed down into the cmt_driver to cut out the middle man. It reduces the number of generic interfaces we have to deal with, and lessens the indirection.

Again, I may be missing a subtlety of the timer_inc code, so keep hammering on me if I'm just being thick headed. :)

On Wed, Dec 3, 2008 at 1:04 PM, john stultz <[email protected]>

Quoted 4 lines

I definitely am interesting in trying to figure out if the clockevent and clocksource interfaces need refinement to better adapt to hardware if they are not sufficient. But I'm very hesitant to recommend creating super-interfaces that try to span both.

The timer_inc interface sure is a something that spans both. Actually, that's the only thing it does - keeps track of the aggregated state and makes sure the timer is disabled whenever it's unused.

So maybe it's better to scrap the timer_inc part and just include that logic in the cmt driver? We can always go back later on if there is a direct need.

Quoted 4 lines

It just seems to me that most of the timer_inc code could be pushed down into the cmt_driver to cut out the middle man. It reduces the number of generic interfaces we have to deal with, and lessens the indirection.

Quoted 2 lines

Again, I may be missing a subtlety of the timer_inc code, so keep hammering on me if I'm just being thick headed. :)

You're not so think headed. =) I don't mind either way with timer_inc!

Regarding read2() patch, the enable()/disable() patch and a future unregistration patch - I still like to see that included. Do you see any disadvantages with that?

Quoted 3 lines · Magnus Damm

So maybe it's better to scrap the timer_inc part and just include that logic in the cmt driver? We can always go back later on if there is a direct need.

I think I'd be happier with this. Hopefully this will make any needed changes to the clockevent or clocksource code more apparent.

Quoted 3 lines

Regarding read2() patch, the enable()/disable() patch and a future unregistration patch - I still like to see that included. Do you see any disadvantages with that?

The enable/disable bits seem great. The read2() bit I'm not against, but would like to see how it ends up being used before we commit to the interface change. And unregistration is something that's been needed.

Thanks again for dealing with my feedback. I know its not fun to rework or throwout code and start again. But I think in this case it will help the amount of future maintenance needed.