[PATCH] Beaglebone: Fix the IRQ handling code

Martin Galvan martin.galvan at tallertechnologies.com
Fri Feb 26 21:32:24 UTC 2016


Hi Ben! Sorry to bother, were you able to test my changes to the BBB
interrupt handler?

On Fri, Feb 12, 2016 at 11:09 AM, Martin Galvan
<martin.galvan at tallertechnologies.com> wrote:
> Thanks Ben! Indeed, any further testing is more than welcome :)
>
> On Thu, Feb 11, 2016 at 7:55 PM, Ben Gras <beng at shrike-systems.com> wrote:
>> 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
>>>
>
>
>
> --
>
>
> Martin Galvan
>
> Software Engineer
>
> Taller Technologies Argentina
>
>
> San Lorenzo 47, 3rd Floor, Office 5
>
> Córdoba, Argentina
>
> Phone: 54 351 4217888 / +54 351 4218211



-- 


Martin Galvan

Software Engineer

Taller Technologies Argentina


San Lorenzo 47, 3rd Floor, Office 5

Córdoba, Argentina

Phone: 54 351 4217888 / +54 351 4218211



More information about the devel mailing list