Skip to contentSkip to conversation

ARM: ep93xx: clockevent support

PATCH New Yan Burman 4 participants 6 messages Aug 7, 2012 – Aug 9, 2012

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm/Cirrus logic EP93XX Arm architecture
lore
Share & tools
lore

Yan Burman

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
arch/arm/Kconfig +1 -1
index e91c7cd..6dbb828 100644
@@ -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.
arch/arm/mach-ep93xx/core.c +100 -31

175 lines not on this page · raw message

#1 lore raw

PATCH 1/1 ARM: ep93xx: clockevent support

Quoted 6 lines · Yan Burman

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 <[email protected]>

Hi Yan, some comments below.

~Ryan

Quoted 74 lines
arch/arm/Kconfig +1 -1
index e91c7cd..6dbb828 100644
@@ -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.
arch/arm/mach-ep93xx/core.c +18 -12
index 4afe52a..1497cba 100644
@@ -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,

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 152

Quoted 10 lines
arch/arm/mach-ep93xx/core.c +10 -0
+			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.

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 160

Quoted 8 lines
arch/arm/mach-ep93xx/core.c +8 -0
+	__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.

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 169

Quoted 9 lines
arch/arm/mach-ep93xx/core.c +9 -0
+	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 16 lines
arch/arm/mach-ep93xx/core.c +12 -0
+	}
+}
+
+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,

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 192

Quoted 10 lines
arch/arm/mach-ep93xx/core.c +4 -3
 };
 
-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.

Quoted 7 lines
arch/arm/mach-ep93xx/core.c +7 -0
+	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);
+}

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 204

Quoted 10 lines
arch/arm/mach-ep93xx/core.c +4 -5
 
-	/* 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.

Quoted 4 lines
arch/arm/mach-ep93xx/core.c +4 -0
+ */
+static cycle_t ep93xx_get_cycles(struct clocksource *unused)
+{
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 215

Quoted 10 lines
arch/arm/mach-ep93xx/core.c +6 -3
+}
 
-	/* 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.

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 225

Quoted 11 lines
arch/arm/mach-ep93xx/core.c +9 -1
+	.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.

Quoted 29 lines
arch/arm/mach-ep93xx/core.c +9 -7
+	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,
 };
#2 lore raw

PATCH 1/1 ARM: ep93xx: clockevent support

Quoted 8 lines · Ryan Mallon
Quoted 6 lines · Yan Burman

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 <[email protected]>

Hello Yan,

Quoted 3 lines

Hi Yan, some comments below.

~Ryan

I have a couple minor comments as well...

<snip>

Quoted 11 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 123

Quoted 11 lines
arch/arm/mach-ep93xx/core.c +0 -4
@@ -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 42 lines
Quoted 28 lines
arch/arm/mach-ep93xx/core.c +15 -8
-	__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,

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 152

Quoted 10 lines
arch/arm/mach-ep93xx/core.c +10 -0
+			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 6 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 158

Quoted 6 lines
arch/arm/mach-ep93xx/core.c +6 -0
+	__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 2 lines

Review void __init ep93xx_map_io(void) · line 160

Quoted 2 lines
(quoted diff) +2 -0
+		tmode |= EP93XX_TIMER123_CONTROL_MODE;
+

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

Quoted 13 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 169

Quoted 9 lines
arch/arm/mach-ep93xx/core.c +9 -0
+	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 4 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 173

Quoted 4 lines
arch/arm/mach-ep93xx/core.c +4 -0
+	}
+}
+
+static struct clock_event_device clockevent_ep93xx = {

Nitpick, rename this to ep93xx_clockevent please.

Quoted 14 lines
Quoted 14 lines
arch/arm/mach-ep93xx/core.c +8 -0
+	.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,
 };
Quoted 3 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 189

Quoted 3 lines
arch/arm/mach-ep93xx/core.c +1 -1
-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 9 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 192

Quoted 5 lines
arch/arm/mach-ep93xx/core.c +3 -2
-	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:

Quoted 3 lines

#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 8 lines
Quoted 8 lines
arch/arm/mach-ep93xx/core.c +7 -0
+	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);
+}
Quoted 9 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 204

Quoted 9 lines
arch/arm/mach-ep93xx/core.c +4 -5
-	/* 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.

Quoted 8 lines

Missing space between control and register.

Quoted 6 lines
arch/arm/mach-ep93xx/core.c +5 -0
+ */
+static cycle_t ep93xx_get_cycles(struct clocksource *unused)
+{
+	return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
+}
Quoted 4 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 211

Quoted 4 lines
arch/arm/mach-ep93xx/core.c +1 -3
-	/* 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 1 line

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 212

Quoted 1 line
arch/arm/mach-ep93xx/core.c +1 -0
+	.name		= "ep93xx_timer4",

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

Quoted 7 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 219

Quoted 4 lines
arch/arm/mach-ep93xx/core.c +3 -0
+	.rating		= 200,
+	.read		= ep93xx_get_cycles,
+	.mask		= 0xFFFFFFFF,

Lower case for hex constant.

Quoted 6 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 220

Quoted 6 lines
arch/arm/mach-ep93xx/core.c +4 -1
+	.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 3 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 223

Quoted 3 lines
arch/arm/mach-ep93xx/core.c +3 -0
+{
+	/* 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 28 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 225

Quoted 2 lines
arch/arm/mach-ep93xx/core.c +2 -0
+	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 21 lines
arch/arm/mach-ep93xx/core.c +9 -6
+	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);
 }
Quoted 8 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 244

Quoted 8 lines
arch/arm/mach-ep93xx/core.c +0 -1
 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

author
#3 lore raw

PATCH 1/1 ARM: ep93xx: clockevent support

In reply to Ryan Mallon
Quoted 15 lines · Ryan Mallon
Quoted 7 lines · Yan Burman

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 <[email protected]>

Hi Yan, some comments below.

~Ryan

Quoted 13 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 169

Quoted 9 lines
arch/arm/mach-ep93xx/core.c +9 -0
+	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

#4 lore raw

PATCH 1/1 ARM: ep93xx: clockevent support

Quoted 30 lines · Yan Burman
Quoted 15 lines · Ryan Mallon
Quoted 7 lines · Yan Burman

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 <[email protected]>

Hi Yan, some comments below.

~Ryan

Quoted 13 lines

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 169

Quoted 9 lines
arch/arm/mach-ep93xx/core.c +9 -0
+	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 6 lines

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.

Quoted 4 lines

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

#5 lore raw

PATCH 1/1 ARM: ep93xx: clockevent support

In reply to Yan Burman
Quoted 11 lines · Yan Burman
arch/arm/mach-ep93xx/core.c +9 -1
+/*
+ * 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.  */

arch/arm/mach-ep93xx/core.c void __init ep93xx_map_io(void) · line 218

Quoted 10 lines
arch/arm/mach-ep93xx/core.c +8 -2
-	__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.