spi: fix config break and reduce overhead of the bus lock on SPI1

The SPI bus lock on SPI1 introduces two side effects:

1. The device lock for the main flash requires the
`CONFIG_FREERTOS_SUPPORT_STATIC_ALLOCATION` to be selected, however this
option is disabled by default in earlier IDF versions. Some developers
may find their project cannot be built by their old sdkconfig files.

2. Usually we don't need the lock on the SPI1 bus, due to it's
restrictions. However the overhead still exists in this case, the IRAM
cost for static version of semaphore functions, and the time cost when
getting and releasing the lock.

This commit:

1. Add a CONFIG_SPI_FLASH_BYPASS_MAIN_LOCK option, which will forbid the
space cost, as well as the initialization of the main bus lock.

2. When the option is not selected, the bus lock is used, the
`CONFIG_FREERTOS_SUPPORT_STATIC_ALLOCATION` will be selected explicitly.

3. Revert default value of `CONFIG_FREERTOS_SUPPORT_STATIC_ALLOCATION`
to `n`.

introduced in 49a48644e4.

Closes https://github.com/espressif/esp-idf/issues/5046

components/driver/include/driver/spi_common_internal.h

@@ -767,12 +767,19 @@ bool spi_bus_lock_bg_req_exist(spi_bus_lock_handle_t lock);
 /*******************************************************************************
  * Variable and APIs for the OS to initialize the locks for the main chip
  ******************************************************************************/
-/// The lock for the main flash device
-extern const spi_bus_lock_dev_handle_t g_spi_lock_main_flash_dev;
-
 /// The lock for the main bus
 extern const spi_bus_lock_handle_t g_main_spi_bus_lock;
 
+/**
+ * @brief Initialize the main SPI bus, called during chip startup.
+ *
+ * @return always ESP_OK
+ */
+esp_err_t spi_bus_lock_init_main_bus(void);
+
+/// The lock for the main flash device
+extern const spi_bus_lock_dev_handle_t g_spi_lock_main_flash_dev;
+
 /**
  * @brief Initialize the main flash device, called during chip startup.
  *

components/driver/spi_bus_lock.c

@@ -590,6 +590,7 @@ static int try_acquire_free_dev(spi_bus_lock_t *lock, bool cs_required)
 esp_err_t spi_bus_lock_register_dev(spi_bus_lock_handle_t lock, spi_bus_lock_dev_config_t *config,
                                     spi_bus_lock_dev_handle_t *out_dev_handle)
 {
+    if (lock == NULL) return ESP_ERR_INVALID_ARG;
     int id = try_acquire_free_dev(lock, config->flags & SPI_BUS_LOCK_DEV_FLAG_CS_REQUIRED);
     if (id == -1) return ESP_ERR_NOT_SUPPORTED;
 
@@ -792,7 +793,7 @@ SPI_MASTER_ISR_ATTR bool spi_bus_lock_bg_req_exist(spi_bus_lock_t *lock)
 /*******************************************************************************
  * Static variables of the locks of the main flash
  ******************************************************************************/
-static StaticSemaphore_t main_flash_semphr;
+#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
 static spi_bus_lock_dev_t lock_main_flash_dev;
 
 static spi_bus_lock_t main_spi_bus_lock = {
@@ -806,23 +807,34 @@ static spi_bus_lock_t main_spi_bus_lock = {
     .new_req = 0,
     .periph_cs_num = SOC_SPI_PERIPH_CS_NUM(0),
 };
+const spi_bus_lock_handle_t g_main_spi_bus_lock = &main_spi_bus_lock;
+
+esp_err_t spi_bus_lock_init_main_bus(void)
+{
+    spi_bus_main_set_lock(g_main_spi_bus_lock);
+    return ESP_OK;
+}
+
+static StaticSemaphore_t main_flash_semphr;
 
 static spi_bus_lock_dev_t lock_main_flash_dev = {
     .semphr = NULL,
     .parent = &main_spi_bus_lock,
     .mask = DEV_MASK(0),
 };
-
-const spi_bus_lock_handle_t g_main_spi_bus_lock = &main_spi_bus_lock;
 const spi_bus_lock_dev_handle_t g_spi_lock_main_flash_dev = &lock_main_flash_dev;
 
 esp_err_t spi_bus_lock_init_main_dev(void)
 {
-    spi_bus_main_set_lock(g_main_spi_bus_lock);
     g_spi_lock_main_flash_dev->semphr = xSemaphoreCreateBinaryStatic(&main_flash_semphr);
     if (g_spi_lock_main_flash_dev->semphr == NULL) {
         return ESP_ERR_NO_MEM;
     }
-
     return ESP_OK;
-}
+}
+#else //CONFIG_SPI_FLASH_SHARE_SPI1_BUS
+
+//when the dev lock is not initialized, point to NULL
+const spi_bus_lock_dev_handle_t g_spi_lock_main_flash_dev = NULL;
+
+#endif

components/driver/spi_master.c

@@ -204,6 +204,7 @@ static esp_err_t spi_master_init_driver(spi_host_device_t host_id)
 
     const spi_bus_attr_t* bus_attr = spi_bus_get_attr(host_id);
     SPI_CHECK(bus_attr != NULL, "host_id not initialized", ESP_ERR_INVALID_STATE);
+    SPI_CHECK(bus_attr->lock != NULL, "SPI Master cannot attach to bus. (Check CONFIG_SPI_FLASH_SHARE_SPI1_BUS)", ESP_ERR_INVALID_ARG);
     // spihost contains atomic variables, which should not be put in PSRAM
     spi_host_t* host = heap_caps_malloc(sizeof(spi_host_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
     if (host == NULL) {
@@ -307,7 +308,7 @@ esp_err_t spi_bus_add_device(spi_host_device_t host_id, const spi_device_interfa
 
     SPI_CHECK(is_valid_host(host_id), "invalid host", ESP_ERR_INVALID_ARG);
     if (bus_driver_ctx[host_id] == NULL) {
-        //lasy initialization the driver, get deinitialized by the bus is freed
+        //lazy initialization the driver, get deinitialized by the bus is freed
         err = spi_master_init_driver(host_id);
         if (err != ESP_OK) {
             return err;

components/driver/test/test_spi_bus_lock.c

@@ -336,6 +336,9 @@ TEST_CASE("spi master can be used on SPI1", "[spi]")
     err = test_polling_send(handle);
     TEST_ESP_OK(err);
     test_acquire(handle);
+
+    err = spi_bus_remove_device(handle);
+    TEST_ESP_OK(err);
 }
 #endif //!TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2)
 

components/freertos/Kconfig

@@ -204,7 +204,7 @@ menu "FreeRTOS"
 
     config FREERTOS_SUPPORT_STATIC_ALLOCATION
         bool "Enable FreeRTOS static allocation API"
-        default y
+        default n
         help
             FreeRTOS gives the application writer the ability to instead provide the memory
             themselves, allowing the following objects to optionally be created without any

components/spi_flash/Kconfig

@@ -84,6 +84,22 @@ menu "SPI Flash driver"
             The implementation of SPI flash has been greatly changed in IDF v4.0.
             Enable this option to use the legacy implementation.
 
+    config SPI_FLASH_SHARE_SPI1_BUS
+        bool  "Support other devices attached to SPI1 bus"
+        default n
+        # The bus lock on SPI1 is meaningless when the legacy implementation is used, or the SPI
+        # driver does not support SPI1.
+        depends on !SPI_FLASH_USE_LEGACY_IMPL && !IDF_TARGET_ESP32S2
+        select FREERTOS_SUPPORT_STATIC_ALLOCATION
+        help
+            Each SPI bus needs a lock for arbitration among devices. This allows multiple
+            devices on a same bus, but may reduce the speed of esp_flash driver access to the
+            main flash chip.
+
+            If you only need to use esp_flash driver to access the main flash chip, disable
+            this option, and the lock will be bypassed on SPI1 bus. Otherwise if extra devices
+            are needed to attach to SPI1 bus, enable this option.
+
     menu "Auto-detect flash chips"
 
         config SPI_FLASH_SUPPORT_ISSI_CHIP

components/spi_flash/esp_flash_api.c

@@ -687,7 +687,7 @@ esp_err_t esp_flash_app_disable_protect(bool disable)
     if (disable) {
         return esp_flash_app_disable_os_functions(esp_flash_default_chip);
     } else {
-        return esp_flash_app_init_os_functions(esp_flash_default_chip);
+        return esp_flash_app_enable_os_functions(esp_flash_default_chip);
     }
 }
 #endif

components/spi_flash/esp_flash_spi_init.c

@@ -147,6 +147,11 @@ esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_d
 
     int dev_id;
     esp_err_t err = esp_flash_init_os_functions(chip, config->host_id, &dev_id);
+    if (err == ESP_ERR_NOT_SUPPORTED) {
+        ESP_LOGE(TAG, "Init os functions failed! No free CS.");
+    } else if (err == ESP_ERR_INVALID_ARG) {
+        ESP_LOGE(TAG, "Init os functions failed! Bus lock not initialized (check CONFIG_SPI_FLASH_SHARE_SPI1_BUS).");
+    }
     if (err != ESP_OK) {
         ret = err;
         goto fail;
@@ -241,7 +246,13 @@ esp_err_t esp_flash_init_default_chip(void)
 
 esp_err_t esp_flash_app_init(void)
 {
-    return esp_flash_app_init_os_functions(&default_chip);
+    esp_err_t err = ESP_OK;
+#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
+    err = esp_flash_init_main_bus_lock();
+    if (err != ESP_OK) return err;
+#endif
+    err = esp_flash_app_enable_os_functions(&default_chip);
+    return err;
 }
 
-#endif
+#endif //!CONFIG_SPI_FLASH_USE_LEGACY_IMPL

components/spi_flash/include/esp_flash_internal.h

@@ -52,7 +52,7 @@ esp_err_t esp_flash_app_init(void);
 #endif
 
 /**
- *  Disable OS-level SPI flash protections in IDF
+ *  Disable (or enable) OS-level SPI flash protections in IDF
  *
  *  Called by the IDF internal code (e.g. coredump). You do not need to call this in your own applications.
  *
@@ -85,6 +85,16 @@ esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int *out_d
  */
 esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip);
 
+/**
+ * @brief Initialize the bus lock on the SPI1 bus. Should be called if drivers (including esp_flash)
+ * wants to use SPI1 bus.
+ *
+ * @note When using legacy spi flash API, the bus lock will not be available on SPI1 bus.
+ *
+ * @return esp_err_t always ESP_OK.
+ */
+esp_err_t esp_flash_init_main_bus_lock(void);
+
 /**
  *  Initialize OS-level functions for the main flash chip.
  *
@@ -92,7 +102,7 @@ esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip);
  *
  * @return always ESP_OK
  */
-esp_err_t esp_flash_app_init_os_functions(esp_flash_t* chip);
+esp_err_t esp_flash_app_enable_os_functions(esp_flash_t* chip);
 
 /**
  *  Disable OS-level functions for the main flash chip during special phases (e.g. coredump)

components/spi_flash/spi_flash_os_func_app.c

@@ -18,7 +18,7 @@
 #include "esp_flash.h"
 #include "esp_flash_partitions.h"
 #include "hal/spi_types.h"
-
+#include "sdkconfig.h"
 
 #if CONFIG_IDF_TARGET_ESP32
 #include "esp32/rom/ets_sys.h"
@@ -45,38 +45,56 @@ typedef struct {
     bool no_protect;    //to decide whether to check protected region (for the main chip) or not.
 } spi1_app_func_arg_t;
 
-
-// in the future we will have arbitration among devices, including flash on the same flash bus
-static IRAM_ATTR esp_err_t spi_bus_acquire(spi_bus_lock_dev_handle_t dev_lock)
+IRAM_ATTR static void cache_enable(void* arg)
 {
-    // was in BG operation (cache). Disable it and schedule
+    g_flash_guard_default_ops.end();
+}
+
+IRAM_ATTR static void cache_disable(void* arg)
+{
+    g_flash_guard_default_ops.start();
+}
+
+static IRAM_ATTR esp_err_t spi_start(void *arg)
+{
+    spi_bus_lock_dev_handle_t dev_lock = ((app_func_arg_t *)arg)->dev_lock;
+
+    // wait for other devices (or cache) to finish their operation
     esp_err_t ret = spi_bus_lock_acquire_start(dev_lock, portMAX_DELAY);
     if (ret != ESP_OK) {
         return ret;
     }
-    return ESP_OK;
-}
-
-static IRAM_ATTR esp_err_t spi_bus_release(spi_bus_lock_dev_handle_t dev_lock)
-{
-    return spi_bus_lock_acquire_end(dev_lock);
-}
-
-//for SPI1, we have to disable the cache and interrupts before using the SPI bus
-static IRAM_ATTR esp_err_t spi_start(void *arg)
-{
-    spi_bus_lock_dev_handle_t dev_lock = ((app_func_arg_t *)arg)->dev_lock;
-    spi_bus_acquire(dev_lock);
     spi_bus_lock_touch(dev_lock);
     return ESP_OK;
 }
 
 static IRAM_ATTR esp_err_t spi_end(void *arg)
 {
-    spi_bus_release(((app_func_arg_t *)arg)->dev_lock);
+    return spi_bus_lock_acquire_end(((app_func_arg_t *)arg)->dev_lock);
+}
+
+static IRAM_ATTR esp_err_t spi1_start(void *arg)
+{
+#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
+    //use the lock to disable the cache and interrupts before using the SPI bus
+    return spi_start(arg);
+#else
+    //directly disable the cache and interrupts when lock is not used
+    cache_disable(NULL);
+#endif
     return ESP_OK;
 }
 
+static IRAM_ATTR esp_err_t spi1_end(void *arg)
+{
+#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
+    return spi_end(arg);
+#else
+    cache_enable(NULL);
+    return ESP_OK;
+#endif
+}
+
 static IRAM_ATTR esp_err_t delay_ms(void *arg, unsigned ms)
 {
     ets_delay_us(1000 * ms);
@@ -96,14 +114,14 @@ static IRAM_ATTR esp_err_t main_flash_region_protected(void* arg, size_t start_a
 static DRAM_ATTR spi1_app_func_arg_t main_flash_arg = {};
 
 //for SPI1, we have to disable the cache and interrupts before using the SPI bus
-const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functions = {
-    .start = spi_start,
-    .end = spi_end,
+static const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functions = {
+    .start = spi1_start,
+    .end = spi1_end,
     .delay_ms = delay_ms,
     .region_protected = main_flash_region_protected,
 };
 
-const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = {
+static const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = {
     .start = spi_start,
     .end = spi_end,
     .delay_ms = delay_ms,
@@ -164,36 +182,27 @@ esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip)
     return ESP_OK;
 }
 
-IRAM_ATTR static void cache_enable(void* arg)
+esp_err_t esp_flash_init_main_bus_lock(void)
 {
-    g_flash_guard_default_ops.end();
-}
+    spi_bus_lock_init_main_bus();
+    spi_bus_lock_set_bg_control(g_main_spi_bus_lock, cache_enable, cache_disable, NULL);
 
-IRAM_ATTR static void cache_disable(void* arg)
-{
-    g_flash_guard_default_ops.start();
-}
-
-esp_err_t esp_flash_app_init_os_functions(esp_flash_t* chip)
-{
     esp_err_t err = spi_bus_lock_init_main_dev();
     if (err != ESP_OK) {
         return err;
     }
+    return ESP_OK;
+}
 
-    spi_bus_lock_set_bg_control(g_main_spi_bus_lock,
-        cache_enable, cache_disable, NULL);
-
-    chip->os_func = &esp_flash_spi1_default_os_functions;
-    chip->os_func_data = &main_flash_arg;
+esp_err_t esp_flash_app_enable_os_functions(esp_flash_t* chip)
+{
     main_flash_arg = (spi1_app_func_arg_t) {
         .common_arg = {
             .dev_lock = g_spi_lock_main_flash_dev,   //for SPI1,
         },
         .no_protect = false,
     };
+    chip->os_func = &esp_flash_spi1_default_os_functions;
+    chip->os_func_data = &main_flash_arg;
     return ESP_OK;
 }
-
-
-

examples/peripherals/spi_master/hd_eeprom/main/Kconfig.projbuild

@@ -1,9 +1,9 @@
 menu "Example Configuration"
 
     config EXAMPLE_USE_SPI1_PINS
-        bool "The example runs on SPI1 pins or some other pins"
+        bool "The example uses SPI1 shared pins for EEPROM connection"
         default n
-        depends on IDF_TARGET_ESP32
+        depends on SPI_FLASH_SHARE_SPI1_BUS
         help
             Enable this option will make the EEPROM use SPI1 pins, which is shared with the main
             flash chip.

examples/peripherals/spi_master/hd_eeprom/main/spi_eeprom_main.c

@@ -85,9 +85,11 @@ void app_main(void)
     eeprom_handle_t eeprom_handle;
 
     ESP_LOGI(TAG, "Initializing device...");
-    spi_eeprom_init(&eeprom_config, &eeprom_handle);
+    ret = spi_eeprom_init(&eeprom_config, &eeprom_handle);
+    ESP_ERROR_CHECK(ret);
 
-    spi_eeprom_write_enable(eeprom_handle);
+    ret = spi_eeprom_write_enable(eeprom_handle);
+    ESP_ERROR_CHECK(ret);
 
     const char test_str[] = "Hello World!";
     ESP_LOGI(TAG, "Write: %s", test_str);

tools/unit-test-app/sdkconfig.defaults.esp32

@@ -1,3 +1,4 @@
 CONFIG_ESP32_DEFAULT_CPU_FREQ_240=y
 CONFIG_ESP32_XTAL_FREQ_AUTO=y
 CONFIG_ESP32_ULP_COPROC_ENABLED=y
+CONFIG_SPI_FLASH_SHARE_SPI1_BUS=y