This looks like great work. Please let me test it (I'll try the GPIO interrupt trigger) & I'll merge it as soon as I have time.
Thank you! Cheers, Ben On Thu, Feb 11, 2016 at 3:27 PM, Martin Galvan <martin.gal...@tallertechnologies.com> wrote: > 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 > _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
