> From: "Ezequiel Garcia" <ezequiel@xxxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, November 25, 2014 2:08:59 PM
>
> On 11/08/2014 04:11 PM, Aaron Sierra wrote:
> [..]
> > +
> > + /* 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
