From 68694255b530e1779558cba9f65535090e42aef1 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 4 Aug 2021 11:33:04 +0100 Subject: [PATCH 1/4] SFDP: Move handling of no sector map into `if` When a flash chip's SFDP table has no sector map, we treat the whole flash as a single region. As an optimization, this should only be done if there is indeed no sector map. --- storage/blockdevice/source/SFDP.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/storage/blockdevice/source/SFDP.cpp b/storage/blockdevice/source/SFDP.cpp index ff640f56094..6c3c01b6649 100644 --- a/storage/blockdevice/source/SFDP.cpp +++ b/storage/blockdevice/source/SFDP.cpp @@ -239,12 +239,13 @@ int sfdp_parse_sector_map_table(Callback sfdp // Default set to all type bits 1-4 are common int min_common_erase_type_bits = SFDP_ERASE_BITMASK_ALL; - // If there's no region map, we have a single region sized the entire device size - sfdp_info.smptbl.region_size[0] = sfdp_info.bptbl.device_size_bytes; - sfdp_info.smptbl.region_high_boundary[0] = sfdp_info.bptbl.device_size_bytes - 1; - if (!sfdp_info.smptbl.addr || !sfdp_info.smptbl.size) { tr_debug("No Sector Map Table"); + + // If there's no sector map, we have a single region sized the entire device size + sfdp_info.smptbl.region_size[0] = sfdp_info.bptbl.device_size_bytes; + sfdp_info.smptbl.region_high_boundary[0] = sfdp_info.bptbl.device_size_bytes - 1; + return MBED_SUCCESS; } From 651099225e925af16ed673af7cbd9b513e82d887 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 4 Aug 2021 15:51:43 +0100 Subject: [PATCH 2/4] SFDP: Set region count to 1 for no sector map A flash device with no sector map table has uniform sectors, and we treat the whole flash as one region. In this case the function `sfdp_parse_sector_map_table()` sets the single region's size and high boundary accordingly, but it lacks setting of region count to 1, so users of the SFDP parser may not get the correct number of regions. This commit fixes the issue. --- storage/blockdevice/source/SFDP.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/storage/blockdevice/source/SFDP.cpp b/storage/blockdevice/source/SFDP.cpp index 6c3c01b6649..3b5ac248f9f 100644 --- a/storage/blockdevice/source/SFDP.cpp +++ b/storage/blockdevice/source/SFDP.cpp @@ -243,6 +243,7 @@ int sfdp_parse_sector_map_table(Callback sfdp tr_debug("No Sector Map Table"); // If there's no sector map, we have a single region sized the entire device size + sfdp_info.smptbl.region_cnt = 1; sfdp_info.smptbl.region_size[0] = sfdp_info.bptbl.device_size_bytes; sfdp_info.smptbl.region_high_boundary[0] = sfdp_info.bptbl.device_size_bytes - 1; From f08c3cdfb5216efabb0632f1484373224bc60105 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Thu, 5 Aug 2021 15:54:44 +0100 Subject: [PATCH 3/4] SFDP: TestEraseTypeAlgorithm: Move setup into test case The test data `struct mbed::sfdp_smptbl_info smptbl` is only relevant to `TestEraseTypeAlgorithm` and not shared with other test cases. --- .../tests/UNITTESTS/SFDP/test_sfdp.cpp | 50 ++++++++----------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp index 3a7523a91a9..ea64519a5b5 100644 --- a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp +++ b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp @@ -1,4 +1,4 @@ -/* Copyright (c) 2020 ARM Limited +/* Copyright (c) 2020-2021 ARM Limited * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -19,35 +19,6 @@ #include "blockdevice/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 - } }; /** @@ -63,6 +34,25 @@ TEST_F(TestSFDP, TestEraseTypeAlgorithm) int region = 1; int type; + // The mock flash supports 4KB, 32KB and 64KB erase types + struct mbed::sfdp_smptbl_info smptbl; + 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 + // Expected outcome: // * The starting position 92KB is 4KB-aligned // * The next position 96KB (92KB + 4KB) is 32KB-aligned From a16c2bf1735654047cc0513161c03ad65bca57a6 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 4 Aug 2021 15:29:21 +0100 Subject: [PATCH 4/4] SFDP: Add unit tests for Sector Map Parameter Table parsing Add tests for `sfdp_parse_sector_map_table()` which currently (at the time of this commit) supports flash devices with * no Sector Map Parameter Table (i.e. the whole flash is uniform and non-configurable) * a single descriptor in the Sector Map Parameter Table (i.e. the flash layout is non-configurable and has multiple regions) Support and unit tests for flashes with multiple configurable layouts will be added in the future. Note: The implementation of `sfdp_parse_sector_map_table()` assumes the table to be valid if read succeeds, so the SFDP reader callback needs to ensure it reads data correctly or return an error. --- .../tests/UNITTESTS/SFDP/test_sfdp.cpp | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp index ea64519a5b5..3bb3ea6df9f 100644 --- a/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp +++ b/storage/blockdevice/tests/UNITTESTS/SFDP/test_sfdp.cpp @@ -18,9 +18,97 @@ #include "gmock/gmock.h" #include "blockdevice/internal/SFDP.h" +using ::testing::_; +using ::testing::MockFunction; +using ::testing::Return; + +/** + * The Sector Map Parameter Table of Cypress S25FS512S: + * https://www.cypress.com/file/216376/download Table 71. + */ +static const mbed::bd_addr_t sector_map_start_addr = 0xD81000; + +/** + * Based on Cypress S25FS512S, modified to have one descriptor, + * three regions for test purpose. + */ +static const uint8_t sector_map_single_descriptor[] { + 0xFF, 0x01, 0x02, 0xFF, // header, highest region = 0x02 + 0xF1, 0x7F, 0x00, 0x00, // region 0 + 0xF4, 0x7F, 0x03, 0x00, // region 1 + 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: + 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 mock_return = sfdp_reader_mock.Call(addr, buff, buff_size); + if (mock_return != 0) { + return mock_return; + } + + memcpy(buff, sector_descriptors, sector_descriptors_size); + return 0; + }; + + void set_sector_map_param_table(mbed::sfdp_smptbl_info &smptbl, const uint8_t *table, const size_t table_size) + { + smptbl.size = table_size; + smptbl.addr = sector_map_start_addr; + + sector_descriptors = table; + sector_descriptors_size = table_size; + } + + MockFunction sfdp_reader_mock; + const uint8_t *sector_descriptors; + bd_size_t sector_descriptors_size; }; +/** + * Utilities for conversions to bytes. + */ +namespace{ + auto operator "" _B(unsigned long long int size) { + return size; + } + + auto operator "" _KB(unsigned long long int size) { + return size * 1024; + } + + auto operator "" _MB(unsigned long long int size) { + return size * 1024 * 1024; + } +} + /** * Test if sfdp_iterate_next_largest_erase_type() returns the most * optimal erase type, whose erase size is as large as possible @@ -89,3 +177,93 @@ TEST_F(TestSFDP, TestEraseTypeAlgorithm) smptbl); EXPECT_EQ(type, -1); // Invalid erase } + +/** + * Test that sfdp_parse_sector_map_table() treats a whole flash + * as one region if no sector map is available. + */ +TEST_F(TestSFDP, TestNoSectorMap) +{ + const bd_size_t device_size = 512_KB; + + mbed::sfdp_hdr_info header_info; + header_info.smptbl.size = 0; // No Sector Map Table + header_info.bptbl.device_size_bytes = device_size; + + // No need to read anything + EXPECT_CALL(sfdp_reader_mock, Call(_, _, _)).Times(0); + + EXPECT_EQ(0, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); + + EXPECT_EQ(1, header_info.smptbl.region_cnt); + EXPECT_EQ(device_size, header_info.smptbl.region_size[0]); + EXPECT_EQ(device_size - 1, header_info.smptbl.region_high_boundary[0]); +} + +/** + * When a Sector Map Parameter Table has a single descriptor (i.e. non-configurable flash layout). + */ +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_EQ(0, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); + + // Three regions + EXPECT_EQ(3, header_info.smptbl.region_cnt); + + // Region 0: erase type 1 (32KB erase) + EXPECT_EQ(32_KB, header_info.smptbl.region_size[0]); + EXPECT_EQ(32_KB - 1_B, header_info.smptbl.region_high_boundary[0]); + EXPECT_EQ(1 << (1 - 1), header_info.smptbl.region_erase_types_bitfld[0]); + + // Region 1: erase type 3 (256KB erase which includes 32KB from Region 0) + EXPECT_EQ(224_KB, header_info.smptbl.region_size[1]); + EXPECT_EQ(256_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 3 (256KB erase) + EXPECT_EQ(64_MB - 32_KB - 224_KB, header_info.smptbl.region_size[2]); + EXPECT_EQ(64_MB - 1_B, header_info.smptbl.region_high_boundary[2]); + EXPECT_EQ(1 << (3 - 1), header_info.smptbl.region_erase_types_bitfld[2]); +} + +/** + * When an SFDP reader fails to read data requested by sfdp_parse_sector_map_table(). + */ +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_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); +} + +/** + * When a flash layout has more regions than Mbed OS supports (10). + * Note: This is unlikely to happens in practice. + */ +TEST_F(TestSFDP, TestMoreRegionsThanSupported) +{ + mbed::sfdp_hdr_info header_info; + set_sector_map_param_table( + header_info.smptbl, + sector_map_single_descriptor_twelve_regions, + 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_EQ(-1, sfdp_parse_sector_map_table(sfdp_reader_callback, header_info)); +}