On 13-03-28 05:57 AM, Johan Hovold wrote:

On Tue, Mar 26, 2013 at 05:09:59PM -0400, Douglas Gilbert wrote:

quoted

On 13-03-26 03:27 PM, Johan Hovold wrote:

quoted

On Fri, Mar 15, 2013 at 06:37:12PM +0100, Nicolas Ferre wrote:

quoted

On some revisions of AT91 SoCs, the RTC IMR register is not working.
Instead of elaborating a workaround for that specific SoC or IP version,
we simply use a software variable to store the Interrupt Mask Register and
modify it for each enabling/disabling of an interrupt. The overhead of this
is negligible anyway.

The patch does not add any memory barriers or register read-backs when
manipulating the interrupt-mask variable. This could possibly lead to
spurious interrupts both when enabling and disabling the various
RTC-interrupts due to write reordering and bus latencies.

Has this been considered? And is this reason enough for a more targeted
work-around so that the SOCs with functional RTC_IMR are not affected?

The SoCs in question use a single embedded ARM926EJ-S and
according to the Atmel documentation, that CPU's instruction
set contains no barrier (or related) instructions.

The ARM926EJ-S actually does have a Drain Write Buffer instruction but
it's not used by the ARM barrier-implementation unless
CONFIG_ARM_DMA_MEM_BUFFERABLE or CONFIG_SMP is set.

The ARM926EJ-S is ARMv5 so CONFIG_ARM_DMA_MEM_BUFFERABLE is not
available. SMP is not an option for arm/mach-at91.

However, wmb() always implies a compiler barrier which is what is needed
in this case.

Even if wmb() did anything, would it make this case "safe"?

quoted

In the arch/arm/mach-at91 sub-tree of the kernel source
I can find no use of the wmb() call. Also checked all drivers
in the kernel containing "at91" and none called wmb().

I/O-operations are normally not reordered, but this patch is faking a
hardware register and thus extra care needs to be taken.

To repeat:

quoted

@@ -198,9 +203,12 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)

       if (enabled) {
               at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
+             at91_rtc_imr |= AT91_RTC_ALARM;

Here a barrier is needed to prevent the compiler from reordering the two
writes (i.e., mask update and interrupt enable).

Isn't either order potentially unsafe? So even if the compiler
did foolishly re-order them, the sequence is still unsafe when
a SYS interrupt splits those two lines (since the SYS interrupt
is shared, it can occur at any time).

quoted

               at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
-     } else
+     } else {
               at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);

Here a barrier is again needed to prevent the compiler from reordering,
but we also need a register read back (of some RTC-register) before
updating the mask. Without the register read back, there could be a
window where the mask does not match the hardware state due to bus
latencies.

Note that even with a register read back there is a (theoretical)
possibility that the interrupts have not yet been disabled when the fake
mask is updated. The only way to know for sure is to poll RTC_IMR but
that is the very register you're trying to emulate.

quoted

+             at91_rtc_imr &= ~AT91_RTC_ALARM;
+     }

       return 0;
}

In the worst-case scenario ignoring the shared RTC-interrupt could lead
to the disabling of the system interrupt and thus also PIT, DBGU, ...

And how often does the AT91_RTC_ALARM alarm interrupt fire?

I think this patch should be reverted and a fix for the broken SoCs be
implemented which does not penalise the other SoCs. That is, only
fall-back to faking IMR on the SoCs where it is actually broken.

Even though I sent a patch to fix this problem to Nicolas,
what was presented is not my version. In mine I added DT
support:

#ifdef CONFIG_OF
static const struct of_device_id at91rm9200_rtc_dt_ids[] = {
        { .compatible = "atmel,at91rm9200-rtc", .data = &at91rm9200_config },
        { .compatible = "atmel,at91sam9x5-rtc", .data = &at91sam9x5_config },
        { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, at91rm9200_rtc_dt_ids);
#else
#define at91rm9200_rtc_dt_ids NULL
#endif /* CONFIG_OF */


The shadow IMR variable was only active in the
      .compatible = "atmel,at91sam9x5-rtc"
case. That protected all existing users from any problems
that might be introduced.

Doug Gilbert