From e09f3cc0184d6b5c3816f921b7ffb67623e5e834 Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Thu, 18 Jul 2013 16:59:28 -0700 Subject: clocksource: arch_timer: Make register accessors less error-prone Using an enum for the register we wish to access allows newer compilers to determine if we've forgotten a case in our switch statement. This allows us to remove the BUILD_BUG() instances in the arm64 port, avoiding problems where optimizations may not happen. To try and force better code generation we're currently marking the accessor functions as inline, but newer compilers can ignore the inline keyword unless it's marked __always_inline. Luckily on arm and arm64 inline is __always_inline, but let's make everything __always_inline to be explicit. Suggested-by: Thomas Gleixner Cc: Thomas Gleixner Cc: Mark Rutland Cc: Marc Zyngier Signed-off-by: Stephen Boyd Signed-off-by: Daniel Lezcano Acked-by: Mark Rutland --- arch/arm/include/asm/arch_timer.h | 14 ++++++-------- arch/arm64/include/asm/arch_timer.h | 23 +++++++++-------------- 2 files changed, 15 insertions(+), 22 deletions(-) (limited to 'arch') diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h index accefe099182..aeb93f38e9c9 100644 --- a/arch/arm/include/asm/arch_timer.h +++ b/arch/arm/include/asm/arch_timer.h @@ -17,7 +17,8 @@ int arch_timer_arch_init(void); * nicely work out which register we want, and chuck away the rest of * the code. At least it does so with a recent GCC (4.6.3). */ -static inline void arch_timer_reg_write(const int access, const int reg, u32 val) +static __always_inline +void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val) { if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { @@ -28,9 +29,7 @@ static inline void arch_timer_reg_write(const int access, const int reg, u32 val asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val)); break; } - } - - if (access == ARCH_TIMER_VIRT_ACCESS) { + } else if (access == ARCH_TIMER_VIRT_ACCESS) { switch (reg) { case ARCH_TIMER_REG_CTRL: asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val)); @@ -44,7 +43,8 @@ static inline void arch_timer_reg_write(const int access, const int reg, u32 val isb(); } -static inline u32 arch_timer_reg_read(const int access, const int reg) +static __always_inline +u32 arch_timer_reg_read(int access, enum arch_timer_reg reg) { u32 val = 0; @@ -57,9 +57,7 @@ static inline u32 arch_timer_reg_read(const int access, const int reg) asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val)); break; } - } - - if (access == ARCH_TIMER_VIRT_ACCESS) { + } else if (access == ARCH_TIMER_VIRT_ACCESS) { switch (reg) { case ARCH_TIMER_REG_CTRL: asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val)); diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h index d56ed11ba9a3..dbca77168e81 100644 --- a/arch/arm64/include/asm/arch_timer.h +++ b/arch/arm64/include/asm/arch_timer.h @@ -26,7 +26,13 @@ #include -static inline void arch_timer_reg_write(int access, int reg, u32 val) +/* + * These register accessors are marked inline so the compiler can + * nicely work out which register we want, and chuck away the rest of + * the code. + */ +static __always_inline +void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val) { if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { @@ -36,8 +42,6 @@ static inline void arch_timer_reg_write(int access, int reg, u32 val) case ARCH_TIMER_REG_TVAL: asm volatile("msr cntp_tval_el0, %0" : : "r" (val)); break; - default: - BUILD_BUG(); } } else if (access == ARCH_TIMER_VIRT_ACCESS) { switch (reg) { @@ -47,17 +51,14 @@ static inline void arch_timer_reg_write(int access, int reg, u32 val) case ARCH_TIMER_REG_TVAL: asm volatile("msr cntv_tval_el0, %0" : : "r" (val)); break; - default: - BUILD_BUG(); } - } else { - BUILD_BUG(); } isb(); } -static inline u32 arch_timer_reg_read(int access, int reg) +static __always_inline +u32 arch_timer_reg_read(int access, enum arch_timer_reg reg) { u32 val; @@ -69,8 +70,6 @@ static inline u32 arch_timer_reg_read(int access, int reg) case ARCH_TIMER_REG_TVAL: asm volatile("mrs %0, cntp_tval_el0" : "=r" (val)); break; - default: - BUILD_BUG(); } } else if (access == ARCH_TIMER_VIRT_ACCESS) { switch (reg) { @@ -80,11 +79,7 @@ static inline u32 arch_timer_reg_read(int access, int reg) case ARCH_TIMER_REG_TVAL: asm volatile("mrs %0, cntv_tval_el0" : "=r" (val)); break; - default: - BUILD_BUG(); } - } else { - BUILD_BUG(); } return val; -- cgit v1.2.3-59-g8ed1b From 60faddf6eb3aba16068032bdcf35e18ace4bfb21 Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Thu, 18 Jul 2013 16:59:31 -0700 Subject: clocksource: arch_timer: Push the read/write wrappers deeper We're going to introduce support to read and write the memory mapped timer registers in the next patch, so push the cp15 read/write functions one level deeper. This simplifies the next patch and makes it clearer what's going on. Cc: Mark Rutland Cc: Marc Zyngier Signed-off-by: Stephen Boyd Signed-off-by: Daniel Lezcano Acked-by: Mark Rutland --- arch/arm/include/asm/arch_timer.h | 4 ++-- arch/arm64/include/asm/arch_timer.h | 4 ++-- drivers/clocksource/arm_arch_timer.c | 46 ++++++++++++++++++++++++------------ 3 files changed, 35 insertions(+), 19 deletions(-) (limited to 'arch') diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h index aeb93f38e9c9..556094689724 100644 --- a/arch/arm/include/asm/arch_timer.h +++ b/arch/arm/include/asm/arch_timer.h @@ -18,7 +18,7 @@ int arch_timer_arch_init(void); * the code. At least it does so with a recent GCC (4.6.3). */ static __always_inline -void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val) +void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val) { if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { @@ -44,7 +44,7 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val) } static __always_inline -u32 arch_timer_reg_read(int access, enum arch_timer_reg reg) +u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) { u32 val = 0; diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h index dbca77168e81..7181e777c2c5 100644 --- a/arch/arm64/include/asm/arch_timer.h +++ b/arch/arm64/include/asm/arch_timer.h @@ -32,7 +32,7 @@ * the code. */ static __always_inline -void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val) +void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val) { if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { @@ -58,7 +58,7 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val) } static __always_inline -u32 arch_timer_reg_read(int access, enum arch_timer_reg reg) +u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) { u32 val; diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 7624ba574144..a9ca28447b49 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -43,14 +43,28 @@ static bool arch_timer_use_virtual = true; * Architected system timer support. */ +static __always_inline +void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val, + struct clock_event_device *clk) +{ + arch_timer_reg_write_cp15(access, reg, val); +} + +static __always_inline +u32 arch_timer_reg_read(int access, enum arch_timer_reg reg, + struct clock_event_device *clk) +{ + return arch_timer_reg_read_cp15(access, reg); +} + static __always_inline irqreturn_t timer_handler(const int access, struct clock_event_device *evt) { unsigned long ctrl; - ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL); + ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt); if (ctrl & ARCH_TIMER_CTRL_IT_STAT) { ctrl |= ARCH_TIMER_CTRL_IT_MASK; - arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl); + arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt); evt->event_handler(evt); return IRQ_HANDLED; } @@ -72,15 +86,16 @@ static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id) return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt); } -static __always_inline void timer_set_mode(const int access, int mode) +static __always_inline void timer_set_mode(const int access, int mode, + struct clock_event_device *clk) { unsigned long ctrl; switch (mode) { case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL); + ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk); ctrl &= ~ARCH_TIMER_CTRL_ENABLE; - arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl); + arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); break; default: break; @@ -90,36 +105,37 @@ static __always_inline void timer_set_mode(const int access, int mode) static void arch_timer_set_mode_virt(enum clock_event_mode mode, struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode); + timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk); } static void arch_timer_set_mode_phys(enum clock_event_mode mode, struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode); + timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk); } -static __always_inline void set_next_event(const int access, unsigned long evt) +static __always_inline void set_next_event(const int access, unsigned long evt, + struct clock_event_device *clk) { unsigned long ctrl; - ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL); + ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk); ctrl |= ARCH_TIMER_CTRL_ENABLE; ctrl &= ~ARCH_TIMER_CTRL_IT_MASK; - arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt); - arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk); + arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); } static int arch_timer_set_next_event_virt(unsigned long evt, - struct clock_event_device *unused) + struct clock_event_device *clk) { - set_next_event(ARCH_TIMER_VIRT_ACCESS, evt); + set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk); return 0; } static int arch_timer_set_next_event_phys(unsigned long evt, - struct clock_event_device *unused) + struct clock_event_device *clk) { - set_next_event(ARCH_TIMER_PHYS_ACCESS, evt); + set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk); return 0; } -- cgit v1.2.3-59-g8ed1b