drivers/flash: Add flash_get_parameters call to flash API #25947

de-nordic · 2020-06-03T18:40:37Z

Provided call returns const pointer to flash_parameters structure that can be used for passing memory parameters from drivers.

Resolves #22063

drivers/flash/Kconfig

include/drivers/flash.h

zephyrbot · 2020-06-04T12:55:16Z

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

nvlsianpu

I like how code looks like now.
Is it possible to make erased_val configurable for flash simulator via DTS?
Is it then possible to add test-case with either 0xff and 0x00 erase_value for nvs test (https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/subsys/fs/nvs/testcase.yaml)?

Laczen · 2020-06-16T10:40:13Z

Yes, this is completely what I was thinking. I don't know if it is possible to use a const struct *flash_parameters.

@Laczen How do you want to use it? I do not encourage anyone to modify contents of flash_parameters outside of drivers.

Exactly how you are using it. Probably my remark is a result of limited c knowledge, can you assign the const struct *flash_parameters in nvs_init() ?

de-nordic · 2020-06-16T10:42:54Z

Yes, this is completely what I was thinking. I don't know if it is possible to use a const struct *flash_parameters.

@Laczen How do you want to use it? I do not encourage anyone to modify contents of flash_parameters outside of drivers.

Exactly how you are using it. Probably my remark is a result of limited c knowledge, can you assign the const struct *flash_parameters in nvs_init() ?

Yes, const qualifier 'protects' here the structure as it is in the part that declares pointed type. If you want to protect pointer, you need to move const:
struct *const flash_parameters
or you can have both:
const struct *const flash_parameters

de-nordic · 2020-06-16T10:45:29Z

@pabigot

(...) I'd like to document that the caller has the choice of caching the pointer, or copying the information desired to a different data structure with a guarantee it will never change.

You want this in the doxygen of the flash_get_parameters() ?

pabigot · 2020-06-16T11:08:14Z

(...) I'd like to document that the caller has the choice of caching the pointer, or copying the information desired to a different data structure with a guarantee it will never change.

You want this in the doxygen of the flash_get_parameters() ?

I think so, yes, assuming you and others think this is worth making clear. I'd like to have this API raised during the API telecon today for visibility. But also we should confirm that nobody has a use case for the values changing for a given device while the system is running.

de-nordic · 2020-06-16T11:16:19Z

I think so, yes, assuming you and others think this is worth making clear. I'd like to have this API raised during the API telecon today for visibility. But also we should confirm that nobody has a use case for the values changing for a given device while the system is running.

I do not think there is a case for the contents of flash_parameters to get modified at runtime. I will add the note to documentation, but we will need to wait until it gets clarified.

Laczen · 2020-06-16T11:29:36Z

Yes, this is completely what I was thinking. I don't know if it is possible to use a const struct *flash_parameters.

@Laczen How do you want to use it? I do not encourage anyone to modify contents of flash_parameters outside of drivers.

Exactly how you are using it. Probably my remark is a result of limited c knowledge, can you assign the const struct *flash_parameters in nvs_init() ?

Yes, const qualifier 'protects' here the structure as it is in the part that declares pointed type. If you want to protect pointer, you need to move const:
struct *const flash_parameters
or you can have both:
const struct *const flash_parameters

OK, thanks for the clarification

nvlsianpu · 2020-06-16T11:46:06Z

NVS changes looks ok

erwango · 2020-06-16T12:49:30Z

drivers/flash/flash_stm32.c

This doesn't apply to all series.
On a handfull, this should be 0x00.
I'll get back to you with the references

OK, thanks for looking into this.

@utzig has mentioned same thing here: #25947 (comment) though I do not know how does it apply to our stm32 drivers.

@erwango Can you advise me on the stm32 characteristic values for erase_value?

@de-nordic After check, only L0/L1 use 0x00 as erase value (I guess reason is they have an EEPROM and this is easier to use same erase value for both).
Since L0/L1 have no flash driver yet, proposed code is fine. Maybe adding a warning comment in case of L0/L1 future addition would be a nice thing though.

@erwango Thanks for the information. Will add warning information.

There is a PR for an L0 driver (#25245) that will hit this issue. The easiest way to distinguish between the EEPROM and FLASH implementations is probably to check for the FLASH_CR_LOCK (erase value 0xff) or FLASH_PECR_PELOCK (erase value 0x00). It's not exactly pretty, but it avoids hard-coding the SOC family.

There is a PR for an L0 driver (#25245) that will hit this issue. The easiest way to distinguish between the EEPROM and FLASH implementations is probably to check for the FLASH_CR_LOCK (erase value 0xff) or FLASH_PECR_PELOCK (erase value 0x00). It's not exactly pretty, but it avoids hard-coding the SOC family.

@andysan, does this mean that also for L0 the flash erase value is 0xff?

carlescufi · 2020-06-19T16:28:01Z

@pabigot @nvlsianpu could you take another look please?

pabigot

OK as is, a couple minor cleanups identified.

pabigot · 2020-06-19T17:52:36Z

drivers/flash/flash_simulator.c

Placement of this one isn't the same as the others.

pabigot · 2020-06-19T17:56:04Z

include/drivers/flash.h

I'd make this end at "copy its contents." The rest is opinion that isn't good advice for some uses. If you want it anyway just change "if preferred" to "is the preferred".

de-nordic · 2020-06-19T21:07:21Z

@Laczen @pabigot Thanks for reviewing.

Adds flash_get_parameters call to API that returns pointer to structure describing flash parameters. Currently only erase_value parameter is provided via the structure. Signed-off-by: Dominik Ermel <[email protected]>

With addition of flash_get_parameters API call, it is needed to provide support for the API to flash drivers. Signed-off-by: Dominik Ermel <[email protected]>

Use new flash API call to obtain erase value instead of relaying on hardcoded literals. Signed-off-by: Dominik Ermel <[email protected]>

With addition of flash_parameters structure, and supporting API call to retrieve it, it is no longer needed to store write_block_size as a part of flash_driver_api and it should be part of flash_parameters. Signed-off-by: Dominik Ermel <[email protected]>

Pointer to flash_parameters have been added to nvs_fs structure and it is no longer needed to store write_block_size within the nvs_fs. Signed-off-by: Dominik Ermel <[email protected]>

de-nordic requested review from hakonfam and nvlsianpu and removed request for nvlsianpu June 3, 2020 18:40

de-nordic commented Jun 3, 2020

View reviewed changes

drivers/flash/Kconfig Outdated Show resolved Hide resolved

de-nordic self-assigned this Jun 3, 2020

de-nordic added area: Flash area: Drivers area: API Changes to public APIs labels Jun 3, 2020

zephyrbot added area: File System area: Tests Issues related to a particular existing or missing test labels Jun 3, 2020

nvlsianpu suggested changes Jun 4, 2020

View reviewed changes

drivers/flash/Kconfig Outdated Show resolved Hide resolved

include/drivers/flash.h Outdated Show resolved Hide resolved

de-nordic force-pushed the flash-api-get-erase-val branch 4 times, most recently from eaaef7e to 5c9169e Compare June 4, 2020 12:45

de-nordic force-pushed the flash-api-get-erase-val branch from 5c9169e to 2560b16 Compare June 4, 2020 15:03

de-nordic requested a review from nvlsianpu June 5, 2020 04:52

nvlsianpu requested a review from Laczen June 5, 2020 06:40

nvlsianpu suggested changes Jun 5, 2020

View reviewed changes

de-nordic force-pushed the flash-api-get-erase-val branch from 2560b16 to 4932e9a Compare June 8, 2020 11:03

zephyrbot added area: Devicetree area: Boards area: native port Host native arch port (native_sim) labels Jun 8, 2020

nvlsianpu added the Enhancement Changes/Updates/Additions to existing features label Jun 8, 2020

de-nordic force-pushed the flash-api-get-erase-val branch from 4932e9a to e0dc705 Compare June 8, 2020 12:00

de-nordic requested a review from nvlsianpu June 8, 2020 12:01

de-nordic force-pushed the flash-api-get-erase-val branch from e0dc705 to 6e12dbd Compare June 8, 2020 12:06

de-nordic marked this pull request as ready for review June 8, 2020 12:08

de-nordic force-pushed the flash-api-get-erase-val branch from e76e380 to e13c9d8 Compare June 16, 2020 11:42

erwango reviewed Jun 16, 2020

View reviewed changes

de-nordic mentioned this pull request Jun 19, 2020

STM32 flash driver cleanup and support for STM32L0 #25245

Merged

de-nordic force-pushed the flash-api-get-erase-val branch from e13c9d8 to 68607a7 Compare June 19, 2020 14:29

pabigot approved these changes Jun 19, 2020

View reviewed changes

de-nordic force-pushed the flash-api-get-erase-val branch from 68607a7 to eba084c Compare June 19, 2020 19:44

de-nordic requested a review from nvlsianpu June 19, 2020 19:44

Laczen approved these changes Jun 19, 2020

View reviewed changes

de-nordic force-pushed the flash-api-get-erase-val branch from eba084c to 051eeaa Compare June 22, 2020 07:10

nvlsianpu approved these changes Jun 22, 2020

View reviewed changes

de-nordic added 5 commits June 22, 2020 10:05

drivers/flash: Add API call returning flash parameters

aad5528

Adds flash_get_parameters call to API that returns pointer to structure describing flash parameters. Currently only erase_value parameter is provided via the structure. Signed-off-by: Dominik Ermel <[email protected]>

drivers/flash: Add support for flash_get_parameters to drivers

4e9e14d

With addition of flash_get_parameters API call, it is needed to provide support for the API to flash drivers. Signed-off-by: Dominik Ermel <[email protected]>

subsys/fs/nvs: Use flash_get_parameters() to get erase value

c273621

Use new flash API call to obtain erase value instead of relaying on hardcoded literals. Signed-off-by: Dominik Ermel <[email protected]>

subsys/fs/nvs: Move write_block_size to flash_parameters

a946de6

Pointer to flash_parameters have been added to nvs_fs structure and it is no longer needed to store write_block_size within the nvs_fs. Signed-off-by: Dominik Ermel <[email protected]>

de-nordic force-pushed the flash-api-get-erase-val branch from 051eeaa to a946de6 Compare June 22, 2020 10:07

carlescufi approved these changes Jun 22, 2020

View reviewed changes

carlescufi merged commit 38f623d into zephyrproject-rtos:master Jun 22, 2020

de-nordic deleted the flash-api-get-erase-val branch June 25, 2020 12:07

de-nordic mentioned this pull request Jun 26, 2020

fs/NVS: NVS is not compatible with flash memories which have 0x00 as erased #22063

Closed

erwango mentioned this pull request Oct 14, 2021

mcuboot not upgrade for stm32l1 series #39423

Closed

drivers/flash: Add flash_get_parameters call to flash API #25947

drivers/flash: Add flash_get_parameters call to flash API #25947

Uh oh!

Conversation

de-nordic commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zephyrbot commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvlsianpu left a comment

Choose a reason for hiding this comment

Uh oh!

Laczen commented Jun 16, 2020

Uh oh!

de-nordic commented Jun 16, 2020

Uh oh!

de-nordic commented Jun 16, 2020

Uh oh!

pabigot commented Jun 16, 2020

Uh oh!

de-nordic commented Jun 16, 2020

Uh oh!

Laczen commented Jun 16, 2020

Uh oh!

nvlsianpu commented Jun 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Jun 19, 2020

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

de-nordic commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

de-nordic commented Jun 3, 2020 •

edited

Loading

zephyrbot commented Jun 4, 2020 •

edited

Loading