From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753973AbdCXRsi (ORCPT ); Fri, 24 Mar 2017 13:48:38 -0400 Received: from mail-wm0-f41.google.com ([74.125.82.41]:36188 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753449AbdCXRs3 (ORCPT ); Fri, 24 Mar 2017 13:48:29 -0400 MIME-Version: 1.0 In-Reply-To: <20170320174829.28182-7-marc.zyngier@arm.com> References: <20170320174829.28182-1-marc.zyngier@arm.com> <20170320174829.28182-7-marc.zyngier@arm.com> From: dann frazier Date: Fri, 24 Mar 2017 11:48:16 -0600 Message-ID: Subject: Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods To: Marc Zyngier Cc: linux-arm-kernel , "linux-kernel@vger.kernel.org" , Mark Rutland , Catalin Marinas , Daniel Lezcano , Will Deacon , Scott Wood , Hanjun Guo , Ding Tianhong , Seth Forshee Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 20, 2017 at 11:48 AM, Marc Zyngier wrote: > We're currently stuck with DT when it comes to handling errata, which > is pretty restrictive. In order to make things more flexible, let's > introduce an infrastructure that could support alternative discovery > methods. No change in functionality. > > Reviewed-by: Hanjun Guo > Signed-off-by: Marc Zyngier > --- > arch/arm64/include/asm/arch_timer.h | 7 +++- > drivers/clocksource/arm_arch_timer.c | 80 +++++++++++++++++++++++++++++++----- > 2 files changed, 75 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index b4b34004a21e..5cd964e90d11 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -37,9 +37,14 @@ extern struct static_key_false arch_timer_read_ool_enabled; > #define needs_unstable_timer_counter_workaround() false > #endif > > +enum arch_timer_erratum_match_type { > + ate_match_dt, > +}; > > struct arch_timer_erratum_workaround { > - const char *id; /* Indicate the Erratum ID */ > + enum arch_timer_erratum_match_type match_type; > + const void *id; > + const char *desc; > u32 (*read_cntp_tval_el0)(void); > u32 (*read_cntv_tval_el0)(void); > u64 (*read_cntvct_el0)(void); > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 7a8a4117f123..6a0f0e161a0f 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -182,7 +182,9 @@ EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled); > static const struct arch_timer_erratum_workaround ool_workarounds[] = { > #ifdef CONFIG_FSL_ERRATUM_A008585 > { > + .match_type = ate_match_dt, > .id = "fsl,erratum-a008585", > + .desc = "Freescale erratum a005858", > .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, > @@ -190,13 +192,78 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = { > #endif > #ifdef CONFIG_HISILICON_ERRATUM_161010101 > { > + .match_type = ate_match_dt, > .id = "hisilicon,erratum-161010101", > + .desc = "HiSilicon erratum 161010101", > .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0, > .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0, > .read_cntvct_el0 = hisi_161010101_read_cntvct_el0, > }, > #endif > }; > + > +typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *, > + const void *); > + > +static > +bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround *wa, > + const void *arg) > +{ > + const struct device_node *np = arg; > + > + return of_property_read_bool(np, wa->id); > +} > + > +static const struct arch_timer_erratum_workaround * > +arch_timer_iterate_errata(enum arch_timer_erratum_match_type type, > + ate_match_fn_t match_fn, > + void *arg) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(ool_workarounds); i++) { > + if (ool_workarounds[i].match_type != type) > + continue; > + > + if (match_fn(&ool_workarounds[i], arg)) > + return &ool_workarounds[i]; > + } > + > + return NULL; > +} > + > +static > +void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa) > +{ > + timer_unstable_counter_workaround = wa; > + static_branch_enable(&arch_timer_read_ool_enabled); > +} > + > +static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type type, > + void *arg) > +{ > + const struct arch_timer_erratum_workaround *wa; > + ate_match_fn_t match_fn = NULL; > + > + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) > + return; > + > + switch (type) { > + case ate_match_dt: > + match_fn = arch_timer_check_dt_erratum; > + break; hey Marc, Would it make sense to have a default case here that warns & returns? That wouldn't get hit by this series as-is, but might avoid a NULL callback in the future. -dann > + } > + > + wa = arch_timer_iterate_errata(type, match_fn, arg); > + if (!wa) > + return; > + > + arch_timer_enable_workaround(wa); > + pr_info("Enabling global workaround for %s\n", wa->desc); > +} > + > +#else > +#define arch_timer_check_ool_workaround(t,a) do { } while(0) > #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */ > > static __always_inline > @@ -960,17 +1027,8 @@ static int __init arch_timer_of_init(struct device_node *np) > > arch_timer_c3stop = !of_property_read_bool(np, "always-on"); > > -#ifdef CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND > - for (i = 0; i < ARRAY_SIZE(ool_workarounds); i++) { > - if (of_property_read_bool(np, ool_workarounds[i].id)) { > - timer_unstable_counter_workaround = &ool_workarounds[i]; > - static_branch_enable(&arch_timer_read_ool_enabled); > - pr_info("arch_timer: Enabling workaround for %s\n", > - timer_unstable_counter_workaround->id); > - break; > - } > - } > -#endif > + /* Check for globally applicable workarounds */ > + arch_timer_check_ool_workaround(ate_match_dt, np); > > /* > * If we cannot rely on firmware initializing the timer registers then > -- > 2.11.0 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: dann.frazier@canonical.com (dann frazier) Date: Fri, 24 Mar 2017 11:48:16 -0600 Subject: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods In-Reply-To: <20170320174829.28182-7-marc.zyngier@arm.com> References: <20170320174829.28182-1-marc.zyngier@arm.com> <20170320174829.28182-7-marc.zyngier@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Mar 20, 2017 at 11:48 AM, Marc Zyngier wrote: > We're currently stuck with DT when it comes to handling errata, which > is pretty restrictive. In order to make things more flexible, let's > introduce an infrastructure that could support alternative discovery > methods. No change in functionality. > > Reviewed-by: Hanjun Guo > Signed-off-by: Marc Zyngier > --- > arch/arm64/include/asm/arch_timer.h | 7 +++- > drivers/clocksource/arm_arch_timer.c | 80 +++++++++++++++++++++++++++++++----- > 2 files changed, 75 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index b4b34004a21e..5cd964e90d11 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -37,9 +37,14 @@ extern struct static_key_false arch_timer_read_ool_enabled; > #define needs_unstable_timer_counter_workaround() false > #endif > > +enum arch_timer_erratum_match_type { > + ate_match_dt, > +}; > > struct arch_timer_erratum_workaround { > - const char *id; /* Indicate the Erratum ID */ > + enum arch_timer_erratum_match_type match_type; > + const void *id; > + const char *desc; > u32 (*read_cntp_tval_el0)(void); > u32 (*read_cntv_tval_el0)(void); > u64 (*read_cntvct_el0)(void); > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 7a8a4117f123..6a0f0e161a0f 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -182,7 +182,9 @@ EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled); > static const struct arch_timer_erratum_workaround ool_workarounds[] = { > #ifdef CONFIG_FSL_ERRATUM_A008585 > { > + .match_type = ate_match_dt, > .id = "fsl,erratum-a008585", > + .desc = "Freescale erratum a005858", > .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, > @@ -190,13 +192,78 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = { > #endif > #ifdef CONFIG_HISILICON_ERRATUM_161010101 > { > + .match_type = ate_match_dt, > .id = "hisilicon,erratum-161010101", > + .desc = "HiSilicon erratum 161010101", > .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0, > .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0, > .read_cntvct_el0 = hisi_161010101_read_cntvct_el0, > }, > #endif > }; > + > +typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *, > + const void *); > + > +static > +bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround *wa, > + const void *arg) > +{ > + const struct device_node *np = arg; > + > + return of_property_read_bool(np, wa->id); > +} > + > +static const struct arch_timer_erratum_workaround * > +arch_timer_iterate_errata(enum arch_timer_erratum_match_type type, > + ate_match_fn_t match_fn, > + void *arg) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(ool_workarounds); i++) { > + if (ool_workarounds[i].match_type != type) > + continue; > + > + if (match_fn(&ool_workarounds[i], arg)) > + return &ool_workarounds[i]; > + } > + > + return NULL; > +} > + > +static > +void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa) > +{ > + timer_unstable_counter_workaround = wa; > + static_branch_enable(&arch_timer_read_ool_enabled); > +} > + > +static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type type, > + void *arg) > +{ > + const struct arch_timer_erratum_workaround *wa; > + ate_match_fn_t match_fn = NULL; > + > + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) > + return; > + > + switch (type) { > + case ate_match_dt: > + match_fn = arch_timer_check_dt_erratum; > + break; hey Marc, Would it make sense to have a default case here that warns & returns? That wouldn't get hit by this series as-is, but might avoid a NULL callback in the future. -dann > + } > + > + wa = arch_timer_iterate_errata(type, match_fn, arg); > + if (!wa) > + return; > + > + arch_timer_enable_workaround(wa); > + pr_info("Enabling global workaround for %s\n", wa->desc); > +} > + > +#else > +#define arch_timer_check_ool_workaround(t,a) do { } while(0) > #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */ > > static __always_inline > @@ -960,17 +1027,8 @@ static int __init arch_timer_of_init(struct device_node *np) > > arch_timer_c3stop = !of_property_read_bool(np, "always-on"); > > -#ifdef CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND > - for (i = 0; i < ARRAY_SIZE(ool_workarounds); i++) { > - if (of_property_read_bool(np, ool_workarounds[i].id)) { > - timer_unstable_counter_workaround = &ool_workarounds[i]; > - static_branch_enable(&arch_timer_read_ool_enabled); > - pr_info("arch_timer: Enabling workaround for %s\n", > - timer_unstable_counter_workaround->id); > - break; > - } > - } > -#endif > + /* Check for globally applicable workarounds */ > + arch_timer_check_ool_workaround(ate_match_dt, np); > > /* > * If we cannot rely on firmware initializing the timer registers then > -- > 2.11.0 >