| Message ID | 20180618045255.8015-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F | expand |
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
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 >
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.
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
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?