From 7a8ff5f1151fb1664c02766f44f8072111e09b8b Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Tue, 17 Aug 2021 14:58:50 +0100 Subject: [PATCH 01/12] drivers: Add missing OSPI.cpp to CMakeLists.txt --- drivers/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/CMakeLists.txt b/drivers/CMakeLists.txt index 256ff5b5584..a0b5feab063 100644 --- a/drivers/CMakeLists.txt +++ b/drivers/CMakeLists.txt @@ -34,6 +34,7 @@ target_sources(mbed-core source/I2CSlave.cpp source/InterruptIn.cpp source/MbedCRC.cpp + source/OSPI.cpp source/PortIn.cpp source/PortInOut.cpp source/PortOut.cpp From 258125449270af8124fb9b05a303eb2e6d63e907 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 8 Sep 2021 17:36:57 +0100 Subject: [PATCH 02/12] SFDP: Fix sector map table allocation check When passing an allocation size directly to `std::make_unique`, `std::nothrow` is unavailable, so any failed allocation results in an exception which we cannot catch because Mbed OS is compiled with `-fno-exceptions`. To fix this, chain `std::make_unique` with `new (std::nothrow)`, and the allocated `unique_ptr` retains the `nullptr` property. --- storage/blockdevice/source/SFDP.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/blockdevice/source/SFDP.cpp b/storage/blockdevice/source/SFDP.cpp index 3b5ac248f9f..5f287386955 100644 --- a/storage/blockdevice/source/SFDP.cpp +++ b/storage/blockdevice/source/SFDP.cpp @@ -256,7 +256,7 @@ int sfdp_parse_sector_map_table(Callback sfdp * - regions in each configuration * is variable -> the size of this table is variable */ - auto smptbl_buff = std::make_unique(sfdp_info.smptbl.size); + auto smptbl_buff = std::unique_ptr(new (std::nothrow) uint8_t[sfdp_info.smptbl.size]); if (!smptbl_buff) { tr_error("Failed to allocate memory"); return -1; From b5e7dd9d32a21a1f68e84ca7837b0975209b0cff Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Fri, 6 Aug 2021 11:52:15 +0100 Subject: [PATCH 03/12] SFDP: Add more parameters to the reader callback The SFDP functions parse SFDP data which is fetched by a callback called `sfdp_reader` provided by {SPIF,QSPIF,OSPIF}BlockDevice. Currently, this callback interface only takes a read address and an RX buffer to store output data. This has been enough, because other SPI parameters are always the same when fetching the SFDP table only - they are just hardcoded in each reader. But in the future we will add support for flash devices with multiple configurations (in a subsequent commit), and to detect which configuration is enabled, we will need to send detection commands which require device-dependent SPI parameters: * address size * instruction * dummy cycles This commit * turns the above SPI parameters from predefined/hardcoded values into parameters of the callback * lets the SFDP functions pass the above parameters to the callback (Note: To read the SFDP table itself, those values are constants defined by the standard, not tied to any particular device, so they can be known to the SFDP functions) * updates the callbacks implemented by {SPIF,QSPIF,OSPIF}BlockDevice * updates the mock callback for unit tests and expectations --- .../include/OSPIF/OSPIFBlockDevice.h | 8 ++- .../source/OSPIFBlockDevice.cpp | 57 +++++++++++++++--- .../include/QSPIF/QSPIFBlockDevice.h | 6 +- .../source/QSPIFBlockDevice.cpp | 59 +++++++++++++++---- .../include/SPIF/SPIFBlockDevice.h | 6 +- .../COMPONENT_SPIF/source/SPIFBlockDevice.cpp | 42 ++++++++++--- .../include/blockdevice/internal/SFDP.h | 20 ++++++- storage/blockdevice/source/SFDP.cpp | 32 ++++++++-- .../tests/UNITTESTS/SFDP/test_sfdp.cpp | 52 +++++++++++----- 9 files changed, 227 insertions(+), 55 deletions(-) diff --git a/storage/blockdevice/COMPONENT_OSPIF/include/OSPIF/OSPIFBlockDevice.h b/storage/blockdevice/COMPONENT_OSPIF/include/OSPIF/OSPIFBlockDevice.h index 2b0799dd8eb..801fe74a02f 100644 --- a/storage/blockdevice/COMPONENT_OSPIF/include/OSPIF/OSPIFBlockDevice.h +++ b/storage/blockdevice/COMPONENT_OSPIF/include/OSPIF/OSPIFBlockDevice.h @@ -327,7 +327,9 @@ class OSPIFBlockDevice : public mbed::BlockDevice { mbed::bd_size_t tx_length, const char *rx_buffer, mbed::bd_size_t rx_length); // Send command to read from the SFDP table - int _ospi_send_read_sfdp_command(mbed::bd_addr_t addr, void *rx_buffer, mbed::bd_size_t rx_length); + int _ospi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, + uint8_t inst, uint8_t dummy_cycles, + void *rx_buffer, mbed::bd_size_t rx_length); // Read the contents of status registers 1 and 2 into a buffer (buffer must have a length of 2) ospi_status_t _ospi_read_status_registers(uint8_t *reg_buffer); @@ -366,11 +368,11 @@ class OSPIFBlockDevice : public mbed::BlockDevice { /* SFDP Detection and Parsing Functions */ /****************************************/ // Parse and Detect required Basic Parameters from Table - int _sfdp_parse_basic_param_table(mbed::Callback sfdp_reader, + int _sfdp_parse_basic_param_table(mbed::Callback sfdp_reader, mbed::sfdp_hdr_info &sfdp_info); // Parse and Detect 4-Byte Address Instruction Parameters from Table - int _sfdp_parse_4_byte_inst_table(mbed::Callback sfdp_reader, + int _sfdp_parse_4_byte_inst_table(mbed::Callback sfdp_reader, mbed::sfdp_hdr_info &sfdp_info); // Detect the soft reset protocol and reset - returns error if soft reset is not supported diff --git a/storage/blockdevice/COMPONENT_OSPIF/source/OSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_OSPIF/source/OSPIFBlockDevice.cpp index f6857e6ca77..a0954b394ce 100644 --- a/storage/blockdevice/COMPONENT_OSPIF/source/OSPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_OSPIF/source/OSPIFBlockDevice.cpp @@ -921,12 +921,19 @@ int OSPIFBlockDevice::remove_csel_instance(PinName csel) /*********************************************************/ /********** SFDP Parsing and Detection Functions *********/ /*********************************************************/ -int OSPIFBlockDevice::_sfdp_parse_basic_param_table(Callback sfdp_reader, +int OSPIFBlockDevice::_sfdp_parse_basic_param_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) { uint8_t param_table[SFDP_BASIC_PARAMS_TBL_SIZE]; /* Up To 20 DWORDS = 80 Bytes */ - int status = sfdp_reader(sfdp_info.bptbl.addr, param_table, sfdp_info.bptbl.size); + int status = sfdp_reader( + sfdp_info.bptbl.addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + param_table, + sfdp_info.bptbl.size + ); if (status != OSPI_STATUS_OK) { tr_error("Init - Read SFDP First Table Failed"); return -1; @@ -1383,12 +1390,19 @@ int OSPIFBlockDevice::_sfdp_detect_reset_protocol_and_reset(uint8_t *basic_param return status; } -int OSPIFBlockDevice::_sfdp_parse_4_byte_inst_table(Callback sfdp_reader, +int OSPIFBlockDevice::_sfdp_parse_4_byte_inst_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) { uint8_t four_byte_inst_table[SFDP_DEFAULT_4_BYTE_INST_TABLE_SIZE_BYTES]; /* Up To 2 DWORDS = 8 Bytes */ - int status = sfdp_reader(sfdp_info.fbatbl.addr, four_byte_inst_table, sfdp_info.fbatbl.size); + int status = sfdp_reader( + sfdp_info.fbatbl.addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + four_byte_inst_table, + sfdp_info.fbatbl.size + ); if (status != OSPI_STATUS_OK) { tr_error("Init - Read SFDP Four Byte Inst Table Failed"); return -1; @@ -1823,13 +1837,39 @@ ospi_status_t OSPIFBlockDevice::_ospi_send_general_command(ospi_inst_t instructi return OSPI_STATUS_OK; } -int OSPIFBlockDevice::_ospi_send_read_sfdp_command(bd_addr_t addr, void *rx_buffer, bd_size_t rx_length) +int OSPIFBlockDevice::_ospi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, + uint8_t inst, uint8_t dummy_cycles, + void *rx_buffer, mbed::bd_size_t rx_length) { - size_t rx_len = rx_length; uint8_t *rx_buffer_tmp = (uint8_t *)rx_buffer; + // Set default here to avoid uninitialized variable warning + ospi_address_size_t address_size = _address_size; + int address = addr; + switch (addr_size) { + case SFDP_CMD_ADDR_3_BYTE: + address_size = OSPI_CFG_ADDR_SIZE_24; + break; + case SFDP_CMD_ADDR_4_BYTE: + address_size = OSPI_CFG_ADDR_SIZE_32; + break; + case SFDP_CMD_ADDR_SIZE_VARIABLE: // use current setting + break; + case SFDP_CMD_ADDR_NONE: // no address in command + address = static_cast(OSPI_NO_ADDRESS_COMMAND); + break; + default: + tr_error("Invalid SFDP command address size: 0x%02X", addr_size); + return -1; + } + + if (dummy_cycles == SFDP_CMD_DUMMY_CYCLES_VARIABLE) { + // use current setting + dummy_cycles = _dummy_cycles; + } + // SFDP read instruction requires 1-1-1 bus mode with 8 dummy cycles and a 3-byte address - ospi_status_t status = _ospi.configure_format(OSPI_CFG_BUS_SINGLE, OSPI_CFG_INST_SIZE_8, OSPI_CFG_BUS_SINGLE, OSPI_CFG_ADDR_SIZE_24, OSPI_CFG_BUS_SINGLE, 0, OSPI_CFG_BUS_SINGLE, OSPIF_RSFDP_DUMMY_CYCLES); + ospi_status_t status = _ospi.configure_format(OSPI_CFG_BUS_SINGLE, OSPI_CFG_INST_SIZE_8, OSPI_CFG_BUS_SINGLE, address_size, OSPI_CFG_BUS_SINGLE, 0, OSPI_CFG_BUS_SINGLE, dummy_cycles); if (OSPI_STATUS_OK != status) { tr_error("_ospi_configure_format failed"); return status; @@ -1837,7 +1877,8 @@ int OSPIFBlockDevice::_ospi_send_read_sfdp_command(bd_addr_t addr, void *rx_buff // Don't check the read status until after we've configured the format back to 1-1-1, to avoid leaving the interface in an // incorrect state if the read fails. - status = _ospi.read(OSPIF_INST_RSFDP, -1, (unsigned int) addr, (char *) rx_buffer, &rx_len); + size_t rx_len = rx_length; + status = _ospi.read(inst, -1, address, static_cast(rx_buffer), &rx_len); ospi_status_t format_status = _ospi.configure_format(OSPI_CFG_BUS_SINGLE, OSPI_CFG_INST_SIZE_8, OSPI_CFG_BUS_SINGLE, _address_size, OSPI_CFG_BUS_SINGLE, 0, OSPI_CFG_BUS_SINGLE, 0); // All commands other than Read and RSFDP use default 1-1-1 bus mode (Program/Erase are constrained by flash memory performance more than bus performance) diff --git a/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h b/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h index 1aae02bf4cf..8bf4c6cccf0 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h +++ b/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h @@ -277,7 +277,9 @@ class QSPIFBlockDevice : public mbed::BlockDevice { mbed::bd_size_t tx_length, const char *rx_buffer, mbed::bd_size_t rx_length); // Send command to read from the SFDP table - int _qspi_send_read_sfdp_command(mbed::bd_addr_t addr, void *rx_buffer, mbed::bd_size_t rx_length); + int _qspi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, + uint8_t inst, uint8_t dummy_cycles, + void *rx_buffer, mbed::bd_size_t rx_length); // Read the contents of status registers 1 and 2 into a buffer (buffer must have a length of 2) qspi_status_t _qspi_read_status_registers(uint8_t *reg_buffer); @@ -313,7 +315,7 @@ class QSPIFBlockDevice : public mbed::BlockDevice { /* SFDP Detection and Parsing Functions */ /****************************************/ // Parse and Detect required Basic Parameters from Table - int _sfdp_parse_basic_param_table(mbed::Callback sfdp_reader, + int _sfdp_parse_basic_param_table(mbed::Callback sfdp_reader, mbed::sfdp_hdr_info &sfdp_info); // Detect the soft reset protocol and reset - returns error if soft reset is not supported diff --git a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp index e07a5217db0..d8d14224025 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp @@ -614,12 +614,19 @@ int QSPIFBlockDevice::remove_csel_instance(PinName csel) /*********************************************************/ /********** SFDP Parsing and Detection Functions *********/ /*********************************************************/ -int QSPIFBlockDevice::_sfdp_parse_basic_param_table(Callback sfdp_reader, +int QSPIFBlockDevice::_sfdp_parse_basic_param_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) { uint8_t param_table[SFDP_BASIC_PARAMS_TBL_SIZE]; /* Up To 20 DWORDS = 80 Bytes */ - int status = sfdp_reader(sfdp_info.bptbl.addr, param_table, sfdp_info.bptbl.size); + int status = sfdp_reader( + sfdp_info.bptbl.addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + param_table, + sfdp_info.bptbl.size + ); if (status != QSPI_STATUS_OK) { tr_error("Init - Read SFDP First Table Failed"); return -1; @@ -1405,23 +1412,53 @@ qspi_status_t QSPIFBlockDevice::_qspi_send_general_command(qspi_inst_t instructi return QSPI_STATUS_OK; } -int QSPIFBlockDevice::_qspi_send_read_sfdp_command(bd_addr_t addr, void *rx_buffer, bd_size_t rx_length) +int QSPIFBlockDevice::_qspi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, + uint8_t inst, uint8_t dummy_cycles, + void *rx_buffer, mbed::bd_size_t rx_length) { - size_t rx_len = rx_length; + // Set default here to avoid uninitialized variable warning + qspi_address_size_t address_size = _address_size; + int address = addr; + switch (addr_size) { + case SFDP_CMD_ADDR_3_BYTE: + address_size = QSPI_CFG_ADDR_SIZE_24; + break; + case SFDP_CMD_ADDR_4_BYTE: + address_size = QSPI_CFG_ADDR_SIZE_32; + break; + case SFDP_CMD_ADDR_SIZE_VARIABLE: // use current setting + break; + case SFDP_CMD_ADDR_NONE: // no address in command + address = static_cast(QSPI_NO_ADDRESS_COMMAND); + break; + default: + tr_error("Invalid SFDP command address size: 0x%02X", addr_size); + return -1; + } - // SFDP read instruction requires 1-1-1 bus mode with 8 dummy cycles and a 3-byte address - qspi_status_t status = _qspi.configure_format(QSPI_CFG_BUS_SINGLE, QSPI_CFG_BUS_SINGLE, QSPI_CFG_ADDR_SIZE_24, QSPI_CFG_BUS_SINGLE, 0, QSPI_CFG_BUS_SINGLE, QSPIF_RSFDP_DUMMY_CYCLES); + if (dummy_cycles == SFDP_CMD_DUMMY_CYCLES_VARIABLE) { + // use current setting + dummy_cycles = _dummy_cycles; + } + + qspi_status_t status = _qspi.configure_format(QSPI_CFG_BUS_SINGLE, QSPI_CFG_BUS_SINGLE, + address_size, QSPI_CFG_BUS_SINGLE, + 0, QSPI_CFG_BUS_SINGLE, dummy_cycles); if (QSPI_STATUS_OK != status) { tr_error("_qspi_configure_format failed"); return status; } - // Don't check the read status until after we've configured the format back to 1-1-1, to avoid leaving the interface in an - // incorrect state if the read fails. - status = _qspi.read(QSPIF_INST_RSFDP, -1, (unsigned int) addr, (char *) rx_buffer, &rx_len); + // Don't check the read status until after we've configured the format back to 1-1-1, + // to avoid leaving the interface in an incorrect state if the read fails. + size_t rx_len = rx_length; + status = _qspi.read(inst, -1, address, static_cast(rx_buffer), &rx_len); - qspi_status_t format_status = _qspi.configure_format(QSPI_CFG_BUS_SINGLE, QSPI_CFG_BUS_SINGLE, _address_size, QSPI_CFG_BUS_SINGLE, 0, QSPI_CFG_BUS_SINGLE, 0); - // All commands other than Read and RSFDP use default 1-1-1 bus mode (Program/Erase are constrained by flash memory performance more than bus performance) + qspi_status_t format_status = _qspi.configure_format(QSPI_CFG_BUS_SINGLE, QSPI_CFG_BUS_SINGLE, + address_size, QSPI_CFG_BUS_SINGLE, + 0, QSPI_CFG_BUS_SINGLE, 0); + // All commands other than Read and RSFDP use default 1-1-1 bus mode + // (Program/Erase are constrained by flash memory performance more than bus performance) if (QSPI_STATUS_OK != format_status) { tr_error("_qspi_configure_format failed"); return format_status; diff --git a/storage/blockdevice/COMPONENT_SPIF/include/SPIF/SPIFBlockDevice.h b/storage/blockdevice/COMPONENT_SPIF/include/SPIF/SPIFBlockDevice.h index c68c4575361..a0a87ab78b2 100644 --- a/storage/blockdevice/COMPONENT_SPIF/include/SPIF/SPIFBlockDevice.h +++ b/storage/blockdevice/COMPONENT_SPIF/include/SPIF/SPIFBlockDevice.h @@ -221,10 +221,12 @@ class SPIFBlockDevice : public mbed::BlockDevice { /* SFDP Detection and Parsing Functions */ /****************************************/ // Send SFDP Read command to Driver - int _spi_send_read_sfdp_command(mbed::bd_addr_t addr, void *rx_buffer, mbed::bd_size_t rx_length); + int _spi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, + uint8_t inst, uint8_t dummy_cycles, + void *rx_buffer, mbed::bd_size_t rx_length); // Parse and Detect required Basic Parameters from Table - int _sfdp_parse_basic_param_table(mbed::Callback sfdp_reader, + int _sfdp_parse_basic_param_table(mbed::Callback sfdp_reader, mbed::sfdp_hdr_info &hdr_info); // Detect fastest read Bus mode supported by device diff --git a/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp index 823ed2ae3d9..bdbcae9e383 100644 --- a/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_SPIF/source/SPIFBlockDevice.cpp @@ -497,13 +497,32 @@ spif_bd_error SPIFBlockDevice::_spi_send_read_command(int read_inst, uint8_t *bu return SPIF_BD_ERROR_OK; } -int SPIFBlockDevice::_spi_send_read_sfdp_command(bd_addr_t addr, void *rx_buffer, bd_size_t rx_length) +int SPIFBlockDevice::_spi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, + uint8_t inst, uint8_t dummy_cycles, + void *rx_buffer, mbed::bd_size_t rx_length) { - // Set 1-1-1 bus mode for SFDP header parsing - // Initial SFDP read tables are read with 8 dummy cycles - _dummy_and_mode_cycles = 8; + switch (addr_size) { + case SFDP_CMD_ADDR_3_BYTE: + _address_size = SPIF_ADDR_SIZE_3_BYTES; + break; + case SFDP_CMD_ADDR_4_BYTE: + _address_size = SPIF_ADDR_SIZE_4_BYTES; + break; + case SFDP_CMD_ADDR_SIZE_VARIABLE: // use current setting + break; + case SFDP_CMD_ADDR_NONE: // no address in command + addr = static_cast(SPI_NO_ADDRESS_COMMAND); + break; + default: + tr_error("Invalid SFDP command address size: 0x%02X", addr_size); + return -1; + } + + if (dummy_cycles != SFDP_CMD_DUMMY_CYCLES_VARIABLE) { + _dummy_and_mode_cycles = dummy_cycles; + } - int status = _spi_send_read_command(SPIF_SFDP, (uint8_t *)rx_buffer, addr, rx_length); + int status = _spi_send_read_command(inst, static_cast(rx_buffer), addr, rx_length); if (status < 0) { tr_error("_spi_send_read_sfdp_command failed"); } @@ -588,12 +607,19 @@ spif_bd_error SPIFBlockDevice::_spi_send_general_command(int instruction, bd_add /*********************************************************/ /********** SFDP Parsing and Detection Functions *********/ /*********************************************************/ -int SPIFBlockDevice::_sfdp_parse_basic_param_table(Callback sfdp_reader, - mbed::sfdp_hdr_info &sfdp_info) +int SPIFBlockDevice::_sfdp_parse_basic_param_table(Callback sfdp_reader, + sfdp_hdr_info &sfdp_info) { uint8_t param_table[SFDP_BASIC_PARAMS_TBL_SIZE]; /* Up To 20 DWORDS = 80 Bytes */ - int status = sfdp_reader(sfdp_info.bptbl.addr, param_table, sfdp_info.bptbl.size); + int status = sfdp_reader( + sfdp_info.bptbl.addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + param_table, + sfdp_info.bptbl.size + ); if (status != SPIF_BD_ERROR_OK) { tr_error("init - Read SFDP First Table Failed"); return -1; diff --git a/storage/blockdevice/include/blockdevice/internal/SFDP.h b/storage/blockdevice/include/blockdevice/internal/SFDP.h index 96f7518e018..b1b51a1a60f 100644 --- a/storage/blockdevice/include/blockdevice/internal/SFDP.h +++ b/storage/blockdevice/include/blockdevice/internal/SFDP.h @@ -47,6 +47,22 @@ constexpr int SFDP_ERASE_BITMASK_ALL = 0x0F; ///< Erase type All constexpr int SFDP_MAX_NUM_OF_ERASE_TYPES = 4; ///< Maximum number of different erase types (erase granularity) +// Size of a command specified by SFDP +enum sfdp_cmd_addr_size_t { + SFDP_CMD_ADDR_NONE = 0x00, // No address in command + SFDP_CMD_ADDR_3_BYTE = 0x01, // 3-byte address + SFDP_CMD_ADDR_4_BYTE = 0x02, // 4-byte address + SFDP_CMD_ADDR_SIZE_VARIABLE = 0x03 // Address size from current setting +}; + +// Parameters for SFDP Read command +constexpr sfdp_cmd_addr_size_t SFDP_READ_CMD_ADDR_TYPE = SFDP_CMD_ADDR_3_BYTE; // Read SFDP has 3-byte address +constexpr uint8_t SFDP_READ_CMD_INST = 0x5A; // Read SFDP instruction +constexpr uint8_t SFDP_READ_CMD_DUMMY_CYCLES = 8; // READ SFDP dummy cycles + +// Special value from SFDP for using dummy cycles from current setting +constexpr uint8_t SFDP_CMD_DUMMY_CYCLES_VARIABLE = 0xF; + /** JEDEC Basic Flash Parameter Table info */ struct sfdp_bptbl_info { uint32_t addr; ///< Address @@ -92,7 +108,7 @@ struct sfdp_hdr_info { * * @return MBED_SUCCESS on success, negative error code on failure */ -int sfdp_parse_headers(Callback sfdp_reader, sfdp_hdr_info &sfdp_info); +int sfdp_parse_headers(Callback sfdp_reader, sfdp_hdr_info &sfdp_info); /** Parse Sector Map Parameter Table * Retrieves the table from a device and parses the information contained by the table @@ -102,7 +118,7 @@ int sfdp_parse_headers(Callback sfdp_reader, * * @return MBED_SUCCESS on success, negative error code on failure */ -int sfdp_parse_sector_map_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info); +int sfdp_parse_sector_map_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info); /** Detect page size used for writing on flash * diff --git a/storage/blockdevice/source/SFDP.cpp b/storage/blockdevice/source/SFDP.cpp index 5f287386955..e737057cc0e 100644 --- a/storage/blockdevice/source/SFDP.cpp +++ b/storage/blockdevice/source/SFDP.cpp @@ -181,7 +181,7 @@ int sfdp_parse_single_param_header(sfdp_prm_hdr *phdr_ptr, sfdp_hdr_info &hdr_in return 0; } -int sfdp_parse_headers(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) +int sfdp_parse_headers(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) { bd_addr_t addr = 0x0; int number_of_param_headers = 0; @@ -191,7 +191,14 @@ int sfdp_parse_headers(Callback sfdp_reader, data_length = SFDP_HEADER_SIZE; uint8_t sfdp_header[SFDP_HEADER_SIZE]; - int status = sfdp_reader(addr, sfdp_header, data_length); + int status = sfdp_reader( + addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + sfdp_header, + data_length + ); if (status < 0) { tr_error("Retrieving SFDP Header failed"); return -1; @@ -213,7 +220,14 @@ int sfdp_parse_headers(Callback sfdp_reader, // Loop over Param Headers and parse them (currently supports Basic Param Table and Sector Region Map Table) for (int idx = 0; idx < number_of_param_headers; idx++) { - status = sfdp_reader(addr, param_header, data_length); + status = sfdp_reader( + addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + param_header, + data_length + ); if (status < 0) { tr_error("Retrieving a parameter header %d failed", idx + 1); return -1; @@ -231,7 +245,7 @@ int sfdp_parse_headers(Callback sfdp_reader, return 0; } -int sfdp_parse_sector_map_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) +int sfdp_parse_sector_map_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) { uint32_t tmp_region_size = 0; uint8_t type_mask; @@ -264,7 +278,14 @@ int sfdp_parse_sector_map_table(Callback sfdp tr_debug("Parsing Sector Map Table - addr: 0x%" PRIx32 ", Size: %d", sfdp_info.smptbl.addr, sfdp_info.smptbl.size); - int status = sfdp_reader(sfdp_info.smptbl.addr, smptbl_buff.get(), sfdp_info.smptbl.size); + int status = sfdp_reader( + sfdp_info.smptbl.addr, + SFDP_READ_CMD_ADDR_TYPE, + SFDP_READ_CMD_INST, + SFDP_READ_CMD_DUMMY_CYCLES, + smptbl_buff.get(), + sfdp_info.smptbl.size + ); if (status < 0) { tr_error("Sector Map: Table retrieval failed"); return -1; @@ -314,6 +335,7 @@ int sfdp_parse_sector_map_table(Callback sfdp return 0; } + size_t sfdp_detect_page_size(uint8_t *basic_param_table_ptr, size_t basic_param_table_size) { constexpr int SFDP_BASIC_PARAM_TABLE_PAGE_SIZE = 40; diff --git a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp index 3bb3ea6df9f..b5742e7c088 100644 --- a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp +++ b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp @@ -62,14 +62,14 @@ static const uint8_t sector_map_single_descriptor_twelve_regions[] { class TestSFDP : public testing::Test { public: - mbed::Callback sfdp_reader_callback; + mbed::Callback sfdp_reader_callback; protected: TestSFDP() : sfdp_reader_callback(this, &TestSFDP::sfdp_reader) {}; - int sfdp_reader(mbed::bd_addr_t addr, void *buff, bd_size_t buff_size) + int sfdp_reader(mbed::bd_addr_t addr, mbed::sfdp_cmd_addr_size_t addr_size, uint8_t instr, uint8_t cycles, void *buff, bd_size_t buff_size) { - int mock_return = sfdp_reader_mock.Call(addr, buff, buff_size); + int mock_return = sfdp_reader_mock.Call(addr, addr_size, instr, cycles, buff, buff_size); if (mock_return != 0) { return mock_return; } @@ -87,7 +87,7 @@ class TestSFDP : public testing::Test { sector_descriptors_size = table_size; } - MockFunction sfdp_reader_mock; + MockFunction sfdp_reader_mock; const uint8_t *sector_descriptors; bd_size_t sector_descriptors_size; }; @@ -191,7 +191,7 @@ TEST_F(TestSFDP, TestNoSectorMap) header_info.bptbl.device_size_bytes = device_size; // No need to read anything - EXPECT_CALL(sfdp_reader_mock, Call(_, _, _)).Times(0); + EXPECT_CALL(sfdp_reader_mock, Call(_, _, _, _, _, _)).Times(0); EXPECT_EQ(0, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); @@ -208,9 +208,17 @@ TEST_F(TestSFDP, TestSingleSectorConfig) mbed::sfdp_hdr_info header_info; set_sector_map_param_table(header_info.smptbl, sector_map_single_descriptor, sizeof(sector_map_single_descriptor)); - EXPECT_CALL(sfdp_reader_mock, Call(sector_map_start_addr, _, sizeof(sector_map_single_descriptor))) - .Times(1) - .WillOnce(Return(0)); + EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(sector_map_single_descriptor) + ) + ).Times(1).WillOnce(Return(0)); EXPECT_EQ(0, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); @@ -241,9 +249,17 @@ TEST_F(TestSFDP, TestSFDPReadFailure) mbed::sfdp_hdr_info header_info; set_sector_map_param_table(header_info.smptbl, sector_map_single_descriptor, sizeof(sector_map_single_descriptor)); - EXPECT_CALL(sfdp_reader_mock, Call(sector_map_start_addr, _, sizeof(sector_map_single_descriptor))) - .Times(1) - .WillOnce(Return(-1)); // Emulate read failure + EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(sector_map_single_descriptor) + ) + ).Times(1).WillOnce(Return(-1)); // Emulate read failure EXPECT_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); } @@ -261,9 +277,17 @@ TEST_F(TestSFDP, TestMoreRegionsThanSupported) sizeof(sector_map_single_descriptor_twelve_regions) ); - EXPECT_CALL(sfdp_reader_mock, Call(sector_map_start_addr, _, sizeof(sector_map_single_descriptor_twelve_regions))) - .Times(1) - .WillOnce(Return(0)); + EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(sector_map_single_descriptor_twelve_regions) + ) + ).Times(1).WillOnce(Return(0)); EXPECT_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); } From 50183d2ee4ef85fda8d8046b67a225719c04d35d Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Mon, 9 Aug 2021 11:06:36 +0100 Subject: [PATCH 04/12] SFDP: unit tests: Move unshared test data into case Some test data in test_sfdp.cpp is used by one test case only, so we turn it into a local variable. --- .../tests/UNITTESTS/SFDP/test_sfdp.cpp | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp index b5742e7c088..7c2df92f569 100644 --- a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp +++ b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp @@ -39,26 +39,6 @@ static const uint8_t sector_map_single_descriptor[] { 0xF4, 0xFF, 0xFB, 0x03 // region 2 }; -/** - * Based on Cypress S25FS512S, modified to have one descriptor, - * twelve regions for test purpose. - */ -static const uint8_t sector_map_single_descriptor_twelve_regions[] { - 0xFF, 0x01, 0x0B, 0xFF, // header, highest region = 0x0B - 0xF1, 0x7F, 0x00, 0x00, // region 0 - 0xF4, 0x7F, 0x03, 0x00, // region 1 - 0xF4, 0xFF, 0xFB, 0x03, // region 2 - 0xF1, 0x7F, 0x00, 0x00, // region 3 - 0xF4, 0x7F, 0x03, 0x00, // region 4 - 0xF4, 0xFF, 0xFB, 0x03, // region 5 - 0xF1, 0x7F, 0x00, 0x00, // region 6 - 0xF4, 0x7F, 0x03, 0x00, // region 7 - 0xF4, 0xFF, 0xFB, 0x03, // region 8 - 0xF1, 0x7F, 0x00, 0x00, // region 9 - 0xF4, 0x7F, 0x03, 0x00, // region 10 - 0xF4, 0xFF, 0xFB, 0x03, // region 11 -}; - class TestSFDP : public testing::Test { public: @@ -270,6 +250,26 @@ TEST_F(TestSFDP, TestSFDPReadFailure) */ TEST_F(TestSFDP, TestMoreRegionsThanSupported) { + /** + * Based on Cypress S25FS512S, modified to have one descriptor, + * twelve regions for test purpose. + */ + const uint8_t sector_map_single_descriptor_twelve_regions[] { + 0xFF, 0x01, 0x0B, 0xFF, // header, highest region = 0x0B + 0xF1, 0x7F, 0x00, 0x00, // region 0 + 0xF4, 0x7F, 0x03, 0x00, // region 1 + 0xF4, 0xFF, 0xFB, 0x03, // region 2 + 0xF1, 0x7F, 0x00, 0x00, // region 3 + 0xF4, 0x7F, 0x03, 0x00, // region 4 + 0xF4, 0xFF, 0xFB, 0x03, // region 5 + 0xF1, 0x7F, 0x00, 0x00, // region 6 + 0xF4, 0x7F, 0x03, 0x00, // region 7 + 0xF4, 0xFF, 0xFB, 0x03, // region 8 + 0xF1, 0x7F, 0x00, 0x00, // region 9 + 0xF4, 0x7F, 0x03, 0x00, // region 10 + 0xF4, 0xFF, 0xFB, 0x03, // region 11 + }; + mbed::sfdp_hdr_info header_info; set_sector_map_param_table( header_info.smptbl, From 0cb62c4944b7f4a42e9f5062bc8a1b1f27d8ded7 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Mon, 9 Aug 2021 13:02:03 +0100 Subject: [PATCH 05/12] SFDP: unit tests: Fix Configuration ID in fake test data The second byte of the sector map descriptor is the configuration ID. On a device with non-configurable layout, the only available map descriptor's configuration ID must be 0x00 as required by the JESD216D standard. This value is important, because we will check each descriptor's configuration ID when we support multiple configurations. Note: The test data is fake - when we modified real data of a configurable device to become non-configurable for test purpose, we forgot to change this field. --- storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp index 7c2df92f569..9b4448e3bbc 100644 --- a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp +++ b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp @@ -33,7 +33,7 @@ static const mbed::bd_addr_t sector_map_start_addr = 0xD81000; * three regions for test purpose. */ static const uint8_t sector_map_single_descriptor[] { - 0xFF, 0x01, 0x02, 0xFF, // header, highest region = 0x02 + 0xFF, 0x00, 0x02, 0xFF, // header, highest region = 0x02 0xF1, 0x7F, 0x00, 0x00, // region 0 0xF4, 0x7F, 0x03, 0x00, // region 1 0xF4, 0xFF, 0xFB, 0x03 // region 2 @@ -255,7 +255,7 @@ TEST_F(TestSFDP, TestMoreRegionsThanSupported) * twelve regions for test purpose. */ const uint8_t sector_map_single_descriptor_twelve_regions[] { - 0xFF, 0x01, 0x0B, 0xFF, // header, highest region = 0x0B + 0xFF, 0x00, 0x0B, 0xFF, // header, highest region = 0x0B 0xF1, 0x7F, 0x00, 0x00, // region 0 0xF4, 0x7F, 0x03, 0x00, // region 1 0xF4, 0xFF, 0xFB, 0x03, // region 2 From e04a16fd9f2ec86f31d721616fd0e015e8e30100 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Fri, 13 Aug 2021 15:02:15 +0100 Subject: [PATCH 06/12] QSPI: Move destructor into source file This allows the entire QSPI class to be mocked/faked for unit testing purpose, without dependencies from the real implementation such as `qspi_free()` from the HAL. --- drivers/include/drivers/QSPI.h | 7 +------ drivers/source/QSPI.cpp | 7 +++++++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/include/drivers/QSPI.h b/drivers/include/drivers/QSPI.h index 039090043ff..86beb2b98f6 100644 --- a/drivers/include/drivers/QSPI.h +++ b/drivers/include/drivers/QSPI.h @@ -115,12 +115,7 @@ class QSPI : private NonCopyable { QSPI(const qspi_pinmap_t &pinmap, int mode = 0); QSPI(const qspi_pinmap_t &&, int = 0) = delete; // prevent passing of temporary objects - virtual ~QSPI() - { - lock(); - qspi_free(&_qspi); - unlock(); - } + virtual ~QSPI(); /** Configure the data transmission format * diff --git a/drivers/source/QSPI.cpp b/drivers/source/QSPI.cpp index 6472a8905f7..aa430e1b59e 100644 --- a/drivers/source/QSPI.cpp +++ b/drivers/source/QSPI.cpp @@ -92,6 +92,13 @@ QSPI::QSPI(const qspi_pinmap_t &pinmap, int mode) : _qspi() MBED_ASSERT(success); } +QSPI::~QSPI() +{ + lock(); + qspi_free(&_qspi); + unlock(); +} + qspi_status_t QSPI::configure_format(qspi_bus_width_t inst_width, qspi_bus_width_t address_width, qspi_address_size_t address_size, qspi_bus_width_t alt_width, qspi_alt_size_t alt_size, qspi_bus_width_t data_width, int dummy_cycles) { // Check that alt_size/alt_width are a valid combination From 8dfad16f5defc5985ffd6c92ece4de90f839cf4c Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 11 Aug 2021 15:19:28 +0100 Subject: [PATCH 07/12] QSPIFBlockDevice: Use fully-qualified include path In Mbed OS, each library has an `include//` subdirectory containing headers. The recommended way to include a header is `#include "/
.h"` to avoid potential conflicts with any external modules that have same names of headers. This is not enforced yet, and both include/ and include// are in a library's include paths, to avoid breaking preexisting Mbed projects that don't follow the recommendation. But code within Mbed OS should follow it at least. --- .../blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp index d8d14224025..05c9f2babd9 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp @@ -17,7 +17,7 @@ #include "blockdevice/internal/SFDP.h" #include "platform/Callback.h" -#include "QSPIFBlockDevice.h" +#include "QSPIF/QSPIFBlockDevice.h" #include #include "rtos/ThisThread.h" @@ -25,7 +25,7 @@ #define MBED_CONF_MBED_TRACE_ENABLE 0 #endif -#include "mbed_trace.h" +#include "mbed-trace/mbed_trace.h" #define TRACE_GROUP "QSPIF" using namespace std::chrono; From 6032671b41eebdcdb38b70593e67ba689a3c82df Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Tue, 7 Sep 2021 17:42:02 +0100 Subject: [PATCH 08/12] QSPIFBlockDevice: Add quirk for inconsistent CR3NV register value Cypress S25FS512S's SFDP table suggests that bit-1 of the register CR3NV on 25FS512S should equal 1. But it is a known issue that the value is actually 0 on hardware. So if we query the value of CR3NV during configuration detection, we can't find a configuration that matches the SFDP data. This issue has been discussed in the Linux MTD (Memory Technology Devices) mailing list: https://linux-mtd.infradead.narkive.com/ylHK6CyT/spi-nor-fs512s-incorrect-cr3nv-1-value This commit adds a quirk to report bit-1 of CR3NV as 1. Note: In Mbed OS, vendor-specific quirks can only be handled in block devices. The SFDP functions assume data from hardware to be correct. So this quirk is in the block device. --- .../include/QSPIF/QSPIFBlockDevice.h | 3 +++ .../source/QSPIFBlockDevice.cpp | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h b/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h index 8bf4c6cccf0..3ebd0e504cf 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h +++ b/storage/blockdevice/COMPONENT_QSPIF/include/QSPIF/QSPIFBlockDevice.h @@ -376,6 +376,9 @@ class QSPIFBlockDevice : public mbed::BlockDevice { bool _needs_fast_mode; + // S25FS512S needs a quirk + bool _S25FS512S_quirk; + // Clear block protection qspif_clear_protection_method_t _clear_protection_method; diff --git a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp index 05c9f2babd9..77d3e8ce7e3 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp @@ -176,6 +176,9 @@ QSPIFBlockDevice::QSPIFBlockDevice(PinName io0, PinName io1, PinName io2, PinNam // Set default 4-byte addressing extension register write instruction _attempt_4_byte_addressing = true; _4byte_msb_reg_write_inst = QSPIF_INST_4BYTE_REG_WRITE_DEFAULT; + + // Quirk for Cypress S25FS512S + _S25FS512S_quirk = false; } int QSPIFBlockDevice::init() @@ -1099,6 +1102,16 @@ int QSPIFBlockDevice::_handle_vendor_quirks() tr_debug("Applying quirks for ISSI"); _num_status_registers = 1; break; + case 0x01: + if (vendor_device_ids[1] == 0x02 && vendor_device_ids[2] == 0x20) { + tr_debug("Applying quirks for Cypress S25FS512S"); + // On a Cypress S25FS512S flash chip + // * The SFDP table expects the register bitfield CR3NV[1] to be 1 + // but its actual value on the hardware is 0. In order for SFDP parsing + // to work, the quirk reports CR3NV[1] as 1. + _S25FS512S_quirk = true; + } + break; } return 0; @@ -1469,6 +1482,16 @@ int QSPIFBlockDevice::_qspi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::s return status; } + // Handle S25FS512S quirk. + const mbed::bd_addr_t S25FS512S_CR3NV = 0x4; + if (_S25FS512S_quirk) { + if (addr == S25FS512S_CR3NV) { + // If we reach here, rx_buffer is guaranteed to be non-null + // because it's been checked by _qspi.read() above. + static_cast(rx_buffer)[0] |= (1 << 1); + } + } + return QSPI_STATUS_OK; } From 23a79ef459f11c0f48188c5b1072954c095bd785 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Tue, 10 Aug 2021 10:01:59 +0100 Subject: [PATCH 09/12] QSPIFBlockDevice: Add quirk to ignore overlaying sectors on S25FS512S The entire flash chip S25FS512S consists of uniform 256KB sectors. Additionally, it has three configurations: * 0x01: Eight 4KB sectors (32KB in total) overlaying the start of the first 256KB sector * 0x03: Eight 4KB sectors (32KB in total) overlaying the end of the last 256KB sector * 0x05: No overlaying sectors The active configuration is determined from bit fields of two registers, CR1NV and CR3NV. Mbed OS does not currently support partially overlaying sectors, meaning that with eight 4KB sectors overlay a 256KB sectors, the remaining 224KB (== 256KB - 8 * 4KB) of the big sector can't be correctly handled. Supporting such scenario would involve a large amount of rewriting in Mbed OS's BlockDevice, SFDP and their tests, and may increase the code size. So, this commit applies a quirk to always report configuration 0x05 (no overlaying sectors). Even if 0x01 or 0x03 is the real configuration, they are compatible with the 0x05 configuration because 256KB sectors exist in all cases. Note: This quirk does *not* change actual configurations on hardware, because registers CR1NV and CR3NV are one-time configurable (OTP) - each bit field has a factory value of 0 and can be changed to 1 by the user but not back to 0. So QSPIFBlockDevice avoids changing them. --- .../COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp index 77d3e8ce7e3..39b8c1c43c6 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp +++ b/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp @@ -1109,6 +1109,13 @@ int QSPIFBlockDevice::_handle_vendor_quirks() // * The SFDP table expects the register bitfield CR3NV[1] to be 1 // but its actual value on the hardware is 0. In order for SFDP parsing // to work, the quirk reports CR3NV[1] as 1. + // * All three possible configurations support 256KB sectors across + // the entire chip. But when CR3NV[3] is 0, eight 4KB sectors overlay + // either the first 32KB or the last 32KB of the chip, whereas when + // CR3NV[3] is 1 there are no overlaying 4KB sectors. Mbed OS can't + // handle this type of overlay, so the quirk reports CR3NV[3] as 1 to + // let the code treat the chip as if it has no overlay. (Also CR1NV[2] + // is required to be 0 when CR3NV[3] is 1.) _S25FS512S_quirk = true; } break; @@ -1483,12 +1490,16 @@ int QSPIFBlockDevice::_qspi_send_read_sfdp_command(mbed::bd_addr_t addr, mbed::s } // Handle S25FS512S quirk. + const mbed::bd_addr_t S25FS512S_CR1NV = 0x2; const mbed::bd_addr_t S25FS512S_CR3NV = 0x4; if (_S25FS512S_quirk) { if (addr == S25FS512S_CR3NV) { // If we reach here, rx_buffer is guaranteed to be non-null // because it's been checked by _qspi.read() above. static_cast(rx_buffer)[0] |= (1 << 1); + static_cast(rx_buffer)[0] |= (1 << 3); + } else if (addr == S25FS512S_CR1NV) { + static_cast(rx_buffer)[0] &= ~(1 << 2); } } From f88bf828ab7453d3e7fdf4cfa0a1620739d00db2 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 11 Aug 2021 16:52:29 +0100 Subject: [PATCH 10/12] QSPIFBlockDevice: Add unit test for the S25FS512S quirk Add a test case to verify that the quirk on `CR1NV` and `CR3NV` registers get applied by `QSPIFBlockDevice` when the flash device is S25FS512S. `QSPIFBlockDevice` depends on the SFDP functions and the `QSPI` class, but we can't use gMock on them because: * SFDP functions are global functions, whereas gMock only supports mocking class member functions. * A mocked class is injected into the test subject, but passing a preexisting `QSPI` instance into `QSPIFBlockDevice` is not possible (unless we resort to pointers). For details, see comments in test_QSPIFBlockDevice.cpp. So fakes of the `QSPI` class and any SFDP functions involved are defined in test_QSPIFBlockDevice.cpp. --- storage/blockdevice/CMakeLists.txt | 1 + .../COMPONENT_QSPIF/CMakeLists.txt | 4 + .../COMPONENT_QSPIF/UNITTESTS/.mbedignore | 1 + .../COMPONENT_QSPIF/UNITTESTS/CMakeLists.txt | 39 ++ .../UNITTESTS/test_QSPIFBlockDevice.cpp | 336 ++++++++++++++++++ 5 files changed, 381 insertions(+) create mode 100644 storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/.mbedignore create mode 100644 storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/CMakeLists.txt create mode 100644 storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/test_QSPIFBlockDevice.cpp diff --git a/storage/blockdevice/CMakeLists.txt b/storage/blockdevice/CMakeLists.txt index 6b03244aebd..8b77f0fbc05 100644 --- a/storage/blockdevice/CMakeLists.txt +++ b/storage/blockdevice/CMakeLists.txt @@ -5,6 +5,7 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING) if(BUILD_GREENTEA_TESTS) # add greentea test else() + add_subdirectory(COMPONENT_QSPIF) add_subdirectory(tests/UNITTESTS) endif() endif() diff --git a/storage/blockdevice/COMPONENT_QSPIF/CMakeLists.txt b/storage/blockdevice/COMPONENT_QSPIF/CMakeLists.txt index 17ee0e28c75..f92553fe989 100644 --- a/storage/blockdevice/COMPONENT_QSPIF/CMakeLists.txt +++ b/storage/blockdevice/COMPONENT_QSPIF/CMakeLists.txt @@ -11,3 +11,7 @@ target_sources(mbed-storage-qspif INTERFACE source/QSPIFBlockDevice.cpp ) + +if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING) + add_subdirectory(UNITTESTS) +endif() diff --git a/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/.mbedignore b/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/.mbedignore new file mode 100644 index 00000000000..72e8ffc0db8 --- /dev/null +++ b/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/.mbedignore @@ -0,0 +1 @@ +* diff --git a/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/CMakeLists.txt b/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/CMakeLists.txt new file mode 100644 index 00000000000..1958cefa901 --- /dev/null +++ b/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/CMakeLists.txt @@ -0,0 +1,39 @@ +# Copyright (c) 2021 Arm Limited. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +include(GoogleTest) + +add_executable(qspif-unittest) + +target_compile_definitions(qspif-unittest + PRIVATE + DEVICE_QSPI=1 + MBED_CONF_QSPIF_QSPI_MIN_READ_SIZE=1 + MBED_CONF_QSPIF_QSPI_MIN_PROG_SIZE=1 +) + +target_include_directories(qspif-unittest + PRIVATE + ${mbed-os_SOURCE_DIR}/storage/blockdevice/COMPONENT_QSPIF/include +) + +target_sources(qspif-unittest + PRIVATE + ${mbed-os_SOURCE_DIR}/storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp + test_QSPIFBlockDevice.cpp +) + +target_link_libraries(qspif-unittest + PRIVATE + mbed-headers-blockdevice + mbed-headers-drivers + mbed-headers-platform + mbed-headers-rtos + mbed-stubs-blockdevice + mbed-stubs-platform + mbed-stubs-drivers + mbed-stubs-rtos + gmock_main +) + +gtest_discover_tests(qspif-unittest PROPERTIES LABELS "storage") diff --git a/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/test_QSPIFBlockDevice.cpp b/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/test_QSPIFBlockDevice.cpp new file mode 100644 index 00000000000..08523680288 --- /dev/null +++ b/storage/blockdevice/COMPONENT_QSPIF/UNITTESTS/test_QSPIFBlockDevice.cpp @@ -0,0 +1,336 @@ +/* Copyright (c) 2021 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 + +#include "gtest/gtest.h" +#include "gmock/gmock.h" + +#include "QSPIF/QSPIFBlockDevice.h" + +// S25FS512S's CR3NV register needs a quirk +static const int S25FS512S_CR1NV = 0x2; +static const int S25FS512S_CR3NV = 0x4; + +// JEDEC Manufacturer and Device ID, set by the test cases +static uint8_t const *device_id; + +namespace mbed { + +/** + * Fake implementation of mbed::QSPI class for QSPIFBlockDevice unit tests. + * + * Note: Data output by this is either dummy or based on Cypress S25FS512S. + * We can't use gMock for this because + * - The entire mbed::QSPI is non-virtual. Mocks are derived classes, and + * when a derived class instance is used as an instance of its base class, + * non-virtual members of the base class get used. + * - QSPIFBlockDevice's member _qspi (an instance of mbed::QSPI) is a value + * instead of a reference, in order to support initializing from pins directly. + * Also, mbed::QSPI is declared as NonCopyable whose copy-constructor is deleted, + * so we can't copy a preexisting mbed::QSPI instance into QSPIFBlockDevice. + * There's no way to dependency inject a mock instance. + */ +QSPI::QSPI(PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, PinName ssel, int mode) : _qspi() +{ +} + +QSPI::~QSPI() +{ +} + +qspi_status_t QSPI::configure_format(qspi_bus_width_t inst_width, qspi_bus_width_t address_width, qspi_address_size_t address_size, qspi_bus_width_t alt_width, qspi_alt_size_t alt_size, qspi_bus_width_t data_width, int dummy_cycles) +{ + return QSPI_STATUS_OK; +} + +qspi_status_t QSPI::set_frequency(int hz) +{ + return QSPI_STATUS_OK; +} + +qspi_status_t QSPI::read(int address, char *rx_buffer, size_t *rx_length) +{ + return QSPI_STATUS_OK; +} + +qspi_status_t QSPI::write(int address, const char *tx_buffer, size_t *tx_length) +{ + return QSPI_STATUS_OK; +} + +qspi_status_t QSPI::read(qspi_inst_t instruction, int alt, int address, char *rx_buffer, size_t *rx_length) +{ + if (!rx_buffer) { + return QSPI_STATUS_INVALID_PARAMETER; + } + + switch (address) { + // CR1NV and CR3NV registers' factory default values are both 0x0 + case S25FS512S_CR1NV: + case S25FS512S_CR3NV: + if (!rx_length || *rx_length < 1) { + return QSPI_STATUS_INVALID_PARAMETER; + } + rx_buffer[0] = 0x00; + break; + } + + switch (instruction) { + case mbed::SFDP_READ_CMD_INST: // read SFDP table + if (!rx_length || *rx_length < SFDP_BASIC_PARAMS_TBL_SIZE) { + return QSPI_STATUS_INVALID_PARAMETER; + } + // set dummy data (zeros), to avoid warning of uninitialized + // data from Valgrind and inconsistent test behavior. + memset(rx_buffer, 0x00, *rx_length); + // soft reset needs one instruction to enable reset, + // another instruction to perform reset + rx_buffer[61] = 0x30; + break; + } + + return QSPI_STATUS_OK; +} + +qspi_status_t QSPI::write(qspi_inst_t instruction, int alt, int address, const char *tx_buffer, size_t *tx_length) +{ + return QSPI_STATUS_OK; +} + +qspi_status_t QSPI::command_transfer(qspi_inst_t instruction, int address, const char *tx_buffer, size_t tx_length, const char *rx_buffer, size_t rx_length) +{ + static char status_registers[2]; + switch (instruction) { + case 0x01: // write status registers + if (!tx_buffer || tx_length < 2) { + return QSPI_STATUS_INVALID_PARAMETER; + } + memcpy(status_registers, tx_buffer, tx_length); + break; + case 0x05: // read status register 1 + if (!rx_buffer || rx_length < 1) { + return QSPI_STATUS_INVALID_PARAMETER; + } + const_cast(rx_buffer)[0] = status_registers[0]; + break; + case 0x35: // read status register 2 + if (!rx_buffer || rx_length < 1) { + return QSPI_STATUS_INVALID_PARAMETER; + } + const_cast(rx_buffer)[0] = status_registers[1]; + break; + case 0x06: // set write enabled bit + status_registers[0] |= 0x2; + break; + case 0x9F: // read Manufacturer and JDEC Device ID + assert(nullptr != device_id); // Test cases should set device_id + if (!rx_buffer || rx_length < 3) { + return QSPI_STATUS_INVALID_PARAMETER; + } + memcpy(const_cast(rx_buffer), device_id, 3); + break; + } + return QSPI_STATUS_OK; +} + +void QSPI::lock() +{ +} + +void QSPI::unlock() +{ +} + +bool QSPI::_initialize() +{ + return true; +} + +bool QSPI::_initialize_direct() +{ + return true; +} + +void QSPI::_build_qspi_command(qspi_inst_t instruction, int address, int alt) +{ +} + +/** + * Fake implementation of SFDP functions. + * They can't be mocked with gMock which supports only class member functions, + * not free/global functions. + */ +static Callback test_sfdp_reader; + +int sfdp_parse_headers(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) +{ + // The SFDP Basic Parameters Table size is used as a QSPI buffer + // size, so it must be set. + sfdp_info.bptbl.size = SFDP_BASIC_PARAMS_TBL_SIZE; + return 0; +} + +int sfdp_parse_sector_map_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) +{ + // The real implementation of this function queries + // the CR3NV register on S25FS512S. + test_sfdp_reader = sfdp_reader; + return 0; +} + +size_t sfdp_detect_page_size(uint8_t *bptbl_ptr, size_t bptbl_size) +{ + return 0; +} + +int sfdp_detect_erase_types_inst_and_size(uint8_t *bptbl_ptr, sfdp_hdr_info &sfdp_info) +{ + return 0; +} + +int sfdp_find_addr_region(bd_addr_t offset, const sfdp_hdr_info &sfdp_info) +{ + return 0; +} + +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) +{ + return 0; +} + +int sfdp_detect_device_density(uint8_t *bptbl_ptr, sfdp_bptbl_info &bptbl_info) +{ + return 0; +} + +int sfdp_detect_addressability(uint8_t *bptbl_ptr, sfdp_bptbl_info &bptbl_info) +{ + return 0; +} + +} // namespace mbed + + +class TestQSPIFBlockDevice : public testing::Test { +protected: + TestQSPIFBlockDevice() + : bd(PinName(0), PinName(1), PinName(2), PinName(3), PinName(4), PinName(5)) // fake pins + { + } + + virtual void TearDown() + { + device_id = nullptr; + } + + QSPIFBlockDevice bd; +}; + +/** + * Test that the quirk on the reported values of the CR1NV and CR3NV + * registers is applied when the flash chip is S25FS512S. + */ +TEST_F(TestQSPIFBlockDevice, TestS25FS512SQuirkApplied) +{ + // Use flash chip S25FS512S + const uint8_t id_S25FS512S[] { 0x01, 0x02, 0x20 }; + device_id = id_S25FS512S; + EXPECT_EQ(QSPIF_BD_ERROR_OK, bd.init()); + + const uint8_t id_N25Q128A[] { 0x20, 0xBB, 0x18 }; + + // Read the CR1NV register + uint8_t CR1NV; + EXPECT_EQ(QSPIF_BD_ERROR_OK, mbed::test_sfdp_reader( + S25FS512S_CR1NV, + mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, + 0x65, + mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, + &CR1NV, + sizeof(CR1NV))); + + // Read the CR3NV register + uint8_t CR3NV; + EXPECT_EQ(QSPIF_BD_ERROR_OK, mbed::test_sfdp_reader( + S25FS512S_CR3NV, + mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, + 0x65, + mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, + &CR3NV, + sizeof(CR3NV))); + + // The hardware value of CR3NV[1] is 0 but S25FS512S's SFDP table + // expects it to be 1. + EXPECT_EQ(0x2, CR3NV & 0x02); + + // The factory default configuration is CR1NV[2] == 0 and CR3NV[3] == 0 + // (eight 4KB sectors overlaying the start of the flash) but we treat + // the flash chip as if CR1NV[2] == 0 and CR3NV[3] == 1 (no overlaying 4KB + // sectors), as Mbed OS does not support this type of flash layout. + EXPECT_EQ(0x0, CR1NV & 0x4); + EXPECT_EQ(0x8, CR3NV & 0x8); + + // Deinitialization + EXPECT_EQ(QSPIF_BD_ERROR_OK, bd.deinit()); +} + +/** + * Test that the quirk on the reported values of the CR1NV and CR3NV + * registers is not applied when the flash chip is not S25FS512S. + * Note: Other flash chips most likely do not have CR1NV or CR3NV, but + * they might have something else readable at the same addresses. + */ +TEST_F(TestQSPIFBlockDevice, TestS25FS512SQuirkNotApplied) +{ + // Use flash chip N25Q128A + const uint8_t id_N25Q128A[] { 0x20, 0xBB, 0x18 }; + device_id = id_N25Q128A; + EXPECT_EQ(QSPIF_BD_ERROR_OK, bd.init()); + + // Read the value at the address of S25FS512S's CR1NV register, + // assuming this address is readable. + uint8_t CR1NV; + EXPECT_EQ(QSPIF_BD_ERROR_OK, mbed::test_sfdp_reader( + S25FS512S_CR1NV, + mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, + 0x65, + mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, + &CR1NV, + sizeof(CR1NV))); + + // Read the value at the address of S25FS512S's CR3NV register, + // assuming this address is readable. + uint8_t CR3NV; + EXPECT_EQ(QSPIF_BD_ERROR_OK, mbed::test_sfdp_reader( + S25FS512S_CR3NV, + mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, + 0x65, + mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, + &CR3NV, + sizeof(CR3NV))); + + // Values (0) reported by the fake QSPI::read() should be unmodifed + EXPECT_EQ(0, CR1NV); + EXPECT_EQ(0, CR3NV); + + // Deinitialization + EXPECT_EQ(QSPIF_BD_ERROR_OK, bd.deinit()); +} From 6bb23815c5d3ac0c852505e3a2d56532e7355a19 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Mon, 9 Aug 2021 11:27:08 +0100 Subject: [PATCH 11/12] SFDP: Add support for multiple configurations and sector maps A Sector Map Parameter Table contains a sequence of the following descriptors: * (Optional) configuration detection command descriptors, one for each command to run to determine the current configuration. This exists only if the flash layout is configurable. * Sector map descriptors, one for each possible configuration. On a flash device with a non-configurable layout, there is only one such descriptor. Previously we only supported the non-configurable case with a single descriptor. This commit adds support for multiple configurations. --- storage/blockdevice/source/SFDP.cpp | 138 ++++++- .../tests/UNITTESTS/SFDP/test_sfdp.cpp | 338 +++++++++++++++++- 2 files changed, 464 insertions(+), 12 deletions(-) diff --git a/storage/blockdevice/source/SFDP.cpp b/storage/blockdevice/source/SFDP.cpp index e737057cc0e..e523c3449c4 100644 --- a/storage/blockdevice/source/SFDP.cpp +++ b/storage/blockdevice/source/SFDP.cpp @@ -245,6 +245,110 @@ int sfdp_parse_headers(Callback sfdp_reader, + sfdp_hdr_info &sfdp_info, + uint8_t *&descriptor, + const uint8_t *table_end, + uint8_t &config) +{ + config = 0; + + // If the table starts with a sector map descriptor instead of a configuration + // detection command descriptor, this device has only one configuration (i.e. is + // not configurable) with ID equal to 0. + if (is_sector_map_descriptor(descriptor)) { + return 0; + } + + // Loop through all configuration detection descriptors and run detection commands + while (!is_sector_map_descriptor(descriptor) && (descriptor + min_descriptor_size <= table_end)) { + uint8_t instruction = descriptor[1]; + uint8_t dummy_cycles = descriptor[2] & 0x0F; + auto addr_size = static_cast(descriptor[2] >> 6); + uint8_t mask = descriptor[3]; + uint32_t cmd_addr; + memcpy(&cmd_addr, &descriptor[4], sizeof(cmd_addr)); // last 32 bits of the descriptor + + uint8_t rx; + int status = sfdp_reader(cmd_addr, addr_size, instruction, dummy_cycles, &rx, sizeof(rx)); + if (status < 0) { + tr_error("Sector Map: Configuration detection command failed"); + return -1; + } + + // Shift existing bits to the left, so we can add the newly detected bit + config <<= 1; + + // The mask may apply to any bit of rx, so we can't directly combine + // (rx & mask) with config. Instead, treat (rx & mask) as a boolean. + if (rx & mask) { + config |= 0x01; + } + + if (is_last_descriptor(descriptor)) { + // We've processed the last configuration detection command descriptor + descriptor += min_descriptor_size; // Increment the descriptor for the caller + return 0; + } + descriptor += min_descriptor_size; // next descriptor + } + + tr_error("Sector Map: Incomplete configuration detection command descriptors"); + return -1; +} + +static int sfdp_locate_sector_map_by_config( + const uint8_t config, + sfdp_hdr_info &sfdp_info, + uint8_t *&descriptor, + const uint8_t *table_end) +{ + // The size of a sector map descriptor depends on the number of regions. Before + // the number of regions is calculated, use the minimum possible size in the a loop condition. + while (is_sector_map_descriptor(descriptor) && (descriptor + min_descriptor_size <= table_end)) { + size_t regions = descriptor[2] + 1; // Region ID starts at 0 + size_t current_descriptor_size = (1 /*header*/ + regions) * 4 /*DWORD size*/; + if (descriptor + current_descriptor_size > table_end) { + tr_error("Sector Map: Incomplete sector map descriptor at the end of the table"); + return -1; + } + + if (descriptor[1] == config) { + // matching sector map found + return 0; + } + + if (is_last_descriptor(descriptor)) { + // We've processed the last sector map descriptor + tr_error("Sector Map: Failed to find a sector map that matches the current configuration"); + return -1; + } + + descriptor += current_descriptor_size; // next descriptor + } + + tr_error("Sector Map: Incomplete sector map descriptors"); + return -1; +} + int sfdp_parse_sector_map_table(Callback sfdp_reader, sfdp_hdr_info &sfdp_info) { uint32_t tmp_region_size = 0; @@ -268,7 +372,7 @@ int sfdp_parse_sector_map_table(Callback the size of this table is variable + * are variable -> the size of this table is variable */ auto smptbl_buff = std::unique_ptr(new (std::nothrow) uint8_t[sfdp_info.smptbl.size]); if (!smptbl_buff) { @@ -291,13 +395,26 @@ int sfdp_parse_sector_map_table(Callback SFDP_SECTOR_MAP_MAX_REGIONS) { tr_error("Sector Map: Supporting up to %d regions, current setup to %d regions - fail", SFDP_SECTOR_MAP_MAX_REGIONS, @@ -305,13 +422,13 @@ int sfdp_parse_sector_map_table(Callback> 8) & 0x00FFFFFF; // bits 9-32 + tmp_region_size = ((*((uint32_t *)&descriptor[(idx + 1) * 4])) >> 8) & 0x00FFFFFF; // bits 9-32 sfdp_info.smptbl.region_size[idx] = (tmp_region_size + 1) * 256; // Region size is 0 based multiple of 256 bytes; - sfdp_info.smptbl.region_erase_types_bitfld[idx] = smptbl_buff[(idx + 1) * 4] & 0x0F; // bits 1-4 + sfdp_info.smptbl.region_erase_types_bitfld[idx] = descriptor[(idx + 1) * 4] & 0x0F; // bits 1-4 min_common_erase_type_bits &= sfdp_info.smptbl.region_erase_types_bitfld[idx]; @@ -335,7 +452,6 @@ int sfdp_parse_sector_map_table(Callback(buff); + switch (addr) { + case sector_map_start_addr: + memcpy(buff, sector_descriptors, sector_descriptors_size); + break; + case register_CR1NV: + out[0] = 0x04; + break; + case register_CR3NV: + out[0] = 0x02; + break; + } return 0; }; @@ -291,3 +339,291 @@ TEST_F(TestSFDP, TestMoreRegionsThanSupported) EXPECT_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); } + +/** + * When a Sector Map Parameter Table has multiple configuration detection + * commands and sector maps, sfdp_parse_sector_map_table() runs all commands + * to find the active configuration and selects the matching sector map. + */ +TEST_F(TestSFDP, TestMultipleSectorConfigs) +{ + mbed::sfdp_hdr_info header_info; + set_sector_map_param_table( + header_info.smptbl, + sector_map_multiple_descriptors, + sizeof(sector_map_multiple_descriptors) + ); + + // First call: get all detection command and sector map descriptors + Expectation call_1 = EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(sector_map_multiple_descriptors) + ) + ).Times(1).WillOnce(Return(0)); + + // Second call: detect bit-0 of configuration + Expectation call_2 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR3NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_1).WillOnce(Return(0)); + + // Third call: detect bit-1 of configuration + Expectation call_3 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR1NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_2).WillOnce(Return(0)); + + // Fourth call: detect bit-2 of configuration + Expectation call_4 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR3NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_3).WillOnce(Return(0)); + + EXPECT_EQ(0, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); + + // Expecting sector map for Configuration ID = 0x03: + // Three regions + EXPECT_EQ(3, header_info.smptbl.region_cnt); + + // Region 0: erase type 3 (256KB erase) + // Range: first 64 MB minus 256 KB + EXPECT_EQ(64_MB - 256_KB, header_info.smptbl.region_size[0]); + EXPECT_EQ(64_MB - 256_KB - 1_B, header_info.smptbl.region_high_boundary[0]); + EXPECT_EQ(1 << (3 - 1), header_info.smptbl.region_erase_types_bitfld[0]); + + // Region 1: erase type 3 (256KB erase, which also covers 32KB from Region 2) + // Range: between Region 0 and Region 2 + EXPECT_EQ(256_KB - 32_KB, header_info.smptbl.region_size[1]); + EXPECT_EQ(64_MB - 32_KB - 1_B, header_info.smptbl.region_high_boundary[1]); + EXPECT_EQ(1 << (3 - 1), header_info.smptbl.region_erase_types_bitfld[1]); + + // Region 2: erase type 1 (4KB erase) + // Range: last 32 KB + EXPECT_EQ(32_KB, header_info.smptbl.region_size[2]); + EXPECT_EQ(64_MB - 1_B, header_info.smptbl.region_high_boundary[2]); + EXPECT_EQ(1 << (1 - 1), header_info.smptbl.region_erase_types_bitfld[2]); +} + +/** + * When a Sector Map Parameter Table has multiple configuration detection + * commands and sector maps, but one of the detection commands returns + * an error (e.g. due to a bus fault). + */ +TEST_F(TestSFDP, TestConfigDetectCmdFailure) +{ + mbed::sfdp_hdr_info header_info; + set_sector_map_param_table( + header_info.smptbl, + sector_map_multiple_descriptors, + sizeof(sector_map_multiple_descriptors) + ); + + // First call: get all detection command and sector map descriptors + Expectation call_1 = EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(sector_map_multiple_descriptors) + ) + ).Times(1).WillOnce(Return(0)); + + // Second call: detect bit-0 of configuration + Expectation call_2 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR3NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_1).WillOnce(Return(0)); + + // Third call: detect bit-1 of configuration (failed) + Expectation call_3 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR1NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_2).WillOnce(Return(-1)); // Emulate command failure + + // No further calls after failure + Expectation call_4 = EXPECT_CALL( + sfdp_reader_mock, + Call(_, _, _, _, _, _) + ).Times(0).After(call_3); + + EXPECT_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); +} + +/** + * When a Sector Map Parameter Table has multiple configuration detection + * commands and sector maps, but no detection command is declared as the + * last command. + * Note: This means either reading went wrong, or the SFDP data is inconsistent + * possibly due to hardware manufactured with wrong data. When the latter happens in + * practice, the solution is to let the block device apply a device-specific quirk + * and supply "corrected" SFDP data in its callback. + */ +TEST_F(TestSFDP, TestConfigIncompleteDetectCommands) +{ + const uint8_t table_incomplete_detect_commands[] = { + // Detect 1 + 0xFC, 0x65, 0xFF, 0x08, + 0x04, 0x00, 0x00, 0x00, + + // Detect 2 + 0xFC, 0x65, 0xFF, 0x04, + 0x02, 0x00, 0x00, 0x00, + + // Detect 3 + // Removed to trigger a parsing error + + // Config 1 + 0xFE, 0x01, 0x02, 0xFF, // header + 0xF1, 0x7F, 0x00, 0x00, // region 0 + 0xF4, 0x7F, 0x03, 0x00, // region 1 + 0xF4, 0xFF, 0xFB, 0x03, // region 2 + + // No Config 2 + + // Config 3 + 0xFE, 0x03, 0x02, 0xFF, // header + 0xF4, 0xFF, 0xFB, 0x03, // region 0 + 0xF4, 0x7F, 0x03, 0x00, // region 1 + 0xF1, 0x7F, 0x00, 0x00, // region 2 + + // Config 4 + 0xFF, 0x05, 0x00, 0xFF, // header + 0xF4, 0xFF, 0xFF, 0x03 // region 0 + }; + + mbed::sfdp_hdr_info header_info; + set_sector_map_param_table( + header_info.smptbl, + table_incomplete_detect_commands, + sizeof(table_incomplete_detect_commands) + ); + + // First call: get all detection command and sector map descriptors + Expectation call_1 = EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(table_incomplete_detect_commands) + ) + ).Times(1).WillOnce(Return(0)); + + // Second call: detect bit-0 of configuration + Expectation call_2 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR3NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_1).WillOnce(Return(0)); + + // Third call: detect bit-1 of configuration + Expectation call_3 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR1NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_2).WillOnce(Return(0)); + + // No further calls - incomplete detect command + Expectation call_4 = EXPECT_CALL( + sfdp_reader_mock, + Call(_, _, _, _, _, _) + ).Times(0).After(call_3); + + EXPECT_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); +} + +/** + * When a Sector Map Parameter Table has multiple configuration detection + * commands and sector maps, but no sector map matches the active + * configuration. + * Note: This means either detection went wrong, or the SFDP data is inconsistent + * possibly due to hardware manufactured with wrong data. When the latter happens in + * practice, the solution is to let the block device apply a device-specific quirk + * and supply "corrected" SFDP data in its callback. + */ +TEST_F(TestSFDP, TestConfigNoMatchingSectorMap) +{ + const uint8_t table_no_matching_sector_map[] = { + // Detect 1 + 0xFC, 0x65, 0xFF, 0x08, + 0x04, 0x00, 0x00, 0x00, + + // Detect 2 + 0xFC, 0x65, 0xFF, 0x04, + 0x02, 0x00, 0x00, 0x00, + + // Detect 3 + 0xFD, 0x65, 0xFF, 0x02, + 0x04, 0x00, 0x00, 0x00, + + // Config 1 + 0xFE, 0x01, 0x02, 0xFF, // header + 0xF1, 0x7F, 0x00, 0x00, // region 0 + 0xF4, 0x7F, 0x03, 0x00, // region 1 + 0xF4, 0xFF, 0xFB, 0x03, // region 2 + + // No Config 2 + + // Config 3 + // The active configuration (for test purpose) is 0x03 which should match header[1], + // but we change the latter to 0x02 to trigger a parsing error. + 0xFE, 0x02, 0x02, 0xFF, // header + 0xF4, 0xFF, 0xFB, 0x03, // region 0 + 0xF4, 0x7F, 0x03, 0x00, // region 1 + 0xF1, 0x7F, 0x00, 0x00, // region 2 + + // Config 4 + 0xFF, 0x05, 0x00, 0xFF, // header + 0xF4, 0xFF, 0xFF, 0x03 // region 0 + }; + + mbed::sfdp_hdr_info header_info; + set_sector_map_param_table( + header_info.smptbl, + table_no_matching_sector_map, + sizeof(table_no_matching_sector_map) + ); + + // First call: get all detection command and sector map descriptors + Expectation call_1 = EXPECT_CALL( + sfdp_reader_mock, + Call( + sector_map_start_addr, + mbed::SFDP_READ_CMD_ADDR_TYPE, + mbed::SFDP_READ_CMD_INST, + mbed::SFDP_READ_CMD_DUMMY_CYCLES, + _, + sizeof(table_no_matching_sector_map) + ) + ).Times(1).WillOnce(Return(0)); + + // Second call: detect bit-0 of configuration + Expectation call_2 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR3NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_1).WillOnce(Return(0)); + + // Third call: detect bit-1 of configuration + Expectation call_3 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR1NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_2).WillOnce(Return(0)); + + // Fourth call: detect bit-2 of configuration + Expectation call_4 = EXPECT_CALL( + sfdp_reader_mock, + Call(register_CR3NV, mbed::SFDP_CMD_ADDR_SIZE_VARIABLE, 0x65, mbed::SFDP_CMD_DUMMY_CYCLES_VARIABLE, _, 1) + ).Times(1).After(call_3).WillOnce(Return(0)); + + // Failed to find a sector map for the active configuration. + EXPECT_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); +} From 9ade440860e88652dc784000bc7fab8481cb00b1 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Tue, 10 Aug 2021 15:24:47 +0100 Subject: [PATCH 12/12] CYW9P62S1_43012EVB_01: Enable QSPI and QSPIF QSPIF was disabled on CYW9P62S1_43012EVB_01 because its S25FS512S flash chip has multiple configurations but our SFDP parser did not support this scenario. Now having added support for this, we can enable QSPIF (the component label for QSPIFBlockDevice) and QSPI (the label for Quad-SPI communication). --- targets/targets.json | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/targets/targets.json b/targets/targets.json index d26e94f586f..819a6c0b0c2 100644 --- a/targets/targets.json +++ b/targets/targets.json @@ -7750,12 +7750,8 @@ "CYW43XXX", "UDB_SDIO_P12" ], - "components_remove": [ - "QSPIF" - ], "device_has_remove": [ - "ANALOGOUT", - "QSPI" + "ANALOGOUT" ], "extra_labels_add": [ "PSOC6_01",