Unify peripheral guard impls, avoid resetting the peripherals multiple times

Author: bugadani

esp-hal/src/gpio/dedicated.rs

@@ -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)]
         {

esp-hal/src/ledc/mod.rs

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

esp-hal/src/system.rs

@@ -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 }
     }
@@ -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 {}
     }
@@ -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
@@ -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)
@@ -200,10 +198,6 @@ impl PeripheralClockControl {
             assert!(*ref_count == 0);
         }
 
-        if !enable {
-            unsafe { Self::reset_racey(peripheral) };
-        }
-
         debug!("Enable {:?} {}", peripheral, enable);
         unsafe { enable_internal_racey(peripheral, enable) };
 

esp-hal/src/timer/systimer.rs

@@ -221,8 +221,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) {
+            // Don't reset Systimer as it will break `time::Instant::now`, only enable it
+        } 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();

esp-hal/src/usb_serial_jtag.rs

@@ -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
@@ -349,9 +349,14 @@ where
     Dm: DriverMode,
 {
     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) {
+            // Do NOT reset the peripheral. Doing so will result in a broken USB JTAG
+            // connection.
+        } 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();