| Message ID | 1365059554-10662-1-git-send-email-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
Dear Stefan Roese, In message <1365059554-10662-1-git-send-email-sr@denx.de> you wrote: > Sometimes it might make sense to verify the written data to NOR flash. > This patch adds this feature. To enable this verify-after-write, you > need to define CONFIG_SYS_FLASH_VERIFY_AFTER_WRITE in your board > config header. > > Signed-off-by: Stefan Roese <sr@denx.de> > --- > common/flash.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) This is a user selectable feature, thus the option should be CONFIG_FLASH_VERIFY_AFTER_WRITE. Thinking about it - obviously you cannot verify _before_ the write, so all the "after write" is redundant. Better call it just "CONFIG_FLASH_VERIFY". Please also change the error message: Instead printf("\nVerify-after-write failed!\n"); just: printf("\nVerify failed!\n"); Finally - you are introducing a new CONFIG_ option; this must be documented in the README. And it might make sense to add a comment that this option is totally useless in almost all cases, and should only be enabled if you know EXACTLY what you are doing - and that it does not really work even then. Best regards, Wolfgang Denk
Hi Wolfgang, On 04.04.2013 14:46, Wolfgang Denk wrote: > In message <1365059554-10662-1-git-send-email-sr@denx.de> you wrote: >> Sometimes it might make sense to verify the written data to NOR flash. >> This patch adds this feature. To enable this verify-after-write, you >> need to define CONFIG_SYS_FLASH_VERIFY_AFTER_WRITE in your board >> config header. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> --- >> common/flash.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) > > This is a user selectable feature, thus the option should be > CONFIG_FLASH_VERIFY_AFTER_WRITE. Thinking about it - obviously you > cannot verify _before_ the write, so all the "after write" is > redundant. Better call it just "CONFIG_FLASH_VERIFY". Please also > change the error message: > > Instead > > printf("\nVerify-after-write failed!\n"); > > just: > > printf("\nVerify failed!\n"); > > Finally - you are introducing a new CONFIG_ option; this must be > documented in the README. > > And it might make sense to add a comment that this option is totally > useless in almost all cases, and should only be enabled if you know > EXACTLY what you are doing - and that it does not really work even > then. Thanks for your review. Everything you mentioned makes perfect sense. I'll address those issues in v2 of the patch. Thanks, Stefan
diff --git a/common/flash.c b/common/flash.c index 8244ba2..3ae3c9a 100644 --- a/common/flash.c +++ b/common/flash.c @@ -149,6 +149,9 @@ flash_write (char *src, ulong addr, ulong cnt) flash_info_t *info_first = addr2info (addr); flash_info_t *info_last = addr2info (end ); flash_info_t *info; + __maybe_unused char *src_orig = src; + __maybe_unused char *addr_orig = (char *)addr; + __maybe_unused ulong cnt_orig = cnt; if (cnt == 0) { return (ERR_OK); @@ -185,6 +188,14 @@ flash_write (char *src, ulong addr, ulong cnt) addr += len; src += len; } + +#if defined(CONFIG_SYS_FLASH_VERIFY_AFTER_WRITE) + if (memcmp(src_orig, addr_orig, cnt_orig)) { + printf("\nVerify-after-write failed!\n"); + return ERR_PROG_ERROR; + } +#endif /* CONFIG_SYS_FLASH_VERIFY_AFTER_WRITE */ + return (ERR_OK); #endif /* CONFIG_SPD823TS */ }
Sometimes it might make sense to verify the written data to NOR flash. This patch adds this feature. To enable this verify-after-write, you need to define CONFIG_SYS_FLASH_VERIFY_AFTER_WRITE in your board config header. Signed-off-by: Stefan Roese <sr@denx.de> --- common/flash.c | 11 +++++++++++ 1 file changed, 11 insertions(+)