[PATCH v3 0/4] pxa3xx-nand: Allow to specify an ECC scheme through DT

[PATCH v3 0/4] pxa3xx-nand: Allow to specify an ECC scheme through DT

[Ezequiel Garcia]

Now that we have agreed on the ECC devicetree binding, here's a patchset
that makes pxa3xx-nand driver use it.

This work is required on platforms where the flash device reports a required
ECC strength, but that are capable of handling a stronger one. Simon Guinot
reported a situation where the vendor provided a bootloader configured with
a stronger than required ECC scheme. Without this patches, such a situation
would require a hack to make the driver use a specific ECC scheme.

Addressing Brian's feedback [1], this v3 series improves the ECC strength check,
and moves it to NAND core's code. If the configured ECC strength is weaker
than the minimum strength required by the datasheet, a noisy warning is shown.
This is done in the first patch.

The second patch is a minor cleanup of the pxa_ecc_init().

The third patch makes use of the ECC strength and ECC step size parameters.
To ensure full backward compatibility, the patch *tries* to use DT-specified
ECC parameters, falling back to using the datasheet-specified settings if the
former are missing.

Finally, the last patch updates the documentation binding, with the supported
ECC parameters. We claim to support the following ECC schemes:
 * strength = 1, size = 512 (Hamming strength)
 * strength = 4, size = 512 (BCH-4 strength)
 * strength = 8, size = 512 (BCH-8 strength)

It must be noted that this ECC engine actually provides a stronger
correction capability, which are at least as strong as the above:
 * strength = 1, size = 512 (Hamming strength)
 * strength = 16, size = 2048
 * strength = 16, size = 1024

I've chosen to describe the supported schemes in terms of 512B steps because
it's results in less intrusive changes. Moreover, taking the step size to be
512B seems to be a widespread convention.

[1] http://permalink.gmane.org/gmane.linux.drivers.devicetree/73823

Changes from v2:
  * Improved ECC strength check and moved it to nand_base.c common code,
    and drop the now unneeded normalization.

  * Define the ecc_strength and ecc_step_size as signed integers, as noted
    by Brian Norris.

Changes from v1:
  * Fixed typo in ECC strength normalization calculus.

Ezequiel Garcia (4):
  mtd: nand: Warn the user if the selected ECC strength is too weak
  mtd: nand: pxa3xx: Clean pxa_ecc_init() error handling
  mtd: nand: pxa3xx: Use ECC strength and step size devicetree binding
  mtd: nand: pxa3xx: Add supported ECC strength and step size to the DT
    binding

 .../devicetree/bindings/mtd/pxa3xx-nand.txt        |  8 +++++
 drivers/mtd/nand/nand_base.c                       | 36 ++++++++++++++++++++
 drivers/mtd/nand/pxa3xx_nand.c                     | 38 ++++++++++++++--------
 include/linux/platform_data/mtd-nand-pxa3xx.h      |  3 ++
 4 files changed, 72 insertions(+), 13 deletions(-)

-- 
1.9.1

[PATCH v3 1/4] mtd: nand: Warn the user if the selected ECC strength is too weak

[Ezequiel Garcia]

This commit makes use of the chip->ecc_strength_ds and chip->ecc_step_ds which
contain the datasheet minimum requested ECC strength to produce a noisy warning
if the configured ECC strength is weaker.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7853b9b..b84450a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3769,6 +3769,39 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 }
 EXPORT_SYMBOL(nand_scan_ident);
 
+/*
+ * Check if the chip configuration meet the datasheet requirements.
+
+ * If our configuration corrects A bits per B bytes and the minimum
+ * required correction level is X bits per Y bytes, then we must ensure
+ * both of the following are true:
+ *
+ * (1) A / B >= X / Y
+ * (2) A >= X
+ *
+ * Requirement (1) ensures we can correct for the required bitflip density.
+ * Requirement (2) ensures we can correct even when all bitflips are clumped
+ * in the same sector.
+ */
+static bool nand_ecc_strength_good(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int corr, ds_corr;
+
+	if (ecc->size == 0 || chip->ecc_step_ds == 0)
+		/* Not enough information */
+		return true;
+
+	/*
+	 * We get the number of corrected bits per page to compare
+	 * the correction density.
+	 */
+	corr = (mtd->writesize * ecc->strength) / ecc->size;
+	ds_corr = (mtd->writesize * chip->ecc_strength_ds) / chip->ecc_step_ds;
+
+	return corr >= ds_corr && ecc->strength >= chip->ecc_strength_ds;
+}
 
 /**
  * nand_scan_tail - [NAND Interface] Scan for the NAND device
@@ -3989,6 +4022,9 @@ int nand_scan_tail(struct mtd_info *mtd)
 		ecc->layout->oobavail += ecc->layout->oobfree[i].length;
 	mtd->oobavail = ecc->layout->oobavail;
 
+	/* ECC sanity check: warn noisily if it's too weak */
+	WARN_ON(!nand_ecc_strength_good(mtd));
+
 	/*
 	 * Set the number of read / write steps for one page depending on ECC
 	 * mode.
-- 
1.9.1

[PATCH v3 2/4] mtd: nand: pxa3xx: Clean pxa_ecc_init() error handling

[Ezequiel Garcia]

Let's make pxa_ecc_init() return a negative errno on error or zero
if succesful, which is standard kernel practice. Also, report the
selected ECC strength and step size, which is important information.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 940e6c7..3b66a64 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1354,7 +1354,6 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->mode = NAND_ECC_HW;
 		ecc->size = 512;
 		ecc->strength = 1;
-		return 1;
 
 	} else if (strength == 1 && ecc_stepsize == 512 && page_size == 512) {
 		info->chunk_size = 512;
@@ -1363,7 +1362,6 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->mode = NAND_ECC_HW;
 		ecc->size = 512;
 		ecc->strength = 1;
-		return 1;
 
 	/*
 	 * Required ECC: 4-bit correction per 512 bytes
@@ -1378,7 +1376,6 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->size = info->chunk_size;
 		ecc->layout = &ecc_layout_2KB_bch4bit;
 		ecc->strength = 16;
-		return 1;
 
 	} else if (strength == 4 && ecc_stepsize == 512 && page_size == 4096) {
 		info->ecc_bch = 1;
@@ -1389,7 +1386,6 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->size = info->chunk_size;
 		ecc->layout = &ecc_layout_4KB_bch4bit;
 		ecc->strength = 16;
-		return 1;
 
 	/*
 	 * Required ECC: 8-bit correction per 512 bytes
@@ -1404,8 +1400,15 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->size = info->chunk_size;
 		ecc->layout = &ecc_layout_4KB_bch8bit;
 		ecc->strength = 16;
-		return 1;
+	} else {
+		dev_err(&info->pdev->dev,
+			"ECC strength %d at page size %d is not supported\n",
+			strength, page_size);
+		return -ENODEV;
 	}
+
+	dev_info(&info->pdev->dev, "ECC strength %d, ECC step size %d\n",
+		 ecc->strength, ecc->size);
 	return 0;
 }
 
@@ -1527,12 +1530,8 @@ KEEP_CONFIG:
 
 	ret = pxa_ecc_init(info, &chip->ecc, ecc_strength,
 			   ecc_step, mtd->writesize);
-	if (!ret) {
-		dev_err(&info->pdev->dev,
-			"ECC strength %d at page size %d is not supported\n",
-			ecc_strength, mtd->writesize);
-		return -ENODEV;
-	}
+	if (ret)
+		return ret;
 
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
-- 
1.9.1

[PATCH v3 3/4] mtd: nand: pxa3xx: Use ECC strength and step size devicetree binding

[Ezequiel Garcia]

This commit adds support for the user to specify the ECC strength
and step size through the devicetree. We keep the previous behavior,
when there is no DT parameter provided.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c                | 17 +++++++++++++++--
 include/linux/platform_data/mtd-nand-pxa3xx.h |  3 +++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 3b66a64..2a9add0 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1519,8 +1519,13 @@ KEEP_CONFIG:
 		}
 	}
 
-	ecc_strength = chip->ecc_strength_ds;
-	ecc_step = chip->ecc_step_ds;
+	if (pdata->ecc_strength && pdata->ecc_step_size) {
+		ecc_strength = pdata->ecc_strength;
+		ecc_step = pdata->ecc_step_size;
+	} else {
+		ecc_strength = chip->ecc_strength_ds;
+		ecc_step = chip->ecc_step_ds;
+	}
 
 	/* Set default ECC strength requirements on non-ONFI devices */
 	if (ecc_strength < 1 && ecc_step < 1) {
@@ -1729,6 +1734,14 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 	of_property_read_u32(np, "num-cs", &pdata->num_cs);
 	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);
 
+	pdata->ecc_strength = of_get_nand_ecc_strength(np);
+	if (pdata->ecc_strength < 0)
+		pdata->ecc_strength = 0;
+
+	pdata->ecc_step_size = of_get_nand_ecc_step_size(np);
+	if (pdata->ecc_step_size < 0)
+		pdata->ecc_step_size = 0;
+
 	pdev->dev.platform_data = pdata;
 
 	return 0;
diff --git a/include/linux/platform_data/mtd-nand-pxa3xx.h b/include/linux/platform_data/mtd-nand-pxa3xx.h
index a941471..ac4ea2e 100644
--- a/include/linux/platform_data/mtd-nand-pxa3xx.h
+++ b/include/linux/platform_data/mtd-nand-pxa3xx.h
@@ -58,6 +58,9 @@ struct pxa3xx_nand_platform_data {
 	/* use an flash-based bad block table */
 	bool	flash_bbt;
 
+	/* requested ECC strength and ECC step size */
+	int ecc_strength, ecc_step_size;
+
 	const struct mtd_partition		*parts[NUM_CHIP_SELECT];
 	unsigned int				nr_parts[NUM_CHIP_SELECT];
 
-- 
1.9.1

[PATCH v3 4/4] mtd: nand: pxa3xx: Add supported ECC strength and step size to the DT binding

[Ezequiel Garcia]

This commit updates the devicetree binding documentation for this driver
with the supported ECC strength and step size combinations.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt b/Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
index 86e0a56..de8b517 100644
--- a/Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
@@ -17,6 +17,14 @@ Optional properties:
  - num-cs:			Number of chipselect lines to usw
  - nand-on-flash-bbt: 		boolean to enable on flash bbt option if
 				not present false
+ - nand-ecc-strength:           number of bits to correct per ECC step
+ - nand-ecc-step-size:          number of data bytes covered by a single ECC step
+
+The following ECC strength and step size are currently supported:
+
+ - nand-ecc-strength = <1>, nand-ecc-step-size = <512>
+ - nand-ecc-strength = <4>, nand-ecc-step-size = <512>
+ - nand-ecc-strength = <8>, nand-ecc-step-size = <512>
 
 Example:
 
-- 
1.9.1

Re: [PATCH v3 0/4] pxa3xx-nand: Allow to specify an ECC scheme through DT

[Brian Norris]

On Wed, May 14, 2014 at 02:58:05PM -0300, Ezequiel Garcia wrote:
> Now that we have agreed on the ECC devicetree binding, here's a patchset
> that makes pxa3xx-nand driver use it.
[...]
> Ezequiel Garcia (4):
>   mtd: nand: Warn the user if the selected ECC strength is too weak
>   mtd: nand: pxa3xx: Clean pxa_ecc_init() error handling
>   mtd: nand: pxa3xx: Use ECC strength and step size devicetree binding
>   mtd: nand: pxa3xx: Add supported ECC strength and step size to the DT
>     binding

Pushed all 4 to l2-mtd.git. Thanks!

Brian