[PATCH] Beaglebone: Fix the IRQ handling code

Martin Galvan martin.galvan at tallertechnologies.com
Thu Feb 11 14:27:41 UTC 2016


This patch makes the following changes to the Beaglebone IRQ handling code:

- Disable support for nested interrupts.
- Detect spurious IRQs using the SPURIOUSIRQ field of the INTC_SIR_IRQ register.
- Acknowledge spurious IRQs by setting the NewIRQAgr bit of the INTC_CONTROL
  register. This cleans the SPURIOUSIRQ field and allows new interrupts
  to be generated.
- Improve the get_mir_reg function a bit.

The Beaglebone bsp_interrupt_dispach function has been troublesome for a while now.
We've seen it break the GPIO API (https://lists.rtems.org/pipermail/devel/2015-November/012995.html),
the RTEMS interrupt server (https://lists.rtems.org/pipermail/devel/2015-July/011865.html),
and now we've been getting spurious interrupts when trying to use the I2C module.

We've done a lot of testing and concluded that the cause of most of these issues
is the way nested interrupts are being handled. The AM335X manual states that
the interrupt handling sequence should be as follows:

1. Identify the IRQ source by reading the ActiveIRQ field of the INTC_SIR_IRQ
   register.
2. Jump to the corresponding IRQ handler, which should serve the IRQ and
   deassert the interrupt condition at the peripheral side.
3. Enable the processing of new IRQs at the Interrupt Controller side by setting
   the NewIRQAgr bit of the INTC_CONTROL register.
4. Finally, enable IRQs at the CPU side. This is done later in
   _ARMV4_Exception_interrupt.

Right now the Beaglebone bsp_interrupt_dispach enables IRQs at the INTC and CPU
before jumping to the interrupt handler to allow for nested IRQs.
Before doing so, it calls bsp_interrupt_disable to mask the IRQ source and avoid
having it constantly fire new IRQs. After it's done it re-enables the IRQ
by calling bsp_interrupt_enable. These calls break the GPIO API and the
RTEMS interrupt server machinery, and we suspect it's also causing the spurious
interrupts we saw with the I2C module.

The correct way to enable interrupt nesting according to both the manual and
the AM335X StarterWare code is to allow only interrupts of a higher priority
to preempt the current one. This can be achieved by setting the INTC_THRESHOLD
register to the priority of the current IRQ. However, in our case this isn't
necessary since all the interrupt priorities are set to 0 (the highest possible)
in bsp_interrupt_facility_initialize. We may implement this in a future patch,
if required.

We've tested this quite extensively on a number of different applications, and
it's working fine.

Closes #2580.
---
 c/src/lib/libbsp/arm/beagle/irq.c           | 82 +++++++++++++++--------------
 c/src/lib/libcpu/arm/shared/include/omap3.h |  3 +-
 2 files changed, 44 insertions(+), 41 deletions(-)

diff --git a/c/src/lib/libbsp/arm/beagle/irq.c b/c/src/lib/libbsp/arm/beagle/irq.c
index c6485cd..d080a5e 100644
--- a/c/src/lib/libbsp/arm/beagle/irq.c
+++ b/c/src/lib/libbsp/arm/beagle/irq.c
@@ -18,6 +18,7 @@
 #include <bsp.h>
 #include <bsp/irq-generic.h>
 #include <bsp/linker-symbols.h>
+#include <bsp/fatal.h>

 #include <rtems/score/armv4.h>

@@ -43,77 +44,78 @@ static struct omap_intr omap_intr = {
 };
 #endif

-static int irqs_enabled[BSP_INTERRUPT_VECTOR_MAX+1];
+/* Enables interrupts at the Interrupt Controller side. */
+static inline void omap_irq_ack(void)
+{
+  mmio_write(omap_intr.base + OMAP3_INTCPS_CONTROL, OMAP3_INTR_NEWIRQAGR);

-volatile static int level = 0;
+  /* Flush data cache to make sure all the previous writes are done
+     before re-enabling interrupts. */
+  flush_data_cache();
+}

 void bsp_interrupt_dispatch(void)
 {
-  /* get irq */
-  uint32_t reg = mmio_read(omap_intr.base + OMAP3_INTCPS_SIR_IRQ);
-  int irq;
-  irq = reg & OMAP3_INTR_ACTIVEIRQ_MASK;
+  const uint32_t reg = mmio_read(omap_intr.base + OMAP3_INTCPS_SIR_IRQ);

-  if(!irqs_enabled[irq]) {
-	/* Ignore spurious interrupt */
-  } else {
-    bsp_interrupt_vector_disable(irq);
-
-    /* enable new interrupts, and flush data cache to make sure
-     * it hits the intc
-     */
-    mmio_write(omap_intr.base + OMAP3_INTCPS_CONTROL, OMAP3_INTR_NEWIRQAGR);
-    flush_data_cache();
-    mmio_read(omap_intr.base + OMAP3_INTCPS_SIR_IRQ);
-    flush_data_cache();
-
-    /* keep current irq masked but enable unmasked ones */
-    uint32_t psr = _ARMV4_Status_irq_enable();
-    bsp_interrupt_handler_dispatch(irq);
-
-    _ARMV4_Status_restore(psr);
+  if ((reg & OMAP3_INTR_SPURIOUSIRQ_MASK) != OMAP3_INTR_SPURIOUSIRQ_MASK) {
+    const rtems_vector_number irq = reg & OMAP3_INTR_ACTIVEIRQ_MASK;

-    bsp_interrupt_vector_enable(irq);
+    bsp_interrupt_handler_dispatch(irq);
+  } else {
+    /* Ignore spurious interrupts. We'll still ACK it so new interrupts
+       can be generated. */
   }
+
+  omap_irq_ack();
 }

-static uint32_t get_mir_reg(int vector, uint32_t *mask)
+/* There are 4 32-bit interrupt mask registers for a total of 128 interrupts.
+   The IRQ number tells us which register to use. */
+static uint32_t omap_get_mir_reg(rtems_vector_number vector, uint32_t *const mask)
 {
-  *mask = 1UL << (vector % 32);
-
-  if(vector <   0) while(1) ;
-  if(vector <  32) return OMAP3_INTCPS_MIR0;
-  if(vector <  64) return OMAP3_INTCPS_MIR1;
-  if(vector <  96) return OMAP3_INTCPS_MIR2;
-  if(vector < 128) return OMAP3_INTCPS_MIR3;
-  while(1) ;
+  uint32_t mir_reg;
+
+  /* Select which bit to set/clear in the MIR register. */
+  *mask = 1ul << (vector % 32u);
+
+  if (vector < 32u) {
+    mir_reg = OMAP3_INTCPS_MIR0;
+  } else if (vector < 64u) {
+    mir_reg = OMAP3_INTCPS_MIR1;
+  } else if (vector < 96u) {
+    mir_reg = OMAP3_INTCPS_MIR2;
+  } else if (vector < 128u) {
+    mir_reg = OMAP3_INTCPS_MIR3;
+  } else {
+    /* Invalid IRQ number. This should never happen. */
+    bsp_fatal(0);
+  }
+
+  return mir_reg;
 }

 rtems_status_code bsp_interrupt_vector_enable(rtems_vector_number vector)
 {
   uint32_t mask, cur;
-  uint32_t mir_reg = get_mir_reg(vector, &mask);
+  uint32_t mir_reg = omap_get_mir_reg(vector, &mask);

   cur = mmio_read(omap_intr.base + mir_reg);
   mmio_write(omap_intr.base + mir_reg, cur & ~mask);
   flush_data_cache();

-  irqs_enabled[vector] = 1;
-
   return RTEMS_SUCCESSFUL;
 }

 rtems_status_code bsp_interrupt_vector_disable(rtems_vector_number vector)
 {
   uint32_t mask, cur;
-  uint32_t mir_reg = get_mir_reg(vector, &mask);
+  uint32_t mir_reg = omap_get_mir_reg(vector, &mask);

   cur = mmio_read(omap_intr.base + mir_reg);
   mmio_write(omap_intr.base + mir_reg, cur | mask);
   flush_data_cache();

-  irqs_enabled[vector] = 0;
-
   return RTEMS_SUCCESSFUL;
 }

diff --git a/c/src/lib/libcpu/arm/shared/include/omap3.h b/c/src/lib/libcpu/arm/shared/include/omap3.h
index f28e5e5..0cc43d6 100644
--- a/c/src/lib/libcpu/arm/shared/include/omap3.h
+++ b/c/src/lib/libcpu/arm/shared/include/omap3.h
@@ -72,7 +72,8 @@
 #define OMAP3_INTR_ILR(base,m) \
     (base + OMAP3_INTCPS_ILR0 + 0x4 * (m))

-#define OMAP3_INTR_ACTIVEIRQ_MASK 0x7f /* Active IRQ mask for SIR_IRQ */
+#define OMAP3_INTR_SPURIOUSIRQ_MASK (0x1FFFFFF << 7) /* Spurious IRQ mask for SIR_IRQ */
+#define OMAP3_INTR_ACTIVEIRQ_MASK 0x7F /* Active IRQ mask for SIR_IRQ */
 #define OMAP3_INTR_NEWIRQAGR      0x1  /* New IRQ Generation */

 #define OMAP3_DM337X_NR_IRQ_VECTORS    96
--
2.7.1



More information about the devel mailing list