Skip to content

FlashIAP driver modifications #6140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 23, 2018
Merged

FlashIAP driver modifications #6140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 51 additions & 6 deletions TESTS/mbed_drivers/flashiap/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,56 @@ void flashiap_program_test()
TEST_ASSERT_EQUAL_INT32(0, ret);
}

void flashiap_cross_sector_program_test()
{
FlashIAP flash_device;
uint32_t ret = flash_device.init();
TEST_ASSERT_EQUAL_INT32(0, ret);

uint32_t page_size = flash_device.get_page_size();

// Erase last two sectors
uint32_t address = flash_device.get_flash_start() + flash_device.get_flash_size();
uint32_t sector_size, agg_size = 0;
for (uint32_t i = 0; i < 2; i++) {
sector_size = flash_device.get_sector_size(address - 1UL);
TEST_ASSERT_NOT_EQUAL(0, sector_size);
TEST_ASSERT_TRUE(sector_size % page_size == 0);
agg_size += sector_size;
address -= sector_size;
}
ret = flash_device.erase(address, agg_size);
TEST_ASSERT_EQUAL_INT32(0, ret);

address += sector_size - page_size;
uint32_t aligned_prog_size = 2 * page_size;
uint32_t prog_size = aligned_prog_size;
if (page_size > 1) {
prog_size--;
}
uint8_t *data = new uint8_t[aligned_prog_size];
for (uint32_t i = 0; i < prog_size; i++) {
data[i] = rand() % 256;
}
for (uint32_t i = prog_size; i < aligned_prog_size; i++) {
data[i] = 0xFF;
}

ret = flash_device.program(data, address, prog_size);
TEST_ASSERT_EQUAL_INT32(0, ret);

uint8_t *data_flashed = new uint8_t[aligned_prog_size];
ret = flash_device.read(data_flashed, address, aligned_prog_size);
TEST_ASSERT_EQUAL_INT32(0, ret);
TEST_ASSERT_EQUAL_UINT8_ARRAY(data, data_flashed, aligned_prog_size);

delete[] data;
delete[] data_flashed;

ret = flash_device.deinit();
TEST_ASSERT_EQUAL_INT32(0, ret);
}

void flashiap_program_error_test()
{
FlashIAP flash_device;
Expand Down Expand Up @@ -111,12 +161,6 @@ void flashiap_program_error_test()
TEST_ASSERT_EQUAL_INT32(-1, ret);
}

if (flash_device.get_page_size() > 1) {
// unaligned page size
ret = flash_device.program(data, address, page_size + 1);
TEST_ASSERT_EQUAL_INT32(-1, ret);
}

delete[] data;

ret = flash_device.deinit();
Expand All @@ -126,6 +170,7 @@ void flashiap_program_error_test()
Case cases[] = {
Case("FlashIAP - init", flashiap_init_test),
Case("FlashIAP - program", flashiap_program_test),
Case("FlashIAP - program across sectors", flashiap_cross_sector_program_test),
Case("FlashIAP - program errors", flashiap_program_error_test),
};

Expand Down
62 changes: 47 additions & 15 deletions drivers/FlashIAP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
* SOFTWARE.
*/

#include <stdio.h>
#include <string.h>
#include <algorithm>
#include "FlashIAP.h"
#include "mbed_assert.h"

Expand Down Expand Up @@ -57,6 +59,9 @@ int FlashIAP::init()
if (flash_init(&_flash)) {
ret = -1;
}
uint32_t page_size = get_page_size();
_page_buf = new uint8_t[page_size];

_mutex->unlock();
return ret;
}
Expand All @@ -68,6 +73,7 @@ int FlashIAP::deinit()
if (flash_free(&_flash)) {
ret = -1;
}
delete[] _page_buf;
_mutex->unlock();
return ret;
}
Expand All @@ -85,22 +91,43 @@ int FlashIAP::read(void *buffer, uint32_t addr, uint32_t size)
int FlashIAP::program(const void *buffer, uint32_t addr, uint32_t size)
{
uint32_t page_size = get_page_size();
uint32_t current_sector_size = flash_get_sector_size(&_flash, addr);
// addr and size should be aligned to page size, and multiple of page size
// page program should not cross sector boundaries
if (!is_aligned(addr, page_size) ||
!is_aligned(size, page_size) ||
(size < page_size) ||
(((addr % current_sector_size) + size) > current_sector_size)) {
uint32_t flash_size = flash_get_size(&_flash);
uint32_t flash_start_addr = flash_get_start_address(&_flash);
uint32_t chunk, prog_size;
const uint8_t *buf = (uint8_t *) buffer;
const uint8_t *prog_buf;

// addr should be aligned to page size
if (!is_aligned(addr, page_size) || (!buffer) ||
((addr + size) > (flash_start_addr + flash_size))) {
return -1;
}

int ret = 0;
_mutex->lock();
if (flash_program_page(&_flash, addr, (const uint8_t *)buffer, size)) {
ret = -1;
while (size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while reviewing this, this is similar to #6077 (this is however only program, but erase should do the same) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand the comment. Erase seems to be following a similar algorithm as program now. What's the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see the problem now.
My thought here is that the entire check should just be eliminated. Why not erase everything that's in the range, and let the driver worry about alignments, not the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand correctly, an unaligned erase does this?

            --- sector 0 ---  ->  --- sector 0 ---    
            data                  data
            data                  data
            data                  data
            --- sector 1 ---      --- sector 1 ---
            data                  ff
          > data                  ff
unaligned | data                  ff
erase     | --- sector 2 ---      --- sector 2 ---
          > data                  ff
            data                  ff
            data                  ff

That seems like the sort of thing we'd want to error on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said, don't think there should even be an unaligned erase check. Don't even know what it stands for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me rephrase: I believe the only acceptable validation here is to check that the address is on a sector boundary, and that the size is a sum of all the consecutive sector sizes. The only way to achieve this is by iterating on the sectors for the validation, and then iterating again for the erase action. It's either that or check nothing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. Don't let me block this pr then.

Could we check that address + size is on a sector boundary? Would that be cheap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, figured this out after writing the answer. This would be the correct check.

uint32_t current_sector_size = flash_get_sector_size(&_flash, addr);
chunk = std::min(current_sector_size - (addr % current_sector_size), size);
if (chunk < page_size) {
memcpy(_page_buf, buf, chunk);
memset(_page_buf + chunk, 0xFF, page_size - chunk);
prog_buf = _page_buf;
prog_size = page_size;
} else {
chunk = chunk / page_size * page_size;
prog_buf = buf;
prog_size = chunk;
}
if (flash_program_page(&_flash, addr, prog_buf, prog_size)) {
ret = -1;
break;
}
size -= chunk;
addr += chunk;
buf += chunk;
}
_mutex->unlock();

return ret;
}

Expand All @@ -117,10 +144,19 @@ bool FlashIAP::is_aligned_to_sector(uint32_t addr, uint32_t size)

int FlashIAP::erase(uint32_t addr, uint32_t size)
{
uint32_t current_sector_size = 0UL;
uint32_t current_sector_size;
uint32_t flash_size = flash_get_size(&_flash);
uint32_t flash_start_addr = flash_get_start_address(&_flash);
uint32_t flash_end_addr = flash_start_addr + flash_size;
uint32_t erase_end_addr = addr + size;

if (!is_aligned_to_sector(addr, size)) {
if (erase_end_addr > flash_end_addr) {
return -1;
} else if (erase_end_addr < flash_end_addr){
uint32_t following_sector_size = flash_get_sector_size(&_flash, erase_end_addr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this "following" sector or the "last sector size to be erased"? From the code I read it as it is the last sector to be erased.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LiyouZhou please review

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Following means the one after this range. Couldn't find a better name (any recommendation would be welcome).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are making the assumption that: flash sector always start on an address that is aligned to the flash sector size. This is true for all platfroms that I know but I don't know if it is generally true.

I think this will work for the issue i raised. Can we have a testcase to make sure there is no regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also never saw cases where sector addresses weren't aligned to their own sizes. This is pretty easy to fix (subtract flash start from relevant cases). Not sure it justifies a special test (if I understood you correctly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant a test for trying to erase 2 sectors of different sizes at once. basically the situation i describe here: #6077

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. The NUCLEO_F410RB has two last sectors having different sizes (16KB and 64KB). I can change my new test to erase the last two sectors in one call, and this would cover the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the test to erase the two last sectors in one call. This will now check your described scenarios in boards whose last two sector sizes are different (there are a few).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LiyouZhou Can you review the latest changes and leave feedback here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LiyouZhou Would appreciate a quick review, as it seems your review is all that's needed to apply this PR.

if (!is_aligned(erase_end_addr, following_sector_size)) {
return -1;
}
}

int32_t ret = 0;
Expand All @@ -132,10 +168,6 @@ int FlashIAP::erase(uint32_t addr, uint32_t size)
break;
}
current_sector_size = flash_get_sector_size(&_flash, addr);
if (!is_aligned_to_sector(addr, size)) {
ret = -1;
break;
}
size -= current_sector_size;
addr += current_sector_size;
}
Expand Down
5 changes: 3 additions & 2 deletions drivers/FlashIAP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ class FlashIAP : private NonCopyable<FlashIAP> {
* The sectors must have been erased prior to being programmed
*
* @param buffer Buffer of data to be written
* @param addr Address of a page to begin writing to, must be a multiple of program and sector sizes
* @param size Size to write in bytes, must be a multiple of program and sector sizes
* @param addr Address of a page to begin writing to
* @param size Size to write in bytes, must be a multiple of program size
* @return 0 on success, negative error code on failure
*/
int program(const void *buffer, uint32_t addr, uint32_t size);
Expand Down Expand Up @@ -128,6 +128,7 @@ class FlashIAP : private NonCopyable<FlashIAP> {
bool is_aligned_to_sector(uint32_t addr, uint32_t size);

flash_t _flash;
uint8_t *_page_buf;
static SingletonPtr<PlatformMutex> _mutex;
};

Expand Down