[U-Boot-Users] PATCH for drivers/cfi_flash.c
This patch is prepared against current CVS:
* Patch by Tolunay Orkun, 06 May 2005: Fixes for drivers/cfi_flash.c: - Fix wrong timeout value usage in flash_status_check() - Fix incorrect if condition in flash_full_status_check() - Remove clearing flash status at the end of flash_write_cfibuffer() which set Intel 28F640J3 flash back to command mode on CSB472
~~~~
Verbose description of fixes:
Fix #1: flash_status_check() receives the timeout as a parameter. Existing code is using erase_blk_tout which is too conservative for some operations but insufficient for others (like buffered writes)
Fix #2: flash_full_status_check() does not combine conditions properly in the if statement.
This fix was suggested by "Peter Pearse" Peter.Pearse@arm.com but a patch was never submitted as far as I can tell. Refer to:
http://sourceforge.net/mailarchive/message.php?msg_id=10546157
Fix #3: flash_write_cfibuffer() would leave the flash in command mode. I had mentioned this problem on the list a couple of times but never had time to investigate until now.
* Intel 28F640J3 flash on CSB472 board reverts to command mode if the removed line is present. Thus immediately after cp to flash operation, imls, boot etc would not work properly. md to flash displays 0x0080 (repeated) which is the value of status register of the flash.
* When this line is present and executed the flash is already in Array Read mode (flash_full_status_check issues a reset command in the end). So clearing the status is unnecessary at best.
Best regards, Tolunay Orkun
Index: drivers/cfi_flash.c =================================================================== RCS file: /cvsroot/u-boot/u-boot/drivers/cfi_flash.c,v retrieving revision 1.17 diff -p -u -r1.17 cfi_flash.c --- drivers/cfi_flash.c 13 Apr 2005 10:02:47 -0000 1.17 +++ drivers/cfi_flash.c 6 May 2005 07:06:04 -0000 @@ -676,7 +676,7 @@ static int flash_status_check (flash_inf /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) { - if (get_timer (start) > info->erase_blk_tout * CFG_HZ) { + if (get_timer (start) > tout * CFG_HZ) { printf ("Flash %s timeout at address %lx data %lx\n", prompt, info->start[sector], flash_read_long (info, sector, 0)); @@ -701,7 +701,7 @@ static int flash_full_status_check (flas case CFI_CMDSET_INTEL_EXTENDED: case CFI_CMDSET_INTEL_STANDARD: if ((retcode != ERR_OK) - && !flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { + || !flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { retcode = ERR_INVAL; printf ("Flash %s error at address %lx\n", prompt, info->start[sector]); @@ -1263,7 +1263,6 @@ static int flash_write_cfibuffer (flash_ info->buffer_write_tout, "buffer write"); } - flash_write_cmd (info, sector, 0, FLASH_CMD_CLEAR_STATUS); return retcode; } #endif /* CFG_FLASH_USE_BUFFER_WRITE */
On 5/6/05, Tolunay Orkun listmember@orkun.us wrote:
This patch is prepared against current CVS:
- Patch by Tolunay Orkun, 06 May 2005: Fixes for drivers/cfi_flash.c:
- Fix wrong timeout value usage in flash_status_check()
I tried this part of this patch out and aggravated some problems in the cfi_flash code.
Several of the timeout parameters in the CFI query structure return units of microseconds. I use an AMD 29LV160 part. In the CFI parser code these microsecond values are converted to milliseconds by integer division. For the 29LV160 this leaves some of the timeout parameters in the info structure at zero.
Using these zero values, the code in flash_status_check() will sometimes read busy once, the timer count happens to roll over, get read, and a timeout is registered if tout is zero.
I can see a few ways to fix this:
1) adjust the info values after the scaling to millisecond values 2) force a one tick delay no matter what in flash_status_check() 3) modify the code to report everything in microseconds and use udelay 4) use set_timer() to make sure there is a full timer tick
Any opinions about the best way to fix this? Or is it fixed in someones patch already?
===================================================================
RCS file: /cvsroot/u-boot/u-boot/drivers/cfi_flash.c,v retrieving revision 1.17 diff -p -u -r1.17 cfi_flash.c --- drivers/cfi_flash.c 13 Apr 2005 10:02:47 -0000 1.17 +++ drivers/cfi_flash.c 6 May 2005 07:06:04 -0000 @@ -676,7 +676,7 @@ static int flash_status_check (flash_inf /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) {
if (get_timer (start) > info->erase_blk_tout * CFG_HZ) {
if (get_timer (start) > tout * CFG_HZ) { printf ("Flash %s timeout at address %lx data %lx\n", prompt, info->start[sector], flash_read_long (info, sector, 0));
In message c166aa9f0505281515495cf1c5@mail.gmail.com you wrote:
I tried this part of this patch out and aggravated some problems in the cfi_flash code.
Several of the timeout parameters in the CFI query structure return units of microseconds. I use an AMD 29LV160 part. In the CFI parser code these microsecond values are converted to milliseconds by integer division. For the 29LV160 this leaves some of the timeout parameters in the info structure at zero.
I see.
I can see a few ways to fix this:
- adjust the info values after the scaling to millisecond values
- force a one tick delay no matter what in flash_status_check()
1) and 2) sound similar to me - if you count in millisecs you will have to round up any non-zero delay in any case.
- modify the code to report everything in microseconds and use udelay
This would probably slow down execution, as you then will delay always instead of actively waitingin a busy loop.
- use set_timer() to make sure there is a full timer tick
This is just another variant of 1) and 2), right?
I recommend to implement correct rounding of the timeouts (i. e. round up to fill lilliseconds); note that his does NOT slow down normal operation.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
I recommend to implement correct rounding of the timeouts (i. e. round up to fill lilliseconds); note that his does NOT slow down normal operation.
Sorry for late reply. Monday was Memorial Day (holiday) in the US.
I just replied to Andrew's email. I do prefer a rouding up solution as well. If agreed, I will provide a patch to implement rounding up for solution #1.
One question: Do we need to maintain actual timeout values in the info structure for flinfo command or is it OK for flinfo to report timing based on rounded up values?
Best regards, Tolunay Orkun
In message 429C8D77.8040105@orkun.us you wrote:
One question: Do we need to maintain actual timeout values in the info structure for flinfo command or is it OK for flinfo to report timing based on rounded up values?
Please report correct timeouts, and round up where necessary when accessing the data.
Best regards,
Wolfgang Denk
Andrew,
Thanks for bringing this up. Here is my opinion on the solutions you have proposed. Depending on the choice, we can provide a patch on top of my earlier patch to fix.
Several of the timeout parameters in the CFI query structure return units of microseconds. I use an AMD 29LV160 part. In the CFI parser code these microsecond values are converted to milliseconds by integer division. For the 29LV160 this leaves some of the timeout parameters in the info structure at zero.
Using these zero values, the code in flash_status_check() will sometimes read busy once, the timer count happens to roll over, get read, and a timeout is registered if tout is zero.
I can see a few ways to fix this:
- adjust the info values after the scaling to millisecond values
I think this would be an easy fix. We can round the values up one tick if there is a remainder during integer division.
Pro: Easy to do. Con: flinfo would not report correct timing info.
One way to report actual values by flinfo would be to duplicate the timing values in info structure, one set representing display values and another set would be the one actually used in loops.
Still, since this calculation would be done once when driver is initialized it would be my first choice.
- force a one tick delay no matter what in flash_status_check()
Similar to #1.
Pro: Easy to do. Info structure contains correct values Con: Rounding up tout is done every time we are in flash_status_check.
This would be my second choice.
- modify the code to report everything in microseconds and use udelay
Pro: Correct timeout. Con: Requires more extensive re-work of the code.
Also using udelay would probably not work well here. The current design allows breaking out early (when flash is not busy) before timeout occurs. Changing get_timer() to report time in usec would probably trigger a lot more changes.
- use set_timer() to make sure there is a full timer tick
I think #2 would be better way.
Any opinions about the best way to fix this? Or is it fixed in someones patch already?
===================================================================
RCS file: /cvsroot/u-boot/u-boot/drivers/cfi_flash.c,v retrieving revision 1.17 diff -p -u -r1.17 cfi_flash.c --- drivers/cfi_flash.c 13 Apr 2005 10:02:47 -0000 1.17 +++ drivers/cfi_flash.c 6 May 2005 07:06:04 -0000 @@ -676,7 +676,7 @@ static int flash_status_check (flash_inf /* Wait for command completion */ start = get_timer (0); while (flash_is_busy (info, sector)) {
if (get_timer (start) > info->erase_blk_tout * CFG_HZ) {
if (get_timer (start) > tout * CFG_HZ) { printf ("Flash %s timeout at address %lx data %lx\n", prompt, info->start[sector], flash_read_long (info, sector, 0));
In message 429C8A90.5080004@orkun.us you wrote:
- adjust the info values after the scaling to millisecond values
I think this would be an easy fix. We can round the values up one tick if there is a remainder during integer division.
Pro: Easy to do. Con: flinfo would not report correct timing info.
The "Con" counts for me.
- force a one tick delay no matter what in flash_status_check()
Actually it should read: round up always.
Pro: Easy to do. Info structure contains correct values Con: Rounding up tout is done every time we are in flash_status_check.
So what? Regarding the code size it does not matter if we round up here or there; regarding speed it does not matter either since we're going to enter some sort of delay loop anyway.
This is the way to go.
- modify the code to report everything in microseconds and use udelay
Pro: Correct timeout. Con: Requires more extensive re-work of the code.
Major con: instead of waiting until done it will wait longer, thus reducing the performance without need.
Not accepted.
Also using udelay would probably not work well here. The current design allows breaking out early (when flash is not busy) before timeout occurs. Changing get_timer() to report time in usec would probably trigger a lot more changes.
There is no reason for such a change.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
- force a one tick delay no matter what in flash_status_check()
Actually it should read: round up always.
Pro: Easy to do. Info structure contains correct values Con: Rounding up tout is done every time we are in flash_status_check.
So what? Regarding the code size it does not matter if we round up here or there; regarding speed it does not matter either since we're going to enter some sort of delay loop anyway.
This is the way to go.
So it shall be ;) I'll submit a patch for this.
Question 1: Should I redo my last patch or submit a patch on top of it assuming that it will go through?
Question 2: Assuming I will re-work my original patch. I had also included a fix in my last patch for logic error in flash_full_status_check() but today Peter Pearse came forward saying that fix was part of his patch submitted in March. ( http://sourceforge.net/mailarchive/message.php?msg_id=11164812 ). However, I do not have access to the patch file mentioned in that messages to compare. Should I exclude this change from my patch?
In message 429CCF13.6020809@orkun.us you wrote:
So it shall be ;) I'll submit a patch for this.
Thanks.
Question 1: Should I redo my last patch or submit a patch on top of it assuming that it will go through?
Please send a new summary patch (and if possible let me know which other,previously submitted patches this obsoletes).
Best regards,
Wolfgang Denk
participants (3)
-
Andrew Dyer -
Tolunay Orkun -
Wolfgang Denk