[PATCH v2 0/4] Allow to specify an ECC scheme through DT

[PATCH v2 0/4] 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.

(Simon: it would be great if you can confirm this allows you to remove any hacks).

The first two patches clean the pxa_ecc_init(), preparing for the real
work to be done.

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 ONFI-specified settings if the
former are missing.

Also, we use the ONFI 'datasheet' ECC strength requirement, to ensure the
DT-specified ECC strength is strong enough to meet the requirement.

Finally, the last patch updates the documentation binding, with the supported
ECC parameters.

Tested on Marvell's Armada 370 RD and also Compulab's CM-X300 (PXA3xx SoC),
which is a non-DT platform. Based on today's l2-mtd.git.

Changes from v1:

  * Fixed typo in ECC strength normalization calculus.

Ezequiel Garcia (4):
  mtd: nand: pxa3xx: Normalize ECC strength for ECC scheme selection
  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/pxa3xx_nand.c                     | 78 +++++++++++++---------
 include/linux/platform_data/mtd-nand-pxa3xx.h      |  3 +
 3 files changed, 59 insertions(+), 30 deletions(-)

-- 
1.9.0

[PATCH v2 1/4] mtd: nand: pxa3xx: Normalize ECC strength for ECC scheme selection

[Ezequiel Garcia]

As preparation work to use the ECC devicetree binding, let's normalize
the ECC strength to a 512B step size. This will allow comparison between
different strengths.

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

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 7588fe2..92b3439 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1344,10 +1344,9 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 }
 
 static int pxa_ecc_init(struct pxa3xx_nand_info *info,
-			struct nand_ecc_ctrl *ecc,
-			int strength, int ecc_stepsize, int page_size)
+			struct nand_ecc_ctrl *ecc, int strength, int page_size)
 {
-	if (strength == 1 && ecc_stepsize == 512 && page_size == 2048) {
+	if (strength == 1 && page_size == 2048) {
 		info->chunk_size = 2048;
 		info->spare_size = 40;
 		info->ecc_size = 24;
@@ -1356,7 +1355,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->strength = 1;
 		return 1;
 
-	} else if (strength == 1 && ecc_stepsize == 512 && page_size == 512) {
+	} else if (strength == 1 && page_size == 512) {
 		info->chunk_size = 512;
 		info->spare_size = 8;
 		info->ecc_size = 8;
@@ -1369,7 +1368,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 	 * Required ECC: 4-bit correction per 512 bytes
 	 * Select: 16-bit correction per 2048 bytes
 	 */
-	} else if (strength == 4 && ecc_stepsize == 512 && page_size == 2048) {
+	} else if (strength == 4 && page_size == 2048) {
 		info->ecc_bch = 1;
 		info->chunk_size = 2048;
 		info->spare_size = 32;
@@ -1380,7 +1379,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->strength = 16;
 		return 1;
 
-	} else if (strength == 4 && ecc_stepsize == 512 && page_size == 4096) {
+	} else if (strength == 4 && page_size == 4096) {
 		info->ecc_bch = 1;
 		info->chunk_size = 2048;
 		info->spare_size = 32;
@@ -1395,7 +1394,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 	 * Required ECC: 8-bit correction per 512 bytes
 	 * Select: 16-bit correction per 1024 bytes
 	 */
-	} else if (strength == 8 && ecc_stepsize == 512 && page_size == 4096) {
+	} else if (strength == 8 && page_size == 4096) {
 		info->ecc_bch = 1;
 		info->chunk_size = 1024;
 		info->spare_size = 0;
@@ -1525,8 +1524,8 @@ KEEP_CONFIG:
 		ecc_step = 512;
 	}
 
-	ret = pxa_ecc_init(info, &chip->ecc, ecc_strength,
-			   ecc_step, mtd->writesize);
+	ret = pxa_ecc_init(info, &chip->ecc, (ecc_strength * 512) / ecc_step,
+			   mtd->writesize);
 	if (!ret) {
 		dev_err(&info->pdev->dev,
 			"ECC strength %d at page size %d is not supported\n",
-- 
1.9.0

[PATCH v2 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 92b3439..cf7d757 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1353,7 +1353,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 && page_size == 512) {
 		info->chunk_size = 512;
@@ -1362,7 +1361,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
@@ -1388,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
@@ -1403,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;
 }
 
@@ -1526,12 +1530,8 @@ KEEP_CONFIG:
 
 	ret = pxa_ecc_init(info, &chip->ecc, (ecc_strength * 512) / 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.0

[PATCH v2 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.

Also, if there is an ONFI-specified minimum ECC strength requirement,
and the DT-specified ECC strength is weaker, print a warning and try to
select a strength compatible with the ONFI requirement.

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

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index cf7d757..cc13896 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1344,8 +1344,30 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 }
 
 static int pxa_ecc_init(struct pxa3xx_nand_info *info,
-			struct nand_ecc_ctrl *ecc, int strength, int page_size)
+			struct nand_chip *chip,
+			struct pxa3xx_nand_platform_data *pdata,
+			unsigned int page_size)
 {
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	unsigned int strength, onfi_strength;
+
+	/* We need to normalize the ECC strengths, in order to compare them. */
+	strength = (pdata->ecc_strength * 512) / pdata->ecc_step_size;
+	onfi_strength = (chip->ecc_strength_ds * 512) / chip->ecc_step_ds;
+
+	/*
+	 * The user requested ECC strength cannot be weaker than the ONFI
+	 * minimum specified ECC strength.
+	 */
+	if (strength && strength < onfi_strength) {
+		dev_warn(&info->pdev->dev,
+			 "requested ECC strength is too weak\n");
+		strength = onfi_strength;
+	}
+
+	if (!strength)
+		strength = onfi_strength ? onfi_strength : 1;
+
 	if (strength == 1 && page_size == 2048) {
 		info->chunk_size = 2048;
 		info->spare_size = 40;
@@ -1375,7 +1397,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 && page_size == 4096) {
 		info->ecc_bch = 1;
@@ -1424,7 +1445,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 	uint32_t id = -1;
 	uint64_t chipsize;
 	int i, ret, num;
-	uint16_t ecc_strength, ecc_step;
 
 	if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
 		goto KEEP_CONFIG;
@@ -1519,17 +1539,8 @@ KEEP_CONFIG:
 		}
 	}
 
-	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) {
-		ecc_strength = 1;
-		ecc_step = 512;
-	}
-
-	ret = pxa_ecc_init(info, &chip->ecc, (ecc_strength * 512) / ecc_step,
-			   mtd->writesize);
+	/* Selects an ECC and fills pxa3xx_nand_info and nand_chip */
+	ret = pxa_ecc_init(info, chip, pdata, mtd->writesize);
 	if (ret)
 		return ret;
 
@@ -1729,6 +1740,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..5c0f2e3 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 */
+	unsigned int ecc_strength, ecc_step_size;
+
 	const struct mtd_partition		*parts[NUM_CHIP_SELECT];
 	unsigned int				nr_parts[NUM_CHIP_SELECT];
 
-- 
1.9.0

[PATCH v2 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..650c0dc 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>   (aka Hamming)
+ - nand-ecc-strength = <16>, nand-ecc-step-size = <2048> (aka BCH-4)
+ - nand-ecc-strength = <16>, nand-ecc-step-size = <1024> (aka BCH-8)
 
 Example:
 
-- 
1.9.0

Re: [PATCH v2 0/4] Allow to specify an ECC scheme through DT

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

On Fri, Mar 21, 2014 at 08:34:46AM -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.
> 
> 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.
> 
> (Simon: it would be great if you can confirm this allows you to remove any hacks).

Hi Ezequiel,

I confirm this allows me to configure the ECC strength and step size
from DT.

Tested-by: Simon Guinot <simon.guinot@sequanux.org>

Thanks.

Simon

> 
> The first two patches clean the pxa_ecc_init(), preparing for the real
> work to be done.
> 
> 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 ONFI-specified settings if the
> former are missing.
> 
> Also, we use the ONFI 'datasheet' ECC strength requirement, to ensure the
> DT-specified ECC strength is strong enough to meet the requirement.
> 
> Finally, the last patch updates the documentation binding, with the supported
> ECC parameters.
> 
> Tested on Marvell's Armada 370 RD and also Compulab's CM-X300 (PXA3xx SoC),
> which is a non-DT platform. Based on today's l2-mtd.git.
> 
> Changes from v1:
> 
>   * Fixed typo in ECC strength normalization calculus.
> 
> Ezequiel Garcia (4):
>   mtd: nand: pxa3xx: Normalize ECC strength for ECC scheme selection
>   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/pxa3xx_nand.c                     | 78 +++++++++++++---------
>  include/linux/platform_data/mtd-nand-pxa3xx.h      |  3 +
>  3 files changed, 59 insertions(+), 30 deletions(-)
> 
> -- 
> 1.9.0
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 0/4] Allow to specify an ECC scheme through DT

[Ezequiel Garcia]

On Apr 01, Simon Guinot wrote:
> On Fri, Mar 21, 2014 at 08:34:46AM -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.
> > 
> > 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.
> > 
> > (Simon: it would be great if you can confirm this allows you to remove any hacks).
> 
> Hi Ezequiel,
> 
> I confirm this allows me to configure the ECC strength and step size
> from DT.
> 
> Tested-by: Simon Guinot <simon.guinot@sequanux.org>
> 

Thanks a lot for the test.

Brian, any comments?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2 0/4] Allow to specify an ECC scheme through DT

[Ezequiel Garcia]

On Apr 03, Ezequiel Garcia wrote:
> On Apr 01, Simon Guinot wrote:
> > On Fri, Mar 21, 2014 at 08:34:46AM -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.
> > > 
> > > 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.
> > > 
> > > (Simon: it would be great if you can confirm this allows you to remove any hacks).
> > 
> > Hi Ezequiel,
> > 
> > I confirm this allows me to configure the ECC strength and step size
> > from DT.
> > 
> > Tested-by: Simon Guinot <simon.guinot@sequanux.org>
> > 
> 
> Thanks a lot for the test.
> 
> Brian, any comments?

Any chance this can be applied?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Brian Norris]

Hi Ezequiel,

On Fri, Mar 21, 2014 at 08:34:49AM -0300, Ezequiel Garcia wrote:
> 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.
> 
> Also, if there is an ONFI-specified minimum ECC strength requirement,
> and the DT-specified ECC strength is weaker, print a warning and try to
> select a strength compatible with the ONFI requirement.

Are you sure you want to override? That seems contrary to the idea of a
DT property for specifying ECC. But maybe you have a good reason?

If you'd rather just warn the user, it's possible we could move that to
common code in nand_base.c.

> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c                | 47 +++++++++++++++++++--------
>  include/linux/platform_data/mtd-nand-pxa3xx.h |  3 ++
>  2 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index cf7d757..cc13896 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1344,8 +1344,30 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
>  }
>  
>  static int pxa_ecc_init(struct pxa3xx_nand_info *info,
> -			struct nand_ecc_ctrl *ecc, int strength, int page_size)
> +			struct nand_chip *chip,
> +			struct pxa3xx_nand_platform_data *pdata,
> +			unsigned int page_size)
>  {
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	unsigned int strength, onfi_strength;
> +
> +	/* We need to normalize the ECC strengths, in order to compare them. */
> +	strength = (pdata->ecc_strength * 512) / pdata->ecc_step_size;
> +	onfi_strength = (chip->ecc_strength_ds * 512) / chip->ecc_step_ds;

This is not necessarily an "ONFI" property; we also have the full-ID
table in which a minimum ECC strength can be specified. Maybe you can
match the existing naming with "strength" and "strength_ds" (for
"datasheet").

> +
> +	/*
> +	 * The user requested ECC strength cannot be weaker than the ONFI
> +	 * minimum specified ECC strength.
> +	 */
> +	if (strength && strength < onfi_strength) {
> +		dev_warn(&info->pdev->dev,
> +			 "requested ECC strength is too weak\n");
> +		strength = onfi_strength;
> +	}
> +
> +	if (!strength)
> +		strength = onfi_strength ? onfi_strength : 1;
> +

I'm also not sure your "normalization" is generally correct. You can't
really normalize correction strengths to get a fully valid comparison.
Remember, 4-bit/2048B correction is not the same as 1-bit/512B
correction; it is at least as strong, yes. But then, the reverse is
*not* a good comparison. So your code might say that a flash which
requires 4-bit/2KB can be satisfied by 1-bit/512B. (These numbers may
not be seen in practice, but you get my point, right?)

By my understanding, a correct comparison requires two parts, which I've
summarized like this in my own code:

        /*
         * Does the given configuration meet the 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.
         */

I think you are doing (1), but not (2).

>  	if (strength == 1 && page_size == 2048) {
>  		info->chunk_size = 2048;
>  		info->spare_size = 40;
> @@ -1375,7 +1397,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 && page_size == 4096) {
>  		info->ecc_bch = 1;
> @@ -1424,7 +1445,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
>  	uint32_t id = -1;
>  	uint64_t chipsize;
>  	int i, ret, num;
> -	uint16_t ecc_strength, ecc_step;
>  
>  	if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
>  		goto KEEP_CONFIG;
> @@ -1519,17 +1539,8 @@ KEEP_CONFIG:
>  		}
>  	}
>  
> -	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) {
> -		ecc_strength = 1;
> -		ecc_step = 512;
> -	}
> -
> -	ret = pxa_ecc_init(info, &chip->ecc, (ecc_strength * 512) / ecc_step,
> -			   mtd->writesize);
> +	/* Selects an ECC and fills pxa3xx_nand_info and nand_chip */
> +	ret = pxa_ecc_init(info, chip, pdata, mtd->writesize);
>  	if (ret)
>  		return ret;
>  
> @@ -1729,6 +1740,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)

ecc_strength is unsigned, so this error check doesn't work. Maybe you
want a local 'int ret' to temporarily stash the return value?

> +		pdata->ecc_strength = 0;
> +
> +	pdata->ecc_step_size = of_get_nand_ecc_step_size(np);
> +	if (pdata->ecc_step_size < 0)

Ditto for ecc_step_size.

> +		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..5c0f2e3 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 */
> +	unsigned int ecc_strength, ecc_step_size;
> +
>  	const struct mtd_partition		*parts[NUM_CHIP_SELECT];
>  	unsigned int				nr_parts[NUM_CHIP_SELECT];
>  

Brian

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

[Ezequiel Garcia]

Thanks for looking at this.

On 12 May 11:01 AM, Brian Norris wrote:
> On Fri, Mar 21, 2014 at 08:34:49AM -0300, Ezequiel Garcia wrote:
> > 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.
> > 
> > Also, if there is an ONFI-specified minimum ECC strength requirement,
> > and the DT-specified ECC strength is weaker, print a warning and try to
> > select a strength compatible with the ONFI requirement.
> 
> Are you sure you want to override? That seems contrary to the idea of a
> DT property for specifying ECC. But maybe you have a good reason?
> 

Actually, on IRC discussions you enforced the idea that the kernel shouldn't
allow a weaker ECC strength than the datasheet (ONFI or ID table) specified.
Hence, following your request, the implementation considers such devicetree
setting as illegal and tries to find another one.

> If you'd rather just warn the user, it's possible we could move that to
> common code in nand_base.c.
> 

Now that you mention this, I think you're right: it's very stupid to try to
match an ECC scheme, different from the requested.

So, we either just warn the user (nosily) or we fail to probe the driver.
If you ask me, I'd say let's just WARN() and let the user take the blame.

> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/pxa3xx_nand.c                | 47 +++++++++++++++++++--------
> >  include/linux/platform_data/mtd-nand-pxa3xx.h |  3 ++
> >  2 files changed, 36 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index cf7d757..cc13896 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -1344,8 +1344,30 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
> >  }
> >  
> >  static int pxa_ecc_init(struct pxa3xx_nand_info *info,
> > -			struct nand_ecc_ctrl *ecc, int strength, int page_size)
> > +			struct nand_chip *chip,
> > +			struct pxa3xx_nand_platform_data *pdata,
> > +			unsigned int page_size)
> >  {
> > +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> > +	unsigned int strength, onfi_strength;
> > +
> > +	/* We need to normalize the ECC strengths, in order to compare them. */
> > +	strength = (pdata->ecc_strength * 512) / pdata->ecc_step_size;
> > +	onfi_strength = (chip->ecc_strength_ds * 512) / chip->ecc_step_ds;
> 
> This is not necessarily an "ONFI" property; we also have the full-ID
> table in which a minimum ECC strength can be specified. Maybe you can
> match the existing naming with "strength" and "strength_ds" (for
> "datasheet").
>

Right.
 
> > +
> > +	/*
> > +	 * The user requested ECC strength cannot be weaker than the ONFI
> > +	 * minimum specified ECC strength.
> > +	 */
> > +	if (strength && strength < onfi_strength) {
> > +		dev_warn(&info->pdev->dev,
> > +			 "requested ECC strength is too weak\n");
> > +		strength = onfi_strength;
> > +	}
> > +
> > +	if (!strength)
> > +		strength = onfi_strength ? onfi_strength : 1;
> > +
> 
> I'm also not sure your "normalization" is generally correct. You can't
> really normalize correction strengths to get a fully valid comparison.
> Remember, 4-bit/2048B correction is not the same as 1-bit/512B
> correction; it is at least as strong, yes.

So far so good.

> But then, the reverse is
> *not* a good comparison. So your code might say that a flash which
> requires 4-bit/2KB can be satisfied by 1-bit/512B. (These numbers may
> not be seen in practice, but you get my point, right?)
> 

Ah... missed the reverse.

> By my understanding, a correct comparison requires two parts, which I've
> summarized like this in my own code:
> 
>         /*
>          * Does the given configuration meet the 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.
>          */
> 
> I think you are doing (1), but not (2).
> 

Great catch! I missed this detail entirely.

> >  	if (strength == 1 && page_size == 2048) {
> >  		info->chunk_size = 2048;
> >  		info->spare_size = 40;
> > @@ -1375,7 +1397,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 && page_size == 4096) {
> >  		info->ecc_bch = 1;
> > @@ -1424,7 +1445,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
> >  	uint32_t id = -1;
> >  	uint64_t chipsize;
> >  	int i, ret, num;
> > -	uint16_t ecc_strength, ecc_step;
> >  
> >  	if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
> >  		goto KEEP_CONFIG;
> > @@ -1519,17 +1539,8 @@ KEEP_CONFIG:
> >  		}
> >  	}
> >  
> > -	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) {
> > -		ecc_strength = 1;
> > -		ecc_step = 512;
> > -	}
> > -
> > -	ret = pxa_ecc_init(info, &chip->ecc, (ecc_strength * 512) / ecc_step,
> > -			   mtd->writesize);
> > +	/* Selects an ECC and fills pxa3xx_nand_info and nand_chip */
> > +	ret = pxa_ecc_init(info, chip, pdata, mtd->writesize);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1729,6 +1740,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)
> 
> ecc_strength is unsigned, so this error check doesn't work. Maybe you
> want a local 'int ret' to temporarily stash the return value?
> 

Correct.

I'll prepare new patchset.

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Brian Norris]

On Mon, May 12, 2014 at 04:18:41PM -0300, Ezequiel Garcia wrote:
> On 12 May 11:01 AM, Brian Norris wrote:
> > On Fri, Mar 21, 2014 at 08:34:49AM -0300, Ezequiel Garcia wrote:
> > > Also, if there is an ONFI-specified minimum ECC strength requirement,
> > > and the DT-specified ECC strength is weaker, print a warning and try to
> > > select a strength compatible with the ONFI requirement.
> > 
> > Are you sure you want to override? That seems contrary to the idea of a
> > DT property for specifying ECC. But maybe you have a good reason?
> > 
> 
> Actually, on IRC discussions you enforced the idea that the kernel shouldn't
> allow a weaker ECC strength than the datasheet (ONFI or ID table) specified.
> Hence, following your request, the implementation considers such devicetree
> setting as illegal and tries to find another one.

Hmm, I don't recall saying that exactly, although I might have said
something similar that could have been misconstrued as such.

I mostly recall discussing setting the ECC strength *without* device
tree (as pxa3xx_nand currently does), which is a different case, since
we don't have any outside input.

But anyway...

> > If you'd rather just warn the user, it's possible we could move that to
> > common code in nand_base.c.
> > 
> 
> Now that you mention this, I think you're right: it's very stupid to try to
> match an ECC scheme, different from the requested.
> 
> So, we either just warn the user (nosily) or we fail to probe the driver.
> If you ask me, I'd say let's just WARN() and let the user take the blame.

...yes, I (regardless of what "IRC Brian" said!) think that is the
correct approach when given an ECC scheme via device-tree. We should
honor it, and blame the user loudly.

Brian