[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[Josh Wu]

This patch fix pmecc's read_page() to return maximum number of bitflips, 0 if uncorrectable.

In the commit: 3f91e94f7f511de74c0d2abe08672ccdbdd1961c ("mtd: nand: read_page() returns max_bitflips ()"),
The ecc.read_page() is changed to return the maximum number of bitflips.
And when meet uncorrectable bitflips it needs to return 0.

See the comment in nand.h:
 * @read_page:	function to read a page according to the ECC generator
 *		requirements; returns maximum number of bitflips corrected in
 *		any single ECC step, 0 if bitflips uncorrectable, -EIO hw error

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
change since v1:
  1. add detail commit message for the fix.
  2. return 0 when meet uncorrectable bitflips according to Mike Dunn's suggestion.

 drivers/mtd/nand/atmel_nand.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index c918386..1669d27 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -723,6 +723,7 @@ static int pmecc_correction(struct mtd_info *mtd, u32 pmecc_stat, uint8_t *buf,
 	struct atmel_nand_host *host = nand_chip->priv;
 	int i, err_nbr, eccbytes;
 	uint8_t *buf_pos;
+	int total_err = 0;
 
 	eccbytes = nand_chip->ecc.bytes;
 	for (i = 0; i < eccbytes; i++)
@@ -750,12 +751,13 @@ normal_check:
 				pmecc_correct_data(mtd, buf_pos, ecc, i,
 					host->pmecc_bytes_per_sector, err_nbr);
 				mtd->ecc_stats.corrected += err_nbr;
+				total_err += err_nbr;
 			}
 		}
 		pmecc_stat >>= 1;
 	}
 
-	return 0;
+	return total_err;
 }
 
 static int atmel_nand_pmecc_read_page(struct mtd_info *mtd,
@@ -767,6 +769,7 @@ static int atmel_nand_pmecc_read_page(struct mtd_info *mtd,
 	uint32_t *eccpos = chip->ecc.layout->eccpos;
 	uint32_t stat;
 	unsigned long end_time;
+	int bitflips = 0;
 
 	pmecc_writel(host->ecc, CTRL, PMECC_CTRL_RST);
 	pmecc_writel(host->ecc, CTRL, PMECC_CTRL_DISABLE);
@@ -789,11 +792,14 @@ static int atmel_nand_pmecc_read_page(struct mtd_info *mtd,
 	}
 
 	stat = pmecc_readl_relaxed(host->ecc, ISR);
-	if (stat != 0)
-		if (pmecc_correction(mtd, stat, buf, &oob[eccpos[0]]) != 0)
-			return -EIO;
+	if (stat != 0) {
+		bitflips = pmecc_correction(mtd, stat, buf, &oob[eccpos[0]]);
+		if (bitflips < 0)
+			/* uncorrectable errors */
+			return 0;
+	}
 
-	return 0;
+	return bitflips;
 }
 
 static int atmel_nand_pmecc_write_page(struct mtd_info *mtd,
-- 
1.7.9.5

[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[Mike Dunn]

On 11/27/2012 02:50 AM, Josh Wu wrote:
> This patch fix pmecc's read_page() to return maximum number of bitflips, 0 if uncorrectable.
> 
> In the commit: 3f91e94f7f511de74c0d2abe08672ccdbdd1961c ("mtd: nand: read_page() returns max_bitflips ()"),
> The ecc.read_page() is changed to return the maximum number of bitflips.
> And when meet uncorrectable bitflips it needs to return 0.
> 
> See the comment in nand.h:
>  * @read_page:	function to read a page according to the ECC generator
>  *		requirements; returns maximum number of bitflips corrected in
>  *		any single ECC step, 0 if bitflips uncorrectable, -EIO hw error
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> change since v1:
>   1. add detail commit message for the fix.
>   2. return 0 when meet uncorrectable bitflips according to Mike Dunn's suggestion.


Reviewed-by: Mike Dunn <mikedunn@newsguy.com>

I see now the pmecc controller patch in the git log.  Nice work.

Mike

[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[Mike Dunn]

On 11/27/2012 10:59 AM, Mike Dunn wrote:
> On 11/27/2012 02:50 AM, Josh Wu wrote:
>> This patch fix pmecc's read_page() to return maximum number of bitflips, 0 if uncorrectable.
>>
>> In the commit: 3f91e94f7f511de74c0d2abe08672ccdbdd1961c ("mtd: nand: read_page() returns max_bitflips ()"),
>> The ecc.read_page() is changed to return the maximum number of bitflips.
>> And when meet uncorrectable bitflips it needs to return 0.
>>
>> See the comment in nand.h:
>>  * @read_page:	function to read a page according to the ECC generator
>>  *		requirements; returns maximum number of bitflips corrected in
>>  *		any single ECC step, 0 if bitflips uncorrectable, -EIO hw error
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> change since v1:
>>   1. add detail commit message for the fix.
>>   2. return 0 when meet uncorrectable bitflips according to Mike Dunn's suggestion.
> 
> 
> Reviewed-by: Mike Dunn <mikedunn@newsguy.com>
> 
> I see now the pmecc controller patch in the git log.  Nice work.


BTW, with such a wide range for ecc strength - up to 24 bits, according to the
commit message for the pmecc patch - you may want to think about setting an
appropriate bitflip_threshold in the driver.  I'm not a nand expert, and I don't
know much about the atmel_nand specifically, but I would think that if 23 bits
are corrected on a page of size 2k (or less), maybe a return code of -EUCLEAN
from mtd_read() might be appropriate.

Mike

[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[Josh Wu]

Hi, Mike

On 11/28/2012 3:35 AM, Mike Dunn wrote:
> On 11/27/2012 10:59 AM, Mike Dunn wrote:
>> On 11/27/2012 02:50 AM, Josh Wu wrote:
>>> This patch fix pmecc's read_page() to return maximum number of bitflips, 0 if uncorrectable.
>>>
>>> In the commit: 3f91e94f7f511de74c0d2abe08672ccdbdd1961c ("mtd: nand: read_page() returns max_bitflips ()"),
>>> The ecc.read_page() is changed to return the maximum number of bitflips.
>>> And when meet uncorrectable bitflips it needs to return 0.
>>>
>>> See the comment in nand.h:
>>>   * @read_page:	function to read a page according to the ECC generator
>>>   *		requirements; returns maximum number of bitflips corrected in
>>>   *		any single ECC step, 0 if bitflips uncorrectable, -EIO hw error
>>>
>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---
>>> change since v1:
>>>    1. add detail commit message for the fix.
>>>    2. return 0 when meet uncorrectable bitflips according to Mike Dunn's suggestion.
>>
>> Reviewed-by: Mike Dunn <mikedunn@newsguy.com>
>>
>> I see now the pmecc controller patch in the git log.  Nice work.
>
> BTW, with such a wide range for ecc strength - up to 24 bits, according to the
> commit message for the pmecc patch - you may want to think about setting an
> appropriate bitflip_threshold in the driver.  I'm not a nand expert, and I don't
> know much about the atmel_nand specifically, but I would think that if 23 bits
> are corrected on a page of size 2k (or less), maybe a return code of -EUCLEAN
> from mtd_read() might be appropriate.

After checking the nand_base.c, I saw it will set the 
mtd.bitflip_threshold to mtd->ecc.strength during nand_scan_tail().
in the atmel_nand code, the ecc strength will be set correctly, that 
means bitflip_threashold should be set up correctly by default.
so I think I don't need set up the the bitflip_threshold anymore if I 
set ecc strength correctly. Am I missing any point here?

Thanks,
Josh Wu

>
> Mike

[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[Mike Dunn]

On 11/28/2012 11:45 PM, Josh Wu wrote:
> Hi, Mike
> 
> On 11/28/2012 3:35 AM, Mike Dunn wrote:
>>
>> BTW, with such a wide range for ecc strength - up to 24 bits, according to the
>> commit message for the pmecc patch - you may want to think about setting an
>> appropriate bitflip_threshold in the driver.  I'm not a nand expert, and I don't
>> know much about the atmel_nand specifically, but I would think that if 23 bits
>> are corrected on a page of size 2k (or less), maybe a return code of -EUCLEAN
>> from mtd_read() might be appropriate.
> 
> After checking the nand_base.c, I saw it will set the mtd.bitflip_threshold to
> mtd->ecc.strength during nand_scan_tail().
> in the atmel_nand code, the ecc strength will be set correctly, that means
> bitflip_threashold should be set up correctly by default.
> so I think I don't need set up the the bitflip_threshold anymore if I set ecc
> strength correctly. Am I missing any point here?


No, you are correct; bitflip_threshold is set to ecc.strength by default if the
driver has not assigned it a value already.  I just wondered if the default
value is too liberal given the possibility of such high ecc strength in your
case.  With bitflip_threshold == 24 (default for the greatest possible
ecc.strength on atmel_nand with pmecc) mtd_read() will not return -EUCLEAN until
24 bitflips are corrected on a page that is no greater than 2k in size, and may
be as small as 512 bytes (if I read the code correctly).

The -EUCLEAN return code is used by higher layers as an indication that a block
may be going bad.  I don't know whether or not a block bears scrutiny when 23
bitflips are corrected on a page.  I guess it depends on the particular flash
device.  It was just a thought.

Hope this makes sense.
Mike

[PATCH v2] MTD: at91: atmel_nand: return bit flips for the PMECC read_page()

[Artem Bityutskiy]

On Tue, 2012-11-27 at 18:50 +0800, Josh Wu wrote:
> This patch fix pmecc's read_page() to return maximum number of bitflips, 0 if uncorrectable.
> 
> In the commit: 3f91e94f7f511de74c0d2abe08672ccdbdd1961c ("mtd: nand: read_page() returns max_bitflips ()"),
> The ecc.read_page() is changed to return the maximum number of bitflips.
> And when meet uncorrectable bitflips it needs to return 0.
> 
> See the comment in nand.h:
>  * @read_page:	function to read a page according to the ECC generator
>  *		requirements; returns maximum number of bitflips corrected in
>  *		any single ECC step, 0 if bitflips uncorrectable, -EIO hw error
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121203/3dc8bf5f/attachment.sig>