| Message ID | 1461578481-26567-2-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, 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? > 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 <boris.brezillon@free-electrons.com> > --- > drivers/mtd/mtdcore.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ > drivers/mtd/mtdpart.c | 1 + > include/linux/mtd/mtd.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 127 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index e3936b8..315adc0 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -376,6 +376,68 @@ static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state, > } > > /** > + * mtd_wunit_to_pairing_info - get pairing information of a wunit > + * @mtd: pointer to new MTD device info structure > + * @wunit: the write unit we are interrested in > + * @info: pairing information struct > + * > + * Retrieve pairing information associated to the wunit. > + * This is mainly useful when dealing with MLC/TLC NANDs where pages > + * can be paired together, and where programming a page may influence > + * the page it is paired with. > + * The notion of page is replaced by the term wunit (write-unit). > + */ > +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, > + struct mtd_pairing_info *info) > +{ > + if (!mtd->pairing || !mtd->pairing->get_info) { > + info->group = 0; > + info->pair = wunit; > + } else { > + mtd->pairing->get_info(mtd, wunit, info); > + } > +} > +EXPORT_SYMBOL_GPL(mtd_wunit_to_pairing_info); > + > +/** > + * mtd_wunit_to_pairing_info - get wunit from pairing information > + * @mtd: pointer to new MTD device info structure > + * @info: pairing information struct > + * > + * Return a positive number representing the wunit associated to the > + * info struct, or a negative error code. > + */ > +int mtd_pairing_info_to_wunit(struct mtd_info *mtd, > + const struct mtd_pairing_info *info) > +{ > + if (!mtd->pairing || !mtd->pairing->get_info) { > + if (info->group) > + return -EINVAL; > + > + return info->pair; > + } > + > + return mtd->pairing->get_wunit(mtd, info); > +} > +EXPORT_SYMBOL_GPL(mtd_pairing_info_to_wunit); > + > +/** > + * mtd_pairing_groups_per_eb - get the number of pairing groups per erase > + * block > + * @mtd: pointer to new MTD device info structure > + * > + * Return the number of pairing groups per erase block. > + */ > +int mtd_pairing_groups_per_eb(struct mtd_info *mtd) > +{ > + if (!mtd->pairing || !mtd->pairing->ngroups) > + return 1; > + > + return mtd->pairing->ngroups; > +} > +EXPORT_SYMBOL_GPL(mtd_pairing_groups_per_eb); > + > +/** > * add_mtd_device - register an MTD device > * @mtd: pointer to new MTD device info structure > * > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index 1f13e32..e32a0ac 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -397,6 +397,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *master, > slave->mtd.oobsize = master->oobsize; > slave->mtd.oobavail = master->oobavail; > slave->mtd.subpage_sft = master->subpage_sft; > + slave->mtd.pairing = master->pairing; > > slave->mtd.name = name; > slave->mtd.owner = master->owner; > 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. > + * 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. > + * @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. 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/. > + */ > +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 > + * 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); ? > + > + return offs; > +} > + > +static inline loff_t mtd_wunit_to_offset(struct mtd_info *mtd, loff_t base, > + int wunit) > +{ > + return base + (wunit * mtd->writesize); > +} > + > +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, > + struct mtd_pairing_info *info); > +int mtd_pairing_info_to_wunit(struct mtd_info *mtd, > + const struct mtd_pairing_info *info); > +int mtd_pairing_groups_per_eb(struct mtd_info *mtd); > int mtd_erase(struct mtd_info *mtd, struct erase_info *instr); > int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, > void **virt, resource_size_t *phys); > @@ -397,6 +456,11 @@ static inline uint32_t mtd_mod_by_ws(uint64_t sz, struct mtd_info *mtd) > return do_div(sz, mtd->writesize); > } > > +static inline int mtd_wunit_per_eb(struct mtd_info *mtd) > +{ > + return mtd->erasesize / mtd->writesize; > +} > + > static inline int mtd_has_oob(const struct mtd_info *mtd) > { > return mtd->_read_oob && mtd->_write_oob; That's all for now. I'll try to get a chance to look some more this weekend. Brian
On Fri, 10 Jun 2016 19:17:15 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > Hi, > > 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. > > > 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 <boris.brezillon@free-electrons.com> > > --- > > drivers/mtd/mtdcore.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/mtd/mtdpart.c | 1 + > > include/linux/mtd/mtd.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 127 insertions(+) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index e3936b8..315adc0 100644 > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -376,6 +376,68 @@ static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state, > > } > > > > /** > > + * mtd_wunit_to_pairing_info - get pairing information of a wunit > > + * @mtd: pointer to new MTD device info structure > > + * @wunit: the write unit we are interrested in > > + * @info: pairing information struct > > + * > > + * Retrieve pairing information associated to the wunit. > > + * This is mainly useful when dealing with MLC/TLC NANDs where pages > > + * can be paired together, and where programming a page may influence > > + * the page it is paired with. > > + * The notion of page is replaced by the term wunit (write-unit). > > + */ > > +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, > > + struct mtd_pairing_info *info) > > +{ > > + if (!mtd->pairing || !mtd->pairing->get_info) { > > + info->group = 0; > > + info->pair = wunit; > > + } else { > > + mtd->pairing->get_info(mtd, wunit, info); > > + } > > +} > > +EXPORT_SYMBOL_GPL(mtd_wunit_to_pairing_info); > > + > > +/** > > + * mtd_wunit_to_pairing_info - get wunit from pairing information > > + * @mtd: pointer to new MTD device info structure > > + * @info: pairing information struct > > + * > > + * Return a positive number representing the wunit associated to the > > + * info struct, or a negative error code. > > + */ > > +int mtd_pairing_info_to_wunit(struct mtd_info *mtd, > > + const struct mtd_pairing_info *info) > > +{ > > + if (!mtd->pairing || !mtd->pairing->get_info) { > > + if (info->group) > > + return -EINVAL; > > + > > + return info->pair; > > + } > > + > > + return mtd->pairing->get_wunit(mtd, info); > > +} > > +EXPORT_SYMBOL_GPL(mtd_pairing_info_to_wunit); > > + > > +/** > > + * mtd_pairing_groups_per_eb - get the number of pairing groups per erase > > + * block > > + * @mtd: pointer to new MTD device info structure > > + * > > + * Return the number of pairing groups per erase block. > > + */ > > +int mtd_pairing_groups_per_eb(struct mtd_info *mtd) > > +{ > > + if (!mtd->pairing || !mtd->pairing->ngroups) > > + return 1; > > + > > + return mtd->pairing->ngroups; > > +} > > +EXPORT_SYMBOL_GPL(mtd_pairing_groups_per_eb); > > + > > +/** > > * add_mtd_device - register an MTD device > > * @mtd: pointer to new MTD device info structure > > * > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > > index 1f13e32..e32a0ac 100644 > > --- a/drivers/mtd/mtdpart.c > > +++ b/drivers/mtd/mtdpart.c > > @@ -397,6 +397,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *master, > > slave->mtd.oobsize = master->oobsize; > > slave->mtd.oobavail = master->oobavail; > > slave->mtd.subpage_sft = master->subpage_sft; > > + slave->mtd.pairing = master->pairing; > > > > slave->mtd.name = name; > > slave->mtd.owner = master->owner; > > 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? > > 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 > > + * 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.
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 <computersforpeace@gmail.com> 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 <boris.brezillon@free-electrons.com> > > > --- > > > 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
On Sun, Jun 12, 2016 at 10:55:32PM -0700, Brian Norris wrote: > On Sat, Jun 11, 2016 at 08:54:08AM +0200, Boris Brezillon wrote: > > On Fri, 10 Jun 2016 19:17:15 -0700 > > Brian Norris <computersforpeace@gmail.com> wrote: [...] > > > 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 see that George highlighted a Micron datasheet in other parts of this thread, and I noticed it uses the term "shared page." That explains why I couldn't find the word "pair" in my quick search of Micron datasheets! So I guess "shared page" would be a nomination, though I'm certainly not forcing it, if you think pair is better. Brian
On Sun, 12 Jun 2016 23:22:29 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > On Sun, Jun 12, 2016 at 10:55:32PM -0700, Brian Norris wrote: > > On Sat, Jun 11, 2016 at 08:54:08AM +0200, Boris Brezillon wrote: > > > On Fri, 10 Jun 2016 19:17:15 -0700 > > > Brian Norris <computersforpeace@gmail.com> wrote: > [...] > > > > 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 see that George highlighted a Micron datasheet in other parts of this > thread, and I noticed it uses the term "shared page." That explains why > I couldn't find the word "pair" in my quick search of Micron datasheets! > So I guess "shared page" would be a nomination, though I'm certainly not > forcing it, if you think pair is better. Samsung and Hynix datasheets are using the term 'paired', and Toshiba ones are not naming this concept. It's just a detail anyway, I'm fine switching to 'shared pages'.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index e3936b8..315adc0 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -376,6 +376,68 @@ static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state, } /** + * mtd_wunit_to_pairing_info - get pairing information of a wunit + * @mtd: pointer to new MTD device info structure + * @wunit: the write unit we are interrested in + * @info: pairing information struct + * + * Retrieve pairing information associated to the wunit. + * This is mainly useful when dealing with MLC/TLC NANDs where pages + * can be paired together, and where programming a page may influence + * the page it is paired with. + * The notion of page is replaced by the term wunit (write-unit). + */ +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, + struct mtd_pairing_info *info) +{ + if (!mtd->pairing || !mtd->pairing->get_info) { + info->group = 0; + info->pair = wunit; + } else { + mtd->pairing->get_info(mtd, wunit, info); + } +} +EXPORT_SYMBOL_GPL(mtd_wunit_to_pairing_info); + +/** + * mtd_wunit_to_pairing_info - get wunit from pairing information + * @mtd: pointer to new MTD device info structure + * @info: pairing information struct + * + * Return a positive number representing the wunit associated to the + * info struct, or a negative error code. + */ +int mtd_pairing_info_to_wunit(struct mtd_info *mtd, + const struct mtd_pairing_info *info) +{ + if (!mtd->pairing || !mtd->pairing->get_info) { + if (info->group) + return -EINVAL; + + return info->pair; + } + + return mtd->pairing->get_wunit(mtd, info); +} +EXPORT_SYMBOL_GPL(mtd_pairing_info_to_wunit); + +/** + * mtd_pairing_groups_per_eb - get the number of pairing groups per erase + * block + * @mtd: pointer to new MTD device info structure + * + * Return the number of pairing groups per erase block. + */ +int mtd_pairing_groups_per_eb(struct mtd_info *mtd) +{ + if (!mtd->pairing || !mtd->pairing->ngroups) + return 1; + + return mtd->pairing->ngroups; +} +EXPORT_SYMBOL_GPL(mtd_pairing_groups_per_eb); + +/** * add_mtd_device - register an MTD device * @mtd: pointer to new MTD device info structure * diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 1f13e32..e32a0ac 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -397,6 +397,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *master, slave->mtd.oobsize = master->oobsize; slave->mtd.oobavail = master->oobavail; slave->mtd.subpage_sft = master->subpage_sft; + slave->mtd.pairing = master->pairing; slave->mtd.name = name; slave->mtd.owner = master->owner; 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 + * page 0 and page 2 are paired together they form the first pair. + * @group: the group represent the bit position in the cell. For example, + * page 0 uses bit 0 and is thus part of group 0. + */ +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 + * 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; + + if (mtd->writesize_shift) + offs >>= mtd->writesize_shift; + else + offs %= mtd->writesize; + + return offs; +} + +static inline loff_t mtd_wunit_to_offset(struct mtd_info *mtd, loff_t base, + int wunit) +{ + return base + (wunit * mtd->writesize); +} + +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, + struct mtd_pairing_info *info); +int mtd_pairing_info_to_wunit(struct mtd_info *mtd, + const struct mtd_pairing_info *info); +int mtd_pairing_groups_per_eb(struct mtd_info *mtd); int mtd_erase(struct mtd_info *mtd, struct erase_info *instr); int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, void **virt, resource_size_t *phys); @@ -397,6 +456,11 @@ static inline uint32_t mtd_mod_by_ws(uint64_t sz, struct mtd_info *mtd) return do_div(sz, mtd->writesize); } +static inline int mtd_wunit_per_eb(struct mtd_info *mtd) +{ + return mtd->erasesize / mtd->writesize; +} + static inline int mtd_has_oob(const struct mtd_info *mtd) { return mtd->_read_oob && mtd->_write_oob;
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). 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 <boris.brezillon@free-electrons.com> --- drivers/mtd/mtdcore.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/mtd/mtdpart.c | 1 + include/linux/mtd/mtd.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+)