[U-Boot-Users] bug in cfi_flash error detection

[U-Boot-Users] bug in cfi_flash error detection

[Martin Krause]

Hi all,

recently I got back a board from our customer, where some 
bits in the (nor) flash could not be programmed due to 
an error in the flash chip (-> very rare error).

I tought, this would be a good reason, to test the common
cfi_flash driver with this board (so far we used a board 
specific flash driver on this board).

But the cfi_flash driver does not detect the error as I
expected. I took a closer look to the driver and found
some bugs (at least in my opinion).

I added a patch wich tries to fix it, but I'm not sure 
if this is the proper way it should be done (e. g. 
because it adds bytes to cfi_flash.o).

Here a brief description of what the patch tries to fix:

- Timeout calculation in flash_status_check():
  The function calculates seconds instead of miliseconds. 
  And on some boards the range of ulong is exceeded in the
  calculation (because CFG_HZ is very big on some boards).

- In flash_status_check() a bogus error message is printed
  (secotor number is always 0 and the printed data value is
  not from the address where the error occours).

- flash_status_check() uses always the erase block timeout,
  regardles if eraseing flash or writing flash.

- If the write timeout of the flash device is below 1 ms
  it is erroneously calculated to zero.

Any comment would be welcome.

Regards,
Martin

TQ-Systems GmbH
M?hlstra?e 2, Gut Delling, 82229 Seefeld
Tel. +49 (8153) 93 08-157, Fax +49 (8153) 93 08-7157
martin.krause at tqs.de
www.tq-group.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_cfi_flash
Type: application/octet-stream
Size: 2796 bytes
Desc: patch_cfi_flash
Url : http://lists.denx.de/pipermail/u-boot/attachments/20051017/cd5df089/attachment.obj 

[U-Boot-Users] bug in cfi_flash error detection

[Tolunay Orkun]

Martin Krause wrote:
> Hi all,
> 
> Here a brief description of what the patch tries to fix:
> 
> - Timeout calculation in flash_status_check():
>   The function calculates seconds instead of miliseconds. 
>   And on some boards the range of ulong is exceeded in the
>   calculation (because CFG_HZ is very big on some boards).
> 
> - In flash_status_check() a bogus error message is printed
>   (secotor number is always 0 and the printed data value is
>   not from the address where the error occours).
> 
> - flash_status_check() uses always the erase block timeout,
>   regardles if eraseing flash or writing flash.
> 
> - If the write timeout of the flash device is below 1 ms
>   it is erroneously calculated to zero.

Some of the issues you are fixing are also fixed my earlier patch in the 
pipeline (from July). See my patch below for items 1,3,4 in your list.

http://sourceforge.net/mailarchive/message.php?msg_id=12234135

I think, in flash_status_check() your patch timeout check is not 
correct. get_timer() returns in msec as well as tout is in msecs. So, 
they are directly comparable without involving CFG_HZ.

I think the full status check needs to be applied to sector 0 
irrespective of what sector you are operating with because this is where 
the flash status register resides. I think some flash ignore the sector 
address while others require sector 0. I would be careful in changing 
this without verifying for a wide range of flash chips. Yes, the sector 
number will be wrong in error if you activate debug outputs but it is 
only cosmetic as far as I can see. Also, finding the sector number each 
time from address is a time consuming search that will slow the flash 
write operation unnecessarily. I am more willing to take the wrong 
sector number in debug output (if enabled) than slowed down status check 
which is used in a number of places.

Best regards,
Tolunay

[U-Boot-Users] bug in cfi_flash error detection

[Wolfgang Denk]

In message <47F3F98010FF784EBEE6526EAAB078D1C05E04@tq-mailsrv.tq-net.de> you wrote:
> 
> - Timeout calculation in flash_status_check():
>   The function calculates seconds instead of miliseconds.=20
>   And on some boards the range of ulong is exceeded in the
>   calculation (because CFG_HZ is very big on some boards).

This is a bug with such boards. CFG_HZ should always be set to  1000.
Looking  back  I  have  to  admit that it was a major design error to
create this variable at all.


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
It's not an optical illusion, it just looks like one.   -- Phil White

[U-Boot-Users] bug in cfi_flash error detection

[Martin Krause]

Hi Tolunay,

thank you for your reply!

Tolunay Orkun wrote on Monday, October 17, 2005 8:24 PM:
> Martin Krause wrote:
> Some of the issues you are fixing are also fixed my earlier patch in
> the pipeline (from July). See my patch below for items 1,3,4 in your
> list. 
> 
> http://sourceforge.net/mailarchive/message.php?msg_id=12234135

OK, I missed this :(

> I think, in flash_status_check() your patch timeout check is not
> correct. get_timer() returns in msec as well as tout is in msecs. So,
> they are directly comparable without involving CFG_HZ.

I think this is only true for boards where CFG_HZ is 1000.

> I think the full status check needs to be applied to sector 0
> irrespective of what sector you are operating with because this is
> where the flash status register resides. I think some flash ignore
> the sector address while others require sector 0. I would be careful
> in changing this without verifying for a wide range of flash chips.

It seems that during flash commands other than for writing (flash
erase, flash protect, ...), the sector address has not to be 0 while 
checking status register. Is this correct?

Do you know a flash device, where the sector address has to bee 0
during DQ6 toggle bit staus checking? I only ask to avoid such 
flash devices in new designs ;-)

> Yes, the sector number will be wrong in error if you activate debug
> outputs but it is only cosmetic as far as I can see. Also, finding
> the sector number each time from address is a time consuming search
> that will slow the flash write operation unnecessarily. I am more
> willing to take the wrong sector number in debug output (if enabled)

As far as I can see, the error message in flash_status_check()
is printed always, not only if debug output is enabled.

> than slowed down status check which is used in a number of places.

Maybe it is better then to print no details about the sector or
address where the error occours, than to print wrong values?

Regards,
Martin

[U-Boot-Users] bug in cfi_flash error detection

[Wolfgang Denk]

In message <47F3F98010FF784EBEE6526EAAB078D1C05E05@tq-mailsrv.tq-net.de> you wrote:
> 
> > I think, in flash_status_check() your patch timeout check is not
> > correct. get_timer() returns in msec as well as tout is in msecs. So,
> > they are directly comparable without involving CFG_HZ.
> 
> I think this is only true for boards where CFG_HZ is 1000.

Other boards are broken and should be fixed.

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
Totally illogical, there was no chance.
	-- Spock, "The Galileo Seven", stardate 2822.3