Skip to content

Commit 89b9d24

Browse files
bugadanibugadani
authored
Minor TIMG tweaks (#5256)
* Only avoid resetting TIMG0 on ESP32 * Move logic out of TimerGroupInstance * Unify peripheral guard impls, avoid resetting the peripherals multiple times
1 parent bdf5d3d commit 89b9d24

File tree

6 files changed

+67
-104
lines changed

6 files changed

+67
-104
lines changed

‎esp-hal/src/gpio/dedicated.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,14 @@ for_each_dedicated_gpio!(
177177
impl DedicatedGpio<'_> {
178178
#[cfg(dedicated_gpio_needs_initialization)]
179179
fn initialize() {
180-
crate::system::PeripheralClockControl::enable(crate::system::Peripheral::DedicatedGpio);
180+
use crate::system::{Peripheral, PeripheralClockControl};
181+
if PeripheralClockControl::enable(Peripheral::DedicatedGpio) {
182+
PeripheralClockControl::reset(Peripheral::DedicatedGpio);
183+
} else {
184+
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
185+
// dropping the driver.
186+
PeripheralClockControl::disable(Peripheral::DedicatedGpio);
187+
}
181188

182189
#[cfg(esp32s2)]
183190
{

‎esp-hal/src/ledc/mod.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,10 @@ impl<'d> Ledc<'d> {
123123
pub fn new(_instance: LEDC<'d>) -> Self {
124124
if PeripheralClockControl::enable(PeripheralEnable::Ledc) {
125125
PeripheralClockControl::reset(PeripheralEnable::Ledc);
126+
} else {
127+
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
128+
// dropping the driver.
129+
PeripheralClockControl::disable(PeripheralEnable::Ledc);
126130
}
127131

128132
let ledc = LEDC::regs();

‎esp-hal/src/system.rs‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,7 @@ pub(crate) struct PeripheralGuard {
6565

6666
impl PeripheralGuard {
6767
pub(crate) fn new_with(p: Peripheral, init: fn()) -> Self {
68-
if PeripheralClockControl::enable(p) {
69-
PeripheralClockControl::reset(p);
70-
init();
71-
}
68+
PeripheralClockControl::request_peripheral(p, init);
7269

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

10198
impl<const P: u8> GenericPeripheralGuard<P> {
10299
pub(crate) fn new_with(init: fn()) -> Self {
103-
let peripheral = const { Peripheral::try_from(P).unwrap() };
104-
105-
PERIPHERAL_REF_COUNT.with(|ref_counts| {
106-
if PeripheralClockControl::enable_with_counts(peripheral, ref_counts) {
107-
unsafe { PeripheralClockControl::reset_racey(peripheral) };
108-
init();
109-
}
110-
});
100+
let p = const { Peripheral::try_from(P).unwrap() };
101+
PeripheralClockControl::request_peripheral(p, init);
111102

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

141132
impl PeripheralClockControl {
133+
fn request_peripheral(p: Peripheral, init: fn()) {
134+
PERIPHERAL_REF_COUNT.with(|ref_counts| {
135+
if Self::enable_with_counts(p, ref_counts) {
136+
unsafe { Self::reset_racey(p) };
137+
init();
138+
}
139+
});
140+
}
141+
142142
/// Enables the given peripheral.
143143
///
144144
/// This keeps track of enabling a peripheral - i.e. a peripheral
@@ -165,8 +165,6 @@ impl PeripheralClockControl {
165165
/// gets disabled when the number of enable/disable attempts is balanced.
166166
///
167167
/// Returns `true` if it actually disabled the peripheral.
168-
///
169-
/// Before disabling a peripheral it will also get reset
170168
pub(crate) fn disable(peripheral: Peripheral) -> bool {
171169
PERIPHERAL_REF_COUNT.with(|ref_counts| {
172170
Self::enable_forced_with_counts(peripheral, false, false, ref_counts)
@@ -200,10 +198,6 @@ impl PeripheralClockControl {
200198
assert!(*ref_count == 0);
201199
}
202200

203-
if !enable {
204-
unsafe { Self::reset_racey(peripheral) };
205-
}
206-
207201
debug!("Enable {:?} {}", peripheral, enable);
208202
unsafe { enable_internal_racey(peripheral, enable) };
209203

‎esp-hal/src/timer/systimer.rs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,13 @@ impl<'d> SystemTimer<'d> {
222222
/// Create a new instance.
223223
pub fn new(_systimer: SYSTIMER<'d>) -> Self {
224224
// Don't reset Systimer as it will break `time::Instant::now`, only enable it
225-
PeripheralClockControl::enable(PeripheralEnable::Systimer);
225+
if PeripheralClockControl::enable(PeripheralEnable::Systimer) {
226+
PeripheralClockControl::reset(PeripheralEnable::Systimer);
227+
} else {
228+
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
229+
// dropping the driver.
230+
PeripheralClockControl::disable(PeripheralEnable::Systimer);
231+
}
226232

227233
#[cfg(etm_driver_supported)]
228234
etm::enable_etm();

‎esp-hal/src/timer/timg.rs‎

Lines changed: 28 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -125,13 +125,7 @@ pub trait TimerGroupInstance {
125125
fn register_block() -> *const RegisterBlock;
126126
#[cfg(soc_has_clock_node_timg_function_clock)]
127127
fn clock_instance() -> clocks::TimgInstance;
128-
fn enable_peripheral();
129-
fn reset_peripheral();
130-
#[cfg(soc_has_clock_node_timg_wdt_clock)]
131-
fn configure_wdt_src_clk(src: TimgWdtClockConfig);
132-
#[cfg(soc_has_clock_node_timg_wdt_clock)]
133-
fn gate_wdt_src_clk(enable: bool);
134-
fn wdt_src_frequency() -> Rate;
128+
fn peripheral() -> crate::system::Peripheral;
135129
fn wdt_interrupt() -> Interrupt;
136130
}
137131

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

154-
fn enable_peripheral() {
155-
PeripheralClockControl::enable(crate::system::Peripheral::Timg0);
156-
}
157-
158-
fn reset_peripheral() {
159-
// FIXME: for TIMG0 do nothing for now because the reset breaks
160-
// `time::Instant::now`
161-
}
162-
163-
#[cfg(soc_has_clock_node_timg_wdt_clock)]
164-
fn configure_wdt_src_clk(src: TimgWdtClockConfig) {
165-
clocks::ClockTree::with(|clocks| Self::clock_instance().configure_wdt_clock(clocks, src));
166-
}
167-
168-
#[cfg(soc_has_clock_node_timg_wdt_clock)]
169-
fn gate_wdt_src_clk(enable: bool) {
170-
clocks::ClockTree::with(|clocks| {
171-
if enable {
172-
Self::clock_instance().request_wdt_clock(clocks)
173-
} else {
174-
Self::clock_instance().release_wdt_clock(clocks)
175-
}
176-
});
177-
}
178-
179-
fn wdt_src_frequency() -> Rate {
180-
clocks::ClockTree::with(|clocks| {
181-
cfg_if::cfg_if! {
182-
if #[cfg(soc_has_clock_node_timg_wdt_clock)] {
183-
Rate::from_hz(Self::clock_instance().wdt_clock_frequency(clocks))
184-
} else {
185-
Rate::from_hz(clocks::apb_clk_frequency(clocks))
186-
}
187-
}
188-
})
148+
fn peripheral() -> crate::system::Peripheral {
149+
crate::system::Peripheral::Timg0
189150
}
190151

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

212-
fn enable_peripheral() {
213-
PeripheralClockControl::enable(crate::system::Peripheral::Timg1);
214-
}
215-
216-
fn reset_peripheral() {
217-
PeripheralClockControl::reset(crate::system::Peripheral::Timg1);
218-
}
219-
220-
#[cfg(soc_has_clock_node_timg_wdt_clock)]
221-
fn configure_wdt_src_clk(src: TimgWdtClockConfig) {
222-
clocks::ClockTree::with(|clocks| Self::clock_instance().configure_wdt_clock(clocks, src));
223-
}
224-
225-
#[cfg(soc_has_clock_node_timg_wdt_clock)]
226-
fn gate_wdt_src_clk(enable: bool) {
227-
clocks::ClockTree::with(|clocks| {
228-
if enable {
229-
Self::clock_instance().request_wdt_clock(clocks)
230-
} else {
231-
Self::clock_instance().release_wdt_clock(clocks)
232-
}
233-
});
234-
}
235-
236-
fn wdt_src_frequency() -> Rate {
237-
clocks::ClockTree::with(|clocks| {
238-
cfg_if::cfg_if! {
239-
if #[cfg(soc_has_clock_node_timg_wdt_clock)] {
240-
Rate::from_hz(Self::clock_instance().wdt_clock_frequency(clocks))
241-
} else {
242-
Rate::from_hz(clocks::apb_clk_frequency(clocks))
243-
}
244-
}
245-
})
173+
fn peripheral() -> crate::system::Peripheral {
174+
crate::system::Peripheral::Timg1
246175
}
247176

248177
fn wdt_interrupt() -> Interrupt {
@@ -256,8 +185,14 @@ where
256185
{
257186
/// Construct a new instance of [`TimerGroup`] in blocking mode
258187
pub fn new(_timer_group: T) -> Self {
259-
T::reset_peripheral();
260-
T::enable_peripheral();
188+
// TODO: use PeripheralGuard
189+
if PeripheralClockControl::enable(T::peripheral()) {
190+
PeripheralClockControl::reset(T::peripheral());
191+
} else {
192+
// Refcount was more than 0. Decrement to avoid overflow because we don't handle
193+
// dropping the driver.
194+
PeripheralClockControl::disable(T::peripheral());
195+
}
261196

262197
#[cfg(soc_has_clock_node_timg_function_clock)]
263198
clocks::ClockTree::with(|clocks| {
@@ -665,7 +600,9 @@ where
665600

666601
this.set_write_protection(false);
667602
#[cfg(soc_has_clock_node_timg_wdt_clock)]
668-
TG::configure_wdt_src_clk(TimgWdtClockConfig::default());
603+
clocks::ClockTree::with(|clocks| {
604+
TG::clock_instance().configure_wdt_clock(clocks, TimgWdtClockConfig::default())
605+
});
669606
this.set_write_protection(true);
670607

671608
this
@@ -699,7 +636,7 @@ where
699636

700637
#[cfg(soc_has_clock_node_timg_wdt_clock)]
701638
if enabled {
702-
TG::gate_wdt_src_clk(true);
639+
clocks::ClockTree::with(|clocks| TG::clock_instance().request_wdt_clock(clocks));
703640
}
704641

705642
reg_block
@@ -724,7 +661,7 @@ where
724661

725662
#[cfg(soc_has_clock_node_timg_wdt_clock)]
726663
if !enabled {
727-
TG::gate_wdt_src_clk(false);
664+
clocks::ClockTree::with(|clocks| TG::clock_instance().release_wdt_clock(clocks));
728665
}
729666

730667
self.set_write_protection(true);
@@ -753,7 +690,16 @@ where
753690

754691
/// Set the timeout, in microseconds, of the watchdog timer
755692
pub fn set_timeout(&mut self, stage: MwdtStage, timeout: Duration) {
756-
let clk_src = TG::wdt_src_frequency();
693+
let clk_src = clocks::ClockTree::with(|clocks| {
694+
cfg_if::cfg_if! {
695+
if #[cfg(soc_has_clock_node_timg_wdt_clock)] {
696+
Rate::from_hz(TG::clock_instance().wdt_clock_frequency(clocks))
697+
} else {
698+
Rate::from_hz(clocks::apb_clk_frequency(clocks))
699+
}
700+
}
701+
});
702+
757703
let timeout_ticks = timeout.as_micros() * clk_src.as_mhz() as u64;
758704

759705
let reg_block = unsafe { &*TG::register_block() };

‎esp-hal/src/usb_serial_jtag.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ use crate::{
130130
asynch::AtomicWaker,
131131
pac::usb_device::RegisterBlock,
132132
peripherals::USB_DEVICE,
133-
system::PeripheralClockControl,
133+
system::{Peripheral, PeripheralClockControl},
134134
};
135135

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

356362
usb_device.disable_tx_interrupts();
357363
usb_device.disable_rx_interrupts();

0 commit comments

Comments
 (0)