Skip to content
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

Reduce flash page size from 512 to 32 bytes in PSOC6 based boards #9940

Merged
merged 1 commit into from Mar 6, 2019

Conversation

@davidsaada
Copy link
Contributor

commented Mar 5, 2019

Description

Page size in all PSOC6 boards is 512 bytes. This is very problematic in all storage applications. This change reduces the page size (in flash_api's flash_program_page API) to 32 by reading the original page, modifying it with programmed data and programming it back. The number 32 for page size conforms to the number of times (16) this action can be done.

Tests:
FUTURE_SEQUANA_PSA - FlashIAP driver, all PSA ITS compliance tests.
CY8CKIT_062_BLE (with added FLASHIAP component) - FlashIAP driver, KVStore static tests, NVStore.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Release Notes

Reduce flash page size from 512 to 32 bytes in PSOC6 based boards

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@davidsaada, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-maintainers Mar 5, 2019

@NirSonnenschein NirSonnenschein requested a review from maclobdell Mar 5, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@maclobdell could you please notify the relevant people from the cypress team

@lrusinowicz

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

What about the number of flash sector erases? This change will reduce the usefull lifetime of flash about 16 times, won't it?
It will also slow down the write 16 times making it dramatically slow.

@0xc0170 0xc0170 requested a review from ARMmbed/team-cypress Mar 5, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

What about the number of flash sector erases? This change will reduce the usefull lifetime of flash about 16 times, won't it?

On the contrary, it will reduce the flash wearing by 16 times. Let me try and explain it using a numerical example:
Let's say we have a 16KB flash component, to which we wish to save circular data of 32 bytes items.
Now, without this change, we have a 512 page size. This means that we can only write each of these items on a single page. This means that we'll exhaust our flash after 32 writes and will need to start erasing sectors after that time.
With this change, we will write those items on the erased portion of the 512 pages. This means that we can use each physical page to store 16 items. In this case, we will exhaust the flash after 512 (32*16) times. Sector erase rate will be 16 times slower.

@hennadiykytsun

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

You can't change page (row) size, because write API does write of the whole row = 512 bytes.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

You can't change page (row) size, because write API does write of the whole row = 512 bytes.

This PR doesn't change the physical page size, but the one reflected to the user of this driver. The user will now be able to write pages of 32 bytes, while the driver read the 512 byte physical page, modify the 32 bytes portion in the read data and write the 512 page again.

@lrusinowicz

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

OK, I haven't noticed you are now making page size != sector size.
So this should work and it should slow the flash writes only a bit and the overall performance should be much better - it's erase operation that is slow.
The only thing that might be required is to flush flash cache after each write.

@hennadiykytsun

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Each Flash sector has 512 row. The current flash driver implementation does not have APIs to erase whole sector (512 rows) and subsector (8 rows).

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

OK, I haven't noticed you are now making page size != sector size.
So this should work and it should slow the flash writes only a bit and the overall performance should be much better - it's erase operation that is slow.
The only thing that might be required is to flush flash cache after each write.

Exactly. Was going to answer your added question above, but you figured it out before that.
I'm not sure I understand what you said about flushing. I use the same Cy_Flash_ProgramRow API when programming the rows. Why should it differ from the previous state?

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Each Flash sector has 512 row. The current flash driver implementation does not have APIs to erase whole sector (512 rows) and subsector (8 rows).

Not sure I understand what you're saying here. Erase API wasn't changed (still erases 512 byte sectors). Only change was in flash_program_page, which physically keeps programming with pages of 512 bytes, with the difference allowing the user to program 32 bytes.

@hennadiykytsun

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

  1. The FLASH page should be in erased state prior to calling ProgramRow API.
  2. We are going to develop the EraseSector and EraseSubSector APIs, it will do the Flash erase operation easiest.
  3. You did not change the page size, you just do the modification of the data that is going to be written into the Flash. If it is make sense for you it will work, because you do read->modify->write of the whole row. Just you need to care about item #1.
@lrusinowicz

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@hennadiykytsun
We are not talking about the same sectors.
Mbed flash sector is PSOC6 flash row (so it has nothing to do with PSOC6 flash sector). Sorry for the confusion.

@davidsaada

I'm not sure I understand what you said about flushing. I use the same Cy_Flash_ProgramRow API when programming the rows. Why should it differ from the previous state?

Cypress API states, that flash cache should be invalidated with Cy_SysLib_ClearFlashCacheAndBuffer() before reading flash row after it has been erased/programmed. I have missed this previously and the whole thing might be working just by chance. And now, with effectively read-modify-write operation it may not.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

1. The FLASH page should be in erased state prior to calling ProgramRow API.

2. We are going to develop the EraseSector and EraseSubSector APIs, it will do the Flash erase operation easiest.

3. You did not change the page size, you just do the modification of the data that is going to be written into the Flash. If it is make sense for you it will work, because you do read->modify->write of the whole row. Just you need to care about item #1.

Maybe the headline is deceiving, but again - I indeed didn't change the page size, just the program size as reflected to the user. What I do there is indeed read-modify-write, which makes the user think that the page size is 32.
Regarding the row being in erase state before programming the row - true. However, it's the user's responsibility. I didn't change anything here - calling program without erasing would have failed before my change and will fail the same way after it.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@hennadiykytsun
We are not talking about the same sectors.
Mbed flash sector is PSOC6 flash row (so it has nothing to do with PSOC6 flash sector). Sorry for the confusion.

@davidsaada

I'm not sure I understand what you said about flushing. I use the same Cy_Flash_ProgramRow API when programming the rows. Why should it differ from the previous state?

Cypress API states, that flash cache should be invalidated with Cy_SysLib_ClearFlashCacheAndBuffer() before reading flash row after it has been erased/programmed. I have missed this previously and the whole thing might be working just by chance. And now, with effectively read-modify-write operation it may not.

I don't mind calling Cy_SysLib_ClearFlashCacheAndBuffer after each program, but again, unless I'm missing big time here, I don't understand the difference between current code and the previous one, as far as the flash is concerned, As I see it, the only difference from the flash point of view is the fact that I'm reading from the flash prior to the program. Is this what makes the difference?

@lrusinowicz

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

As I see it, the only difference from the flash point of view is the fact that I'm reading from the flash prior to the program. Is this what makes the difference?

First, based on the description of Cypress Flash API:

  • \note Before reading data from previously programmed/erased flash rows, the
  • user must clear the flash cache with the Cy_SysLib_ClearFlashCacheAndBuffer()
  • function.

I know think, that original flash_program_page() should have called this clearing function in the first place. This cache (and function) is not well documented. Original implementation may be working just because read accesses to the written data (e.g. for verification) has been typically separated from the write operation with multiple instruction reads (same flash), that happened to expel previous row data from the cache. But again, I don't know the internal details of Cypress flash operations.
Now, if you happen to write consecutive pages, you will be reading and writing the same flash row without much separation, and the bug may manifest itself.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

As I see it, the only difference from the flash point of view is the fact that I'm reading from the flash prior to the program. Is this what makes the difference?

First, based on the description of Cypress Flash API:

  • \note Before reading data from previously programmed/erased flash rows, the
  • user must clear the flash cache with the Cy_SysLib_ClearFlashCacheAndBuffer()
  • function.

I know think, that original flash_program_page() should have called this clearing function in the first place. This cache (and function) is not well documented. Original implementation may be working just because read accesses to the written data (e.g. for verification) has been typically separated from the write operation with multiple instruction reads (same flash), that happened to expel previous row data from the cache. But again, I don't know the internal details of Cypress flash operations.
Now, if you happen to write consecutive pages, you will be reading and writing the same flash row without much separation, and the bug may manifest itself.

Thanks, it's clearer now. So will use this PR to fix current logic.

@davidsaada davidsaada force-pushed the davidsaada:david_psoc6_reduce_prog_size branch Mar 6, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Added call to Cy_SysLib_ClearFlashCacheAndBuffer at the end of flash program.
@lrusinowicz please re-review.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@hennadiykytsun
Copy link
Contributor

left a comment

I am OK with this update, but Erase row was not done before Program row. It is possible place for bugs.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@ARMmbed/mbed-os-maintainers Travis failures seem unrelated to the PR (tried couple of restarts).

Reduce flash page size from 512 to 32 bytes in PSOC6 based boards
Page size in all PSOC6 boards is 512 bytes. This is very problematic in
all storage applications. This change reduces the page size (in flash_api's
flash_program_page API) to 32 by reading the original page, modifying it
with programmed data and programming it back. The number 32 for page size
conforms to the number of times (16) this action can be done.

@0xc0170 0xc0170 requested a review from ChiefBureaucraticOfficer Mar 6, 2019

@davidsaada davidsaada force-pushed the davidsaada:david_psoc6_reduce_prog_size branch to 9cacd02 Mar 6, 2019

@0xc0170

0xc0170 approved these changes Mar 6, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Just pushed a small modification to the program logic, as it could get a size bigger than page size.
(verified with SEQUANA & CY8CKIT_062_BLE boards)

@dannybenor
Copy link

left a comment

Please check line 53 in flash_api.c

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Please check line 53 in flash_api.c

Are you sure? It's a variable declaration (maybe you need to refresh).

@ChiefBureaucraticOfficer
Copy link

left a comment

Meets PM criteria. Approved.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Mar 6, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 6, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 6, 2019

@cmonr cmonr merged commit f3ecc0b into ARMmbed:master Mar 6, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9151 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 6, 2019

@davidsaada davidsaada deleted the davidsaada:david_psoc6_reduce_prog_size branch Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.