Re: [PATCH] mtd: fsl_upm: Enable software BCH ECC support

[Brian Norris <computersforpeace@gmail.com>]

On Wed, Aug 26, 2015 at 11:22:18AM -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Brian Norris" <computersforpeace@gmail.com>
> > Sent: Tuesday, August 25, 2015 4:34:08 PM

> > > +	case NAND_ECC_SOFT:
> > > +		if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1)
> > > +			dev_warn(fun->dev, "Ignoring %d-bit software ECC\n",
> > > +				fun->chip.ecc.strength);
> > 
> > Do you really want to ignore this? I think it's fair to make this an
> > error case in nand_scan_tail(). Like:
> > 
> > 	case NAND_ECC_SOFT:
> > 		...
> > 		if (chip->ecc.strength && chip->ecc.strength != 1) {
> > 			// error out
> > 			// we really need to kill the heavy usage of
> > 			// BUG() in nand_scan_tail()...
> > 		}
> 
> My rationale was that for "soft" ECC mode, ecc.strength is really a
> don't-care property; the driver doesn't use it since the algorithm only
> supports 1-bit. Therefore it could be OK to output a warning and carry on.

Understood.

> I can see the other side, too, where we might want all of the
> user-supplied values to agree with the capabilities of the driver.

Right. I especially would want to keep sanity on the device tree, as
this is something closer to an ABI (depending on who you ask...) than
internal driver-provided values.

> If I rework this, I would intend to accept zeros for ecc.strength and
> ecc.size as valid for this mode (size defaults to 256 within nand_ecc.c).
> Does that sit well with you?

Sure, that's fair. We've been allowing this in nand_scan_tail() already
(we tend to fill in reasonable defaults if the driver doesn't specify),
so I don't want to remove that fallback. I just don't want people
specifying 4-bit hamming ECC in their DT and not noticing your little
warning log message saying that we're ignoring them and using 1-bit.

But the DT binding doc should not suggest that they can be left at 0. If
you're up for it, perhaps you can add some details in
Documentation/devicetree/bindings/mtd/nand.txt about the "soft" and
"soft_bch" ECC modes while you're at it.

Thanks,
Brian