Crash_Override/OVMS3-idf
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

esp_flash: fix the regression of non-quad mode by default chip driver

Browse source 
The issue is introduced in 571864e8ae. 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.
This commit is contained in:
Michael (XIAO Xufeng) 2020-04-02 17:30:08 +08:00
parent b732abec7f
commit dc26065a72
3 changed files with 40 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
components/spi_flash/spi_flash_chip_drivers.c
View file

@ -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,
};

13
components/spi_flash/spi_flash_chip_generic.c
View file

@ -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) {

37
components/spi_flash/test/test_esp_flash.c
View file

@ -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)