arm64: arch_timer: Add device tree binding for A-008585 erratum

Erratum A-008585 says that the ARM generic timer counter "has the potential to contain an erroneous value for a small number of core clock cycles every time the timer value changes". Accesses to TVAL (both read and write) are also affected due to the implicit counter read. Accesses to CVAL are not affected.

The workaround is to reread TVAL and count registers until successive reads return the same value, and when writing TVAL to retry until counter reads before and after the write return the same value.

arch/arm64/include/asm/arch_timer.h +4 -0

+static inline u64 _##func(void) \
+{ \
+	if (IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) && \
+	    static_branch_unlikely(&arch_timer_read_ool_enabled)) \

drivers/clocksource/arm_arch_timer.c +14 -0

+		cval_old = func();
+		cval_new = func();
+		timeout--;
+	} while (unlikely(cval_old != cval_new) && timeout);
+
+	WARN_ON_ONCE(!timeout);
+	return cval_new;
+}
+
+u64 arch_counter_get_cntvct_ool(void)
+{
+	return fsl_a008585_reread_counter(__arch_counter_get_cntvct);
+}
+EXPORT_SYMBOL(arch_counter_get_cntvct_ool);

drivers/clocksource/arm_arch_timer.c +32 -0

+#ifdef CONFIG_FSL_ERRATUM_A008585
+static __always_inline void fsl_a008585_set_next_event(const int access,
+		unsigned long evt, struct clock_event_device *clk)
+{
+	unsigned long ctrl;
+	u64 cval = evt + arch_counter_get_cntvct();
+
+	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
+	ctrl |= ARCH_TIMER_CTRL_ENABLE;
+	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
+	arch_timer_cval_write_cp15(access, cval);
+	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
+}
+
+static int fsl_a008585_set_next_event_virt(unsigned long evt,
+					   struct clock_event_device *clk)
+{
+	fsl_a008585_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
+	return 0;
+}
+
+static int fsl_a008585_set_next_event_phys(unsigned long evt,
+					   struct clock_event_device *clk)
+{
+	fsl_a008585_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
+	return 0;
+}
+#endif /* CONFIG_FSL_ERRATUM_A008585 */
+
 static int arch_timer_set_next_event_virt(unsigned long evt,
 					  struct clock_event_device *clk)
 {

@@ -271,6 +346,19 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
 	return 0;
 }
 
+static void fsl_a008585_set_sne(struct clock_event_device *clk)
+{
+#ifdef CONFIG_FSL_ERRATUM_A008585

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Quoted 4 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Quoted 6 lines · Will Deacon

Quoted 4 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Quoted 3 lines

Well, we should figure out what tree this is going through first. There are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

Quoted 11 lines · Mark Rutland

Quoted 9 lines · Will Deacon

Quoted 4 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Quoted 3 lines

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

Now I see that I'd missed the arch/arm changes in patch 4, which lack a relevant ack.

Given that, I don't know what to suggest.

Quoted 19 lines · Mark Rutland

Quoted 17 lines · Mark Rutland

Quoted 7 lines · Will Deacon

Quoted 5 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Quoted 5 lines

Quoted 1 line

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There
are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

Quoted 4 lines

Now I see that I'd missed the arch/arm changes in patch 4, which lack a relevant ack.

Given that, I don't know what to suggest.

I wouldn't mind delaying patch 4 until it gets acked by RMK, as it doesn't impact the functionality.

Quoted 23 lines · Marc Zyngier

Quoted 22 lines · Mark Rutland

Quoted 9 lines · Mark Rutland

Quoted 7 lines · Will Deacon

Quoted 5 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There
are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

Now I see that I'd missed the arch/arm changes in patch 4, which lack a relevant ack.

Quoted 3 lines

Quoted 1 line

Given that, I don't know what to suggest.

I wouldn't mind delaying patch 4 until it gets acked by RMK, as it doesn't impact the functionality.

Mark asked me to move that patch before the workaround patch, to avoid ever having to add more code that messes with the name. ?Should I keep the order as is then?

Quoted 10 lines · Will Deacon

Quoted 5 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

The dts change could go through arm-soc in parallel, I guess. It doesn't look like arm-soc have been Cc'd for that, though.

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Will Deacon

Quoted 5 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

Quoted 2 lines

The dts change could go through arm-soc in parallel, I guess. It doesn't look like arm-soc have been Cc'd for that, though.

The arm-soc section of MAINTAINERS says to e-mail linux-arm-kernel, which I did. There doesn't appear to be a separate arm-soc mailing list, nor is there a request to CC Olof/Arnd. I did CC Shawn Guo who has been handling the device tree patches for these chips.

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Will Deacon

Quoted 5 lines · Mark Rutland

The changes in arm64's <asm/arch_timer.h> are going to conflict with some cleanup [1,2] that just landed in the arm64 for-next/core branch.

Could you please rebase atop of that?

Well, we should figure out what tree this is going through first. There are a mixture of arm, arm64, driver and dts changes here and not all of it is carrying the appropriate acks for me to queue it.

Given that mix, I had assumed that this would all go through the arm64 tree -- I see that Rob has already acked the binding, and I'm happy to give my ack for the driver once that's in shape.

Quoted 2 lines

The dts change could go through arm-soc in parallel, I guess. It doesn't look like arm-soc have been Cc'd for that, though.

The arm-soc section of MAINTAINERS says to e-mail linux-arm-kernel, which I did. ?There doesn't appear to be a separate arm-soc mailing list, nor is there a request to CC Olof/Arnd. ?I did CC Shawn Guo who has been handling the device tree patches for these chips.

arch/arm64/include/asm/arch_timer.h void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val) · line 95

Quoted 10 lines · Scott Wood

+static __always_inline void arch_timer_cval_write_cp15(int access, u64 val)

arch/arm64/include/asm/arch_timer.h +8 -0

+{
+	if (access == ARCH_TIMER_PHYS_ACCESS)
+		asm volatile("msr cntp_cval_el0, %0" : : "r" (val));
+	else if (access == ARCH_TIMER_VIRT_ACCESS)
+		asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
+
+	isb();
+}

Please add ARCH_TIMER_REG_CVAL to enum arch_timer_reg, and move these accesses into arch_timer_reg_write_cp15().

drivers/clocksource/arm_arch_timer.c early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg); · line 115

Quoted 4 lines

drivers/clocksource/arm_arch_timer.c +3 -0

+
+	do {
+		isb();

What's the ISB for?

The core should order accesses to the same counter, and any ISB required for ordering against other counters should already be present. So I don't follow what this is trying to achieve.

If this is necessary, please add a comment explaining what it is intended to ensure.

Quoted 8 lines

(quoted diff) +0 -0

@@ -271,6 +346,19 @@ static int
 arch_timer_set_next_event_phys_mem(unsigned long evt,
 ?	return 0;
 ?}

?

drivers/clocksource/arm_arch_timer.c +3 -0

+static void fsl_a008585_set_sne(struct clock_event_device *clk)
+{
+#ifdef CONFIG_FSL_ERRATUM_A008585

drivers/clocksource/arm_arch_timer.c static int arch_timer_set_next_event_phys_mem(unsigned long evt, · line 361

Quoted 10 lines

drivers/clocksource/arm_arch_timer.c +10 -0

+	if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
+		return;
+
+	if (arch_timer_uses_ppi == VIRT_PPI)
+		clk->set_next_event = fsl_a008585_set_next_event_virt;
+	else
+		clk->set_next_event = fsl_a008585_set_next_event_phys;
+#endif
+}
+

I'm not keen on the magic hook to reset the function pointers, and the additional phys/virt stubs seem pointless. Instead, can we fold this into the existing set_next_event? e.g. have that do:

	if (needs_fsl_a008585_workaround() {
		fsl_a008585_set_next_event(access, evt, clk);
		return;
	}

Quoted 13 lines · Mark Rutland

arch/arm64/include/asm/arch_timer.h void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val) · line 95

Quoted 10 lines · Scott Wood

+static __always_inline void arch_timer_cval_write_cp15(int access, u64 val)

arch/arm64/include/asm/arch_timer.h +8 -0

+{
+	if (access == ARCH_TIMER_PHYS_ACCESS)
+		asm volatile("msr cntp_cval_el0, %0" : : "r" (val));
+	else if (access == ARCH_TIMER_VIRT_ACCESS)
+		asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
+
+	isb();
+}

Please add ARCH_TIMER_REG_CVAL to enum arch_timer_reg, and move these accesses into arch_timer_reg_write_cp15().

Adding ARCH_TIMER_REG_CVAL to the enum means we get warnings from bunch of switch statements that don't actually need a CVAL implementation -- or else we have to add untested CVAL accessors for arm32 and mmio. ?The arm32 part would add another dependency on getting an ack from RMK, that can't be postponed as easily as the archdata/vdso patch.

Since this is specific to an erratum rather than general cval support, I can move the accesses into fsl_a008585_set_next_event (and convert to write_sysreg).

Quoted 17 lines

drivers/clocksource/arm_arch_timer.c static int arch_timer_set_next_event_phys_mem(unsigned long evt, · line 361

Quoted 13 lines

drivers/clocksource/arm_arch_timer.c +13 -0

+static void fsl_a008585_set_sne(struct clock_event_device *clk)
+{
+#ifdef CONFIG_FSL_ERRATUM_A008585
+	if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
+		return;
+
+	if (arch_timer_uses_ppi == VIRT_PPI)
+		clk->set_next_event = fsl_a008585_set_next_event_virt;
+	else
+		clk->set_next_event = fsl_a008585_set_next_event_phys;
+#endif
+}
+

I'm not keen on the magic hook to reset the function pointers, and the additional phys/virt stubs seem pointless. Instead, can we fold this into the existing set_next_event? e.g. have that do:

Quoted 4 lines

	if (needs_fsl_a008585_workaround() {
		fsl_a008585_set_next_event(access, evt, clk);
		return;
	}

OK. ?I had been trying to avoid messing with the standard set_next_event, but it doesn't matter as much now that static branches are being used. ?In that case we can avoid duplicating the ctrl code, and only replace the tval write.

Quoted 24 lines

Quoted 20 lines

Quoted 13 lines

drivers/clocksource/arm_arch_timer.c +8 -0

+static void fsl_a008585_set_sne(struct clock_event_device *clk)
+{
+#ifdef CONFIG_FSL_ERRATUM_A008585
+	if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
+		return;
+
+	if (arch_timer_uses_ppi == VIRT_PPI)
+		clk->set_next_event =

fsl_a008585_set_next_event_virt;

(quoted diff) +2 -0

+	else
+		clk->set_next_event =

fsl_a008585_set_next_event_phys;

drivers/clocksource/arm_arch_timer.c static int arch_timer_set_next_event_phys_mem(unsigned long evt, · line 361

Quoted 3 lines

drivers/clocksource/arm_arch_timer.c +3 -0

+#endif
+}
+

I'm not keen on the magic hook to reset the function pointers, and the additional phys/virt stubs seem pointless. Instead, can we fold this into the existing set_next_event? e.g. have that do:

Quoted 4 lines

	if (needs_fsl_a008585_workaround() {
		fsl_a008585_set_next_event(access, evt, clk);
		return;
	}

Quoted 6 lines

OK. ?I had been trying to avoid messing with the standard set_next_event, but
it doesn't matter as much now that static branches are being used. ?In that
case we can avoid duplicating the ctrl code, and only replace the tval write.

Reconsidering my suggestion, I realise this will also affect the MMIO timers, so that doesn't work.

So for the moment, I guess we have to keep fsl_a008585_set_next_event().

Quoted 30 lines · Mark Rutland

Quoted 30 lines

Quoted 20 lines

Quoted 13 lines

drivers/clocksource/arm_arch_timer.c +8 -0

+static void fsl_a008585_set_sne(struct clock_event_device *clk)
+{
+#ifdef CONFIG_FSL_ERRATUM_A008585
+	if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
+		return;
+
+	if (arch_timer_uses_ppi == VIRT_PPI)
+		clk->set_next_event =

fsl_a008585_set_next_event_virt;

(quoted diff) +2 -0

+	else
+		clk->set_next_event =

fsl_a008585_set_next_event_phys;

drivers/clocksource/arm_arch_timer.c static int arch_timer_set_next_event_phys_mem(unsigned long evt, · line 361

Quoted 3 lines

drivers/clocksource/arm_arch_timer.c +3 -0

+#endif
+}
+

I'm not keen on the magic hook to reset the function pointers, and the additional phys/virt stubs seem pointless. Instead, can we fold this into the existing set_next_event? e.g. have that do:

Quoted 4 lines

	if (needs_fsl_a008585_workaround() {
		fsl_a008585_set_next_event(access, evt, clk);
		return;
	}

OK. ?I had been trying to avoid messing with the standard set_next_event, but
it doesn't matter as much now that static branches are being used. ?In that
case we can avoid duplicating the ctrl code, and only replace the tval write.

Quoted 4 lines

Reconsidering my suggestion, I realise this will also affect the MMIO timers, so that doesn't work.

So for the moment, I guess we have to keep fsl_a008585_set_next_event().

What is the problem with MMIO timers? ?needs_fsl_a008585_workaround() should always be false for them.

Quoted 39 lines · Scott Wood

Quoted 37 lines · Mark Rutland

Quoted 37 lines

Quoted 26 lines

Quoted 18 lines

drivers/clocksource/arm_arch_timer.c +4 -0

+static void fsl_a008585_set_sne(struct clock_event_device *clk)
+{
+#ifdef CONFIG_FSL_ERRATUM_A008585
+	if

(!static_branch_unlikely(&arch_timer_read_ool_enabled))

drivers/clocksource/arm_arch_timer.c +4 -0

+		return;
+
+	if (arch_timer_uses_ppi == VIRT_PPI)
+		clk->set_next_event =

fsl_a008585_set_next_event_virt;

(quoted diff) +2 -0

+	else
+		clk->set_next_event =

fsl_a008585_set_next_event_phys;

drivers/clocksource/arm_arch_timer.c static int arch_timer_set_next_event_phys_mem(unsigned long evt, · line 361

Quoted 3 lines

drivers/clocksource/arm_arch_timer.c +3 -0

+#endif
+}
+

I'm not keen on the magic hook to reset the function pointers, and the
additional phys/virt stubs seem pointless. Instead, can we fold this into the existing set_next_event? e.g. have that do:

Quoted 4 lines

	if (needs_fsl_a008585_workaround() {
		fsl_a008585_set_next_event(access, evt, clk);
		return;
	}

OK. ?I had been trying to avoid messing with the standard set_next_event,
but
it doesn't matter as much now that static branches are being used. ?In that
case we can avoid duplicating the ctrl code, and only replace the tval write.

Quoted 7 lines

Quoted 4 lines

Reconsidering my suggestion, I realise this will also affect the MMIO timers, so that doesn't work.

So for the moment, I guess we have to keep fsl_a008585_set_next_event().

What is the problem with MMIO timers? ?needs_fsl_a008585_workaround() should
always be false for them.

As suggested, needs_fsl_a008585_workaround() takes no parameter, and set_next_event is called for both cp15/sysreg and MMIO timers. So it would either be true for all, or false for all.

If it's true for all, we'd end up calling fsl_a008585_set_next_event() for the MMIO timers too.

Quoted 11 lines · Mark Rutland

Quoted 7 lines

Quoted 4 lines

Reconsidering my suggestion, I realise this will also affect the MMIO timers, so that doesn't work.

So for the moment, I guess we have to keep fsl_a008585_set_next_event().

What is the problem with MMIO timers? ?needs_fsl_a008585_workaround() should
always be false for them.

As suggested, needs_fsl_a008585_workaround() takes no parameter, and set_next_event is called for both cp15/sysreg and MMIO timers. So it would either be true for all, or false for all.

Quoted 2 lines

If it's true for all, we'd end up calling fsl_a008585_set_next_event() for the MMIO timers too.

There should not be any MMIO timers on a system where fsl_a008585_set_next_event() returns true.

Quoted 18 lines · Scott Wood

Quoted 16 lines · Mark Rutland

Quoted 7 lines

Quoted 7 lines

Reconsidering my suggestion, I realise this will also affect the MMIO
timers, so that doesn't work.

Quoted 5 lines

Quoted 2 lines

So for the moment, I guess we have to keep
fsl_a008585_set_next_event().

What is the problem with MMIO timers? ?needs_fsl_a008585_workaround() should
always be false for them.

As suggested, needs_fsl_a008585_workaround() takes no parameter, and set_next_event is called for both cp15/sysreg and MMIO timers. So it would either be true for all, or false for all.

Quoted 4 lines

Quoted 2 lines

If it's true for all, we'd end up calling fsl_a008585_set_next_event() for the MMIO timers too.

There should not be any MMIO timers on a system where fsl_a008585_set_next_event() returns true.

I'm generally not keen on relying on that.

For reference, are no MMIO timers implemented at all, or are they simply not listed in the DT today?