From 2c5340d47e2a482c2fc58ae02178332775c3a699 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 21 Oct 2016 17:28:50 +0800 Subject: [PATCH] spi_flash: change argument types spi_flash_read and spi_flash_write currently have a limitation that source and destination must be word-aligned. This can be fixed by adding code paths for various unaligned scenarios, but function signatures also need to be adjusted. As a first step (since we are pre-1.0 and can still change function signatures) alignment checks are added, and pointer types are relaxed to uint8_t. Later we will add handling of unaligned operations. This change also introduces spi_flash_erase_range and spi_flash_get_chip_size functions. We probably need something like spi_flash_chip_size_detect which will detect actual chip size. This is to allow single application binary to be used on a variety of boards and modules. --- components/nvs_flash/src/nvs_page.cpp | 27 ++++--- .../nvs_flash/test/spi_flash_emulation.cpp | 10 +-- .../nvs_flash/test/spi_flash_emulation.h | 6 +- .../test/test_spi_flash_emulation.cpp | 20 ++--- components/spi_flash/flash_ops.c | 81 +++++++++++++++++-- components/spi_flash/include/esp_spi_flash.h | 50 +++++++++--- 6 files changed, 146 insertions(+), 48 deletions(-) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index f4fc5430c..9ba478e21 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -37,7 +37,7 @@ esp_err_t Page::load(uint32_t sectorNumber) mErasedEntryCount = 0; Header header; - auto rc = spi_flash_read(mBaseAddress, reinterpret_cast(&header), sizeof(header)); + auto rc = spi_flash_read(mBaseAddress, reinterpret_cast(&header), sizeof(header)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -48,7 +48,7 @@ esp_err_t Page::load(uint32_t sectorNumber) // reading the whole page takes ~40 times less than erasing it uint32_t line[8]; for (uint32_t i = 0; i < SPI_FLASH_SEC_SIZE; i += sizeof(line)) { - rc = spi_flash_read(mBaseAddress + i, line, sizeof(line)); + rc = spi_flash_read(mBaseAddress + i, reinterpret_cast(line), sizeof(line)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -86,7 +86,7 @@ esp_err_t Page::load(uint32_t sectorNumber) esp_err_t Page::writeEntry(const Item& item) { - auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), reinterpret_cast(&item), sizeof(item)); + auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), reinterpret_cast(&item), sizeof(item)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -114,7 +114,7 @@ esp_err_t Page::writeEntryData(const uint8_t* data, size_t size) assert(mFirstUsedEntry != INVALID_ENTRY); const uint16_t count = size / ENTRY_SIZE; - auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), reinterpret_cast(data), static_cast(size)); + auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), data, size); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -396,8 +396,8 @@ esp_err_t Page::mLoadEntryTable() if (mState == PageState::ACTIVE || mState == PageState::FULL || mState == PageState::FREEING) { - auto rc = spi_flash_read(mBaseAddress + ENTRY_TABLE_OFFSET, mEntryTable.data(), - static_cast(mEntryTable.byteSize())); + auto rc = spi_flash_read(mBaseAddress + ENTRY_TABLE_OFFSET, reinterpret_cast(mEntryTable.data()), + mEntryTable.byteSize()); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -435,7 +435,7 @@ esp_err_t Page::mLoadEntryTable() while (mNextFreeEntry < ENTRY_COUNT) { uint32_t entryAddress = getEntryAddress(mNextFreeEntry); uint32_t header; - auto rc = spi_flash_read(entryAddress, &header, sizeof(header)); + auto rc = spi_flash_read(entryAddress, reinterpret_cast(&header), sizeof(header)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -559,7 +559,7 @@ esp_err_t Page::initialize() header.mSeqNumber = mSeqNumber; header.mCrc32 = header.calculateCrc32(); - auto rc = spi_flash_write(mBaseAddress, reinterpret_cast(&header), sizeof(header)); + auto rc = spi_flash_write(mBaseAddress, reinterpret_cast(&header), sizeof(header)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -577,7 +577,8 @@ esp_err_t Page::alterEntryState(size_t index, EntryState state) mEntryTable.set(index, state); size_t wordToWrite = mEntryTable.getWordIndex(index); uint32_t word = mEntryTable.data()[wordToWrite]; - auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast(wordToWrite) * 4, &word, 4); + auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast(wordToWrite) * 4, + reinterpret_cast(&word), sizeof(word)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -600,7 +601,8 @@ esp_err_t Page::alterEntryRangeState(size_t begin, size_t end, EntryState state) } if (nextWordIndex != wordIndex) { uint32_t word = mEntryTable.data()[wordIndex]; - auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast(wordIndex) * 4, &word, 4); + auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast(wordIndex) * 4, + reinterpret_cast(&word), 4); if (rc != ESP_OK) { return rc; } @@ -612,7 +614,8 @@ esp_err_t Page::alterEntryRangeState(size_t begin, size_t end, EntryState state) esp_err_t Page::alterPageState(PageState state) { - auto rc = spi_flash_write(mBaseAddress, reinterpret_cast(&state), sizeof(state)); + uint32_t state_val = static_cast(state); + auto rc = spi_flash_write(mBaseAddress, reinterpret_cast(&state_val), sizeof(state)); if (rc != ESP_OK) { mState = PageState::INVALID; return rc; @@ -623,7 +626,7 @@ esp_err_t Page::alterPageState(PageState state) esp_err_t Page::readEntry(size_t index, Item& dst) const { - auto rc = spi_flash_read(getEntryAddress(index), reinterpret_cast(&dst), sizeof(dst)); + auto rc = spi_flash_read(getEntryAddress(index), reinterpret_cast(&dst), sizeof(dst)); if (rc != ESP_OK) { return rc; } diff --git a/components/nvs_flash/test/spi_flash_emulation.cpp b/components/nvs_flash/test/spi_flash_emulation.cpp index 5185bd34c..bd1482268 100644 --- a/components/nvs_flash/test/spi_flash_emulation.cpp +++ b/components/nvs_flash/test/spi_flash_emulation.cpp @@ -22,7 +22,7 @@ void spi_flash_emulator_set(SpiFlashEmulator* e) s_emulator = e; } -esp_err_t spi_flash_erase_sector(uint16_t sec) +esp_err_t spi_flash_erase_sector(size_t sec) { if (!s_emulator) { return ESP_ERR_FLASH_OP_TIMEOUT; @@ -35,26 +35,26 @@ esp_err_t spi_flash_erase_sector(uint16_t sec) return ESP_OK; } -esp_err_t spi_flash_write(uint32_t des_addr, const uint32_t *src_addr, uint32_t size) +esp_err_t spi_flash_write(size_t des_addr, const uint8_t *src_addr, size_t size) { if (!s_emulator) { return ESP_ERR_FLASH_OP_TIMEOUT; } - if (!s_emulator->write(des_addr, src_addr, size)) { + if (!s_emulator->write(des_addr, reinterpret_cast(src_addr), size)) { return ESP_ERR_FLASH_OP_FAIL; } return ESP_OK; } -esp_err_t spi_flash_read(uint32_t src_addr, uint32_t *des_addr, uint32_t size) +esp_err_t spi_flash_read(size_t src_addr, uint8_t *des_addr, size_t size) { if (!s_emulator) { return ESP_ERR_FLASH_OP_TIMEOUT; } - if (!s_emulator->read(des_addr, src_addr, size)) { + if (!s_emulator->read(reinterpret_cast(des_addr), src_addr, size)) { return ESP_ERR_FLASH_OP_FAIL; } diff --git a/components/nvs_flash/test/spi_flash_emulation.h b/components/nvs_flash/test/spi_flash_emulation.h index d5a242b24..ba50c4f9e 100644 --- a/components/nvs_flash/test/spi_flash_emulation.h +++ b/components/nvs_flash/test/spi_flash_emulation.h @@ -44,7 +44,7 @@ public: spi_flash_emulator_set(nullptr); } - bool read(uint32_t* dest, uint32_t srcAddr, size_t size) const + bool read(uint32_t* dest, size_t srcAddr, size_t size) const { if (srcAddr % 4 != 0 || size % 4 != 0 || @@ -60,7 +60,7 @@ public: return true; } - bool write(uint32_t dstAddr, const uint32_t* src, size_t size) + bool write(size_t dstAddr, const uint32_t* src, size_t size) { uint32_t sectorNumber = dstAddr/SPI_FLASH_SEC_SIZE; if (sectorNumber < mLowerSectorBound || sectorNumber >= mUpperSectorBound) { @@ -96,7 +96,7 @@ public: return true; } - bool erase(uint32_t sectorNumber) + bool erase(size_t sectorNumber) { size_t offset = sectorNumber * SPI_FLASH_SEC_SIZE / 4; if (offset > mData.size()) { diff --git a/components/nvs_flash/test/test_spi_flash_emulation.cpp b/components/nvs_flash/test/test_spi_flash_emulation.cpp index ea233da61..329e721ce 100644 --- a/components/nvs_flash/test/test_spi_flash_emulation.cpp +++ b/components/nvs_flash/test/test_spi_flash_emulation.cpp @@ -30,7 +30,7 @@ TEST_CASE("flash starts with all bytes == 0xff", "[spi_flash_emu]") uint8_t sector[SPI_FLASH_SEC_SIZE]; for (int i = 0; i < 4; ++i) { - CHECK(spi_flash_read(0, reinterpret_cast(sector), sizeof(sector)) == ESP_OK); + CHECK(spi_flash_read(0, sector, sizeof(sector)) == ESP_OK); for (auto v: sector) { CHECK(v == 0xff); } @@ -42,9 +42,9 @@ TEST_CASE("invalid writes are checked", "[spi_flash_emu]") SpiFlashEmulator emu(1); uint32_t val = 0; - CHECK(spi_flash_write(0, &val, 4) == ESP_OK); + CHECK(spi_flash_write(0, reinterpret_cast(&val), 4) == ESP_OK); val = 1; - CHECK(spi_flash_write(0, &val, 4) == ESP_ERR_FLASH_OP_FAIL); + CHECK(spi_flash_write(0, reinterpret_cast(&val), 4) == ESP_ERR_FLASH_OP_FAIL); } @@ -53,11 +53,11 @@ TEST_CASE("out of bounds writes fail", "[spi_flash_emu]") SpiFlashEmulator emu(4); uint32_t vals[8]; std::fill_n(vals, 8, 0); - CHECK(spi_flash_write(0, vals, sizeof(vals)) == ESP_OK); + CHECK(spi_flash_write(0, reinterpret_cast(vals), sizeof(vals)) == ESP_OK); - CHECK(spi_flash_write(4*4096 - sizeof(vals), vals, sizeof(vals)) == ESP_OK); + CHECK(spi_flash_write(4*4096 - sizeof(vals), reinterpret_cast(vals), sizeof(vals)) == ESP_OK); - CHECK(spi_flash_write(4*4096 - sizeof(vals) + 4, vals, sizeof(vals)) == ESP_ERR_FLASH_OP_FAIL); + CHECK(spi_flash_write(4*4096 - sizeof(vals) + 4, reinterpret_cast(vals), sizeof(vals)) == ESP_ERR_FLASH_OP_FAIL); } @@ -65,9 +65,9 @@ TEST_CASE("after erase the sector is set to 0xff", "[spi_flash_emu]") { SpiFlashEmulator emu(4); uint32_t val1 = 0xab00cd12; - CHECK(spi_flash_write(0, &val1, sizeof(val1)) == ESP_OK); + CHECK(spi_flash_write(0, reinterpret_cast(&val1), sizeof(val1)) == ESP_OK); uint32_t val2 = 0x5678efab; - CHECK(spi_flash_write(4096 - 4, &val2, sizeof(val2)) == ESP_OK); + CHECK(spi_flash_write(4096 - 4, reinterpret_cast(&val2), sizeof(val2)) == ESP_OK); CHECK(emu.words()[0] == val1); CHECK(range_empty_n(emu.words() + 1, 4096 / 4 - 2)); @@ -83,7 +83,7 @@ TEST_CASE("after erase the sector is set to 0xff", "[spi_flash_emu]") TEST_CASE("read/write/erase operation times are calculated correctly", "[spi_flash_emu]") { SpiFlashEmulator emu(1); - uint32_t data[128]; + uint8_t data[512]; spi_flash_read(0, data, 4); CHECK(emu.getTotalTime() == 7); CHECK(emu.getReadOps() == 1); @@ -141,7 +141,7 @@ TEST_CASE("read/write/erase operation times are calculated correctly", "[spi_fla CHECK(emu.getTotalTime() == 37142); } -TEST_CASE("data is randomized predicatbly", "[spi_flash_emu]") +TEST_CASE("data is randomized predictably", "[spi_flash_emu]") { SpiFlashEmulator emu1(3); emu1.randomize(0x12345678); diff --git a/components/spi_flash/flash_ops.c b/components/spi_flash/flash_ops.c index 99e8ef77f..512f6d20d 100644 --- a/components/spi_flash/flash_ops.c +++ b/components/spi_flash/flash_ops.c @@ -64,6 +64,11 @@ void spi_flash_init() #endif } +size_t spi_flash_get_chip_size() +{ + return g_rom_flashchip.chip_size; +} + SpiFlashOpResult IRAM_ATTR spi_flash_unlock() { static bool unlocked = false; @@ -77,28 +82,74 @@ SpiFlashOpResult IRAM_ATTR spi_flash_unlock() return SPI_FLASH_RESULT_OK; } -esp_err_t IRAM_ATTR spi_flash_erase_sector(uint16_t sec) +esp_err_t IRAM_ATTR spi_flash_erase_sector(size_t sec) { + return spi_flash_erase_range(sec * SPI_FLASH_SEC_SIZE, SPI_FLASH_SEC_SIZE); +} + +esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size) +{ + if (start_addr % SPI_FLASH_SEC_SIZE != 0) { + return ESP_ERR_INVALID_ARG; + } + if (size % SPI_FLASH_SEC_SIZE != 0) { + return ESP_ERR_INVALID_SIZE; + } + if (size + start_addr > spi_flash_get_chip_size()) { + return ESP_ERR_INVALID_SIZE; + } + size_t start = start_addr / SPI_FLASH_SEC_SIZE; + size_t end = start + size / SPI_FLASH_SEC_SIZE; + const size_t sectors_per_block = 16; COUNTER_START(); spi_flash_disable_interrupts_caches_and_other_cpu(); SpiFlashOpResult rc; rc = spi_flash_unlock(); if (rc == SPI_FLASH_RESULT_OK) { - rc = SPIEraseSector(sec); + for (size_t sector = start; sector != end && rc == SPI_FLASH_RESULT_OK; ) { + if (sector % sectors_per_block == 0 && end - sector > sectors_per_block) { + rc = SPIEraseBlock(sector / sectors_per_block); + sector += sectors_per_block; + COUNTER_ADD_BYTES(erase, sectors_per_block * SPI_FLASH_SEC_SIZE); + } + else { + rc = SPIEraseSector(sector); + ++sector; + COUNTER_ADD_BYTES(erase, SPI_FLASH_SEC_SIZE); + } + } } spi_flash_enable_interrupts_caches_and_other_cpu(); COUNTER_STOP(erase); return spi_flash_translate_rc(rc); } -esp_err_t IRAM_ATTR spi_flash_write(uint32_t dest_addr, const uint32_t *src, uint32_t size) +esp_err_t IRAM_ATTR spi_flash_write(size_t dest_addr, const uint8_t *src, size_t size) { + // TODO: replace this check with code which deals with unaligned sources + if (((ptrdiff_t) src) % 4 != 0) { + return ESP_ERR_INVALID_ARG; + } + // Destination alignment is also checked in ROM code, but we can give + // better error code here + // TODO: add handling of unaligned destinations + if (dest_addr % 4 != 0) { + return ESP_ERR_INVALID_ARG; + } + if (size % 4 != 0) { + return ESP_ERR_INVALID_SIZE; + } + // Out of bound writes are checked in ROM code, but we can give better + // error code here + if (dest_addr + size > g_rom_flashchip.chip_size) { + return ESP_ERR_INVALID_SIZE; + } COUNTER_START(); spi_flash_disable_interrupts_caches_and_other_cpu(); SpiFlashOpResult rc; rc = spi_flash_unlock(); if (rc == SPI_FLASH_RESULT_OK) { - rc = SPIWrite(dest_addr, src, (int32_t) size); + rc = SPIWrite((uint32_t) dest_addr, (const uint32_t*) src, (int32_t) size); COUNTER_ADD_BYTES(write, size); } spi_flash_enable_interrupts_caches_and_other_cpu(); @@ -106,11 +157,29 @@ esp_err_t IRAM_ATTR spi_flash_write(uint32_t dest_addr, const uint32_t *src, uin return spi_flash_translate_rc(rc); } -esp_err_t IRAM_ATTR spi_flash_read(uint32_t src_addr, uint32_t *dest, uint32_t size) +esp_err_t IRAM_ATTR spi_flash_read(size_t src_addr, uint8_t *dest, size_t size) { + // TODO: replace this check with code which deals with unaligned destinations + if (((ptrdiff_t) dest) % 4 != 0) { + return ESP_ERR_INVALID_ARG; + } + // Source alignment is also checked in ROM code, but we can give + // better error code here + // TODO: add handling of unaligned destinations + if (src_addr % 4 != 0) { + return ESP_ERR_INVALID_ARG; + } + if (size % 4 != 0) { + return ESP_ERR_INVALID_SIZE; + } + // Out of bound reads are checked in ROM code, but we can give better + // error code here + if (src_addr + size > g_rom_flashchip.chip_size) { + return ESP_ERR_INVALID_SIZE; + } COUNTER_START(); spi_flash_disable_interrupts_caches_and_other_cpu(); - SpiFlashOpResult rc = SPIRead(src_addr, dest, (int32_t) size); + SpiFlashOpResult rc = SPIRead((uint32_t) src_addr, (uint32_t*) dest, (int32_t) size); COUNTER_ADD_BYTES(read, size); spi_flash_enable_interrupts_caches_and_other_cpu(); COUNTER_STOP(read); diff --git a/components/spi_flash/include/esp_spi_flash.h b/components/spi_flash/include/esp_spi_flash.h index 597e41857..ab66a4204 100644 --- a/components/spi_flash/include/esp_spi_flash.h +++ b/components/spi_flash/include/esp_spi_flash.h @@ -38,37 +38,63 @@ extern "C" { */ void spi_flash_init(); +/** + * @brief Get flash chip size, as set in binary image header + * + * @note This value does not necessarily match real flash size. + * + * @return size of flash chip, in bytes + */ +size_t spi_flash_get_chip_size(); + /** * @brief Erase the Flash sector. * - * @param uint16 sec : Sector number, the count starts at sector 0, 4KB per sector. + * @param sector Sector number, the count starts at sector 0, 4KB per sector. * * @return esp_err_t */ -esp_err_t spi_flash_erase_sector(uint16_t sec); +esp_err_t spi_flash_erase_sector(size_t sector); + +/** + * @brief Erase a range of flash sectors + * + * @param uint32_t start_address : Address where erase operation has to start. + * Must be 4kB-aligned + * @param uint32_t size : Size of erased range, in bytes. Must be divisible by 4kB. + * + * @return esp_err_t + */ +esp_err_t spi_flash_erase_range(size_t start_addr, size_t size); + /** * @brief Write data to Flash. * - * @param uint32 des_addr : destination address in Flash. - * @param uint32 *src_addr : source address of the data. - * @param uint32 size : length of data + * @note Both des_addr and src_addr have to be 4-byte aligned. + * This is a temporary limitation which will be removed. + * + * @param des_addr destination address in Flash + * @param src_addr source address of the data + * @param size length of data, in bytes * * @return esp_err_t */ -esp_err_t spi_flash_write(uint32_t des_addr, const uint32_t *src_addr, uint32_t size); +esp_err_t spi_flash_write(size_t des_addr, const uint8_t *src_addr, size_t size); /** * @brief Read data from Flash. * - * @param uint32 src_addr : source address of the data in Flash. - * @param uint32 *des_addr : destination address. - * @param uint32 size : length of data + * @note Both des_addr and src_addr have to be 4-byte aligned. + * This is a temporary limitation which will be removed. + * + * @param src_addr source address of the data in Flash. + * @param des_addr destination address + * @param size length of data * * @return esp_err_t */ -esp_err_t spi_flash_read(uint32_t src_addr, uint32_t *des_addr, uint32_t size); - +esp_err_t spi_flash_read(size_t src_addr, uint8_t *des_addr, size_t size); /** * @brief Enumeration which specifies memory space requested in an mmap call @@ -135,7 +161,7 @@ void spi_flash_mmap_dump(); typedef struct { uint32_t count; // number of times operation was executed uint32_t time; // total time taken, in microseconds - uint32_t bytes; // total number of bytes, for read and write operations + uint32_t bytes; // total number of bytes } spi_flash_counter_t; typedef struct {