esp_flash: fix the set/get write protection functions

Add support for get write protection support, fixed the duplicated
set_write_protection link.

All the write_protection check in the top layer are removed. The lower
levels (chip) should ensure to disable write protection before the
operation start.

diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c
index f064bcbdb0f00dc32d36b14f627d2f29a4ab92f0..0e16fc80d2c0f9fab39845e81a67e149d0c6b3de 100644 (file)
--- a/components/spi_flash/esp_flash_api.c
+++ b/components/spi_flash/esp_flash_api.c
@@ -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);
 }

diff --git a/components/spi_flash/include/esp_flash.h b/components/spi_flash/include/esp_flash.h
index efcfccc2473a578d4de8dd4e579fab45afcc4ce1..4a7c184de43137621f9b9bae2a3f9e0c857a95be 100644 (file)
--- a/components/spi_flash/include/esp_flash.h
+++ b/components/spi_flash/include/esp_flash.h
@@ -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.
  *

diff --git a/components/spi_flash/include/spi_flash_chip_driver.h b/components/spi_flash/include/spi_flash_chip_driver.h
index fd5c1e4c9212cfb2f0a6f66345b99f3e4c387425..9dd8ffa971677cb72b46e00cde703454078f862a 100644 (file)
--- a/components/spi_flash/include/spi_flash_chip_driver.h
+++ b/components/spi_flash/include/spi_flash_chip_driver.h
@@ -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.
 

diff --git a/components/spi_flash/include/spi_flash_chip_generic.h b/components/spi_flash/include/spi_flash_chip_generic.h
index 676e173111ff94c61db0159b45c5965f71550ea0..885bd72e75d45b4d5dfae71ae779e3d390fd8377 100644 (file)
--- a/components/spi_flash/include/spi_flash_chip_generic.h
+++ b/components/spi_flash/include/spi_flash_chip_generic.h
@@ -173,7 +173,19 @@ spi_flash_chip_generic_write_encrypted(esp_flash_t *chip, const void *buffer, ui
  *      - ESP_OK if success
  *      - 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 Check whether WEL (write enable latch) bit is set in the Status Register read from RDSR (05h).
+ *
+ * @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 (05h) and wait for bit 0 (write

diff --git a/components/spi_flash/spi_flash_chip_generic.c b/components/spi_flash/spi_flash_chip_generic.c
index 0e80e8eb1d9c4a911f494b75bf4bd4e89ebad52f..8bbb9d1374a21b7e23ebd3dd80d476b124b931f8 100644 (file)
--- a/components/spi_flash/spi_flash_chip_generic.c
+++ b/components/spi_flash/spi_flash_chip_generic.c
@@ -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,
 };

diff --git a/components/spi_flash/spi_flash_chip_issi.c b/components/spi_flash/spi_flash_chip_issi.c
index d32c3a19294337181a69793fb75e9065020f76e8..d0b025637f6f2ddeec5f17800f0c30270d6cd7b1 100644 (file)
--- a/components/spi_flash/spi_flash_chip_issi.c
+++ b/components/spi_flash/spi_flash_chip_issi.c
@@ -58,8 +58,8 @@ const spi_flash_chip_t esp_flash_chip_issi = {
     .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 +73,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,
 };

diff --git a/components/spi_flash/test/test_esp_flash.c b/components/spi_flash/test/test_esp_flash.c
index 49d144c9c18ccd45f9071ffba4a19481af776949..19a93c8bf429220aef743bd79714c2546095f9f4 100644 (file)
--- a/components/spi_flash/test/test_esp_flash.c
+++ b/components/spi_flash/test/test_esp_flash.c
@@ -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