Skip to content

Minor TIMG tweaks #5256

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

Merged
bugadani merged 3 commits into from
Mar 26, 2026
Merged

Minor TIMG tweaks #5256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fa562f
Only avoid resetting TIMG0 on ESP32
bugadani Mar 25, 2026
deffffb
Move logic out of TimerGroupInstance
bugadani Mar 25, 2026
5927fa9
Unify peripheral guard impls, avoid resetting the peripherals multipl…
bugadani Mar 26, 2026
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
9 changes: 8 additions & 1 deletion esp-hal/src/gpio/dedicated.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,14 @@ for_each_dedicated_gpio!(
impl DedicatedGpio<'_> {
#[cfg(dedicated_gpio_needs_initialization)]
fn initialize() {
crate::system::PeripheralClockControl::enable(crate::system::Peripheral::DedicatedGpio);
use crate::system::{Peripheral, PeripheralClockControl};
if PeripheralClockControl::enable(Peripheral::DedicatedGpio) {
PeripheralClockControl::reset(Peripheral::DedicatedGpio);
} else {
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
// dropping the driver.
PeripheralClockControl::disable(Peripheral::DedicatedGpio);
}

#[cfg(esp32s2)]
{
Expand Down
4 changes: 4 additions & 0 deletions esp-hal/src/ledc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ impl<'d> Ledc<'d> {
pub fn new(_instance: LEDC<'d>) -> Self {
if PeripheralClockControl::enable(PeripheralEnable::Ledc) {
PeripheralClockControl::reset(PeripheralEnable::Ledc);
} else {
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
// dropping the driver.
PeripheralClockControl::disable(PeripheralEnable::Ledc);
}

let ledc = LEDC::regs();
Expand Down
30 changes: 12 additions & 18 deletions esp-hal/src/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,7 @@ pub(crate) struct PeripheralGuard {

impl PeripheralGuard {
pub(crate) fn new_with(p: Peripheral, init: fn()) -> Self {
if PeripheralClockControl::enable(p) {
PeripheralClockControl::reset(p);
init();
}
PeripheralClockControl::request_peripheral(p, init);

Self { peripheral: p }
}
Expand Down Expand Up @@ -100,14 +97,8 @@ pub(crate) struct GenericPeripheralGuard<const P: u8> {}

impl<const P: u8> GenericPeripheralGuard<P> {
pub(crate) fn new_with(init: fn()) -> Self {
let peripheral = const { Peripheral::try_from(P).unwrap() };

PERIPHERAL_REF_COUNT.with(|ref_counts| {
if PeripheralClockControl::enable_with_counts(peripheral, ref_counts) {
unsafe { PeripheralClockControl::reset_racey(peripheral) };
init();
}
});
let p = const { Peripheral::try_from(P).unwrap() };
PeripheralClockControl::request_peripheral(p, init);

Self {}
}
Expand Down Expand Up @@ -139,6 +130,15 @@ impl<const P: u8> Drop for GenericPeripheralGuard<P> {
pub(crate) struct PeripheralClockControl;

impl PeripheralClockControl {
fn request_peripheral(p: Peripheral, init: fn()) {
PERIPHERAL_REF_COUNT.with(|ref_counts| {
if Self::enable_with_counts(p, ref_counts) {
unsafe { Self::reset_racey(p) };
init();
}
});
}

/// Enables the given peripheral.
///
/// This keeps track of enabling a peripheral - i.e. a peripheral
Expand All @@ -165,8 +165,6 @@ impl PeripheralClockControl {
/// gets disabled when the number of enable/disable attempts is balanced.
///
/// Returns `true` if it actually disabled the peripheral.
///
/// Before disabling a peripheral it will also get reset
pub(crate) fn disable(peripheral: Peripheral) -> bool {
PERIPHERAL_REF_COUNT.with(|ref_counts| {
Self::enable_forced_with_counts(peripheral, false, false, ref_counts)
Expand Down Expand Up @@ -200,10 +198,6 @@ impl PeripheralClockControl {
assert!(*ref_count == 0);
}

if !enable {
unsafe { Self::reset_racey(peripheral) };
Copy link
Copy Markdown
Contributor Author

@bugadani bugadani Mar 26, 2026

Choose a reason for hiding this comment

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

This goes because resetting TIMG0 enables its WDT, and that's just a source of frustration.

}

debug!("Enable {:?} {}", peripheral, enable);
unsafe { enable_internal_racey(peripheral, enable) };

Expand Down
8 changes: 7 additions & 1 deletion esp-hal/src/timer/systimer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,13 @@ impl<'d> SystemTimer<'d> {
/// Create a new instance.
pub fn new(_systimer: SYSTIMER<'d>) -> Self {
// Don't reset Systimer as it will break `time::Instant::now`, only enable it
PeripheralClockControl::enable(PeripheralEnable::Systimer);
if PeripheralClockControl::enable(PeripheralEnable::Systimer) {
PeripheralClockControl::reset(PeripheralEnable::Systimer);
} else {
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
// dropping the driver.
PeripheralClockControl::disable(PeripheralEnable::Systimer);
}

#[cfg(etm_driver_supported)]
etm::enable_etm();
Expand Down
110 changes: 28 additions & 82 deletions esp-hal/src/timer/timg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,7 @@ pub trait TimerGroupInstance {
fn register_block() -> *const RegisterBlock;
#[cfg(soc_has_clock_node_timg_function_clock)]
fn clock_instance() -> clocks::TimgInstance;
fn enable_peripheral();
fn reset_peripheral();
#[cfg(soc_has_clock_node_timg_wdt_clock)]
fn configure_wdt_src_clk(src: TimgWdtClockConfig);
#[cfg(soc_has_clock_node_timg_wdt_clock)]
fn gate_wdt_src_clk(enable: bool);
fn wdt_src_frequency() -> Rate;
fn peripheral() -> crate::system::Peripheral;
fn wdt_interrupt() -> Interrupt;
}

Expand All @@ -151,41 +145,8 @@ impl TimerGroupInstance for TIMG0<'_> {
clocks::TimgInstance::Timg0
}

fn enable_peripheral() {
PeripheralClockControl::enable(crate::system::Peripheral::Timg0);
}

fn reset_peripheral() {
// FIXME: for TIMG0 do nothing for now because the reset breaks
// `time::Instant::now`
}

#[cfg(soc_has_clock_node_timg_wdt_clock)]
fn configure_wdt_src_clk(src: TimgWdtClockConfig) {
clocks::ClockTree::with(|clocks| Self::clock_instance().configure_wdt_clock(clocks, src));
}

#[cfg(soc_has_clock_node_timg_wdt_clock)]
fn gate_wdt_src_clk(enable: bool) {
clocks::ClockTree::with(|clocks| {
if enable {
Self::clock_instance().request_wdt_clock(clocks)
} else {
Self::clock_instance().release_wdt_clock(clocks)
}
});
}

fn wdt_src_frequency() -> Rate {
clocks::ClockTree::with(|clocks| {
cfg_if::cfg_if! {
if #[cfg(soc_has_clock_node_timg_wdt_clock)] {
Rate::from_hz(Self::clock_instance().wdt_clock_frequency(clocks))
} else {
Rate::from_hz(clocks::apb_clk_frequency(clocks))
}
}
})
fn peripheral() -> crate::system::Peripheral {
crate::system::Peripheral::Timg0
}

fn wdt_interrupt() -> Interrupt {
Expand All @@ -209,40 +170,8 @@ impl TimerGroupInstance for crate::peripherals::TIMG1<'_> {
clocks::TimgInstance::Timg1
}

fn enable_peripheral() {
PeripheralClockControl::enable(crate::system::Peripheral::Timg1);
}

fn reset_peripheral() {
PeripheralClockControl::reset(crate::system::Peripheral::Timg1);
}

#[cfg(soc_has_clock_node_timg_wdt_clock)]
fn configure_wdt_src_clk(src: TimgWdtClockConfig) {
clocks::ClockTree::with(|clocks| Self::clock_instance().configure_wdt_clock(clocks, src));
}

#[cfg(soc_has_clock_node_timg_wdt_clock)]
fn gate_wdt_src_clk(enable: bool) {
clocks::ClockTree::with(|clocks| {
if enable {
Self::clock_instance().request_wdt_clock(clocks)
} else {
Self::clock_instance().release_wdt_clock(clocks)
}
});
}

fn wdt_src_frequency() -> Rate {
clocks::ClockTree::with(|clocks| {
cfg_if::cfg_if! {
if #[cfg(soc_has_clock_node_timg_wdt_clock)] {
Rate::from_hz(Self::clock_instance().wdt_clock_frequency(clocks))
} else {
Rate::from_hz(clocks::apb_clk_frequency(clocks))
}
}
})
fn peripheral() -> crate::system::Peripheral {
crate::system::Peripheral::Timg1
}

fn wdt_interrupt() -> Interrupt {
Expand All @@ -256,8 +185,14 @@ where
{
/// Construct a new instance of [`TimerGroup`] in blocking mode
pub fn new(_timer_group: T) -> Self {
T::reset_peripheral();
T::enable_peripheral();
// TODO: use PeripheralGuard
if PeripheralClockControl::enable(T::peripheral()) {
PeripheralClockControl::reset(T::peripheral());
} else {
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
// dropping the driver.
PeripheralClockControl::disable(T::peripheral());
}

#[cfg(soc_has_clock_node_timg_function_clock)]
clocks::ClockTree::with(|clocks| {
Expand Down Expand Up @@ -665,7 +600,9 @@ where

this.set_write_protection(false);
#[cfg(soc_has_clock_node_timg_wdt_clock)]
TG::configure_wdt_src_clk(TimgWdtClockConfig::default());
clocks::ClockTree::with(|clocks| {
TG::clock_instance().configure_wdt_clock(clocks, TimgWdtClockConfig::default())
});
this.set_write_protection(true);

this
Expand Down Expand Up @@ -699,7 +636,7 @@ where

#[cfg(soc_has_clock_node_timg_wdt_clock)]
if enabled {
TG::gate_wdt_src_clk(true);
clocks::ClockTree::with(|clocks| TG::clock_instance().request_wdt_clock(clocks));
}

reg_block
Expand All @@ -724,7 +661,7 @@ where

#[cfg(soc_has_clock_node_timg_wdt_clock)]
if !enabled {
TG::gate_wdt_src_clk(false);
clocks::ClockTree::with(|clocks| TG::clock_instance().release_wdt_clock(clocks));
}

self.set_write_protection(true);
Expand Down Expand Up @@ -753,7 +690,16 @@ where

/// Set the timeout, in microseconds, of the watchdog timer
pub fn set_timeout(&mut self, stage: MwdtStage, timeout: Duration) {
let clk_src = TG::wdt_src_frequency();
let clk_src = clocks::ClockTree::with(|clocks| {
cfg_if::cfg_if! {
if #[cfg(soc_has_clock_node_timg_wdt_clock)] {
Rate::from_hz(TG::clock_instance().wdt_clock_frequency(clocks))
} else {
Rate::from_hz(clocks::apb_clk_frequency(clocks))
}
}
});

let timeout_ticks = timeout.as_micros() * clk_src.as_mhz() as u64;

let reg_block = unsafe { &*TG::register_block() };
Expand Down
10 changes: 8 additions & 2 deletions esp-hal/src/usb_serial_jtag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ use crate::{
asynch::AtomicWaker,
pac::usb_device::RegisterBlock,
peripherals::USB_DEVICE,
system::PeripheralClockControl,
system::{Peripheral, PeripheralClockControl},
};

/// Custom USB serial error type
Expand Down Expand Up @@ -351,7 +351,13 @@ where
fn new_inner(usb_device: USB_DEVICE<'d>) -> Self {
// Do NOT reset the peripheral. Doing so will result in a broken USB JTAG
// connection.
PeripheralClockControl::enable(crate::system::Peripheral::UsbDevice);
if PeripheralClockControl::enable(Peripheral::UsbDevice) {
PeripheralClockControl::reset(Peripheral::UsbDevice);
} else {
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
// dropping the driver.
PeripheralClockControl::disable(Peripheral::UsbDevice);
}

usb_device.disable_tx_interrupts();
usb_device.disable_rx_interrupts();
Expand Down
Loading