[PATCH] Beaglebone: Fix the IRQ handling code
Ben Gras
beng at shrike-systems.com
Thu Feb 11 22:55:34 UTC 2016
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.galvan at 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
>
More information about the devel
mailing list