From 179056118f3fb975a0970babf557e356a6e7943b Mon Sep 17 00:00:00 2001 From: zwx Date: Thu, 16 Apr 2026 14:42:15 +0800 Subject: [PATCH] fix(openthread): fix SPI host-RCP communication loss - 1. H2 SPI slave DMA transaction length insufficient for CMD_RESET frame - 2. H2 SPI slave BUSY race at boot causing permanent deadlock - 3. H2 INT pin spurious NEGEDGE on chip reset - 4. H2 SPI slave trans_len exceeding buffer size - 5. S3 host RST flag handling preventing Spinel-layer reset detection - 6. S3 stale eventfd events after HardwareReset causing WaitResponse timeout - 7. ot_br missing coprocessor reset failure callback --- .../esp_spi_spinel_interface.hpp | 1 + .../src/port/esp_openthread_spi_slave.c | 66 +++++++++++++------ .../src/port/esp_spi_spinel_interface.cpp | 44 +++++++++---- examples/openthread/ot_br/main/esp_ot_br.c | 1 + 4 files changed, 80 insertions(+), 32 deletions(-) diff --git a/components/openthread/private_include/esp_spi_spinel_interface.hpp b/components/openthread/private_include/esp_spi_spinel_interface.hpp index e5b0976e1fd..47a54628fd7 100644 --- a/components/openthread/private_include/esp_spi_spinel_interface.hpp +++ b/components/openthread/private_include/esp_spi_spinel_interface.hpp @@ -168,6 +168,7 @@ private: esp_openthread_spi_host_config_t m_spi_config; uint8_t m_tx_buffer[kSPIFrameSize]; + uint8_t *m_rx_dma_buf; ///< DMA-aligned RX buffer; avoids cache-coherency issues with unaligned frame buffer int m_event_fd; volatile uint16_t m_pending_data_len; diff --git a/components/openthread/src/port/esp_openthread_spi_slave.c b/components/openthread/src/port/esp_openthread_spi_slave.c index 1a47a00451f..9daf496f5f8 100644 --- a/components/openthread/src/port/esp_openthread_spi_slave.c +++ b/components/openthread/src/port/esp_openthread_spi_slave.c @@ -3,15 +3,6 @@ * * SPDX-License-Identifier: Apache-2.0 */ - -/* SPI Slave example, receiver (uses SPI Slave driver to communicate with sender) - - This example code is in the Public Domain (or CC0 licensed, at your option.) - - Unless required by applicable law or agreed to in writing, this - software is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR - CONDITIONS OF ANY KIND, either express or implied. -*/ #include #include "esp_attr.h" @@ -42,6 +33,15 @@ typedef struct { uint16_t input_buf_len; } pending_transaction_t; +// DMA bounce buffer for RX — always sized to max(input, output) so MISO is +// driven for the full output even when NcpSpi passes a small input buffer. +#define SPI_SLAVE_RX_DMA_BUF_SIZE OPENTHREAD_CONFIG_NCP_SPI_BUFFER_SIZE +static DRAM_ATTR uint8_t *s_rx_dma_buf = NULL; + +// Guards the BUSY path: only return OT_ERROR_BUSY when a transaction is truly +// queued in the driver, so post_trans_cb is guaranteed to fire and re-queue. +static volatile DRAM_ATTR bool s_transaction_in_flight = false; + static otPlatSpiSlaveTransactionProcessCallback s_process_callback = NULL; static otPlatSpiSlaveTransactionCompleteCallback s_complete_callback = NULL; @@ -59,12 +59,25 @@ static void IRAM_ATTR handle_spi_setup_done(spi_slave_transaction_t *trans) static void IRAM_ATTR handle_spi_transaction_done(spi_slave_transaction_t *trans) { gpio_set_level(s_spi_config->intr_pin, 1); + s_transaction_in_flight = false; pending_transaction_t *pending_transaction = (pending_transaction_t *)(trans->user); trans->trans_len /= CHAR_BIT; + // Cap trans_len: HW reports actual clock count which may exceed slave buffer. + uint16_t max_buf_len = pending_transaction->output_buf_len > pending_transaction->input_buf_len + ? pending_transaction->output_buf_len : pending_transaction->input_buf_len; + if (trans->trans_len > max_buf_len) { + trans->trans_len = max_buf_len; + } + + // Copy RX bounce buffer back to the actual NcpSpi input buffer. + if (s_input_buf && s_rx_dma_buf && s_rx_dma_buf != s_input_buf) { + memcpy(s_input_buf, s_rx_dma_buf, pending_transaction->input_buf_len); + } + if (s_complete_callback && s_complete_callback(s_context, (void*)trans->tx_buffer, pending_transaction->output_buf_len, - trans->rx_buffer, pending_transaction->input_buf_len, trans->trans_len)) { + s_input_buf, pending_transaction->input_buf_len, trans->trans_len)) { esp_openthread_task_queue_post(s_process_callback, s_context); } } @@ -83,22 +96,23 @@ esp_err_t esp_openthread_host_rcp_spi_init(const esp_openthread_platform_config_ .pin_bit_mask = (1ULL << s_spi_config->intr_pin), }; ESP_GOTO_ON_ERROR(gpio_config(&io_conf), err, OT_PLAT_LOG_TAG, "fail to configure SPI gpio"); + // Deassert INT: GPIO latch resets to LOW after chip reset, which would + // cause a spurious NEGEDGE before the slave is ready. + gpio_set_level(s_spi_config->intr_pin, 1); gpio_set_pull_mode(s_spi_config->bus_config.mosi_io_num, GPIO_PULLUP_ONLY); gpio_set_pull_mode(s_spi_config->bus_config.sclk_io_num, GPIO_PULLUP_ONLY); gpio_set_pull_mode(s_spi_config->slave_config.spics_io_num, GPIO_PULLUP_ONLY); s_spi_transaction = heap_caps_malloc(sizeof(spi_slave_transaction_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + ESP_GOTO_ON_FALSE(s_spi_transaction != NULL, ESP_ERR_NO_MEM, err, OT_PLAT_LOG_TAG, "failed to allocate memory for SPI transaction on internal heap"); s_pending_transaction = heap_caps_malloc(sizeof(pending_transaction_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); - if (s_spi_transaction == NULL || s_pending_transaction == NULL) { - ESP_LOGE(OT_PLAT_LOG_TAG, "failed to allocate memory for SPI transaction on internal heap"); - ret = ESP_ERR_NO_MEM; - goto err; - } + ESP_GOTO_ON_FALSE(s_pending_transaction != NULL, ESP_ERR_NO_MEM, err, OT_PLAT_LOG_TAG, "failed to allocate memory for pending transaction on internal heap"); + s_rx_dma_buf = heap_caps_malloc(SPI_SLAVE_RX_DMA_BUF_SIZE, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL); + ESP_GOTO_ON_FALSE(s_rx_dma_buf != NULL, ESP_ERR_NO_MEM, err, OT_PLAT_LOG_TAG, "failed to allocate memory for RX DMA buffer on internal heap"); s_spi_transaction->user = (void *)s_pending_transaction; - /* Initialize SPI slave interface */ s_spi_config->slave_config.post_setup_cb = handle_spi_setup_done; s_spi_config->slave_config.post_trans_cb = handle_spi_transaction_done; ESP_GOTO_ON_ERROR(spi_slave_initialize(s_spi_config->host_device, &s_spi_config->bus_config, @@ -114,6 +128,8 @@ err: s_spi_transaction = NULL; heap_caps_free(s_pending_transaction); s_pending_transaction = NULL; + heap_caps_free(s_rx_dma_buf); + s_rx_dma_buf = NULL; return ret; } @@ -125,9 +141,11 @@ void esp_openthread_spi_slave_deinit(void) heap_caps_free(s_spi_config); heap_caps_free(s_spi_transaction); heap_caps_free(s_pending_transaction); + heap_caps_free(s_rx_dma_buf); s_spi_config = NULL; s_spi_transaction = NULL; s_pending_transaction = NULL; + s_rx_dma_buf = NULL; return; } @@ -155,14 +173,21 @@ otError IRAM_ATTR otPlatSpiSlavePrepareTransaction(uint8_t *aOutputBuf, uint16_t s_input_len = aInputBufLen; } - trans_length = s_output_len > s_input_len ? s_output_len : s_input_len; - trans_length *= CHAR_BIT; - if ((gpio_get_level(s_spi_config->slave_config.spics_io_num) == 0)) { + // Use max(input, output) so MISO is driven for the full output frame; + // s_rx_dma_buf absorbs extra RX bytes to avoid overflowing the NcpSpi buffer. + uint16_t trans_data_len = (s_input_len > s_output_len) ? s_input_len : s_output_len; + trans_length = trans_data_len * CHAR_BIT; + + // In task context, return BUSY only when a transaction is already in flight + // AND CS is asserted — ensures post_trans_cb will fire to re-queue. + // In ISR context (post_trans_cb) we always queue unconditionally. + if (xPortCanYield() && s_transaction_in_flight && + (gpio_get_level(s_spi_config->slave_config.spics_io_num) == 0)) { ESP_EARLY_LOGE(SPI_SLAVE_TAG, "SPI busy"); return OT_ERROR_BUSY; } s_spi_transaction->length = trans_length; - s_spi_transaction->rx_buffer = s_input_buf; + s_spi_transaction->rx_buffer = s_rx_dma_buf; s_spi_transaction->tx_buffer = s_output_buf; pending_transaction_t *pending_transaction = (pending_transaction_t *)s_spi_transaction->user; @@ -179,6 +204,7 @@ otError IRAM_ATTR otPlatSpiSlavePrepareTransaction(uint8_t *aOutputBuf, uint16_t } if (trans_state == ESP_OK) { + s_transaction_in_flight = true; return OT_ERROR_NONE; } else { return OT_ERROR_FAILED; diff --git a/components/openthread/src/port/esp_spi_spinel_interface.cpp b/components/openthread/src/port/esp_spi_spinel_interface.cpp index 3a409491697..16be363c78a 100644 --- a/components/openthread/src/port/esp_spi_spinel_interface.cpp +++ b/components/openthread/src/port/esp_spi_spinel_interface.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -8,6 +8,7 @@ #include "openthread/error.h" #include "esp_check.h" +#include "esp_heap_caps.h" #include "esp_openthread_common_macro.h" #include "esp_rom_sys.h" #include "esp_vfs.h" @@ -17,6 +18,7 @@ #include "driver/gpio.h" #include "driver/spi_master.h" #include "hal/gpio_types.h" +#include "soc/soc_caps.h" #include "ncp/ncp_spi.hpp" using ot::Spinel::SpiFrame; @@ -26,7 +28,8 @@ namespace esp { namespace openthread { SpiSpinelInterface::SpiSpinelInterface(void) - : m_event_fd(-1) + : m_rx_dma_buf(nullptr) + , m_event_fd(-1) , m_receiver_frame_callback(nullptr) , m_receiver_frame_context(nullptr) , m_receive_frame_buffer(nullptr) @@ -73,7 +76,7 @@ esp_err_t SpiSpinelInterface::Enable(const esp_openthread_spi_host_config_t &spi io_conf.mode = GPIO_MODE_INPUT; io_conf.pull_up_en = GPIO_PULLUP_ENABLE; ESP_RETURN_ON_ERROR(gpio_config(&io_conf), OT_PLAT_LOG_TAG, "fail to config spi gpio"); - gpio_install_isr_service(0); // The gpio isr service may has been installed. + gpio_install_isr_service(0); ESP_RETURN_ON_ERROR(gpio_isr_handler_add(spi_config.intr_pin, GpioIntrHandler, this), OT_PLAT_LOG_TAG, "fail to add gpio isr handler"); m_has_pending_device_frame = false; @@ -82,6 +85,9 @@ esp_err_t SpiSpinelInterface::Enable(const esp_openthread_spi_host_config_t &spi ESP_RETURN_ON_FALSE(m_event_fd >= 0, ESP_FAIL, OT_PLAT_LOG_TAG, "fail to get event fd"); + m_rx_dma_buf = (uint8_t *)heap_caps_malloc(kSPIFrameSize, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL); + ESP_RETURN_ON_FALSE(m_rx_dma_buf != nullptr, ESP_ERR_NO_MEM, OT_PLAT_LOG_TAG, "fail to alloc SPI RX DMA buffer"); + ESP_LOGI(OT_PLAT_LOG_TAG, "spinel SPI interface initialization completed"); return ESP_OK; @@ -105,6 +111,8 @@ esp_err_t SpiSpinelInterface::Disable(void) ESP_RETURN_ON_ERROR(spi_bus_free(m_spi_config.host_device), OT_PLAT_LOG_TAG, "fail to free spi bus"); gpio_uninstall_isr_service(); } + heap_caps_free(m_rx_dma_buf); + m_rx_dma_buf = nullptr; return ESP_OK; } @@ -115,8 +123,7 @@ otError SpiSpinelInterface::SendFrame(const uint8_t *frame, uint16_t length) "send frame is too long"); memcpy(&m_tx_buffer[kSPIFrameHeaderSize], frame, length); - uint16_t rx_data_size = - length < kSmallPacketSize ? kSmallPacketSize : length; // We'll use tx_size to receive small packets piggybacked + uint16_t rx_data_size = length < kSmallPacketSize ? kSmallPacketSize : length; if (ConductSPITransaction(false, length, rx_data_size) == ESP_OK) { return OT_ERROR_NONE; } else { @@ -152,9 +159,12 @@ esp_err_t SpiSpinelInterface::ConductSPITransaction(bool reset, uint16_t tx_data transaction.length = data_size * CHAR_BIT; transaction.rxlength = (rx_data_size + kSPIFrameHeaderSize) * CHAR_BIT; transaction.tx_buffer = m_tx_buffer; - transaction.rx_buffer = rx_buffer; + // Use aligned DMA buffer: MultiFrameBuffer's internal array is not + // cache-line-aligned, causing stale D-cache reads after DMA completes. + transaction.rx_buffer = m_rx_dma_buf; ESP_RETURN_ON_ERROR(spi_device_polling_transmit(m_device, &transaction), OT_PLAT_LOG_TAG, "SPI transaction failed"); + memcpy(rx_buffer, m_rx_dma_buf, data_size); SpiFrame rx_frame(rx_buffer); if (!rx_frame.IsValid() || rx_frame.GetHeaderAcceptLen() > kSPIFrameSize || @@ -162,17 +172,14 @@ esp_err_t SpiSpinelInterface::ConductSPITransaction(bool reset, uint16_t tx_data vTaskDelay(pdMS_TO_TICKS(15)); ESP_RETURN_ON_ERROR(spi_device_polling_transmit(m_device, &transaction), OT_PLAT_LOG_TAG, "fail to retry SPI invalid transaction"); + memcpy(rx_buffer, m_rx_dma_buf, data_size); } - if (rx_frame.IsResetFlagSet()) { - ESP_LOGW(OT_PLAT_LOG_TAG, "RCP Reset"); - m_receive_frame_buffer->DiscardFrame(); - return ESP_OK; - } if (rx_frame.GetHeaderDataLen() == 0 && rx_frame.GetHeaderAcceptLen() == 0) { vTaskDelay(pdMS_TO_TICKS(15)); ESP_RETURN_ON_ERROR(spi_device_polling_transmit(m_device, &transaction), OT_PLAT_LOG_TAG, "fail to retry SPI empty transaction"); + memcpy(rx_buffer, m_rx_dma_buf, data_size); } if (rx_frame.GetHeaderDataLen() > 0 && rx_frame.GetHeaderDataLen() < tx_frame.GetHeaderAcceptLen()) { @@ -267,7 +274,20 @@ otError SpiSpinelInterface::HardwareReset(void) { if (mRcpFailureHandler) { mRcpFailureHandler(); - ConductSPITransaction(true, 0, 0); // clear + ConductSPITransaction(true, 0, 0); + // Drain stale GPIO interrupt events accumulated before reset, otherwise + // WaitForFrame() would fire immediately on a stale event and fail. + uint64_t event; + struct timeval zero_timeout = {0, 0}; + fd_set read_fds; + FD_ZERO(&read_fds); + FD_SET(m_event_fd, &read_fds); + if (select(m_event_fd + 1, &read_fds, NULL, NULL, &zero_timeout) > 0 && + FD_ISSET(m_event_fd, &read_fds)) { + read(m_event_fd, &event, sizeof(event)); + } + m_pending_data_len = 0; + } return OT_ERROR_NONE; } diff --git a/examples/openthread/ot_br/main/esp_ot_br.c b/examples/openthread/ot_br/main/esp_ot_br.c index 4111a02c3f8..89449a25ecf 100644 --- a/examples/openthread/ot_br/main/esp_ot_br.c +++ b/examples/openthread/ot_br/main/esp_ot_br.c @@ -97,6 +97,7 @@ void app_main(void) ESP_ERROR_CHECK(mdns_hostname_set("esp-ot-br")); #if CONFIG_OPENTHREAD_SUPPORT_HW_RESET_RCP esp_openthread_register_rcp_failure_handler(rcp_failure_hardware_reset_handler); + esp_openthread_set_coprocessor_reset_failure_callback(rcp_failure_hardware_reset_handler); #endif #if CONFIG_OPENTHREAD_CLI