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.

Sign up for GitHub

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

Jump to bottom

Enhance timer creation constraint #671

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Enhance timer creation constraint #671

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5763a2e
timer: Take &ReferenceClock rather than &ClocksManager
ithinuel Aug 16, 2023
4b9722c
timer: add an assert to the ref clock operating frequency
ithinuel Aug 16, 2023
f933b0c
timer: Check the timer's source is ref_clk ÷ watchdog tick.
ithinuel Aug 16, 2023
bc3add6
Timer: Use multiplication in assert instead of division.
ithinuel Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion rp2040-hal/examples/adc_fifo_dma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,12 @@ fn main() -> ! {
adc_fifo.resume();

// initialize a timer, to measure the total sampling time (printed below)
let timer = hal::Timer::new(pac.TIMER, &mut pac.RESETS, &clocks);
let timer = hal::Timer::new(
pac.TIMER,
&mut pac.RESETS,
&watchdog,
&clocks.reference_clock,
);

// NOTE: in a real-world program, instead of calling `wait` now, you would probably:
// 1. Enable one of the DMA interrupts for the channel (e.g. `dma.ch0.enable_irq0()`)
Expand Down
7 changes: 6 additions & 1 deletion rp2040-hal/examples/adc_fifo_poll.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,12 @@ fn main() -> ! {
let mut i = 0;

// initialize a timer, to measure the total sampling time (printed below)
let timer = hal::Timer::new(pac.TIMER, &mut pac.RESETS, &clocks);
let timer = hal::Timer::new(
pac.TIMER,
&mut pac.RESETS,
&watchdog,
&clocks.reference_clock,
);

loop {
// busy-wait until the FIFO contains at least two samples:
Expand Down
7 changes: 6 additions & 1 deletion rp2040-hal/examples/blinky.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ fn main() -> ! {
.ok()
.unwrap();

let mut timer = rp2040_hal::Timer::new(pac.TIMER, &mut pac.RESETS, &clocks);
let mut timer = rp2040_hal::Timer::new(
pac.TIMER,
&mut pac.RESETS,
&watchdog,
&clocks.reference_clock,
);

// The single-cycle I/O block controls our GPIO pins
let sio = hal::Sio::new(pac.SIO);
Expand Down
7 changes: 6 additions & 1 deletion rp2040-hal/examples/vector_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@ fn main() -> ! {
// Configure GPIO25 as an output
let led_pin = pins.gpio25.into_push_pull_output();

let mut timer = hal::Timer::new(pac.TIMER, &mut pac.RESETS, &clocks);
let mut timer = hal::Timer::new(
pac.TIMER,
&mut pac.RESETS,
&watchdog,
&clocks.reference_clock,
);
critical_section::with(|cs| {
let mut alarm = timer.alarm_0().unwrap();
// Schedule an alarm in 1 second
Expand Down
4 changes: 2 additions & 2 deletions rp2040-hal/src/clocks/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
//! let xosc = setup_xosc_blocking(peripherals.XOSC, XOSC_CRYSTAL_FREQ.Hz()).map_err(InitError::XoscErr)?;
//!
//! // Start tick in watchdog
//! watchdog.enable_tick_generation((XOSC_CRYSTAL_FREQ / 1_000_000) as u8);
//! watchdog.enable_tick_generation((XOSC_CRYSTAL_FREQ / 1_000_000) as u16);
//!
//! let mut clocks = ClocksManager::new(peripherals.CLOCKS);
//!
Expand Down Expand Up @@ -330,7 +330,7 @@ pub fn init_clocks_and_plls(
let xosc = setup_xosc_blocking(xosc_dev, xosc_crystal_freq.Hz()).map_err(InitError::XoscErr)?;

// Configure watchdog tick generation to tick over every microsecond
watchdog.enable_tick_generation((xosc_crystal_freq / 1_000_000) as u8);
watchdog.enable_tick_generation((xosc_crystal_freq / 1_000_000) as u16);

let mut clocks = ClocksManager::new(clocks_dev);

Expand Down
17 changes: 14 additions & 3 deletions rp2040-hal/src/timer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ use fugit::{MicrosDurationU32, MicrosDurationU64, TimerInstantU64};

use crate::{
atomic_register_access::{write_bitmask_clear, write_bitmask_set},
clocks::ClocksManager,
clocks::ReferenceClock,
pac::{self, RESETS, TIMER},
resets::SubsystemReset,
typelevel::Sealed,
Clock, Watchdog,
};

/// Instant type used by the Timer & Alarm methods.
Expand Down Expand Up @@ -59,7 +60,16 @@ impl Timer {
/// Make sure that clocks and watchdog are configured, so
/// that timer ticks happen at a frequency of 1MHz.
/// Otherwise, `Timer` won't work as expected.
pub fn new(timer: TIMER, resets: &mut RESETS, _clocks: &ClocksManager) -> Self {
pub fn new(
timer: TIMER,
resets: &mut RESETS,
watchdog: &Watchdog,
clocks: &ReferenceClock,
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of using ReferenceClock instead of ClocksManager? It makes the examples longer, it I'd expect the same to be true for most use cases. Do you have an example where you do have a ReferenceClock available but not a `ClocksManager´?

Copy link
Member Author

@ithinuel ithinuel Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

USB takes its clock by value (in order to own it).
This at least partially destructures the ClockManager making it mandatory to initialize the timer before USB. This isn't actually required (Timer can be created after UsbBus).

The actual clock chain for the Timer is:

       |
       |   +-----------+            +------------------+
       |   | ref_clk   |  ref_clk   | prescaler shared |      +-------+
xtal --|-> | prescaler | ---------> | with watchdog    | ---> | Timer |
       |   +-----------+            +------------------+      +-------+
       |

where the clk_ref can be sourced from rosc, aux src (pll_usb, gpin0, gpin1) or xosc.
The timer only needs to know the prescaller shared with the watchdog and the ref_clk frequency (to figure what its actual frequency is).

Note:
I had fun hand crafting this diagram 😄

Copy link
Member

Choose a reason for hiding this comment

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

USB takes its clock by value (in order to own it).
This at least partially destructures the ClockManager making it mandatory to initialize the timer before USB.

Perhaps we should change that? USB doesn't really need to own its clock. I guess it's meant as a safeguard to prevent other code from messing with the usb clock. But that's not foolproof anyway, as it's still possible to mess with the XOSC which will also affect the usb clock.

Timer can be created after UsbBus

Is there a use case where that's actually useful?

Copy link
Member Author

@ithinuel ithinuel Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we should change that? USB doesn't really need to own its clock. I guess it's meant as a safeguard to prevent other code from messing with the usb clock. But that's not foolproof anyway, as it's still possible to mess with the XOSC which will also affect the usb clock.

Indeed, the more I work on clocks, sleep & dormant, the more I doubt this can be that easily handled.
Taking ownership of the clock is good enough when there's no usecase where the clock speed and/or source changes but there's lots of cases where lowering clock to save power is essential.

But then, making sure things remain consistent becomes much harder to keep simple. ie orchestrating:

  • clock requirements (eg on USB)
  • clock speed and source change: to switch between fast processing and power-saving modes
  • keeping baudrates valid on clock change: should already set rate prevent clock change if it'd bring the requested baudrate un-reachable?

All that requiring system wide power and state management. Not unrealisable but maybe out of scope for our lightweight HAL objective?
If not, then that'd be great to have and would definitely enable lots of really fancy power efficient usecases :D

Is there a use case where that's actually useful?

What's the usefulness of initializing the Timer before the UsbBus vs the other way around?
These are not related in hardware, there's no reason to have them so in software.

) -> Self {
assert_eq!(
clocks.freq().to_Hz() / u32::from(watchdog.cycles_per_ticks()),
1_000_000
ithinuel marked this conversation as resolved.
Show resolved Hide resolved
);
timer.reset_bring_down(resets);
timer.reset_bring_up(resets);
Self { _private: () }
Expand Down Expand Up @@ -184,8 +194,9 @@ impl eh1_0_alpha::delay::DelayUs for Timer {
/// // Make sure to initialize clocks, otherwise the timer wouldn't work
/// // properly. Omitted here for terseness.
/// let clocks: rp2040_hal::clocks::ClocksManager = todo!();
/// let watchdog: rp2040_hal::Watchdog = todo!();
/// // Configure the Timer peripheral in count-down mode
/// let timer = rp2040_hal::Timer::new(pac.TIMER, &mut pac.RESETS, &clocks);
/// let timer = rp2040_hal::Timer::new(pac.TIMER, &mut pac.RESETS, &watchdog, &clocks.reference_clock);
/// let mut count_down = timer.count_down();
/// // Create a count_down timer for 500 milliseconds
/// count_down.start(500.millis());
Expand Down
9 changes: 8 additions & 1 deletion rp2040-hal/src/watchdog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,21 @@ impl Watchdog {
///
/// * `cycles` - Total number of tick cycles before the next tick is generated.
/// It is expected to be the frequency in MHz of clk_ref.
pub fn enable_tick_generation(&mut self, cycles: u8) {
pub fn enable_tick_generation(&mut self, cycles: u16) {
jannic marked this conversation as resolved.
Show resolved Hide resolved
const WATCHDOG_TICK_ENABLE_BITS: u32 = 0x200;

assert_eq!(cycles & !0x1FF, 0);

self.watchdog
.tick
.write(|w| unsafe { w.bits(WATCHDOG_TICK_ENABLE_BITS | cycles as u32) })
}

/// Returns the number of `clk_ref` cycles between each watchdog (and Timer) ticks.
pub fn cycles_per_ticks(&self) -> u16 {
self.watchdog.tick.read().cycles().bits()
}

/// Defines whether or not the watchdog timer should be paused when processor(s) are in debug mode
/// or when JTAG is accessing bus fabric
///
Expand Down
Loading