[PATCH v2 0/3] mtd: add support for pairing scheme description

[PATCH v2 0/3] mtd: add support for pairing scheme description

[Boris Brezillon]

Hi,

This series is the first step towards reliable MLC/TLC NAND support.
Those patches allows the NAND layer to expose page pairing information
to MTD users.
The plan is to teach UBI about those constraints and let UBI code take
the appropriate precautions when dealing with those multi-level cells
NANDs. The way we'll handle this "paired pages" constraint will be
described soon in a series adapting the UBI layer, so stay tune ;).

Note that this implementation only allows page pairing scheme description
when the NAND has a full-id entry in the nand_ids table.
This should be addressed in some way for ONFI and JEDEC NANDs, though
I'm not sure how to handle this yet.

Brian, I tried document a bit more the structures and helpers added in
the first patch, but I'm not sure my descriptions are understandable.

Georges, I took most of your suggestions into account, except for the
struct mtd_pairing_info to unsigned int migration. I'm really unsure
we need this level of optimization right now, and that's still
something we'll be able to change afterwards (I don't expect to see a
lot of users for this API).

Best Regards,

Boris

Changes since v1:
- document the paring scheme concepts and the associate structures and
  helpers
- rework the dist3 and dist6 implementation according to George
  comments
- introduce the mtd_set_pairing_scheme() helper to avoid directly
  manipulating the mtd->pairing field
- drop the patch assigning dist6 pairing scheme to the H27UCG8T2ATR
  NAND (in the light of George comment I'm not longer sure this scheme
  is suitable for this NAND, and I can't test it)

Boris Brezillon (3):
  mtd: introduce the mtd_pairing_scheme concept
  mtd: nand: implement two pairing scheme
  mtd: nand: add a pairing field to nand_flash_dev

 drivers/mtd/mtdcore.c        |  94 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdpart.c        |   1 +
 drivers/mtd/nand/nand_base.c |  98 +++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h      | 106 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |   5 ++
 5 files changed, 304 insertions(+)

-- 
2.7.4

[PATCH v2 1/3] mtd: introduce the mtd_pairing_scheme concept

[Boris Brezillon]

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   |  94 ++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdpart.c   |   1 +
 include/linux/mtd/mtd.h | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e3936b847c6b..decceb9fdf32 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -376,6 +376,100 @@ 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: 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) to stay
+ * consistent with the ->writesize field.
+ *
+ * The @wunit argument can be extracted from an absolute offset using
+ * mtd_offset_to_wunit(). @info is filled with the pairing information attached
+ * to @wunit.
+ *
+ * From the pairing info the MTD user can find all the wunits paired with
+ * @wunit using the following loop:
+ *
+ * for (i = 0; i < mtd_pairing_groups(mtd); i++) {
+ *	info.pair = i;
+ *	mtd_pairing_info_to_wunit(mtd, &info);
+ *	...
+ * }
+ */
+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
+ *
+ * Returns a positive number representing the wunit associated to the info
+ * struct, or a negative error code.
+ *
+ * This is the reverse of mtd_wunit_to_pairing_info(), and can help one to
+ * iterate over all wunits of a given pair (see mtd_wunit_to_pairing_info()
+ * doc).
+ *
+ * It can also be used to only program the first page of each pair (i.e.
+ * page attached to group 0), which allows one to use an MLC NAND in
+ * software-emulated SLC mode:
+ *
+ * info.group = 0;
+ * for (info.pair = 0; info < mtd_wunit_per_eb(mtd); info.pair++) {
+ *	wunit = mtd_pairing_info_to_wunit(mtd, &info);
+ *	mtd_write(mtd, mtd_wunit_to_offset(mtd, blkoffs, wunit),
+ *		  mtd->writesize, &retlen, buf + (i * mtd->writesize));
+ * }
+ */
+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 - get the number of pairing groups
+ * @mtd: pointer to new MTD device info structure
+ *
+ * Returns the number of pairing groups.
+ *
+ * This number is usually equal to the number of bits exposed by a single
+ * cell, and can be used in conjunction with mtd_pairing_info_to_wunit()
+ * to iterate over all pages of a given pair.
+ */
+int mtd_pairing_groups(struct mtd_info *mtd)
+{
+	if (!mtd->pairing || !mtd->pairing->ngroups)
+		return 1;
+
+	return mtd->pairing->ngroups;
+}
+EXPORT_SYMBOL_GPL(mtd_pairing_groups);
+
+/**
  *	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 1f13e32556f8..e32a0ac2298f 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 29a170612203..00bcacb16176 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -127,6 +127,81 @@ struct mtd_ooblayout_ops {
 		    struct mtd_oob_region *oobfree);
 };
 
+/**
+ * struct mtd_pairing_info - page pairing information
+ *
+ * @pair: pair id
+ * @group: group id
+ *
+ * The pair word is used here, even though TLC NANDs might group pages by 3
+ * (3 bits in a single cell). A pair should regroup all pages that are sharing
+ * the same cell. Pairs are then indexed in ascending order.
+ *
+ * @group is defining the position of a page in a given pair. It can also be
+ * seen as the bit position in the cell: page attached to bit 0 belongs to
+ * group 0, page attached to bit 1 belongs to group 1, etc.
+ *
+ * Example:
+ * The H27UCG8T2BTR-BC datasheet describes the following pairing scheme:
+ *
+ *		group-0		group-1
+ *
+ *  pair-0	page-0		page-4
+ *  pair-1	page-1		page-5
+ *  pair-2	page-2		page-8
+ *  ...
+ *  pair-127	page-251	page-255
+ *
+ *
+ * Note that the "group" and "pair" terms were extracted from Samsung and
+ * Hynix datasheets, and might be referenced under other names in other
+ * datasheets (Micron is describing this concept as "shared pages").
+ */
+struct mtd_pairing_info {
+	int pair;
+	int group;
+};
+
+/**
+ * struct mtd_pairing_scheme - page pairing scheme description
+ *
+ * @ngroups: number of groups. Should be related to the number of bits
+ *	     per cell.
+ * @get_info: converts a write-unit (page number within an erase block) into
+ *	      mtd_pairing information (pair + group). This function should
+ *	      fill the info parameter based on the wunit index.
+ * @get_wunit: converts pairing information into a write-unit (page) number.
+ *	       This function should return the wunit index pointed by the
+ *	       pairing information described in the info argument. It should
+ *	       return -EINVAL, if there's no wunit corresponding to the
+ *	       passed pairing information.
+ *
+ * See mtd_pairing_info documentation for a detailed explanation of the
+ * pair and group concepts.
+ *
+ * The mtd_pairing_scheme structure provides a generic solution to represent
+ * NAND page pairing scheme. Instead of exposing two big tables to do the
+ * write-unit <-> (pair + group) conversions, we ask the MTD drivers to
+ * implement the ->get_info() and ->get_wunit() functions.
+ *
+ * MTD users will then be able to query these information by using the
+ * mtd_pairing_info_to_wunit() and mtd_wunit_to_pairing_info() helpers.
+ *
+ * @ngroups is here to help MTD users iterating over all the pages in a
+ * given pair. This value can be retrieved by MTD users using the
+ * mtd_pairing_groups() helper.
+ *
+ * Examples are given in the mtd_pairing_info_to_wunit() and
+ * mtd_wunit_to_pairing_info() documentation.
+ */
+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 +263,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;
 
@@ -296,6 +374,12 @@ static inline void mtd_set_ooblayout(struct mtd_info *mtd,
 	mtd->ooblayout = ooblayout;
 }
 
+static inline void mtd_set_pairing_scheme(struct mtd_info *mtd,
+				const struct mtd_pairing_scheme *pairing)
+{
+	mtd->pairing = pairing;
+}
+
 static inline void mtd_set_of_node(struct mtd_info *mtd,
 				   struct device_node *np)
 {
@@ -312,6 +396,11 @@ 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;
 }
 
+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(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 +486,23 @@ 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_offset_to_wunit(struct mtd_info *mtd, loff_t offs)
+{
+	return mtd_div_by_ws(mtd_mod_by_eb(offs, mtd), mtd);
+}
+
+static inline loff_t mtd_wunit_to_offset(struct mtd_info *mtd, loff_t base,
+					 int wunit)
+{
+	return base + (wunit * mtd->writesize);
+}
+
+
 static inline int mtd_has_oob(const struct mtd_info *mtd)
 {
 	return mtd->_read_oob && mtd->_write_oob;
-- 
2.7.4

[PATCH v2 2/3] mtd: nand: implement two pairing scheme

[Boris Brezillon]

Implement two common pairing scheme (found on many MLC devices), and name
them in reference to the paired pages distance (3 or 6 pages).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/nand/nand_base.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  3 ++
 2 files changed, 100 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 0b0dc29d2af7..dc30e3c5c032 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4293,6 +4293,103 @@ static bool nand_ecc_strength_good(struct mtd_info *mtd)
 	return corr >= ds_corr && ecc->strength >= chip->ecc_strength_ds;
 }
 
+static void nand_pairing_dist3_get_info(struct mtd_info *mtd, int page,
+					struct mtd_pairing_info *info)
+{
+	bool lastpage = ((page + 1) * mtd->writesize) == mtd->erasesize;
+
+	/*
+	 * The first and last pages are special cases.
+	 * To simplify the code and keep the same distance for everyone, we
+	 * increment all pages by 1 except the first one (page 0). The last
+	 * page receives an extra +1 for the same reason.
+	 */
+	page += (page != 0) + lastpage;
+
+	/*
+	 * The datasheets state that odd pages should be part of group
+	 * 0 and even ones part of group 1 (except for the last and
+	 * first pages), but we incremented the page number, that's why we're
+	 * doing the reverse test here.
+	 */
+	info->group = page & 1;
+
+	/*
+	 * We're trying to extract the pair id, which is always equal to
+	 * first_page_of_a_pair / 2. Subtract the distance to the page if it's
+	 * not part of group 0.
+	 */
+	if (page & 1)
+		page -= 3;
+
+	info->pair = page >> 1;
+}
+
+static int __pure nand_pairing_dist3_get_wunit(struct mtd_info *mtd,
+					const struct mtd_pairing_info *info)
+{
+	int page = (info->pair * 2) + (3 * info->group);
+	bool lastpage = ((page * mtd->writesize) > mtd->erasesize);
+
+	/*
+	 * The first and last pages are special cases.
+	 * To simplify the code and keep the same distance for everyone, we
+	 * incremented all pages by 1 except the first one (page 0). The last
+	 * page received an extra +1 for the same reason. Now we need to
+	 * revert that to get the real page number.
+	 */
+	page -= (page != 0) + lastpage;
+
+	return page;
+}
+
+const struct mtd_pairing_scheme dist3_pairing_scheme = {
+	.ngroups = 2,
+	.get_info = nand_pairing_dist3_get_info,
+	.get_wunit = nand_pairing_dist3_get_wunit,
+};
+EXPORT_SYMBOL_GPL(dist3_pairing_scheme);
+
+/*
+ * Distance-6 pairing works like distance-3 pairing, except that pages
+ * are taken two at a time.  The lsbit of the page number is chopped off
+ * and later re-added as the lsbit of the pair number.
+ */
+static void nand_pairing_dist6_get_info(struct mtd_info *mtd, int page,
+					struct mtd_pairing_info *info)
+{
+	bool last2pages, lsbit = page & 1;
+
+	page >>= 1;
+	last2pages = ((page + 1) * 2 * mtd->writesize) == mtd->erasesize;
+
+	page += (page != 0) + last2pages;
+
+	info->group = page & 1;
+	if (page & 1)
+		page -= 3;
+
+	info->pair = page | lsbit;
+}
+
+static int __pure nand_pairing_dist6_get_wunit(struct mtd_info *mtd,
+					const struct mtd_pairing_info *info)
+{
+	int page = (info->pair & ~1) + (3 * info->group);
+	bool last2pages = (page * 2 * mtd->writesize) > mtd->erasesize;
+
+	page -= (page != 0) + last2pages;
+
+	return 2 * page + (info->pair & 1);
+}
+
+const struct mtd_pairing_scheme dist6_pairing_scheme = {
+	.ngroups = 2,
+	.get_info = nand_pairing_dist6_get_info,
+	.get_wunit = nand_pairing_dist6_get_wunit,
+};
+EXPORT_SYMBOL_GPL(dist6_pairing_scheme);
+
 /**
  * nand_scan_tail - [NAND Interface] Scan for the NAND device
  * @mtd: MTD device structure
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index fbe8e164a4ee..874b2671af90 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -1092,4 +1092,7 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
 /* Default read_oob syndrome implementation */
 int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 			   int page);
+
+extern const struct mtd_pairing_scheme dist3_pairing_scheme;
+extern const struct mtd_pairing_scheme dist6_pairing_scheme;
 #endif /* __LINUX_MTD_NAND_H */
-- 
2.7.4

[PATCH v2 3/3] mtd: nand: add a pairing field to nand_flash_dev

[Boris Brezillon]

Add a new field to attach a pairing scheme to a NAND chip definition
and assign it to mtd->pairing when a full-id match is detected.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 1 +
 include/linux/mtd/nand.h     | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index dc30e3c5c032..920b0b31ff71 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3791,6 +3791,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
 		mtd->writesize = type->pagesize;
 		mtd->erasesize = type->erasesize;
 		mtd->oobsize = type->oobsize;
+		mtd_set_pairing_scheme(mtd, type->pairing);
 
 		chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
 		chip->chipsize = (uint64_t)type->chipsize << 20;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 874b2671af90..685b26efcf82 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -855,6 +855,7 @@ static inline void nand_set_controller_data(struct nand_chip *chip, void *priv)
  * @onfi_timing_mode_default: the default ONFI timing mode entered after a NAND
  *			      reset. Should be deduced from timings described
  *			      in the datasheet.
+ * @pairing: The page pairing scheme used by this NAND, if any.
  *
  */
 struct nand_flash_dev {
@@ -877,6 +878,7 @@ struct nand_flash_dev {
 		uint16_t step_ds;
 	} ecc;
 	int onfi_timing_mode_default;
+	const struct mtd_pairing_scheme *pairing;
 };
 
 /**
-- 
2.7.4

Re: [PATCH v2 0/3] mtd: add support for pairing scheme description

[Boris Brezillon]

Brian, Georges,

It's already too late for 4.8, but I'd like to get this sorted out
quickly, so I can apply the patches on my nand/next branch and get it
tested in linux-next.

Note that there's nothing risky in there: we're only introducing a
new in-kernel API that is not used yet, so even if we get it wrong
we'll be able to fix it without impacting MTD users.

Meanwhile, we'll be able to rebase our UBI/MLC work on top of these
changes, and let people test more easily (only a single series to
apply instead of 2).

To sum-up, can you review this v2 and let me know if you see other
things to fix.

Thanks,

Boris

On Mon, 20 Jun 2016 15:50:15 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi,
> 
> This series is the first step towards reliable MLC/TLC NAND support.
> Those patches allows the NAND layer to expose page pairing information
> to MTD users.
> The plan is to teach UBI about those constraints and let UBI code take
> the appropriate precautions when dealing with those multi-level cells
> NANDs. The way we'll handle this "paired pages" constraint will be
> described soon in a series adapting the UBI layer, so stay tune ;).
> 
> Note that this implementation only allows page pairing scheme description
> when the NAND has a full-id entry in the nand_ids table.
> This should be addressed in some way for ONFI and JEDEC NANDs, though
> I'm not sure how to handle this yet.
> 
> Brian, I tried document a bit more the structures and helpers added in
> the first patch, but I'm not sure my descriptions are understandable.
> 
> Georges, I took most of your suggestions into account, except for the
> struct mtd_pairing_info to unsigned int migration. I'm really unsure
> we need this level of optimization right now, and that's still
> something we'll be able to change afterwards (I don't expect to see a
> lot of users for this API).
> 
> Best Regards,
> 
> Boris
> 
> Changes since v1:
> - document the paring scheme concepts and the associate structures and
>   helpers
> - rework the dist3 and dist6 implementation according to George
>   comments
> - introduce the mtd_set_pairing_scheme() helper to avoid directly
>   manipulating the mtd->pairing field
> - drop the patch assigning dist6 pairing scheme to the H27UCG8T2ATR
>   NAND (in the light of George comment I'm not longer sure this scheme
>   is suitable for this NAND, and I can't test it)
> 
> Boris Brezillon (3):
>   mtd: introduce the mtd_pairing_scheme concept
>   mtd: nand: implement two pairing scheme
>   mtd: nand: add a pairing field to nand_flash_dev
> 
>  drivers/mtd/mtdcore.c        |  94 ++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/mtdpart.c        |   1 +
>  drivers/mtd/nand/nand_base.c |  98 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h      | 106 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h     |   5 ++
>  5 files changed, 304 insertions(+)
> 

Re: [PATCH v2 1/3] mtd: introduce the mtd_pairing_scheme concept

[Brian Norris]

Hi Boris,

On Mon, Jun 20, 2016 at 03:50:16PM +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).
> 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>

Overall, the comments and documentation are a lot better on this one.
Thanks for doing that! I only have a few more small comments, and with
those, I think it's ready to land IMO. I'll try to review the NAND
implementation bits too (look OK for now), but I'm not as worried about
that, if we agree on the high-level API.

BTW, I don't know if we're likely to hit any conflicts on the
mtdcore and mtd.h bits. Perhaps it will make sense for us to apply this
first patch as a mini-branch to both our trees? Maybe if you just fixup
any last comments, you can send me a trivial pull request / tag /
whatever (doesn't need to be formal), with just this patch.

> ---
>  drivers/mtd/mtdcore.c   |  94 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/mtdpart.c   |   1 +
>  include/linux/mtd/mtd.h | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 201 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index e3936b847c6b..decceb9fdf32 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -376,6 +376,100 @@ 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: write unit we are interrested in

s/interrested/interested/

> + * @info: pairing information struct

Maybe something to indicate this is the return value? e.g., "returned
pairing information"?

> + *
> + * 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) to stay
> + * consistent with the ->writesize field.
> + *
> + * The @wunit argument can be extracted from an absolute offset using
> + * mtd_offset_to_wunit(). @info is filled with the pairing information attached
> + * to @wunit.
> + *
> + * From the pairing info the MTD user can find all the wunits paired with
> + * @wunit using the following loop:
> + *
> + * for (i = 0; i < mtd_pairing_groups(mtd); i++) {
> + *	info.pair = i;
> + *	mtd_pairing_info_to_wunit(mtd, &info);
> + *	...
> + * }
> + */
> +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit,
> +			       struct mtd_pairing_info *info)
> +{

Do we want to do any range-checking here? i.e., make this return int? Or
is that too paranoid? We've done similarly on most of the rest of the
MTD API.

Notably, I think we're probably safe keeping the ->pairing->get_info()
callback as returning void, since the driver can expect this core helper
to do the range checking for us.

> +	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
> + *
> + * Returns a positive number representing the wunit associated to the info
> + * struct, or a negative error code.
> + *
> + * This is the reverse of mtd_wunit_to_pairing_info(), and can help one to
> + * iterate over all wunits of a given pair (see mtd_wunit_to_pairing_info()
> + * doc).
> + *
> + * It can also be used to only program the first page of each pair (i.e.
> + * page attached to group 0), which allows one to use an MLC NAND in
> + * software-emulated SLC mode:
> + *
> + * info.group = 0;
> + * for (info.pair = 0; info < mtd_wunit_per_eb(mtd); info.pair++) {

(I know it's just example code, but...) the second clause should have
'info.pair < ...', not 'info < ...'.

> + *	wunit = mtd_pairing_info_to_wunit(mtd, &info);
> + *	mtd_write(mtd, mtd_wunit_to_offset(mtd, blkoffs, wunit),
> + *		  mtd->writesize, &retlen, buf + (i * mtd->writesize));
> + * }
> + */
> +int mtd_pairing_info_to_wunit(struct mtd_info *mtd,
> +			      const struct mtd_pairing_info *info)
> +{

Any range checking on info->group or info->pair? What about
NULL-checking '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 - get the number of pairing groups
> + * @mtd: pointer to new MTD device info structure
> + *
> + * Returns the number of pairing groups.
> + *
> + * This number is usually equal to the number of bits exposed by a single
> + * cell, and can be used in conjunction with mtd_pairing_info_to_wunit()
> + * to iterate over all pages of a given pair.
> + */
> +int mtd_pairing_groups(struct mtd_info *mtd)
> +{
> +	if (!mtd->pairing || !mtd->pairing->ngroups)
> +		return 1;
> +
> +	return mtd->pairing->ngroups;
> +}
> +EXPORT_SYMBOL_GPL(mtd_pairing_groups);
> +
> +/**
>   *	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 1f13e32556f8..e32a0ac2298f 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 29a170612203..00bcacb16176 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -127,6 +127,81 @@ struct mtd_ooblayout_ops {
>  		    struct mtd_oob_region *oobfree);
>  };
>  
> +/**
> + * struct mtd_pairing_info - page pairing information
> + *
> + * @pair: pair id
> + * @group: group id
> + *
> + * The pair word is used here, even though TLC NANDs might group pages by 3

Nit: "The pair word is used" is somewhat confusing on first read, IMO. I
think maybe it's partly the ordering of the words, as well as the use
"word" which has different technical meaning sometime... Maybe one of
the following?

  The word "pair" is used here ...
  The term "pair" is used here ...

(Sorry, very nitpicky.)

> + * (3 bits in a single cell). A pair should regroup all pages that are sharing
> + * the same cell. Pairs are then indexed in ascending order.
> + *
> + * @group is defining the position of a page in a given pair. It can also be
> + * seen as the bit position in the cell: page attached to bit 0 belongs to
> + * group 0, page attached to bit 1 belongs to group 1, etc.
> + *
> + * Example:
> + * The H27UCG8T2BTR-BC datasheet describes the following pairing scheme:
> + *
> + *		group-0		group-1
> + *
> + *  pair-0	page-0		page-4
> + *  pair-1	page-1		page-5
> + *  pair-2	page-2		page-8
> + *  ...
> + *  pair-127	page-251	page-255
> + *
> + *
> + * Note that the "group" and "pair" terms were extracted from Samsung and
> + * Hynix datasheets, and might be referenced under other names in other
> + * datasheets (Micron is describing this concept as "shared pages").

Very, very helpful (to me, even though I'm moderately familiar with the
concepts, but hopefully moreso for others who want to read and
understand this). Thanks for writing this up.

> + */
> +struct mtd_pairing_info {
> +	int pair;
> +	int group;
> +};
> +
> +/**
> + * struct mtd_pairing_scheme - page pairing scheme description
> + *
> + * @ngroups: number of groups. Should be related to the number of bits
> + *	     per cell.
> + * @get_info: converts a write-unit (page number within an erase block) into
> + *	      mtd_pairing information (pair + group). This function should
> + *	      fill the info parameter based on the wunit index.
> + * @get_wunit: converts pairing information into a write-unit (page) number.
> + *	       This function should return the wunit index pointed by the
> + *	       pairing information described in the info argument. It should
> + *	       return -EINVAL, if there's no wunit corresponding to the
> + *	       passed pairing information.
> + *
> + * See mtd_pairing_info documentation for a detailed explanation of the
> + * pair and group concepts.
> + *
> + * The mtd_pairing_scheme structure provides a generic solution to represent
> + * NAND page pairing scheme. Instead of exposing two big tables to do the
> + * write-unit <-> (pair + group) conversions, we ask the MTD drivers to
> + * implement the ->get_info() and ->get_wunit() functions.
> + *
> + * MTD users will then be able to query these information by using the
> + * mtd_pairing_info_to_wunit() and mtd_wunit_to_pairing_info() helpers.
> + *
> + * @ngroups is here to help MTD users iterating over all the pages in a
> + * given pair. This value can be retrieved by MTD users using the
> + * mtd_pairing_groups() helper.
> + *
> + * Examples are given in the mtd_pairing_info_to_wunit() and
> + * mtd_wunit_to_pairing_info() documentation.
> + */
> +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);

Wait, I noted above that get_info() doesn't return errors (and that's
OK, if we do bounds checking in mtdcore), but why does get_wunit(),
then? From the looks of it, you don't actually do any bounds checking in
the implementations in patch 2, right? And couldn't we do any checking
in the mtdcore.c helper anyway?

Unless I'm misunderstanding something, I think we should have both
return errors, or neither.

> +};
> +
>  struct module;	/* only needed for owner field in mtd_info */
>  
>  struct mtd_info {
> @@ -188,6 +263,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;
>  
> @@ -296,6 +374,12 @@ static inline void mtd_set_ooblayout(struct mtd_info *mtd,
>  	mtd->ooblayout = ooblayout;
>  }
>  
> +static inline void mtd_set_pairing_scheme(struct mtd_info *mtd,
> +				const struct mtd_pairing_scheme *pairing)
> +{
> +	mtd->pairing = pairing;
> +}
> +
>  static inline void mtd_set_of_node(struct mtd_info *mtd,
>  				   struct device_node *np)
>  {
> @@ -312,6 +396,11 @@ 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;
>  }
>  
> +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(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 +486,23 @@ 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_offset_to_wunit(struct mtd_info *mtd, loff_t offs)
> +{
> +	return mtd_div_by_ws(mtd_mod_by_eb(offs, mtd), mtd);
> +}
> +
> +static inline loff_t mtd_wunit_to_offset(struct mtd_info *mtd, loff_t base,
> +					 int wunit)
> +{
> +	return base + (wunit * mtd->writesize);
> +}
> +
> +
>  static inline int mtd_has_oob(const struct mtd_info *mtd)
>  {
>  	return mtd->_read_oob && mtd->_write_oob;

With the above addressed:

Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH v2 1/3] mtd: introduce the mtd_pairing_scheme concept

[Boris Brezillon]

Hi Brian,

On Thu, 4 Aug 2016 12:37:51 +0800
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> On Mon, Jun 20, 2016 at 03:50:16PM +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).
> > 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>  
> 
> Overall, the comments and documentation are a lot better on this one.
> Thanks for doing that! I only have a few more small comments, and with
> those, I think it's ready to land IMO. I'll try to review the NAND
> implementation bits too (look OK for now), but I'm not as worried about
> that, if we agree on the high-level API.
> 
> BTW, I don't know if we're likely to hit any conflicts on the
> mtdcore and mtd.h bits. Perhaps it will make sense for us to apply this
> first patch as a mini-branch to both our trees? Maybe if you just fixup
> any last comments, you can send me a trivial pull request / tag /
> whatever (doesn't need to be formal), with just this patch.

Sure.

> 
> > ---
> >  drivers/mtd/mtdcore.c   |  94 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/mtd/mtdpart.c   |   1 +
> >  include/linux/mtd/mtd.h | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 201 insertions(+)
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index e3936b847c6b..decceb9fdf32 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -376,6 +376,100 @@ 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: write unit we are interrested in  
> 
> s/interrested/interested/
> 
> > + * @info: pairing information struct  
> 
> Maybe something to indicate this is the return value? e.g., "returned
> pairing information"?

I'll change the description.

> 
> > + *
> > + * 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) to stay
> > + * consistent with the ->writesize field.
> > + *
> > + * The @wunit argument can be extracted from an absolute offset using
> > + * mtd_offset_to_wunit(). @info is filled with the pairing information attached
> > + * to @wunit.
> > + *
> > + * From the pairing info the MTD user can find all the wunits paired with
> > + * @wunit using the following loop:
> > + *
> > + * for (i = 0; i < mtd_pairing_groups(mtd); i++) {
> > + *	info.pair = i;
> > + *	mtd_pairing_info_to_wunit(mtd, &info);
> > + *	...
> > + * }
> > + */
> > +void mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit,
> > +			       struct mtd_pairing_info *info)
> > +{  
> 
> Do we want to do any range-checking here? i.e., make this return int? Or
> is that too paranoid? We've done similarly on most of the rest of the
> MTD API.

I'm fine changing the prototype to return an int (with -ERANGE if the
wunit parameter is exceeding the number of write-units per eraseblock).
As you say later in your review, we'd better be consistent on the
->get_info()/->get_wunit() semantic.

> 
> Notably, I think we're probably safe keeping the ->pairing->get_info()
> callback as returning void, since the driver can expect this core helper
> to do the range checking for us.
> 
> > +	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
> > + *
> > + * Returns a positive number representing the wunit associated to the info
> > + * struct, or a negative error code.
> > + *
> > + * This is the reverse of mtd_wunit_to_pairing_info(), and can help one to
> > + * iterate over all wunits of a given pair (see mtd_wunit_to_pairing_info()
> > + * doc).
> > + *
> > + * It can also be used to only program the first page of each pair (i.e.
> > + * page attached to group 0), which allows one to use an MLC NAND in
> > + * software-emulated SLC mode:
> > + *
> > + * info.group = 0;
> > + * for (info.pair = 0; info < mtd_wunit_per_eb(mtd); info.pair++) {  
> 
> (I know it's just example code, but...) the second clause should have
> 'info.pair < ...', not 'info < ...'.

I'll fix the example.

> 
> > + *	wunit = mtd_pairing_info_to_wunit(mtd, &info);
> > + *	mtd_write(mtd, mtd_wunit_to_offset(mtd, blkoffs, wunit),
> > + *		  mtd->writesize, &retlen, buf + (i * mtd->writesize));
> > + * }
> > + */
> > +int mtd_pairing_info_to_wunit(struct mtd_info *mtd,
> > +			      const struct mtd_pairing_info *info)
> > +{  
> 
> Any range checking on info->group or info->pair? What about
> NULL-checking 'info'?

I'll add those checks.

> 
> > +	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 - get the number of pairing groups
> > + * @mtd: pointer to new MTD device info structure
> > + *
> > + * Returns the number of pairing groups.
> > + *
> > + * This number is usually equal to the number of bits exposed by a single
> > + * cell, and can be used in conjunction with mtd_pairing_info_to_wunit()
> > + * to iterate over all pages of a given pair.
> > + */
> > +int mtd_pairing_groups(struct mtd_info *mtd)
> > +{
> > +	if (!mtd->pairing || !mtd->pairing->ngroups)
> > +		return 1;
> > +
> > +	return mtd->pairing->ngroups;
> > +}
> > +EXPORT_SYMBOL_GPL(mtd_pairing_groups);
> > +
> > +/**
> >   *	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 1f13e32556f8..e32a0ac2298f 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 29a170612203..00bcacb16176 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -127,6 +127,81 @@ struct mtd_ooblayout_ops {
> >  		    struct mtd_oob_region *oobfree);
> >  };
> >  
> > +/**
> > + * struct mtd_pairing_info - page pairing information
> > + *
> > + * @pair: pair id
> > + * @group: group id
> > + *
> > + * The pair word is used here, even though TLC NANDs might group pages by 3  
> 
> Nit: "The pair word is used" is somewhat confusing on first read, IMO. I
> think maybe it's partly the ordering of the words, as well as the use
> "word" which has different technical meaning sometime... Maybe one of
> the following?
> 
>   The word "pair" is used here ...
>   The term "pair" is used here ...
> 
> (Sorry, very nitpicky.)

No problem :), I'll pick one of those.

> 
> > + * (3 bits in a single cell). A pair should regroup all pages that are sharing
> > + * the same cell. Pairs are then indexed in ascending order.
> > + *
> > + * @group is defining the position of a page in a given pair. It can also be
> > + * seen as the bit position in the cell: page attached to bit 0 belongs to
> > + * group 0, page attached to bit 1 belongs to group 1, etc.
> > + *
> > + * Example:
> > + * The H27UCG8T2BTR-BC datasheet describes the following pairing scheme:
> > + *
> > + *		group-0		group-1
> > + *
> > + *  pair-0	page-0		page-4
> > + *  pair-1	page-1		page-5
> > + *  pair-2	page-2		page-8
> > + *  ...
> > + *  pair-127	page-251	page-255
> > + *
> > + *
> > + * Note that the "group" and "pair" terms were extracted from Samsung and
> > + * Hynix datasheets, and might be referenced under other names in other
> > + * datasheets (Micron is describing this concept as "shared pages").  
> 
> Very, very helpful (to me, even though I'm moderately familiar with the
> concepts, but hopefully moreso for others who want to read and
> understand this). Thanks for writing this up.

Actually, the more I think about it, the more I doubt those terms are
appropriate (even if they are widely used in technical documents).

How about using the following names instead:

struct mtd_cell_sharing_scheme {
	...
};

struct mtd_cell_sharing_info {
	/* the bit position in the cell */
	int bitpos;
	/*
	 * What was previously known as 'pair': an id representing a
	 * group of cells forming a 'pair of pages'.
	 * I can't find a good description/word for this concept. Do
	 * you have better ideas?
	 */
	int group;
};

What do you think?

> 
> > + */
> > +struct mtd_pairing_info {
> > +	int pair;
> > +	int group;
> > +};
> > +
> > +/**
> > + * struct mtd_pairing_scheme - page pairing scheme description
> > + *
> > + * @ngroups: number of groups. Should be related to the number of bits
> > + *	     per cell.
> > + * @get_info: converts a write-unit (page number within an erase block) into
> > + *	      mtd_pairing information (pair + group). This function should
> > + *	      fill the info parameter based on the wunit index.
> > + * @get_wunit: converts pairing information into a write-unit (page) number.
> > + *	       This function should return the wunit index pointed by the
> > + *	       pairing information described in the info argument. It should
> > + *	       return -EINVAL, if there's no wunit corresponding to the
> > + *	       passed pairing information.
> > + *
> > + * See mtd_pairing_info documentation for a detailed explanation of the
> > + * pair and group concepts.
> > + *
> > + * The mtd_pairing_scheme structure provides a generic solution to represent
> > + * NAND page pairing scheme. Instead of exposing two big tables to do the
> > + * write-unit <-> (pair + group) conversions, we ask the MTD drivers to
> > + * implement the ->get_info() and ->get_wunit() functions.
> > + *
> > + * MTD users will then be able to query these information by using the
> > + * mtd_pairing_info_to_wunit() and mtd_wunit_to_pairing_info() helpers.
> > + *
> > + * @ngroups is here to help MTD users iterating over all the pages in a
> > + * given pair. This value can be retrieved by MTD users using the
> > + * mtd_pairing_groups() helper.
> > + *
> > + * Examples are given in the mtd_pairing_info_to_wunit() and
> > + * mtd_wunit_to_pairing_info() documentation.
> > + */
> > +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);  
> 
> Wait, I noted above that get_info() doesn't return errors (and that's
> OK, if we do bounds checking in mtdcore), but why does get_wunit(),
> then? From the looks of it, you don't actually do any bounds checking in
> the implementations in patch 2, right? And couldn't we do any checking
> in the mtdcore.c helper anyway?
> 
> Unless I'm misunderstanding something, I think we should have both
> return errors, or neither.

I agree. ->get-info() could fill mtd_pairing_info to reflect an error,
but that's confusing. Let's switch to functions returning int and patch
the implementation to do bounds checking. 

> 
> > +};
> > +
> >  struct module;	/* only needed for owner field in mtd_info */
> >  
> >  struct mtd_info {
> > @@ -188,6 +263,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;
> >  
> > @@ -296,6 +374,12 @@ static inline void mtd_set_ooblayout(struct mtd_info *mtd,
> >  	mtd->ooblayout = ooblayout;
> >  }
> >  
> > +static inline void mtd_set_pairing_scheme(struct mtd_info *mtd,
> > +				const struct mtd_pairing_scheme *pairing)
> > +{
> > +	mtd->pairing = pairing;
> > +}
> > +
> >  static inline void mtd_set_of_node(struct mtd_info *mtd,
> >  				   struct device_node *np)
> >  {
> > @@ -312,6 +396,11 @@ 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;
> >  }
> >  
> > +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(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 +486,23 @@ 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_offset_to_wunit(struct mtd_info *mtd, loff_t offs)
> > +{
> > +	return mtd_div_by_ws(mtd_mod_by_eb(offs, mtd), mtd);
> > +}
> > +
> > +static inline loff_t mtd_wunit_to_offset(struct mtd_info *mtd, loff_t base,
> > +					 int wunit)
> > +{
> > +	return base + (wunit * mtd->writesize);
> > +}
> > +
> > +
> >  static inline int mtd_has_oob(const struct mtd_info *mtd)
> >  {
> >  	return mtd->_read_oob && mtd->_write_oob;  
> 
> With the above addressed:
> 
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Thanks for the review!

Regards,

Boris

Re: [PATCH v2 1/3] mtd: introduce the mtd_pairing_scheme concept

[Brian Norris]

Hi,

I've had this on my plate to respond to for a while now, and I haven't
brought myself to actually care that much about the choice. So I'll
respond now to keep from leaving you hanging, but I'm not sure I'm that
helpful :(

On Tue, Aug 09, 2016 at 12:42:18AM +0200, Boris Brezillon wrote:
> On Thu, 4 Aug 2016 12:37:51 +0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> > On Mon, Jun 20, 2016 at 03:50:16PM +0200, Boris Brezillon wrote:
> 
> > 
> > > + * (3 bits in a single cell). A pair should regroup all pages that are sharing
> > > + * the same cell. Pairs are then indexed in ascending order.
> > > + *
> > > + * @group is defining the position of a page in a given pair. It can also be
> > > + * seen as the bit position in the cell: page attached to bit 0 belongs to
> > > + * group 0, page attached to bit 1 belongs to group 1, etc.
> > > + *
> > > + * Example:
> > > + * The H27UCG8T2BTR-BC datasheet describes the following pairing scheme:
> > > + *
> > > + *		group-0		group-1
> > > + *
> > > + *  pair-0	page-0		page-4
> > > + *  pair-1	page-1		page-5
> > > + *  pair-2	page-2		page-8
> > > + *  ...
> > > + *  pair-127	page-251	page-255
> > > + *
> > > + *
> > > + * Note that the "group" and "pair" terms were extracted from Samsung and
> > > + * Hynix datasheets, and might be referenced under other names in other
> > > + * datasheets (Micron is describing this concept as "shared pages").  
> > 
> > Very, very helpful (to me, even though I'm moderately familiar with the
> > concepts, but hopefully moreso for others who want to read and
> > understand this). Thanks for writing this up.
> 
> Actually, the more I think about it, the more I doubt those terms are
> appropriate (even if they are widely used in technical documents).
> 
> How about using the following names instead:
> 
> struct mtd_cell_sharing_scheme {
> 	...
> };
> 
> struct mtd_cell_sharing_info {
> 	/* the bit position in the cell */
> 	int bitpos;
> 	/*
> 	 * What was previously known as 'pair': an id representing a

Wait, so you're replacing the literature's "pair" term with "group", but
the literature already used "group" to mean something else? That seems
to be an unwise choice. (Or I'm misreading you.)

> 	 * group of cells forming a 'pair of pages'.
> 	 * I can't find a good description/word for this concept. Do
> 	 * you have better ideas?
> 	 */
> 	int group;
> };
> 
> What do you think?

I think there's something to be said for matching the literature out
there, and I personally thought that simply providing a little bit of
clarifying explanation in the comments was sufficient. But if you feel
like choosing a more generic name is better, then that's probably OK
too. So other than the above comment (don't overload terms too freely!),
I'd use your judgment.

FWIW, it still takes me a while to parse what the "pair" and "group" (or
"bitpos" and "group" -- although "bitpos" is actually quite clear, so I
guess I like that) actually mean, so I tend to refer back to these
comments every time I'm reading it.

Brian

Re: [PATCH v2 1/3] mtd: introduce the mtd_pairing_scheme concept

[Boris Brezillon]

On Thu, 1 Sep 2016 11:15:24 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi,
> 
> I've had this on my plate to respond to for a while now, and I haven't
> brought myself to actually care that much about the choice. So I'll
> respond now to keep from leaving you hanging, but I'm not sure I'm that
> helpful :(

No problem. Actually, I've been busy with other problems too.

> 
> On Tue, Aug 09, 2016 at 12:42:18AM +0200, Boris Brezillon wrote:
> > On Thu, 4 Aug 2016 12:37:51 +0800
> > Brian Norris <computersforpeace@gmail.com> wrote:  
> > > On Mon, Jun 20, 2016 at 03:50:16PM +0200, Boris Brezillon wrote:  
> >   
> > >   
> > > > + * (3 bits in a single cell). A pair should regroup all pages that are sharing
> > > > + * the same cell. Pairs are then indexed in ascending order.
> > > > + *
> > > > + * @group is defining the position of a page in a given pair. It can also be
> > > > + * seen as the bit position in the cell: page attached to bit 0 belongs to
> > > > + * group 0, page attached to bit 1 belongs to group 1, etc.
> > > > + *
> > > > + * Example:
> > > > + * The H27UCG8T2BTR-BC datasheet describes the following pairing scheme:
> > > > + *
> > > > + *		group-0		group-1
> > > > + *
> > > > + *  pair-0	page-0		page-4
> > > > + *  pair-1	page-1		page-5
> > > > + *  pair-2	page-2		page-8
> > > > + *  ...
> > > > + *  pair-127	page-251	page-255
> > > > + *
> > > > + *
> > > > + * Note that the "group" and "pair" terms were extracted from Samsung and
> > > > + * Hynix datasheets, and might be referenced under other names in other
> > > > + * datasheets (Micron is describing this concept as "shared pages").    
> > > 
> > > Very, very helpful (to me, even though I'm moderately familiar with the
> > > concepts, but hopefully moreso for others who want to read and
> > > understand this). Thanks for writing this up.  
> > 
> > Actually, the more I think about it, the more I doubt those terms are
> > appropriate (even if they are widely used in technical documents).
> > 
> > How about using the following names instead:
> > 
> > struct mtd_cell_sharing_scheme {
> > 	...
> > };
> > 
> > struct mtd_cell_sharing_info {
> > 	/* the bit position in the cell */
> > 	int bitpos;
> > 	/*
> > 	 * What was previously known as 'pair': an id representing a  
> 
> Wait, so you're replacing the literature's "pair" term with "group", but
> the literature already used "group" to mean something else? That seems
> to be an unwise choice. (Or I'm misreading you.)
> 
> > 	 * group of cells forming a 'pair of pages'.
> > 	 * I can't find a good description/word for this concept. Do
> > 	 * you have better ideas?
> > 	 */
> > 	int group;
> > };
> > 
> > What do you think?  
> 
> I think there's something to be said for matching the literature out
> there, and I personally thought that simply providing a little bit of
> clarifying explanation in the comments was sufficient. But if you feel
> like choosing a more generic name is better, then that's probably OK
> too. So other than the above comment (don't overload terms too freely!),
> I'd use your judgment.
> 
> FWIW, it still takes me a while to parse what the "pair" and "group" (or
> "bitpos" and "group" -- although "bitpos" is actually quite clear, so I
> guess I like that) actually mean, so I tend to refer back to these
> comments every time I'm reading it.

Let's stick to my first proposal. I'll address you comment and send a
new version. If you're happy with it, I'll create a branch that we can
share and ask you to pull it.

Thanks,

Boris