Re: [PATCH v2] mtd: fsl_elbc_nand Add ECC mode selection in DT

[Scott Wood <scottwood@freescale.com>]

On Thu, 2015-05-21 at 13:46 +0200, Tomas Hlavacek wrote:
> Add device tree pointer to chip structure in order to allow turn off the
> HW ECC and force own ECC mode and ECC parameters. Newly supported entries
> are as per documentation: nand-ecc-mode, nand-ecc-step-size
> and nand-ecc-strength.

If you're using these properties with eLBC, the eLBC binding needs to be
updated to say that they're allowed for use with software ECC.

> @@ -677,8 +689,8 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
>  		priv->page_size = 1;
>  		setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
>  		/* adjust ecc setup if needed */
> -		if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> -		    BR_DECC_CHK_GEN) {
> +		if (((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> +		    BR_DECC_CHK_GEN) && (chip->ecc.mode == NAND_ECC_HW)) {

There's no need to check both.  If you're selecting software ECC in the
device tree then you must also remove ECC checking from BRn.

If you want to make Linux write to BRn to ensure that it's consistent
with ecc.mode after the DT init, fine, but don't just silently proceed
without fixing it if there's an inconsistency.

> @@ -786,6 +801,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>  		chip->ecc.size = 512;
>  		chip->ecc.bytes = 3;
>  		chip->ecc.strength = 1;
> +		chip->ecc.write_subpage = fsl_elbc_write_subpage;
>  	} else {
>  		/* otherwise fall back to default software ECC */
>  		chip->ecc.mode = NAND_ECC_SOFT;

...but here you are relying on BRn to be set correctly before
nand_dt_init(), so I'm even more confused by what the previous change is
for.

-Scott