[Bug 1001456] HAL misses Interrupt Clear-Pending Registers handling:wasted processing power

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001456

--- Comment #16 from Bernard Fouchà <bernard.fouche@kuantic.com> 2012-02-16 16:45:35 GMT ---
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Changes to the Kernel should be done with caution. I think that this addition
> > > is not necessary. The desired functionality, for a given platform, can be
> > > introduced by #defining HAL_VAR_INTERRUPT_ACKNOWLEDGE (and/or its cousins).
> > 
> > There are two problems:
> > 
> > - clearing the pending interrupt bit is completely different from acknowledging
> > an interrupt. Acknowledgment is usually done at the end of DSR/ISR while
> > clearing the pending interrupt bit must be done *before* reading the peripheral
> > registers describing interrupt source(s): HAL_VAR_INTERRUPT_ACKNOWLEDGE is not
> > a solution.
> > 
> > - if HAL_VAR_INTERRUPT_CLEAR_PENDING is added only to Cortex-M targets, then
> > how can one share a driver between an arch not requiring clearing pending
> > interrupt and Cortex-M cores? For instance the generic serial 16x5x driver?
> > Today cyg_drv_interrupt_acknowledge() resolves to CYG_EMPTY_STATEMENT for
> > Cortex-M, IMHO we need cyg_drv_interrupt_clear_pending() to resolve to
> > CYG_EMPTY_STATEMENT for non-Cortex-M, hence one can write a driver using both
> > cyg_drv_interrupt_acknowledge() and cyg_drv_interrupt_clear_pending() at the
> > correct place so the driver can work in different arch.
> 
> Normally acknowledge should be enough. Regarding the IRQ re-triggering you have
> found out, maybe (some of) LPC17xx peripherals employ pulsed rather than level
> interrupts. Here is what's Cortex-M NVIC doc saying about it.
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/Bhchgeei.html

I've read this, did you look at the section just afterwards ('NVIC usage hints
and tips') that states: "A interrupt can enter pending state even if it is
disabled. Disabling an interrupt only prevents the processor from taking that
interrupt.": there is no difference between pulse and level interrupts for
pending interrupt management.

Since eCos disable interrupts when using cyg_interrupt_mask() my guess is the
issue should show up on all Cortex-M cores having the ICPR0/1 registers (all?).

Most if not all drivers I've seen do:

1) read interrupt source(s)
2) work
3) acknowledge and unmask interrupt, exit

If there are multiple memory buffers (CAN), many interrupt conditions described
in a single register (ser16x5x,CAN,SPI), and/or a FIFO (ser16x5x,SPI) to
process then it's:

1) read interrupt source(s)
2) work
3) goto step 1 if there is still at least an interrupt condition
4) acknowledge and unmask interrupt, exit

The issue appears more clearly with this second type of drivers, they must be
changed to:

0) clear pending interrupts
1) read interrupt source(s)
2) work
3) goto step *0* if there is still at least an interrupt condition
4) acknowledge and unmask interrupt

You see that it's not possible to rewrite cyg_drv_interrupt_acknowledge() to
solve the problem: the need is different because it occurs at a different time.
Or you change the semantics of cyg_drv_interrupt_acknowledge() and then you
break driver re-use.

Doing:

1) read interrupt source(s)
2) work
3) goto step 1 if there is still at least an interrupt condition
4) clear pending interrupt, acknowledge and unmask interrupt

won't work: an interrupt could occur between steps 3 and 4 and you would lose
it, that's why clearing pending ints must be done at the beginning of the
processing.

> 
> Then, if we must implement HAL_VAR_INTERRUPT_CLEAR_PENDING I would suggest to
> do it on LPC17xx variant or, if the problem appears on other variants consider
> Cortex-M architecture level, and avoid patching kernel files. FYI as i
> mentioned earlier I'll do some tests on Kinetis during next week.

If there is a way to do that and keep driver code re-use between arch that's
would be fine!

When you perform your tests, please try clocking SLOWLY (no PLL for instance):
if the ISR/DSR sequence is faster than the time it takes to the MCU to generate
a new interrupt, then there won't be any pending interrupt and the dust will
stay hidden under the carpet and re-appear only when many different drivers
trigger more important amounts of interrupts. (my guess is this is what
happened to have this matter appearing only now, or if nobody counts the
numbers of ISR/DSR/DSR-that does-nothing/volume-of-data-handled-per-DSR-run
when testing drivers)

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.