ChibiOS-RPi
ChibiOS-RPi copied to clipboard
steve-bate
Pointers
I tried to merge this pull request locally, but sadly it does not work out.
git clone https://github.com/steve-bate/ChibiOS-RPi.git cd ChibiOS-RPi curl https://github.com/steve-bate/ChibiOS-RPi/pull/3.patch | git am
Is there an way that I could get the already merged and new version?
Hi,
I have noticed what I think are some consistent errors in the interrupt enabling/disabling code at serveral places in this repo, e.g. in gpt_lld_start_timer and sd_lld_stop. This is also the case in this PR in the external int enable/disable code in disable_bank_interrupt
In these functions, interrupts are enabled or disabled using a binary OR, like:
IRQ_DISABLE1 |= gptp->irq_mask;
However, IRQ_ENABLE_x and IRQ_DISABLE_x are set-by-write and clear-by-write, i.e. writing a 1 bit sets/clears the bit and writing a 0 does nothing. Thus, the line above should read:
IRQ_DISABLE1= gptp->irq_mask;
If you use the binary OR, the processor will read IRQ_DISABLE1, do the or, and then write the result back. As a consequence, any bit set in the register will be cleared.
For the IRQ_ENABLE1, the effect is less obvious but still there: what could happen if you use |= is the following:
IRQ_ENABLE|=1;
- the processor reads IRQ_ENABLE1
- the processor ORs it with 1
- some (unrelated) interrupt happens, the interrupt handler for this could disable any interrupt in IRQ_ENABLE1
- return from interrupt
- the processor writes the ORed value to IRQ_ENABLE1, inadvertendly re-enabling the interupt that was disabled by the interrupt handler
Clearly, enabling and disabling interrupts in IRQ_ENABLE1, IRQ_ENABLE2 and IRQ_ENABLE_BASIC should use the = and not the |= operation. The separate ENABLE and DISABLE registers are a way to safely enable/disable interrupts in a single monotonic operation. The hardware designers provided this for us, and for a reason :-)
I am happy to create a PR that resolves these cases if desired.
