* Re: [PATCH 1/1] [ARM] ep93xx clockevent support
2009-07-22 9:09 ` [PATCH 1/1] [ARM] ep93xx clockevent support Ahmed Ammar
@ 2009-07-22 9:25 ` Russell King - ARM Linux
2009-07-22 18:31 ` H Hartley Sweeten
2009-07-22 10:12 ` Ahmed Ammar
2009-07-22 18:29 ` H Hartley Sweeten
2 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2009-07-22 9:25 UTC (permalink / raw)
To: Ahmed Ammar; +Cc: linux-kernel, linux-arm-kernel, Ahmed Ammar
On Wed, Jul 22, 2009 at 12:09:46PM +0300, Ahmed Ammar wrote:
> @@ -849,6 +851,7 @@ config HZ
> default AT91_TIMER_HZ if ARCH_AT91
> default 100
>
> +
Please don't add unrelated blank lines.
> @@ -100,55 +103,129 @@ void __init ep93xx_map_io(void)
> * to use this timer for something else. We also use timer 4 for keeping
> * track of lost jiffies.
> */
> -static unsigned int last_jiffy_time;
> -
> -#define TIMER4_TICKS_PER_JIFFY ((CLOCK_TICK_RATE + (HZ/2)) / HZ)
> +static struct clock_event_device clockevent_ep93xx;
>
> static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
> {
> - __raw_writel(1, EP93XX_TIMER1_CLEAR);
> - 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(EP93XX_TC_CLEAR, EP93XX_TIMER1_CLEAR);
> + clockevent_ep93xx.event_handler(&clockevent_ep93xx);
You can avoid the forward declaration by doing:
struct clk_event_device *evt = dev_id;
evt->event_handler(evt);
here, and:
> +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,
adding here:
.dev_id = &clockevent_ep93xx,
> diff --git a/arch/arm/mach-ep93xx/include/mach/timex.h b/arch/arm/mach-ep93xx/include/mach/timex.h
> index 6b3503b..ddf3a8a 100644
> --- a/arch/arm/mach-ep93xx/include/mach/timex.h
> +++ b/arch/arm/mach-ep93xx/include/mach/timex.h
> @@ -1,5 +1,11 @@
> /*
> * arch/arm/mach-ep93xx/include/mach/timex.h
> */
> +#include <mach/ep93xx-regs.h>
> +#include <asm/io.h>
>
> #define CLOCK_TICK_RATE 983040
> +
> +#define mach_read_cycles() __raw_readl(EP93XX_TIMER4_VALUE_LOW)
> +#define mach_cycles_to_usecs(d) (((d) * ((1000000LL << 32) / CLOCK_TICK_RATE)) >> 32)
> +#define mach_usecs_to_cycles(d) (((d) * (((long long)CLOCK_TICK_RATE << 32) / 1000000)) >> 32)
Two points here:
1. You don't seem to use these in this patch, they're unrelated so please
send as a separate patch.
2. Please don't pollute mach/timex.h with anything other than the
CLOCK_TICK_RATE definition. That's what it's there for, and it gets
included by almost the entire kernel, so please don't pollute this
file.
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH 1/1] [ARM] ep93xx clockevent support
2009-07-22 9:25 ` Russell King - ARM Linux
@ 2009-07-22 18:31 ` H Hartley Sweeten
0 siblings, 0 replies; 9+ messages in thread
From: H Hartley Sweeten @ 2009-07-22 18:31 UTC (permalink / raw)
To: Russell King - ARM Linux, Ahmed Ammar
Cc: linux-kernel, linux-arm-kernel, Ahmed Ammar
On Wednesday, July 22, 2009 2:26 AM, Russell King wrote:
> On Wed, Jul 22, 2009 at 12:09:46PM +0300, Ahmed Ammar wrote:
Oops... Didn't see Russell already responded.
Ahmed,
Please follow Russell's suggestions where they differ from mine.
Thanks,
Hartley
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] [ARM] ep93xx clockevent support
2009-07-22 9:09 ` [PATCH 1/1] [ARM] ep93xx clockevent support Ahmed Ammar
2009-07-22 9:25 ` Russell King - ARM Linux
@ 2009-07-22 10:12 ` Ahmed Ammar
2009-07-30 22:18 ` H Hartley Sweeten
2009-07-22 18:29 ` H Hartley Sweeten
2 siblings, 1 reply; 9+ messages in thread
From: Ahmed Ammar @ 2009-07-22 10:12 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, Ahmed Ammar
From: Ahmed Ammar <aammar@edge-techno.com>
Based on previous work by Thomas Gleixner.
Signed-off-by: Ahmed Ammar <aammar@edge-techno.com>
---
arch/arm/Kconfig | 2 +
arch/arm/mach-ep93xx/core.c | 130 ++++++++++++++++++-----
arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h | 6 +
3 files changed, 112 insertions(+), 26 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 74713a5..f33fd06 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -273,6 +273,8 @@ config ARCH_EP93XX
select HAVE_CLK
select COMMON_CLKDEV
select ARCH_REQUIRE_GPIOLIB
+ select GENERIC_TIME
+ select GENERIC_CLOCKEVENTS
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 7983a3b..cbd9187 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -33,6 +33,9 @@
#include <linux/termios.h>
#include <linux/amba/bus.h>
#include <linux/amba/serial.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+
#include <linux/io.h>
#include <linux/i2c.h>
#include <linux/i2c-gpio.h>
@@ -100,55 +103,130 @@ void __init ep93xx_map_io(void)
* to use this timer for something else. We also use timer 4 for keeping
* track of lost jiffies.
*/
-static unsigned int last_jiffy_time;
-
-#define TIMER4_TICKS_PER_JIFFY ((CLOCK_TICK_RATE + (HZ/2)) / HZ)
static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
{
- __raw_writel(1, EP93XX_TIMER1_CLEAR);
- 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();
- }
-
+ struct clock_event_device *evt = dev_id;
+ __raw_writel(EP93XX_TC_CLEAR, 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_TC123_ENABLE, EP93XX_TIMER1_CONTROL);
+ /* program timer */
+ __raw_writel(evt, EP93XX_TIMER1_LOAD);
+ /* start timer */
+ __raw_writel(tmode | EP93XX_TC123_ENABLE, EP93XX_TIMER1_CONTROL);
+
+ return 0;
+}
+
+static void ep93xx_set_mode(enum clock_event_mode mode,
+ struct clock_event_device *evt)
+{
+ u32 tmode = EP93XX_TC123_SEL_508KHZ;
+ /* 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_TC123_PERIODIC;
+
+ case CLOCK_EVT_MODE_ONESHOT:
+ tmode |= EP93XX_TC123_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)
{
- /* Enable periodic HZ timer. */
- __raw_writel(0x48, EP93XX_TIMER1_CONTROL);
- __raw_writel((508469 / HZ) - 1, EP93XX_TIMER1_LOAD);
- __raw_writel(0xc8, EP93XX_TIMER1_CONTROL);
+ 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 lost jiffy timer. */
- __raw_writel(0x100, EP93XX_TIMER4_VALUE_HIGH);
+/*
+ * 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
+ */
- setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
+cycle_t ep93xx_get_cycles(void)
+{
+ return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
}
-static unsigned long ep93xx_gettimeoffset(void)
+static struct clocksource clocksource_ep93xx = {
+ .name = "ep93xx_timer4",
+ .rating = 200,
+ .read = ep93xx_get_cycles,
+ .mask = 0xFFFFFFFF,
+ .shift = 20,
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+/*
+ * Returns current time from boot in nsecs. It's OK for this to wrap
+ * around for now, as it's just a relative time stamp.
+ */
+unsigned long long sched_clock(void)
{
- int offset;
+ return cyc2ns(&clocksource_ep93xx, ep93xx_get_cycles());
+}
- offset = __raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time;
+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);
+}
- /* Calculate (1000000 / 983040) * offset. */
- return offset + (53 * offset / 3072);
+static void __init ep93xx_timer_init(void)
+{
+ ep93xx_clocksource_init();
+ ep93xx_clockevent_init();
}
struct sys_timer ep93xx_timer = {
- .init = ep93xx_timer_init,
- .offset = ep93xx_gettimeoffset,
+ .init = ep93xx_timer_init,
};
diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
index 36c23fb..473cdec 100644
--- a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
+++ b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
@@ -101,6 +101,12 @@
#define EP93XX_TIMER3_CONTROL EP93XX_TIMER_REG(0x88)
#define EP93XX_TIMER3_CLEAR EP93XX_TIMER_REG(0x8c)
+#define EP93XX_TC_CLEAR 0x00000001
+#define EP93XX_TC123_ENABLE 0x00000080
+#define EP93XX_TC123_PERIODIC 0x00000040
+#define EP93XX_TC123_SEL_508KHZ 0x00000008
+#define EP93XX_TC4_ENABLE 0x00000100
+
#define EP93XX_I2S_BASE (EP93XX_APB_VIRT_BASE + 0x00020000)
#define EP93XX_SECURITY_BASE (EP93XX_APB_VIRT_BASE + 0x00030000)
--
1.6.3.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH 1/1] [ARM] ep93xx clockevent support
2009-07-22 10:12 ` Ahmed Ammar
@ 2009-07-30 22:18 ` H Hartley Sweeten
2009-07-30 22:49 ` H Hartley Sweeten
0 siblings, 1 reply; 9+ messages in thread
From: H Hartley Sweeten @ 2009-07-30 22:18 UTC (permalink / raw)
To: Ahmed Ammar, linux-kernel; +Cc: linux-arm-kernel, Ahmed Ammar
On Wednesday, July 22, 2009 3:13 AM, Ahmed Ammar wrote:
> From: Ahmed Ammar <aammar@edge-techno.com>
>
> Based on previous work by Thomas Gleixner.
>
> Signed-off-by: Ahmed Ammar <aammar@edge-techno.com>
Hmm.. With kernel 2.6.30.2 this does not build. See below.
> ---
> arch/arm/Kconfig | 2 +
> arch/arm/mach-ep93xx/core.c | 130 ++++++++++++++++++-----
> arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h | 6 +
> 3 files changed, 112 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 74713a5..f33fd06 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -273,6 +273,8 @@ config ARCH_EP93XX
> select HAVE_CLK
> select COMMON_CLKDEV
> select ARCH_REQUIRE_GPIOLIB
> + select GENERIC_TIME
> + select GENERIC_CLOCKEVENTS
> 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 7983a3b..cbd9187 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -33,6 +33,9 @@
> #include <linux/termios.h>
> #include <linux/amba/bus.h>
> #include <linux/amba/serial.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +
> #include <linux/io.h>
> #include <linux/i2c.h>
> #include <linux/i2c-gpio.h>
> @@ -100,55 +103,130 @@ void __init ep93xx_map_io(void)
> * to use this timer for something else. We also use timer 4 for keeping
> * track of lost jiffies.
> */
> -static unsigned int last_jiffy_time;
> -
> -#define TIMER4_TICKS_PER_JIFFY ((CLOCK_TICK_RATE + (HZ/2)) / HZ)
>
> static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
> {
> - __raw_writel(1, EP93XX_TIMER1_CLEAR);
> - 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();
> - }
> -
> + struct clock_event_device *evt = dev_id;
> + __raw_writel(EP93XX_TC_CLEAR, EP93XX_TIMER1_CLEAR);
> + evt->event_handler(evt);
Here I get "error: dereferencing pointer to incomplete type"
> 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_TC123_ENABLE, EP93XX_TIMER1_CONTROL);
> + /* program timer */
> + __raw_writel(evt, EP93XX_TIMER1_LOAD);
> + /* start timer */
> + __raw_writel(tmode | EP93XX_TC123_ENABLE, EP93XX_TIMER1_CONTROL);
> +
> + return 0;
> +}
> +
> +static void ep93xx_set_mode(enum clock_event_mode mode,
> + struct clock_event_device *evt)
> +{
> + u32 tmode = EP93XX_TC123_SEL_508KHZ;
> + /* 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_TC123_PERIODIC;
> +
> + case CLOCK_EVT_MODE_ONESHOT:
> + tmode |= EP93XX_TC123_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)
> {
> - /* Enable periodic HZ timer. */
> - __raw_writel(0x48, EP93XX_TIMER1_CONTROL);
> - __raw_writel((508469 / HZ) - 1, EP93XX_TIMER1_LOAD);
> - __raw_writel(0xc8, EP93XX_TIMER1_CONTROL);
> + 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 lost jiffy timer. */
> - __raw_writel(0x100, EP93XX_TIMER4_VALUE_HIGH);
> +/*
> + * 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
> + */
>
> - setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
> +cycle_t ep93xx_get_cycles(void)
> +{
> + return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
> }
>
> -static unsigned long ep93xx_gettimeoffset(void)
> +static struct clocksource clocksource_ep93xx = {
> + .name = "ep93xx_timer4",
> + .rating = 200,
> + .read = ep93xx_get_cycles,
Here I get "warning: initialization from incompatible pointer type"
> + .mask = 0xFFFFFFFF,
> + .shift = 20,
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +/*
> + * Returns current time from boot in nsecs. It's OK for this to wrap
> + * around for now, as it's just a relative time stamp.
> + */
> +unsigned long long sched_clock(void)
> {
> - int offset;
> + return cyc2ns(&clocksource_ep93xx, ep93xx_get_cycles());
> +}
>
> - offset = __raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time;
> +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);
> +}
>
> - /* Calculate (1000000 / 983040) * offset. */
> - return offset + (53 * offset / 3072);
> +static void __init ep93xx_timer_init(void)
> +{
> + ep93xx_clocksource_init();
> + ep93xx_clockevent_init();
> }
>
> struct sys_timer ep93xx_timer = {
> - .init = ep93xx_timer_init,
> - .offset = ep93xx_gettimeoffset,
> + .init = ep93xx_timer_init,
> };
>
>
> diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> index 36c23fb..473cdec 100644
> --- a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> +++ b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> @@ -101,6 +101,12 @@
> #define EP93XX_TIMER3_CONTROL EP93XX_TIMER_REG(0x88)
> #define EP93XX_TIMER3_CLEAR EP93XX_TIMER_REG(0x8c)
>
> +#define EP93XX_TC_CLEAR 0x00000001
> +#define EP93XX_TC123_ENABLE 0x00000080
> +#define EP93XX_TC123_PERIODIC 0x00000040
> +#define EP93XX_TC123_SEL_508KHZ 0x00000008
> +#define EP93XX_TC4_ENABLE 0x00000100
> +
> #define EP93XX_I2S_BASE (EP93XX_APB_VIRT_BASE + 0x00020000)
>
> #define EP93XX_SECURITY_BASE (EP93XX_APB_VIRT_BASE + 0x00030000)
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH 1/1] [ARM] ep93xx clockevent support
2009-07-30 22:18 ` H Hartley Sweeten
@ 2009-07-30 22:49 ` H Hartley Sweeten
0 siblings, 0 replies; 9+ messages in thread
From: H Hartley Sweeten @ 2009-07-30 22:49 UTC (permalink / raw)
To: Ahmed Ammar, linux-kernel; +Cc: linux-arm-kernel, Ahmed Ammar
On Thursday, July 30, 2009 3:18 PM, H Hartley Sweeten wrote:
>> From: Ahmed Ammar <aammar@edge-techno.com>
>>
>> Based on previous work by Thomas Gleixner.
>>
>> Signed-off-by: Ahmed Ammar <aammar@edge-techno.com>
>
> Hmm.. With kernel 2.6.30.2 this does not build. See below.
Oops.. My bad. One of the issues was my fault...
[snip]
>> static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
>> {
>> - __raw_writel(1, EP93XX_TIMER1_CLEAR);
>> - 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();
>> - }
>> -
>> + struct clock_event_device *evt = dev_id;
>> + __raw_writel(EP93XX_TC_CLEAR, EP93XX_TIMER1_CLEAR);
>> + evt->event_handler(evt);
>
> Here I get "error: dereferencing pointer to incomplete type"
I added a typo that caused this... ;-)
[snip]
>> - setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
>> +cycle_t ep93xx_get_cycles(void)
>> +{
>> + return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
>> }
>>
>> -static unsigned long ep93xx_gettimeoffset(void)
>> +static struct clocksource clocksource_ep93xx = {
>> + .name = "ep93xx_timer4",
>> + .rating = 200,
>> + .read = ep93xx_get_cycles,
>
> Here I get "warning: initialization from incompatible pointer type"
This one is real.
The read callback is supposed to be:
cycle_t (*read)(struct clocksource *cs);
You need to fix the ep93xx_get_cycles() function and the call below.
>> + .mask = 0xFFFFFFFF,
>> + .shift = 20,
>> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
>> +};
>> +
>> +/*
>> + * Returns current time from boot in nsecs. It's OK for this to wrap
>> + * around for now, as it's just a relative time stamp.
>> + */
>> +unsigned long long sched_clock(void)
>> {
>> - int offset;
>> + return cyc2ns(&clocksource_ep93xx, ep93xx_get_cycles());
Here.
Regards,
Hartley
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] [ARM] ep93xx clockevent support
2009-07-22 9:09 ` [PATCH 1/1] [ARM] ep93xx clockevent support Ahmed Ammar
2009-07-22 9:25 ` Russell King - ARM Linux
2009-07-22 10:12 ` Ahmed Ammar
@ 2009-07-22 18:29 ` H Hartley Sweeten
2009-07-23 2:52 ` Ryan Mallon
2 siblings, 1 reply; 9+ messages in thread
From: H Hartley Sweeten @ 2009-07-22 18:29 UTC (permalink / raw)
To: Ahmed Ammar, linux-kernel; +Cc: linux-arm-kernel, Ahmed Ammar
On Wednesday, July 22, 2009 2:10 AM, Ahmed Ammar wrote:
> From: Ahmed Ammar <aammar@edge-techno.com>
>
> Based on previous work by Thomas Gleixner.
Please add some sort of commit message. And your Signed-off-by.
I still need to test this but have a couple initial comments.
>
> ---
> arch/arm/Kconfig | 3 +
> arch/arm/mach-ep93xx/core.c | 129 ++++++++++++++++++-----
> arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h | 6 +
> arch/arm/mach-ep93xx/include/mach/timex.h | 6 +
> 4 files changed, 118 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 74713a5..90ffb1d 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -273,6 +273,8 @@ config ARCH_EP93XX
> select HAVE_CLK
> select COMMON_CLKDEV
> select ARCH_REQUIRE_GPIOLIB
> + select GENERIC_TIME
> + select GENERIC_CLOCKEVENTS
> help
> This enables support for the Cirrus EP93xx series of CPUs.
>
> @@ -849,6 +851,7 @@ config HZ
> default AT91_TIMER_HZ if ARCH_AT91
> default 100
>
> +
Why this extra space?
> config AEABI
> bool "Use the ARM EABI to compile the kernel"
> help
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 7983a3b..3357df4 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -33,6 +33,9 @@
> #include <linux/termios.h>
> #include <linux/amba/bus.h>
> #include <linux/amba/serial.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +
Unnecessary extra space.
> #include <linux/io.h>
> #include <linux/i2c.h>
> #include <linux/i2c-gpio.h>
> @@ -100,55 +103,129 @@ void __init ep93xx_map_io(void)
> * to use this timer for something else. We also use timer 4 for keeping
> * track of lost jiffies.
> */
Is the comment blurb above this still valid? Does it need any extra
information based on the clockevent support?
> -static unsigned int last_jiffy_time;
> -
> -#define TIMER4_TICKS_PER_JIFFY ((CLOCK_TICK_RATE + (HZ/2)) / HZ)
> +static struct clock_event_device clockevent_ep93xx;
>
> static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
> {
> - __raw_writel(1, EP93XX_TIMER1_CLEAR);
> - 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(EP93XX_TC_CLEAR, EP93XX_TIMER1_CLEAR);
> + clockevent_ep93xx.event_handler(&clockevent_ep93xx);
> return IRQ_HANDLED;
> }
>
> +
Unnecessary extra space.
> +static int ep93xx_set_next_event(unsigned long evt,
> + struct clock_event_device *unused)
Whitespace issue here. Maybe email related?
> +{
> + u32 tmode = __raw_readl(EP93XX_TIMER1_CONTROL);
> +
> + /* stop timer */
> + __raw_writel(tmode & ~EP93XX_TC123_ENABLE, EP93XX_TIMER1_CONTROL);
> + /* program timer */
> + __raw_writel(evt, EP93XX_TIMER1_LOAD);
> + /* start timer */
> + __raw_writel(tmode | EP93XX_TC123_ENABLE, EP93XX_TIMER1_CONTROL);
> +
> + return 0;
> +}
> +
> +static void ep93xx_set_mode(enum clock_event_mode mode,
> + struct clock_event_device *evt)
> +{
> + u32 tmode = EP93XX_TC123_SEL_508KHZ;
> + /* 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_TC123_PERIODIC;
> +
> + case CLOCK_EVT_MODE_ONESHOT:
> + tmode |= EP93XX_TC123_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,
> +};
With the clockevent support is it possible to add the other 2 timers?
> +
> static struct irqaction ep93xx_timer_irq = {
> .name = "ep93xx timer",
> .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> .handler = ep93xx_timer_interrupt,
> };
>
> -static void __init ep93xx_timer_init(void)
> +static void __init ep93xx_clockevent_init(void)
> {
> - /* Enable periodic HZ timer. */
> - __raw_writel(0x48, EP93XX_TIMER1_CONTROL);
> - __raw_writel((508469 / HZ) - 1, EP93XX_TIMER1_LOAD);
> - __raw_writel(0xc8, EP93XX_TIMER1_CONTROL);
> + 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 lost jiffy timer. */
> - __raw_writel(0x100, EP93XX_TIMER4_VALUE_HIGH);
> +/*
> + * 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
> + */
>
> - setup_irq(IRQ_EP93XX_TIMER1, &ep93xx_timer_irq);
> +cycle_t ep93xx_get_cycles(void)
> +{
> + return __raw_readl(EP93XX_TIMER4_VALUE_LOW);
> }
>
> -static unsigned long ep93xx_gettimeoffset(void)
> +static struct clocksource clocksource_ep93xx = {
> + .name = "ep93xx_timer4",
> + .rating = 200,
> + .read = ep93xx_get_cycles,
> + .mask = 0xFFFFFFFF,
> + .shift = 20,
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +/*
> + * Returns current time from boot in nsecs. It's OK for this to wrap
> + * around for now, as it's just a relative time stamp.
> + */
> +unsigned long long sched_clock(void)
> {
> - int offset;
> + return cyc2ns(&clocksource_ep93xx, ep93xx_get_cycles());
> +}
>
> - offset = __raw_readl(EP93XX_TIMER4_VALUE_LOW) - last_jiffy_time;
> +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);
> +}
>
> - /* Calculate (1000000 / 983040) * offset. */
> - return offset + (53 * offset / 3072);
> +static void __init ep93xx_timer_init(void)
> +{
> + ep93xx_clocksource_init();
> + ep93xx_clockevent_init();
> }
>
> struct sys_timer ep93xx_timer = {
> - .init = ep93xx_timer_init,
> - .offset = ep93xx_gettimeoffset,
> + .init = ep93xx_timer_init,
Please remove one of the tabs after .init.
> };
>
>
> diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> index 36c23fb..473cdec 100644
> --- a/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> +++ b/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> @@ -101,6 +101,12 @@
> #define EP93XX_TIMER3_CONTROL EP93XX_TIMER_REG(0x88)
> #define EP93XX_TIMER3_CLEAR EP93XX_TIMER_REG(0x8c)
>
> +#define EP93XX_TC_CLEAR 0x00000001
This define is not needed. Writing any value to the associated
register clears the interrupt generated by the time. Just
change the line in core.c to write the value directly as was
previously done.
> +#define EP93XX_TC123_ENABLE 0x00000080
> +#define EP93XX_TC123_PERIODIC 0x00000040
> +#define EP93XX_TC123_SEL_508KHZ 0x00000008
I would prefer these defines be like the others in this file:
#define EP93XX_TIMER1_CONTROL EP93XX_TIMER_REG(0x08)
+#define EP93XX_TIMER_CONTROL_ENABLE (1<<7)
+#define EP93XX_TIMER_CONTROL_MODE (1<<6)
+#define EP93XX_TIMER_CONTROL_CLKSEL (1<<3)
> +#define EP93XX_TC4_ENABLE 0x00000100
And this one as:
#define EP93XX_TIMER4_VALUE_HIGH EP93XX_TIMER_REG(0x64)
#define EP93XX_TIMER4_VALUE_HIGH_ENABLE (1<<8)
> +
> #define EP93XX_I2S_BASE (EP93XX_APB_VIRT_BASE + 0x00020000)
>
> #define EP93XX_SECURITY_BASE (EP93XX_APB_VIRT_BASE + 0x00030000)
> diff --git a/arch/arm/mach-ep93xx/include/mach/timex.h b/arch/arm/mach-ep93xx/include/mach/timex.h
> index 6b3503b..ddf3a8a 100644
> --- a/arch/arm/mach-ep93xx/include/mach/timex.h
> +++ b/arch/arm/mach-ep93xx/include/mach/timex.h
> @@ -1,5 +1,11 @@
> /*
> * arch/arm/mach-ep93xx/include/mach/timex.h
> */
> +#include <mach/ep93xx-regs.h>
<mach/hardware.h> please
> +#include <asm/io.h>
Should be <linux/io.h> and moved before the <mach/*> include.
>
> #define CLOCK_TICK_RATE 983040
> +
> +#define mach_read_cycles() __raw_readl(EP93XX_TIMER4_VALUE_LOW)
> +#define mach_cycles_to_usecs(d) (((d) * ((1000000LL << 32) / CLOCK_TICK_RATE)) >> 32)
> +#define mach_usecs_to_cycles(d) (((d) * (((long long)CLOCK_TICK_RATE << 32) / 1000000)) >> 32)
Please reformat these lines to be < 80 characters. Maybe like:
+ #define mach_read_cycles() __raw_readl(EP93XX_TIMER4_VALUE_LOW)
+
+ #define mach_cycles_to_usecs(d) \
+ (((d) * ((1000000LL << 32) / CLOCK_TICK_RATE)) >> 32)
+ #define mach_usecs_to_cycles(d) \
+ (((d) * (((long long)CLOCK_TICK_RATE << 32) / 1000000)) >> 32)
What's the advantage of adding clockevent support? I'm not familiar
with this yet.
Thanks,
Hartley
^ permalink raw reply [flat|nested] 9+ messages in thread