-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clocking API V2 (for thumbv7em) #450
Conversation
5866efb
to
f40598a
Compare
hal/src/thumbv7em/clock/v2/rtc.rs
Outdated
pub enum Output32kOn {} | ||
|
||
impl Sealed for Output32kOn {} | ||
impl Output32k for Output32kOn {} | ||
|
||
pub enum Output1kOn {} | ||
|
||
impl Sealed for Output1kOn {} | ||
impl Output1k for Output1kOn {} | ||
|
||
pub enum Output32kOff {} | ||
|
||
impl Sealed for Output32kOff {} | ||
impl Output32k for Output32kOff {} | ||
|
||
pub enum Output1kOff {} | ||
|
||
impl Sealed for Output1kOff {} | ||
impl Output1k for Output1kOff {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the RTC well at all, so maybe I'm misunderstanding. But it seems like output on/off is not something we typically encode into the type system. Or is on/off really supposed to be enabled/disabled?
hal/src/thumbv7em/clock/v2/rtc.rs
Outdated
//! This is a bit of a hack right now. I think it might be best if the RTC | ||
//! migrates into the `clock` module, since it's so integrated with OSC32KCTRL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it doesn't really conform very well to the rest of the module. But it's fine for now.
run_standby: bool, | ||
on_demand_mode: bool, | ||
start_up_masking: StartUp32k, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example. This struct grew by at least 3 bytes, because it has to store the state until the enable
function.
X: Output32k, | ||
Y: Output1k, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't this previously a run-time parameter, not compile-time? What's the motivation for this change? Maybe I just don't understand the Xosc32k
very well?
|
||
/// TODO | ||
#[inline] | ||
pub fn set_1k_output(mut self, enabled: bool) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, maybe you can't enable both 1k and 32k output simultaneously? That would make sense to prevent at compile-time.
$tokens:expr | ||
) => {{ | ||
let (gclk0, gclk5, dpll0, dfll) = | ||
crate::clocking_preset_gclk0_120mhz_gclk5_2mhz!($gclk0, $dfll, $tokens); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need $crate
instead of crate
. The former will always resolve to the crate in which it is written.
/// ); | ||
/// ``` | ||
#[macro_export] | ||
macro_rules! clocking_preset_gclk0_120mhz_gclk5_2mhz_gclk1_external_32khz { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the macro approach, but I hate the names. I don't know how to make them any better though, because you can't have macro namespaces. Bleh 🤮
a3fdfea
to
07b1883
Compare
04cb9a4
to
3673837
Compare
4d24a74
to
64242d2
Compare
1ac02fa
to
bddd31f
Compare
ff8fa93
to
447f5db
Compare
Is it possible to reset all clocks? Otherwise this is quite problematic I think, because most(?) bootloaders will setup the clocks in some way. |
Yes and no. Gclks expose HW register for SWRST. The rest is hit or miss. We had a conversation about this with @bradleyharden , solutions heavily depend on circumstances. If one knows a clock state before reaching I came up with this custom ghetto solution that tries its best to restore default state in a correct order. It is impossible to restore everything to the default state, as there are some register that are populated by MCU with factory settings (like fine and coarse params for some oscillators and whatnot). In my approach I kinda ignore that and assume that user didn't mess with these too much. //! Module containing clock system related utilities that go beyond of
//! [`atsamd_hal::clock::v2`] scope
use atsamd_hal::pac::{GCLK, MCLK, OSC32KCTRL, OSCCTRL};
/// Function implementing HW clocking reset (with some caveats).
///
/// In most cases this step is redundant. However, [`retrieve_clocks`] might be
/// unsound in scenarios when there is no full software reset between
/// application runs (eg. bootloader jumping to application or to secondary
/// bootloader). Enforcing a default state of a clocking system improves overall
/// soundness of an API.
///
/// Notes:
/// - If `Wrtlock` And Peripheral Access Controller is engaged, this procedure
/// might not be enough.
/// - Some parameters cannot be restored, eg. fine and coarse values for `Dfll`
/// running in an open mode. They are set to factory values after reset; if
/// user has changed them there is no way to restore them (is that for
/// certain?).
/// - Resetting Rtc source (to 1kHz UlpOsc32k) might corrupt Rtc operation if it
/// uses a source of different frequency AND it is supposed operate
/// continuously regardless of a switch from BL to APP/SBL.
pub unsafe fn hard_reset_clock_system(
oscctrl: &mut OSCCTRL,
osc32kctrl: &mut OSC32KCTRL,
gclk: &mut GCLK,
mclk: &mut MCLK,
) {
// Reset main clock
mclk.cpudiv.reset();
mclk.ahbmask.reset();
mclk.apbamask.reset();
mclk.apbbmask.reset();
mclk.apbcmask.reset();
mclk.apbdmask.reset();
mclk.intenclr.reset();
mclk.intenset.reset();
mclk.intflag.reset();
// Reset Dfll
oscctrl.dfllctrla.reset();
while oscctrl.dfllsync.read().enable().bit_is_set() {}
oscctrl.dfllctrlb.reset();
while oscctrl.dfllsync.read().dfllctrlb().bit_is_set() {}
//// Note:
//// DFLLVAL contains FINE and COURSE values that come from factory
//// If user managed to set these in previous application run, there's no easy
//// way of resetting them
// oscctrl.dfllval.reset();
// while oscctrl.dfllsync.read().dfllval().bit_is_set() {}
oscctrl.dfllmul.reset();
while oscctrl.dfllsync.read().dfllmul().bit_is_set() {}
// Reset Gclks and Pclks
gclk.ctrla.write(|w| w.swrst().set_bit());
while gclk.ctrla.read().swrst().bit_is_set() || gclk.syncbusy.read().swrst().bit_is_set() {}
// Reset Dplls
oscctrl.dpll.iter().for_each(|dpll| {
dpll.dpllctrla.reset();
while dpll.dpllsyncbusy.read().enable().bit_is_set() {}
dpll.dpllratio.reset();
while dpll.dpllsyncbusy.read().dpllratio().bit_is_set() {}
dpll.dpllctrlb.reset();
});
// Reset Xoscs
oscctrl.xoscctrl.iter().for_each(|xosc| {
xosc.reset();
});
// Interrupts and miscellaneous
oscctrl.evctrl.reset();
oscctrl.intenclr.reset();
oscctrl.intenset.reset();
oscctrl.intflag.reset();
// Reset Xosc32ks
osc32kctrl.xosc32k.reset();
// Cannot just reset, CALIB[5:0] is non zero
osc32kctrl
.osculp32k
.write(|w| w.en1k().set_bit().en32k().set_bit());
osc32kctrl.evctrl.reset();
osc32kctrl.cfdctrl.reset();
osc32kctrl.rtcctrl.reset();
osc32kctrl.intenclr.reset();
osc32kctrl.intenset.reset();
osc32kctrl.intflag.reset();
} Not sure if we want something like this in a HAL though. Maybe you or others have better idea how to tackle this. Probably the best option would be to introduce this dynamic clock layer to equation ( |
Invert the way instances of `GclkOut` are created. Create them by feeding a `Pin` to an `EnabledGclk` rather than using `GclkOut::enable`.
- Store the `Pin` for the GCLK_IN case and remove `unsafe` conjuring - Consolidate into a `Settings` struct and minimize the number of register writes during configuration
852801e
to
5f3caa5
Compare
For the moment, I don't think we should do anything other than merge
Agreed. Before we deprecate or remove anything, I think we need to port it to thumbv6m. I have no idea how much work that would be, though. I haven't looked at the datasheet in any depth. |
It has been over two years since my initial issue and over a year and a half since @glaeqen's issue. Moreover, this PR has been open for well over a year, and several people are working directly from this branch (including myself, @glaeqen, @sakian and probably others). With two maintainer approvals and a green CI, I think it's time we finally merged. 🚀 |
Shout out to @AfoHT aka @vcchtjader as well, considering his invaluable contribution at multiple stages of this PR. Great job everyone! |
I think we are doing a squash merge, right? That means we should have a reasonable commit message. |
I'm writing it as we speak. |
Add a new API for the `clock` module on `thumbv7em` targets Add a new `clock` module API that enforces correctness by construction. It is impossible to create an invalid clock tree without using `unsafe`. Specifically, use typed tokens and clocks to guarantee memory safety and act as proof that a clock is correctly configured, and use type-level numbers to count consumer clocks at compile-time and restrict a given clock's API while it is in use. This commit comes after two years of work, starting with #272, then with #429 and culminating with PR #450. Co-authored-by: Bradley Harden <[email protected]> Co-authored-by: Gabriel Górski <[email protected]> Co-authored-by: Henrik Tjäder <[email protected]>
Summary
New clocking API allowing to build a typesafe chain of dependent clock sources, generic clock generators and peripheral channels.
Motivation
Current clocking API assumes the following workflow:
This API has the following limitations.
GenericClockController
constructor configures and provides an opinionated set of clock sources and generic clock generators that cannot be changed afterwards. Following clock configuration is set forthumbv7em
.GCLK0
(Generic Clock Generator 0) is driven byDFLL48M
(48MHz) throughGCLK5
(divider: 24 -> 2MHz) throughPclk1
(Peripheral Channel 1) throughDPLL0
(multiplier: 60 -> 120MHz)GCLK1
is driven either byXOSC32K
(use_external_32kosc) orOSCULP32K
(use_internal_32kosc)Clock Source - GCLK
pair, it is impossible to change it later.GCLK - PeripheralChannel
pair, it is impossible to change it later.Main clock locked to 120MHz by HAL, even though being acceptable in basic scenarios, is a serious design flaw that severely diminishes flexibility of a HAL and might either encourage users to hack unsafe solutions on top of existing abstractions or discourage them from using the HAL altogether.
Having these points in mind and also the fact that current clocking API implementation would benefit from refactoring anyway, striving for improvement seems to be fully motivated and justified.
Detailed design
Introduction
ATSAMD clocking system assumes 3 types of major building blocks:
ClkSrc
)Gclk
)Pclk
)Properties of ATSAMD clocking system:
Gclk
s depend onClkSrc
s in aN:1
relationshipPclk
s depend onGclk
s in aM:1
relationshipClkSrc
s can depend on somePclk
sSpecifically:
thumbv7em
-based MCUsPclk0
can serve as a reference clock provider forClkSrc:Dfll48M
Pclk1
can serve as a reference clock provider forClkSrc:Dpll0
Pclk2
can serve as a reference clock provider forClkSrc:Dpll1
thumbv6m
-based MCUsPclk0
(onthumbv6m
Peripheral Channels are called GCLK Multiplexers) can serve as a reference forClkSrc:Dfll48M
ClkSrc
can depend on some otherClkSrc
sSpecifically
thumbv7em
-based MCUsClkSrc:Xosc32K
can serve as a clock source forClkSrc:Dpll{0, 1}
ClkSrc:Xosc{0, 1}
can serve as a clock source forClkSrc:Dpll{0, 1}
Mapping HW model to Rust
In order to model one-to-many type of relationship between dependencies a few additional concepts/abstractions were introduced.
Enabled
type and its helper types/traitsEnabled
type wrapper represents a clocking component in its enabled state while also holding an information about current amount of dependencies (usually0
upon construction). This amount of dependencies is embedded into the type via second generic parameter leveragingtypenum::{UInt, UTerm}
types.Via specialized implementation blocks for this type (like for
Enabled<Gclk<G, H, U0>
) it is possible to introduce special behaviour; e.g.fn disable
will only exist for clocking component havingU0
current users.SourceMarker
trait and its subtraitsThis marker trait unifies family of more specific traits. These ones are essential during a construction
fn ::{new, enable}
and deconstructionfn ::{free, disable}
of clocking components as they provide information to the constructed/deconstructed type what its source is (shown in the example later) and which variant of source (associated constant) is applicable while performing a HW register write.These are implemented by marker types corresponding to existing clocking abstractions e.g.:
Source
trait and its subtraitsThis trait represents a source of clocking signal while subtraits its more specialized flavours (source of signal for Dpll, Pclk, Gclk, etc.).
These are implemented by corresponding specialized types of
Enabled
e.g.:Regardless of how complicated it might seem to look, it can be roughly understood as: Enabled
Dpll
peripheral can be a source of signal for any Gclk.*Token
typesUnfortunately,
PAC::Peripherals
granularity is too low for them to be useful when spliting clocking system into such small, semi-independent pieces. In order to solve this problem, we consume PAC at the very beginning and return a set of tokens that internally use appropriateRegisterBlock
directly, retrieved from a raw pointer . It is safe because register regions managed by different tokens do not overlap. Tokens cannot be created by a user; they are provided during initialization and do not expose any public API. Memory accesses are read/write-synchronized (Chapter 13.3; p. 138).Source/SourceMarker
traits usage in an APIThis is a slightly simplified example of how more less every clocking component that relies on one-to-many depenedency relationships is implemented
Gclk::new
consumes, upon construction, aGclkSource
provided to it and returns it with a type ofEnabled<_, N++>
(as mentioned previously, specializations ofEnabled
implementSource
based traits). Analogically,Gclk::free
consumes aGclkSource
passed in and returns it with a new type ofEnabled<_, N-->
. By design it is impossible to go below0
, because there is always less or equal amount of users than a counter value.Gclk0
caseAmount of users might be less than a value of a counter in case of special types like
Gclk<Gen0>
which always has an implicit single user -- synchronous clocking domain. Minimal amount of users for it is1
, making it impossible to disable and therefore consistent with its documented HW characteristics.It also makes it impossible to change a configuration of a
Gclk0
as aEnabled<Gclk0, _>
cannot be deconstructed. Therefore,Enabled<Gclk0, U1>
exposes additional methods that are usually available only for disabledGclk
s.Usage from the HAL perspective
Peripheral Y associated with
Pclk<Y, _>
will just consume it upon construction.Thus, clocking tree that is relevant for HAL peripheral X is locked and has to be released step by step. It is worth noting, that only that part of clocking tree is locked so the granularity is quite fine.
V1 clocking API compatibility
In order to support v1 clocking compatible peripherals, every
clock::v1::*Clock
object implements aFrom<Pclk<_, _>>
trait. Therefore, it is feasible to use existing peripheral implementations with new clocking API.To make a migration simplier, there're going to be standalone functions returning available clocking presets that were available in v1 clocking.
Handling of default clocking system state
fn crate::clock::v2::retrieve_clocks
returns a tuple of following typewhich is supposed represent a default state of clocking system after reset (Chapter 13.7; p. 141). As mentioned before
Gclk0
is owned by virtual single user representing CPU and synchronous clocking domain and count of1
cannot be decreased further.What's still missing?
fn ::{new,free,<setter>,<getter>}
kind of methods; only infn ::{enable,disable}
fn ::{enable,disable}
fn ::{enable,disable}
fn ::{enable,disable}
fn ::{enable,disable}
GclkOutSource
trait approachfn ::{enable,disable}
fn ::{enable,disable,activate_*}
Enabled
type > @vcchtjader <fn ::{enable,disable}
fn ::{enable,disable,activate_*}
fn ::{enable,disable}
cargo test --doc
). Maybe we should postpone it until we have BSC-less examples crate figured out? @bradleyharden @vcchtjaderSource
andSourceMarker
traits#[inline]
is applied where it makes sensefunctionsmacros replicating clocking presets available in v1 clockingReferences
If there's a reference to an external document with a specified page, it is implicit reference to
SAM D5x/E5x Family Data Sheet
(DS60001507F
) in a context ofthumbv7em
based MCUs.For
thumbv6m
-based MCU, it is a reference toSAM D21/DA1 Family Data Sheet
(DS40001882F
).Related issues
Closes #114