From dc26065a7246c0e9a8913b9d9c708ded4aa26072 Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Thu, 2 Apr 2020 17:30:08 +0800 Subject: [PATCH] esp_flash: fix the regression of non-quad mode by default chip driver The issue is introduced in 571864e8aeba85b941133766601543e0decd0faf. The esp_flash API tries to clear the QE bit when the flash is not working in quad modes. However this introduces a regression, compared to earlier versions and the legacy API. When the chip is not detected, the generic chip driver is used, which cannot 100% handle the QE bit properly for all flash vendors. There may be some flash chips (e.g. MXIC) that can be used in dual modes by legacy API, but output wrong data when the esp_flash API clears the QE bit in a wrong way. This commit reverts the QE force clearing behavior, so that it's safer for the generic chip driver to work under dual modes. --- components/spi_flash/spi_flash_chip_drivers.c | 2 + components/spi_flash/spi_flash_chip_generic.c | 13 ++++--- components/spi_flash/test/test_esp_flash.c | 37 +++++++++++++++---- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/components/spi_flash/spi_flash_chip_drivers.c b/components/spi_flash/spi_flash_chip_drivers.c index 3b46b13e7..6e06d963e 100644 --- a/components/spi_flash/spi_flash_chip_drivers.c +++ b/components/spi_flash/spi_flash_chip_drivers.c @@ -39,6 +39,8 @@ static const spi_flash_chip_t *default_registered_chips[] = { #ifdef CONFIG_SPI_FLASH_SUPPORT_MXIC_CHIP &esp_flash_chip_mxic, #endif + // Default chip drivers that will accept all chip ID. + // FM, Winbond and XMC chips are supposed to be supported by this chip driver. &esp_flash_chip_generic, NULL, }; diff --git a/components/spi_flash/spi_flash_chip_generic.c b/components/spi_flash/spi_flash_chip_generic.c index 1b2995e29..539e11665 100644 --- a/components/spi_flash/spi_flash_chip_generic.c +++ b/components/spi_flash/spi_flash_chip_generic.c @@ -456,12 +456,15 @@ esp_err_t spi_flash_common_set_io_mode(esp_flash_t *chip, esp_flash_wrsr_func_t esp_err_t ret = ESP_OK; const bool is_quad_mode = esp_flash_is_quad_mode(chip); bool update_config = false; - const bool force_check = true; //in case some chips doesn't support erase QE + /* + * By default, we don't clear the QE bit even the flash mode is not QIO or QOUT. Force clearing + * QE bit by the generic chip driver (command 01H with 2 bytes) may cause the output of some + * chips (MXIC) no longer valid. + * Enable this option when testing a new flash chip for clearing of QE. + */ + const bool force_check = false; - bool need_check = is_quad_mode; - if (force_check) { - need_check = true; - } + bool need_check = is_quad_mode || force_check; uint32_t sr_update; if (need_check) { diff --git a/components/spi_flash/test/test_esp_flash.c b/components/spi_flash/test/test_esp_flash.c index d029ac8f2..f431362a8 100644 --- a/components/spi_flash/test/test_esp_flash.c +++ b/components/spi_flash/test/test_esp_flash.c @@ -77,6 +77,9 @@ typedef void (*flash_test_func_t)(esp_flash_t* chip); #define FLASH_TEST_CASE(STR, FUNC_TO_RUN) \ TEST_CASE(STR, "[esp_flash]") {flash_test_func(FUNC_TO_RUN, 1 /* first index reserved for main flash */ );} +#define FLASH_TEST_CASE_IGNORE(STR, FUNC_TO_RUN) \ + TEST_CASE(STR, "[esp_flash][ignore]") {flash_test_func(FUNC_TO_RUN, 1 /* first index reserved for main flash */ );} + /* Use FLASH_TEST_CASE_3 for tests which also run on external flash, which sits in the place of PSRAM (these tests are incompatible with PSRAM) @@ -84,10 +87,14 @@ typedef void (*flash_test_func_t)(esp_flash_t* chip); */ #if defined(CONFIG_SPIRAM_SUPPORT) || TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2) #define FLASH_TEST_CASE_3(STR, FUNCT_TO_RUN) +#define FLASH_TEST_CASE_3_IGNORE(STR, FUNCT_TO_RUN) #else // Disabled for ESP32-S2 due to lack of runners #define FLASH_TEST_CASE_3(STR, FUNC_TO_RUN) \ TEST_CASE(STR", 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]") {flash_test_func(FUNC_TO_RUN, TEST_CONFIG_NUM);} + +#define FLASH_TEST_CASE_3_IGNORE(STR, FUNC_TO_RUN) \ + TEST_CASE(STR", 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH][ignore]") {flash_test_func(FUNC_TO_RUN, TEST_CONFIG_NUM);} #endif //currently all the configs are the same with esp_flash_spi_device_config_t, no more information required @@ -499,8 +506,9 @@ static void read_and_check(esp_flash_t *chip, const esp_partition_t *part, const esp_err_t esp_flash_set_io_mode(esp_flash_t* chip, bool qe); esp_err_t esp_flash_get_io_mode(esp_flash_t* chip, bool* qe); esp_err_t esp_flash_read_chip_id(esp_flash_t* chip, uint32_t* flash_id); +esp_err_t spi_flash_chip_mxic_probe(esp_flash_t *chip, uint32_t flash_id); -static bool check_winbond_chip(esp_flash_t* chip) +static bool is_winbond_chip(esp_flash_t* chip) { uint32_t flash_id; esp_err_t ret = esp_flash_read_chip_id(chip, &flash_id); @@ -512,6 +520,14 @@ static bool check_winbond_chip(esp_flash_t* chip) } } +static bool is_mxic_chip(esp_flash_t* chip) +{ + uint32_t flash_id; + esp_err_t ret = esp_flash_read_chip_id(chip, &flash_id); + TEST_ESP_OK(ret); + return (spi_flash_chip_mxic_probe(chip, flash_id)==ESP_OK); +} + static void test_toggle_qe(esp_flash_t* chip) { bool qe; @@ -522,14 +538,14 @@ static void test_toggle_qe(esp_flash_t* chip) esp_err_t ret = esp_flash_get_io_mode(chip, &qe); TEST_ESP_OK(ret); - bool is_winbond_chip = check_winbond_chip(chip); + bool allow_failure = is_winbond_chip(chip) || is_mxic_chip(chip); for (int i = 0; i < 4; i ++) { ESP_LOGI(TAG, "write qe: %d->%d", qe, !qe); qe = !qe; chip->read_mode = qe? SPI_FLASH_QOUT: SPI_FLASH_SLOWRD; ret = esp_flash_set_io_mode(chip, qe); - if (is_winbond_chip && !qe && ret == ESP_ERR_FLASH_NO_RESPONSE) { + if (allow_failure && !qe && ret == ESP_ERR_FLASH_NO_RESPONSE) { //allows clear qe failure for Winbond chips ret = ESP_OK; } @@ -539,8 +555,12 @@ static void test_toggle_qe(esp_flash_t* chip) ret = esp_flash_get_io_mode(chip, &qe_read); TEST_ESP_OK(ret); ESP_LOGD(TAG, "qe read: %d", qe_read); - if (qe != qe_read && !qe && is_winbond_chip) { - ESP_LOGE(TAG, "cannot clear QE bit, this may be normal for Winbond chips."); + if (!qe && qe_read) { + if (allow_failure) { + ESP_LOGW(TAG, "cannot clear QE bit for known permanent QE (Winbond or MXIC) chips."); + } else { + ESP_LOGE(TAG, "cannot clear QE bit, please make sure force clearing QE option is enabled in `spi_flash_common_set_io_mode`, and this chip is not a permanent QE one."); + } chip->read_mode = io_mode_before; return; } @@ -550,8 +570,11 @@ static void test_toggle_qe(esp_flash_t* chip) chip->read_mode = io_mode_before; } -FLASH_TEST_CASE("Test esp_flash_write can toggle QE bit", test_toggle_qe); -FLASH_TEST_CASE_3("Test esp_flash_write can toggle QE bit", test_toggle_qe); +// These tests show whether the QE is permanent or not for the chip tested. +// To test the behaviour of a new SPI flash chip, enable force_check flag in generic driver +// `spi_flash_common_set_io_mode` and then run this test. +FLASH_TEST_CASE_IGNORE("Test esp_flash_write can toggle QE bit", test_toggle_qe); +FLASH_TEST_CASE_3_IGNORE("Test esp_flash_write can toggle QE bit", test_toggle_qe); void test_permutations(flashtest_config_t* config)