]> granicus.if.org Git - esp-idf/commitdiff
Merge branch 'fix/esp_flash_set_get_wp' into 'master'
authorAngus Gratton <angus@espressif.com>
Thu, 15 Aug 2019 22:14:48 +0000 (06:14 +0800)
committerAngus Gratton <angus@espressif.com>
Thu, 15 Aug 2019 22:14:48 +0000 (06:14 +0800)
esp_flash: fix the set/get write protection functions

See merge request espressif/esp-idf!5682

components/soc/include/hal/spi_flash_hal.h
components/spi_flash/esp_flash_api.c
components/spi_flash/include/esp_flash.h
components/spi_flash/include/memspi_host_driver.h
components/spi_flash/include/spi_flash_chip_driver.h
components/spi_flash/include/spi_flash_chip_generic.h
components/spi_flash/spi_flash_chip_generic.c
components/spi_flash/spi_flash_chip_issi.c
components/spi_flash/test/test_esp_flash.c

index 3a18e4ca5f2477068b2de22ebfad9dbc47f0e1b4..bf89c41488b90d28fd8879d603e2ce0a7ab4f0fd 100644 (file)
@@ -88,14 +88,15 @@ esp_err_t spi_flash_hal_device_config(spi_flash_host_driver_t *driver);
 esp_err_t spi_flash_hal_common_command(spi_flash_host_driver_t *driver, spi_flash_trans_t *trans);
 
 /**
- * Erase whole flash chip.
+ * Erase whole flash chip by using the erase chip (C7h) command.
  *
  * @param driver The driver context.
  */
 void spi_flash_hal_erase_chip(spi_flash_host_driver_t *driver);
 
 /**
- * Erase a specific sector by its start address.
+ * Erase a specific sector by its start address through the sector erase (20h)
+ * command.
  *
  * @param driver The driver context.
  * @param start_address Start address of the sector to erase.
@@ -103,7 +104,8 @@ void spi_flash_hal_erase_chip(spi_flash_host_driver_t *driver);
 void spi_flash_hal_erase_sector(spi_flash_host_driver_t *driver, uint32_t start_address);
 
 /**
- * Erase a specific block by its start address.
+ * Erase a specific 64KB block by its start address through the 64KB block
+ * erase (D8h) command.
  *
  * @param driver The driver context.
  * @param start_address Start address of the block to erase.
@@ -111,7 +113,7 @@ void spi_flash_hal_erase_sector(spi_flash_host_driver_t *driver, uint32_t start_
 void spi_flash_hal_erase_block(spi_flash_host_driver_t *driver, uint32_t start_address);
 
 /**
- * Program a page of the flash.
+ * Program a page of the flash using the page program (02h) command.
  *
  * @param driver The driver context.
  * @param address Address of the page to program
@@ -121,7 +123,8 @@ void spi_flash_hal_erase_block(spi_flash_host_driver_t *driver, uint32_t start_a
 void spi_flash_hal_program_page(spi_flash_host_driver_t *driver, const void *buffer, uint32_t address, uint32_t length);
 
 /**
- * Read from the flash. The read command should be set by ``spi_flash_hal_configure_host_read_mode`` before.
+ * Read from the flash. Call ``spi_flash_hal_configure_host_read_mode`` to
+ * configure the read command before calling this function.
  *
  * @param driver The driver context.
  * @param buffer Buffer to store the read data
@@ -133,7 +136,7 @@ void spi_flash_hal_program_page(spi_flash_host_driver_t *driver, const void *buf
 esp_err_t spi_flash_hal_read(spi_flash_host_driver_t *driver, void *buffer, uint32_t address, uint32_t read_len);
 
 /**
- * Enable or disable the write protection of the flash chip.
+ * @brief Send the write enable (06h) or write disable (04h) command to the flash chip.
  *
  * @param driver The driver context.
  * @param wp true to enable the write protection, otherwise false.
index f064bcbdb0f00dc32d36b14f627d2f29a4ab92f0..0e16fc80d2c0f9fab39845e81a67e149d0c6b3de 100644 (file)
@@ -265,23 +265,13 @@ esp_err_t IRAM_ATTR esp_flash_erase_chip(esp_flash_t *chip)
 {
     VERIFY_OP(erase_chip);
     CHECK_WRITE_ADDRESS(chip, 0, chip->size);
-    bool write_protect = false;
 
     esp_err_t err = spiflash_start(chip);
     if (err != ESP_OK) {
         return err;
     }
 
-    err  = esp_flash_get_chip_write_protect(chip, &write_protect);
-
-    if (err == ESP_OK && write_protect) {
-        err = ESP_ERR_FLASH_PROTECTED;
-    }
-
-    if (err == ESP_OK) {
-        err = chip->chip_drv->erase_chip(chip);
-    }
-
+    err = chip->chip_drv->erase_chip(chip);
     return spiflash_end(chip, err);
 }
 
@@ -292,7 +282,6 @@ esp_err_t IRAM_ATTR esp_flash_erase_region(esp_flash_t *chip, uint32_t start, ui
     CHECK_WRITE_ADDRESS(chip, start, len);
     uint32_t block_erase_size = chip->chip_drv->erase_block == NULL ? 0 : chip->chip_drv->block_erase_size;
     uint32_t sector_size = chip->chip_drv->sector_size;
-    bool write_protect = false;
 
     if (sector_size == 0 || (block_erase_size % sector_size) != 0) {
         return ESP_ERR_FLASH_NOT_INITIALISED;
@@ -310,16 +299,9 @@ esp_err_t IRAM_ATTR esp_flash_erase_region(esp_flash_t *chip, uint32_t start, ui
         return err;
     }
 
-    // Check for write protection on whole chip
-    if (chip->chip_drv->get_chip_write_protect != NULL) {
-        err = chip->chip_drv->get_chip_write_protect(chip, &write_protect);
-        if (err == ESP_OK && write_protect) {
-            err = ESP_ERR_FLASH_PROTECTED;
-        }
-    }
-
     // Check for write protected regions overlapping the erase region
-    if (err == ESP_OK && chip->chip_drv->get_protected_regions != NULL && chip->chip_drv->num_protectable_regions > 0) {
+    if (chip->chip_drv->get_protected_regions != NULL &&
+        chip->chip_drv->num_protectable_regions > 0) {
         uint64_t protected = 0;
         err = chip->chip_drv->get_protected_regions(chip, &protected);
         if (err == ESP_OK && protected != 0) {
@@ -360,10 +342,10 @@ esp_err_t IRAM_ATTR esp_flash_erase_region(esp_flash_t *chip, uint32_t start, ui
     return err;
 }
 
-esp_err_t IRAM_ATTR esp_flash_get_chip_write_protect(esp_flash_t *chip, bool *write_protected)
+esp_err_t IRAM_ATTR esp_flash_get_chip_write_protect(esp_flash_t *chip, bool *out_write_protected)
 {
     VERIFY_OP(get_chip_write_protect);
-    if (write_protected == NULL) {
+    if (out_write_protected == NULL) {
         return ESP_ERR_INVALID_ARG;
     }
 
@@ -372,7 +354,7 @@ esp_err_t IRAM_ATTR esp_flash_get_chip_write_protect(esp_flash_t *chip, bool *wr
         return err;
     }
 
-    err = chip->chip_drv->get_chip_write_protect(chip, write_protected);
+    err = chip->chip_drv->get_chip_write_protect(chip, out_write_protected);
 
     return spiflash_end(chip, err);
 }
index efcfccc2473a578d4de8dd4e579fab45afcc4ce1..4a7c184de43137621f9b9bae2a3f9e0c857a95be 100644 (file)
@@ -160,8 +160,6 @@ esp_err_t esp_flash_get_chip_write_protect(esp_flash_t *chip, bool *write_protec
  * @note Correct behaviour of this function depends on the SPI flash chip model and chip_drv in use (via the 'chip->drv'
  * field).
  *
- * If write protection is enabled, destructive operations will fail with ESP_ERR_FLASH_PROTECTED.
- *
  * Some SPI flash chips may require a power cycle before write protect status can be cleared. Otherwise,
  * write protection can be removed via a follow-up call to this function.
  *
index 2347398aa16e7a322ca6bca916c85ebdad7a77de..434361d9700004ccd91c5d848d1a8f279c8898d6 100644 (file)
@@ -62,6 +62,8 @@ esp_err_t memspi_host_init_pointers(spi_flash_host_driver_t *host, memspi_host_d
  ******************************************************************************/
 
 /**
+ * @brief Read the Status Register read from RDSR (05h).
+ *
  * High speed implementation of RDID through memspi interface relying on the
  * ``common_command``.
  *
index fd5c1e4c9212cfb2f0a6f66345b99f3e4c387425..9dd8ffa971677cb72b46e00cde703454078f862a 100644 (file)
@@ -90,10 +90,10 @@ struct spi_flash_chip_t {
     uint32_t block_erase_size; /* Optimal (fastest) block size for multi-sector erases on this chip */
 
     /* Read the write protect status of the entire chip. */
-    esp_err_t (*get_chip_write_protect)(esp_flash_t *chip, bool *write_protected);
+    esp_err_t (*get_chip_write_protect)(esp_flash_t *chip, bool *out_write_protected);
 
     /* Set the write protect status of the entire chip. */
-    esp_err_t (*set_chip_write_protect)(esp_flash_t *chip, bool write_protect_chip);
+    esp_err_t (*set_chip_write_protect)(esp_flash_t *chip, bool chip_write_protect);
 
     /* Number of individually write protectable regions on this chip. Range 0-63. */
     uint8_t num_protectable_regions;
@@ -135,9 +135,6 @@ struct spi_flash_chip_t {
     /* Perform an encrypted write to the chip, using internal flash encryption hardware. */
     esp_err_t (*write_encrypted)(esp_flash_t *chip, const void *buffer, uint32_t address, uint32_t length);
 
-    /* Set the write enable flag. This function is called internally by other functions in this structure, before a destructive
-       operation takes place. */
-    esp_err_t (*set_write_protect)(esp_flash_t *chip, bool write_protect);
 
     /* Wait for the SPI flash chip to be idle (any write operation to be complete.) This function is both called from the higher-level API functions, and from other functions in this structure.
 
index 676e173111ff94c61db0159b45c5965f71550ea0..672ca6550b05a119666dd7b88fe9c2f8e06b541b 100644 (file)
@@ -66,7 +66,7 @@ esp_err_t spi_flash_chip_generic_reset(esp_flash_t *chip);
 esp_err_t spi_flash_chip_generic_detect_size(esp_flash_t *chip, uint32_t *size);
 
 /**
- * @brief Erase chip by using the generic erase chip (C7h) command.
+ * @brief Erase chip by using the generic erase chip command.
  *
  * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted.
  *
@@ -77,7 +77,7 @@ esp_err_t spi_flash_chip_generic_detect_size(esp_flash_t *chip, uint32_t *size);
 esp_err_t spi_flash_chip_generic_erase_chip(esp_flash_t *chip);
 
 /**
- * @brief Erase sector by using the generic sector erase (20h) command.
+ * @brief Erase sector by using the generic sector erase command.
  *
  * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted.
  * @param start_address Start address of the sector to erase
@@ -89,7 +89,7 @@ esp_err_t spi_flash_chip_generic_erase_chip(esp_flash_t *chip);
 esp_err_t spi_flash_chip_generic_erase_sector(esp_flash_t *chip, uint32_t start_address);
 
 /**
- * @brief Erase block by using the generic 64KB block erase (D8h) command
+ * @brief Erase block by the generic 64KB block erase command
  *
  * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted.
  * @param start_address Start address of the block to erase
@@ -114,7 +114,7 @@ esp_err_t spi_flash_chip_generic_erase_block(esp_flash_t *chip, uint32_t start_a
 esp_err_t spi_flash_chip_generic_read(esp_flash_t *chip, void *buffer, uint32_t address, uint32_t length);
 
 /**
- * @brief Perform a page program using the page program (02h) command.
+ * @brief Perform a page program using the page program command.
  *
  * @note Length of each call should not excced the limitation in
  * ``chip->host->max_write_bytes``. This function is called in
@@ -163,7 +163,7 @@ esp_err_t
 spi_flash_chip_generic_write_encrypted(esp_flash_t *chip, const void *buffer, uint32_t address, uint32_t length);
 
 /**
- * @brief Send the write enable (06h) command and verify the expected bit (1) in
+ * @brief Send the write enable or write disable command and verify the expected bit (1) in
  * the status register is set.
  *
  * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted.
@@ -171,13 +171,26 @@ spi_flash_chip_generic_write_encrypted(esp_flash_t *chip, const void *buffer, ui
  *
  * @return
  *      - ESP_OK if success
- *      - or other error passed from the ``wait_idle``, ``read_status`` or ``set_write_protect`` function of host driver
+ *      - or other error passed from the ``wait_idle``, ``read_status`` or
+ *        ``set_write_protect`` function of host driver
  */
-esp_err_t spi_flash_chip_generic_write_enable(esp_flash_t *chip, bool write_protect);
+esp_err_t spi_flash_chip_generic_set_write_protect(esp_flash_t *chip, bool write_protect);
 
 /**
- * @brief Read flash status via the RDSR command (05h) and wait for bit 0 (write
- * in progress bit) to be cleared.
+ * @brief Check whether WEL (write enable latch) bit is set in the Status Register read from RDSR.
+ *
+ * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted.
+ * @param out_write_protect Output of whether the write protect is set.
+ *
+ * @return
+ *      - ESP_OK if success
+ *      - or other error passed from the ``read_status`` function of host driver
+ */
+esp_err_t spi_flash_chip_generic_get_write_protect(esp_flash_t *chip, bool *out_write_protect);
+
+/**
+ * @brief Read flash status via the RDSR command and wait for bit 0 (write in
+ * 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.
@@ -232,13 +245,15 @@ extern const spi_flash_chip_t esp_flash_chip_generic;
 esp_err_t spi_flash_generic_wait_host_idle(esp_flash_t *chip, uint32_t *timeout_ms);
 
 /**
- * @brief Utility function for set_read_mode chip_drv function
+ * @brief Utility function for set_read_mode chip_drv function. If required,
+ * set and check the QE bit in the flash chip to enable the QIO/QOUT mode.
  *
- * Most setting of read mode follows a common pattern, except for how to enable Quad I/O modes (QIO/QOUT).
- * These use different commands to read/write the status register, and a different bit is set/cleared.
+ * Most chip QE enable follows a common pattern, though commands to read/write
+ * the status register may be different, as well as the position of QE bit.
  *
- * This is a generic utility function to implement set_read_mode() for this pattern. Also configures host
- * registers via spi_flash_common_configure_host_read_mode().
+ * Registers to actually do Quad transtions and command to be sent in reading
+ * should also be configured via
+ * spi_flash_chip_generic_config_host_read_mode().
  *
  * @param qe_rdsr_command SPI flash command to read status register
  * @param qe_wrsr_command SPI flash command to write status register
@@ -250,7 +265,11 @@ esp_err_t spi_flash_generic_wait_host_idle(esp_flash_t *chip, uint32_t *timeout_
 esp_err_t spi_flash_common_set_read_mode(esp_flash_t *chip, uint8_t qe_rdsr_command, uint8_t qe_wrsr_command, uint8_t qe_sr_bitwidth, unsigned qe_sr_bit);
 
 /**
- * @brief Configure the host to use the specified read mode set in the ``chip->read_mode``.
+ * @brief Configure the host registers to use the specified read mode set in
+ *        the ``chip->read_mode``.
+ *
+ * Usually called in chip_drv read() functions before actual reading
+ * transactions. Also prepare the command to be sent in read functions.
  *
  * @param chip Pointer to SPI flash chip to use. If NULL, esp_flash_default_chip is substituted.
  *
index 0e80e8eb1d9c4a911f494b75bf4bd4e89ebad52f..8bbb9d1374a21b7e23ebd3dd80d476b124b931f8 100644 (file)
@@ -82,7 +82,7 @@ esp_err_t spi_flash_chip_generic_erase_chip(esp_flash_t *chip)
 {
     esp_err_t err;
 
-    err = chip->chip_drv->set_write_protect(chip, false);
+    err = chip->chip_drv->set_chip_write_protect(chip, false);
     if (err == ESP_OK) {
         err = chip->chip_drv->wait_idle(chip, DEFAULT_IDLE_TIMEOUT);
     }
@@ -102,7 +102,7 @@ esp_err_t spi_flash_chip_generic_erase_chip(esp_flash_t *chip)
 
 esp_err_t spi_flash_chip_generic_erase_sector(esp_flash_t *chip, uint32_t start_address)
 {
-    esp_err_t err = chip->chip_drv->set_write_protect(chip, false);
+    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);
     }
@@ -122,7 +122,7 @@ esp_err_t spi_flash_chip_generic_erase_sector(esp_flash_t *chip, uint32_t start_
 
 esp_err_t spi_flash_chip_generic_erase_block(esp_flash_t *chip, uint32_t start_address)
 {
-    esp_err_t err = chip->chip_drv->set_write_protect(chip, false);
+    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);
     }
@@ -185,7 +185,7 @@ esp_err_t spi_flash_chip_generic_write(esp_flash_t *chip, const void *buffer, ui
             page_len = page_size - (address % page_size);
         }
 
-        err = chip->chip_drv->set_write_protect(chip, false);
+        err = chip->chip_drv->set_chip_write_protect(chip, false);
 
         if (err == ESP_OK) {
             err = chip->chip_drv->program_page(chip, buffer, address, page_len);
@@ -205,7 +205,7 @@ esp_err_t spi_flash_chip_generic_write_encrypted(esp_flash_t *chip, const void *
     return ESP_ERR_FLASH_UNSUPPORTED_HOST; // TODO
 }
 
-esp_err_t spi_flash_chip_generic_write_enable(esp_flash_t *chip, bool write_protect)
+esp_err_t spi_flash_chip_generic_set_write_protect(esp_flash_t *chip, bool write_protect)
 {
     esp_err_t err = ESP_OK;
 
@@ -215,17 +215,26 @@ esp_err_t spi_flash_chip_generic_write_enable(esp_flash_t *chip, bool write_prot
         chip->host->set_write_protect(chip->host, write_protect);
     }
 
+    bool wp_read;
+    err = chip->chip_drv->get_chip_write_protect(chip, &wp_read);
+    if (err == ESP_OK && wp_read != write_protect) {
+        // WREN flag has not been set!
+        err = ESP_ERR_NOT_FOUND;
+    }
+    return err;
+}
+
+esp_err_t spi_flash_chip_generic_get_write_protect(esp_flash_t *chip, bool *out_write_protect)
+{
+    esp_err_t err = ESP_OK;
     uint8_t status;
+    assert(out_write_protect!=NULL);
     err = chip->host->read_status(chip->host, &status);
     if (err != ESP_OK) {
         return err;
     }
 
-    if ((status & SR_WREN) == 0) {
-        // WREN flag has not been set!
-        err = ESP_ERR_NOT_FOUND;
-    }
-
+    *out_write_protect = ((status & SR_WREN) == 0);
     return err;
 }
 
@@ -329,7 +338,7 @@ esp_err_t spi_flash_common_set_read_mode(esp_flash_t *chip, uint8_t qe_rdsr_comm
         ESP_EARLY_LOGV(TAG, "set_read_mode: status before 0x%x", sr);
         if ((sr & qe_sr_bit) == 0) {
             //some chips needs the write protect to be disabled before writing to Status Register
-            chip->chip_drv->set_write_protect(chip, false);
+            chip->chip_drv->set_chip_write_protect(chip, false);
 
             sr |= qe_sr_bit;
             spi_flash_trans_t t = {
@@ -354,7 +363,7 @@ esp_err_t spi_flash_common_set_read_mode(esp_flash_t *chip, uint8_t qe_rdsr_comm
                 return ESP_ERR_FLASH_NO_RESPONSE;
             }
 
-            chip->chip_drv->set_write_protect(chip, true);
+            chip->chip_drv->set_chip_write_protect(chip, true);
         }
     }
     return ESP_OK;
@@ -383,8 +392,8 @@ const spi_flash_chip_t esp_flash_chip_generic = {
     .block_erase_size = 64 * 1024,
 
     // TODO: figure out if generic chip-wide protection bits exist across some manufacturers
-    .get_chip_write_protect = NULL,
-    .set_chip_write_protect = NULL,
+    .get_chip_write_protect = spi_flash_chip_generic_get_write_protect,
+    .set_chip_write_protect = spi_flash_chip_generic_set_write_protect,
 
     // Chip write protection regions do not appear to be standardised
     // at all, this is implemented in chip-specific drivers only.
@@ -399,7 +408,6 @@ const spi_flash_chip_t esp_flash_chip_generic = {
     .page_size = 256,
     .write_encrypted = spi_flash_chip_generic_write_encrypted,
 
-    .set_write_protect = spi_flash_chip_generic_write_enable,
     .wait_idle = spi_flash_chip_generic_wait_idle,
     .set_read_mode = spi_flash_chip_generic_set_read_mode,
 };
index d32c3a19294337181a69793fb75e9065020f76e8..71684ec1956cadf2dfd6b01b3cd420bb44e2c670 100644 (file)
@@ -57,9 +57,8 @@ const spi_flash_chip_t esp_flash_chip_issi = {
     .sector_size = 4 * 1024,
     .block_erase_size = 64 * 1024,
 
-    // TODO: support get/set chip write protect for ISSI flash
-    .get_chip_write_protect = NULL,
-    .set_chip_write_protect = NULL,
+    .get_chip_write_protect = spi_flash_chip_generic_get_write_protect,
+    .set_chip_write_protect = spi_flash_chip_generic_set_write_protect,
 
     // TODO support protected regions on ISSI flash
     .num_protectable_regions = 0,
@@ -73,7 +72,6 @@ const spi_flash_chip_t esp_flash_chip_issi = {
     .page_size = 256,
     .write_encrypted = spi_flash_chip_generic_write_encrypted,
 
-    .set_write_protect = spi_flash_chip_generic_write_enable,
     .wait_idle = spi_flash_chip_generic_wait_idle,
     .set_read_mode = spi_flash_chip_issi_set_read_mode,
 };
index 49d144c9c18ccd45f9071ffba4a19481af776949..19a93c8bf429220aef743bd79714c2546095f9f4 100644 (file)
@@ -366,6 +366,36 @@ TEST_CASE("SPI flash erase large region", "[esp_flash]")
 #endif
 }
 
+static void test_write_protection(esp_flash_t* chip)
+{
+    bool wp = true;
+    esp_err_t ret = ESP_OK;
+    ret = esp_flash_get_chip_write_protect(chip, &wp);
+    TEST_ESP_OK(ret);
+
+    for (int i = 0; i < 4; i ++) {
+        bool wp_write = !wp;
+        ret = esp_flash_set_chip_write_protect(chip, wp_write);
+        TEST_ESP_OK(ret);
+
+        bool wp_read;
+        ret = esp_flash_get_chip_write_protect(chip, &wp_read);
+        TEST_ESP_OK(ret);
+        TEST_ASSERT(wp_read == wp_write);
+        wp = wp_read;
+    }
+}
+
+TEST_CASE("Test esp_flash can enable/disable write protetion", "[esp_flash]")
+{
+    test_write_protection(NULL);
+#ifndef SKIP_EXTENDED_CHIP_TEST
+    setup_new_chip(TEST_SPI_READ_MODE, TEST_SPI_SPEED);
+    test_write_protection(test_chip);
+    teardown_test_chip();
+#endif
+}
+
 static const uint8_t large_const_buffer[16400] = {
     203, // first byte
     1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
@@ -491,5 +521,4 @@ static void test_write_large_buffer(esp_flash_t *chip, const uint8_t *source, si
 
     write_large_buffer(chip, part, source, length);
     read_and_check(chip, part, source, length);
-}
-
+}
\ No newline at end of file