[U-Boot-Users] PATCH for drivers/cfi_flash.c

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Tolunay Orkun]

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

-------------- 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 

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Andrew Dyer]

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));


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

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Wolfgang Denk]

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:
> 
> 1) adjust the info values after the scaling to millisecond values
> 2) 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.

> 3) 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.

> 4) 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

-- 
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

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Tolunay Orkun]

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:
> 
> 1) 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.

> 2) 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.

> 3) 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.

> 4) 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));
> 
> 
> 

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Tolunay Orkun]

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

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Wolfgang Denk]

In message <429C8A90.5080004@orkun.us> you wrote:
> 
> > 1) 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.

> > 2) 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.

> > 3) 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

-- 
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_

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Wolfgang Denk]

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

-- 
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

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Tolunay Orkun]

Wolfgang Denk wrote:

>>>2) 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?

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Peter Pearse]

Tolkay Orkun wrote:

> I do not have access to the patch file mentioned in that messages to compare

I have sent him a copy

Peter

-------------------------------------------------------------------------
Peter Pearse??????????? ?????????                                 ARM Ltd
------------------------ Peter.Pearse at arm.com   -------------------------

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Wolfgang Denk]

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

-- 
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

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Tolunay Orkun]

This patch reworks the patch submitted on May 6, 2005:
Ref: http://sourceforge.net/mailarchive/message.php?msg_id=11674386

This patch (cfi_flash.patch.tolunay) is prepared against current CVS 
version + Peter Pearse Patch dated March 15, 2005.
Ref: http://sourceforge.net/mailarchive/message.php?msg_id=11164812

CHANGELOG:

* Patch by Tolunay Orkun, 02 July 2005:
   Fixes for drivers/cfi_flash.c:
   - Fix wrong timeout value usage in flash_status_check()
   - Round write_tout up when converting to msec in flash_get_size()
   - Remove clearing flash status at the end of flash_write_cfibuffer()
     which sets Intel 28F640J3 flash back to command mode on CSB472

~~~

In case Peter's Patch is rejected for some reason please apply the 
alternate patch (cfi_flash.patch.tolunay2) instead. It makes one 
additional fix that I had submitted in my original patch.

CHANGELOG (Alternate)

* Patch by Tolunay Orkun, 02 July 2005:
   Fixes for drivers/cfi_flash.c:
   - Fix wrong timeout value usage in flash_status_check()
   - Round write_tout up when converting to msec in flash_get_size()
   - Remove clearing flash status at the end of flash_write_cfibuffer()
     which sets Intel 28F640J3 flash back to command mode on CSB472
   - Fix incorrect if condition in flash_full_status_check()

Best regards,
Tolunay Orkun
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cfi_flash.patch.tolunay
Url: http://lists.denx.de/pipermail/u-boot/attachments/20050702/265535cc/attachment.txt 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cfi_flash.patch.tolunay2
Url: http://lists.denx.de/pipermail/u-boot/attachments/20050702/265535cc/attachment-0001.txt 

[U-Boot-Users] PATCH for drivers/cfi_flash.c

[Wolfgang Denk]

In message <42C73653.4020705@orkun.us> you wrote:
> 
> CHANGELOG:
> 
> * Patch by Tolunay Orkun, 02 July 2005:
>    Fixes for drivers/cfi_flash.c:
>    - Fix wrong timeout value usage in flash_status_check()
>    - Round write_tout up when converting to msec in flash_get_size()
>    - Remove clearing flash status at the end of flash_write_cfibuffer()
>      which sets Intel 28F640J3 flash back to command mode on CSB472

Applied. Thanks.

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
"The whole world is about three drinks behind."     - Humphrey Bogart