`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 RTC peripheral can be used without configuring the RTC clock #642 discusses that the RTC can be used without configuring the clock, which is a known issue that was discussed further in PR Fixes problems with the rtc::Rtc abstraction and refactors to use the new rtc::modes abstractions. #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 Clock v2 thumbv6m #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.