From 892b3ff14b923dc15eb136f1c2b1c76895027d9e Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Fri, 6 Oct 2017 15:38:01 +1100 Subject: [PATCH 1/2] spi_flash: Add option to verify all writes by reading back data Helpful when debugging SPI flash hardware related issues. TW15203 --- components/spi_flash/Kconfig | 17 ++++++++ components/spi_flash/flash_ops.c | 69 ++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/components/spi_flash/Kconfig b/components/spi_flash/Kconfig index 4028cf389..5a05d859b 100644 --- a/components/spi_flash/Kconfig +++ b/components/spi_flash/Kconfig @@ -1,5 +1,22 @@ menu "SPI Flash driver" +config SPI_FLASH_VERIFY_WRITE + bool "Verify SPI flash writes" + default n + help + If this option is enabled, any time SPI flash is written then the data will be read + back and verified. This can catch hardware problems with SPI flash, or flash which + was not erased before verification. + +config SPI_FLASH_LOG_FAILED_WRITE + bool "Log errors if verification fails" + depends on SPI_FLASH_VERIFY_WRITE + default n + help + If this option is enabled, if SPI flash write verification fails then a log error line + will be written with the address, expected & actual values. This can be useful when + debugging hardware SPI flash problems. + config SPI_FLASH_ENABLE_COUNTERS bool "Enable operation counters" default 0 diff --git a/components/spi_flash/flash_ops.c b/components/spi_flash/flash_ops.c index 7fd47dcca..eb2b23cf4 100644 --- a/components/spi_flash/flash_ops.c +++ b/components/spi_flash/flash_ops.c @@ -44,8 +44,9 @@ #define MAX_WRITE_CHUNK 8192 #define MAX_READ_CHUNK 16384 +static const char *TAG __attribute__((unused)) = "spi_flash"; + #if CONFIG_SPI_FLASH_ENABLE_COUNTERS -static const char *TAG = "spi_flash"; static spi_flash_counters_t s_flash_stats; #define COUNTER_START() uint32_t ts_begin = xthal_get_ccount() @@ -233,6 +234,66 @@ esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size) return spi_flash_translate_rc(rc); } +/* Wrapper around esp_rom_spiflash_write() that verifies data as written if CONFIG_SPI_FLASH_VERIFY_WRITE is set. + + If CONFIG_SPI_FLASH_VERIFY_WRITE is not set, this is esp_rom_spiflash_write(). +*/ +static IRAM_ATTR esp_rom_spiflash_result_t spi_flash_write_inner(uint32_t target, const uint32_t *src_addr, int32_t len) +{ +#ifndef CONFIG_SPI_FLASH_VERIFY_WRITE + return esp_rom_spiflash_write(target, src_addr, len); +#else // CONFIG_SPI_FLASH_VERIFY_WRITE + esp_rom_spiflash_result_t res = ESP_ROM_SPIFLASH_RESULT_OK; + assert(len % sizeof(uint32_t) == 0); + + uint32_t before_buf[ESP_ROM_SPIFLASH_BUFF_BYTE_READ_NUM / sizeof(uint32_t)]; + uint32_t after_buf[ESP_ROM_SPIFLASH_BUFF_BYTE_READ_NUM / sizeof(uint32_t)]; + int32_t remaining = len; + for(int i = 0; i < len; i += sizeof(before_buf)) { + int i_w = i / sizeof(uint32_t); // index in words (i is an index in bytes) + + int32_t read_len = MIN(sizeof(before_buf), remaining); + + // Read "before" contents from flash + res = esp_rom_spiflash_read(target + i, before_buf, read_len); + if (res != ESP_ROM_SPIFLASH_RESULT_OK) { + break; + } + + res = esp_rom_spiflash_write(target + i, &src_addr[i_w], read_len); + if (res != ESP_ROM_SPIFLASH_RESULT_OK) { + break; + } + + res = esp_rom_spiflash_read(target + i, after_buf, read_len); + if (res != ESP_ROM_SPIFLASH_RESULT_OK) { + break; + } + + for (int r = 0; r < read_len; r += sizeof(uint32_t)) { + int r_w = r / sizeof(uint32_t); // index in words (r is index in bytes) + + uint32_t expected = src_addr[i_w + r_w] & before_buf[r_w]; + uint32_t actual = after_buf[r_w]; + if (expected != actual) { +#ifdef CONFIG_SPI_FLASH_LOG_FAILED_WRITE + spi_flash_guard_end(); + ESP_LOGE(TAG, "Bad write at offset 0x%x expected 0x%08x readback 0x%08x", target + i + r, expected, actual); + spi_flash_guard_start(); +#endif + res = ESP_ROM_SPIFLASH_RESULT_ERR; + } + } + if (res != ESP_ROM_SPIFLASH_RESULT_OK) { + break; + } + remaining -= read_len; + } + return res; +#endif // CONFIG_SPI_FLASH_VERIFY_WRITE +} + + esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size) { CHECK_WRITE_ADDRESS(dst, size); @@ -269,7 +330,7 @@ esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size) uint32_t t = 0xffffffff; memcpy(((uint8_t *) &t) + (dst - left_off), srcc, left_size); spi_flash_guard_start(); - rc = esp_rom_spiflash_write(left_off, &t, 4); + rc = spi_flash_write_inner(left_off, &t, 4); spi_flash_guard_end(); if (rc != ESP_ROM_SPIFLASH_RESULT_OK) { goto out; @@ -296,7 +357,7 @@ esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size) write_src = (const uint8_t *)write_buf; } spi_flash_guard_start(); - rc = esp_rom_spiflash_write(dst + mid_off, (const uint32_t *) write_src, write_size); + rc = spi_flash_write_inner(dst + mid_off, (const uint32_t *) write_src, write_size); spi_flash_guard_end(); COUNTER_ADD_BYTES(write, write_size); mid_size -= write_size; @@ -311,7 +372,7 @@ esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size) uint32_t t = 0xffffffff; memcpy(&t, srcc + right_off, right_size); spi_flash_guard_start(); - rc = esp_rom_spiflash_write(dst + right_off, &t, 4); + rc = spi_flash_write_inner(dst + right_off, &t, 4); spi_flash_guard_end(); if (rc != ESP_ROM_SPIFLASH_RESULT_OK) { goto out; From f7ac41c2daf92920310b5975cbb6f3318a5a53f6 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 29 Nov 2017 14:26:57 +1100 Subject: [PATCH 2/2] spi_flash: Add option to log warnings if (spuriously) writing zero bits to ones Won't work for SPIFFS, maybe some other implementations? --- components/spi_flash/Kconfig | 14 ++++++++++++++ components/spi_flash/flash_ops.c | 15 +++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/components/spi_flash/Kconfig b/components/spi_flash/Kconfig index 5a05d859b..f5e3c7ecd 100644 --- a/components/spi_flash/Kconfig +++ b/components/spi_flash/Kconfig @@ -17,6 +17,20 @@ config SPI_FLASH_LOG_FAILED_WRITE will be written with the address, expected & actual values. This can be useful when debugging hardware SPI flash problems. +config SPI_FLASH_WARN_SETTING_ZERO_TO_ONE + bool "Log warning if writing zero bits to ones" + depends on SPI_FLASH_VERIFY_WRITE + default n + help + If this option is enabled, any SPI flash write which tries to set zero bits in the flash to + ones will log a warning. Such writes will not result in the requested data appearing identically + in flash once written, as SPI NOR flash can only set bits to one when an entire sector is erased. + After erasing, individual bits can only be written from one to zero. + + Note that some software (such as SPIFFS) which is aware of SPI NOR flash may write one bits as an + optimisation, relying on the data in flash becoming a bitwise AND of the new data and any existing data. + Such software will log spurious warnings if this option is enabled. + config SPI_FLASH_ENABLE_COUNTERS bool "Enable operation counters" default 0 diff --git a/components/spi_flash/flash_ops.c b/components/spi_flash/flash_ops.c index eb2b23cf4..aab0c1210 100644 --- a/components/spi_flash/flash_ops.c +++ b/components/spi_flash/flash_ops.c @@ -260,6 +260,21 @@ static IRAM_ATTR esp_rom_spiflash_result_t spi_flash_write_inner(uint32_t target break; } +#ifdef CONFIG_SPI_FLASH_WARN_SETTING_ZERO_TO_ONE + for (int r = 0; r < read_len; r += sizeof(uint32_t)) { + int r_w = r / sizeof(uint32_t); // index in words (r is index in bytes) + + uint32_t write = src_addr[i_w + r_w]; + uint32_t before = before_buf[r_w]; + if ((before & write) != write) { + spi_flash_guard_end(); + ESP_LOGW(TAG, "Write at offset 0x%x requests 0x%08x but will write 0x%08x -> 0x%08x", + target + i + r, write, before, before & write); + spi_flash_guard_start(); + } + } +#endif + res = esp_rom_spiflash_write(target + i, &src_addr[i_w], read_len); if (res != ESP_ROM_SPIFLASH_RESULT_OK) { break;