last commit breaks the AMD NOR flash functionality?

last commit breaks the AMD NOR flash functionality?

[Vitaly Wool]

Greetings,

the latest commit for cfi_cmdset_0002.c:

>Index: cfi_cmdset_0002.c
>===================================================================
>RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0002.c,v
>retrieving revision 1.115
>retrieving revision 1.116
>diff -u -r1.115 -r1.116
>--- cfi_cmdset_0002.c	20 May 2005 03:28:23 -0000	1.115
>+++ cfi_cmdset_0002.c	24 May 2005 13:29:42 -0000	1.116
>@@ -1320,7 +1320,7 @@
> 		cfi_spin_lock(chip->mutex);
> 	}
> 	/* Did we succeed? */
>-	if (chip_good(map, adr, map_word_ff(map))) {
>+	if (!chip_good(map, adr, map_word_ff(map))) {
> 		/* reset on all failures. */
> 		map_write( map, CMD(0xF0), chip->start );
> 		/* FIXME - should have reset delay before continuing */
>
resulted in the following behavior for my AMD NOR flash:

# flash_eraseall /dev/mtd/3
Erasing 64 Kibyte @ 0 --  0 % complete.
flash_eraseall: /dev/mtd/3: MTD Erase failure: Input/output error
Erasing 64 Kibyte @ 10000 -- 25 % complete.
flash_eraseall: /dev/mtd/3: MTD Erase failure: Input/output error
Erasing 64 Kibyte @ 20000 -- 50 % complete.
flash_eraseall: /dev/mtd/3: MTD Erase failure: Input/output error
Erasing 64 Kibyte @ 30000 -- 75 % complete.
flash_eraseall: /dev/mtd/3: MTD Erase failure: Input/output error

If I roll it back, it starts working.
I also suspected something strange with 'if(chip_good(...)) ...' instead 
of 'if(!chip_good(...)) ...' but it worked before and it doesn't work 
now. Can anyone elaborate this a bit?

Best regards,
   Vitaly

Re: last commit breaks the AMD NOR flash functionality?

[Thomas Gleixner]

On Tue, 2005-05-24 at 18:37 +0400, Vitaly Wool wrote:
> > 	/* Did we succeed? */
> >-	if (chip_good(map, adr, map_word_ff(map))) {
> >+	if (!chip_good(map, adr, map_word_ff(map))) {
> > 		/* reset on all failures. */
> > 		map_write( map, CMD(0xF0), chip->start );
> > 		/* FIXME - should have reset delay before continuing */
> >
> If I roll it back, it starts working.
> I also suspected something strange with 'if(chip_good(...)) ...' instead 
> of 'if(!chip_good(...)) ...' but it worked before and it doesn't work 
> now. Can anyone elaborate this a bit?

I had the same problem you see before the fix. It took a bit to figure
out what was going wrong. 

I'm quite sure, that my fix is correct, because the previous revision
exited via the error path in case of no error.

I suspect, that something goes really wrong with your erase. You have to
check the results in chip_good.

The chip_good function reads the same address twice and compares whether
the results are equal. The chip good result also requires that the reads
are all 0xff.

I suggest to add a check into chip_good, where you can identify which of
the requirements is not fulfilled to further narrow down the source of
trouble

tglx

Re: last commit breaks the AMD NOR flash functionality?

[Vitaly Wool]

Thanks Thomas,
I think I found the bug.

This is the 'problematic' part code in cfi_cmdset_0002.c:

>         /* Did we succeed? */
>         if (!chip_good(map, adr, map_word_ff(map))) {
>                 /* reset on all failures. */
>                 map_write( map, CMD(0xF0), chip->start );
>                 /* FIXME - should have reset delay before continuing */
>
>                 ret = -EIO;
>         }

I inserted a stupid printk into the chip_good function:

> static int chip_good(struct map_info *map, unsigned long addr, 
> map_word expected)
> {
>         map_word oldd, curd;
>
>         oldd = map_read(map, addr);
>         curd = map_read(map, addr);
>
>         printk("FFFFFF %ld %ld %ld\n", *oldd.x, *curd.x, *expected.x);
>         return  map_word_equal(map, oldd, curd) &&
>                 map_word_equal(map, curd, expected);
> }

And what I saw was:

...
FFFFFF 65535 65535 -1
...

And that's because map_word_ff is the following:

> static inline map_word map_word_ff(struct map_info *map)
> {
>         map_word r;
>         int i;
>
>         for (i=0; i<map_words(map); i++) {
>                 r.x[i] = ~0UL;
>         }
>         return r;
> }

which won't work in my case (16-bit flash access).
The suggested fix is the following:

Index: include/linux/mtd/map.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/mtd/map.h,v
retrieving revision 1.48
diff -u -r1.48 map.h
--- include/linux/mtd/map.h     16 Feb 2005 15:54:59 -0000      1.48
+++ include/linux/mtd/map.h     24 May 2005 15:51:28 -0000
@@ -346,7 +346,10 @@
        int i;

        for (i=0; i<map_words(map); i++) {
-               r.x[i] = ~0UL;
+               if (map_bankwidth_is_1(map) || map_bankwidth_is_2(map))
+                       r.x[i] = (1 << (8*map_bankwidth(map))) - 1;
+               else
+                       r.x[i] = ~0UL;
        }
        return r;
 }

This fix at least works for my case :)

Best regards,
   Vitaly


Thomas Gleixner wrote:

>On Tue, 2005-05-24 at 18:37 +0400, Vitaly Wool wrote:
>  
>
>>>	/* Did we succeed? */
>>>-	if (chip_good(map, adr, map_word_ff(map))) {
>>>+	if (!chip_good(map, adr, map_word_ff(map))) {
>>>		/* reset on all failures. */
>>>		map_write( map, CMD(0xF0), chip->start );
>>>		/* FIXME - should have reset delay before continuing */
>>>
>>>      
>>>
>>If I roll it back, it starts working.
>>I also suspected something strange with 'if(chip_good(...)) ...' instead 
>>of 'if(!chip_good(...)) ...' but it worked before and it doesn't work 
>>now. Can anyone elaborate this a bit?
>>    
>>
>
>I had the same problem you see before the fix. It took a bit to figure
>out what was going wrong. 
>
>I'm quite sure, that my fix is correct, because the previous revision
>exited via the error path in case of no error.
>
>I suspect, that something goes really wrong with your erase. You have to
>check the results in chip_good.
>
>The chip_good function reads the same address twice and compares whether
>the results are equal. The chip good result also requires that the reads
>are all 0xff.
>
>I suggest to add a check into chip_good, where you can identify which of
>the requirements is not fulfilled to further narrow down the source of
>trouble
>
>tglx
>
>
>
>  
>

Re: last commit breaks the AMD NOR flash functionality?

[Thomas Gleixner]

Vitaly,

On Tue, 2005-05-24 at 20:04 +0400, Vitaly Wool wrote:
> which won't work in my case (16-bit flash access).
> The suggested fix is the following:

Thanks for pointing that out. I fixed it in CVS so it is working for
64bit machines too.

tglx