Merge pull request #13947 from LDong-Arm/erase_algorithm_fix

0xc0170 · web-flow · commit 61e4b55a22e8 · 2020-11-26T15:53:03.000Z
Fix erase type determination for [Q/O/]BlockDevice::erase()
diff --git a/drivers/include/drivers/internal/SFDP.h b/drivers/include/drivers/internal/SFDP.h
@@ -141,11 +141,11 @@ int sfdp_find_addr_region(bd_addr_t offset, const sfdp_hdr_info &sfdp_info);
  * @param region   Region number
  * @param smptbl   Information about different erase types
  *
- * @return Largest erase type
+ * @return Largest erase type, or -1 if none matches the given address and size
  */
-int sfdp_iterate_next_largest_erase_type(uint8_t &bitfield,
-                                         int size,
-                                         int offset,
+int sfdp_iterate_next_largest_erase_type(uint8_t bitfield,
+                                         bd_size_t size,
+                                         bd_addr_t offset,
                                          int region,
                                          const sfdp_smptbl_info &smptbl);
 
diff --git a/drivers/source/SFDP.cpp b/drivers/source/SFDP.cpp
@@ -393,34 +393,31 @@ int sfdp_find_addr_region(bd_addr_t offset, const sfdp_hdr_info &sfdp_info)
 
 }
 
-int sfdp_iterate_next_largest_erase_type(uint8_t &bitfield,
-                                         int size,
-                                         int offset,
+int sfdp_iterate_next_largest_erase_type(uint8_t bitfield,
+                                         bd_size_t size,
+                                         bd_addr_t offset,
                                          int region,
                                          const sfdp_smptbl_info &smptbl)
 {
     uint8_t type_mask = SFDP_ERASE_BITMASK_TYPE4;
-    int largest_erase_type = 0;
-
-    int idx;
-    for (idx = 3; idx >= 0; idx--) {
+    unsigned int erase_size;
+    for (int idx = 3; idx >= 0; idx--) {
         if (bitfield & type_mask) {
-            largest_erase_type = idx;
-            if ((size > (int)(smptbl.erase_type_size_arr[largest_erase_type])) &&
-                    ((smptbl.region_high_boundary[region] - offset)
-                     > (uint64_t)(smptbl.erase_type_size_arr[largest_erase_type]))) {
-                break;
-            } else {
-                bitfield &= ~type_mask;
+            erase_size = smptbl.erase_type_size_arr[idx];
+            // Criteria:
+            // * offset is aligned to the type's erase size
+            // * erase size is no larger than the requested size,
+            // * erase range does not exceed the region boundary
+            if ((offset % erase_size == 0) && (size >= erase_size) &&
+                    (offset + erase_size - 1 <= smptbl.region_high_boundary[region])) {
+                return idx;
             }
         }
         type_mask = type_mask >> 1;
     }
 
-    if (idx == -1) {
-        tr_error("No erase type was found for current region addr");
-    }
-    return largest_erase_type;
+    tr_error("No erase type was found for current region addr");
+    return -1;
 }
 
 int sfdp_detect_device_density(uint8_t *bptbl_ptr, sfdp_bptbl_info &bptbl_info)
diff --git a/drivers/tests/UNITTESTS/SFDP/test_sfdp.cpp b/drivers/tests/UNITTESTS/SFDP/test_sfdp.cpp
@@ -0,0 +1,101 @@
+/* Copyright (c) 2020 ARM Limited
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+#include "drivers/internal/SFDP.h"
+
+class TestSFDP : public testing::Test {
+protected:
+    struct mbed::sfdp_smptbl_info smptbl;
+
+    /**
+     * Construct Mbed OS SFDP info.
+     * Normally this is parsed from the flash-chips's
+     * raw SFDP table bytes, but for unit test we construct
+     * SFDP info manually
+     */
+    virtual void SetUp()
+    {
+        // The mock flash supports 4KB, 32KB and 64KB erase types
+        smptbl.erase_type_size_arr[0] = 4 * 1024;
+        smptbl.erase_type_size_arr[1] = 32 * 1024;
+        smptbl.erase_type_size_arr[2] = 64 * 1024;
+
+        // The mock flash has three regions, with address ranges:
+        // * 0 to 64KB - 1B
+        // * 64KB to 256KB - 1B
+        // * 256KB to 1024KB - 1B
+        smptbl.region_high_boundary[0] = 64 * 1024 - 1;
+        smptbl.region_high_boundary[1] = 256 * 1024 - 1;
+        smptbl.region_high_boundary[2] = 1024 * 1024 - 1;
+
+        // Bitfields indicating which regions support which erase types
+        smptbl.region_erase_types_bitfld[0] = 0b0001;   // 4KB only
+        smptbl.region_erase_types_bitfld[1] = 0b0111;   // 64KB, 32KB, 4KB
+        smptbl.region_erase_types_bitfld[2] = 0b0110;   // 64KB, 32KB
+    }
+};
+
+/**
+ * Test if sfdp_iterate_next_largest_erase_type() returns the most
+ * optimal erase type, whose erase size is as large as possible
+ * while being compatible with both alignment and size requirements.
+ */
+TEST_F(TestSFDP, TestEraseTypeAlgorithm)
+{
+    // Erase 104KB starting at 92KB
+    int address = 92 * 1024;
+    int size = 104 * 1024;
+    int region = 1;
+    int type;
+
+    // Expected outcome:
+    // * The starting position 92KB is 4KB-aligned
+    // * The next position 96KB (92KB + 4KB) is 32KB-aligned
+    // * The next position 128KB (96KB + 32KB) is 64KB-aligned
+    // * At the final position 192KB (128KB + 64KB), we only
+    //   have 4KB (104KB - 4KB - 32KB - 64KB) remaining to erase
+    int expected_erase_KBs[] = {4, 32, 64, 4};
+
+    for (int i = 0; i < sizeof(expected_erase_KBs) / sizeof(int); i++) {
+        type = sfdp_iterate_next_largest_erase_type(
+                    smptbl.region_erase_types_bitfld[region],
+                    size,
+                    address,
+                    region,
+                    smptbl);
+        int erase_size = smptbl.erase_type_size_arr[type];
+        EXPECT_EQ(erase_size, expected_erase_KBs[i] * 1024);
+        address += erase_size;
+        size -= erase_size;
+    }
+
+    EXPECT_EQ(size, 0); // All erased
+
+    // Test unaligned erase
+    // Region 2 only allows at least 32KB-aligned erases
+    address = (512 + 16) * 1024;
+    size = 64 * 1024;
+    region = 2;
+    type = sfdp_iterate_next_largest_erase_type(
+                smptbl.region_erase_types_bitfld[region],
+                size,
+                address,
+                region,
+                smptbl);
+    EXPECT_EQ(type, -1); // Invalid erase
+}
diff --git a/drivers/tests/UNITTESTS/SFDP/unittest.cmake b/drivers/tests/UNITTESTS/SFDP/unittest.cmake
@@ -0,0 +1,20 @@
+
+####################
+# UNIT TESTS
+####################
+set(TEST_SUITE_NAME "SFDP")
+
+# Source files
+set(unittest-sources
+  ../drivers/source/SFDP.cpp
+)
+
+# Test files
+set(unittest-test-sources
+  ../drivers/tests/UNITTESTS/SFDP/test_sfdp.cpp
+  stubs/mbed_assert_stub.cpp
+)
+
+set(unittest-test-flags
+  -DDEVICE_SPI
+)
diff --git a/storage/blockdevice/COMPONENT_OSPIF/source/OSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_OSPIF/source/OSPIFBlockDevice.cpp
@@ -31,6 +31,7 @@
 #include "mbed_trace.h"
 #define TRACE_GROUP "OSPIF"
 
+using namespace std::chrono;
 using namespace mbed;
 
 /* Default OSPIF Parameters */
@@ -409,7 +410,7 @@ int OSPIFBlockDevice::program(const void *buffer, bd_addr_t addr, bd_size_t size
     uint32_t chunk = 0;
     bd_size_t written_bytes = 0;
 
-    tr_debug("Program - Buff: 0x%lxh, addr: %llu, size: %llu", (uint32_t)buffer, addr, size);
+    tr_debug("Program - Buff: %p, addr: %llu, size: %llu", buffer, addr, size);
 
     while (size > 0) {
         // Write on _page_size_bytes boundaries (Default 256 bytes a page)
@@ -457,28 +458,25 @@ int OSPIFBlockDevice::program(const void *buffer, bd_addr_t addr, bd_size_t size
     return status;
 }
 
-int OSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
+int OSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t size)
 {
     int type = 0;
-    uint32_t offset = 0;
-    uint32_t chunk = 4096;
     ospi_inst_t cur_erase_inst = OSPI_NO_INST;
-    int size = (int)in_size;
     bool erase_failed = false;
     int status = OSPIF_BD_ERROR_OK;
     // Find region of erased address
     int region = sfdp_find_addr_region(addr, _sfdp_info);
     // Erase Types of selected region
     uint8_t bitfield = _sfdp_info.smptbl.region_erase_types_bitfld[region];
 
-    tr_debug("Erase - addr: %llu, in_size: %llu", addr, in_size);
+    tr_debug("Erase - addr: %llu, size: %llu", addr, size);
 
-    if ((addr + in_size) > _sfdp_info.bptbl.device_size_bytes) {
+    if ((addr + size) > _sfdp_info.bptbl.device_size_bytes) {
         tr_error("Erase exceeds flash device size");
         return OSPIF_BD_ERROR_INVALID_ERASE_PARAMS;
     }
 
-    if (((addr % get_erase_size(addr)) != 0) || (((addr + in_size) % get_erase_size(addr + in_size - 1)) != 0)) {
+    if (((addr % get_erase_size(addr)) != 0) || (((addr + size) % get_erase_size(addr + size - 1)) != 0)) {
         tr_error("Invalid erase - unaligned address and size");
         return OSPIF_BD_ERROR_INVALID_ERASE_PARAMS;
     }
@@ -500,11 +498,16 @@ int OSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
             cur_erase_inst = _sfdp_info.bptbl.legacy_erase_instruction;
             eu_size = OSPIF_DEFAULT_SE_SIZE;
         }
-        offset = addr % eu_size;
-        chunk = ((offset + size) < eu_size) ? size : (eu_size - offset);
 
-        tr_debug("Erase - addr: %llu, size:%d, Inst: 0x%xh, chunk: %lu ",
-                 addr, size, cur_erase_inst, chunk);
+        if (addr % eu_size != 0 || addr + size < eu_size) {
+            // Should not happen if the erase table parsing
+            // and alignment checks were performed correctly
+            tr_error("internal error: address %llu not aligned to erase size %u (type %d)",
+                     addr, eu_size, type);
+        }
+
+        tr_debug("Erase - addr: %llu, size:%llu, Inst: 0x%xh, erase size: %u ",
+                 addr, size, cur_erase_inst, eu_size);
         tr_debug("Erase - Region: %d, Type:%d ",
                  region, type);
 
@@ -524,8 +527,8 @@ int OSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
             goto exit_point;
         }
 
-        addr += chunk;
-        size -= chunk;
+        addr += eu_size;
+        size -= eu_size;
 
         if ((size > 0) && (addr > _sfdp_info.smptbl.region_high_boundary[region])) {
             // erase crossed to next region
@@ -1517,7 +1520,7 @@ bool OSPIFBlockDevice::_is_mem_ready()
     bool mem_ready = true;
 
     do {
-        rtos::ThisThread::sleep_for(1);
+        rtos::ThisThread::sleep_for(1ms);
         retries++;
         //Read Status Register 1 from device, the number of read byte need to be even in octa flash DOPI mode
         if (OSPI_STATUS_OK != _ospi_send_general_command(OSPIF_INST_RSR1, OSPI_NO_ADDRESS_COMMAND,
@@ -1664,7 +1667,7 @@ ospi_status_t OSPIFBlockDevice::_ospi_send_general_command(ospi_inst_t instructi
                 (instruction == OSPIF_INST_RDCR2) || (instruction == OSPIF_INST_RDCR)) {
             _ospi.configure_format(_inst_width, _inst_size, _address_width, _address_size, OSPI_CFG_BUS_SINGLE, 0, _data_width, _dummy_cycles);
             addr = 0;
-        } else if ((instruction == OSPIF_INST_WSR1)) {
+        } else if (instruction == OSPIF_INST_WSR1) {
             addr = 0;
         }
     }
diff --git a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp
@@ -338,7 +338,7 @@ int QSPIFBlockDevice::program(const void *buffer, bd_addr_t addr, bd_size_t size
     uint32_t chunk = 0;
     bd_size_t written_bytes = 0;
 
-    tr_debug("Program - Buff: 0x%lxh, addr: %llu, size: %llu", (uint32_t)buffer, addr, size);
+    tr_debug("Program - Buff: %p, addr: %llu, size: %llu", buffer, addr, size);
 
     while (size > 0) {
         // Write on _page_size_bytes boundaries (Default 256 bytes a page)
@@ -385,28 +385,25 @@ int QSPIFBlockDevice::program(const void *buffer, bd_addr_t addr, bd_size_t size
     return status;
 }
 
-int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
+int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t size)
 {
     int type = 0;
-    uint32_t offset = 0;
-    uint32_t chunk = 4096;
     qspi_inst_t cur_erase_inst = QSPI_NO_INST;
-    int size = (int)in_size;
     bool erase_failed = false;
     int status = QSPIF_BD_ERROR_OK;
     // Find region of erased address
     int region = sfdp_find_addr_region(addr, _sfdp_info);
     // Erase Types of selected region
     uint8_t bitfield = _sfdp_info.smptbl.region_erase_types_bitfld[region];
 
-    tr_debug("Erase - addr: %llu, in_size: %llu", addr, in_size);
+    tr_debug("Erase - addr: %llu, size: %llu", addr, size);
 
-    if ((addr + in_size) > _sfdp_info.bptbl.device_size_bytes) {
+    if ((addr + size) > _sfdp_info.bptbl.device_size_bytes) {
         tr_error("Erase exceeds flash device size");
         return QSPIF_BD_ERROR_INVALID_ERASE_PARAMS;
     }
 
-    if (((addr % get_erase_size(addr)) != 0) || (((addr + in_size) % get_erase_size(addr + in_size - 1)) != 0)) {
+    if (((addr % get_erase_size(addr)) != 0) || (((addr + size) % get_erase_size(addr + size - 1)) != 0)) {
         tr_error("Invalid erase - unaligned address and size");
         return QSPIF_BD_ERROR_INVALID_ERASE_PARAMS;
     }
@@ -417,7 +414,7 @@ int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
         if (_sfdp_info.bptbl.legacy_erase_instruction == QSPI_NO_INST) {
             // Iterate to find next largest erase type that is a) supported by region, and b) smaller than size.
             // Find the matching instruction and erase size chunk for that type.
-            type = sfdp_iterate_next_largest_erase_type(bitfield, size, (int)addr,
+            type = sfdp_iterate_next_largest_erase_type(bitfield, size, addr,
                                                         region,
                                                         _sfdp_info.smptbl);
             cur_erase_inst = _sfdp_info.smptbl.erase_type_inst_arr[type];
@@ -427,11 +424,16 @@ int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
             cur_erase_inst = _sfdp_info.bptbl.legacy_erase_instruction;
             eu_size = QSPIF_DEFAULT_SE_SIZE;
         }
-        offset = addr % eu_size;
-        chunk = ((offset + size) < eu_size) ? size : (eu_size - offset);
 
-        tr_debug("Erase - addr: %llu, size:%d, Inst: 0x%xh, chunk: %lu ",
-                 addr, size, cur_erase_inst, chunk);
+        if (addr % eu_size != 0 || addr + size < eu_size) {
+            // Should not happen if the erase table parsing
+            // and alignment checks were performed correctly
+            tr_error("internal error: address %llu not aligned to erase size %u (type %d)",
+                     addr, eu_size, type);
+        }
+
+        tr_debug("Erase - addr: %llu, size:%llu, Inst: 0x%xh, erase size: %u",
+                 addr, size, cur_erase_inst, eu_size);
         tr_debug("Erase - Region: %d, Type:%d ",
                  region, type);
 
@@ -451,8 +453,8 @@ int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
             goto exit_point;
         }
 
-        addr += chunk;
-        size -= chunk;
+        addr += eu_size;
+        size -= eu_size;
 
         if ((size > 0) && (addr > _sfdp_info.smptbl.region_high_boundary[region])) {
             // erase crossed to next region
diff --git a/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp
