ARM64: arch_timer: Work around QorIQ Erratum A-008585
Subsystems (5)
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.
This erratum can be found on LS1043A and LS2080A.
---
v2:
Significant rework based on feedback, including using static_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.
Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate set_next_event() code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.
--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)
Documentation/devicetree/bindings/arm/arch_timer.txt +6 -0
@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context. +- fsl,erratum-a008585 : A boolean property. Indicates the presence of + QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties: - arm,cpu-registers-not-fw-configured : Firmware does not initialize
arch/arm64/include/asm/arch_timer.h +28 -9
@@ -23,10 +23,33 @@#include <linux/bug.h> #include <linux/init.h> +#include <linux/jump_label.h> #include <linux/types.h> #include <clocksource/arm_arch_timer.h> +extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ (reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ + else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval) +ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ ("cntvct_el0" , arch_counter_get_cntvct) +/* * These register accessors are marked inline so the compiler can * nicely work out which register we want, and chuck away the rest of
@@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)if (access ==ARCH_TIMER_PHYS_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val));break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntp_tval_el0" :"=r" (val)); + val =_arch_timer_get_ptval ();break ; } }else if (access ==ARCH_TIMER_VIRT_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val));break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntv_tval_el0" :"=r" (val)); + val =_arch_timer_get_vtval ();break ; } }
@@ -116,12 +139,8 @@ static inline u64 arch_counter_get_cntpct(void)static inline u64 arch_counter_get_cntvct (void ) { -u64 cval; -isb (); - asmvolatile ("mrs %0, cntvct_el0" :"=r" (cval)); - -return cval; +return _arch_counter_get_cntvct (); }static inline int arch_timer_arch_init (void )
drivers/clocksource/arm_arch_timer.c +110 -0
150 lines not on this page · raw message
Signature · 1 line
2.5.0
PATCH v2 2/2 arm64: dts: Add timer erratum property for LS2080A and LS1043A
--- arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 1 + arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 1 + 2 files changed, 2 insertions(+)
arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi +1 -0
@@ -115,6 +115,7 @@ <1 14 0x1>, /* Physical Non-Secure PPI */ <1 11 0x1>, /* Virtual PPI */ <1 10 0x1>; /* Hypervisor PPI */ + fsl,erratum-a008585; }; pmu {
arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi +1 -0
@@ -171,6 +171,7 @@ <1 14 0x8>, /* Physical Non-Secure PPI, active-low */ <1 11 0x8>, /* Virtual PPI, active-low */ <1 10 0x8>; /* Hypervisor PPI, active-low */ + fsl,erratum-a008585; }; pmu {
Signature · 1 line
2.5.0
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
In reply to Scott WoodOn Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:
Hi Scott,
Quoted 48 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.
This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context.
Documentation/devicetree/bindings/arm/arch_timer.txt to deliver its interrupts via SPIs. · line 36
Quoted 10 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +6
-0+- fsl,erratum-a008585 : A boolean property. Indicates the presence of + QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties: - arm,cpu-registers-not-fw-configured : Firmware does not initialize
This should be part of a separate patch. Also, errata should be documented in Documentation/arm64/silicon-errata.txt.
Quoted 25 lines
arch/arm64/include/asm/arch_timer.h +13
-0@@ -23,10 +23,33 @@#include <linux/bug.h> #include <linux/init.h> +#include <linux/jump_label.h> #include <linux/types.h> #include <clocksource/arm_arch_timer.h> +extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ (reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \
arch/arm64/include/asm/arch_timer.h line 52
Quoted 10 lines
arch/arm64/include/asm/arch_timer.h +10
-0+if (static_branch_unlikely (&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ +else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval) +ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ ("cntvct_el0" , arch_counter_get_cntvct) +
Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.
arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 92
Quoted 9 lines
arch/arm64/include/asm/arch_timer.h +1
-1/* * These register accessors are marked inline so the compiler can * nicely work out which register we want, and chuck away the rest of @@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)if (access ==ARCH_TIMER_PHYS_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val));
Spurious change?
arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 101
Quoted 11 lines
arch/arm64/include/asm/arch_timer.h +2
-2break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntp_tval_el0" :"=r" (val)); + val =_arch_timer_get_ptval ();break ; } }else if (access ==ARCH_TIMER_VIRT_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val));
Same here.
Quoted 26 lines
arch/arm64/include/asm/arch_timer.h +2
-6break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntv_tval_el0" :"=r" (val)); + val =_arch_timer_get_vtval ();break ; } }@@ -116,12 +139,8 @@ static inline u64 arch_counter_get_cntpct(void)static inline u64 arch_counter_get_cntvct (void ) { -u64 cval; -isb (); - asmvolatile ("mrs %0, cntvct_el0" :"=r" (cval)); - -return cval; +return _arch_counter_get_cntvct (); }static inline int arch_timer_arch_init (void )drivers/clocksource/arm_arch_timer.c +0
-0
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 84
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +3
-0@@ -79,10 +79,52 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;static bool arch_timer_c3stop;static bool arch_timer_mem_use_virtual; +DEFINE_STATIC_KEY_FALSE (arch_timer_read_ool_enabled); +EXPORT_SYMBOL_GPL (arch_timer_read_ool_enabled); +
Once you have a config option, this can move to the guarded section below...
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 89
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +1
-0/* * Architected system timer support. */ +#ifdef CONFIG_ARM64
which can become CONFIG_FSL_ERRATUM_008585
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 95
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +6
-0+/* +* __always_inline is used to ensure that func() is not an actual function +* pointer, which would result in the register accesses potentially being too +* far apart for the loop to work. +*/ +static __always_inline u64 arch_timer_reread(u64 (*func)(void))
This is quite a generic function name. I'd expect something that refers to the erratum number.
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 98
Quoted 3 lines
drivers/clocksource/arm_arch_timer.c +3
-0+{ +u64 cval_old, cval_new; +int timeout =200 ;
Can we have a comment on how this value has been chosen?
Quoted 37 lines
drivers/clocksource/arm_arch_timer.c +30
-0+ +do { +isb (); + cval_old =func (); + cval_new =func (); + timeout--; + }while (cval_old != cval_new && timeout); + +WARN_ON_ONCE (!timeout); +return cval_new; +} + +u64 arch_counter_get_cntvct_ool (void ) +{ +return arch_timer_reread (__arch_counter_get_cntvct); +} + +u64 arch_timer_get_vtval_ool (void ) +{ +return arch_timer_reread (__arch_timer_get_vtval); +} + +u64 arch_timer_get_ptval_ool (void ) +{ +return arch_timer_reread (__arch_timer_get_ptval); +} + +#endif /* ARM64 */ +static __always_inline void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,struct clock_event_device *clk)@@ -232,6 +274,50 @@ static __always_inline void set_next_event(const int access, unsigned long evt,arch_timer_reg_write (access,ARCH_TIMER_REG_CTRL , ctrl, clk); } +#ifdef CONFIG_ARM64
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 287
Quoted 10 lines
drivers/clocksource/arm_arch_timer.c +10
-0+static __always_inline void rewrite_tval(const int access, +unsigned long evt,struct clock_event_device *clk) +{ +u64 cval_old, cval_new; +int timeout =200 ; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk); + cval_new = __arch_counter_get_cntvct();
Don't you need to guarantee the order of accesses here?
Quoted 21 lines
drivers/clocksource/arm_arch_timer.c +21
-0+ timeout--; + }while (cval_old != cval_new && timeout); + +WARN_ON_ONCE (!timeout); +} + +static __always_inline void set_next_event_errata(const int access, +unsigned long evt,struct clock_event_device *clk) +{ +unsigned long ctrl; + + ctrl =arch_timer_reg_read (access,ARCH_TIMER_REG_CTRL , clk); + ctrl |=ARCH_TIMER_CTRL_ENABLE ; + ctrl &= ~ARCH_TIMER_CTRL_IT_MASK ; +rewrite_tval (access, evt, clk); +arch_timer_reg_write (access,ARCH_TIMER_REG_CTRL , ctrl, clk); +} + +static int arch_timer_set_next_event_virt_errata (unsigned long evt, +struct clock_event_device *clk) +{
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 318
Quoted 10 lines
drivers/clocksource/arm_arch_timer.c +10
-0+set_next_event_errata (ARCH_TIMER_VIRT_ACCESS , evt, clk); +return 0 ; +} + +static int arch_timer_set_next_event_phys_errata (unsigned long evt, +struct clock_event_device *clk) +{ +set_next_event_errata (ARCH_TIMER_PHYS_ACCESS , evt, clk); +return 0 ; +}
Same comment about the function names.
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +2
-0+#endif /* ARM64 */ +static int arch_timer_set_next_event_virt (unsigned long evt,struct clock_event_device *clk) {
drivers/clocksource/arm_arch_timer.c static void __arch_timer_setup(unsigned type, · line 370
Quoted 8 lines
drivers/clocksource/arm_arch_timer.c +5
-0@@ -277,6 +363,13 @@ static void __arch_timer_setup(unsigned type, clk->set_state_shutdown = arch_timer_shutdown_virt; clk->set_state_oneshot_stopped = arch_timer_shutdown_virt; clk->set_next_event = arch_timer_set_next_event_virt; + +#ifdef CONFIG_ARM64 +if (static_branch_unlikely (&arch_timer_read_ool_enabled)) + clk->set_next_event = + arch_timer_set_next_event_virt_errata;
On the same line, please. You could also rewrite this as a helper function that would populate the set_next_event field in all cases.
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +2
-0+#endif +break ;case PHYS_SECURE_PPI :case PHYS_NONSECURE_PPI :
drivers/clocksource/arm_arch_timer.c static void __arch_timer_setup(unsigned type, · line 384
Quoted 8 lines
drivers/clocksource/arm_arch_timer.c +5
-0@@ -284,6 +377,13 @@ static void __arch_timer_setup(unsigned type, clk->set_state_shutdown = arch_timer_shutdown_phys; clk->set_state_oneshot_stopped = arch_timer_shutdown_phys; clk->set_next_event = arch_timer_set_next_event_phys; + +#ifdef CONFIG_ARM64 +if (static_branch_unlikely (&arch_timer_read_ool_enabled)) + clk->set_next_event = + arch_timer_set_next_event_phys_errata;
Same here.
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +2
-0+#endif +break ;default :BUG ();
drivers/clocksource/arm_arch_timer.c static void __init arch_counter_register(unsigned type) · line 594
Quoted 10 lines
drivers/clocksource/arm_arch_timer.c +7
-0@@ -485,6 +585,13 @@ static void __init arch_counter_register(unsigned type) arch_timer_read_counter = arch_counter_get_cntvct; else arch_timer_read_counter = arch_counter_get_cntpct; + +/* +* Don't use the vdso fastpath if errata require using +* the out-of-line counter accessor. +*/ +if (static_branch_unlikely (&arch_timer_read_ool_enabled)) + clocksource_counter.name ="arch_sys_counter_ool" ;
This looks really ugly. How about telling the vdso subsystem directly? Will, do you have a preference?
drivers/clocksource/arm_arch_timer.c static void __init arch_timer_of_init(struct device_node *np) · line 878
Quoted 13 lines
drivers/clocksource/arm_arch_timer.c +3
-0}else { arch_timer_read_counter = arch_counter_get_cntvct_mem;@@ -763,6 +870,9 @@ static void __init arch_timer_of_init(struct device_node *np) arch_timer_c3stop = !of_property_read_bool (np,"always-on" ); +if (of_property_read_bool (np,"fsl,erratum-a008585" )) +static_branch_enable (&arch_timer_read_ool_enabled); +/* * If we cannot rely on firmware initializing the timer registers then * we should use the physical timers instead.
An outstanding question is how we're going to deal with this in KVM, because a guest absolutely needs to know about it (I can definitely see time jumping in guests running on a LS2080).
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny.
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Quoted 66 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Hi Scott,
Quoted 61 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads
return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is poweredthrough an
always-on power domain, therefore it never loses context.+- fsl,erratum-a008585 : A boolean property. Indicates the presence of
Documentation/devicetree/bindings/arm/arch_timer.txt +5
-0+ QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties:
Quoted 4 lines
Quoted 1 line
- arm,cpu-registers-not-fw-configured : Firmware does not initialize
This should be part of a separate patch. Also, errata should be documented in
Documentation/arm64/silicon-errata.txt.
Oh right, forgot.
Quoted 19 lines
Quoted 19 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +19
-0+extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ(reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ + else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval)
Quoted 8 lines
Documentation/devicetree/bindings/arm/arch_timer.txt
Quoted 3 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ("cntvct_el0", arch_counter_get_cntvct) +Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (
CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.
OK.
Quoted 15 lines
Documentation/devicetree/bindings/arm/arch_timer.txt u32 arch_timer_reg_read_cp15(int access, enum · line 95
Quoted 13 lines
/*
* These register accessors are marked inline so the compiler can
* nicely work out which register we want, and chuck away the rest ofDocumentation/devicetree/bindings/arm/arch_timer.txt +1
-1@@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { case ARCH_TIMER_REG_CTRL: - asm volatile("mrs %0, cntp_ctl_el0" : "=r" (val)); + asm volatile("mrs %0, cntp_ctl_el0" : "=r" (val));Spurious change?
The extra spacing seemed to be an attempt to get things to line up between the CTRL and TVAL asm statements. When the TVAL case was converted to a function call, there was nothing for the above to line up with, so I moved it back to normal spacing.
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt u32 arch_timer_reg_read_cp15(int access, enum · line 98
Quoted 3 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+{ + u64 cval_old, cval_new; + int timeout = 200;Can we have a comment on how this value has been chosen?
It's an arbitrary value well beyond the point at which we've seen it fail.
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -232,6 +274,50 @@ static __always_inline void set_next_event(const int access, unsigned long evt, arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); }
Quoted 14 lines
Documentation/devicetree/bindings/arm/arch_timer.txt static __always_inline void set_next_event(const int · line 289
Quoted 12 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +11
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, + unsigned long evt, struct clock_event_device *clk) +{ + u64 cval_old, cval_new; + int timeout = 200; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk); + cval_new = __arch_counter_get_cntvct();Don't you need to guarantee the order of accesses here?
I'm not 100% sure. The erratum workaround sample code doesn't show any barriers, and adding more barriers could make it harder for the loop to successfully complete. There's already a barrier after the write, so the only concern should be whether the timer read could be reordered after the timer write, which could cause the loop to exit even if the write was bad. Do you know if A53 or A57 will reorder a counter read relative to a tval write?
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+#endif /* ARM64 */ + static int arch_timer_set_next_event_virt(unsigned long evt, struct clock_event_device *clk) {@@ -277,6 +363,13 @@ static void __arch_timer_setup(unsigned type, clk->set_state_shutdown = arch_timer_shutdown_virt; clk->set_state_oneshot_stopped = arch_timer_shutdown_virt; clk->set_next_event = arch_timer_set_next_event_virt; +
Quoted 8 lines
Quoted 6 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +2
-0+#ifdef CONFIG_ARM64 + if(
static_branch_unlikely(&arch_timer_read_ool_enabled))Documentation/devicetree/bindings/arm/arch_timer.txt +2
-0+ clk->set_next_event = + arch_timer_set_next_event_virt_errata;
On the same line, please.
I was trying to avoid going beyond 80 columns.
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0@@ -485,6 +585,13 @@ static void __init arch_counter_register(unsigned type) arch_timer_read_counter = arch_counter_get_cntvct; else arch_timer_read_counter = arch_counter_get_cntpct; + + /* + * Don't use the vdso fastpath if errata require using
Quoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +4
-0+ * the out-of-line counter accessor. + */ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) + clocksource_counter.name ="
arch_sys_counter_ool";This looks really ugly. How about telling the vdso subsystem directly? Will, do you have a preference?
I was following the example set by "arch_mem_counter".
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0} else { arch_timer_read_counter = arch_counter_get_cntvct_mem;@@ -763,6 +870,9 @@ static void __init arch_timer_of_init(struct device_node *np) arch_timer_c3stop = !of_property_read_bool(np, "always-on");
Quoted 11 lines
Quoted 7 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+ if (of_property_read_bool(np, "fsl,erratum-a008585")) + static_branch_enable(&arch_timer_read_ool_enabled); + /* * If we cannot rely on firmware initializing the timer registersthen
* we should use the physical timers instead.An outstanding question is how we're going to deal with this in KVM, because a guest absolutely needs to know about it (I can definitely see time jumping in guests running on a LS2080).
The property will need to be in the guest's device tree. I'm not too familiar with how KVM handles device trees on arm... From looking at the QEMU source it seems that the dtb is passed in by the user. So either that dtb will need the erratum property in it, or QEMU (and KVM tool?) would need to patch it
into the guest dtb based on seeing the property in /proc/device-tree.
-Scott
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Hi Scott:
I have read this patch, looks fine to me, and why not add unlikely to cval_old != cval_new? I think this problem is hard to occur.
Thanks.
Ding
Quoted 241 lines · Scott Wood
Quoted 66 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Hi Scott,
Quoted 61 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads
return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is poweredthrough an
always-on power domain, therefore it never loses context.+- fsl,erratum-a008585 : A boolean property. Indicates the presence of
Documentation/devicetree/bindings/arm/arch_timer.txt +5
-0+ QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties:Quoted 4 lines
Quoted 1 line
- arm,cpu-registers-not-fw-configured : Firmware does not initialize
This should be part of a separate patch. Also, errata should be documented in
Documentation/arm64/silicon-errata.txt.Oh right, forgot.
Quoted 19 lines
Quoted 19 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +19
-0+extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ(reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ + else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval)Quoted 8 lines
Documentation/devicetree/bindings/arm/arch_timer.txt
Quoted 3 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ("cntvct_el0", arch_counter_get_cntvct) +Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (
CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.OK.
Quoted 15 lines
Documentation/devicetree/bindings/arm/arch_timer.txt u32 arch_timer_reg_read_cp15(int access, enum · line 95
Quoted 13 lines
/*
* These register accessors are marked inline so the compiler can
* nicely work out which register we want, and chuck away the rest ofDocumentation/devicetree/bindings/arm/arch_timer.txt +1
-1@@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { case ARCH_TIMER_REG_CTRL: - asm volatile("mrs %0, cntp_ctl_el0" : "=r" (val)); + asm volatile("mrs %0, cntp_ctl_el0" : "=r" (val));Spurious change?
The extra spacing seemed to be an attempt to get things to line up between the CTRL and TVAL asm statements. When the TVAL case was converted to a function call, there was nothing for the above to line up with, so I moved it back to normal spacing.
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt u32 arch_timer_reg_read_cp15(int access, enum · line 98
Quoted 3 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+{ + u64 cval_old, cval_new; + int timeout = 200;Can we have a comment on how this value has been chosen?
It's an arbitrary value well beyond the point at which we've seen it fail.
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -232,6 +274,50 @@ static __always_inline void set_next_event(const int access, unsigned long evt, arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); }Quoted 14 lines
Documentation/devicetree/bindings/arm/arch_timer.txt static __always_inline void set_next_event(const int · line 289
Quoted 12 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +11
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, + unsigned long evt, struct clock_event_device *clk) +{ + u64 cval_old, cval_new; + int timeout = 200; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk); + cval_new = __arch_counter_get_cntvct();Don't you need to guarantee the order of accesses here?
I'm not 100% sure. The erratum workaround sample code doesn't show any barriers, and adding more barriers could make it harder for the loop to successfully complete. There's already a barrier after the write, so the only concern should be whether the timer read could be reordered after the timer write, which could cause the loop to exit even if the write was bad. Do you know if A53 or A57 will reorder a counter read relative to a tval write?
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+#endif /* ARM64 */ + static int arch_timer_set_next_event_virt(unsigned long evt, struct clock_event_device *clk) {@@ -277,6 +363,13 @@ static void __arch_timer_setup(unsigned type, clk->set_state_shutdown = arch_timer_shutdown_virt; clk->set_state_oneshot_stopped = arch_timer_shutdown_virt; clk->set_next_event = arch_timer_set_next_event_virt; +Quoted 8 lines
Quoted 6 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +2
-0+#ifdef CONFIG_ARM64 + if(
static_branch_unlikely(&arch_timer_read_ool_enabled))Documentation/devicetree/bindings/arm/arch_timer.txt +2
-0+ clk->set_next_event = + arch_timer_set_next_event_virt_errata;
On the same line, please.
I was trying to avoid going beyond 80 columns.
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0@@ -485,6 +585,13 @@ static void __init arch_counter_register(unsigned type) arch_timer_read_counter = arch_counter_get_cntvct; else arch_timer_read_counter = arch_counter_get_cntpct; + + /* + * Don't use the vdso fastpath if errata require usingQuoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +4
-0+ * the out-of-line counter accessor. + */ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) + clocksource_counter.name ="
arch_sys_counter_ool";This looks really ugly. How about telling the vdso subsystem directly? Will, do you have a preference?
I was following the example set by "
arch_mem_counter".Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0} else { arch_timer_read_counter = arch_counter_get_cntvct_mem;@@ -763,6 +870,9 @@ static void __init arch_timer_of_init(struct device_node *np) arch_timer_c3stop = !of_property_read_bool(np, "always-on");Quoted 11 lines
Quoted 7 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+ if (of_property_read_bool(np, "fsl,erratum-a008585")) + static_branch_enable(&arch_timer_read_ool_enabled); + /* * If we cannot rely on firmware initializing the timer registersthen
* we should use the physical timers instead.An outstanding question is how we're going to deal with this in KVM, because a guest absolutely needs to know about it (I can definitely see time jumping in guests running on a LS2080).
The property will need to be in the guest's device tree. I'm not too familiar with how KVM handles device trees on arm... From looking at the QEMU source it seems that the dtb is passed in by the user. So either that dtb will need the erratum property in it, or QEMU (and KVM tool?) would need to patch it
into the guest dtb based on seeing the property in/proc/device-tree.-Scott
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel.
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
In reply to Scott WoodQuoted 125 lines · Scott Wood
Quoted 66 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Hi Scott,
Quoted 61 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads
return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is poweredthrough an
always-on power domain, therefore it never loses context.+- fsl,erratum-a008585 : A boolean property. Indicates the presence of
Documentation/devicetree/bindings/arm/arch_timer.txt +5
-0+ QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties:Quoted 4 lines
Quoted 1 line
- arm,cpu-registers-not-fw-configured : Firmware does not initialize
This should be part of a separate patch. Also, errata should be documented in
Documentation/arm64/silicon-errata.txt.Oh right, forgot.
Quoted 19 lines
Quoted 19 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +19
-0+extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ(reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ + else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval)Quoted 8 lines
Documentation/devicetree/bindings/arm/arch_timer.txt
Quoted 3 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ("cntvct_el0", arch_counter_get_cntvct) +Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (
CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.OK.
Quoted 15 lines
Documentation/devicetree/bindings/arm/arch_timer.txt u32 arch_timer_reg_read_cp15(int access, enum · line 95
Quoted 13 lines
/*
* These register accessors are marked inline so the compiler can
* nicely work out which register we want, and chuck away the rest ofDocumentation/devicetree/bindings/arm/arch_timer.txt +1
-1@@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) if (access == ARCH_TIMER_PHYS_ACCESS) { switch (reg) { case ARCH_TIMER_REG_CTRL: - asm volatile("mrs %0, cntp_ctl_el0" : "=r" (val)); + asm volatile("mrs %0, cntp_ctl_el0" : "=r" (val));Spurious change?
The extra spacing seemed to be an attempt to get things to line up between the CTRL and TVAL asm statements. When the TVAL case was converted to a function call, there was nothing for the above to line up with, so I moved it back to normal spacing.
Quoted 7 lines
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt u32 arch_timer_reg_read_cp15(int access, enum · line 98
Quoted 3 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+{ + u64 cval_old, cval_new; + int timeout = 200;Can we have a comment on how this value has been chosen?
It's an arbitrary value well beyond the point at which we've seen it fail.
So can we please have a comment *in the code* that explains how this value has been picked?
Quoted 21 lines
Quoted 6 lines
Quoted 6 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -232,6 +274,50 @@ static __always_inline void set_next_event(const int access, unsigned long evt, arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); }Quoted 14 lines
Documentation/devicetree/bindings/arm/arch_timer.txt static __always_inline void set_next_event(const int · line 289
Quoted 12 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +11
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, + unsigned long evt, struct clock_event_device *clk) +{ + u64 cval_old, cval_new; + int timeout = 200; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk); + cval_new = __arch_counter_get_cntvct();Don't you need to guarantee the order of accesses here?
Quoted 6 lines
I'm not 100% sure. The erratum workaround sample code doesn't show any barriers, and adding more barriers could make it harder for the loop to successfully complete. There's already a barrier after the write, so the only concern should be whether the timer read could be reordered after the timer write, which could cause the loop to exit even if the write was bad. Do you know if A53 or A57 will reorder a counter read relative to a tval write?
I can't see any absolute guarantee that they wouldn't be reordered (but I have no insight on the micro-architecture either). I'd rather err on the side of caution here.
Quoted 23 lines
Quoted 14 lines
Quoted 14 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+#endif /* ARM64 */ + static int arch_timer_set_next_event_virt(unsigned long evt, struct clock_event_device *clk) {@@ -277,6 +363,13 @@ static void __arch_timer_setup(unsigned type, clk->set_state_shutdown = arch_timer_shutdown_virt; clk->set_state_oneshot_stopped = arch_timer_shutdown_virt; clk->set_next_event = arch_timer_set_next_event_virt; +Quoted 8 lines
Quoted 6 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +2
-0+#ifdef CONFIG_ARM64 + if(
static_branch_unlikely(&arch_timer_read_ool_enabled))Documentation/devicetree/bindings/arm/arch_timer.txt +2
-0+ clk->set_next_event = + arch_timer_set_next_event_virt_errata;
On the same line, please.
Quoted 1 line
I was trying to avoid going beyond 80 columns.
Please ignore what checkpatch says. Readability is more important (and I've given up using a vintage vt100...).
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0@@ -485,6 +585,13 @@ static void __init arch_counter_register(unsigned type) arch_timer_read_counter = arch_counter_get_cntvct; else arch_timer_read_counter = arch_counter_get_cntpct; + + /* + * Don't use the vdso fastpath if errata require usingQuoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +4
-0+ * the out-of-line counter accessor. + */ + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) + clocksource_counter.name ="
arch_sys_counter_ool";This looks really ugly. How about telling the vdso subsystem directly? Will, do you have a preference?
Quoted 1 line
I was following the example set by "
arch_mem_counter".
The right thing should probably be to use a flag in the clocksource structure to mark it as "suitable for vdso", but I guess we can fix that in a subsequent patch.
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0} else { arch_timer_read_counter = arch_counter_get_cntvct_mem;@@ -763,6 +870,9 @@ static void __init arch_timer_of_init(struct device_node *np) arch_timer_c3stop = !of_property_read_bool(np, "always-on");Quoted 11 lines
Quoted 7 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +3
-0+ if (of_property_read_bool(np, "fsl,erratum-a008585")) + static_branch_enable(&arch_timer_read_ool_enabled); + /* * If we cannot rely on firmware initializing the timer registersthen
* we should use the physical timers instead.An outstanding question is how we're going to deal with this in KVM, because a guest absolutely needs to know about it (I can definitely see time jumping in guests running on a LS2080).
Quoted 5 lines
The property will need to be in the guest's device tree. I'm not too familiar with how KVM handles device trees on arm... From looking at the QEMU source it seems that the dtb is passed in by the user. So either that dtb will need the erratum property in it, or QEMU (and KVM tool?) would need to patch it
into the guest dtb based on seeing the property in/proc/device-tree.
There is no guarantee that the host device tree is always accessible to userspace. So we're probably looking at requiring a new KVM device API that would expose the timer properties, one of them being this erratum. That's certainly going to be fun to handle.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Quoted 7 lines · Marc Zyngier
Quoted 6 lines · Scott Wood
Quoted 4 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 98
Quoted 3 lines
drivers/clocksource/arm_arch_timer.c +3
-0+{ +u64 cval_old, cval_new; +int timeout =200 ;Can we have a comment on how this value has been chosen??
It's an arbitrary value well beyond the point at which we've seen it fail.
So can we please have a comment *in the code* that explains how this value has been picked?
Sure, if you want. ?I just wasn't sure there was much value in a comment that essentially says that there's no special meaning to this particular value.
Quoted 28 lines
Quoted 19 lines
Quoted 11 lines
Quoted 11 lines
int
access, unsigned long evt,
?arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
?}
?drivers/clocksource/arm_arch_timer.c +6
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, +unsigned long evt,struct clock_event_device *clk) +{ +u64 cval_old, cval_new; +int timeout =200 ;Quoted 8 lines
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 287
Quoted 7 lines
drivers/clocksource/arm_arch_timer.c +4
-0+ + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL,evt,
clk);drivers/clocksource/arm_arch_timer.c +1
-0+ cval_new =__arch_counter_get_cntvct ();Don't you need to guarantee the order of accesses here?
Quoted 9 lines
I'm not 100% sure.??The erratum workaround sample code doesn't show any barriers, and adding more barriers could make it harder for the loop to successfully complete.??There's already a barrier after the write, so the only
concern should be whether the timer read could be reordered after the timer
write, which could cause the loop to exit even if the write was bad.??Do you
know if A53 or A57 will reorder a counter read relative to a tval write?
Quoted 3 lines
I can't see any absolute guarantee that they wouldn't be reordered (but I have no insight on the micro-architecture either). I'd rather err on the side of caution here.
OK, I'll see how well it works with the added barrier.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 12 lines
?
clk->set_state_shutdown=arch_timer_shutdown_virt;
?clk->set_state_oneshot_stopped=arch_timer_shutdown_virt;
?clk->set_next_event=arch_timer_set_next_event_virt;drivers/clocksource/arm_arch_timer.c +3
-0+ +#ifdef CONFIG_ARM64 + if(
static_branch_unlikely(&arch_timer_read_ool_enabled))(quoted diff) +2
-0+ clk->set_next_event = + arch_timer_set_next_event_virQuoted 2 lines
t_er
rata;On the same line, please.
Quoted 3 lines
Quoted 1 line
I was trying to avoid going beyond 80 columns.
Please ignore what checkpatch says. Readability is more important (and I've given up using a vintage vt100...).
I'm not using a vintage vt100 either but I still have (approximately) 80- column terminals, because I like having two terminals side-by-side with a reasonable font size... and since different people have different terminal setups, CodingStyle specifies a standard limit.
I think I can make it moot with the suggestion to have a helper function, though.
Quoted 28 lines
Quoted 20 lines
Quoted 12 lines
? } else {
Quoted 11 lines
?
arch_timer_read_counter=arch_counter_get_cntvct_mem;
?(quoted diff) +0
-0@@ -763,6 +870,9 @@ static void __init arch_timer_of_init(struct device_node *np)?
?arch_timer_c3stop= !of_property_read_bool(np, "always-on");
?drivers/clocksource/arm_arch_timer.c +3
-0+if (of_property_read_bool (np,"fsl,erratum-a008585" )) +static_branch_enable (&arch_timer_read_ool_enabled); +Quoted 8 lines
Quoted 5 lines
? /*
? ?* If we cannot rely on firmware initializing the timer
registers
then
? ?* we should use the physical timers instead.An outstanding question is how we're going to deal with this in KVM, because a guest absolutely needs to know about it (I can definitely see time jumping in guests running on a LS2080).
Quoted 8 lines
The property will need to be in the guest's device tree.??I'm not too familiar
with how KVM handles device trees on arm...??From looking at the QEMU source
it seems that the dtb is passed in by the user.??So either that dtb will need
the erratum property in it, or QEMU (and KVM tool?) would need to patch it ?into the guest dtb based on seeing the property in/proc/device-tree.
Quoted 4 lines
There is no guarantee that the host device tree is always accessible to userspace. So we're probably looking at requiring a new KVM device API that would expose the timer properties, one of them being this erratum. That's certainly going to be fun to handle.
KVM on PPC relies on /proc/device-tree -- when would it not be available? ?Where is the dtb passed to QEMU expected to come from?
In any case, let's get the kernel sorted out first.
-Scott
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
In reply to Marc ZyngierQuoted 30 lines · Marc Zyngier
Quoted 20 lines · Scott Wood
Quoted 4 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Quoted 14 lines
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 287
Quoted 13 lines
drivers/clocksource/arm_arch_timer.c +10
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, + unsigned long evt, struct clock_event_device *clk) +{ + u64 cval_old, cval_new; + int timeout = 200; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL,evt,
clk);drivers/clocksource/arm_arch_timer.c +1
-0+ cval_new =__arch_counter_get_cntvct ();Don't you need to guarantee the order of accesses here?
Quoted 9 lines
I'm not 100% sure.??The erratum workaround sample code doesn't show any barriers, and adding more barriers could make it harder for the loop to successfully complete.??There's already a barrier after the write, so the only
concern should be whether the timer read could be reordered after the timer
write, which could cause the loop to exit even if the write was bad.??Do you
know if A53 or A57 will reorder a counter read relative to a tval write?
Quoted 3 lines
I can't see any absolute guarantee that they wouldn't be reordered (but I have no insight on the micro-architecture either). I'd rather err on the side of caution here.
Adding another isb() before arch_timer_reg_write() causes the loop to not complete with any reasonable timeout. ?A testing loop (that repeatedly writes to TVAL (using the workaround code), reads it back, and checks that the value read back is not greater than the value that was written) shows that the workaround without the extra isb() is effective -- lots of assertions without the workaround, and none with it -- but I'll go with the cval workaround instead.
-Scott
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
In reply to Marc ZyngierQuoted 172 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Hi Scott,
Quoted 48 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.
This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context.Documentation/devicetree/bindings/arm/arch_timer.txt to deliver its interrupts via SPIs. · line 36
Quoted 10 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +6
-0+- fsl,erratum-a008585 : A boolean property. Indicates the presence of + QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties: - arm,cpu-registers-not-fw-configured : Firmware does not initializeThis should be part of a separate patch. Also, errata should be documented in
Documentation/arm64/silicon-errata.txt.Quoted 25 lines
arch/arm64/include/asm/arch_timer.h +13
-0@@ -23,10 +23,33 @@#include <linux/bug.h> #include <linux/init.h> +#include <linux/jump_label.h> #include <linux/types.h> #include <clocksource/arm_arch_timer.h> +extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ (reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \arch/arm64/include/asm/arch_timer.h line 52
Quoted 10 lines
arch/arm64/include/asm/arch_timer.h +10
-0+if (static_branch_unlikely (&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ +else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval) +ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ ("cntvct_el0" , arch_counter_get_cntvct) +Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (
CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 92
Quoted 9 lines
arch/arm64/include/asm/arch_timer.h +1
-1/* * These register accessors are marked inline so the compiler can * nicely work out which register we want, and chuck away the rest of @@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)if (access ==ARCH_TIMER_PHYS_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val));Spurious change?
arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 101
Quoted 11 lines
arch/arm64/include/asm/arch_timer.h +2
-2break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntp_tval_el0" :"=r" (val)); + val =_arch_timer_get_ptval ();break ; } }else if (access ==ARCH_TIMER_VIRT_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val));Same here.
Quoted 26 lines
arch/arm64/include/asm/arch_timer.h +2
-6break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntv_tval_el0" :"=r" (val)); + val =_arch_timer_get_vtval ();break ; } }@@ -116,12 +139,8 @@ static inline u64 arch_counter_get_cntpct(void)static inline u64 arch_counter_get_cntvct (void ) { -u64 cval; -isb (); - asmvolatile ("mrs %0, cntvct_el0" :"=r" (cval)); - -return cval; +return _arch_counter_get_cntvct (); }static inline int arch_timer_arch_init (void )drivers/clocksource/arm_arch_timer.c +0
-0drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 84
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +3
-0@@ -79,10 +79,52 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;static bool arch_timer_c3stop;static bool arch_timer_mem_use_virtual; +DEFINE_STATIC_KEY_FALSE (arch_timer_read_ool_enabled); +EXPORT_SYMBOL_GPL (arch_timer_read_ool_enabled); +Once you have a config option, this can move to the guarded section below...
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 89
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +1
-0/* * Architected system timer support. */ +#ifdef CONFIG_ARM64 which can become
CONFIG_FSL_ERRATUM_008585
Quoted 8 lines
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 95
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +6
-0+/* +* __always_inline is used to ensure that func() is not an actual function +* pointer, which would result in the register accesses potentially being too +* far apart for the loop to work. +*/ +static __always_inline u64 arch_timer_reread(u64 (*func)(void))This is quite a generic function name. I'd expect something that refers to the erratum number.
This is a generic solution to reread the timer counter, how about using a generic one here for more potential consumers in the future?
Thanks
Hanjun
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Quoted 182 lines · Hanjun Guo
Quoted 172 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Hi Scott,
Quoted 48 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.
This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context.Documentation/devicetree/bindings/arm/arch_timer.txt to deliver its interrupts via SPIs. · line 36
Quoted 10 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +6
-0+- fsl,erratum-a008585 : A boolean property. Indicates the presence of + QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties: - arm,cpu-registers-not-fw-configured : Firmware does not initializeThis should be part of a separate patch. Also, errata should be documented in
Documentation/arm64/silicon-errata.txt.Quoted 25 lines
arch/arm64/include/asm/arch_timer.h +13
-0@@ -23,10 +23,33 @@#include <linux/bug.h> #include <linux/init.h> +#include <linux/jump_label.h> #include <linux/types.h> #include <clocksource/arm_arch_timer.h> +extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ (reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \arch/arm64/include/asm/arch_timer.h line 52
Quoted 10 lines
arch/arm64/include/asm/arch_timer.h +10
-0+if (static_branch_unlikely (&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ +else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval) +ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ ("cntvct_el0" , arch_counter_get_cntvct) +Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (
CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 92
Quoted 9 lines
arch/arm64/include/asm/arch_timer.h +1
-1/* * These register accessors are marked inline so the compiler can * nicely work out which register we want, and chuck away the rest of @@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)if (access ==ARCH_TIMER_PHYS_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val));Spurious change?
arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 101
Quoted 11 lines
arch/arm64/include/asm/arch_timer.h +2
-2break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntp_tval_el0" :"=r" (val)); + val =_arch_timer_get_ptval ();break ; } }else if (access ==ARCH_TIMER_VIRT_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val));Same here.
Quoted 26 lines
arch/arm64/include/asm/arch_timer.h +2
-6break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntv_tval_el0" :"=r" (val)); + val =_arch_timer_get_vtval ();break ; } }@@ -116,12 +139,8 @@ static inline u64 arch_counter_get_cntpct(void)static inline u64 arch_counter_get_cntvct (void ) { -u64 cval; -isb (); - asmvolatile ("mrs %0, cntvct_el0" :"=r" (cval)); - -return cval; +return _arch_counter_get_cntvct (); }static inline int arch_timer_arch_init (void )drivers/clocksource/arm_arch_timer.c +0
-0drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 84
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +3
-0@@ -79,10 +79,52 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;static bool arch_timer_c3stop;static bool arch_timer_mem_use_virtual; +DEFINE_STATIC_KEY_FALSE (arch_timer_read_ool_enabled); +EXPORT_SYMBOL_GPL (arch_timer_read_ool_enabled); +Once you have a config option, this can move to the guarded section below...
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 89
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +1
-0/* * Architected system timer support. */ +#ifdef CONFIG_ARM64 which can become
CONFIG_FSL_ERRATUM_008585Quoted 8 lines
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 95
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +6
-0+/* +* __always_inline is used to ensure that func() is not an actual function +* pointer, which would result in the register accesses potentially being too +* far apart for the loop to work. +*/ +static __always_inline u64 arch_timer_reread(u64 (*func)(void))This is quite a generic function name. I'd expect something that refers to the erratum number.
Quoted 2 lines
This is a generic solution to reread the timer counter, how about using a generic one here for more potential consumers in the future?
In that case, how about something like
"my_arch_timer_is_so_broken_that_I_have_to_poll_it"?
It should be generic enough, don't you think?
Joke aside, I want it to be clear that this is a workaround for an erratum. This is not a general purpose function.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Quoted 182 lines · Marc Zyngier
Quoted 181 lines · Hanjun Guo
Quoted 180 lines · Marc Zyngier
On Thu, 12 May 2016 23:37:39 -0500
Scott Wood <[email protected]> wrote:Hi Scott,
Quoted 48 lines
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.
This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++ arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)Documentation/devicetree/bindings/arm/arch_timer.txt +0
-0@@ -25,6 +25,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context.Documentation/devicetree/bindings/arm/arch_timer.txt to deliver its interrupts via SPIs. · line 36
Quoted 10 lines
Documentation/devicetree/bindings/arm/arch_timer.txt +6
-0+- fsl,erratum-a008585 : A boolean property. Indicates the presence of + QorIQ erratum A-008585, which says that reading the counter is + unreliable unless the same value is returned by back-to-back reads. + This also affects writes to the tval register, due to the implicit + counter read. + ** Optional properties: - arm,cpu-registers-not-fw-configured : Firmware does not initializeThis should be part of a separate patch. Also, errata should be documented in
Documentation/arm64/silicon-errata.txt.Quoted 25 lines
arch/arm64/include/asm/arch_timer.h +13
-0@@ -23,10 +23,33 @@#include <linux/bug.h> #include <linux/init.h> +#include <linux/jump_label.h> #include <linux/types.h> #include <clocksource/arm_arch_timer.h> +extern struct static_key_false arch_timer_read_ool_enabled; + +#define ARCH_TIMER_REG_READ (reg, func) \ +extern u64 func##_ool(void); \ +static inline u64 __##func(void) \ +{ \ + u64 val; \ + asm volatile("mrs %0, " reg : "=r" (val)); \ + return val; \ +} \ +static inline u64 _##func(void) \ +{ \arch/arm64/include/asm/arch_timer.h line 52
Quoted 10 lines
arch/arm64/include/asm/arch_timer.h +10
-0+if (static_branch_unlikely (&arch_timer_read_ool_enabled)) \ + return func##_ool(); \ +else \ + return __##func(); \ +} + +ARCH_TIMER_REG_READ("cntp_tval_el0", arch_timer_get_ptval) +ARCH_TIMER_REG_READ("cntv_tval_el0", arch_timer_get_vtval) +ARCH_TIMER_REG_READ ("cntvct_el0" , arch_counter_get_cntvct) +Given that this will have a (small) impact on non-affected platforms, it'd be good to have this guarded by a erratum-specific config option (
CONFIG_FSL_ERRATUM_008585?) and turn it into trivial accessors when not defined.arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 92
Quoted 9 lines
arch/arm64/include/asm/arch_timer.h +1
-1/* * These register accessors are marked inline so the compiler can * nicely work out which register we want, and chuck away the rest of @@ -66,19 +89,19 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)if (access ==ARCH_TIMER_PHYS_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntp_ctl_el0" :"=r" (val));Spurious change?
arch/arm64/include/asm/arch_timer.h u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) · line 101
Quoted 11 lines
arch/arm64/include/asm/arch_timer.h +2
-2break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntp_tval_el0" :"=r" (val)); + val =_arch_timer_get_ptval ();break ; } }else if (access ==ARCH_TIMER_VIRT_ACCESS ) {switch (reg) {case ARCH_TIMER_REG_CTRL : - asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val)); + asmvolatile ("mrs %0, cntv_ctl_el0" :"=r" (val));Same here.
Quoted 26 lines
arch/arm64/include/asm/arch_timer.h +2
-6break ;case ARCH_TIMER_REG_TVAL : - asmvolatile ("mrs %0, cntv_tval_el0" :"=r" (val)); + val =_arch_timer_get_vtval ();break ; } }@@ -116,12 +139,8 @@ static inline u64 arch_counter_get_cntpct(void)static inline u64 arch_counter_get_cntvct (void ) { -u64 cval; -isb (); - asmvolatile ("mrs %0, cntvct_el0" :"=r" (cval)); - -return cval; +return _arch_counter_get_cntvct (); }static inline int arch_timer_arch_init (void )drivers/clocksource/arm_arch_timer.c +0
-0drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 84
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +3
-0@@ -79,10 +79,52 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;static bool arch_timer_c3stop;static bool arch_timer_mem_use_virtual; +DEFINE_STATIC_KEY_FALSE (arch_timer_read_ool_enabled); +EXPORT_SYMBOL_GPL (arch_timer_read_ool_enabled); +Once you have a config option, this can move to the guarded section below...
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 89
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +1
-0/* * Architected system timer support. */ +#ifdef CONFIG_ARM64 which can become
CONFIG_FSL_ERRATUM_008585drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 95
Quoted 6 lines
drivers/clocksource/arm_arch_timer.c +6
-0+/* +* __always_inline is used to ensure that func() is not an actual function +* pointer, which would result in the register accesses potentially being too +* far apart for the loop to work. +*/ +static __always_inline u64 arch_timer_reread(u64 (*func)(void))This is quite a generic function name. I'd expect something that refers to the erratum number.
Quoted 7 lines
Quoted 2 lines
This is a generic solution to reread the timer counter, how about using a generic one here for more potential consumers in the future?
In that case, how about something like
"
my_arch_timer_is_so_broken_that_I_have_to_poll_it"?It should be generic enough, don't you think?
I will vote for it ;)
Quoted 4 lines
Joke aside, I want it to be clear that this is a workaround for an erratum. This is not a general purpose function.
Fair enough, thanks for the clarify.
Regards
Hanjun
Rob Herring
maintainerPATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
In reply to Scott WoodQuoted 35 lines · Scott Wood
Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.
The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.
This erratum can be found on LS1043A and LS2080A.
Signed-off-by Scott Wood <[email protected]>---
v2:
Significant rework based on feedback, including usingstatic_key, disabling VDSO counter access rather than adding the workaround to the VDSO, and uninlining the loops.Dropped the separate property for indicating that writes to TVAL are affected, as I believe that's just a side effect of the implicit counter read being corrupted, and thus a chip that is affected by one will always be affected by the other.
Dropped the arm32 portion as it seems there was confusion about whether LS1021A is affected. Currently I am being told that it is not affected.
I considered writing to CVAL rather than looping on TVAL writes, but that would still have required separate
set_next_event()code for the erratum, and adding CVAL to the enum would have required a bunch of extra handlers in switch statements (even where unused, due to compiler warnings about unhandled enum values) including in an arm32 header. It seemed better to avoid the arm32 interaction and new untested accessors.
Quoted 2 lines
--- .../devicetree/bindings/arm/arch_timer.txt | 6 ++
Quoted 3 lines
arch/arm64/include/asm/arch_timer.h | 37 +++++-- drivers/clocksource/arm_arch_timer.c | 110 +++++++++++++++++++++ 3 files changed, 144 insertions(+), 9 deletions(-)
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
In reply to Scott WoodHello Scott,
On 2016/5/13 12:37, Scott Wood wrote:
[...]
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 286
Quoted 11 lines
drivers/clocksource/arm_arch_timer.c +10
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, +unsigned long evt,struct clock_event_device *clk) +{ +u64 cval_old, cval_new; +int timeout =200 ; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
For not memory mapped timer, it will call arch_timer_reg_write_cp15() which has isb() at the end of arch_timer_reg_write_cp15()...
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 287
Quoted 1 line
drivers/clocksource/arm_arch_timer.c +1
-0+ cval_new =__arch_counter_get_cntvct ();
So there is isb() between counter retry read, I think it's likely cval_new will not be equal with cval_old when the cntvct is correct (time lapse is more than one arch timer cycle).
drivers/clocksource/arm_arch_timer.c static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI; · line 107
Quoted 4 lines
drivers/clocksource/arm_arch_timer.c +4
-0+ timeout--; + }while (cval_old != cval_new && timeout); + +WARN_ON_ONCE (!timeout);
Then trigger the warning here, correct me if I miss something else.
Thanks
Hanjun
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Quoted 19 lines · Hanjun Guo
Hello Scott,
On 2016/5/13 12:37, Scott Wood wrote:
[...]drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 286
Quoted 11 lines
drivers/clocksource/arm_arch_timer.c +10
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, +unsigned long evt,struct clock_event_device *clk) +{ +u64 cval_old, cval_new; +int timeout =200 ; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);For not memory mapped timer, it will call
arch_timer_reg_write_cp15()which hasisb()at the end ofarch_timer_reg_write_cp15()...
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 287
Quoted 1 line
drivers/clocksource/arm_arch_timer.c +1
-0+ cval_new =__arch_counter_get_cntvct ();So there is
isb()between counter retry read, I think it's likelycval_newwill not be equal withcval_oldwhen the cntvct is correct (time lapse is more than one arch timer cycle).
Are you saying that the isb will delay the execution of the read so much that your timer will always change? Well, if your timer is in the GHz range, maybe. But that also implies that it is out of spec (it should be in the 1-50MHz range).
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v2 1/2 ARM64: arch_timer: Work around QorIQ Erratum A-008585
Quoted 26 lines · Marc Zyngier
Quoted 19 lines · Hanjun Guo
Hello Scott,
On 2016/5/13 12:37, Scott Wood wrote:
[...]drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 286
Quoted 11 lines
drivers/clocksource/arm_arch_timer.c +10
-0+#ifdef CONFIG_ARM64 +static __always_inline void rewrite_tval(const int access, +unsigned long evt,struct clock_event_device *clk) +{ +u64 cval_old, cval_new; +int timeout =200 ; + + do { + cval_old = __arch_counter_get_cntvct(); + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);For not memory mapped timer, it will call
arch_timer_reg_write_cp15()which hasisb()at the end ofarch_timer_reg_write_cp15()...Quoted 5 lines
drivers/clocksource/arm_arch_timer.c static __always_inline void set_next_event(const int access, unsigned long evt, · line 287
Quoted 1 line
drivers/clocksource/arm_arch_timer.c +1
-0+ cval_new =__arch_counter_get_cntvct ();So there is
isb()between counter retry read, I think it's likelycval_newwill not be equal withcval_oldwhen the cntvct is correct (time lapse is more than one arch timer cycle).
Quoted 5 lines
Are you saying that the isb will delay the execution of the read so much that your timer will always change? Well, if your timer is in the GHz range, maybe. But that also implies that it is out of spec (it should be in the 1-50MHz range).
I have test this patch on the D02 board, the arch timer is 50Mhz and the cpu is 2.1Ghz, if I don't remove the isb from the arch_timer_reg_write(xxx) function, the cval_new is always bigger than the cval_old, and the timeout is always running to 0, so I don't think the rewrite_tval is a correct solution for this problem. So I think the other board will met the same problem.
Thanks.
Ding
Quoted 4 lines
Thanks,
M.