Skip to content

Commit e09f3cc

Browse files
bebarinodlezcano
bebarino
authored and
dlezcano
committed
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 <tglx@linutronix.de> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <Marc.Zyngier@arm.com> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Mark Rutland <mark.rutland@arm.com>
1 parent 766acb8 commit e09f3cc

4 files changed

Lines changed: 22 additions & 27 deletions

File tree

‎arch/arm/include/asm/arch_timer.h‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ int arch_timer_arch_init(void);
1717
* nicely work out which register we want, and chuck away the rest of
1818
* the code. At least it does so with a recent GCC (4.6.3).
1919
*/
20-
static inline void arch_timer_reg_write(const int access, const int reg, u32 val)
20+
static __always_inline
21+
void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val)
2122
{
2223
if (access == ARCH_TIMER_PHYS_ACCESS) {
2324
switch (reg) {
@@ -28,9 +29,7 @@ static inline void arch_timer_reg_write(const int access, const int reg, u32 val
2829
asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
2930
break;
3031
}
31-
}
32-
33-
if (access == ARCH_TIMER_VIRT_ACCESS) {
32+
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
3433
switch (reg) {
3534
case ARCH_TIMER_REG_CTRL:
3635
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
4443
isb();
4544
}
4645

47-
static inline u32 arch_timer_reg_read(const int access, const int reg)
46+
static __always_inline
47+
u32 arch_timer_reg_read(int access, enum arch_timer_reg reg)
4848
{
4949
u32 val = 0;
5050

@@ -57,9 +57,7 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
5757
asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
5858
break;
5959
}
60-
}
61-
62-
if (access == ARCH_TIMER_VIRT_ACCESS) {
60+
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
6361
switch (reg) {
6462
case ARCH_TIMER_REG_CTRL:
6563
asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));

‎arch/arm64/include/asm/arch_timer.h‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,13 @@
2626

2727
#include <clocksource/arm_arch_timer.h>
2828

29-
static inline void arch_timer_reg_write(int access, int reg, u32 val)
29+
/*
30+
* These register accessors are marked inline so the compiler can
31+
* nicely work out which register we want, and chuck away the rest of
32+
* the code.
33+
*/
34+
static __always_inline
35+
void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val)
3036
{
3137
if (access == ARCH_TIMER_PHYS_ACCESS) {
3238
switch (reg) {
@@ -36,8 +42,6 @@ static inline void arch_timer_reg_write(int access, int reg, u32 val)
3642
case ARCH_TIMER_REG_TVAL:
3743
asm volatile("msr cntp_tval_el0, %0" : : "r" (val));
3844
break;
39-
default:
40-
BUILD_BUG();
4145
}
4246
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
4347
switch (reg) {
@@ -47,17 +51,14 @@ static inline void arch_timer_reg_write(int access, int reg, u32 val)
4751
case ARCH_TIMER_REG_TVAL:
4852
asm volatile("msr cntv_tval_el0, %0" : : "r" (val));
4953
break;
50-
default:
51-
BUILD_BUG();
5254
}
53-
} else {
54-
BUILD_BUG();
5555
}
5656

5757
isb();
5858
}
5959

60-
static inline u32 arch_timer_reg_read(int access, int reg)
60+
static __always_inline
61+
u32 arch_timer_reg_read(int access, enum arch_timer_reg reg)
6162
{
6263
u32 val;
6364

@@ -69,8 +70,6 @@ static inline u32 arch_timer_reg_read(int access, int reg)
6970
case ARCH_TIMER_REG_TVAL:
7071
asm volatile("mrs %0, cntp_tval_el0" : "=r" (val));
7172
break;
72-
default:
73-
BUILD_BUG();
7473
}
7574
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
7675
switch (reg) {
@@ -80,11 +79,7 @@ static inline u32 arch_timer_reg_read(int access, int reg)
8079
case ARCH_TIMER_REG_TVAL:
8180
asm volatile("mrs %0, cntv_tval_el0" : "=r" (val));
8281
break;
83-
default:
84-
BUILD_BUG();
8582
}
86-
} else {
87-
BUILD_BUG();
8883
}
8984

9085
return val;

‎drivers/clocksource/arm_arch_timer.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ static bool arch_timer_use_virtual = true;
4343
* Architected system timer support.
4444
*/
4545

46-
static inline irqreturn_t timer_handler(const int access,
46+
static __always_inline irqreturn_t timer_handler(const int access,
4747
struct clock_event_device *evt)
4848
{
4949
unsigned long ctrl;
@@ -72,7 +72,7 @@ static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
7272
return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
7373
}
7474

75-
static inline void timer_set_mode(const int access, int mode)
75+
static __always_inline void timer_set_mode(const int access, int mode)
7676
{
7777
unsigned long ctrl;
7878
switch (mode) {
@@ -99,7 +99,7 @@ static void arch_timer_set_mode_phys(enum clock_event_mode mode,
9999
timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode);
100100
}
101101

102-
static inline void set_next_event(const int access, unsigned long evt)
102+
static __always_inline void set_next_event(const int access, unsigned long evt)
103103
{
104104
unsigned long ctrl;
105105
ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);

‎include/clocksource/arm_arch_timer.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@
2323
#define ARCH_TIMER_CTRL_IT_MASK (1 << 1)
2424
#define ARCH_TIMER_CTRL_IT_STAT (1 << 2)
2525

26-
#define ARCH_TIMER_REG_CTRL 0
27-
#define ARCH_TIMER_REG_TVAL 1
26+
enum arch_timer_reg {
27+
ARCH_TIMER_REG_CTRL,
28+
ARCH_TIMER_REG_TVAL,
29+
};
2830

2931
#define ARCH_TIMER_PHYS_ACCESS 0
3032
#define ARCH_TIMER_VIRT_ACCESS 1

0 commit comments

Comments
 (0)