| Message ID | 1477553651-13428-1-git-send-email-dingtianhong@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
On 27/10/16 08:34, Ding Tianhong wrote: > The workaround for hisilicon,161601 will check the return value of the system counter > by different way, in order to distinguish with the fsl-a008585 workaround, introduce > a new generic erratum handing mechanism for fsl-a008585 and rename some functions. > > v2: Introducing a new generic erratum handling mechanism for fsl erratum a008585. > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > --- > arch/arm64/include/asm/arch_timer.h | 20 +++++++++----- > drivers/clocksource/arm_arch_timer.c | 51 +++++++++++++++++++++--------------- > 2 files changed, 43 insertions(+), 28 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index eaa5bbe..118719d8 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -31,15 +31,21 @@ > > #if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) > extern struct static_key_false arch_timer_read_ool_enabled; > -#define needs_fsl_a008585_workaround() \ > +#define needs_timer_erratum_workaround() \ > static_branch_unlikely(&arch_timer_read_ool_enabled) This is too generic a name. Please make it more specific. > #else > -#define needs_fsl_a008585_workaround() false > +#define needs_timer_erratum_workaround() false > #endif > > -u32 __fsl_a008585_read_cntp_tval_el0(void); > -u32 __fsl_a008585_read_cntv_tval_el0(void); > -u64 __fsl_a008585_read_cntvct_el0(void); > + > +struct arch_timer_erratum_workaround { > + int erratum; What is the use of this field? > + u32 (*read_cntp_tval_el0)(void); > + u32 (*read_cntv_tval_el0)(void); > + u64 (*read_cntvct_el0)(void); > +}; > + > +extern struct arch_timer_erratum_workaround *erratum_workaround; This is a very generic name for something that has a global visibility. Please choose a more specific variable name. > > /* > * The number of retries is an arbitrary value well beyond the highest number > @@ -62,8 +68,8 @@ u64 __fsl_a008585_read_cntvct_el0(void); > #define arch_timer_reg_read_stable(reg) \ > ({ \ > u64 _val; \ > - if (needs_fsl_a008585_workaround()) \ > - _val = __fsl_a008585_read_##reg(); \ > + if (needs_timer_erratum_workaround()) \ > + _val = erratum_workaround->read_##reg(); \ As mentioned in my initial review, you've now broken modular access to any of the registers. Please fix it. > else \ > _val = read_sysreg(reg); \ > _val; \ > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 73c487d..e4f7fa1 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -95,10 +95,32 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg); > */ > > #ifdef CONFIG_FSL_ERRATUM_A008585 > +struct arch_timer_erratum_workaround *erratum_workaround = NULL; > + > DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled); > EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled); > > -static int fsl_a008585_enable = -1; > + > +static u32 fsl_a008585_read_cntp_tval_el0(void) > +{ > + return __fsl_a008585_read_reg(cntp_tval_el0); > +} > + > +static u32 fsl_a008585_read_cntv_tval_el0(void) > +{ > + return __fsl_a008585_read_reg(cntv_tval_el0); > +} > + > +static u64 fsl_a008585_read_cntvct_el0(void) > +{ > + return __fsl_a008585_read_reg(cntvct_el0); > +} So now that you've indirected all calls through a set of pointers, why do you keep the __fsl_a008585_read_reg() macro inside the include file? I don't think it has any purpose in being globally visible now. > + > +static struct arch_timer_erratum_workaround arch_timer_fsl_a008585 = { And here's the proof that the erratum field is useless. > + .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0, > + .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0, > + .read_cntvct_el0 = fsl_a008585_read_cntvct_el0, > +}; > > static int __init early_fsl_a008585_cfg(char *buf) > { > @@ -109,26 +131,12 @@ static int __init early_fsl_a008585_cfg(char *buf) > if (ret) > return ret; > > - fsl_a008585_enable = val; > + if (val) > + erratum_workaround = &arch_timer_fsl_a008585; > + > return 0; > } > early_param("clocksource.arm_arch_timer.fsl-a008585", early_fsl_a008585_cfg); > - > -u32 __fsl_a008585_read_cntp_tval_el0(void) > -{ > - return __fsl_a008585_read_reg(cntp_tval_el0); > -} > - > -u32 __fsl_a008585_read_cntv_tval_el0(void) > -{ > - return __fsl_a008585_read_reg(cntv_tval_el0); > -} > - > -u64 __fsl_a008585_read_cntvct_el0(void) > -{ > - return __fsl_a008585_read_reg(cntvct_el0); > -} > -EXPORT_SYMBOL(__fsl_a008585_read_cntvct_el0); > #endif /* CONFIG_FSL_ERRATUM_A008585 */ > > static __always_inline > @@ -891,9 +899,10 @@ static int __init arch_timer_of_init(struct device_node *np) > arch_timer_c3stop = !of_property_read_bool(np, "always-on"); > > #ifdef CONFIG_FSL_ERRATUM_A008585 > - if (fsl_a008585_enable < 0) > - fsl_a008585_enable = of_property_read_bool(np, "fsl,erratum-a008585"); > - if (fsl_a008585_enable) { > + if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585")) > + erratum_workaround = &arch_timer_fsl_a008585; It used to be possible to disable the erratum workaround on the command line, and you've just removed that feature. Please restore it. > + > + if (erratum_workaround) { > static_branch_enable(&arch_timer_read_ool_enabled); > pr_info("Enabling workaround for FSL erratum A-008585\n"); > } > Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 27/10/16 08:34, Ding Tianhong wrote: > Erratum Hisilicon-161601 says that the ARM generic timer counter "has the > potential to contain an erroneous value when 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 the system count registers until the value of the second > read is larger than the first one by less than 32, the system counter can be guaranteed > not to return wrong value twice by back-to-back read and the error value is always larger > than the correct one by 32. Writes to TVAL are replaced with an equivalent write to CVAL. > > The workaround is enabled if the hisilicon,erratum-161601 property is found in > the timer node in the device tree. This can be overridden with the > clocksource.arm_arch_timer.hisilicon-161601 boot parameter, which allows KVM > users to enable the workaround until a mechanism is implemented to > automatically communicate this information. > > Fix some description for fsl erratum a008585. > > v2: Significant rework based on feedback, including seperate the fsl erratum a008585 > to another patch, update the erratum name and remove unwanted code. > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > --- > Documentation/arm64/silicon-errata.txt | 1 + > Documentation/kernel-parameters.txt | 9 ++++ > arch/arm64/include/asm/arch_timer.h | 28 ++++++++++- > drivers/clocksource/Kconfig | 14 +++++- > drivers/clocksource/arm_arch_timer.c | 88 ++++++++++++++++++++++++++-------- > 5 files changed, 118 insertions(+), 22 deletions(-) > > diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt > index 405da11..70c5d5e 100644 > --- a/Documentation/arm64/silicon-errata.txt > +++ b/Documentation/arm64/silicon-errata.txt > @@ -63,3 +63,4 @@ stable kernels. > | Cavium | ThunderX SMMUv2 | #27704 | N/A | > | | | | | > | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | > +| Hisilicon | Hip05/Hip06/Hip07 | #161601 | HISILICON_ERRATUM_161601| I've already commented on the alignment. Please read my initial review. > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt > index 6fa1d8a..735b4b6 100644 > --- a/Documentation/kernel-parameters.txt > +++ b/Documentation/kernel-parameters.txt > @@ -707,6 +707,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted. > erratum. If unspecified, the workaround is > enabled based on the device tree. > > + clocksource.arm_arch_timer.hisilicon-161601= > + [ARM64] > + Format: <bool> > + Enable/disable the workaround of Hisilicon > + erratum 161601. This can be useful for KVM > + guests, if the guest device tree doesn't show the > + erratum. If unspecified, the workaround is > + enabled based on the device tree. > + > clearcpuid=BITNUM [X86] > Disable CPUID feature X for the kernel. See > arch/x86/include/asm/cpufeatures.h for the valid bit > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index 118719d8..49b3041 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -29,7 +29,7 @@ > > #include <clocksource/arm_arch_timer.h> > > -#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) > +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) > extern struct static_key_false arch_timer_read_ool_enabled; > #define needs_timer_erratum_workaround() \ > static_branch_unlikely(&arch_timer_read_ool_enabled) > @@ -65,11 +65,35 @@ extern struct arch_timer_erratum_workaround *erratum_workaround; > _new; \ > }) > > + > + > +/* > + * The number of retries is an arbitrary value well beyond the highest number > + * of iterations the loop has been observed to take. > + * Verify whether the value of the second read is larger than the first by > + * less than 32 is the only way to confirm the value is correct, the system > + * counter can be guaranteed not to return wrong value twice by back-to-back read > + * and the error value is always larger than the correct one by 32. > + */ > +#define __hisi_161601_read_reg(reg) ({ \ > + u64 _old, _new; \ > + int _retries = 200; \ Please document how this value was found (either in the code or in the commit message). > + \ > + do { \ > + _old = read_sysreg(reg); \ > + _new = read_sysreg(reg); \ > + _retries--; \ > + } while (unlikely((_new - _old) >> 5) && _retries); \ > + \ > + WARN_ON_ONCE(!_retries); \ > + _new; \ > +}) Same remark as in the previous patch. > + > #define arch_timer_reg_read_stable(reg) \ > ({ \ > u64 _val; \ > if (needs_timer_erratum_workaround()) \ > - _val = erratum_workaround->read_##reg(); \ > + _val = erratum_workaround->read_##reg();\ > else \ > _val = read_sysreg(reg); \ > _val; \ > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 8a753fd..4aafb6a 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -312,8 +312,20 @@ config FSL_ERRATUM_A008585 > help > This option enables a workaround for Freescale/NXP Erratum > A-008585 ("ARM generic timer may contain an erroneous > - value"). The workaround will only be active if the > + value"). The workaround will be active if the > fsl,erratum-a008585 property is found in the timer node. > + This can be overridden with the clocksource.arm_arch_timer.fsl-a008585 > + boot parameter. Move this hunk to the previous patch. > + > +config HISILICON_ERRATUM_161601 > + bool "Workaround for Hisilicon Erratum 161601" > + default y > + depends on ARM_ARCH_TIMER && ARM64 > + help > + This option enables a workaround for Hisilicon Erratum > + 161601. The workaround will be active if the hisilicon,erratum-161601 > + property is found in the timer node. This can be overridden with > + the clocksource.arm_arch_timer.hisilicon-161601 boot parameter. > > config ARM_GLOBAL_TIMER > bool "Support for the ARM global timer" if COMPILE_TEST > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index e4f7fa1..89f1895 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -94,13 +94,14 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg); > * Architected system timer support. > */ > > -#ifdef CONFIG_FSL_ERRATUM_A008585 > +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) > struct arch_timer_erratum_workaround *erratum_workaround = NULL; > > DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled); > EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled); > +#endif > > - > +#ifdef CONFIG_FSL_ERRATUM_A008585 > static u32 fsl_a008585_read_cntp_tval_el0(void) > { > return __fsl_a008585_read_reg(cntp_tval_el0); > @@ -139,6 +140,45 @@ static int __init early_fsl_a008585_cfg(char *buf) > early_param("clocksource.arm_arch_timer.fsl-a008585", early_fsl_a008585_cfg); > #endif /* CONFIG_FSL_ERRATUM_A008585 */ > > +#ifdef CONFIG_HISILICON_ERRATUM_161601 > +static u32 hisi_161601_read_cntp_tval_el0(void) > +{ > + return __hisi_161601_read_reg(cntp_tval_el0); > +} > + > +static u32 hisi_161601_read_cntv_tval_el0(void) > +{ > + return __hisi_161601_read_reg(cntv_tval_el0); > +} > + > +static u64 hisi_161601_read_cntvct_el0(void) > +{ > + return __hisi_161601_read_reg(cntvct_el0); > +} > + > +static struct arch_timer_erratum_workaround arch_timer_hisi_161601 = { > + .read_cntp_tval_el0 = hisi_161601_read_cntp_tval_el0, > + .read_cntv_tval_el0 = hisi_161601_read_cntv_tval_el0, > + .read_cntvct_el0 = hisi_161601_read_cntvct_el0, > +}; > + > +static int __init early_hisi_161601_cfg(char *buf) > +{ > + int ret; > + bool val; > + > + ret = strtobool(buf, &val); > + if (ret) > + return ret; > + > + if (val) > + erratum_workaround = &arch_timer_hisi_161601; > + > + return 0; > +} > +early_param("clocksource.arm_arch_timer.hisilicon-161601", early_hisi_161601_cfg); > +#endif /* CONFIG_HISILICON_ERRATUM_161601 */ > + > static __always_inline > void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val, > struct clock_event_device *clk) > @@ -288,8 +328,8 @@ 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_FSL_ERRATUM_A008585 > -static __always_inline void fsl_a008585_set_next_event(const int access, > +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) > +static __always_inline void erratum_set_next_event(const int access, > unsigned long evt, struct clock_event_device *clk) > { > unsigned long ctrl; > @@ -307,20 +347,20 @@ static __always_inline void fsl_a008585_set_next_event(const int access, > arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); > } > > -static int fsl_a008585_set_next_event_virt(unsigned long evt, > +static int erratum_set_next_event_virt(unsigned long evt, > struct clock_event_device *clk) > { > - fsl_a008585_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk); > + erratum_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk); > return 0; > } > > -static int fsl_a008585_set_next_event_phys(unsigned long evt, > +static int erratum_set_next_event_phys(unsigned long evt, > struct clock_event_device *clk) > { > - fsl_a008585_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk); > + erratum_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk); > return 0; > } > -#endif /* CONFIG_FSL_ERRATUM_A008585 */ > +#endif > > static int arch_timer_set_next_event_virt(unsigned long evt, > struct clock_event_device *clk) > @@ -350,16 +390,16 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt, > return 0; > } > > -static void fsl_a008585_set_sne(struct clock_event_device *clk) > +static void erratum_set_sne(struct clock_event_device *clk) > { > -#ifdef CONFIG_FSL_ERRATUM_A008585 > +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) > if (!static_branch_unlikely(&arch_timer_read_ool_enabled)) > return; > > if (arch_timer_uses_ppi == VIRT_PPI) > - clk->set_next_event = fsl_a008585_set_next_event_virt; > + clk->set_next_event = erratum_set_next_event_virt; > else > - clk->set_next_event = fsl_a008585_set_next_event_phys; > + clk->set_next_event = erratum_set_next_event_phys; > #endif This should be in the previous patch as well, as it only messes with the FSL erratum. > } > > @@ -392,7 +432,7 @@ static void __arch_timer_setup(unsigned type, > BUG(); > } > > - fsl_a008585_set_sne(clk); > + erratum_set_sne(clk); > } else { > clk->features |= CLOCK_EVT_FEAT_DYNIRQ; > clk->name = "arch_mem_timer"; > @@ -612,7 +652,7 @@ static void __init arch_counter_register(unsigned type) > > clocksource_counter.archdata.vdso_direct = true; > > -#ifdef CONFIG_FSL_ERRATUM_A008585 > +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) > /* > * Don't use the vdso fastpath if errata require using > * the out-of-line counter accessor. > @@ -899,12 +939,22 @@ static int __init arch_timer_of_init(struct device_node *np) > arch_timer_c3stop = !of_property_read_bool(np, "always-on"); > > #ifdef CONFIG_FSL_ERRATUM_A008585 > - if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585")) > + if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585")) { > erratum_workaround = &arch_timer_fsl_a008585; > + if (erratum_workaround) { Brilliant! > + static_branch_enable(&arch_timer_read_ool_enabled); > + pr_info("Enabling workaround for FSL erratum A-008585\n"); > + } > + } > +#endif > > - if (erratum_workaround) { > - static_branch_enable(&arch_timer_read_ool_enabled); > - pr_info("Enabling workaround for FSL erratum A-008585\n"); > +#ifdef CONFIG_HISILICON_ERRATUM_161601 > + if (!erratum_workaround && of_property_read_bool(np, "hisilicon,erratum-161601")) { > + erratum_workaround = &arch_timer_hisi_161601; > + if (erratum_workaround) { > + static_branch_enable(&arch_timer_read_ool_enabled); > + pr_info("Enabling workaround for HISILICON erratum 161601\n"); > + } > } > #endif Do you see a pattern here? Surely you can factor a bit of that code (and remove the nonsensical bits). Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2016/10/27 18:58, Marc Zyngier wrote: > On 27/10/16 08:34, Ding Tianhong wrote: >> Erratum Hisilicon-161601 says that the ARM generic timer counter "has the >> potential to contain an erroneous value when 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 the system count registers until the value of the second >> read is larger than the first one by less than 32, the system counter can be guaranteed >> not to return wrong value twice by back-to-back read and the error value is always larger >> than the correct one by 32. Writes to TVAL are replaced with an equivalent write to CVAL. >> >> The workaround is enabled if the hisilicon,erratum-161601 property is found in >> the timer node in the device tree. This can be overridden with the >> clocksource.arm_arch_timer.hisilicon-161601 boot parameter, which allows KVM >> users to enable the workaround until a mechanism is implemented to >> automatically communicate this information. >> >> Fix some description for fsl erratum a008585. >> >> v2: Significant rework based on feedback, including seperate the fsl erratum a008585 >> to another patch, update the erratum name and remove unwanted code. >> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> --- >> Documentation/arm64/silicon-errata.txt | 1 + >> Documentation/kernel-parameters.txt | 9 ++++ >> arch/arm64/include/asm/arch_timer.h | 28 ++++++++++- >> drivers/clocksource/Kconfig | 14 +++++- >> drivers/clocksource/arm_arch_timer.c | 88 ++++++++++++++++++++++++++-------- >> 5 files changed, 118 insertions(+), 22 deletions(-) >> >> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt >> index 405da11..70c5d5e 100644 >> --- a/Documentation/arm64/silicon-errata.txt >> +++ b/Documentation/arm64/silicon-errata.txt >> @@ -63,3 +63,4 @@ stable kernels. >> | Cavium | ThunderX SMMUv2 | #27704 | N/A | >> | | | | | >> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | >> +| Hisilicon | Hip05/Hip06/Hip07 | #161601 | HISILICON_ERRATUM_161601| > > I've already commented on the alignment. Please read my initial review. > It sees misunderstood, fix it this time. >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt >> index 6fa1d8a..735b4b6 100644 >> --- a/Documentation/kernel-parameters.txt >> +++ b/Documentation/kernel-parameters.txt >> @@ -707,6 +707,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted. >> erratum. If unspecified, the workaround is >> enabled based on the device tree. >> >> + clocksource.arm_arch_timer.hisilicon-161601= >> + [ARM64] >> + Format: <bool> >> + Enable/disable the workaround of Hisilicon >> + erratum 161601. This can be useful for KVM >> + guests, if the guest device tree doesn't show the >> + erratum. If unspecified, the workaround is >> + enabled based on the device tree. >> + >> clearcpuid=BITNUM [X86] >> Disable CPUID feature X for the kernel. See >> arch/x86/include/asm/cpufeatures.h for the valid bit >> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h >> index 118719d8..49b3041 100644 >> --- a/arch/arm64/include/asm/arch_timer.h >> +++ b/arch/arm64/include/asm/arch_timer.h >> @@ -29,7 +29,7 @@ >> >> #include <clocksource/arm_arch_timer.h> >> >> -#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) >> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >> extern struct static_key_false arch_timer_read_ool_enabled; >> #define needs_timer_erratum_workaround() \ >> static_branch_unlikely(&arch_timer_read_ool_enabled) >> @@ -65,11 +65,35 @@ extern struct arch_timer_erratum_workaround *erratum_workaround; >> _new; \ >> }) >> >> + >> + >> +/* >> + * The number of retries is an arbitrary value well beyond the highest number >> + * of iterations the loop has been observed to take. >> + * Verify whether the value of the second read is larger than the first by >> + * less than 32 is the only way to confirm the value is correct, the system >> + * counter can be guaranteed not to return wrong value twice by back-to-back read >> + * and the error value is always larger than the correct one by 32. >> + */ >> +#define __hisi_161601_read_reg(reg) ({ \ >> + u64 _old, _new; \ >> + int _retries = 200; \ > > Please document how this value was found (either in the code or in the > commit message). It really difficult to give the accurate standard, theoretically the error should not happened twice together, so maybe 2 is enough here, I just give a arbitrary value. > >> + \ >> + do { \ >> + _old = read_sysreg(reg); \ >> + _new = read_sysreg(reg); \ >> + _retries--; \ >> + } while (unlikely((_new - _old) >> 5) && _retries); \ >> + \ >> + WARN_ON_ONCE(!_retries); \ >> + _new; \ >> +}) > > Same remark as in the previous patch. > I think the sentence *Verify whether the value of the second read is larger than the first by less than 32 is the only way to confirm the value is correct* could explain why should *(_new - _old) >> 5*. it is the same as (_new - _old)/32, also mean the _new should never bigger than _old more than 32. >> + >> #define arch_timer_reg_read_stable(reg) \ >> ({ \ >> u64 _val; \ >> if (needs_timer_erratum_workaround()) \ >> - _val = erratum_workaround->read_##reg(); \ >> + _val = erratum_workaround->read_##reg();\ >> else \ >> _val = read_sysreg(reg); \ >> _val; \ >> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> index 8a753fd..4aafb6a 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -312,8 +312,20 @@ config FSL_ERRATUM_A008585 >> help >> This option enables a workaround for Freescale/NXP Erratum >> A-008585 ("ARM generic timer may contain an erroneous >> - value"). The workaround will only be active if the >> + value"). The workaround will be active if the >> fsl,erratum-a008585 property is found in the timer node. >> + This can be overridden with the clocksource.arm_arch_timer.fsl-a008585 >> + boot parameter. > > Move this hunk to the previous patch. > >> + >> +config HISILICON_ERRATUM_161601 >> + bool "Workaround for Hisilicon Erratum 161601" >> + default y >> + depends on ARM_ARCH_TIMER && ARM64 >> + help >> + This option enables a workaround for Hisilicon Erratum >> + 161601. The workaround will be active if the hisilicon,erratum-161601 >> + property is found in the timer node. This can be overridden with >> + the clocksource.arm_arch_timer.hisilicon-161601 boot parameter. >> >> config ARM_GLOBAL_TIMER >> bool "Support for the ARM global timer" if COMPILE_TEST >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c >> index e4f7fa1..89f1895 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -94,13 +94,14 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg); >> * Architected system timer support. >> */ >> >> -#ifdef CONFIG_FSL_ERRATUM_A008585 >> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >> struct arch_timer_erratum_workaround *erratum_workaround = NULL; >> >> DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled); >> EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled); >> +#endif >> >> - >> +#ifdef CONFIG_FSL_ERRATUM_A008585 >> static u32 fsl_a008585_read_cntp_tval_el0(void) >> { >> return __fsl_a008585_read_reg(cntp_tval_el0); >> @@ -139,6 +140,45 @@ static int __init early_fsl_a008585_cfg(char *buf) >> early_param("clocksource.arm_arch_timer.fsl-a008585", early_fsl_a008585_cfg); >> #endif /* CONFIG_FSL_ERRATUM_A008585 */ >> >> +#ifdef CONFIG_HISILICON_ERRATUM_161601 >> +static u32 hisi_161601_read_cntp_tval_el0(void) >> +{ >> + return __hisi_161601_read_reg(cntp_tval_el0); >> +} >> + >> +static u32 hisi_161601_read_cntv_tval_el0(void) >> +{ >> + return __hisi_161601_read_reg(cntv_tval_el0); >> +} >> + >> +static u64 hisi_161601_read_cntvct_el0(void) >> +{ >> + return __hisi_161601_read_reg(cntvct_el0); >> +} >> + >> +static struct arch_timer_erratum_workaround arch_timer_hisi_161601 = { >> + .read_cntp_tval_el0 = hisi_161601_read_cntp_tval_el0, >> + .read_cntv_tval_el0 = hisi_161601_read_cntv_tval_el0, >> + .read_cntvct_el0 = hisi_161601_read_cntvct_el0, >> +}; >> + >> +static int __init early_hisi_161601_cfg(char *buf) >> +{ >> + int ret; >> + bool val; >> + >> + ret = strtobool(buf, &val); >> + if (ret) >> + return ret; >> + >> + if (val) >> + erratum_workaround = &arch_timer_hisi_161601; >> + >> + return 0; >> +} >> +early_param("clocksource.arm_arch_timer.hisilicon-161601", early_hisi_161601_cfg); >> +#endif /* CONFIG_HISILICON_ERRATUM_161601 */ >> + >> static __always_inline >> void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val, >> struct clock_event_device *clk) >> @@ -288,8 +328,8 @@ 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_FSL_ERRATUM_A008585 >> -static __always_inline void fsl_a008585_set_next_event(const int access, >> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >> +static __always_inline void erratum_set_next_event(const int access, >> unsigned long evt, struct clock_event_device *clk) >> { >> unsigned long ctrl; >> @@ -307,20 +347,20 @@ static __always_inline void fsl_a008585_set_next_event(const int access, >> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); >> } >> >> -static int fsl_a008585_set_next_event_virt(unsigned long evt, >> +static int erratum_set_next_event_virt(unsigned long evt, >> struct clock_event_device *clk) >> { >> - fsl_a008585_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk); >> + erratum_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk); >> return 0; >> } >> >> -static int fsl_a008585_set_next_event_phys(unsigned long evt, >> +static int erratum_set_next_event_phys(unsigned long evt, >> struct clock_event_device *clk) >> { >> - fsl_a008585_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk); >> + erratum_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk); >> return 0; >> } >> -#endif /* CONFIG_FSL_ERRATUM_A008585 */ >> +#endif >> >> static int arch_timer_set_next_event_virt(unsigned long evt, >> struct clock_event_device *clk) >> @@ -350,16 +390,16 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt, >> return 0; >> } >> >> -static void fsl_a008585_set_sne(struct clock_event_device *clk) >> +static void erratum_set_sne(struct clock_event_device *clk) >> { >> -#ifdef CONFIG_FSL_ERRATUM_A008585 >> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >> if (!static_branch_unlikely(&arch_timer_read_ool_enabled)) >> return; >> >> if (arch_timer_uses_ppi == VIRT_PPI) >> - clk->set_next_event = fsl_a008585_set_next_event_virt; >> + clk->set_next_event = erratum_set_next_event_virt; >> else >> - clk->set_next_event = fsl_a008585_set_next_event_phys; >> + clk->set_next_event = erratum_set_next_event_phys; >> #endif > > This should be in the previous patch as well, as it only messes with the > FSL erratum. > Ok. >> } >> >> @@ -392,7 +432,7 @@ static void __arch_timer_setup(unsigned type, >> BUG(); >> } >> >> - fsl_a008585_set_sne(clk); >> + erratum_set_sne(clk); >> } else { >> clk->features |= CLOCK_EVT_FEAT_DYNIRQ; >> clk->name = "arch_mem_timer"; >> @@ -612,7 +652,7 @@ static void __init arch_counter_register(unsigned type) >> >> clocksource_counter.archdata.vdso_direct = true; >> >> -#ifdef CONFIG_FSL_ERRATUM_A008585 >> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >> /* >> * Don't use the vdso fastpath if errata require using >> * the out-of-line counter accessor. >> @@ -899,12 +939,22 @@ static int __init arch_timer_of_init(struct device_node *np) >> arch_timer_c3stop = !of_property_read_bool(np, "always-on"); >> >> #ifdef CONFIG_FSL_ERRATUM_A008585 >> - if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585")) >> + if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585")) { >> erratum_workaround = &arch_timer_fsl_a008585; >> + if (erratum_workaround) { > > Brilliant! > >> + static_branch_enable(&arch_timer_read_ool_enabled); >> + pr_info("Enabling workaround for FSL erratum A-008585\n"); >> + } >> + } >> +#endif >> >> - if (erratum_workaround) { >> - static_branch_enable(&arch_timer_read_ool_enabled); >> - pr_info("Enabling workaround for FSL erratum A-008585\n"); >> +#ifdef CONFIG_HISILICON_ERRATUM_161601 >> + if (!erratum_workaround && of_property_read_bool(np, "hisilicon,erratum-161601")) { >> + erratum_workaround = &arch_timer_hisi_161601; >> + if (erratum_workaround) { >> + static_branch_enable(&arch_timer_read_ool_enabled); >> + pr_info("Enabling workaround for HISILICON erratum 161601\n"); >> + } >> } >> #endif > > Do you see a pattern here? Surely you can factor a bit of that code (and > remove the nonsensical bits). > Yes, found it, try to fix it. Thanks. Ding > Thanks, > > M. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 27/10/16 13:17, Ding Tianhong wrote: > > > On 2016/10/27 18:58, Marc Zyngier wrote: >> On 27/10/16 08:34, Ding Tianhong wrote: >>> Erratum Hisilicon-161601 says that the ARM generic timer counter "has the >>> potential to contain an erroneous value when 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 the system count registers until the value of the second >>> read is larger than the first one by less than 32, the system counter can be guaranteed >>> not to return wrong value twice by back-to-back read and the error value is always larger >>> than the correct one by 32. Writes to TVAL are replaced with an equivalent write to CVAL. >>> >>> The workaround is enabled if the hisilicon,erratum-161601 property is found in >>> the timer node in the device tree. This can be overridden with the >>> clocksource.arm_arch_timer.hisilicon-161601 boot parameter, which allows KVM >>> users to enable the workaround until a mechanism is implemented to >>> automatically communicate this information. >>> >>> Fix some description for fsl erratum a008585. >>> >>> v2: Significant rework based on feedback, including seperate the fsl erratum a008585 >>> to another patch, update the erratum name and remove unwanted code. >>> >>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >>> --- >>> Documentation/arm64/silicon-errata.txt | 1 + >>> Documentation/kernel-parameters.txt | 9 ++++ >>> arch/arm64/include/asm/arch_timer.h | 28 ++++++++++- >>> drivers/clocksource/Kconfig | 14 +++++- >>> drivers/clocksource/arm_arch_timer.c | 88 ++++++++++++++++++++++++++-------- >>> 5 files changed, 118 insertions(+), 22 deletions(-) >>> >>> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt >>> index 405da11..70c5d5e 100644 >>> --- a/Documentation/arm64/silicon-errata.txt >>> +++ b/Documentation/arm64/silicon-errata.txt >>> @@ -63,3 +63,4 @@ stable kernels. >>> | Cavium | ThunderX SMMUv2 | #27704 | N/A | >>> | | | | | >>> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | >>> +| Hisilicon | Hip05/Hip06/Hip07 | #161601 | HISILICON_ERRATUM_161601| >> >> I've already commented on the alignment. Please read my initial review. >> > > It sees misunderstood, fix it this time. > >>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt >>> index 6fa1d8a..735b4b6 100644 >>> --- a/Documentation/kernel-parameters.txt >>> +++ b/Documentation/kernel-parameters.txt >>> @@ -707,6 +707,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted. >>> erratum. If unspecified, the workaround is >>> enabled based on the device tree. >>> >>> + clocksource.arm_arch_timer.hisilicon-161601= >>> + [ARM64] >>> + Format: <bool> >>> + Enable/disable the workaround of Hisilicon >>> + erratum 161601. This can be useful for KVM >>> + guests, if the guest device tree doesn't show the >>> + erratum. If unspecified, the workaround is >>> + enabled based on the device tree. >>> + >>> clearcpuid=BITNUM [X86] >>> Disable CPUID feature X for the kernel. See >>> arch/x86/include/asm/cpufeatures.h for the valid bit >>> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h >>> index 118719d8..49b3041 100644 >>> --- a/arch/arm64/include/asm/arch_timer.h >>> +++ b/arch/arm64/include/asm/arch_timer.h >>> @@ -29,7 +29,7 @@ >>> >>> #include <clocksource/arm_arch_timer.h> >>> >>> -#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) >>> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >>> extern struct static_key_false arch_timer_read_ool_enabled; >>> #define needs_timer_erratum_workaround() \ >>> static_branch_unlikely(&arch_timer_read_ool_enabled) >>> @@ -65,11 +65,35 @@ extern struct arch_timer_erratum_workaround *erratum_workaround; >>> _new; \ >>> }) >>> >>> + >>> + >>> +/* >>> + * The number of retries is an arbitrary value well beyond the highest number >>> + * of iterations the loop has been observed to take. >>> + * Verify whether the value of the second read is larger than the first by >>> + * less than 32 is the only way to confirm the value is correct, the system >>> + * counter can be guaranteed not to return wrong value twice by back-to-back read >>> + * and the error value is always larger than the correct one by 32. >>> + */ >>> +#define __hisi_161601_read_reg(reg) ({ \ >>> + u64 _old, _new; \ >>> + int _retries = 200; \ >> >> Please document how this value was found (either in the code or in the >> commit message). > > It really difficult to give the accurate standard, theoretically the error should not happened > twice together, so maybe 2 is enough here, I just give a arbitrary value. > >> >>> + \ >>> + do { \ >>> + _old = read_sysreg(reg); \ >>> + _new = read_sysreg(reg); \ >>> + _retries--; \ >>> + } while (unlikely((_new - _old) >> 5) && _retries); \ >>> + \ >>> + WARN_ON_ONCE(!_retries); \ >>> + _new; \ >>> +}) >> >> Same remark as in the previous patch. >> > > I think the sentence *Verify whether the value of the second read is larger than the first by > less than 32 is the only way to confirm the value is correct* could explain why should *(_new - _old) >> 5*. > it is the same as (_new - _old)/32, also mean the _new should never bigger than _old more than 32. This is not about the explanation of the erratum, but about the location of the #define, which can be made private to the .c file instead of being globally visible. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Oct 27, 2016 at 03:34:08PM +0800, Ding Tianhong wrote: > This erratum describes a bug in logic outside the core, so MIDR can't be > used to identify its presence, and reading an SoC-specific revision > register from common arch timer code would be awkward. So, describe it > in the device tree. > > v2: Use the new erratum name and update the description. > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > --- > Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++ > 1 file changed, 8 insertions(+) Acked-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt index ef5fbe9..c27b2c4 100644 --- a/Documentation/devicetree/bindings/arm/arch_timer.txt +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt @@ -31,6 +31,14 @@ to deliver its interrupts via SPIs. This also affects writes to the tval register, due to the implicit counter read. +- hisilicon,erratum-161601 : A boolean property. Indicates the presence of + erratum 161601, which says that reading the counter is unreliable unless + reading twice on the register and the value of the second read is larger + than the first by less than 32. If the verification is unsuccessful, then + discard the value of this read and repeat this procedure until the verification + is successful. 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 erratum describes a bug in logic outside the core, so MIDR can't be used to identify its presence, and reading an SoC-specific revision register from common arch timer code would be awkward. So, describe it in the device tree. v2: Use the new erratum name and update the description. Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++ 1 file changed, 8 insertions(+) -- 1.9.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel