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

[Boris Brezillon]

On Wed, 17 Apr 2019 10:46:57 +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.   
> > > 
> > > 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.