Skip to content
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

Add clock measurement support, fix VCO min freq & add GPin0/1 clock source support. #683

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
11 changes: 11 additions & 0 deletions rp2040-hal/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## Fixed

- Fixed minimum PLL's VCO frequency according to updated datasheet - @ithinuel

### Added

- Support for GPin0 and GPin1 clock sources - @ithinuel
- Clock frequency approximation function using the frequency counter - @ithinuel

## [0.9.0]

### MSRV
Expand Down
50 changes: 43 additions & 7 deletions rp2040-hal/src/clocks/clock_sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use pac::{PLL_SYS, PLL_USB};
pub(crate) type PllSys = PhaseLockedLoop<Locked, PLL_SYS>;
impl Sealed for PllSys {}
impl ClockSource for PllSys {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::PLL_SYS_CLKSRC_PRIMARY;

fn get_freq(&self) -> HertzU32 {
self.operating_frequency()
}
Expand All @@ -24,36 +26,48 @@ impl ClockSource for PllSys {
pub(crate) type PllUsb = PhaseLockedLoop<Locked, PLL_USB>;
impl Sealed for PllUsb {}
impl ClockSource for PllUsb {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::PLL_USB_CLKSRC_PRIMARY;

fn get_freq(&self) -> HertzU32 {
self.operating_frequency()
}
}

impl ClockSource for UsbClock {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLK_USB;

fn get_freq(&self) -> HertzU32 {
self.frequency
}
}

impl ClockSource for AdcClock {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLK_ADC;

fn get_freq(&self) -> HertzU32 {
self.frequency
}
}

impl ClockSource for RtcClock {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLK_RTC;

fn get_freq(&self) -> HertzU32 {
self.frequency
}
}

impl ClockSource for SystemClock {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLK_SYS;

fn get_freq(&self) -> HertzU32 {
self.frequency
}
}

impl ClockSource for ReferenceClock {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLK_REF;

fn get_freq(&self) -> HertzU32 {
self.frequency
}
Expand All @@ -62,6 +76,8 @@ impl ClockSource for ReferenceClock {
pub(crate) type Xosc = CrystalOscillator<Stable>;
impl Sealed for Xosc {}
impl ClockSource for Xosc {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::XOSC_CLKSRC;

fn get_freq(&self) -> HertzU32 {
self.operating_frequency()
}
Expand All @@ -71,23 +87,43 @@ pub(crate) type Rosc = RingOscillator<Enabled>;
impl Sealed for Rosc {}
// We are assuming the second output is never phase shifted (see 2.17.4)
impl ClockSource for RingOscillator<Enabled> {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::ROSC_CLKSRC;

fn get_freq(&self) -> HertzU32 {
self.operating_frequency()
}
}

// GPIN0
pub(crate) type GPin0<M = PullNone> = Pin<Gpio20, FunctionClock, M>;
/// Gpio20 in clock function associated with a frequency.
pub struct GPin0<M: PullType = PullNone>(Pin<Gpio20, FunctionClock, M>, HertzU32);
impl<M: PullType> GPin0<M> {
/// Assemble Gpio20 and a frequency into a clock source.
pub fn new(p: Pin<Gpio20, FunctionClock, M>, freq: HertzU32) -> Self {
GPin0(p, freq)
}
}
impl<M: PullType> Sealed for GPin0<M> {}
impl<M: PullType> ClockSource for GPin0<M> {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLKSRC_GPIN0;

fn get_freq(&self) -> HertzU32 {
todo!()
self.1
}
}

/// Gpio22 in clock function associated with a frequency.
pub struct GPin1<M: PullType = PullNone>(Pin<Gpio22, FunctionClock, M>, HertzU32);
impl<M: PullType> GPin1<M> {
/// Assemble Gpio22 and a frequency into a clock source.
pub fn new(p: Pin<Gpio22, FunctionClock, M>, freq: HertzU32) -> Self {
GPin1(p, freq)
}
}
impl<M: PullType> Sealed for GPin1<M> {}
impl<M: PullType> ClockSource for GPin1<M> {
const FCOUNTER_SRC: FC0_SRC_A = FC0_SRC_A::CLKSRC_GPIN1;

// GPIN1
pub(crate) type GPin1<M = PullNone> = Pin<Gpio22, FunctionClock, M>;
impl<M: PullType> ClockSource for Pin<Gpio22, FunctionClock, M> {
fn get_freq(&self) -> HertzU32 {
todo!()
self.1
}
}
48 changes: 46 additions & 2 deletions rp2040-hal/src/clocks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
//! See [Chapter 2 Section 15](https://datasheets.raspberrypi.org/rp2040/rp2040_datasheet.pdf) for more details
use core::{convert::Infallible, marker::PhantomData};
use fugit::{HertzU32, RateExtU32};
use pac::clocks::fc0_src::FC0_SRC_A;

use crate::{
pac::{self, CLOCKS, PLL_SYS, PLL_USB, RESETS, XOSC},
Expand All @@ -79,9 +80,9 @@ use crate::{
mod macros;
mod clock_sources;

use clock_sources::PllSys;
pub use clock_sources::{GPin0, GPin1};

use self::clock_sources::{GPin0, GPin1, PllUsb, Rosc, Xosc};
use clock_sources::{PllSys, PllUsb, Rosc, Xosc};

#[derive(Copy, Clone)]
/// Provides refs to the CLOCKS block.
Expand Down Expand Up @@ -167,6 +168,9 @@ pub trait StoppableClock: Sealed {

/// Trait for things that can be used as clock source
pub trait ClockSource: Sealed {
/// Associated Frequency counter source.
const FCOUNTER_SRC: FC0_SRC_A;

/// Get the operating frequency for this source
///
/// Used to determine the divisor
Expand Down Expand Up @@ -301,6 +305,46 @@ impl ClocksManager {
.configure_clock(&self.system_clock, self.system_clock.freq())
}

/// Approximates the frequency of the given clock source.
pub fn approximate_frequency<C: ClockSource>(&mut self, _trg_clk: C) -> HertzU32 {
// Wait for the frequency counter to be ready
while self.clocks.fc0_status.read().running().bit_is_set() {
core::hint::spin_loop()
}

// Set the speed of the reference clock in kHz.
self.clocks.fc0_ref_khz.write(|w| unsafe {
w.fc0_ref_khz()
.bits(self.reference_clock.get_freq().to_kHz())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious if this is safe: fc0_ref_khz only has 20 non-reserved bits, so writing a larger value might be undefined.

});

// Corresponds to a 1ms test time, which seems to give good enough accuracy
ithinuel marked this conversation as resolved.
Show resolved Hide resolved
self.clocks
.fc0_interval
.write(|w| unsafe { w.fc0_interval().bits(10) });

// We don't really care about the min/max, so these are just set to min/max values.
self.clocks
.fc0_min_khz
.write(|w| unsafe { w.fc0_min_khz().bits(0) });
self.clocks
.fc0_max_khz
.write(|w| unsafe { w.fc0_max_khz().bits(0xffffffff) });

// To measure rosc directly we use the value 0x03.
ithinuel marked this conversation as resolved.
Show resolved Hide resolved
self.clocks
.fc0_src
.write(|w| w.fc0_src().variant(C::FCOUNTER_SRC));

// Wait until the measurement is ready
while self.clocks.fc0_status.read().done().bit_is_clear() {
core::hint::spin_loop()
}

let speed_hz = self.clocks.fc0_result.read().khz().bits() * 1000;
speed_hz.Hz()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few error bits in FC0_STATUS. This method could check those bits and return an error if any are set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, however it is unclear to me which is set and when.

  • I'd assume WAITING, RUNNING and DONE are part of a onehot state machine.
    But is DIED part of it or is it only set when DONE is reached?
  • I'd expect FAIL and PASS to be opposite but why would there be two flags for the same thing?
    • Can they ever be both set?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some explanation in section 2.15.6.2. Using the frequency counter:

"The frequency counter can also be used in a test mode. This allows the hardware to check if the frequency is within
a minimum frequency and a maximum frequency, set in FC0_MIN_KHZ and FC0_MAX_KHZ. In this mode, the PASS bit in FC0_STATUS will be set when DONE is set if the frequency is within the specified range. Otherwise, either the FAST or SLOW bit will be set.
If the programmer attempts to count a stopped clock, or the clock stops running then the DIED bit will be set. If any of DIED, FAST, or SLOW are set then FAIL will be set."

In this case, FAST and SLOW don't apply. So the only realistic error case would be DIED. So a simple Result<HertzU32, ()> would be sufficient to communicate the possible return values.

Or we could define an enum FrequencyCounterError { Died, Fast, Slow } so we match the hardware, even if in practice two of the cases won't occur unless we also make the lower/upper frequency limits configurable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange: I tried to provoke a DIED response:

     let m1: Rate<u32, 1, 1> = clocks.measure_frequency(_trg_clk: &clocks.adc_clock, accuracy: FCAccuracy::_1kHz ); 
     info!("measurement 1: {} kHz", m1.to_kHz());                             
     clocks.adc_clock.disable(); 
     let m2: Rate<u32, 1, 1> = clocks.measure_frequency(_trg_clk: &clocks.adc_clock, accuracy: FCAccuracy::_1kHz ); 
     info!("measurement 2: {} kHz", m2.to_kHz()); 

But I got a successful measurement:

└─ rp2040_project_template::__cortex_m_rt_main @ src/main.rs:27
INFO  measurement 1: 48000 kHz
└─ rp2040_project_template::__cortex_m_rt_main @ src/main.rs:59
INFO  measurement 2: 0 kHz

Which isn't exactly wrong - a disabled clock is a 0kHz clock. But then, when does DIED get set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does when it sees a couple clock cycles and then it stops while measuring is taking place.

}

/// Releases the CLOCKS block
pub fn free(self) -> CLOCKS {
self.clocks
Expand Down
2 changes: 1 addition & 1 deletion rp2040-hal/src/pll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ impl<D: PhaseLockedLoopDevice> PhaseLockedLoop<Disabled, D> {
xosc_frequency: HertzU32,
config: PLLConfig,
) -> Result<PhaseLockedLoop<Disabled, D>, Error> {
const VCO_FREQ_RANGE: RangeInclusive<HertzU32> = HertzU32::MHz(400)..=HertzU32::MHz(1_600);
const VCO_FREQ_RANGE: RangeInclusive<HertzU32> = HertzU32::MHz(750)..=HertzU32::MHz(1_600);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we consider this a breaking change and defer it to version 0.10.0?

In any case: This will break with the current value of vco_freq in PLL_USB_48MHZ, in line 130.

IMHO we should change the value of PLL_USB_48MHZ now (as it's not a breaking change), and change VCO_FREQ_RANGE with version 0.10.0.

PLL_USB_48MHZ should become:

    pub const PLL_USB_48MHZ: PLLConfig = PLLConfig {
        vco_freq: HertzU32::MHz(1200),
        refdiv: 1,
        post_div1: 5,
        post_div2: 5,
    };

(New values taken from https://github.com/raspberrypi/pico-sdk/pull/869/files)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(BTW: This is the error I get when trying to run it without fixing PLL_USB_48MHZ:

INFO  Program start
└─ rp2040_project_template::__cortex_m_rt_main @ src/main.rs:27
ERROR panicked at src/main.rs:45:6:
called `Option::unwrap()` on a `None` value
└─ panic_probe::print_defmt::print @ /home/jan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:104
────────────────────────────────────────────────────────────────────────────────
stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: lib::inline::__udf
        at ./asm/inline.rs:181:5
   2: __udf
        at ./asm/lib.rs:51:17
   3: cortex_m::asm::udf
        at /home/jan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cortex-m-0.7.7/src/asm.rs:43:5
   4: panic_probe::hard_fault
        at /home/jan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:86:5
   5: rust_begin_unwind
        at /home/jan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:54:9
   6: core::panicking::panic_fmt
        at /rustc/bc28abf92efc32f8f9312851bf8af38fbd23be42/library/core/src/panicking.rs:67:14
   7: core::panicking::panic
        at /rustc/bc28abf92efc32f8f9312851bf8af38fbd23be42/library/core/src/panicking.rs:117:5
   8: core::option::Option<T>::unwrap
        at /rustc/bc28abf92efc32f8f9312851bf8af38fbd23be42/library/core/src/option.rs:935:21
   9: rp2040_project_template::__cortex_m_rt_main
        at src/main.rs:45:6
  10: main
        at src/main.rs:25:1
  11: Reset
(HOST) ERROR the program panicked

)

const POSTDIV_RANGE: Range<u8> = 1..7;
const FBDIV_RANGE: Range<u16> = 16..320;

Expand Down