From dc26065a7246c0e9a8913b9d9c708ded4aa26072 Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Thu, 2 Apr 2020 17:30:08 +0800 Subject: [PATCH 1/4] 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) From 11501dbaa96e37fdf669a84771a2a68d55d09f8d Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Tue, 7 Apr 2020 23:04:41 +0800 Subject: [PATCH 2/4] esp_flash: fix the cleanup when add device fails --- components/spi_flash/esp_flash_spi_init.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/components/spi_flash/esp_flash_spi_init.c b/components/spi_flash/esp_flash_spi_init.c index 11cf6a22a..ad519a829 100644 --- a/components/spi_flash/esp_flash_spi_init.c +++ b/components/spi_flash/esp_flash_spi_init.c @@ -121,25 +121,35 @@ esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_d if (config->host_id == SPI_HOST) caps = MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT; chip = (esp_flash_t*)heap_caps_malloc(sizeof(esp_flash_t), caps); - host = (spi_flash_host_driver_t*)heap_caps_malloc(sizeof(spi_flash_host_driver_t), caps); - host_data = (memspi_host_data_t*)heap_caps_malloc(sizeof(memspi_host_data_t), caps); - if (!chip || !host || !host_data) { + if (!chip) { ret = ESP_ERR_NO_MEM; goto fail; } + host = (spi_flash_host_driver_t*)heap_caps_malloc(sizeof(spi_flash_host_driver_t), caps); *chip = (esp_flash_t) { .read_mode = config->io_mode, .host = host, }; + if (!host) { + ret = ESP_ERR_NO_MEM; + goto fail; + } + + host_data = (memspi_host_data_t*)heap_caps_malloc(sizeof(memspi_host_data_t), caps); + host->driver_data = host_data; + if (!host_data) { + ret = ESP_ERR_NO_MEM; + goto fail; + } int dev_id; esp_err_t err = esp_flash_init_os_functions(chip, config->host_id, &dev_id); - assert(dev_id < SOC_SPI_PERIPH_CS_NUM(config->host_id) && dev_id >= 0); if (err != ESP_OK) { ret = err; goto fail; } + assert(dev_id < SOC_SPI_PERIPH_CS_NUM(config->host_id) && dev_id >= 0); bool use_iomux = spicommon_bus_using_iomux(config->host_id); memspi_host_config_t host_cfg = { @@ -159,6 +169,7 @@ esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_d *out_chip = chip; return ret; fail: + // The memory allocated are free'd in the `spi_bus_remove_flash_device`. spi_bus_remove_flash_device(chip); return ret; } From 5404e3d4341b59679cd386dcf160bdb273ddb692 Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Wed, 8 Apr 2020 00:42:46 +0800 Subject: [PATCH 3/4] esp_flash: fix cache exception when CS pin is through IOMUX --- components/spi_flash/esp_flash_spi_init.c | 2 ++ components/spi_flash/linker.lf | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/components/spi_flash/esp_flash_spi_init.c b/components/spi_flash/esp_flash_spi_init.c index ad519a829..ad94931a9 100644 --- a/components/spi_flash/esp_flash_spi_init.c +++ b/components/spi_flash/esp_flash_spi_init.c @@ -88,6 +88,8 @@ static IRAM_ATTR NOINLINE_ATTR void cs_initialize(esp_flash_t *chip, const esp_f //initialization, disable the cache temporarily chip->os_func->start(chip->os_func_data); if (use_iomux) { + // This requires `gpio_iomux_in` and `gpio_iomux_out` to be in the IRAM. + // `linker.lf` is used fulfill this requirement. gpio_iomux_in(cs_io_num, spics_in); gpio_iomux_out(cs_io_num, spics_func, false); } else { diff --git a/components/spi_flash/linker.lf b/components/spi_flash/linker.lf index efd5875d5..a31658712 100644 --- a/components/spi_flash/linker.lf +++ b/components/spi_flash/linker.lf @@ -8,3 +8,9 @@ entries: spi_flash_chip_gd(noflash) memspi_host_driver (noflash) +# `spi_bus_add_flash_device` uses these functions when the cache is disabled +[mapping:driver_spiflash] +archive: libdriver.a +entries: + gpio:gpio_iomux_out (noflash) + gpio:gpio_iomux_in (noflash) From 9d9d22c9203f78a025c1f5264a0f4e848cf721da Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Tue, 7 Apr 2020 23:53:43 +0800 Subject: [PATCH 4/4] esp_flash: deprecate the cs_id member, which is no longer used. We used to manually specify the CS id. However after the SPI bus lock is introduced, the lock is responsible to assign the CS lines and provide the CS id. The esp_flash driver now depends on the ID assigned by the SPI bus lock, the configuration field is deprecated. --- components/spi_flash/esp_flash_spi_init.c | 11 ++++++----- components/spi_flash/include/esp_flash_spi_init.h | 5 +++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/components/spi_flash/esp_flash_spi_init.c b/components/spi_flash/esp_flash_spi_init.c index ad94931a9..9713a24ae 100644 --- a/components/spi_flash/esp_flash_spi_init.c +++ b/components/spi_flash/esp_flash_spi_init.c @@ -73,14 +73,14 @@ __attribute__((unused)) static const char TAG[] = "spi_flash"; esp_flash_t *esp_flash_default_chip = NULL; -static IRAM_ATTR NOINLINE_ATTR void cs_initialize(esp_flash_t *chip, const esp_flash_spi_device_config_t *config, bool use_iomux) +static IRAM_ATTR NOINLINE_ATTR void cs_initialize(esp_flash_t *chip, const esp_flash_spi_device_config_t *config, bool use_iomux, int cs_id) { //Not using spicommon_cs_initialize since we don't want to put the whole //spi_periph_signal into the DRAM. Copy these data from flash before the //cache disabling int cs_io_num = config->cs_io_num; int spics_in = spi_periph_signal[config->host_id].spics_in; - int spics_out = spi_periph_signal[config->host_id].spics_out[config->cs_id]; + int spics_out = spi_periph_signal[config->host_id].spics_out[cs_id]; int spics_func = spi_periph_signal[config->host_id].func; uint32_t iomux_reg = GPIO_PIN_MUX_REG[cs_io_num]; @@ -101,7 +101,7 @@ static IRAM_ATTR NOINLINE_ATTR void cs_initialize(esp_flash_t *chip, const esp_f } GPIO.pin[cs_io_num].pad_driver = 0; gpio_matrix_out(cs_io_num, spics_out, false, false); - if (config->cs_id == 0) { + if (cs_id == 0) { gpio_matrix_in(cs_io_num, spics_in, false); } PIN_FUNC_SELECT(iomux_reg, PIN_FUNC_GPIO); @@ -167,7 +167,8 @@ esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_d goto fail; } - cs_initialize(chip, config, use_iomux); + // The cs_id inside `config` is deprecated, use the `dev_id` provided by the bus lock instead. + cs_initialize(chip, config, use_iomux, dev_id); *out_chip = chip; return ret; fail: @@ -210,7 +211,7 @@ esp_err_t esp_flash_init_default_chip(void) { memspi_host_config_t cfg = ESP_FLASH_HOST_CONFIG_DEFAULT(); - #ifdef CONFIG_IDF_TARGET_ESP32S2 + #ifdef CONFIG_IDF_TARGET_ESP32S2 // For esp32s2 spi IOs are configured as from IO MUX by default cfg.iomux = ets_efuse_get_spiconfig() == 0 ? true : false; #endif diff --git a/components/spi_flash/include/esp_flash_spi_init.h b/components/spi_flash/include/esp_flash_spi_init.h index 0e0e72d83..f3ac315ee 100644 --- a/components/spi_flash/include/esp_flash_spi_init.h +++ b/components/spi_flash/include/esp_flash_spi_init.h @@ -24,11 +24,12 @@ extern "C" { /// Configurations for the SPI Flash to init typedef struct { spi_host_device_t host_id; ///< Bus to use - int cs_id; ///< CS pin (signal) to use int cs_io_num; ///< GPIO pin to output the CS signal - esp_flash_io_mode_t io_mode; ///< IO mode to read from the Flash + esp_flash_io_mode_t io_mode; ///< IO mode to read from the Flash esp_flash_speed_t speed; ///< Speed of the Flash clock int input_delay_ns; ///< Input delay of the data pins, in ns. Set to 0 if unknown. + + int cs_id; ///< @deprecated CS pin (signal) to use } esp_flash_spi_device_config_t; /**