| Message ID | 1396025800-18444-1-git-send-email-davidm@egauge.net |
|---|---|
| State | Rejected |
| Headers | show |
a few general notes on your style of patch submission: please check how subject lines usually are created, the above "[REV3]" is wrong in multiple ways -- see if you can detect a pattern in how other series are re-sent please do provide a history of changes upon re-iteration, don't expect others to dig up those details for you -- help those people whom you expect to help you it's hard to determine how this followup patch relates to the former on-die-ECC introduction, please provide comments on how things interact with or depend on each other when multiple sets of patches are floating around -- is the on-die-ECC support patch obsolete, do you want to re-start by creating infrastructure in preparation, to add more support later? if this is not an iteration of the on-die-ECC support, why is it v3 then? can you provide this information for those of us who are not as much into the details of your work as you are? On Fri, 2014-03-28 at 10:56 -0600, David Mosberger wrote: > > This patch adds NAND_ECC_HW_ON_DIE and all other changes to generic code. > On-die ECC detection now has moved into nand_onfi_detect_micron(). I guess this was too quick a change with too little of information, and should not get applied, see below > @@ -3049,16 +3049,29 @@ static int nand_setup_read_retry_micron(struct mtd_info *mtd, int retry_mode) > /* > * Configure chip properties from Micron vendor-specific ONFI table > */ > -static void nand_onfi_detect_micron(struct nand_chip *chip, > - struct nand_onfi_params *p) > +static void nand_onfi_detect_micron(struct mtd_info *mtd, > + struct nand_chip *chip, struct nand_onfi_params *p) > { > struct nand_onfi_vendor_micron *micron = (void *)p->vendor; > + u8 features[ONFI_SUBFEATURE_PARAM_LEN]; > > if (le16_to_cpu(p->vendor_revision) < 1) > return; > > chip->read_retries = micron->read_retry_options; > chip->setup_read_retry = nand_setup_read_retry_micron; > + > + if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_OP_MODE, > + features) >= 0) { > + if (features[0] & ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC) { > + /* > + * If the chip has on-die ECC enabled, we kind > + * of have to do the same... > + */ > + chip->ecc.mode = NAND_ECC_HW_ON_DIE; > + pr_info("Using on-die ECC\n"); > + } > + } > } ISTR that the test should not be done for a single bit, but for the specific 0x08 data pattern, i.e. for equality of the byte can you re-check the documentation? this is how I read table 14 putting the new code into a separate routine and bailing out upon unmet conditions might help to avoid a few levels of indentation > @@ -3792,13 +3805,24 @@ int nand_scan_tail(struct mtd_info *mtd) > !(chip->bbt_options & NAND_BBT_USE_FLASH)); > > if (!(chip->options & NAND_OWN_BUFFERS)) { > + size_t on_die_bufsz = 0; > + > + if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) > + on_die_bufsz = 2*(mtd->writesize + mtd->oobsize); > + > nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize > - + mtd->oobsize * 3, GFP_KERNEL); > + + mtd->oobsize * 3 + on_die_bufsz, GFP_KERNEL); > if (!nbuf) > return -ENOMEM; > nbuf->ecccalc = (uint8_t *)(nbuf + 1); > nbuf->ecccode = nbuf->ecccalc + mtd->oobsize; > nbuf->databuf = nbuf->ecccode + mtd->oobsize; > + if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) { > + nbuf->chkbuf = (nbuf->databuf + mtd->writesize > + + mtd->oobsize); > + nbuf->rawbuf = (nbuf->chkbuf + mtd->writesize > + + mtd->oobsize); > + } > > chip->buffers = nbuf; > } else { so the additional buffers only get allocated when upon identification the on-die-ECC feature already is enabled? not when it's supported, and might get enabled at any later point in time? this might be OK if there is no intention to enable the on-die-ECC feature at runtime, but only to use it when enabled by other entities earlier in the boot progress -- though I guess the comments and the commit message should reflect such a limitation that applies by design > @@ -214,6 +215,10 @@ struct nand_chip; > /* Vendor-specific feature address (Micron) */ > #define ONFI_FEATURE_ADDR_READ_RETRY 0x89 > > +/* Vendor-specific array operation mode (Micron) */ > +#define ONFI_FEATURE_ADDR_OP_MODE 0x90 > +#define ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC 0x08 > + > /* ONFI subfeature parameters length */ > #define ONFI_SUBFEATURE_PARAM_LEN 4 > is something like "array op mode" more appropriate, in case other things might have their individual mode of operation in the future? virtually yours Gerhard Sittig
On Sat, Mar 29, 2014 at 7:16 AM, Gerhard Sittig <gsi@denx.de> wrote: > ISTR that the test should not be done for a single bit, but for > the specific 0x08 data pattern, i.e. for equality of the byte > > can you re-check the documentation? this is how I read table 14 Yes, the data sheet certainly makes it look that way. However, Micron's own sample code checks individual bits. Also, I don't know why the other array-op features (OTP) would be exclusive with internal ECC, so I'm leaning towards believing the sample code (see function test_on_die_ecc() on page 8 & 9): http://www.micron.com/~/media/Documents/Products/Technical%20Note/NAND%20Flash/tn2956_ondie_ecc_omap3_linux.pdf >> if (!(chip->options & NAND_OWN_BUFFERS)) { >> + size_t on_die_bufsz = 0; >> + >> + if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) >> + on_die_bufsz = 2*(mtd->writesize + mtd->oobsize); >> + >> nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize >> - + mtd->oobsize * 3, GFP_KERNEL); >> + + mtd->oobsize * 3 + on_die_bufsz, GFP_KERNEL); >> if (!nbuf) >> return -ENOMEM; >> nbuf->ecccalc = (uint8_t *)(nbuf + 1); >> nbuf->ecccode = nbuf->ecccalc + mtd->oobsize; >> nbuf->databuf = nbuf->ecccode + mtd->oobsize; >> + if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) { >> + nbuf->chkbuf = (nbuf->databuf + mtd->writesize >> + + mtd->oobsize); >> + nbuf->rawbuf = (nbuf->chkbuf + mtd->writesize >> + + mtd->oobsize); >> + } >> >> chip->buffers = nbuf; >> } else { > > so the additional buffers only get allocated when upon > identification the on-die-ECC feature already is enabled? Yes. > not > when it's supported, and might get enabled at any later point in > time? There is no way to switch ECC mode later on, is there? >> /* Vendor-specific feature address (Micron) */ >> #define ONFI_FEATURE_ADDR_READ_RETRY 0x89 >> >> +/* Vendor-specific array operation mode (Micron) */ >> +#define ONFI_FEATURE_ADDR_OP_MODE 0x90 >> +#define ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC 0x08 >> + >> /* ONFI subfeature parameters length */ >> #define ONFI_SUBFEATURE_PARAM_LEN 4 >> > > is something like "array op mode" more appropriate, in case other > things might have their individual mode of operation in the future? Either way is fine with me. ARRAY_OP_MODE is more accurate, but it tends to get long and there is no other op-mode at this point in time. --david
On Mon, 2014-03-31 at 12:33 -0600, David Mosberger wrote: > > On Sat, Mar 29, 2014 at 7:16 AM, Gerhard Sittig <gsi@denx.de> wrote: > > > so the additional buffers only get allocated when upon > > identification the on-die-ECC feature already is enabled? > > Yes. > > > not > > when it's supported, and might get enabled at any later point in > > time? > > There is no way to switch ECC mode later on, is there? Isn't there the ONFI SETFEATURE request? And don't you use this very request to disable and enable chip internal ECC support, to get the raw (uncorrected) bits after bitflips were detected? So I understand that there is support to enable this mode at will, and we already have (or will have) code to do so. If we consider disabling on-die-ECC support when we find it enabled and know it's not what the user wants to run, then the next logical step might be to support enabling this feature if the user wants it and the chip doesn't have it enabled at this point in time. So I guess the buffers should get allocated as soon as the chip supports on-die-ECC. (While the allocation should only get introduced in the phase where the raw-read and bitflip count gets added.) virtually yours Gerhard Sittig
Gerhard, On Mon, Mar 31, 2014 at 1:45 PM, Gerhard Sittig <gsi@denx.de> wrote: > Isn't there the ONFI SETFEATURE request? And don't you use this > very request to disable and enable chip internal ECC support, to > get the raw (uncorrected) bits after bitflips were detected? So > I understand that there is support to enable this mode at will, > and we already have (or will have) code to do so. > > If we consider disabling on-die-ECC support when we find it > enabled and know it's not what the user wants to run, then the > next logical step might be to support enabling this feature if > the user wants it and the chip doesn't have it enabled at this > point in time. > > So I guess the buffers should get allocated as soon as the chip > supports on-die-ECC. (While the allocation should only get > introduced in the phase where the raw-read and bitflip count gets > added.) The ECC-mode of the driver never changes. Sure, what you describe could be implemented as an additional feature in the future (I don't have any plans to do so since it makes little sense to me, as no other ECC-mode uses the layout of the on-die-controller). To be honest, I'm a bit confused: on the one hand, you're asking me to simplify the patches into smaller pieces, on the other you seem to ask for new features. --david
Hi David, >From: David Mosberger [mailto:davidm@egauge.net] >>On Mon, Mar 31, 2014 at 1:45 PM, Gerhard Sittig <gsi@denx.de> wrote: > >> Isn't there the ONFI SETFEATURE request? And don't you use this >> very request to disable and enable chip internal ECC support, to >> get the raw (uncorrected) bits after bitflips were detected? So >> I understand that there is support to enable this mode at will, >> and we already have (or will have) code to do so. >> >> If we consider disabling on-die-ECC support when we find it >> enabled and know it's not what the user wants to run, then the >> next logical step might be to support enabling this feature if >> the user wants it and the chip doesn't have it enabled at this >> point in time. >> >> So I guess the buffers should get allocated as soon as the chip >> supports on-die-ECC. (While the allocation should only get >> introduced in the phase where the raw-read and bitflip count gets >> added.) > >The ECC-mode of the driver never changes. >Sure, what you describe could be implemented as an additional feature >in the future (I don't have any plans to do so since it makes little sense to >me, as no other ECC-mode uses the layout of the on-die-controller). > >To be honest, I'm a bit confused: on the one hand, you're asking me to >simplify the patches into smaller pieces, on the other you seem to ask >for new features. > I too would like to see ECC-mode selection based on user's choice. So there should be mechanism to enable/disable this feature based on DT or platform-data. And onfi_set_feature() is the correct thing to use. This would make your patches more simple, without disturbing the generic frame-work. If you patch causes any regression, or abruptly changes behavior of any working driver, then it's less likely to get accepted. So, instead of re-sending patches it's important to discuss the changes. with regards, pekon
On Mon, Mar 31, 2014 at 02:58:12PM -0600, David Mosberger wrote: > On Mon, Mar 31, 2014 at 1:45 PM, Gerhard Sittig <gsi@denx.de> wrote: > > Isn't there the ONFI SETFEATURE request? And don't you use this > > very request to disable and enable chip internal ECC support, to > > get the raw (uncorrected) bits after bitflips were detected? So > > I understand that there is support to enable this mode at will, > > and we already have (or will have) code to do so. > > > > If we consider disabling on-die-ECC support when we find it > > enabled and know it's not what the user wants to run, then the > > next logical step might be to support enabling this feature if > > the user wants it and the chip doesn't have it enabled at this > > point in time. > > > > So I guess the buffers should get allocated as soon as the chip > > supports on-die-ECC. (While the allocation should only get > > introduced in the phase where the raw-read and bitflip count gets > > added.) > > The ECC-mode of the driver never changes. > Sure, what you describe could be implemented as an additional feature > in the future (I don't have any plans to do so since it makes little sense to > me, as no other ECC-mode uses the layout of the on-die-controller). I don't think we should expect changes at runtime. I'd go with your assessment and implementation (although I don't really like 2 extra buffers...). Brian
On Tue, Apr 01, 2014 at 05:29:54AM +0000, Pekon Gupta wrote: > >From: David Mosberger [mailto:davidm@egauge.net] > >The ECC-mode of the driver never changes. > >Sure, what you describe could be implemented as an additional feature > >in the future (I don't have any plans to do so since it makes little sense to > >me, as no other ECC-mode uses the layout of the on-die-controller). > > > >To be honest, I'm a bit confused: on the one hand, you're asking me to > >simplify the patches into smaller pieces, on the other you seem to ask > >for new features. > > > I too would like to see ECC-mode selection based on user's choice. I think what Gerhard was asking and what Pekon is asking for are different. I don't think Gerhard's suggestion is realistic (that users would change ECC modes mid-use). But Pekon's following concern (which I share) sounds more reasonable, I think, for init-time configuration. > So there should be mechanism to enable/disable this feature based on > DT or platform-data. And onfi_set_feature() is the correct thing to use. > This would make your patches more simple, without disturbing the generic > frame-work. If you patch causes any regression, or abruptly changes > behavior of any working driver, then it's less likely to get accepted. I actually don't think his patches would abruptly change any working driver, since it only utilizes on-die ECC if the chip already had it enabled (and existing drivers would thus not function properly). But I don't think it's the best design, since it assumes that the flash's initial state is the expected mode of operation. > So, instead of re-sending patches it's important to discuss the changes. Yes, please. Brian
On Mon, 2014-03-31 at 14:58 -0600, David Mosberger wrote: > > Gerhard, > > On Mon, Mar 31, 2014 at 1:45 PM, Gerhard Sittig <gsi@denx.de> wrote: > > > Isn't there the ONFI SETFEATURE request? And don't you use this > > very request to disable and enable chip internal ECC support, to > > get the raw (uncorrected) bits after bitflips were detected? So > > I understand that there is support to enable this mode at will, > > and we already have (or will have) code to do so. > > > > If we consider disabling on-die-ECC support when we find it > > enabled and know it's not what the user wants to run, then the > > next logical step might be to support enabling this feature if > > the user wants it and the chip doesn't have it enabled at this > > point in time. > > > > So I guess the buffers should get allocated as soon as the chip > > supports on-die-ECC. (While the allocation should only get > > introduced in the phase where the raw-read and bitflip count gets > > added.) > > The ECC-mode of the driver never changes. > Sure, what you describe could be implemented as an additional feature > in the future (I don't have any plans to do so since it makes little sense to > me, as no other ECC-mode uses the layout of the on-die-controller). I'm sorry, it appears that I was unspecific or sloppy there. The situation is that there are several phases in the setup: There is the identification of the chip and its capabilities, an optional adjustment of pre-set values according to the information that was gathered, and then the "final" setup after everything was evaluated. This (in my mind) maps to the sequence of nand_scan_ident(), nand_scan_tail(), and what might glue them together. So yes, after the ECC scheme was determined, it won't change for the remainder of the driver's runtime. Yet there is the code in your submission that a pre-set software ECC would "turn into" on-die-ECC if the chip was found in a specific state. There has been discussion that we might not blindly follow the chip's current state, but involve a user's request and potentially adjust what was found if it's different. So please drop my thought of chaning ECC mode at runtime, after the initial determination of which mode to use. This really does not apply. The only change that might occur would be to enable/disable the chip's feature if it differs from the user's request, and this would happen before resource allocation in nand_scan_tail(). At the very point of resource allocation, the situation is stable. Having determined the use of on-die-ECC here at this point in time has involved all of the "supported", "enabled", and "wanted" conditions. I'm sorry for the confusion that I may have caused. > To be honest, I'm a bit confused: on the one hand, you're asking me to > simplify the patches into smaller pieces, on the other you seem to ask > for new features. This might look like a conflict only at first glance. But I feel that you can construct larger things with higher confidence if you can inspect and verify individual pieces which you then snug together. If you know that the foundation is solid, you can put more stuff on it with less worry. Nothing in the kernel started out at the complexity that's present now. So no, splitting a large patch into a series does not conflict with growing more features within the resulting series. virtually yours Gerhard Sittig
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 5826da3..1f4a9d8 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3049,16 +3049,29 @@ static int nand_setup_read_retry_micron(struct mtd_info *mtd, int retry_mode) /* * Configure chip properties from Micron vendor-specific ONFI table */ -static void nand_onfi_detect_micron(struct nand_chip *chip, - struct nand_onfi_params *p) +static void nand_onfi_detect_micron(struct mtd_info *mtd, + struct nand_chip *chip, struct nand_onfi_params *p) { struct nand_onfi_vendor_micron *micron = (void *)p->vendor; + u8 features[ONFI_SUBFEATURE_PARAM_LEN]; if (le16_to_cpu(p->vendor_revision) < 1) return; chip->read_retries = micron->read_retry_options; chip->setup_read_retry = nand_setup_read_retry_micron; + + if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_OP_MODE, + features) >= 0) { + if (features[0] & ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC) { + /* + * If the chip has on-die ECC enabled, we kind + * of have to do the same... + */ + chip->ecc.mode = NAND_ECC_HW_ON_DIE; + pr_info("Using on-die ECC\n"); + } + } } /* @@ -3160,7 +3173,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, } if (p->jedec_id == NAND_MFR_MICRON) - nand_onfi_detect_micron(chip, p); + nand_onfi_detect_micron(mtd, chip, p); return 1; } @@ -3792,13 +3805,24 @@ int nand_scan_tail(struct mtd_info *mtd) !(chip->bbt_options & NAND_BBT_USE_FLASH)); if (!(chip->options & NAND_OWN_BUFFERS)) { + size_t on_die_bufsz = 0; + + if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) + on_die_bufsz = 2*(mtd->writesize + mtd->oobsize); + nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize - + mtd->oobsize * 3, GFP_KERNEL); + + mtd->oobsize * 3 + on_die_bufsz, GFP_KERNEL); if (!nbuf) return -ENOMEM; nbuf->ecccalc = (uint8_t *)(nbuf + 1); nbuf->ecccode = nbuf->ecccalc + mtd->oobsize; nbuf->databuf = nbuf->ecccode + mtd->oobsize; + if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) { + nbuf->chkbuf = (nbuf->databuf + mtd->writesize + + mtd->oobsize); + nbuf->rawbuf = (nbuf->chkbuf + mtd->writesize + + mtd->oobsize); + } chip->buffers = nbuf; } else { @@ -3956,6 +3980,7 @@ int nand_scan_tail(struct mtd_info *mtd) ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size); break; + case NAND_ECC_HW_ON_DIE: case NAND_ECC_NONE: pr_warn("NAND_ECC_NONE selected by board driver. " "This is not recommended!\n"); @@ -4023,8 +4048,13 @@ int nand_scan_tail(struct mtd_info *mtd) /* Invalidate the pagebuffer reference */ chip->pagebuf = -1; - /* Large page NAND with SOFT_ECC should support subpage reads */ - if ((ecc->mode == NAND_ECC_SOFT) && (chip->page_shift > 9)) + /* + * Large page NAND with SOFT_ECC or on-die ECC should support + * subpage reads. + */ + if (((ecc->mode == NAND_ECC_SOFT) + || (chip->ecc.mode == NAND_ECC_HW_ON_DIE)) + && (chip->page_shift > 9)) chip->options |= NAND_SUBPAGE_READ; /* Fill in remaining MTD driver data */ diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c index b7361ed..c844c84 100644 --- a/drivers/of/of_mtd.c +++ b/drivers/of/of_mtd.c @@ -23,6 +23,7 @@ static const char *nand_ecc_modes[] = { [NAND_ECC_HW_SYNDROME] = "hw_syndrome", [NAND_ECC_HW_OOB_FIRST] = "hw_oob_first", [NAND_ECC_SOFT_BCH] = "soft_bch", + [NAND_ECC_HW_ON_DIE] = "hw_on_die", }; /** diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 450d61e..a1cc980 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -115,6 +115,7 @@ typedef enum { NAND_ECC_HW_SYNDROME, NAND_ECC_HW_OOB_FIRST, NAND_ECC_SOFT_BCH, + NAND_ECC_HW_ON_DIE, } nand_ecc_modes_t; /* @@ -214,6 +215,10 @@ struct nand_chip; /* Vendor-specific feature address (Micron) */ #define ONFI_FEATURE_ADDR_READ_RETRY 0x89 +/* Vendor-specific array operation mode (Micron) */ +#define ONFI_FEATURE_ADDR_OP_MODE 0x90 +#define ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC 0x08 + /* ONFI subfeature parameters length */ #define ONFI_SUBFEATURE_PARAM_LEN 4 @@ -516,6 +521,8 @@ struct nand_buffers { uint8_t *ecccalc; uint8_t *ecccode; uint8_t *databuf; + uint8_t *chkbuf; + uint8_t *rawbuf; }; /**
This patch adds NAND_ECC_HW_ON_DIE and all other changes to generic code. On-die ECC detection now has moved into nand_onfi_detect_micron(). Signed-off-by: David Mosberger <davidm@egauge.net> --- drivers/mtd/nand/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++------ drivers/of/of_mtd.c | 1 + include/linux/mtd/nand.h | 7 +++++++ 3 files changed, 44 insertions(+), 6 deletions(-)