dma_irqn_acknowledge_channel incorrectly clears ALL channel interrupts

[NomiChirps]

After spending a couple of days chasing this down, I've discovered that attempting to use hw_set_bits (or hw_clear_bits, or hw_xor_bits) with the DMA INTS0/INTS1 interrupt flag register causes ALL of the active channel interrupts to be cleared, not just the ones in the bitmask. This happens even when an empty bitmask is used.

The correct behavior is obtained by simply assigning a bitmask to the register, e.g. dma_hw->ints0 = 1 << channel_num. Unfortunately the current implementation of dma_irqn_acknowledge_channel uses hw_set_bits.

Discussion: https://forums.raspberrypi.com/viewtopic.php?p=2029939
Minimal reproduction: https://github.com/NomiChirps/missed_interrupt

The DMA INTS0/INTS1 register is marked as "WC" type in the RP2040 datasheet:

WC
This is a single bit that is typically set by a piece of hardware and then written to by the processor to clear the bit. The
bit is cleared by writing a 1, using either a normal write or the clear alias. See Section 2.1.2 for more information about
the clear alias.

So it's advertised that hw_clear_bits() should work with this register, and not clear that hw_set_bits() does anything well-defined. My testing has found that none of the hw_*_bits() aliases function as advertised here, though; they all clear the entire register no matter what bitmask is used.

In conclusion...

• I'd be happy to author a pull request to correct dma_irqn_acknowledge_channel, if you'd like.
• I think it might be worth checking to make sure no other "WC"-typed registers are accidentally misused(?) this way by the SDK.
• It's a mystery why hw_clear_bits() doesn't work with this register, although the data sheet seems to advertise that it should.

Thanks for reading :)

[Wren6991]

Thanks for raising this, this is an inconsistency in the behaviour of fields that are implemented with our register block generator (most of them) and those which either were handwritten, or are vendor register blocks with an external atomic shim added.

For the following blocks:

• DMA
• XIP CTRL
• XIP SSI
• I2C
• UART
• SPI

One of the above criteria is true, and the effect of an atomic set/clear/xor is as though the current value were read, then the set/clr/xor were applied to that value, and finally the new value were written. For RW fields this is behaves the same as the generated register blocks, but for some others (particularly WC) the behaviour differs.

Doing a hw_set_bits with a bitmask of zero on the DMA IRQ register is equivalent to writing back its current value, which will clear all IRQs.

The dma_irqn_acknowledge_channel function is buggy, so yes please, I'd be grateful if you could raise a PR to change that to a raw write with a single bit set.

[kilograham]

note this also affects dma_channel_acknowledge_irq0 and dma_channel_acknowledge_irq1

Note, you'd expect that more people would have hit this, however people often split multiple DMA irqs between INTS0/INTS1, and the pico_scanvideo which would definitely expose this problem, pre-dates the methods and writes the registers directly (without the set alias)

[daveythacher]

This is a habit for many. Also, the datasheet recommends using INTS0/INTS1 quietly.

In theory hw_clear_bits would work? (r & !m) If given !m instead of m?

[daveythacher]

Question how do you plan to mark this in documentation? You could say that all WC registers do not support atomic. You could also mark this as an errata. The SDK has a few changes, but will this be declared a silicon defect?

[aallan]

The documentation will be updated appropriately, there may be an SDK fix as well. It's not an errata.

[kilograham]

merged into develop

[aallan]

@Wren6991 Are you going to do an update to the datasheet?

[daveythacher]

@Wren6991 Are you going to do an update to the datasheet?

@aallan @Wren6991 Are these comments relevant? #978