[RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

Hi,

I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
to one of our boards which uses the Marvell NFCv2 controller.

This particular chip is a bit odd in that the datasheet states support
for ONFI 1.0 but the revision number field is 00 00. It also is marked
ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
which cannot be disabled.

I think that last point may cause some hassle since the internal ECC
implementation is probably not compatible with the Marvell NFCv2
implementation.

This series is very much RFC because even with these changes so far I
don't have a working system.

Here's a dump of the parameter page if anyone is interested

00000000: 4f 4e 46 49 00 00 18 00 3f 00 00 00 00 00 00 00 ONFI....?.......
00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00000020: 4d 49 43 52 4f 4e 20 20 20 20 20 20 4d 54 32 39  MICRON MT29
00000030: 46 31 47 30 38 41 42 41 47 41 57 50 20 20 20 20  F1G08ABAGAWP    
00000040: 2c 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ,...............
00000050: 00 08 00 00 80 00 00 02 00 00 20 00 40 00 00 00  ..........  .@...
00000060: 00 04 00 00 01 22 01 14 00 01 05 08 00 00 04 00 ....."..........
00000070: 08 01 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00000080: 08 3f 00 3f 00 58 02 10 27 46 00 64 00 00 00 00 .?.?.X..'F.d....
00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000a0: 00 00 00 00 01 00 00 00 00 02 04 80 01 81 04 03 ................
000000b0: 02 01 1e 90 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 85 a6 ................

Chris Packham (2):
  mtd: rawnand: handle ONFI revision number field being 0
  mtd: rawnand: marvell: Support page size of 2048 with 8-bit ECC

 drivers/mtd/nand/raw/marvell_nand.c | 1 +
 drivers/mtd/nand/raw/nand_base.c    | 2 ++
 2 files changed, 3 insertions(+)

-- 
2.17.1

[RFC PATCH 1/2] mtd: rawnand: handle ONFI revision number field being 0

[Chris Packham]

Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
revision number field of the ONFI parameter page. Rather than rejecting
these outright assume ONFI version 1.0 if the revision number is 00 00.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
At the moment I haven't qualified this check on anything, I should
probably at least include vendor == MICRON.

As far as I can tell revision number == 0 is not permitted by the ONFI
spec but this wouldn't be the first time a vendor has ignored a spec. On
the other hand maybe I'm reading the spec wrong and someone here will
say "oh 0 means ...".

 drivers/mtd/nand/raw/nand_base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 0cd3e216b95c..1691c7005ae4 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5184,6 +5184,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 		chip->parameters.onfi.version = 20;
 	else if (val & (1 << 1))
 		chip->parameters.onfi.version = 10;
+	else if (val == 0)
+		chip->parameters.onfi.version = 10;
 
 	if (!chip->parameters.onfi.version) {
 		pr_info("unsupported ONFI version: %d\n", val);
-- 
2.17.1

Re: [RFC PATCH 1/2] mtd: rawnand: handle ONFI revision number field being 0

[Miquel Raynal]

Hi Chris,

On Mon, 18 Jun 2018 16:52:54 +1200, Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:

> Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
> revision number field of the ONFI parameter page. Rather than rejecting
> these outright assume ONFI version 1.0 if the revision number is 00 00.
> 

Thanks for getting your hands into this.

> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> At the moment I haven't qualified this check on anything, I should
> probably at least include vendor == MICRON.

The more I think about it the more I convince myself that this is not
needed. If the 4 first bytes are "ONFI", then the chip is ONFI...

Then what you do below is simple and readable and (sadly) probably
right.

> 
> As far as I can tell revision number == 0 is not permitted by the ONFI
> spec but this wouldn't be the first time a vendor has ignored a spec. On
> the other hand maybe I'm reading the spec wrong and someone here will
> say "oh 0 means ...".
> 
>  drivers/mtd/nand/raw/nand_base.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 0cd3e216b95c..1691c7005ae4 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5184,6 +5184,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  		chip->parameters.onfi.version = 20;
>  	else if (val & (1 << 1))
>  		chip->parameters.onfi.version = 10;
> +	else if (val == 0)
> +		chip->parameters.onfi.version = 10;
>  
>  	if (!chip->parameters.onfi.version) {
>  		pr_info("unsupported ONFI version: %d\n", val);

Regards,
Miquèl

Re: [RFC PATCH 1/2] mtd: rawnand: handle ONFI revision number field being 0

[Boris Brezillon]

On Mon, 18 Jun 2018 15:05:21 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Chris,
> 
> On Mon, 18 Jun 2018 16:52:54 +1200, Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> 
> > Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
> > revision number field of the ONFI parameter page. Rather than rejecting
> > these outright assume ONFI version 1.0 if the revision number is 00 00.
> >   
> 
> Thanks for getting your hands into this.
> 
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> > At the moment I haven't qualified this check on anything, I should
> > probably at least include vendor == MICRON.  
> 
> The more I think about it the more I convince myself that this is not
> needed. If the 4 first bytes are "ONFI", then the chip is ONFI...
> 
> Then what you do below is simple and readable and (sadly) probably
> right.

Hm, I'm not entirely convinced considering version = 0 <=> version = 1
is the right thing to do. Clearly, we don't know if other chips are
also broken WRT version field but the actual version might differ.

I'd recommend letting the manufacturer driver fix the param page
instead of guessing what has to been done in the core. That means
adding a new hook to nand_manufacturer_ops (->fixup_onfi_param_page()?)
and calling it just after the CRC has been checked [1].

[1]https://elixir.bootlin.com/linux/v4.18-rc1/source/drivers/mtd/nand/raw/nand_base.c#L5170

[RFC PATCH 2/2] mtd: rawnand: marvell: Support page size of 2048 with 8-bit ECC

[Chris Packham]

The MT29F1G08ABAFAWP-ITE:F chip has 2048 byte pages and requires a
minimum ECC strength of 8-bits. Allow for this combination of
requirements using the marvell_nand controller.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
I've tried to follow the recommended AN-379 from Marvell. They do seem
to have information that covers this particular set of chip requirements
but I'm not confident I've translated their code correctly into the
current marvell_nand implementation.

This is enough to make the nand_scan work but ubi/ubifs fails to initialise
and/or mount so I may have something completely wrong. This may also be
because this chip has internal ECC enabled which cannot be disabled. I
turned up an old thread on this from April last year[1] but I didn't see
anything resulting from this. Can this combination of ECC
implementations even co-exist?

[1] - http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html

 drivers/mtd/nand/raw/marvell_nand.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ebb1d141b900..5712df553a8e 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -217,6 +217,7 @@ static const struct marvell_hw_ecc_layout marvell_nfc_layouts[] = {
 	MARVELL_LAYOUT(  512,   512,  1,  1,  1,  512,  8,  8,  0,  0,  0),
 	MARVELL_LAYOUT( 2048,   512,  1,  1,  1, 2048, 40, 24,  0,  0,  0),
 	MARVELL_LAYOUT( 2048,   512,  4,  1,  1, 2048, 32, 30,  0,  0,  0),
+	MARVELL_LAYOUT( 2048,   512,  8,  1,  1, 1024, 0, 30,  1024,  32,  30),
 	MARVELL_LAYOUT( 4096,   512,  4,  2,  2, 2048, 32, 30,  0,  0,  0),
 	MARVELL_LAYOUT( 4096,   512,  8,  5,  4, 1024,  0, 30,  0, 64, 30),
 };
-- 
2.17.1

Re: [RFC PATCH 2/2] mtd: rawnand: marvell: Support page size of 2048 with 8-bit ECC

[Miquel Raynal]

Hi Chris,

On Mon, 18 Jun 2018 16:52:55 +1200, Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:

> The MT29F1G08ABAFAWP-ITE:F chip has 2048 byte pages and requires a
> minimum ECC strength of 8-bits. Allow for this combination of
> requirements using the marvell_nand controller.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> I've tried to follow the recommended AN-379 from Marvell. They do seem
> to have information that covers this particular set of chip requirements
> but I'm not confident I've translated their code correctly into the
> current marvell_nand implementation.
> 
> This is enough to make the nand_scan work but ubi/ubifs fails to initialise
> and/or mount so I may have something completely wrong. This may also be
> because this chip has internal ECC enabled which cannot be disabled. I
> turned up an old thread on this from April last year[1] but I didn't see
> anything resulting from this. Can this combination of ECC
> implementations even co-exist?
> 
> [1] - http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html
> 
>  drivers/mtd/nand/raw/marvell_nand.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index ebb1d141b900..5712df553a8e 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -217,6 +217,7 @@ static const struct marvell_hw_ecc_layout marvell_nfc_layouts[] = {
>  	MARVELL_LAYOUT(  512,   512,  1,  1,  1,  512,  8,  8,  0,  0,  0),
>  	MARVELL_LAYOUT( 2048,   512,  1,  1,  1, 2048, 40, 24,  0,  0,  0),
>  	MARVELL_LAYOUT( 2048,   512,  4,  1,  1, 2048, 32, 30,  0,  0,  0),
> +	MARVELL_LAYOUT( 2048,   512,  8,  1,  1, 1024, 0, 30,  1024,  32,  30),

I suppose you should not use HW_ECC for this chip. Hence this line is
useless. However I think it should be:

	MARVELL_LAYOUT( 2048,   512,  8,  2,  1, 1024, 0, 30, 1024, 32,	30),

                                          ^

>  	MARVELL_LAYOUT( 4096,   512,  4,  2,  2, 2048, 32, 30,  0,  0,  0),
>  	MARVELL_LAYOUT( 4096,   512,  8,  5,  4, 1024,  0, 30,  0, 64, 30),
>  };

Regards,
Miquèl

Re: [RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Miquel Raynal]

Hi Chris,

On Mon, 18 Jun 2018 16:52:53 +1200, Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:

> Hi,
> 
> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
> to one of our boards which uses the Marvell NFCv2 controller.
> 
> This particular chip is a bit odd in that the datasheet states support
> for ONFI 1.0 but the revision number field is 00 00. It also is marked
> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
> which cannot be disabled.

Boris and I agree that in this case, the chip should not be probed if
ecc->type != ON_DIE (and eventually NONE).

This should be handled in the Micron driver.

Also, what is the returned value of micron_supports_on_die_ecc() (with
patch 1/2)?

Regards,
Miquèl

Re: [RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

On 19/06/18 01:15, Miquel Raynal wrote:
> Hi Chris,
> 
> On Mon, 18 Jun 2018 16:52:53 +1200, Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> 
>> Hi,
>>
>> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
>> to one of our boards which uses the Marvell NFCv2 controller.
>>
>> This particular chip is a bit odd in that the datasheet states support
>> for ONFI 1.0 but the revision number field is 00 00. It also is marked
>> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
>> which cannot be disabled.
> 
> Boris and I agree that in this case, the chip should not be probed if
> ecc->type != ON_DIE (and eventually NONE).
> 
> This should be handled in the Micron driver.
> 
> Also, what is the returned value of micron_supports_on_die_ecc() (with
> patch 1/2)?

micron_supports_on_die_ecc() returns MICRON_ON_DIE_UNSUPPORTED. 
Technically this chip should be MICRON_ON_DIE_MANDATORY since it can't 
be disabled but that wouldn't be much help since that would still result 
in -EINVAL. I'll dig into micron_supports_on_die_ecc() and see if I can 
find something in the datasheet to use.

> 
> Regards,
> Miquèl
> 

Re: [RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

On 19/06/18 12:35, Chris Packham wrote:
> On 19/06/18 01:15, Miquel Raynal wrote:
>> Hi Chris,
>>
>> On Mon, 18 Jun 2018 16:52:53 +1200, Chris Packham
>> <chris.packham@alliedtelesis.co.nz> wrote:
>>
>>> Hi,
>>>
>>> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
>>> to one of our boards which uses the Marvell NFCv2 controller.
>>>
>>> This particular chip is a bit odd in that the datasheet states support
>>> for ONFI 1.0 but the revision number field is 00 00. It also is marked
>>> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
>>> which cannot be disabled.
>>
>> Boris and I agree that in this case, the chip should not be probed if
>> ecc->type != ON_DIE (and eventually NONE).
>>
>> This should be handled in the Micron driver.
>>
>> Also, what is the returned value of micron_supports_on_die_ecc() (with
>> patch 1/2)?
> 
> micron_supports_on_die_ecc() returns MICRON_ON_DIE_UNSUPPORTED.
> Technically this chip should be MICRON_ON_DIE_MANDATORY since it can't
> be disabled but that wouldn't be much help since that would still result
> in -EINVAL. I'll dig into micron_supports_on_die_ecc() and see if I can
> find something in the datasheet to use.
> 

Some further debugging. Nothing (in 4.17) calls 
set_bit(ONFI_FEATURE_ON_DIE_ECC) so I don't think 
micron_supports_on_die_ecc() can return anything other than 
MICRON_ON_DIE_UNSUPPORTED, unless I'm missing something for how the 
{get,set}_feature_list is populated.

With the onfi.version fix and the following

--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -66,7 +66,9 @@ static int micron_nand_onfi_init(struct nand_chip *chip)

         if (p->supports_set_get_features) {
                 set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->set_feature_list);
+               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->set_feature_list);
                 set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);
+               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->get_feature_list);
         }
@@ -240,7 +246,7 @@ static int micron_supports_on_die_ecc(struct 
nand_chip *chip)
          * Some Micron NANDs have an on-die ECC of 4/512, some other
-         * 8/512. We only support the former.
+         * 8/512.
          */
-       if (chip->ecc_strength_ds != 4)
+       if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
                 return MICRON_ON_DIE_UNSUPPORTED;

I can get micron_supports_on_die_ecc() to return MICRON_ON_DIE_SUPPORTED.

Then I run into a problem with the marvell_nand.c which currently 
doesn't handle NAND_ECC_ON_DIE which is easily fixed.

But then I have the issue that I need to handle systems with either type 
of ECC scheme ("on-die" or "hw") which I'm not sure is even possible 
within the dts.

I'll re-base against 4.18-rc1 and send what I have so-far.

Re: [RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Boris Brezillon]

Hi Chris,

On Tue, 19 Jun 2018 01:44:24 +0000
Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:

> On 19/06/18 12:35, Chris Packham wrote:
> > On 19/06/18 01:15, Miquel Raynal wrote:  
> >> Hi Chris,
> >>
> >> On Mon, 18 Jun 2018 16:52:53 +1200, Chris Packham
> >> <chris.packham@alliedtelesis.co.nz> wrote:
> >>  
> >>> Hi,
> >>>
> >>> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
> >>> to one of our boards which uses the Marvell NFCv2 controller.
> >>>
> >>> This particular chip is a bit odd in that the datasheet states support
> >>> for ONFI 1.0 but the revision number field is 00 00. It also is marked
> >>> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
> >>> which cannot be disabled.  
> >>
> >> Boris and I agree that in this case, the chip should not be probed if
> >> ecc->type != ON_DIE (and eventually NONE).
> >>
> >> This should be handled in the Micron driver.
> >>
> >> Also, what is the returned value of micron_supports_on_die_ecc() (with
> >> patch 1/2)?  
> > 
> > micron_supports_on_die_ecc() returns MICRON_ON_DIE_UNSUPPORTED.
> > Technically this chip should be MICRON_ON_DIE_MANDATORY since it can't
> > be disabled but that wouldn't be much help since that would still result
> > in -EINVAL. I'll dig into micron_supports_on_die_ecc() and see if I can
> > find something in the datasheet to use.
> >   
> 
> Some further debugging. Nothing (in 4.17) calls 
> set_bit(ONFI_FEATURE_ON_DIE_ECC) so I don't think 
> micron_supports_on_die_ecc() can return anything other than 
> MICRON_ON_DIE_UNSUPPORTED, unless I'm missing something for how the 
> {get,set}_feature_list is populated.

Nope you're not. Looks like we broke Micron on-die ECC in 4.17.

> 
> With the onfi.version fix and the following
> 
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -66,7 +66,9 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
> 
>          if (p->supports_set_get_features) {
>                  set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->set_feature_list);
> +               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->set_feature_list);
>                  set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);
> +               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->get_feature_list);
>          }

Can you send a patch containing only the above changes with the
Cc-stable and Fixes tags?

> @@ -240,7 +246,7 @@ static int micron_supports_on_die_ecc(struct 
> nand_chip *chip)
>           * Some Micron NANDs have an on-die ECC of 4/512, some other
> -         * 8/512. We only support the former.
> +         * 8/512.
>           */
> -       if (chip->ecc_strength_ds != 4)
> +       if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>                  return MICRON_ON_DIE_UNSUPPORTED;
>

This should be done in a separate patch.
 
> I can get micron_supports_on_die_ecc() to return MICRON_ON_DIE_SUPPORTED.
> 

That's weird. You should have MICRON_ON_DIE_MANDATORY here. Could it be
that the ONFI_FEATURE_ON_DIE_ECC_EN bit does not really reflect the ECC
engine state? If that's the case, we'll have to change the way we
detect if on-die ECC is supported/mandatory/not-supported (based on the
model name stored in the ONFI param page?).

> Then I run into a problem with the marvell_nand.c which currently 
> doesn't handle NAND_ECC_ON_DIE which is easily fixed.

Yep, adding that to the new driver should be pretty easy.

> 
> But then I have the issue that I need to handle systems with either type 
> of ECC scheme ("on-die" or "hw") which I'm not sure is even possible 
> within the dts.

You mean having the same dts for both setup. Indeed, that's not
supported right now.

> 
> I'll re-base against 4.18-rc1 and send what I have so-far.

Cool. I'm particularly interested by the SET/GET_FEATURE(ECC) fix.

Thanks,

Boris

Re: [RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

Adding participants from 
http://lists.infradead.org/pipermail/linux-mtd/2017-March/072974.html

On 19/06/18 16:56, Boris Brezillon wrote:

> Hi Chris, >
> On Tue, 19 Jun 2018 01:44:24 +0000
> Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:
> 
>> On 19/06/18 12:35, Chris Packham wrote:
>>> On 19/06/18 01:15, Miquel Raynal wrote:
>>>> Hi Chris,
>>>>
>>>> On Mon, 18 Jun 2018 16:52:53 +1200, Chris Packham
>>>> <chris.packham@alliedtelesis.co.nz> wrote:
>>>>   
>>>>> Hi,
>>>>>
>>>>> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
>>>>> to one of our boards which uses the Marvell NFCv2 controller.
>>>>>
>>>>> This particular chip is a bit odd in that the datasheet states support
>>>>> for ONFI 1.0 but the revision number field is 00 00. It also is marked
>>>>> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
>>>>> which cannot be disabled.
>>>>
>>>> Boris and I agree that in this case, the chip should not be probed if
>>>> ecc->type != ON_DIE (and eventually NONE).
>>>>
>>>> This should be handled in the Micron driver.
>>>>
>>>> Also, what is the returned value of micron_supports_on_die_ecc() (with
>>>> patch 1/2)?
>>>
>>> micron_supports_on_die_ecc() returns MICRON_ON_DIE_UNSUPPORTED.
>>> Technically this chip should be MICRON_ON_DIE_MANDATORY since it can't
>>> be disabled but that wouldn't be much help since that would still result
>>> in -EINVAL. I'll dig into micron_supports_on_die_ecc() and see if I can
>>> find something in the datasheet to use.
>>>    
>>
>> Some further debugging. Nothing (in 4.17) calls
>> set_bit(ONFI_FEATURE_ON_DIE_ECC) so I don't think
>> micron_supports_on_die_ecc() can return anything other than
>> MICRON_ON_DIE_UNSUPPORTED, unless I'm missing something for how the
>> {get,set}_feature_list is populated.
> 
> Nope you're not. Looks like we broke Micron on-die ECC in 4.17.
> 
>>
>> With the onfi.version fix and the following
>>
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>> @@ -66,7 +66,9 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
>>
>>           if (p->supports_set_get_features) {
>>                   set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->set_feature_list);
>> +               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->set_feature_list);
>>                   set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);
>> +               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->get_feature_list);
>>           }
> 
> Can you send a patch containing only the above changes with the
> Cc-stable and Fixes tags?
> 
>> @@ -240,7 +246,7 @@ static int micron_supports_on_die_ecc(struct
>> nand_chip *chip)
>>            * Some Micron NANDs have an on-die ECC of 4/512, some other
>> -         * 8/512. We only support the former.
>> +         * 8/512.
>>            */
>> -       if (chip->ecc_strength_ds != 4)
>> +       if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>>                   return MICRON_ON_DIE_UNSUPPORTED;
>>
> 
> This should be done in a separate patch.
>   
>> I can get micron_supports_on_die_ecc() to return MICRON_ON_DIE_SUPPORTED.
>>
> 
> That's weird. You should have MICRON_ON_DIE_MANDATORY here. Could it be
> that the ONFI_FEATURE_ON_DIE_ECC_EN bit does not really reflect the ECC
> engine state? If that's the case, we'll have to change the way we
> detect if on-die ECC is supported/mandatory/not-supported (based on the
> model name stored in the ONFI param page?).
> 

Even though though MT29F1G08ABAFAWP-ITE:F says on-die ECC is enabled and 
cannot be disabled it still seems to respond to 
micron_nand_on_die_ecc_setup(chip, false); by clearing the feature bit 
retrieved by nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature).

I see in the original thread that the detection of the 70s parts can be 
done by the "Number of bits ECC correctability". Can we assume that all 
70s has MICRON_ON_DIE_MANDATORY or do I need to make it based on 
specific IDs?