Updates the NVM peripheral to use the clock::v2 API#931
Conversation
5d9b606 to
de14e8b
Compare
* `nvm::Nvm::new` now requires the `AhbClk<NvmCtrl>` and `ApbClk<NvmCtrl>`. * Adds the `nvm::Nvm::free` method that returns the PAC controller and bus clocks. * `nvm::Nvm::smart_eeprom` now requires the `AhbClk<NvmCtrlSmeeProm>`. * Adds the `nvm::smart_eeprom::SmartEeprom::free` method that returns the bus clock. * Breaks some Tier 1 BSP examples, which cannot be repaired at the moment due to requiring other peripheral migrations to the `clock::v2` API.
|
Apologies for the very long delay in reviewing this! It looks good, however the only major drawback I see here is that having the SmartEEPROM clock passed down to the SmartEEPROM might make passing around NVM a lot harder, since SmartEEPROM takes a reference to NVM, so when SmartEEPROM is in use, you can't move or use NVM. So now you need to keep track of both the clock token and NVM when sharing it in order to make use of the SmartEEPROM. That said, it is still a viable option, but in an RTIC app that uses NVM+SmartEEPROM for example, I can see some shared NVM resource going from this: to and then some redundant extra code to access the SmartEEPROM every time to pull the clock token out of the option, use it, and then place it back. Maybe, the simpler solution would be to just put the NvmCtrlSmeeProm clock into NVM itself? - I know this would mean the ahb is enabled when maybe a user will never use the SmartEEPROM functionality....but I think makes using it a lot easier. |
|
@rnd-ash I've been away from this stuff for a few months, but wanted to address your points. I see the inconvenience of having to carry around the Certainly the simplest solution to avoid this inconvenience is to just require the While I haven't thought it through completely, it seems like this inefficiency would be eliminated if desired by using a type-level enum parameter on trait UseEeprom {}
struct NvmNoEeeprom {}
impl UseEeeprom for NvmNoEeeprom {}
struct NvmEeprom {}
impl UseEeeprom for NvmEeeprom {}
struct Nvm<E: UseEeprom> {
...
}Then the |
Summary
As part of the
clock::v2effort tracked in Issue #912, this PR updates thenvmto use theclock::v2API by requiring ownership of itsAhbClks andApbClk. Note that this peripheral is only onthumbv7targets.The NVM has three AHB bus clocks:
CLK_NVMCTRL_AHB,CLK_NVMCTRL_CACHE, andCLK_NVMCTRL_SMEEPROM. It seems thatCLK_NVMCTRL_AHBis needed for basic operation, but the documentation does not talk about exactly whatCLK_NVMCTRL_CACHEandCLK_NVMCTRL_SMEEPROMare needed for at all. As the thenvm::Nvmdoes not seem to do anything with the cache at all, it was assumed thatAhbClk<NvmCtrlCache>is not needed. However, it is assumed thatAhbClk<NvmCtrlSmeeProm>is needed for SmartEEPROM functionality, and so this is now required when callingnvm::Nvm::smart_eeprom, and the clock can be freed withnvm::smart_eeprom::SmartEeprom::free.Note that automatic wait states are enabled when
clock::v2::clock_system_at_resetis called so that the notes here about wait states should not be an issue.The following Tier 1 BSP examples are now broken and cannot be fixed until the noted peripherals are also migrated and merged (see the notes about this in Issue #912):
feather_m4/nvm_dsu(dsu::Dsu,usb::UsbBus)feather_m4/smart_eeprom(usb::UsbBus)Checklist
#[allow]certain lints where reasonable, but ideally justify those with a short comment.