[PATCH] Beaglebone: Fix the IRQ handling code
Martin Galvan
martin.galvan at tallertechnologies.com
Sun Feb 28 20:30:45 UTC 2016
Btw, should this fix go to 4.11 as well?
On Sun, Feb 28, 2016 at 5:28 PM, Martin Galvan
<martin.galvan at tallertechnologies.com> wrote:
> Thanks a lot!
>
> On Sat, Feb 27, 2016 at 8:28 PM, Ben Gras <beng at shrike-systems.com> wrote:
>> All,
>>
>> I tested this on current HEAD and verified it fixes a case I'd ran
>> into trouble with before, namely gpio-triggered irqs. Great to see. I
>> pushed your change with a commit message based on your original email,
>> Martin. I hope you like it.
>>
>> Thank you!
>>
>>
>> On Sat, Feb 27, 2016 at 10:16 PM, Ben Gras <beng at shrike-systems.com> wrote:
>>> I'm doing a rebase & build right now, thanks for the reminder.
>>>
>>> On Fri, Feb 26, 2016 at 10:32 PM, Martin Galvan
>>> <martin.galvan at tallertechnologies.com> wrote:
>>>> 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
>
>
>
> --
>
>
> 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