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

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

[Boris Brezillon]

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

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

[Bean Huo (beanhuo)]

>> 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...
>
This is very interesting. MT29F2G08ABAEAH4 is 60s NAND, unfortunately, I don't have 60s sample.
I will check, and back you.

>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

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

[Bean Huo (beanhuo)]

Hi, Boris
It is true. Bit7 of byte4 in READID changes after enable/disable internal ECC in case of  its default value is 0. 
Also this bit value  is volatile. After power cyling, it resets to default value. I don't know if
this condition will be changed or kept since maybe it was a specific design request from customers.
But GET Feature is still the preferred way to determine the state of ECC, we recommend using this way. 
If internal ECC is default on, we don't do anything, and if default is off, then SET feature and GET feature.
This always makes sense.

I am still digging into why this doesn't depict in datasheet and whether this will be kept in coming design.
As long I have new update, I will back to you.

>
>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

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

[Boris Brezillon]

On Thu, 12 Jul 2018 08:40:33 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris
> It is true. Bit7 of byte4 in READID changes after enable/disable internal ECC in case of  its default value is 0. 
> Also this bit value  is volatile. After power cyling, it resets to default value.

That's actually a good thing that ECC status get resets on PoR. I'd
wish that would be the case when issuing a SW reset (RESET command),
but unfortunately it's not.

> I don't know if
> this condition will be changed or kept since maybe it was a specific design request from customers.
> But GET Feature is still the preferred way to determine the state of ECC, we recommend using this way. 
> If internal ECC is default on, we don't do anything, and if default is off, then SET feature and GET feature.
> This always makes sense.

No, it doesn't work. When Linux acquires control on the NAND, we don't
know what was done before (maybe u-boot enabled ECC and didn't disable
it), so checking READID[4].bit7 only does not work. We need to first
disable the ECC through the SET_FEATURES(DIS_ECC) and then check the
READID bytes.

Also, using GET_FEATURES() does not work either, because Micron NANDs
with "forced on-die ECC" do not expose the ECC status in the 0x90
(Array Operation Mode) feature reg. So if we do that we report ECC as
always disabled which means "not supported"

> 
> I am still digging into why this doesn't depict in datasheet and whether this will be kept in coming design.
> As long I have new update, I will back to you.

Arrrggg! Please don't change that in future designs. We seem to have
something that works to detect whether on-die ECC is supported and if
it can be disabled.