Re: [PATCH 4/5] mtd: nand: add support for Micron on-die ECC

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 11 Apr 2017 16:49:52 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Tue, 11 Apr 2017 14:26:02 +0000
> "Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:
> 
> > >
> > >Hi Bean,
> > >
> > >On Mon, 3 Apr 2017 11:31:05 +0000
> > >"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:
> > >    
> > >> Hi, Boris and Thomas
> > >>    
> > >> >>
> > >> >> Ok, but I recommend that 70s should be the first choice on this
> > >> >> single solution, it doesn't need to read twice to detect its bitflips count.    
> > >> >
> > >> >That's exactly why we need to differentiate the 2 chips.    
> > >>
> > >> Sorry for later this response.
> > >> Below is the pseudo codes about how to differentiate these 2 series
> > >> parallel NAND with on-die ECC:
> > >>
> > >> if (NAND == SLC ) { // on-die ECC only exists in SLC //check device ID
> > >> byte 4
> > >>      if ((ID.byte4 & 0x02) == 0x02) {// internal ECC level ==10b    
> > >
> > >So here the MT29F1G08ABADAWP datasheet says 0x2 <=> 4bit/512bytes ECC.
> > >    
> > >> 	if (ID.byte4 & 0x80) {//on-Die ECC enabled    
> > >
> > >Did you read my last reply?
> > >Thomas discovered that ID[4].bit7 is actually reflecting the ECC engine state (1 if
> > >the engine is enabled, 0 if it's disabled), not whether the NAND supports on-die
> > >ECC or not, so no this test is not reliable.
> > >    
> > >>                     if (ONFI.byte112 == 4)
> > >> 		 60s SLC NAND with on-die ECC
> > >> 	    else if (ONFI.byte112 == 8)
> > >>      	              70s SLC NAND with on-die ECC    
> > >
> > >This is completely fucked up! Now the ONFI param page says the NAND requires
> > >8bits/512bytes, while the ID bytes advertised an on-die ECC providing
> > >4bits/512bytes correctability.
> > >So either your algorithm is wrong, or the ID and ONFI param page are contracting
> > >(not sure what solution I'd prefer...).
> > >    
> > >> 	    else
> > >>                           Doesn't support on-die ECC    
> > >
> > >Sorry to say that, but I find it worrisome that even someone from Micron is not
> > >able to get it right.
> > >    
> > 
> > Sorry, would you please specify which one is wrong or confuse you?  
> 
> The initial 'if (ID.byte4 & 0x80)' is wrong, because this bit is only
> set when someone enabled the ECC engine using the SET_FEATURE command
> (this has been verified by Thomas who tried to disable the feature in
> the bootloader and noticed that on-die ECC was reported as
> 'unsupported' by the kernel).
> 
> Maybe I was wrong about your 'if ((ID.byte4 & 0x02) == 0x02)' test,
> because you apparently only mask bit 1 and not bits 0 and 1.
> Anyway, I can't tell if this is valid because I don't have access to
> the M79A datasheets you're referring to.

Okay, I managed to download the MT29F2G08ABAGAWP datasheet (from the
MT79A family), and it seems that the test should be

	if ((ID.byte4 & 0x03) == 0x02)

and not

	if ((ID.byte4 & 0x02) == 0x02)

Also, this field named "Internal ECC level" clearly does not reflect
the on-die ECC strength because it's set to the same value on both
parts (0x2) while MT29F2G08ABAGAWP provides 8bits/512bytes and
MT29F1G08ABADAWP 4bits/512bytes.

See why I say we can't rely on READ_ID information. It's changing all
the time, and nothing clearly say how to differentiate the scheme used
in a specific NAND part.