[PATCH 1/3] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[PATCH 1/3] 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>

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

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: 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
---
 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

[PATCH 2/3] 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>
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
---
 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

[PATCH 3/3] mtd: cfi_cmdset_0002: Remove goto statemnet from do_write_buffer()

[Tokunori Ikegami]

Also change to call xip_enable() once only.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: 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
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 48 +++++++++++++++--------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c2e51768a02c..060edd2f1693 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1883,39 +1883,41 @@ 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);
+		/* 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);
+	}
+
+	xip_enable(map, chip, adr);
 
-	ret = -EIO;
- op_done:
 	chip->state = FL_READY;
 	DISABLE_VPP(map);
 	put_chip(map, chip, adr);
-- 
2.18.0

Re: [PATCH 0/3] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

[Boris Brezillon]

+mtd ML and all people CC-ed on other patches

On Fri, 19 Oct 2018 17:13:17 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> 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.
> 
> Tokunori Ikegami (3):
>   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 statemnet from do_write_buffer()

I see that Fabio was the original author of the OpenWRT patch. We
usually keep authorship when porting patching from one project to
another, so I just want to check with Fabio if he's okay to handover
this authorship to you.

> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 68 ++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 

Re: [PATCH 1/3] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[Boris Brezillon]

On Fri, 19 Oct 2018 17:13:18 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> 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>

This part is not really explaining why this fix is needed. Can you
please give more details about why chip_good() should be used instead of
chip_ready().

> 
> So change to use chip_good() instead of chip_ready().
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: 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

Would be great to have Fixes and Cc-stable tags here.

> ---
>  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 2/3] mtd: cfi_cmdset_0002: Remove chip_ready() from do_write_buffer()

[Boris Brezillon]

On Fri, 19 Oct 2018 17:13:19 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> 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.

								^ change

And again, you're not explaining why.

> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: 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
> ---
>  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);
>  	}

Re: [PATCH 3/3] mtd: cfi_cmdset_0002: Remove goto statemnet from do_write_buffer()

[Boris Brezillon]

In the subject: s/statemnet/statement/

On Fri, 19 Oct 2018 17:13:20 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> Also change to call xip_enable() once only.

Just like for the other patches I'd like to have more explanation in
the commit message. And please split the commit in 2 commits since you
seem to do 2 unrelated changes:
- drop a label+gotos
- call xip_enable() only once.

> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: 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
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 48 +++++++++++++++--------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index c2e51768a02c..060edd2f1693 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1883,39 +1883,41 @@ 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);
> +		/* 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);
> +	}
> +
> +	xip_enable(map, chip, adr);
>  
> -	ret = -EIO;
> - op_done:
>  	chip->state = FL_READY;
>  	DISABLE_VPP(map);
>  	put_chip(map, chip, adr);

RE: [PATCH 0/3] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

[IKEGAMI Tokunori]

Sorry let me resend the mail as text mail format since error returned.

From: IKEGAMI Tokunori 
Sent: Friday, October 19, 2018 6:20 PM
To: 'FabioB'; boris.brezillon@bootlin.com; 'Hauke Mehrtens'; 'Koen Vandeputte'
Cc: linux-mtd@lists.infradead.org; PACKHAM Chris; Joakim.Tjernlund@infinera.com
Subject: RE: [PATCH 0/3] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

Thank you so much for your comments.

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 I will do add Hauke-san and Koen-san also in this patch mail.

Hauke-san and Koen-san,

　Can you also agree to fix the above OpenWRT patch in the upstream by me?
　Of course if you like please do it by yourself but I think that you are very busy so I will do this if okay.

By the way how can I add the originator name in the commit message with tag?
So I have just checked the Documentation and I think that the Co-Developed-by tag can be used for the originator name.
Is this correct?
Anyway I will do update the patches.


From: FabioB [mailto:fbettoni@gmail.com] 
Sent: Friday, October 19, 2018 5:37 PM
To: boris.brezillon@bootlin.com
Cc: IKEGAMI Tokunori; linux-mtd@lists.infradead.org; PACKHAM Chris; Joakim.Tjernlund@infinera.com
Subject: Re: [PATCH 0/3] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

Hi Boris,
sure it's OK to handover autorship to Tokunori,
I wrote to him to ask for help and he told me to restore removed patch.
So I only had to edit by hand because patching phase was not working during build.
Tokunori must have all authorship and credits to the solution.

Fabio

Il giorno ven 19 ott 2018 alle ore 10:27 Boris Brezillon <boris.brezillon@bootlin.com> ha scritto:
+mtd ML and all people CC-ed on other patches

On Fri, 19 Oct 2018 17:13:17 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> 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.
> 
> Tokunori Ikegami (3):
>   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 statemnet from do_write_buffer()

I see that Fabio was the original author of the OpenWRT patch. We
usually keep authorship when porting patching from one project to
another, so I just want to check with Fabio if he's okay to handover
this authorship to you.

> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 68 ++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 

RE: [PATCH 1/3] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[IKEGAMI Tokunori]

Let me comment below in the mail.

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Friday, October 19, 2018 5:30 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH 1/3] mtd: cfi_cmdset_0002: Change do_write_oneword()
> to use chip_good()
> 
> On Fri, 19 Oct 2018 17:13:18 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> 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>
> 
> This part is not really explaining why this fix is needed. Can you
> please give more details about why chip_good() should be used instead of
> chip_ready().

[Ikegami] The reason is that just actually fix the issue.
And also in the past I 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..

Hauke-san and Koen-san,

  If you have any advice please let me know.


> 
> >
> > So change to use chip_good() instead of chip_ready().
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Cc: 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
> 
> Would be great to have Fixes and Cc-stable tags here.

[Ikegami] I see I will do that.

> 
> > ---
> >  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 3/3] mtd: cfi_cmdset_0002: Remove goto statemnet from do_write_buffer()

[IKEGAMI Tokunori]

Thank you so much for your comments.

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Friday, October 19, 2018 5:33 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH 3/3] mtd: cfi_cmdset_0002: Remove goto statemnet from
> do_write_buffer()
> 
> In the subject: s/statemnet/statement/

[Ikegami] Will do fix this sorry.

> 
> On Fri, 19 Oct 2018 17:13:20 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > Also change to call xip_enable() once only.
> 
> Just like for the other patches I'd like to have more explanation in
> the commit message. And please split the commit in 2 commits since you
> seem to do 2 unrelated changes:
> - drop a label+gotos
> - call xip_enable() only once.

[Ikegami] Yes will do I think so.
Also I will do add additional refactoring patches to remove goto statement more and reduce the longer functions lines later.

> 
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Cc: 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
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 48
> +++++++++++++++--------------
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index c2e51768a02c..060edd2f1693 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1883,39 +1883,41 @@ 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_Bu
> ffer_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_Bu
> ffer_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 */
> >
> > -	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);
> > +	}
> > +
> > +	xip_enable(map, chip, adr);
> >
> > -	ret = -EIO;
> > - op_done:
> >  	chip->state = FL_READY;
> >  	DISABLE_VPP(map);
> >  	put_chip(map, chip, adr);