From 651eb1a69446622bbeaa9628807a8cbb2437b6fd Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Fri, 8 May 2020 17:35:22 +0800 Subject: [PATCH] esp_flash: fix the write performance regression Also changed internal delay unit into microsecond. --- components/spi_flash/include/esp_flash.h | 4 +- .../include/spi_flash_chip_generic.h | 8 +-- components/spi_flash/spi_flash_chip_generic.c | 70 +++++++++++-------- components/spi_flash/spi_flash_os_func_app.c | 8 +-- components/spi_flash/spi_flash_os_func_noos.c | 6 +- 5 files changed, 53 insertions(+), 43 deletions(-) diff --git a/components/spi_flash/include/esp_flash.h b/components/spi_flash/include/esp_flash.h index ec196deff..a60bfe437 100644 --- a/components/spi_flash/include/esp_flash.h +++ b/components/spi_flash/include/esp_flash.h @@ -48,8 +48,8 @@ typedef struct { /** Called before any erase/write operations to check whether the region is limited by the OS */ esp_err_t (*region_protected)(void* arg, size_t start_addr, size_t size); - /** Delay for at least 'ms' milliseconds. Called in between 'start' and 'end'. */ - esp_err_t (*delay_ms)(void *arg, unsigned ms); + /** Delay for at least 'us' microseconds. Called in between 'start' and 'end'. */ + esp_err_t (*delay_us)(void *arg, unsigned us); } esp_flash_os_functions_t; /** @brief Structure to describe a SPI flash chip connected to the system. diff --git a/components/spi_flash/include/spi_flash_chip_generic.h b/components/spi_flash/include/spi_flash_chip_generic.h index 36dd3cb89..15ff8f7b1 100644 --- a/components/spi_flash/include/spi_flash_chip_generic.h +++ b/components/spi_flash/include/spi_flash_chip_generic.h @@ -193,14 +193,14 @@ esp_err_t spi_flash_chip_generic_get_write_protect(esp_flash_t *chip, bool *out_ * progress bit) to be cleared. * * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted. - * @param timeout_ms Time to wait before timeout, in ms. + * @param timeout_us Time to wait before timeout, in us. * * @return * - ESP_OK if success * - ESP_ERR_TIMEOUT if not idle before timeout * - or other error passed from the ``wait_idle`` or ``read_status`` function of host driver */ -esp_err_t spi_flash_chip_generic_wait_idle(esp_flash_t *chip, uint32_t timeout_ms); +esp_err_t spi_flash_chip_generic_wait_idle(esp_flash_t *chip, uint32_t timeout_us); /** * @brief Set the specified SPI read mode according to the data in the chip @@ -247,7 +247,7 @@ extern const spi_flash_chip_t esp_flash_chip_generic; * spi_flash_chip_generic_wait_idle() and may be useful when implementing * alternative drivers. * - * timeout_ms will be decremented if the function needs to wait until the host hardware is idle. + * timeout_us will be decremented if the function needs to wait until the host hardware is idle. * * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted. * @@ -256,7 +256,7 @@ extern const spi_flash_chip_t esp_flash_chip_generic; * - ESP_ERR_TIMEOUT if not idle before timeout * - or other error passed from the ``set_write_protect`` or ``common_command`` function of host driver */ -esp_err_t spi_flash_generic_wait_host_idle(esp_flash_t *chip, uint32_t *timeout_ms); +esp_err_t spi_flash_generic_wait_host_idle(esp_flash_t *chip, uint32_t *timeout_us); /// Function pointer type for reading status register with QE bit. typedef esp_err_t (*esp_flash_rdsr_func_t)(esp_flash_t* chip, uint32_t* out_sr); diff --git a/components/spi_flash/spi_flash_chip_generic.c b/components/spi_flash/spi_flash_chip_generic.c index 539e11665..e0c3badd5 100644 --- a/components/spi_flash/spi_flash_chip_generic.c +++ b/components/spi_flash/spi_flash_chip_generic.c @@ -20,12 +20,15 @@ static const char TAG[] = "chip_generic"; -#define SPI_FLASH_GENERIC_CHIP_ERASE_TIMEOUT 4000 -#define SPI_FLASH_GENERIC_SECTOR_ERASE_TIMEOUT 500 //according to GD25Q127 + 100ms -#define SPI_FLASH_GENERIC_BLOCK_ERASE_TIMEOUT 1300 //according to GD25Q127 + 100ms +#define SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS 200 +#define SPI_FLASH_GENERIC_CHIP_ERASE_TIMEOUT_MS 4000 +#define SPI_FLASH_GENERIC_SECTOR_ERASE_TIMEOUT_MS 500 //according to GD25Q127 + 100ms +#define SPI_FLASH_GENERIC_BLOCK_ERASE_TIMEOUT_MS 1300 //according to GD25Q127 + 100ms +#define SPI_FLASH_GENERIC_PAGE_PROGRAM_TIMEOUT_MS 500 + +#define HOST_DELAY_INTERVAL_US 1 +#define CHIP_WAIT_IDLE_INTERVAL_US 20 -#define DEFAULT_IDLE_TIMEOUT 200 -#define DEFAULT_PAGE_PROGRAM_TIMEOUT 500 esp_err_t spi_flash_chip_generic_probe(esp_flash_t *chip, uint32_t flash_id) { @@ -54,7 +57,7 @@ esp_err_t spi_flash_chip_generic_reset(esp_flash_t *chip) return err; } - err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); return err; } @@ -80,7 +83,7 @@ esp_err_t spi_flash_chip_generic_erase_chip(esp_flash_t *chip) err = chip->chip_drv->set_chip_write_protect(chip, false); if (err == ESP_OK) { - err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); } if (err == ESP_OK) { chip->host->erase_chip(chip->host); @@ -91,7 +94,7 @@ esp_err_t spi_flash_chip_generic_erase_chip(esp_flash_t *chip) return err; } } - err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_CHIP_ERASE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_CHIP_ERASE_TIMEOUT_MS * 1000); } return err; } @@ -100,7 +103,7 @@ esp_err_t spi_flash_chip_generic_erase_sector(esp_flash_t *chip, uint32_t start_ { esp_err_t err = chip->chip_drv->set_chip_write_protect(chip, false); if (err == ESP_OK) { - err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); } if (err == ESP_OK) { chip->host->erase_sector(chip->host, start_address); @@ -111,7 +114,7 @@ esp_err_t spi_flash_chip_generic_erase_sector(esp_flash_t *chip, uint32_t start_ return err; } } - err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_SECTOR_ERASE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_SECTOR_ERASE_TIMEOUT_MS * 1000); } return err; } @@ -120,7 +123,7 @@ esp_err_t spi_flash_chip_generic_erase_block(esp_flash_t *chip, uint32_t start_a { esp_err_t err = chip->chip_drv->set_chip_write_protect(chip, false); if (err == ESP_OK) { - err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); } if (err == ESP_OK) { chip->host->erase_block(chip->host, start_address); @@ -131,7 +134,7 @@ esp_err_t spi_flash_chip_generic_erase_block(esp_flash_t *chip, uint32_t start_a return err; } } - err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_BLOCK_ERASE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_BLOCK_ERASE_TIMEOUT_MS * 1000); } return err; } @@ -163,13 +166,13 @@ esp_err_t spi_flash_chip_generic_page_program(esp_flash_t *chip, const void *buf { esp_err_t err; - err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); if (err == ESP_OK) { // Perform the actual Page Program command chip->host->program_page(chip->host, buffer, address, length); - err = chip->chip_drv->wait_idle(chip, DEFAULT_PAGE_PROGRAM_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_GENERIC_PAGE_PROGRAM_TIMEOUT_MS * 1000); } return err; } @@ -210,7 +213,7 @@ esp_err_t spi_flash_chip_generic_set_write_protect(esp_flash_t *chip, bool write { esp_err_t err = ESP_OK; - err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + err = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); if (err == ESP_OK) { chip->host->set_write_protect(chip->host, write_protect); @@ -239,25 +242,31 @@ esp_err_t spi_flash_chip_generic_get_write_protect(esp_flash_t *chip, bool *out_ return err; } -esp_err_t spi_flash_generic_wait_host_idle(esp_flash_t *chip, uint32_t *timeout_ms) +esp_err_t spi_flash_generic_wait_host_idle(esp_flash_t *chip, uint32_t *timeout_us) { - while (chip->host->host_idle(chip->host) && *timeout_ms > 0) { - if (*timeout_ms > 1) { - chip->os_func->delay_ms(chip->os_func_data, 1); + while (chip->host->host_idle(chip->host) && *timeout_us > 0) { +#if HOST_DELAY_INTERVAL_US > 0 + if (*timeout_us > 1) { + int delay = MIN(HOST_DELAY_INTERVAL_US, *timeout_us); + chip->os_func->delay_us(chip->os_func_data, delay); + *timeout_us -= delay; + } else { + return ESP_ERR_TIMEOUT; } - (*timeout_ms)--; +#endif } - return (*timeout_ms > 0) ? ESP_OK : ESP_ERR_TIMEOUT; + return ESP_OK; } -esp_err_t spi_flash_chip_generic_wait_idle(esp_flash_t *chip, uint32_t timeout_ms) +esp_err_t spi_flash_chip_generic_wait_idle(esp_flash_t *chip, uint32_t timeout_us) { - timeout_ms++; // allow at least one pass before timeout, last one has no sleep cycle + timeout_us++; // allow at least one pass before timeout, last one has no sleep cycle uint8_t status = 0; - while (timeout_ms > 0) { + const int interval = CHIP_WAIT_IDLE_INTERVAL_US; + while (timeout_us > 0) { - esp_err_t err = spi_flash_generic_wait_host_idle(chip, &timeout_ms); + esp_err_t err = spi_flash_generic_wait_host_idle(chip, & timeout_us); if (err != ESP_OK) { return err; } @@ -269,13 +278,14 @@ esp_err_t spi_flash_chip_generic_wait_idle(esp_flash_t *chip, uint32_t timeout_m if ((status & SR_WIP) == 0) { break; // Write in progress is complete } - if (timeout_ms > 1) { - chip->os_func->delay_ms(chip->os_func_data, 1); + if (timeout_us > 0 && interval > 0) { + int delay = MIN(interval, timeout_us); + chip->os_func->delay_us(chip->os_func_data, delay); + timeout_us -= delay; } - timeout_ms--; } - return (timeout_ms > 0) ? ESP_OK : ESP_ERR_TIMEOUT; + return (timeout_us > 0) ? ESP_OK : ESP_ERR_TIMEOUT; } esp_err_t spi_flash_chip_generic_config_host_io_mode(esp_flash_t *chip) @@ -495,7 +505,7 @@ esp_err_t spi_flash_common_set_io_mode(esp_flash_t *chip, esp_flash_wrsr_func_t return ret; } - ret = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT); + ret = chip->chip_drv->wait_idle(chip, SPI_FLASH_DEFAULT_IDLE_TIMEOUT_MS * 1000); if (ret != ESP_OK) { return ret; } diff --git a/components/spi_flash/spi_flash_os_func_app.c b/components/spi_flash/spi_flash_os_func_app.c index f6cc03874..af1ee8aa1 100644 --- a/components/spi_flash/spi_flash_os_func_app.c +++ b/components/spi_flash/spi_flash_os_func_app.c @@ -95,9 +95,9 @@ static IRAM_ATTR esp_err_t spi1_end(void *arg) #endif } -static IRAM_ATTR esp_err_t delay_ms(void *arg, unsigned ms) +static IRAM_ATTR esp_err_t delay_us(void *arg, unsigned us) { - ets_delay_us(1000 * ms); + ets_delay_us(us); return ESP_OK; } @@ -117,14 +117,14 @@ static DRAM_ATTR spi1_app_func_arg_t main_flash_arg = {}; static const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functions = { .start = spi1_start, .end = spi1_end, - .delay_ms = delay_ms, + .delay_us = delay_us, .region_protected = main_flash_region_protected, }; static const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = { .start = spi_start, .end = spi_end, - .delay_ms = delay_ms, + .delay_us = delay_us, }; static spi_bus_lock_dev_handle_t register_dev(int host_id) diff --git a/components/spi_flash/spi_flash_os_func_noos.c b/components/spi_flash/spi_flash_os_func_noos.c index b42a64edd..851a2d6ab 100644 --- a/components/spi_flash/spi_flash_os_func_noos.c +++ b/components/spi_flash/spi_flash_os_func_noos.c @@ -65,16 +65,16 @@ static IRAM_ATTR esp_err_t end(void *arg) return ESP_OK; } -static IRAM_ATTR esp_err_t delay_ms(void *arg, unsigned ms) +static IRAM_ATTR esp_err_t delay_us(void *arg, unsigned us) { - ets_delay_us(1000 * ms); + ets_delay_us(us); return ESP_OK; } const DRAM_ATTR esp_flash_os_functions_t esp_flash_noos_functions = { .start = start, .end = end, - .delay_ms = delay_ms, + .delay_us = delay_us, .region_protected = NULL, };