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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
