[RFC 2/8] ARM:global_timer: Add ARM global timer support.
From: Linus Walleij <hidden>
Date: 2013-05-13 19:05:00
Also in:
linux-devicetree, linux-serial
On Wed, May 8, 2013 at 4:11 PM, Srinivas KANDAGATLA [off-list ref] wrote: Thomas Gleixner and John Stultz should always be included on timer code review. This is one reason I really like to move the clocksource/clockevent/etc code out of arch/arm, so that people can get their To: line right from MAINTAINERS. (Whether it goes to drivers/clocksource or not is another issue.)
quoted hunk
diff --git a/arch/arm/include/asm/global_timer.h b/arch/arm/include/asm/global_timer.h
Using CLKSRC_OF_INIT() this header goes away entirely I guess.
quoted hunk
diff --git a/arch/arm/kernel/global_timer.c b/arch/arm/kernel/global_timer.c
+#define GT_COUNTER0 0x00 +#define GT_COUNTER1 0x04
So two counters, nice.
quoted hunk
+union gt_counter { + cycle_t cycles; + struct { + uint32_t lower; + uint32_t upper;
Just u32 is fine.
quoted hunk
+ }; +}; + +static union gt_counter gt_counter_read(void) +{ + union gt_counter res; + uint32_t upper;
u32
quoted hunk
+ + upper = readl(gt_base + GT_COUNTER1); + do { + res.upper = upper; + res.lower = readl(gt_base + GT_COUNTER0); + upper = readl(gt_base + GT_COUNTER1); + } while (upper != res.upper); + + return res; +}
I guess this is some cleverness to avoid wrap-around... A comment stating what's going on wouldn't hurt. But why on earth do you need this complicated union to hold the result? Have two local u32 variables and return a u64 from this function. Since this will be performance critical, isn't readl_relaxed() suffient here, or what's the rationale for just using readl()?
quoted hunk
+static void gt_compare_set(unsigned long delta, int periodic) +{ + union gt_counter counter = gt_counter_read();
So here you get something easy to read like u64 counter = gt_counter_read();
quoted hunk
+ unsigned long ctrl = readl(gt_base + GT_CONTROL); + + BUG_ON(!(ctrl & GT_CONTROL_TIMER_ENABLE)); + BUG_ON(ctrl & (GT_CONTROL_COMP_ENABLE | + GT_CONTROL_IRQ_ENABLE | + GT_CONTROL_AUTO_INC));
Do you really have to check this *whenever the counter is set*? Will it not suffice to do this once during initialization? It looks like some leftover debug code. It also looks kind of clock dangerous, can you explain exactly why this check is here?
quoted hunk
+ + counter.cycles += delta; + writel(counter.lower, gt_base + GT_COMP0); + writel(counter.upper, gt_base + GT_COMP1);
This is another instance of the union struct making things complicated. With a u64 just: counter += delta; writel(lower_32_bits(counter), gt_base + GT_COMP0); writel(upper_32_bits(counter), gt_base + GT_COMP1); As you can see <linux/kernel.h> has really nice helpers in place to deal with 64 bit arithmetics.
quoted hunk
+ + ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE; + + if (periodic) { + writel(delta, gt_base + GT_AUTO_INC); + ctrl |= GT_CONTROL_AUTO_INC; + } + + writel(ctrl, gt_base + GT_CONTROL); +} + +static void gt_clockevent_set_mode(enum clock_event_mode mode, + struct clock_event_device *clk) +{ + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + gt_compare_set(gt_clk_rate/HZ, 1);
Use gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1); to get integer arithmetics right.
quoted hunk
+ break; + case CLOCK_EVT_MODE_ONESHOT: + /* period set, and timer enabled in 'next_event' hook */ + BUG_ON(readl(gt_base + GT_CONTROL) & + (GT_CONTROL_COMP_ENABLE | + GT_CONTROL_IRQ_ENABLE | + GT_CONTROL_AUTO_INC));
Here is another one of these checks. Why aren't you just zeroing these flags if you don't want them, especially since you're writing the register at the end of the function? Now it looks like instead of setting up the timer the way you want it you bug out if it isn't already set up as you want it (hint, the name of this function).
quoted hunk
+ /* Fall through */ + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + default: + writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
Why are you enabling the timer in unused and shutdown mode? This doesn't make sense.
quoted hunk
+ break; + } +} + +static int gt_clockevent_set_next_event(unsigned long evt, + struct clock_event_device *unused) +{ + gt_compare_set(evt, 0); + return 0; +} + +static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id) +{ + struct clock_event_device *evt = *(struct clock_event_device **)dev_id; + + writel(GT_INT_STATUS_EVENT_FLAG, gt_base + GT_INT_STATUS);
You're clearing the flag before checking if it was set. What happens if this was a spurious interrupt that should be disregarded?
quoted hunk
+ evt->event_handler(evt); + + return IRQ_HANDLED; +} + +static int __cpuinit gt_clockevents_init(struct clock_event_device *clk) +{ + struct clock_event_device **this_cpu_clk; + int cpu = smp_processor_id(); + + clk->name = "Global Timer CE";
Name it after the hardware feature. "ARM global timer clock event" there is no need to abbreviate randomly.
quoted hunk
+ clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; + clk->set_mode = gt_clockevent_set_mode; + clk->set_next_event = gt_clockevent_set_next_event; + this_cpu_clk = __this_cpu_ptr(gt_evt); + *this_cpu_clk = clk; + clk->irq = gt_ppi; + clockevents_config_and_register(clk, gt_clk_rate, + 0xf, 0xffffffff);
Why can't this clock event handle anything lower than 0xf? Does that come from the datasheet or have you just copied some code? Further, since this clock event hardware *most definately* supports using a delta upper bound *beyond* 32 bits, I think the clock event core code should be altered to allow for registereing such clock events, but TGLX may have some idea here. This will work but will not expose the full potential of this 64-bit counter hardware.
quoted hunk
+ per_cpu(percpu_init_called, cpu) = true; + enable_percpu_irq(clk->irq, IRQ_TYPE_NONE); + return 0; +} + +static void gt_clockevents_stop(struct clock_event_device *clk) +{ + gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, clk); + disable_percpu_irq(clk->irq); +} + +static int __cpuinit gt_clockevents_setup(struct clock_event_device *clk) +{ + int cpu = smp_processor_id(); + + /* Use existing clock_event for boot cpu */ + if (per_cpu(percpu_init_called, cpu)) + return 0; + + writel(0, gt_base + GT_CONTROL); + writel(0, gt_base + GT_COUNTER0); + writel(0, gt_base + GT_COUNTER1); + writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); + + return gt_clockevents_init(clk); +} + +static cycle_t gt_clocksource_read(struct clocksource *cs) +{ + union gt_counter res = gt_counter_read(); + return res.cycles; +} + +static struct clocksource gt_clocksource = { + .name = "Global Timer CS",
"ARM global timer clock source"
quoted hunk
+ .rating = 300, + .read = gt_clocksource_read, + .mask = CLOCKSOURCE_MASK(64), + .flags = CLOCK_SOURCE_IS_CONTINUOUS, +}; + +static void __init gt_clocksource_init(void) +{ + writel(0, gt_base + GT_CONTROL); + writel(0, gt_base + GT_COUNTER0); + writel(0, gt_base + GT_COUNTER1); + writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); + + gt_clocksource.shift = 20;
So how did you come up with that?
quoted hunk
+ gt_clocksource.mult = + clocksource_hz2mult(gt_clk_rate, gt_clocksource.shift); + clocksource_register(>_clocksource);
Why don't you just replace all of this hazzle with: clocksource_register_hz(>_clocksource, gt_clk_rate); That will calculate mult and shift for you? (Leave these unassigned.) Since the hpet timer does this it's pretty well tested... No more comments right now... Yours, Linus Walleij