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
}
}
97 changes: 94 additions & 3 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,34 @@ use crate::{
mod macros;
mod clock_sources;

use clock_sources::PllSys;

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

use clock_sources::{PllSys, PllUsb, Rosc, Xosc};

/// Frequency counter accuracy
///
/// See: https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf#table-fc-test-interval
#[repr(u8)]
#[allow(missing_docs)]
#[allow(clippy::enum_variant_names)]
pub enum FCAccuracy {
_2048kHz = 0,
_1024kHz,
_512kHz,
_256kHz,
_128kHz,
_64kHz,
_32kHz,
_16kHz,
_8kHz,
_4kHz,
_2kHz,
_1kHz,
_500Hz,
_250Hz,
_125Hz,
_62_5Hz,
}

#[derive(Copy, Clone)]
/// Provides refs to the CLOCKS block.
Expand Down Expand Up @@ -111,6 +137,11 @@ pub enum ClockError {
FrequencyTooLow,
}

/// The clock stopped while its frequency was being measured.
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct ClockDiedError;

/// For clocks
pub trait Clock: Sealed + Sized {
/// Enum with valid source clocks register values for `Clock`
Expand Down Expand Up @@ -167,6 +198,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 +335,63 @@ impl ClocksManager {
.configure_clock(&self.system_clock, self.system_clock.freq())
}

/// Measure the frequency of the given clock source by approximation.
pub fn measure_frequency<C: ClockSource>(
&mut self,
_trg_clk: C,
accuracy: FCAccuracy,
) -> Result<HertzU32, ClockDiedError> {
Copy link
Member

@jannic jannic Sep 4, 2023

Choose a reason for hiding this comment

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

I had some problems calling this function at all:

error[E0505]: cannot move out of `clocks.adc_clock` because it is borrowed
  --> src/main.rs:58:39
   |
35 |     let mut clocks = init_clocks_and_plls(
   |         ---------- binding `clocks` declared here
...
58 |     let m1 = clocks.measure_frequency(clocks.adc_clock, FCAccuracy::_1kHz );
   |              ------ ----------------- ^^^^^^^^^^^^^^^^ move out of `clocks.adc_clock` occurs here
   |              |      |
   |              |      borrow later used by call
   |              borrow of `clocks` occurs here

How is it meant to be used?
(I worked around that by changing the signature:

    pub fn measure_frequency<C: ClockSource>(
        &self,
        _trg_clk: &C,
        accuracy: FCAccuracy,
    ) -> Result<HertzU32, ClockDiedError> {

Which works, but I guess the &mut self is desirable to guarantee exclusive access.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh, let me remove that part of the PR then. It's causing more troubles than anything.
I thought that'd be an incremental step on the clock update (rather than the monolithic thing that GPIO were and PIO will be :( )

Copy link
Member

Choose a reason for hiding this comment

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

Don't get me wrong, I like the feature. But it would be nice to be able to call the function :-)

Keeping it separate from fixing the VCO frequency is a good idea, though. The minimal fix, updating the settings for the USB PLL, should be in its own PR.

Copy link
Member

Choose a reason for hiding this comment

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

This could work:

     pub fn measure_frequency(                                  
         &mut self,                   
         clock: FC0_SRC_A,                                                 
         accuracy: FCAccuracy, 
     ) -> HertzU32 {                                                                                

Little bit ugly at the call site, but no ownership issues:

let m1 = clocks.measure_frequency(AdcClock::FCOUNTER_SRC, FCAccuracy::_1kHz );

// 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.

});

// > The test interval is 0.98us * 2**interval, but let's call it 1us * 2**interval.
// > The default gives a test interval of 250us
//
// https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf#reg-clocks-FC0_INTERVAL
// https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf#table-fc-test-interval
self.clocks
.fc0_interval
.write(|w| unsafe { w.fc0_interval().bits(accuracy as u8) });

// 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) });

// Select which clock to measure.
self.clocks
.fc0_src
.write(|w| w.fc0_src().variant(C::FCOUNTER_SRC));

// Wait until the measurement is ready
let mut status;
loop {
status = self.clocks.fc0_status.read();
if status.done().bit_is_set() {
break;
}
}

if status.fail().bit_is_set() {
Err(ClockDiedError)
} else {
let result = self.clocks.fc0_result.read();
let speed_hz = result.khz().bits() * 1000 + u32::from(result.frac().bits());
Ok(speed_hz.Hz())
}
}

/// 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
Loading