[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix
From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001 From: Renato Andreola renato.andreola@imagos.it Date: Fri, 21 Aug 2009 18:05:51 +0200 Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
Signed-off-by: Alessandro Rubini rubini@gnudd.com Renato Andreola renato.andreola@imagos.it --- drivers/mtd/cfi_flash.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 81ac5d3..0d8fc54 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector, ulong start;
#if CONFIG_SYS_HZ != 1000 - tout *= CONFIG_SYS_HZ/1000; -#endif - + if ((ulong)CONFIG_SYS_HZ > 100000) + tout *= (ulong)CONFIG_SYS_HZ/1000; /* for a big HZ, avoid overflow */ + else + tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000); +#endif /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) {
Renato Andreola wrote:
From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001 From: Renato Andreola renato.andreola@imagos.it Date: Fri, 21 Aug 2009 18:05:51 +0200 Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
Signed-off-by: Alessandro Rubini rubini@gnudd.com Renato Andreola renato.andreola@imagos.it
drivers/mtd/cfi_flash.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 81ac5d3..0d8fc54 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector, ulong start;
#if CONFIG_SYS_HZ != 1000
- tout *= CONFIG_SYS_HZ/1000;
-#endif
- if ((ulong)CONFIG_SYS_HZ > 100000)
tout *= (ulong)CONFIG_SYS_HZ/1000; /* for a big HZ, avoid overflow */- else
tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);+#endif /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) {
What should to be fixed first in this case, would be your CONFIG_SYS_HZ setting, that is NIOS2? timer implementation, yeah really. But I would also point out that there is another case flash_status_check() doensn't work as expected.
One of my colleagues found that with some flash device(s) (I don't recall precisely, sorry), 'tout' would be probed to be zero. In that case, a workaround something like above still doesn't work.
We have not sorted out where the problem is; it might be in cfi_flash.c, or in the flash device itself. This is observed with v2009.03 release, and we've been having a workaround for it for months. I'd like to have a look someday.
Anyway, checking to see if 'tout' is zero or not would be sometimes worth a try, when you think cfi_flash.c doesn't work as expected.
Just for your information,
This patch is necessary to fix the common case in which CONFIG_SYS_HZ is just below 1000 (e.g. 999 that results from an integer division between a system clock frequency like 83333333Hz and a divisor like 83334). In that case the CONFIG_SYS_HZ/1000 expression returns 0 and the tout *= CONFIG_SYS_HZ/1000 multiplication is always 0. As you said, an incorrect Common Flash Interface interrogation of the flash registers can lead to a tout of 0 but this misbehavior should have been caught by the first stages of the flash interrogation code. Note that the AMD/Spansion chips does not work correctly in a Nios2 environment due to "spurious" read cycles generated when flash bus width is lower than 32. If the flash chip you used is not an Intel/Numoyx than you should have to do some work to make them behave correctly (e.g. add extra wait state into the procedures or some other tricks).
Renato
Shinya Kuribayashi wrote:
Renato Andreola wrote:
From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001 From: Renato Andreola renato.andreola@imagos.it Date: Fri, 21 Aug 2009 18:05:51 +0200 Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
Signed-off-by: Alessandro Rubini rubini@gnudd.com Renato Andreola renato.andreola@imagos.it
drivers/mtd/cfi_flash.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 81ac5d3..0d8fc54 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector, ulong start;
#if CONFIG_SYS_HZ != 1000
- tout *= CONFIG_SYS_HZ/1000;
-#endif
- if ((ulong)CONFIG_SYS_HZ > 100000)
tout *= (ulong)CONFIG_SYS_HZ/1000; /* for a big HZ, avoid overflow */- else
tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);+#endif /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) {
What should to be fixed first in this case, would be your CONFIG_SYS_HZ setting, that is NIOS2? timer implementation, yeah really. But I would also point out that there is another case flash_status_check() doensn't work as expected.
One of my colleagues found that with some flash device(s) (I don't recall precisely, sorry), 'tout' would be probed to be zero. In that case, a workaround something like above still doesn't work.
We have not sorted out where the problem is; it might be in cfi_flash.c, or in the flash device itself. This is observed with v2009.03 release, and we've been having a workaround for it for months. I'd like to have a look someday.
Anyway, checking to see if 'tout' is zero or not would be sometimes worth a try, when you think cfi_flash.c doesn't work as expected.
Just for your information,
On Monday 24 August 2009 10:32:17 Renato Andreola wrote:
From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001 From: Renato Andreola renato.andreola@imagos.it Date: Fri, 21 Aug 2009 18:05:51 +0200 Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
Signed-off-by: Alessandro Rubini rubini@gnudd.com Renato Andreola renato.andreola@imagos.it
A few mostly formal comments about this patch:
- Patch doesn't apply:
[stefan@stefan-desktop u-boot-cfi-flash (master)]$ git am -s patches_misc/[U- Boot]\ PATCH\ mtd\ CFI\ flash:\ timeout\ calculation\ underflow\ if\ imprecise\ 1kHz\ timer:\ fix.mbox Applying: PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix /home/stefan/git/u-boot/u-boot-cfi-flash/.git/rebase-apply/patch:20: trailing whitespace. #endif error: patch failed: drivers/mtd/cfi_flash.c:660 error: drivers/mtd/cfi_flash.c: patch does not apply
Please fix by using "git format-patch" and "git send-email".
- Patch subject is mangled:
Applying your patch (if possible) would lead to this commit text/subject: "PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix". This is because your email subject is incorrect (e.g. PATCH should be [PATCH]). And the subject it too long as well. Please use a shorter line here.
- Minor coding style problem (see below)
drivers/mtd/cfi_flash.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 81ac5d3..0d8fc54 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector, ulong start;
#if CONFIG_SYS_HZ != 1000
- tout *= CONFIG_SYS_HZ/1000;
-#endif
- if ((ulong)CONFIG_SYS_HZ > 100000)
tout *= (ulong)CONFIG_SYS_HZ/1000; /* for a big HZ, avoid
overflow */
Please insert spaces before and after the "/" above.
- else
tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);+#endif /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) {
Thanks.
Cheers, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
On Tuesday 25 August 2009 17:39:56 Stefan Roese wrote:
- if ((ulong)CONFIG_SYS_HZ > 100000)
tout *= (ulong)CONFIG_SYS_HZ/1000; /* for a big HZ, avoidoverflow */
Please insert spaces before and after the "/" above.
- else
tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);
And spaces before and after the "*" above as well.
Thanks.
Cheers, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Dear Renato Andreola,
In message 4A925011.9080906@imagos.it you wrote:
From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001 From: Renato Andreola renato.andreola@imagos.it Date: Fri, 21 Aug 2009 18:05:51 +0200 Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
Signed-off-by: Alessandro Rubini rubini@gnudd.com Renato Andreola renato.andreola@imagos.it
I think we are still waiting for your resubmit? Is this correct?
Best regards,
Wolfgang Denk
Dear Wolfgang Denk, I've sent the last patch regarding the CFI timeout underflow on 24th August 2009 and then I Have done no more activity on U-Boot for Nios2. I've done no more activity regarding the cache flush in lib_nios2/bootm.c too. As I remember, the problems with the 2 patches were related to code white space formatting and a copyright note that can be deleted.
I hope I'll have time to go on with the activity on nios2 starting from the beginning of 2010.
Best regards, Renato Andreola
Wolfgang Denk wrote:
Dear Renato Andreola,
In message 4A925011.9080906@imagos.it you wrote:
From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001 From: Renato Andreola renato.andreola@imagos.it Date: Fri, 21 Aug 2009 18:05:51 +0200 Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
Signed-off-by: Alessandro Rubini rubini@gnudd.com Renato Andreola renato.andreola@imagos.it
I think we are still waiting for your resubmit? Is this correct?
Best regards,
Wolfgang Denk
Dear Renato Andreola,
In message 4B0A4D04.4030901@imagos.it you wrote:
I've sent the last patch regarding the CFI timeout underflow on 24th August 2009 and then I Have done no more activity on U-Boot for Nios2. I've done no more activity regarding the cache flush in lib_nios2/bootm.c too. As I remember, the problems with the 2 patches were related to code white space formatting and a copyright note that can be deleted.
Yes, and we are waiting for you to resubmit a cleaned up patch. Maybe you can squeeze this in somehow?
Best regards,
Wolfgang Denk
participants (4)
-
Renato Andreola -
Shinya Kuribayashi -
Stefan Roese -
Wolfgang Denk