timer_group: update hal api && fix intr_enable

timer group interrupt enable is controled by level_int_ena instead of int_ena

Closes https://github.com/espressif/esp-idf/issues/5103

components/driver/timer.c

@@ -80,7 +80,7 @@ esp_err_t timer_get_counter_time_sec(timer_group_t group_num, timer_idx_t timer_
     uint64_t timer_val;
     esp_err_t err = timer_get_counter_value(group_num, timer_num, &timer_val);
     if (err == ESP_OK) {
-        uint16_t div;
+        uint32_t div;
         timer_hal_get_divider(&(p_timer_obj[group_num][timer_num]->hal), &div);
         *time = (double)timer_val * div / rtc_clk_apb_freq_get();
 #ifdef TIMER_GROUP_SUPPORTS_XTAL_CLOCK
@@ -262,7 +262,7 @@ esp_err_t timer_isr_register(timer_group_t group_num, timer_idx_t timer_num,
 
     int intr_source = 0;
     uint32_t status_reg = 0;
-    int mask = 0;
+    uint32_t mask = 0;
     switch (group_num) {
     case TIMER_GROUP_0:
     default:
@@ -271,8 +271,7 @@ esp_err_t timer_isr_register(timer_group_t group_num, timer_idx_t timer_num,
         } else {
             intr_source = ETS_TG0_T0_EDGE_INTR_SOURCE + timer_num;
         }
-        timer_hal_get_intr_status_reg(&(p_timer_obj[TIMER_GROUP_0][timer_num]->hal), &status_reg);
-        mask = 1 << timer_num;
+        timer_hal_get_status_reg_mask_bit(&(p_timer_obj[TIMER_GROUP_0][timer_num]->hal), &status_reg, &mask);
         break;
     case TIMER_GROUP_1:
         if ((intr_alloc_flags & ESP_INTR_FLAG_EDGE) == 0) {
@@ -280,8 +279,7 @@ esp_err_t timer_isr_register(timer_group_t group_num, timer_idx_t timer_num,
         } else {
             intr_source = ETS_TG1_T0_EDGE_INTR_SOURCE + timer_num;
         }
-        timer_hal_get_intr_status_reg(&(p_timer_obj[TIMER_GROUP_1][timer_num]->hal), &status_reg);
-        mask = 1 << timer_num;
+        timer_hal_get_status_reg_mask_bit(&(p_timer_obj[TIMER_GROUP_1][timer_num]->hal), &status_reg, &mask);
         break;
     }
     return esp_intr_alloc_intrstatus(intr_source, intr_alloc_flags, status_reg, mask, fn, arg, handle);
@@ -363,13 +361,9 @@ esp_err_t timer_get_config(timer_group_t group_num, timer_idx_t timer_num, timer
     config->counter_dir = timer_hal_get_counter_increase(&(p_timer_obj[group_num][timer_num]->hal));
     config->counter_en = timer_hal_get_counter_enable(&(p_timer_obj[group_num][timer_num]->hal));
 
-    uint16_t div;
+    uint32_t div;
     timer_hal_get_divider(&(p_timer_obj[group_num][timer_num]->hal), &div);
-    if (div == 0) {
-        config->divider = 65536;
-    } else {
     config->divider = div;
-    }
 
     if (timer_hal_get_level_int_enable(&(p_timer_obj[group_num][timer_num]->hal))) {
         config->intr_type = TIMER_INTR_LEVEL;

components/freertos/test/test_suspend_scheduler.c

@@ -113,6 +113,7 @@ TEST_CASE("Scheduler disabled can handle a pending context switch on resume", "[
         // When we resume scheduler, we expect the counter task
         // will preempt and count at least one more item
         esp_intr_noniram_enable();
+        timer_enable_intr(TIMER_GROUP_0, TIMER_0);
         xTaskResumeAll();
 
         TEST_ASSERT_NOT_EQUAL(count_config.counter, no_sched_task);

components/soc/include/hal/timer_hal.h

@@ -50,6 +50,15 @@ typedef struct {
  */
 void timer_hal_init(timer_hal_context_t *hal, timer_group_t group_num, timer_idx_t timer_num);
 
+/**
+ * @brief Get interrupt status register address and corresponding control bits mask
+ *
+ * @param hal Context of the HAL layer
+ * @param status_reg[out] interrupt status register address
+ * @param mask_bit[out] control bits mask
+ */
+void timer_hal_get_status_reg_mask_bit(timer_hal_context_t *hal, uint32_t *status_reg, uint32_t *mask_bit);
+
 /**
  * @brief Set timer clock prescale value
  *
@@ -288,13 +297,12 @@ void timer_hal_init(timer_hal_context_t *hal, timer_group_t group_num, timer_idx
  * @brief Get interrupt status register address.
  *
  * @param hal Context of the HAL layer
- * @param intr_status_reg Interrupt status register address
  *
- * @return None
+ * @return Interrupt status register address
  */
-#define timer_hal_get_intr_status_reg(hal, intr_status_reg)  timer_ll_get_intr_status_reg((hal)->dev, intr_status_reg)
+#define timer_hal_get_intr_status_reg(hal)  timer_ll_get_intr_status_reg((hal)->dev)
 
-#ifdef TIMER_GROUP_SUPPORTS_XTAL_CLOCK
+#ifdef SOC_TIMER_GROUP_SUPPORT_XTAL
 /**
  * @brief Set clock source.
  *

components/soc/include/hal/timer_types.h

@@ -98,7 +98,7 @@ typedef enum {
     TIMER_AUTORELOAD_MAX,
 } timer_autoreload_t;
 
-#ifdef TIMER_GROUP_SUPPORTS_XTAL_CLOCK
+#ifdef SOC_TIMER_GROUP_SUPPORT_XTAL
 /**
  * @brief Select timer source clock.
  */
@@ -118,7 +118,7 @@ typedef struct {
     timer_count_dir_t counter_dir; /*!< Counter direction  */
     timer_autoreload_t auto_reload;   /*!< Timer auto-reload */
     uint32_t divider;   /*!< Counter clock divider. The divider's range is from from 2 to 65536. */
-#ifdef TIMER_GROUP_SUPPORTS_XTAL_CLOCK
+#ifdef SOC_TIMER_GROUP_SUPPORT_XTAL
     timer_src_clk_t clk_src;  /*!< Use XTAL as source clock. */
 #endif
 } timer_config_t;

components/soc/soc/esp32/include/soc/timer_group_caps.h

@@ -13,5 +13,3 @@
 // limitations under the License.
 
 #pragma once
-
-#define WDT_SOURCE_CLK_FREQ_MHZ (80) // Watch Dog clock source comes from APB, which is 80MHz by default

components/soc/soc/esp32s2/include/soc/timer_group_caps.h

@@ -14,6 +14,4 @@
 
 #pragma once
 
-#define TIMER_GROUP_SUPPORTS_XTAL_CLOCK
-
-#define WDT_SOURCE_CLK_FREQ_MHZ (80) // Watch Dog clock source comes from APB, which is 80MHz by default
+#define SOC_TIMER_GROUP_SUPPORT_XTAL

components/soc/src/esp32/include/hal/timer_ll.h

@@ -29,11 +29,6 @@ _Static_assert(TIMER_INTR_T0 == TIMG_T0_INT_CLR, "Add mapping to LL interrupt ha
 _Static_assert(TIMER_INTR_T1 == TIMG_T1_INT_CLR, "Add mapping to LL interrupt handling, since it's no longer naturally compatible with the timer_intr_t");
 _Static_assert(TIMER_INTR_WDT == TIMG_WDT_INT_CLR, "Add mapping to LL interrupt handling, since it's no longer naturally compatible with the timer_intr_t");
 
-typedef struct {
-    timg_dev_t *dev;
-    timer_idx_t idx;
-} timer_ll_context_t;
-
 // Get timer group instance with giving group number
 #define TIMER_LL_GET_HW(num) ((num == 0) ? (&TIMERG0) : (&TIMERG1))
 
@@ -42,12 +37,18 @@ typedef struct {
  *
  * @param hw Beginning address of the peripheral registers.
  * @param timer_num The timer number
- * @param divider Prescale value
+ * @param divider Prescale value (0 and 1 are not valid)
  *
  * @return None
  */
-static inline void timer_ll_set_divider(timg_dev_t *hw, timer_idx_t timer_num, uint16_t divider)
+static inline void timer_ll_set_divider(timg_dev_t *hw, timer_idx_t timer_num, uint32_t divider)
 {
+    // refer to TRM 18.2.1
+    if (divider == 65536) {
+        divider = 0;
+    } else if (divider == 1) {
+        divider = 2;
+    }
     int timer_en = hw->hw_timer[timer_num].config.enable;
     hw->hw_timer[timer_num].config.enable = 0;
     hw->hw_timer[timer_num].config.divider = divider;
@@ -63,9 +64,15 @@ static inline void timer_ll_set_divider(timg_dev_t *hw, timer_idx_t timer_num, u
  *
  * @return None
  */
-static inline void timer_ll_get_divider(timg_dev_t *hw, timer_idx_t timer_num, uint16_t *divider)
+static inline void timer_ll_get_divider(timg_dev_t *hw, timer_idx_t timer_num, uint32_t *divider)
 {
-    *divider = hw->hw_timer[timer_num].config.divider;
+    uint32_t d = hw->hw_timer[timer_num].config.divider;
+    if (d == 0) {
+        d = 65536;
+    } else if (d == 1) {
+        d = 2;
+    }
+    *divider = d;
 }
 
 /**
@@ -255,6 +262,7 @@ static inline bool timer_ll_get_alarm_enable(timg_dev_t *hw, timer_idx_t timer_n
 FORCE_INLINE_ATTR void timer_ll_intr_enable(timg_dev_t *hw, timer_idx_t timer_num)
 {
     hw->int_ena.val |= BIT(timer_num);
+    hw->hw_timer[timer_num].config.level_int_en = 1;
 }
 
 /**
@@ -268,6 +276,7 @@ FORCE_INLINE_ATTR void timer_ll_intr_enable(timg_dev_t *hw, timer_idx_t timer_nu
 FORCE_INLINE_ATTR void timer_ll_intr_disable(timg_dev_t *hw, timer_idx_t timer_num)
 {
     hw->int_ena.val &= (~BIT(timer_num));
+    hw->hw_timer[timer_num].config.level_int_en = 0;
 }
 
 /**
@@ -372,13 +381,17 @@ static inline bool timer_ll_get_edge_int_enable(timg_dev_t *hw, timer_idx_t time
  * @brief Get interrupt status register address.
  *
  * @param hw Beginning address of the peripheral registers.
- * @param intr_status_reg Interrupt status register address
  *
- * @return None
+ * @return Interrupt status register address
  */
-static inline void timer_ll_get_intr_status_reg(timg_dev_t *hw, uint32_t *intr_status_reg)
+static inline uint32_t timer_ll_get_intr_status_reg(timg_dev_t *hw)
 {
-    *intr_status_reg = (uint32_t)&(hw->int_st_timers.val);
+    return (uint32_t) & (hw->int_st_timers.val);
+}
+
+static inline uint32_t timer_ll_get_intr_mask_bit(timg_dev_t *hw, timer_idx_t timer_num)
+{
+    return (1U << timer_num);
 }
 
 #ifdef __cplusplus

components/soc/src/esp32s2/include/hal/timer_ll.h

@@ -29,11 +29,6 @@ _Static_assert(TIMER_INTR_T0 == TIMG_T0_INT_CLR, "Add mapping to LL interrupt ha
 _Static_assert(TIMER_INTR_T1 == TIMG_T1_INT_CLR, "Add mapping to LL interrupt handling, since it's no longer naturally compatible with the timer_intr_t");
 _Static_assert(TIMER_INTR_WDT == TIMG_WDT_INT_CLR, "Add mapping to LL interrupt handling, since it's no longer naturally compatible with the timer_intr_t");
 
-typedef struct {
-    timg_dev_t *dev;
-    timer_idx_t idx;
-} timer_ll_context_t;
-
 // Get timer group instance with giving group number
 #define TIMER_LL_GET_HW(num) ((num == 0) ? (&TIMERG0) : (&TIMERG1))
 
@@ -42,12 +37,16 @@ typedef struct {
  *
  * @param hw Beginning address of the peripheral registers.
  * @param timer_num The timer number
- * @param divider Prescale value
+ * @param divider Prescale value (0 is not valid)
  *
  * @return None
  */
-static inline void timer_ll_set_divider(timg_dev_t *hw, timer_idx_t timer_num, uint16_t divider)
+static inline void timer_ll_set_divider(timg_dev_t *hw, timer_idx_t timer_num, uint32_t divider)
 {
+    // refer to TRM 12.2.1
+    if (divider == 65536) {
+        divider = 0;
+    }
     int timer_en = hw->hw_timer[timer_num].config.enable;
     hw->hw_timer[timer_num].config.enable = 0;
     hw->hw_timer[timer_num].config.divider = divider;
@@ -63,9 +62,13 @@ static inline void timer_ll_set_divider(timg_dev_t *hw, timer_idx_t timer_num, u
  *
  * @return None
  */
-static inline void timer_ll_get_divider(timg_dev_t *hw, timer_idx_t timer_num, uint16_t *divider)
+static inline void timer_ll_get_divider(timg_dev_t *hw, timer_idx_t timer_num, uint32_t *divider)
 {
-    *divider = hw->hw_timer[timer_num].config.divider;
+    uint32_t d = hw->hw_timer[timer_num].config.divider;
+    if (d == 0) {
+        d = 65536;
+    }
+    *divider = d;
 }
 
 /**
@@ -255,6 +258,7 @@ static inline bool timer_ll_get_alarm_enable(timg_dev_t *hw, timer_idx_t timer_n
 FORCE_INLINE_ATTR void timer_ll_intr_enable(timg_dev_t *hw, timer_idx_t timer_num)
 {
     hw->int_ena.val |= BIT(timer_num);
+    hw->hw_timer[timer_num].config.level_int_en = 1;
 }
 
 /**
@@ -268,6 +272,7 @@ FORCE_INLINE_ATTR void timer_ll_intr_enable(timg_dev_t *hw, timer_idx_t timer_nu
 FORCE_INLINE_ATTR void timer_ll_intr_disable(timg_dev_t *hw, timer_idx_t timer_num)
 {
     hw->int_ena.val &= (~BIT(timer_num));
+    hw->hw_timer[timer_num].config.level_int_en = 0;
 }
 
 /**
@@ -372,13 +377,17 @@ static inline bool timer_ll_get_edge_int_enable(timg_dev_t *hw, timer_idx_t time
  * @brief Get interrupt status register address.
  *
  * @param hw Beginning address of the peripheral registers.
- * @param intr_status_reg Interrupt status register address
  *
- * @return None
+ * @return uint32_t Interrupt status register address
  */
-static inline void timer_ll_get_intr_status_reg(timg_dev_t *hw, uint32_t *intr_status_reg)
+static inline uint32_t timer_ll_get_intr_status_reg(timg_dev_t *hw)
 {
-    *intr_status_reg = (uint32_t)&(hw->int_st.val);
+    return (uint32_t) & (hw->int_st.val);
+}
+
+static inline uint32_t timer_ll_get_intr_mask_bit(timg_dev_t *hw, timer_idx_t timer_num)
+{
+    return (1U << timer_num);
 }
 
 /**

components/soc/src/hal/timer_hal.c

@@ -20,3 +20,9 @@ void timer_hal_init(timer_hal_context_t *hal, timer_group_t group_num, timer_idx
     hal->dev = TIMER_LL_GET_HW(group_num);
     hal->idx = timer_num;
 }
+
+void timer_hal_get_status_reg_mask_bit(timer_hal_context_t *hal, uint32_t *status_reg, uint32_t *mask_bit)
+{
+    *status_reg = timer_ll_get_intr_status_reg(hal->dev);
+    *mask_bit = timer_ll_get_intr_mask_bit(hal->dev, hal->idx);
+}

examples/peripherals/timer_group/main/timer_group_example_main.c

@@ -104,16 +104,13 @@ static void example_tg0_timer_init(int timer_idx,
                                    bool auto_reload, double timer_interval_sec)
 {
     /* Select and initialize basic parameters of the timer */
-    timer_config_t config;
-    config.divider = TIMER_DIVIDER;
-    config.counter_dir = TIMER_COUNT_UP;
-    config.counter_en = TIMER_PAUSE;
-    config.alarm_en = TIMER_ALARM_EN;
-    config.intr_type = TIMER_INTR_LEVEL;
-    config.auto_reload = auto_reload;
-#ifdef TIMER_GROUP_SUPPORTS_XTAL_CLOCK
-    config.clk_src = TIMER_SRC_CLK_APB;
-#endif
+    timer_config_t config = {
+        .divider = TIMER_DIVIDER,
+        .counter_dir = TIMER_COUNT_UP,
+        .counter_en = TIMER_PAUSE,
+        .alarm_en = TIMER_ALARM_EN,
+        .auto_reload = auto_reload,
+    }; // default clock source is APB
     timer_init(TIMER_GROUP_0, timer_idx, &config);
 
     /* Timer's counter will initially start from value below.