| Message ID | 1554780172-23111-1-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| State | Not Applicable |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: rawnand: Add Macronix NAND read retry and randomizer support | expand |
On Tue, 9 Apr 2019 11:22:52 +0800 Mason Yang <masonccyang@mxic.com.tw> wrote: > Add a driver for Macronix NAND read retry and randomizer. These are 2 orthogonal changes, and should thus bit split in 2 patches. > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > --- > drivers/mtd/nand/raw/nand_macronix.c | 169 +++++++++++++++++++++++++++++++++++ > 1 file changed, 169 insertions(+) > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c > index 47d8cda..a19caa4 100644 > --- a/drivers/mtd/nand/raw/nand_macronix.c > +++ b/drivers/mtd/nand/raw/nand_macronix.c > @@ -17,6 +17,174 @@ > > #include "internals.h" > > +#define MACRONIX_READ_RETRY_BIT BIT(0) > +#define MACRONIX_RANDOMIZER_BIT BIT(1) > +#define MACRONIX_READ_RETRY_MODE 5 > + > +#define ONFI_FEATURE_ADDR_MXIC_RANDOMIZER 0xB0 > + > +struct nand_onfi_vendor_macronix { > + u8 reserved[1]; > + u8 reliability_func; > +} __packed; > + > +struct nand_chip *mxic_sysfs; > + > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) > +{ > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; > + int ret; > + > + if (mode > MACRONIX_READ_RETRY_MODE) > + mode = MACRONIX_READ_RETRY_MODE; > + > + feature[0] = mode; > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature); > + if (ret) > + pr_err("failed to enter read retry moded:%d\n", mode); > + > + if (mode == 0) > + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, > + feature); > + if (ret) > + pr_err("failed to exits read retry moded:%d\n", mode); > + > + return ret; > +} > + > +static ssize_t mxic_nand_rand_type_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct nand_chip *chip = mxic_sysfs; > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; > + int ret; > + > + nand_select_target(chip, 0); > + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + feature); > + nand_deselect_target(chip); > + if (ret) > + pr_err("failed to check mxic nand device randomizer\n"); > + > + return sprintf(buf, "MXIC NAND device randomizer %s(0x%x)\n", > + feature[0] & MACRONIX_RANDOMIZER_BIT ? > + "enable" : "disable", feature[0]); > +} > + > +static ssize_t mxic_nand_rand_type_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct nand_chip *chip = mxic_sysfs; > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; > + unsigned int rand_layout; > + int ret; > + > + nand_select_target(chip, 0); > + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + feature); > + nand_deselect_target(chip); > + > + if (feature[0]) { > + pr_err("Randomizer is enabled 0x%x\n", feature[0]); > + goto err_out; > + } > + > + ret = kstrtouint(buf, 0, &rand_layout); > + if (ret) > + goto err_out; > + > + if (rand_layout > 7) { > + pr_err("Error parameter value:0x%x\n", rand_layout); > + goto err_out; > + } > + > + feature[0] = rand_layout & 0x07; > + nand_select_target(chip, 0); > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + feature); > + nand_deselect_target(chip); > + if (ret) { > + pr_err("device randomizer set feature failed\n"); > + goto err_out; > + } > + > + feature[0] = 0x0; > + nand_select_target(chip, 0); > + ret = nand_prog_page_op(chip, 0, 0, feature, 1); > + nand_deselect_target(chip); > + if (ret) { > + pr_err("Prog device randomizer failed\n"); > + goto err_out; > + } > + > + feature[0] = 0x0; > + nand_select_target(chip, 0); > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + feature); > + nand_deselect_target(chip); > + if (ret) > + pr_err("failed to exits prog device randomizer\n"); > + > +err_out: > + return count; > +} > + > +static const struct kobj_attribute sysfs_mxic_nand = > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > + mxic_nand_rand_type_show, > + mxic_nand_rand_type_store); No, we don't want to expose that through a sysfs file, especially since changing the randomizer config means making the NAND unreadable for those that have used it before the change. BTW, why would we want to disable the randomizer? If it's here I guess it's needed. The only use case I have in mind is when the controller also has a randomizer and you want to use this one instead of the on-die one. > + > +static void macronix_nand_onfi_init(struct nand_chip *chip) > +{ > + struct nand_parameters *p = &chip->parameters; > + struct kobject *kobj; > + int ret; > + > + mxic_sysfs = chip; > + if (p->onfi) { > + struct nand_onfi_vendor_macronix *mxic = > + (void *)p->onfi->vendor; > + > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; > + chip->setup_read_retry = > + macronix_nand_setup_read_retry; > + if (p->supports_set_get_features) { > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->set_feature_list); > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->get_feature_list); > + } > + } > + > + if (mxic->reliability_func & MACRONIX_RANDOMIZER_BIT) { > + if (p->supports_set_get_features) { > + set_bit(ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + p->set_feature_list); > + set_bit(ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + p->get_feature_list); > + /* > + * create syfs-fs for MXIC NAND device > + * randomizer status check & enable > + * operations. > + */ > + kobj = kobject_create_and_add("mxic_rand_nand", > + NULL); > + if (!kobj) > + return; > + > + ret = sysfs_create_file(kobj, > + &sysfs_mxic_nand.attr); > + if (ret) { > + pr_err("Err: mxic_rand_nand sysfs"); > + kobject_put(kobj); > + } > + } > + } > + } > +} > + > /* > * Macronix AC series does not support using SET/GET_FEATURES to change > * the timings unlike what is declared in the parameter page. Unflag > @@ -65,6 +233,7 @@ static int macronix_nand_init(struct nand_chip *chip) > chip->bbt_options |= NAND_BBT_SCAN2NDPAGE; > > macronix_nand_fix_broken_get_timings(chip); > + macronix_nand_onfi_init(chip); > > return 0; > }
On Tue, 9 Apr 2019 17:35:39 +0800 masonccyang@mxic.com.tw wrote: > > > + > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > + mxic_nand_rand_type_show, > > > + mxic_nand_rand_type_store); > > > > No, we don't want to expose that through a sysfs file, especially since > > changing the randomizer config means making the NAND unreadable for > > those that have used it before the change. > > > > Our on-die randomizer is still readable from user after the function > is enabled. You mean the memory is still readable no matter the randomizer state. Not sure how that's possible, but okay. > This randomizer is just like a internal memory cell > reliability enhanced. Why don't you enable it by default then? > It could be enable at any time with OTP bit function and that's why > we patch it by sys-fs. Sorry, but that's not a good reason to expose that through sysfs.
Hi Mason, masonccyang@mxic.com.tw wrote on Wed, 10 Apr 2019 09:14:14 +0800: > Hi Boris, > > > > > Subject > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and randomizer > support > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > masonccyang@mxic.com.tw wrote: > > > > > > > + > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > + mxic_nand_rand_type_show, > > > > > + mxic_nand_rand_type_store); > > > > > > > > No, we don't want to expose that through a sysfs file, especially > since > > > > changing the randomizer config means making the NAND unreadable for > > > > those that have used it before the change. > > > > > > > > > > Our on-die randomizer is still readable from user after the function > > > is enabled. > > > > You mean the memory is still readable no matter the randomizer state. > > Not sure how that's possible, but okay. So if you write non-randomized data to the NAND chip, then enable the randomizer en read back the data, all will be ok? And if randomized data is written to the NAND chip and we disable the randomizer, then the data will also be correct? > > > > > This randomizer is just like a internal memory cell > > > reliability enhanced. > > > > Why don't you enable it by default then? > > The penalty of randomizer is read/write performance down. > i.e,. tPROG 300 us to 340 us (randomizer enable) > therefore, disable it by default. Is this info somewhere in the ONFI param page? I suppose once randomization is enabled we should also tweak the timings and verify that the controller supports it. > > > It could be enable at any time with OTP bit function and that's why > > > we patch it by sys-fs. > > > > Sorry, but that's not a good reason to expose that through sysfs. > > Any good way to expose randomizer function for user ? > > thanks & best regards, > Mason > Thanks, Miquèl
On Wed, 10 Apr 2019 09:14:14 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > Subject > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and randomizer > support > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > masonccyang@mxic.com.tw wrote: > > > > > > > + > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > + mxic_nand_rand_type_show, > > > > > + mxic_nand_rand_type_store); > > > > > > > > No, we don't want to expose that through a sysfs file, especially > since > > > > changing the randomizer config means making the NAND unreadable for > > > > those that have used it before the change. > > > > > > > > > > Our on-die randomizer is still readable from user after the function > > > is enabled. > > > > You mean the memory is still readable no matter the randomizer state. > > Not sure how that's possible, but okay. > > > > > This randomizer is just like a internal memory cell > > > reliability enhanced. > > > > Why don't you enable it by default then? > > The penalty of randomizer is read/write performance down. > i.e,. tPROG 300 us to 340 us (randomizer enable) > therefore, disable it by default. I'm a bit puzzled. On the NAND I've seen that required data randomization it's not something you'd want to disable as this implied poor data retention. What's the use case here? Are we talking about SLC or MLC NANDs? Should we enable this feature once we start seeing that the NAND starts being less reliable (basically when read-retry happens more often)? I really think this is something you should decide kernel side, because users have no clue when it's appropriate to switch this feature on/off. > > > > > > It could be enable at any time with OTP bit function and that's why > > > we patch it by sys-fs. > > > > Sorry, but that's not a good reason to expose that through sysfs. > > Any good way to expose randomizer function for user ? Don't expose it :P.
Hi masonccyang@mxic.com.tw, masonccyang@mxic.com.tw wrote on Wed, 10 Apr 2019 16:40:25 +0800: > Hi Miquel, > > > > > > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and > randomizer > > > support > > > > > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > > > masonccyang@mxic.com.tw wrote: > > > > > > > > > > > + > > > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > > > + mxic_nand_rand_type_show, > > > > > > > + mxic_nand_rand_type_store); > > > > > > > > > > > > No, we don't want to expose that through a sysfs file, > especially > > > since > > > > > > changing the randomizer config means making the NAND unreadable > for > > > > > > those that have used it before the change. > > > > > > > > > > > > > > > > Our on-die randomizer is still readable from user after the > function > > > > > is enabled. > > > > > > > > You mean the memory is still readable no matter the randomizer > state. > > > > Not sure how that's possible, but okay. > > > > So if you write non-randomized data to the NAND chip, then enable the > > randomizer en read back the data, all will be ok? s/en/and/ > > yes ! I don't understand how this is possible. Have you tried it yourself? Can you explain how this is supposed to work? > > > > > And if randomized data is written to the NAND chip and we disable the > > randomizer, then the data will also be correct? > > > > Enable randomizer is a OTP(One-Time-Program) bit and can't be erased > again! > That means never disable it once it has been enabled. > > > > > > > > > > This randomizer is just like a internal memory cell > > > > > reliability enhanced. > > > > > > > > Why don't you enable it by default then? > > > > > > The penalty of randomizer is read/write performance down. > > > i.e,. tPROG 300 us to 340 us (randomizer enable) > > > therefore, disable it by default. > > > > Is this info somewhere in the ONFI param page? I suppose once > > randomization is enabled we should also tweak the timings and verify > > that the controller supports it. > > yes, it is in ONFI param page@Byte# 167 of bit 1 (Vendor Blocks > starting@Byte# 164). > > +#define ONFI_FEATURE_ADDR_MXIC_RANDOMIZER 0xB0 > + > +struct nand_onfi_vendor_macronix { > + u8 reserved[1]; > + u8 reliability_func; > +} __packed; > > > in addition, enable randomizer has no any impact on timing of tRC, tRP, > tWC and so on. > host driver just need to check the status pin of RY/#BY as like standard > read/write operation flow. > > thanks & best regards, > Mason > Thanks, Miquèl
On Thu, 11 Apr 2019 12:23:11 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > > Subject > > > > > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and > randomizer > > > support > > > > > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > > > masonccyang@mxic.com.tw wrote: > > > > > > > > > > > + > > > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > > > + mxic_nand_rand_type_show, > > > > > > > + mxic_nand_rand_type_store); > > > > > > > > > > > > No, we don't want to expose that through a sysfs file, > especially > > > since > > > > > > changing the randomizer config means making the NAND unreadable > for > > > > > > those that have used it before the change. > > > > > > > > > > > > > > > > Our on-die randomizer is still readable from user after the > function > > > > > is enabled. > > > > > > > > You mean the memory is still readable no matter the randomizer > state. > > > > Not sure how that's possible, but okay. > > > > > > > > > This randomizer is just like a internal memory cell > > > > > reliability enhanced. > > > > > > > > Why don't you enable it by default then? > > > > > > The penalty of randomizer is read/write performance down. > > > i.e,. tPROG 300 us to 340 us (randomizer enable) > > > therefore, disable it by default. > > > > I'm a bit puzzled. On the NAND I've seen that required data > > randomization it's not something you'd want to disable as this implied > > poor data retention. What's the use case here? Are we talking about SLC > > or MLC NANDs? Should we enable this feature once we start seeing that > > the NAND starts being less reliable (basically when read-retry happens > > more often)? I really think this is something you should decide kernel > > side, because users have no clue when it's appropriate to switch this > > feature on/off. > > > > It's SLC NAND and seems to has nothing to do with read-retry happens. > later, I will get more information for your concerns. Well, this feature is optional, and can be enabled to improve reliability. Sounds like a good reason to enable it when your NAND device starts showing reliability issues, and the number of read_retry attempts reflects the wear level pretty well. Alternatively, you could use the number of bitflips, but, in any case, don't expect the user to take this decision, because almost nobody knows what the randomizer is needed for. > > > > > > > > > > > > > It could be enable at any time with OTP bit function and that's > why > > > > > we patch it by sys-fs. > > > > > > > > Sorry, but that's not a good reason to expose that through sysfs. > > > > > > Any good way to expose randomizer function for user ? > > > > Don't expose it :P. > > oh, okay, I will remove sys-fs randomizer. > > Is it OK to keep set/get features for randomizer ? I don't think it's a good idea to have dead code, so no. But I'm pretty sure we'll find a way to use/expose this feature. > that is : > > +static void macronix_nand_onfi_init(struct nand_chip *chip) > +{ > + struct nand_parameters *p = &chip->parameters; > + struct kobject *kobj; > + int ret; > + > + if (p->onfi) { > + struct nand_onfi_vendor_macronix *mxic = > + (void > *)p->onfi->vendor; > + > + if (mxic->reliability_func & > MACRONIX_READ_RETRY_BIT) { > + chip->read_retries = > MACRONIX_READ_RETRY_MODE + 1; > + chip->setup_read_retry = > + macronix_nand_setup_read_retry; > + if > (p->supports_set_get_features) { > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->set_feature_list); > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->get_feature_list); > + } > + } > + > + if (mxic->reliability_func & > MACRONIX_RANDOMIZER_BIT) { > + if > (p->supports_set_get_features) { > + set_bit(ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + p->set_feature_list); > + set_bit(ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, > + p->get_feature_list); > + } > + } > + } > +} > + > > thanks & best regards, > Mason > > > > CONFIDENTIALITY NOTE: > > This e-mail and any attachments may contain confidential information > and/or personal data, which is protected by applicable laws. Please be > reminded that duplication, disclosure, distribution, or use of this e-mail > (and/or its attachments) or any part thereof is prohibited. If you receive > this e-mail in error, please notify us immediately and delete this mail as > well as its attachment(s) from your system. In addition, please be > informed that collection, processing, and/or use of personal data is > prohibited unless expressly permitted by personal data protection laws. > Thank you for your attention and cooperation. > > Macronix International Co., Ltd. > > ===================================================================== > > > > ============================================================================ > > CONFIDENTIALITY NOTE: > > This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. > > Macronix International Co., Ltd. > > =====================================================================
On Thu, 11 Apr 2019 17:24:09 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > > > > Subject > > > > > > > > > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and > > > randomizer > > > > > support > > > > > > > > > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > > > > > masonccyang@mxic.com.tw wrote: > > > > > > > > > > > > > > > + > > > > > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > > > > > + mxic_nand_rand_type_show, > > > > > > > > > + mxic_nand_rand_type_store); > > > > > > > > > > > > > > > > No, we don't want to expose that through a sysfs file, > > > especially > > > > > since > > > > > > > > changing the randomizer config means making the NAND > unreadable > > > for > > > > > > > > those that have used it before the change. > > > > > > > > > > > > > > > > > > > > > > Our on-die randomizer is still readable from user after the > > > function > > > > > > > is enabled. > > > > > > > > > > > > You mean the memory is still readable no matter the randomizer > > > state. > > > > > > Not sure how that's possible, but okay. > > > > > > > > > > > > > This randomizer is just like a internal memory cell > > > > > > > reliability enhanced. > > > > > > > > > > > > Why don't you enable it by default then? > > > > > > > > > > The penalty of randomizer is read/write performance down. > > > > > i.e,. tPROG 300 us to 340 us (randomizer enable) > > > > > therefore, disable it by default. > > > > > > > > I'm a bit puzzled. On the NAND I've seen that required data > > > > randomization it's not something you'd want to disable as this > implied > > > > poor data retention. What's the use case here? Are we talking about > SLC > > > > or MLC NANDs? Should we enable this feature once we start seeing > that > > > > the NAND starts being less reliable (basically when read-retry > happens > > > > more often)? I really think this is something you should decide > kernel > > > > side, because users have no clue when it's appropriate to switch > this > > > > feature on/off. > > > > > > > > > > It's SLC NAND and seems to has nothing to do with read-retry happens. > > > later, I will get more information for your concerns. > > > > Well, this feature is optional, and can be enabled to improve > > reliability. Sounds like a good reason to enable it when your NAND > > device starts showing reliability issues, and the number of read_retry > > attempts reflects the wear level pretty well. Alternatively, you could > > use the number of bitflips, but, in any case, don't expect the user to > > take this decision, because almost nobody knows what the randomizer > > is needed for. > > > > > > > > > > > > > > > > > > > > > > > It could be enable at any time with OTP bit function and > that's > > > why > > > > > > > we patch it by sys-fs. > > > > > > > > > > > > Sorry, but that's not a good reason to expose that through > sysfs. > > > > > > > > > > Any good way to expose randomizer function for user ? > > > > > > > > Don't expose it :P. > > > > > > oh, okay, I will remove sys-fs randomizer. > > > > > > Is it OK to keep set/get features for randomizer ? > > > > I don't think it's a good idea to have dead code, so no. But I'm pretty > > sure we'll find a way to use/expose this feature. > > okay, great! > Looking forward to hearing this feature use/expose. But for that to happen we are waiting for inputs about when this is supposed to be used...
Hi Mason, masonccyang@mxic.com.tw wrote on Wed, 17 Apr 2019 10:46:57 +0800: > Hi Boris, > > > > > > > > > > Subject > > > > > > > > > > > > > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and > > > > > randomizer > > > > > > > support > > > > > > > > > > > > > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > > > > > > > masonccyang@mxic.com.tw wrote: > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > > > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > > > > > > > + mxic_nand_rand_type_show, > > > > > > > > > > > + mxic_nand_rand_type_store); > > > > > > > > > > > > > > > > > > > > No, we don't want to expose that through a sysfs file, > > > > > especially > > > > > > > since > > > > > > > > > > changing the randomizer config means making the NAND > > > unreadable > > > > > for > > > > > > > > > > those that have used it before the change. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Our on-die randomizer is still readable from user after > the > > > > > function > > > > > > > > > is enabled. > > > > > > > > > > > > > > > > You mean the memory is still readable no matter the > randomizer > > > > > state. > > > > > > > > Not sure how that's possible, but okay. > > > > > > > > > > > > > > > > > This randomizer is just like a internal memory cell > > > > > > > > > reliability enhanced. > > > > > > > > > > > > > > > > Why don't you enable it by default then? > > > > > > > > > > > > > > The penalty of randomizer is read/write performance down. > > > > > > > i.e,. tPROG 300 us to 340 us (randomizer enable) > > > > > > > therefore, disable it by default. > > > > > > > > > > > > I'm a bit puzzled. On the NAND I've seen that required data > > > > > > randomization it's not something you'd want to disable as this > > > implied > > > > > > poor data retention. What's the use case here? Are we talking > about > > > SLC > > > > > > or MLC NANDs? Should we enable this feature once we start seeing > > > > that > > > > > > the NAND starts being less reliable (basically when read-retry > > > happens > > > > > > more often)? I really think this is something you should decide > > > > kernel > > > > > > side, because users have no clue when it's appropriate to switch > > > > this > > > > > > feature on/off. > > > > > > > > > > > > > > > > It's SLC NAND and seems to has nothing to do with read-retry > happens. > > > > > later, I will get more information for your concerns. > > > > > > > > Well, this feature is optional, and can be enabled to improve > > > > reliability. Sounds like a good reason to enable it when your NAND > > > > device starts showing reliability issues, and the number of > read_retry > > > > attempts reflects the wear level pretty well. Alternatively, you > could > > > > use the number of bitflips, but, in any case, don't expect the user > to > > > > take this decision, because almost nobody knows what the randomizer > > > > is needed for. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It could be enable at any time with OTP bit function and > > > that's > > > > > why > > > > > > > > > we patch it by sys-fs. > > > > > > > > > > > > > > > > Sorry, but that's not a good reason to expose that through > > > sysfs. > > > > > > > > > > > > > > Any good way to expose randomizer function for user ? > > > > > > > > > > > > Don't expose it :P. > > > > > > > > > > oh, okay, I will remove sys-fs randomizer. > > > > > > > > > > Is it OK to keep set/get features for randomizer ? > > > > > > > > I don't think it's a good idea to have dead code, so no. But I'm > pretty > > > > sure we'll find a way to use/expose this feature. > > > > > > okay, great! > > > Looking forward to hearing this feature use/expose. > > > > But for that to happen we are waiting for inputs about when this is > > supposed to be used... > > > The main reason to disable Randomizer in default is > NOP = 4 (default) change to NOP = 1 (Randomizer enable), > NOP: number of partial program cycles in same page I am not sure to understand, is this related to what we call 'subpages'? > > Some OS file systems(or FTL) much concern NOP = 4 and > any better way than sys-fs to enable it? sysfs entry => user action The user has absolutely no way to know when it is relevant to enable the randomizer. The kernel must be in charge of it. So the question is: when is it relevant to enable the randomizer? What criteria? What threshold? Thanks, Miquèl
On Wed, 17 Apr 2019 10:46:57 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > > > > > > Subject > > > > > > > > > > > > > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and > > > > > randomizer > > > > > > > support > > > > > > > > > > > > > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > > > > > > > masonccyang@mxic.com.tw wrote: > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > +static const struct kobj_attribute sysfs_mxic_nand = > > > > > > > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > > > > > > > + mxic_nand_rand_type_show, > > > > > > > > > > > + mxic_nand_rand_type_store); > > > > > > > > > > > > > > > > > > > > No, we don't want to expose that through a sysfs file, > > > > > especially > > > > > > > since > > > > > > > > > > changing the randomizer config means making the NAND > > > unreadable > > > > > for > > > > > > > > > > those that have used it before the change. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Our on-die randomizer is still readable from user after > the > > > > > function > > > > > > > > > is enabled. > > > > > > > > > > > > > > > > You mean the memory is still readable no matter the > randomizer > > > > > state. > > > > > > > > Not sure how that's possible, but okay. > > > > > > > > > > > > > > > > > This randomizer is just like a internal memory cell > > > > > > > > > reliability enhanced. > > > > > > > > > > > > > > > > Why don't you enable it by default then? > > > > > > > > > > > > > > The penalty of randomizer is read/write performance down. > > > > > > > i.e,. tPROG 300 us to 340 us (randomizer enable) > > > > > > > therefore, disable it by default. > > > > > > > > > > > > I'm a bit puzzled. On the NAND I've seen that required data > > > > > > randomization it's not something you'd want to disable as this > > > implied > > > > > > poor data retention. What's the use case here? Are we talking > about > > > SLC > > > > > > or MLC NANDs? Should we enable this feature once we start seeing > > > > that > > > > > > the NAND starts being less reliable (basically when read-retry > > > happens > > > > > > more often)? I really think this is something you should decide > > > > kernel > > > > > > side, because users have no clue when it's appropriate to switch > > > > this > > > > > > feature on/off. > > > > > > > > > > > > > > > > It's SLC NAND and seems to has nothing to do with read-retry > happens. > > > > > later, I will get more information for your concerns. > > > > > > > > Well, this feature is optional, and can be enabled to improve > > > > reliability. Sounds like a good reason to enable it when your NAND > > > > device starts showing reliability issues, and the number of > read_retry > > > > attempts reflects the wear level pretty well. Alternatively, you > could > > > > use the number of bitflips, but, in any case, don't expect the user > to > > > > take this decision, because almost nobody knows what the randomizer > > > > is needed for. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It could be enable at any time with OTP bit function and > > > that's > > > > > why > > > > > > > > > we patch it by sys-fs. > > > > > > > > > > > > > > > > Sorry, but that's not a good reason to expose that through > > > sysfs. > > > > > > > > > > > > > > Any good way to expose randomizer function for user ? > > > > > > > > > > > > Don't expose it :P. > > > > > > > > > > oh, okay, I will remove sys-fs randomizer. > > > > > > > > > > Is it OK to keep set/get features for randomizer ? > > > > > > > > I don't think it's a good idea to have dead code, so no. But I'm > pretty > > > > sure we'll find a way to use/expose this feature. > > > > > > okay, great! > > > Looking forward to hearing this feature use/expose. > > > > But for that to happen we are waiting for inputs about when this is > > supposed to be used... > > > The main reason to disable Randomizer in default is > NOP = 4 (default) change to NOP = 1 (Randomizer enable), > NOP: number of partial program cycles in same page > > Some OS file systems(or FTL) much concern NOP = 4 and > any better way than sys-fs to enable it? Well, that's more a constraint (no sub-page program when the randomizer is enabled) than something that would help decide whether it's relevant to enable the randomizer or not. Oh, and that's actually a proof that changing this property after the flash has been programmed is not possible (UBI will complain if subpage size changes). BTW, you should check this prop to decide if subpage writes are supported... > > of course, the other penalty of randomizer is > read/write performance down. > i.e,. tPROG 300 us to 340 us (randomizer enable) Again, that's a constraint.
Hi Mason, Boris, masonccyang@mxic.com.tw wrote on Thu, 18 Apr 2019 11:30:05 +0800: > Hi Miquel, > > > > > > > > > > > > > > > > > > > > > > Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry > and > > > > > > > randomizer > > > > > > > > > support > > > > > > > > > > > > > > > > > > > > On Tue, 9 Apr 2019 17:35:39 +0800 > > > > > > > > > > masonccyang@mxic.com.tw wrote: > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > +static const struct kobj_attribute > sysfs_mxic_nand = > > > > > > > > > > > > > + __ATTR(nand_random, S_IRUGO | S_IWUSR, > > > > > > > > > > > > > + mxic_nand_rand_type_show, > > > > > > > > > > > > > + mxic_nand_rand_type_store); > > > > > > > > > > > > > > > > > > > > > > > > No, we don't want to expose that through a sysfs > file, > > > > > > > especially > > > > > > > > > since > > > > > > > > > > > > changing the randomizer config means making the NAND > > > > > > unreadable > > > > > > > for > > > > > > > > > > > > those that have used it before the change. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Our on-die randomizer is still readable from user > after > > > the > > > > > > > function > > > > > > > > > > > is enabled. > > > > > > > > > > > > > > > > > > > > You mean the memory is still readable no matter the > > > randomizer > > > > > > > state. > > > > > > > > > > Not sure how that's possible, but okay. > > > > > > > > > > > > > > > > > > > > > This randomizer is just like a internal memory cell > > > > > > > > > > > reliability enhanced. > > > > > > > > > > > > > > > > > > > > Why don't you enable it by default then? > > > > > > > > > > > > > > > > > > The penalty of randomizer is read/write performance down. > > > > > > > > > i.e,. tPROG 300 us to 340 us (randomizer enable) > > > > > > > > > therefore, disable it by default. > > > > > > > > > > > > > > > > I'm a bit puzzled. On the NAND I've seen that required data > > > > > > > > randomization it's not something you'd want to disable as > this > > > > > implied > > > > > > > > poor data retention. What's the use case here? Are we > talking > > > about > > > > > SLC > > > > > > > > or MLC NANDs? Should we enable this feature once we > > start seeing > > > > > > > > that > > > > > > > > the NAND starts being less reliable (basically when > read-retry > > > > > happens > > > > > > > > more often)? I really think this is something you > shoulddecide > > > > > > > > kernel > > > > > > > > side, because users have no clue when it's appropriate > > to switch > > > > > > > > this > > > > > > > > feature on/off. > > > > > > > > > > > > > > > > > > > > > > It's SLC NAND and seems to has nothing to do with read-retry > > > happens. > > > > > > > later, I will get more information for your concerns. > > > > > > > > > > > > Well, this feature is optional, and can be enabled to improve > > > > > > reliability. Sounds like a good reason to enable it when your > NAND > > > > > > device starts showing reliability issues, and the number of > > > read_retry > > > > > > attempts reflects the wear level pretty well. Alternatively, you > > > > could > > > > > > use the number of bitflips, but, in any case, don't expect the > user > > > to > > > > > > take this decision, because almost nobody knows what the > randomizer > > > > > > is needed for. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It could be enable at any time with OTP bit function > and > > > > > that's > > > > > > > why > > > > > > > > > > > we patch it by sys-fs. > > > > > > > > > > > > > > > > > > > > Sorry, but that's not a good reason to expose that > through > > > > > sysfs. > > > > > > > > > > > > > > > > > > Any good way to expose randomizer function for user ? > > > > > > > > > > > > > > > > Don't expose it :P. > > > > > > > > > > > > > > oh, okay, I will remove sys-fs randomizer. > > > > > > > > > > > > > > Is it OK to keep set/get features for randomizer ? > > > > > > > > > > > > I don't think it's a good idea to have dead code, so no. But I'm > > > > pretty > > > > > > sure we'll find a way to use/expose this feature. > > > > > > > > > > okay, great! > > > > > Looking forward to hearing this feature use/expose. > > > > > > > > But for that to happen we are waiting for inputs about when this is > > > > supposed to be used... > > > > > > > > > The main reason to disable Randomizer in default is > > > NOP = 4 (default) change to NOP = 1 (Randomizer enable), > > > NOP: number of partial program cycles in same page > > > > I am not sure to understand, is this related to what we call 'subpages'? > > > yes, > > > > > > > Some OS file systems(or FTL) much concern NOP = 4 and > > > any better way than sys-fs to enable it? > > > > sysfs entry => user action > > The user has absolutely no way to know when it is relevant to enable > > the randomizer. The kernel must be in charge of it. So the question is: > > when is it relevant to enable the randomizer? What criteria? What > > threshold? > > > > Randomizer is according to users' demand that at least two different use > cases. > 1. a need for an operation mode/use case to take advantage of NOP of 4 > without turning on randomizer > 2. another use case for high data integrity by enabling randomizer and > sacrificing NOP > > If user application don't need subpage program (NOP = 1 is ok), > they could enable Randomizer from kernel driver > (i.e., chip->options |= NAND_NO_SUBPAGE_WRITE; & set feature to enable > randomizer) > or user space(i.e., sys-fs.). > > Therefore, default to disbale randomizer(for NOP=4). What about a DT property in the NAND chip node that would be checked in Macronix driver? Or maybe a defconfig entry? This cannot be changed at runtime. Thanks, Miquèl
diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c index 47d8cda..a19caa4 100644 --- a/drivers/mtd/nand/raw/nand_macronix.c +++ b/drivers/mtd/nand/raw/nand_macronix.c @@ -17,6 +17,174 @@ #include "internals.h" +#define MACRONIX_READ_RETRY_BIT BIT(0) +#define MACRONIX_RANDOMIZER_BIT BIT(1) +#define MACRONIX_READ_RETRY_MODE 5 + +#define ONFI_FEATURE_ADDR_MXIC_RANDOMIZER 0xB0 + +struct nand_onfi_vendor_macronix { + u8 reserved[1]; + u8 reliability_func; +} __packed; + +struct nand_chip *mxic_sysfs; + +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) +{ + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; + int ret; + + if (mode > MACRONIX_READ_RETRY_MODE) + mode = MACRONIX_READ_RETRY_MODE; + + feature[0] = mode; + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature); + if (ret) + pr_err("failed to enter read retry moded:%d\n", mode); + + if (mode == 0) + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, + feature); + if (ret) + pr_err("failed to exits read retry moded:%d\n", mode); + + return ret; +} + +static ssize_t mxic_nand_rand_type_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct nand_chip *chip = mxic_sysfs; + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; + int ret; + + nand_select_target(chip, 0); + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, + feature); + nand_deselect_target(chip); + if (ret) + pr_err("failed to check mxic nand device randomizer\n"); + + return sprintf(buf, "MXIC NAND device randomizer %s(0x%x)\n", + feature[0] & MACRONIX_RANDOMIZER_BIT ? + "enable" : "disable", feature[0]); +} + +static ssize_t mxic_nand_rand_type_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct nand_chip *chip = mxic_sysfs; + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; + unsigned int rand_layout; + int ret; + + nand_select_target(chip, 0); + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, + feature); + nand_deselect_target(chip); + + if (feature[0]) { + pr_err("Randomizer is enabled 0x%x\n", feature[0]); + goto err_out; + } + + ret = kstrtouint(buf, 0, &rand_layout); + if (ret) + goto err_out; + + if (rand_layout > 7) { + pr_err("Error parameter value:0x%x\n", rand_layout); + goto err_out; + } + + feature[0] = rand_layout & 0x07; + nand_select_target(chip, 0); + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, + feature); + nand_deselect_target(chip); + if (ret) { + pr_err("device randomizer set feature failed\n"); + goto err_out; + } + + feature[0] = 0x0; + nand_select_target(chip, 0); + ret = nand_prog_page_op(chip, 0, 0, feature, 1); + nand_deselect_target(chip); + if (ret) { + pr_err("Prog device randomizer failed\n"); + goto err_out; + } + + feature[0] = 0x0; + nand_select_target(chip, 0); + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, + feature); + nand_deselect_target(chip); + if (ret) + pr_err("failed to exits prog device randomizer\n"); + +err_out: + return count; +} + +static const struct kobj_attribute sysfs_mxic_nand = + __ATTR(nand_random, S_IRUGO | S_IWUSR, + mxic_nand_rand_type_show, + mxic_nand_rand_type_store); + +static void macronix_nand_onfi_init(struct nand_chip *chip) +{ + struct nand_parameters *p = &chip->parameters; + struct kobject *kobj; + int ret; + + mxic_sysfs = chip; + if (p->onfi) { + struct nand_onfi_vendor_macronix *mxic = + (void *)p->onfi->vendor; + + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; + chip->setup_read_retry = + macronix_nand_setup_read_retry; + if (p->supports_set_get_features) { + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, + p->set_feature_list); + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, + p->get_feature_list); + } + } + + if (mxic->reliability_func & MACRONIX_RANDOMIZER_BIT) { + if (p->supports_set_get_features) { + set_bit(ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, + p->set_feature_list); + set_bit(ONFI_FEATURE_ADDR_MXIC_RANDOMIZER, + p->get_feature_list); + /* + * create syfs-fs for MXIC NAND device + * randomizer status check & enable + * operations. + */ + kobj = kobject_create_and_add("mxic_rand_nand", + NULL); + if (!kobj) + return; + + ret = sysfs_create_file(kobj, + &sysfs_mxic_nand.attr); + if (ret) { + pr_err("Err: mxic_rand_nand sysfs"); + kobject_put(kobj); + } + } + } + } +} + /* * Macronix AC series does not support using SET/GET_FEATURES to change * the timings unlike what is declared in the parameter page. Unflag @@ -65,6 +233,7 @@ static int macronix_nand_init(struct nand_chip *chip) chip->bbt_options |= NAND_BBT_SCAN2NDPAGE; macronix_nand_fix_broken_get_timings(chip); + macronix_nand_onfi_init(chip); return 0; }
Add a driver for Macronix NAND read retry and randomizer. Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> --- drivers/mtd/nand/raw/nand_macronix.c | 169 +++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+)