[PATCH v3 00/10] About the SLC/MLC

[PATCH v3 00/10] About the SLC/MLC

[Huang Shijie]

In current mtd code, the MTD_NANDFLASH is used to represent both the
SLC nand MLC(including the TLC). But we already have the MTD_MLCNANDFLASH
to stand for the MLC. What is worse is that the JFFS2 may run on the MLC
nand with current code. For the reason of READ/WRITE disturbance, the JFFS2
should runs on the SLC only,
       
This patch set tries to make clear what is the SLC/MLC by adding the macros,
adding helpers, adding the comments. ..

After this patch set, the gpmi can support the JFFS2 for some SLC NAND now 
(only when the left oob area is big enough).

v2 --> v3:
	[0] fix the wrong patch for jffs2.
	[1] add more comments to nand_get_bits_per_cell(). 
	[2] others.

v1 --> v2:
	[0] rename the @cellinfo to @bits_per_cell
	[1] abandon the patch: "mtd: add more information for the MTD_NANDFLASH case"
	[2] update the ABI for patch 8
	[3] misc

Huang Shijie (10):
  mtd: nand: rename the cellinfo to bits_per_cell
  mtd: set the cell information for ONFI nand
  mtd: print out the cell information for nand chip
  mtd: gpmi: rewrite the gpmi_ecc_write_oob() to support the jffs2
  mtd: add more comment for MTD_NANDFLASH/MTD_MLCNANDFLASH
  mtd: fix the wrong mtd->type for nand chip
  jffs2: do not support the MLC nand
  mtd: add MTD_MLCNANDFLASH case for mtd_type_show()
  mtd: add a helper to detect the nand type
  mtd: mtd-abi: add a helper to detect the nand type

 Documentation/ABI/testing/sysfs-class-mtd |    2 +-
 drivers/mtd/inftlcore.c                   |    2 +-
 drivers/mtd/mtdcore.c                     |    3 ++
 drivers/mtd/nand/denali.c                 |    2 +-
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c    |   31 ++++++++++++++------
 drivers/mtd/nand/nand_base.c              |   45 +++++++++++++++++++---------
 drivers/mtd/nftlcore.c                    |    2 +-
 drivers/mtd/ssfdc.c                       |    2 +-
 drivers/mtd/tests/nandbiterrs.c           |    2 +-
 drivers/mtd/tests/oobtest.c               |    2 +-
 drivers/mtd/tests/pagetest.c              |    2 +-
 drivers/mtd/tests/subpagetest.c           |    2 +-
 fs/jffs2/fs.c                             |    4 ++
 include/linux/mtd/mtd.h                   |    5 +++
 include/linux/mtd/nand.h                  |   14 ++++++++-
 include/uapi/mtd/mtd-abi.h                |    9 ++++-
 16 files changed, 92 insertions(+), 37 deletions(-)

[PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Huang Shijie]

The @cellinfo fields contains unused information, such as write caching,
internal chip numbering, etc. But we only use it to check the SLC or MLC.

This patch tries to make it more clear and simple, renames the @cellinfo
to @bits_per_cell.

In order to avoiding the bisect issue, this patch also does the following
changes:
  (0) add a macro NAND_CI_CELLTYPE_SHIFT to avoid the hardcode.

  (1) add a helper to check the SLC : nand_is_slc()

  (2) add a helper to parse out the cell type : nand_get_bits_per_cell()

  (3) parse out the cell type for legacy nand chips and extended-ID
      chips, and the full-id nand chips.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/denali.c    |    2 +-
 drivers/mtd/nand/nand_base.c |   31 +++++++++++++++++++++----------
 include/linux/mtd/nand.h     |   14 ++++++++++++--
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 2ed2bb3..645721e 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1520,7 +1520,7 @@ int denali_init(struct denali_nand_info *denali)
 	 * so just let controller do 15bit ECC for MLC and 8bit ECC for
 	 * SLC if possible.
 	 * */
-	if (denali->nand.cellinfo & NAND_CI_CELLTYPE_MSK &&
+	if (!nand_is_slc(&denali->nand) &&
 			(denali->mtd.oobsize > (denali->bbtskipbytes +
 			ECC_15BITS * (denali->mtd.writesize /
 			ECC_SECTOR_SIZE)))) {
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7ed4841..7e29ea5 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3077,6 +3077,16 @@ static int nand_id_len(u8 *id_data, int arrlen)
 	return arrlen;
 }
 
+/* Extract the bits of per cell from the 3rd byte of the extended ID */
+static int nand_get_bits_per_cell(u8 cellinfo)
+{
+	int bits;
+
+	bits = cellinfo & NAND_CI_CELLTYPE_MSK;
+	bits >>= NAND_CI_CELLTYPE_SHIFT;
+	return bits + 1;
+}
+
 /*
  * Many new NAND share similar device ID codes, which represent the size of the
  * chip. The rest of the parameters must be decoded according to generic or
@@ -3087,7 +3097,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 {
 	int extid, id_len;
 	/* The 3rd id byte holds MLC / multichip data */
-	chip->cellinfo = id_data[2];
+	chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
 	/* The 4th id byte is the important one */
 	extid = id_data[3];
 
@@ -3103,8 +3113,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 	 * ID to decide what to do.
 	 */
 	if (id_len == 6 && id_data[0] == NAND_MFR_SAMSUNG &&
-			(chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
-			id_data[5] != 0x00) {
+			!nand_is_slc(chip) && id_data[5] != 0x00) {
 		/* Calc pagesize */
 		mtd->writesize = 2048 << (extid & 0x03);
 		extid >>= 2;
@@ -3136,7 +3145,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 			(((extid >> 1) & 0x04) | (extid & 0x03));
 		*busw = 0;
 	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
-			(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
+			!nand_is_slc(chip)) {
 		unsigned int tmp;
 
 		/* Calc pagesize */
@@ -3199,7 +3208,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 		 * - ID byte 5, bit[7]:    1 -> BENAND, 0 -> raw SLC
 		 */
 		if (id_len >= 6 && id_data[0] == NAND_MFR_TOSHIBA &&
-				!(chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
+				nand_is_slc(chip) &&
 				(id_data[5] & 0x7) == 0x6 /* 24nm */ &&
 				!(id_data[4] & 0x80) /* !BENAND */) {
 			mtd->oobsize = 32 * mtd->writesize >> 9;
@@ -3224,6 +3233,9 @@ static void nand_decode_id(struct mtd_info *mtd, struct nand_chip *chip,
 	mtd->oobsize = mtd->writesize / 32;
 	*busw = type->options & NAND_BUSWIDTH_16;
 
+	/* All legacy ID NAND are small-page, SLC */
+	chip->bits_per_cell = 1;
+
 	/*
 	 * Check for Spansion/AMD ID + repeating 5th, 6th byte since
 	 * some Spansion chips have erasesize that conflicts with size
@@ -3260,11 +3272,11 @@ static void nand_decode_bbm_options(struct mtd_info *mtd,
 	 * Micron devices with 2KiB pages and on SLC Samsung, Hynix, Toshiba,
 	 * AMD/Spansion, and Macronix.  All others scan only the first page.
 	 */
-	if ((chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
+	if (!nand_is_slc(chip) &&
 			(maf_id == NAND_MFR_SAMSUNG ||
 			 maf_id == NAND_MFR_HYNIX))
 		chip->bbt_options |= NAND_BBT_SCANLASTPAGE;
-	else if ((!(chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
+	else if ((nand_is_slc(chip) &&
 				(maf_id == NAND_MFR_SAMSUNG ||
 				 maf_id == NAND_MFR_HYNIX ||
 				 maf_id == NAND_MFR_TOSHIBA ||
@@ -3288,7 +3300,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
 		mtd->erasesize = type->erasesize;
 		mtd->oobsize = type->oobsize;
 
-		chip->cellinfo = id_data[2];
+		chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
 		chip->chipsize = (uint64_t)type->chipsize << 20;
 		chip->options |= type->options;
 		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
@@ -3740,8 +3752,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	chip->ecc.total = chip->ecc.steps * chip->ecc.bytes;
 
 	/* Allow subpage writes up to ecc.steps. Not possible for MLC flash */
-	if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
-	    !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
+	if (!(chip->options & NAND_NO_SUBPAGE_WRITE) && nand_is_slc(chip)) {
 		switch (chip->ecc.steps) {
 		case 2:
 			mtd->subpage_sft = 1;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index ac8e89d..6e9106b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -198,6 +198,7 @@ typedef enum {
 /* Cell info constants */
 #define NAND_CI_CHIPNR_MSK	0x03
 #define NAND_CI_CELLTYPE_MSK	0x0C
+#define NAND_CI_CELLTYPE_SHIFT	2
 
 /* Keep gcc happy */
 struct nand_chip;
@@ -477,7 +478,7 @@ struct nand_buffers {
  * @badblockbits:	[INTERN] minimum number of set bits in a good block's
  *			bad block marker position; i.e., BBM == 11110111b is
  *			not bad when badblockbits == 7
- * @cellinfo:		[INTERN] MLC/multichip data from chip ident
+ * @bits_per_cell:	[INTERN] the bits of per cell. i.e., 1 means SLC.
  * @ecc_strength_ds:	[INTERN] ECC correctability from the datasheet.
  *			Minimum amount of bit errors per @ecc_step_ds guaranteed
  *			to be correctable. If unknown, set to zero.
@@ -559,7 +560,7 @@ struct nand_chip {
 	int pagebuf;
 	unsigned int pagebuf_bitflips;
 	int subpagesize;
-	uint8_t cellinfo;
+	uint8_t bits_per_cell;
 	uint16_t ecc_strength_ds;
 	uint16_t ecc_step_ds;
 	int badblockpos;
@@ -797,4 +798,13 @@ static inline int onfi_get_sync_timing_mode(struct nand_chip *chip)
 	return le16_to_cpu(chip->onfi_params.src_sync_timing_mode);
 }
 
+/*
+ * Check if it is a SLC nand.
+ * The !nand_is_slc() can be used to check the MLC/TLC nand chips.
+ * We do not distinguish the MLC and TLC now.
+ */
+static inline bool nand_is_slc(struct nand_chip *chip)
+{
+	return chip->bits_per_cell == 1;
+}
 #endif /* __LINUX_MTD_NAND_H */
-- 
1.7.1

[PATCH v3 02/10] mtd: set the cell information for ONFI nand

[Huang Shijie]

The current code does not set the SLC/MLC information for onfi nand.
(This makes that the kernel treats all the onfi nand as SLC nand.)

This patch fills the cell type information for ONFI nands.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7e29ea5..0177ea5 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2988,6 +2988,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
 	chip->chipsize = le32_to_cpu(p->blocks_per_lun);
 	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
+	chip->bits_per_cell = p->bits_per_cell;
 
 	if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
 		*busw = NAND_BUSWIDTH_16;
-- 
1.7.1

[PATCH v3 03/10] mtd: print out the cell information for nand chip

[Huang Shijie]

Print out the cell information for nand chip.

(Since the message is too long, this patch also splits the log
with two separate pr_info())

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 0177ea5..8755a74 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3456,11 +3456,14 @@ ident_done:
 	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
 		chip->cmdfunc = nand_command_lp;
 
-	pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
-		" %dMiB, page size: %d, OOB size: %d\n",
+	pr_info("NAND device: Manufacturer ID: 0x%02x,"
+			"Chip ID: 0x%02x (%s %s) \n",
 		*maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
-		chip->onfi_version ? chip->onfi_params.model : type->name,
-		(int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
+		chip->onfi_version ? chip->onfi_params.model : type->name);
+
+	pr_info("NAND device: %dMiB, %s, page size: %d, OOB size: %d\n",
+		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
+		mtd->writesize, mtd->oobsize);
 
 	return type;
 }
-- 
1.7.1

[PATCH v3 04/10] mtd: gpmi: rewrite the gpmi_ecc_write_oob() to support the jffs2

[Huang Shijie]

When we use the ECC info which is get from the nand chip's datasheet,
we may have some freed oob area now.

This patch rewrites the gpmi_ecc_write_oob() to implement the ecc.write_oob().
We also update the comment for gpmi_hw_ecclayout.

Yes! We can support the JFFS2 for the SLC nand now.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 9c89e80..8d8a814 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -45,7 +45,10 @@ static struct nand_bbt_descr gpmi_bbt_descr = {
 	.pattern	= scan_ff_pattern
 };
 
-/*  We will use all the (page + OOB). */
+/*
+ * We may change the layout if we can get the ECC info from the datasheet,
+ * else we will use all the (page + OOB).
+ */
 static struct nand_ecclayout gpmi_hw_ecclayout = {
 	.eccbytes = 0,
 	.eccpos = { 0, },
@@ -1263,14 +1266,24 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 static int
 gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
 {
-	/*
-	 * The BCH will use all the (page + oob).
-	 * Our gpmi_hw_ecclayout can only prohibit the JFFS2 to write the oob.
-	 * But it can not stop some ioctls such MEMWRITEOOB which uses
-	 * MTD_OPS_PLACE_OOB. So We have to implement this function to prohibit
-	 * these ioctls too.
-	 */
-	return -EPERM;
+	struct nand_oobfree *of = mtd->ecclayout->oobfree;
+	int status = 0;
+
+	/* Do we have available oob area? */
+	if (!of->length)
+		return -EPERM;
+
+	if (!nand_is_slc(chip))
+		return -EPERM;
+
+	pr_debug("page number is %d\n", page);
+
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize + of->offset, page);
+	chip->write_buf(mtd, chip->oob_poi + of->offset, of->length);
+	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+	status = chip->waitfunc(mtd, chip);
+	return status & NAND_STATUS_FAIL ? -EIO : 0;
 }
 
 static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
-- 
1.7.1

[PATCH v3 05/10] mtd: add more comment for MTD_NANDFLASH/MTD_MLCNANDFLASH

[Huang Shijie]

In current code, the MTD_NANDFLASH is used to represent both the SLC and
MLC. It is confusing to us.

By adding an explicit comment about these two macros, this patch makes it
clear that:
	MTD_NANDFLASH    : stands for SLC nand,
	MTD_MLCNANDFLASH : stands for MLC nand(including TLC).

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/uapi/mtd/mtd-abi.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index 36eace0..16a9406 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -94,10 +94,10 @@ struct mtd_write_req {
 #define MTD_RAM			1
 #define MTD_ROM			2
 #define MTD_NORFLASH		3
-#define MTD_NANDFLASH		4
+#define MTD_NANDFLASH		4	/* stand for SLC nand */
 #define MTD_DATAFLASH		6
 #define MTD_UBIVOLUME		7
-#define MTD_MLCNANDFLASH	8
+#define MTD_MLCNANDFLASH	8	/* stand for MLC nand(including TLC) */
 
 #define MTD_WRITEABLE		0x400	/* Device is writeable */
 #define MTD_BIT_WRITEABLE	0x800	/* Single bits can be flipped */
-- 
1.7.1

[PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

Current code sets the mtd->type with MTD_NANDFLASH for both
SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
the jffs2 should not support the MLC.

This patch uses the nand_is_slc() to check the nand cell type,
and set the mtd->type with the right nand type.

After this patch, the jffs2 only can support the SLC nand.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8755a74..5957fe7 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3781,7 +3781,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		chip->options |= NAND_SUBPAGE_READ;
 
 	/* Fill in remaining MTD driver data */
-	mtd->type = MTD_NANDFLASH;
+	mtd->type = nand_is_slc(chip) ? MTD_NANDFLASH : MTD_MLCNANDFLASH;
 	mtd->flags = (chip->options & NAND_ROM) ? MTD_CAP_ROM :
 						MTD_CAP_NANDFLASH;
 	mtd->_erase = nand_erase;
-- 
1.7.1

[PATCH v3 07/10] jffs2: do not support the MLC nand

[Huang Shijie]

We should not support the MLC nand for jffs2. So if the nand type is
MLC, we quit immediatly.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 fs/jffs2/fs.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index fe3c052..09b3ed4 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -515,6 +515,10 @@ int jffs2_do_fill_super(struct super_block *sb, void *data, int silent)
 
 	c = JFFS2_SB_INFO(sb);
 
+	/* Do not support the MLC nand */
+	if (c->mtd->type == MTD_MLCNANDFLASH)
+		return -EINVAL;
+
 #ifndef CONFIG_JFFS2_FS_WRITEBUFFER
 	if (c->mtd->type == MTD_NANDFLASH) {
 		pr_err("Cannot operate on NAND flash unless jffs2 NAND support is compiled in\n");
-- 
1.7.1

[PATCH v3 08/10] mtd: add MTD_MLCNANDFLASH case for mtd_type_show()

[Huang Shijie]

The current mtd_type_show() misses the MTD_MLCNANDFLASH case.
This patch adds the case for it, and also updates the ABI.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 Documentation/ABI/testing/sysfs-class-mtd |    2 +-
 drivers/mtd/mtdcore.c                     |    3 +++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index bfd119a..1399bb2 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -104,7 +104,7 @@ Description:
 		One of the following ASCII strings, representing the device
 		type:
 
-		absent, ram, rom, nor, nand, dataflash, ubi, unknown
+		absent, ram, rom, nor, nand, mlc-nand, dataflash, ubi, unknown
 
 What:		/sys/class/mtd/mtdX/writesize
 Date:		April 2009
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5e14d54..92311a5 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -157,6 +157,9 @@ static ssize_t mtd_type_show(struct device *dev,
 	case MTD_UBIVOLUME:
 		type = "ubi";
 		break;
+	case MTD_MLCNANDFLASH:
+		type = "mlc-nand";
+		break;
 	default:
 		type = "unknown";
 	}
-- 
1.7.1

[PATCH v3 09/10] mtd: add a helper to detect the nand type

[Huang Shijie]

This helper detects that whether the mtd's type is nand type.

Now, it's clear that the MTD_NANDFLASH stands for SLC nand only.
So use the mtd_type_is_nand() to replace the old check method
to do the nand type (include the SLC and MLC) check.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/inftlcore.c         |    2 +-
 drivers/mtd/nftlcore.c          |    2 +-
 drivers/mtd/ssfdc.c             |    2 +-
 drivers/mtd/tests/nandbiterrs.c |    2 +-
 drivers/mtd/tests/oobtest.c     |    2 +-
 drivers/mtd/tests/pagetest.c    |    2 +-
 drivers/mtd/tests/subpagetest.c |    2 +-
 include/linux/mtd/mtd.h         |    5 +++++
 8 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/inftlcore.c b/drivers/mtd/inftlcore.c
index 3af3514..b66b541 100644
--- a/drivers/mtd/inftlcore.c
+++ b/drivers/mtd/inftlcore.c
@@ -50,7 +50,7 @@ static void inftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	struct INFTLrecord *inftl;
 	unsigned long temp;
 
-	if (mtd->type != MTD_NANDFLASH || mtd->size > UINT_MAX)
+	if (!mtd_type_is_nand(mtd) || mtd->size > UINT_MAX)
 		return;
 	/* OK, this is moderately ugly.  But probably safe.  Alternatives? */
 	if (memcmp(mtd->name, "DiskOnChip", 10))
diff --git a/drivers/mtd/nftlcore.c b/drivers/mtd/nftlcore.c
index c5f4ebf..46f27de 100644
--- a/drivers/mtd/nftlcore.c
+++ b/drivers/mtd/nftlcore.c
@@ -50,7 +50,7 @@ static void nftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	struct NFTLrecord *nftl;
 	unsigned long temp;
 
-	if (mtd->type != MTD_NANDFLASH || mtd->size > UINT_MAX)
+	if (!mtd_type_is_nand(mtd) || mtd->size > UINT_MAX)
 		return;
 	/* OK, this is moderately ugly.  But probably safe.  Alternatives? */
 	if (memcmp(mtd->name, "DiskOnChip", 10))
diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
index ab2a52a..daf82ba 100644
--- a/drivers/mtd/ssfdc.c
+++ b/drivers/mtd/ssfdc.c
@@ -290,7 +290,7 @@ static void ssfdcr_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	int cis_sector;
 
 	/* Check for small page NAND flash */
-	if (mtd->type != MTD_NANDFLASH || mtd->oobsize != OOB_SIZE ||
+	if (!mtd_type_is_nand(mtd) || mtd->oobsize != OOB_SIZE ||
 	    mtd->size > UINT_MAX)
 		return;
 
diff --git a/drivers/mtd/tests/nandbiterrs.c b/drivers/mtd/tests/nandbiterrs.c
index 3cd3aab..6f97615 100644
--- a/drivers/mtd/tests/nandbiterrs.c
+++ b/drivers/mtd/tests/nandbiterrs.c
@@ -349,7 +349,7 @@ static int __init mtd_nandbiterrs_init(void)
 		goto exit_mtddev;
 	}
 
-	if (mtd->type != MTD_NANDFLASH) {
+	if (!mtd_type_is_nand(mtd)) {
 		pr_info("this test requires NAND flash\n");
 		err = -ENODEV;
 		goto exit_nand;
diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
index ff35c46..2e9e2d1 100644
--- a/drivers/mtd/tests/oobtest.c
+++ b/drivers/mtd/tests/oobtest.c
@@ -289,7 +289,7 @@ static int __init mtd_oobtest_init(void)
 		return err;
 	}
 
-	if (mtd->type != MTD_NANDFLASH) {
+	if (!mtd_type_is_nand(mtd)) {
 		pr_info("this test requires NAND flash\n");
 		goto out;
 	}
diff --git a/drivers/mtd/tests/pagetest.c b/drivers/mtd/tests/pagetest.c
index 44b96e9..ed2d3f6 100644
--- a/drivers/mtd/tests/pagetest.c
+++ b/drivers/mtd/tests/pagetest.c
@@ -353,7 +353,7 @@ static int __init mtd_pagetest_init(void)
 		return err;
 	}
 
-	if (mtd->type != MTD_NANDFLASH) {
+	if (!mtd_type_is_nand(mtd)) {
 		pr_info("this test requires NAND flash\n");
 		goto out;
 	}
diff --git a/drivers/mtd/tests/subpagetest.c b/drivers/mtd/tests/subpagetest.c
index e2c0adf..a876371 100644
--- a/drivers/mtd/tests/subpagetest.c
+++ b/drivers/mtd/tests/subpagetest.c
@@ -299,7 +299,7 @@ static int __init mtd_subpagetest_init(void)
 		return err;
 	}
 
-	if (mtd->type != MTD_NANDFLASH) {
+	if (!mtd_type_is_nand(mtd)) {
 		pr_info("this test requires NAND flash\n");
 		goto out;
 	}
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index f9bfe52..88409b8 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -354,6 +354,11 @@ static inline int mtd_has_oob(const struct mtd_info *mtd)
 	return mtd->_read_oob && mtd->_write_oob;
 }
 
+static inline int mtd_type_is_nand(const struct mtd_info *mtd)
+{
+	return mtd->type == MTD_NANDFLASH || mtd->type == MTD_MLCNANDFLASH;
+}
+
 static inline int mtd_can_have_bb(const struct mtd_info *mtd)
 {
 	return !!mtd->_block_isbad;
-- 
1.7.1

[PATCH v3 10/10] mtd: mtd-abi: add a helper to detect the nand type

[Huang Shijie]

The helper is for user applications, and it is just a copy of
the kernel helper: mtd_type_is_nand();

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/uapi/mtd/mtd-abi.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index 16a9406..66c2b0c 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -275,4 +275,9 @@ enum mtd_file_modes {
 	MTD_FILE_MODE_RAW,
 };
 
+static inline int mtd_type_is_nand_user(const struct mtd_info_user *mtd)
+{
+	return mtd->type == MTD_NANDFLASH || mtd->type == MTD_MLCNANDFLASH;
+}
+
 #endif /* __MTD_ABI_H__ */
-- 
1.7.1

Re: [PATCH v3 00/10] About the SLC/MLC

[Thomas Petazzoni]

Dear Huang Shijie,

On Mon, 26 Aug 2013 17:36:38 +0800, Huang Shijie wrote:
> In current mtd code, the MTD_NANDFLASH is used to represent both the
> SLC nand MLC(including the TLC). But we already have the MTD_MLCNANDFLASH
> to stand for the MLC. What is worse is that the JFFS2 may run on the MLC
> nand with current code. For the reason of READ/WRITE disturbance, the JFFS2
> should runs on the SLC only,

Pardon the probably very silly question, but would you mind giving more
details about why JFFS2 is not appropriate on MLC flashes? Is there
anything that UBI/UBIFS does that JFFS2 isn't doing to take into
account read/write disturbance?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v3 00/10] About the SLC/MLC

[Huang Shijie]

On Mon, Aug 26, 2013 at 02:41:53PM +0200, Thomas Petazzoni wrote:
> Dear Huang Shijie,
> 
> On Mon, 26 Aug 2013 17:36:38 +0800, Huang Shijie wrote:
> > In current mtd code, the MTD_NANDFLASH is used to represent both the
> > SLC nand MLC(including the TLC). But we already have the MTD_MLCNANDFLASH
> > to stand for the MLC. What is worse is that the JFFS2 may run on the MLC
> > nand with current code. For the reason of READ/WRITE disturbance, the JFFS2
> > should runs on the SLC only,
> 
> Pardon the probably very silly question, but would you mind giving more
> details about why JFFS2 is not appropriate on MLC flashes? Is there
> anything that UBI/UBIFS does that JFFS2 isn't doing to take into
> account read/write disturbance?
I try to explain it.

For the UBIFS, it will writes the page(including the oob) only one time;
but for jffs2, it may writes the page(including the oob) twice, one for the
marker, one for the real data. 

For the MLC, the write disturbance will cause some bitflips in this
page. Since you write a page twice, so in the jffs2, you will meet uncorrectable
ECC error.

thanks
Huang Shijie

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Vikram Narayanan]

On 26/Aug/2013 3:06 PM, Huang Shijie wrote:
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 2ed2bb3..645721e 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1520,7 +1520,7 @@ int denali_init(struct denali_nand_info *denali)
>   	 * so just let controller do 15bit ECC for MLC and 8bit ECC for
>   	 * SLC if possible.
>   	 * */
> -	if (denali->nand.cellinfo & NAND_CI_CELLTYPE_MSK &&
> +	if (!nand_is_slc(&denali->nand) &&
>   			(denali->mtd.oobsize > (denali->bbtskipbytes +
>   			ECC_15BITS * (denali->mtd.writesize /
>   			ECC_SECTOR_SIZE)))) {

Was just skimming thro this patchset.
Isn't the above change logically conflicting with what this patch is 
supposed to address? Please move this to a different patch.

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Huang Shijie]

于 2013年08月28日 01:10, Vikram Narayanan 写道:
> Isn't the above change logically conflicting with what this patch is 
> supposed to address? Please move this to a different patch. 
OK, :)

thanks
Huang Shijie

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Brian Norris]

On 08/27/2013 10:10 AM, Vikram Narayanan wrote:
> On 26/Aug/2013 3:06 PM, Huang Shijie wrote:
>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> index 2ed2bb3..645721e 100644
>> --- a/drivers/mtd/nand/denali.c
>> +++ b/drivers/mtd/nand/denali.c
>> @@ -1520,7 +1520,7 @@ int denali_init(struct denali_nand_info *denali)
>>        * so just let controller do 15bit ECC for MLC and 8bit ECC for
>>        * SLC if possible.
>>        * */
>> -    if (denali->nand.cellinfo & NAND_CI_CELLTYPE_MSK &&
>> +    if (!nand_is_slc(&denali->nand) &&
>>               (denali->mtd.oobsize > (denali->bbtskipbytes +
>>               ECC_15BITS * (denali->mtd.writesize /
>>               ECC_SECTOR_SIZE)))) {
>
> Was just skimming thro this patchset.
> Isn't the above change logically conflicting with what this patch is
> supposed to address? Please move this to a different patch.

To move it to a different patch by itself means he has to change this 
line twice: once to
   denali->nand.bits_per_cell != 1
and once to
   !nand_is_slc(&denali->nand)
which doesn't really help at all.

Or, I guess it's helpful to first add the nand_is_slc() helper in one 
patch (using the existing 'cellinfo' field), then introduce the simple 
patch to change 'cellinfo' to 'bits_per_cell'. Maybe that's what you 
were recommending?

Brian

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Huang Shijie]

于 2013年08月28日 10:42, Brian Norris 写道:
> Or, I guess it's helpful to first add the nand_is_slc() helper in one 
> patch (using the existing 'cellinfo' field), then introduce the simple 
> patch to change 'cellinfo' to 'bits_per_cell'. Maybe that's what you 
> were recommending? 
I ever thought of this method, but it will add more patches.

thanks
Huang Shijie

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Brian Norris]

On 08/27/2013 07:45 PM, Huang Shijie wrote:
> 于 2013年08月28日 10:42, Brian Norris 写道:
>> Or, I guess it's helpful to first add the nand_is_slc() helper in one
>> patch (using the existing 'cellinfo' field), then introduce the simple
>> patch to change 'cellinfo' to 'bits_per_cell'. Maybe that's what you
>> were recommending?
> I ever thought of this method, but it will add more patches.

Use your best judgment. We tread a fine line between time 
wasting/bikeshedding and readability/understandability here. But 
splitting this to 2 patches is probably helpful and reasonable.

Brian

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Huang Shijie]

于 2013年08月28日 10:48, Brian Norris 写道:
> But splitting this to 2 patches is probably helpful and reasonable. 
Split to 2 patches may cause the Bisect issue.

Maybe i can add more comment for this patch.

thanks
Huang Shijie

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Brian Norris]

On 08/27/2013 07:53 PM, Huang Shijie wrote:
> 于 2013年08月28日 10:48, Brian Norris 写道:
>> But splitting this to 2 patches is probably helpful and reasonable.
> Split to 2 patches may cause the Bisect issue.

Not if you "first add the nand_is_slc() helper in one patch (using the 
existing 'cellinfo' field), then introduce the simple patch to change 
'cellinfo' to 'bits_per_cell'"

You can introduce the helper as

+static inline bool nand_is_slc(struct nand_chip *chip)
+{
+	return !(chip->cellinfo & NAND_CI_CELLTYPE_MSK);
+}

Brian

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Brian Norris]

Since we may see a v4 anyway, I'll make a comment here that I've been 
mulling over.

On 08/26/2013 02:36 AM, Huang Shijie wrote:
> Current code sets the mtd->type with MTD_NANDFLASH for both
> SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
> the jffs2 should not support the MLC.
>
> This patch uses the nand_is_slc() to check the nand cell type,
> and set the mtd->type with the right nand type.
>
> After this patch, the jffs2 only can support the SLC nand.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>

This patch doesn't note here that it is breaking the ABI: it is the 
first (AFAIK; I don't think onenand ever actually had MLC, right?) patch 
to cause an MTD to show up as MTD_MLCNANDFLASH (and "unknown" in sysfs 
-- but you change this later to "mlc-nand"). This should be made more 
clear, if we're going to do this. And you should document and explain it 
*before* this patch.

But more importantly, you don't really answer this question I have: why 
we want to expose this MTD_MLCNANDFLASH type to user-space? It seems you 
need this for internal drivers' usage and for JFFS2 (both valid 
reasons). But exporting it to user-space just makes us work to change 
mtd-utils (and any other scripts that might rely on these types). Note 
that not everybody upgrades mtd-utils along with their kernel.

One argument in favor of your change: so a user can programmatically 
determine whether to use JFFS2 or UBIFS on a particular NAND. (But then 
again, SLC are getting more and more MLC-like properties that make them 
unsuitable for JFFS2...)

So, I would recommend a few rearrangements of part of this series:
(1) fixup the sysfs show() function and ABI documentation (in the same 
patch) to cover "mlc-nand". *** must clearly explain the rationale for 
userspace change (currently missing) ***
(2) do any changes to JFFS2 based on the MLC type
(3) allow nand_base.c to export MTD_MLCNANDFLASH

> ---
>   drivers/mtd/nand/nand_base.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 8755a74..5957fe7 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3781,7 +3781,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>   		chip->options |= NAND_SUBPAGE_READ;
>
>   	/* Fill in remaining MTD driver data */
> -	mtd->type = MTD_NANDFLASH;
> +	mtd->type = nand_is_slc(chip) ? MTD_NANDFLASH : MTD_MLCNANDFLASH;
>   	mtd->flags = (chip->options & NAND_ROM) ? MTD_CAP_ROM :
>   						MTD_CAP_NANDFLASH;
>   	mtd->_erase = nand_erase;
>

Brian

Re: [PATCH v3 01/10] mtd: nand: rename the cellinfo to bits_per_cell

[Huang Shijie]

于 2013年08月28日 11:02, Brian Norris 写道:
> Not if you "first add the nand_is_slc() helper in one patch (using the 
> existing 'cellinfo' field), then introduce the simple patch to change 
> 'cellinfo' to 'bits_per_cell'"
ok. I will re-arange this patch set.

thanks
Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

于 2013年08月28日 11:08, Brian Norris 写道:
> Since we may see a v4 anyway, I'll make a comment here that I've been 
> mulling over.
>
> On 08/26/2013 02:36 AM, Huang Shijie wrote:
>> Current code sets the mtd->type with MTD_NANDFLASH for both
>> SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
>> the jffs2 should not support the MLC.
>>
>> This patch uses the nand_is_slc() to check the nand cell type,
>> and set the mtd->type with the right nand type.
>>
>> After this patch, the jffs2 only can support the SLC nand.
>>
>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>
> This patch doesn't note here that it is breaking the ABI: it is the 
> first (AFAIK; I don't think onenand ever actually had MLC, right?) 
> patch to cause an MTD to show up as MTD_MLCNANDFLASH (and "unknown" in 
> sysfs -- but you change this later to "mlc-nand"). This should be made 
> more clear, if we're going to do this. And you should document and 
> explain it *before* this patch.
okay.
>
> But more importantly, you don't really answer this question I have: 
> why we want to expose this MTD_MLCNANDFLASH type to user-space? It 
> seems you need this for internal drivers' usage and for JFFS2 (both 
> valid reasons). But exporting it to user-space just makes us work to 
> change mtd-utils (and any other scripts that might rely on these 
> types). Note that not everybody upgrades mtd-utils along with their 
> kernel.
>
After we change the code with this patch, the SLC still expose the 
'nand' to the user space, it's ok;
but the MLC will expose "unknown" as you said above. Some mtd-utils, 
such as mtd_debug will not work well any more.

If we do expose the MTD_MLCNANDFLASH to use space, the mtd-utils may can 
not work with MLC nand, some code only checks the
MTD_NANDFLASH. Please see the :
http://lists.infradead.org/pipermail/linux-mtd/2013-August/048213.html


That's why we should expose the MTD_MLCNANDFLASH to the user-space

> One argument in favor of your change: so a user can programmatically 
> determine whether to use JFFS2 or UBIFS on a particular NAND. (But 
> then again, SLC are getting more and more MLC-like properties that 
> make them unsuitable for JFFS2...)
>
> So, I would recommend a few rearrangements of part of this series:
> (1) fixup the sysfs show() function and ABI documentation (in the same 
> patch) to cover "mlc-nand". *** must clearly explain the rationale for 
> userspace change (currently missing) ***
> (2) do any changes to JFFS2 based on the MLC type
> (3) allow nand_base.c to export MTD_MLCNANDFLASH
I will arrange the next version in this order.

thanks
Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Brian Norris]

On 08/27/2013 11:59 PM, Huang Shijie wrote:
> 于 2013年08月28日 11:08, Brian Norris 写道:
>> Since we may see a v4 anyway, I'll make a comment here that I've been
>> mulling over.
>>
>> On 08/26/2013 02:36 AM, Huang Shijie wrote:
>>> Current code sets the mtd->type with MTD_NANDFLASH for both
>>> SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
>>> the jffs2 should not support the MLC.
>>>
>>> This patch uses the nand_is_slc() to check the nand cell type,
>>> and set the mtd->type with the right nand type.
>>>
>>> After this patch, the jffs2 only can support the SLC nand.
>>>
>>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>>
>> This patch doesn't note here that it is breaking the ABI: it is the
>> first (AFAIK; I don't think onenand ever actually had MLC, right?)
>> patch to cause an MTD to show up as MTD_MLCNANDFLASH (and "unknown" in
>> sysfs -- but you change this later to "mlc-nand"). This should be made
>> more clear, if we're going to do this. And you should document and
>> explain it *before* this patch.
> okay.
>>
>> But more importantly, you don't really answer this question I have:
>> why we want to expose this MTD_MLCNANDFLASH type to user-space? It
>> seems you need this for internal drivers' usage and for JFFS2 (both
>> valid reasons). But exporting it to user-space just makes us work to
>> change mtd-utils (and any other scripts that might rely on these
>> types). Note that not everybody upgrades mtd-utils along with their
>> kernel.
>>
> After we change the code with this patch, the SLC still expose the
> 'nand' to the user space, it's ok;
> but the MLC will expose "unknown" as you said above. Some mtd-utils,
> such as mtd_debug will not work well any more.

Right. That's part of the ABI breakage.

> If we do expose the MTD_MLCNANDFLASH to use space, the mtd-utils may can
> not work with MLC nand, some code only checks the
> MTD_NANDFLASH. Please see the :
> http://lists.infradead.org/pipermail/linux-mtd/2013-August/048213.html

Yes, I saw that.

> That's why we should expose the MTD_MLCNANDFLASH to the user-space

All you've shown is the breakage, not the reason for exposing 
MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being in 
the mtd-abi.h header and actually using it in a driver (and documenting 
it, and giving it a new sysfs string).

There is an alternative: that we don't pass MTD_MLCNANDFLASH through to 
user-space (we intercept it and change it to MTD_NANDFLASH in 
ioctl(MEMGETINFO)), and it just appears as "nand" in the sysfs 'type' 
entry. I'm not saying we should do that--I think it's useful to know SLC 
vs. MLC in user-space--but I am saying that we need a proper 
justification. So far I'm the only one who has explained why user-space 
needs this...

>> One argument in favor of your change: so a user can programmatically
>> determine whether to use JFFS2 or UBIFS on a particular NAND. (But
>> then again, SLC are getting more and more MLC-like properties that
>> make them unsuitable for JFFS2...)
>>
>> So, I would recommend a few rearrangements of part of this series:
>> (1) fixup the sysfs show() function and ABI documentation (in the same
>> patch) to cover "mlc-nand". *** must clearly explain the rationale for
>> userspace change (currently missing) ***
>> (2) do any changes to JFFS2 based on the MLC type
>> (3) allow nand_base.c to export MTD_MLCNANDFLASH
> I will arrange the next version in this order.

Brian

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

于 2013年08月28日 15:28, Brian Norris 写道:
> All you've shown is the breakage, not the reason for exposing 
> MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being 
> in the mtd-abi.h header and actually using it in a driver (and 
> documenting it, and giving it a new sysfs string).
>


Just as you said belowing, the app may calls the MEMGETINFO get the nand 
type(SLC OR MLC).
So after this patch, the app will get the MTD_MLCNANDFLASH type for MLC 
which will is the breakage.

Are'nt the reason for exposing the MTD_MLCNANDFLASH? :(

> There is an alternative: that we don't pass MTD_MLCNANDFLASH through 
> to user-space (we intercept it and change it to MTD_NANDFLASH in 
> ioctl(MEMGETINFO)), and it just appears as "nand" in the sysfs 'type' 
> entry. I'm not saying we should do that--I think it's useful to know 
> SLC vs. MLC in user-space--but I am saying that we need a proper 
> justification. So far I'm the only one who has explained why 
> user-space needs this...
You concern is that exposing the MTD_MLCNANDFLASH may breaks mtd-utils 
or other app tools.

So you suggest to hack the IOCTL(MEMGETINFO) to avoid any changes of the 
apps?

yes, we can use this way to avoid changing the apps, but the side-effect 
is the mtd-utils can not get the
right SLC/MLC info.  If we does do the hack code to the ioctrl, I think 
some one will try to fix the hack code in the future.

thanks
Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

于 2013年08月28日 15:28, Brian Norris 写道:
> There is an alternative: that we don't pass MTD_MLCNANDFLASH through 
> to user-space (we intercept it and change it to MTD_NANDFLASH in 
> ioctl(MEMGETINFO)), and it just appears as "nand" in the sysfs 'type' 
> entry. I'm not saying we should do that--I think it's useful to know 
> SLC vs. MLC in user-space--but I am saying that we need a proper 
> justification. So far I'm the only one who has explained why 
> user-space needs this... 
There is another issue if we hack the ioctl(MEMGETINFO):
      The "flash_eraseall --jffs2" can writes the marker even it is a 
MLC nand, since the flash_eraseall
regards the MLC as the SLC.

thanks
Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

On Wed, Aug 28, 2013 at 12:28:51AM -0700, Brian Norris wrote:
> On 08/27/2013 11:59 PM, Huang Shijie wrote:
> 
> Yes, I saw that.
> 
> >That's why we should expose the MTD_MLCNANDFLASH to the user-space
> 
> All you've shown is the breakage, not the reason for exposing
> MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being
Do you mean this patch exposes the MTD_MLCNANDFLASH to user space?
or the patch 8 exposes the MTD_MLCNANDFLASH to user space?


thanks
Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Brian Norris]

On 08/28/2013 06:24 PM, Huang Shijie wrote:
> On Wed, Aug 28, 2013 at 12:28:51AM -0700, Brian Norris wrote:
>> On 08/27/2013 11:59 PM, Huang Shijie wrote:
>>
>> Yes, I saw that.
>>
>>> That's why we should expose the MTD_MLCNANDFLASH to the user-space
>>
>> All you've shown is the breakage, not the reason for exposing
>> MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being
> Do you mean this patch exposes the MTD_MLCNANDFLASH to user space?
> or the patch 8 exposes the MTD_MLCNANDFLASH to user space?

This patch (patch 6) changes the mtd->type for MLC NAND. And mtd->type 
is exposed to user-space via ioctl(MEMGETINFO), and (as of this patch), 
it is exposed as "unknown" in sysfs.

Patch 8 only changes the "unknown" to "mlc-nand"---an improvement, but 
patch 6 is still the first one that makes a visible change to the ABI.

Brian

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

于 2013年08月29日 13:05, Brian Norris 写道:
> This patch (patch 6) changes the mtd->type for MLC NAND. And mtd->type 
> is exposed to user-space via ioctl(MEMGETINFO), and (as of this 
> patch), it is exposed as "unknown" in sysfs.
thanks.

this patch changes the mtd->type for the jffs2, and the jffs2 can uses 
the mtd->type to do the sanity check.

If without this patch, how the jffs2 can distinguish a SLC or a MLC?

Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Huang Shijie]

于 2013年08月29日 13:34, Huang Shijie 写道:
> 于 2013年08月29日 13:05, Brian Norris 写道:
>> This patch (patch 6) changes the mtd->type for MLC NAND. And 
>> mtd->type is exposed to user-space via ioctl(MEMGETINFO), and (as of 
>> this patch), it is exposed as "unknown" in sysfs.
> thanks.
>
> this patch changes the mtd->type for the jffs2, and the jffs2 can uses 
> the mtd->type to do the sanity check.
>
> If without this patch, how the jffs2 can distinguish a SLC or a MLC?
hi Brian:

I find a method to distinguish the SLC/MLC for jffs2, we can add the 
code for jffs2:
   -------------------------------------------------------
      if (mtd->flags == MTD_CAP_NANDFLASH) {

              struch nand_chip *chip = c->mtd->priv;
              if (!nand_is_slc)
                 return -EINVAL;
      }


   ------------------------------------------------------

If you think this code is okay, we can abandon serveral patches.

thanks
Huang Shijie

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Brian Norris]

On Wed, Aug 28, 2013 at 10:50 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年08月29日 13:34, Huang Shijie 写道:
>> 于 2013年08月29日 13:05, Brian Norris 写道:
>>>
>>> This patch (patch 6) changes the mtd->type for MLC NAND. And mtd->type is
>>> exposed to user-space via ioctl(MEMGETINFO), and (as of this patch), it is
>>> exposed as "unknown" in sysfs.
>>
>> thanks.
>>
>> this patch changes the mtd->type for the jffs2, and the jffs2 can uses the
>> mtd->type to do the sanity check.

But the key point is that not only does this change mtd->type for
JFFS2, but it also exports this new mtd->type to user-space.

>> If without this patch, how the jffs2 can distinguish a SLC or a MLC?

You could precede this patch with one that prevents exporting
MTD_MLCNANDFLASH in mtd_type_show() and in ioctl(MEMGETINFO). I'm not
saying you *must* do this, but that this side effect must be
acknowledged and explained better, or else it seems that you don't
understand the ramifications of your proposed changes.

> hi Brian:
>
> I find a method to distinguish the SLC/MLC for jffs2, we can add the code
> for jffs2:
>   -------------------------------------------------------
>      if (mtd->flags == MTD_CAP_NANDFLASH) {
>
>              struch nand_chip *chip = c->mtd->priv;
>              if (!nand_is_slc)
>                 return -EINVAL;
>      }
>
>
>   ------------------------------------------------------
>
> If you think this code is okay, we can abandon serveral patches.

No, we don't want to leak the mtd_info and nand_chip abstractions that
far. I think the MTD_MLCNANDFLASH type is the right way to do this for
the in-kernel MTD API, but it is a bigger step to automatically leak
that to user-space.

Brian

Re: [PATCH v3 06/10] mtd: fix the wrong mtd->type for nand chip

[Brian Norris]

Hi Huang,

Sorry, I overlooked responding to this one.

On Thu, Sep 05, 2013 at 12:03:04PM +0800, Huang Shijie wrote:
> 于 2013年09月05日 02:51, Brian Norris 写道:
> 
>     You could precede this patch with one that prevents exporting
>     MTD_MLCNANDFLASH in mtd_type_show() and in ioctl(MEMGETINFO). I'm not
>     saying you *must* do this, but that this side effect must be
>     acknowledged and explained better, or else it seems that you don't
>     understand the ramifications of your proposed changes.
> 
> [1] If you think we should not _change_ any code of the application, such as
> the mtd-utils,

That's the true meaning of "we don't break ABI compatibility". mtd-utils
that work today on a given board with a given set of hardware (e.g., MLC
NAND flash) should not need modified to handle the same features on a
newer kernel.

However, I think it is safe to say that the MLC vs. SLC distinction
isn't made clear in mtd-abi.h (i.e., we already have MTD_MLCNANDFLASH),
and that this change is justified.

>     I can create a patch to prevents exporting the MTD_MLCNANDFLASH in
> mtd_type_show() and
>     in ioctrl(MEMGETINFO).

I don't think this is actually necessary. It is desirable that users can
see the difference between SLC and MLC, but this should be explained.

> [2] else, I prefer to explain better for this patch.

Yes, please explain it better (and reorder some patches, as I explained
previously) in order to

(1) show that you understand completely what your patches are doing and

(2) more clearly notify other developers of the modified ABI

Thanks,
Brian

Re: [PATCH v3 09/10] mtd: add a helper to detect the nand type

[Brian Norris]

On Mon, Aug 26, 2013 at 05:36:47PM +0800, Huang Shijie wrote:
> This helper detects that whether the mtd's type is nand type.
> 
> Now, it's clear that the MTD_NANDFLASH stands for SLC nand only.
> So use the mtd_type_is_nand() to replace the old check method
> to do the nand type (include the SLC and MLC) check.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

This patch also needs to be placed ahead of the patch that allows
nand_base.c to use MTD_MLCNANDFLASH. Otherwise there's a (temporary)
breakage between the time nand_base.c uses MLCNANDFLASH and the time
this patch is introduced.

Brian

Re: [PATCH v3 00/10] About the SLC/MLC

[Ezequiel Garcia]

Hi Huang,

On Mon, Aug 26, 2013 at 11:30:22PM -0400, Huang Shijie wrote:
> On Mon, Aug 26, 2013 at 02:41:53PM +0200, Thomas Petazzoni wrote:
> > 
> > On Mon, 26 Aug 2013 17:36:38 +0800, Huang Shijie wrote:
> > > In current mtd code, the MTD_NANDFLASH is used to represent both the
> > > SLC nand MLC(including the TLC). But we already have the MTD_MLCNANDFLASH
> > > to stand for the MLC. What is worse is that the JFFS2 may run on the MLC
> > > nand with current code. For the reason of READ/WRITE disturbance, the JFFS2
> > > should runs on the SLC only,
> > 
> > Pardon the probably very silly question, but would you mind giving more
> > details about why JFFS2 is not appropriate on MLC flashes? Is there
> > anything that UBI/UBIFS does that JFFS2 isn't doing to take into
> > account read/write disturbance?
> I try to explain it.
> 
> For the UBIFS, it will writes the page(including the oob) only one time;
> but for jffs2, it may writes the page(including the oob) twice, one for the
> marker, one for the real data. 
> 

It seems I overlooked this when I first read it.

Do you mean that JFFS2 will write twice: one write to the spare region,
one write in the data region, with no erase between those two?

I've looked at the JFFS2 code, but it's not trivial to follow :)
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v3 00/10] About the SLC/MLC

[Huang Shijie]

于 2013年10月19日 21:32, Ezequiel Garcia 写道:
> Do you mean that JFFS2 will write twice: one write to the spare region,
> one write in the data region, with no erase between those two?
yes. The first time to write the clean marker, the another is to write 
the data.

thanks
Huang Shijie

RE: [PATCH v3 00/10] About the SLC/MLC

[Gupta, Pekon]

> From:  Huang Shijie
> On Mon, Aug 26, 2013 at 02:41:53PM +0200, Thomas Petazzoni wrote:
> > Dear Huang Shijie,
> >
> > On Mon, 26 Aug 2013 17:36:38 +0800, Huang Shijie wrote:
> > > In current mtd code, the MTD_NANDFLASH is used to represent both the
> > > SLC nand MLC(including the TLC). But we already have the
> MTD_MLCNANDFLASH
> > > to stand for the MLC. What is worse is that the JFFS2 may run on the MLC
> > > nand with current code. For the reason of READ/WRITE disturbance, the
> JFFS2
> > > should runs on the SLC only,
> >
> > Pardon the probably very silly question, but would you mind giving more
> > details about why JFFS2 is not appropriate on MLC flashes? Is there
> > anything that UBI/UBIFS does that JFFS2 isn't doing to take into
> > account read/write disturbance?
> I try to explain it.
> 
> For the UBIFS, it will writes the page(including the oob) only one time;
> but for jffs2, it may writes the page(including the oob) twice, one for the
> marker, one for the real data.
> 
> For the MLC, the write disturbance will cause some bitflips in this
> page. Since you write a page twice, so in the jffs2, you will meet
> uncorrectable
> ECC error.
> 
But, doesn't JFFS2 checks for the blank page before second-write of data ?
UBI does it here..
File: $KERNEL/drivers/mtd/ubi/io.c
@@ubi_io_write (... )
[...]
	err = ubi_self_check_all_ff(ubi, pnum, offset, len);

So, isn't it better to add this check in JFFS2 code so that its more
bit-flips tolerant ?
This way bit-flips caused by first write to OOB (cleanmarker) would be
caught before using that page for data write. And so MLC should also be
good for JFFS2..

with regards, pekon

Re: [PATCH v3 00/10] About the SLC/MLC

[Brian Norris]

Late response, but better late than never?

On Tue, Oct 22, 2013 at 05:11:37AM +0000, Pekon Gupta wrote:
> > From:  Huang Shijie
> > On Mon, Aug 26, 2013 at 02:41:53PM +0200, Thomas Petazzoni wrote:
> > > Dear Huang Shijie,
> > >
> > > On Mon, 26 Aug 2013 17:36:38 +0800, Huang Shijie wrote:
> > > > In current mtd code, the MTD_NANDFLASH is used to represent both the
> > > > SLC nand MLC(including the TLC). But we already have the
> > MTD_MLCNANDFLASH
> > > > to stand for the MLC. What is worse is that the JFFS2 may run on the MLC
> > > > nand with current code. For the reason of READ/WRITE disturbance, the
> > JFFS2
> > > > should runs on the SLC only,
> > >
> > > Pardon the probably very silly question, but would you mind giving more
> > > details about why JFFS2 is not appropriate on MLC flashes? Is there
> > > anything that UBI/UBIFS does that JFFS2 isn't doing to take into
> > > account read/write disturbance?
> > I try to explain it.
> > 
> > For the UBIFS, it will writes the page(including the oob) only one time;
> > but for jffs2, it may writes the page(including the oob) twice, one for the
> > marker, one for the real data.
> > 
> > For the MLC, the write disturbance will cause some bitflips in this
> > page. Since you write a page twice, so in the jffs2, you will meet
> > uncorrectable
> > ECC error.
> > 
> But, doesn't JFFS2 checks for the blank page before second-write of data ?
> UBI does it here..
> File: $KERNEL/drivers/mtd/ubi/io.c
> @@ubi_io_write (... )
> [...]
> 	err = ubi_self_check_all_ff(ubi, pnum, offset, len);
> 
> So, isn't it better to add this check in JFFS2 code so that its more
> bit-flips tolerant ?
> This way bit-flips caused by first write to OOB (cleanmarker) would be
> caught before using that page for data write. And so MLC should also be
> good for JFFS2..

Sorry, no. MLC (and some SLC these days, actually) has a fundamental
limitation that it can only be programmed once per erase-cycle. This
measure is abbreviated as "NOP" (Number Of Partial-programs?), and for
MLC, NOP=1.

So trying to support the JFFS2 model of double-programming a page is
simply counter-productive when NOP=1.

Brian