On Tue, Dec 07, 2010 at 08:28:19PM -0800, Jeff Ohlstein wrote: > Signed-off-by: Jeff Ohlstein <johlstei@xxxxxxxxxxxxxx> I think Thomas' comments still apply to this patch. In addition: > @@ -65,49 +78,67 @@ struct msm_clock { > void __iomem *regbase; > uint32_t freq; > uint32_t shift; > + void *global_counter; > + void *local_counter; These seem to be mmio addresses, so they should be 'void __iomem *' not just 'void *'. > }; > > +enum { > + MSM_CLOCK_GPT, > + MSM_CLOCK_DGT, > + NR_TIMERS, > +}; If these are supposed to be indexes to msm_clocks[], then please use them as explicit array initializers in there - it adds useful documentation so its not necessary to search around to find out what's going on. > -static cycle_t msm_gpt_read(struct clocksource *cs) > +static cycle_t msm_read_timer_count(struct clocksource *cs) > { > - return readl(MSM_GPT_BASE + TIMER_COUNT_VAL); > + struct msm_clock *clk = container_of(cs, struct msm_clock, clocksource); > + > + return readl(clk->global_counter) >> clk->shift; > } Err. This is what the core clocksource code does: cycle_now = tc->cc->read(tc->cc); /* calculate the delta since the last timecounter_read_delta(): */ cycle_delta = (cycle_now - tc->cycle_last) & tc->cc->mask; /* convert to nanoseconds: */ ns_offset = cyclecounter_cyc2ns(tc->cc, cycle_delta); static inline u64 cyclecounter_cyc2ns(const struct cyclecounter *cc, cycle_t cycles) { u64 ret = (u64)cycles; ret = (ret * cc->mult) >> cc->shift; return ret; So doesn't this result in shifting left twice? > +#ifdef CONFIG_SMP > +void local_timer_setup(struct clock_event_device *evt) > +{ > + unsigned long flags; > + struct msm_clock *clock = &msm_clocks[MSM_GLOBAL_TIMER]; > + > + writel(DGT_CLK_CTL_DIV_4, MSM_TMR_BASE + DGT_CLK_CTL); > + > + if (!local_clock_event) { > + writel(0, clock->regbase + TIMER_ENABLE); > + writel(0, clock->regbase + TIMER_CLEAR); > + writel(~0, clock->regbase + TIMER_MATCH_VAL); > + } > + evt->irq = clock->irq.irq; > + evt->name = "local_timer"; > + evt->features = CLOCK_EVT_FEAT_ONESHOT; > + evt->rating = clock->clockevent.rating; > + evt->set_mode = msm_timer_set_mode; > + evt->set_next_event = msm_timer_set_next_event; > + evt->shift = clock->clockevent.shift; > + evt->mult = div_sc(clock->freq, NSEC_PER_SEC, evt->shift); > + evt->max_delta_ns = > + clockevent_delta2ns(0xf0000000 >> clock->shift, evt); > + evt->min_delta_ns = clockevent_delta2ns(4, evt); > + evt->cpumask = cpumask_of(smp_processor_id()); > + > + local_clock_event = evt; > + > + local_irq_save(flags); > + get_irq_chip(clock->irq.irq)->unmask(clock->irq.irq); > + local_irq_restore(flags); I can't make out what the situation is here. Are you using what is a local timer on CPU0 as a global timer? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html