Add support for atomic xor/clear/set register operations for Raspberry Pi RP2040 microcontroller

[richardanaya]

As per https://datasheets.raspberrypi.org/rp2040/rp2040-datasheet.pdf "2.1.2. Atomic Register Access"

This looks vaguely similar to something that was done for the msp430. Unsure how common this is for cortex M registers across the board. Would this be some type of very specific target?

[henkkuli]

I have described one possible implementation for this in a comment elsewhere, but I don't know how to gate that for specific registers or peripherals only. I tried to look at the corresponding SVD file but haven't found any indication there that some registers should be atomic*. I also couldn't find a way to encode this info in a SVD file, at least based on a surface look at svd_parser. Does anyone here have an idea how to mark only some peripherals/registers as atomic? Maybe have another file listing all atomic peripherals/registers? Or list each atomic peripheral as a flag?

*All registers except those belonging to SIO peripheral should be atomic.

[burrbull]

As SVD doesn't have such info, the only way I see is to implement atomic operation for all registers, but make them unsafe as it done for write_with_zero.

[henkkuli]

That would be unfortunate, but at least it would work.

By the way, I have a POC available in my fork if someone wants to take a look. Would something like that fly? I can try to finish a PR, but for that I need some help with how to gate the feature for RP2040 target only.

[adfernandes]

FWIW, on the RP235X at least here's the definitive answer on "Which peripherals, specifically, support atomic modification?"

I'm trying to add this as a type-safe trait right now...

[burrbull]

If those writes are marked unsafe do we really need to restrict them?
Lets just implement xor/clear/set for all Regs.

[jannic]

Just playing devils advocate: If writes are marked unsafe, why do we bother with a PAC at all and don't just do volatile writes do raw addresses?

There is some value in only providing write access to registers that actually exist. So just implementing xor/clear/set for all registers even if some of the registers don't provide that function would technically work, and wouldn't be unsound (due to unsafe), but would slightly reduce the value the PAC provides.

It might still be the right trade-off, if an exact implementation would be too much work, or would clutter the PAC implementations with lots of manual patches because the vendor SVDs don't provide the required information.

[burrbull]

Just playing devils advocate: If writes are marked unsafe, why do we bother with a PAC at all and don't just do volatile writes do raw addresses?

I did not say about ordinary writes, only about atomic:

svd2rust/src/generate/generic_atomic.rs

Line 51 in 29bcfd8

pub unsafe fn set_bits<F>(&self, f: F)

[adfernandes]

Lets just implement xor/clear/set for all Regs.

Another problem with this approach is that it makes static analysis difficult if not impossible.

This is especially important since there is a huge push to use rust in safety critical applications including industrial robotics, automotive control, and avionics.

[jannic]

I did not say about ordinary writes, only about atomic:

Yes, I know. But with the same argument "it's unsafe anyway, so it doesn't matter if we implement it for some registers that don't actually implement it" one could also ask "why do we need a PAC at all, we can do unsafe register writes without it?".

Part of the answer is that PACs do not allow access to every arbitrary address, but only to registers that actually exist. This doesn't prevent you from writing to arbitrary addresses (you can still use ptr::volatile_write), but it makes it less likely that you accidentally confuse addresses and write to some register that doesn't actually exist on your MCU.

Therefore, I'm not sure which one of these two choices is better:

• let the PAC provide atomic write functions for all registers (even those which don't exist)
• don't provide atomic write functions at all, and let users implement something like https://github.com/rp-rs/rp-hal/blob/main/rp2040-hal/src/atomic_register_access.rs (usage example: https://github.com/rp-rs/rp-hal/blob/247dce8c68d1be46c22ccd01f812252c908cc13a/rp2040-hal/src/gpio/mod.rs#L710-L720)

Sure, the second approach requires users to write a few more lines of code. But at the same time, it becomes obvious that they are doing something not directly supported by the PAC, so the caller needs to be careful whether it's actually allowed.

Of course, from the user's point of view, a third choice is better than both of the two above:

• let the PAC provide atomic write functions for exactly the registers that support it

But that may be a lot of work for PAC authors, even if svd2rust provided support for it.

[adfernandes]

I am in the process right now of trying to figure out how registers in the PAC can be annotated for atomic access, since that is unfortunately not something supported in the SVD, nor is it supported bt the svdtools patcher.

I do not think that the HAL is the correct place to house the atomic access API because it is not a hardware abstraction, it is a physical property of how the peripheral interacts with the bus… You can't get more PAC than that!

The atomic registers are real registers at a real address on a real bus, so I think there is a strong argument to treat them as first class citizens, no different than readable, writable, or read/writable.

[burrbull]

I am in the process right now of trying to figure out how registers in the PAC can be annotated for atomic access, since that is unfortunately not something supported in the SVD, nor is it supported bt the svdtools patcher.

#856 (comment)

[romancardenas]

Something like this would also be great for RISC-V Zaamo targets, which only support AMO* instructions. We are currently dealing with this special situation with portable-atomic.

[burrbull]

As #856 is merged you could try to add a section in Settings describing ranges.