[RFC 2/8] ARM:global_timer: Add ARM global timer support.
Srinivas KANDAGATLA
srinivas.kandagatla at st.com
Tue May 14 04:46:38 EDT 2013
Thankyou for the comments.
On 13/05/13 20:05, Linus Walleij wrote:
> On Wed, May 8, 2013 at 4:11 PM, Srinivas KANDAGATLA
> <srinivas.kandagatla at st.com> 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.)
>
>> 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.
>
>> 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.
>
>> +union gt_counter {
>> + cycle_t cycles;
>> + struct {
>> + uint32_t lower;
>> + uint32_t upper;
>
> Just u32 is fine.
It makes sense to remove this struct totally and use u64 as suggested.
>
>> + };
>> +};
>> +
>> +static union gt_counter gt_counter_read(void)
>> +{
>> + union gt_counter res;
>> + uint32_t upper;
>
> u32
>
>> +
>> + 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.
This cleverness is defined in the datasheet
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407i/CIHGECHJ.html
I will add a comment here.
>
> 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()?
Yes, it makes sense to go for readl_relaxed here as it is device-memory
type.
>
>> +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();
>
>> + 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?
I will get this debug out of here.
>
>> +
>> + 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.
Same as first comment.
>
>> +
>> + 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.
Will move to use DIV_ROUND_CLOSEST.
>
>> + 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).
>
>> + /* 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.
It is because we are using the global-timer block for both clocksource
and clockevents and we do not want the clocksource to disappear when
clockevent is unused/shutdown.
>
>> + 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?
Makes sense, I will add a check here.
>
>> + 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.
Yes, will change all such instances.
>
>> + 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?
There is no such limitation on the minimum clock ticks, I think it was
copied.
Will fix it in next version.
>
> 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.
>
>> + 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"
>
>> + .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?
Hmm..
You are right, this should not be a constant here, I will use
clocksource_register_hz instead.
-srini
>
>> + 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
>
>
More information about the linux-arm-kernel
mailing list