Skip to content

FlashIAP: Fix problem of programming source buffer not aligned to 4 #6864

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.

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
May 14, 2018

Conversation

davidsaada
Copy link
Contributor

Description

This PR resolves a problem (found by @yossi2le) when programming the internal flash via FlashIAP: The program API receives a const void *buffer parameter as the source buffer to program. As such, the program buffer can point to data of any kind, without alignment restrictions. However, many of the target specific flash drivers assume this buffer is aligned to uint32_t and thus cast it to uint32_t*.
Freescale's flash_api module shows a typical example for that. Other targets do the same as well.
Solution is by using an aligned buffer for this case in FlashIAP. This was already implemented for unaligned program sizes. Same code is now used for the unaligned source buffer case.
This PR also modifies the FlashIAP programming test code, as the previous one was quite trivial and didn't check this scenario as well.
This fix also resolves issue #6706.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

// 1. Size is not page aligned
// 2. Source buffer is not aligned to uint32_t. This is not supported by many targets (although
// the pointer they accept is of uint8_t).
if (unaligned_src || (chunk < page_size)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on just copying to the internal buffer for all program calls?

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 very efficient. I suspect that the vast majority of calls will have both buffer and size aligned.

Copy link
Contributor

@geky geky left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@geky geky requested a review from 0xc0170 May 10, 2018 15:06
@cmonr
Copy link
Contributor

cmonr commented May 11, 2018

/morph build

@@ -22,6 +22,7 @@
#include "utest/utest.h"
#include "unity/unity.h"
#include "greentea-client/test_env.h"
#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is this importing? This is the first time I've ever seen a system file simply called "algorithm".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually used quite a lot. It imports the std::min and std::max functions.

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

Build : SUCCESS

Build number : 1978
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6864/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

Test : SUCCESS

Build number : 1791
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6864/1791

@davidsaada
Copy link
Contributor Author

Morph exporter needs to be restarted. Looks like failure is due related to tools.

@cmonr
Copy link
Contributor

cmonr commented May 11, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

@cmonr cmonr merged commit 15ff9a8 into ARMmbed:master May 14, 2018
@davidsaada davidsaada deleted the david_flashiap_unaligned_src branch July 9, 2018 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants