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

ithinuel · 2023-09-04T15:00:35Z

No description provided.

jannic · 2023-09-04T15:38:57Z

rp2040-hal/src/pll.rs

@@ -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);


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)

(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

)

rp2040-hal/src/clocks/mod.rs

jannic · 2023-09-04T16:09:15Z

rp2040-hal/src/clocks/mod.rs

+        // 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())


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.

jannic · 2023-09-04T16:13:16Z

rp2040-hal/src/clocks/mod.rs

+        }
+
+        let speed_hz = self.clocks.fc0_result.read().khz().bits() * 1000;
+        speed_hz.Hz()


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

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?

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.

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?

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

jannic · 2023-09-04T20:40:56Z

rp2040-hal/src/clocks/mod.rs

+        &mut self,
+        _trg_clk: C,
+        accuracy: FCAccuracy,
+    ) -> Result<HertzU32, ClockDiedError> {


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

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 :( )

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.

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

ithinuel added 5 commits September 4, 2023 05:23

Update PLL's minimum VCO frequency (Fixes rp-rs#682)

08504b6

Add GPin0 and GPin1 clock initial source support

4dbadef

Add clock measurement support

8445d1f

update changelog

442b662

fix formatting

6cbfae2

jannic reviewed Sep 4, 2023

View reviewed changes

clocks: Fix comments & rename approximate_frequency

fc6a348

ithinuel force-pushed the add-clock-measurement-support branch from 3dfcd02 to fc6a348 Compare September 4, 2023 18:10

clocks: add error support for measure_frequency

8b48e81

jannic reviewed Sep 4, 2023

View reviewed changes

update the usb pll configuration according to the new vco min limit

3766cde

ithinuel marked this pull request as draft September 5, 2023 07:49

ithinuel mentioned this pull request Dec 3, 2023

ClockManager "partial move" with RealTimeClock #724

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ithinuel commented Sep 4, 2023

jannic Sep 4, 2023

jannic Sep 4, 2023

jannic Sep 4, 2023

jannic Sep 4, 2023

ithinuel Sep 4, 2023

jannic Sep 4, 2023

jannic Sep 4, 2023

ithinuel Sep 4, 2023

jannic Sep 4, 2023 •

edited

Loading

ithinuel Sep 4, 2023

jannic Sep 4, 2023

jannic Sep 4, 2023

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

Are you sure you want to change the base?

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

Conversation

ithinuel commented Sep 4, 2023

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

jannic Sep 4, 2023 • edited Loading

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

jannic Sep 4, 2023 •

edited

Loading