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

[PATCH 0/4] 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.

Best Regards,

Boris

Boris Brezillon (4):
  mtd: introduce the mtd_pairing_scheme concept
  mtd: nand: implement two pairing scheme
  mtd: nand: add a pairing field to nand_flash_dev
  mtd: nand: H27UCG8T2ATR: point to the correct pairing scheme
    implementation

 drivers/mtd/mtdcore.c        | 62 ++++++++++++++++++++++++++++
 drivers/mtd/mtdpart.c        |  1 +
 drivers/mtd/nand/nand_base.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/nand_ids.c  |  2 +-
 include/linux/mtd/mtd.h      | 64 +++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  5 +++
 6 files changed, 230 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH 1/4] 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   | 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
+ *	  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;
-- 
2.7.4

[PATCH 2/4] 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 | 96 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  3 ++
 2 files changed, 99 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7bc37b4..a64e0d5 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4274,6 +4274,102 @@ 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)
+{
+	int lastpage = (mtd->erasesize / mtd->writesize) - 1;
+	int dist = 3;
+
+	if (page == lastpage)
+		dist = 2;
+
+	if (!page || (page & 1)) {
+		info->group = 0;
+		info->pair = (page + 1) / 2;
+	} else {
+		info->group = 1;
+		info->pair = (page + 1 - dist) / 2;
+	}
+}
+
+static int nand_pairing_dist3_get_wunit(struct mtd_info *mtd,
+					const struct mtd_pairing_info *info)
+{
+	int lastpair = ((mtd->erasesize / mtd->writesize) - 1) / 2;
+	int page = info->pair * 2;
+	int dist = 3;
+
+	if (!info->group && !info->pair)
+		return 0;
+
+	if (info->pair == lastpair && info->group)
+		dist = 2;
+
+	if (!info->group)
+		page--;
+	else if (info->pair)
+		page += dist - 1;
+
+	if (page >= mtd->erasesize / mtd->writesize)
+		return -EINVAL;
+
+	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);
+
+static void nand_pairing_dist6_get_info(struct mtd_info *mtd, int page,
+					struct mtd_pairing_info *info)
+{
+	int lastpage = (mtd->erasesize / mtd->writesize) - 1;
+	int half = page / 2;
+	int dist = 6;
+
+	if (half == lastpage / 2)
+		dist = 4;
+
+	if (!half  || (half & 1)) {
+		info->group = 0;
+		info->pair = (page + 2) / 2;
+	} else {
+		info->group = 1;
+		info->pair = (page + 2 - dist) / 2;
+	}
+}
+
+static int nand_pairing_dist6_get_wunit(struct mtd_info *mtd,
+				       const struct mtd_pairing_info *info)
+{
+	int lastpair = ((mtd->erasesize / mtd->writesize) - 1) / 2;
+	int page = info->pair * 2;
+	int dist = 6;
+
+	if (!info->group && !info->pair)
+		return 0;
+
+	if (info->pair >= lastpair - 1 && info->group)
+		dist = 4;
+
+	if (!info->group)
+		page += 2;
+	else
+		page += dist - 2;
+
+	return page;
+}
+
+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 fbe8e16..874b267 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 3/4] 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 a64e0d5..0666b59 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->pairing = 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 874b267..685b26e 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

[PATCH 4/4] mtd: nand: H27UCG8T2ATR: point to the correct pairing scheme implementation

[Boris Brezillon]

H27UCG8T2ATR is using a dist6_pairing_scheme pairing scheme.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_ids.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index ccc05f5..3471401 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -51,7 +51,7 @@ struct nand_flash_dev nand_flash_ids[] = {
 	{"H27UCG8T2ATR-BC 64G 3.3V 8-bit",
 		{ .id = {0xad, 0xde, 0x94, 0xda, 0x74, 0xc4} },
 		  SZ_8K, SZ_8K, SZ_2M, NAND_NEED_SCRAMBLING, 6, 640,
-		  NAND_ECC_INFO(40, SZ_1K), 4 },
+		  NAND_ECC_INFO(40, SZ_1K), 4, &dist6_pairing_scheme },
 
 	LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 4, SZ_8K, SP_OPTIONS),
 	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),
-- 
2.7.4

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

[Richard Weinberger]

Am 25.04.2016 um 12:01 schrieb 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.
> 
> Best Regards,
> 
> Boris
> 
> Boris Brezillon (4):
>   mtd: introduce the mtd_pairing_scheme concept
>   mtd: nand: implement two pairing scheme
>   mtd: nand: add a pairing field to nand_flash_dev
>   mtd: nand: H27UCG8T2ATR: point to the correct pairing scheme
>     implementation

For the whole series:

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

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

[Brian Norris]

Hi Boris,

I've mostly just reviewed the cover and first patch for now, since that
sets up the rest. A few questions and comments. I hope to review some
more and have more to say later this weekend.

On Mon, Apr 25, 2016 at 12:01:17PM +0200, Boris Brezillon 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.

Have you surveyed many types of NAND to get a representative sampling of
what kind of pairing schemes are out there? Do you think you've covered
the possibilities well enough in your API? I have a few comments on the
patches to this effect. I honestly don't know the answer to these
questions, because AFAIR, this is rarely well documented in datasheets.

> 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.

Do ONFI or JEDEC parameter pages even provide this kind of info? The
ONFI spec doesn't mention paired pages.

Brian

> Best Regards,
> 
> Boris
> 
> Boris Brezillon (4):
>   mtd: introduce the mtd_pairing_scheme concept
>   mtd: nand: implement two pairing scheme
>   mtd: nand: add a pairing field to nand_flash_dev
>   mtd: nand: H27UCG8T2ATR: point to the correct pairing scheme
>     implementation
> 
>  drivers/mtd/mtdcore.c        | 62 ++++++++++++++++++++++++++++
>  drivers/mtd/mtdpart.c        |  1 +
>  drivers/mtd/nand/nand_base.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/nand_ids.c  |  2 +-
>  include/linux/mtd/mtd.h      | 64 +++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h     |  5 +++
>  6 files changed, 230 insertions(+), 1 deletion(-)
> 
> -- 
> 2.7.4
> 

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

[Brian Norris]

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

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

[Boris Brezillon]

On Fri, 10 Jun 2016 19:16:25 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> I've mostly just reviewed the cover and first patch for now, since that
> sets up the rest. A few questions and comments. I hope to review some
> more and have more to say later this weekend.
> 
> On Mon, Apr 25, 2016 at 12:01:17PM +0200, Boris Brezillon 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.  
> 
> Have you surveyed many types of NAND to get a representative sampling of
> what kind of pairing schemes are out there? Do you think you've covered
> the possibilities well enough in your API? I have a few comments on the
> patches to this effect. I honestly don't know the answer to these
> questions, because AFAIR, this is rarely well documented in datasheets.

I only tested on 3 different NANDs from Micron, Toshiba and Hynix, but
I had a look at several datasheets. Unlike read-retry this part is
usually documented in public datasheets, and on a panel of approximately
20 NANDs (mainly from Toshiba, Samsung, Hynix and Micron), all of them
where using the 'distance 3' or 'distance 6' pairing scheme.
The only exception I've seen so far is the one pointed by Bean here [1],
and it can be described using the mtd_pairing_scheme approach.

> 
> > 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.  
> 
> Do ONFI or JEDEC parameter pages even provide this kind of info? The
> ONFI spec doesn't mention paired pages.

Nope that's the problem. The only way you can deduce that is to extract
it from other information, but I think my series reworking the NAND
initialization will help us [2].


[1]http://thread.gmane.org/gmane.linux.drivers.mtd/67084
[2]https://lkml.org/lkml/2016/5/27/264

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

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.




-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Brian Norris]

On Sat, Jun 11, 2016 at 08:45:18AM +0200, Boris Brezillon wrote:
> On Fri, 10 Jun 2016 19:16:25 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> > On Mon, Apr 25, 2016 at 12:01:17PM +0200, Boris Brezillon 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.  
> > 
> > Have you surveyed many types of NAND to get a representative sampling of
> > what kind of pairing schemes are out there? Do you think you've covered
> > the possibilities well enough in your API? I have a few comments on the
> > patches to this effect. I honestly don't know the answer to these
> > questions, because AFAIR, this is rarely well documented in datasheets.
> 
> I only tested on 3 different NANDs from Micron, Toshiba and Hynix, but

I'm curious, do you have an example part number for Micron? When I
looked briefly last week, I only found either MLC that don't mention it
at all (they fundamentally *have* to have write pairing, don't they?) or
TLC that required too much work for me to get past their login screens.

> I had a look at several datasheets. Unlike read-retry this part is
> usually documented in public datasheets, and on a panel of approximately
> 20 NANDs (mainly from Toshiba, Samsung, Hynix and Micron), all of them
> where using the 'distance 3' or 'distance 6' pairing scheme.
> The only exception I've seen so far is the one pointed by Bean here [1],
> and it can be described using the mtd_pairing_scheme approach.

Yeah, I suppose the API is rather generic. It doesn't really assume
anything about patterns/distances -- just that the pairings are formed
in groups of the same size.

> > > 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.  
> > 
> > Do ONFI or JEDEC parameter pages even provide this kind of info? The
> > ONFI spec doesn't mention paired pages.
> 
> Nope that's the problem. The only way you can deduce that is to extract
> it from other information, but I think my series reworking the NAND
> initialization will help us [2].

Sure, I suppose.

Brian

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

[Brian Norris]

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

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

[Brian Norris]

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

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

[Boris Brezillon]

On Sun, 12 Jun 2016 22:54:02 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Sat, Jun 11, 2016 at 08:45:18AM +0200, Boris Brezillon wrote:
> > On Fri, 10 Jun 2016 19:16:25 -0700
> > Brian Norris <computersforpeace@gmail.com> wrote:  
> > > On Mon, Apr 25, 2016 at 12:01:17PM +0200, Boris Brezillon 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.    
> > > 
> > > Have you surveyed many types of NAND to get a representative sampling of
> > > what kind of pairing schemes are out there? Do you think you've covered
> > > the possibilities well enough in your API? I have a few comments on the
> > > patches to this effect. I honestly don't know the answer to these
> > > questions, because AFAIR, this is rarely well documented in datasheets.  
> > 
> > I only tested on 3 different NANDs from Micron, Toshiba and Hynix, but  
> 
> I'm curious, do you have an example part number for Micron? When I
> looked briefly last week, I only found either MLC that don't mention it
> at all (they fundamentally *have* to have write pairing, don't they?) or
> TLC that required too much work for me to get past their login screens.

I had to register and request the datasheet on their website (after
accepting the NDA). I looked at the L73A and L85A. Not sure about the
other Micron MLC NANDs, but maybe Beal can answer that one.
Oh, and BTW, paired pages are called shared pages in Hynix datasheets.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

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'.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com