[PATCHv3/RESEND 4/4] clocksource: arch_timer: Add support for memory mapped timers

[Thomas Gleixner]

On Fri, 21 Jun 2013, Stephen Boyd wrote:
>  /*
>   * Architected system timer support.
> @@ -46,13 +73,69 @@ static bool arch_timer_use_virtual = true;
>  static inline void arch_timer_reg_write(int access, int reg, u32 val,
>  					struct clock_event_device *clk)
>  {
> -	arch_timer_reg_write_cp15(access, reg, val);
> +	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
> +		struct arch_timer *timer = to_arch_timer(clk);
> +		switch (reg) {
> +		case ARCH_TIMER_REG_CTRL:
> +			writel_relaxed(val, timer->base + CNTP_CTL);
> +			break;
> +		case ARCH_TIMER_REG_TVAL:
> +			writel_relaxed(val, timer->base + CNTP_TVAL);
> +			break;
> +		default:
> +			BUILD_BUG();

So you are relying on the compiler cleverness to identify a caller
which calls that inline with a not supported reg value.

How does that work, when the compiler decides not to inline that?

enum without a default case emits at least a reliable warning.

> +		}
> +	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> +		struct arch_timer *timer = to_arch_timer(clk);
> +		switch (reg) {
> +		case ARCH_TIMER_REG_CTRL:
> +			writel_relaxed(val, timer->base + CNTV_CTL);
> +			break;
> +		case ARCH_TIMER_REG_TVAL:
> +			writel_relaxed(val, timer->base + CNTV_TVAL);
> +			break;
> +		default:
> +			BUILD_BUG();
> +		}
> +	} else {
> +		arch_timer_reg_write_cp15(access, reg, val);
> +	}
>  }

Something in my little brain yells: function pointer

You can't be serious about hacking nested if/else/switch constructs
into a hot path.

Why not making your cpu data:

struct arch_timer {
       struct clock_event_device evt;
       ....
       void (*write_ctrl)(val, timer);
       void (*write_tval)(val, timer);
       ....
}

and get rid of all that conditionals?

Thanks,

	tglx