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. 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 :-)
This PR resolves the issue (and it fixes an error in the definition of the IRQ_FIQ_CONTROL register)
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_timerandsd_lld_stop. 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;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 :-)
This PR resolves the issue (and it fixes an error in the definition of the IRQ_FIQ_CONTROL register)