diff --git a/components/driver/Kconfig b/components/driver/Kconfig index 8e40c8b5db..1e2bb35d20 100644 --- a/components/driver/Kconfig +++ b/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. diff --git a/components/driver/spi/gpspi/spi_master.c b/components/driver/spi/gpspi/spi_master.c index 84263fe192..0f4c5639dd 100644 --- a/components/driver/spi/gpspi/spi_master.c +++ b/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); diff --git a/components/driver/spi/gpspi/spi_slave.c b/components/driver/spi/gpspi/spi_slave.c index 46117b4a2b..b087238be9 100644 --- a/components/driver/spi/gpspi/spi_slave.c +++ b/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; diff --git a/components/driver/spi/spi_bus_lock.c b/components/driver/spi/spi_bus_lock.c index 506c1444b7..c74998ecf7 100644 --- a/components/driver/spi/spi_bus_lock.c +++ b/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 #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. diff --git a/components/driver/test_apps/spi/master/CMakeLists.txt b/components/driver/test_apps/spi/master/CMakeLists.txt index 9f572a2fe7..7f40d3b5ea 100644 --- a/components/driver/test_apps/spi/master/CMakeLists.txt +++ b/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() diff --git a/components/driver/test_apps/spi/master/main/test_spi_master.c b/components/driver/test_apps/spi/master/main/test_spi_master.c index 1a929ae373..8c63cf8aa5 100644 --- a/components/driver/test_apps/spi/master/main/test_spi_master.c +++ b/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 diff --git a/components/driver/test_apps/spi/master/pytest_spi_master.py b/components/driver/test_apps/spi/master/pytest_spi_master.py index 91be5ebbf6..90c22cbe73 100644 --- a/components/driver/test_apps/spi/master/pytest_spi_master.py +++ b/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': diff --git a/components/driver/test_apps/spi/master/sdkconfig.ci.iram_safe b/components/driver/test_apps/spi/master/sdkconfig.ci.iram_safe new file mode 100644 index 0000000000..a18c660069 --- /dev/null +++ b/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 diff --git a/components/driver/test_apps/spi/slave/main/test_spi_slave.c b/components/driver/test_apps/spi/slave/main/test_spi_slave.c index 55317f7032..0989453c7f 100644 --- a/components/driver/test_apps/spi/slave/main/test_spi_slave.c +++ b/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; diff --git a/docs/en/api-reference/peripherals/spi_master.rst b/docs/en/api-reference/peripherals/spi_master.rst index e5000a9d90..aca8b40722 100644 --- a/docs/en/api-reference/peripherals/spi_master.rst +++ b/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. +-----------+----------------------+--------------------+------------+-------------+