From f81772210976dc1ffbaaff6218b7edcb6b06162f Mon Sep 17 00:00:00 2001 From: songruojing Date: Wed, 17 Nov 2021 12:07:00 +0800 Subject: [PATCH 1/2] gpio: Bugfix - Move esp_intr_free() out of the critical section in gpio_uninstall_isr_service() Closes https://github.com/espressif/esp-idf/issues/5571 Fix the bug that if the API was called from one core to free the interrupt source on the other core, it would trigger interrupt watchdog. (cherry picked from commit 0e8286c57b33ca76183151c677f3880c6485db79) --- components/driver/gpio.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/components/driver/gpio.c b/components/driver/gpio.c index 3054bdaf96..bfab495634 100644 --- a/components/driver/gpio.c +++ b/components/driver/gpio.c @@ -502,15 +502,21 @@ esp_err_t gpio_isr_handler_remove(gpio_num_t gpio_num) void gpio_uninstall_isr_service(void) { + gpio_isr_func_t *gpio_isr_func_free = NULL; + gpio_isr_handle_t gpio_isr_handle_free = NULL; + portENTER_CRITICAL(&gpio_context.gpio_spinlock); if (gpio_context.gpio_isr_func == NULL) { + portEXIT_CRITICAL(&gpio_context.gpio_spinlock); return; } - portENTER_CRITICAL(&gpio_context.gpio_spinlock); - esp_intr_free(gpio_context.gpio_isr_handle); - free(gpio_context.gpio_isr_func); + gpio_isr_func_free = gpio_context.gpio_isr_func; gpio_context.gpio_isr_func = NULL; + gpio_isr_handle_free = gpio_context.gpio_isr_handle; + gpio_context.gpio_isr_handle = NULL; gpio_context.isr_core_id = GPIO_ISR_CORE_ID_UNINIT; portEXIT_CRITICAL(&gpio_context.gpio_spinlock); + esp_intr_free(gpio_isr_handle_free); + free(gpio_isr_func_free); return; } @@ -543,7 +549,12 @@ esp_err_t gpio_isr_register(void (*fn)(void *), void *arg, int intr_alloc_flags, #else /* CONFIG_FREERTOS_UNICORE */ ret = esp_ipc_call_blocking(gpio_context.isr_core_id, gpio_isr_register_on_core_static, (void *)&p); #endif /* !CONFIG_FREERTOS_UNICORE */ - if(ret != ESP_OK || p.ret != ESP_OK) { + if (ret != ESP_OK) { + ESP_LOGE(GPIO_TAG, "esp_ipc_call_blocking failed (0x%x)", ret); + return ESP_ERR_NOT_FOUND; + } + if (p.ret != ESP_OK) { + ESP_LOGE(GPIO_TAG, "esp_intr_alloc failed (0x%x)", p.ret); return ESP_ERR_NOT_FOUND; } return ESP_OK; From f5f7a77895bc5becb8daf2010296964cb22e4bfa Mon Sep 17 00:00:00 2001 From: songruojing Date: Thu, 30 Dec 2021 13:06:00 +0800 Subject: [PATCH 2/2] gpio: Fix the bug that gpio interrupt cannot be triggered on app cpu on ESP32S3 Closes https://github.com/espressif/esp-idf/issues/7885 (cherry picked from commit 91f1159f9c74a120dcacec46fbc7f66f48e169cb) --- components/driver/test/test_gpio.c | 45 +++++++++++++++++-- components/hal/esp32s3/include/hal/gpio_ll.h | 14 +++--- .../soc/esp32s3/include/soc/gpio_struct.h | 14 +++--- 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/components/driver/test/test_gpio.c b/components/driver/test/test_gpio.c index c8b3a69c26..7a78f9255b 100644 --- a/components/driver/test/test_gpio.c +++ b/components/driver/test/test_gpio.c @@ -15,6 +15,7 @@ #include "sdkconfig.h" #include "esp_rom_uart.h" #include "esp_rom_sys.h" +#include "test_utils.h" #define WAKE_UP_IGNORE 1 // gpio_wakeup function development is not completed yet, set it deprecated. @@ -76,16 +77,16 @@ static gpio_config_t init_io(gpio_num_t num) return io_conf; } -#if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2, ESP32S3, ESP32C3) -//No runners // edge interrupt event -static void gpio_isr_edge_handler(void* arg) +__attribute__((unused)) static void gpio_isr_edge_handler(void* arg) { uint32_t gpio_num = (uint32_t) arg; - esp_rom_printf("GPIO[%d] intr, val: %d\n", gpio_num, gpio_get_level(gpio_num)); + esp_rom_printf("GPIO[%d] intr on core %d, val: %d\n", gpio_num, cpu_hal_get_core_id(), gpio_get_level(gpio_num)); edge_intr_times++; } +#if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2, ESP32S3, ESP32C3) +//No runners // level interrupt event with "gpio_intr_disable" static void gpio_isr_level_handler(void* arg) { @@ -388,6 +389,42 @@ TEST_CASE("GPIO enable and disable interrupt test", "[gpio][test_env=UT_T1_GPIO] } #endif //DISABLED_FOR_TARGETS(ESP32S2, ESP32S3, ESP32C3) +#if !CONFIG_FREERTOS_UNICORE +static void install_isr_service_task(void *arg) +{ + uint32_t gpio_num = (uint32_t) arg; + //rising edge intr + TEST_ESP_OK(gpio_set_intr_type(gpio_num, GPIO_INTR_POSEDGE)); + TEST_ESP_OK(gpio_install_isr_service(0)); + gpio_isr_handler_add(gpio_num, gpio_isr_edge_handler, (void *) gpio_num); + vTaskSuspend(NULL); +} + +TEST_CASE("GPIO interrupt on other CPUs test", "[gpio]") +{ + TaskHandle_t gpio_task_handle; + gpio_config_t input_output_io = init_io(TEST_GPIO_EXT_OUT_IO); + input_output_io.mode = GPIO_MODE_INPUT_OUTPUT; + input_output_io.pull_up_en = 1; + TEST_ESP_OK(gpio_config(&input_output_io)); + + for (int cpu_num = 1; cpu_num < portNUM_PROCESSORS; ++cpu_num) { + // We assume unit-test task is running on core 0, so we install gpio interrupt on other cores + edge_intr_times = 0; + TEST_ESP_OK(gpio_set_level(TEST_GPIO_EXT_OUT_IO, 0)); + xTaskCreatePinnedToCore(install_isr_service_task, "install_isr_service_task", 2048, (void *) TEST_GPIO_EXT_OUT_IO, 1, &gpio_task_handle, cpu_num); + + vTaskDelay(200 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_set_level(TEST_GPIO_EXT_OUT_IO, 1)); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ASSERT_EQUAL_INT(edge_intr_times, 1); + gpio_isr_handler_remove(TEST_GPIO_EXT_OUT_IO); + gpio_uninstall_isr_service(); + test_utils_task_delete(gpio_task_handle); + } +} +#endif //!CONFIG_FREERTOS_UNICORE + // ESP32 Connect GPIO18 with GPIO19, ESP32-S2 Connect GPIO17 with GPIO21, // ESP32-S3 Connect GPIO19 with GPIO20, ESP32C3 Connect GPIO2 with GPIO3 // use multimeter to test the voltage, so it is ignored in CI diff --git a/components/hal/esp32s3/include/hal/gpio_ll.h b/components/hal/esp32s3/include/hal/gpio_ll.h index 234b477578..3e3c092664 100644 --- a/components/hal/esp32s3/include/hal/gpio_ll.h +++ b/components/hal/esp32s3/include/hal/gpio_ll.h @@ -35,8 +35,9 @@ extern "C" { // Get GPIO hardware instance with giving gpio num #define GPIO_LL_GET_HW(num) (((num) == 0) ? (&GPIO) : NULL) -#define GPIO_LL_PRO_CPU_INTR_ENA (BIT(0)) -#define GPIO_LL_PRO_CPU_NMI_INTR_ENA (BIT(1)) +// On ESP32S3, pro cpu and app cpu shares the same interrupt enable bit +#define GPIO_LL_INTR_ENA (BIT(0)) +#define GPIO_LL_NMI_INTR_ENA (BIT(1)) /** * @brief Enable pull-up on GPIO. @@ -103,6 +104,8 @@ static inline void gpio_ll_set_intr_type(gpio_dev_t *hw, gpio_num_t gpio_num, gp */ static inline void gpio_ll_get_intr_status(gpio_dev_t *hw, uint32_t core_id, uint32_t *status) { + // On ESP32S3, pcpu_int register represents GPIO0-31 interrupt status on both cores + (void)core_id; *status = hw->pcpu_int; } @@ -115,6 +118,8 @@ static inline void gpio_ll_get_intr_status(gpio_dev_t *hw, uint32_t core_id, uin */ static inline void gpio_ll_get_intr_status_high(gpio_dev_t *hw, uint32_t core_id, uint32_t *status) { + // On ESP32S3, pcpu_int1 register represents GPIO32-48 interrupt status on both cores + (void)core_id; *status = hw->pcpu_int1.intr; } @@ -149,9 +154,8 @@ static inline void gpio_ll_clear_intr_status_high(gpio_dev_t *hw, uint32_t mask) */ static inline void gpio_ll_intr_enable_on_core(gpio_dev_t *hw, uint32_t core_id, gpio_num_t gpio_num) { - if (core_id == 0) { - GPIO.pin[gpio_num].int_ena = GPIO_LL_PRO_CPU_INTR_ENA; //enable pro cpu intr - } + (void)core_id; + GPIO.pin[gpio_num].int_ena = GPIO_LL_INTR_ENA; //enable intr } /** diff --git a/components/soc/esp32s3/include/soc/gpio_struct.h b/components/soc/esp32s3/include/soc/gpio_struct.h index ff589aac9b..fdcca9a038 100644 --- a/components/soc/esp32s3/include/soc/gpio_struct.h +++ b/components/soc/esp32s3/include/soc/gpio_struct.h @@ -115,20 +115,20 @@ typedef volatile struct { }; uint32_t val; } status1_w1tc; - uint32_t pcpu_int; /**/ - uint32_t pcpu_nmi_int; /**/ - uint32_t cpusdio_int; /**/ + uint32_t pcpu_int; /*GPIO0~31 PRO & APP CPU interrupt status*/ + uint32_t pcpu_nmi_int; /*GPIO0~31 PRO & APP CPU non-maskable interrupt status*/ + uint32_t cpusdio_int; union { struct { - uint32_t intr: 22; - uint32_t reserved22: 10; + uint32_t intr : 22; /*GPIO32-48 PRO & APP CPU interrupt status*/ + uint32_t reserved22 : 10; }; uint32_t val; } pcpu_int1; union { struct { - uint32_t intr: 22; - uint32_t reserved22: 10; + uint32_t intr : 22; /*GPIO32-48 PRO & APP CPU non-maskable interrupt status*/ + uint32_t reserved22 : 10; }; uint32_t val; } pcpu_nmi_int1;