-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: add basic CryptoCell RNG driver #5176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
lulf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I think the rng driver can do it, ideally if it can check if it's enabled already first.
Also, maybe the peripheral and types could be named RNG and Rng? It aligns with existing peripherals and doesn't conflict with any other rng for those chips.
It would become a conflict if it gets extended to the nrf52840 that has both RNG and CryptoCell peripherals. |
| EGU5_S as EGU5, | ||
| FICR_S as FICR, | ||
| FPU_S as FPU, | ||
| FPU_NS as FPU, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It came with the new generated PAC, FPU_S doesn't exist anymore. I saw that FPU_NS was used even in s mode on the nrf9120 so I made the same modification as a fix.
9958880 to
f85f144
Compare
|
Alright, squashed my fixup commits, managed to get Is there any more changes needed ? |
|
Oh also I changed the |
|
Ah I just thought about the naming, should the struct be named |
|
|
||
| let on_drop = OnDrop::new(|| { | ||
| self.stop(); | ||
| self.disable_irq(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable_irq should come before stop, since stop stops the RNG_CLK and "The registers of a cryptographic engine are only accessible when its clock is enabled."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think about that, this should be fixed now.
| fn enable_irq(&self) { | ||
| pac::CC_HOST_RGF | ||
| .imr() | ||
| .write(|w| w.set_rng_mask(pac::cc_host_rgf::vals::RngMask::IRQENABLE)); | ||
| self.r | ||
| .rng_imr() | ||
| .write(|w| w.set_ehr_valid_mask(pac::cc_rng::vals::EhrValidMask::IRQENABLE)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is considered a pac bug or not, but the default value of IMR from nrf-pac is 0, which means enable all the interrupts. (According to the datasheet and SVD file, the reset value is all 1's.). You could use modify instead. Same issue exists in disable_irq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah my bad, I used write too often when experimenting. Should be good now.
| fn disable_irq(&self) { | ||
| self.r.rng_icr().write(|w| w.set_ehr_valid_clear(true)); | ||
| pac::CC_HOST_RGF.icr().write(|w| w.set_rng_clear(true)); | ||
| self.r | ||
| .rng_imr() | ||
| .write(|w| w.set_ehr_valid_mask(pac::cc_rng::vals::EhrValidMask::IRQDISABLE)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you forget CC_HOST_RGF.IMR here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I now added the instruction to mask the rng interrupt
|
|
||
| // https://docs.nordicsemi.com/bundle/ps_nrf9151/page/cryptocell.html#ariaid-title96 | ||
|
|
||
| pac::CRYPTOCELL.enable().write(|w| w.set_enable(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No 👍
embassy-nrf/src/cryptocell_rng.rs
Outdated
| // without calling its destructor. | ||
|
|
||
| for i in 0..6 { | ||
| let bytes = r.ehr_data(i).read().to_be_bytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to_be_bytes? I saw you use from_ne_bytes elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_ne_bytes comes from the rng driver on main, I tend to use to_be_bytes by default with no real reason. It shouldn't matter as it's random bits right ? Is there a performance impact ?
I can change it to to_ne_bytes if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is at least a size impact, it's one extra instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to_be_bytes occurrences to to_ne_bytes
|
Mentioned on matrix earlier, but I tested this on an nrf5340 and blocking api works, but async doesn't. The CRYPTOCELL interrupt never fires for some reason. But if I read some registers, like CRYPTOCELL.ENABLE or the RNG_ISR register, then the interrupt fires, might be an undocumented erratum. Interestingly, I found a library within TF-M for using the cryptocell, and it seems like they always busy wait for RNG interrupts specifically. |
|
I can try to add nrf5340 support. What's the best way to busy wait ? Is it a good idea to make the task sleep for short durations using |
Signed-off-by: Nils Ponsard <[email protected]>
Signed-off-by: Nils Ponsard <[email protected]>
Signed-off-by: Nils Ponsard <[email protected]>
Signed-off-by: Nils Ponsard <[email protected]>
Signed-off-by: Nils Ponsard <[email protected]>
Signed-off-by: Nils Ponsard <[email protected]>
it's better to do Sleeping adds ltency, plus embassy-time is an optional dep. |
|
I'd also be fine with excluding the async functionality from nrf53. |
|
I made it so only the async driver of the nrf5340 CryptoCell is behind a |
This PR adds a CryptoCell RNG driver for the nrf91 series. I built it following the RNG driver for the other nrf chips, an example has been added for the nrf9151 to show how to use it.
It needs an updated nrf-pac, PR here: embassy-rs/nrf-pac#14
I tested the example on the nrf9160dk and Thingy:91 X (nrf9151).
Concerning the activation of the CryptoCell (
Cryptocell::enable()), should this be set up by the user or the RNG driver ?