All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project
@ 2019-05-26 15:38 Tokunori Ikegami
  2019-05-26 15:38   ` Tokunori Ikegami
                   ` (10 more replies)
  0 siblings, 11 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

The change is based on the fix for flash erase to use chip_good() done in the past.
And it is fixed as same way in the OpenWrt Project as below.
  <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=ddc11c3932c7b7b7df7d5fbd48f207e77619eaa7>
Also includes some refactoring changes.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org

Tokunori Ikegami (11):
  mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
  mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()
  mtd: cfi_cmdset_0002: Call xip_enable() once only in
    do_write_buffer().
  mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size
  mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement
  mtd: cfi_cmdset_0002: Remove op_done goto statement from
    do_write_oneword()
  mtd: cfi_cmdset_0002: Remove retry goto statement from
    do_write_oneword()
  mtd: cfi_cmdset_0002: Split write-to-buffer-reset sequence
  mtd: cfi_cmdset_0002: Split to wait write buffer to check if completed
  mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths
  mtd: cfi_cmdset_0002: Disable write buffer functions if
    FORCE_WORD_WRITE is 1

 drivers/mtd/chips/cfi_cmdset_0002.c | 288 ++++++++++++++++++++++--------------
 1 file changed, 180 insertions(+), 108 deletions(-)
 mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c

-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v6 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
@ 2019-05-26 15:38   ` Tokunori Ikegami
  2019-05-26 15:38 ` [PATCH v6 02/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer() Tokunori Ikegami
                     ` (9 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Felix Fietkau, Chris Packham, Joakim Tjernlund,
	linux-mtd, stable

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.
Also it seems that it is caused after repeated about 1,000 times to retry
the write one word with the reset command.
By using chip_good() to check the state to be done it can be reduced the
retry with reset.
It is depended on the actual flash chip behavior so the root cause is
unknown.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>
Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Reported-by: Fabio Bettoni <fbettoni@gmail.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change to follow Liu Jian's fixes in master for the write buffer.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Update the commit message for the comments.
- Drop the addition of blanks lines around xip_enable().
- Delete unnecessary setting the ret variable to -EIO.
- Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

Changes since v2:
- Just update the commit message for the comment.

Changes since v1:
- Just update the commit message.

Background:
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(-)
 mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
old mode 100644
new mode 100755
index c8fa5906bdf9..348b54820e4c
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1628,29 +1628,35 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 			continue;
 		}
 
-		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+		/*
+		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
+		 * the failure due to scheduling.
+		 */
+		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)){
 			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))
+		if (chip_good(map, adr, datum))
 			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);
  op_done:
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
@ 2019-05-26 15:38   ` Tokunori Ikegami
  0 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, stable, Joakim Tjernlund, Chris Packham,
	linux-mtd, Felix Fietkau

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.
Also it seems that it is caused after repeated about 1,000 times to retry
the write one word with the reset command.
By using chip_good() to check the state to be done it can be reduced the
retry with reset.
It is depended on the actual flash chip behavior so the root cause is
unknown.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>
Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Reported-by: Fabio Bettoni <fbettoni@gmail.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change to follow Liu Jian's fixes in master for the write buffer.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Update the commit message for the comments.
- Drop the addition of blanks lines around xip_enable().
- Delete unnecessary setting the ret variable to -EIO.
- Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

Changes since v2:
- Just update the commit message for the comment.

Changes since v1:
- Just update the commit message.

Background:
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(-)
 mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
old mode 100644
new mode 100755
index c8fa5906bdf9..348b54820e4c
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1628,29 +1628,35 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 			continue;
 		}
 
-		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+		/*
+		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
+		 * the failure due to scheduling.
+		 */
+		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)){
 			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))
+		if (chip_good(map, adr, datum))
 			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);
  op_done:
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 02/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
  2019-05-26 15:38   ` Tokunori Ikegami
@ 2019-05-26 15:38 ` Tokunori Ikegami
  2019-06-15 13:42   ` Vignesh Raghavendra
  2019-05-26 15:38 ` [PATCH v6 03/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer() Tokunori Ikegami
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

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

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Just change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

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 348b54820e4c..ca41f47c00c1 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1887,40 +1887,42 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
 		 * the failure due to scheduling.
 		 */
-		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum))
+		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) {
+			ret = -EIO;
 			break;
+		}
 
 		if (chip_good(map, adr, datum)) {
 			xip_enable(map, chip, adr);
-			goto op_done;
+			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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 03/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer().
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
  2019-05-26 15:38   ` Tokunori Ikegami
  2019-05-26 15:38 ` [PATCH v6 02/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer() Tokunori Ikegami
@ 2019-05-26 15:38 ` Tokunori Ikegami
  2019-06-15 13:46   ` Vignesh Raghavendra
  2019-05-26 15:38 ` [PATCH v6 04/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size Tokunori Ikegami
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

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 <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Just change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

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 ca41f47c00c1..2654019ee24b 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1892,10 +1892,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 			break;
 		}
 
-		if (chip_good(map, adr, datum)) {
-			xip_enable(map, chip, adr);
+		if (chip_good(map, adr, datum))
 			break;
-		}
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
@@ -1916,13 +1914,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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 04/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (2 preceding siblings ...)
  2019-05-26 15:38 ` [PATCH v6 03/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer() Tokunori Ikegami
@ 2019-05-26 15:38 ` Tokunori Ikegami
  2019-05-26 15:38 ` [PATCH v6 05/11] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement Tokunori Ikegami
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, linux-mtd, Fabio Bettoni, Joakim Tjernlund

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

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Drop the unnecessary comment and blank line.
- Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

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, 49 insertions(+), 40 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 2654019ee24b..3cd491c54ff5 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1548,11 +1548,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
@@ -1565,42 +1564,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);
@@ -1647,7 +1611,52 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 		UDELAY(map, chip, adr, 1);
 	}
 
-	/* Did we succeed? */
+	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);
 	if (ret) {
 		/* reset on all failures. */
 		map_write(map, CMD(0xF0), chip->start);
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 05/11] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (3 preceding siblings ...)
  2019-05-26 15:38 ` [PATCH v6 04/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size Tokunori Ikegami
@ 2019-05-26 15:38 ` Tokunori Ikegami
  2019-05-26 15:38 ` [PATCH v6 06/11] mtd: cfi_cmdset_0002: Remove op_done goto statement from do_write_oneword() Tokunori Ikegami
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

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 <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Rebased on the patch v4 01/11.
- Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

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 3cd491c54ff5..ae4d1649d47c 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1614,6 +1614,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)
@@ -1625,19 +1659,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
@@ -1646,8 +1675,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;
 	}
 
@@ -1669,12 +1697,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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 06/11] mtd: cfi_cmdset_0002: Remove op_done goto statement from do_write_oneword()
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (4 preceding siblings ...)
  2019-05-26 15:38 ` [PATCH v6 05/11] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement Tokunori Ikegami
@ 2019-05-26 15:38 ` Tokunori Ikegami
  2019-05-26 15:39 ` [PATCH v6 07/11] mtd: cfi_cmdset_0002: Remove retry " Tokunori Ikegami
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:38 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

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

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Rebased on the patch v4 01/11.
- Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

Changes since v2:
- Just rebased.

Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ae4d1649d47c..597b0f18269f 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1676,7 +1676,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));
@@ -1696,7 +1697,7 @@ 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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 07/11] mtd: cfi_cmdset_0002: Remove retry goto statement from do_write_oneword()
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (5 preceding siblings ...)
  2019-05-26 15:38 ` [PATCH v6 06/11] mtd: cfi_cmdset_0002: Remove op_done goto statement from do_write_oneword() Tokunori Ikegami
@ 2019-05-26 15:39 ` Tokunori Ikegami
  2019-06-15 14:52   ` Vignesh Raghavendra
  2019-05-26 15:39 ` [PATCH v6 08/11] mtd: cfi_cmdset_0002: Split write-to-buffer-reset sequence Tokunori Ikegami
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:39 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

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 <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Rebased.
- Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

Changes since v2:
- None.

Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 597b0f18269f..7784be8246cb 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1684,17 +1684,15 @@ 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);
-	if (ret) {
+	for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
+		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 */
 
-		if (++retry_cnt <= MAX_RETRIES) {
-			ret = 0;
-			goto retry;
-		}
+		/* FIXME - should have reset delay before continuing */
 	}
 	xip_enable(map, chip, adr);
 
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 08/11] mtd: cfi_cmdset_0002: Split write-to-buffer-reset sequence
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (6 preceding siblings ...)
  2019-05-26 15:39 ` [PATCH v6 07/11] mtd: cfi_cmdset_0002: Remove retry " Tokunori Ikegami
@ 2019-05-26 15:39 ` Tokunori Ikegami
  2019-05-26 15:39 ` [PATCH v6 09/11] mtd: cfi_cmdset_0002: Split to wait write buffer to check if completed Tokunori Ikegami
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:39 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

Just refactor to split the sequence from do_write_buffer().

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Just change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

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 7784be8246cb..a3e57788bc40 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1825,6 +1825,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!
@@ -1931,22 +1952,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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 09/11] mtd: cfi_cmdset_0002: Split to wait write buffer to check if completed
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (7 preceding siblings ...)
  2019-05-26 15:39 ` [PATCH v6 08/11] mtd: cfi_cmdset_0002: Split write-to-buffer-reset sequence Tokunori Ikegami
@ 2019-05-26 15:39 ` Tokunori Ikegami
  2019-05-26 15:39 ` [PATCH v6 10/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths Tokunori Ikegami
  2019-05-26 15:39 ` [PATCH v6 11/11] mtd: cfi_cmdset_0002: Disable write buffer functions if FORCE_WORD_WRITE is 1 Tokunori Ikegami
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:39 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

Just refactor to split the wait from do_write_buffer().

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Just change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

Changes since v2:
- None.

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 a3e57788bc40..ebf420e1db43 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1825,6 +1825,55 @@ 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;
+		}
+
+		/*
+		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
+		 * the failure due to scheduling.
+		 */
+		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) {
+			ret = -EIO;
+			break;
+		}
+
+		if (chip_good(map, adr, datum))
+			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)
@@ -1855,13 +1904,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;
@@ -1918,38 +1960,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;
-		}
-
-		/*
-		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
-		 * the failure due to scheduling.
-		 */
-		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) {
-			ret = -EIO;
-			break;
-		}
-
-		if (chip_good(map, adr, datum))
-			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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 10/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (8 preceding siblings ...)
  2019-05-26 15:39 ` [PATCH v6 09/11] mtd: cfi_cmdset_0002: Split to wait write buffer to check if completed Tokunori Ikegami
@ 2019-05-26 15:39 ` Tokunori Ikegami
  2019-05-26 15:39 ` [PATCH v6 11/11] mtd: cfi_cmdset_0002: Disable write buffer functions if FORCE_WORD_WRITE is 1 Tokunori Ikegami
  10 siblings, 0 replies; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:39 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

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 <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Rebased on top of Liu Jian's fixes in master.
- Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.

Changes since v4:
- None.

Changes since v3:
- Just change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.

Changes since v2:
- 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 ebf420e1db43..477bc3d65e68 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1648,25 +1648,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
@@ -1676,7 +1667,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;
 	}
 
@@ -1696,6 +1686,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.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 11/11] mtd: cfi_cmdset_0002: Disable write buffer functions if FORCE_WORD_WRITE is 1
  2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
                   ` (9 preceding siblings ...)
  2019-05-26 15:39 ` [PATCH v6 10/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths Tokunori Ikegami
@ 2019-05-26 15:39 ` Tokunori Ikegami
  2019-06-18  8:05   ` Vignesh Raghavendra
  10 siblings, 1 reply; 19+ messages in thread
From: Tokunori Ikegami @ 2019-05-26 15:39 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tokunori Ikegami, Chris Packham, linux-mtd, Fabio Bettoni,
	Joakim Tjernlund

Some write buffer functions are not used when FORCE_WORD_WRITE is set to 1.
So the compile warning messages are output if FORCE_WORD_WRITE is 1.
To resolve this disable the write buffer functions if FORCE_WORD_WRITE is 1.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: Fabio Bettoni <fbettoni@gmail.com>
Co: Hauke Mehrtens <hauke@hauke-m.de>
Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v5:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 477bc3d65e68..2144221029e9 100755
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -51,7 +51,9 @@
 
 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 *);
+#if !FORCE_WORD_WRITE
 static int cfi_amdstd_write_buffers(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
+#endif
 static int cfi_amdstd_erase_chip(struct mtd_info *, struct erase_info *);
 static int cfi_amdstd_erase_varsize(struct mtd_info *, struct erase_info *);
 static void cfi_amdstd_sync (struct mtd_info *);
@@ -202,6 +204,7 @@ static void fixup_amd_bootblock(struct mtd_info *mtd)
 }
 #endif
 
+#if !FORCE_WORD_WRITE
 static void fixup_use_write_buffers(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
@@ -211,6 +214,7 @@ static void fixup_use_write_buffers(struct mtd_info *mtd)
 		mtd->_write = cfi_amdstd_write_buffers;
 	}
 }
+#endif /* !FORCE_WORD_WRITE */
 
 /* Atmel chips don't use the same PRI format as AMD chips */
 static void fixup_convert_atmel_pri(struct mtd_info *mtd)
@@ -1836,6 +1840,7 @@ static int cfi_amdstd_write_words(struct mtd_info *mtd, loff_t to, size_t len,
 	return 0;
 }
 
+#if !FORCE_WORD_WRITE
 static int __xipram do_write_buffer_wait(struct map_info *map,
 					 struct flchip *chip, unsigned long adr,
 					 map_word datum)
@@ -2064,6 +2069,7 @@ static int cfi_amdstd_write_buffers(struct mtd_info *mtd, loff_t to, size_t len,
 
 	return 0;
 }
+#endif /* !FORCE_WORD_WRITE */
 
 /*
  * Wait for the flash chip to become ready to write data
-- 
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
  2019-05-26 15:38   ` Tokunori Ikegami
@ 2019-06-15 13:38     ` Vignesh Raghavendra
  -1 siblings, 0 replies; 19+ messages in thread
From: Vignesh Raghavendra @ 2019-06-15 13:38 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: Felix Fietkau, Chris Packham, Joakim Tjernlund, linux-mtd, stable



On 26-May-19 9:08 PM, Tokunori Ikegami wrote:
> 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.
> Also it seems that it is caused after repeated about 1,000 times to retry
> the write one word with the reset command.
> By using chip_good() to check the state to be done it can be reduced the
> retry with reset.
> It is depended on the actual flash chip behavior so the root cause is
> unknown.
> 
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>

I need sign of all co-developers before applying the patch.
Please run ./scripts/checkpatch.pl --strict on all patches and address
all the issues.

Regards
Vignesh

> Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org
> ---
> Changes since v5:
> - Rebased on top of Liu Jian's fixes in master.
> - Change to follow Liu Jian's fixes in master for the write buffer.
> - Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.
> 
> Changes since v4:
> - None.
> 
> Changes since v3:
> - Update the commit message for the comments.
> - Drop the addition of blanks lines around xip_enable().
> - Delete unnecessary setting the ret variable to -EIO.
> - Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.
> 
> Changes since v2:
> - Just update the commit message for the comment.
> 
> Changes since v1:
> - Just update the commit message.
> 
> Background:
> 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(-)
>  mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> old mode 100644
> new mode 100755
> index c8fa5906bdf9..348b54820e4c
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1628,29 +1628,35 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
>  			continue;
>  		}
>  
> -		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> +		/*
> +		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
> +		 * the failure due to scheduling.
> +		 */
> +		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)){
>  			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))
> +		if (chip_good(map, adr, datum))
>  			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);
>   op_done:
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
@ 2019-06-15 13:38     ` Vignesh Raghavendra
  0 siblings, 0 replies; 19+ messages in thread
From: Vignesh Raghavendra @ 2019-06-15 13:38 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: Joakim Tjernlund, Chris Packham, linux-mtd, stable, Felix Fietkau



On 26-May-19 9:08 PM, Tokunori Ikegami wrote:
> 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.
> Also it seems that it is caused after repeated about 1,000 times to retry
> the write one word with the reset command.
> By using chip_good() to check the state to be done it can be reduced the
> retry with reset.
> It is depended on the actual flash chip behavior so the root cause is
> unknown.
> 
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>

I need sign of all co-developers before applying the patch.
Please run ./scripts/checkpatch.pl --strict on all patches and address
all the issues.

Regards
Vignesh

> Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org
> ---
> Changes since v5:
> - Rebased on top of Liu Jian's fixes in master.
> - Change to follow Liu Jian's fixes in master for the write buffer.
> - Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.
> 
> Changes since v4:
> - None.
> 
> Changes since v3:
> - Update the commit message for the comments.
> - Drop the addition of blanks lines around xip_enable().
> - Delete unnecessary setting the ret variable to -EIO.
> - Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.
> 
> Changes since v2:
> - Just update the commit message for the comment.
> 
> Changes since v1:
> - Just update the commit message.
> 
> Background:
> 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(-)
>  mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> old mode 100644
> new mode 100755
> index c8fa5906bdf9..348b54820e4c
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1628,29 +1628,35 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
>  			continue;
>  		}
>  
> -		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> +		/*
> +		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
> +		 * the failure due to scheduling.
> +		 */
> +		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)){
>  			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))
> +		if (chip_good(map, adr, datum))
>  			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);
>   op_done:
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 02/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()
  2019-05-26 15:38 ` [PATCH v6 02/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer() Tokunori Ikegami
@ 2019-06-15 13:42   ` Vignesh Raghavendra
  0 siblings, 0 replies; 19+ messages in thread
From: Vignesh Raghavendra @ 2019-06-15 13:42 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: Joakim Tjernlund, Chris Packham, linux-mtd, Fabio Bettoni



On 26-May-19 9:08 PM, Tokunori Ikegami wrote:
> For a maintainability by reducing the goto statement remove it from
> do_write_buffer().
> 
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Cc: Fabio Bettoni <fbettoni@gmail.com>
> Co: Hauke Mehrtens <hauke@hauke-m.de>
> Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> ---
[...]
>  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 348b54820e4c..ca41f47c00c1 100755
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1887,40 +1887,42 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
>  		 * We check "time_after" and "!chip_good" before checking "chip_good" to avoid
>  		 * the failure due to scheduling.
>  		 */
> -		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum))
> +		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) {
> +			ret = -EIO;
>  			break;
> +		}
>  
>  		if (chip_good(map, adr, datum)) {
>  			xip_enable(map, chip, adr);
> -			goto op_done;
> +			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);
> +	}
>  

While at that, could you convert this to pr_err()?

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 03/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer().
  2019-05-26 15:38 ` [PATCH v6 03/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer() Tokunori Ikegami
@ 2019-06-15 13:46   ` Vignesh Raghavendra
  0 siblings, 0 replies; 19+ messages in thread
From: Vignesh Raghavendra @ 2019-06-15 13:46 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: Joakim Tjernlund, Chris Packham, linux-mtd, Fabio Bettoni



On 26-May-19 9:08 PM, Tokunori Ikegami wrote:
> By the removed goto statement it can be called xip_enable() once.
> Also for a maintainability refactor it to call the function only once.
> 

Please squash this into previous patch.

Regards
Vignesh

> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Cc: Fabio Bettoni <fbettoni@gmail.com>
> Co: Hauke Mehrtens <hauke@hauke-m.de>
> Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> ---
> Changes since v5:
> - Rebased on top of Liu Jian's fixes in master.
> - Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.
> 
> Changes since v4:
> - None.
> 
> Changes since v3:
> - Just change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.
> 
> 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 ca41f47c00c1..2654019ee24b 100755
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1892,10 +1892,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
>  			break;
>  		}
>  
> -		if (chip_good(map, adr, datum)) {
> -			xip_enable(map, chip, adr);
> +		if (chip_good(map, adr, datum))
>  			break;
> -		}
>  
>  		/* Latency issues. Drop the lock, wait a while and retry */
>  		UDELAY(map, chip, adr, 1);
> @@ -1916,13 +1914,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);
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 07/11] mtd: cfi_cmdset_0002: Remove retry goto statement from do_write_oneword()
  2019-05-26 15:39 ` [PATCH v6 07/11] mtd: cfi_cmdset_0002: Remove retry " Tokunori Ikegami
@ 2019-06-15 14:52   ` Vignesh Raghavendra
  0 siblings, 0 replies; 19+ messages in thread
From: Vignesh Raghavendra @ 2019-06-15 14:52 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: Joakim Tjernlund, Chris Packham, linux-mtd, Fabio Bettoni



On 26-May-19 9:09 PM, Tokunori Ikegami wrote:
> This is just to refactor the function by removing the goto statement.
> Change to use the for loop instead of the goto statement.
> 

This "retry: ... goto retry;" construct is consistently used across the
file. Using goto to implement retry loops is common in kernel. Unless
there is a better reason, lets not do this change.

Regards
Vignesh

> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Cc: Fabio Bettoni <fbettoni@gmail.com>
> Co: Hauke Mehrtens <hauke@hauke-m.de>
> Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> ---
> Changes since v5:
> - Rebased on top of Liu Jian's fixes in master.
> - Change the email address of Tokunori Ikegami to ikegami.t@gmail.com.
> 
> Changes since v4:
> - None.
> 
> Changes since v3:
> - Rebased.
> - Change the email address of Tokunori Ikegami to ikegami_to@yahoo.co.jp.
> 
> Changes since v2:
> - None.
> 
> Changes since v1:
> - Add the patch.
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 597b0f18269f..7784be8246cb 100755
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1684,17 +1684,15 @@ 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);
> -	if (ret) {
> +	for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> +		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 */
>  
> -		if (++retry_cnt <= MAX_RETRIES) {
> -			ret = 0;
> -			goto retry;
> -		}
> +		/* FIXME - should have reset delay before continuing */
>  	}
>  	xip_enable(map, chip, adr);
>  
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 11/11] mtd: cfi_cmdset_0002: Disable write buffer functions if FORCE_WORD_WRITE is 1
  2019-05-26 15:39 ` [PATCH v6 11/11] mtd: cfi_cmdset_0002: Disable write buffer functions if FORCE_WORD_WRITE is 1 Tokunori Ikegami
@ 2019-06-18  8:05   ` Vignesh Raghavendra
  0 siblings, 0 replies; 19+ messages in thread
From: Vignesh Raghavendra @ 2019-06-18  8:05 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: Joakim Tjernlund, Chris Packham, linux-mtd, Fabio Bettoni



On 26/05/19 9:09 PM, Tokunori Ikegami wrote:
> Some write buffer functions are not used when FORCE_WORD_WRITE is set to 1.
> So the compile warning messages are output if FORCE_WORD_WRITE is 1.
> To resolve this disable the write buffer functions if FORCE_WORD_WRITE is 1.
> 
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Cc: Fabio Bettoni <fbettoni@gmail.com>
> Co: Hauke Mehrtens <hauke@hauke-m.de>
> Co: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> ---
> Changes since v5:
> - Add the patch.
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 477bc3d65e68..2144221029e9 100755

Your patch seems to be changing permissions here:
warning: drivers/mtd/chips/cfi_cmdset_0002.c has type 100644, expected
100755

Please fix this up.

> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c

[...]

> @@ -1836,6 +1840,7 @@ static int cfi_amdstd_write_words(struct mtd_info *mtd, loff_t to, size_t len,
>  	return 0;
>  }
>  > +#if !FORCE_WORD_WRITE
>  static int __xipram do_write_buffer_wait(struct map_info *map,
>  					 struct flchip *chip, unsigned long adr,
>  					 map_word datum)
> @@ -2064,6 +2069,7 @@ static int cfi_amdstd_write_buffers(struct mtd_info *mtd, loff_t to, size_t len,
>  
>  	return 0;
>  }
> +#endif /* !FORCE_WORD_WRITE */
>  

This should be part of the patch that introduces do_write_buffer_wait()

>  /*
>   * Wait for the flash chip to become ready to write data
> 

-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-06-18  8:05 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-26 15:38 [PATCH v6 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project Tokunori Ikegami
2019-05-26 15:38 ` [PATCH v6 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword() Tokunori Ikegami
2019-05-26 15:38   ` Tokunori Ikegami
2019-06-15 13:38   ` Vignesh Raghavendra
2019-06-15 13:38     ` Vignesh Raghavendra
2019-05-26 15:38 ` [PATCH v6 02/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer() Tokunori Ikegami
2019-06-15 13:42   ` Vignesh Raghavendra
2019-05-26 15:38 ` [PATCH v6 03/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer() Tokunori Ikegami
2019-06-15 13:46   ` Vignesh Raghavendra
2019-05-26 15:38 ` [PATCH v6 04/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size Tokunori Ikegami
2019-05-26 15:38 ` [PATCH v6 05/11] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement Tokunori Ikegami
2019-05-26 15:38 ` [PATCH v6 06/11] mtd: cfi_cmdset_0002: Remove op_done goto statement from do_write_oneword() Tokunori Ikegami
2019-05-26 15:39 ` [PATCH v6 07/11] mtd: cfi_cmdset_0002: Remove retry " Tokunori Ikegami
2019-06-15 14:52   ` Vignesh Raghavendra
2019-05-26 15:39 ` [PATCH v6 08/11] mtd: cfi_cmdset_0002: Split write-to-buffer-reset sequence Tokunori Ikegami
2019-05-26 15:39 ` [PATCH v6 09/11] mtd: cfi_cmdset_0002: Split to wait write buffer to check if completed Tokunori Ikegami
2019-05-26 15:39 ` [PATCH v6 10/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths Tokunori Ikegami
2019-05-26 15:39 ` [PATCH v6 11/11] mtd: cfi_cmdset_0002: Disable write buffer functions if FORCE_WORD_WRITE is 1 Tokunori Ikegami
2019-06-18  8:05   ` Vignesh Raghavendra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.