Merge branch 'fix/spi_callback_in_iram' into 'master'

spi: fix the crash when callbacks are not in the IRAM

See merge request idf/esp-idf!3498

components/driver/Kconfig

@@ -41,8 +41,10 @@ config SPI_MASTER_ISR_IN_IRAM
     bool "Place SPI master ISR function into IRAM"
     default y
     help
-        Place the SPI master ISR in to IRAM to avoid possibly cache miss, or
-        being disabled during flash writing access.
+        Place the SPI master ISR in to IRAM to avoid possible cache miss.
+
+        Also you can forbid the ISR being disabled during flash writing
+        access, by add ESP_INTR_FLAG_IRAM when initializing the driver.
 
 config SPI_SLAVE_IN_IRAM
     bool "Place transmitting functions of SPI slave into IRAM"
@@ -61,8 +63,10 @@ config SPI_SLAVE_ISR_IN_IRAM
     bool "Place SPI slave ISR function into IRAM"
     default y
     help
-        Place the SPI slave ISR in to IRAM to avoid possibly cache miss, or
-        being disabled during flash writing access.
+        Place the SPI slave ISR in to IRAM to avoid possible cache miss.
+
+        Also you can forbid the ISR being disabled during flash writing
+        access, by add ESP_INTR_FLAG_IRAM when initializing the driver.
 
 endmenu # SPI Configuration
 

components/driver/include/driver/spi_common.h

@@ -87,6 +87,11 @@ typedef struct {
     int quadhd_io_num;              ///< GPIO pin for HD (HolD) signal which is used as D3 in 4-bit communication modes, or -1 if not used.
     int max_transfer_sz;            ///< Maximum transfer size, in bytes. Defaults to 4094 if 0.
     uint32_t flags;                 ///< Abilities of bus to be checked by the driver. Or-ed value of ``SPICOMMON_BUSFLAG_*`` flags.
+    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
+                         *  the driver, and their callee functions, should be put in the IRAM.
+                         */
 } spi_bus_config_t;
 
 

components/driver/include/driver/spi_master.h

@@ -79,8 +79,26 @@ typedef struct {
     int spics_io_num;               ///< CS GPIO pin for this device, or -1 if not used
     uint32_t flags;                 ///< Bitwise OR of SPI_DEVICE_* flags
     int queue_size;                 ///< Transaction queue size. This sets how many transactions can be 'in the air' (queued using spi_device_queue_trans but not yet finished using spi_device_get_trans_result) at the same time
-    transaction_cb_t pre_cb;        ///< Callback to be called before a transmission is started. This callback is called within interrupt context.
-    transaction_cb_t post_cb;       ///< Callback to be called after a transmission has completed. This callback is called within interrupt context.
+    transaction_cb_t pre_cb;   /**< Callback to be called before a transmission is started.
+                                 *
+                                 *  This callback is called within interrupt
+                                 *  context should be in IRAM for best
+                                 *  performance, see "Transferring Speed"
+                                 *  section in the SPI Master documentation for
+                                 *  full details. If not, the callback may crash
+                                 *  during flash operation when the driver is
+                                 *  initialized with ESP_INTR_FLAG_IRAM.
+                                 */
+    transaction_cb_t post_cb;  /**< Callback to be called after a transmission has completed.
+                                 *
+                                 *  This callback is called within interrupt
+                                 *  context should be in IRAM for best
+                                 *  performance, see "Transferring Speed"
+                                 *  section in the SPI Master documentation for
+                                 *  full details. If not, the callback may crash
+                                 *  during flash operation when the driver is
+                                 *  initialized with ESP_INTR_FLAG_IRAM.
+                                 */
 } spi_device_interface_config_t;
 
 

components/driver/include/driver/spi_slave.h

@@ -44,8 +44,26 @@ typedef struct {
     uint32_t flags;                 ///< Bitwise OR of SPI_SLAVE_* flags
     int queue_size;                 ///< Transaction queue size. This sets how many transactions can be 'in the air' (queued using spi_slave_queue_trans but not yet finished using spi_slave_get_trans_result) at the same time
     uint8_t mode;                   ///< SPI mode (0-3)
-    slave_transaction_cb_t post_setup_cb; ///< Callback called after the SPI registers are loaded with new data
-    slave_transaction_cb_t post_trans_cb; ///< Callback called after a transaction is done
+    slave_transaction_cb_t post_setup_cb;  /**< Callback called after the SPI registers are loaded with new data.
+                                             *
+                                             *  This callback is called within interrupt
+                                             *  context should be in IRAM for best
+                                             *  performance, see "Transferring Speed"
+                                             *  section in the SPI Master documentation for
+                                             *  full details. If not, the callback may crash
+                                             *  during flash operation when the driver is
+                                             *  initialized with ESP_INTR_FLAG_IRAM.
+                                             */
+    slave_transaction_cb_t post_trans_cb;  /**< Callback called after a transaction is done.
+                                             *
+                                             *  This callback is called within interrupt
+                                             *  context should be in IRAM for best
+                                             *  performance, see "Transferring Speed"
+                                             *  section in the SPI Master documentation for
+                                             *  full details. If not, the callback may crash
+                                             *  during flash operation when the driver is
+                                             *  initialized with ESP_INTR_FLAG_IRAM.
+                                             */
 } spi_slave_interface_config_t;
 
 /**

components/driver/spi_master.c

@@ -233,6 +233,10 @@ esp_err_t spi_bus_initialize(spi_host_device_t host, const spi_bus_config_t *bus
 
     SPI_CHECK(host>=SPI_HOST && host<=VSPI_HOST, "invalid host", ESP_ERR_INVALID_ARG);
     SPI_CHECK( dma_chan >= 0 && dma_chan <= 2, "invalid dma channel", ESP_ERR_INVALID_ARG );
+    SPI_CHECK((bus_config->intr_flags & (ESP_INTR_FLAG_HIGH|ESP_INTR_FLAG_EDGE|ESP_INTR_FLAG_INTRDISABLED))==0, "intr flag not allowed", ESP_ERR_INVALID_ARG);
+#ifndef CONFIG_SPI_MASTER_ISR_IN_IRAM
+    SPI_CHECK((bus_config->intr_flags & ESP_INTR_FLAG_IRAM)==0, "ESP_INTR_FLAG_IRAM should be disabled when CONFIG_SPI_MASTER_ISR_IN_IRAM is not set.", ESP_ERR_INVALID_ARG);
+#endif
 
     spi_chan_claimed=spicommon_periph_claim(host, "spi master");
     SPI_CHECK(spi_chan_claimed, "host already in use", ESP_ERR_INVALID_STATE);
@@ -284,10 +288,7 @@ esp_err_t spi_bus_initialize(spi_host_device_t host, const spi_bus_config_t *bus
         }
     }
 
-    int flags = ESP_INTR_FLAG_INTRDISABLED;
-#ifdef CONFIG_SPI_MASTER_ISR_IN_IRAM
-    flags |= ESP_INTR_FLAG_IRAM;
-#endif
+    int flags = bus_config->intr_flags | ESP_INTR_FLAG_INTRDISABLED;
     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/spi_slave.c

@@ -109,6 +109,10 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b
     //We only support HSPI/VSPI, period.
     SPI_CHECK(VALID_HOST(host), "invalid host", ESP_ERR_INVALID_ARG);
     SPI_CHECK( dma_chan >= 0 && dma_chan <= 2, "invalid dma channel", ESP_ERR_INVALID_ARG );
+    SPI_CHECK((bus_config->intr_flags & (ESP_INTR_FLAG_HIGH|ESP_INTR_FLAG_EDGE|ESP_INTR_FLAG_INTRDISABLED))==0, "intr flag not allowed", ESP_ERR_INVALID_ARG);
+#ifndef CONFIG_SPI_SLAVE_ISR_IN_IRAM
+    SPI_CHECK((bus_config->intr_flags & ESP_INTR_FLAG_IRAM)==0, "ESP_INTR_FLAG_IRAM should be disabled when CONFIG_SPI_SLAVE_ISR_IN_IRAM is not set.", ESP_ERR_INVALID_ARG);
+#endif
 
     spi_chan_claimed=spicommon_periph_claim(host, "spi slave");
     SPI_CHECK(spi_chan_claimed, "host already in use", ESP_ERR_INVALID_STATE);
@@ -174,10 +178,7 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b
         goto cleanup;
     }
 
-    int flags = ESP_INTR_FLAG_INTRDISABLED;
-#ifdef CONFIG_SPI_SLAVE_ISR_IN_IRAM
-    flags |= ESP_INTR_FLAG_IRAM;
-#endif
+    int flags = bus_config->intr_flags | ESP_INTR_FLAG_INTRDISABLED;
     err = esp_intr_alloc(spicommon_irqsource_for_host(host), flags, spi_intr, (void *)spihost[host], &spihost[host]->intr);
     if (err != ESP_OK) {
         ret = err;

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

@@ -305,7 +305,8 @@ Speed and Timing Considerations
 Transferring speed
 ^^^^^^^^^^^^^^^^^^
 
-There're two factors limiting the transferring speed: (1) The transaction interval, (2) The SPI clock frequency used.
+There're three factors limiting the transferring speed: (1) The transaction interval, (2) The SPI clock frequency used.
+(3) The cache miss of SPI functions including callbacks.
 When large transactions are used, the clock frequency determines the transferring speed; while the interval effects the
 speed a lot if small transactions are used.
 
@@ -343,6 +344,13 @@ speed a lot if small transactions are used.
     2. SPI clock frequency: Each byte transferred takes 8 times of the clock period *8/fspi*. If the clock frequency is
        too high, some functions may be limited to use. See :ref:`timing_considerations`.
 
+    3. The cache miss: the default config puts only the ISR into the IRAM.
+       Other SPI related functions including the driver itself and the callback
+       may suffer from the cache miss and wait for some time while reading code
+       from the 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 this.
+
 For an interrupt transaction, the overall cost is *20+8n/Fspi[MHz]* [us] for n bytes tranferred
 in one transaction. Hence the transferring speed is : *n/(20+8n/Fspi)*. Example of transferring speed under 8MHz
 clock speed:
@@ -366,6 +374,15 @@ clock speed:
 When the length of transaction is short, the cost of transaction interval is really high. Please try to squash data
 into one transaction if possible to get higher transfer speed.
 
+BTW, the ISR is disabled during flash operation by default. To keep sending
+transactions during flash operations, enable
+:ref:`CONFIG_SPI_MASTER_ISR_IN_IRAM` and set :cpp:class:`ESP_INTR_FLAG_IRAM`
+in the ``intr_flags`` member of :cpp:class:`spi_bus_config_t`. Then all the
+transactions queued before the flash operations will be handled by the ISR
+continuously during flash operation. Note that the callback of each devices,
+and their callee functions, should be in the IRAM in this case, or your
+callback will crash due to cache miss.
+
 .. _timing_considerations:
 
 Timing considerations