Merge branch 'feature/spi_flash_verify_write' into 'master'

spi_flash: verify_write feature, and ESP-IDF vs ESP-ROM spi flash driver difference doc

Closes IDF-5624

See merge request espressif/esp-idf!19998

components/spi_flash/Kconfig

@@ -3,6 +3,7 @@ menu "SPI Flash driver"
 
     config SPI_FLASH_VERIFY_WRITE
         bool "Verify SPI flash writes"
+        depends on !SPI_FLASH_ROM_IMPL
         default n
         help
             If this option is enabled, any time SPI flash is written then the data will be read

components/spi_flash/esp_flash_api.c

@@ -23,7 +23,7 @@
 #include "esp_crypto_lock.h" // for locking flash encryption peripheral
 #endif //CONFIG_IDF_TARGET_ESP32S2
 
-static const char TAG[] = "spi_flash";
+DRAM_ATTR static const char TAG[] = "spi_flash";
 
 #ifdef CONFIG_SPI_FLASH_WRITE_CHUNK_SIZE
 #define MAX_WRITE_CHUNK CONFIG_SPI_FLASH_WRITE_CHUNK_SIZE /* write in chunks */
@@ -32,6 +32,7 @@ static const char TAG[] = "spi_flash";
 #endif // CONFIG_SPI_FLASH_WRITE_CHUNK_SIZE
 
 #define MAX_READ_CHUNK 16384
+#define VERIFY_BUF_LEN 64
 
 
 #ifdef CONFIG_SPI_FLASH_DANGEROUS_WRITE_ABORTS
@@ -847,8 +848,91 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add
     return err;
 }
 
+#if CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE
+static esp_err_t IRAM_ATTR s_check_setting_zero_to_one(esp_flash_t *chip, uint32_t verify_address, uint32_t remain_verify_len, const uint32_t *to_write_buf, bool is_encrypted)
+{
+    esp_err_t err = ESP_FAIL;
+    uint8_t verify_buffer[VERIFY_BUF_LEN];
+    uint32_t *val_in_flash = (uint32_t *)verify_buffer;
+
+    while (remain_verify_len) {
+        uint32_t this_len = MIN(remain_verify_len, VERIFY_BUF_LEN);
+
+        err = chip->chip_drv->read(chip, verify_buffer, verify_address, this_len);
+        if (err != ESP_OK) {
+            ESP_DRAM_LOGE(TAG, "failed to read flash to verify if setting zero to one, err: 0x%x", err);
+            return err;
+        }
+
+        for (int r = 0; r < this_len / sizeof(uint32_t); r++) {
+            if (is_encrypted) {
+                (void)to_write_buf;
+                if (val_in_flash[r] != 0xFFFFFFFF) {
+                    ESP_DRAM_LOGW(TAG, "Write at offset 0x%x but not erased (0x%08x)",
+                                  verify_address + r, val_in_flash[r]);
+                }
+            } else {
+                if ((val_in_flash[r] & to_write_buf[r]) != to_write_buf[r]) {
+                    ESP_DRAM_LOGW(TAG, "Write at offset 0x%x requests 0x%08x but will write 0x%08x -> 0x%08x",
+                                  verify_address + r, to_write_buf[r], val_in_flash[r], (val_in_flash[r] & to_write_buf[r]));
+                }
+            }
+        }
+
+        remain_verify_len -= this_len;
+        verify_address += this_len;
+    }
+
+    return ESP_OK;
+}
+#endif  //#if CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE
+
+#if CONFIG_SPI_FLASH_VERIFY_WRITE
+static esp_err_t IRAM_ATTR s_verify_write(esp_flash_t *chip, uint32_t verify_address, uint32_t remain_verify_len, const uint32_t *expected_buf, bool is_encrypted)
+{
+    esp_err_t err = ESP_FAIL;
+    uint8_t verify_buffer[VERIFY_BUF_LEN];
+    uint32_t *val_in_flash = (uint32_t *)verify_buffer;
+
+    while (remain_verify_len) {
+        uint32_t this_len = MIN(remain_verify_len, VERIFY_BUF_LEN);
+
+        if (is_encrypted) {
+            err = esp_flash_read_encrypted(chip, verify_address, verify_buffer, this_len);
+        } else {
+            err = chip->chip_drv->read(chip, verify_buffer, verify_address, this_len);
+        }
+        if (err != ESP_OK) {
+            ESP_DRAM_LOGE(TAG, "failed to read flash to verify previous write, err: 0x%x", err);
+            return err;
+        }
+
+        for (int r = 0; r < this_len / sizeof(uint32_t); r++) {
+            if (val_in_flash[r] != expected_buf[r]) {
+#if CONFIG_SPI_FLASH_LOG_FAILED_WRITE
+                ESP_DRAM_LOGE(TAG, "Bad write at %d offset: 0x%x, expected: 0x%08x, readback: 0x%08x", r, verify_address + r, expected_buf[r], val_in_flash[r]);
+#endif  //#if CONFIG_SPI_FLASH_LOG_FAILED_WRITE
+                return ESP_FAIL;
+            }
+        }
+
+        expected_buf = (uint32_t *)((void *)expected_buf + this_len);
+        remain_verify_len -= this_len;
+        verify_address += this_len;
+    }
+
+    return ESP_OK;
+}
+#endif //#if CONFIG_SPI_FLASH_VERIFY_WRITE
+
 esp_err_t IRAM_ATTR esp_flash_write(esp_flash_t *chip, const void *buffer, uint32_t address, uint32_t length)
 {
+    esp_err_t ret = ESP_FAIL;
+#if CONFIG_SPI_FLASH_VERIFY_WRITE
+    //used for verify write
+    bool is_encrypted = false;
+#endif //CONFIG_SPI_FLASH_VERIFY_WRITE
+
     esp_err_t err = rom_spiflash_api_funcs->chip_check(&chip);
     VERIFY_CHIP_OP(write);
     CHECK_WRITE_ADDRESS(chip, address, length);
@@ -898,37 +982,74 @@ esp_err_t IRAM_ATTR esp_flash_write(esp_flash_t *chip, const void *buffer, uint3
 
         err = rom_spiflash_api_funcs->start(chip);
         if (err != ESP_OK) {
-            break;
+            goto restore_cache;
         }
         bus_acquired = true;
 
+#if CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE
+        err = s_check_setting_zero_to_one(chip, write_addr, write_len, write_buf, is_encrypted);
+        if (err != ESP_OK) {
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
+        }
+#endif  //#if CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE
+
         err = chip->chip_drv->write(chip, write_buf, write_addr, write_len);
         len_remain -= write_len;
         assert(len_remain < length);
 
-        if (err != ESP_OK || len_remain == 0) {
-            // On ESP32, the cache re-enable is in the end() function, while flush_cache should
-            // happen when the cache is still disabled on ESP32. Break before the end() function and
-            // do end() later
+        if (err != ESP_OK) {
+            //Error happens, we end flash operation. Re-enable cache and flush it
             assert(bus_acquired);
+            goto restore_cache;
+        }
+
+#if CONFIG_SPI_FLASH_VERIFY_WRITE
+        err = s_verify_write(chip, write_addr, write_len, write_buf, is_encrypted);
+        if (err != ESP_OK) {
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
+        }
+#endif  //#if CONFIG_SPI_FLASH_VERIFY_WRITE
+
+
+        if (len_remain == 0) {
+            //Flash operation done
             break;
         }
 
         err = rom_spiflash_api_funcs->end(chip, err);
         if (err != ESP_OK) {
-            break;
+            goto restore_cache;
         }
         bus_acquired = false;
 
         write_addr += write_len;
         buffer = (void *)((intptr_t)buffer + write_len);
     }
 
-    return rom_spiflash_api_funcs->flash_end_flush_cache(chip, err, bus_acquired, address, length);
+    err = rom_spiflash_api_funcs->flash_end_flush_cache(chip, err, bus_acquired, address, length);
+
+    return err;
+
+restore_cache:
+
+    ret = rom_spiflash_api_funcs->flash_end_flush_cache(chip, err, bus_acquired, address, length);
+    if (ret != ESP_OK) {
+       ESP_DRAM_LOGE(TAG, "restore cache fail\n");
+    }
+
+    return err;
 }
 
 esp_err_t IRAM_ATTR esp_flash_write_encrypted(esp_flash_t *chip, uint32_t address, const void *buffer, uint32_t length)
 {
+    esp_err_t ret = ESP_FAIL;
+#if CONFIG_SPI_FLASH_VERIFY_WRITE
+    //used for verify write
+    bool is_encrypted = true;
+#endif //CONFIG_SPI_FLASH_VERIFY_WRITE
+
     esp_err_t err = rom_spiflash_api_funcs->chip_check(&chip);
     // Flash encryption only support on main flash.
     if (chip != esp_flash_default_chip) {
@@ -1014,6 +1135,14 @@ esp_err_t IRAM_ATTR esp_flash_write_encrypted(esp_flash_t *chip, uint32_t addres
         row_size_length = row_size;
 #endif //CONFIG_IDF_TARGET_ESP32
 
+#if CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE
+        err = s_check_setting_zero_to_one(chip, row_addr, encrypt_byte, NULL, is_encrypted);
+        if (err != ESP_OK) {
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
+        }
+#endif  //#if CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE
+
 #if CONFIG_IDF_TARGET_ESP32S2
         esp_crypto_dma_lock_acquire();
 #endif //CONFIG_IDF_TARGET_ESP32S2
@@ -1023,7 +1152,8 @@ esp_err_t IRAM_ATTR esp_flash_write_encrypted(esp_flash_t *chip, uint32_t addres
 #if CONFIG_IDF_TARGET_ESP32S2
             esp_crypto_dma_lock_release();
 #endif //CONFIG_IDF_TARGET_ESP32S2
-            break;
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
         }
         bus_acquired = true;
 
@@ -1034,19 +1164,41 @@ esp_err_t IRAM_ATTR esp_flash_write_encrypted(esp_flash_t *chip, uint32_t addres
 #endif //CONFIG_IDF_TARGET_ESP32S2
             bus_acquired = false;
             assert(bus_acquired);
-            break;
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
         }
         err = rom_spiflash_api_funcs->end(chip, ESP_OK);
 #if CONFIG_IDF_TARGET_ESP32S2
         esp_crypto_dma_lock_release();
 #endif //CONFIG_IDF_TARGET_ESP32S2
         if (err != ESP_OK) {
             bus_acquired = false;
-            break;
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
         }
         bus_acquired = false;
+
+#if CONFIG_SPI_FLASH_VERIFY_WRITE
+        err = s_verify_write(chip, row_addr, encrypt_byte, (uint32_t *)encrypt_buf, is_encrypted);
+        if (err != ESP_OK) {
+            //Error happens, we end flash operation. Re-enable cache and flush it
+            goto restore_cache;
+        }
+#endif //CONFIG_SPI_FLASH_VERIFY_WRITE
     }
-    return rom_spiflash_api_funcs->flash_end_flush_cache(chip, err, bus_acquired, address, length);
+
+    err = rom_spiflash_api_funcs->flash_end_flush_cache(chip, err, bus_acquired, address, length);
+
+    return err;
+
+restore_cache:
+
+    ret = rom_spiflash_api_funcs->flash_end_flush_cache(chip, err, bus_acquired, address, length);
+    if (ret != ESP_OK) {
+       ESP_DRAM_LOGE(TAG, "restore cache fail\n");
+    }
+
+    return err;
 }
 
 inline static IRAM_ATTR bool regions_overlap(uint32_t a_start, uint32_t a_len,uint32_t b_start, uint32_t b_len)

components/spi_flash/include/esp_flash.h

@@ -317,6 +317,7 @@ esp_err_t esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t address, uint
  *
  * @return
  *      - ESP_OK on success,
+ *      - ESP_FAIL, bad write, this will be detected only when CONFIG_SPI_FLASH_VERIFY_WRITE is enabled
  *      - ESP_ERR_NOT_SUPPORTED if the chip is not able to perform the operation. This is indicated by WREN = 1 after the command is sent.
  *      - Other flash error code if operation failed.
  */
@@ -333,6 +334,7 @@ esp_err_t esp_flash_write(esp_flash_t *chip, const void *buffer, uint32_t addres
  *
  * @return
  *  - ESP_OK: on success
+ *  - ESP_FAIL: bad write, this will be detected only when CONFIG_SPI_FLASH_VERIFY_WRITE is enabled
  *  - ESP_ERR_NOT_SUPPORTED: encrypted write not supported for this chip.
  *  - ESP_ERR_INVALID_ARG: Either the address, buffer or length is invalid.
  */

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

@@ -411,6 +411,7 @@ TEST_CASE_MULTI_FLASH("SPI flash three byte reads/writes", test_three_byte_read_
 void test_erase_large_region(const esp_partition_t *part)
 {
     esp_flash_t* chip = part->flash_chip;
+    erase_test_region(part, 2);
 
     /* Write some noise at the start and the end of the region */
     const char *ohai = "OHAI";

components/spi_flash/test_apps/esp_flash/pytest_esp_flash.py

@@ -16,6 +16,7 @@
     [
         'release',
         'flash_qio',
+        'verify'
     ],
     indirect=True,
 )

components/spi_flash/test_apps/esp_flash/sdkconfig.ci.flash_qio

@@ -1,4 +1,2 @@
 CONFIG_ESP_TASK_WDT_EN=n
-CONFIG_PARTITION_TABLE_CUSTOM=y
-CONFIG_PARTITION_TABLE_CUSTOM_FILENAME="partitions.csv"
 CONFIG_ESPTOOLPY_FLASHMODE_QIO=y

components/spi_flash/test_apps/esp_flash/sdkconfig.ci.release

@@ -3,5 +3,3 @@ CONFIG_FREERTOS_USE_TICKLESS_IDLE=y
 CONFIG_COMPILER_OPTIMIZATION_SIZE=y
 CONFIG_BOOTLOADER_COMPILER_OPTIMIZATION_SIZE=y
 CONFIG_COMPILER_OPTIMIZATION_ASSERTIONS_SILENT=y
-CONFIG_PARTITION_TABLE_CUSTOM=y
-CONFIG_PARTITION_TABLE_CUSTOM_FILENAME="partitions.csv"

components/spi_flash/test_apps/esp_flash/sdkconfig.ci.rom_impl

@@ -1,4 +1,2 @@
 CONFIG_ESP_TASK_WDT_EN=n
-CONFIG_PARTITION_TABLE_CUSTOM=y
-CONFIG_PARTITION_TABLE_CUSTOM_FILENAME="partitions.csv"
 CONFIG_SPI_FLASH_ROM_IMPL=y

components/spi_flash/test_apps/esp_flash/sdkconfig.ci.verify

@@ -0,0 +1,4 @@
+CONFIG_ESP_TASK_WDT=n
+CONFIG_SPI_FLASH_VERIFY_WRITE=y
+CONFIG_SPI_FLASH_LOG_FAILED_WRITE=y
+CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE=y

components/spi_flash/test_apps/flash_encryption/sdkconfig.ci.verify

@@ -0,0 +1,19 @@
+CONFIG_ESP_TASK_WDT_EN=n
+CONFIG_FREERTOS_USE_TICKLESS_IDLE=y
+CONFIG_COMPILER_OPTIMIZATION_SIZE=y
+CONFIG_BOOTLOADER_COMPILER_OPTIMIZATION_SIZE=y
+CONFIG_COMPILER_OPTIMIZATION_ASSERTIONS_SILENT=y
+CONFIG_PARTITION_TABLE_CUSTOM=y
+CONFIG_PARTITION_TABLE_CUSTOM_FILENAME="partitions.csv"
+CONFIG_SECURE_FLASH_ENC_ENABLED=y
+CONFIG_SECURE_FLASH_ENCRYPTION_MODE_DEVELOPMENT=y
+CONFIG_SECURE_BOOT_ALLOW_ROM_BASIC=y
+CONFIG_SECURE_BOOT_ALLOW_JTAG=y
+CONFIG_SECURE_FLASH_UART_BOOTLOADER_ALLOW_ENC=y
+CONFIG_SECURE_FLASH_UART_BOOTLOADER_ALLOW_DEC=y
+CONFIG_SECURE_FLASH_UART_BOOTLOADER_ALLOW_CACHE=y
+CONFIG_SECURE_FLASH_REQUIRE_ALREADY_ENABLED=y
+
+CONFIG_SPI_FLASH_VERIFY_WRITE=y
+CONFIG_SPI_FLASH_LOG_FAILED_WRITE=y
+CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE=y

docs/en/api-guides/performance/ram-usage.rst

@@ -134,7 +134,7 @@ The following options will reduce IRAM usage of some ESP-IDF features:
     - Enable :ref:`CONFIG_RINGBUF_PLACE_FUNCTIONS_INTO_FLASH`. Provided these functions are not (incorrectly) used from ISRs, this option is safe to enable in all configurations.
     - Enable :ref:`CONFIG_RINGBUF_PLACE_ISR_FUNCTIONS_INTO_FLASH`. This option is not safe to use if the ISR ringbuf functions are used from an IRAM interrupt context, e.g. if :ref:`CONFIG_UART_ISR_IN_IRAM` is enabled. For the IDF drivers where this is the case you will get an error at run-time when installing the driver in question.
     :SOC_WIFI_SUPPORTED: - Disable Wi-Fi options :ref:`CONFIG_ESP_WIFI_IRAM_OPT` and/or :ref:`CONFIG_ESP_WIFI_RX_IRAM_OPT`. Disabling these options will free available IRAM at the cost of Wi-Fi performance.
-    :esp32c3 or esp32s3: - :ref:`CONFIG_SPI_FLASH_ROM_IMPL` enabling this option will free some IRAM but will mean that esp_flash bugfixes and new flash chip support is not available.
+    :CONFIG_ESP_ROM_HAS_SPI_FLASH: - :ref:`CONFIG_SPI_FLASH_ROM_IMPL` enabling this option will free some IRAM but will mean that esp_flash bugfixes and new flash chip support is not available, see :doc:`/api-reference/peripherals/spi_flash/spi_flash_idf_vs_rom` for details.
     :esp32: - :ref:`CONFIG_SPI_FLASH_ROM_DRIVER_PATCH` disabling this option will free some IRAM but is only available in some flash configurations (see the configuration item help text).
     :esp32: - If the application uses PSRAM and is based on ESP32 rev. 3 (ECO3), setting :ref:`CONFIG_ESP32_REV_MIN` to ``3`` will disable PSRAM bug workarounds, saving ~10kB or more of IRAM.
     - Disabling :ref:`CONFIG_ESP_EVENT_POST_FROM_IRAM_ISR` prevents posting ``esp_event`` events from :ref:`iram-safe-interrupt-handlers` but will save some IRAM.

docs/en/api-reference/peripherals/spi_flash/index.rst

@@ -250,6 +250,20 @@ Additionally, all API functions are protected with a mutex (``s_flash_op_mutex``
 
 In a single core environment (:ref:`CONFIG_FREERTOS_UNICORE` enabled), you need to disable both caches, so that no inter-CPU communication can take place.
 
+
+.. toctree::
+    :hidden:
+
+    spi_flash_idf_vs_rom
+
+.. only:: CONFIG_ESP_ROM_HAS_SPI_FLASH
+
+    ESP-IDF vs Chip-ROM SPI Flash Driver
+    ------------------------------------
+
+    Refer to :doc:`spi_flash_idf_vs_rom`.
+
+
 API Reference - SPI Flash
 -------------------------