* Re: [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic [not found] <1be1ede4dcf14f24b68b8497b167c1b3@SIWEX5A.sing.micron.com> @ 2018-07-11 10:32 ` Boris Brezillon 2018-07-11 11:41 ` [EXT] " Bean Huo (beanhuo) 2018-07-12 8:40 ` Bean Huo (beanhuo) 0 siblings, 2 replies; 6+ messages in thread From: Boris Brezillon @ 2018-07-11 10:32 UTC (permalink / raw) To: Bean Huo (beanhuo) Cc: Richard Weinberger, Miquel Raynal, linux-mtd, Chris Packham, David Woodhouse, Brian Norris, Marek Vasut 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic 2018-07-11 10:32 ` [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic Boris Brezillon @ 2018-07-11 11:41 ` Bean Huo (beanhuo) 2018-07-12 8:40 ` Bean Huo (beanhuo) 1 sibling, 0 replies; 6+ messages in thread From: Bean Huo (beanhuo) @ 2018-07-11 11:41 UTC (permalink / raw) To: Boris Brezillon Cc: Richard Weinberger, Miquel Raynal, linux-mtd, Chris Packham, David Woodhouse, Brian Norris, Marek Vasut >> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic 2018-07-11 10:32 ` [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic Boris Brezillon 2018-07-11 11:41 ` [EXT] " Bean Huo (beanhuo) @ 2018-07-12 8:40 ` Bean Huo (beanhuo) 2018-07-12 8:57 ` Boris Brezillon 1 sibling, 1 reply; 6+ messages in thread From: Bean Huo (beanhuo) @ 2018-07-12 8:40 UTC (permalink / raw) To: Boris Brezillon Cc: Richard Weinberger, Miquel Raynal, linux-mtd, Chris Packham, David Woodhouse, Brian Norris, Marek Vasut 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic 2018-07-12 8:40 ` Bean Huo (beanhuo) @ 2018-07-12 8:57 ` Boris Brezillon 0 siblings, 0 replies; 6+ messages in thread From: Boris Brezillon @ 2018-07-12 8:57 UTC (permalink / raw) To: Bean Huo (beanhuo) Cc: Richard Weinberger, Miquel Raynal, linux-mtd, Chris Packham, David Woodhouse, Brian Norris, Marek Vasut 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC @ 2018-07-09 21:09 Boris Brezillon 2018-07-09 21:09 ` [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic Boris Brezillon 0 siblings, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2018-07-09 21:09 UTC (permalink / raw) To: Boris Brezillon, Richard Weinberger, Miquel Raynal, linux-mtd, Bean Huo (beanhuo), Chris Packham Cc: David Woodhouse, Brian Norris, Marek Vasut Chris, Bean, Here are 2 patches for you to review/test. The first one is fixing the layout definition, and unless I missed something it should be correct. The second one is just my understanding of how byte 5 of READ_ID works based on our experience with the 4bit/512 on-die ECC chip we have worked on and the other datasheet I had a look at. I'm not 100% sure this will work for all chips, but might work for the 2 chips we support right now. So please test and/or review it and let me know if this approach works. Regards, Boris Boris Brezillon (2): mtd: rawnand: micron: Define the proper layout for 8bit/512bytes on-die ECC mtd: rawnand: micron: Fix on-die ECC detection logic drivers/mtd/nand/raw/nand_micron.c | 110 +++++++++++++++++++++++++------------ 1 file changed, 75 insertions(+), 35 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic 2018-07-09 21:09 [PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC Boris Brezillon @ 2018-07-09 21:09 ` Boris Brezillon 2018-07-16 8:36 ` Chris Packham 0 siblings, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2018-07-09 21:09 UTC (permalink / raw) To: Boris Brezillon, Richard Weinberger, Miquel Raynal, linux-mtd, Bean Huo (beanhuo), Chris Packham Cc: David Woodhouse, Brian Norris, Marek Vasut Basing the "mandatory on-die" detection on ID byte 2 does not work, because Micron has plenty of NANDs using the same device ID code, and not all of them have forcibly enabled on-die ECC. Since the "Array Operation" feature does not provide the "ECC enabled/disabled" bit when the ECC can't be disabled, let's try to use the "ECC enabled/disabled" bit in the READ_ID bytes. It seems that this bit is dynamically updated on NANDs where on-die ECC can freely be enabled/disabled, so let's hope it stays at one when we have a NAND with on-die ECC forcibly enabled. Fixes: 51f3b3970a8c ("mtd: rawnand: micron: detect forced on-die ECC") Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/mtd/nand/raw/nand_micron.c | 51 +++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c index 00a965ad7ab2..6ae708fbbd3a 100644 --- a/drivers/mtd/nand/raw/nand_micron.c +++ b/drivers/mtd/nand/raw/nand_micron.c @@ -345,13 +345,8 @@ enum { MICRON_ON_DIE_MANDATORY, }; -/* - * These parts are known to have on-die ECC forceably enabled - */ -static u8 micron_on_die_ecc[] = { - 0xd1, /* MT29F1G08ABAFA */ - 0xa1, /* MT29F1G08ABBFA */ -}; +#define MICRON_ID_INTERNAL_ECC_MASK GENMASK(1, 0) +#define MICRON_ID_ECC_ENABLED BIT(7) /* * Try to detect if the NAND support on-die ECC. To do this, we enable @@ -366,12 +361,8 @@ static u8 micron_on_die_ecc[] = { static int micron_supports_on_die_ecc(struct nand_chip *chip) { u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, }; + u8 id[5]; int ret; - int i; - - for (i = 0; i < ARRAY_SIZE(micron_on_die_ecc); i++) - if (chip->id.data[1] == micron_on_die_ecc[i]) - return MICRON_ON_DIE_MANDATORY; if (!chip->parameters.onfi.version) return MICRON_ON_DIE_UNSUPPORTED; @@ -379,34 +370,42 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip) if (chip->bits_per_cell != 1) return MICRON_ON_DIE_UNSUPPORTED; + /* + * We only support on-die ECC of 4/512 or 8/512 + */ + if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8) + return MICRON_ON_DIE_UNSUPPORTED; + + /* + * We don't know what INTERNAL_ECC means, but it seems that 0x2 matches + * all NANDs with on-die ECC. + */ + if (chip->id.len != 5 || + (chip->id.data[4] & MICRON_ID_INTERNAL_ECC_MASK) != 0x2) + return MICRON_ON_DIE_UNSUPPORTED; + 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; ret = micron_nand_on_die_ecc_setup(chip, false); 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) + if (id[4] & MICRON_ID_ECC_ENABLED) return MICRON_ON_DIE_MANDATORY; - /* - * We only support on-die ECC of 4/512 or 8/512 - */ - if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8) - return MICRON_ON_DIE_UNSUPPORTED; - return MICRON_ON_DIE_SUPPORTED; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic 2018-07-09 21:09 ` [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic Boris Brezillon @ 2018-07-16 8:36 ` Chris Packham 0 siblings, 0 replies; 6+ messages in thread From: Chris Packham @ 2018-07-16 8:36 UTC (permalink / raw) To: Boris Brezillon, Richard Weinberger, Miquel Raynal, linux-mtd, Bean Huo (beanhuo) Cc: David Woodhouse, Brian Norris, Marek Vasut Hi Boris, On 10/07/18 09:09, Boris Brezillon wrote: > Basing the "mandatory on-die" detection on ID byte 2 does not work, > because Micron has plenty of NANDs using the same device ID code, and > not all of them have forcibly enabled on-die ECC. > > Since the "Array Operation" feature does not provide the "ECC > enabled/disabled" bit when the ECC can't be disabled, let's try to use > the "ECC enabled/disabled" bit in the READ_ID bytes. > > It seems that this bit is dynamically updated on NANDs where on-die ECC > can freely be enabled/disabled, so let's hope it stays at one when we > have a NAND with on-die ECC forcibly enabled. > > Fixes: 51f3b3970a8c ("mtd: rawnand: micron: detect forced on-die ECC") > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> With d73e5d29 reverted, Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz> One compiler warning below. > --- > drivers/mtd/nand/raw/nand_micron.c | 51 +++++++++++++++++++------------------- > 1 file changed, 25 insertions(+), 26 deletions(-) > > diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c > index 00a965ad7ab2..6ae708fbbd3a 100644 > --- a/drivers/mtd/nand/raw/nand_micron.c > +++ b/drivers/mtd/nand/raw/nand_micron.c > @@ -345,13 +345,8 @@ enum { > MICRON_ON_DIE_MANDATORY, > }; > > -/* > - * These parts are known to have on-die ECC forceably enabled > - */ > -static u8 micron_on_die_ecc[] = { > - 0xd1, /* MT29F1G08ABAFA */ > - 0xa1, /* MT29F1G08ABBFA */ > -}; > +#define MICRON_ID_INTERNAL_ECC_MASK GENMASK(1, 0) > +#define MICRON_ID_ECC_ENABLED BIT(7) > > /* > * Try to detect if the NAND support on-die ECC. To do this, we enable > @@ -366,12 +361,8 @@ static u8 micron_on_die_ecc[] = { > static int micron_supports_on_die_ecc(struct nand_chip *chip) > { > u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, }; feature is now unused so this causes a compiler warning. > + u8 id[5]; > int ret; > - int i; > - > - for (i = 0; i < ARRAY_SIZE(micron_on_die_ecc); i++) > - if (chip->id.data[1] == micron_on_die_ecc[i]) > - return MICRON_ON_DIE_MANDATORY; > > if (!chip->parameters.onfi.version) > return MICRON_ON_DIE_UNSUPPORTED; > @@ -379,34 +370,42 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip) > if (chip->bits_per_cell != 1) > return MICRON_ON_DIE_UNSUPPORTED; > > + /* > + * We only support on-die ECC of 4/512 or 8/512 > + */ > + if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8) > + return MICRON_ON_DIE_UNSUPPORTED; > + > + /* > + * We don't know what INTERNAL_ECC means, but it seems that 0x2 matches > + * all NANDs with on-die ECC. > + */ > + if (chip->id.len != 5 || > + (chip->id.data[4] & MICRON_ID_INTERNAL_ECC_MASK) != 0x2) > + return MICRON_ON_DIE_UNSUPPORTED; > + > 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; > > ret = micron_nand_on_die_ecc_setup(chip, false); > 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) > + if (id[4] & MICRON_ID_ECC_ENABLED) > return MICRON_ON_DIE_MANDATORY; > > - /* > - * We only support on-die ECC of 4/512 or 8/512 > - */ > - if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8) > - return MICRON_ON_DIE_UNSUPPORTED; > - > return MICRON_ON_DIE_SUPPORTED; > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-16 8:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1be1ede4dcf14f24b68b8497b167c1b3@SIWEX5A.sing.micron.com> 2018-07-11 10:32 ` [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic Boris Brezillon 2018-07-11 11:41 ` [EXT] " Bean Huo (beanhuo) 2018-07-12 8:40 ` Bean Huo (beanhuo) 2018-07-12 8:57 ` Boris Brezillon 2018-07-09 21:09 [PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC Boris Brezillon 2018-07-09 21:09 ` [PATCH 2/2] mtd: rawnand: micron: Fix on-die ECC detection logic Boris Brezillon 2018-07-16 8:36 ` Chris Packham
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.