[PATCH 04/10] timer_inc: timer helper code, clockevent only

[PATCH 04/10] timer_inc: timer helper code, clockevent only

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

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 <damm@igel.co.jp>
---

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

--- /dev/null
+++ work/drivers/clocksource/timer_inc.c	2008-12-01 12:51:28.000000000 +0900
@@ -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);
+}
--- /dev/null
+++ work/include/linux/timer_inc.h	2008-12-01 12:47:28.000000000 +0900
@@ -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__ */

Re: [PATCH 04/10] timer_inc: timer helper code, clockevent only

[john stultz]

On Mon, 2008-12-01 at 19:33 +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> 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.

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.

thanks
-john


> 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 <damm@igel.co.jp>
> ---
> 
>  drivers/clocksource/timer_inc.c |  349 +++++++++++++++++++++++++++++++++++++++
>  include/linux/timer_inc.h       |   44 ++++
>  2 files changed, 393 insertions(+)
> 
> --- /dev/null
> +++ work/drivers/clocksource/timer_inc.c	2008-12-01 12:51:28.000000000 +0900
> @@ -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);
> +}
> --- /dev/null
> +++ work/include/linux/timer_inc.h	2008-12-01 12:47:28.000000000 +0900
> @@ -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__ */

Re: [PATCH 04/10] timer_inc: timer helper code, clockevent only

[Magnus Damm]

On Tue, Dec 2, 2008 at 5:29 AM, john stultz <johnstul@us.ibm.com> wrote:
> On Mon, 2008-12-01 at 19:33 +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> 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.
>
> 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.

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

> 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 hope this clarifies. Thank you.

/ magnus

Re: [PATCH 04/10] timer_inc: timer helper code, clockevent only

[john stultz]

On Tue, 2008-12-02 at 14:30 +0900, Magnus Damm wrote:
> On Tue, Dec 2, 2008 at 5:29 AM, john stultz <johnstul@us.ibm.com> wrote:
> > On Mon, 2008-12-01 at 19:33 +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> 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.
> >
> > 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.
> 
> > 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...

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.

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

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.

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

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

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

thanks
-john

Re: [PATCH 04/10] timer_inc: timer helper code, clockevent only

[Magnus Damm]

On Wed, Dec 3, 2008 at 1:04 PM, john stultz <johnstul@us.ibm.com> wrote:
> On Tue, 2008-12-02 at 14:30 +0900, Magnus Damm wrote:
>> On Tue, Dec 2, 2008 at 5:29 AM, john stultz <johnstul@us.ibm.com> wrote:
>> > On Mon, 2008-12-01 at 19:33 +0900, Magnus Damm wrote:
>> >> From: Magnus Damm <damm@igel.co.jp>
>> >>
>> >> 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.
>> >
>> > 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.
>>
>> > 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...
>
> 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.

Yes, correct - the timer_inc code is very similar to the PIT
implementation, with the exception that clocksource and clockevent in
oneshot mode are supported together. So the match value varies with
time which makes the implementation a bit more difficult.

And you are right, software hacks like these come with all sorts of
potential overflow problems. Otoh, if you have an interrupt source
that needs to keep track of  count then you better serve in time. =)

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

I don't think there is any downside in the cases where the hardware
fits with just a few lines of code wrapping the interface. And that's
certainly the case for the HPET. Regarding the PIT I'd say it fits
well too, but I'd like to have a single timer for both oneshot and
clocksource. The simple PIT implementation does periodic clockevent
only in combination with clocksource - unless I'm mistaken - but I
need oneshot to be able to powersave. Or have I misunderstood the
basics? =)

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

I wish I could have two timers running under deep sleep instead. =)

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

That is true. I agree with the potential confusion. And I'm not a big
fan of layers and layers of crap on top of each other, and I think the
timer_inc code is pretty much yet another layer. It does however keep
track of the aggregated state of both the clocksource and clockevent,
and shuts down / starts the timer whenever needed. This is a bit
tricky to get sorted out correctly in my mind, and I think it would be
nice to avoid duplicating it in all drivers that wish to support both
clocksource and clockevent.

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

I had the idea of reusing the timer_inc code for a MTU2 timer on
another SH board that I have here, but maybe I should leave that for
now and just focus on the CMT timer with a combined clocksource and
clockevent. That may be a good first step. We do have some processors
with multiple CMT timers, so having the read2() would be good either
way in my opinion.

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

Yes, but then other drivers have to implement the same logic if they
wish to share clockevent and clocksource with a single timer. That may
be ok though. A single timer for both clocksource and oneshot
clockevents should maybe not be encouraged due to the potential
wrapping situations.

> I'm still not sure I see what necessary function the timer_inc structure
> provides.
>
>> > 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.

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.

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

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?

Cheers,

/ magnus

Re: [PATCH 04/10] timer_inc: timer helper code, clockevent only

[John Stultz]

On Thu, 2008-12-04 at 00:30 +0900, Magnus Damm wrote:
> On Wed, Dec 3, 2008 at 1:04 PM, john stultz <johnstul@us.ibm.com> 
> > 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.

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

> > 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. :)
> 
> 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?

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.

thanks
-john

Re: [PATCH 04/10] timer_inc: timer helper code, clockevent only

[Magnus Damm]

On Thu, Dec 4, 2008 at 1:35 AM, John Stultz <johnstul@us.ibm.com> wrote:
> On Thu, 2008-12-04 at 00:30 +0900, Magnus Damm wrote:
>> 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.

Sure!

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

Ok, I'm not 100% sure how to do the unregistration properly, but I'll
try to come up with something. That goes for the clockevent stuff
too..

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

No worries, I usually do a couple of iterations of my code before
everyone gets happy. =)

Thanks for your help so far!

/ magnus