From 235bb6f2942370b81e31d79e59dc81c261733fe8 Mon Sep 17 00:00:00 2001 From: LiPeng Date: Fri, 8 Jul 2022 19:22:27 +0800 Subject: [PATCH 1/3] fix(uart): Fixed issue that TX be blocked by auto-lightsleep --- components/esp_driver_uart/src/uart.c | 45 +++++++++-- .../uart/main/test_uart_auto_lightsleep.c | 80 +++++++++++++++++++ 2 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c diff --git a/components/esp_driver_uart/src/uart.c b/components/esp_driver_uart/src/uart.c index 508e6fc350..98a0b912e6 100644 --- a/components/esp_driver_uart/src/uart.c +++ b/components/esp_driver_uart/src/uart.c @@ -31,6 +31,7 @@ #include "sdkconfig.h" #include "esp_rom_gpio.h" #include "clk_ctrl_os.h" +#include "esp_pm.h" #ifdef CONFIG_UART_ISR_IN_IRAM #define UART_ISR_ATTR IRAM_ATTR @@ -40,6 +41,14 @@ #define UART_MALLOC_CAPS MALLOC_CAP_DEFAULT #endif +// Whether to use the APB_MAX lock. +// Requirement of each chip, to keep sending: +// - ESP32, S2, C3, S3: Protect APB, which is the core clock and clock of UART FIFO +// - ESP32-C2: Protect APB (UART FIFO clock), core clock (TODO: IDF-8348) +// - ESP32-C6, H2 and later chips: Protect core clock. Run in light-sleep hasn't been developed yet (TODO: IDF-8349), +// also need to avoid auto light-sleep. +#define PROTECT_APB (CONFIG_PM_ENABLE) + #define XOFF (0x13) #define XON (0x11) @@ -140,6 +149,9 @@ typedef struct { SemaphoreHandle_t tx_fifo_sem; /*!< UART TX FIFO semaphore*/ SemaphoreHandle_t tx_done_sem; /*!< UART TX done semaphore*/ SemaphoreHandle_t tx_brk_sem; /*!< UART TX send break done semaphore*/ +#if PROTECT_APB + esp_pm_lock_handle_t pm_lock; ///< Power management lock +#endif } uart_obj_t; typedef struct { @@ -1271,15 +1283,19 @@ esp_err_t uart_wait_tx_done(uart_port_t uart_num, TickType_t ticks_to_wait) } else { ticks_to_wait = ticks_to_wait - (ticks_end - ticks_start); } + +#if PROTECT_APB + esp_pm_lock_acquire(p_uart_obj[uart_num]->pm_lock); +#endif //take 2nd tx_done_sem, wait given from ISR res = xSemaphoreTake(p_uart_obj[uart_num]->tx_done_sem, (TickType_t)ticks_to_wait); - if (res == pdFALSE) { - // The TX_DONE interrupt will be disabled in ISR - xSemaphoreGive(p_uart_obj[uart_num]->tx_mux); - return ESP_ERR_TIMEOUT; - } +#if PROTECT_APB + esp_pm_lock_release(p_uart_obj[uart_num]->pm_lock); +#endif + + // The TX_DONE interrupt will be disabled in ISR xSemaphoreGive(p_uart_obj[uart_num]->tx_mux); - return ESP_OK; + return (res == pdFALSE) ? ESP_ERR_TIMEOUT : ESP_OK; } int uart_tx_chars(uart_port_t uart_num, const char *buffer, uint32_t len) @@ -1306,6 +1322,9 @@ static int uart_tx_all(uart_port_t uart_num, const char *src, size_t size, bool //lock for uart_tx xSemaphoreTake(p_uart_obj[uart_num]->tx_mux, (TickType_t)portMAX_DELAY); +#if PROTECT_APB + esp_pm_lock_acquire(p_uart_obj[uart_num]->pm_lock); +#endif p_uart_obj[uart_num]->coll_det_flg = false; if (p_uart_obj[uart_num]->tx_buf_size > 0) { size_t max_size = xRingbufferGetMaxItemSize(p_uart_obj[uart_num]->tx_ring_buf); @@ -1349,6 +1368,9 @@ static int uart_tx_all(uart_port_t uart_num, const char *src, size_t size, bool } xSemaphoreGive(p_uart_obj[uart_num]->tx_fifo_sem); } +#if PROTECT_APB + esp_pm_lock_release(p_uart_obj[uart_num]->pm_lock); +#endif xSemaphoreGive(p_uart_obj[uart_num]->tx_mux); return original_size; } @@ -1533,6 +1555,11 @@ static void uart_free_driver_obj(uart_obj_t *uart_obj) } heap_caps_free(uart_obj->rx_data_buf); +#if PROTECT_APB + if (uart_obj->pm_lock) { + esp_pm_lock_delete(uart_obj->pm_lock); + } +#endif heap_caps_free(uart_obj); } @@ -1568,6 +1595,12 @@ static uart_obj_t *uart_alloc_driver_obj(uart_port_t uart_num, int event_queue_s !uart_obj->tx_done_sem || !uart_obj->tx_fifo_sem) { goto err; } +#if PROTECT_APB + if (esp_pm_lock_create(ESP_PM_APB_FREQ_MAX, 0, "uart_driver", + &uart_obj->pm_lock) != ESP_OK) { + goto err; + } +#endif return uart_obj; diff --git a/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c b/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c new file mode 100644 index 0000000000..2cc24559f8 --- /dev/null +++ b/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c @@ -0,0 +1,80 @@ +/* + * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "sdkconfig.h" +#include "unity.h" +#include "driver/uart.h" +#include "esp_pm.h" +#include "esp_log.h" + +#define UART_TAG "Uart" +#define TEST_BUF_SIZE 256 + +//This should be larger than FIFO_SIZE + 2 * TEST_DRIVER_BUF_SIZE, so that blocking will happen +#define TEST_WRITE_SIZE 1024 + +#define TEST_TXD 12 +#define TEST_RXD 13 +#define TEST_RTS UART_PIN_NO_CHANGE +#define TEST_CTS UART_PIN_NO_CHANGE + +#define TEST_UART_PORT_NUM 1 +#define TEST_UART_BAUD_RATE 115200 + +#define MAX_FREQ (CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ) + +#if CONFIG_XTAL_FREQ_40 +#define MIN_FREQ 10 +#elif CONFIG_XTAL_FREQ_32 +#define MIN_FREQ 8 +#elif CONFIG_XTAL_FREQ_26 +#define MIN_FREQ 13 +#endif + +#if CONFIG_PM_ENABLE + +TEST_CASE("uart tx won't be blocked by auto light sleep", "[uart]") +{ + // Configure dynamic frequency scaling: + // maximum and minimum frequencies are set in sdkconfig, + // automatic light sleep is enabled if tickless idle support is enabled. + esp_pm_config_t pm_config = { + .max_freq_mhz = MAX_FREQ, + .min_freq_mhz = MIN_FREQ, +#if CONFIG_FREERTOS_USE_TICKLESS_IDLE + .light_sleep_enable = true +#endif + }; + TEST_ESP_OK(esp_pm_configure(&pm_config)); + + uart_config_t uart_config = { + .baud_rate = TEST_UART_BAUD_RATE, + .data_bits = UART_DATA_8_BITS, + .parity = UART_PARITY_DISABLE, + .stop_bits = UART_STOP_BITS_1, + .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, + .source_clk = UART_SCLK_DEFAULT, + }; + int intr_alloc_flags = 0; + + TEST_ESP_OK(uart_driver_install(TEST_UART_PORT_NUM, TEST_BUF_SIZE, 0, 0, NULL, intr_alloc_flags)); + TEST_ESP_OK(uart_param_config(TEST_UART_PORT_NUM, &uart_config)); + TEST_ESP_OK(uart_set_pin(TEST_UART_PORT_NUM, TEST_TXD, TEST_RXD, TEST_RTS, TEST_CTS)); + + // Configure a temporary buffer for the incoming data + const int len = TEST_WRITE_SIZE; + uint8_t *data = (uint8_t *) malloc(len); + + //If auto lightsleep happen, there will be deadlock in either one of the two following functions + uart_write_bytes(TEST_UART_PORT_NUM, (const char *) data, len); + uart_wait_tx_done(TEST_UART_PORT_NUM, portMAX_DELAY); + + ESP_LOGI(UART_TAG, "return from uart_write_bytes"); + + uart_driver_delete(TEST_UART_PORT_NUM); + free(data); +} +#endif // CONFIG_PM_ENABLE From f251e32f48ca7214da1744f1e3ca698cfd01f5de Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Tue, 20 Dec 2022 23:49:49 +0800 Subject: [PATCH 2/3] feat(uart_test): add test case for uart tx blocked by auto-suspend --- .../test_apps/uart/main/CMakeLists.txt | 5 +-- .../test_apps/uart/main/test_common.h | 16 ++++++++++ .../test_apps/uart/main/test_uart.c | 11 ++----- .../uart/main/test_uart_auto_lightsleep.c | 31 +++++++++++++------ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 components/esp_driver_uart/test_apps/uart/main/test_common.h diff --git a/components/esp_driver_uart/test_apps/uart/main/CMakeLists.txt b/components/esp_driver_uart/test_apps/uart/main/CMakeLists.txt index ce15fb9702..22ab1202d2 100644 --- a/components/esp_driver_uart/test_apps/uart/main/CMakeLists.txt +++ b/components/esp_driver_uart/test_apps/uart/main/CMakeLists.txt @@ -1,7 +1,8 @@ # In order for the cases defined by `TEST_CASE` to be linked into the final elf, # the component can be registered as WHOLE_ARCHIVE idf_component_register( - SRCS "test_app_main.c" "test_uart.c" - REQUIRES esp_driver_uart unity esp_psram test_utils esp_driver_gpio + SRCS "test_app_main.c" "test_uart.c" "test_uart_auto_lightsleep.c" + REQUIRES esp_driver_uart unity esp_psram test_utils esp_driver_gpio esp_pm + PRIV_INCLUDE_DIRS . WHOLE_ARCHIVE ) diff --git a/components/esp_driver_uart/test_apps/uart/main/test_common.h b/components/esp_driver_uart/test_apps/uart/main/test_common.h new file mode 100644 index 0000000000..3035626b55 --- /dev/null +++ b/components/esp_driver_uart/test_apps/uart/main/test_common.h @@ -0,0 +1,16 @@ +/* + * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Unlicense OR CC0-1.0 + */ +#include "driver/uart.h" + +typedef struct { + uart_port_t port_num; + soc_module_clk_t default_src_clk; + int tx_pin_num; + int rx_pin_num; + uint32_t rx_flow_ctrl_thresh; +} uart_port_param_t; + +bool port_select(uart_port_param_t *port_param); diff --git a/components/esp_driver_uart/test_apps/uart/main/test_uart.c b/components/esp_driver_uart/test_apps/uart/main/test_uart.c index 277b11fa5a..76f6f93df4 100644 --- a/components/esp_driver_uart/test_apps/uart/main/test_uart.c +++ b/components/esp_driver_uart/test_apps/uart/main/test_uart.c @@ -15,21 +15,14 @@ #include "soc/uart_pins.h" #include "soc/soc_caps.h" #include "soc/clk_tree_defs.h" +#include "test_common.h" #define BUF_SIZE (100) #define UART_BAUD_11520 (11520) #define UART_BAUD_115200 (115200) #define TOLERANCE (0.02) //baud rate error tolerance 2%. -typedef struct { - uart_port_t port_num; - soc_module_clk_t default_src_clk; - int tx_pin_num; - int rx_pin_num; - uint32_t rx_flow_ctrl_thresh; -} uart_port_param_t; - -static bool port_select(uart_port_param_t *port_param) +bool port_select(uart_port_param_t *port_param) { char argv[10]; unity_wait_for_signal_param("select to test 'uart' or 'lp_uart' port", argv, sizeof(argv)); diff --git a/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c b/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c index 2cc24559f8..b409deafd8 100644 --- a/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c +++ b/components/esp_driver_uart/test_apps/uart/main/test_uart_auto_lightsleep.c @@ -9,6 +9,8 @@ #include "driver/uart.h" #include "esp_pm.h" #include "esp_log.h" +#include "test_common.h" +#include "esp_private/sleep_cpu.h" //for sleep_cpu_configure #define UART_TAG "Uart" #define TEST_BUF_SIZE 256 @@ -16,12 +18,9 @@ //This should be larger than FIFO_SIZE + 2 * TEST_DRIVER_BUF_SIZE, so that blocking will happen #define TEST_WRITE_SIZE 1024 -#define TEST_TXD 12 -#define TEST_RXD 13 #define TEST_RTS UART_PIN_NO_CHANGE #define TEST_CTS UART_PIN_NO_CHANGE -#define TEST_UART_PORT_NUM 1 #define TEST_UART_BAUD_RATE 115200 #define MAX_FREQ (CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ) @@ -34,10 +33,16 @@ #define MIN_FREQ 13 #endif +#if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32P4) #if CONFIG_PM_ENABLE TEST_CASE("uart tx won't be blocked by auto light sleep", "[uart]") { + uart_port_param_t port_param = {}; + TEST_ASSERT(port_select(&port_param)); + + uart_port_t port_num = port_param.port_num; + // Configure dynamic frequency scaling: // maximum and minimum frequencies are set in sdkconfig, // automatic light sleep is enabled if tickless idle support is enabled. @@ -56,25 +61,31 @@ TEST_CASE("uart tx won't be blocked by auto light sleep", "[uart]") .parity = UART_PARITY_DISABLE, .stop_bits = UART_STOP_BITS_1, .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, - .source_clk = UART_SCLK_DEFAULT, + .source_clk = port_param.default_src_clk, }; int intr_alloc_flags = 0; - TEST_ESP_OK(uart_driver_install(TEST_UART_PORT_NUM, TEST_BUF_SIZE, 0, 0, NULL, intr_alloc_flags)); - TEST_ESP_OK(uart_param_config(TEST_UART_PORT_NUM, &uart_config)); - TEST_ESP_OK(uart_set_pin(TEST_UART_PORT_NUM, TEST_TXD, TEST_RXD, TEST_RTS, TEST_CTS)); + TEST_ESP_OK(uart_driver_install(port_num, TEST_BUF_SIZE, 0, 0, NULL, intr_alloc_flags)); + TEST_ESP_OK(uart_param_config(port_num, &uart_config)); + TEST_ESP_OK(uart_set_pin(port_num, port_param.tx_pin_num, port_param.rx_pin_num, TEST_RTS, TEST_CTS)); // Configure a temporary buffer for the incoming data const int len = TEST_WRITE_SIZE; uint8_t *data = (uint8_t *) malloc(len); //If auto lightsleep happen, there will be deadlock in either one of the two following functions - uart_write_bytes(TEST_UART_PORT_NUM, (const char *) data, len); - uart_wait_tx_done(TEST_UART_PORT_NUM, portMAX_DELAY); + uart_write_bytes(port_num, (const char *) data, len); + uart_wait_tx_done(port_num, portMAX_DELAY); ESP_LOGI(UART_TAG, "return from uart_write_bytes"); - uart_driver_delete(TEST_UART_PORT_NUM); + uart_driver_delete(port_num); free(data); + +#if CONFIG_PM_POWER_DOWN_CPU_IN_LIGHT_SLEEP + //When PD_CPU enabled, retention may cause 14K memory leak. Workaround to release the memory + sleep_cpu_configure(false); +#endif } #endif // CONFIG_PM_ENABLE +#endif //!TEMPORARY_DISABLED_FOR_TARGETS(ESP32P4) From 72a001b8517092565d8b28a8111c2320ab83d35b Mon Sep 17 00:00:00 2001 From: Xiao Xufeng Date: Fri, 22 Sep 2023 11:56:56 +0800 Subject: [PATCH 3/3] fix(soc): fixed uart_periph.h not including reg.h issue This will cause rom/uart.h can't compile. --- components/soc/include/soc/uart_periph.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/soc/include/soc/uart_periph.h b/components/soc/include/soc/uart_periph.h index 2cf1fe9333..45781b208a 100644 --- a/components/soc/include/soc/uart_periph.h +++ b/components/soc/include/soc/uart_periph.h @@ -10,6 +10,8 @@ #include "soc/gpio_sig_map.h" #include "soc/io_mux_reg.h" #include "soc/uart_pins.h" +#include "soc/uart_struct.h" +#include "soc/uart_reg.h" #ifdef __cplusplus extern "C" {