Re: [PATCH] mtd: rawnand: Add Macronix NAND read retry and randomizer support

[Boris Brezillon]

On Thu, 11 Apr 2019 12:23:11 +0800
masonccyang@xxxxxxxxxxx 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@xxxxxxxxxxx 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.
> 
> =====================================================================