Re: [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic

[Boris Brezillon <boris.brezillon@bootlin.com>]

Hi Bean,

On Wed, 11 Jul 2018 10:05:06 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> 
> >+	if (chip->id.len != 5 ||
> >+	    (chip->id.data[4] & MICRON_ID_INTERNAL_ECC_MASK) != 0x2)
> >+		return MICRON_ON_DIE_UNSUPPORTED;
> >+  
> 
> I think, we should firstly check if by default internal ECC is on.
> If default is off, then can try to enable through set/get feature command.
> if default is on, there is no need to set up completely.

No, see my comment below.

>   
> 
> > 	ret = micron_nand_on_die_ecc_setup(chip, true);
> > 	if (ret)
> > 		return MICRON_ON_DIE_UNSUPPORTED;
> >  
> 
> >-	ret = nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
> >-	if (ret < 0)
> >-		return ret;
> >+	ret = nand_readid_op(chip, 0, id, sizeof(id));
> >+	if (ret)
> >+		return MICRON_ON_DIE_UNSUPPORTED;
> >
> >-	if ((feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN) == 0)
> >+	if (!(id[4] & MICRON_ID_ECC_ENABLED))
> > 		return MICRON_ON_DIE_UNSUPPORTED;
> >  
> 
> Here I say for sure, the value of Bit7 in byte 4 of READID is permanent. It is a reflection of
> the default state of the device. it cannot change by set feature.

That's not what the board I have on my desk says. I have a
MT29F2G08ABAEAH4, and I can tell you for sure this bit changes when I
enable/disable on-die ECC. Do the test, and you'll see...

See, that's what I'm complaining about. You keep saying things that
appear to be untrue when when we check on real HW parts. So, either we
have NANDs that are buggy, or you didn't check yourself.

Regards,

Boris