[PATCH v3 00/11] mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project

[PATCH v3 00/11] 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 (11):
  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
  mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths

 drivers/mtd/chips/cfi_cmdset_0002.c | 277 +++++++++++++++++-----------
 1 file changed, 171 insertions(+), 106 deletions(-)

-- 
2.18.0

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

[Tokunori Ikegami]

In OpenWrt Project the flash write error caused on some products.
Also the issue can be fixed by using chip_good() instead of chip_ready().
The chip_ready() just checks the value from flash memory twice.
And the chip_good() checks the value with the expected value.
Probably the issue can be fixed as checked correctly by the chip_good().
So change to use chip_good() instead of chip_ready().

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

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 v3 01/11] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[Boris Brezillon]

On Fri, 26 Oct 2018 01:32:09 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> In OpenWrt Project the flash write error caused on some products.

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

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

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

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

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

Anyway, this is the sort of clarification I'd like to have.

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

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

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

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

> 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 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(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 72428b6bfc47..251c9e1675bd 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1627,31 +1627,37 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
>  			continue;
>  		}
>  
> -		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> +		if (chip_good(map, adr, datum))
> +			break;
> +
> +		if (time_after(jiffies, timeo)){
>  			xip_enable(map, chip, adr);
>  			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
>  			xip_disable(map, chip, adr);
> +			ret = -EIO;
>  			break;
>  		}
>  
> -		if (chip_ready(map, adr))
> -			break;
> -
>  		/* Latency issues. Drop the lock, wait a while and retry */
>  		UDELAY(map, chip, adr, 1);
>  	}
> +
>  	/* Did we succeed? */
> -	if (!chip_good(map, adr, datum)) {
> +	if (ret) {
>  		/* reset on all failures. */
>  		map_write(map, CMD(0xF0), chip->start);
>  		/* FIXME - should have reset delay before continuing */
>  
> -		if (++retry_cnt <= MAX_RETRIES)
> +		if (++retry_cnt <= MAX_RETRIES) {
> +			ret = 0;
>  			goto retry;
> +		}
>  
>  		ret = -EIO;
>  	}
> +
>  	xip_enable(map, chip, adr);
> +

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

>   op_done:
>  	if (mode == FL_OTP_WRITE)
>  		otp_exit(map, chip, adr, map_bankwidth(map));

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

[Joakim Tjernlund]

On Mon, 2018-11-05 at 11:15 +0100, Boris Brezillon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Fri, 26 Oct 2018 01:32:09 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > In OpenWrt Project the flash write error caused on some products.
> 
> It's okay to mention that the issue was discovered by the OpenWRT team,
> but I'd rephrase it differently.
> 
> "As reported by the OpenWRT team, write requests sometimes fail on some
> platforms".
> 
> > Also the issue can be fixed by using chip_good() instead of chip_ready().
> > The chip_ready() just checks the value from flash memory twice.
> > And the chip_good() checks the value with the expected value.
> > Probably the issue can be fixed as checked correctly by the chip_good().
> > So change to use chip_good() instead of chip_ready().
> 
> Well, that's not really explaining why you think chip_good() should be
> used instead of chip_ready(). So I went on and looked at the
> chip_good(), chip_ready() and do_write_oneword() implementation, and
> also looked at users of do_write_oneword(). It seems this function is
> used to write data to the flash, and apparently the "one bit should
> toggle to reflect a busy state" does not apply when writing things to
> the memory array (it's probably working for other CFI commands, but I
> guess it takes more time to actually change the level of a NOR cell,
> hence the result of 2 identical reads does not mean that the write is
> done).
> 
> Also, it seems that cmdset_0001 is not implementing chip_ready() the
> same way, and I wonder if cmdset_0002 implementation is correct to
> start with. Or maybe I don't get what chip_ready() is for.
> 
The 0001 cmd set is quite different to 0002 and 0001 is the superior one.
If you look at recent 0002 cmd sets they offer an alternative cmd
set to replace the all the "toggle" ones with something that is
same/similar to what 0001 offers.

 Jocke

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

[Boris Brezillon]

On Mon, 5 Nov 2018 12:03:04 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Mon, 2018-11-05 at 11:15 +0100, Boris Brezillon wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Fri, 26 Oct 2018 01:32:09 +0900
> > Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> >   
> > > In OpenWrt Project the flash write error caused on some products.  
> > 
> > It's okay to mention that the issue was discovered by the OpenWRT team,
> > but I'd rephrase it differently.
> > 
> > "As reported by the OpenWRT team, write requests sometimes fail on some
> > platforms".
> >   
> > > Also the issue can be fixed by using chip_good() instead of chip_ready().
> > > The chip_ready() just checks the value from flash memory twice.
> > > And the chip_good() checks the value with the expected value.
> > > Probably the issue can be fixed as checked correctly by the chip_good().
> > > So change to use chip_good() instead of chip_ready().  
> > 
> > Well, that's not really explaining why you think chip_good() should be
> > used instead of chip_ready(). So I went on and looked at the
> > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > also looked at users of do_write_oneword(). It seems this function is
> > used to write data to the flash, and apparently the "one bit should
> > toggle to reflect a busy state" does not apply when writing things to
> > the memory array (it's probably working for other CFI commands, but I
> > guess it takes more time to actually change the level of a NOR cell,
> > hence the result of 2 identical reads does not mean that the write is
> > done).
> > 
> > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > same way, and I wonder if cmdset_0002 implementation is correct to
> > start with. Or maybe I don't get what chip_ready() is for.
> >   
> The 0001 cmd set is quite different to 0002 and 0001 is the superior one.
> If you look at recent 0002 cmd sets they offer an alternative cmd
> set to replace the all the "toggle" ones with something that is
> same/similar to what 0001 offers.

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

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

[Joakim Tjernlund]

On Mon, 2018-11-05 at 13:52 +0100, Boris Brezillon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Mon, 5 Nov 2018 12:03:04 +0000
> Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> 
> > On Mon, 2018-11-05 at 11:15 +0100, Boris Brezillon wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > 
> > > 
> > > On Fri, 26 Oct 2018 01:32:09 +0900
> > > Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> > > 
> > > > In OpenWrt Project the flash write error caused on some products.
> > > 
> > > It's okay to mention that the issue was discovered by the OpenWRT team,
> > > but I'd rephrase it differently.
> > > 
> > > "As reported by the OpenWRT team, write requests sometimes fail on some
> > > platforms".
> > > 
> > > > Also the issue can be fixed by using chip_good() instead of chip_ready().
> > > > The chip_ready() just checks the value from flash memory twice.
> > > > And the chip_good() checks the value with the expected value.
> > > > Probably the issue can be fixed as checked correctly by the chip_good().
> > > > So change to use chip_good() instead of chip_ready().
> > > 
> > > Well, that's not really explaining why you think chip_good() should be
> > > used instead of chip_ready(). So I went on and looked at the
> > > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > > also looked at users of do_write_oneword(). It seems this function is
> > > used to write data to the flash, and apparently the "one bit should
> > > toggle to reflect a busy state" does not apply when writing things to
> > > the memory array (it's probably working for other CFI commands, but I
> > > guess it takes more time to actually change the level of a NOR cell,
> > > hence the result of 2 identical reads does not mean that the write is
> > > done).
> > > 
> > > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > > same way, and I wonder if cmdset_0002 implementation is correct to
> > > start with. Or maybe I don't get what chip_ready() is for.
> > > 
> > The 0001 cmd set is quite different to 0002 and 0001 is the superior one.
> > If you look at recent 0002 cmd sets they offer an alternative cmd
> > set to replace the all the "toggle" ones with something that is
> > same/similar to what 0001 offers.
> 
> Okay. Do you know when chip_ready() (the one that checks if something
> changes between 2 reads) should be used and when it shouldn't?

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

 Jocke

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

[Boris Brezillon]

Hi Joakim,

On Mon, 5 Nov 2018 13:22:19 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Mon, 2018-11-05 at 13:52 +0100, Boris Brezillon wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Mon, 5 Nov 2018 12:03:04 +0000
> > Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> >   
> > > On Mon, 2018-11-05 at 11:15 +0100, Boris Brezillon wrote:  
> > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > 
> > > > 
> > > > On Fri, 26 Oct 2018 01:32:09 +0900
> > > > Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> > > >   
> > > > > In OpenWrt Project the flash write error caused on some products.  
> > > > 
> > > > It's okay to mention that the issue was discovered by the OpenWRT team,
> > > > but I'd rephrase it differently.
> > > > 
> > > > "As reported by the OpenWRT team, write requests sometimes fail on some
> > > > platforms".
> > > >   
> > > > > Also the issue can be fixed by using chip_good() instead of chip_ready().
> > > > > The chip_ready() just checks the value from flash memory twice.
> > > > > And the chip_good() checks the value with the expected value.
> > > > > Probably the issue can be fixed as checked correctly by the chip_good().
> > > > > So change to use chip_good() instead of chip_ready().  
> > > > 
> > > > Well, that's not really explaining why you think chip_good() should be
> > > > used instead of chip_ready(). So I went on and looked at the
> > > > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > > > also looked at users of do_write_oneword(). It seems this function is
> > > > used to write data to the flash, and apparently the "one bit should
> > > > toggle to reflect a busy state" does not apply when writing things to
> > > > the memory array (it's probably working for other CFI commands, but I
> > > > guess it takes more time to actually change the level of a NOR cell,
> > > > hence the result of 2 identical reads does not mean that the write is
> > > > done).
> > > > 
> > > > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > > > same way, and I wonder if cmdset_0002 implementation is correct to
> > > > start with. Or maybe I don't get what chip_ready() is for.
> > > >   
> > > The 0001 cmd set is quite different to 0002 and 0001 is the superior one.
> > > If you look at recent 0002 cmd sets they offer an alternative cmd
> > > set to replace the all the "toggle" ones with something that is
> > > same/similar to what 0001 offers.  
> > 
> > Okay. Do you know when chip_ready() (the one that checks if something
> > changes between 2 reads) should be used and when it shouldn't?  
> 
> It is next to impossible to do proper error handling(analysing status) with
> toggle method, especially when you have interleaved chips.

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

> Try with erase suspend when something goes wrong and you want
> to address that properly.

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

> Best is to add support for the extended 0002 cmd set and use that
> whenever possible.

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

Thanks,

Boris

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

[IKEGAMI Tokunori]

Thank you so much for your reviewing.

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Monday, November 05, 2018 7:16 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Hauke Mehrtens;
> stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> On Fri, 26 Oct 2018 01:32:09 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > In OpenWrt Project the flash write error caused on some products.
> 
> It's okay to mention that the issue was discovered by the OpenWRT team,
> but I'd rephrase it differently.
> 
> "As reported by the OpenWRT team, write requests sometimes fail on some
> platforms".

Yes I will do fix as this.

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

I am thinking to update the commit message as below.

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

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

If any comment please let me know.

> 
> >
> > 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>
> 
> Has the patch really gone through all those people? SoB is used when you
> apply a patch in your tree or when you're the original author.

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

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

Yes so I am thinking to change as below.

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

If any problem let me know.

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

> 
> > 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 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=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>
> >
> > The reason to use chip_good() is that just actually fix the issue.
> > And also in the past I had fixed the erase function also as same way by
> the
> > patch below.
> >   <https://patchwork.ozlabs.org/patch/922656/>
> >     Note: The reason for the patch for erase is same.
> >
> > In my understanding the chip_ready() is just checked the value twice from
> > flash.
> > So I think that sometimes incorrect value is read twice and it is depended
> > on the flash device behavior but not sure..
> >
> > So change to use chip_good() instead of chip_ready().
> >
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 72428b6bfc47..251c9e1675bd 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1627,31 +1627,37 @@ static int __xipram do_write_oneword(struct
> map_info *map, struct flchip *chip,
> >  			continue;
> >  		}
> >
> > -		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> > +		if (chip_good(map, adr, datum))
> > +			break;
> > +
> > +		if (time_after(jiffies, timeo)){
> >  			xip_enable(map, chip, adr);
> >  			printk(KERN_WARNING "MTD %s(): software
> timeout\n", __func__);
> >  			xip_disable(map, chip, adr);
> > +			ret = -EIO;
> >  			break;
> >  		}
> >
> > -		if (chip_ready(map, adr))
> > -			break;
> > -
> >  		/* Latency issues. Drop the lock, wait a while and retry
> */
> >  		UDELAY(map, chip, adr, 1);
> >  	}
> > +
> >  	/* Did we succeed? */
> > -	if (!chip_good(map, adr, datum)) {
> > +	if (ret) {
> >  		/* reset on all failures. */
> >  		map_write(map, CMD(0xF0), chip->start);
> >  		/* FIXME - should have reset delay before continuing */
> >
> > -		if (++retry_cnt <= MAX_RETRIES)
> > +		if (++retry_cnt <= MAX_RETRIES) {
> > +			ret = 0;
> >  			goto retry;
> > +		}
> >
> >  		ret = -EIO;
> >  	}
> > +
> >  	xip_enable(map, chip, adr);
> > +
> 
> Not a big deal, but I'd prefer to not have coding style changes mixed
> with functional changes (in other words, you can drop the addition of
> blanks lines around xip_enable()).

Yes I will do fix this.

Regards,
Ikegami

> 
> >   op_done:
> >  	if (mode == FL_OTP_WRITE)
> >  		otp_exit(map, chip, adr, map_bankwidth(map));

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

[Boris Brezillon]

Hi IKEGAMI,

On Tue, 6 Nov 2018 00:25:43 +0000
IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:

> > > Also the issue can be fixed by using chip_good() instead of chip_ready().
> > > The chip_ready() just checks the value from flash memory twice.
> > > And the chip_good() checks the value with the expected value.
> > > Probably the issue can be fixed as checked correctly by the chip_good().
> > > So change to use chip_good() instead of chip_ready().  
> > 
> > Well, that's not really explaining why you think chip_good() should be
> > used instead of chip_ready(). So I went on and looked at the
> > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > also looked at users of do_write_oneword(). It seems this function is
> > used to write data to the flash, and apparently the "one bit should
> > toggle to reflect a busy state" does not apply when writing things to
> > the memory array (it's probably working for other CFI commands, but I
> > guess it takes more time to actually change the level of a NOR cell,
> > hence the result of 2 identical reads does not mean that the write is
> > done).
> > 
> > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > same way, and I wonder if cmdset_0002 implementation is correct to
> > start with. Or maybe I don't get what chip_ready() is for.
> > 
> > Anyway, this is the sort of clarification I'd like to have.  
> 
> I am thinking to update the commit message as below.
> 
>     mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
> 
>     As reported by the OpenWRT team, write requests sometimes fail on some
>     platforms.
>     Currently to check the state chip_ready() is used correctly as described by
>     the flash memory S29GL256P11TFI01 datasheet.

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

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

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

>     It is depended on the actual flash chip behavior so the root cause is
>     unknown.

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

> 
> If any comment please let me know.
> 
> >   
> > >
> > > 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>  
> > 
> > Has the patch really gone through all those people? SoB is used when you
> > apply a patch in your tree or when you're the original author.  
> 
> I have just checked the OpenWRT git log again and it looks that it was originally
> implemented by Felix Fietkau <nbd@openwrt.org> by the patch below so I will update the Signed-off-by tag as so.
>   <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640f07cd2b3b14fe9ec03fa63a586452cc5f>
> 
> >   
> > > 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>  
> > 
> > Not sure we want to add new undocumented tags, but you can mention
> > that all those people helped you find/debug the issue. They can also
> > add their Reviewed-by/Tested-by if they like.

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

Regards,

Boris

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

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

[IKEGAMI Tokunori]

Hi Boris-san,

> -----Original Message-----
> From: stable-owner@vger.kernel.org
> [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> Sent: Tuesday, November 06, 2018 5:34 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> Hi IKEGAMI,
> 
> On Tue, 6 Nov 2018 00:25:43 +0000
> IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> 
> > > > Also the issue can be fixed by using chip_good() instead of
> chip_ready().
> > > > The chip_ready() just checks the value from flash memory twice.
> > > > And the chip_good() checks the value with the expected value.
> > > > Probably the issue can be fixed as checked correctly by the chip_good().
> > > > So change to use chip_good() instead of chip_ready().
> > >
> > > Well, that's not really explaining why you think chip_good() should
> be
> > > used instead of chip_ready(). So I went on and looked at the
> > > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > > also looked at users of do_write_oneword(). It seems this function is
> > > used to write data to the flash, and apparently the "one bit should
> > > toggle to reflect a busy state" does not apply when writing things to
> > > the memory array (it's probably working for other CFI commands, but
> I
> > > guess it takes more time to actually change the level of a NOR cell,
> > > hence the result of 2 identical reads does not mean that the write is
> > > done).
> > >
> > > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > > same way, and I wonder if cmdset_0002 implementation is correct to
> > > start with. Or maybe I don't get what chip_ready() is for.
> > >
> > > Anyway, this is the sort of clarification I'd like to have.
> >
> > I am thinking to update the commit message as below.
> >
> >     mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
> >
> >     As reported by the OpenWRT team, write requests sometimes fail on
> some
> >     platforms.
> >     Currently to check the state chip_ready() is used correctly as
> described by
> >     the flash memory S29GL256P11TFI01 datasheet.
> 
> I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> it's using cmdset 0001.

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

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

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

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

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

> 
> >     Also chip_good() is used to check if the write is succeeded and it
> was
> >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve
> error
> >     checking").
> >     But actually the write failure is caused on some platforms and also
> it can
> >     be fixed by using chip_good() to check the state and retry instead.
> 
> Do you know on which NOR chips this happens? Do you have access to the
> datasheet?

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

+#define SST49LF008A            0x005a

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

> 
> >     It is depended on the actual flash chip behavior so the root cause
> is
> >     unknown.
> 
> Yes, and that's what I'd like you to figure out, or at least have a
> good idea why this doesn't work on some chips but works on others.

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

> 
> >
> > If any comment please let me know.
> >
> > >
> > > >
> > > > 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>
> > >
> > > Has the patch really gone through all those people? SoB is used when
> you
> > > apply a patch in your tree or when you're the original author.
> >
> > I have just checked the OpenWRT git log again and it looks that it was
> originally
> > implemented by Felix Fietkau <nbd@openwrt.org> by the patch below so I
> will update the Signed-off-by tag as so.
> >
> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640
> f07cd2b3b14fe9ec03fa63a586452cc5f>
> >
> > >
> > > > 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>
> > >
> > > Not sure we want to add new undocumented tags, but you can mention
> > > that all those people helped you find/debug the issue. They can also
> > > add their Reviewed-by/Tested-by if they like.
> 
> My bad, I just noticed these are valid flags [2], so you can keep them,
> and according to the doc, you should also keep the SoB.

I see.
Yes I had also checked it.

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

Regards,
Ikegami

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

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

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

[IKEGAMI Tokunori]

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

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

Hi Boris-san,

> -----Original Message-----
> From: stable-owner@vger.kernel.org
> [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> Sent: Tuesday, November 06, 2018 5:34 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
>
> Hi IKEGAMI,
>
> On Tue, 6 Nov 2018 00:25:43 +0000
> IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
>
> > > > Also the issue can be fixed by using chip_good() instead of
> chip_ready().
> > > > The chip_ready() just checks the value from flash memory twice.
> > > > And the chip_good() checks the value with the expected value.
> > > > Probably the issue can be fixed as checked correctly by the
chip_good().
> > > > So change to use chip_good() instead of chip_ready().
> > >
> > > Well, that's not really explaining why you think chip_good() should
> be
> > > used instead of chip_ready(). So I went on and looked at the
> > > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > > also looked at users of do_write_oneword(). It seems this function
is
> > > used to write data to the flash, and apparently the "one bit should
> > > toggle to reflect a busy state" does not apply when writing things
to
> > > the memory array (it's probably working for other CFI commands, but
> I
> > > guess it takes more time to actually change the level of a NOR cell,
> > > hence the result of 2 identical reads does not mean that the write
is
> > > done).
> > >
> > > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > > same way, and I wonder if cmdset_0002 implementation is correct to
> > > start with. Or maybe I don't get what chip_ready() is for.
> > >
> > > Anyway, this is the sort of clarification I'd like to have.
> >
> > I am thinking to update the commit message as below.
> >
> >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
do_write_oneword()
> >
> >     As reported by the OpenWRT team, write requests sometimes fail on
> some
> >     platforms.
> >     Currently to check the state chip_ready() is used correctly as
> described by
> >     the flash memory S29GL256P11TFI01 datasheet.
>
> I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> it's using cmdset 0001.

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

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

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

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

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

>
> >     Also chip_good() is used to check if the write is succeeded and
it
> was
> >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve
> error
> >     checking").
> >     But actually the write failure is caused on some platforms and also
> it can
> >     be fixed by using chip_good() to check the state and retry instead.
>
> Do you know on which NOR chips this happens? Do you have access to the
> datasheet?

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

+#define SST49LF008A            0x005a

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

>
> >     It is depended on the actual flash chip behavior so the root cause
> is
> >     unknown.
>
> Yes, and that's what I'd like you to figure out, or at least have a
> good idea why this doesn't work on some chips but works on others.

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

>
> >
> > If any comment please let me know.
> >
> > >
> > > >
> > > > 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>
> > >
> > > Has the patch really gone through all those people? SoB is used when
> you
> > > apply a patch in your tree or when you're the original author.
> >
> > I have just checked the OpenWRT git log again and it looks that it was
> originally
> > implemented by Felix Fietkau <nbd@openwrt.org> by the patch below so
I
> will update the Signed-off-by tag as so.
> >
>
<https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640
> f07cd2b3b14fe9ec03fa63a586452cc5f>
> >
> > >
> > > > 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>
> > >
> > > Not sure we want to add new undocumented tags, but you can mention
> > > that all those people helped you find/debug the issue. They can also
> > > add their Reviewed-by/Tested-by if they like.
>
> My bad, I just noticed these are valid flags [2], so you can keep them,
> and according to the doc, you should also keep the SoB.

I see.
Yes I had also checked it.

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

Regards,
Ikegami

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

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

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

[Tokunori Ikegami]

Hi Boris-san,

Very sorry for too late to update about this.
But could you please let me consult below about this patch?

I have tried to investigate the issue root cause and confirmed below but
still the root cause seems not clear.

  1. Without the change the write oneword is retried and recovered by the
current existing chip_good() checking.
     But after the 1,001 times recovery it was continued to fail recovery
with the 3 times retry.

  2. By the patch change the recovery failure can be avoided and the write
oneword works correctly without any failure.
     There are different from the original chip_good() checking as the
original code resets the chip before the retry.
     The patch change wait the chip_good() status until the timeout expiry
without the chip reset.
       Note: There is a different from the original OpenWrt patch and needed
to be changed as same and it will be done by the next v4 patch.

  3. To narrow down the cause I have added some delays into the original
code to check the chip ready and good status.
     But the failure behavior was not changed so it seems that the issue is
not depended to the timing. (But not sure)

  4. On the OpenWrt the write buffer is disabled but to narrow down the
issue I have changed to enable the write buffer.
     Then the flash error was not happened by the write buffer operation so
it seems that the flash driver works correctly.
     But another issue was caused and it is similar issue with the original
OpenWrt behavior with the patch change.
       Note: On the original OpenWrt needs to wait the file system
completion to build but it was not finished with the write buffer. (But not
sure about this behavior)

Do you have any comment about this result?

If you can agree about the patch change basically with the current situation
I will do send the v4 patch set later with fix for the comments.

But it seems that it is difficult to investigate the root cause more at this
moment to me.
Since but the behavior looks depended on the flash chip hardware behavior
and I cannot debug the hardware behavior more I think.
  Note: Now I can reproduce the flash error issue behavior on the OpenWrt
unit.

> > >     It is depended on the actual flash chip behavior so the root cause
> > is
> > >     unknown.
> >
> > Yes, and that's what I'd like you to figure out, or at least have a
> > good idea why this doesn't work on some chips but works on others.
> 
> I see.
> But it is a little bit difficult situation since I do not have the failure
> environment locally at this moment.
> But if needed I may ask to get the help for this to Fabio-san.

Regards,
Ikegami

> -----Original Message-----
> From: IKEGAMI Tokunori [mailto:ikegami@allied-telesis.co.jp]
> Sent: Tuesday, November 6, 2018 6:47 PM
> To: Boris Brezillon; ikegami_to@yahoo.co.jp
> Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> Sorry let me resend the mail below by changing the email address of
> Felix-san.
> 
> -----Original Message-----
> From: IKEGAMI Tokunori
> Sent: Tuesday, November 06, 2018 6:37 PM
> To: 'Boris Brezillon'; 'ikegami_to@yahoo.co.jp'
> Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> Hi Boris-san,
> 
> > -----Original Message-----
> > From: stable-owner@vger.kernel.org
> > [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> > Sent: Tuesday, November 06, 2018 5:34 PM
> > To: IKEGAMI Tokunori
> > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > do_write_oneword() to use chip_good()
> >
> > Hi IKEGAMI,
> >
> > On Tue, 6 Nov 2018 00:25:43 +0000
> > IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> >
> > > > > Also the issue can be fixed by using chip_good() instead of
> > chip_ready().
> > > > > The chip_ready() just checks the value from flash memory twice.
> > > > > And the chip_good() checks the value with the expected value.
> > > > > Probably the issue can be fixed as checked correctly by the
> chip_good().
> > > > > So change to use chip_good() instead of chip_ready().
> > > >
> > > > Well, that's not really explaining why you think chip_good() should
> > be
> > > > used instead of chip_ready(). So I went on and looked at the
> > > > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > > > also looked at users of do_write_oneword(). It seems this function
> is
> > > > used to write data to the flash, and apparently the "one bit should
> > > > toggle to reflect a busy state" does not apply when writing things
> to
> > > > the memory array (it's probably working for other CFI commands, but
> > I
> > > > guess it takes more time to actually change the level of a NOR cell,
> > > > hence the result of 2 identical reads does not mean that the write
> is
> > > > done).
> > > >
> > > > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > > > same way, and I wonder if cmdset_0002 implementation is correct to
> > > > start with. Or maybe I don't get what chip_ready() is for.
> > > >
> > > > Anyway, this is the sort of clarification I'd like to have.
> > >
> > > I am thinking to update the commit message as below.
> > >
> > >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
> do_write_oneword()
> > >
> > >     As reported by the OpenWRT team, write requests sometimes fail on
> > some
> > >     platforms.
> > >     Currently to check the state chip_ready() is used correctly as
> > described by
> > >     the flash memory S29GL256P11TFI01 datasheet.
> >
> > I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> > it's using cmdset 0001.
> 
> No actually the cmdset 0002 is used on the flash chip.
> The manufacturer ID xx01h and Device ID 2201h are used to decide.
> 
> There is information from Fobis-san below also about this.
> 
> On forum thread musashino posted picture of flash chip:
> https://forum.openwrt.org/t/impossible-to-install-update-any-packages-
> on-wzr-hp-g300nh-18-06-1
> http://www.cypress.com/part/s29gl256p11tfi010
> 
> [    0.862264] physmap platform flash device: 02000000 at 1e000000
> [    0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
> bank. Manufacturer ID 0x000001 Chip ID 0x002201
> [    0.878493] Amd/Fujitsu Extended Query Table at 0x0040
> [    0.883668]   Amd/Fujitsu Extended Query version 1.3.
> [    0.888768] number of CFI chips: 1
> [    0.894557] Searching for RedBoot partition table in physmap-flash
> at offset 0x1fc0000
> [    0.918009] Searching for RedBoot partition table in physmap-flash
> at offset 0x1fe0000
> [    0.941464] No RedBoot partition table detected in physmap-flash
> [    0.947926] Creating 5 MTD partitions on "physmap-flash":
> [    0.953384] 0x000000000000-0x000000040000 : "u-boot"
> [    0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
> [    0.968803] 0x000000060000-0x000001fc0000 : "firmware"
> [    0.981859] 2 uimage-fw partitions found on MTD device firmware
> [    0.987900] 0x000000060000-0x0000001b5706 : "kernel"
> [    0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
> [    1.001986] mtd: device 4 (rootfs) set to be root filesystem
> [    1.007789] 1 squashfs-split partitions found on MTD device rootfs
> [    1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
> [    1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
> [    1.030283] 0x000001fe0000-0x000002000000 : "art"
> 
> Maybe you could post links to forum thread, and data sheet.
> 
> >
> > >     Also chip_good() is used to check if the write is succeeded and
> it
> > was
> > >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve
> > error
> > >     checking").
> > >     But actually the write failure is caused on some platforms and
also
> > it can
> > >     be fixed by using chip_good() to check the state and retry
instead.
> >
> > Do you know on which NOR chips this happens? Do you have access to the
> > datasheet?
> 
> But it looks SST49LF008A [3] from the changes below but I am not sure at
> this moment and probably it should be confirmed to the authr Eric W.
> Biedermann <ebiederman@lnxi.com> to make sure.
> 
> +#define SST49LF008A            0x005a
> 
>  static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *,
> u_char *);
>  static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t,
> size_t *, const u_char *);
> @@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
>  };
>  static struct cfi_fixup jedec_fixup_table[] = {
>         { MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock, NULL, },
> +       { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock, NULL, },
> 
> >
> > >     It is depended on the actual flash chip behavior so the root cause
> > is
> > >     unknown.
> >
> > Yes, and that's what I'd like you to figure out, or at least have a
> > good idea why this doesn't work on some chips but works on others.
> 
> I see.
> But it is a little bit difficult situation since I do not have the failure
> environment locally at this moment.
> But if needed I may ask to get the help for this to Fabio-san.
> 
> >
> > >
> > > If any comment please let me know.
> > >
> > > >
> > > > >
> > > > > 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>
> > > >
> > > > Has the patch really gone through all those people? SoB is used when
> > you
> > > > apply a patch in your tree or when you're the original author.
> > >
> > > I have just checked the OpenWRT git log again and it looks that it was
> > originally
> > > implemented by Felix Fietkau <nbd@openwrt.org> by the patch below so
> I
> > will update the Signed-off-by tag as so.
> > >
> >
> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640
> > f07cd2b3b14fe9ec03fa63a586452cc5f>
> > >
> > > >
> > > > > 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>
> > > >
> > > > Not sure we want to add new undocumented tags, but you can mention
> > > > that all those people helped you find/debug the issue. They can also
> > > > add their Reviewed-by/Tested-by if they like.
> >
> > My bad, I just noticed these are valid flags [2], so you can keep them,
> > and according to the doc, you should also keep the SoB.
> 
> I see.
> Yes I had also checked it.
> 
> By the way in near future my company email address will be not able to
use.
> So I will change the mail address to my personal email address [4] after
> that or before.
> 
> Regards,
> Ikegami
> 
> >
> > Regards,
> >
> > Boris
> >
> > [1]http://www.cypress.com/file/219926/download
> >
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546
> 
> [3]https://www.microchip.com/wwwproducts/en/SST49LF008A
> [4]ikegami_to@yahoo.co.jp


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

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

[Joakim Tjernlund]

On Wed, 2019-01-23 at 00:49 +0900, Tokunori Ikegami wrote:
> 
> 
> Hi Boris-san,
> 
> Very sorry for too late to update about this.
> But could you please let me consult below about this patch?
> 
> I have tried to investigate the issue root cause and confirmed below but
> still the root cause seems not clear.
> 
>   1. Without the change the write oneword is retried and recovered by the
> current existing chip_good() checking.
>      But after the 1,001 times recovery it was continued to fail recovery
> with the 3 times retry.

I have lost track of all the details regarding this issue. I just want to add:

There is a max number of suspend/resume cycles one can do during an erase(possibly also for write)
and once that number is hit you get an error. One way to avoid this could be to
wait after each resume until the erase has started again(look in status register)
before continuing.

If this has anything to do with this problem, I do not know.

 Jocke

>   2. By the patch change the recovery failure can be avoided and the write
> oneword works correctly without any failure.
>      There are different from the original chip_good() checking as the
> original code resets the chip before the retry.
>      The patch change wait the chip_good() status until the timeout expiry
> without the chip reset.
>        Note: There is a different from the original OpenWrt patch and needed
> to be changed as same and it will be done by the next v4 patch.
> 
>   3. To narrow down the cause I have added some delays into the original
> code to check the chip ready and good status.
>      But the failure behavior was not changed so it seems that the issue is
> not depended to the timing. (But not sure)
> 
>   4. On the OpenWrt the write buffer is disabled but to narrow down the
> issue I have changed to enable the write buffer.
>      Then the flash error was not happened by the write buffer operation so
> it seems that the flash driver works correctly.
>      But another issue was caused and it is similar issue with the original
> OpenWrt behavior with the patch change.
>        Note: On the original OpenWrt needs to wait the file system
> completion to build but it was not finished with the write buffer. (But not
> sure about this behavior)
> 
> Do you have any comment about this result?
> 
> If you can agree about the patch change basically with the current situation
> I will do send the v4 patch set later with fix for the comments.
> 
> But it seems that it is difficult to investigate the root cause more at this
> moment to me.
> Since but the behavior looks depended on the flash chip hardware behavior
> and I cannot debug the hardware behavior more I think.
>   Note: Now I can reproduce the flash error issue behavior on the OpenWrt
> unit.
> 
> > > >     It is depended on the actual flash chip behavior so the root cause
> > > is
> > > >     unknown.
> > > 
> > > Yes, and that's what I'd like you to figure out, or at least have a
> > > good idea why this doesn't work on some chips but works on others.
> > 
> > I see.
> > But it is a little bit difficult situation since I do not have the failure
> > environment locally at this moment.
> > But if needed I may ask to get the help for this to Fabio-san.
> 
> Regards,
> Ikegami
> 
> > -----Original Message-----
> > From: IKEGAMI Tokunori [mailto:ikegami@allied-telesis.co.jp]
> > Sent: Tuesday, November 6, 2018 6:47 PM
> > To: Boris Brezillon; ikegami_to@yahoo.co.jp
> > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > do_write_oneword() to use chip_good()
> > 
> > Sorry let me resend the mail below by changing the email address of
> > Felix-san.
> > 
> > -----Original Message-----
> > From: IKEGAMI Tokunori
> > Sent: Tuesday, November 06, 2018 6:37 PM
> > To: 'Boris Brezillon'; 'ikegami_to@yahoo.co.jp'
> > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > do_write_oneword() to use chip_good()
> > 
> > Hi Boris-san,
> > 
> > > -----Original Message-----
> > > From: stable-owner@vger.kernel.org
> > > [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> > > Sent: Tuesday, November 06, 2018 5:34 PM
> > > To: IKEGAMI Tokunori
> > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > do_write_oneword() to use chip_good()
> > > 
> > > Hi IKEGAMI,
> > > 
> > > On Tue, 6 Nov 2018 00:25:43 +0000
> > > IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> > > 
> > > > > > Also the issue can be fixed by using chip_good() instead of
> > > chip_ready().
> > > > > > The chip_ready() just checks the value from flash memory twice.
> > > > > > And the chip_good() checks the value with the expected value.
> > > > > > Probably the issue can be fixed as checked correctly by the
> > chip_good().
> > > > > > So change to use chip_good() instead of chip_ready().
> > > > > 
> > > > > Well, that's not really explaining why you think chip_good() should
> > > be
> > > > > used instead of chip_ready(). So I went on and looked at the
> > > > > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > > > > also looked at users of do_write_oneword(). It seems this function
> > is
> > > > > used to write data to the flash, and apparently the "one bit should
> > > > > toggle to reflect a busy state" does not apply when writing things
> > to
> > > > > the memory array (it's probably working for other CFI commands, but
> > > I
> > > > > guess it takes more time to actually change the level of a NOR cell,
> > > > > hence the result of 2 identical reads does not mean that the write
> > is
> > > > > done).
> > > > > 
> > > > > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > > > > same way, and I wonder if cmdset_0002 implementation is correct to
> > > > > start with. Or maybe I don't get what chip_ready() is for.
> > > > > 
> > > > > Anyway, this is the sort of clarification I'd like to have.
> > > > 
> > > > I am thinking to update the commit message as below.
> > > > 
> > > >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
> > do_write_oneword()
> > > >     As reported by the OpenWRT team, write requests sometimes fail on
> > > some
> > > >     platforms.
> > > >     Currently to check the state chip_ready() is used correctly as
> > > described by
> > > >     the flash memory S29GL256P11TFI01 datasheet.
> > > 
> > > I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> > > it's using cmdset 0001.
> > 
> > No actually the cmdset 0002 is used on the flash chip.
> > The manufacturer ID xx01h and Device ID 2201h are used to decide.
> > 
> > There is information from Fobis-san below also about this.
> > 
> > On forum thread musashino posted picture of flash chip:
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforum.openwrt.org%2Ft%2Fimpossible-to-install-update-any-packages-&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=NNGSYgq1VTuofPPMMlyKIm9W1DJHQFw0s94Ernq5cts%3D&amp;reserved=0
> > on-wzr-hp-g300nh-18-06-1
> > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.cypress.com%2Fpart%2Fs29gl256p11tfi010&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=Twk1VUEESz14UpdJjU4ohuhiQ5jN1uHLh0cAhlAznW0%3D&amp;reserved=0
> > 
> > [    0.862264] physmap platform flash device: 02000000 at 1e000000
> > [    0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
> > bank. Manufacturer ID 0x000001 Chip ID 0x002201
> > [    0.878493] Amd/Fujitsu Extended Query Table at 0x0040
> > [    0.883668]   Amd/Fujitsu Extended Query version 1.3.
> > [    0.888768] number of CFI chips: 1
> > [    0.894557] Searching for RedBoot partition table in physmap-flash
> > at offset 0x1fc0000
> > [    0.918009] Searching for RedBoot partition table in physmap-flash
> > at offset 0x1fe0000
> > [    0.941464] No RedBoot partition table detected in physmap-flash
> > [    0.947926] Creating 5 MTD partitions on "physmap-flash":
> > [    0.953384] 0x000000000000-0x000000040000 : "u-boot"
> > [    0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
> > [    0.968803] 0x000000060000-0x000001fc0000 : "firmware"
> > [    0.981859] 2 uimage-fw partitions found on MTD device firmware
> > [    0.987900] 0x000000060000-0x0000001b5706 : "kernel"
> > [    0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
> > [    1.001986] mtd: device 4 (rootfs) set to be root filesystem
> > [    1.007789] 1 squashfs-split partitions found on MTD device rootfs
> > [    1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
> > [    1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
> > [    1.030283] 0x000001fe0000-0x000002000000 : "art"
> > 
> > Maybe you could post links to forum thread, and data sheet.
> > 
> > > >     Also chip_good() is used to check if the write is succeeded and
> > it
> > > was
> > > >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve
> > > error
> > > >     checking").
> > > >     But actually the write failure is caused on some platforms and
> also
> > > it can
> > > >     be fixed by using chip_good() to check the state and retry
> instead.
> > > Do you know on which NOR chips this happens? Do you have access to the
> > > datasheet?
> > 
> > But it looks SST49LF008A [3] from the changes below but I am not sure at
> > this moment and probably it should be confirmed to the authr Eric W.
> > Biedermann <ebiederman@lnxi.com> to make sure.
> > 
> > +#define SST49LF008A            0x005a
> > 
> >  static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *,
> > u_char *);
> >  static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t,
> > size_t *, const u_char *);
> > @@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
> >  };
> >  static struct cfi_fixup jedec_fixup_table[] = {
> >         { MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock, NULL, },
> > +       { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock, NULL, },
> > 
> > > >     It is depended on the actual flash chip behavior so the root cause
> > > is
> > > >     unknown.
> > > 
> > > Yes, and that's what I'd like you to figure out, or at least have a
> > > good idea why this doesn't work on some chips but works on others.
> > 
> > I see.
> > But it is a little bit difficult situation since I do not have the failure
> > environment locally at this moment.
> > But if needed I may ask to get the help for this to Fabio-san.
> > 
> > > > If any comment please let me know.
> > > > 
> > > > > > 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>
> > > > > 
> > > > > Has the patch really gone through all those people? SoB is used when
> > > you
> > > > > apply a patch in your tree or when you're the original author.
> > > > 
> > > > I have just checked the OpenWRT git log again and it looks that it was
> > > originally
> > > > implemented by Felix Fietkau <nbd@openwrt.org> by the patch below so
> > I
> > > will update the Signed-off-by tag as so.
> > <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.openwrt.org%2F%3Fp%3Dopenwrt%2Fopenwrt.git%3Ba%3Dcommitdiff%3Bh%3D2530640&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=w13ZTKwD1NiUQzxQfUou92KVDlW80qGUiZVIcjU%2BGPA%3D&amp;reserved=0
> > > f07cd2b3b14fe9ec03fa63a586452cc5f>
> > > > > > 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>
> > > > > 
> > > > > Not sure we want to add new undocumented tags, but you can mention
> > > > > that all those people helped you find/debug the issue. They can also
> > > > > add their Reviewed-by/Tested-by if they like.
> > > 
> > > My bad, I just noticed these are valid flags [2], so you can keep them,
> > > and according to the doc, you should also keep the SoB.
> > 
> > I see.
> > Yes I had also checked it.
> > 
> > By the way in near future my company email address will be not able to
> use.
> > So I will change the mail address to my personal email address [4] after
> > that or before.
> > 
> > Regards,
> > Ikegami
> > 
> > > Regards,
> > > 
> > > Boris
> > > 
> > > [1]https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.cypress.com%2Ffile%2F219926%2Fdownload&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=ILqgA75bFHZz9GnswZiDnknzEb3Dryha6CFuVb5Hvhs%3D&amp;reserved=0
> > > 
> > [2]https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=xRCLc%2FnYHeLAiyKoj3obuOuY19JnZPZFZAYrZh%2BJXUI%3D&amp;reserved=0
> > > tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546
> > 
> > [3]https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.microchip.com%2Fwwwproducts%2Fen%2FSST49LF008A&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=3v3AaOaxogw4PAydIISO3PTmkzXo4Tdbux2D0Q1V5sg%3D&amp;reserved=0
> > [4]ikegami_to@yahoo.co.jp

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

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

[Tokunori Ikegami]

Hi Jocke-san,

Thanks for your advice.

To make sure let me confirm below.

The OpenWrt code includes your patch below.

  f69cd2d30a80 2018-05-01 12:58:18 -0700 mtd: cfi: cmdset_0002: Do not allow read/write to suspend erase block.  [Joakim Tjernlund]

Do you mean that it is possible to be needed an additional change more based on this?
Or is it not related to the patch fixed by you?
  Note: Sorry now I am not able to check the patches to try sent by you before.

> I have lost track of all the details regarding this issue. I just want to
> add:
> 
> There is a max number of suspend/resume cycles one can do during an
> erase(possibly also for write)
> and once that number is hit you get an error. One way to avoid this could
> be to
> wait after each resume until the erase has started again(look in status
> register)
> before continuing.
> 
> If this has anything to do with this problem, I do not know.
> 
>  Jocke

Regards,
Ikegami

> -----Original Message-----
> From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> Sent: Wednesday, January 23, 2019 1:58 AM
> To: boris.brezillon@bootlin.com; ikegami_to@yahoo.co.jp
> Cc: linux-mtd@lists.infradead.org; chris.packham@alliedtelesis.co.nz;
> fbettoni@gmail.com; nbd@nbd.name; stable@vger.kernel.org;
> hauke@hauke-m.de; koen.vandeputte@ncentric.com;
> boris.brezillon@free-electrons.com
> Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> On Wed, 2019-01-23 at 00:49 +0900, Tokunori Ikegami wrote:
> >
> >
> > Hi Boris-san,
> >
> > Very sorry for too late to update about this.
> > But could you please let me consult below about this patch?
> >
> > I have tried to investigate the issue root cause and confirmed below but
> > still the root cause seems not clear.
> >
> >   1. Without the change the write oneword is retried and recovered by
> the
> > current existing chip_good() checking.
> >      But after the 1,001 times recovery it was continued to fail recovery
> > with the 3 times retry.
> 
> I have lost track of all the details regarding this issue. I just want to
> add:
> 
> There is a max number of suspend/resume cycles one can do during an
> erase(possibly also for write)
> and once that number is hit you get an error. One way to avoid this could
> be to
> wait after each resume until the erase has started again(look in status
> register)
> before continuing.
> 
> If this has anything to do with this problem, I do not know.
> 
>  Jocke
> 
> >   2. By the patch change the recovery failure can be avoided and the write
> > oneword works correctly without any failure.
> >      There are different from the original chip_good() checking as the
> > original code resets the chip before the retry.
> >      The patch change wait the chip_good() status until the timeout expiry
> > without the chip reset.
> >        Note: There is a different from the original OpenWrt patch and
> needed
> > to be changed as same and it will be done by the next v4 patch.
> >
> >   3. To narrow down the cause I have added some delays into the original
> > code to check the chip ready and good status.
> >      But the failure behavior was not changed so it seems that the issue
> is
> > not depended to the timing. (But not sure)
> >
> >   4. On the OpenWrt the write buffer is disabled but to narrow down the
> > issue I have changed to enable the write buffer.
> >      Then the flash error was not happened by the write buffer operation
> so
> > it seems that the flash driver works correctly.
> >      But another issue was caused and it is similar issue with the original
> > OpenWrt behavior with the patch change.
> >        Note: On the original OpenWrt needs to wait the file system
> > completion to build but it was not finished with the write buffer. (But
> not
> > sure about this behavior)
> >
> > Do you have any comment about this result?
> >
> > If you can agree about the patch change basically with the current
> situation
> > I will do send the v4 patch set later with fix for the comments.
> >
> > But it seems that it is difficult to investigate the root cause more at
> this
> > moment to me.
> > Since but the behavior looks depended on the flash chip hardware behavior
> > and I cannot debug the hardware behavior more I think.
> >   Note: Now I can reproduce the flash error issue behavior on the OpenWrt
> > unit.
> >
> > > > >     It is depended on the actual flash chip behavior so the root
> cause
> > > > is
> > > > >     unknown.
> > > >
> > > > Yes, and that's what I'd like you to figure out, or at least have
> a
> > > > good idea why this doesn't work on some chips but works on others.
> > >
> > > I see.
> > > But it is a little bit difficult situation since I do not have the failure
> > > environment locally at this moment.
> > > But if needed I may ask to get the help for this to Fabio-san.
> >
> > Regards,
> > Ikegami
> >
> > > -----Original Message-----
> > > From: IKEGAMI Tokunori [mailto:ikegami@allied-telesis.co.jp]
> > > Sent: Tuesday, November 6, 2018 6:47 PM
> > > To: Boris Brezillon; ikegami_to@yahoo.co.jp
> > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > do_write_oneword() to use chip_good()
> > >
> > > Sorry let me resend the mail below by changing the email address of
> > > Felix-san.
> > >
> > > -----Original Message-----
> > > From: IKEGAMI Tokunori
> > > Sent: Tuesday, November 06, 2018 6:37 PM
> > > To: 'Boris Brezillon'; 'ikegami_to@yahoo.co.jp'
> > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > do_write_oneword() to use chip_good()
> > >
> > > Hi Boris-san,
> > >
> > > > -----Original Message-----
> > > > From: stable-owner@vger.kernel.org
> > > > [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> > > > Sent: Tuesday, November 06, 2018 5:34 PM
> > > > To: IKEGAMI Tokunori
> > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> Mehrtens;
> > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > do_write_oneword() to use chip_good()
> > > >
> > > > Hi IKEGAMI,
> > > >
> > > > On Tue, 6 Nov 2018 00:25:43 +0000
> > > > IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> > > >
> > > > > > > Also the issue can be fixed by using chip_good() instead of
> > > > chip_ready().
> > > > > > > The chip_ready() just checks the value from flash memory twice.
> > > > > > > And the chip_good() checks the value with the expected value.
> > > > > > > Probably the issue can be fixed as checked correctly by the
> > > chip_good().
> > > > > > > So change to use chip_good() instead of chip_ready().
> > > > > >
> > > > > > Well, that's not really explaining why you think chip_good() should
> > > > be
> > > > > > used instead of chip_ready(). So I went on and looked at the
> > > > > > chip_good(), chip_ready() and do_write_oneword() implementation,
> and
> > > > > > also looked at users of do_write_oneword(). It seems this function
> > > is
> > > > > > used to write data to the flash, and apparently the "one bit should
> > > > > > toggle to reflect a busy state" does not apply when writing things
> > > to
> > > > > > the memory array (it's probably working for other CFI commands,
> but
> > > > I
> > > > > > guess it takes more time to actually change the level of a NOR
> cell,
> > > > > > hence the result of 2 identical reads does not mean that the write
> > > is
> > > > > > done).
> > > > > >
> > > > > > Also, it seems that cmdset_0001 is not implementing chip_ready()
> the
> > > > > > same way, and I wonder if cmdset_0002 implementation is correct
> to
> > > > > > start with. Or maybe I don't get what chip_ready() is for.
> > > > > >
> > > > > > Anyway, this is the sort of clarification I'd like to have.
> > > > >
> > > > > I am thinking to update the commit message as below.
> > > > >
> > > > >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
> > > do_write_oneword()
> > > > >     As reported by the OpenWRT team, write requests sometimes fail
> on
> > > > some
> > > > >     platforms.
> > > > >     Currently to check the state chip_ready() is used correctly
> as
> > > > described by
> > > > >     the flash memory S29GL256P11TFI01 datasheet.
> > > >
> > > > I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> > > > it's using cmdset 0001.
> > >
> > > No actually the cmdset 0002 is used on the flash chip.
> > > The manufacturer ID xx01h and Device ID 2201h are used to decide.
> > >
> > > There is information from Fobis-san below also about this.
> > >
> > > On forum thread musashino posted picture of flash chip:
> > >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforum
> .openwrt.org%2Ft%2Fimpossible-to-install-update-any-packages-&amp;data
> =02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d68
> 0812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683768968012655
> 7&amp;sdata=NNGSYgq1VTuofPPMMlyKIm9W1DJHQFw0s94Ernq5cts%3D&amp;reserve
> d=0
> > > on-wzr-hp-g300nh-18-06-1
> > >
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.cy
> press.com%2Fpart%2Fs29gl256p11tfi010&amp;data=02%7C01%7Cjoakim.tjernlu
> nd%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b0
> 3a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=Twk1VUEESz14U
> pdJjU4ohuhiQ5jN1uHLh0cAhlAznW0%3D&amp;reserved=0
> > >
> > > [    0.862264] physmap platform flash device: 02000000 at 1e000000
> > > [    0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
> > > bank. Manufacturer ID 0x000001 Chip ID 0x002201
> > > [    0.878493] Amd/Fujitsu Extended Query Table at 0x0040
> > > [    0.883668]   Amd/Fujitsu Extended Query version 1.3.
> > > [    0.888768] number of CFI chips: 1
> > > [    0.894557] Searching for RedBoot partition table in physmap-flash
> > > at offset 0x1fc0000
> > > [    0.918009] Searching for RedBoot partition table in physmap-flash
> > > at offset 0x1fe0000
> > > [    0.941464] No RedBoot partition table detected in physmap-flash
> > > [    0.947926] Creating 5 MTD partitions on "physmap-flash":
> > > [    0.953384] 0x000000000000-0x000000040000 : "u-boot"
> > > [    0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
> > > [    0.968803] 0x000000060000-0x000001fc0000 : "firmware"
> > > [    0.981859] 2 uimage-fw partitions found on MTD device firmware
> > > [    0.987900] 0x000000060000-0x0000001b5706 : "kernel"
> > > [    0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
> > > [    1.001986] mtd: device 4 (rootfs) set to be root filesystem
> > > [    1.007789] 1 squashfs-split partitions found on MTD device rootfs
> > > [    1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
> > > [    1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
> > > [    1.030283] 0x000001fe0000-0x000002000000 : "art"
> > >
> > > Maybe you could post links to forum thread, and data sheet.
> > >
> > > > >     Also chip_good() is used to check if the write is succeeded
> and
> > > it
> > > > was
> > > > >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 -
> Improve
> > > > error
> > > > >     checking").
> > > > >     But actually the write failure is caused on some platforms and
> > also
> > > > it can
> > > > >     be fixed by using chip_good() to check the state and retry
> > instead.
> > > > Do you know on which NOR chips this happens? Do you have access to
> the
> > > > datasheet?
> > >
> > > But it looks SST49LF008A [3] from the changes below but I am not sure
> at
> > > this moment and probably it should be confirmed to the authr Eric W.
> > > Biedermann <ebiederman@lnxi.com> to make sure.
> > >
> > > +#define SST49LF008A            0x005a
> > >
> > >  static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t
> *,
> > > u_char *);
> > >  static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t,
> > > size_t *, const u_char *);
> > > @@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
> > >  };
> > >  static struct cfi_fixup jedec_fixup_table[] = {
> > >         { MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock, NULL, },
> > > +       { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock, NULL, },
> > >
> > > > >     It is depended on the actual flash chip behavior so the root
> cause
> > > > is
> > > > >     unknown.
> > > >
> > > > Yes, and that's what I'd like you to figure out, or at least have
> a
> > > > good idea why this doesn't work on some chips but works on others.
> > >
> > > I see.
> > > But it is a little bit difficult situation since I do not have the failure
> > > environment locally at this moment.
> > > But if needed I may ask to get the help for this to Fabio-san.
> > >
> > > > > If any comment please let me know.
> > > > >
> > > > > > > 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>
> > > > > >
> > > > > > Has the patch really gone through all those people? SoB is used
> when
> > > > you
> > > > > > apply a patch in your tree or when you're the original author.
> > > > >
> > > > > I have just checked the OpenWRT git log again and it looks that
> it was
> > > > originally
> > > > > implemented by Felix Fietkau <nbd@openwrt.org> by the patch below
> so
> > > I
> > > > will update the Signed-off-by tag as so.
> > >
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> openwrt.org%2F%3Fp%3Dopenwrt%2Fopenwrt.git%3Ba%3Dcommitdiff%3Bh%3D2530
> 640&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a4
> 02da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683
> 7689680126557&amp;sdata=w13ZTKwD1NiUQzxQfUou92KVDlW80qGUiZVIcjU%2BGPA%
> 3D&amp;reserved=0
> > > > f07cd2b3b14fe9ec03fa63a586452cc5f>
> > > > > > > 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>
> > > > > >
> > > > > > Not sure we want to add new undocumented tags, but you can mention
> > > > > > that all those people helped you find/debug the issue. They can
> also
> > > > > > add their Reviewed-by/Tested-by if they like.
> > > >
> > > > My bad, I just noticed these are valid flags [2], so you can keep
> them,
> > > > and according to the doc, you should also keep the SoB.
> > >
> > > I see.
> > > Yes I had also checked it.
> > >
> > > By the way in near future my company email address will be not able
> to
> > use.
> > > So I will change the mail address to my personal email address [4] after
> > > that or before.
> > >
> > > Regards,
> > > Ikegami
> > >
> > > > Regards,
> > > >
> > > > Boris
> > > >
> > > >
> [1]https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .cypress.com%2Ffile%2F219926%2Fdownload&amp;data=02%7C01%7Cjoakim.tjer
> nlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b
> 4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=ILqgA75bFH
> Zz9GnswZiDnknzEb3Dryha6CFuVb5Hvhs%3D&amp;reserved=0
> > > >
> > >
> [2]https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> t.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git
> %2F&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a4
> 02da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683
> 7689680126557&amp;sdata=xRCLc%2FnYHeLAiyKoj3obuOuY19JnZPZFZAYrZh%2BJXU
> I%3D&amp;reserved=0
> > > > tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546
> > >
> > >
> [3]https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w.microchip.com%2Fwwwproducts%2Fen%2FSST49LF008A&amp;data=02%7C01%7Cjo
> akim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285
> 643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=3
> v3AaOaxogw4PAydIISO3PTmkzXo4Tdbux2D0Q1V5sg%3D&amp;reserved=0
> > > [4]ikegami_to@yahoo.co.jp



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

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

[Joakim Tjernlund]

On Wed, 2019-01-23 at 20:56 +0900, Tokunori Ikegami wrote:
> 
> 
> Hi Jocke-san,
> 
> Thanks for your advice.
> 
> To make sure let me confirm below.
> 
> The OpenWrt code includes your patch below.
> 
>   f69cd2d30a80 2018-05-01 12:58:18 -0700 mtd: cfi: cmdset_0002: Do not allow read/write to suspend erase block.  [Joakim Tjernlund]
> 
> Do you mean that it is possible to be needed an additional change more based on this?

That patch resolves another problem. I have not sent a patch for problem I mentioned in this mail. 

> Or is it not related to the patch fixed by you?
>   Note: Sorry now I am not able to check the patches to try sent by you before.

NP
   Jocke

> 
> > I have lost track of all the details regarding this issue. I just want to
> > add:
> > 
> > There is a max number of suspend/resume cycles one can do during an
> > erase(possibly also for write)
> > and once that number is hit you get an error. One way to avoid this could
> > be to
> > wait after each resume until the erase has started again(look in status
> > register)
> > before continuing.
> > 
> > If this has anything to do with this problem, I do not know.
> > 
> >  Jocke
> 
> Regards,
> Ikegami
> 
> > -----Original Message-----
> > From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> > Sent: Wednesday, January 23, 2019 1:58 AM
> > To: boris.brezillon@bootlin.com; ikegami_to@yahoo.co.jp
> > Cc: linux-mtd@lists.infradead.org; chris.packham@alliedtelesis.co.nz;
> > fbettoni@gmail.com; nbd@nbd.name; stable@vger.kernel.org;
> > hauke@hauke-m.de; koen.vandeputte@ncentric.com;
> > boris.brezillon@free-electrons.com
> > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > do_write_oneword() to use chip_good()
> > 
> > On Wed, 2019-01-23 at 00:49 +0900, Tokunori Ikegami wrote:
> > > 
> > > Hi Boris-san,
> > > 
> > > Very sorry for too late to update about this.
> > > But could you please let me consult below about this patch?
> > > 
> > > I have tried to investigate the issue root cause and confirmed below but
> > > still the root cause seems not clear.
> > > 
> > >   1. Without the change the write oneword is retried and recovered by
> > the
> > > current existing chip_good() checking.
> > >      But after the 1,001 times recovery it was continued to fail recovery
> > > with the 3 times retry.
> > 
> > I have lost track of all the details regarding this issue. I just want to
> > add:
> > 
> > There is a max number of suspend/resume cycles one can do during an
> > erase(possibly also for write)
> > and once that number is hit you get an error. One way to avoid this could
> > be to
> > wait after each resume until the erase has started again(look in status
> > register)
> > before continuing.
> > 
> > If this has anything to do with this problem, I do not know.
> > 
> >  Jocke
> > 
> > >   2. By the patch change the recovery failure can be avoided and the write
> > > oneword works correctly without any failure.
> > >      There are different from the original chip_good() checking as the
> > > original code resets the chip before the retry.
> > >      The patch change wait the chip_good() status until the timeout expiry
> > > without the chip reset.
> > >        Note: There is a different from the original OpenWrt patch and
> > needed
> > > to be changed as same and it will be done by the next v4 patch.
> > > 
> > >   3. To narrow down the cause I have added some delays into the original
> > > code to check the chip ready and good status.
> > >      But the failure behavior was not changed so it seems that the issue
> > is
> > > not depended to the timing. (But not sure)
> > > 
> > >   4. On the OpenWrt the write buffer is disabled but to narrow down the
> > > issue I have changed to enable the write buffer.
> > >      Then the flash error was not happened by the write buffer operation
> > so
> > > it seems that the flash driver works correctly.
> > >      But another issue was caused and it is similar issue with the original
> > > OpenWrt behavior with the patch change.
> > >        Note: On the original OpenWrt needs to wait the file system
> > > completion to build but it was not finished with the write buffer. (But
> > not
> > > sure about this behavior)
> > > 
> > > Do you have any comment about this result?
> > > 
> > > If you can agree about the patch change basically with the current
> > situation
> > > I will do send the v4 patch set later with fix for the comments.
> > > 
> > > But it seems that it is difficult to investigate the root cause more at
> > this
> > > moment to me.
> > > Since but the behavior looks depended on the flash chip hardware behavior
> > > and I cannot debug the hardware behavior more I think.
> > >   Note: Now I can reproduce the flash error issue behavior on the OpenWrt
> > > unit.
> > > 
> > > > > >     It is depended on the actual flash chip behavior so the root
> > cause
> > > > > is
> > > > > >     unknown.
> > > > > 
> > > > > Yes, and that's what I'd like you to figure out, or at least have
> > a
> > > > > good idea why this doesn't work on some chips but works on others.
> > > > 
> > > > I see.
> > > > But it is a little bit difficult situation since I do not have the failure
> > > > environment locally at this moment.
> > > > But if needed I may ask to get the help for this to Fabio-san.
> > > 
> > > Regards,
> > > Ikegami
> > > 
> > > > -----Original Message-----
> > > > From: IKEGAMI Tokunori [mailto:ikegami@allied-telesis.co.jp]
> > > > Sent: Tuesday, November 6, 2018 6:47 PM
> > > > To: Boris Brezillon; ikegami_to@yahoo.co.jp
> > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > do_write_oneword() to use chip_good()
> > > > 
> > > > Sorry let me resend the mail below by changing the email address of
> > > > Felix-san.
> > > > 
> > > > -----Original Message-----
> > > > From: IKEGAMI Tokunori
> > > > Sent: Tuesday, November 06, 2018 6:37 PM
> > > > To: 'Boris Brezillon'; 'ikegami_to@yahoo.co.jp'
> > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke Mehrtens;
> > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > do_write_oneword() to use chip_good()
> > > > 
> > > > Hi Boris-san,
> > > > 
> > > > > -----Original Message-----
> > > > > From: stable-owner@vger.kernel.org
> > > > > [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> > > > > Sent: Tuesday, November 06, 2018 5:34 PM
> > > > > To: IKEGAMI Tokunori
> > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> > Mehrtens;
> > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > do_write_oneword() to use chip_good()
> > > > > 
> > > > > Hi IKEGAMI,
> > > > > 
> > > > > On Tue, 6 Nov 2018 00:25:43 +0000
> > > > > IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> > > > > 
> > > > > > > > Also the issue can be fixed by using chip_good() instead of
> > > > > chip_ready().
> > > > > > > > The chip_ready() just checks the value from flash memory twice.
> > > > > > > > And the chip_good() checks the value with the expected value.
> > > > > > > > Probably the issue can be fixed as checked correctly by the
> > > > chip_good().
> > > > > > > > So change to use chip_good() instead of chip_ready().
> > > > > > > 
> > > > > > > Well, that's not really explaining why you think chip_good() should
> > > > > be
> > > > > > > used instead of chip_ready(). So I went on and looked at the
> > > > > > > chip_good(), chip_ready() and do_write_oneword() implementation,
> > and
> > > > > > > also looked at users of do_write_oneword(). It seems this function
> > > > is
> > > > > > > used to write data to the flash, and apparently the "one bit should
> > > > > > > toggle to reflect a busy state" does not apply when writing things
> > > > to
> > > > > > > the memory array (it's probably working for other CFI commands,
> > but
> > > > > I
> > > > > > > guess it takes more time to actually change the level of a NOR
> > cell,
> > > > > > > hence the result of 2 identical reads does not mean that the write
> > > > is
> > > > > > > done).
> > > > > > > 
> > > > > > > Also, it seems that cmdset_0001 is not implementing chip_ready()
> > the
> > > > > > > same way, and I wonder if cmdset_0002 implementation is correct
> > to
> > > > > > > start with. Or maybe I don't get what chip_ready() is for.
> > > > > > > 
> > > > > > > Anyway, this is the sort of clarification I'd like to have.
> > > > > > 
> > > > > > I am thinking to update the commit message as below.
> > > > > > 
> > > > > >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
> > > > do_write_oneword()
> > > > > >     As reported by the OpenWRT team, write requests sometimes fail
> > on
> > > > > some
> > > > > >     platforms.
> > > > > >     Currently to check the state chip_ready() is used correctly
> > as
> > > > > described by
> > > > > >     the flash memory S29GL256P11TFI01 datasheet.
> > > > > 
> > > > > I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> > > > > it's using cmdset 0001.
> > > > 
> > > > No actually the cmdset 0002 is used on the flash chip.
> > > > The manufacturer ID xx01h and Device ID 2201h are used to decide.
> > > > 
> > > > There is information from Fobis-san below also about this.
> > > > 
> > > > On forum thread musashino posted picture of flash chip:
> > > > 
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforum
> > .openwrt.org%2Ft%2Fimpossible-to-install-update-any-packages-&amp;data
> > =02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d68
> > 0812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683768968012655
> > 7&amp;sdata=NNGSYgq1VTuofPPMMlyKIm9W1DJHQFw0s94Ernq5cts%3D&amp;reserve
> > d=0
> > > > on-wzr-hp-g300nh-18-06-1
> > > > 
> > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.cy
> > press.com%2Fpart%2Fs29gl256p11tfi010&amp;data=02%7C01%7Cjoakim.tjernlu
> > nd%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b0
> > 3a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=Twk1VUEESz14U
> > pdJjU4ohuhiQ5jN1uHLh0cAhlAznW0%3D&amp;reserved=0
> > > > [    0.862264] physmap platform flash device: 02000000 at 1e000000
> > > > [    0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
> > > > bank. Manufacturer ID 0x000001 Chip ID 0x002201
> > > > [    0.878493] Amd/Fujitsu Extended Query Table at 0x0040
> > > > [    0.883668]   Amd/Fujitsu Extended Query version 1.3.
> > > > [    0.888768] number of CFI chips: 1
> > > > [    0.894557] Searching for RedBoot partition table in physmap-flash
> > > > at offset 0x1fc0000
> > > > [    0.918009] Searching for RedBoot partition table in physmap-flash
> > > > at offset 0x1fe0000
> > > > [    0.941464] No RedBoot partition table detected in physmap-flash
> > > > [    0.947926] Creating 5 MTD partitions on "physmap-flash":
> > > > [    0.953384] 0x000000000000-0x000000040000 : "u-boot"
> > > > [    0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
> > > > [    0.968803] 0x000000060000-0x000001fc0000 : "firmware"
> > > > [    0.981859] 2 uimage-fw partitions found on MTD device firmware
> > > > [    0.987900] 0x000000060000-0x0000001b5706 : "kernel"
> > > > [    0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
> > > > [    1.001986] mtd: device 4 (rootfs) set to be root filesystem
> > > > [    1.007789] 1 squashfs-split partitions found on MTD device rootfs
> > > > [    1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
> > > > [    1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
> > > > [    1.030283] 0x000001fe0000-0x000002000000 : "art"
> > > > 
> > > > Maybe you could post links to forum thread, and data sheet.
> > > > 
> > > > > >     Also chip_good() is used to check if the write is succeeded
> > and
> > > > it
> > > > > was
> > > > > >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 -
> > Improve
> > > > > error
> > > > > >     checking").
> > > > > >     But actually the write failure is caused on some platforms and
> > > also
> > > > > it can
> > > > > >     be fixed by using chip_good() to check the state and retry
> > > instead.
> > > > > Do you know on which NOR chips this happens? Do you have access to
> > the
> > > > > datasheet?
> > > > 
> > > > But it looks SST49LF008A [3] from the changes below but I am not sure
> > at
> > > > this moment and probably it should be confirmed to the authr Eric W.
> > > > Biedermann <ebiederman@lnxi.com> to make sure.
> > > > 
> > > > +#define SST49LF008A            0x005a
> > > > 
> > > >  static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t
> > *,
> > > > u_char *);
> > > >  static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t,
> > > > size_t *, const u_char *);
> > > > @@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
> > > >  };
> > > >  static struct cfi_fixup jedec_fixup_table[] = {
> > > >         { MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock, NULL, },
> > > > +       { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock, NULL, },
> > > > 
> > > > > >     It is depended on the actual flash chip behavior so the root
> > cause
> > > > > is
> > > > > >     unknown.
> > > > > 
> > > > > Yes, and that's what I'd like you to figure out, or at least have
> > a
> > > > > good idea why this doesn't work on some chips but works on others.
> > > > 
> > > > I see.
> > > > But it is a little bit difficult situation since I do not have the failure
> > > > environment locally at this moment.
> > > > But if needed I may ask to get the help for this to Fabio-san.
> > > > 
> > > > > > If any comment please let me know.
> > > > > > 
> > > > > > > > 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>
> > > > > > > 
> > > > > > > Has the patch really gone through all those people? SoB is used
> > when
> > > > > you
> > > > > > > apply a patch in your tree or when you're the original author.
> > > > > > 
> > > > > > I have just checked the OpenWRT git log again and it looks that
> > it was
> > > > > originally
> > > > > > implemented by Felix Fietkau <nbd@openwrt.org> by the patch below
> > so
> > > > I
> > > > > will update the Signed-off-by tag as so.
> > <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> > openwrt.org%2F%3Fp%3Dopenwrt%2Fopenwrt.git%3Ba%3Dcommitdiff%3Bh%3D2530
> > 640&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a4
> > 02da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683
> > 7689680126557&amp;sdata=w13ZTKwD1NiUQzxQfUou92KVDlW80qGUiZVIcjU%2BGPA%
> > 3D&amp;reserved=0
> > > > > f07cd2b3b14fe9ec03fa63a586452cc5f>
> > > > > > > > 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>
> > > > > > > 
> > > > > > > Not sure we want to add new undocumented tags, but you can mention
> > > > > > > that all those people helped you find/debug the issue. They can
> > also
> > > > > > > add their Reviewed-by/Tested-by if they like.
> > > > > 
> > > > > My bad, I just noticed these are valid flags [2], so you can keep
> > them,
> > > > > and according to the doc, you should also keep the SoB.
> > > > 
> > > > I see.
> > > > Yes I had also checked it.
> > > > 
> > > > By the way in near future my company email address will be not able
> > to
> > > use.
> > > > So I will change the mail address to my personal email address [4] after
> > > > that or before.
> > > > 
> > > > Regards,
> > > > Ikegami
> > > > 
> > > > > Regards,
> > > > > 
> > > > > Boris
> > > > > 
> > > > > 
> 

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

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

[Tokunori Ikegami]

Thanks for your quick response.
Noted it so I will try to confirm your advice.

Regards,
Ikegami

> -----Original Message-----
> From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> Sent: Wednesday, January 23, 2019 9:13 PM
> To: boris.brezillon@bootlin.com; ikegami_to@yahoo.co.jp
> Cc: linux-mtd@lists.infradead.org; chris.packham@alliedtelesis.co.nz;
> fbettoni@gmail.com; nbd@nbd.name; stable@vger.kernel.org;
> hauke@hauke-m.de; koen.vandeputte@ncentric.com;
> boris.brezillon@free-electrons.com
> Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> On Wed, 2019-01-23 at 20:56 +0900, Tokunori Ikegami wrote:
> >
> >
> > Hi Jocke-san,
> >
> > Thanks for your advice.
> >
> > To make sure let me confirm below.
> >
> > The OpenWrt code includes your patch below.
> >
> >   f69cd2d30a80 2018-05-01 12:58:18 -0700 mtd: cfi: cmdset_0002: Do not
> allow read/write to suspend erase block.  [Joakim Tjernlund]
> >
> > Do you mean that it is possible to be needed an additional change more
> based on this?
> 
> That patch resolves another problem. I have not sent a patch for problem
> I mentioned in this mail.
> 
> > Or is it not related to the patch fixed by you?
> >   Note: Sorry now I am not able to check the patches to try sent by you
> before.
> 
> NP
>    Jocke
> 
> >
> > > I have lost track of all the details regarding this issue. I just want
> to
> > > add:
> > >
> > > There is a max number of suspend/resume cycles one can do during an
> > > erase(possibly also for write)
> > > and once that number is hit you get an error. One way to avoid this
> could
> > > be to
> > > wait after each resume until the erase has started again(look in status
> > > register)
> > > before continuing.
> > >
> > > If this has anything to do with this problem, I do not know.
> > >
> > >  Jocke
> >
> > Regards,
> > Ikegami
> >
> > > -----Original Message-----
> > > From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> > > Sent: Wednesday, January 23, 2019 1:58 AM
> > > To: boris.brezillon@bootlin.com; ikegami_to@yahoo.co.jp
> > > Cc: linux-mtd@lists.infradead.org;
> chris.packham@alliedtelesis.co.nz;
> > > fbettoni@gmail.com; nbd@nbd.name; stable@vger.kernel.org;
> > > hauke@hauke-m.de; koen.vandeputte@ncentric.com;
> > > boris.brezillon@free-electrons.com
> > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > do_write_oneword() to use chip_good()
> > >
> > > On Wed, 2019-01-23 at 00:49 +0900, Tokunori Ikegami wrote:
> > > >
> > > > Hi Boris-san,
> > > >
> > > > Very sorry for too late to update about this.
> > > > But could you please let me consult below about this patch?
> > > >
> > > > I have tried to investigate the issue root cause and confirmed below
> but
> > > > still the root cause seems not clear.
> > > >
> > > >   1. Without the change the write oneword is retried and recovered
> by
> > > the
> > > > current existing chip_good() checking.
> > > >      But after the 1,001 times recovery it was continued to fail
> recovery
> > > > with the 3 times retry.
> > >
> > > I have lost track of all the details regarding this issue. I just want
> to
> > > add:
> > >
> > > There is a max number of suspend/resume cycles one can do during an
> > > erase(possibly also for write)
> > > and once that number is hit you get an error. One way to avoid this
> could
> > > be to
> > > wait after each resume until the erase has started again(look in status
> > > register)
> > > before continuing.
> > >
> > > If this has anything to do with this problem, I do not know.
> > >
> > >  Jocke
> > >
> > > >   2. By the patch change the recovery failure can be avoided and the
> write
> > > > oneword works correctly without any failure.
> > > >      There are different from the original chip_good() checking as
> the
> > > > original code resets the chip before the retry.
> > > >      The patch change wait the chip_good() status until the timeout
> expiry
> > > > without the chip reset.
> > > >        Note: There is a different from the original OpenWrt patch
> and
> > > needed
> > > > to be changed as same and it will be done by the next v4 patch.
> > > >
> > > >   3. To narrow down the cause I have added some delays into the original
> > > > code to check the chip ready and good status.
> > > >      But the failure behavior was not changed so it seems that the
> issue
> > > is
> > > > not depended to the timing. (But not sure)
> > > >
> > > >   4. On the OpenWrt the write buffer is disabled but to narrow down
> the
> > > > issue I have changed to enable the write buffer.
> > > >      Then the flash error was not happened by the write buffer
> operation
> > > so
> > > > it seems that the flash driver works correctly.
> > > >      But another issue was caused and it is similar issue with the
> original
> > > > OpenWrt behavior with the patch change.
> > > >        Note: On the original OpenWrt needs to wait the file system
> > > > completion to build but it was not finished with the write buffer.
> (But
> > > not
> > > > sure about this behavior)
> > > >
> > > > Do you have any comment about this result?
> > > >
> > > > If you can agree about the patch change basically with the current
> > > situation
> > > > I will do send the v4 patch set later with fix for the comments.
> > > >
> > > > But it seems that it is difficult to investigate the root cause more
> at
> > > this
> > > > moment to me.
> > > > Since but the behavior looks depended on the flash chip hardware
> behavior
> > > > and I cannot debug the hardware behavior more I think.
> > > >   Note: Now I can reproduce the flash error issue behavior on the
> OpenWrt
> > > > unit.
> > > >
> > > > > > >     It is depended on the actual flash chip behavior so the
> root
> > > cause
> > > > > > is
> > > > > > >     unknown.
> > > > > >
> > > > > > Yes, and that's what I'd like you to figure out, or at least have
> > > a
> > > > > > good idea why this doesn't work on some chips but works on others.
> > > > >
> > > > > I see.
> > > > > But it is a little bit difficult situation since I do not have the
> failure
> > > > > environment locally at this moment.
> > > > > But if needed I may ask to get the help for this to Fabio-san.
> > > >
> > > > Regards,
> > > > Ikegami
> > > >
> > > > > -----Original Message-----
> > > > > From: IKEGAMI Tokunori [mailto:ikegami@allied-telesis.co.jp]
> > > > > Sent: Tuesday, November 6, 2018 6:47 PM
> > > > > To: Boris Brezillon; ikegami_to@yahoo.co.jp
> > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> Mehrtens;
> > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > do_write_oneword() to use chip_good()
> > > > >
> > > > > Sorry let me resend the mail below by changing the email address
> of
> > > > > Felix-san.
> > > > >
> > > > > -----Original Message-----
> > > > > From: IKEGAMI Tokunori
> > > > > Sent: Tuesday, November 06, 2018 6:37 PM
> > > > > To: 'Boris Brezillon'; 'ikegami_to@yahoo.co.jp'
> > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> Mehrtens;
> > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > do_write_oneword() to use chip_good()
> > > > >
> > > > > Hi Boris-san,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: stable-owner@vger.kernel.org
> > > > > > [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris Brezillon
> > > > > > Sent: Tuesday, November 06, 2018 5:34 PM
> > > > > > To: IKEGAMI Tokunori
> > > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> > > Mehrtens;
> > > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > > do_write_oneword() to use chip_good()
> > > > > >
> > > > > > Hi IKEGAMI,
> > > > > >
> > > > > > On Tue, 6 Nov 2018 00:25:43 +0000
> > > > > > IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> > > > > >
> > > > > > > > > Also the issue can be fixed by using chip_good() instead
> of
> > > > > > chip_ready().
> > > > > > > > > The chip_ready() just checks the value from flash memory
> twice.
> > > > > > > > > And the chip_good() checks the value with the expected value.
> > > > > > > > > Probably the issue can be fixed as checked correctly by
> the
> > > > > chip_good().
> > > > > > > > > So change to use chip_good() instead of chip_ready().
> > > > > > > >
> > > > > > > > Well, that's not really explaining why you think chip_good()
> should
> > > > > > be
> > > > > > > > used instead of chip_ready(). So I went on and looked at the
> > > > > > > > chip_good(), chip_ready() and do_write_oneword()
> implementation,
> > > and
> > > > > > > > also looked at users of do_write_oneword(). It seems this
> function
> > > > > is
> > > > > > > > used to write data to the flash, and apparently the "one bit
> should
> > > > > > > > toggle to reflect a busy state" does not apply when writing
> things
> > > > > to
> > > > > > > > the memory array (it's probably working for other CFI commands,
> > > but
> > > > > > I
> > > > > > > > guess it takes more time to actually change the level of a
> NOR
> > > cell,
> > > > > > > > hence the result of 2 identical reads does not mean that the
> write
> > > > > is
> > > > > > > > done).
> > > > > > > >
> > > > > > > > Also, it seems that cmdset_0001 is not implementing chip_ready()
> > > the
> > > > > > > > same way, and I wonder if cmdset_0002 implementation is correct
> > > to
> > > > > > > > start with. Or maybe I don't get what chip_ready() is for.
> > > > > > > >
> > > > > > > > Anyway, this is the sort of clarification I'd like to have.
> > > > > > >
> > > > > > > I am thinking to update the commit message as below.
> > > > > > >
> > > > > > >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
> > > > > do_write_oneword()
> > > > > > >     As reported by the OpenWRT team, write requests sometimes
> fail
> > > on
> > > > > > some
> > > > > > >     platforms.
> > > > > > >     Currently to check the state chip_ready() is used correctly
> > > as
> > > > > > described by
> > > > > > >     the flash memory S29GL256P11TFI01 datasheet.
> > > > > >
> > > > > > I had a look at the S29GL256P datasheet here [1], and if I'm correct,
> > > > > > it's using cmdset 0001.
> > > > >
> > > > > No actually the cmdset 0002 is used on the flash chip.
> > > > > The manufacturer ID xx01h and Device ID 2201h are used to decide.
> > > > >
> > > > > There is information from Fobis-san below also about this.
> > > > >
> > > > > On forum thread musashino posted picture of flash chip:
> > > > >
> > >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforum
> >
> > .openwrt.org%2Ft%2Fimpossible-to-install-update-any-packages-&amp;da
> ta
> > >
> =02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d68
> > >
> 0812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683768968012655
> > >
> 7&amp;sdata=NNGSYgq1VTuofPPMMlyKIm9W1DJHQFw0s94Ernq5cts%3D&amp;reserve
> > > d=0
> > > > > on-wzr-hp-g300nh-18-06-1
> > > > >
> > >
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.cy
> > >
> press.com%2Fpart%2Fs29gl256p11tfi010&amp;data=02%7C01%7Cjoakim.tjernlu
> > >
> nd%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b0
> > >
> 3a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=Twk1VUEESz14U
> > > pdJjU4ohuhiQ5jN1uHLh0cAhlAznW0%3D&amp;reserved=0
> > > > > [    0.862264] physmap platform flash device: 02000000 at 1e000000
> > > > > [    0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
> > > > > bank. Manufacturer ID 0x000001 Chip ID 0x002201
> > > > > [    0.878493] Amd/Fujitsu Extended Query Table at 0x0040
> > > > > [    0.883668]   Amd/Fujitsu Extended Query version 1.3.
> > > > > [    0.888768] number of CFI chips: 1
> > > > > [    0.894557] Searching for RedBoot partition table in
> physmap-flash
> > > > > at offset 0x1fc0000
> > > > > [    0.918009] Searching for RedBoot partition table in
> physmap-flash
> > > > > at offset 0x1fe0000
> > > > > [    0.941464] No RedBoot partition table detected in physmap-flash
> > > > > [    0.947926] Creating 5 MTD partitions on "physmap-flash":
> > > > > [    0.953384] 0x000000000000-0x000000040000 : "u-boot"
> > > > > [    0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
> > > > > [    0.968803] 0x000000060000-0x000001fc0000 : "firmware"
> > > > > [    0.981859] 2 uimage-fw partitions found on MTD device firmware
> > > > > [    0.987900] 0x000000060000-0x0000001b5706 : "kernel"
> > > > > [    0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
> > > > > [    1.001986] mtd: device 4 (rootfs) set to be root filesystem
> > > > > [    1.007789] 1 squashfs-split partitions found on MTD device rootfs
> > > > > [    1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
> > > > > [    1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
> > > > > [    1.030283] 0x000001fe0000-0x000002000000 : "art"
> > > > >
> > > > > Maybe you could post links to forum thread, and data sheet.
> > > > >
> > > > > > >     Also chip_good() is used to check if the write is succeeded
> > > and
> > > > > it
> > > > > > was
> > > > > > >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002
> -
> > > Improve
> > > > > > error
> > > > > > >     checking").
> > > > > > >     But actually the write failure is caused on some platforms
> and
> > > > also
> > > > > > it can
> > > > > > >     be fixed by using chip_good() to check the state and retry
> > > > instead.
> > > > > > Do you know on which NOR chips this happens? Do you have access
> to
> > > the
> > > > > > datasheet?
> > > > >
> > > > > But it looks SST49LF008A [3] from the changes below but I am not
> sure
> > > at
> > > > > this moment and probably it should be confirmed to the authr Eric
> W.
> > > > > Biedermann <ebiederman@lnxi.com> to make sure.
> > > > >
> > > > > +#define SST49LF008A            0x005a
> > > > >
> > > > >  static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t,
> size_t
> > > *,
> > > > > u_char *);
> > > > >  static int cfi_amdstd_write_words(struct mtd_info *, loff_t,
> size_t,
> > > > > size_t *, const u_char *);
> > > > > @@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[] =
> {
> > > > >  };
> > > > >  static struct cfi_fixup jedec_fixup_table[] = {
> > > > >         { MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock,
> NULL, },
> > > > > +       { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock,
> NULL, },
> > > > >
> > > > > > >     It is depended on the actual flash chip behavior so the
> root
> > > cause
> > > > > > is
> > > > > > >     unknown.
> > > > > >
> > > > > > Yes, and that's what I'd like you to figure out, or at least have
> > > a
> > > > > > good idea why this doesn't work on some chips but works on others.
> > > > >
> > > > > I see.
> > > > > But it is a little bit difficult situation since I do not have the
> failure
> > > > > environment locally at this moment.
> > > > > But if needed I may ask to get the help for this to Fabio-san.
> > > > >
> > > > > > > If any comment please let me know.
> > > > > > >
> > > > > > > > > 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>
> > > > > > > >
> > > > > > > > Has the patch really gone through all those people? SoB is
> used
> > > when
> > > > > > you
> > > > > > > > apply a patch in your tree or when you're the original author.
> > > > > > >
> > > > > > > I have just checked the OpenWRT git log again and it looks that
> > > it was
> > > > > > originally
> > > > > > > implemented by Felix Fietkau <nbd@openwrt.org> by the patch
> below
> > > so
> > > > > I
> > > > > > will update the Signed-off-by tag as so.
> > >
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> > >
> openwrt.org%2F%3Fp%3Dopenwrt%2Fopenwrt.git%3Ba%3Dcommitdiff%3Bh%3D2530
> > >
> 640&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a4
> > >
> 02da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683
> > >
> 7689680126557&amp;sdata=w13ZTKwD1NiUQzxQfUou92KVDlW80qGUiZVIcjU%2BGPA%
> > > 3D&amp;reserved=0
> > > > > > f07cd2b3b14fe9ec03fa63a586452cc5f>
> > > > > > > > > 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>
> > > > > > > >
> > > > > > > > Not sure we want to add new undocumented tags, but you can
> mention
> > > > > > > > that all those people helped you find/debug the issue. They
> can
> > > also
> > > > > > > > add their Reviewed-by/Tested-by if they like.
> > > > > >
> > > > > > My bad, I just noticed these are valid flags [2], so you can keep
> > > them,
> > > > > > and according to the doc, you should also keep the SoB.
> > > > >
> > > > > I see.
> > > > > Yes I had also checked it.
> > > > >
> > > > > By the way in near future my company email address will be not able
> > > to
> > > > use.
> > > > > So I will change the mail address to my personal email address [4]
> after
> > > > > that or before.
> > > > >
> > > > > Regards,
> > > > > Ikegami
> > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Boris
> > > > > >
> > > > > >
> >



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

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

[Tokunori Ikegami]

Hi Jocke-san,

I have just confirmed the issue behavior again below as based on your advice.
So it seems that the issue behavior is not depended on the maximum suspend and resume cycles number mentioned by you.

1. On OpenWrt code the erasing state is not suspended since it is changed to sleep always for the erasing state in get_chip().
     Note: The issue behavior is caused by reverting the change to sleep for the erasing.
2. Additionally tried to change to not suspend for the writing state also but still the issue has been caused.
3. Added 1 us delay after the reset to retry to write one word but the issue is caused.
4. Added the unlock bypass reset to retry to write one word to make sure the issue behavior situation but the issue is caused.

It seems that the reset to retry to write one word causes the write error issue behavior that seems not able to be recovered.
Also the current checking for chip ready seems not enough so the reset is repeated about 1,000 times then the issue happened.

Now still the chip good checking seems correct to use until the time out to write one word.

> > > > I have lost track of all the details regarding this issue. I just
> want
> > to
> > > > add:
> > > >
> > > > There is a max number of suspend/resume cycles one can do during an
> > > > erase(possibly also for write)
> > > > and once that number is hit you get an error. One way to avoid this
> > could
> > > > be to
> > > > wait after each resume until the erase has started again(look in status
> > > > register)
> > > > before continuing.
> > > >
> > > > If this has anything to do with this problem, I do not know.

Regards,
Ikegami

> -----Original Message-----
> From: Tokunori Ikegami [mailto:ikegami_to@yahoo.co.jp]
> Sent: Wednesday, January 23, 2019 9:34 PM
> To: 'Joakim Tjernlund'; 'boris.brezillon@bootlin.com'
> Cc: 'linux-mtd@lists.infradead.org';
> 'chris.packham@alliedtelesis.co.nz'; 'fbettoni@gmail.com';
> 'nbd@nbd.name'; 'stable@vger.kernel.org'; 'hauke@hauke-m.de';
> 'koen.vandeputte@ncentric.com'; 'boris.brezillon@free-electrons.com'
> Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> Thanks for your quick response.
> Noted it so I will try to confirm your advice.
> 
> Regards,
> Ikegami
> 
> > -----Original Message-----
> > From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> > Sent: Wednesday, January 23, 2019 9:13 PM
> > To: boris.brezillon@bootlin.com; ikegami_to@yahoo.co.jp
> > Cc: linux-mtd@lists.infradead.org; chris.packham@alliedtelesis.co.nz;
> > fbettoni@gmail.com; nbd@nbd.name; stable@vger.kernel.org;
> > hauke@hauke-m.de; koen.vandeputte@ncentric.com;
> > boris.brezillon@free-electrons.com
> > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > do_write_oneword() to use chip_good()
> >
> > On Wed, 2019-01-23 at 20:56 +0900, Tokunori Ikegami wrote:
> > >
> > >
> > > Hi Jocke-san,
> > >
> > > Thanks for your advice.
> > >
> > > To make sure let me confirm below.
> > >
> > > The OpenWrt code includes your patch below.
> > >
> > >   f69cd2d30a80 2018-05-01 12:58:18 -0700 mtd: cfi: cmdset_0002: Do not
> > allow read/write to suspend erase block.  [Joakim Tjernlund]
> > >
> > > Do you mean that it is possible to be needed an additional change more
> > based on this?
> >
> > That patch resolves another problem. I have not sent a patch for problem
> > I mentioned in this mail.
> >
> > > Or is it not related to the patch fixed by you?
> > >   Note: Sorry now I am not able to check the patches to try sent by
> you
> > before.
> >
> > NP
> >    Jocke
> >
> > >
> > > > I have lost track of all the details regarding this issue. I just
> want
> > to
> > > > add:
> > > >
> > > > There is a max number of suspend/resume cycles one can do during an
> > > > erase(possibly also for write)
> > > > and once that number is hit you get an error. One way to avoid this
> > could
> > > > be to
> > > > wait after each resume until the erase has started again(look in status
> > > > register)
> > > > before continuing.
> > > >
> > > > If this has anything to do with this problem, I do not know.
> > > >
> > > >  Jocke
> > >
> > > Regards,
> > > Ikegami
> > >
> > > > -----Original Message-----
> > > > From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> > > > Sent: Wednesday, January 23, 2019 1:58 AM
> > > > To: boris.brezillon@bootlin.com; ikegami_to@yahoo.co.jp
> > > > Cc: linux-mtd@lists.infradead.org;
> > chris.packham@alliedtelesis.co.nz;
> > > > fbettoni@gmail.com; nbd@nbd.name; stable@vger.kernel.org;
> > > > hauke@hauke-m.de; koen.vandeputte@ncentric.com;
> > > > boris.brezillon@free-electrons.com
> > > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > do_write_oneword() to use chip_good()
> > > >
> > > > On Wed, 2019-01-23 at 00:49 +0900, Tokunori Ikegami wrote:
> > > > >
> > > > > Hi Boris-san,
> > > > >
> > > > > Very sorry for too late to update about this.
> > > > > But could you please let me consult below about this patch?
> > > > >
> > > > > I have tried to investigate the issue root cause and confirmed below
> > but
> > > > > still the root cause seems not clear.
> > > > >
> > > > >   1. Without the change the write oneword is retried and recovered
> > by
> > > > the
> > > > > current existing chip_good() checking.
> > > > >      But after the 1,001 times recovery it was continued to fail
> > recovery
> > > > > with the 3 times retry.
> > > >
> > > > I have lost track of all the details regarding this issue. I just
> want
> > to
> > > > add:
> > > >
> > > > There is a max number of suspend/resume cycles one can do during an
> > > > erase(possibly also for write)
> > > > and once that number is hit you get an error. One way to avoid this
> > could
> > > > be to
> > > > wait after each resume until the erase has started again(look in status
> > > > register)
> > > > before continuing.
> > > >
> > > > If this has anything to do with this problem, I do not know.
> > > >
> > > >  Jocke
> > > >
> > > > >   2. By the patch change the recovery failure can be avoided and
> the
> > write
> > > > > oneword works correctly without any failure.
> > > > >      There are different from the original chip_good() checking
> as
> > the
> > > > > original code resets the chip before the retry.
> > > > >      The patch change wait the chip_good() status until the timeout
> > expiry
> > > > > without the chip reset.
> > > > >        Note: There is a different from the original OpenWrt patch
> > and
> > > > needed
> > > > > to be changed as same and it will be done by the next v4 patch.
> > > > >
> > > > >   3. To narrow down the cause I have added some delays into the
> original
> > > > > code to check the chip ready and good status.
> > > > >      But the failure behavior was not changed so it seems that the
> > issue
> > > > is
> > > > > not depended to the timing. (But not sure)
> > > > >
> > > > >   4. On the OpenWrt the write buffer is disabled but to narrow down
> > the
> > > > > issue I have changed to enable the write buffer.
> > > > >      Then the flash error was not happened by the write buffer
> > operation
> > > > so
> > > > > it seems that the flash driver works correctly.
> > > > >      But another issue was caused and it is similar issue with the
> > original
> > > > > OpenWrt behavior with the patch change.
> > > > >        Note: On the original OpenWrt needs to wait the file system
> > > > > completion to build but it was not finished with the write buffer.
> > (But
> > > > not
> > > > > sure about this behavior)
> > > > >
> > > > > Do you have any comment about this result?
> > > > >
> > > > > If you can agree about the patch change basically with the current
> > > > situation
> > > > > I will do send the v4 patch set later with fix for the comments.
> > > > >
> > > > > But it seems that it is difficult to investigate the root cause
> more
> > at
> > > > this
> > > > > moment to me.
> > > > > Since but the behavior looks depended on the flash chip hardware
> > behavior
> > > > > and I cannot debug the hardware behavior more I think.
> > > > >   Note: Now I can reproduce the flash error issue behavior on the
> > OpenWrt
> > > > > unit.
> > > > >
> > > > > > > >     It is depended on the actual flash chip behavior so the
> > root
> > > > cause
> > > > > > > is
> > > > > > > >     unknown.
> > > > > > >
> > > > > > > Yes, and that's what I'd like you to figure out, or at least
> have
> > > > a
> > > > > > > good idea why this doesn't work on some chips but works on others.
> > > > > >
> > > > > > I see.
> > > > > > But it is a little bit difficult situation since I do not have
> the
> > failure
> > > > > > environment locally at this moment.
> > > > > > But if needed I may ask to get the help for this to Fabio-san.
> > > > >
> > > > > Regards,
> > > > > Ikegami
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: IKEGAMI Tokunori [mailto:ikegami@allied-telesis.co.jp]
> > > > > > Sent: Tuesday, November 6, 2018 6:47 PM
> > > > > > To: Boris Brezillon; ikegami_to@yahoo.co.jp
> > > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> > Mehrtens;
> > > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > > do_write_oneword() to use chip_good()
> > > > > >
> > > > > > Sorry let me resend the mail below by changing the email address
> > of
> > > > > > Felix-san.
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: IKEGAMI Tokunori
> > > > > > Sent: Tuesday, November 06, 2018 6:37 PM
> > > > > > To: 'Boris Brezillon'; 'ikegami_to@yahoo.co.jp'
> > > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> > Mehrtens;
> > > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > > Subject: RE: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > > do_write_oneword() to use chip_good()
> > > > > >
> > > > > > Hi Boris-san,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: stable-owner@vger.kernel.org
> > > > > > > [mailto:stable-owner@vger.kernel.org] On Behalf Of Boris
> Brezillon
> > > > > > > Sent: Tuesday, November 06, 2018 5:34 PM
> > > > > > > To: IKEGAMI Tokunori
> > > > > > > Cc: boris.brezillon@free-electrons.com; Felix Fietkau; Hauke
> > > > Mehrtens;
> > > > > > > stable@vger.kernel.org; Joakim Tjernlund; PACKHAM Chris;
> > > > > > > linux-mtd@lists.infradead.org; Koen Vandeputte; Fabio Bettoni
> > > > > > > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change
> > > > > > > do_write_oneword() to use chip_good()
> > > > > > >
> > > > > > > Hi IKEGAMI,
> > > > > > >
> > > > > > > On Tue, 6 Nov 2018 00:25:43 +0000
> > > > > > > IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> > > > > > >
> > > > > > > > > > Also the issue can be fixed by using chip_good() instead
> > of
> > > > > > > chip_ready().
> > > > > > > > > > The chip_ready() just checks the value from flash memory
> > twice.
> > > > > > > > > > And the chip_good() checks the value with the expected
> value.
> > > > > > > > > > Probably the issue can be fixed as checked correctly by
> > the
> > > > > > chip_good().
> > > > > > > > > > So change to use chip_good() instead of chip_ready().
> > > > > > > > >
> > > > > > > > > Well, that's not really explaining why you think chip_good()
> > should
> > > > > > > be
> > > > > > > > > used instead of chip_ready(). So I went on and looked at
> the
> > > > > > > > > chip_good(), chip_ready() and do_write_oneword()
> > implementation,
> > > > and
> > > > > > > > > also looked at users of do_write_oneword(). It seems this
> > function
> > > > > > is
> > > > > > > > > used to write data to the flash, and apparently the "one
> bit
> > should
> > > > > > > > > toggle to reflect a busy state" does not apply when writing
> > things
> > > > > > to
> > > > > > > > > the memory array (it's probably working for other CFI commands,
> > > > but
> > > > > > > I
> > > > > > > > > guess it takes more time to actually change the level of
> a
> > NOR
> > > > cell,
> > > > > > > > > hence the result of 2 identical reads does not mean that
> the
> > write
> > > > > > is
> > > > > > > > > done).
> > > > > > > > >
> > > > > > > > > Also, it seems that cmdset_0001 is not implementing
> chip_ready()
> > > > the
> > > > > > > > > same way, and I wonder if cmdset_0002 implementation is
> correct
> > > > to
> > > > > > > > > start with. Or maybe I don't get what chip_ready() is for.
> > > > > > > > >
> > > > > > > > > Anyway, this is the sort of clarification I'd like to have.
> > > > > > > >
> > > > > > > > I am thinking to update the commit message as below.
> > > > > > > >
> > > > > > > >     mtd: cfi_cmdset_0002: Use chip_good() to retry in
> > > > > > do_write_oneword()
> > > > > > > >     As reported by the OpenWRT team, write requests sometimes
> > fail
> > > > on
> > > > > > > some
> > > > > > > >     platforms.
> > > > > > > >     Currently to check the state chip_ready() is used correctly
> > > > as
> > > > > > > described by
> > > > > > > >     the flash memory S29GL256P11TFI01 datasheet.
> > > > > > >
> > > > > > > I had a look at the S29GL256P datasheet here [1], and if I'm
> correct,
> > > > > > > it's using cmdset 0001.
> > > > > >
> > > > > > No actually the cmdset 0002 is used on the flash chip.
> > > > > > The manufacturer ID xx01h and Device ID 2201h are used to decide.
> > > > > >
> > > > > > There is information from Fobis-san below also about this.
> > > > > >
> > > > > > On forum thread musashino posted picture of flash chip:
> > > > > >
> > > >
> >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforum
> > >
> >
> > .openwrt.org%2Ft%2Fimpossible-to-install-update-any-packages-&amp;da
> > ta
> > > >
> >
> =02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a402da8cf08d68
> > > >
> >
> 0812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683768968012655
> > > >
> >
> 7&amp;sdata=NNGSYgq1VTuofPPMMlyKIm9W1DJHQFw0s94Ernq5cts%3D&amp;reserve
> > > > d=0
> > > > > > on-wzr-hp-g300nh-18-06-1
> > > > > >
> > > >
> >
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.cy
> > > >
> >
> press.com%2Fpart%2Fs29gl256p11tfi010&amp;data=02%7C01%7Cjoakim.tjernlu
> > > >
> >
> nd%40infinera.com%7C916af968b27a402da8cf08d680812fce%7C285643de5f5b4b0
> > > >
> >
> 3a1530ae2dc8aaf77%7C1%7C1%7C636837689680126557&amp;sdata=Twk1VUEESz14U
> > > > pdJjU4ohuhiQ5jN1uHLh0cAhlAznW0%3D&amp;reserved=0
> > > > > > [    0.862264] physmap platform flash device: 02000000 at 1e000000
> > > > > > [    0.868331] physmap-flash: Found 1 x16 devices at 0x0 in 16-bit
> > > > > > bank. Manufacturer ID 0x000001 Chip ID 0x002201
> > > > > > [    0.878493] Amd/Fujitsu Extended Query Table at 0x0040
> > > > > > [    0.883668]   Amd/Fujitsu Extended Query version 1.3.
> > > > > > [    0.888768] number of CFI chips: 1
> > > > > > [    0.894557] Searching for RedBoot partition table in
> > physmap-flash
> > > > > > at offset 0x1fc0000
> > > > > > [    0.918009] Searching for RedBoot partition table in
> > physmap-flash
> > > > > > at offset 0x1fe0000
> > > > > > [    0.941464] No RedBoot partition table detected in physmap-flash
> > > > > > [    0.947926] Creating 5 MTD partitions on "physmap-flash":
> > > > > > [    0.953384] 0x000000000000-0x000000040000 : "u-boot"
> > > > > > [    0.960853] 0x000000040000-0x000000060000 : "u-boot-env"
> > > > > > [    0.968803] 0x000000060000-0x000001fc0000 : "firmware"
> > > > > > [    0.981859] 2 uimage-fw partitions found on MTD device firmware
> > > > > > [    0.987900] 0x000000060000-0x0000001b5706 : "kernel"
> > > > > > [    0.994916] 0x0000001b5706-0x000001fc0000 : "rootfs"
> > > > > > [    1.001986] mtd: device 4 (rootfs) set to be root filesystem
> > > > > > [    1.007789] 1 squashfs-split partitions found on MTD device
> rootfs
> > > > > > [    1.014014] 0x0000003c0000-0x000001fc0000 : "rootfs_data"
> > > > > > [    1.022093] 0x000001fc0000-0x000001fe0000 : "user_property"
> > > > > > [    1.030283] 0x000001fe0000-0x000002000000 : "art"
> > > > > >
> > > > > > Maybe you could post links to forum thread, and data sheet.
> > > > > >
> > > > > > > >     Also chip_good() is used to check if the write is succeeded
> > > > and
> > > > > > it
> > > > > > > was
> > > > > > > >     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002
> > -
> > > > Improve
> > > > > > > error
> > > > > > > >     checking").
> > > > > > > >     But actually the write failure is caused on some platforms
> > and
> > > > > also
> > > > > > > it can
> > > > > > > >     be fixed by using chip_good() to check the state and retry
> > > > > instead.
> > > > > > > Do you know on which NOR chips this happens? Do you have access
> > to
> > > > the
> > > > > > > datasheet?
> > > > > >
> > > > > > But it looks SST49LF008A [3] from the changes below but I am not
> > sure
> > > > at
> > > > > > this moment and probably it should be confirmed to the authr Eric
> > W.
> > > > > > Biedermann <ebiederman@lnxi.com> to make sure.
> > > > > >
> > > > > > +#define SST49LF008A            0x005a
> > > > > >
> > > > > >  static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t,
> > size_t
> > > > *,
> > > > > > u_char *);
> > > > > >  static int cfi_amdstd_write_words(struct mtd_info *, loff_t,
> > size_t,
> > > > > > size_t *, const u_char *);
> > > > > > @@ -191,6 +192,7 @@ static struct cfi_fixup cfi_fixup_table[]
> =
> > {
> > > > > >  };
> > > > > >  static struct cfi_fixup jedec_fixup_table[] = {
> > > > > >         { MANUFACTURER_SST, SST49LF004B, fixup_use_fwh_lock,
> > NULL, },
> > > > > > +       { MANUFACTURER_SST, SST49LF008A, fixup_use_fwh_lock,
> > NULL, },
> > > > > >
> > > > > > > >     It is depended on the actual flash chip behavior so the
> > root
> > > > cause
> > > > > > > is
> > > > > > > >     unknown.
> > > > > > >
> > > > > > > Yes, and that's what I'd like you to figure out, or at least
> have
> > > > a
> > > > > > > good idea why this doesn't work on some chips but works on others.
> > > > > >
> > > > > > I see.
> > > > > > But it is a little bit difficult situation since I do not have
> the
> > failure
> > > > > > environment locally at this moment.
> > > > > > But if needed I may ask to get the help for this to Fabio-san.
> > > > > >
> > > > > > > > If any comment please let me know.
> > > > > > > >
> > > > > > > > > > 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>
> > > > > > > > >
> > > > > > > > > Has the patch really gone through all those people? SoB
> is
> > used
> > > > when
> > > > > > > you
> > > > > > > > > apply a patch in your tree or when you're the original author.
> > > > > > > >
> > > > > > > > I have just checked the OpenWRT git log again and it looks
> that
> > > > it was
> > > > > > > originally
> > > > > > > > implemented by Felix Fietkau <nbd@openwrt.org> by the patch
> > below
> > > > so
> > > > > > I
> > > > > > > will update the Signed-off-by tag as so.
> > > >
> >
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> > > >
> >
> openwrt.org%2F%3Fp%3Dopenwrt%2Fopenwrt.git%3Ba%3Dcommitdiff%3Bh%3D2530
> > > >
> >
> 640&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C916af968b27a4
> > > >
> >
> 02da8cf08d680812fce%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C63683
> > > >
> >
> 7689680126557&amp;sdata=w13ZTKwD1NiUQzxQfUou92KVDlW80qGUiZVIcjU%2BGPA%
> > > > 3D&amp;reserved=0
> > > > > > > f07cd2b3b14fe9ec03fa63a586452cc5f>
> > > > > > > > > > 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>
> > > > > > > > >
> > > > > > > > > Not sure we want to add new undocumented tags, but you can
> > mention
> > > > > > > > > that all those people helped you find/debug the issue. They
> > can
> > > > also
> > > > > > > > > add their Reviewed-by/Tested-by if they like.
> > > > > > >
> > > > > > > My bad, I just noticed these are valid flags [2], so you can
> keep
> > > > them,
> > > > > > > and according to the doc, you should also keep the SoB.
> > > > > >
> > > > > > I see.
> > > > > > Yes I had also checked it.
> > > > > >
> > > > > > By the way in near future my company email address will be not
> able
> > > > to
> > > > > use.
> > > > > > So I will change the mail address to my personal email address
> [4]
> > after
> > > > > > that or before.
> > > > > >
> > > > > > Regards,
> > > > > > Ikegami
> > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > > Boris
> > > > > > >
> > > > > > >
> > >



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

[PATCH v3 02/11] 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 v2:
- None.

Changes since v1:
- None.

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

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

Re: [PATCH v3 02/11] mtd: cfi_cmdset_0002: Remove chip_ready() from do_write_buffer()

[Boris Brezillon]

On Fri, 26 Oct 2018 01:32:10 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> It is enough to use chip_good() only so chip_ready() is not necessary.

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

> For this change the order to check timeout is also needed to chagne.

								^change.

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

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

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

RE: [PATCH v3 02/11] mtd: cfi_cmdset_0002: Remove chip_ready() from do_write_buffer()

[IKEGAMI Tokunori]

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

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Monday, November 05, 2018 10:03 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH v3 02/11] mtd: cfi_cmdset_0002: Remove chip_ready()
> from do_write_buffer()
> 
> On Fri, 26 Oct 2018 01:32:10 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > It is enough to use chip_good() only so chip_ready() is not necessary.
> 
> I'd like a short explanation saying why chip_good() is enough:
> chip_good() is doing the same check chip_ready() is doing plus an extra
> check to make sure we end up with the data we wrote.

I see so I will update as this.

> 
> > For this change the order to check timeout is also needed to chagne.
> 
> 
> 	^change.

Will fix this.

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

I will update as so.

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

I see.

Regards,
Ikegami

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

[PATCH v3 03/11] 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 v2:
- None.

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

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c2e51768a02c..deffafab067e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 
 		if (chip_good(map, adr, datum)) {
 			xip_enable(map, chip, adr);
-			goto op_done;
+			break;
 		}
 
-		if (time_after(jiffies, timeo))
+		if (time_after(jiffies, timeo)) {
+			ret = -EIO;
 			break;
+		}
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
 	}
 
-	/*
-	 * Recovery from write-buffer programming failures requires
-	 * the write-to-buffer-reset sequence.  Since the last part
-	 * of the sequence also works as a normal reset, we can run
-	 * the same commands regardless of why we are here.
-	 * See e.g.
-	 * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
-	 */
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	xip_enable(map, chip, adr);
-	/* FIXME - should have reset delay before continuing */
+	if (ret) {
+		/*
+		 * Recovery from write-buffer programming failures requires
+		 * the write-to-buffer-reset sequence.  Since the last part
+		 * of the sequence also works as a normal reset, we can run
+		 * the same commands regardless of why we are here.
+		 * See e.g.
+		 * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
+		 */
+		cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
+				 cfi->device_type, NULL);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
+				 cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
+				 cfi->device_type, NULL);
+		xip_enable(map, chip, adr);
+		/* FIXME - should have reset delay before continuing */
 
-	printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
-	       __func__, adr);
+		printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
+		       __func__, adr);
+	}
 
-	ret = -EIO;
- op_done:
 	chip->state = FL_READY;
 	DISABLE_VPP(map);
 	put_chip(map, chip, adr);
-- 
2.18.0

Re: [PATCH v3 03/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()

[Boris Brezillon]

On Fri, 26 Oct 2018 01:32:11 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

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

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

> 
> 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 v2:
> - None.
> 
> Changes since v1:
> - Split the patch v1 3/3.
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 46 +++++++++++++++--------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index c2e51768a02c..deffafab067e 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
>  
>  		if (chip_good(map, adr, datum)) {
>  			xip_enable(map, chip, adr);
> -			goto op_done;
> +			break;
>  		}
>  
> -		if (time_after(jiffies, timeo))
> +		if (time_after(jiffies, timeo)) {
> +			ret = -EIO;
>  			break;
> +		}
>  
>  		/* Latency issues. Drop the lock, wait a while and retry */
>  		UDELAY(map, chip, adr, 1);
>  	}
>  
> -	/*
> -	 * Recovery from write-buffer programming failures requires
> -	 * the write-to-buffer-reset sequence.  Since the last part
> -	 * of the sequence also works as a normal reset, we can run
> -	 * the same commands regardless of why we are here.
> -	 * See e.g.
> -	 * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
> -	 */
> -	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
> -			 cfi->device_type, NULL);
> -	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
> -			 cfi->device_type, NULL);
> -	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
> -			 cfi->device_type, NULL);
> -	xip_enable(map, chip, adr);
> -	/* FIXME - should have reset delay before continuing */
> +	if (ret) {
> +		/*
> +		 * Recovery from write-buffer programming failures requires
> +		 * the write-to-buffer-reset sequence.  Since the last part
> +		 * of the sequence also works as a normal reset, we can run
> +		 * the same commands regardless of why we are here.
> +		 * See e.g.
> +		 * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
> +		 */
> +		cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
> +				 cfi->device_type, NULL);
> +		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
> +				 cfi->device_type, NULL);
> +		cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
> +				 cfi->device_type, NULL);
> +		xip_enable(map, chip, adr);
> +		/* FIXME - should have reset delay before continuing */
>  
> -	printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
> -	       __func__, adr);
> +		printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
> +		       __func__, adr);
> +	}
>  
> -	ret = -EIO;
> - op_done:
>  	chip->state = FL_READY;
>  	DISABLE_VPP(map);
>  	put_chip(map, chip, adr);

RE: [PATCH v3 03/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()

[IKEGAMI Tokunori]

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Monday, November 05, 2018 10:15 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH v3 03/11] mtd: cfi_cmdset_0002: Remove goto statement
> from do_write_buffer()
> 
> On Fri, 26 Oct 2018 01:32:11 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > For a maintainability by reducing the goto statement remove it from
> > do_write_buffer().
> 
> I guess that's a matter of taste, but I don't find this version more
> readable than the previous one. Any strong reason to get rid of the
> op_done label?

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

Regards,
Ikegami

> 
> >
> > 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 v2:
> > - None.
> >
> > Changes since v1:
> > - Split the patch v1 3/3.
> >
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 46
> +++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index c2e51768a02c..deffafab067e 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct
> map_info *map, struct flchip *chip,
> >
> >  		if (chip_good(map, adr, datum)) {
> >  			xip_enable(map, chip, adr);
> > -			goto op_done;
> > +			break;
> >  		}
> >
> > -		if (time_after(jiffies, timeo))
> > +		if (time_after(jiffies, timeo)) {
> > +			ret = -EIO;
> >  			break;
> > +		}
> >
> >  		/* Latency issues. Drop the lock, wait a while and retry
> */
> >  		UDELAY(map, chip, adr, 1);
> >  	}
> >
> > -	/*
> > -	 * Recovery from write-buffer programming failures requires
> > -	 * the write-to-buffer-reset sequence.  Since the last part
> > -	 * of the sequence also works as a normal reset, we can run
> > -	 * the same commands regardless of why we are here.
> > -	 * See e.g.
> > -	 *
> http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_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);
> > +		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);

[PATCH v3 04/11] 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 v2:
- None.

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

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index deffafab067e..a3fa2d7b1ba0 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1882,10 +1882,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 			continue;
 		}
 
-		if (chip_good(map, adr, datum)) {
-			xip_enable(map, chip, adr);
+		if (chip_good(map, adr, datum))
 			break;
-		}
 
 		if (time_after(jiffies, timeo)) {
 			ret = -EIO;
@@ -1911,13 +1909,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 				 cfi->device_type, NULL);
 		cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
 				 cfi->device_type, NULL);
-		xip_enable(map, chip, adr);
 		/* FIXME - should have reset delay before continuing */
 
 		printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
 		       __func__, adr);
 	}
 
+	xip_enable(map, chip, adr);
+
 	chip->state = FL_READY;
 	DISABLE_VPP(map);
 	put_chip(map, chip, adr);
-- 
2.18.0

Re: [PATCH v3 04/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer().

[Boris Brezillon]

On Fri, 26 Oct 2018 01:32:12 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> 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.

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

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

RE: [PATCH v3 04/11] mtd: cfi_cmdset_0002: Call xip_enable() once only in do_write_buffer().

[IKEGAMI Tokunori]

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Monday, November 05, 2018 10:20 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH v3 04/11] mtd: cfi_cmdset_0002: Call xip_enable() once
> only in do_write_buffer().
> 
> On Fri, 26 Oct 2018 01:32:12 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> 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.
> 
> Would have worked with the op_done label too if you place the
> xip_enable() call just after this label, right?

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

Regards,
Ikegami

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

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

[Tokunori Ikegami]

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

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
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: 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 v2:
- Just update the commit message for the comment
- Add Reviewed-by tag.

Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a3fa2d7b1ba0..ae2d8bd7154e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1547,11 +1547,10 @@ static int cfi_amdstd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
 				   do_otp_lock, 1);
 }
 
-static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
-				     unsigned long adr, map_word datum,
-				     int mode)
+static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *chip,
+					  unsigned long adr, map_word datum,
+					  int mode, struct cfi_private *cfi)
 {
-	struct cfi_private *cfi = map->fldrv_priv;
 	unsigned long timeo = jiffies + HZ;
 	/*
 	 * We use a 1ms + 1 jiffies generic timeout for writes (most devices
@@ -1564,42 +1563,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	 */
 	unsigned long uWriteTimeout = (HZ / 1000) + 1;
 	int ret = 0;
-	map_word oldd;
-	int retry_cnt = 0;
-
-	adr += chip->start;
-
-	mutex_lock(&chip->mutex);
-	ret = get_chip(map, chip, adr, mode);
-	if (ret) {
-		mutex_unlock(&chip->mutex);
-		return ret;
-	}
 
-	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
-		 __func__, adr, datum.x[0]);
-
-	if (mode == FL_OTP_WRITE)
-		otp_enter(map, chip, adr, map_bankwidth(map));
-
-	/*
-	 * Check for a NOP for the case when the datum to write is already
-	 * present - it saves time and works around buggy chips that corrupt
-	 * data at other locations when 0xff is written to a location that
-	 * already contains 0xff.
-	 */
-	oldd = map_read(map, adr);
-	if (map_word_equal(map, oldd, datum)) {
-		pr_debug("MTD %s(): NOP\n",
-		       __func__);
-		goto op_done;
-	}
-
-	XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
-	ENABLE_VPP(map);
-	xip_disable(map, chip, adr);
-
- retry:
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -1642,6 +1606,53 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 		UDELAY(map, chip, adr, 1);
 	}
 
+	return ret;
+}
+
+static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
+				     unsigned long adr, map_word datum,
+				     int mode)
+{
+	struct cfi_private *cfi = map->fldrv_priv;
+	int ret = 0;
+	map_word oldd;
+	int retry_cnt = 0;
+
+	adr += chip->start;
+
+	mutex_lock(&chip->mutex);
+	ret = get_chip(map, chip, adr, mode);
+	if (ret) {
+		mutex_unlock(&chip->mutex);
+		return ret;
+	}
+
+	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
+		 __func__, adr, datum.x[0]);
+
+	if (mode == FL_OTP_WRITE)
+		otp_enter(map, chip, adr, map_bankwidth(map));
+
+	/*
+	 * Check for a NOP for the case when the datum to write is already
+	 * present - it saves time and works around buggy chips that corrupt
+	 * data at other locations when 0xff is written to a location that
+	 * already contains 0xff.
+	 */
+	oldd = map_read(map, adr);
+	if (map_word_equal(map, oldd, datum)) {
+		pr_debug("MTD %s(): NOP\n",
+		       __func__);
+		goto op_done;
+	}
+
+	XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
+	ENABLE_VPP(map);
+	xip_disable(map, chip, adr);
+
+ retry:
+	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+
 	/* Did we succeed? */
 	if (ret) {
 		/* reset on all failures. */
-- 
2.18.0

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

[Boris Brezillon]

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

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

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

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

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

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

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

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

>  	if (ret) {
>  		/* reset on all failures. */

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

[IKEGAMI Tokunori]

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Monday, November 05, 2018 10:32 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH v3 05/11] mtd: cfi_cmdset_0002: Split
> do_write_oneword() to reduce function size
> 
> On Fri, 26 Oct 2018 01:32:13 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > Reduce the size of do_write_oneword() by extracting a helper function
> > for the hardware access.
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > 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: 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 v2:
> > - Just update the commit message for the comment
> > - Add Reviewed-by tag.
> >
> > Changes since v1:
> > - Add the patch.
> >
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 89
> ++++++++++++++++-------------
> >  1 file changed, 50 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index a3fa2d7b1ba0..ae2d8bd7154e 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1547,11 +1547,10 @@ static int cfi_amdstd_lock_user_prot_reg(struct
> mtd_info *mtd, loff_t from,
> >  				   do_otp_lock, 1);
> >  }
> >
> > -static int __xipram do_write_oneword(struct map_info *map, struct flchip
> *chip,
> > -				     unsigned long adr, map_word datum,
> > -				     int mode)
> > +static int __xipram do_write_oneword_once(struct map_info *map, struct
> flchip *chip,
> > +					  unsigned long adr, map_word
> datum,
> > +					  int mode, struct cfi_private
> *cfi)
> >  {
> > -	struct cfi_private *cfi = map->fldrv_priv;
> >  	unsigned long timeo = jiffies + HZ;
> >  	/*
> >  	 * We use a 1ms + 1 jiffies generic timeout for writes (most devices
> > @@ -1564,42 +1563,7 @@ static int __xipram do_write_oneword(struct
> map_info *map, struct flchip *chip,
> >  	 */
> >  	unsigned long uWriteTimeout = (HZ / 1000) + 1;
> >  	int ret = 0;
> > -	map_word oldd;
> > -	int retry_cnt = 0;
> > -
> > -	adr += chip->start;
> > -
> > -	mutex_lock(&chip->mutex);
> > -	ret = get_chip(map, chip, adr, mode);
> > -	if (ret) {
> > -		mutex_unlock(&chip->mutex);
> > -		return ret;
> > -	}
> >
> > -	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
> > -		 __func__, adr, datum.x[0]);
> > -
> > -	if (mode == FL_OTP_WRITE)
> > -		otp_enter(map, chip, adr, map_bankwidth(map));
> > -
> > -	/*
> > -	 * Check for a NOP for the case when the datum to write is already
> > -	 * present - it saves time and works around buggy chips that corrupt
> > -	 * data at other locations when 0xff is written to a location that
> > -	 * already contains 0xff.
> > -	 */
> > -	oldd = map_read(map, adr);
> > -	if (map_word_equal(map, oldd, datum)) {
> > -		pr_debug("MTD %s(): NOP\n",
> > -		       __func__);
> > -		goto op_done;
> > -	}
> > -
> > -	XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
> > -	ENABLE_VPP(map);
> > -	xip_disable(map, chip, adr);
> > -
> > - retry:
> >  	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
> cfi->device_type, NULL);
> >  	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
> cfi->device_type, NULL);
> >  	cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi,
> cfi->device_type, NULL);
> > @@ -1642,6 +1606,53 @@ static int __xipram do_write_oneword(struct
> map_info *map, struct flchip *chip,
> >  		UDELAY(map, chip, adr, 1);
> >  	}
> >
> > +	return ret;
> > +}
> > +
> > +static int __xipram do_write_oneword(struct map_info *map, struct flchip
> *chip,
> > +				     unsigned long adr, map_word datum,
> > +				     int mode)
> > +{
> > +	struct cfi_private *cfi = map->fldrv_priv;
> > +	int ret = 0;
> > +	map_word oldd;
> > +	int retry_cnt = 0;
> > +
> > +	adr += chip->start;
> > +
> > +	mutex_lock(&chip->mutex);
> > +	ret = get_chip(map, chip, adr, mode);
> > +	if (ret) {
> > +		mutex_unlock(&chip->mutex);
> > +		return ret;
> > +	}
> > +
> > +	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
> > +		 __func__, adr, datum.x[0]);
> > +
> > +	if (mode == FL_OTP_WRITE)
> > +		otp_enter(map, chip, adr, map_bankwidth(map));
> > +
> > +	/*
> > +	 * Check for a NOP for the case when the datum to write is already
> > +	 * present - it saves time and works around buggy chips that corrupt
> > +	 * data at other locations when 0xff is written to a location that
> > +	 * already contains 0xff.
> > +	 */
> > +	oldd = map_read(map, adr);
> > +	if (map_word_equal(map, oldd, datum)) {
> > +		pr_debug("MTD %s(): NOP\n",
> > +		       __func__);
> > +		goto op_done;
> > +	}
> > +
> > +	XIP_INVAL_CACHED_RANGE(map, adr, map_bankwidth(map));
> > +	ENABLE_VPP(map);
> > +	xip_disable(map, chip, adr);
> > +
> > + retry:
> > +	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
> > +
> >  	/* Did we succeed? */
> 
> We usually place the ret code check just after the call to the function
> returning this ret code:
> 
> 	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
> 	if (ret) {
> 		...
> 	}
> 
> And I don't think we need the "Did we succeed?" comment, since it's
> pretty obvious what this check does.

I see so I will do fix as this.

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

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

Regards,
Ikegami

> 
> >  	if (ret) {
> >  		/* reset on all failures. */

[PATCH v3 06/11] 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.
Also split the start part into do_write_oneword_start() to find easier pairs.

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 v2:
- Change to split the start part of do_write_oneword() additionally.
- Fix indentation to call pr_debug().

Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ae2d8bd7154e..5009c1941a50 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1609,6 +1609,40 @@ static int __xipram do_write_oneword_once(struct map_info *map, struct flchip *c
 	return ret;
 }
 
+static int __xipram do_write_oneword_start(struct map_info *map,
+					   struct flchip *chip,
+					   unsigned long adr, int mode)
+{
+	int ret = 0;
+
+	mutex_lock(&chip->mutex);
+
+	ret = get_chip(map, chip, adr, mode);
+	if (ret) {
+		mutex_unlock(&chip->mutex);
+		return ret;
+	}
+
+	if (mode == FL_OTP_WRITE)
+		otp_enter(map, chip, adr, map_bankwidth(map));
+
+	return ret;
+}
+
+static void __xipram do_write_oneword_done(struct map_info *map,
+					   struct flchip *chip,
+					   unsigned long adr, int mode)
+{
+	if (mode == FL_OTP_WRITE)
+		otp_exit(map, chip, adr, map_bankwidth(map));
+
+	chip->state = FL_READY;
+	DISABLE_VPP(map);
+	put_chip(map, chip, adr);
+
+	mutex_unlock(&chip->mutex);
+}
+
 static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 				     unsigned long adr, map_word datum,
 				     int mode)
@@ -1620,19 +1654,14 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 
 	adr += chip->start;
 
-	mutex_lock(&chip->mutex);
-	ret = get_chip(map, chip, adr, mode);
+	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", __func__, adr,
+		 datum.x[0]);
+
+	ret = do_write_oneword_start(map, chip, adr, mode);
 	if (ret) {
-		mutex_unlock(&chip->mutex);
 		return ret;
 	}
 
-	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n",
-		 __func__, adr, datum.x[0]);
-
-	if (mode == FL_OTP_WRITE)
-		otp_enter(map, chip, adr, map_bankwidth(map));
-
 	/*
 	 * Check for a NOP for the case when the datum to write is already
 	 * present - it saves time and works around buggy chips that corrupt
@@ -1641,8 +1670,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	 */
 	oldd = map_read(map, adr);
 	if (map_word_equal(map, oldd, datum)) {
-		pr_debug("MTD %s(): NOP\n",
-		       __func__);
+		pr_debug("MTD %s(): NOP\n", __func__);
 		goto op_done;
 	}
 
@@ -1670,12 +1698,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	xip_enable(map, chip, adr);
 
  op_done:
-	if (mode == FL_OTP_WRITE)
-		otp_exit(map, chip, adr, map_bankwidth(map));
-	chip->state = FL_READY;
-	DISABLE_VPP(map);
-	put_chip(map, chip, adr);
-	mutex_unlock(&chip->mutex);
+	do_write_oneword_done(map, chip, adr, mode);
 
 	return ret;
 }
-- 
2.18.0

[PATCH v3 07/11] 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 v2:
- Just rebased.

Changes since v1:
- Add the patch.

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

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

[PATCH v3 08/11] 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 v2:
- None.

Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5fb986ea3243..97e5b3948e20 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1679,21 +1679,17 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
- retry:
-	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+	for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
+		ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
+
+		/* Did we succeed? */
+		if (!ret)
+			break;
 
-	/* Did we succeed? */
-	if (ret) {
 		/* reset on all failures. */
 		map_write(map, CMD(0xF0), chip->start);
-		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_RETRIES) {
-			ret = 0;
-			goto retry;
-		}
-
-		ret = -EIO;
+		/* FIXME - should have reset delay before continuing */
 	}
 
 	xip_enable(map, chip, adr);
-- 
2.18.0

[PATCH v3 09/11] 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 v2:
- None.

Changes since v1:
- Add the patch.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 97e5b3948e20..b54599cb6ae5 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1823,6 +1823,27 @@ static int cfi_amdstd_write_words(struct mtd_info *mtd, loff_t to, size_t len,
 	return 0;
 }
 
+static void __xipram do_write_buffer_reset(struct map_info *map,
+					   struct flchip *chip,
+					   struct cfi_private *cfi)
+{
+	/*
+	 * Recovery from write-buffer programming failures requires
+	 * the write-to-buffer-reset sequence.  Since the last part
+	 * of the sequence also works as a normal reset, we can run
+	 * the same commands regardless of why we are here.
+	 * See e.g.
+	 * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
+	 */
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
+			 cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
+			 cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
+			 cfi->device_type, NULL);
+
+	/* FIXME - should have reset delay before continuing */
+}
 
 /*
  * FIXME: interleaved mode not tested, and probably not supported!
@@ -1925,22 +1946,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 	}
 
 	if (ret) {
-		/*
-		 * Recovery from write-buffer programming failures requires
-		 * the write-to-buffer-reset sequence.  Since the last part
-		 * of the sequence also works as a normal reset, we can run
-		 * the same commands regardless of why we are here.
-		 * See e.g.
-		 * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
-		 */
-		cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-				 cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
-				 cfi->device_type, NULL);
-		cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi,
-				 cfi->device_type, NULL);
-		/* FIXME - should have reset delay before continuing */
-
+		do_write_buffer_reset(map, chip, cfi);
 		printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n",
 		       __func__, adr);
 	}
-- 
2.18.0

[PATCH v3 10/11] 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 v2:
- None.

Changes since v1:
- Add the patch.

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

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

[PATCH v3 11/11] mtd: cfi_cmdset_0002: Split do_write_oneword() to reduce exit paths

[Tokunori Ikegami]

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@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 | 39 ++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 0188187939c8..d76af85cfe59 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1643,25 +1643,16 @@ static void __xipram do_write_oneword_done(struct map_info *map,
 	mutex_unlock(&chip->mutex);
 }
 
-static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
-				     unsigned long adr, map_word datum,
-				     int mode)
+static int __xipram do_write_oneword_retry(struct map_info *map,
+					   struct flchip *chip,
+					   unsigned long adr, map_word datum,
+					   int mode)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
 	int ret = 0;
 	map_word oldd;
 	int retry_cnt = 0;
 
-	adr += chip->start;
-
-	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", __func__, adr,
-		 datum.x[0]);
-
-	ret = do_write_oneword_start(map, chip, adr, mode);
-	if (ret) {
-		return ret;
-	}
-
 	/*
 	 * Check for a NOP for the case when the datum to write is already
 	 * present - it saves time and works around buggy chips that corrupt
@@ -1671,7 +1662,6 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	oldd = map_read(map, adr);
 	if (map_word_equal(map, oldd, datum)) {
 		pr_debug("MTD %s(): NOP\n", __func__);
-		do_write_oneword_done(map, chip, adr, mode);
 		return ret;
 	}
 
@@ -1694,6 +1684,27 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 
 	xip_enable(map, chip, adr);
 
+	return ret;
+}
+
+static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
+				     unsigned long adr, map_word datum,
+				     int mode)
+{
+	int ret = 0;
+
+	adr += chip->start;
+
+	pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", __func__, adr,
+		 datum.x[0]);
+
+	ret = do_write_oneword_start(map, chip, adr, mode);
+	if (ret) {
+		return ret;
+	}
+
+	ret = do_write_oneword_retry(map, chip, adr, datum, mode);
+
 	do_write_oneword_done(map, chip, adr, mode);
 
 	return ret;
-- 
2.18.0