Merge branch 'feature/ext_flash_yield_on_erase' into 'master'

Yield functionality for external flash, mask esp_flash_erase_chip with esp_flash_erase_sector

Closes IDFGH-7665, IDFGH-8487, and IDFGH-6479

See merge request espressif/esp-idf!19205

components/spi_flash/esp_flash_api.c

@@ -512,31 +512,15 @@ esp_err_t IRAM_ATTR esp_flash_get_size(esp_flash_t *chip, uint32_t *out_size)
 
 esp_err_t IRAM_ATTR esp_flash_erase_chip(esp_flash_t *chip)
 {
-    esp_err_t err = rom_spiflash_api_funcs->chip_check(&chip);
-    VERIFY_CHIP_OP(erase_chip);
-    CHECK_WRITE_ADDRESS(chip, 0, chip->size);
-
-    //check before the operation, in case this is called too close to the last operation
-    if (chip->chip_drv->yield) {
-        err = chip->chip_drv->yield(chip, 0);
-        if (err != ESP_OK) {
-            return err;
-        }
-    }
-
-    err = rom_spiflash_api_funcs->start(chip);
+    esp_err_t err = ESP_OK;
+    uint32_t size = 0;
+    err = esp_flash_get_size(chip, &size);
     if (err != ESP_OK) {
+        ESP_LOGE(TAG, "esp_flash_get_size failed, flash error code: %d", err);
         return err;
     }
-
-    err = chip->chip_drv->erase_chip(chip);
-    if (chip->host->driver->flush_cache) {
-        esp_err_t flush_cache_err = chip->host->driver->flush_cache(chip->host, 0, chip->size);
-        if (err == ESP_OK) {
-            err = flush_cache_err;
-        }
-    }
-    return rom_spiflash_api_funcs->end(chip, err);
+    err = esp_flash_erase_region(chip, 0, size);
+    return err;
 }
 
 esp_err_t IRAM_ATTR esp_flash_erase_region(esp_flash_t *chip, uint32_t start, uint32_t len)

components/spi_flash/spi_flash_os_func_app.c

@@ -32,10 +32,6 @@ static const char TAG[] = "spi_flash";
  * into the IRAM,and their data should be put into the DRAM.
  */
 
-typedef struct {
-    spi_bus_lock_dev_handle_t dev_lock;
-} app_func_arg_t;
-
 /*
  * Time yield algorithm:
  * Every time spi_flash_os_check_yield() is called:
@@ -48,16 +44,16 @@ typedef struct {
  *       counter mentioned in #2, but still be counted into the time mentioned in #2.
  */
 typedef struct {
-    app_func_arg_t common_arg; //shared args, must be the first item
+    spi_bus_lock_dev_handle_t dev_lock;
     bool no_protect;    //to decide whether to check protected region (for the main chip) or not.
     uint32_t acquired_since_us;    // Time since last explicit yield()
     uint32_t released_since_us;    // Time since last end() (implicit yield)
-} spi1_app_func_arg_t;
+} app_func_arg_t;
 
-static inline void on_spi1_released(spi1_app_func_arg_t* ctx);
-static inline void on_spi1_acquired(spi1_app_func_arg_t* ctx);
-static inline void on_spi1_yielded(spi1_app_func_arg_t* ctx);
-static inline bool on_spi1_check_yield(spi1_app_func_arg_t* ctx);
+static inline void on_spi_released(app_func_arg_t* ctx);
+static inline void on_spi_acquired(app_func_arg_t* ctx);
+static inline void on_spi_yielded(app_func_arg_t* ctx);
+static inline bool on_spi_check_yield(app_func_arg_t* ctx);
 
 IRAM_ATTR static void cache_enable(void* arg)
 {
@@ -91,6 +87,18 @@ static IRAM_ATTR esp_err_t spi_end(void *arg)
     return spi_bus_lock_acquire_end(((app_func_arg_t *)arg)->dev_lock);
 }
 
+static IRAM_ATTR esp_err_t spi23_start(void *arg){
+    esp_err_t ret = spi_start(arg);
+    on_spi_acquired((app_func_arg_t*)arg);
+    return ret;
+}
+
+static IRAM_ATTR esp_err_t spi23_end(void *arg){
+    esp_err_t ret = spi_end(arg);
+    on_spi_released((app_func_arg_t*)arg);
+    return ret;
+}
+
 static IRAM_ATTR esp_err_t spi1_start(void *arg)
 {
 #if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
@@ -99,7 +107,7 @@ static IRAM_ATTR esp_err_t spi1_start(void *arg)
 #else
     //directly disable the cache and interrupts when lock is not used
     cache_disable(NULL);
-    on_spi1_acquired((spi1_app_func_arg_t*)arg);
+    on_spi_acquired((app_func_arg_t*)arg);
     return ESP_OK;
 #endif
 }
@@ -112,17 +120,17 @@ static IRAM_ATTR esp_err_t spi1_end(void *arg)
 #else
     cache_enable(NULL);
 #endif
-    on_spi1_released((spi1_app_func_arg_t*)arg);
+    on_spi_released((app_func_arg_t*)arg);
     return ret;
 }
 
-static IRAM_ATTR esp_err_t spi1_flash_os_check_yield(void *arg, uint32_t chip_status, uint32_t* out_request)
+static IRAM_ATTR esp_err_t spi_flash_os_check_yield(void *arg, uint32_t chip_status, uint32_t* out_request)
 {
     assert (chip_status == 0);  //TODO: support suspend
     esp_err_t ret = ESP_ERR_TIMEOUT;    //Nothing happened
     uint32_t request = 0;
 
-    if (on_spi1_check_yield((spi1_app_func_arg_t *)arg)) {
+    if (on_spi_check_yield((app_func_arg_t *)arg)) {
         request = SPI_FLASH_YIELD_REQ_YIELD;
         ret = ESP_OK;
     }
@@ -132,7 +140,7 @@ static IRAM_ATTR esp_err_t spi1_flash_os_check_yield(void *arg, uint32_t chip_st
     return ret;
 }
 
-static IRAM_ATTR esp_err_t spi1_flash_os_yield(void *arg, uint32_t* out_status)
+static IRAM_ATTR esp_err_t spi_flash_os_yield(void *arg, uint32_t* out_status)
 {
     if (likely(xTaskGetSchedulerState() == taskSCHEDULER_RUNNING)) {
 #ifdef CONFIG_SPI_FLASH_ERASE_YIELD_TICKS
@@ -141,7 +149,7 @@ static IRAM_ATTR esp_err_t spi1_flash_os_yield(void *arg, uint32_t* out_status)
         vTaskDelay(1);
 #endif
     }
-    on_spi1_yielded((spi1_app_func_arg_t*)arg);
+    on_spi_yielded((app_func_arg_t*)arg);
     return ESP_OK;
 }
 
@@ -179,7 +187,7 @@ static IRAM_ATTR void release_buffer_malloc(void* arg, void *temp_buf)
 
 static IRAM_ATTR esp_err_t main_flash_region_protected(void* arg, size_t start_addr, size_t size)
 {
-    if (((spi1_app_func_arg_t*)arg)->no_protect || esp_partition_main_flash_region_safe(start_addr, size)) {
+    if (((app_func_arg_t*)arg)->no_protect || esp_partition_main_flash_region_safe(start_addr, size)) {
         //ESP_OK = 0, also means protected==0
         return ESP_OK;
     } else {
@@ -194,7 +202,7 @@ static IRAM_ATTR void main_flash_op_status(uint32_t op_status)
     spi_flash_set_erasing_flag(is_erasing);
 }
 
-static DRAM_ATTR spi1_app_func_arg_t main_flash_arg = {};
+static DRAM_ATTR app_func_arg_t main_flash_arg = {};
 
 //for SPI1, we have to disable the cache and interrupts before using the SPI bus
 static const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functions = {
@@ -204,8 +212,8 @@ static const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functi
     .delay_us = delay_us,
     .get_temp_buffer = get_buffer_malloc,
     .release_temp_buffer = release_buffer_malloc,
-    .check_yield = spi1_flash_os_check_yield,
-    .yield = spi1_flash_os_yield,
+    .check_yield = spi_flash_os_check_yield,
+    .yield = spi_flash_os_yield,
 #if CONFIG_SPI_FLASH_BROWNOUT_RESET
     .set_flash_op_status = main_flash_op_status,
 #else
@@ -214,14 +222,14 @@ static const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functi
 };
 
 static const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = {
-    .start = spi_start,
-    .end = spi_end,
+    .start = spi23_start,
+    .end = spi23_end,
     .delay_us = delay_us,
     .get_temp_buffer = get_buffer_malloc,
     .release_temp_buffer = release_buffer_malloc,
     .region_protected = NULL,
-    .check_yield = NULL,
-    .yield = NULL,
+    .check_yield = spi_flash_os_check_yield,
+    .yield = spi_flash_os_yield,
     .set_flash_op_status = NULL,
 };
 
@@ -237,28 +245,24 @@ static bool use_bus_lock(int host_id)
 #endif
 }
 
+// This function is only called by users usually via `spi_bus_add_flash_device` to initialise os functions.
+// System will initialise them via `esp_flash_app_enable_os_functions`
 esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, spi_bus_lock_dev_handle_t dev_handle)
 {
     if (use_bus_lock(host_id) && !dev_handle) {
         return ESP_ERR_INVALID_ARG;
     }
 
-    switch (host_id)
-    {
+    chip->os_func_data = heap_caps_malloc(sizeof(app_func_arg_t),
+                                     MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
+    if (chip->os_func_data == NULL) {
+        return ESP_ERR_NO_MEM;
+    }
+
+    switch (host_id) {
         case SPI1_HOST:
             //SPI1
             chip->os_func = &esp_flash_spi1_default_os_functions;
-            chip->os_func_data = heap_caps_malloc(sizeof(spi1_app_func_arg_t),
-                                            MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
-            if (chip->os_func_data == NULL) {
-                return ESP_ERR_NO_MEM;
-            }
-            *(spi1_app_func_arg_t*) chip->os_func_data = (spi1_app_func_arg_t) {
-                .common_arg = {
-                    .dev_lock = dev_handle,
-                },
-                .no_protect = true,
-            };
             break;
         case SPI2_HOST:
 #if SOC_SPI_PERIPH_NUM > 2
@@ -266,18 +270,17 @@ esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, spi_bus_lo
 #endif
             //SPI2, SPI3
             chip->os_func = &esp_flash_spi23_default_os_functions;
-            chip->os_func_data = heap_caps_malloc(sizeof(app_func_arg_t),
-                                            MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
-            if (chip->os_func_data == NULL) {
-                return ESP_ERR_NO_MEM;
-            }
-            *(app_func_arg_t*) chip->os_func_data = (app_func_arg_t) {
-                .dev_lock = dev_handle,
-            };
             break;
-        default: return ESP_ERR_INVALID_ARG;
+        default:
+            return ESP_ERR_INVALID_ARG;
+            break;
     }
 
+    *(app_func_arg_t*) chip->os_func_data = (app_func_arg_t) {
+        .dev_lock = dev_handle,
+        .no_protect = true, // This is OK because this code path isn't used for the main flash chip which requires `no_protect = false`
+    };
+
     return ESP_OK;
 }
 
@@ -314,11 +317,9 @@ esp_err_t esp_flash_init_main_bus_lock(void)
 
 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,
+    main_flash_arg = (app_func_arg_t) {
+        .dev_lock = g_spi_lock_main_flash_dev,
+        .no_protect = false, // Required for the main flash chip
     };
     chip->os_func = &esp_flash_spi1_default_os_functions;
     chip->os_func_data = &main_flash_arg;
@@ -331,11 +332,11 @@ esp_err_t esp_flash_app_enable_os_functions(esp_flash_t* chip)
 // Valid task execution interval: continuous time with the cache enabled, which is longer than
 // CONFIG_SPI_FLASH_ERASE_YIELD_TICKS. Yield time shorter than CONFIG_SPI_FLASH_ERASE_YIELD_TICKS is
 // not treated as valid interval.
-static inline IRAM_ATTR bool on_spi1_check_yield(spi1_app_func_arg_t* ctx)
+static inline IRAM_ATTR bool on_spi_check_yield(app_func_arg_t* ctx)
 {
 #ifdef CONFIG_SPI_FLASH_YIELD_DURING_ERASE
     uint32_t time = esp_system_get_time();
-    // We handle the reset here instead of in `on_spi1_acquired()`, when acquire() and release() is
+    // We handle the reset here instead of in `on_spi_acquired()`, when acquire() and release() is
     // larger than CONFIG_SPI_FLASH_ERASE_YIELD_TICKS, to save one `esp_system_get_time()` call
     if ((time - ctx->released_since_us) >= CONFIG_SPI_FLASH_ERASE_YIELD_TICKS * portTICK_PERIOD_MS * 1000) {
         // Reset the acquired time as if the yield has just happened.
@@ -346,22 +347,22 @@ static inline IRAM_ATTR bool on_spi1_check_yield(spi1_app_func_arg_t* ctx)
 #endif
     return false;
 }
-static inline IRAM_ATTR void on_spi1_released(spi1_app_func_arg_t* ctx)
+static inline IRAM_ATTR void on_spi_released(app_func_arg_t* ctx)
 {
 #ifdef CONFIG_SPI_FLASH_YIELD_DURING_ERASE
     ctx->released_since_us = esp_system_get_time();
 #endif
 }
 
-static inline IRAM_ATTR void on_spi1_acquired(spi1_app_func_arg_t* ctx)
+static inline IRAM_ATTR void on_spi_acquired(app_func_arg_t* ctx)
 {
-    // Ideally, when the time after `on_spi1_released()` before this function is called is larger
+    // Ideally, when the time after `on_spi_released()` before this function is called is larger
     // than CONFIG_SPI_FLASH_ERASE_YIELD_TICKS, the acquired time should be reset. We assume the
-    // time after `on_spi1_check_yield()` before this function is so short that we can do the reset
+    // time after `on_spi_check_yield()` before this function is so short that we can do the reset
     // in that function instead.
 }
 
-static inline IRAM_ATTR void on_spi1_yielded(spi1_app_func_arg_t* ctx)
+static inline IRAM_ATTR void on_spi_yielded(app_func_arg_t* ctx)
 {
     uint32_t time = esp_system_get_time();
     ctx->acquired_since_us = time;

components/spi_flash/test/test_esp_flash.c

@@ -651,4 +651,5 @@ TEST_CASE("Test esp_flash read/write performance", "[esp_flash][test_env=UT_T1_E
 #endif
 
 TEST_CASE_MULTI_FLASH("Test esp_flash read/write performance", test_flash_read_write_performance);
+
 #endif //!TEMPORARY_DISABLED_FOR_TARGETS(ESP32C6)

components/spi_flash/test_apps/esp_flash/main/test_esp_flash_def.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Unlicense OR CC0-1.0
  */
@@ -153,6 +153,10 @@ typedef void (*flash_test_func_t)(const esp_partition_t *part);
 #define TEST_CASE_MULTI_FLASH(STR, FUNC_TO_RUN) \
     TEST_CASE(STR", "TEST_CHIP_NUM_STR" chips", "[esp_flash_multi][test_env=UT_T1_ESP_FLASH][timeout=35]") {flash_test_func(FUNC_TO_RUN, TEST_CONFIG_NUM);}
 #endif
+
+#define TEST_CASE_MULTI_FLASH_LONG(STR, FUNC_TO_RUN) \
+    TEST_CASE(STR", "TEST_CHIP_NUM_STR" chips", "[esp_flash_multi][test_env=UT_T1_ESP_FLASH][timeout=120]") {flash_test_func(FUNC_TO_RUN, TEST_CONFIG_NUM);}
+
 #endif
 
 #define TEST_CASE_MULTI_FLASH_IGNORE(STR, FUNC_TO_RUN) \

components/spi_flash/test_apps/esp_flash/main/test_esp_flash_drv.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Unlicense OR CC0-1.0
  */
@@ -15,6 +15,7 @@
 #include "esp_private/spi_common_internal.h"
 #include "esp_flash_spi_init.h"
 #include "memspi_host_driver.h"
+#include "spi_flash_mmap.h"
 #include <esp_attr.h>
 #include "esp_log.h"
 
@@ -444,6 +445,21 @@ void test_erase_large_region(const esp_partition_t *part)
 TEST_CASE_FLASH("SPI flash erase large region", test_erase_large_region);
 TEST_CASE_MULTI_FLASH("SPI flash erase large region", test_erase_large_region);
 
+static void test_flash_erase_not_trigger_wdt(const esp_partition_t *part)
+{
+    spi_host_device_t out_host_id;
+    get_chip_host(part->flash_chip, &out_host_id, NULL);
+    // don't erase main flash (spi1), test only spi2 & spi3
+    if (out_host_id != SPI1_HOST) {
+        ESP_ERROR_CHECK(esp_flash_erase_chip(part->flash_chip));
+    } else {
+        ESP_LOGI(TAG, "spi1 skipped");
+    }
+}
+
+TEST_CASE_MULTI_FLASH_LONG("Test erasing flash chip not triggering WDT", test_flash_erase_not_trigger_wdt);
+
+
 #if CONFIG_SPI_FLASH_AUTO_SUSPEND
 void esp_test_for_suspend(void)
 {

components/spi_flash/test_apps/esp_flash/pytest_esp_flash.py

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
 # SPDX-License-Identifier: Apache-2.0
 
 import pytest
@@ -56,6 +56,4 @@ def test_esp_flash_rom(dut: Dut) -> None:
     indirect=True,
 )
 def test_esp_flash_multi(dut: Dut) -> None:
-    dut.expect_exact('Press ENTER to see the list of tests')
-    dut.write('[esp_flash_multi]')
-    dut.expect_unity_test_output()
+    dut.run_all_single_board_cases(group='esp_flash_multi', timeout=120)