Merge branch 'feature/esp_timer_uses_shared_isr' into 'master'

esp_timer: Adds TASK and ISR AFFINITY to core options

Closes IDFGH-9053

See merge request espressif/esp-idf!21923

components/esp_system/system_init_fn.txt

@@ -14,7 +14,7 @@
 
 
 # esp_timer has to be initialized early, since it is used by several other components
-100: esp_timer_startup_init in components/esp_timer/src/esp_timer.c on BIT(0)
+100: esp_timer_startup_init in components/esp_timer/src/esp_timer.c on CONFIG_ESP_TIMER_ISR_AFFINITY
 
 # esp_sleep doesn't have init dependencies
 105: esp_sleep_startup_init in components/esp_hw_support/sleep_gpio.c on BIT(0)

components/esp_timer/Kconfig

@@ -30,7 +30,7 @@ menu "High resolution timer (esp_timer)"
 
             Note that this is not the same as FreeRTOS timer task. To configure
             FreeRTOS timer task size, see "FreeRTOS timer task stack size" option
-            in "FreeRTOS" menu.
+            in "FreeRTOS".
 
     config ESP_TIMER_INTERRUPT_LEVEL
         int "Interrupt level"
@@ -41,6 +41,69 @@ menu "High resolution timer (esp_timer)"
             It sets the interrupt level for esp_timer ISR in range 1..3.
             A higher level (3) helps to decrease the ISR esp_timer latency.
 
+    config ESP_TIMER_SHOW_EXPERIMENTAL
+        bool "show esp_timer's experimental features"
+        help
+            This shows some hidden features of esp_timer.
+            Note that they may break other features, use them with care.
+
+    config ESP_TIMER_TASK_AFFINITY
+        hex
+        default 0x0 if ESP_TIMER_TASK_AFFINITY_CPU0
+        default 0x1 if ESP_TIMER_TASK_AFFINITY_CPU1
+        default FREERTOS_NO_AFFINITY if ESP_TIMER_TASK_AFFINITY_NO_AFFINITY
+
+    choice ESP_TIMER_TASK_AFFINITY
+        prompt "esp_timer task core affinity"
+        default ESP_TIMER_TASK_AFFINITY_CPU0
+        help
+            The default settings: timer TASK on CPU0 and timer ISR on CPU0.
+            Other settings may help in certain cases, but note that they may break
+            other features, use them with care.
+            - "CPU0": (default) esp_timer task is processed by CPU0.
+            - "CPU1": esp_timer task is processed by CPU1.
+            - "No affinity": esp_timer task can be processed by any CPU.
+
+        config ESP_TIMER_TASK_AFFINITY_CPU0
+            bool "CPU0"
+        config ESP_TIMER_TASK_AFFINITY_CPU1
+            bool "CPU1"
+            depends on !FREERTOS_UNICORE && ESP_TIMER_SHOW_EXPERIMENTAL
+        config ESP_TIMER_TASK_AFFINITY_NO_AFFINITY
+            bool "No affinity"
+            depends on !FREERTOS_UNICORE && ESP_TIMER_SHOW_EXPERIMENTAL
+    endchoice
+
+    config ESP_TIMER_ISR_AFFINITY
+        hex
+        default 0x1 if ESP_TIMER_ISR_AFFINITY_CPU0
+        default 0x2 if ESP_TIMER_ISR_AFFINITY_CPU1
+        default FREERTOS_NO_AFFINITY if ESP_TIMER_ISR_AFFINITY_NO_AFFINITY
+
+    choice ESP_TIMER_ISR_AFFINITY
+        prompt "timer interrupt core affinity"
+        default ESP_TIMER_ISR_AFFINITY_CPU0
+        help
+            The default settings: timer TASK on CPU0 and timer ISR on CPU0.
+            Other settings may help in certain cases, but note that they may break
+            other features, use them with care.
+            - "CPU0": (default) timer interrupt is processed by CPU0.
+            - "CPU1": timer interrupt is processed by CPU1.
+            - "No affinity": timer interrupt can be processed by any CPU. It helps
+            to reduce latency but there is a disadvantage it leads to the timer ISR
+            running on every core. It increases the CPU time usage for timer ISRs
+            by N on an N-core system.
+
+        config ESP_TIMER_ISR_AFFINITY_CPU0
+            bool "CPU0"
+        config ESP_TIMER_ISR_AFFINITY_CPU1
+            bool "CPU1"
+            depends on !FREERTOS_UNICORE && ESP_TIMER_SHOW_EXPERIMENTAL
+        config ESP_TIMER_ISR_AFFINITY_NO_AFFINITY
+            bool "No affinity"
+            depends on !FREERTOS_UNICORE && ESP_TIMER_SHOW_EXPERIMENTAL
+    endchoice
+
     config ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD
         bool "Support ISR dispatch method"
         default n

components/esp_timer/include/esp_timer.h

@@ -98,6 +98,11 @@ esp_err_t esp_timer_early_init(void);
  * Before calling this function, esp_timer_early_init must be called by the
  * startup code.
  *
+ * This function will be called from startup code on every core
+ * if CONFIG_ESP_TIMER_ISR_AFFINITY_NO_AFFINITY is enabled,
+ * It allocates the timer ISR on MULTIPLE cores and
+ * creates the timer task which can be run on any core.
+ *
  * @return
  *      - ESP_OK on success
  *      - ESP_ERR_NO_MEM if allocation has failed

components/esp_timer/src/esp_timer.c

@@ -15,6 +15,7 @@
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
 #include "freertos/semphr.h"
+#include "esp_ipc.h"
 #include "esp_timer.h"
 #include "esp_timer_impl.h"
 
@@ -479,37 +480,58 @@ esp_err_t esp_timer_early_init(void)
     return ESP_OK;
 }
 
-esp_err_t esp_timer_init(void)
+static esp_err_t init_timer_task(void)
 {
-    esp_err_t err;
+    esp_err_t err = ESP_OK;
     if (is_initialized()) {
-        return ESP_ERR_INVALID_STATE;
-    }
-
-    int ret = xTaskCreatePinnedToCore(&timer_task, "esp_timer",
-            ESP_TASK_TIMER_STACK, NULL, ESP_TASK_TIMER_PRIO, &s_timer_task, PRO_CPU_NUM);
-    if (ret != pdPASS) {
-        err = ESP_ERR_NO_MEM;
-        goto out;
-    }
-
-    err = esp_timer_impl_init(&timer_alarm_handler);
-    if (err != ESP_OK) {
-        goto out;
+        ESP_EARLY_LOGE(TAG, "Task is already initialized");
+        err = ESP_ERR_INVALID_STATE;
+    } else {
+        int ret = xTaskCreatePinnedToCore(
+                    &timer_task, "esp_timer",
+                    ESP_TASK_TIMER_STACK, NULL, ESP_TASK_TIMER_PRIO,
+                    &s_timer_task, CONFIG_ESP_TIMER_TASK_AFFINITY);
+        if (ret != pdPASS) {
+            ESP_EARLY_LOGE(TAG, "Not enough memory to create timer task");
+            err = ESP_ERR_NO_MEM;
+        }
     }
+    return err;
+}
 
-    return ESP_OK;
-
-out:
+static void deinit_timer_task(void)
+{
     if (s_timer_task) {
         vTaskDelete(s_timer_task);
         s_timer_task = NULL;
     }
+}
 
-    return ESP_ERR_NO_MEM;
+esp_err_t esp_timer_init(void)
+{
+    esp_err_t err = ESP_OK;
+#ifndef CONFIG_ESP_TIMER_ISR_AFFINITY_NO_AFFINITY
+    err = init_timer_task();
+#else
+    /* This function will be run on all cores if CONFIG_ESP_TIMER_ISR_AFFINITY_NO_AFFINITY is enabled,
+     * We do it that way because we need to allocate the timer ISR on MULTIPLE cores.
+     * timer task will be created by CPU0.
+     */
+    if (xPortGetCoreID() == 0) {
+        err = init_timer_task();
+    }
+#endif // CONFIG_ESP_TIMER_ISR_AFFINITY_NO_AFFINITY
+    if (err == ESP_OK) {
+        err = esp_timer_impl_init(&timer_alarm_handler);
+        if (err != ESP_OK) {
+            ESP_EARLY_LOGE(TAG, "ISR init failed");
+            deinit_timer_task();
+        }
+    }
+    return err;
 }
 
-ESP_SYSTEM_INIT_FN(esp_timer_startup_init, BIT(0), 100)
+ESP_SYSTEM_INIT_FN(esp_timer_startup_init, CONFIG_ESP_TIMER_ISR_AFFINITY, 100)
 {
     return esp_timer_init();
 }
@@ -539,9 +561,7 @@ esp_err_t esp_timer_deinit(void)
 #endif
 
     esp_timer_impl_deinit();
-
-    vTaskDelete(s_timer_task);
-    s_timer_task = NULL;
+    deinit_timer_task();
     return ESP_OK;
 }
 

components/esp_timer/src/esp_timer_impl_lac.c

@@ -83,8 +83,15 @@ typedef struct {
 
 static const char* TAG = "esp_timer_impl";
 
+#define NOT_USED 0xBAD00FAD
+
 /* Interrupt handle returned by the interrupt allocator */
-static intr_handle_t s_timer_interrupt_handle;
+#ifdef CONFIG_ESP_TIMER_ISR_AFFINITY_NO_AFFINITY
+#define ISR_HANDLERS (portNUM_PROCESSORS)
+#else
+#define ISR_HANDLERS (1)
+#endif
+static intr_handle_t s_timer_interrupt_handle[ISR_HANDLERS] = { NULL };
 
 /* Function from the upper layer to be called when the interrupt happens.
  * Registered in esp_timer_impl_init.
@@ -180,10 +187,47 @@ void IRAM_ATTR esp_timer_impl_set_alarm(uint64_t timestamp)
 
 static void IRAM_ATTR timer_alarm_isr(void *arg)
 {
+#if ISR_HANDLERS == 1
     /* Clear interrupt status */
     REG_WRITE(INT_CLR_REG, TIMG_LACT_INT_CLR);
-    /*  Call the upper layer handler */
+    /* Call the upper layer handler */
     (*s_alarm_handler)(arg);
+#else
+    static volatile uint32_t processed_by = NOT_USED;
+    static volatile bool pending_alarm = false;
+    /* CRITICAL section ensures the read/clear is atomic between cores */
+    portENTER_CRITICAL_ISR(&s_time_update_lock);
+    if (REG_GET_FIELD(INT_ST_REG, TIMG_LACT_INT_ST)) {
+        // Clear interrupt status
+        REG_WRITE(INT_CLR_REG, TIMG_LACT_INT_CLR);
+        // Is the other core already processing a previous alarm?
+        if (processed_by == NOT_USED) {
+            // Current core is not processing an alarm yet
+            processed_by = xPortGetCoreID();
+            do {
+                pending_alarm = false;
+                // Clear interrupt status
+                REG_WRITE(INT_CLR_REG, TIMG_LACT_INT_CLR);
+                portEXIT_CRITICAL_ISR(&s_time_update_lock);
+
+                (*s_alarm_handler)(arg);
+
+                portENTER_CRITICAL_ISR(&s_time_update_lock);
+               // Another alarm could have occurred while were handling the previous alarm.
+               // Check if we need to call the s_alarm_handler again:
+               //   1) if the alarm has already been fired, it helps to handle it immediately without an additional ISR call.
+               //   2) handle pending alarm that was cleared by the other core in time when this core worked with the current alarm.
+            } while (REG_GET_FIELD(INT_ST_REG, TIMG_LACT_INT_ST) || pending_alarm);
+            processed_by = NOT_USED;
+        } else {
+            // Current core arrived at ISR but the other core is still handling a previous alarm.
+            // Once we already cleared the ISR status we need to let the other core know that it was.
+            // Set the flag to handle the current alarm by the other core later.
+            pending_alarm = true;
+        }
+    }
+    portEXIT_CRITICAL_ISR(&s_time_update_lock);
+#endif // ISR_HANDLERS != 1
 }
 
 void IRAM_ATTR esp_timer_impl_update_apb_freq(uint32_t apb_ticks_per_us)
@@ -232,45 +276,61 @@ esp_err_t esp_timer_impl_early_init(void)
 
 esp_err_t esp_timer_impl_init(intr_handler_t alarm_handler)
 {
-    s_alarm_handler = alarm_handler;
+    if (s_timer_interrupt_handle[(ISR_HANDLERS == 1) ? 0 : xPortGetCoreID()] != NULL) {
+        ESP_EARLY_LOGE(TAG, "timer ISR is already initialized");
+        return ESP_ERR_INVALID_STATE;
+    }
+
+    int isr_flags = ESP_INTR_FLAG_INTRDISABLED
+                    | ((1 << CONFIG_ESP_TIMER_INTERRUPT_LEVEL) & ESP_INTR_FLAG_LEVELMASK)
+                    | ESP_INTR_FLAG_IRAM;
 
-    const int interrupt_lvl = (1 << CONFIG_ESP_TIMER_INTERRUPT_LEVEL) & ESP_INTR_FLAG_LEVELMASK;
-    esp_err_t err = esp_intr_alloc(INTR_SOURCE_LACT,
-            ESP_INTR_FLAG_INTRDISABLED | ESP_INTR_FLAG_IRAM | interrupt_lvl,
-            &timer_alarm_isr, NULL, &s_timer_interrupt_handle);
+    esp_err_t err = esp_intr_alloc(INTR_SOURCE_LACT, isr_flags,
+                                   &timer_alarm_isr, NULL,
+                                   &s_timer_interrupt_handle[(ISR_HANDLERS == 1) ? 0 : xPortGetCoreID()]);
 
     if (err != ESP_OK) {
-        ESP_EARLY_LOGE(TAG, "esp_intr_alloc failed (0x%0x)", err);
+        ESP_EARLY_LOGE(TAG, "Can not allocate ISR handler (0x%0x)", err);
         return err;
     }
 
-    /* In theory, this needs a shared spinlock with the timer group driver.
-     * However since esp_timer_impl_init is called early at startup, this
-     * will not cause issues in practice.
-     */
-    REG_SET_BIT(INT_ENA_REG, TIMG_LACT_INT_ENA);
+    if (s_alarm_handler == NULL) {
+        s_alarm_handler = alarm_handler;
+        /* In theory, this needs a shared spinlock with the timer group driver.
+        * However since esp_timer_impl_init is called early at startup, this
+        * will not cause issues in practice.
+        */
+        REG_SET_BIT(INT_ENA_REG, TIMG_LACT_INT_ENA);
 
-    esp_timer_impl_update_apb_freq(esp_clk_apb_freq() / 1000000);
+        esp_timer_impl_update_apb_freq(esp_clk_apb_freq() / 1000000);
 
-    // Set the step for the sleep mode when the timer will work
-    // from a slow_clk frequency instead of the APB frequency.
-    uint32_t slowclk_ticks_per_us = esp_clk_slowclk_cal_get() * TICKS_PER_US;
-    REG_SET_FIELD(RTC_STEP_REG, TIMG_LACT_RTC_STEP_LEN, slowclk_ticks_per_us);
+        // Set the step for the sleep mode when the timer will work
+        // from a slow_clk frequency instead of the APB frequency.
+        uint32_t slowclk_ticks_per_us = esp_clk_slowclk_cal_get() * TICKS_PER_US;
+        REG_SET_FIELD(RTC_STEP_REG, TIMG_LACT_RTC_STEP_LEN, slowclk_ticks_per_us);
+    }
 
-    ESP_ERROR_CHECK( esp_intr_enable(s_timer_interrupt_handle) );
+    err = esp_intr_enable(s_timer_interrupt_handle[(ISR_HANDLERS == 1) ? 0 : xPortGetCoreID()]);
+    if (err != ESP_OK) {
+        ESP_EARLY_LOGE(TAG, "Can not enable ISR (0x%0x)", err);
+    }
 
-    return ESP_OK;
+    return err;
 }
 
 void esp_timer_impl_deinit(void)
 {
     REG_WRITE(CONFIG_REG, 0);
     REG_SET_BIT(INT_CLR_REG, TIMG_LACT_INT_CLR);
     /* TODO: also clear TIMG_LACT_INT_ENA; however see the note in esp_timer_impl_init. */
-
-    esp_intr_disable(s_timer_interrupt_handle);
-    esp_intr_free(s_timer_interrupt_handle);
-    s_timer_interrupt_handle = NULL;
+    for (unsigned i = 0; i < ISR_HANDLERS; i++) {
+        if (s_timer_interrupt_handle[i] != NULL) {
+            esp_intr_disable(s_timer_interrupt_handle[i]);
+            esp_intr_free(s_timer_interrupt_handle[i]);
+            s_timer_interrupt_handle[i] = NULL;
+        }
+    }
+    s_alarm_handler = NULL;
 }
 
 /* FIXME: This value is safe for 80MHz APB frequency, should be modified to depend on clock frequency. */