Fix recovery after failed write-buffer operation in cfi_cmdset_0002.c

Fix recovery after failed write-buffer operation in cfi_cmdset_0002.c

[Harald Nordgard-Hansen]

Hi.

When working on a problem with some flash chips that lock up during
write-buffer operations, I think there may be a bug in the linux
handling of chips using cfi_cmdset_0002.c.

The datasheets I have found for a number of these chips all specify that
when aborting a write-buffer command, it is not enough to use the
standard reset.  Rather a "write-to-buffer-reset command" is needed.
This command is quite similar for all chips, the main variance seem to
be if the final 0xF0 can go to any address or must go to addr_unlock1.

The bug is then in the recovery handling when timing out at the end of
do_write_buffer, where using the normal reset command is not sufficient.

Without this change, if the write-buffer command fails then any
following operations on the flash also fail.

The small patch here should apply against just about all kernels I've
seen over the last 5 years, the code has not changed in this area for a
long time...

-Harald Nordgård-Hansen

--------
--- a/drivers/mtd/chips/cfi_cmdset_0002.c      2012-05-21
13:46:28.679794861 +0200
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c      2012-10-30
18:27:49.939109556 +0100
@@ -1536,8 +1536,10 @@
                UDELAY(map, chip, adr, 1);
        }

-       /* reset on all failures. */
-       map_write( map, CMD(0xF0), chip->start );
+       /* write-to-buffer-reset on all failures. */
+       cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
cfi->device_type, NULL);
+       cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
cfi->device_type, NULL);
+       cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
cfi->device_type, NULL);
        xip_enable(map, chip, adr);
        /* FIXME - should have reset delay before continuing */
--------


-- 
Harald Nordgård-Hansen

Re: Fix recovery after failed write-buffer operation in cfi_cmdset_0002.c

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]

Hi,

On Fri, 2012-11-09 at 14:23 +0100, Harald Nordgard-Hansen wrote:
> When working on a problem with some flash chips that lock up during
> write-buffer operations, I think there may be a bug in the linux
> handling of chips using cfi_cmdset_0002.c.
> 
> The datasheets I have found for a number of these chips all specify that
> when aborting a write-buffer command, it is not enough to use the
> standard reset.  Rather a "write-to-buffer-reset command" is needed.
> This command is quite similar for all chips, the main variance seem to
> be if the final 0xF0 can go to any address or must go to addr_unlock1.
> 
> The bug is then in the recovery handling when timing out at the end of
> do_write_buffer, where using the normal reset command is not sufficient.
> 
> Without this change, if the write-buffer command fails then any
> following operations on the flash also fail.
> 
> The small patch here should apply against just about all kernels I've
> seen over the last 5 years, the code has not changed in this area for a
> long time...
> 
> -Harald Nordgård-Hansen

Hi, your patch is not applicable, it is line-wrapped. Can you please
send a patch I can easily apply?
> 
> --------
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c      2012-05-21
> 13:46:28.679794861 +0200
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c      2012-10-30
> 18:27:49.939109556 +0100
> @@ -1536,8 +1536,10 @@
>                 UDELAY(map, chip, adr, 1);
>         }
> 
> -       /* reset on all failures. */
> -       map_write( map, CMD(0xF0), chip->start );

Would be nice to put a short comment about what you do and why, may be
some reference as well. Just to make sure if someone reads the code,
he/she has some clue what are these about.

Thanks!

> +       /* write-to-buffer-reset on all failures. */
> +       cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
> cfi->device_type, NULL);
> +       cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
> cfi->device_type, NULL);
> +       cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
> cfi->device_type, NULL);
>         xip_enable(map, chip, adr);
>         /* FIXME - should have reset delay before continuing */


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Fix recovery after failed write-buffer operation in cfi_cmdset_0002.c

[Harald Nordgard-Hansen]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On 11/21/12 08:48, Artem Bityutskiy wrote:
> Hi, your patch is not applicable, it is line-wrapped. Can you
> please send a patch I can easily apply?

Sorry about that.  It's been way too many years since I regularly sent
patches in mail.  If this attempt does not work better, I'll just do
it all manually instead of trying to use these new-fangled MUAs. :-)

> Would be nice to put a short comment about what you do and why, may
> be some reference as well. Just to make sure if someone reads the
> code, he/she has some clue what are these about.

Hopefully in place now.

-Harald
-- 
Harald Nordgård-Hansen

[-- Attachment #2: write_buffer_recovery.patch --]
[-- Type: text/x-patch, Size: 1110 bytes --]

diff -ru a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
--- a/drivers/mtd/chips/cfi_cmdset_0002.c	2012-11-23 22:43:05.000000000 +0100
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c	2012-11-23 23:01:58.000000000 +0100
@@ -1536,8 +1536,16 @@
 		UDELAY(map, chip, adr, 1);
 	}
 
-	/* reset on all failures. */
-	map_write( map, CMD(0xF0), chip->start );
+	/*
+	 * Recovery from write-buffer programming failures requires
+	 * the write-to-buffer-reset sequence.  Since the last part
+	 * of the sequence also works as a normal reset, we can run
+	 * the same commands regardless of why we are here.
+	 * See e.g. http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
+	 */
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
 	xip_enable(map, chip, adr);
 	/* FIXME - should have reset delay before continuing */
 

Re: Fix recovery after failed write-buffer operation in cfi_cmdset_0002.c

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

On Fri, 2012-11-23 at 23:11 +0100, Harald Nordgard-Hansen wrote:
> > Would be nice to put a short comment about what you do and why, may
> > be some reference as well. Just to make sure if someone reads the
> > code, he/she has some clue what are these about.
> 
> Hopefully in place now.

Pushed to l2-mtd.git, thanks.

However, I had to do the following:

1. Make a commit message for you by copying parts of your e-mail.
2. Create a Signe-of-by for you.
3. Fix these checkpatch.pl warnings:

WARNING:LONG_LINE: line over 80 characters
#48: FILE: drivers/mtd/chips/cfi_cmdset_0002.c:1546:
+       cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);

WARNING:LONG_LINE: line over 80 characters
#49: FILE: drivers/mtd/chips/cfi_cmdset_0002.c:1547:
+       cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);

WARNING:LONG_LINE: line over 80 characters
#50: FILE: drivers/mtd/chips/cfi_cmdset_0002.c:1548:
+       cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]