Re: [PATCH v2] mtd: fsl_upm: Support NAND ECC DTS properties

From: "Ezequiel Garcia" <redacted>
Sent: Tuesday, November 25, 2014 2:08:59 PM

On 11/08/2014 04:11 PM, Aaron Sierra wrote:
[..]

quoted

+
+	/* We know mode is either NAND_ECC_SOFT or NAND_ECC_SOFT_BCH */
+	if (strength < 0 && mode == NAND_ECC_SOFT_BCH) {
+		dev_err(fun->dev,
+			"ECC BCH mode requires nand-ecc-strength property");
+		ret = -EINVAL;
+		goto err;
+	} else if (strength == 0) {
+		dev_err(fun->dev, "ECC strength of 0 bits is unsupported");
+		ret = -EINVAL;
+		goto err;
+	} else if (strength == 1 && mode == NAND_ECC_SOFT_BCH) {
+		dev_err(fun->dev, "ECC BCH mode requires > 1-bit strength");
+		ret = -EINVAL;
+		goto err;
+	} else if (strength > 1 && mode == NAND_ECC_SOFT) {
+		dev_warn(fun->dev,
+			"Forcing ECC BCH due to %d-bit strength\n", strength);
+		mode = NAND_ECC_SOFT_BCH;
+	}
+	fun->chip.ecc.mode = mode;
+	fun->chip.ecc.strength = strength;
+

Aside from my comment about the lack of ECC specification in the
binding, I think the above is wrong.

You don't have hardware ECC, but software ECC (either hamming or BCH).
So, you don't need to specify any nand_ecc_ctrl.strength (i.e.
ecc.strength above).

It'll be set by the NAND core and override any value you set
See nand_scan_tail.

Ezequiel,
This patch was originally the second of two patches. The first was applied
to l2-mtd.git on 11/5/2014:

    mtd: nand: Base BCH ECC bytes on required strength

It affects the code in nand_scan_tail that you're referring to so that it
looks like this:

		/*
		 * Board driver should supply ecc.size and ecc.bytes values to
		 * select how many bits are correctable; see nand_bch_init()
		 * for details. Otherwise, default to 4 bits for large page
		 * devices.
		 */
		if (!ecc->size && (mtd->oobsize >= 64)) {
			ecc->size = 512;
			ecc->bytes = DIV_ROUND_UP(13 * ecc->strength, 8);
		}
		ecc->priv = nand_bch_init(mtd, ecc->size, ecc->bytes,
					       &ecc->layout);
		if (!ecc->priv) {
			pr_warn("BCH ECC initialization failed!\n");
			BUG();
		}
		ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size);

In this case ecc->strength is unnecessarily recalculated at the end and
ecc->strength isn't checked for zero in the initial calculation. Perhaps
this block in nand_scan_tail should be patched again to preserve the
original behaviour if ecc->strength or ecc->size are zero.

The only other in-kernel user of SOFT_BCH is sunxi_nand.c and it calculates
ecc->bytes from ecc->size and ecc->strength itself, so it has defined all
three values by this point.

So, I'd say you just need to specify the nand-ecc-mode in the devicetree
binding.

The nand-ecc-strength and nand-ecc-step-size are meant for controllers
with hardware ECC support.

Software BCH allows controllers without hardware support for multiple
bit correction to be used with NAND devices that require multiple bit
correction, so like a hardware controller it needs to know how many
bits to correct.

--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html