Re: [PATCH 3/5] msm: timer: SMP timer support for msm

[Thomas Gleixner <tglx@linutronix.de>]

On Sun, 5 Dec 2010, Jeff Ohlstein wrote:
> +
> +static struct msm_clock msm_clocks[];
> +static struct clock_event_device *local_clock_event;
> +
>  static irqreturn_t msm_timer_interrupt(int irq, void *dev_id)
>  {
>  	struct clock_event_device *evt = dev_id;
> +	if (smp_processor_id() != 0)
> +		evt = local_clock_event;

Why is dev_id not pointing to the correct device in the first place?

> +	if (evt->event_handler == NULL)
> +		return IRQ_HANDLED;
>  	evt->event_handler(evt);
>  	return IRQ_HANDLED;
>  }
>  
> +static uint32_t msm_read_timer_count(struct msm_clock *clock,
> +				     enum timer_location global)
> +{
> +	uint32_t t1;
> +
> +	if (global)
> +		t1 = readl(clock->regbase + TIMER_COUNT_VAL + MSM_TMR_GLOBAL);
> +	else
> +		t1 = readl(clock->regbase + TIMER_COUNT_VAL);
> +
> +	return t1;

Adding such conditionals into a fast path is brilliant. Granted, gcc
might optimize it out, but I'm not sure given the number of call sites.

What's the point of this exercise ?

> +}
> +
>  static cycle_t msm_gpt_read(struct clocksource *cs)
>  {
> -	return readl(MSM_GPT_BASE + TIMER_COUNT_VAL);
> +	struct msm_clock *clock = &msm_clocks[MSM_CLOCK_GPT];
> +	return msm_read_timer_count(clock, GLOBAL_TIMER);

Why don't you store the address of the read register including the
shift value into the msm_clock structure ?

      clock->counter_reg
      clock->counter_shift

And then embedd the clocksource struct there as well, so you can
dereference msm_clock with container_of() and reduce the 3 read
functions to a single one.

>  }
>  
>  static cycle_t msm_dgt_read(struct clocksource *cs)
>  {
> -	return readl(MSM_DGT_BASE + TIMER_COUNT_VAL) >> MSM_DGT_SHIFT;
> +	struct msm_clock *clock = &msm_clocks[MSM_CLOCK_DGT];
> +
> +	return msm_read_timer_count(clock, GLOBAL_TIMER) >> MSM_DGT_SHIFT;
> +}
> +
> +static struct msm_clock *clockevent_to_clock(struct clock_event_device *evt)
> +{
> +#ifdef CONFIG_SMP
> +	int i;
> +	for (i = 0; i < NR_TIMERS; i++)
> +		if (evt == &(msm_clocks[i].clockevent))
> +			return &msm_clocks[i];

Why don't you use container_of here as well ?

> +	return &msm_clocks[MSM_GLOBAL_TIMER];
> +#else
> +	return container_of(evt, struct msm_clock, clockevent);
> +#endif
>  }
>  
>  static int msm_timer_set_next_event(unsigned long cycles,
>  				    struct clock_event_device *evt)
>  {
> -	struct msm_clock *clock = container_of(evt, struct msm_clock, clockevent);
> -	uint32_t now = readl(clock->regbase + TIMER_COUNT_VAL);
> +	struct msm_clock *clock = clockevent_to_clock(evt);
> +	uint32_t now = msm_read_timer_count(clock, LOCAL_TIMER);
>  	uint32_t alarm = now + (cycles << clock->shift);
>  	int late;
>  
>  	writel(alarm, clock->regbase + TIMER_MATCH_VAL);
> -	now = readl(clock->regbase + TIMER_COUNT_VAL);
> +
> +	now = msm_read_timer_count(clock, LOCAL_TIMER);
>  	late = now - alarm;
>  	if (late >= (-2 << clock->shift) && late < DGT_HZ*5) {
> -		printk(KERN_NOTICE "msm_timer_set_next_event(%lu) clock %s, "
> -		       "alarm already expired, now %x, alarm %x, late %d\n",
> -		       cycles, clock->clockevent.name, now, alarm, late);
> +		static int print_limit = 10;
> +		if (print_limit > 0) {
> +			print_limit--;
> +			printk(KERN_NOTICE "msm_timer_set_next_event(%lu) "
> +			       "clock %s, alarm already expired, now %x, "
> +			       "alarm %x, late %d%s\n",
> +			       cycles, clock->clockevent.name, now, alarm, late,
> +			       print_limit ? "" : " stop printing");
> +		}

The generic clockevents layer has already a check for this. No need
for extra printk spam.

>  		return -ETIME;
>  	}
>  	return 0;
> @@ -107,7 +171,11 @@ static int msm_timer_set_next_event(unsigned long cycles,
>  static void msm_timer_set_mode(enum clock_event_mode mode,
>  			      struct clock_event_device *evt)
>  {
> -	struct msm_clock *clock = container_of(evt, struct msm_clock, clockevent);
> +	struct msm_clock *clock = clockevent_to_clock(evt);
> +	unsigned long irq_flags;
> +
> +	local_irq_save(irq_flags);

Always called with interrupts disabled.

> +
>  	switch (mode) {
>  	case CLOCK_EVT_MODE_RESUME:
>  	case CLOCK_EVT_MODE_PERIODIC:
> @@ -120,6 +188,7 @@ static void msm_timer_set_mode(enum clock_event_mode mode,
>  		writel(0, clock->regbase + TIMER_ENABLE);
>  		break;
>  	}
> +	local_irq_restore(irq_flags);
>  }
>  
>  static struct msm_clock msm_clocks[] = {
> @@ -220,6 +289,58 @@ static void __init msm_timer_init(void)
>  	}
>  }
>  
> +#ifdef CONFIG_SMP
> +void local_timer_setup(struct clock_event_device *evt)
> +{
> +	unsigned long flags;
> +	struct msm_clock *clock = &msm_clocks[MSM_GLOBAL_TIMER];
> +
> +#ifdef CONFIG_ARCH_MSM8X60
> +	writel(DGT_CLK_CTL_DIV_4, MSM_TMR_BASE + DGT_CLK_CTL);
> +#endif
> +
> +	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);

Why are you fiddling wiht the irqchip functions directly ? Please use
disable_irq/enable_irq if at all.

> +	local_irq_restore(flags);
> +
> +	clockevents_register_device(evt);
> +}
> +
> +int local_timer_ack(void)
> +{
> +	return 1;

Shouldn't that be an inline ? Why calling code which the compiler
could optimize out.

> +}
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +void __cpuexit local_timer_stop(void)
> +{
> +	local_clock_event->set_mode(CLOCK_EVT_MODE_SHUTDOWN, local_clock_event);

Aarg. No. The generic code already handles cpu hotplug. 

Thanks,

	tglx