diff --git a/drivers/include/drivers/internal/SFDP.h b/drivers/include/drivers/internal/SFDP.h index be94b8f0bd..96f7518e01 100644 --- 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 index 1450305195..2870c0ca8e 100644 --- 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 new file mode 100644 index 0000000000..ac6915dde1 --- /dev/null +++ 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 new file mode 100644 index 0000000000..db832e9b3e --- /dev/null +++ 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 index 4bebfb5435..11c88f27e9 100644 --- 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,13 +458,10 @@ exit_point: 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 @@ -471,14 +469,14 @@ int OSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size) // 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 index e3c962f38b..92d9dd6886 100644 --- 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,13 +385,10 @@ exit_point: 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 @@ -399,14 +396,14 @@ int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size) // 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 index 870cb161b6..43ba157ed1 100644 --- a/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp @@ -25,6 +25,7 @@ #include "mbed_trace.h" #define TRACE_GROUP "SPIF" +using namespace std::chrono; using namespace mbed; /* Default SPIF Parameters */ @@ -298,17 +299,15 @@ exit_point: return status; } -int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size) +int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t size) { if (!_is_initialized) { return BD_ERROR_DEVICE_ERROR; } int type = 0; - uint32_t offset = 0; - uint32_t chunk = 4096; int cur_erase_inst = _erase_instruction; - int size = (int)in_size; + unsigned int curr_erase_size = 0; bool erase_failed = false; int status = SPIF_BD_ERROR_OK; // Find region of erased address @@ -320,14 +319,14 @@ int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size) // 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 SPIF_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 SPIF_BD_ERROR_INVALID_ERASE_PARAMS; } @@ -337,14 +336,18 @@ int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size) // iterate to find next Largest erase type ( a. supported by region, b. smaller than size) // find the matching instruction and erase size chunk for that type. - type = sfdp_iterate_next_largest_erase_type(bitfield, size, (unsigned int)addr, region, _sfdp_info.smptbl); + 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]; - offset = addr % _sfdp_info.smptbl.erase_type_size_arr[type]; - chunk = ((offset + size) < _sfdp_info.smptbl.erase_type_size_arr[type]) ? - size : (_sfdp_info.smptbl.erase_type_size_arr[type] - offset); + curr_erase_size = _sfdp_info.smptbl.erase_type_size_arr[type]; + if (addr % curr_erase_size != 0 || addr + size < curr_erase_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, curr_erase_size, type); + } - tr_debug("erase - addr: %llu, size:%d, Inst: 0x%xh, chunk: %" PRIu32 " , ", - addr, size, cur_erase_inst, chunk); + tr_debug("erase - addr: %llu, size:%llu, Inst: 0x%xh, erase size: %u", + addr, size, cur_erase_inst, curr_erase_size); tr_debug("erase - Region: %d, Type:%d", region, type); @@ -359,8 +362,8 @@ int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size) _spi_send_erase_command(cur_erase_inst, addr, size); - addr += chunk; - size -= chunk; + addr += curr_erase_size; + size -= curr_erase_size; if ((size > 0) && (addr > _sfdp_info.smptbl.region_high_boundary[region])) { // erase crossed to next region @@ -693,7 +696,7 @@ bool SPIFBlockDevice::_is_mem_ready() bool mem_ready = true; do { - rtos::ThisThread::sleep_for(1); + rtos::ThisThread::sleep_for(1ms); retries++; //Read the Status Register from device if (SPIF_BD_ERROR_OK != _spi_send_general_command(SPIF_RDSR, SPI_NO_ADDRESS_COMMAND, NULL, 0, status_value,