Re: [PATCH v5 3/5] arm64: arch_timer: Work around QorIQ Erratum A-008585

On Sun, Sep 18, 2016 at 11:41:25PM -0500, Scott Wood wrote:

On Mon, 2016-09-12 at 12:36 +0100, Mark Rutland wrote:

quoted

On Fri, Sep 09, 2016 at 08:03:31PM -0500, Scott Wood wrote:

quoted

+static __always_inline void arch_timer_cval_write_cp15(int access, u64
val)
+{
+	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.

That's annoying. Never mind, then.

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).

Ok. I think that's preferable given we have no other users.

quoted

quoted

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

	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.

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().

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 2016-09-19 at 17:52 +0100, Mark Rutland wrote:

quoted

quoted

quoted

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

	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.

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.

-Scott

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