Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1604844 > unrolled thread

[PATCH v2 00/18] clocksource/arch_timer: Errata workaround infrastructure rework

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/18] clocksource/arch_timer: Errata workaround infrastructure rework Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 18:50 +0100
    [PATCH v2 01/18] arm64: Allow checking of a CPU-local erratum Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 18:50 +0100
      Re: [PATCH v2 01/18] arm64: Allow checking of a CPU-local erratum Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-22 10:30 +0100
    [PATCH v2 11/18] arm64: arch_timer: Rework the set_next_event workarounds Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:00 +0100
    [PATCH v2 08/18] arm64: arch_timer: Add erratum handler for CPU-specific capability Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:00 +0100
    [PATCH v2 05/18] arm64: cpu_errata: Add capability to advertise Cortex-A73 erratum 858921 Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:00 +0100
      Re: [PATCH v2 05/18] arm64: cpu_errata: Add capability to advertise  Cortex-A73 erratum 858921 Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-22 16:10 +0100
    [PATCH v2 09/18] arm64: arch_timer: Move arch_timer_reg_read/write around Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:00 +0100
      Re: [PATCH v2 09/18] arm64: arch_timer: Move  arch_timer_reg_read/write around Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-22 17:00 +0100
    [PATCH v2 07/18] arm64: arch_timer: Add erratum handler for globally defined capability Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:00 +0100
    [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
      Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-22 16:50 +0100
        Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Marc Zyngier <marc.zyngier@arm.com> - 2017-03-22 17:00 +0100
        Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Marc Zyngier <marc.zyngier@arm.com> - 2017-03-22 17:00 +0100
          Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-22 18:10 +0100
          Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-23 18:40 +0100
            Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Marc Zyngier <marc.zyngier@arm.com> - 2017-03-24 15:00 +0100
              Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-27 10:00 +0200
      Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods dann frazier <dann.frazier@canonical.com> - 2017-03-24 18:50 +0100
        Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for  multiple erratum detection methods Marc Zyngier <marc.zyngier@arm.com> - 2017-03-24 19:20 +0100
    [PATCH v2 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 16/18] arm64: arch_timer: Workaround for Cortex-A73 erratum 858921 Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 13/18] arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 04/18] arm64: cpu_errata: Allow an erratum to be match for all revisions of a core Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
      Re: [PATCH v2 04/18] arm64: cpu_errata: Allow an erratum to be match  for all revisions of a core Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-22 16:10 +0100
    [PATCH v2 18/18] arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI matching data Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 02/18] arm64: Add CNTVCT_EL0 trap handler Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 03/18] arm64: Define Cortex-A73 MIDR Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 17/18] arm64: arch_timer: Allow erratum matching with ACPI OEM information Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:40 +0100
    [PATCH v2 10/18] arm64: arch_timer: Get rid of erratum_workaround_set_sne Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:50 +0100
    [PATCH v2 12/18] arm64: arch_timer: Make workaround methods optional Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:50 +0100
    [PATCH v2 14/18] arm64: arch_timer: Move clocksource_counter and co around Marc Zyngier <marc.zyngier@arm.com> - 2017-03-20 19:50 +0100
    Re: [PATCH v2 00/18] clocksource/arch_timer: Errata workaround  infrastructure rework Ding Tianhong <dingtianhong@huawei.com> - 2017-03-22 15:00 +0100

Page 1 of 2  [1] 2  Next page →

#1604844 — [PATCH v2 00/18] clocksource/arch_timer: Errata workaround infrastructure rework

It has recently become obvious that a number of arm64 systems have
been blessed with a set of timers that are slightly less than perfect,
and require a bit of hand-holding. We already have a bunch of
errata-specific code to deal with this, but as we're adding more
potential detection methods (DT, ACPI, capability), things are getting
a bit out of hands.

Instead of adding more ad-hoc fixes to an already difficult code base,
let's give ourselves a bit of an infrastructure that can deal with
this and hide most of the uggliness behind frendly accessors.

The series is structured as such:

- A bunch of arm64 specific patches that allow the rest of the
  workaround infrastructure to be built upon (such as being able to
  trap userspace access to the virtual counter). These are now
  separate in order to allow the creation of a shared branch between
  the arm64 and clocksource trees.

- The following patches rework the existing workarounds, allowing
  errata to be matched using a given detection method

- Another patch allows a workaround to affect a subset of the CPUs,
  and not the whole system

- We then work around a Cortex-A73 erratum, whose counter can return a
  wrong value if read while crossing a 32bit boundary

- Finally, we add some ACPI-specific workarounds for HiSilicon
  platforms that have the HISILICON_ERRATUM_161010101 defect.

Note that so far, we only deal with arm64. Once the infrastructure is
agreed upon, we can look at generalizing it (to some extent) to 32bit
ARM (typical use case would be a 32bit guest running on an affected
host).

* From v1:
  - Addressed Hanjun and Mark review comments
  - Moved arm64 specific patches to the beginning of the series,
    leaving the clocksource patches at the end, resulting in an extra
    patch.
  - Added RBs, TBs, and Acks.

Marc Zyngier (18):
  arm64: Allow checking of a CPU-local erratum
  arm64: Add CNTVCT_EL0 trap handler
  arm64: Define Cortex-A73 MIDR
  arm64: cpu_errata: Allow an erratum to be match for all revisions of a
    core
  arm64: cpu_errata: Add capability to advertise Cortex-A73 erratum
    858921
  arm64: arch_timer: Add infrastructure for multiple erratum detection
    methods
  arm64: arch_timer: Add erratum handler for globally defined capability
  arm64: arch_timer: Add erratum handler for CPU-specific capability
  arm64: arch_timer: Move arch_timer_reg_read/write around
  arm64: arch_timer: Get rid of erratum_workaround_set_sne
  arm64: arch_timer: Rework the set_next_event workarounds
  arm64: arch_timer: Make workaround methods optional
  arm64: arch_timer: Allows a CPU-specific erratum to only affect a
    subset of CPUs
  arm64: arch_timer: Move clocksource_counter and co around
  arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled
  arm64: arch_timer: Workaround for Cortex-A73 erratum 858921
  arm64: arch_timer: Allow erratum matching with ACPI OEM information
  arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI matching data

 Documentation/arm64/silicon-errata.txt |   1 +
 arch/arm64/include/asm/arch_timer.h    |  44 ++-
 arch/arm64/include/asm/cpucaps.h       |   3 +-
 arch/arm64/include/asm/cputype.h       |   2 +
 arch/arm64/include/asm/esr.h           |   2 +
 arch/arm64/kernel/cpu_errata.c         |  15 +
 arch/arm64/kernel/cpufeature.c         |  13 +-
 arch/arm64/kernel/traps.c              |  14 +
 drivers/clocksource/Kconfig            |  11 +
 drivers/clocksource/arm_arch_timer.c   | 535 +++++++++++++++++++++++----------
 10 files changed, 471 insertions(+), 169 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1604846 — [PATCH v2 01/18] arm64: Allow checking of a CPU-local erratum

this_cpu_has_cap() only checks the feature array, and not the errata
one. In order to be able to check for a CPU-local erratum, allow it
to inspect the latter as well.

This is consistent with cpus_have_cap()'s behaviour, which includes
errata already.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index abda8e861865..6eb77ae99b79 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1090,20 +1090,29 @@ static void __init setup_feature_capabilities(void)
  * Check if the current CPU has a given feature capability.
  * Should be called from non-preemptible context.
  */
-bool this_cpu_has_cap(unsigned int cap)
+static bool __this_cpu_has_cap(const struct arm64_cpu_capabilities *cap_array,
+			       unsigned int cap)
 {
 	const struct arm64_cpu_capabilities *caps;
 
 	if (WARN_ON(preemptible()))
 		return false;
 
-	for (caps = arm64_features; caps->desc; caps++)
+	for (caps = cap_array; caps->desc; caps++)
 		if (caps->capability == cap && caps->matches)
 			return caps->matches(caps, SCOPE_LOCAL_CPU);
 
 	return false;
 }
 
+extern const struct arm64_cpu_capabilities arm64_errata[];
+
+bool this_cpu_has_cap(unsigned int cap)
+{
+	return (__this_cpu_has_cap(arm64_features, cap) ||
+		__this_cpu_has_cap(arm64_errata, cap));
+}
+
 void __init setup_cpu_features(void)
 {
 	u32 cwg;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1606305 — Re: [PATCH v2 01/18] arm64: Allow checking of a CPU-local erratum

On Mon, Mar 20, 2017 at 05:48:12PM +0000, Marc Zyngier wrote:
> this_cpu_has_cap() only checks the feature array, and not the errata
> one. In order to be able to check for a CPU-local erratum, allow it
> to inspect the latter as well.
> 
> This is consistent with cpus_have_cap()'s behaviour, which includes
> errata already.
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1604853 — [PATCH v2 11/18] arm64: arch_timer: Rework the set_next_event workarounds

The way we work around errata affecting set_next_event is not very
nice, at it imposes this workaround on errata that do not need it.

Add new workaround hooks and let the existing workarounds use them.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/arch_timer.h  |  4 ++++
 drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index e5325299aa6d..58572d38e9db 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -43,6 +43,8 @@ enum arch_timer_erratum_match_type {
 	ate_match_local_cap_id,
 };
 
+struct clock_event_device;
+
 struct arch_timer_erratum_workaround {
 	enum arch_timer_erratum_match_type match_type;
 	const void *id;
@@ -50,6 +52,8 @@ struct arch_timer_erratum_workaround {
 	u32 (*read_cntp_tval_el0)(void);
 	u32 (*read_cntv_tval_el0)(void);
 	u64 (*read_cntvct_el0)(void);
+	int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
+	int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
 };
 
 extern const struct arch_timer_erratum_workaround *timer_unstable_counter_workaround;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 4caafbdefb9d..b9f01daafdfa 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -282,6 +282,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
 		.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,
+		.set_next_event_phys = erratum_set_next_event_tval_phys,
+		.set_next_event_virt = erratum_set_next_event_tval_virt,
 	},
 #endif
 #ifdef CONFIG_HISILICON_ERRATUM_161010101
@@ -292,6 +294,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
 		.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,
+		.set_next_event_phys = erratum_set_next_event_tval_phys,
+		.set_next_event_virt = erratum_set_next_event_tval_virt,
 	},
 #endif
 };
@@ -384,10 +388,24 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 		local ? "local" : "global", wa->desc);
 }
 
+#define erratum_handler(fn, r, ...)					\
+({									\
+	bool __val;							\
+	if (needs_unstable_timer_counter_workaround() &&		\
+	    timer_unstable_counter_workaround->fn) {			\
+		r = timer_unstable_counter_workaround->fn(__VA_ARGS__);	\
+		__val = true;						\
+	} else {							\
+		__val = false;						\
+	}								\
+	__val;								\
+})
+
 #else
 #define arch_timer_check_ool_workaround(t,a)		do { } while(0)
 #define erratum_set_next_event_tval_virt(...)		({BUG(); 0;})
 #define erratum_set_next_event_tval_phys(...)		({BUG(); 0;})
+#define erratum_handler(fn, r, ...)			({false;})
 #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
 
 static __always_inline irqreturn_t timer_handler(const int access,
@@ -480,8 +498,10 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
 static int arch_timer_set_next_event_virt(unsigned long evt,
 					  struct clock_event_device *clk)
 {
-	if (needs_unstable_timer_counter_workaround())
-		return erratum_set_next_event_tval_virt(evt, clk);
+	int ret;
+
+	if (erratum_handler(set_next_event_virt, ret, evt, clk))
+		return ret;
 
 	set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
 	return 0;
@@ -490,8 +510,10 @@ static int arch_timer_set_next_event_virt(unsigned long evt,
 static int arch_timer_set_next_event_phys(unsigned long evt,
 					  struct clock_event_device *clk)
 {
-	if (needs_unstable_timer_counter_workaround())
-		return erratum_set_next_event_tval_phys(evt, clk);
+	int ret;
+
+	if (erratum_handler(set_next_event_phys, ret, evt, clk))
+		return ret;
 
 	set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
 	return 0;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1604854 — [PATCH v2 08/18] arm64: arch_timer: Add erratum handler for CPU-specific capability

Should we ever have a workaround for an erratum that is detected using
a capability and affecting a particular CPU, it'd be nice to have
a way to probe them directly.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 48bd730d568f..e5325299aa6d 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -40,6 +40,7 @@ extern struct static_key_false arch_timer_read_ool_enabled;
 enum arch_timer_erratum_match_type {
 	ate_match_dt,
 	ate_match_global_cap_id,
+	ate_match_local_cap_id,
 };
 
 struct arch_timer_erratum_workaround {
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index a0b1108a4a24..5069cb3d4326 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -221,6 +221,13 @@ bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaro
 	return cpus_have_cap((uintptr_t)wa->id);
 }
 
+static
+bool arch_timer_check_local_cap_erratum(const struct arch_timer_erratum_workaround *wa,
+					const void *arg)
+{
+	return this_cpu_has_cap((uintptr_t)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,
@@ -251,9 +258,7 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 {
 	const struct arch_timer_erratum_workaround *wa;
 	ate_match_fn_t match_fn = NULL;
-
-	if (static_branch_unlikely(&arch_timer_read_ool_enabled))
-		return;
+	bool local = false;
 
 	switch (type) {
 	case ate_match_dt:
@@ -262,14 +267,27 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 	case ate_match_global_cap_id:
 		match_fn = arch_timer_check_global_cap_erratum;
 		break;
+	case ate_match_local_cap_id:
+		match_fn = arch_timer_check_local_cap_erratum;
+		local = true;
+		break;
 	}
 
 	wa = arch_timer_iterate_errata(type, match_fn, arg);
 	if (!wa)
 		return;
 
+	if (static_branch_unlikely(&arch_timer_read_ool_enabled)) {
+		if (wa != timer_unstable_counter_workaround)
+			pr_warn("Can't enable workaround for %s (clashes with %s\n)",
+				wa->desc,
+				timer_unstable_counter_workaround->desc);
+		return;
+	}
+
 	arch_timer_enable_workaround(wa);
-	pr_info("Enabling global workaround for %s\n", wa->desc);
+	pr_info("Enabling %s workaround for %s\n",
+		local ? "local" : "global", wa->desc);
 }
 
 #else
@@ -529,6 +547,8 @@ static void __arch_timer_setup(unsigned type,
 			BUG();
 		}
 
+		arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
+
 		erratum_workaround_set_sne(clk);
 	} else {
 		clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1604872 — [PATCH v2 05/18] arm64: cpu_errata: Add capability to advertise Cortex-A73 erratum 858921

In order to work around Cortex-A73 erratum 858921 in a subsequent
patch, add the required capability that advertise the erratum.

As the configuration option it depends on is not present yet,
this has no immediate effect.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/arm64/silicon-errata.txt | 1 +
 arch/arm64/include/asm/cpucaps.h       | 3 ++-
 arch/arm64/kernel/cpu_errata.c         | 8 ++++++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 2f66683500b8..10f2dddbf449 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -54,6 +54,7 @@ stable kernels.
 | ARM            | Cortex-A57      | #852523         | N/A                         |
 | ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220        |
 | ARM            | Cortex-A72      | #853709         | N/A                         |
+| ARM            | Cortex-A73      | #858921         | ARM64_ERRATUM_858921        |
 | ARM            | MMU-500         | #841119,#826419 | N/A                         |
 |                |                 |                 |                             |
 | Cavium         | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375        |
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index fb78a5d3b60b..b3aab8a17868 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -37,7 +37,8 @@
 #define ARM64_HAS_NO_FPSIMD			16
 #define ARM64_WORKAROUND_REPEAT_TLBI		17
 #define ARM64_WORKAROUND_QCOM_FALKOR_E1003	18
+#define ARM64_WORKAROUND_858921			19
 
-#define ARM64_NCAPS				19
+#define ARM64_NCAPS				20
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 2be1d1c05303..2ed2a7657711 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -158,6 +158,14 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 			   MIDR_CPU_VAR_REV(0, 0)),
 	},
 #endif
+#ifdef CONFIG_ARM64_ERRATUM_858921
+	{
+	/* Cortex-A73 all versions */
+		.desc = "ARM erratum 858921",
+		.capability = ARM64_WORKAROUND_858921,
+		MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
+	},
+#endif
 	{
 	}
 };
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1606647 — Re: [PATCH v2 05/18] arm64: cpu_errata: Add capability to advertise Cortex-A73 erratum 858921

On Mon, Mar 20, 2017 at 05:48:16PM +0000, Marc Zyngier wrote:
> In order to work around Cortex-A73 erratum 858921 in a subsequent
> patch, add the required capability that advertise the erratum.
> 
> As the configuration option it depends on is not present yet,
> this has no immediate effect.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>



-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1604873 — [PATCH v2 09/18] arm64: arch_timer: Move arch_timer_reg_read/write around

As we're about to move things around, let's start with the low
level read/write functions. This allows us to use these functions
in the errata handling code without having to use forward declaration
of static functions.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 124 +++++++++++++++++------------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5069cb3d4326..e0e4b0e6825d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -96,6 +96,68 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
  * Architected system timer support.
  */
 
+static __always_inline
+void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
+			  struct clock_event_device *clk)
+{
+	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
+		struct arch_timer *timer = to_arch_timer(clk);
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			writel_relaxed(val, timer->base + CNTP_CTL);
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			writel_relaxed(val, timer->base + CNTP_TVAL);
+			break;
+		}
+	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
+		struct arch_timer *timer = to_arch_timer(clk);
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			writel_relaxed(val, timer->base + CNTV_CTL);
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			writel_relaxed(val, timer->base + CNTV_TVAL);
+			break;
+		}
+	} else {
+		arch_timer_reg_write_cp15(access, reg, val);
+	}
+}
+
+static __always_inline
+u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
+			struct clock_event_device *clk)
+{
+	u32 val;
+
+	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
+		struct arch_timer *timer = to_arch_timer(clk);
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			val = readl_relaxed(timer->base + CNTP_CTL);
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			val = readl_relaxed(timer->base + CNTP_TVAL);
+			break;
+		}
+	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
+		struct arch_timer *timer = to_arch_timer(clk);
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			val = readl_relaxed(timer->base + CNTV_CTL);
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			val = readl_relaxed(timer->base + CNTV_TVAL);
+			break;
+		}
+	} else {
+		val = arch_timer_reg_read_cp15(access, reg);
+	}
+
+	return val;
+}
+
 #ifdef CONFIG_FSL_ERRATUM_A008585
 /*
  * The number of retries is an arbitrary value well beyond the highest number
@@ -294,68 +356,6 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 #define arch_timer_check_ool_workaround(t,a)		do { } while(0)
 #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
 
-static __always_inline
-void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
-			  struct clock_event_device *clk)
-{
-	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			writel_relaxed(val, timer->base + CNTP_CTL);
-			break;
-		case ARCH_TIMER_REG_TVAL:
-			writel_relaxed(val, timer->base + CNTP_TVAL);
-			break;
-		}
-	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			writel_relaxed(val, timer->base + CNTV_CTL);
-			break;
-		case ARCH_TIMER_REG_TVAL:
-			writel_relaxed(val, timer->base + CNTV_TVAL);
-			break;
-		}
-	} else {
-		arch_timer_reg_write_cp15(access, reg, val);
-	}
-}
-
-static __always_inline
-u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
-			struct clock_event_device *clk)
-{
-	u32 val;
-
-	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			val = readl_relaxed(timer->base + CNTP_CTL);
-			break;
-		case ARCH_TIMER_REG_TVAL:
-			val = readl_relaxed(timer->base + CNTP_TVAL);
-			break;
-		}
-	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			val = readl_relaxed(timer->base + CNTV_CTL);
-			break;
-		case ARCH_TIMER_REG_TVAL:
-			val = readl_relaxed(timer->base + CNTV_TVAL);
-			break;
-		}
-	} else {
-		val = arch_timer_reg_read_cp15(access, reg);
-	}
-
-	return val;
-}
-
 static __always_inline irqreturn_t timer_handler(const int access,
 					struct clock_event_device *evt)
 {
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1606684 — Re: [PATCH v2 09/18] arm64: arch_timer: Move arch_timer_reg_read/write around

On Mon, Mar 20, 2017 at 05:48:20PM +0000, Marc Zyngier wrote:
> As we're about to move things around, let's start with the low
> level read/write functions. This allows us to use these functions
> in the errata handling code without having to use forward declaration
> of static functions.
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1604880 — [PATCH v2 07/18] arm64: arch_timer: Add erratum handler for globally defined capability

Should we ever have a workaround for an erratum that is detected using
a capability (and affecting the whole system), it'd be nice to have
a way to probe them directly.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 5cd964e90d11..48bd730d568f 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -39,6 +39,7 @@ extern struct static_key_false arch_timer_read_ool_enabled;
 
 enum arch_timer_erratum_match_type {
 	ate_match_dt,
+	ate_match_global_cap_id,
 };
 
 struct arch_timer_erratum_workaround {
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 6a0f0e161a0f..a0b1108a4a24 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -214,6 +214,13 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround *wa,
 	return of_property_read_bool(np, wa->id);
 }
 
+static
+bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
+					 const void *arg)
+{
+	return cpus_have_cap((uintptr_t)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,
@@ -252,6 +259,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 	case ate_match_dt:
 		match_fn = arch_timer_check_dt_erratum;
 		break;
+	case ate_match_global_cap_id:
+		match_fn = arch_timer_check_global_cap_erratum;
+		break;
 	}
 
 	wa = arch_timer_iterate_errata(type, match_fn, arg);
@@ -1029,6 +1039,7 @@ static int __init arch_timer_of_init(struct device_node *np)
 
 	/* Check for globally applicable workarounds */
 	arch_timer_check_ool_workaround(ate_match_dt, np);
+	arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
 
 	/*
 	 * If we cannot rely on firmware initializing the timer registers then
@@ -1185,6 +1196,9 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 	/* Always-on capability */
 	arch_timer_c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
 
+	/* Check for globally applicable workarounds */
+	arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
+
 	arch_timer_init();
 	return 0;
 }
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1605016 — [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

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 <hanjun.guo@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 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;
+	}
+
+	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

[toc] | [prev] | [next] | [standalone]

#1606674 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

On Mon, Mar 20, 2017 at 05:48:17PM +0000, 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 <hanjun.guo@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  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;

Putting the match_fn instead will be much more simpler and the code won't
have to deal with ate_match_type, no ?

[ ... ]

> +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;
> +

Why is this check necessary ?

[ ... ]



-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1606676 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

On 22/03/17 15:41, Daniel Lezcano wrote:
> On Mon, Mar 20, 2017 at 05:48:17PM +0000, 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 <hanjun.guo@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  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;
> 
> Putting the match_fn instead will be much more simpler and the code won't
> have to deal with ate_match_type, no ?

I'm not sure about the "much simpler" aspect. Each function is not
necessarily standalone (see patches 8 and 13 for example, dealing with
CPU-local defects). We

> 
> [ ... ]
> 
>> +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;
>> +
> 
> Why is this check necessary ?

We don't allow cumulative workarounds at this stage. This restriction
gets lifted (to some extent) later in the series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1606681 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

[Sorry, sent too quickly]

On 22/03/17 15:41, Daniel Lezcano wrote:
> On Mon, Mar 20, 2017 at 05:48:17PM +0000, 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 <hanjun.guo@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  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;
> 
> Putting the match_fn instead will be much more simpler and the code won't
> have to deal with ate_match_type, no ?

I'm not sure about the "much simpler" aspect. Each function is not
necessarily standalone (see patches 8 and 13 for example, dealing with
CPU-local defects).

Also, given that we have two architectures to cater for, as well as two
firmware interfaces, it makes more sense (at least to me) to have
something that doesn't require to define a bunch of empty stubs (we
already have too many of them) depending on arm vs arm64, DT vs ACPI,
errata handling enabled vs disabled.

We're sidestepping this at the moment because it all lives under one
single config option that cannot be enabled from 32bit, but I hope to
change that.

> 
> [ ... ]
> 
>> +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;
>> +
> 
> Why is this check necessary ?

We don't allow cumulative workarounds at this stage. This restriction
gets lifted (to some extent) later in the series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1606762 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

On Wed, Mar 22, 2017 at 03:59:21PM +0000, Marc Zyngier wrote:
> [Sorry, sent too quickly]
> 

[ ... ]

> >>  struct arch_timer_erratum_workaround {
> >> -	const char *id;		/* Indicate the Erratum ID */
> >> +	enum arch_timer_erratum_match_type match_type;
> > 
> > Putting the match_fn instead will be much more simpler and the code won't
> > have to deal with ate_match_type, no ?
> 
> I'm not sure about the "much simpler" aspect. Each function is not
> necessarily standalone (see patches 8 and 13 for example, dealing with
> CPU-local defects).

Why not write always errata on a per cpu basis ? So there is no need to go
through global/local (at the timer level).

You have been probably looking at this much longer than me and perhaps I'm
missing something. However, I think we can find a way to simplify the approach. 

Give me one day to see if I'm right.

> Also, given that we have two architectures to cater for, as well as two
> firmware interfaces, it makes more sense (at least to me) to have
> something that doesn't require to define a bunch of empty stubs (we
> already have too many of them) depending on arm vs arm64, DT vs ACPI,
> errata handling enabled vs disabled.

That is a fair point.
 
> We're sidestepping this at the moment because it all lives under one
> single config option that cannot be enabled from 32bit, but I hope to
> change that.

Ok, that sounds good.

Thanks for proposing something to deal elegantly with the errata.

  -- Daniel


> > [ ... ]
> > 
> >> +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;
> >> +
> > 
> > Why is this check necessary ?
> 
> We don't allow cumulative workarounds at this stage. This restriction
> gets lifted (to some extent) later in the series.
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1607746 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

On Wed, Mar 22, 2017 at 03:59:21PM +0000, Marc Zyngier wrote:
> [Sorry, sent too quickly]
> 
> On 22/03/17 15:41, Daniel Lezcano wrote:
> > On Mon, Mar 20, 2017 at 05:48:17PM +0000, 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 <hanjun.guo@linaro.org>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  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;
> > 
> > Putting the match_fn instead will be much more simpler and the code won't
> > have to deal with ate_match_type, no ?
> 
> I'm not sure about the "much simpler" aspect. Each function is not
> necessarily standalone (see patches 8 and 13 for example, dealing with
> CPU-local defects).

Hi Marc,

I have been through the driver after applying the patchset. Again thanks for
taking care of this. It is not a simple issue to solve, so here is my minor
contribution.

The resulting code sounds like over-engineered because the errata check and its
workaround are done at the same place/moment, that forces to deal with an array
with element from different origin.

I understand you wanted to create a single array to handle the errata
information from the DT, ACPI and CAPS. But IMHO, it does not fit well.

I would suggest to create 3 arrays: ACPI, DT and CAPS.

Those arrays contains the errata id *and* an unique private id.

At boot time, you go through the corresponding array and fill a list of
detected errata with the private id.

On the other side, an array with the private id and its workaround makes the
assocation. The private id is the contract between the errata and the workaround.

So the errata handling will occur in two steps:
 1. Boot => errata detection
 2. CPU up => workaround put in place

With this approach, you can write everything on a per cpu basis, getting rid of
'global' / 'local'.

What is this different from your approach ?

 - no match_id
 - clear separation of errata and workaround
 - Simpler code
 - clear the scene for a more generic errata framework

That said, now it would make sense to create a generic errata framework to be
filled by the different arch at boot time and retrieve from the different
subsystem in an agnostic way. Well, may be that is a long term suggestion.

What do you think ?

[toc] | [prev] | [next] | [standalone]

#1608410 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

Hi Daniel,

On 23/03/17 17:30, Daniel Lezcano wrote:
> On Wed, Mar 22, 2017 at 03:59:21PM +0000, Marc Zyngier wrote:
>> [Sorry, sent too quickly]
>>
>> On 22/03/17 15:41, Daniel Lezcano wrote:
>>> On Mon, Mar 20, 2017 at 05:48:17PM +0000, 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 <hanjun.guo@linaro.org>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  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;
>>>
>>> Putting the match_fn instead will be much more simpler and the code won't
>>> have to deal with ate_match_type, no ?
>>
>> I'm not sure about the "much simpler" aspect. Each function is not
>> necessarily standalone (see patches 8 and 13 for example, dealing with
>> CPU-local defects).
> 
> Hi Marc,
> 
> I have been through the driver after applying the patchset. Again thanks for
> taking care of this. It is not a simple issue to solve, so here is my minor
> contribution.
> 
> The resulting code sounds like over-engineered because the errata check and its
> workaround are done at the same place/moment, that forces to deal with an array
> with element from different origin.
> 
> I understand you wanted to create a single array to handle the errata
> information from the DT, ACPI and CAPS. But IMHO, it does not fit well.
> 
> I would suggest to create 3 arrays: ACPI, DT and CAPS.
> 
> Those arrays contains the errata id *and* an unique private id.
> 
> At boot time, you go through the corresponding array and fill a list of
> detected errata with the private id.
> 
> On the other side, an array with the private id and its workaround makes the
> assocation. The private id is the contract between the errata and the workaround.
> 
> So the errata handling will occur in two steps:
>  1. Boot => errata detection
>  2. CPU up => workaround put in place
> 
> With this approach, you can write everything on a per cpu basis, getting rid of
> 'global' / 'local'.
> 
> What is this different from your approach ?
> 
>  - no match_id
>  - clear separation of errata and workaround
>  - Simpler code
>  - clear the scene for a more generic errata framework
> 
> That said, now it would make sense to create a generic errata framework to be
> filled by the different arch at boot time and retrieve from the different
> subsystem in an agnostic way. Well, may be that is a long term suggestion.
> 
> What do you think ?

I don't think this buys us anything at all. Separating detection and
enablement is not always feasible. In your example above, you assume
that all errata are detectable at boot time. Consider that with CPU
hotplug, we can bring up a new core at any time, possibly with an
erratum that you haven't detected yet.

And even then, what do we get: we trade a simple match ID for a list we
build at runtime, another private ID, and additional code to perform
that match. The gain is not obvious to me...

What would such a generic errata framework look like? A table containing
match functions returning a boolean, used to decide whether you need to
call yet another function with a bunch of arbitrary parameters.

In my experience, such a framework will be either an empty shell
(because you need to keep it as generic as possible), or will be riddled
with data structures ending up being the union of all the possible cases
you've encountered in the kernel. Not a pretty sight.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1609541 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

On Fri, Mar 24, 2017 at 01:51:47PM +0000, Marc Zyngier wrote:

[ ... ]

> > Hi Marc,
> > 
> > I have been through the driver after applying the patchset. Again thanks for
> > taking care of this. It is not a simple issue to solve, so here is my minor
> > contribution.
> > 
> > The resulting code sounds like over-engineered because the errata check and its
> > workaround are done at the same place/moment, that forces to deal with an array
> > with element from different origin.
> > 
> > I understand you wanted to create a single array to handle the errata
> > information from the DT, ACPI and CAPS. But IMHO, it does not fit well.
> > 
> > I would suggest to create 3 arrays: ACPI, DT and CAPS.
> > 
> > Those arrays contains the errata id *and* an unique private id.
> > 
> > At boot time, you go through the corresponding array and fill a list of
> > detected errata with the private id.
> > 
> > On the other side, an array with the private id and its workaround makes the
> > assocation. The private id is the contract between the errata and the workaround.
> > 
> > So the errata handling will occur in two steps:
> >  1. Boot => errata detection
> >  2. CPU up => workaround put in place
> > 
> > With this approach, you can write everything on a per cpu basis, getting rid of
> > 'global' / 'local'.
> > 
> > What is this different from your approach ?
> > 
> >  - no match_id
> >  - clear separation of errata and workaround
> >  - Simpler code
> >  - clear the scene for a more generic errata framework
> > 
> > That said, now it would make sense to create a generic errata framework to be
> > filled by the different arch at boot time and retrieve from the different
> > subsystem in an agnostic way. Well, may be that is a long term suggestion.
> > 
> > What do you think ?
> 
> I don't think this buys us anything at all. Separating detection and
> enablement is not always feasible. In your example above, you assume
> that all errata are detectable at boot time. Consider that with CPU
> hotplug, we can bring up a new core at any time, possibly with an
> erratum that you haven't detected yet.

I guess it has to pass through an init function before being powered on.
 
> And even then, what do we get: we trade a simple match ID for a list we
> build at runtime, another private ID, and additional code to perform
> that match. The gain is not obvious to me...
>
> What would such a generic errata framework look like? A table containing
> match functions returning a boolean, used to decide whether you need to
> call yet another function with a bunch of arbitrary parameters.
> 
> In my experience, such a framework will be either an empty shell
> (because you need to keep it as generic as possible), or will be riddled
> with data structures ending up being the union of all the possible cases
> you've encountered in the kernel. Not a pretty sight.

I disagree but I can understand you don't see the point to write a generic
framework while the patchset does the job.

Let's refocus on the patchset itself.

Can you do the change to have a percpu basis errata in order to remove
local/global ?

Something as below:

 
 static
-bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
-					 const void *arg)
+bool arch_timer_check_cap_erratum(const struct arch_timer_erratum_workaround *wa,
+				  const void *arg)
 {
-	return cpus_have_cap((uintptr_t)wa->id);
+	return cpus_have_cap((uintptr_t)wa->id) | this_cpu_has_cap((uintptr_t)wa->id);
 }
 
 static
-bool arch_timer_check_local_cap_erratum(const struct arch_timer_erratum_workaround *wa,
-					const void *arg)
-{
-	return this_cpu_has_cap((uintptr_t)wa->id);
-}
-
-
-static
 bool arch_timer_check_acpi_oem_erratum(const struct arch_timer_erratum_workaround *wa,
 				       const void *arg)
 {
@@ -458,17 +450,9 @@ arch_timer_iterate_errata(enum arch_timer_erratum_match_type type,
 }
 
 static
-void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa,
-				  bool local)
+void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa)
 {
-	int i;
-
-	if (local) {
-		__this_cpu_write(timer_unstable_counter_workaround, wa);
-	} else {
-		for_each_possible_cpu(i)
-			per_cpu(timer_unstable_counter_workaround, i) = wa;
-	}
+	__this_cpu_write(timer_unstable_counter_workaround, wa);
 
 	static_branch_enable(&arch_timer_read_ool_enabled);
 
@@ -489,18 +473,16 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 {
 	const struct arch_timer_erratum_workaround *wa;
 	ate_match_fn_t match_fn = NULL;
-	bool local = false;
 
 	switch (type) {
 	case ate_match_dt:
 		match_fn = arch_timer_check_dt_erratum;
 		break;
 	case ate_match_global_cap_id:
-		match_fn = arch_timer_check_global_cap_erratum;
+		match_fn = arch_timer_check_cap_erratum;
 		break;
 	case ate_match_local_cap_id:
-		match_fn = arch_timer_check_local_cap_erratum;
-		local = true;
+		match_fn = arch_timer_check_cap_erratum;
 		break;
 	case ate_match_acpi_oem_info:
 		match_fn = arch_timer_check_acpi_oem_erratum;
@@ -522,9 +504,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 			return;
 	}
 
-	arch_timer_enable_workaround(wa, local);
-	pr_info("Enabling %s workaround for %s\n",
-		local ? "local" : "global", wa->desc);
+	arch_timer_enable_workaround(wa);
+	pr_info("Enabling %s workaround for cpu %d\n",
+		wa->desc, smp_processor_id());
 }
 
 #define erratum_handler(fn, r, ...)					\


-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1608730 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

On Mon, Mar 20, 2017 at 11:48 AM, Marc Zyngier <marc.zyngier@arm.com> 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 <hanjun.guo@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  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
>

[toc] | [prev] | [next] | [standalone]

#1608802 — Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

Hi Dann,

On 24/03/17 17:48, dann frazier wrote:
> On Mon, Mar 20, 2017 at 11:48 AM, Marc Zyngier <marc.zyngier@arm.com> 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 <hanjun.guo@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  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.

Sure, I can add that in the next version of this series. I would have
hoped that GCC would warn you if you don't handle all the values of an
enum, but it doesn't hurt to be cautious.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web