Skip to content

Updates the NVM peripheral to use the clock::v2 API#931

Open
kyp44 wants to merge 1 commit into
atsamd-rs:feat/clock-v2from
kyp44:nvm-clock-v2
Open

Updates the NVM peripheral to use the clock::v2 API#931
kyp44 wants to merge 1 commit into
atsamd-rs:feat/clock-v2from
kyp44:nvm-clock-v2

Conversation

@kyp44
Copy link
Copy Markdown
Contributor

@kyp44 kyp44 commented Aug 4, 2025

Summary

As part of the clock::v2 effort tracked in Issue #912, this PR updates the nvm to use the clock::v2 API by requiring ownership of its AhbClks and ApbClk. Note that this peripheral is only on thumbv7 targets.

The NVM has three AHB bus clocks: CLK_NVMCTRL_AHB, CLK_NVMCTRL_CACHE, and CLK_NVMCTRL_SMEEPROM. It seems that CLK_NVMCTRL_AHB is needed for basic operation, but the documentation does not talk about exactly what CLK_NVMCTRL_CACHE and CLK_NVMCTRL_SMEEPROM are needed for at all. As the the nvm::Nvm does not seem to do anything with the cache at all, it was assumed that AhbClk<NvmCtrlCache> is not needed. However, it is assumed that AhbClk<NvmCtrlSmeeProm> is needed for SmartEEPROM functionality, and so this is now required when calling nvm::Nvm::smart_eeprom, and the clock can be freed with nvm::smart_eeprom::SmartEeprom::free.

Note that automatic wait states are enabled when clock::v2::clock_system_at_reset is 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

  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

@kyp44 kyp44 changed the base branch from master to feat/clock-v2 November 12, 2025 14:57
Comment thread hal/src/peripherals/nvm/mod.rs
* `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.
@rnd-ash
Copy link
Copy Markdown
Contributor

rnd-ash commented May 22, 2026

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:

Shared {
   shared_nvm: Nvm
}

to

Shared {
   shared_nvm: (Nvm, Option<AhbClk<NvmCtrlSmeeProm>>)
}

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.

@kyp44
Copy link
Copy Markdown
Contributor Author

kyp44 commented May 24, 2026

@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 AhbClk<NvmCtrlSmeeProm> so that the SmartEeprom can be recreated using Nvm::smart_eeprom whenever the Nvm is moved (while also needing to call SmartEeprom::free each time to get it back).

Certainly the simplest solution to avoid this inconvenience is to just require the AhbClk<NvmCtrlSmeeProm> in Nvm::new (and return it with Nvm::free), but this can be inefficient since the AHB clock would have to be enabled even if the user has no intention of using the Smart EEPROM, as you pointed out.

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 Nvm. Something like:

trait UseEeprom {}

struct NvmNoEeeprom {}
impl UseEeeprom for NvmNoEeeprom {}

struct NvmEeprom {}
impl UseEeeprom for NvmEeeprom {}

struct Nvm<E: UseEeprom> {
  ...
}

Then the new and free methods of Nvm would have different versions for the E parameter, which would or would not take/return the AhbClk<NvmCtrlSmeeProm>, and the smart_eeprom method would only be implemented for Nvm<NvmEeprom>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants