diff --git a/components/esp_driver_spi/src/gpspi/spi_slave_hd.c b/components/esp_driver_spi/src/gpspi/spi_slave_hd.c index 22ebfe7cd5a..b7bc88d1c69 100644 --- a/components/esp_driver_spi/src/gpspi/spi_slave_hd.c +++ b/components/esp_driver_spi/src/gpspi/spi_slave_hd.c @@ -171,8 +171,8 @@ esp_err_t spi_slave_hd_init(spi_host_device_t host_id, const spi_bus_config_t *b .dma_enabled = true, .append_mode = append_mode, .mode = config->mode, - .tx_lsbfirst = (config->flags & SPI_SLAVE_HD_RXBIT_LSBFIRST), - .rx_lsbfirst = (config->flags & SPI_SLAVE_HD_TXBIT_LSBFIRST), + .tx_lsbfirst = (config->flags & SPI_SLAVE_HD_TXBIT_LSBFIRST), + .rx_lsbfirst = (config->flags & SPI_SLAVE_HD_RXBIT_LSBFIRST), }; //Init the hal according to the hal_config set above diff --git a/components/esp_driver_spi/test_apps/components/spi_bench_mark/include/spi_performance.h b/components/esp_driver_spi/test_apps/components/spi_bench_mark/include/spi_performance.h index 6766eb674d3..1db62ae648b 100644 --- a/components/esp_driver_spi/test_apps/components/spi_bench_mark/include/spi_performance.h +++ b/components/esp_driver_spi/test_apps/components/spi_bench_mark/include/spi_performance.h @@ -55,7 +55,7 @@ #elif CONFIG_IDF_TARGET_ESP32C6 #define IDF_TARGET_MAX_SPI_CLK_FREQ 26666*1000 -#define IDF_TARGET_MAX_TRANS_TIME_INTR_DMA 35 //TODO: IDF-9551, check perform +#define IDF_TARGET_MAX_TRANS_TIME_INTR_DMA 37 //TODO: IDF-9551, check perform #define IDF_TARGET_MAX_TRANS_TIME_POLL_DMA 19 #define IDF_TARGET_MAX_TRANS_TIME_INTR_CPU 32 #define IDF_TARGET_MAX_TRANS_TIME_POLL_CPU 15 diff --git a/components/esp_driver_spi/test_apps/master/main/test_spi_bus_lock.c b/components/esp_driver_spi/test_apps/master/main/test_spi_bus_lock.c index 0b52beda80f..cd857463bed 100644 --- a/components/esp_driver_spi/test_apps/master/main/test_spi_bus_lock.c +++ b/components/esp_driver_spi/test_apps/master/main/test_spi_bus_lock.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2021-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -347,3 +347,61 @@ TEST_CASE("spi master can be used on SPI1", "[spi]") //TODO: add a case when a non-polling transaction happened in the bus-acquiring time and then release the bus then queue a new trans #endif //!(CONFIG_SPIRAM && CONFIG_IDF_TARGET_ESP32) + +#define TEST_LARGE_TRANS_LEN 2048 +static void dev2_polling_task(void *arg) +{ + task_context_t *ctx = (task_context_t *)arg; + spi_transaction_t t = { + .flags = SPI_TRANS_USE_TXDATA, + .length = 32, + }; + while (!ctx->finished) { + TEST_ESP_OK(spi_device_polling_transmit(ctx->handle, &t)); + vTaskDelay(pdMS_TO_TICKS(1)); + } + vTaskDelete(NULL); +} + +TEST_CASE("release_bus during flying is safe to other device acquiring", "[spi]") +{ + spi_bus_config_t buscfg = SPI_BUS_TEST_DEFAULT_CONFIG(); + TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &buscfg, SPI_DMA_CH_AUTO)); + + spi_device_interface_config_t devcfg_p = SPI_DEVICE_TEST_DEFAULT_CONFIG(); + spi_device_interface_config_t devcfg_q = SPI_DEVICE_TEST_DEFAULT_CONFIG(); + devcfg_q.spics_io_num = -1; + devcfg_q.queue_size = 3; + devcfg_q.clock_speed_hz = 500 * 1000; + + spi_device_handle_t dev_q; + task_context_t ctx = {}; + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg_p, &ctx.handle)); + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg_q, &dev_q)); + + // polling task with higher priority than the interrupt task + xTaskCreate(dev2_polling_task, "spi17860_p", 4096, &ctx, 6, NULL); + + uint8_t *q_txb = heap_caps_malloc(TEST_LARGE_TRANS_LEN, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL); + spi_transaction_t *ret_trans, trans = { + .length = TEST_LARGE_TRANS_LEN * 8, + .tx_buffer = q_txb, + }; + for (int i = 0; i < 30; i++) { + TEST_ESP_OK(spi_device_acquire_bus(dev_q, portMAX_DELAY)); + TEST_ESP_OK(spi_device_queue_trans(dev_q, &trans, portMAX_DELAY)); + esp_rom_printf("queue trans %d\n", i); + spi_device_release_bus(dev_q); + } + + ctx.finished = true; + vTaskDelay(pdMS_TO_TICKS(100)); // wait for all trans finished + for (int i = 0; i < devcfg_q.queue_size; i++) { + spi_device_get_trans_result(dev_q, &ret_trans, 0); + } + + free(q_txb); + TEST_ESP_OK(spi_bus_remove_device(ctx.handle)); + TEST_ESP_OK(spi_bus_remove_device(dev_q)); + TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST)); +} diff --git a/components/esp_hw_support/include/esp_private/spi_share_hw_ctrl.h b/components/esp_hw_support/include/esp_private/spi_share_hw_ctrl.h index 6b616a51109..38049614346 100644 --- a/components/esp_hw_support/include/esp_private/spi_share_hw_ctrl.h +++ b/components/esp_hw_support/include/esp_private/spi_share_hw_ctrl.h @@ -29,7 +29,7 @@ extern "C" { #if BUS_LOCK_DEBUG #define BUS_LOCK_DEBUG_EXECUTE_CHECK(x) assert(x) #else -#define BUS_LOCK_DEBUG_EXECUTE_CHECK(x) +#define BUS_LOCK_DEBUG_EXECUTE_CHECK(x) (void)(x) #endif #define CHECK_IOMUX_PIN(HOST, PIN_NAME) if (GPIO.func_in_sel_cfg[spi_periph_signal[(HOST)].PIN_NAME##_in].sig_in_sel) return false diff --git a/components/esp_hw_support/spi_bus_lock.c b/components/esp_hw_support/spi_bus_lock.c index d128205abf8..12aa6dd18fe 100644 --- a/components/esp_hw_support/spi_bus_lock.c +++ b/components/esp_hw_support/spi_bus_lock.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -88,12 +88,14 @@ * -> STATE_ACQ: by `acquire_core` * * - STATE_BG: - * * No acquiring device, the ISR is the acquiring processor, there is BG bits active, but no LOCK - * bits + * * No acquiring device, the ISR is the acquiring processor, and there are BG bits active. There + * may also be LOCK bits pending for another device, but the lock owner must wait until BG is + * fully finished. * * The BG operation should be enabled while turning into this state. * * -> STATE_IDLE: by `bg_exit_core` after `clear_pend_core` for all BG bits - * -> STATE_BG_ACQ: by `schedule_core`, when there is new LOCK bit set (by `acquire_core`) + * -> STATE_BG_ACQ: by `schedule_core`, when there is a new LOCK bit set (by `acquire_core`) and + * BG is active for the same device * * - STATE_BG_ACQ: * * There is acquiring device, the ISR is the acquiring processor, there may be BG bits active for @@ -116,9 +118,11 @@ * * -> STATE_BG_ACQ: by `req_core` * -> STATE_BG_ACQ (other device): by `acquire_end_core`, when there is LOCK bit for another - * device, and the new acquiring device has active BG bits. - * -> STATE_ACQ (other device): by `acquire_end_core`, when there is LOCK bit for another devices, - * but the new acquiring device has no active BG bits. + * device, and BG is active for the new acquiring device. + * -> STATE_ACQ (other device): by `acquire_end_core`, when there is LOCK bit for another device, + * and no BG bits are active. + * -> STATE_BG: by `acquire_end_core`, when there is LOCK bit for another device, but BG is still + * active for a different device. * -> STATE_BG: by `acquire_end_core` when there is no LOCK bit active, but there are active BG * bits. * -> STATE_IDLE: by `acquire_end_core` when there is no LOCK bit, nor BG bit active. @@ -389,19 +393,22 @@ SPI_BUS_LOCK_ISR_ATTR static inline bool acquire_core(spi_bus_lock_dev_t *dev_ha /** * Find the next acquiring processor according to the status. Will directly change - * the acquiring device if new one found. + * the acquiring device if the BG can yield to the lock owner, or if BG is active for the + * lock owner. * * Cases: * - BG should still be the acquiring processor (Return false): * 1. Acquiring device has active BG bits: out_desired_dev = new acquiring device - * 2. No acquiring device, but BG active: out_desired_dev = randomly pick one device with active BG bits + * 2. A new acquiring device exists, but BG is still active for another device: + * out_desired_dev = that BG-active device + * 3. No acquiring device, but BG active: out_desired_dev = randomly pick one device with active BG bits * - BG should yield to the task (Return true): - * 3. Acquiring device has no active BG bits: out_desired_dev = new acquiring device - * 4. No acquiring device while no active BG bits: out_desired_dev=NULL + * 4. Acquiring device has no active BG bits: out_desired_dev = new acquiring device + * 5. No acquiring device while no active BG bits: out_desired_dev=NULL * - * Acquiring device task need to be resumed only when case 3. + * Acquiring device task needs to be resumed only when case 4. * - * This scheduling can happen in either task or ISR, so `in_isr` or `bg_active` not touched. + * This scheduling can happen in either task or ISR, so `in_isr` is not touched. * * @param lock * @param status Current status @@ -420,12 +427,26 @@ schedule_core(spi_bus_lock_t *lock, uint32_t status, spi_bus_lock_dev_t **out_de bool bg_yield; if (lock_bits) { int dev_id = mask_get_id(lock_bits); - desired_dev = (spi_bus_lock_dev_t *)atomic_load(&lock->dev[dev_id]); - BUS_LOCK_DEBUG_EXECUTE_CHECK(desired_dev); + spi_bus_lock_dev_t *lock_dev = (spi_bus_lock_dev_t *)atomic_load(&lock->dev[dev_id]); + BUS_LOCK_DEBUG_EXECUTE_CHECK(lock_dev); - lock->acquiring_dev = desired_dev; - bg_yield = ((bg_bits & desired_dev->mask) == 0); - lock->acq_dev_bg_active = !bg_yield; + if (bg_bits && ((bg_bits & lock_dev->mask) == 0)) { + int bg_dev_id = mask_get_id(bg_bits); + desired_dev = (spi_bus_lock_dev_t *)atomic_load(&lock->dev[bg_dev_id]); + BUS_LOCK_DEBUG_EXECUTE_CHECK(desired_dev); + + // Keep ISR/BG owning the bus until the previous device's in-flight + // interrupt transactions are fully finished. The new lock owner will + // be resumed by a later schedule once BG bits are cleared. + lock->acquiring_dev = NULL; + lock->acq_dev_bg_active = false; + bg_yield = false; + } else { + desired_dev = lock_dev; + lock->acquiring_dev = desired_dev; + bg_yield = ((bg_bits & desired_dev->mask) == 0); + lock->acq_dev_bg_active = !bg_yield; + } } else { lock->acq_dev_bg_active = false; if (bg_bits) { @@ -533,7 +554,7 @@ SPI_BUS_LOCK_ISR_ATTR static inline bool bg_entry_core(spi_bus_lock_t *lock) // Handle the conditions of status and interrupt, avoiding the ISR being disabled when there is any new coming BG requests. // When called with `wip=true`, means the ISR is performing some operations. Will enable the interrupt again and exit unconditionally. // When called with `wip=false`, will only return `true` when there is no coming BG request. If return value is `false`, the ISR should try again. -// Will not change acquiring device. +// May change acquiring device when BG has finished and there is a pending LOCK bit. SPI_BUS_LOCK_ISR_ATTR static inline bool bg_exit_core(spi_bus_lock_t *lock, bool wip, BaseType_t *do_yield) { //See comments in `bg_entry_core`, re-enable interrupt disabled in entry if we do need the interrupt @@ -557,7 +578,20 @@ SPI_BUS_LOCK_ISR_ATTR static inline bool bg_exit_core(spi_bus_lock_t *lock, bool } } else { BUS_LOCK_DEBUG_EXECUTE_CHECK(!lock->acq_dev_bg_active); - ret = !(status & BG_MASK); + if (status & BG_MASK) { + ret = false; + } else if (status & LOCK_MASK) { + spi_bus_lock_dev_t *desired_dev = NULL; + bool bg_yield = schedule_core(lock, status, &desired_dev); + // A waiting lock owner must be selected once BG is fully finished. + BUS_LOCK_DEBUG_EXECUTE_CHECK(bg_yield); + BUS_LOCK_DEBUG_EXECUTE_CHECK(desired_dev); + BUS_LOCK_DEBUG_EXECUTE_CHECK(lock->acquiring_dev == desired_dev); + resume_dev_in_isr(lock->acquiring_dev, do_yield); + ret = true; + } else { + ret = true; + } } if (ret) { //when successfully exit, but no transaction done, mark BG as inactive diff --git a/components/esp_lcd/test_apps/spi_lcd/main/test_spi_lcd_panel.c b/components/esp_lcd/test_apps/spi_lcd/main/test_spi_lcd_panel.c index 8094037e728..e8bc327ced6 100644 --- a/components/esp_lcd/test_apps/spi_lcd/main/test_spi_lcd_panel.c +++ b/components/esp_lcd/test_apps/spi_lcd/main/test_spi_lcd_panel.c @@ -297,3 +297,64 @@ TEST_CASE("spi_lcd_send_colors_to_fixed_region", "[lcd]") TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST_ID)); free(color_data); } + +#define TEST_SPI_LCD_CONCURRENT_COLOR_LEN (120 * 120) +typedef struct { + spi_device_handle_t spi_dev; + TaskHandle_t done_task; + volatile bool stop; + uint32_t trans_count; +} spi_lcd_concurrent_polling_ctx_t; + +static void spi_lcd_concurrent_polling_task(void *arg) +{ + spi_lcd_concurrent_polling_ctx_t *ctx = arg; + spi_transaction_t trans = { + .length = 4 * 8, + .flags = SPI_TRANS_USE_TXDATA, + }; + + while (!ctx->stop) { + vTaskDelay(pdMS_TO_TICKS(50)); + TEST_ESP_OK(spi_device_polling_transmit(ctx->spi_dev, &trans)); + ctx->trans_count++; + } + xTaskNotifyGive(ctx->done_task); + vTaskDelete(NULL); +} + +TEST_CASE("spi_lcd_safe_with_another_device_polling_on_same_bus", "[lcd]") +{ + void *color_data = malloc(TEST_SPI_LCD_CONCURRENT_COLOR_LEN); + TEST_ASSERT_NOT_NULL(color_data); + + esp_lcd_panel_io_handle_t io_handle = NULL; + test_spi_lcd_common_initialize(&io_handle, NULL, NULL, 8, 8, false); + + spi_lcd_concurrent_polling_ctx_t polling_ctx = { .done_task = xTaskGetCurrentTaskHandle() }; + spi_device_interface_config_t other_dev_config = { + .clock_speed_hz = TEST_LCD_PIXEL_CLOCK_HZ, + .spics_io_num = -1, + .queue_size = 1, + }; + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST_ID, &other_dev_config, &polling_ctx.spi_dev)); + + // polling task with higher priority than the interrupt task + xTaskCreate(spi_lcd_concurrent_polling_task, "spi_polling", 4096, &polling_ctx, 10, NULL); + + for (int i = 0; i < 30; i++) { + printf("panel_io_tx_color %d\r\n", i); + TEST_ESP_OK(esp_lcd_panel_io_tx_color(io_handle, -1, color_data, TEST_SPI_LCD_CONCURRENT_COLOR_LEN)); + } + + polling_ctx.stop = true; + vTaskDelay(pdMS_TO_TICKS(100)); // wait for polling task to stop + TEST_ASSERT_GREATER_THAN(0, (int)ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(5000))); + TEST_ASSERT_GREATER_THAN_UINT32(0, polling_ctx.trans_count); + + TEST_ESP_OK(esp_lcd_panel_io_del(io_handle)); + TEST_ESP_OK(spi_bus_remove_device(polling_ctx.spi_dev)); + TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST_ID)); + TEST_ESP_OK(gpio_reset_pin(TEST_LCD_BK_LIGHT_GPIO)); + free(color_data); +}