refactor(bitscrambler): don't use Reset Clock Control lock of RC version

Author: suda-morris

components/esp_driver_bitscrambler/include/driver/bitscrambler.h

@@ -50,6 +50,9 @@ typedef struct {
 /**
  * @brief Allocate BitScrambler handle for a hardware channel
  *
+ * @note This function can only be used to create a single direction BitScrambler handle.
+ *       If you need a loopback BitScrambler, call bitscrambler_loopback_create() instead.
+ *
  * @param config Configuration for requested BitScrambler
  * @param[out] handle BitScrambler controller handle
  *

components/esp_driver_bitscrambler/src/bitscrambler.c

@@ -5,6 +5,7 @@
  */
 #include <string.h>
 #include <stdatomic.h>
+#include "soc/soc_caps.h"
 #include "esp_log.h"
 #include "esp_heap_caps.h"
 #include "driver/bitscrambler.h"
@@ -20,6 +21,13 @@ static const char *TAG = "bitscrambler";
 #define BITSCRAMBLER_MEM_ALLOC_CAPS  MALLOC_CAP_DEFAULT
 #endif
 
+#if !SOC_RCC_IS_INDEPENDENT
+// Reset and Clock Control registers are mixing with other peripherals, so we need to use a critical section
+#define BS_RCC_ATOMIC() PERIPH_RCC_ATOMIC()
+#else
+#define BS_RCC_ATOMIC()
+#endif
+
 #define BITSCRAMBLER_BINARY_VER 1 //max version we're compatible with
 #define BITSCRAMBLER_HW_REV 0
 
@@ -54,95 +62,92 @@ typedef struct {
 atomic_flag tx_in_use = ATOMIC_FLAG_INIT;
 atomic_flag rx_in_use = ATOMIC_FLAG_INIT;
 
-// Claim both TX and RX channels for loopback use
-// Returns true on success, false if any of the two directions already is claimed.
-static bool claim_channel_loopback(void)
-{
-    bool old_val_tx = atomic_flag_test_and_set(&tx_in_use);
-    if (old_val_tx) {
-        return false;
-    }
-    bool old_val_rx = atomic_flag_test_and_set(&rx_in_use);
-    if (old_val_rx) {
-        atomic_flag_clear(&tx_in_use);
-        return false;
-    }
-    return true;
-}
+// This is a reference count for the BitScrambler module. It is used to keep track of how many clients are using the module.
+atomic_int group_ref_count = 0;
 
 // Claim a channel using the direction it indicated.
 // Returns true on success, false if the direction already is claimed
 static bool claim_channel(bitscrambler_direction_t dir)
 {
+    int old_use_count = atomic_fetch_add(&group_ref_count, 1);
+    if (old_use_count == 0) {
+        BS_RCC_ATOMIC() {
+            // This is the first client using the module, so we need to enable the sys clock
+            bitscrambler_ll_set_bus_clock_sys_enable(true);
+            bitscrambler_ll_reset_sys();
+            // also power on the memory
+            bitscrambler_ll_mem_power_by_pmu();
+        }
+    }
     if (dir == BITSCRAMBLER_DIR_TX) {
         bool old_val = atomic_flag_test_and_set(&tx_in_use);
         if (old_val) {
-            return false;
+            goto err;
+        } else {
+            BS_RCC_ATOMIC() {
+                bitscrambler_ll_set_bus_clock_tx_enable(true);
+                bitscrambler_ll_reset_tx();
+            }
         }
-    } else if (dir == BITSCRAMBLER_DIR_RX) {
+    } else {
         bool old_val = atomic_flag_test_and_set(&rx_in_use);
         if (old_val) {
-            return false;
+            goto err;
+        } else {
+            BS_RCC_ATOMIC() {
+                bitscrambler_ll_set_bus_clock_rx_enable(true);
+                bitscrambler_ll_reset_rx();
+            }
         }
     }
     return true;
+err:
+    atomic_fetch_sub(&group_ref_count, 1);
+    return false;
 }
 
-//Initialize the BitScrambler object and hardware using the given config.
-static esp_err_t init_from_config(bitscrambler_t *bs, const bitscrambler_config_t *config)
-{
-    bs->cfg = *config;                  //Copy config over
-    bs->hw = BITSCRAMBLER_LL_GET_HW(0); //there's only one as of now; if there's more, we need to handle them as a pool.
-    return ESP_OK;
-}
-
-static void enable_clocks(bitscrambler_t *bs)
+// Release the channel using the direction it indicated.
+static void release_channel(bitscrambler_direction_t dir)
 {
-    PERIPH_RCC_ACQUIRE_ATOMIC(PERIPH_BITSCRAMBLER_MODULE, ref_count) {
-        if (ref_count == 0) { //we're the first to enable the BitScrambler module
-            bitscrambler_ll_set_bus_clock_sys_enable(1);
-            bitscrambler_ll_reset_sys();
-            bitscrambler_ll_mem_power_by_pmu();
-        }
-        if (bs->cfg.dir == BITSCRAMBLER_DIR_RX || bs->loopback) {
-            bitscrambler_ll_set_bus_clock_rx_enable(1);
-            bitscrambler_ll_reset_rx();
-        }
-        if (bs->cfg.dir == BITSCRAMBLER_DIR_TX || bs->loopback) {
-            bitscrambler_ll_set_bus_clock_tx_enable(1);
-            bitscrambler_ll_reset_tx();
+    if (dir == BITSCRAMBLER_DIR_TX) {
+        atomic_flag_clear(&tx_in_use);
+    } else if (dir == BITSCRAMBLER_DIR_RX) {
+        atomic_flag_clear(&rx_in_use);
+    }
+    int old_use_count = atomic_fetch_sub(&group_ref_count, 1);
+    if (old_use_count == 1) {
+        // This is the last client using the module, so we need to disable the sys clock
+        BS_RCC_ATOMIC() {
+            bitscrambler_ll_set_bus_clock_sys_enable(false);
+            bitscrambler_ll_mem_force_power_off();
         }
     }
 }
 
-static void disable_clocks(bitscrambler_t *bs)
+//Initialize the BitScrambler object and hardware using the given config.
+static esp_err_t init_from_config(bitscrambler_t *bs, const bitscrambler_config_t *config)
 {
-    PERIPH_RCC_RELEASE_ATOMIC(PERIPH_BITSCRAMBLER_MODULE, ref_count) {
-        if (bs->cfg.dir == BITSCRAMBLER_DIR_RX || bs->loopback) {
-            bitscrambler_ll_set_bus_clock_rx_enable(0);
-        }
-        if (bs->cfg.dir == BITSCRAMBLER_DIR_TX || bs->loopback) {
-            bitscrambler_ll_set_bus_clock_tx_enable(0);
-        }
-        if (ref_count == 0) { //we're the last to disable the BitScrambler module
-            bitscrambler_ll_set_bus_clock_sys_enable(0);
-            bitscrambler_ll_mem_force_power_off();
-        }
-    }
+    bs->cfg = *config;                  //Copy config over
+    bs->hw = BITSCRAMBLER_LL_GET_HW(0); //there's only one as of now; if there's more, we need to handle them as a pool.
+    return ESP_OK;
 }
 
-//Private function: init an existing BitScrambler object as a loopback BitScrambler.
+// init an existing BitScrambler object as a loopback BitScrambler, only used by the bitscrambler loopback driver
 esp_err_t bitscrambler_init_loopback(bitscrambler_handle_t handle, const bitscrambler_config_t *config)
 {
-    if (!claim_channel_loopback()) {
+    // claim the TX channel first
+    if (!claim_channel(BITSCRAMBLER_DIR_TX)) {
+        return ESP_ERR_NOT_FOUND;
+    }
+    // claim the RX channel, if it fails, release the TX channel
+    if (!claim_channel(BITSCRAMBLER_DIR_RX)) {
+        release_channel(BITSCRAMBLER_DIR_TX);
         return ESP_ERR_NOT_FOUND;
     }
 
-    assert(config->dir == BITSCRAMBLER_DIR_TX);
+    // mark the BitScrambler object as a loopback BitScrambler
     handle->loopback = true;
-    enable_clocks(handle);
-    esp_err_t r = init_from_config(handle, config);
-    return r;
+    return init_from_config(handle, config);
 }
 
 esp_err_t bitscrambler_new(const bitscrambler_config_t *config, bitscrambler_handle_t *handle)
@@ -172,8 +177,6 @@ esp_err_t bitscrambler_new(const bitscrambler_config_t *config, bitscrambler_han
         return r;
     }
 
-    enable_clocks(bs);
-
     // Return the handle
     *handle = bs;
     return ESP_OK;
@@ -304,14 +307,11 @@ void bitscrambler_free(bitscrambler_handle_t handle)
     if (!handle) {
         return;
     }
-    disable_clocks(handle);
     if (handle->loopback) {
-        atomic_flag_clear(&tx_in_use);
-        atomic_flag_clear(&rx_in_use);
-    } else if (handle->cfg.dir == BITSCRAMBLER_DIR_TX) {
-        atomic_flag_clear(&tx_in_use);
-    } else if (handle->cfg.dir == BITSCRAMBLER_DIR_RX) {
-        atomic_flag_clear(&rx_in_use);
+        release_channel(BITSCRAMBLER_DIR_TX);
+        release_channel(BITSCRAMBLER_DIR_RX);
+    } else {
+        release_channel(handle->cfg.dir);
     }
     if (handle->extra_clean_up) {
         handle->extra_clean_up(handle, handle->clean_up_user_ctx);

components/hal/esp32c5/include/hal/bitscrambler_ll.h

@@ -298,23 +298,23 @@ static inline bool bitscrambler_ll_is_fifo_ready(bitscrambler_dev_t *hw, bitscra
 /**
  * @brief Enable the bus clock for BitScrambler module
  */
-static inline void _bitscrambler_ll_set_bus_clock_sys_enable(bool enable)
+static inline void bitscrambler_ll_set_bus_clock_sys_enable(bool enable)
 {
     PCR.bs_conf.bs_clk_en = enable;
 }
 
 /**
  * @brief Enable the bus clock for RX BitScrambler module
  */
-static inline void _bitscrambler_ll_set_bus_clock_rx_enable(bool enable)
+static inline void bitscrambler_ll_set_bus_clock_rx_enable(bool enable)
 {
     // empty
 }
 
 /**
  * @brief Enable the bus clock for TX BitScrambler module
  */
-static inline void _bitscrambler_ll_set_bus_clock_tx_enable(bool enable)
+static inline void bitscrambler_ll_set_bus_clock_tx_enable(bool enable)
 {
     // empty
 }
@@ -349,7 +349,7 @@ static inline void bitscrambler_ll_mem_power_by_pmu(void)
 /**
  * @brief Reset the BitScrambler module
  */
-static inline void _bitscrambler_ll_reset_sys(void)
+static inline void bitscrambler_ll_reset_sys(void)
 {
     PCR.bs_conf.bs_rst_en = 1;
     PCR.bs_conf.bs_rst_en = 0;
@@ -358,7 +358,7 @@ static inline void _bitscrambler_ll_reset_sys(void)
 /**
  * @brief Reset the BitScrambler RX module
  */
-static inline void _bitscrambler_ll_reset_rx(void)
+static inline void bitscrambler_ll_reset_rx(void)
 {
     PCR.bs_func_conf.bs_rx_rst_en = 1;
     PCR.bs_func_conf.bs_rx_rst_en = 0;
@@ -367,22 +367,12 @@ static inline void _bitscrambler_ll_reset_rx(void)
 /**
  * @brief Reset the BitScrambler TX module
  */
-static inline void _bitscrambler_ll_reset_tx(void)
+static inline void bitscrambler_ll_reset_tx(void)
 {
     PCR.bs_func_conf.bs_tx_rst_en = 1;
     PCR.bs_func_conf.bs_tx_rst_en = 0;
 }
 
-/// use a macro to wrap the function, force the caller to use it in a critical section
-/// the critical section needs to declare the __DECLARE_RCC_RC_ATOMIC_ENV variable in advance
-#define bitscrambler_ll_set_bus_clock_sys_enable(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_set_bus_clock_sys_enable(__VA_ARGS__)
-#define bitscrambler_ll_set_bus_clock_rx_enable(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_set_bus_clock_rx_enable(__VA_ARGS__)
-#define bitscrambler_ll_set_bus_clock_tx_enable(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_set_bus_clock_tx_enable(__VA_ARGS__)
-
-#define bitscrambler_ll_reset_sys(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_reset_sys(__VA_ARGS__)
-#define bitscrambler_ll_reset_rx(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_reset_rx(__VA_ARGS__)
-#define bitscrambler_ll_reset_tx(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_reset_tx(__VA_ARGS__)
-
 #ifdef __cplusplus
 }
 #endif

components/hal/esp32p4/include/hal/bitscrambler_ll.h

@@ -303,6 +303,13 @@ static inline void _bitscrambler_ll_set_bus_clock_sys_enable(bool enable)
     HP_SYS_CLKRST.soc_clk_ctrl1.reg_bitscrambler_sys_clk_en = enable;
 }
 
+/// use a macro to wrap the function, force the caller to use it in a critical section
+/// the critical section needs to declare the __DECLARE_RCC_ATOMIC_ENV variable in advance
+#define bitscrambler_ll_set_bus_clock_sys_enable(...) do {  \
+    (void)__DECLARE_RCC_ATOMIC_ENV;                         \
+    _bitscrambler_ll_set_bus_clock_sys_enable(__VA_ARGS__); \
+} while (0)
+
 /**
  * @brief Enable the bus clock for RX BitScrambler module
  */
@@ -311,6 +318,13 @@ static inline void _bitscrambler_ll_set_bus_clock_rx_enable(bool enable)
     HP_SYS_CLKRST.soc_clk_ctrl1.reg_bitscrambler_rx_sys_clk_en = enable;
 }
 
+/// use a macro to wrap the function, force the caller to use it in a critical section
+/// the critical section needs to declare the __DECLARE_RCC_ATOMIC_ENV variable in advance
+#define bitscrambler_ll_set_bus_clock_rx_enable(...) do {  \
+    (void)__DECLARE_RCC_ATOMIC_ENV;                        \
+    _bitscrambler_ll_set_bus_clock_rx_enable(__VA_ARGS__); \
+} while (0)
+
 /**
  * @brief Enable the bus clock for TX BitScrambler module
  */
@@ -319,6 +333,13 @@ static inline void _bitscrambler_ll_set_bus_clock_tx_enable(bool enable)
     HP_SYS_CLKRST.soc_clk_ctrl1.reg_bitscrambler_tx_sys_clk_en = enable;
 }
 
+/// use a macro to wrap the function, force the caller to use it in a critical section
+/// the critical section needs to declare the __DECLARE_RCC_ATOMIC_ENV variable in advance
+#define bitscrambler_ll_set_bus_clock_tx_enable(...) do {  \
+    (void)__DECLARE_RCC_ATOMIC_ENV;                        \
+    _bitscrambler_ll_set_bus_clock_tx_enable(__VA_ARGS__); \
+} while (0)
+
 /**
  * @brief Force power on the bitscrambler memory block, regardless of the outside PMU logic
  */
@@ -352,6 +373,13 @@ static inline void _bitscrambler_ll_reset_sys(void)
     HP_SYS_CLKRST.hp_rst_en2.reg_rst_en_bitscrambler = 0;
 }
 
+/// use a macro to wrap the function, force the caller to use it in a critical section
+/// the critical section needs to declare the __DECLARE_RCC_ATOMIC_ENV variable in advance
+#define bitscrambler_ll_reset_sys(...) do {  \
+    (void)__DECLARE_RCC_ATOMIC_ENV;          \
+    _bitscrambler_ll_reset_sys(__VA_ARGS__); \
+} while (0)
+
 /**
  * @brief Reset the BitScrambler RX module
  */
@@ -361,6 +389,13 @@ static inline void _bitscrambler_ll_reset_rx(void)
     HP_SYS_CLKRST.hp_rst_en2.reg_rst_en_bitscrambler_rx = 0;
 }
 
+/// use a macro to wrap the function, force the caller to use it in a critical section
+/// the critical section needs to declare the __DECLARE_RCC_ATOMIC_ENV variable in advance
+#define bitscrambler_ll_reset_rx(...) do {  \
+    (void)__DECLARE_RCC_ATOMIC_ENV;         \
+    _bitscrambler_ll_reset_rx(__VA_ARGS__); \
+} while (0)
+
 /**
  * @brief Reset the BitScrambler TX module
  */
@@ -371,14 +406,11 @@ static inline void _bitscrambler_ll_reset_tx(void)
 }
 
 /// use a macro to wrap the function, force the caller to use it in a critical section
-/// the critical section needs to declare the __DECLARE_RCC_RC_ATOMIC_ENV variable in advance
-#define bitscrambler_ll_set_bus_clock_sys_enable(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_set_bus_clock_sys_enable(__VA_ARGS__)
-#define bitscrambler_ll_set_bus_clock_rx_enable(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_set_bus_clock_rx_enable(__VA_ARGS__)
-#define bitscrambler_ll_set_bus_clock_tx_enable(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_set_bus_clock_tx_enable(__VA_ARGS__)
-
-#define bitscrambler_ll_reset_sys(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_reset_sys(__VA_ARGS__)
-#define bitscrambler_ll_reset_rx(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_reset_rx(__VA_ARGS__)
-#define bitscrambler_ll_reset_tx(...) (void)__DECLARE_RCC_RC_ATOMIC_ENV; _bitscrambler_ll_reset_tx(__VA_ARGS__)
+/// the critical section needs to declare the __DECLARE_RCC_ATOMIC_ENV variable in advance
+#define bitscrambler_ll_reset_tx(...) do {  \
+    (void)__DECLARE_RCC_ATOMIC_ENV;         \
+    _bitscrambler_ll_reset_tx(__VA_ARGS__); \
+} while (0)
 
 #ifdef __cplusplus
 }