mspm0: trng driver #5172
mspm0: trng driver #5172
Conversation
embassy-mspm0/src/trng.rs
Outdated
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub struct Trng<'d, C: ClockDiv, D: DecimRate = decim_rate::Decim4> { |
There was a problem hiding this comment.
Clock div should probably be calculated internally. Although getting the MCLK frequency is currently hardcoded (see i2c).
Given than crypto rng only applies with a minimum decimation rate, maybe instead of making decimation rate generic, we define a "crypto" and non crypto rng type state. When making a TRNG you pass a different decimation rate enum value for each type.
There was a problem hiding this comment.
For the clock div, I guess that would mean we can hardcode a divider of 2.
I'm not sure I understand your suggestion for the decimation rate. Do you mean that we have separate enum types for crypto-secure decimation rates and non-secure? I don't think we can restrict the typestate based on the actual enum values.
There was a problem hiding this comment.
Hardcoding 2 might not be ideal? G series devices tend to have a PLL.
For my suggestion, creating a trng you have two functions. One takes an enum with decimations below 4. The other takes decimations above 4.
For the former it's something like Trng<'d, NonSecure> and for the latter Trng<'d, Secure> (names of course may differ)
There was a problem hiding this comment.
Is there a way to determine the MCLK frequency? It seems to be the case that I can't really do better than hardcoding until an API for adjusting the frequency/PLL is added along with a way to determine the configured frequency. I wouldn't mind stubbing out a method to get the (hardcoded) frequency and calculating from there. I'm not sure what the right shape for that stub would be (maybe it needs to get the Config somehow.)
I see. I'm a bit conflicted on splitting the enum from a usability perspective, though. What drawbacks do you see with the current approach? To avoid most of the monomorphization bloat, we could make the internal driver functions free functions. I suspect that there won't be many cases where multiple decimation rates are requested, though.
There was a problem hiding this comment.
Monomorphization bloat would be nice to avoid, especially for decimation. The secure/non-secure is a typestate where you assert whether or not it is truly cryptographically secure.
Implementing CryptoRng for Trng would obviously violate the contract provided by that trait.
For now just hardcode to the boot frequency. I2C does this https://github.com/embassy-rs/embassy/blob/main/embassy-mspm0/src/i2c.rs#L203-L224 When I get around to clock config I will fix this at a larger scale.
There was a problem hiding this comment.
I adjusted the API a bit to avoid needing to use a turbofish to set the decim rate. I tried the two enum approach, but I wasn't too pleased with the resulting API. FWIW, we do save on needing to store the rate value with the generic approach. WDYT?
There was a problem hiding this comment.
To avoid turbofish you could do new_secure and new_nonsecure that return concrete types.
If you wanted to look at potentially eliminating 100% of generics, we could even just not expose non-secure modes (so force a decimation rate of at least 4 always).
There was a problem hiding this comment.
I think the new setup is as discussed. WDYT?
henopied
force-pushed
the
mspm0-trng
branch
2 times, most recently
from
63b6471 to
d0bfef0
Compare
i509VCB
left a comment
i509VCB
left a comment
There was a problem hiding this comment.
Driver looks good, just 2 small internal adjustments.
| static WAKER: AtomicWaker = AtomicWaker::new(); | ||
|
|
||
| mod sealed { | ||
| pub trait Sealed {} |
There was a problem hiding this comment.
I think we already have a Sealed in the crate root?
| fn get_mclk_frequency() -> u32 { | ||
| 32_000_000 | ||
| } |
There was a problem hiding this comment.
This is wrong on C and G series. iirc C series is 24 MHz and G series is 80 MHz (at boot).
M33 I will deal with later.
2 participants
No description provided.