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 <b...@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.gal...@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.gal...@tallertechnologies.com> wrote: >>> Thanks Ben! Indeed, any further testing is more than welcome :) >>> >>> On Thu, Feb 11, 2016 at 7:55 PM, Ben Gras <b...@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.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 >>>>> >>> >>> >>> >>> -- >>> >>> >>> 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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
