spi_slave: fix spi_slave_isr iram_safe and add test case for it

components/driver/Kconfig

@@ -104,6 +104,7 @@ menu "Driver Configurations"
         config SPI_SLAVE_ISR_IN_IRAM
             bool "Place SPI slave ISR function into IRAM"
             default y
+            select PERIPH_CTRL_FUNC_IN_IRAM
             help
                 Place the SPI slave ISR in to IRAM to avoid possible cache miss.
 

components/driver/spi_slave.c

@@ -7,35 +7,33 @@
 #include <string.h>
 #include "esp_types.h"
 #include "esp_attr.h"
+#include "esp_check.h"
 #include "esp_intr_alloc.h"
 #include "esp_log.h"
 #include "esp_err.h"
 #include "esp_pm.h"
 #include "esp_heap_caps.h"
-#include "esp_rom_gpio.h"
 #include "esp_rom_sys.h"
 #include "soc/lldesc.h"
 #include "soc/soc_caps.h"
 #include "soc/spi_periph.h"
 #include "soc/soc_memory_layout.h"
-#include "hal/spi_ll.h"
-#include "hal/spi_slave_hal.h"
 #include "freertos/FreeRTOS.h"
 #include "freertos/semphr.h"
 #include "freertos/task.h"
 #include "sdkconfig.h"
 
 #include "driver/gpio.h"
-#include "esp_private/spi_common_internal.h"
 #include "driver/spi_slave.h"
+#include "hal/gpio_hal.h"
 #include "hal/spi_slave_hal.h"
+#include "esp_private/spi_common_internal.h"
+
 
 static const char *SPI_TAG = "spi_slave";
-#define SPI_CHECK(a, str, ret_val) \
+
-    if (!(a)) { \
+#define SPI_CHECK(a, str, ret_val)  ESP_RETURN_ON_FALSE(a, ret_val, SPI_TAG, str)
-        ESP_LOGE(SPI_TAG,"%s(%d): %s", __FUNCTION__, __LINE__, str); \
+
-        return (ret_val); \
-    }
 
 #ifdef CONFIG_SPI_SLAVE_ISR_IN_IRAM
 #define SPI_SLAVE_ISR_ATTR IRAM_ATTR
@@ -61,6 +59,7 @@ typedef struct {
     QueueHandle_t ret_queue;
     bool dma_enabled;
     bool cs_iomux;
+    uint8_t cs_in_signal;
     uint32_t tx_dma_chan;
     uint32_t rx_dma_chan;
 #ifdef CONFIG_PM_ENABLE
@@ -72,6 +71,7 @@ static spi_slave_t *spihost[SOC_SPI_PERIPH_NUM];
 
 static void spi_intr(void *arg);
 
+__attribute__((always_inline))
 static inline bool is_valid_host(spi_host_device_t host)
 {
 //SPI1 can be used as GPSPI only on ESP32
@@ -91,7 +91,7 @@ static inline bool SPI_SLAVE_ISR_ATTR bus_is_iomux(spi_slave_t *host)
 
 static void SPI_SLAVE_ISR_ATTR freeze_cs(spi_slave_t *host)
 {
-    esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ONE_INPUT, spi_periph_signal[host->id].spics_in, false);
+    esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ONE_INPUT, host->cs_in_signal, false);
 }
 
 // Use this function instead of cs_initial to avoid overwrite the output config
@@ -99,9 +99,9 @@ static void SPI_SLAVE_ISR_ATTR freeze_cs(spi_slave_t *host)
 static inline void SPI_SLAVE_ISR_ATTR restore_cs(spi_slave_t *host)
 {
     if (host->cs_iomux) {
-        gpio_iomux_in(host->cfg.spics_io_num, spi_periph_signal[host->id].spics_in);
+        gpio_ll_iomux_in(GPIO_HAL_GET_HW(GPIO_PORT_0), host->cfg.spics_io_num, host->cs_in_signal);
     } else {
-        esp_rom_gpio_connect_in_signal(host->cfg.spics_io_num, spi_periph_signal[host->id].spics_in, false);
+        esp_rom_gpio_connect_in_signal(host->cfg.spics_io_num, host->cs_in_signal, false);
     }
 }
 
@@ -161,6 +161,7 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b
         spicommon_cs_initialize(host, slave_config->spics_io_num, 0, !bus_is_iomux(spihost[host]));
         // check and save where cs line really route through
         spihost[host]->cs_iomux = (slave_config->spics_io_num == spi_periph_signal[host].spics0_iomux_pin) && bus_is_iomux(spihost[host]);
+        spihost[host]->cs_in_signal = spi_periph_signal[host].spics_in;
     }
 
     // The slave DMA suffers from unexpected transactions. Forbid reading if DMA is enabled by disabling the CS line.
@@ -204,6 +205,9 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b
     }
 
     int flags = bus_config->intr_flags | ESP_INTR_FLAG_INTRDISABLED;
+#ifdef CONFIG_SPI_SLAVE_ISR_IN_IRAM
+    flags |= ESP_INTR_FLAG_IRAM;
+#endif
     err = esp_intr_alloc(spicommon_irqsource_for_host(host), flags, spi_intr, (void *)spihost[host], &spihost[host]->intr);
     if (err != ESP_OK) {
         ret = err;

components/driver/test_apps/spi/slave/CMakeLists.txt

@@ -8,3 +8,16 @@ set(EXTRA_COMPONENT_DIRS
 
 include($ENV{IDF_PATH}/tools/cmake/project.cmake)
 project(spi_slave_test)
+
+if(CONFIG_COMPILER_DUMP_RTL_FILES)
+    add_custom_target(check_test_app_sections ALL
+                      COMMAND ${PYTHON} $ENV{IDF_PATH}/tools/ci/check_callgraph.py
+                      --rtl-dir ${CMAKE_BINARY_DIR}/esp-idf/driver/
+                      --elf-file ${CMAKE_BINARY_DIR}/spi_slave_test.elf
+                      find-refs
+                      --from-sections=.iram0.text
+                      --to-sections=.flash.text,.flash.rodata
+                      --exit-code
+                      DEPENDS ${elf}
+                      )
+endif()

components/driver/test_apps/spi/slave/main/CMakeLists.txt

@@ -10,6 +10,6 @@ set(srcs
 # the component can be registered as WHOLE_ARCHIVE
 idf_component_register(
     SRCS ${srcs}
-    PRIV_REQUIRES test_utils driver test_driver_utils
+    PRIV_REQUIRES test_utils driver test_driver_utils spi_flash
     WHOLE_ARCHIVE
 )

components/driver/test_apps/spi/slave/main/test_spi_slave.c

@@ -15,6 +15,7 @@
 #include "driver/spi_master.h"
 #include "driver/spi_slave.h"
 #include "driver/gpio.h"
+#include "esp_private/cache_utils.h"
 #include "esp_log.h"
 #include "esp_rom_gpio.h"
 
@@ -386,3 +387,129 @@ static void unaligned_test_slave(void)
 TEST_CASE_MULTIPLE_DEVICES("SPI_Slave_Unaligned_Test", "[spi_ms][test_env=generic_multi_device][timeout=120]", unaligned_test_master, unaligned_test_slave);
 
 #endif  //#if (TEST_SPI_PERIPH_NUM == 1)
+
+
+#if CONFIG_SPI_SLAVE_ISR_IN_IRAM
+#define TEST_IRAM_TRANS_NUM     8
+#define TEST_TRANS_LEN          64
+#define TEST_BUFFER_SZ          (TEST_IRAM_TRANS_NUM*TEST_TRANS_LEN)
+
+static void test_slave_iram_master_normal(void){
+    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_handle_t dev_handle = {0};
+    spi_device_interface_config_t devcfg = SPI_DEVICE_TEST_DEFAULT_CONFIG();
+    TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &dev_handle));
+
+    uint8_t *master_send = heap_caps_malloc(TEST_BUFFER_SZ, MALLOC_CAP_DMA);
+    uint8_t *master_recv = heap_caps_calloc(1, TEST_BUFFER_SZ, MALLOC_CAP_DMA);
+    uint8_t *master_exp  = heap_caps_malloc(TEST_BUFFER_SZ, MALLOC_CAP_DEFAULT);
+    get_tx_buffer(1001, master_send, master_exp, TEST_BUFFER_SZ);
+    spi_transaction_t trans_cfg = {
+        .tx_buffer = master_send,
+        .rx_buffer = master_recv,
+        .user = master_exp,
+        .length = TEST_TRANS_LEN * 8,
+    };
+
+    //first trans to trigger slave enter isr
+    unity_send_signal("Master ready");
+    unity_wait_for_signal("Slave ready");
+    TEST_ESP_OK(spi_device_transmit(dev_handle, &trans_cfg));
+
+    for(uint8_t cnt = 0; cnt < TEST_IRAM_TRANS_NUM; cnt ++){
+        trans_cfg.tx_buffer = master_send + TEST_TRANS_LEN*cnt;
+        trans_cfg.rx_buffer = master_recv + TEST_TRANS_LEN*cnt;
+        trans_cfg.user = master_exp + TEST_TRANS_LEN*cnt;
+        unity_wait_for_signal("Slave ready");
+        TEST_ESP_OK(spi_device_transmit(dev_handle, &trans_cfg));
+        ESP_LOG_BUFFER_HEX("master tx", trans_cfg.tx_buffer, TEST_TRANS_LEN);
+        // ESP_LOG_BUFFER_HEX("master rx", trans_cfg.rx_buffer, TEST_TRANS_LEN);
+        // ESP_LOG_BUFFER_HEX("master exp", trans_cfg.user, TEST_TRANS_LEN);
+        spitest_cmp_or_dump(trans_cfg.user, trans_cfg.rx_buffer, TEST_TRANS_LEN);
+    }
+
+    free(master_send);
+    free(master_recv);
+    free(master_exp);
+    spi_bus_remove_device(dev_handle);
+    spi_bus_free(TEST_SPI_HOST);
+}
+
+//------------------------------------test slave func-----------------------------------------
+static IRAM_ATTR void ESP_LOG_BUFFER_HEX_ISR(const char *tag, const uint8_t *buff, const uint32_t byte_len){
+    esp_rom_printf(DRAM_STR("%s: "), tag);
+    for(uint16_t i=0; i<byte_len; i++){
+        if(0 == i%16) esp_rom_printf(DRAM_STR("\n"));
+        if(buff[i] < 0x10) esp_rom_printf(DRAM_STR("0"));
+        esp_rom_printf(DRAM_STR("%x "), buff[i]);
+    } esp_rom_printf(DRAM_STR("\n"));
+}
+
+static uint32_t slave_isr_cnt, test_fail;
+static IRAM_ATTR void test_spi_slave_post_trans_cbk(spi_slave_transaction_t *curr_trans){
+    slave_isr_cnt ++;
+
+    // first trans is the trigger trans with random data by master
+    if(slave_isr_cnt > 1){
+        ESP_LOG_BUFFER_HEX_ISR(DRAM_STR("slave tx"), curr_trans->tx_buffer, curr_trans->trans_len/8);
+        if(memcmp(curr_trans->rx_buffer, curr_trans->user, curr_trans->trans_len/8)){
+            ESP_LOG_BUFFER_HEX_ISR(DRAM_STR("slave rx"), curr_trans->rx_buffer, curr_trans->trans_len/8);
+            ESP_LOG_BUFFER_HEX_ISR(DRAM_STR("slave exp"), curr_trans->user, curr_trans->trans_len/8);
+            test_fail = true;
+        }
+    }
+    if(slave_isr_cnt <= TEST_IRAM_TRANS_NUM) esp_rom_printf(DRAM_STR("Send signal: [Slave ready]!\n"));
+}
+
+static IRAM_ATTR void spi_slave_trans_in_isr(void){
+    spi_bus_config_t bus_cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    spi_slave_interface_config_t slvcfg = {
+        .mode = 0,
+        .spics_io_num = SPI2_IOMUX_PIN_NUM_CS,
+        .flags = SPI_SLAVE_NO_RETURN_RESULT,
+        .queue_size = 16,
+        .post_trans_cb = test_spi_slave_post_trans_cbk,
+    };
+    TEST_ESP_OK(spi_slave_initialize(TEST_SPI_HOST, &bus_cfg, &slvcfg, SPI_DMA_CH_AUTO));
+
+    uint8_t *slave_iram_send = heap_caps_malloc(TEST_BUFFER_SZ, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL);
+    uint8_t *slave_iram_recv = heap_caps_calloc(1, TEST_BUFFER_SZ, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL);
+    uint8_t *slave_iram_exp  = heap_caps_malloc(TEST_BUFFER_SZ, MALLOC_CAP_DEFAULT | MALLOC_CAP_INTERNAL);
+    get_tx_buffer(1001, slave_iram_exp, slave_iram_send, TEST_BUFFER_SZ);
+    spi_slave_transaction_t trans_cfg[TEST_IRAM_TRANS_NUM] = {0};
+
+    unity_wait_for_signal("Master ready");
+    trans_cfg[0].tx_buffer = slave_iram_send;
+    trans_cfg[0].rx_buffer = slave_iram_recv;
+    trans_cfg[0].user = slave_iram_exp;
+    trans_cfg[0].length = TEST_TRANS_LEN * 8;
+    spi_slave_queue_trans(TEST_SPI_HOST, &trans_cfg[0], portMAX_DELAY);
+
+    // mount several transaction first
+    for(uint8_t i=0; i<TEST_IRAM_TRANS_NUM; i++){
+        trans_cfg[i].tx_buffer = slave_iram_send + TEST_TRANS_LEN*i;
+        trans_cfg[i].rx_buffer = slave_iram_recv + TEST_TRANS_LEN*i;
+        trans_cfg[i].user = slave_iram_exp + TEST_TRANS_LEN*i;
+        trans_cfg[i].length = TEST_TRANS_LEN * 8;
+        spi_slave_queue_trans(TEST_SPI_HOST, &trans_cfg[i], portMAX_DELAY);
+    }
+
+    // disable cache then send signal `ready` to start transaction
+    spi_flash_disable_interrupts_caches_and_other_cpu();
+    esp_rom_printf(DRAM_STR("Send signal: [Slave ready]!\n"));
+    while(slave_isr_cnt < TEST_IRAM_TRANS_NUM + 1){
+        esp_rom_delay_us(10);
+    }
+    spi_flash_enable_interrupts_caches_and_other_cpu();
+    if(test_fail) TEST_FAIL();
+
+    free(slave_iram_send);
+    free(slave_iram_recv);
+    free(slave_iram_exp);
+    spi_slave_free(TEST_SPI_HOST);
+}
+
+TEST_CASE_MULTIPLE_DEVICES("SPI_Slave: Test_ISR_IRAM_disable_cache", "[spi_ms]", test_slave_iram_master_normal, spi_slave_trans_in_isr);
+#endif // CONFIG_SPI_SLAVE_ISR_IN_IRAM

components/driver/test_apps/spi/slave/pytest_spi_slave.py

@@ -17,7 +17,7 @@ def test_slave_single_dev(case_tester) -> None:       # type: ignore
 # if `test_env` not defined, will run on `generic_multi_device` by default
 @pytest.mark.supported_targets
 @pytest.mark.generic_multi_device
-@pytest.mark.parametrize('count', [2,], indirect=True)
+@pytest.mark.parametrize('count, config', [(2, 'defaults'), (2, 'iram_safe')], indirect=True)
 def test_slave_multi_dev(case_tester) -> None:        # type: ignore
     for case in case_tester.test_menu:
         if case.attributes.get('test_env', 'generic_multi_device') == 'generic_multi_device':

components/driver/test_apps/spi/slave/sdkconfig.ci.defaults

@@ -0,0 +1,2 @@
+# don't delete.
+# used for CI to compile a default config when 'sdkconfig.ci.xxxx' is exist

components/driver/test_apps/spi/slave/sdkconfig.ci.iram_safe

@@ -0,0 +1,7 @@
+CONFIG_COMPILER_DUMP_RTL_FILES=y
+CONFIG_SPI_MASTER_ISR_IN_IRAM=n
+CONFIG_SPI_SLAVE_ISR_IN_IRAM=y
+CONFIG_COMPILER_OPTIMIZATION_ASSERTIONS_DISABLE=y
+CONFIG_COMPILER_OPTIMIZATION_NONE=y
+# silent the error check, as the error string are stored in rodata, causing RTL check failure
+CONFIG_COMPILER_OPTIMIZATION_CHECKS_SILENT=y

components/esp_hw_support/linker.lf

@@ -22,6 +22,7 @@ entries:
         rtc_wdt (noflash_text)
     if PERIPH_CTRL_FUNC_IN_IRAM = y:
         periph_ctrl: periph_module_reset (noflash)
+    if PERIPH_CTRL_FUNC_IN_IRAM = y && ESP32_WIFI_ENABLED = y:
         periph_ctrl: wifi_module_enable (noflash)
         periph_ctrl: wifi_module_disable (noflash)
     if GDMA_CTRL_FUNC_IN_IRAM = y:

components/hal/esp32/include/hal/gpio_ll.h

@@ -625,10 +625,11 @@ static inline bool gpio_ll_is_digital_io_hold(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(DR_REG_IO_MUX_BASE + GPIO_PIN_MUX_REG_OFFSET[gpio]);
 }
 
 /**

components/hal/esp32c2/include/hal/gpio_ll.h

@@ -441,10 +441,11 @@ static inline bool gpio_ll_is_digital_io_hold(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(IO_MUX_GPIO0_REG + (gpio * 4));
 }
 
 /**

components/hal/esp32c3/include/hal/gpio_ll.h

@@ -453,10 +453,11 @@ static inline bool gpio_ll_is_digital_io_hold(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(IO_MUX_GPIO0_REG + (gpio * 4));
 }
 
 /**

components/hal/esp32c6/include/hal/gpio_ll.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -402,6 +402,7 @@ static inline void gpio_ll_hold_dis(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;

components/hal/esp32h4/include/rev1/hal/gpio_ll.h

@@ -424,10 +424,11 @@ static inline void gpio_ll_hold_dis(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(IO_MUX_GPIO0_REG + (gpio * 4));
 }
 
 /**

components/hal/esp32h4/include/rev2/hal/gpio_ll.h

@@ -400,10 +400,11 @@ static inline void gpio_ll_hold_dis(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(IO_MUX_GPIO0_REG + (gpio * 4));
 }
 
 /**

components/hal/esp32s2/include/hal/gpio_ll.h

@@ -456,10 +456,11 @@ static inline bool gpio_ll_is_digital_io_hold(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(IO_MUX_GPIO0_REG + (gpio * 4));
 }
 
 /**

components/hal/esp32s3/include/hal/gpio_ll.h

@@ -472,10 +472,11 @@ static inline bool gpio_ll_is_digital_io_hold(gpio_dev_t *hw, uint32_t gpio_num)
   * @param gpio_num GPIO number of the pad.
   * @param signal_idx Peripheral signal id to input. One of the ``*_IN_IDX`` signals in ``soc/gpio_sig_map.h``.
   */
+__attribute__((always_inline))
 static inline void gpio_ll_iomux_in(gpio_dev_t *hw, uint32_t gpio, uint32_t signal_idx)
 {
     hw->func_in_sel_cfg[signal_idx].sig_in_sel = 0;
-    PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]);
+    PIN_INPUT_ENABLE(IO_MUX_GPIO0_REG + (gpio * 4));
 }
 
 /**