Merge branch 'feature/spi_isr_pin_to_core' into 'master'

spi: add feature to assign core_id of spi isr registered

Closes IDF-6371

See merge request espressif/esp-idf!21597

components/driver/include/driver/intr_types.h

@@ -0,0 +1,23 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+#pragma once
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+///< Selecting the ISR to be registered on which core
+typedef enum {
+    INTR_CPU_ID_AUTO,       ///< Register intr ISR to core automatically select by FreeRTOS.
+    INTR_CPU_ID_0,          ///< Register intr ISR to core 0.
+    INTR_CPU_ID_1,          ///< Register intr ISR to core 1.
+} intr_cpu_id_t;
+
+#define INTR_CPU_CONVERT_ID(cpu_id)     ((cpu_id) - 1)
+
+#ifdef __cplusplus
+}
+#endif

components/driver/include/driver/spi_common.h

@@ -10,6 +10,8 @@
 #include <stdbool.h>
 #include "sdkconfig.h"
 #include "esp_err.h"
+#include "esp_ipc.h"
+#include "intr_types.h"
 #include "hal/spi_types.h"
 
 #ifdef __cplusplus
@@ -115,6 +117,7 @@ typedef struct {
     int data7_io_num;     ///< GPIO pin for spi data7 signal in octal mode, or -1 if not used.
     int max_transfer_sz;  ///< Maximum transfer size, in bytes. Defaults to 4092 if 0 when DMA enabled, or to `SOC_SPI_MAXIMUM_BUFFER_SIZE` if DMA is disabled.
     uint32_t flags;       ///< Abilities of bus to be checked by the driver. Or-ed value of ``SPICOMMON_BUSFLAG_*`` flags.
+    intr_cpu_id_t  isr_cpu_id;    ///< Select cpu core to register SPI ISR.
     int intr_flags;       /**< Interrupt flag for the bus to set the priority, and IRAM attribute, see
                            *  ``esp_intr_alloc.h``. Note that the EDGE, INTRDISABLED attribute are ignored
                            *  by the driver. Note that if ESP_INTR_FLAG_IRAM is set, ALL the callbacks of

components/driver/spi_master.c

@@ -115,6 +115,7 @@ We have two bits to control the interrupt:
 #include "driver/spi_master.h"
 #include "clk_tree.h"
 #include "esp_log.h"
+#include "esp_ipc.h"
 #include "freertos/task.h"
 #include "freertos/queue.h"
 #include "soc/soc_memory_layout.h"
@@ -191,6 +192,20 @@ static inline bool is_valid_host(spi_host_device_t host)
 #endif
 }
 
+#if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
+typedef struct {
+    spi_host_t *spi_host;
+    esp_err_t *err;
+} spi_ipc_param_t;
+
+static void ipc_isr_reg_to_core(void *args)
+{
+    spi_host_t *host = ((spi_ipc_param_t *)args)->spi_host;
+    const spi_bus_attr_t* bus_attr = host->bus_attr;
+    *((spi_ipc_param_t *)args)->err = esp_intr_alloc(spicommon_irqsource_for_host(host->id), bus_attr->bus_cfg.intr_flags | ESP_INTR_FLAG_INTRDISABLED, spi_intr, host, &host->intr);
+}
+#endif
+
 // Should be called before any devices are actually registered or used.
 // Currently automatically called after `spi_bus_initialize()` and when first device is registered.
 static esp_err_t spi_master_init_driver(spi_host_device_t host_id)
@@ -215,11 +230,21 @@ static esp_err_t spi_master_init_driver(spi_host_device_t host_id)
         .bus_attr = bus_attr,
     };
 
+    // interrupts are not allowed on SPI1 bus
     if (host_id != SPI1_HOST) {
-        // interrupts are not allowed on SPI1 bus
-        err = esp_intr_alloc(spicommon_irqsource_for_host(host_id),
-                            bus_attr->bus_cfg.intr_flags | ESP_INTR_FLAG_INTRDISABLED,
-                            spi_intr, host, &host->intr);
+#if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
+        if(bus_attr->bus_cfg.isr_cpu_id > INTR_CPU_ID_AUTO) {
+            SPI_CHECK(bus_attr->bus_cfg.isr_cpu_id <= INTR_CPU_ID_1, "invalid core id", ESP_ERR_INVALID_ARG);
+            spi_ipc_param_t ipc_arg = {
+                .spi_host = host,
+                .err = &err,
+            };
+            esp_ipc_call_blocking(INTR_CPU_CONVERT_ID(bus_attr->bus_cfg.isr_cpu_id), ipc_isr_reg_to_core, (void *) &ipc_arg);
+        } else
+#endif
+        {
+            err = esp_intr_alloc(spicommon_irqsource_for_host(host_id), bus_attr->bus_cfg.intr_flags | ESP_INTR_FLAG_INTRDISABLED, spi_intr, host, &host->intr);
+        }
         if (err != ESP_OK) {
             goto cleanup;
         }

components/driver/spi_slave.c

@@ -55,6 +55,7 @@ typedef struct {
     spi_slave_hal_context_t hal;
     spi_slave_transaction_t *cur_trans;
     uint32_t flags;
+    uint32_t intr_flags;
     int max_transfer_sz;
     QueueHandle_t trans_queue;
     QueueHandle_t ret_queue;
@@ -106,6 +107,23 @@ static inline void SPI_SLAVE_ISR_ATTR restore_cs(spi_slave_t *host)
     }
 }
 
+#if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
+typedef struct {
+    spi_slave_t *host;
+    esp_err_t *err;
+} spi_ipc_param_t;
+
+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);
+}
+#endif
+
 esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *bus_config, const spi_slave_interface_config_t *slave_config, spi_dma_chan_t dma_chan)
 {
     bool spi_chan_claimed;
@@ -205,11 +223,24 @@ 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;
+#if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
+    if(bus_config->isr_cpu_id > INTR_CPU_ID_AUTO) {
+        spihost[host]->intr_flags = bus_config->intr_flags;
+        SPI_CHECK(bus_config->isr_cpu_id <= INTR_CPU_ID_1, "invalid core id", ESP_ERR_INVALID_ARG);
+        spi_ipc_param_t ipc_args = {
+            .host = spihost[host],
+            .err = &err,
+        };
+        esp_ipc_call_blocking(INTR_CPU_CONVERT_ID(bus_config->isr_cpu_id), ipc_isr_reg_to_core, (void *)&ipc_args);
+    } else
+#endif
+    {
+        int flags = bus_config->intr_flags | ESP_INTR_FLAG_INTRDISABLED;
 #ifdef CONFIG_SPI_SLAVE_ISR_IN_IRAM
-    flags |= ESP_INTR_FLAG_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), flags, spi_intr, (void *)spihost[host], &spihost[host]->intr);
+    }
     if (err != ESP_OK) {
         ret = err;
         goto cleanup;

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

@@ -1560,3 +1560,62 @@ void test_add_device_slave(void)
 }
 
 TEST_CASE_MULTIPLE_DEVICES("SPI_Master:Test multiple devices", "[spi_ms][test_env=generic_multi_device]", test_add_device_master, test_add_device_slave);
+
+
+#if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
+
+#define TEST_ISR_CNT    100
+static void test_master_isr_core_post_trans_cbk(spi_transaction_t *curr_trans){
+    *((int *)curr_trans->user) += esp_cpu_get_core_id();
+}
+
+TEST_CASE("test_master_isr_pin_to_core","[spi]")
+{
+    spi_device_handle_t dev0;
+    uint32_t master_send;
+    uint32_t master_recive;
+    uint32_t master_expect;
+
+    spi_bus_config_t buscfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    spi_device_interface_config_t devcfg = SPI_DEVICE_TEST_DEFAULT_CONFIG();
+    devcfg.post_cb = test_master_isr_core_post_trans_cbk;
+
+    spi_transaction_t trans_cfg = {
+        .tx_buffer = &master_send,
+        .rx_buffer = &master_recive,
+        .user = & master_expect,
+        .length = sizeof(uint32_t) * 8,
+    };
+
+    master_expect = 0;
+    for (int i = 0; i < TEST_ISR_CNT; i++) {
+        TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &buscfg, SPI_DMA_CH_AUTO));
+        TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &dev0));
+
+        TEST_ESP_OK(spi_device_transmit(dev0, &trans_cfg));
+
+        TEST_ESP_OK(spi_bus_remove_device(dev0));
+        TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST));
+    }
+    printf("Test Master ISR Not Assign: %d : %ld\n", TEST_ISR_CNT, master_expect);
+    // by default the esp_intr_alloc is called on ESP_MAIN_TASK_AFFINITY_CPU0 now
+    TEST_ASSERT_EQUAL_UINT32(0, master_expect);
+
+
+    //-------------------------------------CPU1---------------------------------------
+    buscfg.isr_cpu_id = INTR_CPU_ID_1;
+
+    master_expect = 0;
+    for (int i = 0; i < TEST_ISR_CNT; i++) {
+        TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &buscfg, SPI_DMA_CH_AUTO));
+        TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &dev0));
+
+        TEST_ESP_OK(spi_device_transmit(dev0, &trans_cfg));
+
+        TEST_ESP_OK(spi_bus_remove_device(dev0));
+        TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST));
+    }
+    printf("Test Master ISR Assign CPU1: %d : %ld\n", TEST_ISR_CNT, master_expect);
+    TEST_ASSERT_EQUAL_UINT32(TEST_ISR_CNT, master_expect);
+}
+#endif

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

@@ -12,6 +12,7 @@
 #include "unity.h"
 #include "test_utils.h"
 #include "test_spi_utils.h"
+#include "hal/spi_slave_hal.h"
 #include "driver/spi_master.h"
 #include "driver/spi_slave.h"
 #include "driver/gpio.h"
@@ -688,6 +689,62 @@ static IRAM_ATTR void spi_queue_reset_in_isr(void)
     spi_slave_free(TEST_SPI_HOST);
 }
 TEST_CASE_MULTIPLE_DEVICES("SPI_Slave: Test_Queue_Reset_in_ISR", "[spi_ms]", test_slave_iram_master_normal, spi_queue_reset_in_isr);
+#endif // CONFIG_SPI_SLAVE_ISR_IN_IRAM
 
 
-#endif // CONFIG_SPI_SLAVE_ISR_IN_IRAM
+#if (SOC_CPU_CORES_NUM > 1) && (!CONFIG_FREERTOS_UNICORE)
+
+#define TEST_ISR_CNT    100
+static void test_slave_isr_core_setup_cbk(spi_slave_transaction_t *curr_trans){
+    *((int *)curr_trans->user) += esp_cpu_get_core_id();
+}
+
+TEST_CASE("test_slave_isr_pin_to_core","[spi]")
+{
+    uint32_t slave_send;
+    uint32_t slave_recive;
+    uint32_t slave_expect;
+
+    spi_bus_config_t buscfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG();
+    slvcfg.post_setup_cb = test_slave_isr_core_setup_cbk;
+
+    spi_slave_transaction_t trans_cfg = {
+        .tx_buffer = &slave_send,
+        .rx_buffer = &slave_recive,
+        .user = &slave_expect,
+        .length = sizeof(uint32_t) * 8,
+    };
+
+    slave_expect = 0;
+    for (int i = 0; i < TEST_ISR_CNT; i++) {
+        TEST_ESP_OK(spi_slave_initialize(TEST_SPI_HOST, &buscfg, &slvcfg, SPI_DMA_CH_AUTO));
+        TEST_ESP_OK(spi_slave_queue_trans(TEST_SPI_HOST, &trans_cfg, portMAX_DELAY));
+        TEST_ESP_OK(spi_slave_free(TEST_SPI_HOST));
+    }
+    printf("Test Slave ISR Not Assign: %d : %ld\n", TEST_ISR_CNT, slave_expect);
+    // by default the esp_intr_alloc is called on ESP_MAIN_TASK_AFFINITY_CPU0 now
+    TEST_ASSERT_EQUAL_UINT32(0, slave_expect);
+
+
+    //-------------------------------------CPU1---------------------------------------
+    buscfg.isr_cpu_id = INTR_CPU_ID_1;
+
+    slave_expect = 0;
+    for (int i = 0; i < TEST_ISR_CNT; i++) {
+        TEST_ESP_OK(spi_slave_initialize(TEST_SPI_HOST, &buscfg, &slvcfg, SPI_DMA_CH_AUTO));
+
+        TEST_ESP_OK(spi_slave_queue_trans(TEST_SPI_HOST, &trans_cfg, portMAX_DELAY));
+        // This two delay used for hardware to activate a interrupt after invoke
+        vTaskDelay(1);
+        // to invoke a trans_done intr for spi slave without a master
+        spi_ll_set_int_stat(SPI_LL_GET_HW(TEST_SPI_HOST));
+        vTaskDelay(1);
+
+        TEST_ESP_OK(spi_slave_free(TEST_SPI_HOST));
+    }
+    printf("Test Slave ISR Assign CPU1: %d : %ld\n", TEST_ISR_CNT, slave_expect);
+    TEST_ASSERT_EQUAL_UINT32(TEST_ISR_CNT, slave_expect);
+}
+
+#endif

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

@@ -78,7 +78,7 @@ The SPI master driver governs communications of Hosts with Devices. The driver s
 
     The SPI master driver has the concept of multiple Devices connected to a single bus (sharing a single {IDF_TARGET_NAME} SPI peripheral). As long as each Device is accessed by only one task, the driver is thread safe. However, if multiple tasks try to access the same SPI Device, the driver is **not thread-safe**. In this case, it is recommended to either:
 
-    - Refactor your application so that each SPI peripheral is only accessed by a single task at a time.
+    - Refactor your application so that each SPI peripheral is only accessed by a single task at a time. You can use :cpp:member:`spi_bus_config_t::isr_cpu_id` to register the SPI ISR to the same core as SPI peripheral related tasks to ensure thread safety.
     - Add a mutex lock around the shared Device using :c:macro:`xSemaphoreCreateMutex`.
 
 .. toctree::

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

@@ -61,6 +61,7 @@ Driver Features
 
 The SPI slave driver allows using the SPI peripherals as full-duplex Devices. The driver can send/receive transactions up to {IDF_TARGET_MAX_DATA_BUF} bytes in length, or utilize DMA to send/receive longer transactions. However, there are some :ref:`known issues <spi_dma_known_issues>` related to DMA.
 
+The SPI slave driver supports registering the SPI ISR to a certain CPU core. If multiple tasks try to access the same SPI Device, it is recommended to refactor your application so that each SPI peripheral is only accessed by a single task at a time. and use :cpp:member:`spi_bus_config_t::isr_cpu_id` to register the SPI ISR to the same core as SPI peripheral related tasks to ensure thread safe.
 
 SPI Transactions
 ----------------
@@ -79,7 +80,7 @@ As not every transaction requires both writing and reading data, you have a choi
 Driver Usage
 ------------
 
-- Initialize an SPI peripheral as a Device by calling the function cpp:func:`spi_slave_initialize`. Make sure to set the correct I/O pins in the struct `bus_config`. Set the unused signals to ``-1``.
+- Initialize an SPI peripheral as a Device by calling the function :cpp:func:`spi_slave_initialize`. Make sure to set the correct I/O pins in the struct `bus_config`. Set the unused signals to ``-1``.
 
 .. only:: esp32