diff mbox

[v6,1/4] arm64: arch_timer: Add device tree binding for A-008585 erratum

Message ID 1474533318-7796-1-git-send-email-oss@buserror.net
State Not Applicable, archived
Headers show

Commit Message

Crystal Wood Sept. 22, 2016, 8:35 a.m. UTC 
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.

Signed-off-by: Scott Wood <oss@buserror.net>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Marc Zyngier Sept. 23, 2016, 1:17 p.m. UTC | #1 
On 22/09/16 09:35, Scott Wood wrote:
> Erratum A-008585 says that the ARM generic timer counter "has the
> potential to contain an erroneous value for a small number of core
> clock cycles every time the timer value changes".  Accesses to TVAL
> (both read and write) are also affected due to the implicit counter
> read.  Accesses to CVAL are not affected.
> 
> The workaround is to reread TVAL and count registers until successive
> reads return the same value.  Writes to TVAL are replaced with an
> equivalent write to CVAL.
> 
> The workaround is to reread TVAL and count registers until successive reads
> return the same value, and when writing TVAL to retry until counter
> reads before and after the write return the same value.
> 
> The workaround is enabled if the fsl,erratum-a008585 property is found in
> the timer node in the device tree.  This can be overridden with the
> clocksource.arm_arch_timer.fsl-a008585 boot parameter, which allows KVM
> users to enable the workaround until a mechanism is implemented to
> automatically communicate this information.
> 
> This erratum can be found on LS1043A and LS2080A.
> 
> Signed-off-by: Scott Wood <oss@buserror.net>
> ---
> v6:
> - Addressed feedback from Mark Rutland
> 
> v5:
> - Export arch_timer_read_ool_enabled so that get_cycles() can be called
>   from modules.
> 
> v4:
> - Undef ARCH_TIMER_REG_READ after use
> 
> v3:
> - Used cval rather than a loop for the write side of the erratum
> - Added a Kconfig control
> - Moved the device tree binding into its own patch
> - Added erratum to silicon-errata.txt
> - Changed function names to contain the erratum name
> - Factored out the setting of erratum versions of set_next_event
>   to improve readability
> - Added a comment clarifying that the timeout is arbitrary
> 
> v2:
> Significant rework based on feedback, including using static_key,
> disabling VDSO counter access rather than adding the workaround to the
> VDSO, and uninlining the loops.
> 
> Dropped the separate property for indicating that writes to TVAL are
> affected, as I believe that's just a side effect of the implicit
> counter read being corrupted, and thus a chip that is affected by one
> will always be affected by the other.
> 
> Dropped the arm32 portion as it seems there was confusion about whether
> LS1021A is affected.  Currently I am being told that it is not
> affected.
> 
> I considered writing to CVAL rather than looping on TVAL writes, but
> that would still have required separate set_next_event() code for the
> erratum, and adding CVAL to the enum would have required a bunch of
> extra handlers in switch statements (even where unused, due to compiler
> warnings about unhandled enum values) including in an arm32 header.  It
> seemed better to avoid the arm32 interaction and new untested
> accessors.
> 
> Signed-off-by: Scott Wood <oss@buserror.net>
> ---
>  Documentation/arm64/silicon-errata.txt |   2 +
>  Documentation/kernel-parameters.txt    |   9 +++
>  arch/arm64/include/asm/arch_timer.h    |  47 ++++++++++++++-
>  drivers/clocksource/Kconfig            |  10 ++++
>  drivers/clocksource/arm_arch_timer.c   | 104 +++++++++++++++++++++++++++++++++
>  5 files changed, 169 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 4da60b4..041e3a9 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -60,3 +60,5 @@ stable kernels.
>  | Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154    |
>  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456    |
>  | Cavium         | ThunderX SMMUv2 | #27704          | N/A		       |
> +|                |                 |                 |                         |
> +| Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585     |
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 46c030a..fb4de4d 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -698,6 +698,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			loops can be debugged more effectively on production
>  			systems.
>  
> +	clocksource.arm_arch_timer.fsl-a008585=
> +			[ARM64]
> +			Format: <bool>
> +			Enable/disable the workaround of Freescale/NXP
> +			erratum A-008585.  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 7ff386c..cddd5b7 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -24,10 +24,51 @@
>  
>  #include <linux/bug.h>
>  #include <linux/init.h>
> +#include <linux/jump_label.h>
>  #include <linux/types.h>
>  
>  #include <clocksource/arm_arch_timer.h>
>  
> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585)
> +extern struct static_key_false arch_timer_read_ool_enabled;
> +#define needs_fsl_a008585_workaround() \
> +	static_branch_unlikely(&arch_timer_read_ool_enabled)
> +#else
> +#define needs_fsl_a008585_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);
> +
> +/*
> + * The number of retries is an arbitrary value well beyond the highest number
> + * of iterations the loop has been observed to take.
> + */
> +#define __fsl_a008585_read_reg(reg) ({			\
> +	u64 _old, _new;					\
> +	int _retries = 200;				\
> +							\
> +	do {						\
> +		_old = read_sysreg(reg);		\
> +		_new = read_sysreg(reg);		\
> +		_retries--;				\
> +	} while (unlikely(_old != _new) && _retries);	\
> +							\
> +	WARN_ON_ONCE(!_retries);			\
> +	_new;						\
> +})
> +
> +#define arch_timer_unstable_reg_read(reg) 		\

I think this name is the only thing I don't like about this patch,
because it is only unstable if the workaround is on. This is a minor
thing and it can be addressed when/after merging it. No need to respin
it on this account.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

The usual question is "Who takes it?". I'm quite keen on it, as my
LS2085 is otherwise completely unusable.

Thanks,

	M.
Russell King (Oracle) Sept. 23, 2016, 2:32 p.m. UTC | #2 
It helps to add the appropriate people to your email if you want to get
a change into the kernel.  Will has had to point this message out to me.

On Thu, Sep 22, 2016 at 03:35:18AM -0500, Scott Wood wrote:
> Instead of comparing the name to a magic string, use archdata to
> explicitly communicate whether the arch timer is suitable for
> direct vdso access.
> 
> Signed-off-by: Scott Wood <oss@buserror.net>
> Acked-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/Kconfig                     |  1 +
>  arch/arm/include/asm/clocksource.h   |  8 ++++++++
>  arch/arm/kernel/vdso.c               |  2 +-
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/clocksource.h |  8 ++++++++
>  arch/arm64/kernel/vdso.c             |  2 +-
>  drivers/clocksource/arm_arch_timer.c | 11 +++--------
>  7 files changed, 23 insertions(+), 10 deletions(-)
>  create mode 100644 arch/arm/include/asm/clocksource.h
>  create mode 100644 arch/arm64/include/asm/clocksource.h
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index a9c4e48..b2113c2 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1,6 +1,7 @@
>  config ARM
>  	bool
>  	default y
> +	select ARCH_CLOCKSOURCE_DATA
>  	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>  	select ARCH_HAS_DEVMEM_IS_ALLOWED
>  	select ARCH_HAS_ELF_RANDOMIZE
> diff --git a/arch/arm/include/asm/clocksource.h b/arch/arm/include/asm/clocksource.h
> new file mode 100644
> index 0000000..0b350a7
> --- /dev/null
> +++ b/arch/arm/include/asm/clocksource.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASM_CLOCKSOURCE_H
> +#define _ASM_CLOCKSOURCE_H
> +
> +struct arch_clocksource_data {
> +	bool vdso_direct;	/* Usable for direct VDSO access? */
> +};
> +
> +#endif
> diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
> index 994e971..a0affd1 100644
> --- a/arch/arm/kernel/vdso.c
> +++ b/arch/arm/kernel/vdso.c
> @@ -270,7 +270,7 @@ static bool tk_is_cntvct(const struct timekeeper *tk)
>  	if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>  		return false;
>  
> -	if (strcmp(tk->tkr_mono.clock->name, "arch_sys_counter") != 0)
> +	if (!tk->tkr_mono.clock->archdata.vdso_direct)
>  		return false;
>  
>  	return true;

For the ARM bits: 

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
Marc Zyngier Sept. 23, 2016, 2:41 p.m. UTC | #3 
On 22/09/16 09:35, Scott Wood wrote:
> Instead of comparing the name to a magic string, use archdata to
> explicitly communicate whether the arch timer is suitable for
> direct vdso access.
> 
> Signed-off-by: Scott Wood <oss@buserror.net>
> Acked-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/Kconfig                     |  1 +
>  arch/arm/include/asm/clocksource.h   |  8 ++++++++
>  arch/arm/kernel/vdso.c               |  2 +-
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/clocksource.h |  8 ++++++++
>  arch/arm64/kernel/vdso.c             |  2 +-
>  drivers/clocksource/arm_arch_timer.c | 11 +++--------
>  7 files changed, 23 insertions(+), 10 deletions(-)
>  create mode 100644 arch/arm/include/asm/clocksource.h
>  create mode 100644 arch/arm64/include/asm/clocksource.h

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
Marc Zyngier Sept. 23, 2016, 2:41 p.m. UTC | #4 
On 22/09/16 09:35, Scott Wood 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.
> 
> Signed-off-by: Scott Wood <oss@buserror.net>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++
>  1 file changed, 6 insertions(+)

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
Marc Zyngier Sept. 23, 2016, 2:44 p.m. UTC | #5 
With a commit message along the lines of:

Both the LS1043A and LS2080A platforms are affected by the Freescale
A008585 erratum. Advertise it in their respective device trees.

On 22/09/16 09:35, Scott Wood wrote:
> Signed-off-by: Scott Wood <oss@buserror.net>
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 1 +
>  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> index e669fbd..952531d 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -123,6 +123,7 @@
>  			     <1 14 0x1>, /* Physical Non-Secure PPI */
>  			     <1 11 0x1>, /* Virtual PPI */
>  			     <1 10 0x1>; /* Hypervisor PPI */
> +		fsl,erratum-a008585;
>  	};
>  
>  	pmu {
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> index 21023a3..9d3ac19 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> @@ -195,6 +195,7 @@
>  			     <1 14 0x8>, /* Physical Non-Secure PPI, active-low */
>  			     <1 11 0x8>, /* Virtual PPI, active-low */
>  			     <1 10 0x8>; /* Hypervisor PPI, active-low */
> +		fsl,erratum-a008585;
>  	};
>  
>  	pmu {
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
Will Deacon Sept. 23, 2016, 4:27 p.m. UTC | #6 
On Thu, Sep 22, 2016 at 03:35:16AM -0500, Scott Wood wrote:
> Signed-off-by: Scott Wood <oss@buserror.net>
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 1 +
>  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 1 +
>  2 files changed, 2 insertions(+)

This patch conflicts with mainline, but I've queued the other three
patches in the series in the arm64 tree.

Please send the updated .dts changes via arm-soc.

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo Sept. 24, 2016, 2:08 p.m. UTC | #7 
On Fri, Sep 23, 2016 at 05:27:56PM +0100, Will Deacon wrote:
> On Thu, Sep 22, 2016 at 03:35:16AM -0500, Scott Wood wrote:
> > Signed-off-by: Scott Wood <oss@buserror.net>
> > ---
> >  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 1 +
> >  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 1 +
> >  2 files changed, 2 insertions(+)
> 
> This patch conflicts with mainline,

Thanks for the heads-up.

> but I've queued the other three
> patches in the series in the arm64 tree.
> 
> Please send the updated .dts changes via arm-soc.

I will send this to arm-soc when v4.9-rc1 comes out with driver changes
in place.

Shawn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo Sept. 24, 2016, 2:11 p.m. UTC | #8 
On Fri, Sep 23, 2016 at 03:44:11PM +0100, Marc Zyngier wrote:
> With a commit message along the lines of:
> 
> Both the LS1043A and LS2080A platforms are affected by the Freescale
> A008585 erratum. Advertise it in their respective device trees.

<snip>

> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks, Marc.  I queued up the patch with the suggested commit log
added.

Shawn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
index e774128..ef5fbe9 100644
--- a/Documentation/devicetree/bindings/arm/arch_timer.txt
+++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
@@ -25,6 +25,12 @@  to deliver its interrupts via SPIs.
 - always-on : a boolean property. If present, the timer is powered through an
   always-on power domain, therefore it never loses context.
 
+- fsl,erratum-a008585 : A boolean property. Indicates the presence of
+  QorIQ erratum A-008585, which says that reading the counter is
+  unreliable unless the same value is returned by back-to-back reads.
+  This also affects writes to the tval register, due to the implicit
+  counter read.
+
 ** Optional properties:
 
 - arm,cpu-registers-not-fw-configured : Firmware does not initialize