[PATCH,RFC] mmp clockevent handling race
Haojian Zhuang
haojian.zhuang at marvell.com
Tue Jun 7 22:25:46 EDT 2011
On Tue, 2011-06-07 at 07:04 -0700, Lennert Buytenhek wrote:
> Hi!
>
> The patch below has been sitting around in the OLPC XO-1.75 (MMP2-based
> XO) bringup kernel tree for some time now, and upon recent rebasing of
> this tree to 3.0, it was discovered that something like this patch is
> still needed.
>
> Symptoms: e.g. as described here: http://dev.laptop.org/ticket/10945
> -- applications hang for a couple of minutes at a time, and sometimes
> there are several-minute hangs during the boot process.
>
> From the ticket:
>
> The problem in the current upstream mmp timer handling code
> that appears to be triggered here is that it handles clockevents
> by setting up a comparator on the free-running clocksource timer
> to match and trigger an interrupt at 'current_time + delta',
> which if delta is small enough can lead to 'current_time + delta'
> already being in the past when comparator setup has finished,
> and the requested event will not trigger.
>
> What this patch does is to rewrite the timer handling code to use
> two timers, one for the free-running clocksource, and one to trigger
> clockevents with, which is more or less the standard approach to this.
> It's kind of invasive, though (certainly more invasive than it strictly
> needs to be, as it reorganises time.c somewhat at the same time), and
> something like this is probably too invasive for 3.0 at this point.
>
> A safer "fix" for 3.0 may be to disallow NO_HZ/HIGH_RES_TIMERS on mmp.
>
> Any thoughts?
>
>
> thanks,
> Lennert
>
>
> Signed-off-by: Lennert Buytenhek <buytenh at laptop.org>
>
> diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c
> index 99833b9..853c9a6 100644
> --- a/arch/arm/mach-mmp/time.c
> +++ b/arch/arm/mach-mmp/time.c
> @@ -22,11 +22,9 @@
> #include <linux/kernel.h>
> #include <linux/interrupt.h>
> #include <linux/clockchips.h>
> -
> #include <linux/io.h>
> #include <linux/irq.h>
> #include <linux/sched.h>
> -
> #include <asm/sched_clock.h>
> #include <mach/addr-map.h>
> #include <mach/regs-timers.h>
> @@ -34,156 +32,230 @@
> #include <mach/irqs.h>
> #include <mach/cputype.h>
> #include <asm/mach/time.h>
> -
> #include "clock.h"
>
> #define TIMERS_VIRT_BASE TIMERS1_VIRT_BASE
>
> -#define MAX_DELTA (0xfffffffe)
> -#define MIN_DELTA (16)
> -
> -static DEFINE_CLOCK_DATA(cd);
>
> /*
> - * FIXME: the timer needs some delay to stablize the counter capture
> + * MMP sched_clock implementation.
> */
> -static inline uint32_t timer_read(void)
> +static DEFINE_CLOCK_DATA(cd);
> +
> +static inline uint32_t mmp_timer_read(void)
> {
> - int delay = 100;
> + unsigned long flags;
> + int delay;
> + u32 val;
>
> - __raw_writel(1, TIMERS_VIRT_BASE + TMR_CVWR(0));
> + raw_local_irq_save(flags);
>
> + __raw_writel(1, TIMERS_VIRT_BASE + TMR_CVWR(1));
> +
> + delay = 100;
> while (delay--)
> cpu_relax();
>
> - return __raw_readl(TIMERS_VIRT_BASE + TMR_CVWR(0));
> + val = __raw_readl(TIMERS_VIRT_BASE + TMR_CVWR(1));
> +
> + raw_local_irq_restore(flags);
> +
> + return val;
> }
>
> unsigned long long notrace sched_clock(void)
> {
> - u32 cyc = timer_read();
> + u32 cyc = mmp_timer_read();
> return cyc_to_sched_clock(&cd, cyc, (u32)~0);
> }
>
> static void notrace mmp_update_sched_clock(void)
> {
> - u32 cyc = timer_read();
> + u32 cyc = mmp_timer_read();
> update_sched_clock(&cd, cyc, (u32)~0);
> }
>
> -static irqreturn_t timer_interrupt(int irq, void *dev_id)
> -{
> - struct clock_event_device *c = dev_id;
>
> - /* disable and clear pending interrupt status */
> - __raw_writel(0x0, TIMERS_VIRT_BASE + TMR_IER(0));
> - __raw_writel(0x1, TIMERS_VIRT_BASE + TMR_ICR(0));
> - c->event_handler(c);
> - return IRQ_HANDLED;
> +/*
> + * MMP clocksource implementation.
> + */
> +static cycle_t mmp_clksrc_read(struct clocksource *cs)
> +{
> + return mmp_timer_read();
> }
>
> -static int timer_set_next_event(unsigned long delta,
> - struct clock_event_device *dev)
> +static struct clocksource mmp_cksrc = {
> + .name = "clocksource",
> + .rating = 200,
> + .read = mmp_clksrc_read,
> + .mask = CLOCKSOURCE_MASK(32),
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +
> +/*
> + * MMP clockevent implementation.
> + */
> +static u32 ticks_per_jiffy;
> +
> +static void mmp_arm_timer0(u32 value)
> {
> - unsigned long flags, next;
> + unsigned long flags;
>
> local_irq_save(flags);
>
> - /* clear pending interrupt status and enable */
> + /*
> + * Disable timer 0.
> + */
> + __raw_writel(0x02, TIMERS_VIRT_BASE + TMR_CER);
> +
> + /*
> + * Clear and enable timer match 0 interrupt.
> + */
> __raw_writel(0x01, TIMERS_VIRT_BASE + TMR_ICR(0));
> __raw_writel(0x01, TIMERS_VIRT_BASE + TMR_IER(0));
>
> - next = timer_read() + delta;
> - __raw_writel(next, TIMERS_VIRT_BASE + TMR_TN_MM(0, 0));
> + /*
> + * Setup new clockevent timer value.
> + */
> + __raw_writel(value, TIMERS_VIRT_BASE + TMR_TN_MM(0, 0));
> +
> + /*
> + * Enable timer 0.
> + */
> + __raw_writel(0x03, TIMERS_VIRT_BASE + TMR_CER);
>
> local_irq_restore(flags);
> - return 0;
> }
>
> -static void timer_set_mode(enum clock_event_mode mode,
> - struct clock_event_device *dev)
> +static int mmp_timer_set_next_event(unsigned long delta,
> + struct clock_event_device *dev)
> {
> - unsigned long flags;
> + mmp_arm_timer0(delta);
>
> - local_irq_save(flags);
> - switch (mode) {
> - case CLOCK_EVT_MODE_ONESHOT:
> - case CLOCK_EVT_MODE_UNUSED:
> - case CLOCK_EVT_MODE_SHUTDOWN:
> - /* disable the matching interrupt */
> - __raw_writel(0x00, TIMERS_VIRT_BASE + TMR_IER(0));
> - break;
> - case CLOCK_EVT_MODE_RESUME:
> - case CLOCK_EVT_MODE_PERIODIC:
> - break;
> - }
> - local_irq_restore(flags);
> + return 0;
> }
>
> -static struct clock_event_device ckevt = {
> - .name = "clockevent",
> - .features = CLOCK_EVT_FEAT_ONESHOT,
> - .shift = 32,
> - .rating = 200,
> - .set_next_event = timer_set_next_event,
> - .set_mode = timer_set_mode,
> -};
> -
> -static cycle_t clksrc_read(struct clocksource *cs)
> +static void
> +mmp_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
> {
> - return timer_read();
> + if (mode == CLOCK_EVT_MODE_PERIODIC) {
> + printk(KERN_INFO "setting periodic mode\n");
> + mmp_arm_timer0(ticks_per_jiffy - 1);
> + } else {
> + printk(KERN_INFO "setting oneshot mode\n");
> +
> + /*
> + * Disable timer 0.
> + */
> + __raw_writel(0x02, TIMERS_VIRT_BASE + TMR_CER);
> +
> + /*
> + * Clear and disable timer 0 match interrupts.
> + */
> + __raw_writel(0x01, TIMERS_VIRT_BASE + TMR_ICR(0));
> + __raw_writel(0x00, TIMERS_VIRT_BASE + TMR_IER(0));
> + }
> }
>
> -static struct clocksource cksrc = {
> - .name = "clocksource",
> - .rating = 200,
> - .read = clksrc_read,
> - .mask = CLOCKSOURCE_MASK(32),
> - .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +static struct clock_event_device mmp_ckevt = {
> + .name = "mmp_clockevent",
> + .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
> + .shift = 32,
> + .rating = 300,
> + .set_next_event = mmp_timer_set_next_event,
> + .set_mode = mmp_timer_set_mode,
> };
>
> -static void __init timer_config(void)
> +static irqreturn_t timer_interrupt(int irq, void *dev_id)
> {
> - uint32_t ccr = __raw_readl(TIMERS_VIRT_BASE + TMR_CCR);
> - uint32_t cer = __raw_readl(TIMERS_VIRT_BASE + TMR_CER);
> - uint32_t cmr = __raw_readl(TIMERS_VIRT_BASE + TMR_CMR);
> -
> - __raw_writel(cer & ~0x1, TIMERS_VIRT_BASE + TMR_CER); /* disable */
> -
> - ccr &= (cpu_is_mmp2()) ? TMR_CCR_CS_0(0) : TMR_CCR_CS_0(3);
> - __raw_writel(ccr, TIMERS_VIRT_BASE + TMR_CCR);
> + /*
> + * Clear pending interrupt status.
> + */
> + __raw_writel(0x01, TIMERS_VIRT_BASE + TMR_ICR(0));
>
> - /* free-running mode */
> - __raw_writel(cmr | 0x01, TIMERS_VIRT_BASE + TMR_CMR);
> + /*
> + * Disable timer 0 if we are in one-shot mode.
> + */
> + if (mmp_ckevt.mode != CLOCK_EVT_MODE_PERIODIC)
> + __raw_writel(0x02, TIMERS_VIRT_BASE + TMR_CER);
>
> - __raw_writel(0x0, TIMERS_VIRT_BASE + TMR_PLCR(0)); /* free-running */
> - __raw_writel(0x7, TIMERS_VIRT_BASE + TMR_ICR(0)); /* clear status */
> - __raw_writel(0x0, TIMERS_VIRT_BASE + TMR_IER(0));
> + mmp_ckevt.event_handler(&mmp_ckevt);
>
> - /* enable timer counter */
> - __raw_writel(cer | 0x01, TIMERS_VIRT_BASE + TMR_CER);
> + return IRQ_HANDLED;
> }
>
> static struct irqaction timer_irq = {
> .name = "timer",
> - .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> + .flags = IRQF_DISABLED | IRQF_TIMER,
> .handler = timer_interrupt,
> - .dev_id = &ckevt,
> };
>
> +
> +static void __init configure_timers(void)
> +{
> + /*
> + * Disable timers 0 and 1.
> + */
> + __raw_writel(0, TIMERS_VIRT_BASE + TMR_CER);
> +
> + /*
> + * Have timers 0 and 1 run off the configurable clock (6.5 MHz).
> + */
> + __raw_writel(TMR_CCR_CS_0(0) | TMR_CCR_CS_1(0),
> + TIMERS_VIRT_BASE + TMR_CCR);
> +
> + /*
> + * Set timer 0 to periodic mode, timer 1 to free-running mode.
> + */
> + __raw_writel(0x02, TIMERS_VIRT_BASE + TMR_CMR);
> +
> + /*
> + * Set timer 0 to preload from match register 0, timer 1
> + * to free-running mode.
> + */
> + __raw_writel(0x01, TIMERS_VIRT_BASE + TMR_PLCR(0));
> + __raw_writel(0x00, TIMERS_VIRT_BASE + TMR_PLCR(1));
> +
> + /*
> + * Set preload values to zero.
> + */
> + __raw_writel(0, TIMERS_VIRT_BASE + TMR_PLVR(0));
> + __raw_writel(0, TIMERS_VIRT_BASE + TMR_PLVR(1));
> +
> + /*
> + * Clear interrupt status.
> + */
> + __raw_writel(0x07, TIMERS_VIRT_BASE + TMR_ICR(0));
> + __raw_writel(0x07, TIMERS_VIRT_BASE + TMR_ICR(1));
> +
> + /*
> + * Disable interrupt enables.
> + */
> + __raw_writel(0x00, TIMERS_VIRT_BASE + TMR_IER(0));
> + __raw_writel(0x00, TIMERS_VIRT_BASE + TMR_IER(1));
> +
> + /*
> + * Enable timer 1.
> + */
> + __raw_writel(0x02, TIMERS_VIRT_BASE + TMR_CER);
> +}
> +
> void __init timer_init(int irq)
> {
> - timer_config();
> + configure_timers();
>
> init_sched_clock(&cd, mmp_update_sched_clock, 32, CLOCK_TICK_RATE);
>
> - ckevt.mult = div_sc(CLOCK_TICK_RATE, NSEC_PER_SEC, ckevt.shift);
> - ckevt.max_delta_ns = clockevent_delta2ns(MAX_DELTA, &ckevt);
> - ckevt.min_delta_ns = clockevent_delta2ns(MIN_DELTA, &ckevt);
> - ckevt.cpumask = cpumask_of(0);
> + clocksource_register_hz(&mmp_cksrc, CLOCK_TICK_RATE);
>
> - setup_irq(irq, &timer_irq);
> + ticks_per_jiffy = (CLOCK_TICK_RATE + HZ/2) / HZ;
> +
> + mmp_ckevt.mult = div_sc(CLOCK_TICK_RATE, NSEC_PER_SEC, mmp_ckevt.shift);
> + mmp_ckevt.max_delta_ns = clockevent_delta2ns(0xfffffffe, &mmp_ckevt);
> + mmp_ckevt.min_delta_ns = clockevent_delta2ns(16, &mmp_ckevt);
> + mmp_ckevt.cpumask = cpumask_of(0);
> + clockevents_register_device(&mmp_ckevt);
>
> - clocksource_register_hz(&cksrc, CLOCK_TICK_RATE);
> - clockevents_register_device(&ckevt);
> + setup_irq(irq, &timer_irq);
> }
More information about the linux-arm-kernel
mailing list