drivers: flash: flash_hp_ra: perform blank check before reading #88113
Conversation
|
Hello @jeremydick, and thank you very much for your first pull request to the Zephyr project! |
github-actions
bot
added
area: Flash
platform: Renesas RA
github-actions
bot
requested review from
de-nordic,
duynguyenxa,
KhiemNguyenT,
soburi and
thaoluonguw
|
@KhiemNguyenT re-assigning to you, your IC. |
drivers/flash/Kconfig.renesas_ra
Outdated
| @@ -30,4 +30,13 @@ config DUAL_BANK_MODE | |||
| help | |||
| Enable dual bank mode | |||
|
|
|||
| config CHECK_BEFORE_READING | |||
| bool "Verify area before reading it" | |||
| default y | |||
There was a problem hiding this comment.
Not all Renesas RA MCU families with flash hp have undefined value after erasing. Can we add erased-undefined properties in dts and enable this config based on it? like default $(dt_nodelabel_bool_prop,flash1,erased-undefined)
There was a problem hiding this comment.
Yes, it looks like the RA4E1, RA4E2, RA4M2, RA4M3, RA6E1, RA6E2, RA6M1, RA6M2, RA6M3, RA6M4, RA6M5, RA8D1, RA8M1, and RA8T1 families have the data flash the will readback an undefined value when erased. Is that all of them, or are there any others I have missed?
There was a problem hiding this comment.
Thanks. Since the erase value for the RA4L1 family is 0xff nothing needs to be done for it, only families that have an undefined erase value need to have the erased-undefined property added
| @@ -89,9 +144,21 @@ static int flash_ra_read(const struct device *dev, off_t offset, void *data, siz | |||
|
|
|||
| LOG_DBG("flash: read 0x%lx, len: %u", (long)(offset + flash_data->area_address), len); | |||
|
|
|||
| memcpy(data, (uint8_t *)(offset + flash_data->area_address), len); | |||
| #ifdef CONFIG_CHECK_BEFORE_READING | |||
There was a problem hiding this comment.
Can we move the data flash region check here instead of inside the is_area_readable. Then, with the code flash region, we won't need to call is_area_readable
There was a problem hiding this comment.
The flash region check has been moved
jeremydick
force-pushed
the
renesas_flash_read_fix
branch
from
67461ac to
a1b4517
Compare
jeremydick
force-pushed
the
renesas_flash_read_fix
branch
2 times, most recently
from
271089f to
52abfff
Compare
|
@KhiemNguyenT , @thaoluonguw , @duynguyenxa , @thenguyenyf, @khoa-nguyen-18 |
…lash Add an erased-undefined property for Renesas RA series MCUS data flash that will read back undefined values when erased Signed-off-by: Jeremy Dick <[email protected]>
The value read from unwritten areas of Renesas RAxxx SoCs data flash is undefined. To prevent reading unwritten areas a blank check command is performed first. If the area is blank, we return dummy data so it behaves the same as other flash devices. Signed-off-by: Jeremy Dick <[email protected]>
jeremydick
force-pushed
the
renesas_flash_read_fix
branch
from
52abfff to
64d6ce4
Compare
| #ifdef CONFIG_CHECK_BEFORE_READING | ||
| } else if (rc == -ENODATA) { | ||
| /* Erased area, return dummy data as an erased page. */ | ||
| memset(data, 0xFF, len); | ||
| rc = 0; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Not in this PR, nevertheless: @jeremydick @khoa-nguyen-18 @thenguyenyf We need to have a discussion on defining -ENODATA return for flash_read API call.
As far as I can see here, your memory has a erasea value (0xff) as a valid write data and erased is separate state.
I think that it may be useful for users to know that, in case of your memory, you can actually detect that the memory has not yet been written at specified location and in such case function should just return without providing bogus data (erase-value filled buffer).
There was a problem hiding this comment.
Hi @de-nordic, thank you for your feedback.
In my opinion, returning -ENODATA isn’t a typical way for users to check if a flash page has been erased. Most applications, including the flash/common test, simply check for (ret == 0) && (read_buffer_value == 0xFF) to confirm that the page is erased.
The issue we're facing is that the Renesas flash in some SoC series didn't have the 0xFF after erase, which causes compatibility problems with this common check. We’ve considered two options to address this:
- Update the Zephyr's flash test/application condition to accommodate different erased values, and make a broad announcement to inform users of Renesas flash drivers that they need to update their condition checks.
- Provide a configuration option in the Renesas flash driver that allows users to choose whether they want the erased buffer to be
0xFF(as proposed in this PR). If this configuration is not selected, we should return-ENODATA(need to update). We believe this makes sense, especially for users porting applications from other MCU vendors to Renesas MCUs who want to avoid making changes to their existing code.
There was a problem hiding this comment.
As I have stated, we will not be able to resolve this in this PR, but we need a discussion on -ENODATA as valid return from flash_read.
Hi @de-nordic, thank you for your feedback.
In my opinion, returning
-ENODATAisn’t a typical way for users to check if a flash page has been erased. Most applications, including theflash/commontest, simply check for(ret == 0) && (read_buffer_value == 0xFF)to confirm that the page is erased.
It is not for checking whether flash page is erased, it is rather for checking whether user has placed data in specified location.
Note that there is code in Zephyr that reads page and compares it against erase-value to figure out whether page has been erased (looping through buffer), so it can now be written. In case of your device the code will not be able to recognize a difference between erased area and area that has been written with erase-value.
The issue we're facing is that the Renesas flash in some SoC series didn't have the
0xFFafter erase, which causes compatibility problems with this common check. We’ve considered two options to address this:1. Update the Zephyr's flash test/application condition to accommodate different erased values, and make a broad announcement to inform users of Renesas flash drivers that they need to update their condition checks. 2. Provide a configuration option in the Renesas flash driver that allows users to choose whether they want the erased buffer to be `0xFF` (as proposed in this PR). If this configuration is not selected, we should return `-ENODATA` (need to update). We believe this makes sense, especially for users porting applications from other MCU vendors to Renesas MCUs who want to avoid making changes to their existing code.
I agree with what you did; what I mean is that we need to clarify definition of flash_read on the matter of returning -ENODATA, modify in tree code (tests, etc) and announce the change, because it is API breaking change. This will take more effort and I do not think it should happen in this PR.
There was a problem hiding this comment.
@de-nordic I agree, longer term it makes the most sense to update the API to allow -ENODATA as a valid return for flash_read, and update all usages of the flash driver within the tree handle that.
The value read from unwritten areas of Renesas RAxxx SoCs data flash is undefined. To prevent reading unwritten areas a blank check command is performed first. If the area is blank, we return dummy data so it behaves the same as other flash devices.