[PATCH 1/2] mxc_nand: fix err_limit and err_mask

[PATCH 1/2] mxc_nand: fix err_limit and err_mask

[Michael Grzeschik]

This patch fixes the error detection limits for the used
eccsize of the 1, 4 and 8 bit eccmode.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index cb6c845..7fd495e 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -657,10 +657,14 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 	u32 ecc_stat, err;
 	int no_subpages = 1;
 	int ret = 0;
-	u8 ecc_bit_mask, err_limit;
+	u8 ecc_bit_mask, err_limit = 0x1;
 
-	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
-	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
+	ecc_bit_mask = 0xf;
+
+	if (host->eccsize == 4)
+		err_limit = 0x2;
+	else if (host->eccsize == 8)
+		err_limit = 0x4;
 
 	no_subpages = mtd->writesize >> 9;
 
-- 
2.0.0

[PATCH 2/2] mxc_nand: use our own read_page function

[Michael Grzeschik]

The current approach of the read_page function is to iterate over all
subpages and call the correct_data function. The correct_data function
currently does the same. It iterates over all subpages and checks for
correctable and uncorrectable data. This redundant call for each
subpage leads to miscalculations.

This patch changes the driver to use its own read_page function in which
we call the correct_data function only once per page. With that we do
the failure and correct statistics counting inside this function.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 7fd495e..09e3682 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -141,6 +141,8 @@ struct mxc_nand_host;
 
 struct mxc_nand_devtype_data {
 	void (*preset)(struct mtd_info *);
+	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
+			uint8_t *buf, int oob_required, int page);
 	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_page)(struct mtd_info *, unsigned int);
@@ -649,6 +651,59 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
 	return 0;
 }
 
+/**
+ * mxc_nand_read_page_hwecc_v2_v3 - [REPLACEABLE] hardware ECC based page read function
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @buf: buffer to store read data
+ * @oob_required: caller requires OOB data read to chip->oob_poi
+ * @page: page number to read
+ *
+ * Not for syndrome calculating ECC controllers which need a special oob layout.
+ */
+static int
+mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
+			       struct nand_chip *chip,
+			       uint8_t *buf, int oob_required, int page)
+{
+	int i, eccsize = chip->ecc.size;
+	struct nand_chip *nand_chip = mtd->priv;
+	struct mxc_nand_host *host = nand_chip->priv;
+	int eccbytes = chip->ecc.bytes;
+	int eccsteps = chip->ecc.steps;
+	uint8_t *p = buf;
+	uint8_t *ecc_calc = chip->buffers->ecccalc;
+	uint8_t *ecc_code = chip->buffers->ecccode;
+	uint32_t *eccpos = chip->ecc.layout->eccpos;
+	unsigned int max_bitflips = 0;
+	u32 ecc_stat, err;
+	int stat;
+
+	ecc_stat = host->devtype_data->get_ecc_status(host);
+	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+		err = ecc_stat & 0xf;
+		chip->ecc.hwctl(mtd, NAND_ECC_READ);
+		chip->read_buf(mtd, p, eccsize);
+		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
+		ecc_stat >>= 4;
+
+	}
+	ecc_stat = host->devtype_data->get_ecc_status(host);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	for (i = 0; i < chip->ecc.total; i++)
+		ecc_code[i] = chip->oob_poi[eccpos[i]];
+
+	eccsteps = chip->ecc.steps;
+	p = buf;
+
+	stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
+	max_bitflips = max_t(unsigned int, max_bitflips, stat);
+
+	return max_bitflips;
+}
+
+
 static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 				 u_char *read_ecc, u_char *calc_ecc)
 {
@@ -656,7 +711,7 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 	struct mxc_nand_host *host = nand_chip->priv;
 	u32 ecc_stat, err;
 	int no_subpages = 1;
-	int ret = 0;
+	int ret = 0, broken = 0;
 	u8 ecc_bit_mask, err_limit = 0x1;
 
 	ecc_bit_mask = 0xf;
@@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 	do {
 		err = ecc_stat & ecc_bit_mask;
 		if (err > err_limit) {
-			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
-			return -1;
+			broken++;
 		} else {
 			ret += err;
 		}
 		ecc_stat >>= 4;
 	} while (--no_subpages);
 
-	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
+	mtd->ecc_stats.corrected += ret;
+	if (ret)
+		printk("%d Symbol Correctable RS-ECC Error\n", ret);
+
+	mtd->ecc_stats.failed += broken;
+	if (broken)
+		printk(KERN_WARNING "%d Symbol UnCorrectable RS-ECC Error\n",
+			 broken);
 
 	return ret;
 }
@@ -1244,6 +1305,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
 /* v21: i.MX25, i.MX35 */
 static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 	.preset = preset_v2,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v1_v2,
 	.send_addr = send_addr_v1_v2,
 	.send_page = send_page_v2,
@@ -1270,6 +1332,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 /* v3.2a: i.MX51 */
 static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 	.preset = preset_v3,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1297,6 +1360,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 /* v3.2b: i.MX53 */
 static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
 	.preset = preset_v3,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1517,6 +1581,7 @@ static int mxcnd_probe(struct platform_device *pdev)
 	this->ecc.layout = host->devtype_data->ecclayout_512;
 
 	if (host->pdata.hw_ecc) {
+		this->ecc.read_page = host->devtype_data->read_page;
 		this->ecc.calculate = mxc_nand_calculate_ecc;
 		this->ecc.hwctl = mxc_nand_enable_hwecc;
 		this->ecc.correct = host->devtype_data->correct_data;
-- 
2.0.0

Re: [PATCH 2/2] mxc_nand: use our own read_page function

[Michael Grzeschik]

On Thu, Jun 26, 2014 at 10:42:48PM +0200, Michael Grzeschik wrote:
> The current approach of the read_page function is to iterate over all
> subpages and call the correct_data function. The correct_data function
> currently does the same. It iterates over all subpages and checks for
> correctable and uncorrectable data. This redundant call for each
> subpage leads to miscalculations.
> 
> This patch changes the driver to use its own read_page function in which
> we call the correct_data function only once per page. With that we do
> the failure and correct statistics counting inside this function.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 7fd495e..09e3682 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c

[snip]

> @@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	do {
>  		err = ecc_stat & ecc_bit_mask;
>  		if (err > err_limit) {
> -			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
> -			return -1;
> +			broken++;
>  		} else {
>  			ret += err;
>  		}
>  		ecc_stat >>= 4;
>  	} while (--no_subpages);
>  
> -	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> +	mtd->ecc_stats.corrected += ret;
> +	if (ret)
> +		printk("%d Symbol Correctable RS-ECC Error\n", ret);

This was ment to be pr_debug as before.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATH v2] mxc_nand: use our own read_page function

[Michael Grzeschik]

The current approach of the read_page function is to iterate over all
subpages and call the correct_data function. The correct_data function
currently does the same. It iterates over all subpages and checks for
correctable and uncorrectable data. This redundant call for each
subpage leads to miscalculations.

This patch changes the driver to use its own read_page function in which
we call the correct_data function only once per page. With that we do
the failure and correct statistics counting inside this function.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
fixed printk to pr_debug

 drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index a72d508..5f9e36d 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -141,6 +141,8 @@ struct mxc_nand_host;
 
 struct mxc_nand_devtype_data {
 	void (*preset)(struct mtd_info *);
+	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
+			uint8_t *buf, int oob_required, int page);
 	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_page)(struct mtd_info *, unsigned int);
@@ -649,6 +651,59 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
 	return 0;
 }
 
+/**
+ * mxc_nand_read_page_hwecc_v2_v3 - [REPLACEABLE] hardware ECC based page read function
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @buf: buffer to store read data
+ * @oob_required: caller requires OOB data read to chip->oob_poi
+ * @page: page number to read
+ *
+ * Not for syndrome calculating ECC controllers which need a special oob layout.
+ */
+static int
+mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
+			       struct nand_chip *chip,
+			       uint8_t *buf, int oob_required, int page)
+{
+	int i, eccsize = chip->ecc.size;
+	struct nand_chip *nand_chip = mtd->priv;
+	struct mxc_nand_host *host = nand_chip->priv;
+	int eccbytes = chip->ecc.bytes;
+	int eccsteps = chip->ecc.steps;
+	uint8_t *p = buf;
+	uint8_t *ecc_calc = chip->buffers->ecccalc;
+	uint8_t *ecc_code = chip->buffers->ecccode;
+	uint32_t *eccpos = chip->ecc.layout->eccpos;
+	unsigned int max_bitflips = 0;
+	u32 ecc_stat, err;
+	int stat;
+
+	ecc_stat = host->devtype_data->get_ecc_status(host);
+	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+		err = ecc_stat & 0xf;
+		chip->ecc.hwctl(mtd, NAND_ECC_READ);
+		chip->read_buf(mtd, p, eccsize);
+		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
+		ecc_stat >>= 4;
+
+	}
+	ecc_stat = host->devtype_data->get_ecc_status(host);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	for (i = 0; i < chip->ecc.total; i++)
+		ecc_code[i] = chip->oob_poi[eccpos[i]];
+
+	eccsteps = chip->ecc.steps;
+	p = buf;
+
+	stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
+	max_bitflips = max_t(unsigned int, max_bitflips, stat);
+
+	return max_bitflips;
+}
+
+
 static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 				 u_char *read_ecc, u_char *calc_ecc)
 {
@@ -656,7 +711,7 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 	struct mxc_nand_host *host = nand_chip->priv;
 	u32 ecc_stat, err;
 	int no_subpages = 1;
-	int ret = 0;
+	int ret = 0, broken = 0;
 	u8 ecc_bit_mask, err_limit = 0x1;
 
 	ecc_bit_mask = 0xf;
@@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 	do {
 		err = ecc_stat & ecc_bit_mask;
 		if (err > err_limit) {
-			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
-			return -1;
+			broken++;
 		} else {
 			ret += err;
 		}
 		ecc_stat >>= 4;
 	} while (--no_subpages);
 
-	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
+	mtd->ecc_stats.corrected += ret;
+	if (ret)
+		pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
+
+	mtd->ecc_stats.failed += broken;
+	if (broken)
+		printk(KERN_WARNING "%d Symbol UnCorrectable RS-ECC Error\n",
+			 broken);
 
 	return ret;
 }
@@ -1216,6 +1277,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
 /* v21: i.MX25, i.MX35 */
 static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 	.preset = preset_v2,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v1_v2,
 	.send_addr = send_addr_v1_v2,
 	.send_page = send_page_v2,
@@ -1242,6 +1304,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 /* v3.2a: i.MX51 */
 static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 	.preset = preset_v3,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1269,6 +1332,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 /* v3.2b: i.MX53 */
 static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
 	.preset = preset_v3,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1483,6 +1547,7 @@ static int mxcnd_probe(struct platform_device *pdev)
 	this->ecc.layout = host->devtype_data->ecclayout_512;
 
 	if (host->pdata.hw_ecc) {
+		this->ecc.read_page = host->devtype_data->read_page;
 		this->ecc.calculate = mxc_nand_calculate_ecc;
 		this->ecc.hwctl = mxc_nand_enable_hwecc;
 		this->ecc.correct = host->devtype_data->correct_data;
-- 
2.0.0

Re: [PATH v2] mxc_nand: use our own read_page function

[Michael Grzeschik]

Ping!

On Fri, Jun 27, 2014 at 12:38:44PM +0200, Michael Grzeschik wrote:
> The current approach of the read_page function is to iterate over all
> subpages and call the correct_data function. The correct_data function
> currently does the same. It iterates over all subpages and checks for
> correctable and uncorrectable data. This redundant call for each
> subpage leads to miscalculations.
> 
> This patch changes the driver to use its own read_page function in which
> we call the correct_data function only once per page. With that we do
> the failure and correct statistics counting inside this function.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> fixed printk to pr_debug
> 
>  drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index a72d508..5f9e36d 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -141,6 +141,8 @@ struct mxc_nand_host;
>  
>  struct mxc_nand_devtype_data {
>  	void (*preset)(struct mtd_info *);
> +	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> +			uint8_t *buf, int oob_required, int page);
>  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_page)(struct mtd_info *, unsigned int);
> @@ -649,6 +651,59 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
>  	return 0;
>  }
>  
> +/**
> + * mxc_nand_read_page_hwecc_v2_v3 - [REPLACEABLE] hardware ECC based page read function
> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @buf: buffer to store read data
> + * @oob_required: caller requires OOB data read to chip->oob_poi
> + * @page: page number to read
> + *
> + * Not for syndrome calculating ECC controllers which need a special oob layout.
> + */
> +static int
> +mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> +			       struct nand_chip *chip,
> +			       uint8_t *buf, int oob_required, int page)
> +{
> +	int i, eccsize = chip->ecc.size;
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	uint8_t *p = buf;
> +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> +	uint8_t *ecc_code = chip->buffers->ecccode;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	unsigned int max_bitflips = 0;
> +	u32 ecc_stat, err;
> +	int stat;
> +
> +	ecc_stat = host->devtype_data->get_ecc_status(host);
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +		err = ecc_stat & 0xf;
> +		chip->ecc.hwctl(mtd, NAND_ECC_READ);
> +		chip->read_buf(mtd, p, eccsize);
> +		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> +		ecc_stat >>= 4;
> +
> +	}
> +	ecc_stat = host->devtype_data->get_ecc_status(host);
> +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +
> +	for (i = 0; i < chip->ecc.total; i++)
> +		ecc_code[i] = chip->oob_poi[eccpos[i]];
> +
> +	eccsteps = chip->ecc.steps;
> +	p = buf;
> +
> +	stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> +	max_bitflips = max_t(unsigned int, max_bitflips, stat);
> +
> +	return max_bitflips;
> +}
> +
> +
>  static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  				 u_char *read_ecc, u_char *calc_ecc)
>  {
> @@ -656,7 +711,7 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	struct mxc_nand_host *host = nand_chip->priv;
>  	u32 ecc_stat, err;
>  	int no_subpages = 1;
> -	int ret = 0;
> +	int ret = 0, broken = 0;
>  	u8 ecc_bit_mask, err_limit = 0x1;
>  
>  	ecc_bit_mask = 0xf;
> @@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	do {
>  		err = ecc_stat & ecc_bit_mask;
>  		if (err > err_limit) {
> -			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
> -			return -1;
> +			broken++;
>  		} else {
>  			ret += err;
>  		}
>  		ecc_stat >>= 4;
>  	} while (--no_subpages);
>  
> -	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> +	mtd->ecc_stats.corrected += ret;
> +	if (ret)
> +		pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> +
> +	mtd->ecc_stats.failed += broken;
> +	if (broken)
> +		printk(KERN_WARNING "%d Symbol UnCorrectable RS-ECC Error\n",
> +			 broken);
>  
>  	return ret;
>  }
> @@ -1216,6 +1277,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
>  /* v21: i.MX25, i.MX35 */
>  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  	.preset = preset_v2,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v1_v2,
>  	.send_addr = send_addr_v1_v2,
>  	.send_page = send_page_v2,
> @@ -1242,6 +1304,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  /* v3.2a: i.MX51 */
>  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  	.preset = preset_v3,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1269,6 +1332,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  /* v3.2b: i.MX53 */
>  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
>  	.preset = preset_v3,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1483,6 +1547,7 @@ static int mxcnd_probe(struct platform_device *pdev)
>  	this->ecc.layout = host->devtype_data->ecclayout_512;
>  
>  	if (host->pdata.hw_ecc) {
> +		this->ecc.read_page = host->devtype_data->read_page;
>  		this->ecc.calculate = mxc_nand_calculate_ecc;
>  		this->ecc.hwctl = mxc_nand_enable_hwecc;
>  		this->ecc.correct = host->devtype_data->correct_data;
> -- 
> 2.0.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATH v2] mxc_nand: use our own read_page function

[Michael Grzeschik]

@David: Ping!

Some comments on this patch would probably help to get it mainline. :)

Regards,
Michael

On Sat, Jul 05, 2014 at 04:45:21PM +0200, Michael Grzeschik wrote:
> Ping!
> 
> On Fri, Jun 27, 2014 at 12:38:44PM +0200, Michael Grzeschik wrote:
> > The current approach of the read_page function is to iterate over all
> > subpages and call the correct_data function. The correct_data function
> > currently does the same. It iterates over all subpages and checks for
> > correctable and uncorrectable data. This redundant call for each
> > subpage leads to miscalculations.
> > 
> > This patch changes the driver to use its own read_page function in which
> > we call the correct_data function only once per page. With that we do
> > the failure and correct statistics counting inside this function.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > fixed printk to pr_debug
> > 
> >  drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 69 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index a72d508..5f9e36d 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -141,6 +141,8 @@ struct mxc_nand_host;
> >  
> >  struct mxc_nand_devtype_data {
> >  	void (*preset)(struct mtd_info *);
> > +	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > +			uint8_t *buf, int oob_required, int page);
> >  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
> >  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
> >  	void (*send_page)(struct mtd_info *, unsigned int);
> > @@ -649,6 +651,59 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * mxc_nand_read_page_hwecc_v2_v3 - [REPLACEABLE] hardware ECC based page read function
> > + * @mtd: mtd info structure
> > + * @chip: nand chip info structure
> > + * @buf: buffer to store read data
> > + * @oob_required: caller requires OOB data read to chip->oob_poi
> > + * @page: page number to read
> > + *
> > + * Not for syndrome calculating ECC controllers which need a special oob layout.
> > + */
> > +static int
> > +mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> > +			       struct nand_chip *chip,
> > +			       uint8_t *buf, int oob_required, int page)
> > +{
> > +	int i, eccsize = chip->ecc.size;
> > +	struct nand_chip *nand_chip = mtd->priv;
> > +	struct mxc_nand_host *host = nand_chip->priv;
> > +	int eccbytes = chip->ecc.bytes;
> > +	int eccsteps = chip->ecc.steps;
> > +	uint8_t *p = buf;
> > +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> > +	uint8_t *ecc_code = chip->buffers->ecccode;
> > +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> > +	unsigned int max_bitflips = 0;
> > +	u32 ecc_stat, err;
> > +	int stat;
> > +
> > +	ecc_stat = host->devtype_data->get_ecc_status(host);
> > +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> > +		err = ecc_stat & 0xf;
> > +		chip->ecc.hwctl(mtd, NAND_ECC_READ);
> > +		chip->read_buf(mtd, p, eccsize);
> > +		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> > +		ecc_stat >>= 4;
> > +
> > +	}
> > +	ecc_stat = host->devtype_data->get_ecc_status(host);
> > +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> > +
> > +	for (i = 0; i < chip->ecc.total; i++)
> > +		ecc_code[i] = chip->oob_poi[eccpos[i]];
> > +
> > +	eccsteps = chip->ecc.steps;
> > +	p = buf;
> > +
> > +	stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> > +	max_bitflips = max_t(unsigned int, max_bitflips, stat);
> > +
> > +	return max_bitflips;
> > +}
> > +
> > +
> >  static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
> >  				 u_char *read_ecc, u_char *calc_ecc)
> >  {
> > @@ -656,7 +711,7 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
> >  	struct mxc_nand_host *host = nand_chip->priv;
> >  	u32 ecc_stat, err;
> >  	int no_subpages = 1;
> > -	int ret = 0;
> > +	int ret = 0, broken = 0;
> >  	u8 ecc_bit_mask, err_limit = 0x1;
> >  
> >  	ecc_bit_mask = 0xf;
> > @@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
> >  	do {
> >  		err = ecc_stat & ecc_bit_mask;
> >  		if (err > err_limit) {
> > -			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
> > -			return -1;
> > +			broken++;
> >  		} else {
> >  			ret += err;
> >  		}
> >  		ecc_stat >>= 4;
> >  	} while (--no_subpages);
> >  
> > -	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> > +	mtd->ecc_stats.corrected += ret;
> > +	if (ret)
> > +		pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> > +
> > +	mtd->ecc_stats.failed += broken;
> > +	if (broken)
> > +		printk(KERN_WARNING "%d Symbol UnCorrectable RS-ECC Error\n",
> > +			 broken);
> >  
> >  	return ret;
> >  }
> > @@ -1216,6 +1277,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
> >  /* v21: i.MX25, i.MX35 */
> >  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> >  	.preset = preset_v2,
> > +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v1_v2,
> >  	.send_addr = send_addr_v1_v2,
> >  	.send_page = send_page_v2,
> > @@ -1242,6 +1304,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> >  /* v3.2a: i.MX51 */
> >  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> >  	.preset = preset_v3,
> > +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v3,
> >  	.send_addr = send_addr_v3,
> >  	.send_page = send_page_v3,
> > @@ -1269,6 +1332,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> >  /* v3.2b: i.MX53 */
> >  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
> >  	.preset = preset_v3,
> > +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v3,
> >  	.send_addr = send_addr_v3,
> >  	.send_page = send_page_v3,
> > @@ -1483,6 +1547,7 @@ static int mxcnd_probe(struct platform_device *pdev)
> >  	this->ecc.layout = host->devtype_data->ecclayout_512;
> >  
> >  	if (host->pdata.hw_ecc) {
> > +		this->ecc.read_page = host->devtype_data->read_page;
> >  		this->ecc.calculate = mxc_nand_calculate_ecc;
> >  		this->ecc.hwctl = mxc_nand_enable_hwecc;
> >  		this->ecc.correct = host->devtype_data->correct_data;
> > -- 
> > 2.0.0
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATH v2] mxc_nand: use our own read_page function

[Brian Norris]

Hi Michael,

On Fri, Jun 27, 2014 at 12:38:44PM +0200, Michael Grzeschik wrote:
> The current approach of the read_page function is to iterate over all
> subpages and call the correct_data function. The correct_data function
> currently does the same. It iterates over all subpages and checks for
> correctable and uncorrectable data. This redundant call for each
> subpage leads to miscalculations.

Hmm, you certainly do have some statistic bugs, but I'm not sure you're
solving this correctly.

> This patch changes the driver to use its own read_page function in which
> we call the correct_data function only once per page. With that we do
> the failure and correct statistics counting inside this function.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> fixed printk to pr_debug
> 
>  drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index a72d508..5f9e36d 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -141,6 +141,8 @@ struct mxc_nand_host;
>  
>  struct mxc_nand_devtype_data {
>  	void (*preset)(struct mtd_info *);
> +	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> +			uint8_t *buf, int oob_required, int page);
>  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_page)(struct mtd_info *, unsigned int);
> @@ -649,6 +651,59 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
>  	return 0;
>  }
>  
> +/**
> + * mxc_nand_read_page_hwecc_v2_v3 - [REPLACEABLE] hardware ECC based page read function

Don't include the [REPLACEABLE] language here. That's mostly just used
for code like nand_base, where we provide some defaults / helpers that
may or may not be intended to allow overrides. (Not really the best
approach, IMO, but that's beside the point.) So don't mark this in your
low-level driver.

> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @buf: buffer to store read data
> + * @oob_required: caller requires OOB data read to chip->oob_poi
> + * @page: page number to read
> + *
> + * Not for syndrome calculating ECC controllers which need a special oob layout.
> + */
> +static int

I don't think you want a line break here, to match the style of the rest
of the driver.

> +mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> +			       struct nand_chip *chip,
> +			       uint8_t *buf, int oob_required, int page)
> +{
> +	int i, eccsize = chip->ecc.size;
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	uint8_t *p = buf;
> +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> +	uint8_t *ecc_code = chip->buffers->ecccode;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	unsigned int max_bitflips = 0;
> +	u32 ecc_stat, err;
> +	int stat;
> +
> +	ecc_stat = host->devtype_data->get_ecc_status(host);
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +		err = ecc_stat & 0xf;

drivers/mtd/nand/mxc_nand.c: In function 'mxc_nand_read_page_hwecc_v2_v3':
drivers/mtd/nand/mxc_nand.c:679:16: warning: variable 'err' set but not used [-Wunused-but-set-variable]

Is that intentional?

> +		chip->ecc.hwctl(mtd, NAND_ECC_READ);
> +		chip->read_buf(mtd, p, eccsize);
> +		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> +		ecc_stat >>= 4;

You're also never using the value of ecc_stat, except to calculate other
values which are never used.

> +

Drop the extra blank line.

> +	}
> +	ecc_stat = host->devtype_data->get_ecc_status(host);

Result unused?

> +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +
> +	for (i = 0; i < chip->ecc.total; i++)
> +		ecc_code[i] = chip->oob_poi[eccpos[i]];
> +
> +	eccsteps = chip->ecc.steps;
> +	p = buf;
> +
> +	stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> +	max_bitflips = max_t(unsigned int, max_bitflips, stat);

This is wrong. First, you probably don't want to cast 'stat' to
unsigned, in case it's an error. Second, absent an error, this just
resolves to:

	max_bitflips = stat;

So you're not actually determining the maximum per-sector bitflips,
you're just determining the total # of bitflips.

> +
> +	return max_bitflips;

So I think you have some leftover/unused code in this function

Additionally, I'm confused because your ecc.correct() function is now
hiding some of the stat counting -- this is contrary to its usage
elsewhere. See more comments below.

> +}
> +
> +
>  static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  				 u_char *read_ecc, u_char *calc_ecc)
>  {
> @@ -656,7 +711,7 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	struct mxc_nand_host *host = nand_chip->priv;
>  	u32 ecc_stat, err;
>  	int no_subpages = 1;
> -	int ret = 0;
> +	int ret = 0, broken = 0;
>  	u8 ecc_bit_mask, err_limit = 0x1;
>  
>  	ecc_bit_mask = 0xf;
> @@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	do {
>  		err = ecc_stat & ecc_bit_mask;
>  		if (err > err_limit) {
> -			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
> -			return -1;
> +			broken++;
>  		} else {
>  			ret += err;
>  		}
>  		ecc_stat >>= 4;
>  	} while (--no_subpages);
>  
> -	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> +	mtd->ecc_stats.corrected += ret;
> +	if (ret)
> +		pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> +
> +	mtd->ecc_stats.failed += broken;
> +	if (broken)
> +		printk(KERN_WARNING "%d Symbol UnCorrectable RS-ECC Error\n",
> +			 broken);

This is wrong; either this function should not be assigned to
ecc.correct(), or else it should not be modifying the ecc_stats. See all
other examples of ecc.correct() callbacks. Additionally, it should be
returning negative to indicate ECC failure, which you're not doing.

One solution: stop don't assign an chip->ecc.correct() callback any
more, so that your correction logic is encapsulated entirely within
chip->read_page(). You would need to make mxc_nand_read_page_hwecc_v2_v3
call mxc_nand_correct_data_v2_v3() directly, of course. (And you could
even try a BUG() whenever chip->ecc.correct() is called, like
cafe_nand.c does.)

And in fact, I'd just take this one step further; kill
mxc_nand_correct_data_v2_v3() and just merge it with
mxc_nand_read_page_hwecc_v2_v3(). Then you can count bitflips (resolving
the 'max_bitflips' issue I pointed out above) all in one place.

>  
>  	return ret;
>  }
> @@ -1216,6 +1277,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
>  /* v21: i.MX25, i.MX35 */
>  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  	.preset = preset_v2,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v1_v2,
>  	.send_addr = send_addr_v1_v2,
>  	.send_page = send_page_v2,
> @@ -1242,6 +1304,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  /* v3.2a: i.MX51 */
>  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  	.preset = preset_v3,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1269,6 +1332,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  /* v3.2b: i.MX53 */
>  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
>  	.preset = preset_v3,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1483,6 +1547,7 @@ static int mxcnd_probe(struct platform_device *pdev)
>  	this->ecc.layout = host->devtype_data->ecclayout_512;
>  
>  	if (host->pdata.hw_ecc) {
> +		this->ecc.read_page = host->devtype_data->read_page;
>  		this->ecc.calculate = mxc_nand_calculate_ecc;
>  		this->ecc.hwctl = mxc_nand_enable_hwecc;
>  		this->ecc.correct = host->devtype_data->correct_data;

Brian

Re: [PATH v2] mxc_nand: use our own read_page function

[Michael Grzeschik]

Hi Brian,

On Mon, Jul 14, 2014 at 12:19:47PM -0700, Brian Norris wrote:
> Hi Michael,
> 
> On Fri, Jun 27, 2014 at 12:38:44PM +0200, Michael Grzeschik wrote:
> > The current approach of the read_page function is to iterate over all
> > subpages and call the correct_data function. The correct_data function
> > currently does the same. It iterates over all subpages and checks for
> > correctable and uncorrectable data. This redundant call for each
> > subpage leads to miscalculations.
> 
> Hmm, you certainly do have some statistic bugs, but I'm not sure you're
> solving this correctly.
> 

This driver sure has.

> > This patch changes the driver to use its own read_page function in which
> > we call the correct_data function only once per page. With that we do
> > the failure and correct statistics counting inside this function.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > fixed printk to pr_debug
> > 
> >  drivers/mtd/nand/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 69 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index a72d508..5f9e36d 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -141,6 +141,8 @@ struct mxc_nand_host;
> >  
> >  struct mxc_nand_devtype_data {
> >  	void (*preset)(struct mtd_info *);
> > +	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > +			uint8_t *buf, int oob_required, int page);
> >  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
> >  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
> >  	void (*send_page)(struct mtd_info *, unsigned int);
> > @@ -649,6 +651,59 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * mxc_nand_read_page_hwecc_v2_v3 - [REPLACEABLE] hardware ECC based page read function
> 
> Don't include the [REPLACEABLE] language here. That's mostly just used
> for code like nand_base, where we provide some defaults / helpers that
> may or may not be intended to allow overrides. (Not really the best
> approach, IMO, but that's beside the point.) So don't mark this in your
> low-level driver.

It was a copy paste leftover. Thanks for the hint.

> 
> > + * @mtd: mtd info structure
> > + * @chip: nand chip info structure
> > + * @buf: buffer to store read data
> > + * @oob_required: caller requires OOB data read to chip->oob_poi
> > + * @page: page number to read
> > + *
> > + * Not for syndrome calculating ECC controllers which need a special oob layout.
> > + */
> > +static int
> 
> I don't think you want a line break here, to match the style of the rest
> of the driver.

I used Lindent as I didn't know how to align it correctly. I will
keep it in one line instead.

> 
> > +mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> > +			       struct nand_chip *chip,
> > +			       uint8_t *buf, int oob_required, int page)
> > +{
> > +	int i, eccsize = chip->ecc.size;
> > +	struct nand_chip *nand_chip = mtd->priv;
> > +	struct mxc_nand_host *host = nand_chip->priv;
> > +	int eccbytes = chip->ecc.bytes;
> > +	int eccsteps = chip->ecc.steps;
> > +	uint8_t *p = buf;
> > +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> > +	uint8_t *ecc_code = chip->buffers->ecccode;
> > +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> > +	unsigned int max_bitflips = 0;
> > +	u32 ecc_stat, err;
> > +	int stat;
> > +
> > +	ecc_stat = host->devtype_data->get_ecc_status(host);
> > +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> > +		err = ecc_stat & 0xf;
> 
> drivers/mtd/nand/mxc_nand.c: In function 'mxc_nand_read_page_hwecc_v2_v3':
> drivers/mtd/nand/mxc_nand.c:679:16: warning: variable 'err' set but not used [-Wunused-but-set-variable]
> 
> Is that intentional?

No, this is also an leftover. Thanks for the hint.

> 
> > +		chip->ecc.hwctl(mtd, NAND_ECC_READ);
> > +		chip->read_buf(mtd, p, eccsize);
> > +		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> > +		ecc_stat >>= 4;
> 
> You're also never using the value of ecc_stat, except to calculate other
> values which are never used.
> 
> > +
> 
> Drop the extra blank line.
> 
> > +	}
> > +	ecc_stat = host->devtype_data->get_ecc_status(host);
> 
> Result unused?
> 

Ok, this is odd. I must have messed up my patch here.

> > +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> > +
> > +	for (i = 0; i < chip->ecc.total; i++)
> > +		ecc_code[i] = chip->oob_poi[eccpos[i]];
> > +
> > +	eccsteps = chip->ecc.steps;
> > +	p = buf;
> > +
> > +	stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> > +	max_bitflips = max_t(unsigned int, max_bitflips, stat);
> 
> This is wrong. First, you probably don't want to cast 'stat' to
> unsigned, in case it's an error. Second, absent an error, this just
> resolves to:
> 
> 	max_bitflips = stat;
> 
> So you're not actually determining the maximum per-sector bitflips,
> you're just determining the total # of bitflips.
> 
> > +
> > +	return max_bitflips;
> 
> So I think you have some leftover/unused code in this function

Indeed.

> 
> Additionally, I'm confused because your ecc.correct() function is now
> hiding some of the stat counting -- this is contrary to its usage
> elsewhere. See more comments below.
> 

You are right about the hiding functionality.

> > +}
> > +
> > +
> >  static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
> >  				 u_char *read_ecc, u_char *calc_ecc)
> >  {
> > @@ -656,7 +711,7 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
> >  	struct mxc_nand_host *host = nand_chip->priv;
> >  	u32 ecc_stat, err;
> >  	int no_subpages = 1;
> > -	int ret = 0;
> > +	int ret = 0, broken = 0;
> >  	u8 ecc_bit_mask, err_limit = 0x1;
> >  
> >  	ecc_bit_mask = 0xf;
> > @@ -673,15 +728,21 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
> >  	do {
> >  		err = ecc_stat & ecc_bit_mask;
> >  		if (err > err_limit) {
> > -			printk(KERN_WARNING "UnCorrectable RS-ECC Error\n");
> > -			return -1;
> > +			broken++;
> >  		} else {
> >  			ret += err;
> >  		}
> >  		ecc_stat >>= 4;
> >  	} while (--no_subpages);
> >  
> > -	pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> > +	mtd->ecc_stats.corrected += ret;
> > +	if (ret)
> > +		pr_debug("%d Symbol Correctable RS-ECC Error\n", ret);
> > +
> > +	mtd->ecc_stats.failed += broken;
> > +	if (broken)
> > +		printk(KERN_WARNING "%d Symbol UnCorrectable RS-ECC Error\n",
> > +			 broken);
> 
> This is wrong; either this function should not be assigned to
> ecc.correct(), or else it should not be modifying the ecc_stats. See all
> other examples of ecc.correct() callbacks. Additionally, it should be
> returning negative to indicate ECC failure, which you're not doing.
> 
> One solution: stop don't assign an chip->ecc.correct() callback any
> more, so that your correction logic is encapsulated entirely within
> chip->read_page(). You would need to make mxc_nand_read_page_hwecc_v2_v3
> call mxc_nand_correct_data_v2_v3() directly, of course. (And you could
> even try a BUG() whenever chip->ecc.correct() is called, like
> cafe_nand.c does.)
> 
> And in fact, I'd just take this one step further; kill
> mxc_nand_correct_data_v2_v3() and just merge it with
> mxc_nand_read_page_hwecc_v2_v3(). Then you can count bitflips (resolving
> the 'max_bitflips' issue I pointed out above) all in one place.
> 

I think your hints all make sense. I was poking around with the
functions we have and need to asign. Unfortunetly I ran into no clear
picture to fix it the correct way and came up with this hacky patch.


> >  
> >  	return ret;
> >  }
> > @@ -1216,6 +1277,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
> >  /* v21: i.MX25, i.MX35 */
> >  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> >  	.preset = preset_v2,
> > +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v1_v2,
> >  	.send_addr = send_addr_v1_v2,
> >  	.send_page = send_page_v2,
> > @@ -1242,6 +1304,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> >  /* v3.2a: i.MX51 */
> >  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> >  	.preset = preset_v3,
> > +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v3,
> >  	.send_addr = send_addr_v3,
> >  	.send_page = send_page_v3,
> > @@ -1269,6 +1332,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> >  /* v3.2b: i.MX53 */
> >  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
> >  	.preset = preset_v3,
> > +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v3,
> >  	.send_addr = send_addr_v3,
> >  	.send_page = send_page_v3,
> > @@ -1483,6 +1547,7 @@ static int mxcnd_probe(struct platform_device *pdev)
> >  	this->ecc.layout = host->devtype_data->ecclayout_512;
> >  
> >  	if (host->pdata.hw_ecc) {
> > +		this->ecc.read_page = host->devtype_data->read_page;
> >  		this->ecc.calculate = mxc_nand_calculate_ecc;
> >  		this->ecc.hwctl = mxc_nand_enable_hwecc;
> >  		this->ecc.correct = host->devtype_data->correct_data;
> 
> Brian
> 

Many thanks!

Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATH v2] mxc_nand: use our own read_page function

[Brian Norris]

Hi Michael,

On Tue, Jul 22, 2014 at 12:09:54PM +0200, Michael Grzeschik wrote:
> I think your hints all make sense. I was poking around with the
> functions we have and need to asign. Unfortunetly I ran into no clear
> picture to fix it the correct way and came up with this hacky patch.

Your patch is close, so it's not completely a hack. And the various
override functions can be difficult to follow. You can ping back here if
you have questions still.

Regards,
Brian

Re: [PATCH 1/2] mxc_nand: fix err_limit and err_mask

[Brian Norris]

Hi Michael,

Since I had a few comments for changes on patch 2, might as well comment
here...

On Thu, Jun 26, 2014 at 10:42:47PM +0200, Michael Grzeschik wrote:
> This patch fixes the error detection limits for the used
> eccsize of the 1, 4 and 8 bit eccmode.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index cb6c845..7fd495e 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -657,10 +657,14 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	u32 ecc_stat, err;
>  	int no_subpages = 1;
>  	int ret = 0;
> -	u8 ecc_bit_mask, err_limit;
> +	u8 ecc_bit_mask, err_limit = 0x1;
>  
> -	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
> -	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
> +	ecc_bit_mask = 0xf;
> +
> +	if (host->eccsize == 4)
> +		err_limit = 0x2;
> +	else if (host->eccsize == 8)
> +		err_limit = 0x4;

This would be a little more obvious as a switch statement.

But please do address my comments on patch 2 (or at least reply, if you
have questions).

>  
>  	no_subpages = mtd->writesize >> 9;
>  

Thanks,
Brian