Merge branch 'bugfix/fix_spi_master_iram_safe_env' into 'master'

spi_master: fix whole master driver iram safe enviroenment

Closes IDF-6848

See merge request espressif/esp-idf!22401

components/driver/Kconfig

@@ -67,6 +67,7 @@ menu "Driver Configurations"
         config SPI_MASTER_IN_IRAM
             bool "Place transmitting functions of SPI master into IRAM"
             default n
+            depends on !FREERTOS_PLACE_FUNCTIONS_INTO_FLASH
             select SPI_MASTER_ISR_IN_IRAM
             help
                 Normally only the ISR of SPI master is placed in the IRAM, so that it
@@ -77,11 +78,14 @@ menu "Driver Configurations"
                 Enable this to put ``queue_trans``, ``get_trans_result`` and
                 ``transmit`` functions into the IRAM to avoid possible cache miss.
 
+                This configuration won't be available if `CONFIG_FREERTOS_PLACE_FUNCTIONS_INTO_FLASH` is enabled.
+
                 During unit test, this is enabled to measure the ideal case of api.
 
         config SPI_MASTER_ISR_IN_IRAM
             bool "Place SPI master ISR function into IRAM"
             default y
+            select PERIPH_CTRL_FUNC_IN_IRAM
             help
                 Place the SPI master ISR in to IRAM to avoid possible cache miss.
 

components/driver/spi/gpspi/spi_master.c

@@ -116,6 +116,7 @@ We have two bits to control the interrupt:
 #include "clk_tree.h"
 #include "clk_ctrl_os.h"
 #include "esp_log.h"
+#include "esp_check.h"
 #include "esp_ipc.h"
 #include "freertos/task.h"
 #include "freertos/queue.h"
@@ -168,11 +169,7 @@ struct spi_device_t {
 static spi_host_t* bus_driver_ctx[SOC_SPI_PERIPH_NUM] = {};
 
 static const char *SPI_TAG = "spi_master";
-#define SPI_CHECK(a, str, ret_val, ...) \
-    if (unlikely(!(a))) { \
-        ESP_LOGE(SPI_TAG,"%s(%d): "str, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
-        return (ret_val); \
-    }
+#define SPI_CHECK(a, str, ret_val)  ESP_RETURN_ON_FALSE_ISR(a, ret_val, SPI_TAG, str)
 
 
 static void spi_intr(void *arg);

components/driver/spi/gpspi/spi_slave.c

@@ -116,11 +116,7 @@ typedef struct {
 static void ipc_isr_reg_to_core(void *args)
 {
     spi_slave_t *host = ((spi_ipc_param_t *)args)->host;
-    int flags = host->intr_flags | ESP_INTR_FLAG_INTRDISABLED;
-#ifdef CONFIG_SPI_SLAVE_ISR_IN_IRAM
-    flags |= ESP_INTR_FLAG_IRAM;
-#endif
-    *((spi_ipc_param_t *)args)->err = esp_intr_alloc(spicommon_irqsource_for_host(host->id), flags, spi_intr, (void *)host, &host->intr);
+    *((spi_ipc_param_t *)args)->err = esp_intr_alloc(spicommon_irqsource_for_host(host->id), host->intr_flags | ESP_INTR_FLAG_INTRDISABLED, spi_intr, (void *)host, &host->intr);
 }
 #endif
 
@@ -235,11 +231,7 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b
     } else
 #endif
     {
-        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);
+        err = esp_intr_alloc(spicommon_irqsource_for_host(host), bus_config->intr_flags | ESP_INTR_FLAG_INTRDISABLED, spi_intr, (void *)spihost[host], &spihost[host]->intr);
     }
     if (err != ESP_OK) {
         ret = err;

components/driver/spi/spi_bus_lock.c

@@ -13,6 +13,7 @@
 #include "soc/soc_caps.h"
 #include "stdatomic.h"
 #include "esp_log.h"
+#include "esp_check.h"
 #include <strings.h>
 #include "esp_heap_caps.h"
 
@@ -257,12 +258,6 @@ portMUX_TYPE s_spinlock = portMUX_INITIALIZER_UNLOCKED;
 
 DRAM_ATTR static const char TAG[] = "bus_lock";
 
-#define LOCK_CHECK(a, str, ret_val, ...) \
-    if (!(a)) { \
-        ESP_LOGE(TAG,"%s(%d): "str, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
-        return (ret_val); \
-    }
-
 static inline int mask_get_id(uint32_t mask);
 static inline int dev_lock_get_id(spi_bus_lock_dev_t *dev_lock);
 
@@ -709,7 +704,7 @@ IRAM_ATTR bool spi_bus_lock_touch(spi_bus_lock_dev_handle_t dev_handle)
  ******************************************************************************/
 IRAM_ATTR esp_err_t spi_bus_lock_acquire_start(spi_bus_lock_dev_t *dev_handle, TickType_t wait)
 {
-    LOCK_CHECK(wait == portMAX_DELAY, "timeout other than portMAX_DELAY not supported", ESP_ERR_INVALID_ARG);
+    ESP_RETURN_ON_FALSE_ISR(wait == portMAX_DELAY, ESP_ERR_INVALID_ARG, TAG, "timeout other than portMAX_DELAY not supported");
 
     spi_bus_lock_t* lock = dev_handle->parent;
 
@@ -737,7 +732,7 @@ IRAM_ATTR esp_err_t spi_bus_lock_acquire_end(spi_bus_lock_dev_t *dev_handle)
 {
     //release the bus
     spi_bus_lock_t* lock = dev_handle->parent;
-    LOCK_CHECK(lock->acquiring_dev == dev_handle, "Cannot release a lock that hasn't been acquired.", ESP_ERR_INVALID_STATE);
+    ESP_RETURN_ON_FALSE_ISR(lock->acquiring_dev == dev_handle, ESP_ERR_INVALID_STATE, TAG, "Cannot release a lock that hasn't been acquired.");
 
     acquire_end_core(dev_handle);
 
@@ -772,8 +767,9 @@ SPI_MASTER_ATTR esp_err_t spi_bus_lock_bg_request(spi_bus_lock_dev_t *dev_handle
 IRAM_ATTR esp_err_t spi_bus_lock_wait_bg_done(spi_bus_lock_dev_handle_t dev_handle, TickType_t wait)
 {
     spi_bus_lock_t *lock = dev_handle->parent;
-    LOCK_CHECK(lock->acquiring_dev == dev_handle, "Cannot wait for a device that is not acquired", ESP_ERR_INVALID_STATE);
-    LOCK_CHECK(wait == portMAX_DELAY, "timeout other than portMAX_DELAY not supported", ESP_ERR_INVALID_ARG);
+
+    ESP_RETURN_ON_FALSE_ISR(lock->acquiring_dev == dev_handle, ESP_ERR_INVALID_STATE, TAG, "Cannot wait for a device that is not acquired");
+    ESP_RETURN_ON_FALSE_ISR(wait == portMAX_DELAY, ESP_ERR_INVALID_ARG, TAG, "timeout other than portMAX_DELAY not supported");
 
     // If no BG bits active, skip quickly. This is ensured by `spi_bus_lock_wait_bg_done`
     // cannot be executed with `bg_request` on the same device concurrently.

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

@@ -8,3 +8,16 @@ set(EXTRA_COMPONENT_DIRS
 
 include($ENV{IDF_PATH}/tools/cmake/project.cmake)
 project(spi_master_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_master_test.elf
+                      find-refs
+                      --from-sections=.iram0.text
+                      --to-sections=.flash.text,.flash.rodata
+                      --exit-code
+                      DEPENDS ${elf}
+                      )
+endif()

components/driver/test_apps/spi/master/main/test_spi_master.c

@@ -1600,7 +1600,7 @@ void test_add_device_slave(void)
     spi_bus_free(TEST_SPI_HOST);
 }
 
-TEST_CASE_MULTIPLE_DEVICES("SPI_Master:Test multiple devices", "[spi_ms][test_env=generic_multi_device]", test_add_device_master, test_add_device_slave);
+TEST_CASE_MULTIPLE_DEVICES("SPI_Master:Test multiple devices", "[spi_ms]", test_add_device_master, test_add_device_slave);
 
 
 #if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
@@ -1660,3 +1660,110 @@ TEST_CASE("test_master_isr_pin_to_core","[spi]")
     TEST_ASSERT_EQUAL_UINT32(TEST_ISR_CNT, master_expect);
 }
 #endif
+
+#if CONFIG_SPI_MASTER_IN_IRAM
+
+#define TEST_MASTER_IRAM_TRANS_LEN  120
+static IRAM_ATTR void test_master_iram_post_trans_cbk(spi_transaction_t *trans)
+{
+    *((bool *)trans->user) = true;
+}
+
+static IRAM_ATTR void test_master_iram(void)
+{
+    spi_bus_config_t buscfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    buscfg.intr_flags = ESP_INTR_FLAG_IRAM;
+    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();
+    devcfg.post_cb = test_master_iram_post_trans_cbk;
+    TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &dev_handle));
+
+    bool flag_trans_done;
+    uint8_t *master_send = heap_caps_malloc(TEST_MASTER_IRAM_TRANS_LEN, MALLOC_CAP_DMA);
+    uint8_t *master_recv = heap_caps_calloc(1, TEST_MASTER_IRAM_TRANS_LEN, MALLOC_CAP_DMA);
+    uint8_t *master_exp  = heap_caps_malloc(TEST_MASTER_IRAM_TRANS_LEN, MALLOC_CAP_DEFAULT);
+    get_tx_buffer(211, master_send, master_exp, TEST_MASTER_IRAM_TRANS_LEN);
+    spi_transaction_t trans_cfg = {
+        .tx_buffer = master_send,
+        .rx_buffer = master_recv,
+        .user = &flag_trans_done,
+        .length = TEST_MASTER_IRAM_TRANS_LEN * 8,
+    }, *ret_trans;
+
+    // Test intrrupt trans api once -----------------------------
+    unity_send_signal("Master ready");
+    unity_wait_for_signal("Slave ready");
+
+    spi_flash_disable_interrupts_caches_and_other_cpu();
+    flag_trans_done = false;
+    spi_device_queue_trans(dev_handle, &trans_cfg, portMAX_DELAY);
+    while(!flag_trans_done) {
+        // waitting for transaction done and return from ISR
+    }
+    spi_device_get_trans_result(dev_handle, &ret_trans, portMAX_DELAY);
+    spi_flash_enable_interrupts_caches_and_other_cpu();
+
+    ESP_LOG_BUFFER_HEX("master tx", ret_trans->tx_buffer, TEST_MASTER_IRAM_TRANS_LEN);
+    ESP_LOG_BUFFER_HEX("master rx", ret_trans->rx_buffer, TEST_MASTER_IRAM_TRANS_LEN);
+    spitest_cmp_or_dump(master_exp, trans_cfg.rx_buffer, TEST_MASTER_IRAM_TRANS_LEN);
+
+    // Test polling trans api once -------------------------------
+    unity_wait_for_signal("Slave ready");
+    get_tx_buffer(119, master_send, master_exp, TEST_MASTER_IRAM_TRANS_LEN);
+
+    spi_flash_disable_interrupts_caches_and_other_cpu();
+    spi_device_polling_transmit(dev_handle, &trans_cfg);
+    spi_flash_enable_interrupts_caches_and_other_cpu();
+
+    ESP_LOG_BUFFER_HEX("master tx", ret_trans->tx_buffer, TEST_MASTER_IRAM_TRANS_LEN);
+    ESP_LOG_BUFFER_HEX("master rx", ret_trans->rx_buffer, TEST_MASTER_IRAM_TRANS_LEN);
+    spitest_cmp_or_dump(master_exp, trans_cfg.rx_buffer, TEST_MASTER_IRAM_TRANS_LEN);
+
+    free(master_send);
+    free(master_recv);
+    free(master_exp);
+    spi_bus_remove_device(dev_handle);
+    spi_bus_free(TEST_SPI_HOST);
+}
+
+static void test_iram_slave_normal(void)
+{
+    uint8_t *slave_sendbuf = heap_caps_malloc(TEST_MASTER_IRAM_TRANS_LEN, MALLOC_CAP_DMA);
+    uint8_t *slave_recvbuf = heap_caps_calloc(1, TEST_MASTER_IRAM_TRANS_LEN, MALLOC_CAP_DMA);
+    uint8_t *slave_expect = heap_caps_malloc(TEST_MASTER_IRAM_TRANS_LEN, MALLOC_CAP_DEFAULT);
+
+    spi_bus_config_t bus_cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG();
+    TEST_ESP_OK(spi_slave_initialize(TEST_SPI_HOST, &bus_cfg, &slvcfg, SPI_DMA_CH_AUTO));
+
+    spi_slave_transaction_t slave_trans = {};
+    slave_trans.length = TEST_MASTER_IRAM_TRANS_LEN * 8;
+    slave_trans.tx_buffer = slave_sendbuf;
+    slave_trans.rx_buffer = slave_recvbuf;
+    get_tx_buffer(211, slave_expect, slave_sendbuf, TEST_MASTER_IRAM_TRANS_LEN);
+
+    unity_wait_for_signal("Master ready");
+    unity_send_signal("Slave ready");
+    spi_slave_transmit(TEST_SPI_HOST, &slave_trans, portMAX_DELAY);
+    ESP_LOG_BUFFER_HEX("slave tx", slave_sendbuf, TEST_MASTER_IRAM_TRANS_LEN);
+    ESP_LOG_BUFFER_HEX("slave rx", slave_recvbuf, TEST_MASTER_IRAM_TRANS_LEN);
+    spitest_cmp_or_dump(slave_expect, slave_recvbuf, TEST_MASTER_IRAM_TRANS_LEN);
+
+    unity_send_signal("Slave ready");
+    get_tx_buffer(119, slave_expect, slave_sendbuf, TEST_MASTER_IRAM_TRANS_LEN);
+    spi_slave_transmit(TEST_SPI_HOST, &slave_trans, portMAX_DELAY);
+    ESP_LOG_BUFFER_HEX("slave tx", slave_sendbuf, TEST_MASTER_IRAM_TRANS_LEN);
+    ESP_LOG_BUFFER_HEX("slave rx", slave_recvbuf, TEST_MASTER_IRAM_TRANS_LEN);
+    spitest_cmp_or_dump(slave_expect, slave_recvbuf, TEST_MASTER_IRAM_TRANS_LEN);
+
+    free(slave_sendbuf);
+    free(slave_recvbuf);
+    free(slave_expect);
+    spi_slave_free(TEST_SPI_HOST);
+    spi_bus_free(TEST_SPI_HOST);
+}
+
+TEST_CASE_MULTIPLE_DEVICES("SPI_Master:IRAM_safe", "[spi_ms]", test_master_iram, test_iram_slave_normal);
+#endif

components/driver/test_apps/spi/master/pytest_spi_master.py

@@ -31,7 +31,17 @@ def test_master_esp_flash(case_tester) -> None:        # type: ignore
 @pytest.mark.supported_targets
 @pytest.mark.esp32h2
 @pytest.mark.generic_multi_device
-@pytest.mark.parametrize('count, config', [(2, 'defaults',), (2, 'release',), (2, 'freertos_compliance',), (2, 'freertos_flash',)], indirect=True)
+@pytest.mark.parametrize(
+    'count, config',
+    [
+        (2, 'defaults',),
+        (2, 'release',),
+        (2, 'freertos_compliance',),
+        (2, 'freertos_flash',),
+        (2, 'iram_safe'),
+    ],
+    indirect=True
+)
 def test_master_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/master/sdkconfig.ci.iram_safe

@@ -0,0 +1,8 @@
+CONFIG_COMPILER_DUMP_RTL_FILES=y
+CONFIG_SPI_MASTER_ISR_IN_IRAM=y
+CONFIG_SPI_MASTER_IN_IRAM=y
+CONFIG_SPI_SLAVE_ISR_IN_IRAM=n
+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/driver/test_apps/spi/slave/main/test_spi_slave.c

@@ -480,6 +480,7 @@ static IRAM_ATTR void test_slave_iram_post_trans_cbk(spi_slave_transaction_t *cu
 static IRAM_ATTR void test_slave_isr_iram(void)
 {
     spi_bus_config_t bus_cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    bus_cfg.intr_flags |= ESP_INTR_FLAG_IRAM;
     spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG();
     slvcfg.flags = SPI_SLAVE_NO_RETURN_RESULT;
     slvcfg.queue_size = 16;
@@ -561,6 +562,7 @@ static IRAM_ATTR void test_trans_in_isr_post_trans_cbk(spi_slave_transaction_t *
 static IRAM_ATTR void spi_slave_trans_in_isr(void)
 {
     spi_bus_config_t bus_cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    bus_cfg.intr_flags |= ESP_INTR_FLAG_IRAM;
     spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG();
     slvcfg.flags = SPI_SLAVE_NO_RETURN_RESULT;
     slvcfg.queue_size = 16;
@@ -647,6 +649,7 @@ static IRAM_ATTR void test_queue_reset_in_isr_post_trans_cbk(spi_slave_transacti
 static IRAM_ATTR void spi_queue_reset_in_isr(void)
 {
     spi_bus_config_t bus_cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    bus_cfg.intr_flags |= ESP_INTR_FLAG_IRAM;
     spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG();
     slvcfg.flags = SPI_SLAVE_NO_RETURN_RESULT;
     slvcfg.queue_size = 16;

docs/en/api-reference/peripherals/spi_master.rst

@@ -541,6 +541,10 @@ Cache Miss
 
 The default config puts only the ISR into the IRAM. Other SPI related functions, including the driver itself and the callback, might suffer from cache misses and will need to wait until the code is read from flash. Select :ref:`CONFIG_SPI_MASTER_IN_IRAM` to put the whole SPI driver into IRAM and put the entire callback(s) and its callee functions into IRAM to prevent cache misses.
 
+.. note::
+
+    SPI driver implementation is based on FreeRTOS APIs, to use :ref:`CONFIG_SPI_MASTER_IN_IRAM`, you should not enable :ref:`CONFIG_FREERTOS_PLACE_FUNCTIONS_INTO_FLASH`.
+
 For an interrupt transaction, the overall cost is *20+8n/Fspi[MHz]* [us] for n bytes transferred in one transaction. Hence, the transferring speed is: *n/(20+8n/Fspi)*. An example of transferring speed at 8 MHz clock speed is given in the following table.
 
 +-----------+----------------------+--------------------+------------+-------------+