[PATCH v3 01/11] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip

For a maintainability by reducing the goto statement remove it from
do_write_buffer().

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- None.

Changes since v1:
- Split the patch v1 3/3.

drivers/mtd/chips/cfi_cmdset_0002.c | 46 +++++++++++++++--------------
1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c2e51768a02c..deffafab067e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,

if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
- goto op_done;
+ break;
}

- if (time_after(jiffies, timeo))
+ if (time_after(jiffies, timeo)) {
+ ret = -EIO;
break;
+ }

/* Latency issues. Drop the lock, wait a while and retry */
UDELAY(map, chip, adr, 1);
}

- /*
- * 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 */
+ if (ret) {
+ /*
+ * 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 */

- printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
- __func__, adr);
+ printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
+ __func__, adr);
+ }

- ret = -EIO;
- op_done:
chip->state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);

--
2.18.0

Boris Brezillon

2018-11-05 13:14:37 UTC

On Fri, 26 Oct 2018 01:32:11 +0900

Post by Tokunori Ikegami
For a maintainability by reducing the goto statement remove it from
do_write_buffer().

I guess that's a matter of taste, but I don't find this version more
readable than the previous one. Any strong reason to get rid of the
op_done label?

Post by Tokunori Ikegami
---
- None.
- Split the patch v1 3/3.
drivers/mtd/chips/cfi_cmdset_0002.c | 46 +++++++++++++++--------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c2e51768a02c..deffafab067e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
- goto op_done;
+ break;
}
- if (time_after(jiffies, timeo))
+ if (time_after(jiffies, timeo)) {
+ ret = -EIO;
break;
+ }
/* Latency issues. Drop the lock, wait a while and retry */
UDELAY(map, chip, adr, 1);
}
- /*
- * 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 */
+ if (ret) {
+ /*
+ * 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 */
- printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
- __func__, adr);
+ printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
+ __func__, adr);
+ }
- ret = -EIO;
chip->state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);

IKEGAMI Tokunori

2018-11-06 00:32:06 UTC

-----Original Message-----
Sent: Monday, November 05, 2018 10:15 PM
To: IKEGAMI Tokunori
Subject: Re: [PATCH v3 03/11] mtd: cfi_cmdset_0002: Remove goto statement
from do_write_buffer()
On Fri, 26 Oct 2018 01:32:11 +0900

Post by Tokunori Ikegami
For a maintainability by reducing the goto statement remove it from
do_write_buffer().

I guess that's a matter of taste, but I don't find this version more
readable than the previous one. Any strong reason to get rid of the
op_done label?

No there is no strong reason.
I have just thought also that it is better for the maintainability.

Regards,
Ikegami

Post by Tokunori Ikegami
---
- None.
- Split the patch v1 3/3.
drivers/mtd/chips/cfi_cmdset_0002.c | 46

+++++++++++++++--------------

Post by Tokunori Ikegami
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c

b/drivers/mtd/chips/cfi_cmdset_0002.c

Post by Tokunori Ikegami
index c2e51768a02c..deffafab067e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
- goto op_done;
+ break;
}
- if (time_after(jiffies, timeo))
+ if (time_after(jiffies, timeo)) {
+ ret = -EIO;
break;
+ }
/* Latency issues. Drop the lock, wait a while and retry

Post by Tokunori Ikegami
UDELAY(map, chip, adr, 1);
}
- /*
- * 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_Bu
ffer_Prog_Page_Buffer_Read_AN.pdf

Post by Tokunori Ikegami
- */
- 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 */
+ if (ret) {
+ /*
+ * Recovery from write-buffer programming failures

requires

Post by Tokunori Ikegami
+ * the write-to-buffer-reset sequence. Since the last

part

Post by Tokunori Ikegami
+ * of the sequence also works as a normal reset, we can

run

Post by Tokunori Ikegami
+ * the same commands regardless of why we are here.
+ * See e.g.
+ *

http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Bu
ffer_Prog_Page_Buffer_Read_AN.pdf

Post by Tokunori Ikegami
+ */
+ cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start,

map, cfi,

Post by Tokunori Ikegami
+ cfi->device_type, NULL);
+ cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start,

map, cfi,

Post by Tokunori Ikegami
+ cfi->device_type, NULL);
+ cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start,

map, cfi,

Post by Tokunori Ikegami
+ cfi->device_type, NULL);
+ xip_enable(map, chip, adr);
+ /* FIXME - should have reset delay before continuing */
- printk(KERN_WARNING "MTD %s(): software timeout,

address:0x%.8lx.\n",

Post by Tokunori Ikegami
- __func__, adr);
+ printk(KERN_WARNING "MTD %s(): software timeout,

address:0x%.8lx.\n",

Post by Tokunori Ikegami
+ __func__, adr);
+ }
- ret = -EIO;
chip->state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);

Tokunori Ikegami

2018-10-25 16:32:10 UTC

It is enough to use chip_good() only so chip_ready() is not necessary.
For this change the order to check timeout is also needed to chagne.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- None.

Changes since v1:
- None.

drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 251c9e1675bd..c2e51768a02c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,14 +1882,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
continue;
}

- if (time_after(jiffies, timeo) && !chip_ready(map, adr))
- break;
-
if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
goto op_done;
}

+ if (time_after(jiffies, timeo))
+ break;
+
/* Latency issues. Drop the lock, wait a while and retry */
UDELAY(map, chip, adr, 1);
}

--
2.18.0

Boris Brezillon

2018-11-05 13:03:03 UTC

On Fri, 26 Oct 2018 01:32:10 +0900

Post by Tokunori Ikegami
It is enough to use chip_good() only so chip_ready() is not necessary.

I'd like a short explanation saying why chip_good() is enough:
chip_good() is doing the same check chip_ready() is doing plus an extra
check to make sure we end up with the data we wrote.

Post by Tokunori Ikegami
For this change the order to check timeout is also needed to chagne.

^change.

And I don't think changing the order is a hard requirement, it's just
better to avoid the case where the data update happens just after the
timeout has expired.

To sum-up, I'm okay with the diff, I'd just like the commit message
to be adjusted.

Post by Tokunori Ikegami
---
- None.
- None.
drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 251c9e1675bd..c2e51768a02c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,14 +1882,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
continue;
}
- if (time_after(jiffies, timeo) && !chip_ready(map, adr))
- break;
-
if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
goto op_done;
}
+ if (time_after(jiffies, timeo))
+ break;
+
/* Latency issues. Drop the lock, wait a while and retry */
UDELAY(map, chip, adr, 1);
}

IKEGAMI Tokunori

2018-11-06 10:12:50 UTC

Sorry for late to reply to the mail below. (The mail was unexpectedly moved from the inbox.)

-----Original Message-----
Sent: Monday, November 05, 2018 10:03 PM
To: IKEGAMI Tokunori
Subject: Re: [PATCH v3 02/11] mtd: cfi_cmdset_0002: Remove chip_ready()
from do_write_buffer()
On Fri, 26 Oct 2018 01:32:10 +0900

Post by Tokunori Ikegami
It is enough to use chip_good() only so chip_ready() is not necessary.

chip_good() is doing the same check chip_ready() is doing plus an extra
check to make sure we end up with the data we wrote.

I see so I will update as this.

Post by Tokunori Ikegami
For this change the order to check timeout is also needed to chagne.

^change.

Will fix this.

And I don't think changing the order is a hard requirement, it's just
better to avoid the case where the data update happens just after the
timeout has expired.

I will update as so.

To sum-up, I'm okay with the diff, I'd just like the commit message
to be adjusted.

I see.

Regards,
Ikegami

Post by Tokunori Ikegami
---
- None.
- None.
drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c

b/drivers/mtd/chips/cfi_cmdset_0002.c

Post by Tokunori Ikegami
index 251c9e1675bd..c2e51768a02c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,14 +1882,14 @@ static int __xipram do_write_buffer(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
continue;
}
- if (time_after(jiffies, timeo) && !chip_ready(map, adr))
- break;
-
if (chip_good(map, adr, datum)) {
xip_enable(map, chip, adr);
goto op_done;
}
+ if (time_after(jiffies, timeo))
+ break;
+
/* Latency issues. Drop the lock, wait a while and retry

Post by Tokunori Ikegami
UDELAY(map, chip, adr, 1);
}

Tokunori Ikegami

2018-10-25 16:32:13 UTC

Reduce the size of do_write_oneword() by extracting a helper function
for the hardware access.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Reviewed-by: Chris Packham <***@alliedtelesis.co.nz>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- Just update the commit message for the comment
- Add Reviewed-by tag.

Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 89 ++++++++++++++++-------------
1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a3fa2d7b1ba0..ae2d8bd7154e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1547,11 +1547,10 @@ static int cfi_amdstd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
do_otp_lock, 1);
}

-static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
- unsigned long adr, map_word datum,
- int mode)
+static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *chip,
+ unsigned long adr, map_word datum,
+ int mode, struct cfi_private *cfi)
{
- struct cfi_private *cfi = map->fldrv_priv;
unsigned long timeo = jiffies + HZ;
/*
* We use a 1ms + 1 jiffies generic timeout for writes (most devices
@@ -1564,42 +1563,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
*/
unsigned long uWriteTimeout = (HZ / 1000) + 1;
int ret = 0;
- map_word oldd;
- int retry_cnt = 0;
-
- adr += chip->start;
-
- mutex_lock(&chip->mutex);
- ret = get_chip(map, chip, adr, mode);
- if (ret) {
- mutex_unlock(&chip->mutex);
- return ret;
- }

- pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
- __func__, adr, datum.x[0]);
-
- if (mode == FL_OTP_WRITE)
- otp_enter(map, chip, adr, map_bankwidth(map));
-
- /*
- * Check for a NOP for the case when the datum to write is already
- * present - it saves time and works around buggy chips that corrupt
- * data at other locations when 0xff is written to a location that
- * already contains 0xff.
- */
- oldd = map_read(map, adr);
- if (map_word_equal(map, oldd, datum)) {
- pr_debug("MTD %s(): NOP\n",
- __func__);
- goto op_done;
- }
-
- XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
- 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(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -1642,6 +1606,53 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
UDELAY(map, chip, adr, 1);
}

+ return ret;
+}
+
+static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
+ unsigned long adr, map_word datum,
+ int mode)
+{
+ struct cfi_private *cfi = map->fldrv_priv;
+ int ret = 0;
+ map_word oldd;
+ int retry_cnt = 0;
+
+ adr += chip->start;
+
+ mutex_lock(&chip->mutex);
+ ret = get_chip(map, chip, adr, mode);
+ if (ret) {
+ mutex_unlock(&chip->mutex);
+ return ret;
+ }
+
+ pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
+ __func__, adr, datum.x[0]);
+
+ if (mode == FL_OTP_WRITE)
+ otp_enter(map, chip, adr, map_bankwidth(map));
+
+ /*
+ * Check for a NOP for the case when the datum to write is already
+ * present - it saves time and works around buggy chips that corrupt
+ * data at other locations when 0xff is written to a location that
+ * already contains 0xff.
+ */
+ oldd = map_read(map, adr);
+ if (map_word_equal(map, oldd, datum)) {
+ pr_debug("MTD %s(): NOP\n",
+ __func__);
+ goto op_done;
+ }
+
+ XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
+ ENABLE_VPP(map);
+ xip_disable(map, chip, adr);
+
+ retry:
+ ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+
/* Did we succeed? */
if (ret) {
/* reset on all failures. */

--
2.18.0

Boris Brezillon

2018-11-05 13:32:17 UTC

On Fri, 26 Oct 2018 01:32:13 +0900

Post by Tokunori Ikegami
Reduce the size of do_write_oneword() by extracting a helper function
for the hardware access.
---
- Just update the commit message for the comment
- Add Reviewed-by tag.
- Add the patch.
drivers/mtd/chips/cfi_cmdset_0002.c | 89 ++++++++++++++++-------------
1 file changed, 50 insertions(+), 39 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a3fa2d7b1ba0..ae2d8bd7154e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1547,11 +1547,10 @@ static int cfi_amdstd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
do_otp_lock, 1);
}
-static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
- unsigned long adr, map_word datum,
- int mode)
+static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *chip,
+ unsigned long adr, map_word datum,
+ int mode, struct cfi_private *cfi)
{
- struct cfi_private *cfi = map->fldrv_priv;
unsigned long timeo = jiffies + HZ;
/*
* We use a 1ms + 1 jiffies generic timeout for writes (most devices
@@ -1564,42 +1563,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
*/
unsigned long uWriteTimeout = (HZ / 1000) + 1;
int ret = 0;
- map_word oldd;
- int retry_cnt = 0;
-
- adr += chip->start;
-
- mutex_lock(&chip->mutex);
- ret = get_chip(map, chip, adr, mode);
- if (ret) {
- mutex_unlock(&chip->mutex);
- return ret;
- }
- pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
- __func__, adr, datum.x[0]);
-
- if (mode == FL_OTP_WRITE)
- otp_enter(map, chip, adr, map_bankwidth(map));
-
- /*
- * Check for a NOP for the case when the datum to write is already
- * present - it saves time and works around buggy chips that corrupt
- * data at other locations when 0xff is written to a location that
- * already contains 0xff.
- */
- oldd = map_read(map, adr);
- if (map_word_equal(map, oldd, datum)) {
- pr_debug("MTD %s(): NOP\n",
- __func__);
- goto op_done;
- }
-
- XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
- ENABLE_VPP(map);
- xip_disable(map, chip, adr);
-
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(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -1642,6 +1606,53 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
UDELAY(map, chip, adr, 1);
}
+ return ret;
+}
+
+static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
+ unsigned long adr, map_word datum,
+ int mode)
+{
+ struct cfi_private *cfi = map->fldrv_priv;
+ int ret = 0;
+ map_word oldd;
+ int retry_cnt = 0;
+
+ adr += chip->start;
+
+ mutex_lock(&chip->mutex);
+ ret = get_chip(map, chip, adr, mode);
+ if (ret) {
+ mutex_unlock(&chip->mutex);
+ return ret;
+ }
+
+ pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
+ __func__, adr, datum.x[0]);
+
+ if (mode == FL_OTP_WRITE)
+ otp_enter(map, chip, adr, map_bankwidth(map));
+
+ /*
+ * Check for a NOP for the case when the datum to write is already
+ * present - it saves time and works around buggy chips that corrupt
+ * data at other locations when 0xff is written to a location that
+ * already contains 0xff.
+ */
+ oldd = map_read(map, adr);
+ if (map_word_equal(map, oldd, datum)) {
+ pr_debug("MTD %s(): NOP\n",
+ __func__);
+ goto op_done;
+ }
+
+ XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
+ ENABLE_VPP(map);
+ xip_disable(map, chip, adr);
+
+ ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+
/* Did we succeed? */

We usually place the ret code check just after the call to the function
returning this ret code:

ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
if (ret) {
...
}

And I don't think we need the "Did we succeed?" comment, since it's
pretty obvious what this check does.

One extra thing you could do to make this piece of code more readable
is use a for loop for the retry logic:

for (retry_count = 0; retry_count < MAX_RETRIES; retry_count++) {
ret = do_write_oneword_once(map, chip, adr, datum,
mode, cfi);
if (!ret)
break;

/* reset on all failures. */
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */
}

Post by Tokunori Ikegami
if (ret) {
/* reset on all failures. */

IKEGAMI Tokunori

2018-11-06 00:45:15 UTC

-----Original Message-----
Sent: Monday, November 05, 2018 10:32 PM
To: IKEGAMI Tokunori
Subject: Re: [PATCH v3 05/11] mtd: cfi_cmdset_0002: Split
do_write_oneword() to reduce function size
On Fri, 26 Oct 2018 01:32:13 +0900

++++++++++++++++-------------

Post by Tokunori Ikegami
1 file changed, 50 insertions(+), 39 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c

b/drivers/mtd/chips/cfi_cmdset_0002.c

Post by Tokunori Ikegami
index a3fa2d7b1ba0..ae2d8bd7154e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1547,11 +1547,10 @@ static int cfi_amdstd_lock_user_prot_reg(struct

mtd_info *mtd, loff_t from,

Post by Tokunori Ikegami
do_otp_lock, 1);
}
-static int __xipram do_write_oneword(struct map_info *map, struct flchip

*chip,

Post by Tokunori Ikegami
- unsigned long adr, map_word datum,
- int mode)
+static int __xipram do_write_oneword_once(struct map_info *map, struct

flchip *chip,

Post by Tokunori Ikegami
+ unsigned long adr, map_word

datum,

Post by Tokunori Ikegami
+ int mode, struct cfi_private

*cfi)

Post by Tokunori Ikegami
{
- struct cfi_private *cfi = map->fldrv_priv;
unsigned long timeo = jiffies + HZ;
/*
* We use a 1ms + 1 jiffies generic timeout for writes (most devices
@@ -1564,42 +1563,7 @@ static int __xipram do_write_oneword(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
*/
unsigned long uWriteTimeout = (HZ / 1000) + 1;
int ret = 0;
- map_word oldd;
- int retry_cnt = 0;
-
- adr += chip->start;
-
- mutex_lock(&chip->mutex);
- ret = get_chip(map, chip, adr, mode);
- if (ret) {
- mutex_unlock(&chip->mutex);
- return ret;
- }
- pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
- __func__, adr, datum.x[0]);
-
- if (mode == FL_OTP_WRITE)
- otp_enter(map, chip, adr, map_bankwidth(map));
-
- /*
- * Check for a NOP for the case when the datum to write is already
- * present - it saves time and works around buggy chips that corrupt
- * data at other locations when 0xff is written to a location that
- * already contains 0xff.
- */
- oldd = map_read(map, adr);
- if (map_word_equal(map, oldd, datum)) {
- pr_debug("MTD %s(): NOP\n",
- __func__);
- goto op_done;
- }
-
- XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
- ENABLE_VPP(map);
- xip_disable(map, chip, adr);
-
cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,

cfi->device_type, NULL);

Post by Tokunori Ikegami
cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,

cfi->device_type, NULL);

Post by Tokunori Ikegami
cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi,

cfi->device_type, NULL);

Post by Tokunori Ikegami
@@ -1642,6 +1606,53 @@ static int __xipram do_write_oneword(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
UDELAY(map, chip, adr, 1);
}
+ return ret;
+}
+
+static int __xipram do_write_oneword(struct map_info *map, struct flchip

*chip,

Post by Tokunori Ikegami
+ unsigned long adr, map_word datum,
+ int mode)
+{
+ struct cfi_private *cfi = map->fldrv_priv;
+ int ret = 0;
+ map_word oldd;
+ int retry_cnt = 0;
+
+ adr += chip->start;
+
+ mutex_lock(&chip->mutex);
+ ret = get_chip(map, chip, adr, mode);
+ if (ret) {
+ mutex_unlock(&chip->mutex);
+ return ret;
+ }
+
+ pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
+ __func__, adr, datum.x[0]);
+
+ if (mode == FL_OTP_WRITE)
+ otp_enter(map, chip, adr, map_bankwidth(map));
+
+ /*
+ * Check for a NOP for the case when the datum to write is already
+ * present - it saves time and works around buggy chips that corrupt
+ * data at other locations when 0xff is written to a location that
+ * already contains 0xff.
+ */
+ oldd = map_read(map, adr);
+ if (map_word_equal(map, oldd, datum)) {
+ pr_debug("MTD %s(): NOP\n",
+ __func__);
+ goto op_done;
+ }
+
+ XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
+ ENABLE_VPP(map);
+ xip_disable(map, chip, adr);
+
+ ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+
/* Did we succeed? */

We usually place the ret code check just after the call to the function
ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
if (ret) {
...
}
And I don't think we need the "Did we succeed?" comment, since it's
pretty obvious what this check does.

I see so I will do fix as this.

One extra thing you could do to make this piece of code more readable
for (retry_count = 0; retry_count < MAX_RETRIES; retry_count++)
{
ret = do_write_oneword_once(map, chip, adr, datum,
mode, cfi);
if (!ret)
break;
/* reset on all failures. */
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */
}

This is changed by the patch 08/11 as so.
Do you mean that the patches should be combined to a patch?

Regards,
Ikegami

Post by Tokunori Ikegami
if (ret) {
/* reset on all failures. */

Tokunori Ikegami

2018-10-25 16:32:14 UTC

To reduce function size and remove the goto statement split the op_done goto
statement part into do_write_oneword_done() created a function.
Also split the start part into do_write_oneword_start() to find easier pairs.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- Change to split the start part of do_write_oneword() additionally.
- Fix indentation to call pr_debug().

Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 57 ++++++++++++++++++++---------
1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ae2d8bd7154e..5009c1941a50 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1609,6 +1609,40 @@ static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *c
return ret;
}

+static int __xipram do_write_oneword_start(struct map_info *map,
+ struct flchip *chip,
+ unsigned long adr, int mode)
+{
+ int ret = 0;
+
+ mutex_lock(&chip->mutex);
+
+ ret = get_chip(map, chip, adr, mode);
+ if (ret) {
+ mutex_unlock(&chip->mutex);
+ return ret;
+ }
+
+ if (mode == FL_OTP_WRITE)
+ otp_enter(map, chip, adr, map_bankwidth(map));
+
+ return ret;
+}
+
+static void __xipram do_write_oneword_done(struct map_info *map,
+ struct flchip *chip,
+ unsigned long adr, int mode)
+{
+ if (mode == FL_OTP_WRITE)
+ otp_exit(map, chip, adr, map_bankwidth(map));
+
+ chip->state = FL_READY;
+ DISABLE_VPP(map);
+ put_chip(map, chip, adr);
+
+ mutex_unlock(&chip->mutex);
+}
+
static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
unsigned long adr, map_word datum,
int mode)
@@ -1620,19 +1654,14 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,

adr += chip->start;

- mutex_lock(&chip->mutex);
- ret = get_chip(map, chip, adr, mode);
+ pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", __func__, adr,
+ datum.x[0]);
+
+ ret = do_write_oneword_start(map, chip, adr, mode);
if (ret) {
- mutex_unlock(&chip->mutex);
return ret;
}

- pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
- __func__, adr, datum.x[0]);
-
- if (mode == FL_OTP_WRITE)
- otp_enter(map, chip, adr, map_bankwidth(map));
-
/*
* Check for a NOP for the case when the datum to write is already
* present - it saves time and works around buggy chips that corrupt
@@ -1641,8 +1670,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
*/
oldd = map_read(map, adr);
if (map_word_equal(map, oldd, datum)) {
- pr_debug("MTD %s(): NOP\n",
- __func__);
+ pr_debug("MTD %s(): NOP\n", __func__);
goto op_done;
}

@@ -1670,12 +1698,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
xip_enable(map, chip, adr);

op_done:
- if (mode == FL_OTP_WRITE)
- otp_exit(map, chip, adr, map_bankwidth(map));
- chip->state = FL_READY;
- DISABLE_VPP(map);
- put_chip(map, chip, adr);
- mutex_unlock(&chip->mutex);
+ do_write_oneword_done(map, chip, adr, mode);

return ret;
}

--
2.18.0

Tokunori Ikegami

2018-10-25 16:32:15 UTC

This is just to refactor the function by removing the goto statement.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- Just rebased.

Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5009c1941a50..5fb986ea3243 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1671,7 +1671,8 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
oldd = map_read(map, adr);
if (map_word_equal(map, oldd, datum)) {
pr_debug("MTD %s(): NOP\n", __func__);
- goto op_done;
+ do_write_oneword_done(map, chip, adr, mode);
+ return ret;
}

XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
@@ -1697,7 +1698,6 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,

xip_enable(map, chip, adr);

- op_done:
do_write_oneword_done(map, chip, adr, mode);

return ret;

--
2.18.0

Tokunori Ikegami

2018-10-25 16:32:12 UTC

By the removed goto statement it can be called xip_enable() once.
Also for a maintainability refactor it to call the function only once.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- None.

Changes since v1:
- Split from the patch v1 3/3.

drivers/mtd/chips/cfi_cmdset_0002.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index deffafab067e..a3fa2d7b1ba0 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,10 +1882,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
continue;
}

- if (chip_good(map, adr, datum)) {
- xip_enable(map, chip, adr);
+ if (chip_good(map, adr, datum))
break;
- }

if (time_after(jiffies, timeo)) {
ret = -EIO;
@@ -1911,13 +1909,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
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 */

printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
__func__, adr);
}

+ xip_enable(map, chip, adr);
+
chip->state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);

--
2.18.0

Boris Brezillon

2018-11-05 13:20:17 UTC

On Fri, 26 Oct 2018 01:32:12 +0900

Post by Tokunori Ikegami
By the removed goto statement it can be called xip_enable() once.
Also for a maintainability refactor it to call the function only once.

Would have worked with the op_done label too if you place the
xip_enable() call just after this label, right?

Post by Tokunori Ikegami
---
- None.
- Split from the patch v1 3/3.
drivers/mtd/chips/cfi_cmdset_0002.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index deffafab067e..a3fa2d7b1ba0 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,10 +1882,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
continue;
}
- if (chip_good(map, adr, datum)) {
- xip_enable(map, chip, adr);
+ if (chip_good(map, adr, datum))
break;
- }
if (time_after(jiffies, timeo)) {
ret = -EIO;
@@ -1911,13 +1909,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
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 */
printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
__func__, adr);
}
+ xip_enable(map, chip, adr);
+
chip->state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);

IKEGAMI Tokunori

2018-11-06 00:42:46 UTC

-----Original Message-----
Sent: Monday, November 05, 2018 10:20 PM
To: IKEGAMI Tokunori
Subject: Re: [PATCH v3 04/11] mtd: cfi_cmdset_0002: Call xip_enable() once
only in do_write_buffer().
On Fri, 26 Oct 2018 01:32:12 +0900

Post by Tokunori Ikegami
By the removed goto statement it can be called xip_enable() once.
Also for a maintainability refactor it to call the function only once.

Would have worked with the op_done label too if you place the
xip_enable() call just after this label, right?

Yes I think so.
Since both the change does not change the actual behavior as that the place to call xip_enable() is changed but it is called as same with the original.

Regards,
Ikegami

b/drivers/mtd/chips/cfi_cmdset_0002.c

Post by Tokunori Ikegami
index deffafab067e..a3fa2d7b1ba0 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,10 +1882,8 @@ static int __xipram do_write_buffer(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
continue;
}
- if (chip_good(map, adr, datum)) {
- xip_enable(map, chip, adr);
+ if (chip_good(map, adr, datum))
break;
- }
if (time_after(jiffies, timeo)) {
ret = -EIO;
@@ -1911,13 +1909,14 @@ static int __xipram do_write_buffer(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
cfi->device_type, NULL);
cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start,

map, cfi,

Post by Tokunori Ikegami
cfi->device_type, NULL);
- xip_enable(map, chip, adr);
/* FIXME - should have reset delay before continuing */
printk(KERN_WARNING "MTD %s(): software timeout,

address:0x%.8lx.\n",

Post by Tokunori Ikegami
__func__, adr);
}
+ xip_enable(map, chip, adr);
+
chip->state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);

Tokunori Ikegami

2018-10-25 16:32:16 UTC

This is just to refactor the function by removing the goto statement.
Change to use the for loop instead of the goto statement.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- None.

Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5fb986ea3243..97e5b3948e20 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1679,21 +1679,17 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
ENABLE_VPP(map);
xip_disable(map, chip, adr);

- retry:
- ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+ for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
+ ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+
+ /* Did we succeed? */
+ if (!ret)
+ break;

- /* Did we succeed? */
- 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) {
- ret = 0;
- goto retry;
- }
-
- ret = -EIO;
+ /* FIXME - should have reset delay before continuing */
}

xip_enable(map, chip, adr);

--
2.18.0

Tokunori Ikegami

2018-10-25 16:32:17 UTC

Just refactor to split the sequence from do_write_buffer().

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- None.

Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 38 +++++++++++++++++------------
1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 97e5b3948e20..b54599cb6ae5 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1823,6 +1823,27 @@ static int cfi_amdstd_write_words(struct mtd_info *mtd, loff_t to, size_t len,
return 0;
}

+static void __xipram do_write_buffer_reset(struct map_info *map,
+ struct flchip *chip,
+ struct cfi_private *cfi)
+{
+ /*
+ * 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);
+
+ /* FIXME - should have reset delay before continuing */
+}

/*
* FIXME: interleaved mode not tested, and probably not supported!
@@ -1925,22 +1946,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
}

if (ret) {
- /*
- * 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);
- /* FIXME - should have reset delay before continuing */
-
+ do_write_buffer_reset(map, chip, cfi);
printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
__func__, adr);
}

--
2.18.0

Tokunori Ikegami

2018-10-25 16:32:19 UTC

The do_write_oneword_done() is called twice at the exit paths.
By splitting the retry functionality it can be reduced to call once.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 39 ++++++++++++++++++-----------
1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 0188187939c8..d76af85cfe59 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1643,25 +1643,16 @@ static void __xipram do_write_oneword_done(struct map_info *map,
mutex_unlock(&chip->mutex);
}

-static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
- unsigned long adr, map_word datum,
- int mode)
+static int __xipram do_write_oneword_retry(struct map_info *map,
+ struct flchip *chip,
+ unsigned long adr, map_word datum,
+ int mode)
{
struct cfi_private *cfi = map->fldrv_priv;
int ret = 0;
map_word oldd;
int retry_cnt = 0;

- adr += chip->start;
-
- pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", __func__, adr,
- datum.x[0]);
-
- ret = do_write_oneword_start(map, chip, adr, mode);
- if (ret) {
- return ret;
- }
-
/*
* Check for a NOP for the case when the datum to write is already
* present - it saves time and works around buggy chips that corrupt
@@ -1671,7 +1662,6 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
oldd = map_read(map, adr);
if (map_word_equal(map, oldd, datum)) {
pr_debug("MTD %s(): NOP\n", __func__);
- do_write_oneword_done(map, chip, adr, mode);
return ret;
}

@@ -1694,6 +1684,27 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,

xip_enable(map, chip, adr);

+ return ret;
+}
+
+static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
+ unsigned long adr, map_word datum,
+ int mode)
+{
+ int ret = 0;
+
+ adr += chip->start;
+
+ pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", __func__, adr,
+ datum.x[0]);
+
+ ret = do_write_oneword_start(map, chip, adr, mode);
+ if (ret) {
+ return ret;
+ }
+
+ ret = do_write_oneword_retry(map, chip, adr, datum, mode);
+
do_write_oneword_done(map, chip, adr, mode);

return ret;

--
2.18.0

Tokunori Ikegami

2018-10-25 16:32:18 UTC

Just refactor to split the wait from do_write_buffer().

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Cc: Fabio Bettoni <***@gmail.com>
Co: Hauke Mehrtens <***@hauke-m.de>
Co: Koen Vandeputte <***@ncentric.com>
Cc: Chris Packham <***@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <***@infinera.com>
Cc: Boris Brezillon <***@free-electrons.com>
Cc: linux-***@lists.infradead.org
---
Changes since v2:
- None.

Changes since v1:
- Add the patch.

drivers/mtd/chips/cfi_cmdset_0002.c | 81 ++++++++++++++++-------------
1 file changed, 46 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index b54599cb6ae5..0188187939c8 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1823,6 +1823,51 @@ static int cfi_amdstd_write_words(struct mtd_info *mtd, loff_t to, size_t len,
return 0;
}

+static int __xipram do_write_buffer_wait(struct map_info *map,
+ struct flchip *chip, unsigned long adr,
+ map_word datum)
+{
+ unsigned long timeo;
+ unsigned long uWriteTimeout;
+ int ret = 0;
+
+ /*
+ * Timeout is calculated according to CFI data, if available.
+ * See more comments in cfi_cmdset_0002().
+ */
+ uWriteTimeout = usecs_to_jiffies(chip->buffer_write_time_max);
+ timeo = jiffies + uWriteTimeout;
+
+ for (;;) {
+ if (chip->state != FL_WRITING) {
+ /* Someone's suspended the write. Sleep */
+ DECLARE_WAITQUEUE(wait, current);
+
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ add_wait_queue(&chip->wq, &wait);
+ mutex_unlock(&chip->mutex);
+ schedule();
+ remove_wait_queue(&chip->wq, &wait);
+ timeo = jiffies + (HZ / 2); /* FIXME */
+ mutex_lock(&chip->mutex);
+ continue;
+ }
+
+ if (chip_good(map, adr, datum))
+ break;
+
+ if (time_after(jiffies, timeo)) {
+ ret = -EIO;
+ break;
+ }
+
+ /* Latency issues. Drop the lock, wait a while and retry */
+ UDELAY(map, chip, adr, 1);
+ }
+
+ return ret;
+}
+
static void __xipram do_write_buffer_reset(struct map_info *map,
struct flchip *chip,
struct cfi_private *cfi)
@@ -1853,13 +1898,6 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
int len)
{
struct cfi_private *cfi = map->fldrv_priv;
- unsigned long timeo = jiffies + HZ;
- /*
- * Timeout is calculated according to CFI data, if available.
- * See more comments in cfi_cmdset_0002().
- */
- unsigned long uWriteTimeout =
- usecs_to_jiffies(chip->buffer_write_time_max);
int ret = -EIO;
unsigned long cmd_adr;
int z, words;
@@ -1916,34 +1954,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
adr, map_bankwidth(map),
chip->word_write_time);

- timeo = jiffies + uWriteTimeout;
-
- for (;;) {
- if (chip->state != FL_WRITING) {
- /* Someone's suspended the write. Sleep */
- DECLARE_WAITQUEUE(wait, current);
-
- set_current_state(TASK_UNINTERRUPTIBLE);
- add_wait_queue(&chip->wq, &wait);
- mutex_unlock(&chip->mutex);
- schedule();
- remove_wait_queue(&chip->wq, &wait);
- timeo = jiffies + (HZ / 2); /* FIXME */
- mutex_lock(&chip->mutex);
- continue;
- }
-
- if (chip_good(map, adr, datum))
- break;
-
- if (time_after(jiffies, timeo)) {
- ret = -EIO;
- break;
- }
-
- /* Latency issues. Drop the lock, wait a while and retry */
- UDELAY(map, chip, adr, 1);
- }
+ ret = do_write_buffer_wait(map, chip, adr, datum);

if (ret) {
do_write_buffer_reset(map, chip, cfi);

--
2.18.0

Boris Brezillon

2018-11-05 10:15:44 UTC

On Fri, 26 Oct 2018 01:32:09 +0900

Post by Tokunori Ikegami
In OpenWrt Project the flash write error caused on some products.

It's okay to mention that the issue was discovered by the OpenWRT team,
but I'd rephrase it differently.

"As reported by the OpenWRT team, write requests sometimes fail on some
platforms".

Post by Tokunori Ikegami
Also the issue can be fixed by using chip_good() instead of chip_ready().
The chip_ready() just checks the value from flash memory twice.
And the chip_good() checks the value with the expected value.
Probably the issue can be fixed as checked correctly by the chip_good().
So change to use chip_good() instead of chip_ready().

Well, that's not really explaining why you think chip_good() should be
used instead of chip_ready(). So I went on and looked at the
chip_good(), chip_ready() and do_write_oneword() implementation, and
also looked at users of do_write_oneword(). It seems this function is
used to write data to the flash, and apparently the "one bit should
toggle to reflect a busy state" does not apply when writing things to
the memory array (it's probably working for other CFI commands, but I
guess it takes more time to actually change the level of a NOR cell,
hence the result of 2 identical reads does not mean that the write is
done).

Also, it seems that cmdset_0001 is not implementing chip_ready() the
same way, and I wonder if cmdset_0002 implementation is correct to
start with. Or maybe I don't get what chip_ready() is for.

Anyway, this is the sort of clarification I'd like to have.
Has the patch really gone through all those people? SoB is used when you
apply a patch in your tree or when you're the original author.
Not sure we want to add new undocumented tags, but you can mention
that all those people helped you find/debug the issue. They can also
add their Reviewed-by/Tested-by if they like.

Post by Tokunori Ikegami
---
- Just update the commit message for the comment.
- Just update the commit message.
This is required for OpenWrt Project to result the flash write issue as
below patche.
<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=ddc11c3932c7b7b7df7d5fbd48f207e77619eaa7>
Also the original patch in OpenWRT is below.
<https://github.com/openwrt/openwrt/blob/v18.06.0/target/linux/ar71xx/patches-4.9/403-mtd_fix_cfi_cmdset_0002_status_check.patch>
The reason to use chip_good() is that just actually fix the issue.
And also in the past I had fixed the erase function also as same way by the
patch below.
<https://patchwork.ozlabs.org/patch/922656/>
Note: The reason for the patch for erase is same.
In my understanding the chip_ready() is just checked the value twice from
flash.
So I think that sometimes incorrect value is read twice and it is depended
on the flash device behavior but not sure..
So change to use chip_good() instead of chip_ready().
drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 72428b6bfc47..251c9e1675bd 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1627,31 +1627,37 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
continue;
}
- if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+ if (chip_good(map, adr, datum))
+ break;
+
+ if (time_after(jiffies, timeo)){
xip_enable(map, chip, adr);
printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
xip_disable(map, chip, adr);
+ ret = -EIO;
break;
}
- if (chip_ready(map, adr))
- break;
-
/* Latency issues. Drop the lock, wait a while and retry */
UDELAY(map, chip, adr, 1);
}
+
/* Did we succeed? */
- if (!chip_good(map, adr, datum)) {
+ 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;
}
+
xip_enable(map, chip, adr);
+

Not a big deal, but I'd prefer to not have coding style changes mixed
with functional changes (in other words, you can drop the addition of
blanks lines around xip_enable()).

Post by Tokunori Ikegami
if (mode == FL_OTP_WRITE)
otp_exit(map, chip, adr, map_bankwidth(map));

Joakim Tjernlund

2018-11-05 12:03:04 UTC

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Fri, 26 Oct 2018 01:32:09 +0900

Post by Tokunori Ikegami
In OpenWrt Project the flash write error caused on some products.

It's okay to mention that the issue was discovered by the OpenWRT team,
but I'd rephrase it differently.
"As reported by the OpenWRT team, write requests sometimes fail on some
platforms".

The 0001 cmd set is quite different to 0002 and 0001 is the superior one.
If you look at recent 0002 cmd sets they offer an alternative cmd
set to replace the all the "toggle" ones with something that is
same/similar to what 0001 offers.

Jocke

Boris Brezillon

2018-11-05 12:52:11 UTC

On Mon, 5 Nov 2018 12:03:04 +0000

Post by Tokunori Ikegami
In OpenWrt Project the flash write error caused on some products.

It's okay to mention that the issue was discovered by the OpenWRT team,
but I'd rephrase it differently.
"As reported by the OpenWRT team, write requests sometimes fail on some
platforms".

Okay. Do you know when chip_ready() (the one that checks if something
changes between 2 reads) should be used and when it shouldn't?

Joakim Tjernlund

2018-11-05 13:22:19 UTC

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Mon, 5 Nov 2018 12:03:04 +0000

Post by Tokunori Ikegami
In OpenWrt Project the flash write error caused on some products.

It's okay to mention that the issue was discovered by the OpenWRT team,
but I'd rephrase it differently.
"As reported by the OpenWRT team, write requests sometimes fail on some
platforms".

Okay. Do you know when chip_ready() (the one that checks if something
changes between 2 reads) should be used and when it shouldn't?

It is next to impossible to do proper error handling(analysing status) with
toggle method, especially when you have interleaved chips.
Try with erase suspend when something goes wrong and you want
to address that properly.
Best is to add support for the extended 0002 cmd set and use that
whenever possible.

Jocke

Boris Brezillon

2018-11-05 13:58:42 UTC

Hi Joakim,

On Mon, 5 Nov 2018 13:22:19 +0000

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Mon, 5 Nov 2018 12:03:04 +0000

Post by Tokunori Ikegami
In OpenWrt Project the flash write error caused on some products.

It's okay to mention that the issue was discovered by the OpenWRT team,
but I'd rephrase it differently.
"As reported by the OpenWRT team, write requests sometimes fail on some
platforms".

Okay. Do you know when chip_ready() (the one that checks if something
changes between 2 reads) should be used and when it shouldn't?

It is next to impossible to do proper error handling(analysing status) with
toggle method, especially when you have interleaved chips.

It's probably me who does not understand how CFI works, but it sounds
weird to have chip_ready() called on something that's not a status
register (this is my understanding of what do_write_oneword() does).

Post by Joakim Tjernlund
Try with erase suspend when something goes wrong and you want
to address that properly.

I trust you when you say it does not work when using chip_ready(), but
I'd like to understand why. Well, first I'd like to understand what
chip_ready() is supposed to do, and on which kind of access/address it's
supposed to be used. As you already noticed I don't know a lot about
CFI, and that's why it's important to me to have things clearly
explained in the commit message.

Post by Joakim Tjernlund
Best is to add support for the extended 0002 cmd set and use that
whenever possible.

Okay, does that mean we should replace all chip_ready() calls by
chip_good() ones until support for ext 0002 cmdset is added?
To be honest, I have a hard time understanding what chip_ready() is
supposed to tell us. To me it's something that should return 1 when the
chip is ready to accept new requests, but I don't see how comparing
values returned by 2 successive reads can provide me this information.
Can you maybe point me to the CFI 0002 cmdset spec describing that?

Thanks,

Boris

IKEGAMI Tokunori

2018-11-06 00:25:43 UTC

Thank you so much for your reviewing.

-----Original Message-----
Sent: Monday, November 05, 2018 7:16 PM
To: IKEGAMI Tokunori
Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
do_write_oneword() to use chip_good()
On Fri, 26 Oct 2018 01:32:09 +0900

Post by Tokunori Ikegami
In OpenWrt Project the flash write error caused on some products.

It's okay to mention that the issue was discovered by the OpenWRT team,
but I'd rephrase it differently.
"As reported by the OpenWRT team, write requests sometimes fail on some
platforms".

Yes I will do fix as this.

Well, that's not really explaining why you think chip_good() should be
used instead of chip_ready(). So I went on and looked at the
chip_good(), chip_ready() and do_write_oneword() implementation, and
also looked at users of do_write_oneword(). It seems this function is
used to write data to the flash, and apparently the "one bit should
toggle to reflect a busy state" does not apply when writing things to
the memory array (it's probably working for other CFI commands, but I
guess it takes more time to actually change the level of a NOR cell,
hence the result of 2 identical reads does not mean that the write is
done).
Also, it seems that cmdset_0001 is not implementing chip_ready() the
same way, and I wonder if cmdset_0002 implementation is correct to
start with. Or maybe I don't get what chip_ready() is for.
Anyway, this is the sort of clarification I'd like to have.

I am thinking to update the commit message as below.

mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

As reported by the OpenWRT team, write requests sometimes fail on some
platforms.
Currently to check the state chip_ready() is used correctly as described by
the flash memory S29GL256P11TFI01 datasheet.
Also chip_good() is used to check if the write is succeeded and it was
implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
checking").
But actually the write failure is caused on some platforms and also it can
be fixed by using chip_good() to check the state and retry instead.
It is depended on the actual flash chip behavior so the root cause is
unknown.

If any comment please let me know.

Has the patch really gone through all those people? SoB is used when you
apply a patch in your tree or when you're the original author.

I have just checked the OpenWRT git log again and it looks that it was originally
implemented by Felix Fietkau <***@openwrt.org> by the patch below so I will update the Signed-off-by tag as so.
<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640f07cd2b3b14fe9ec03fa63a586452cc5f>

Not sure we want to add new undocumented tags, but you can mention
that all those people helped you find/debug the issue. They can also
add their Reviewed-by/Tested-by if they like.

Yes so I am thinking to change as below.

Signed-off-by: Tokunori Ikegami <***@allied-telesis.co.jp>
Signed-off-by: Felix Fietkau <***@openwrt.org>
Tested-by: Fabio Bettoni <***@gmail.com>
Reported-by: Fabio Bettoni <***@gmail.com>
Cc: Hauke Mehrtens <***@hauke-m.de>
Cc: Koen Vandeputte <***@ncentric.com>

If any problem let me know.

By the way the patch has been tested by Fabio-san then there was still the failure behavior.
And it was not followed the original patch changes correctly.
So I will update the patch change a little by the next version v4 patch.
Note: It has been retested by the Fabio-san as okay.

<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=ddc11c3
932c7b7b7df7d5fbd48f207e77619eaa7>

Post by Tokunori Ikegami
Also the original patch in OpenWRT is below.

<https://github.com/openwrt/openwrt/blob/v18.06.0/target/linux/ar71xx/
patches-4.9/403-mtd_fix_cfi_cmdset_0002_status_check.patch>

Post by Tokunori Ikegami
The reason to use chip_good() is that just actually fix the issue.
And also in the past I had fixed the erase function also as same way by

the

Post by Tokunori Ikegami
patch below.
<https://patchwork.ozlabs.org/patch/922656/>
Note: The reason for the patch for erase is same.
In my understanding the chip_ready() is just checked the value twice from
flash.
So I think that sometimes incorrect value is read twice and it is depended
on the flash device behavior but not sure..
So change to use chip_good() instead of chip_ready().
drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c

b/drivers/mtd/chips/cfi_cmdset_0002.c

Post by Tokunori Ikegami
index 72428b6bfc47..251c9e1675bd 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1627,31 +1627,37 @@ static int __xipram do_write_oneword(struct

map_info *map, struct flchip *chip,

Post by Tokunori Ikegami
continue;
}
- if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+ if (chip_good(map, adr, datum))
+ break;
+
+ if (time_after(jiffies, timeo)){
xip_enable(map, chip, adr);
printk(KERN_WARNING "MTD %s(): software

timeout\n", __func__);

Post by Tokunori Ikegami
xip_disable(map, chip, adr);
+ ret = -EIO;
break;
}
- if (chip_ready(map, adr))
- break;
-
/* Latency issues. Drop the lock, wait a while and retry

Post by Tokunori Ikegami
UDELAY(map, chip, adr, 1);
}
+
/* Did we succeed? */
- if (!chip_good(map, adr, datum)) {
+ 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;
}
+
xip_enable(map, chip, adr);
+

Not a big deal, but I'd prefer to not have coding style changes mixed
with functional changes (in other words, you can drop the addition of
blanks lines around xip_enable()).

Yes I will do fix this.

Regards,
Ikegami

Post by Tokunori Ikegami
if (mode == FL_OTP_WRITE)
otp_exit(map, chip, adr, map_bankwidth(map));

Boris Brezillon

2018-11-06 08:33:34 UTC

Hi IKEGAMI,

On Tue, 6 Nov 2018 00:25:43 +0000

Well, that's not really explaining why you think chip_good() should be
used instead of chip_ready(). So I went on and looked at the
chip_good(), chip_ready() and do_write_oneword() implementation, and
also looked at users of do_write_oneword(). It seems this function is
used to write data to the flash, and apparently the "one bit should
toggle to reflect a busy state" does not apply when writing things to
the memory array (it's probably working for other CFI commands, but I
guess it takes more time to actually change the level of a NOR cell,
hence the result of 2 identical reads does not mean that the write is
done).
Also, it seems that cmdset_0001 is not implementing chip_ready() the
same way, and I wonder if cmdset_0002 implementation is correct to
start with. Or maybe I don't get what chip_ready() is for.
Anyway, this is the sort of clarification I'd like to have.

I had a look at the S29GL256P datasheet here [1], and if I'm correct,
it's using cmdset 0001.

Post by IKEGAMI Tokunori
Also chip_good() is used to check if the write is succeeded and it was
implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
checking").
But actually the write failure is caused on some platforms and also it can
be fixed by using chip_good() to check the state and retry instead.

Do you know on which NOR chips this happens? Do you have access to the
datasheet?

Post by IKEGAMI Tokunori
It is depended on the actual flash chip behavior so the root cause is
unknown.

Yes, and that's what I'd like you to figure out, or at least have a
good idea why this doesn't work on some chips but works on others.

Post by IKEGAMI Tokunori
If any comment please let me know.

Post by Boris Brezillon
Has the patch really gone through all those people? SoB is used when you
apply a patch in your tree or when you're the original author.

I have just checked the OpenWRT git log again and it looks that it was originally
<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640f07cd2b3b14fe9ec03fa63a586452cc5f>

Post by Boris Brezillon
Not sure we want to add new undocumented tags, but you can mention
that all those people helped you find/debug the issue. They can also
add their Reviewed-by/Tested-by if they like.

My bad, I just noticed these are valid flags [2], so you can keep them,
and according to the doc, you should also keep the SoB.

Regards,

Boris

[1]http://www.cypress.com/file/219926/download
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546

IKEGAMI Tokunori

2018-11-06 09:37:12 UTC

Hi Boris-san,

-----Original Message-----
Sent: Tuesday, November 06, 2018 5:34 PM
To: IKEGAMI Tokunori
Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
do_write_oneword() to use chip_good()
Hi IKEGAMI,
On Tue, 6 Nov 2018 00:25:43 +0000

Post by Tokunori Ikegami
Also the issue can be fixed by using chip_good() instead of

chip_ready().

Post by Tokunori Ikegami
The chip_ready() just checks the value from flash memory twice.
And the chip_good() checks the value with the expected value.
Probably the issue can be fixed as checked correctly by the chip_good().
So change to use chip_good() instead of chip_ready().

Well, that's not really explaining why you think chip_good() should

Post by Boris Brezillon
used instead of chip_ready(). So I went on and looked at the
chip_good(), chip_ready() and do_write_oneword() implementation, and
also looked at users of do_write_oneword(). It seems this function is
used to write data to the flash, and apparently the "one bit should
toggle to reflect a busy state" does not apply when writing things to
the memory array (it's probably working for other CFI commands, but

Post by Boris Brezillon
guess it takes more time to actually change the level of a NOR cell,
hence the result of 2 identical reads does not mean that the write is
done).
Also, it seems that cmdset_0001 is not implementing chip_ready() the
same way, and I wonder if cmdset_0002 implementation is correct to
start with. Or maybe I don't get what chip_ready() is for.
Anyway, this is the sort of clarification I'd like to have.

I am thinking to update the commit message as below.
mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
As reported by the OpenWRT team, write requests sometimes fail on

some

Post by IKEGAMI Tokunori
platforms.
Currently to check the state chip_ready() is used correctly as

described by

Post by IKEGAMI Tokunori
the flash memory S29GL256P11TFI01 datasheet.

I had a look at the S29GL256P datasheet here [1], and if I'm correct,
it's using cmdset 0001.

No actually the cmdset 0002 is used on the flash chip.
The manufacturer ID xx01h and Device ID 2201h are used to decide.

There is information from Fobis-san below also about this.

On forum thread musashino posted picture of flash chip:
https://forum.openwrt.org/t/impossible-to-install-update-any-packages-on-wzr-hp-g300nh-18-06-1
http://www.cypress.com/part/s29gl256p11tfi010

[ 0.862264] physmap platform flash device: 02000000 at 1e000000
[ 0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
bank. Manufacturer ID 0x000001 Chip ID 0x002201
[ 0.878493] Amd/Fujitsu Extended Query Table at 0x0040
[ 0.883668] Amd/Fujitsu Extended Query version 1.3.
[ 0.888768] number of CFI chips: 1
[ 0.894557] Searching for RedBoot partition table in physmap-flash
at offset 0x1fc0000
[ 0.918009] Searching for RedBoot partition table in physmap-flash
at offset 0x1fe0000
[ 0.941464] No RedBoot partition table detected in physmap-flash
[ 0.947926] Creating 5 MTD partitions on "physmap-flash":
[ 0.953384] 0x000000000000-0x000000040000 : "u-boot"
[ 0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
[ 0.968803] 0x000000060000-0x000001fc0000 : "firmware"
[ 0.981859] 2 uimage-fw partitions found on MTD device firmware
[ 0.987900] 0x000000060000-0x0000001b5706 : "kernel"
[ 0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
[ 1.001986] mtd: device 4 (rootfs) set to be root filesystem
[ 1.007789] 1 squashfs-split partitions found on MTD device rootfs
[ 1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
[ 1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
[ 1.030283] 0x000001fe0000-0x000002000000 : "art"

Maybe you could post links to forum thread, and data sheet.

Post by IKEGAMI Tokunori
Also chip_good() is used to check if the write is succeeded and it

was

Post by IKEGAMI Tokunori
implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve

error

Post by IKEGAMI Tokunori
checking").
But actually the write failure is caused on some platforms and also

it can

Post by IKEGAMI Tokunori
be fixed by using chip_good() to check the state and retry instead.

Do you know on which NOR chips this happens? Do you have access to the
datasheet?

But it looks SST49LF008A [3] from the changes below but I am not sure at this moment and probably it should be confirmed to the authr Eric W. Biedermann <***@lnxi.com> to make sure.

+#define SST49LF008A 0x005a

static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
@@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
};
static struct cfi_fixup jedec_fixup_table[] = {
{ MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock, NULL, },
+ { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock, NULL, },

Post by IKEGAMI Tokunori
It is depended on the actual flash chip behavior so the root cause

Post by IKEGAMI Tokunori
unknown.

Yes, and that's what I'd like you to figure out, or at least have a
good idea why this doesn't work on some chips but works on others.

I see.
But it is a little bit difficult situation since I do not have the failure environment locally at this moment.
But if needed I may ask to get the help for this to Fabio-san.

Post by IKEGAMI Tokunori
If any comment please let me know.

Post by Boris Brezillon
Has the patch really gone through all those people? SoB is used when

you

Post by Boris Brezillon
apply a patch in your tree or when you're the original author.

I have just checked the OpenWRT git log again and it looks that it was

originally
will update the Signed-off-by tag as so.
<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640
f07cd2b3b14fe9ec03fa63a586452cc5f>

My bad, I just noticed these are valid flags [2], so you can keep them,
and according to the doc, you should also keep the SoB.

I see.
Yes I had also checked it.

By the way in near future my company email address will be not able to use.
So I will change the mail address to my personal email address [4] after that or before.

Regards,
Ikegami

Regards,
Boris
[1]http://www.cypress.com/file/219926/download
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546

[3]https://www.microchip.com/wwwproducts/en/SST49LF008A
[4]***@yahoo.co.jp

IKEGAMI Tokunori

2018-11-06 09:47:23 UTC

Sorry let me resend the mail below by changing the email address of Felix-san.

-----Original Message-----
From: IKEGAMI Tokunori
Sent: Tuesday, November 06, 2018 6:37 PM
To: 'Boris Brezillon'; '***@yahoo.co.jp'
Cc: ***@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
***@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
linux-***@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
do_write_oneword() to use chip_good()

Hi Boris-san,

Post by Tokunori Ikegami
Also the issue can be fixed by using chip_good() instead of

chip_ready().

Well, that's not really explaining why you think chip_good() should

I am thinking to update the commit message as below.
mtd: cfi_cmdset_0002: Use chip_good() to retry in

do_write_oneword()

Post by IKEGAMI Tokunori
As reported by the OpenWRT team, write requests sometimes fail on

some

Post by IKEGAMI Tokunori
platforms.
Currently to check the state chip_ready() is used correctly as

described by

Post by IKEGAMI Tokunori
the flash memory S29GL256P11TFI01 datasheet.

I had a look at the S29GL256P datasheet here [1], and if I'm correct,
it's using cmdset 0001.

No actually the cmdset 0002 is used on the flash chip.
The manufacturer ID xx01h and Device ID 2201h are used to decide.

There is information from Fobis-san below also about this.

On forum thread musashino posted picture of flash chip:
https://forum.openwrt.org/t/impossible-to-install-update-any-packages-
on-wzr-hp-g300nh-18-06-1
http://www.cypress.com/part/s29gl256p11tfi010

[ 0.862264] physmap platform flash device: 02000000 at 1e000000
[ 0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
bank. Manufacturer ID 0x000001 Chip ID 0x002201
[ 0.878493] Amd/Fujitsu Extended Query Table at 0x0040
[ 0.883668] Amd/Fujitsu Extended Query version 1.3.
[ 0.888768] number of CFI chips: 1
[ 0.894557] Searching for RedBoot partition table in physmap-flash
at offset 0x1fc0000
[ 0.918009] Searching for RedBoot partition table in physmap-flash
at offset 0x1fe0000
[ 0.941464] No RedBoot partition table detected in physmap-flash
[ 0.947926] Creating 5 MTD partitions on "physmap-flash":
[ 0.953384] 0x000000000000-0x000000040000 : "u-boot"
[ 0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
[ 0.968803] 0x000000060000-0x000001fc0000 : "firmware"
[ 0.981859] 2 uimage-fw partitions found on MTD device firmware
[ 0.987900] 0x000000060000-0x0000001b5706 : "kernel"
[ 0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
[ 1.001986] mtd: device 4 (rootfs) set to be root filesystem
[ 1.007789] 1 squashfs-split partitions found on MTD device rootfs
[ 1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
[ 1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
[ 1.030283] 0x000001fe0000-0x000002000000 : "art"

Maybe you could post links to forum thread, and data sheet.

Post by IKEGAMI Tokunori
Also chip_good() is used to check if the write is succeeded and

was

Post by IKEGAMI Tokunori
implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve

error

Post by IKEGAMI Tokunori
checking").
But actually the write failure is caused on some platforms and also

it can

Post by IKEGAMI Tokunori
be fixed by using chip_good() to check the state and retry instead.

Do you know on which NOR chips this happens? Do you have access to the
datasheet?

But it looks SST49LF008A [3] from the changes below but I am not sure at
this moment and probably it should be confirmed to the authr Eric W.
Biedermann <***@lnxi.com> to make sure.

+#define SST49LF008A 0x005a

static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *,
u_char *);
static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t,
size_t *, const u_char *);
@@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
};
static struct cfi_fixup jedec_fixup_table[] = {
{ MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock, NULL, },
+ { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock, NULL, },

Post by IKEGAMI Tokunori
It is depended on the actual flash chip behavior so the root cause

Post by IKEGAMI Tokunori
unknown.

Yes, and that's what I'd like you to figure out, or at least have a
good idea why this doesn't work on some chips but works on others.

I see.
But it is a little bit difficult situation since I do not have the failure
environment locally at this moment.
But if needed I may ask to get the help for this to Fabio-san.

Post by IKEGAMI Tokunori
If any comment please let me know.

Post by Boris Brezillon
Has the patch really gone through all those people? SoB is used when

you

Post by Boris Brezillon
apply a patch in your tree or when you're the original author.

I have just checked the OpenWRT git log again and it looks that it was

originally

will update the Signed-off-by tag as so.

<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640

f07cd2b3b14fe9ec03fa63a586452cc5f>