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
