diff mbox

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

Message ID 1377509808-29363-2-git-send-email-b32955@freescale.com
State New, archived
Headers show

Commit Message

Huang Shijie Aug. 26, 2013, 9:36 a.m. UTC 
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(-)

Comments

Vikram Narayanan Aug. 27, 2013, 5:10 p.m. UTC | #1 
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.
Huang Shijie Aug. 28, 2013, 2:23 a.m. UTC | #2 
于 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
Brian Norris Aug. 28, 2013, 2:42 a.m. UTC | #3 
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
Huang Shijie Aug. 28, 2013, 2:45 a.m. UTC | #4 
于 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
Brian Norris Aug. 28, 2013, 2:48 a.m. UTC | #5 
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
Huang Shijie Aug. 28, 2013, 2:53 a.m. UTC | #6 
于 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
Brian Norris Aug. 28, 2013, 3:02 a.m. UTC | #7 
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
Huang Shijie Aug. 28, 2013, 3:09 a.m. UTC | #8 
于 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
diff mbox

Patch

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 */