Discussion:
[PATCH 3.16 096/366] mtd: cfi_cmdset_0002: Change write buffer to check correct value
Ben Hutchings
2018-11-11 19:49:05 UTC
Permalink
3.16.61-rc1 review patch. If anyone has any objections, please let me know.

------------------

From: Tokunori Ikegami <***@allied-telesis.co.jp>

commit dfeae1073583dc35c33b32150e18b7048bbb37e6 upstream.

For the word write it is checked if the chip has the correct value.
But it is not checked for the write buffer as only checked if ready.
To make sure for the write buffer change to check the value.

It is enough as this patch is only checking the last written word.
Since it is described by data sheets to check the operation status.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <***@infinera.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Brian Norris <***@gmail.com>
Cc: David Woodhouse <***@infradead.org>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: Marek Vasut <***@gmail.com>
Cc: Richard Weinberger <***@nod.at>
Cc: Cyrille Pitchen <***@wedev4u.fr>
Cc: linux-***@lists.infradead.org
Signed-off-by: Boris Brezillon <***@bootlin.com>
Signed-off-by: Ben Hutchings <***@decadent.org.uk>
---
drivers/mtd/chips/cfi_cmdset_0002.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1541,7 +1541,7 @@ static int __xipram do_write_buffer(stru
if (time_after(jiffies, timeo) && !chip_ready(map, adr))
break;

- if (chip_ready(map, adr)) {
+ if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
goto op_done;
}
Ben Hutchings
2018-11-11 19:49:05 UTC
Permalink
3.16.61-rc1 review patch. If anyone has any objections, please let me know.

------------------

From: Tokunori Ikegami <***@allied-telesis.co.jp>

commit 79ca484b613041ca223f74b34608bb6f5221724b upstream.

Currently the functions use to check both chip ready and good.
But the chip ready is not enough to check the operation status.
So change this to check the chip good instead of this.
About the retry functions to make sure the error handling remain it.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <***@infinera.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Brian Norris <***@gmail.com>
Cc: David Woodhouse <***@infradead.org>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: Marek Vasut <***@gmail.com>
Cc: Richard Weinberger <***@nod.at>
Cc: Cyrille Pitchen <***@wedev4u.fr>
Cc: linux-***@lists.infradead.org
Signed-off-by: Boris Brezillon <***@bootlin.com>
[bwh: Backported to 3.16: adjust context]
Signed-off-by: Ben Hutchings <***@decadent.org.uk>
---
drivers/mtd/chips/cfi_cmdset_0002.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)

--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1955,12 +1955,13 @@ static int __xipram do_erase_chip(struct
chip->erase_suspended = 0;
}

- if (chip_ready(map, adr))
+ if (chip_good(map, adr, map_word_ff(map)))
break;

if (time_after(jiffies, timeo)) {
printk(KERN_WARNING "MTD %s(): software timeout\n",
__func__ );
+ ret = -EIO;
break;
}

@@ -1968,15 +1969,15 @@ static int __xipram do_erase_chip(struct
UDELAY(map, chip, adr, 1000000/HZ);
}
/* Did we succeed? */
- if (!chip_good(map, adr, map_word_ff(map))) {
+ if (ret) {
/* reset on all failures. */
map_write( map, CMD(0xF0), chip->start );
/* FIXME - should have reset delay before continuing */

- if (++retry_cnt <= MAX_RETRIES)
+ if (++retry_cnt <= MAX_RETRIES) {
+ ret = 0;
goto retry;
-
- ret = -EIO;
+ }
}

chip->state = FL_READY;
@@ -2050,7 +2051,7 @@ static int __xipram do_erase_oneblock(st
chip->erase_suspended = 0;
}

- if (chip_ready(map, adr)) {
+ if (chip_good(map, adr, map_word_ff(map))) {
xip_enable(map, chip, adr);
break;
}
@@ -2059,6 +2060,7 @@ static int __xipram do_erase_oneblock(st
xip_enable(map, chip, adr);
printk(KERN_WARNING "MTD %s(): software timeout\n",
__func__ );
+ ret = -EIO;
break;
}

@@ -2066,15 +2068,15 @@ static int __xipram do_erase_oneblock(st
UDELAY(map, chip, adr, 1000000/HZ);
}
/* Did we succeed? */
- if (!chip_good(map, adr, map_word_ff(map))) {
+ if (ret) {
/* reset on all failures. */
map_write( map, CMD(0xF0), chip->start );
/* FIXME - should have reset delay before continuing */

- if (++retry_cnt <= MAX_RETRIES)
+ if (++retry_cnt <= MAX_RETRIES) {
+ ret = 0;
goto retry;
-
- ret = -EIO;
+ }
}

chip->state = FL_READY;
Ben Hutchings
2018-11-11 19:49:05 UTC
Permalink
3.16.61-rc1 review patch. If anyone has any objections, please let me know.

------------------

From: Tokunori Ikegami <***@allied-telesis.co.jp>

commit 45f75b8a919a4255f52df454f1ffdee0e42443b2 upstream.

For the word write functions it is retried for error.
But it is not implemented to retry for the erase functions.
To make sure for the erase functions change to retry as same.

This is needed to prevent the flash erase error caused only once.
It was caused by the error case of chip_good() in the do_erase_oneblock().
Also it was confirmed on the MACRONIX flash device MX29GL512FHT2I-11G.
But the error issue behavior is not able to reproduce at this moment.
The flash controller is parallel Flash interface integrated on BCM53003.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <***@infinera.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Brian Norris <***@gmail.com>
Cc: David Woodhouse <***@infradead.org>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: Marek Vasut <***@gmail.com>
Cc: Richard Weinberger <***@nod.at>
Cc: Cyrille Pitchen <***@wedev4u.fr>
Cc: linux-***@lists.infradead.org
Signed-off-by: Boris Brezillon <***@bootlin.com>
Signed-off-by: Ben Hutchings <***@decadent.org.uk>
---
drivers/mtd/chips/cfi_cmdset_0002.c | 10 ++++++++++
1 file changed, 10 insertions(+)

--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1900,6 +1900,7 @@ static int __xipram do_erase_chip(struct
unsigned long int adr;
DECLARE_WAITQUEUE(wait, current);
int ret = 0;
+ int retry_cnt = 0;

adr = cfi->addr_unlock1;

@@ -1917,6 +1918,7 @@ static int __xipram do_erase_chip(struct
ENABLE_VPP(map);
xip_disable(map, chip, adr);

+ retry:
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(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -1971,6 +1973,9 @@ static int __xipram do_erase_chip(struct
map_write( map, CMD(0xF0), chip->start );
/* FIXME - should have reset delay before continuing */

+ if (++retry_cnt <= MAX_RETRIES)
+ goto retry;
+
ret = -EIO;
}

@@ -1990,6 +1995,7 @@ static int __xipram do_erase_oneblock(st
unsigned long timeo = jiffies + HZ;
DECLARE_WAITQUEUE(wait, current);
int ret = 0;
+ int retry_cnt = 0;

adr += chip->start;

@@ -2007,6 +2013,7 @@ static int __xipram do_erase_oneblock(st
ENABLE_VPP(map);
xip_disable(map, chip, adr);

+ retry:
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(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -2064,6 +2071,9 @@ static int __xipram do_erase_oneblock(st
map_write( map, CMD(0xF0), chip->start );
/* FIXME - should have reset delay before continuing */

+ if (++retry_cnt <= MAX_RETRIES)
+ goto retry;
+
ret = -EIO;
}
Ben Hutchings
2018-11-11 19:49:05 UTC
Permalink
3.16.61-rc1 review patch. If anyone has any objections, please let me know.

------------------

From: Tokunori Ikegami <***@allied-telesis.co.jp>

commit 85a82e28b023de9b259a86824afbd6ba07bd6475 upstream.

The definition can be used for other program and erase operations also.
So change the naming to MAX_RETRIES from MAX_WORD_RETRIES.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <***@infinera.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Brian Norris <***@gmail.com>
Cc: David Woodhouse <***@infradead.org>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: Marek Vasut <***@gmail.com>
Cc: Richard Weinberger <***@nod.at>
Cc: Cyrille Pitchen <***@wedev4u.fr>
Cc: linux-***@lists.infradead.org
Signed-off-by: Boris Brezillon <***@bootlin.com>
Signed-off-by: Ben Hutchings <***@decadent.org.uk>
---
drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -42,7 +42,7 @@
#define AMD_BOOTLOC_BUG
#define FORCE_WORD_WRITE 0

-#define MAX_WORD_RETRIES 3
+#define MAX_RETRIES 3

#define SST49LF004B 0x0060
#define SST49LF040B 0x0050
@@ -1314,7 +1314,7 @@ static int __xipram do_write_oneword(str
map_write( map, CMD(0xF0), chip->start );
/* FIXME - should have reset delay before continuing */

- if (++retry_cnt <= MAX_WORD_RETRIES)
+ if (++retry_cnt <= MAX_RETRIES)
goto retry;

ret = -EIO;
@@ -1765,7 +1765,7 @@ retry:
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */

- if (++retry_cnt <= MAX_WORD_RETRIES)
+ if (++retry_cnt <= MAX_RETRIES)
goto retry;

ret = -EIO;

Loading...