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" <[email protected]> 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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cfi_flash_bufwrite_fix.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20050506/0492edd7/attachment.txt

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?

===================================================================

Hardware, n.:
        The parts of a computer system that can be kicked.

I see.

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.

This would probably slow down execution, as you then will delay always instead of actively waitingin a busy loop.

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

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Everything should be made as simple as possible, but not simpler."
                                                    - Albert Einstein

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.

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.

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.

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.

I think #2 would be better way.

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

The "Con" counts for me.

Actually it should read: round up always.

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.

Major con: instead of waiting until done it will wait longer, thus reducing the performance without need.

Not accepted.

There is no reason for such a change.

Best regards,

Wolfgang Denk

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A dog always bit deepest on the veterinary hand.
                                    - Terry Pratchett, _Wyrd Sisters_

Please report correct timeouts, and round up where necessary when accessing the data.

Best regards,

Wolfgang Denk

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Wagner's music is better than it sounds."               - Mark Twain

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?

Thanks.

Please send a new summary patch (and if possible let me know which other,previously submitted patches this obsoletes).

Best regards,

Wolfgang Denk

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
One does not thank logic.
	-- Sarek, "Journey to Babel", stardate 3842.4