`clock::v2` API migration discussion and tracking

[kyp44]

Summary

This issue is meant to track and discuss issues related to migration to the clock v2 API.

The clock v2 API "...provides a simple, ergonomic, and most of all safe API to create and manage the clock tree in ATSAMD5x and E5x devices. It uses type-level programming techniques to prevent users from creating invalid or unsound clocking configurations."

The key feature compared to the clock v1 API is that the clock tree configuration is validated at compile time using type-level programming, which is better for most use cases in which the clock tree is set up once at the beginning of the program and then not changed.

Status

Currently, the v2 API is only implemented for thumbv7 targets, and is not widely supported throughout the HAL, making it currently largely unusable for most projects.

Tasks

The following are pending identified tasks at the highest level:

1. Add clock v2 API support throughout the entire HAL for thumbv7 targets.
2. Port the clock v2 API to thumbv6m targets.
3. Add clock v2 API support throughout the entire HAL for thumbv6m targets.

Task 1: HAL support for thumbv7 targets

Migration guidelines

1. The focus should be on fully migrating peripherals to v2 and not worrying about still supporting v1.
2. For safety, peripherals that depend on AHB or APB bus clocks, should require ownership of their AhbClk or ApbClk structs, even if their bus clock is enabled at startup, because these clocks can be disabled, rendering the peripheral non-functional. Requiring ownsership ensures that it cannot be disabled while the peripheral is in use.
3. Peripherals that require a PCLK should require ownership of the Pclk struct. According to the "1:1 clocks" section of the clock::v2 module documentation, this "prevents users from modifying or disabling the Pclk while it is in use by the peripheral".
4. All items that consume clocks (i.e. the above structs) should have a free method that returns them. This should also of course return the pac peripheral struct as well if applicable.
5. To use the v2 API correctly, peripherals requiring a Pclk need to own it, and the Pclk can have different sources via its I: PclkSourceId generic. This means that the peripheral struct itself (e.g. Adc) needs to have the same generic parameter. The problem is that, on thumbv6 targets, this generic on the peripheral struct and its impl blocks cannot exist (since they currently have no v2 API), leading to requiring separate impl blocks for each chip family, hence potentially duplicated code. This is explained nicely in a comment for the Adc. The Adc takes a partial approach by simply requiring a &Pclk at the time of creation, but this does not prevent the Pclk from being disabled while still in use by the Adc. After some discussion below, it was agreed that applicable peripherals should not be migrated until the v2 is ported to thumbv6 targets (i.e. Task 2). These peripherals are noted in the table below.
6. If examples use more than one peripheral that needs migrated and those perihperals are migrated in separate PRs, the example may be broken until both PRs are merged, and then the example can be fixed in a separate PR. An example of this is discussed below.

Peripheral migration tracking

This tracks the various peripherals/modules that require clocking for thumbv7 targets:

Peripheral	thumbv7 only	Awaiting thumbv6 v2 port	Clocks required	v1 Support	v2 Support	Clk ownership	free method	Complete	Notes
ADC		✅	APB, PCLK	No	adc::AdcBuilder		✅		Partial migration with PR #814, see note 1 below
AES	✅		APB	No	aes::Aes::new	✅	✅	✅	Migration merged in PR #920
CAN	✅		AHB, PCLK	No	can::Dependencies::new	✅	✅	✅	Fixed in the merged PR #919
DAC			APB, PCLK	No	dac::Dac::new	✅	✅	✅	New peripheral proposed in PR #904
Delay (SYSTICK)			GCLK0	No	delay::Delay::new	✅	✅	✅	Fixed up in the proposed PR #929
DMAC			AHB	Yes	Yes				Does not require AHB clock proof
DSU	✅		AHB, APB	No	dsu::Dsu::new	✅	✅	✅	Migration proposed in PR #925
EIC		✅	APB, PCLK / ULP32K	eic::Eic::new	Partial				Provides methods to switch clock sources, may need changed, see note 2 below
ICM	✅		AHB, APB	No	icm::Icm::new	✅	✅	✅	Migration proposed in PR #927
NVM	✅		AHB, APB	Yes	Yes				Does not require clock proof. There are several AHB mask bits for this: NVMCTRL_CACHE, NVMCTRL_SMEEPROM, and NVMCTRL
PUKCC	✅		AHB	pukcc::Pukcc::new	No
PWM (TC/TCC)		✅	APB, PCLK	pwm::PwmX::new or pwm::TccXPwm::new	No
QSPI	✅		AHB, APB	No	qspi::QspiBuilder::build				Migration and overhaul proposed in PR #926
RTC		✅	APB, RtcOsc	rtc::Rtc::count32_mode or rtc::Rtc::clock_mode	No				No way to actually set the Rtc clock with v1
RTC RTIC Monotonic		✅	APB, RtcOsc	rtc_monotonic!::start	No				No clock proof required, but RTC clock rate must be known at compile time via rtc::rtic::rtc_clock::RtcClockRate
RTC Embassy time driver		✅	APB, RtcOsc	No	embassy_time!::init				Proposed in PR #825, requires only RtcOsc but not AbpClk
Sercom I2C		✅	APB, PCLK	sercom::i2c::Config::new	No
Sercom SPI		✅	APB, PCLK	sercom::spi::Config::new	No
Sercom UART		✅	APB, PCLK	sercom::uart::Config::new	No
TC		✅	APB, PCLK	timer::TimerCounter::tcx_	No				Open PR #690 updates this, but is out of date and incomplete
TRNG	✅		APB	trng::Trng::new	No
USB			AHB, APB, PCLK	No	usb::UsbBus::new	✅	✅	✅	The PCLK must be 48 MHz. PR #916 proposes the migration
WDT			APB	Yes	Yes				Uses the OSCULP32K, which is always enabled if the WDT is enabled, does not require APB clock proof.

If anything was missed or there are any errors, please comment below.

Notes:

1. The ADC currently only takes a reference to its Pclk and it is problematic to take full ownership until the v2 is ported to thumbv6 chips, see the explanation here.
2. The EIC can use a standard Pclk or the ULP32K clock, which is selected via the CKSEL bit in the EIC CTRLA register. Some discussion is needed on what the right way to handle this and switch clocks should be.

Here are other PRs and issues related to this:

• Issue #642 discusses that the RTC can be used without configuring the clock, which is a known issue that was discussed further in PR #845.

Task 2: Port the API to thumbv6m targets.

Completion of this task is needed to reasonably complete some peripherals in Task 1, see the table and notes above.

Work on this is ongoing:

• PR #892 (Draft)
• A fork that does this. It has recent commits, but is way behind master.

Task 3: HAL support for thumbv6 targets

This of course depends on the completion of Task 2, and will also likely be able to leverage the work done in Task 1.

[ianrrees]

Initially, should both clock v1 and v2 be supported for all peripherals?

My feeling is that the effort involved in clock v1 compatibility would be better spent on moving to clock v2. If people want to PR support for clock v1 on to a peripheral, and that support doesn't require a lot of maintainer effort (to review, and to remove sometime down the line), then it's fine. But, I wouldn't want to block a PR that adds a peripheral with only clock v2 support.

[rnd-ash]

Having been implemented a few peripherals (ADC (#814)/DAC (#904)/EVSYS (#882)) recently, I think I can say for sure that we should consider a consistent way of handling the PCLKs and AHBclks.

For some peripherals, the constructor takes an &Pclk, which means there is no guarantee that clock will stay enabled or stay at the same frequency whilst the Peripheral is active. I think we should therefore make it such that each peripheral takes ownership of the PCLKs and AHBclocks. Then each peripheral has a free method that returns these clocks, guaranteeing that the clocks can only be modified with the peripheral not active.

That said, In #850, I noticed that the EIC (And some other peripherals), have the ability to choose between OSC32K and GCLKs as their clock source (OSC32K can run in sleep but provides a slow ticker, GCLK can only run with the processor awake but has a much higher speed). This means we should also provide swap_clock methods to these kind of peripherals such that the clock source of the peripheral can be switched (I believe the PCLK has a SourceID which we can enforce at compile time depending on the requirements of the peripheral). In this case, we should make it such that the PCLKID does not add to more Generic Pollution of the peripheral itself.

[kyp44]

@ianrrees Yes, maybe the effort should be on just migrating peripherals fully to v2 instead of worrying about how to best provide compatibility with both.

@rnd-ash Thanks for the additional insights and bringing up the awareness of the new peripherals that you've added. I like the idea that all peripherals should have a free method that consumes the peripheral struct and returns anything that was required to use the peripheral (e.g. the pac and the clock tokens).

What I will do here soon (unless someone beats me to it) is to assess all currently implemented peripherals (and upcoming ones if there are open PRs) to see whether they support v1 and/or v2, and summarize the results in a table. This will help us identify what all needs to be done. For now the focus will be on Task 1, so just restricted to the thumbv7 targets.

[rnd-ash]

@kyp44 heres a table of peripheral clock support for V7 targets (Including some of the stuff thats in PRs ive done:

Peripheral	clock::v1 on V7	clock::v2 on V7	APB type	Pclk type
ADC		yes	ApbClk	&Pclk
DAC (#904)		yes	ApbClk	&Pclk
EIC	yes
PWM Driver	yes
Timer	yes
USB	yes
CAN		yes	ApbClk	Pclk AND gclk

Additional notes:

QSPI and TRNG (Which does not need a PCLK) takes a &mut Mclk parameter rather than ApbClk token.

SERCOM is an odd one as it takes &mut Mclk but also just the frequency of its clock source, rather than taking a reference/ownership to the clock itself.

Peripherals that are clock::v1 compatible are technically clock::v2 compatible since V2 clocks can be converted into clock::v1 types

[supersimple33]

I wrote about this on matrix but gonna add it here as well. I cannot use both the Adc and usb peripheral at the same time because although I can use each in different projects. I cannot create an instance of GenericClockController as required by the usb_allocator since that would move peripherals.gclk which is later needed during the clock_system_at_reset call.

[supersimple33]

Also, a similar thing happens when trying to use timers all together via clock::v2 since the following lines have an obvious error since mclk has been moved when trying to create the timer.

let (mut buses, clocks, tokens) = clock_system_at_reset(
        peripherals.oscctrl,
        peripherals.osc32kctrl,
        peripherals.gclk,
        peripherals.mclk,
        &mut peripherals.nvmctrl,
    );
let (tc2_3, gclk0) = Pclk::enable(tokens.pclks.tc2_tc3, clocks.gclk0);
let mut timer = TimerCounter::tc3_(&tc2_3.into(), peripherals.tc3, &mut peripherals.mclk);

[kyp44]

@supersimple33 Yes this is the fundamental problem with v2 support being only partially implemented in the HAL.

[kyp44]

Just added a table above of all peripherals and their status with respect to v2 migration. There is a lot of low hanging fruit here, and I will probably start issuing PRs. Maintainer question: Is it preferable to have many, small PRs, one for each peripheral, or larger PRs that migrate multiple peripherals at once?

@rnd-ash I have a question. In your update of the ADC and addition of the DAC, you require ownership of the ApbClk, but only a reference to the Pclk, which is not retained. The other v2 peripheral, the CAN, does require full ownership of its Pclk. In the "1:1 clocks" section of the clock::v2 module documentation, it is implied that ownership transfer is required to ensure that the PCLK is not disabled or modified while in use by the peripheral. What is the reason for taking a reference instead?

[rnd-ash]

@kyp44 I'm not sure why this is the case. @jbeaurivage helped with the clockV2 implementation. I don't think there is any specific reason for this, so it could be as simple as a mistake.

[kyp44]

@rnd-ash Gotcha, it seems to me like not taking full ownership of the Pclk structs is a mistake.

Can anyone (@jbeaurivage @ianrrees @bradleyharden ) confirm this?

[ianrrees]

I reckon the peripheral should own the Pclk.

At some stage, we're going to need to think about sleep modes etc, and in that context being able to safely adjust clock trees will be important. I imagine this sort of ownership model will be quite helpful in that.

[kyp44]

@ianrrees Gotcha. Do you have an opinion when it comes to the approach taken by PRs? (i.e. multiple things in one PR or one PR peripheral)

[ianrrees]

Keeping PRs easy to review and maintain is nice, but whatever works. I guess for something like deciding on an approach to use, it might be better to just have one representative peripheral (less friction to change as required). But if it's not a lot of change per peripheral then I've got no problem with doing the same thing to multiple peripherals in one PR.

[supersimple33]

@kyp44 Is my draft #916 roughly in line with what we would want to do for the USB migration I used @rnd-ash's code to guide me.

[rnd-ash]

@kyp44 another thing i thought about. If my understanding of the V2 API is correct, If we want PCLK ownership, then the user can still in the background stop or modify the GCLK that drives the PCLK (As they are independent structures). But, if a peripheral then takes ownership of a GCLK too, then now no other peripheral can use the GCLK.

[kyp44]

@rnd-ash The GCLK cannot be disabled as long as the Pclk exists. The reason is that enabling the Pclk will increment the compile-time consumer counter (the N generic type) for the EnabledGclk, and the disable method is only implemented when the consumer counter is zero, i.e. when it is U0. Users cannot increment or decrement this consumer counter manually with safe Rust. Refer to the Tracking N at compile-time for 1:N clocks section of the clock::v2 module documentation for details about how these counters work. This also allows multiple Pclk to share the same Gclk since enabling each Pclk will increment the EnabledGclk counter by one, ensuring that the Gclk cannot be disabled until all the Pclk are using it are disabled. Note that the I generic on Pclk ensures that only the Source that enabled the Pclk can be used to disable it.

[kyp44]

A couple of larger issues with this effort need to be discussed:

1. I tried to migrate the RTC and its RTIC monotonic, and ran into the same issue that the ADC has, as noted above. The issue is that, to use the v2 API correctly, peripherals requiring a Pclk need to own it, and the Pclk can have different sources via its I: PclkSourceId generic. This means that the peripheral struct itself (e.g. Adc) needs to have the same generic parameter. The RtcOsc is analogous with its I: RtcSourceId generic. The problem is that, on thumbv6 targets, this generic on the peripheral struct and its impl blocks cannot exist (since they currently have no v2 API), leading to requiring separate impl blocks for each chip family, hence potentially duplicated code. This is explained nicely in a comment for the Adc. There are two approaches to solving this problem:

   1. The ADC has a temporary solution of requiring only a &Pclk to construct it, ensuring that the PCLK is enabled at construction time, but allowing it to be disabled later, potentially while the peripheral is still in use. It may or may not be worth implementing this stop-gap solution for other applicable peripherals, though it does make converstion to full ownership of the Pclk relatively easy once the v2 API is available on thumbv6 targets.
   2. Perhaps it would be better to put effort into fast-tracking the porting of the v2 API to thumbv6 targets as started in PR #892, before migrating applicable peripherals.

2. There is another issue with broken BSPs or BSP examples when more than one peripheral is used but their migrations are being done in separate PRs. A good example of this is the nvm_dsu example in the feather_m4 BSP, which uses both the DSU and the USB peripherals. The DSU is migrated in PR #925, and the USB in PR #916. The DSU migration in particular has no choice but to leave this example broken as mentioned in PR #925. I'm not sure what the solution should be to this dilemma, but fixing some examples may need to be its own PR once the peripheral migration PRs are both merged.

In the table above I've noted which peripherals are only on thumbv7 targets, as these do not suffer from issue 1 above, and so are lower-hanging fruit for immediate migration. I have already submitted PR's for the AES and DSU peripherals and intend to work through all of these. The DMAC and WDT are on all chips, but may also be easy to migrate on account of only requiring bus clocks and not PCLKs.

[rnd-ash]

@kyp44 I think 926 is ready to go, then we will have QSPI supporting the Clock V2 API (With some added features)

[ianrrees]

I've noticed in the thumbv6 work that there are some peripheral clocks that are shared between two peripheral instances, eg on SAMD51 TCC0 and TCC1 use the same peripheral clock. How to handle that is going to take some thought - offhand I guess we could provide a way to split these shared clocks in to multiple tokens, so a Tcc0Tcc1 peripheral clock could be split in to a Tcc0 and Tcc1 which could be accepted individually by Tcc ctors, but those couldn't be used to disable the Tcc0Tcc1 combined clock.

[kyp44]

@ianrrees The thumbv7 v2 API seems to handle this by using, for example, clock::v2::types::Tc0Tc1 to represent their Pclk. However, this presents difficulties in using both TC0 and TC1 at the same time since they both would need to own the same Pclk. PR #690 by @glaeqen is out of date now, but updates the timer module, and uses the v2 clock API. The idea seems to be treating the timers as pairs with a primary and secondary timer in each. Each in the pair can be used independently but their relationship as a pair is maintained, for example they both need to be destroyed to free their Pclk.

[ianrrees]

Cheers, thanks for the info @kyp44 ! Definitely seems like a project in its own right, and for after the thumbv6 work is merged.

[rnd-ash]

@kyp44 - We now have a feat/clock-v2 branch for us to merge all the V2 migrations into, so we don't break various examples with a PR dependency chain, would you be able to set your PRs to merge to this branch?

[kyp44]

@rnd-ash It is done. Good idea with the separate branch so that everything can be fixed and done here before merging into master down the road.

[rnd-ash]

Awesome, lets start merging our V2 migrations together and then fix up the examples once everything is swtiched to V2!

I think as the ThumbV6 V2 branch is still untested, we should for now just focus on switching all the Thumbv7 peripherals to clock V2 for a milestone

[kyp44]

@rnd-ash That sounds good. I have not had a lot of time to work on this stuff lately, unfortunately, but I am still working on an easy-to-use USB serial crate. After that's released, I'll move back to migrating the thumbv7 peripherals that remain.