[PATCH] ARM: ep93xx: clockevent support

ARM: ep93xx: clockevent support
I have ported to 3.6-rc1 and slightly modified a previous patch for clockevent support for the ep93xx.
The porting mainly consists of sched_clock support.
Tested on 9302 based board.
From: Ahmed Ammar <aammar <at> edge-techno.com>
Signed-off-by: Yan Burman <redacted>

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e91c7cd..6dbb828 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig

@@ -451,7 +451,7 @@ config ARCH_EP93XX
 	select CLKDEV_LOOKUP
 	select ARCH_REQUIRE_GPIOLIB
 	select ARCH_HAS_HOLES_MEMORYMODEL
-	select ARCH_USES_GETTIMEOFFSET
+	select GENERIC_CLOCKEVENTS
 	select NEED_MACH_MEMORY_H
 	help
 	  This enables support for the Cirrus EP93xx series of CPUs.

diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
index 4afe52a..1497cba 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c

@@ -30,6 +30,8 @@
 #include <linux/amba/bus.h>
 #include <linux/amba/serial.h>
 #include <linux/mtd/physmap.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
 #include <linux/i2c.h>
 #include <linux/i2c-gpio.h>
 #include <linux/spi/spi.h>

@@ -43,6 +45,7 @@
 
 #include <asm/mach/map.h>
 #include <asm/mach/time.h>
+#include <asm/sched_clock.h>
 
 #include <asm/hardware/vic.h>
 

@@ -114,63 +117,129 @@ void __init ep93xx_map_io(void)
 #define EP93XX_TIMER4_CLOCK		983040
 
 #define TIMER1_RELOAD			((EP93XX_TIMER123_CLOCK / HZ) - 1)
-#define TIMER4_TICKS_PER_JIFFY		DIV_ROUND_CLOSEST(CLOCK_TICK_RATE, HZ)
-
-static unsigned int last_jiffy_time;
 
 static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
 {
-	/* Writing any value clears the timer interrupt */
-	__raw_writel(1, EP93XX_TIMER1_CLEAR);
+	struct clock_event_device *evt = dev_id;
 
-	/* Recover lost jiffies */
-	while ((signed long)
-		(__raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time)
-						>= TIMER4_TICKS_PER_JIFFY) {
-		last_jiffy_time += TIMER4_TICKS_PER_JIFFY;
-		timer_tick();
-	}
+	__raw_writel(1, EP93XX_TIMER1_CLEAR);
+	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
+static int ep93xx_set_next_event(unsigned long evt,
+				struct clock_event_device *unused)
+{
+	u32 tmode = __raw_readl(EP93XX_TIMER1_CONTROL);
+
+	/* stop timer */
+	__raw_writel(tmode & ~EP93XX_TIMER123_CONTROL_ENABLE,
+			EP93XX_TIMER1_CONTROL);
+	/* program timer */
+	__raw_writel(evt, EP93XX_TIMER1_LOAD);
+	/* start timer */
+	__raw_writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
+			EP93XX_TIMER1_CONTROL);
+
+	return 0;
+}
+
+static void ep93xx_set_mode(enum clock_event_mode mode,
+			    struct clock_event_device *evt)
+{
+	u32 tmode = EP93XX_TIMER123_CONTROL_CLKSEL;
+	/* Disable timer */
+	__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		/* Set timer period  */
+		__raw_writel((508469 / HZ) - 1, EP93XX_TIMER1_LOAD);
+		tmode |= EP93XX_TIMER123_CONTROL_MODE;
+
+	case CLOCK_EVT_MODE_ONESHOT:
+		tmode |= EP93XX_TIMER123_CONTROL_ENABLE;
+		__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+		break;
+
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_RESUME:
+		return;
+	}
+}
+
+static struct clock_event_device clockevent_ep93xx = {
+	.name		= "ep93xx-timer1",
+	.features	= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
+	.shift		= 32,
+	.set_mode	= ep93xx_set_mode,
+	.set_next_event	= ep93xx_set_next_event,
+};
+
 static struct irqaction ep93xx_timer_irq = {
 	.name		= "ep93xx timer",
 	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
 	.handler	= ep93xx_timer_interrupt,
+	.dev_id		= &clockevent_ep93xx,
 };
 
-static void __init ep93xx_timer_init(void)
+static void __init ep93xx_clockevent_init(void)
 {
-	u32 tmode = EP93XX_TIMER123_CONTROL_MODE |
-		    EP93XX_TIMER123_CONTROL_CLKSEL;
+	setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+	clockevent_ep93xx.mult = div_sc(508469, NSEC_PER_SEC,
+					clockevent_ep93xx.shift);
+	clockevent_ep93xx.max_delta_ns =
+		clockevent_delta2ns(0xfffffffe, &clockevent_ep93xx);
+	clockevent_ep93xx.min_delta_ns =
+		clockevent_delta2ns(0xf, &clockevent_ep93xx);
+	clockevent_ep93xx.cpumask = cpumask_of(0);
+	clockevents_register_device(&clockevent_ep93xx);
+}
 
-	/* Enable periodic HZ timer.  */
-	__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
-	__raw_writel(TIMER1_RELOAD, EP93XX_TIMER1_LOAD);
-	__raw_writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
-			EP93XX_TIMER1_CONTROL);
+/*
+ * timer4 is a 40 Bit timer, separated in a 32bit and a 8 bit
+ * register, EP93XX_TIMER4_VALUE_LOW stores 32 bit word. The
+ * controlregister is in EP93XX_TIMER4_VALUE_HIGH
+ */
+static cycle_t ep93xx_get_cycles(struct clocksource *unused)
+{
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Enable lost jiffy timer.  */
-	__raw_writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
-			EP93XX_TIMER4_VALUE_HIGH);
+static struct clocksource clocksource_ep93xx = {
+	.name		= "ep93xx_timer4",
+	.rating		= 200,
+	.read		= ep93xx_get_cycles,
+	.mask		= 0xFFFFFFFF,
+	.shift		= 20,
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};
 
-	setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+static void __init ep93xx_clocksource_init(void)
+{
+	/* Reset time-stamp counter */
+	__raw_writel(0x100, EP93XX_TIMER4_VALUE_HIGH);
+	clocksource_ep93xx.mult =
+		clocksource_hz2mult(983040, clocksource_ep93xx.shift);
+	clocksource_register(&clocksource_ep93xx);
 }
 
-static unsigned long ep93xx_gettimeoffset(void)
+static u32 notrace ep93xx_sched_clock(void)
 {
-	int offset;
-
-	offset = __raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time;
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Calculate (1000000 / 983040) * offset.  */
-	return offset + (53 * offset / 3072);
+static void __init ep93xx_timer_init(void)
+{
+	ep93xx_clocksource_init();
+	ep93xx_clockevent_init();
+	setup_sched_clock(ep93xx_sched_clock, 32, CLOCK_TICK_RATE);
 }
 
 struct sys_timer ep93xx_timer = {
 	.init		= ep93xx_timer_init,
-	.offset		= ep93xx_gettimeoffset,
 };
 
 

On 07/08/12 21:21, Yan Burman wrote:

ARM: ep93xx: clockevent support
I have ported to 3.6-rc1 and slightly modified a previous patch for clockevent support for the ep93xx.
The porting mainly consists of sched_clock support.
Tested on 9302 based board.
From: Ahmed Ammar <aammar <at> edge-techno.com>
Signed-off-by: Yan Burman <redacted>

Hi Yan, some comments below.

~Ryan

Nitpick - blank line between variable declarations and code.

+	__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		/* Set timer period  */
+		__raw_writel((508469 / HZ) - 1, EP93XX_TIMER1_LOAD);
+		tmode |= EP93XX_TIMER123_CONTROL_MODE;
+

Is the fall-through here intentional? If so, please put a comment
to indicate that it is.

+	case CLOCK_EVT_MODE_ONESHOT:
+		tmode |= EP93XX_TIMER123_CONTROL_ENABLE;
+		__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+		break;
+
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_RESUME:
+		return;

These cases can be removed.

+	}
+}
+
+static struct clock_event_device clockevent_ep93xx = {
+	.name		= "ep93xx-timer1",
+	.features	= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
+	.shift		= 32,
+	.set_mode	= ep93xx_set_mode,
+	.set_next_event	= ep93xx_set_next_event,
+};
+
 static struct irqaction ep93xx_timer_irq = {
 	.name		= "ep93xx timer",
 	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
 	.handler	= ep93xx_timer_interrupt,
+	.dev_id		= &clockevent_ep93xx,
 };
 
-static void __init ep93xx_timer_init(void)
+static void __init ep93xx_clockevent_init(void)
 {
-	u32 tmode = EP93XX_TIMER123_CONTROL_MODE |
-		    EP93XX_TIMER123_CONTROL_CLKSEL;
+	setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+	clockevent_ep93xx.mult = div_sc(508469, NSEC_PER_SEC,
+					clockevent_ep93xx.shift);

core.c has:

  #define EP93XX_TIMER123_CLOCK               508469

Maybe we should move that to soc.h so you can use it here.

+	clockevent_ep93xx.max_delta_ns =
+		clockevent_delta2ns(0xfffffffe, &clockevent_ep93xx);
+	clockevent_ep93xx.min_delta_ns =
+		clockevent_delta2ns(0xf, &clockevent_ep93xx);
+	clockevent_ep93xx.cpumask = cpumask_of(0);
+	clockevents_register_device(&clockevent_ep93xx);
+}
 
-	/* Enable periodic HZ timer.  */
-	__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
-	__raw_writel(TIMER1_RELOAD, EP93XX_TIMER1_LOAD);
-	__raw_writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
-			EP93XX_TIMER1_CONTROL);
+/*
+ * timer4 is a 40 Bit timer, separated in a 32bit and a 8 bit
+ * register, EP93XX_TIMER4_VALUE_LOW stores 32 bit word. The
+ * controlregister is in EP93XX_TIMER4_VALUE_HIGH

Missing space between control and register.

+ */
+static cycle_t ep93xx_get_cycles(struct clocksource *unused)
+{
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Enable lost jiffy timer.  */
-	__raw_writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
-			EP93XX_TIMER4_VALUE_HIGH);
+static struct clocksource clocksource_ep93xx = {
+	.name		= "ep93xx_timer4",
+	.rating		= 200,
+	.read		= ep93xx_get_cycles,
+	.mask		= 0xFFFFFFFF,

Lower case for hex constant.

+	.shift		= 20,
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};
 
-	setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+static void __init ep93xx_clocksource_init(void)
+{
+	/* Reset time-stamp counter */
+	__raw_writel(0x100, EP93XX_TIMER4_VALUE_HIGH);
+	clocksource_ep93xx.mult =
+		clocksource_hz2mult(983040, clocksource_ep93xx.shift);

core.c has define EP93XX_TIMER4_CLOCK 983040, probably should move
that to soc.h also.

+	clocksource_register(&clocksource_ep93xx);
 }
 
-static unsigned long ep93xx_gettimeoffset(void)
+static u32 notrace ep93xx_sched_clock(void)
 {
-	int offset;
-
-	offset = __raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time;
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Calculate (1000000 / 983040) * offset.  */
-	return offset + (53 * offset / 3072);
+static void __init ep93xx_timer_init(void)
+{
+	ep93xx_clocksource_init();
+	ep93xx_clockevent_init();
+	setup_sched_clock(ep93xx_sched_clock, 32, CLOCK_TICK_RATE);
 }
 
 struct sys_timer ep93xx_timer = {
 	.init		= ep93xx_timer_init,
-	.offset		= ep93xx_gettimeoffset,
 };
 
 

On Tue, Aug 07, 2012 at 02:21:14PM +0300, Yan Burman wrote:

+/*
+ * timer4 is a 40 Bit timer, separated in a 32bit and a 8 bit
+ * register, EP93XX_TIMER4_VALUE_LOW stores 32 bit word. The
+ * controlregister is in EP93XX_TIMER4_VALUE_HIGH
+ */
+static cycle_t ep93xx_get_cycles(struct clocksource *unused)
+{
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Enable lost jiffy timer.  */
-	__raw_writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
-			EP93XX_TIMER4_VALUE_HIGH);
+static struct clocksource clocksource_ep93xx = {
+	.name		= "ep93xx_timer4",
+	.rating		= 200,
+	.read		= ep93xx_get_cycles,
+	.mask		= 0xFFFFFFFF,
+	.shift		= 20,
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};

Well done for porting this over to the clockevent/clocksource stuff.

Please have a look at the mmio clocksource helpers in
drivers/clocksource/mmio.c which I think you'll find will do everything
that you need without requiring all this code.

On Tuesday, August 07, 2012 6:04 AM, Ryan Mallon wrote:

On 07/08/12 21:21, Yan Burman wrote:

quoted

ARM: ep93xx: clockevent support
I have ported to 3.6-rc1 and slightly modified a previous patch for clockevent support for the ep93xx.
The porting mainly consists of sched_clock support.
Tested on 9302 based board.
From: Ahmed Ammar <aammar <at> edge-techno.com>
Signed-off-by: Yan Burman <redacted>

Hello Yan,

Hi Yan, some comments below.

~Ryan

I have a couple minor comments as well...

<snip>

quoted

@@ -114,63 +117,129 @@ void __init ep93xx_map_io(void)
 #define EP93XX_TIMER4_CLOCK		983040
 
 #define TIMER1_RELOAD			((EP93XX_TIMER123_CLOCK / HZ) - 1)
-#define TIMER4_TICKS_PER_JIFFY		DIV_ROUND_CLOSEST(CLOCK_TICK_RATE, HZ)
-
-static unsigned int last_jiffy_time;
 
 static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
 {
-	/* Writing any value clears the timer interrupt */

Please leave this comment for the write to EP93XX_TIMER1_CLEAR
below.

quoted

-	__raw_writel(1, EP93XX_TIMER1_CLEAR);
+	struct clock_event_device *evt = dev_id;
 
-	/* Recover lost jiffies */
-	while ((signed long)
-		(__raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time)
-						>= TIMER4_TICKS_PER_JIFFY) {
-		last_jiffy_time += TIMER4_TICKS_PER_JIFFY;
-		timer_tick();
-	}
+	__raw_writel(1, EP93XX_TIMER1_CLEAR);
+	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
+static int ep93xx_set_next_event(unsigned long evt,
+				struct clock_event_device *unused)
+{
+	u32 tmode = __raw_readl(EP93XX_TIMER1_CONTROL);
+
+	/* stop timer */
+	__raw_writel(tmode & ~EP93XX_TIMER123_CONTROL_ENABLE,
+			EP93XX_TIMER1_CONTROL);
+	/* program timer */
+	__raw_writel(evt, EP93XX_TIMER1_LOAD);
+	/* start timer */
+	__raw_writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
+			EP93XX_TIMER1_CONTROL);
+
+	return 0;
+}
+
+static void ep93xx_set_mode(enum clock_event_mode mode,
+			    struct clock_event_device *evt)
+{
+	u32 tmode = EP93XX_TIMER123_CONTROL_CLKSEL;
+	/* Disable timer */

Nitpick - blank line between variable declarations and code.

quoted

+	__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		/* Set timer period  */
+		__raw_writel((508469 / HZ) - 1, EP93XX_TIMER1_LOAD);

As Ryan mentions below.  There are defines already in this file
for some of the magic values.

The ((508469 / HZ) - 1) is TIMER1_RELOAD.

quoted

+		tmode |= EP93XX_TIMER123_CONTROL_MODE;
+

Is the fall-through here intentional? If so, please put a comment
to indicate that it is.

quoted

+	case CLOCK_EVT_MODE_ONESHOT:
+		tmode |= EP93XX_TIMER123_CONTROL_ENABLE;
+		__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+		break;
+
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_RESUME:
+		return;

These cases can be removed.

quoted

+	}
+}
+
+static struct clock_event_device clockevent_ep93xx = {

Nitpick, rename this to ep93xx_clockevent please.

quoted

+	.name		= "ep93xx-timer1",
+	.features	= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
+	.shift		= 32,
+	.set_mode	= ep93xx_set_mode,
+	.set_next_event	= ep93xx_set_next_event,
+};
+
 static struct irqaction ep93xx_timer_irq = {
 	.name		= "ep93xx timer",
 	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
 	.handler	= ep93xx_timer_interrupt,
+	.dev_id		= &clockevent_ep93xx,
 };
 
-static void __init ep93xx_timer_init(void)
+static void __init ep93xx_clockevent_init(void)
 {

Assuming the rename above, add:

	struct clock_event_device *evt = &ep93xx_clockevent;

Then you can use 'evt->' instead of the longer 'ep93xx_clockevent.'
in the rest of this function. It makes the code a bit cleaner.

quoted

-	u32 tmode = EP93XX_TIMER123_CONTROL_MODE |
-		    EP93XX_TIMER123_CONTROL_CLKSEL;
+	setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+	clockevent_ep93xx.mult = div_sc(508469, NSEC_PER_SEC,
+					clockevent_ep93xx.shift);

core.c has:

 #define EP93XX_TIMER123_CLOCK               508469

Maybe we should move that to soc.h so you can use it here.

No need to move the defines to soc.h. They are only used in this file.

quoted

+	clockevent_ep93xx.max_delta_ns =
+		clockevent_delta2ns(0xfffffffe, &clockevent_ep93xx);
+	clockevent_ep93xx.min_delta_ns =
+		clockevent_delta2ns(0xf, &clockevent_ep93xx);
+	clockevent_ep93xx.cpumask = cpumask_of(0);
+	clockevents_register_device(&clockevent_ep93xx);
+}
 
-	/* Enable periodic HZ timer.  */
-	__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
-	__raw_writel(TIMER1_RELOAD, EP93XX_TIMER1_LOAD);
-	__raw_writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
-			EP93XX_TIMER1_CONTROL);
+/*
+ * timer4 is a 40 Bit timer, separated in a 32bit and a 8 bit
+ * register, EP93XX_TIMER4_VALUE_LOW stores 32 bit word. The
+ * controlregister is in EP93XX_TIMER4_VALUE_HIGH

I don't think this comment is needed.

Missing space between control and register.

quoted

+ */
+static cycle_t ep93xx_get_cycles(struct clocksource *unused)
+{
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Enable lost jiffy timer.  */
-	__raw_writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
-			EP93XX_TIMER4_VALUE_HIGH);
+static struct clocksource clocksource_ep93xx = {

Nitpick, rename this to ep93xx_clocksource.

quoted

+	.name		= "ep93xx_timer4",

Nitpick, use "ep93xx-timer4" for the name, the "-" is used
consistently in this file for all the names.

quoted

+	.rating		= 200,
+	.read		= ep93xx_get_cycles,
+	.mask		= 0xFFFFFFFF,

Lower case for hex constant.

quoted

+	.shift		= 20,
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};
 
-	setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+static void __init ep93xx_clocksource_init(void)

Again, assuming the rename above, add:

	struct clocksource *src = &ep93xx_clocksource;

Then use the pointer instead of the actual variable.

quoted

+{
+	/* Reset time-stamp counter */
+	__raw_writel(0x100, EP93XX_TIMER4_VALUE_HIGH);

Again, the 0x100 magic value is defined as EP93XX_TIMER4_VALUE_HIGH_ENABLE.

quoted

+	clocksource_ep93xx.mult =
+		clocksource_hz2mult(983040, clocksource_ep93xx.shift);

core.c has define EP93XX_TIMER4_CLOCK 983040, probably should move
that to soc.h also.

quoted

+	clocksource_register(&clocksource_ep93xx);
 }
 
-static unsigned long ep93xx_gettimeoffset(void)
+static u32 notrace ep93xx_sched_clock(void)
 {
-	int offset;
-
-	offset = __raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time;
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
 
-	/* Calculate (1000000 / 983040) * offset.  */
-	return offset + (53 * offset / 3072);
+static void __init ep93xx_timer_init(void)
+{
+	ep93xx_clocksource_init();
+	ep93xx_clockevent_init();
+	setup_sched_clock(ep93xx_sched_clock, 32, CLOCK_TICK_RATE);
 }
 
 struct sys_timer ep93xx_timer = {
 	.init		= ep93xx_timer_init,
-	.offset		= ep93xx_gettimeoffset,
 };
 
 

Other than those comments, looks good. I'll try to test this later.

Thanks,
Hartley

On 8/7/12, Ryan Mallon [off-list ref] wrote:

On 07/08/12 21:21, Yan Burman wrote:

quoted

ARM: ep93xx: clockevent support
I have ported to 3.6-rc1 and slightly modified a previous patch for
clockevent support for the ep93xx.
The porting mainly consists of sched_clock support.
Tested on 9302 based board.
From: Ahmed Ammar <aammar <at> edge-techno.com>
Signed-off-by: Yan Burman <redacted>

Hi Yan, some comments below.

~Ryan

quoted

+	case CLOCK_EVT_MODE_ONESHOT:
+		tmode |= EP93XX_TIMER123_CONTROL_ENABLE;
+		__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+		break;
+
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_RESUME:
+		return;

These cases can be removed.

This case is something that is present in most (if not all) other architectures.
I don't mind removing it, but it will have to be replaced with:
default:
     ;

Since GCC produces warnings otherwise.

Other than that I am going to wait a while more for more comments if
somebody has them,
and post a new version with style fixes as well as switch to
drivers/clocksource/mmio.c usage.

Regards,
Yan

On 09/08/12 18:54, Yan Burman wrote:

On 8/7/12, Ryan Mallon [off-list ref] wrote:

quoted

On 07/08/12 21:21, Yan Burman wrote:

quoted

ARM: ep93xx: clockevent support
I have ported to 3.6-rc1 and slightly modified a previous patch for
clockevent support for the ep93xx.
The porting mainly consists of sched_clock support.
Tested on 9302 based board.
From: Ahmed Ammar <aammar <at> edge-techno.com>
Signed-off-by: Yan Burman <redacted>

Hi Yan, some comments below.

~Ryan

quoted

+	case CLOCK_EVT_MODE_ONESHOT:
+		tmode |= EP93XX_TIMER123_CONTROL_ENABLE;
+		__raw_writel(tmode, EP93XX_TIMER1_CONTROL);
+		break;
+
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_RESUME:
+		return;

These cases can be removed.

This case is something that is present in most (if not all) other architectures.
I don't mind removing it, but it will have to be replaced with:
default:
     ;

Since GCC produces warnings otherwise.

Oh, I missed the fact the switch was on an enum. You are correct, we
should keep the code consistent with other architectures, so this
can be left as is.

Other than that I am going to wait a while more for more comments if
somebody has them,
and post a new version with style fixes as well as switch to
drivers/clocksource/mmio.c usage.

Great, thanks,
~Ryan