From 726a73c04810a33b9cada5b2e7b9089f9693042f Mon Sep 17 00:00:00 2001 From: Kyle Kearney Date: Mon, 30 Sep 2019 14:39:59 -0700 Subject: [PATCH 1/3] Report errors returned by _qspi_configure_format The function returns a qspi_status_t but most usages in QSPIFBlockDevice assume that it always succeeds. --- .../COMPONENT_QSPIF/QSPIFBlockDevice.cpp | 39 ++++++++++++------- .../COMPONENT_QSPIF/QSPIFBlockDevice.h | 7 ++-- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp index daebef73d4..0faed7c4ca 100644 --- a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp +++ b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp @@ -247,8 +247,11 @@ int QSPIFBlockDevice::init() } // Configure BUS Mode to 1_1_1 for all commands other than Read - _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, 0); + if (QSPI_STATUS_OK != _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, 0)) { + status = QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + goto exit_point; + } _is_initialized = true; @@ -303,17 +306,20 @@ int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size) _mutex.lock(); // Configure Bus for Reading - _qspi_configure_format(_inst_width, _address_width, _address_size, _address_width, // Alt width == address width - _alt_size, _data_width, _dummy_cycles); + if (QSPI_STATUS_OK != _qspi_configure_format(_inst_width, _address_width, _address_size, _address_width, // Alt width == address width + _alt_size, _data_width, _dummy_cycles)) { + return QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + } if (QSPI_STATUS_OK != _qspi_send_read_command(_read_instruction, buffer, addr, size)) { - status = QSPIF_BD_ERROR_DEVICE_ERROR; tr_error("Read Command failed"); + return QSPIF_BD_ERROR_DEVICE_ERROR; } // All commands other than Read use default 1-1-1 Bus mode (Program/Erase are constrained by flash memory performance more than bus performance) - _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, 0); + if (QSPI_STATUS_OK != _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, 0)) { + return QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + } _mutex.unlock(); return status; @@ -718,8 +724,10 @@ int QSPIFBlockDevice::_sfdp_parse_sfdp_headers(uint32_t &basic_table_addr, size_ bd_addr_t addr = 0x0; // Set 1-1-1 bus mode for SFDP header parsing - _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, 8); + if (QSPI_STATUS_OK != _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, 8)) { + return -1; + } qspi_status_t status = _qspi_send_read_command(QSPIF_SFDP, (char *)sfdp_header, addr /*address*/, data_length); if (status != QSPI_STATUS_OK) { @@ -885,8 +893,10 @@ int QSPIFBlockDevice::_sfdp_set_quad_enabled(uint8_t *basic_param_table_ptr) } // Configure BUS Mode to 1_1_1 for all commands other than Read - _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, 0); + if (QSPI_STATUS_OK != _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, 0)) { + return -1; + } // Read Status Register if (QSPI_STATUS_OK == _qspi_send_general_command(_read_register_inst, QSPI_NO_ADDRESS_COMMAND, NULL, 0, @@ -1211,8 +1221,11 @@ int QSPIFBlockDevice::_enable_fast_mdoe() status_reg_qer_setup[2] = 0x2; // Bit 1 of config Reg 2 // Configure BUS Mode to 1_1_1 for all commands other than Read - _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, 0); + if (QSPI_STATUS_OK != _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, 0)) { + tr_error("_qspi_configure_format failed"); + return QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + } // Read Status Register if (QSPI_STATUS_OK == _qspi_send_general_command(read_conf_register_inst, QSPI_NO_ADDRESS_COMMAND, NULL, 0, diff --git a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h index 5f78b0cafa..6da362bc6e 100644 --- a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h +++ b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h @@ -29,9 +29,10 @@ enum qspif_bd_error { QSPIF_BD_ERROR_PARSING_FAILED = -4002, /* SFDP Parsing failed */ QSPIF_BD_ERROR_READY_FAILED = -4003, /* Wait for Mem Ready failed */ QSPIF_BD_ERROR_WREN_FAILED = -4004, /* Write Enable Failed */ - QSPIF_BD_ERROR_INVALID_ERASE_PARAMS = -4005, /* Erase command not on sector aligned addresses or exceeds device size */ - QSPIF_BD_ERROR_DEVICE_NOT_UNIQE = -4006, /* Only one instance per csel is allowed */ - QSPIF_BD_ERROR_DEVICE_MAX_EXCEED = -4007 /* Max active QSPIF devices exceeded */ + QSPIF_BD_ERROR_CONF_FORMAT_FAILED = -4005, /* Configure format failed */ + QSPIF_BD_ERROR_INVALID_ERASE_PARAMS = -4006, /* Erase command not on sector aligned addresses or exceeds device size */ + QSPIF_BD_ERROR_DEVICE_NOT_UNIQE = -4007, /* Only one instance per csel is allowed */ + QSPIF_BD_ERROR_DEVICE_MAX_EXCEED = -4008 /* Max active QSPIF devices exceeded */ }; /** Enum qspif polarity mode From 3f20b8085946a18a31f3b48b4545da86c8b6ae24 Mon Sep 17 00:00:00 2001 From: Kyle Kearney Date: Tue, 1 Oct 2019 09:34:36 -0700 Subject: [PATCH 2/3] Reuse existing error for _qspi_configure_format Use QSPIF_BD_ERROR_DEVICE_ERROR instead of introducing a new error code. Add tr_error calls whenever _qspi_configure_format fails to aid in debugging. --- .../COMPONENT_QSPIF/QSPIFBlockDevice.cpp | 13 +++++++++---- .../blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h | 7 +++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp index 0faed7c4ca..406b0de024 100644 --- a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp +++ b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp @@ -249,7 +249,8 @@ int QSPIFBlockDevice::init() // Configure BUS Mode to 1_1_1 for all commands other than Read if (QSPI_STATUS_OK != _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, 0)) { - status = QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + tr_error("_qspi_configure_format failed"); + status = QSPIF_BD_ERROR_DEVICE_ERROR; goto exit_point; } @@ -308,7 +309,8 @@ int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size) // Configure Bus for Reading if (QSPI_STATUS_OK != _qspi_configure_format(_inst_width, _address_width, _address_size, _address_width, // Alt width == address width _alt_size, _data_width, _dummy_cycles)) { - return QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + tr_error("_qspi_configure_format failed"); + return QSPIF_BD_ERROR_DEVICE_ERROR; } if (QSPI_STATUS_OK != _qspi_send_read_command(_read_instruction, buffer, addr, size)) { @@ -318,7 +320,8 @@ int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size) // All commands other than Read use default 1-1-1 Bus mode (Program/Erase are constrained by flash memory performance more than bus performance) if (QSPI_STATUS_OK != _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, 0)) { - return QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + tr_error("_qspi_configure_format failed"); + return QSPIF_BD_ERROR_DEVICE_ERROR; } _mutex.unlock(); @@ -726,6 +729,7 @@ int QSPIFBlockDevice::_sfdp_parse_sfdp_headers(uint32_t &basic_table_addr, size_ // Set 1-1-1 bus mode for SFDP header parsing if (QSPI_STATUS_OK != _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, 8)) { + tr_error("_qspi_configure_format failed"); return -1; } @@ -895,6 +899,7 @@ int QSPIFBlockDevice::_sfdp_set_quad_enabled(uint8_t *basic_param_table_ptr) // Configure BUS Mode to 1_1_1 for all commands other than Read if (QSPI_STATUS_OK != _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, 0)) { + tr_error("_qspi_configure_format failed"); return -1; } @@ -1224,7 +1229,7 @@ int QSPIFBlockDevice::_enable_fast_mdoe() if (QSPI_STATUS_OK != _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, 0)) { tr_error("_qspi_configure_format failed"); - return QSPIF_BD_ERROR_CONF_FORMAT_FAILED; + return QSPIF_BD_ERROR_DEVICE_ERROR; } // Read Status Register diff --git a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h index 6da362bc6e..5f78b0cafa 100644 --- a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h +++ b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h @@ -29,10 +29,9 @@ enum qspif_bd_error { QSPIF_BD_ERROR_PARSING_FAILED = -4002, /* SFDP Parsing failed */ QSPIF_BD_ERROR_READY_FAILED = -4003, /* Wait for Mem Ready failed */ QSPIF_BD_ERROR_WREN_FAILED = -4004, /* Write Enable Failed */ - QSPIF_BD_ERROR_CONF_FORMAT_FAILED = -4005, /* Configure format failed */ - QSPIF_BD_ERROR_INVALID_ERASE_PARAMS = -4006, /* Erase command not on sector aligned addresses or exceeds device size */ - QSPIF_BD_ERROR_DEVICE_NOT_UNIQE = -4007, /* Only one instance per csel is allowed */ - QSPIF_BD_ERROR_DEVICE_MAX_EXCEED = -4008 /* Max active QSPIF devices exceeded */ + QSPIF_BD_ERROR_INVALID_ERASE_PARAMS = -4005, /* Erase command not on sector aligned addresses or exceeds device size */ + QSPIF_BD_ERROR_DEVICE_NOT_UNIQE = -4006, /* Only one instance per csel is allowed */ + QSPIF_BD_ERROR_DEVICE_MAX_EXCEED = -4007 /* Max active QSPIF devices exceeded */ }; /** Enum qspif polarity mode From 52cb2c2cfcb2ca221f783ca6c16f2e4980d12b87 Mon Sep 17 00:00:00 2001 From: Kyle Kearney Date: Mon, 14 Oct 2019 13:59:47 -0700 Subject: [PATCH 3/3] Avoid stale mutex in QSPIFBlockDevice::read Update to follow the same `goto exit_point` pattern that is used by the rest of the functions to avoid leaving the mutex locked when errors are detected and require the function to abort. --- .../blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp index 406b0de024..b7ec358670 100644 --- a/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp +++ b/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp @@ -310,21 +310,26 @@ int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size) if (QSPI_STATUS_OK != _qspi_configure_format(_inst_width, _address_width, _address_size, _address_width, // Alt width == address width _alt_size, _data_width, _dummy_cycles)) { tr_error("_qspi_configure_format failed"); - return QSPIF_BD_ERROR_DEVICE_ERROR; + status = QSPIF_BD_ERROR_DEVICE_ERROR; + goto exit_point; } if (QSPI_STATUS_OK != _qspi_send_read_command(_read_instruction, buffer, addr, size)) { tr_error("Read Command failed"); - return QSPIF_BD_ERROR_DEVICE_ERROR; + status = QSPIF_BD_ERROR_DEVICE_ERROR; + goto exit_point; } // All commands other than Read use default 1-1-1 Bus mode (Program/Erase are constrained by flash memory performance more than bus performance) if (QSPI_STATUS_OK != _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, 0)) { tr_error("_qspi_configure_format failed"); - return QSPIF_BD_ERROR_DEVICE_ERROR; + status = QSPIF_BD_ERROR_DEVICE_ERROR; + goto exit_point; } +exit_point: _mutex.unlock(); + return status; }