flash: fix flash_read API for NXP LPC55S36
#80351
Conversation
zephyrbot
requested review from
danieldegrasse,
dbaluta,
de-nordic,
decsny,
manuargue and
MarkWangChinese
|
Friendly ping for any feedback after a week of silence 🙂 |
drivers/flash/soc_flash_mcux.c
Outdated
| } else { | ||
| /* Check whether the flash is erased ("len" and "addr" must be word-aligned). */ | ||
| rc = FLASH_VerifyErase(&priv->config, ((addr + 0x3) & ~0x3), ((len + 0x3) & ~0x3)); | ||
| #if defined(CONFIG_CHECK_BEFORE_READING) && !defined(CONFIG_SOC_LPC55S36) |
There was a problem hiding this comment.
A suggestion here for readability- could we do something like this:
if (IS_ENABLED(CONFIG_CHECK_BEFORE_READING)) {
if (IS_ENABLED(CONFIG_SOC_LPC55S36) {
/* LPC55S36 specific logic */
} else {
rc = is_area_readable(addr, len)
}
}
if (!rc) {
memcpy(data, (void *)addr, len);
} else if ((rc == -ENODATA) && IS_ENABLED(CONFIG_CHECK_BEFORE_READING)) {
/* Erased area, return dummy data as an erased page. */
memset(data, 0xFF, len);
rc = 0;
}
return rc;
Apologies if formatting there is confusing. At least IMO, the current function implementation in main is hard to follow, and I figure if we are fixing the bug cleaning up the readability wouldn't hurt either
There was a problem hiding this comment.
Completely agree, but it's really a thorny situation. The FLASH_Read does a "copy before verify" which doesn't neatly fit in the previous model:
CHECK is set? |
on 55S36? |
Check stage | Copy stage | 0xff fill |
|---|---|---|---|---|
| no | - | no | memcpy |
no |
| yes | no | is_area_readable |
memcpy |
yes |
| yes | yes | FLASH_Read + VerifyErased |
(already done) | yes |
My first proposal avoided any duplicate code, but ended up creating awkward logical groupings.
There was a problem hiding this comment.
Thank you for the explanation here, this makes more sense (and I really like the rework in the latest push, it is a lot easier to follow)
pillo79
force-pushed
the
pr-lpc55s36-fix-flash_read
branch
from
2766e22 to
ec14af1
Compare
pillo79
force-pushed
the
pr-lpc55s36-fix-flash_read
branch
from
ec14af1 to
292a0e5
Compare
|
v2:
|
drivers/flash/soc_flash_mcux.c
Outdated
| * Ensure the area is readable, since a direct access may cause faults on erased | ||
| * or otherwise unreadable pages. Emulate erased pages, return other errors. | ||
| */ | ||
| if (IS_ENABLED(CONFIG_SOC_LPC55S36)) { |
There was a problem hiding this comment.
@pillo79 it looks like we may need more "#ifdefery", because some older NXP platforms don't define the FLASH_Read API at all. I think we probably need to use #ifdef here. Sorry for the poor guidance
|
On the next push can you reference the github issue within the commit message? IE add
|
As other targets in the LPC55xxx series, the LPC55S36 has a Flash controller that raises ECC errors when reading erased pages directly. To avoid this, there is special code for this platform that calls the HAL FLASH_IsFlashAreaReadable() function. However, this in turn calls a function at an hardcoded address in ROM that _always_ causes an instruction fault, making the situation worse. This patch reworks the read operation to use the FLASH_Read() HAL function for this target to gracefully handle error conditions and properly emulate accesses to erased pages. The preprocessor is required since some targets do not define the FLASH_Read() function. Fixes: zephyrproject-rtos#80325 Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
pillo79
force-pushed
the
pr-lpc55s36-fix-flash_read
branch
from
292a0e5 to
9d78c65
Compare
|
v3:
OT: not sure why Twister decided to run 5000+ tests this time? 😅 🤷♂️ |
The current alignment logic does not work as expected if given unaligned values, resulting in a skip of the first word. The length also has to take into account the starting address: for example, asking for 2 bytes at offset 3 should actually check 8 bytes. This patch adjusts the logic so that it always includes the first and the last word of the input area. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
|
Minor addition I spotted only now - fixed the alignment logic for |
|
@de-nordic can you take a look? |
| /* Check id the ECC issue is due to the Flash being erased | ||
| * ("addr" and "len" must be word-aligned for this call). | ||
| */ | ||
| rc = FLASH_VerifyErase(&priv->config, |
There was a problem hiding this comment.
Question: wouldn't it be better to just ifdef the is_area_readable internally to use this method of verification, whether memory is readable, for CONFIG_SOC_LPC55S36, and then follow it with the memcopy or memset depending on result? Instead of reading first and then checking why it failed, and have streamlined code fall all devices supported?
As far as I understand the description for FLASH_VerifyErase in fsl_iap.h, it is able to tell whether there is ECC problem in range (btw NXP, the function no longer takes margin as parameter, yet you still mention it in description).
Additional note: IMHO making read check configurable makes no sense, because there is always a chance that ECC error will topple device instead of returning with error, unless of course you disable ECC,
There was a problem hiding this comment.
Looks OK, as far as I can tell.
Left a comment on potential place for streamlining execution here https://github.com/zephyrproject-rtos/zephyr/pull/80351/files#r1831641313, but that is up to the vendor here.
This PR reworks the LPC55S36 implementation of the
flash_mcux_read()function to work around issue #80325. Feedback welcome!This is achieved on the
CONFIG_CHECK_BEFORE_READINGpath (the default) by using theFLASH_Read()HAL API call, which is resilient to errors, and map the error codes to properly emulate an erased page read.When
CONFIG_CHECK_BEFORE_READINGis disabled, the code reverts to the previousmemcpy()operation, which is faster but unsafe on erased or otherwise unreadable pages.#ifdefguards kept in the style of the file, though some may be changed toif (IS_ENABLED(...))if desired.Fixes #80325