HAL: Add PMC, EFC configuration APIs #16

tmplt · 2022-04-22T12:00:58Z

Based upon @jamesmunns work, described in #14, but heavily refactored.

Closes #5.

tmplt · 2022-04-22T12:03:50Z

So far defmt usage has been removed/commented out. I'll go over the implementation in detail and likely apply some refactoring and generalization work so the implementation works for my use-case.

tmplt · 2022-04-22T13:56:07Z

Depends on #15.

jamesmunns · 2022-04-22T14:22:14Z

You might want to also take jamesmunns/same70-experiments@58a3c34, which fixed a couple things I found through testing.

Sourced from [0]; /vendor/asamx7x-hal. [0] jamesmunns/same70-experiments@58a3c34

tmplt · 2022-04-22T14:28:02Z

Amended, thanks.

jamesmunns · 2022-04-22T15:09:01Z

hal/src/lib.rs

@@ -145,3 +145,7 @@ pub use atsamv71q21b as target_device;
 pub mod serial;
 #[cfg(feature = "rev-b")]
 pub mod watchdog;
+#[cfg(feature = "rev-b")]


If you haven't seen the cfg-if crate, it's pretty handy for avoiding wordy cfgs like this.

michalfita · 2022-04-22T15:45:49Z

Thanks @jamesmunns for the hint. From what I remember all MCUs in this series share peripherals implementations, only register allocation and number of instances varies.

Thanks @tmplt for bringing this in. I'll review over the weekend.

More features will be added in future commits that require explicit configuration. Moved to build.rs to remove noise from HAL implementation.

tmplt · 2022-04-25T13:58:15Z

I have briefly looked over the current PMC API and considered some changes, inspired by the in-progress atsamd clocking API. Instead of a monolithic set_clocks I think it would be beneficial if each clock is configured seperately. When configuring the clocks the hierarchy in Fig. 31-1 is likely to be consulted first; an equivalent API should make it easier to apply and debug any arising issues (i.e. being able to localize it to a single statement instead of requiring the end-user to dig into the HAL).

Here's is a draft of such an API application (not finalized):

    let pmc = pmc::new(device.PMC);

    // fundamental clock configuration (HCLK, FCLK, MCK)
    let mainck = pmc.get_mainck(MainClkConfig::Internal(12.to_Hz()))?;
    let plla = pmc.get_plla(PllaConfig { source: mainck, div: 1, mult: 4 })?;
    let (hclk, mck) = pmc.get_clocks(ClocksConfig { source: plla, pres: ClocksPrescaler::Pres4, div: MckDiv::Pres1 });

    // peripheral clock configuration
    let pck4 = pmc.get_peripheral(4, PeriphClkConfig { source: plla, pres: 1 })?;

hal/Cargo.toml

device-selected is a meta-feature that is not described in §2.

hal/build.rs

This change supports the theoretical case of non-constant VDDIO (for E70/S70). An enum is also a much stronger association, rather than an environmental variable (feature).

That is, so the crate does not build if only device-selected is enabled.

…into feat/pmc

We will be utilizing this information later down the line, for example when configuring peripheral baud rates.

UPLLCKDIV is derived from UPLLCK by either /1 or /2. Only the divider is forwarded to HCLK and PCK, as per §31.3.

Some clocks will emit MHz (UPLLCK, MAINCK) while others will emit Hz (PLLA - depending on dev/mult, and SLCK).

This will later be moved into a proper examples/*.rs when a GPIO API is available.

tmplt · 2022-05-30T13:21:34Z

This is now ready for review. The documented example (which is a copy of the added atsame70_xpro example) has been tested on hardware. I also tested UPLL configuration, which unfortuately failed. I'll be looking into that in the meantime.

The module can probably be improved by some macro application, but I cannot prioritize that at the moment. More safety can also be added, such as ensuring that a clock that is in used it not turned off, but that too must wait for my part.

tmplt · 2022-05-30T13:50:51Z

I must have spoken too soon: atsame70_xpro has been modified to enable the UPLL and expose it on PA03 via PCK2 @ 2.4MHz. Works as expected.

michalfita

Looks good in overall, but I have a few suggestions if you don't mind.

michalfita · 2022-05-06T21:52:50Z

hal/src/pmc.rs

+}
+
+impl PeripheralIdentifier {
+    pub fn supports_pmc_clocking(&self) -> Result<(), ()> {


Why Result<> and why not const?

michalfita · 2022-06-01T13:15:21Z

Cargo.lock

+[[package]]
+name = "fugit"
+version = "0.3.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f6d8595783d5ca52f0e9830036b3d24f359fae0fcc6bb5fde41f2dd82997cb58"
+dependencies = [
+ "gcd",
+]
+
+[[package]]
+name = "gcd"
+version = "2.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f37978dab2ca789938a83b2f8bc1ef32db6633af9051a6cd409eff72cbaaa79a"
+dependencies = [
+ "paste",


I always thought the rule in Rust world is not to commit Cargo.lock for library crates.

Somehow slipped through; I'll remove it.

michalfita · 2022-06-01T13:17:48Z

hal/Cargo.toml

+# Refer to §2 in the data sheet.
+## Refer to §2.


? Why refer doubled here?

Good catch.

michalfita · 2022-06-01T13:18:26Z

boards/atsame70_xpro/src/main.rs

+        // Configure the clock hierarchy
+        {
+            use hal::pmc::{
+                HostClockConfig, MainCkSource, MckDivider, MckPrescaler, Megahertz, PckId,
+                PllaConfig, Pmc, UpllDivider,
+            };
+
+            let mut pmc = Pmc::new(ctx.device.PMC);
+            let mainck = pmc
+                .get_mainck(MainCkSource::ExternalBypass(Megahertz::from_raw(12)))
+                .unwrap();
+            let _plla = pmc
+                .get_pllack(PllaConfig { div: 1, mult: 8 }, &mainck)
+                .unwrap();
+            let _hclk = pmc
+                .get_hclk(
+                    HostClockConfig {
+                        pres: MckPrescaler::CLK_1,
+                        div: MckDivider::EQ_PCK,
+                    },
+                    &mainck,
+                    &mut efc,
+                )
+                .unwrap();
+
+            let upllck = pmc.get_upllck(&mainck, &mut ctx.device.UTMI).unwrap();
+            let upllckdiv = pmc.get_upllckdiv(&upllck, UpllDivider::Div2);
+            let _pck2 = pmc.get_pck(&upllckdiv, 100 - 1, PckId::Pck2); // @ 2.4MHz
+        }


Wouldn't be more readable if these are separate functions?

I'm not sure what you mean. What functions here could be further separated?

The whole function is long but seems to have clear separation into stages. To improve readability they could be shorter private functions with descriptive names.

michalfita · 2022-06-01T13:27:26Z

hal/src/efc.rs

+pub enum VddioLevel {
+    /// VDDIO = 3.3V, typical
+    V3,
+    /// VDDIO = 1.7V, minimal
+    V1,
+}


Wouldn't V3v3 and V1v7 more accurate in places where code is read?

Good idea; those are much better names.

michalfita · 2022-06-01T13:29:10Z

hal/src/efc.rs

+    pub fn calculate(freq: Megahertz, vddio: &VddioLevel) -> Result<Self, PmcError> {
+        #[cfg(any(feature = "v70", feature = "v71"))]
+        if vddio == &VddioLevel::V1 {
+            // V70/V71 must be driven with VDDIO = 3.3V, typical
+            return Err(PmcError::InvalidConfiguration);
+        }
+
+        Self::fws_from_freq(freq, vddio)
+    }


Shouldn't we prevent the invalid configuration at compile time?

We should. I had not played around with the type system before I began work on #18. I'll review this module and see what can be made static.

michalfita · 2022-06-01T13:36:43Z

hal/src/pmc.rs

+                // TODO hande note for MOSCRCF unhandled (p. 276;
+                // first table, second row)


Can I suggest one of commercial practices that I like? Turn each TODO into an issue, then point the issue ID with the comment like: // TODO #333: Handle note for MOSCRF - I've learnt TODO comments quickly gets neglected and later in time confuse other contributors. With issue in place there's more urge to actually do the stuff.

Yep, let's do that.

michalfita · 2022-06-01T13:36:54Z

hal/src/pmc.rs

+                });
+                while self.pmc.pmc_sr.read().moscsels().bit_is_clear() {}
+
+                // TODO check MAINCK frequency (§31.17; step 5).


michalfita · 2022-06-01T13:38:27Z

hal/src/pmc.rs

+    /// This method corresponds to Step 6 of 31.17 Recommended Programming Sequence.
+    pub fn get_pllack<SRC: PllaSource>(


Would be nice to equip docs of these API functions with some examples?

Do you mean an example for each get_-function?

michalfita · 2022-06-01T13:38:37Z

hal/src/pmc.rs

+        if div == 0 || div > 127 {
+            return Err(PmcError::InvalidConfiguration);
+        }
+        // TODO: Ensure valid requested output frequency.


tmplt · 2022-06-20T15:22:22Z

Thanks for the review, @michalfita. I am busy with internal development of this HAL at the moment, but aim to continue the upstreaming process here when time is available. It can take a while until then.

michalfita · 2022-06-21T09:27:02Z

It can take a while until then.

You @martinmortsell and @jamesmunns are probably only users of this work ATM, so it's fine. However, it would be nice to release it at some point.

tmplt · 2022-09-09T16:47:57Z

Branch is now deprecated. Closing. Refer to #24.

Before this commit other pin bits were not preserved. Closes atsams-rs#16.

hal/pio: correctly modify ABCD registers on Pin::into_peripheral Closes atsams-rs#16 See merge request embedded-rust/atsamx7x-hal!5

manifest: increase internal feature flag granularity

4addd5d

Closes atsams-rs#11

add pmc, efc modules from @jamesmunns's work

c552e47

Sourced from [0]; /vendor/asamx7x-hal. [0] jamesmunns/same70-experiments@58a3c34

tmplt force-pushed the feat/pmc branch from 9379916 to c552e47 Compare April 22, 2022 14:27

jamesmunns reviewed Apr 22, 2022

View reviewed changes

tmplt changed the title ~~HAL: Add PMC, ETC configuration APIs~~ HAL: Add PMC, EFC configuration APIs Apr 22, 2022

move feature checking to a build.rs instead

1c8fcd2

More features will be added in future commits that require explicit configuration. Moved to build.rs to remove noise from HAL implementation.

martinmortsell reviewed Apr 25, 2022

View reviewed changes

hal/Cargo.toml Outdated Show resolved Hide resolved

tmplt force-pushed the feat/pmc branch from bd0c9a1 to e869eed Compare April 25, 2022 14:09

tmplt added 2 commits April 25, 2022 16:09

efc: find FWS via TryFrom impls

d7e0f57

hal: apply fmt

7c5d8b6

tmplt force-pushed the feat/pmc branch from e869eed to 7c5d8b6 Compare April 25, 2022 14:09

features: separate device-selected master feature from MCU info

ecb51b5

device-selected is a meta-feature that is not described in §2.

tmplt commented Apr 25, 2022

View reviewed changes

hal/build.rs Show resolved Hide resolved

martinmortsell and others added 11 commits April 26, 2022 07:27

Add Voltage Level Checks To build.rs

d77f43e

Fix Return Types

83de611

Add get_ Methods For All The Clocks (Except SLCK)

3f1f25f

efc: set VDDIO level via ctor instead of feature

762e919

This change supports the theoretical case of non-constant VDDIO (for E70/S70). An enum is also a much stronger association, rather than an environmental variable (feature).

build: ensure a chip feature is enabled

be19501

That is, so the crate does not build if only device-selected is enabled.

pmc: Add Methods For Configuring Plla And Mck

e42837e

Merge branch 'feat/pmc' of https://github.com/GrepitAB/atsams70-rust …

5d2dd11

…into feat/pmc

pmc: Add Non-functional External Oscillator Support

d8edfe1

pmc: Fix Main Clock Selection

daa8a2c

pmc: Add PCK Configuration

9faca53

Update Documentation

14660e7

tmplt added 9 commits May 25, 2022 16:56

hal/pmc: record clock freq for MAINCK, PLLACK, and HCLK

0b5694b

We will be utilizing this information later down the line, for example when configuring peripheral baud rates.

hal/pmc: get_hclk: set EFC wait states before switching clocks

c47096b

hal/pmc: correctly configure UPLLCK and record its freq

451e3c1

hal/pmc: implement get_upllckdiv

22c9964

UPLLCKDIV is derived from UPLLCK by either /1 or /2. Only the divider is forwarded to HCLK and PCK, as per §31.3.

hal/pmc: use Hertz instead of Megahertz when logical

e035fae

Some clocks will emit MHz (UPLLCK, MAINCK) while others will emit Hz (PLLA - depending on dev/mult, and SLCK).

hal/pmc: record SLCK freq

2d4389c

hal/pmc: remove unnecessary Results

5adcf9f

hal/pmc: handle unused_variables/dead_code warnings

dbe39b1

atsame70_xpro: apply example clock hierarchy configuration

b88ffd6

This will later be moved into a proper examples/*.rs when a GPIO API is available.

tmplt marked this pull request as ready for review May 30, 2022 12:56

tmplt added 4 commits May 30, 2022 15:10

hal/pmc: improve struct/enum docs

27f5bc0

hal/pmc: remove unused enum

0e6a9c5

hal/pmc: get_slck: remove impl details from docs

dd1a9dc

hal/pmc: improve top-level docs

009d478

atsame70_xpro: expose UPLLCKDIV on PCK2, @ 2.4MHz

f74507d

tmplt added 4 commits May 31, 2022 13:54

hal/pmc: globalize magic values

82a87de

hal/pmc: apply formatting

828299f

hal/pmc: refactor SCLK configuration to match MAINCK

b89177d

hal/pmc: remove outdated comment

d249979

tmplt mentioned this pull request May 31, 2022

HAL: add PIO configuration API #18

Closed

michalfita reviewed Jun 1, 2022

View reviewed changes

michalfita mentioned this pull request Jul 26, 2022

Join efforts ju6ge/atsamx7x-hal#1

Open

tmplt closed this Sep 9, 2022

tmplt pushed a commit to GrepitAB/atsams70-rust that referenced this pull request Sep 9, 2022

hal/pio/reg: into_peripheral: correctly set/clear abcdsr

6c0e369

Before this commit other pin bits were not preserved. Closes atsams-rs#16.

tmplt added a commit to GrepitAB/atsams70-rust that referenced this pull request Sep 9, 2022

Merge branch 'fix/pio_into_peripheral' into 'master'

e28517b

hal/pio: correctly modify ABCD registers on Pin::into_peripheral Closes atsams-rs#16 See merge request embedded-rust/atsamx7x-hal!5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HAL: Add PMC, EFC configuration APIs #16

HAL: Add PMC, EFC configuration APIs #16

tmplt commented Apr 22, 2022 •

edited

Loading

tmplt commented Apr 22, 2022

tmplt commented Apr 22, 2022

jamesmunns commented Apr 22, 2022

tmplt commented Apr 22, 2022

jamesmunns Apr 22, 2022

michalfita commented Apr 22, 2022

tmplt commented Apr 25, 2022

tmplt commented May 30, 2022

tmplt commented May 30, 2022

michalfita left a comment

michalfita May 6, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 21, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 1, 2022

michalfita Jun 1, 2022

tmplt Jun 2, 2022

michalfita Jun 21, 2022

michalfita Jun 1, 2022

tmplt commented Jun 20, 2022

michalfita commented Jun 21, 2022

tmplt commented Sep 9, 2022

		// TODO hande note for MOSCRCF unhandled (p. 276;
		// first table, second row)

		/// This method corresponds to Step 6 of 31.17 Recommended Programming Sequence.
		pub fn get_pllack<SRC: PllaSource>(

HAL: Add PMC, EFC configuration APIs #16

HAL: Add PMC, EFC configuration APIs #16

Conversation

tmplt commented Apr 22, 2022 • edited Loading

tmplt commented Apr 22, 2022

tmplt commented Apr 22, 2022

jamesmunns commented Apr 22, 2022

tmplt commented Apr 22, 2022

Choose a reason for hiding this comment

michalfita commented Apr 22, 2022

tmplt commented Apr 25, 2022

tmplt commented May 30, 2022

tmplt commented May 30, 2022

michalfita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmplt commented Jun 20, 2022

michalfita commented Jun 21, 2022

tmplt commented Sep 9, 2022

tmplt commented Apr 22, 2022 •

edited

Loading