mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[IKEGAMI Tokunori]

Hi,

Let us consult to change mtd cfi_cmdset_0002 to read data 3 times as below patch.
Can we change the mtd cfi_cmdset_0002 driver like this?
If any comment or concern please let us know.

>From d924822c996b9c0eccc815e5018a0c3ea6077137 Mon Sep 17 00:00:00 2001
From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Date: Tue, 24 Apr 2018 13:26:37 +0900
Subject: [PATCH 2/2] mtd: cfi_cmdset_0002: Read data 3 times to check write
 operation status

Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6 toggles.
Actually the read data is sometimes changed by the 3rd reading.
Also this is caused on other flash device also.
The flash write failure is possible to be caused by the error.
To resolve the issue change the read number of times to 3 from 2.
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 56aa6b75213d..dc7667ff134a 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -743,12 +743,13 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
  */
 static int __xipram chip_ready(struct map_info *map, unsigned long addr)
 {
-	map_word d, t;
+	map_word r1, r2, r3;
 
-	d = map_read(map, addr);
-	t = map_read(map, addr);
+	r1 = map_read(map, addr);
+	r2 = map_read(map, addr);
+	r3 = map_read(map, addr);
 
-	return map_word_equal(map, d, t);
+	return map_word_equal(map, r1, r2) && map_word_equal(map, r2, r3);
 }
 
 /*
-- 
2.16.1

Regards,
Ikegami

Re: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[Joakim Tjernlund]

On Tue, 2018-04-24 at 05:54 +0000, IKEGAMI Tokunori wrote:
> 
> Hi,
> 
> Let us consult to change mtd cfi_cmdset_0002 to read data 3 times as below patch.
> Can we change the mtd cfi_cmdset_0002 driver like this?
> If any comment or concern please let us know.
> 
> From d924822c996b9c0eccc815e5018a0c3ea6077137 Mon Sep 17 00:00:00 2001
> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Date: Tue, 24 Apr 2018 13:26:37 +0900
> Subject: [PATCH 2/2] mtd: cfi_cmdset_0002: Read data 3 times to check write
>  operation status
> 
> Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6 toggles.
> Actually the read data is sometimes changed by the 3rd reading.
> Also this is caused on other flash device also.
> The flash write failure is possible to be caused by the error.
> To resolve the issue change the read number of times to 3 from 2.

Hi 

I find this a bit odd. I am looking at some other erase suspend problems we
see on Cypress AMD NOR flash and I have noted that there is more than just DQ6 toggling.
Checking DQ5 in combination with DQ6 is one.
How did you you arrive at reading 3 times is correct? Is the any documentation
supporting this?

All in all, error handling in cmdset0002 is lacking.

 Jocke

RE: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[IKEGAMI Tokunori]

Hi Jocke-san,

Thanks for your comment.

> I find this a bit odd. I am looking at some other erase suspend problems we
> see on Cypress AMD NOR flash and I have noted that there is more than just DQ6 toggling.
> Checking DQ5 in combination with DQ6 is one.

  By the way I have also found only once do_erase_oneblock() was failed by the timeout on the write failure situation.
  So I thought that the error also can be resolved by the 3 times reading for checking chip_ready().

> How did you you arrive at reading 3 times is correct? Is the any documentation
> supporting this?

  About the 3 times reading is described by the page 32 on following Cypress S29GLxxxP flash data sheet.
    <http://www.cypress.com/file/219926/download>

> All in all, error handling in cmdset0002 is lacking.

  I have understood as that it is for example FIXME parts for the error case in cfi_cmdset_0002.
  Is this correct?

Regards,
Ikegami

-----Original Message-----
From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com] 
Sent: Tuesday, April 24, 2018 8:05 PM
To: IKEGAMI Tokunori; linux-mtd@lists.infradead.org
Cc: PACKHAM Chris
Subject: Re: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

On Tue, 2018-04-24 at 05:54 +0000, IKEGAMI Tokunori wrote:
> 
> Hi,
> 
> Let us consult to change mtd cfi_cmdset_0002 to read data 3 times as below patch.
> Can we change the mtd cfi_cmdset_0002 driver like this?
> If any comment or concern please let us know.
> 
> From d924822c996b9c0eccc815e5018a0c3ea6077137 Mon Sep 17 00:00:00 2001
> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Date: Tue, 24 Apr 2018 13:26:37 +0900
> Subject: [PATCH 2/2] mtd: cfi_cmdset_0002: Read data 3 times to check write
>  operation status
> 
> Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6 toggles.
> Actually the read data is sometimes changed by the 3rd reading.
> Also this is caused on other flash device also.
> The flash write failure is possible to be caused by the error.
> To resolve the issue change the read number of times to 3 from 2.

Hi 

I find this a bit odd. I am looking at some other erase suspend problems we
see on Cypress AMD NOR flash and I have noted that there is more than just DQ6 toggling.
Checking DQ5 in combination with DQ6 is one.
How did you you arrive at reading 3 times is correct? Is the any documentation
supporting this?

All in all, error handling in cmdset0002 is lacking.

 Jocke

Re: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[Joakim Tjernlund]

On Tue, 2018-04-24 at 14:08 +0000, IKEGAMI Tokunori 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.
> 
> 
> Hi Jocke-san,
> 
> Thanks for your comment.
> 
> > I find this a bit odd. I am looking at some other erase suspend problems we
> > see on Cypress AMD NOR flash and I have noted that there is more than just DQ6 toggling.
> > Checking DQ5 in combination with DQ6 is one.
> 
>   By the way I have also found only once do_erase_oneblock() was failed by the timeout on the write failure situation.
>   So I thought that the error also can be resolved by the 3 times reading for checking chip_ready().

We have see several times that too many Erase suspend for one block will cause 5.5.2.6 DQ5: Exceeded Timing
Limits
This condition is not checked for nor handled in any way.


> 
> > How did you you arrive at reading 3 times is correct? Is the any documentation
> > supporting this?
> 
>   About the 3 times reading is described by the page 32 on following Cypress S29GLxxxP flash data sheet.
>     <http://www.cypress.com/file/219926/download>

I don't have such info for my chip at page 32, I am looking at S29GL01GS/S29GL512S
S29GL256S/S29GL128S from http://www.cypress.com/file/177976/download

I am looking at chap 5.5.2.5 Reading Toggle Bits DQ6/DQ2

> 
> > All in all, error handling in cmdset0002 is lacking.
> 
>   I have understood as that it is for example FIXME parts for the error case in cfi_cmdset_0002.
>   Is this correct?

Haven't looked at all the FIXME's but I am looking are how erase suspend failure are handled

> 
> Regards,
> Ikegami
> 
> -----Original Message-----
> From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> Sent: Tuesday, April 24, 2018 8:05 PM
> To: IKEGAMI Tokunori; linux-mtd@lists.infradead.org
> Cc: PACKHAM Chris
> Subject: Re: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status
> 
> On Tue, 2018-04-24 at 05:54 +0000, IKEGAMI Tokunori wrote:
> > 
> > Hi,
> > 
> > Let us consult to change mtd cfi_cmdset_0002 to read data 3 times as below patch.
> > Can we change the mtd cfi_cmdset_0002 driver like this?
> > If any comment or concern please let us know.
> > 
> > From d924822c996b9c0eccc815e5018a0c3ea6077137 Mon Sep 17 00:00:00 2001
> > From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Date: Tue, 24 Apr 2018 13:26:37 +0900
> > Subject: [PATCH 2/2] mtd: cfi_cmdset_0002: Read data 3 times to check write
> >  operation status
> > 
> > Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6 toggles.
> > Actually the read data is sometimes changed by the 3rd reading.
> > Also this is caused on other flash device also.
> > The flash write failure is possible to be caused by the error.
> > To resolve the issue change the read number of times to 3 from 2.
> 
> Hi
> 
> I find this a bit odd. I am looking at some other erase suspend problems we
> see on Cypress AMD NOR flash and I have noted that there is more than just DQ6 toggling.
> Checking DQ5 in combination with DQ6 is one.
> How did you you arrive at reading 3 times is correct? Is the any documentation
> supporting this?
> 
> All in all, error handling in cmdset0002 is lacking.
> 
>  Jocke
> 

RE: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[IKEGAMI Tokunori]

> -----Original Message-----
> From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> Sent: Wednesday, April 25, 2018 2:22 AM
> To: IKEGAMI Tokunori; linux-mtd@lists.infradead.org
> Cc: PACKHAM Chris
> Subject: Re: mtd: cfi_cmdset_0002: Read data 3 times to check write
> operation status
> 
> On Tue, 2018-04-24 at 14:08 +0000, IKEGAMI Tokunori 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.
> >
> >
> > Hi Jocke-san,
> >
> > Thanks for your comment.
> >
> > > I find this a bit odd. I am looking at some other erase suspend problems
> we
> > > see on Cypress AMD NOR flash and I have noted that there is more than
> just DQ6 toggling.
> > > Checking DQ5 in combination with DQ6 is one.
> >
> >   By the way I have also found only once do_erase_oneblock() was failed
> by the timeout on the write failure situation.
> >   So I thought that the error also can be resolved by the 3 times reading
> for checking chip_ready().
> 
> We have see several times that too many Erase suspend for one block will
> cause 5.5.2.6 DQ5: Exceeded Timing
> Limits
> This condition is not checked for nor handled in any way.

I have just checked the error in the past again.
This was caused by the error case of chip_good() in the do_erase_oneblock().
But it was not caused by the error case of chip_ready() actually.
So it seems a similar case but the flash device is different.
I have confirmed it on the MACRONIX flash device MX29GL512FHT2I-11G.
But the issue was not reproduced at that time.
I thought that the issue is also resolved by the change to read 3 times but not sure.

> >
> > > How did you you arrive at reading 3 times is correct? Is the any
> documentation
> > > supporting this?
> >
> >   About the 3 times reading is described by the page 32 on following
> Cypress S29GLxxxP flash data sheet.
> >     <http://www.cypress.com/file/219926/download>
> 
> I don't have such info for my chip at page 32, I am looking at
> S29GL01GS/S29GL512S
> S29GL256S/S29GL128S from http://www.cypress.com/file/177976/download
> 
> I am looking at chap 5.5.2.5 Reading Toggle Bits DQ6/DQ2

Yes it is described by the Cypress S29GLxxxP flash data sheet only as far as I know.
Also it is not described by the MACRONIX flash device data sheet.
But actually I have confirmed that the 3rd time read data was changed on the MACRONIX flash device also.
Also if the bus timing between CPU and flash was changed by enabling ExternalSync mode on our CPU then the both Cypress and MACRONIX devices are failed to write.
  Note: This is caused on other alternative flash devices on our products.
So it seems that it is better to be changed to read 3 times for all flash devices supported by the cfi_cmdset_0002 driver.

> >
> > > All in all, error handling in cmdset0002 is lacking.
> >
> >   I have understood as that it is for example FIXME parts for the error
> case in cfi_cmdset_0002.
> >   Is this correct?
> 
> Haven't looked at all the FIXME's but I am looking are how erase suspend
> failure are handled

  I see and noted it.

> >
> > Regards,
> > Ikegami
> >
> > -----Original Message-----
> > From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com]
> > Sent: Tuesday, April 24, 2018 8:05 PM
> > To: IKEGAMI Tokunori; linux-mtd@lists.infradead.org
> > Cc: PACKHAM Chris
> > Subject: Re: mtd: cfi_cmdset_0002: Read data 3 times to check write
> operation status
> >
> > On Tue, 2018-04-24 at 05:54 +0000, IKEGAMI Tokunori wrote:
> > >
> > > Hi,
> > >
> > > Let us consult to change mtd cfi_cmdset_0002 to read data 3 times as
> below patch.
> > > Can we change the mtd cfi_cmdset_0002 driver like this?
> > > If any comment or concern please let us know.
> > >
> > > From d924822c996b9c0eccc815e5018a0c3ea6077137 Mon Sep 17 00:00:00 2001
> > > From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > > Date: Tue, 24 Apr 2018 13:26:37 +0900
> > > Subject: [PATCH 2/2] mtd: cfi_cmdset_0002: Read data 3 times to check
> write
> > >  operation status
> > >
> > > Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6
> toggles.
> > > Actually the read data is sometimes changed by the 3rd reading.
> > > Also this is caused on other flash device also.
> > > The flash write failure is possible to be caused by the error.
> > > To resolve the issue change the read number of times to 3 from 2.
> >
> > Hi
> >
> > I find this a bit odd. I am looking at some other erase suspend problems
> we
> > see on Cypress AMD NOR flash and I have noted that there is more than
> just DQ6 toggling.
> > Checking DQ5 in combination with DQ6 is one.
> > How did you you arrive at reading 3 times is correct? Is the any
> documentation
> > supporting this?
> >
> > All in all, error handling in cmdset0002 is lacking.
> >
> >  Jocke
> >

[PATCH] mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[smtpuser]

From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>

Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6 toggles.
Actually the read data is sometimes changed by the 3rd reading.
Also this is caused on other flash device also.
The flash write failure is possible to be caused by the error.
To resolve the issue change the read number of times to 3 from 2.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
---
I have just updated the patch to add Signed-off-by and Cc lines.
This patch will be sent by git-send-email.
Also for our company mail system the sender mail address is needed to be set as smtpuser <smtpuser@allied-telesis.co.jp>.
But do not reply to the email address smtpuser <smtpuser@allied-telesis.co.jp>.
Please reply to my email address Tokunori Ikegami <ikegami@allied-telesis.co.jp> if any comemnt or problem.
Sorry for inconvinient about this.

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

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 56aa6b75213d..dc7667ff134a 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -743,12 +743,13 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
  */
 static int __xipram chip_ready(struct map_info *map, unsigned long addr)
 {
-	map_word d, t;
+	map_word r1, r2, r3;
 
-	d = map_read(map, addr);
-	t = map_read(map, addr);
+	r1 = map_read(map, addr);
+	r2 = map_read(map, addr);
+	r3 = map_read(map, addr);
 
-	return map_word_equal(map, d, t);
+	return map_word_equal(map, r1, r2) && map_word_equal(map, r2, r3);
 }
 
 /*
-- 
2.16.1

Re: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[Boris Brezillon]

On Tue, 24 Apr 2018 05:54:43 +0000
IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:

> Hi,
> 
> Let us consult to change mtd cfi_cmdset_0002 to read data 3 times as below patch.
> Can we change the mtd cfi_cmdset_0002 driver like this?
> If any comment or concern please let us know.
> 
> From d924822c996b9c0eccc815e5018a0c3ea6077137 Mon Sep 17 00:00:00 2001
> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Date: Tue, 24 Apr 2018 13:26:37 +0900
> Subject: [PATCH 2/2] mtd: cfi_cmdset_0002: Read data 3 times to check write
>  operation status
> 
> Cypress S29GLxxxP flash is needed to read data 3 times to check DQ6 toggles.
> Actually the read data is sometimes changed by the 3rd reading.
> Also this is caused on other flash device also.
> The flash write failure is possible to be caused by the error.
> To resolve the issue change the read number of times to 3 from 2.

Is this patch still needed, or is [1] supposed to replace it?

[1]http://patchwork.ozlabs.org/patch/910857/

RE: mtd: cfi_cmdset_0002: Read data 3 times to check write operation status

[IKEGAMI Tokunori]

Hi Boris-san,

> Is this patch still needed, or is [1] supposed to replace it?
> 
> [1]http://patchwork.ozlabs.org/patch/910857/

  No this patch is not needed and as you mentioned the patch [1] is better than this I think now.

Regards,
Ikegami