Btw, should this fix go to 4.11 as well? On Sun, Feb 28, 2016 at 5:28 PM, Martin Galvan <martin.gal...@tallertechnologies.com> wrote: > Thanks a lot! > > On Sat, Feb 27, 2016 at 8:28 PM, Ben Gras <b...@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 <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 > > > > -- > > > 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
