From fdf983e0c49601479a83c82ca021b3e15deacbdd Mon Sep 17 00:00:00 2001 From: michael Date: Thu, 9 Apr 2020 13:30:12 +0800 Subject: [PATCH] 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 49a48644e42458366b2dd7b7d153acc943d50e0f. Closes https://github.com/espressif/esp-idf/issues/5046 --- .../include/driver/spi_common_internal.h | 13 ++- components/driver/spi_bus_lock.c | 24 +++-- components/driver/spi_master.c | 3 +- components/driver/test/test_spi_bus_lock.c | 3 + components/freertos/Kconfig | 2 +- components/spi_flash/Kconfig | 16 ++++ components/spi_flash/esp_flash_api.c | 2 +- components/spi_flash/esp_flash_spi_init.c | 15 ++- .../spi_flash/include/esp_flash_internal.h | 14 ++- components/spi_flash/spi_flash_os_func_app.c | 91 ++++++++++--------- .../hd_eeprom/main/Kconfig.projbuild | 4 +- .../hd_eeprom/main/spi_eeprom_main.c | 6 +- tools/unit-test-app/sdkconfig.defaults.esp32 | 1 + 13 files changed, 133 insertions(+), 61 deletions(-) diff --git a/components/driver/include/driver/spi_common_internal.h b/components/driver/include/driver/spi_common_internal.h index 52fc78642..48b00166e 100644 --- a/components/driver/include/driver/spi_common_internal.h +++ b/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. * diff --git a/components/driver/spi_bus_lock.c b/components/driver/spi_bus_lock.c index b50b1ec02..8c63c9b68 100644 --- a/components/driver/spi_bus_lock.c +++ b/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; -} \ No newline at end of file +} +#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 diff --git a/components/driver/spi_master.c b/components/driver/spi_master.c index 5b8c73b42..981d8b73d 100644 --- a/components/driver/spi_master.c +++ b/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; diff --git a/components/driver/test/test_spi_bus_lock.c b/components/driver/test/test_spi_bus_lock.c index acab33bcf..0f13af1d9 100644 --- a/components/driver/test/test_spi_bus_lock.c +++ b/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) diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index e94690fd5..a2cdb4867 100644 --- a/components/freertos/Kconfig +++ b/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 diff --git a/components/spi_flash/Kconfig b/components/spi_flash/Kconfig index f72161172..7135d0767 100644 --- a/components/spi_flash/Kconfig +++ b/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 diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c index 7073ab62e..a5c04a47b 100644 --- a/components/spi_flash/esp_flash_api.c +++ b/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 diff --git a/components/spi_flash/esp_flash_spi_init.c b/components/spi_flash/esp_flash_spi_init.c index 9713a24ae..d72a38866 100644 --- a/components/spi_flash/esp_flash_spi_init.c +++ b/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 diff --git a/components/spi_flash/include/esp_flash_internal.h b/components/spi_flash/include/esp_flash_internal.h index 379e90eb6..1ef3ed1ea 100644 --- a/components/spi_flash/include/esp_flash_internal.h +++ b/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) diff --git a/components/spi_flash/spi_flash_os_func_app.c b/components/spi_flash/spi_flash_os_func_app.c index cb1dab534..07b8778aa 100644 --- a/components/spi_flash/spi_flash_os_func_app.c +++ b/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; } - - - diff --git a/examples/peripherals/spi_master/hd_eeprom/main/Kconfig.projbuild b/examples/peripherals/spi_master/hd_eeprom/main/Kconfig.projbuild index 3dcb4a701..e48344e2f 100644 --- a/examples/peripherals/spi_master/hd_eeprom/main/Kconfig.projbuild +++ b/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. diff --git a/examples/peripherals/spi_master/hd_eeprom/main/spi_eeprom_main.c b/examples/peripherals/spi_master/hd_eeprom/main/spi_eeprom_main.c index 3130c27c3..06e5282df 100644 --- a/examples/peripherals/spi_master/hd_eeprom/main/spi_eeprom_main.c +++ b/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); diff --git a/tools/unit-test-app/sdkconfig.defaults.esp32 b/tools/unit-test-app/sdkconfig.defaults.esp32 index 1087b70b0..423406bb2 100644 --- a/tools/unit-test-app/sdkconfig.defaults.esp32 +++ b/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