Hi, On Sat, Jun 11, 2016 at 08:54:08AM +0200, Boris Brezillon wrote: > On Fri, 10 Jun 2016 19:17:15 -0700 > Brian Norris <[email protected]> wrote: > > On Mon, Apr 25, 2016 at 12:01:18PM +0200, Boris Brezillon wrote: > > > MLC and TLC NAND devices are using NAND cells exposing more than one bit, > > > but instead of attaching all the bits in a given cell to a single NAND > > > page, each bit is usually attached to a different page. This concept is > > > called 'page pairing', and has significant impacts on the flash storage > > > usage. > > > The main problem showed by these devices is that interrupting a page > > > program operation may not only corrupt the page we are programming > > > but also the page it is paired with, hence the need to expose to MTD > > > users the pairing scheme information. > > > > > > The pairing APIs allows one to query pairing information attached to a > > > given page (here called wunit), or the other way around (the wunit > > > pointed by pairing information). > > > > Why the "write unit" terminology? Is a write unit ever different from a > > page? > > Because there's no concept of pages at the MTD level. The page size is > actually translated into writesize, so I thought keeping the same > wording for pairing scheme would be more appropriate. Not sure other > device types will need this pairing scheme feature though.
Ah, I suppose that makes sense. > > > > > It also provides several helpers to help the conversion between absolute > > > offsets and wunits, and query the number of pairing groups. > > > > > > Signed-off-by: Boris Brezillon <[email protected]> > > > --- > > > drivers/mtd/mtdcore.c | 62 > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > drivers/mtd/mtdpart.c | 1 + > > > include/linux/mtd/mtd.h | 64 > > > +++++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 127 insertions(+) > > > [...] > > > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h > > > index 29a1706..4961092 100644 > > > --- a/include/linux/mtd/mtd.h > > > +++ b/include/linux/mtd/mtd.h > > > @@ -127,6 +127,36 @@ struct mtd_ooblayout_ops { > > > struct mtd_oob_region *oobfree); > > > }; > > > > > > +/** > > > + * struct mtd_pairing_info - Page pairing information > > > + * > > > + * @pair: represent the pair index in the paired pages table.For > > > example, if > > > > Needs a space after the period. > > Yep. > > > > > > + * page 0 and page 2 are paired together they form the first > > > pair. > > > > This example doesn't help. What would the value of @pair be in this > > case? "First pair" doesn't translate to an integer unambiguously. > > pair 0 > > > > > > + * @group: the group represent the bit position in the cell. For example, > > > + * page 0 uses bit 0 and is thus part of group 0. > > > > I can barely understand what your description for these two fields > > means. I think you need a much more verbose overall description for the > > struct (some here, and maybe more in mtd_pairing_scheme?), and some > > better specifics about what values to expect in the fields. For example > > you might include language like: "this struct describes a single write > > unit in terms of its page pairing geometry." > > > > Also, the "pair" term (and examples you use) seem to imply 2-cell MLC, > > whereas I believe you're trying to handle TLC too. I don't know if we > > should drop the "pair" term, or just explain it better. > > I clearly have some problems with the words I've chosen, but those terms > were extracted from NAND datasheets (group and pair), and I think > keeping the same wording help people converting datasheet specs into > pairing scheme implementation. > > Any suggestions to replace those 2 words? I'm not sure we should replace the words (esp. if those are used by multiple vendors). I just think you might need better examples -- for instance, an example witih TLC. Also, (0, 0) is the trivial case; perhaps a non-zero case? I'm also wondering how I use this stuct and accompanying API to answer questions like "what page(s) are paired with page X"? I understand I can convert from a page number to a 'pairing_info', but how do I determine the other pages in my pairing? I guess it's implied that I can modify the 'group' to any other value in [0, ngroups) then run get_wunit() to get the inverse? I can understand why you might do this instead of passing back an array (for instance), but I think it deserves a little bit of explanation. > > > > You also need to steal more documentation from your commit message and > > cover and put it somewhere, whether it's the comments or > > Documentation/mtd/nand/. > > Okay. > > > > > > + */ > > > +struct mtd_pairing_info { > > > + int pair; > > > + int group; > > > +}; > > > + > > > +/** > > > + * struct mtd_pairing_scheme - Page pairing information > > > + * > > > + * @ngroups: number of groups. Should be related to the number of bits > > > + * per cell. > > > + * @get_info: get the paring info of a given write-unit (ie page). This s/ie/i.e.,/ > > > + * function should fill the info struct passed in argument. > > > + * @get_page: convert paring information into a write-unit (page) number. > > > + */ > > > +struct mtd_pairing_scheme { > > > + int ngroups; > > > + void (*get_info)(struct mtd_info *mtd, int wunit, > > > + struct mtd_pairing_info *info); > > > + int (*get_wunit)(struct mtd_info *mtd, > > > + const struct mtd_pairing_info *info); > > > +}; > > > + > > > struct module; /* only needed for owner field in mtd_info */ > > > > > > struct mtd_info { > > > @@ -188,6 +218,9 @@ struct mtd_info { > > > /* OOB layout description */ > > > const struct mtd_ooblayout_ops *ooblayout; > > > > > > + /* NAND pairing scheme, only provided for MLC/TLC NANDs */ > > > + const struct mtd_pairing_scheme *pairing; > > > + > > > /* the ecc step size. */ > > > unsigned int ecc_step_size; > > > > > > @@ -312,6 +345,32 @@ static inline int mtd_oobavail(struct mtd_info *mtd, > > > struct mtd_oob_ops *ops) > > > return ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize; > > > } > > > > > > +static inline int mtd_offset_to_wunit(struct mtd_info *mtd, loff_t offs) > > > +{ > > > + if (mtd->erasesize_mask) > > > + offs &= mtd->erasesize_mask; > > > + else > > > + offs = offs % mtd->erasesize; > > > > Since you're doing the in-place operators everywhere else, why not > > > > offs %= mtd->erasesize; > > > > ? > > > > > + > > > + if (mtd->writesize_shift) > > > + offs >>= mtd->writesize_shift; > > > + else > > > + offs %= mtd->writesize; > > > > Uhh, this is wrong. You meant divide, not mod, right? And in that case, > > why not just use: > > > > return mtd_div_by_ws(offs, mtd); > > > > ? Or even save yourself most of the trouble and replace the whole > > function with this: > > > > return mtd_mod_by_eb(mtd_div_by_ws(offs, mtd), mtd); > > Definitely better, thanks. Well, not really; mine is buggy too! I have those in the wrong order. Should be: return mtd_div_by_ws(mtd_mod_by_eb(offs, mtd), mtd); Brian
