[PATCH v2 00/10] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

[PATCH v2 00/10] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

[Tokunori Ikegami]

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@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org

Tokunori Ikegami (10):
  mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()
  mtd: cfi_cmdset_0002: Remove chip_ready() from do_write_buffer()
  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

 drivers/mtd/chips/cfi_cmdset_0002.c | 253 ++++++++++++++++------------
 1 file changed, 146 insertions(+), 107 deletions(-)

-- 
2.18.0

[PATCH v2 01/10] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[Tokunori Ikegami]

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>

So change to use chip_good() instead of chip_ready().

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().

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Signed-off-by: Fabio Bettoni <fbettoni@gmail.com>
Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>
Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Co-Developed-by: Fabio Bettoni <fbettoni@gmail.com>
Reported-by: Fabio Bettoni <fbettoni@gmail.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
Changes since v1:
- Just update the commit message.

 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);
+
  op_done:
 	if (mode == FL_OTP_WRITE)
 		otp_exit(map, chip, adr, map_bankwidth(map));
-- 
2.18.0

Re: [PATCH v2 01/10] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[Chris Packham]

On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> 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>
> 
> So change to use chip_good() instead of chip_ready().
> 
> 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().

I think much of this is commentary for readers of the mailing list not 
something that should be in the final commit (i.e. it should be after 
the '---'). The commit message should really reflect what the problem 
was and how this change fixes it.

I think you've got a better handle on what chip_good() does than I do so 
I won't attempt to provide a commit message here.

> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Signed-off-by: Fabio Bettoni <fbettoni@gmail.com>
> Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>
> Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> Co-Developed-by: Fabio Bettoni <fbettoni@gmail.com>
> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org
> ---
> Changes since v1:
> - Just update the commit message.
> 
>   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);
> +
>    op_done:
>   	if (mode == FL_OTP_WRITE)
>   		otp_exit(map, chip, adr, map_bankwidth(map));
> 

RE: [PATCH v2 01/10] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[IKEGAMI Tokunori]

> -----Original Message-----
> From: stable-owner@vger.kernel.org
> [mailto:stable-owner@vger.kernel.org] On Behalf Of Chris Packham
> Sent: Thursday, October 25, 2018 6:24 AM
> To: IKEGAMI Tokunori; boris.brezillon@free-electrons.com
> Cc: Hauke Mehrtens; Koen Vandeputte; Fabio Bettoni; Joakim Tjernlund;
> linux-mtd@lists.infradead.org; stable@vger.kernel.org
> Subject: Re: [PATCH v2 01/10] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> > 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=ddc11c3
> 932c7b7b7df7d5fbd48f207e77619eaa7>
> >
> > 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>
> >
> > So change to use chip_good() instead of chip_ready().
> >
> > 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().
> 
> I think much of this is commentary for readers of the mailing list not
> something that should be in the final commit (i.e. it should be after
> the '---'). The commit message should really reflect what the problem
> was and how this change fixes it.

[Ikegami] Yes fixed as so.

> 
> I think you've got a better handle on what chip_good() does than I do so
> I won't attempt to provide a commit message here.
> 
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> > Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> > Signed-off-by: Fabio Bettoni <fbettoni@gmail.com>
> > Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>
> > Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> > Co-Developed-by: Fabio Bettoni <fbettoni@gmail.com>
> > Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: linux-mtd@lists.infradead.org
> > Cc: stable@vger.kernel.org
> > ---
> > Changes since v1:
> > - Just update the commit message.
> >
> >   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);
> > +
> >    op_done:
> >   	if (mode == FL_OTP_WRITE)
> >   		otp_exit(map, chip, adr, map_bankwidth(map));
> >

[PATCH v2 02/10] mtd: cfi_cmdset_0002: Remove chip_ready() from do_write_buffer()

[Tokunori Ikegami]

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 <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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

Re: [PATCH v2 02/10] mtd: cfi_cmdset_0002: Remove chip_ready() from do_write_buffer()

[Chris Packham]

On 20/10/18 5:55 AM, Tokunori Ikegami wrote:

> 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.

s/chagne/change/

> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> 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: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: linux-mtd@lists.infradead.org
> ---
> 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);
>   	}
> 

[PATCH v2 03/10] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()

[Tokunori Ikegami]

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

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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

[PATCH v2 04/10] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer().

[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.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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

[PATCH v2 05/10] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size

[Tokunori Ikegami]

The function is longer so add do_write_oneword_once() to split.
This is also for to reduce goto statement by additional patch.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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

Re: [PATCH v2 05/10] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size

[Chris Packham]

Hi Ikegami-san,

On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> The function is longer so add do_write_oneword_once() to split.
> This is also for to reduce goto statement by additional patch.

I think the following would be easier to understand.

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

With that change

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> 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: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: linux-mtd@lists.infradead.org
> ---
> 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)

No particular objections to the name (i.e. don't change it unless the 
maintainer insists) but often these kinds of helpers are just prefixed 
with '_' (e.g. _do_write_oneword).

>   {
> -	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. */
> 

RE: [PATCH v2 05/10] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce function size

[IKEGAMI Tokunori]

Hi Chris-san,

Thank you so much for your reviewing and comments.

Regards,
Ikegami

> -----Original Message-----
> From: Chris Packham [mailto:Chris.Packham@alliedtelesis.co.nz]
> Sent: Thursday, October 25, 2018 6:11 AM
> To: IKEGAMI Tokunori; boris.brezillon@free-electrons.com
> Cc: Fabio Bettoni; Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH v2 05/10] mtd: cfi_cmdset_0002: Split
> do_write_oneword() to reduce function size
> 
> Hi Ikegami-san,
> 
> On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> > The function is longer so add do_write_oneword_once() to split.
> > This is also for to reduce goto statement by additional patch.
> 
> I think the following would be easier to understand.
> 
> "Reduce the size of do_write_oneword() by extracting a helper function
> for the hardware access."

[Ikegami] Just fixed as this.

> 
> With that change
> 
> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

[Ikegami] Added the Reviewed-by tag.

> 
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > 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: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: linux-mtd@lists.infradead.org
> > ---
> > 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)
> 
> No particular objections to the name (i.e. don't change it unless the
> maintainer insists) but often these kinds of helpers are just prefixed
> with '_' (e.g. _do_write_oneword).

[Ikegami] I see about this.

> 
> >   {
> > -	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. */
> >

[PATCH v2 06/10] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement

[Tokunori Ikegami]

To reduce function size and remove the goto statement split the op_done goto
statement part into do_write_oneword_done() created a function.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ae2d8bd7154e..2340a5a7cc39 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1609,6 +1609,20 @@ static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *c
 	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)
@@ -1670,12 +1684,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

Re: [PATCH v2 06/10] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement

[Chris Packham]

On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> To reduce function size and remove the goto statement split the op_done goto
> statement part into do_write_oneword_done() created a function.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> 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: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: linux-mtd@lists.infradead.org
> ---
> Changes since v1:
> - Add the patch.
> 
>   drivers/mtd/chips/cfi_cmdset_0002.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index ae2d8bd7154e..2340a5a7cc39 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1609,6 +1609,20 @@ static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *c
>   	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);

Personally I find it easier if the mutex_lock()/mutex_unlock() are in 
the same function. That way it's easier to audit the exit paths from a 
single function rather than needing to know that some functions 
implicitly release the lock. The same probably applies to 
get_chip/put_chip pairs.

> +}
> +
>   static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
>   				     unsigned long adr, map_word datum,
>   				     int mode)
> @@ -1670,12 +1684,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;
>   }
> 

RE: [PATCH v2 06/10] mtd: cfi_cmdset_0002: Split do_write_oneword() op_done goto statement

[IKEGAMI Tokunori]

> -----Original Message-----
> From: Chris Packham [mailto:Chris.Packham@alliedtelesis.co.nz]
> Sent: Thursday, October 25, 2018 6:34 AM
> To: IKEGAMI Tokunori; boris.brezillon@free-electrons.com
> Cc: Fabio Bettoni; Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH v2 06/10] mtd: cfi_cmdset_0002: Split
> do_write_oneword() op_done goto statement
> 
> On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> > To reduce function size and remove the goto statement split the op_done
> goto
> > statement part into do_write_oneword_done() created a function.
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > 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: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: linux-mtd@lists.infradead.org
> > ---
> > Changes since v1:
> > - Add the patch.
> >
> >   drivers/mtd/chips/cfi_cmdset_0002.c | 21 +++++++++++++++------
> >   1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index ae2d8bd7154e..2340a5a7cc39 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1609,6 +1609,20 @@ static int __xipram do_write_oneword_once(struct
> map_info *map, struct flchip *c
> >   	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);
> 
> Personally I find it easier if the mutex_lock()/mutex_unlock() are in
> the same function. That way it's easier to audit the exit paths from a
> single function rather than needing to know that some functions
> implicitly release the lock. The same probably applies to
> get_chip/put_chip pairs.

[Ikegami] Thank you. I could understand well. Fixed but still remain the done function.
But added the start function to find easier the pairs instead of not change to split as the done function.
Also for the exit paths to find easier added the patch 11 additionally.

> 
> > +}
> > +
> >   static int __xipram do_write_oneword(struct map_info *map, struct
> flchip *chip,
> >   				     unsigned long adr, map_word datum,
> >   				     int mode)
> > @@ -1670,12 +1684,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;
> >   }
> >

[PATCH v2 07/10] mtd: cfi_cmdset_0002: Remove op_done goto statement from do_write_oneword()

[Tokunori Ikegami]

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

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 2340a5a7cc39..e97b1f390fc8 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1641,8 +1641,8 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 		return ret;
 	}
 
-	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
-		 __func__, adr, datum.x[0]);
+	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));
@@ -1655,9 +1655,9 @@ 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;
+		pr_debug("MTD %s(): NOP\n", __func__);
+		do_write_oneword_done(map, chip, adr, mode);
+		return ret;
 	}
 
 	XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
@@ -1683,7 +1683,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

[PATCH v2 08/10] mtd: cfi_cmdset_0002: Remove retry goto statement from do_write_oneword()

[Tokunori Ikegami]

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@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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 e97b1f390fc8..b8f94f47d07c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1664,21 +1664,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

[PATCH v2 09/10] mtd: cfi_cmdset_0002: Split write-to-buffer-reset sequence

[Tokunori Ikegami]

Just refactor to split the sequence from do_write_buffer().

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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 b8f94f47d07c..e45b906c5ea7 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1808,6 +1808,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!
@@ -1910,22 +1931,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

[PATCH v2 10/10] mtd: cfi_cmdset_0002: Split to wait write buffer to check if completed

[Tokunori Ikegami]

Just refactor to split the wait from do_write_buffer().

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: linux-mtd@lists.infradead.org
---
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 e45b906c5ea7..0b8efb60a3f1 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1808,6 +1808,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)
@@ -1838,13 +1883,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;
@@ -1901,34 +1939,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