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

flash: add note about program size #5000

Merged
merged 1 commit into from Sep 22, 2017

Conversation

Projects
None yet
5 participants
@0xc0170
Member

0xc0170 commented Aug 31, 2017

This unit is the minimum writable size that flash controller supports.

Raised here #4966

@LMESTM @LiyouZhou @c1728p9

@LMESTM

LMESTM approved these changes Aug 31, 2017

LGTM
One question was raised though: is there a risk to actually degrade performance by defining the page size to 1 if the application tries to program page by page ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2017

One question was raised though: is there a risk to actually degrade performance by defining the page size to 1 if the application tries to program page by page ?

I would say yes. @LMESTM there is a PR from you that is setting it to a small number, you can test it by flashing big chunk of data (or do you have datasheets that would have timings for different page sizes) ?

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Aug 31, 2017

@0xc0170 my point is about how people do interpret and use get_page_size as of now in mbed programs.
AS you state in this PR , page_size is the minimum size that can be programmed, but drivers using the API should definitely program several / many "pages" at a time, and I hope they do so.

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_flash_page_doc branch Sep 1, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2017

I rebased, the minimum was not correct, as it invokes a question - what is then maximum. The page size defines a writeable page size - this is the number of bytes that flash controller supports. It could state the optimum writable chunk (in your case it would be set to optimum size).

drivers using the API should definitely program several / many "pages" at a time, and I hope they do so.

FlashIAP is protected with mutex, thus only one program page at the time.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

Any review for this small change? @LMESTM Does it add a value as requested?

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 4, 2017

Looking a bit further, maybe it is possible to add a note like:
"The size to program with flash_program_page shall be a multiple of this page size."

And I think the flash_program_page needs some update as well.
For now it says

/** Program one page starting at defined address
...
int32_t flash_program_page(flash_t *obj, uint32_t address, const uint8_t *data, uint32_t size);

But looking at the code FlashIAP.cpp that calls to flash_program_page, it says

// addr and size should be aligned to page size, and multiple of page size

So I guess that flash_program_page (should be flash_program_pages ?) must actually program a entire number of pages (within a sector). not One

@LiyouZhou

This comment has been minimized.

LiyouZhou commented Sep 4, 2017

should we say minimum programmable size? There is no API that queries the capabilities of the underlying implementation. So i think this api should tell the user what they can do, not what they should do.

flash: add note about program size
Program size - the writable page size that flash controller supports.
Plus fix program page description - multiple pages program

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_flash_page_doc branch to 3f58d3e Sep 6, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 6, 2017

I fixed program_page HAL as suggested above by @LMESTM

@LMESTM

LMESTM approved these changes Sep 6, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 6, 2017

Looks ok with this comment. And now that I better understand the API, maybe "minimum" could even be used, as the "maximum" size is known: this is the number of bytes until next sector boundary:

The pages should not cross multiple sectors.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 7, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 22, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1361

All builds and test passed!

@0xc0170 0xc0170 merged commit 26f9144 into ARMmbed:master Sep 22, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@0xc0170 0xc0170 deleted the 0xc0170:fix_flash_page_doc branch Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment