[PATCH v2 0/5] mtd: nand: support the JEDEC compliant nand.

[PATCH v2 0/5] mtd: nand: support the JEDEC compliant nand.

[Huang Shijie]

Some vendors support the JEDEC standard only, such as Toshiba.
We can get all the information for the JEDEC parameter page, just like
we did with the ONFI parameter page.

This patch set adds the support for the JEDEC compliant nand chips.

You can download the JEDEC standard about the NAND in the:
    www.jedec.org

My code references to the JESD230A, August 2013, revision 1.

Tested with Toshiba TH58TEG7DDKTA20(16K + 1280).
(Unfortuately, this ECC info of its JEDEC parameter page is zero,
 TOSHIBA FAE confirmed that they will fix it in future.)

v1 --> v2:
	[1] use the read_byte to replace the read_buf.

Huang Shijie (5):
  mtd: nand: add the data structures for JEDEC parameter page
  mtd: nand: add fields for JEDEC in nand_chip
  mtd: nand: add a helper to get the supported features for JEDEC
  mtd: nand: parse out the JEDEC compliant nand
  mtd: nand: print out the right information for JEDEC compliant nand

 drivers/mtd/nand/nand_base.c |   99 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/nand.h     |   90 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 186 insertions(+), 3 deletions(-)

-- 
1.7.2.rc3

[PATCH v2 1/5] mtd: nand: add the data structures for JEDEC parameter page

[Huang Shijie]

Create the nand_jedec_params{} and jedec_ecc_info{} according to
the JESD230A(Revision of JESD230, October 2012).

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

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c034dc4..588f8a4 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -342,6 +342,81 @@ struct nand_onfi_vendor_micron {
 	u8 param_revision;
 } __packed;
 
+struct jedec_ecc_info {
+	u8 ecc_bits;
+	u8 codeword_size;
+	__le16 bb_per_lun;
+	__le16 block_endurance;
+	u8 reserved[2];
+} __packed;
+
+struct nand_jedec_params {
+	/* rev info and features block */
+	/* 'J' 'E' 'S' 'D'  */
+	u8 sig[4];
+	__le16 revision;
+	__le16 features;
+	u8 opt_cmd[3];
+	__le16 sec_cmd;
+	u8 num_of_param_pages;
+	u8 reserved0[18];
+
+	/* manufacturer information block */
+	char manufacturer[12];
+	char model[20];
+	u8 jedec_id[6];
+	u8 reserved1[10];
+
+	/* memory organization block */
+	__le32 byte_per_page;
+	__le16 spare_bytes_per_page;
+	u8 reserved2[6];
+	__le32 pages_per_block;
+	__le32 blocks_per_lun;
+	u8 lun_count;
+	u8 addr_cycles;
+	u8 bits_per_cell;
+	u8 programs_per_page;
+	u8 multi_plane_addr;
+	u8 multi_plane_op_attr;
+	u8 reserved3[38];
+
+	/* electrical parameter block */
+	__le16 async_sdr_speed_grade;
+	__le16 toggle_ddr_speed_grade;
+	__le16 sync_ddr_speed_grade;
+	u8 async_sdr_features;
+	u8 toggle_ddr_features;
+	u8 sync_ddr_features;
+	__le16 t_prog;
+	__le16 t_bers;
+	__le16 t_r;
+	__le16 t_r_multi_plane;
+	__le16 t_ccs;
+	__le16 io_pin_capacitance_typ;
+	__le16 input_pin_capacitance_typ;
+	__le16 clk_pin_capacitance_typ;
+	u8 driver_strength_support;
+	__le16 t_ald;
+	u8 reserved4[36];
+
+	/* ECC and endurance block */
+	u8 guaranteed_good_blocks;
+	__le16 guaranteed_block_endurance;
+	struct jedec_ecc_info ecc_info[4];
+	u8 reserved5[29];
+
+	/* reserved */
+	u8 reserved6[148];
+
+	/* vendor */
+	__le16 vendor_rev_num;
+	u8 reserved7[88];
+
+	/* CRC for Parameter Page */
+	__le16 crc;
+} __packed;
+
 /**
  * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
  * @lock:               protection lock
-- 
1.7.2.rc3

[PATCH v2 2/5] mtd: nand: add fields for JEDEC in nand_chip

[Huang Shijie]

Add the jedec_version field, and add an anonymous union which
contains the nand_onfi_params and nand_jedec_params.

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

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 588f8a4..9686390 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -664,7 +664,11 @@ struct nand_chip {
 	int badblockbits;
 
 	int onfi_version;
-	struct nand_onfi_params	onfi_params;
+	int jedec_version;
+	union {
+		struct nand_onfi_params	onfi_params;
+		struct nand_jedec_params jedec_params;
+	};
 
 	int read_retries;
 
-- 
1.7.2.rc3

Re: [PATCH v2 2/5] mtd: nand: add fields for JEDEC in nand_chip

[Brian Norris]

On Sat, Feb 08, 2014 at 02:03:58PM +0800, Huang Shijie wrote:
> Add the jedec_version field, and add an anonymous union which
> contains the nand_onfi_params and nand_jedec_params.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  include/linux/mtd/nand.h |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 588f8a4..9686390 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -664,7 +664,11 @@ struct nand_chip {
>  	int badblockbits;
>  
>  	int onfi_version;
> -	struct nand_onfi_params	onfi_params;
> +	int jedec_version;
> +	union {
> +		struct nand_onfi_params	onfi_params;
> +		struct nand_jedec_params jedec_params;

Can you document the new fields in the kerneldoc comments above struct
nand_chip?

> +	};
>  
>  	int read_retries;
>  

Brian

[PATCH v2 3/5] mtd: nand: add a helper to get the supported features for JEDEC

[Huang Shijie]

Add a helper to get the supported features for JEDEC compliant nand.
Also add a macro JEDEC_FEATURE_16_BIT_BUS.

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

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9686390..84cca61 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -350,6 +350,9 @@ struct jedec_ecc_info {
 	u8 reserved[2];
 } __packed;
 
+/* JEDEC features */
+#define JEDEC_FEATURE_16_BIT_BUS	(1 << 0)
+
 struct nand_jedec_params {
 	/* rev info and features block */
 	/* 'J' 'E' 'S' 'D'  */
@@ -921,4 +924,10 @@ static inline int nand_opcode_8bits(unsigned int command)
 	return command == NAND_CMD_READID;
 }
 
+/* return the supported JEDEC features. */
+static inline int jedec_feature(struct nand_chip *chip)
+{
+	return chip->jedec_version ? le16_to_cpu(chip->jedec_params.features)
+		: 0;
+}
 #endif /* __LINUX_MTD_NAND_H */
-- 
1.7.2.rc3

[PATCH v2 4/5] mtd: nand: parse out the JEDEC compliant nand

[Huang Shijie]

This patch adds the parsing code for the JEDEC compliant nand.

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 47fdf17..eb9fb49 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3163,6 +3163,88 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /*
+ * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
+ */
+static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
+					int *busw)
+{
+	struct nand_jedec_params *p = &chip->jedec_params;
+	struct jedec_ecc_info *ecc;
+	int val;
+	int i, j;
+
+	/* Try JEDEC for unknown chip or LP */
+	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
+	if (chip->read_byte(mtd) != 'J' || chip->read_byte(mtd) != 'E' ||
+		chip->read_byte(mtd) != 'D' || chip->read_byte(mtd) != 'E' ||
+		chip->read_byte(mtd) != 'C')
+		return 0;
+
+	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40, -1);
+	for (i = 0; i < 3; i++) {
+		for (j = 0; j < sizeof(*p); j++)
+			((uint8_t *)p)[j] = chip->read_byte(mtd);
+
+		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
+				le16_to_cpu(p->crc))
+			break;
+	}
+
+	if (i == 3) {
+		pr_err("Could not find valid JEDEC parameter page; aborting\n");
+		return 0;
+	}
+
+	/* Check version */
+	val = le16_to_cpu(p->revision);
+	if (val & (1 << 2))
+		chip->jedec_version = 10;
+	else if (val & (1 << 1))
+		chip->jedec_version = 1; /* vendor specific version */
+
+	if (!chip->jedec_version) {
+		pr_info("unsupported JEDEC version: %d\n", val);
+		return 0;
+	}
+
+	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+	sanitize_string(p->model, sizeof(p->model));
+	if (!mtd->name)
+		mtd->name = p->model;
+
+	mtd->writesize = le32_to_cpu(p->byte_per_page);
+
+	/* Please reference to the comment for nand_flash_detect_onfi. */
+	mtd->erasesize = 1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
+	mtd->erasesize *= mtd->writesize;
+
+	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+
+	/* Please reference to the comment for nand_flash_detect_onfi. */
+	chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
+	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
+	chip->bits_per_cell = p->bits_per_cell;
+
+	if (jedec_feature(chip) & JEDEC_FEATURE_16_BIT_BUS)
+		*busw = NAND_BUSWIDTH_16;
+	else
+		*busw = 0;
+
+	/* ECC info */
+	ecc = p->ecc_info;
+
+	if (ecc->codeword_size) {
+		chip->ecc_strength_ds = ecc->ecc_bits;
+		chip->ecc_step_ds = 1 << ecc->codeword_size;
+	} else {
+		pr_debug("Invalid codeword size\n");
+		return 0;
+	}
+
+	return 1;
+}
+
+/*
  * nand_id_has_period - Check if an ID string has a given wraparound period
  * @id_data: the ID string
  * @arrlen: the length of the @id_data array
@@ -3527,6 +3609,10 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		/* Check is chip is ONFI compliant */
 		if (nand_flash_detect_onfi(mtd, chip, &busw))
 			goto ident_done;
+
+		/* Check if the chip is JEDEC compliant */
+		if (nand_flash_detect_jedec(mtd, chip, &busw))
+			goto ident_done;
 	}
 
 	if (!type->name)
-- 
1.7.2.rc3

Re: [PATCH v2 4/5] mtd: nand: parse out the JEDEC compliant nand

[Brian Norris]

Hi Huang,

Looks good, but I have a few comments, now that I've given the spec a
closer read.

On Sat, Feb 08, 2014 at 02:04:00PM +0800, Huang Shijie wrote:
> This patch adds the parsing code for the JEDEC compliant nand.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |   86 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 86 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 47fdf17..eb9fb49 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3163,6 +3163,88 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  }
>  
>  /*
> + * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
> + */
> +static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> +					int *busw)
> +{
> +	struct nand_jedec_params *p = &chip->jedec_params;
> +	struct jedec_ecc_info *ecc;
> +	int val;
> +	int i, j;
> +
> +	/* Try JEDEC for unknown chip or LP */
> +	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);

Where did you get this READID column address? I may be misreading
something, but I don't even find this in the JESD230A spec.

> +	if (chip->read_byte(mtd) != 'J' || chip->read_byte(mtd) != 'E' ||
> +		chip->read_byte(mtd) != 'D' || chip->read_byte(mtd) != 'E' ||
> +		chip->read_byte(mtd) != 'C')
> +		return 0;
> +
> +	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40, -1);

Same here. I don't actually find this column address in the spec. I
really hope this is specified somewhere.

Also, now that we may have non-zero address to NAND_CMD_PARAM, do we
need to make this adjustment so it can be used on x16 buswidth? Perhaps this diff?

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 84cca611c2fd..1b1ad3d25336 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -921,7 +921,7 @@ static inline bool nand_is_slc(struct nand_chip *chip)
  */
 static inline int nand_opcode_8bits(unsigned int command)
 {
-	return command == NAND_CMD_READID;
+	return command == NAND_CMD_READID || command == NAND_CMD_PARAM;
 }
 
 /* return the supported JEDEC features. */

> +	for (i = 0; i < 3; i++) {
> +		for (j = 0; j < sizeof(*p); j++)
> +			((uint8_t *)p)[j] = chip->read_byte(mtd);
> +
> +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> +				le16_to_cpu(p->crc))
> +			break;
> +	}
> +
> +	if (i == 3) {
> +		pr_err("Could not find valid JEDEC parameter page; aborting\n");
> +		return 0;
> +	}
> +
> +	/* Check version */
> +	val = le16_to_cpu(p->revision);
> +	if (val & (1 << 2))
> +		chip->jedec_version = 10;
> +	else if (val & (1 << 1))
> +		chip->jedec_version = 1; /* vendor specific version */

How did you determine that bit[1] means version 1 (or v0.1)? It's
unclear to me how to interpret the revision bitfield here, but it
appears that bit[1] is not really useful to us yet, unless we're using
vendor specific parameter page.

What do we do if we see bit[1] (vendor specific parameter page) but not
bit[2] (parameter page revision 1.0 / standard revision 1.0)? I'm
thinking that we should just ignore bit[1] entirely for now, and if the
flash is missing bit[2], then we can't support it.

> +
> +	if (!chip->jedec_version) {
> +		pr_info("unsupported JEDEC version: %d\n", val);
> +		return 0;
> +	}
> +
> +	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
> +	sanitize_string(p->model, sizeof(p->model));
> +	if (!mtd->name)
> +		mtd->name = p->model;
> +
> +	mtd->writesize = le32_to_cpu(p->byte_per_page);
> +
> +	/* Please reference to the comment for nand_flash_detect_onfi. */
> +	mtd->erasesize = 1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
> +	mtd->erasesize *= mtd->writesize;
> +
> +	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> +
> +	/* Please reference to the comment for nand_flash_detect_onfi. */
> +	chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
> +	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
> +	chip->bits_per_cell = p->bits_per_cell;
> +
> +	if (jedec_feature(chip) & JEDEC_FEATURE_16_BIT_BUS)
> +		*busw = NAND_BUSWIDTH_16;
> +	else
> +		*busw = 0;
> +
> +	/* ECC info */
> +	ecc = p->ecc_info;

ecc_info is an array, and we're only looking at the first entry. This
might be more clear:

	ecc = &p->ecc_info[0];

> +
> +	if (ecc->codeword_size) {

"The minimum value that shall be reported is 512 bytes (a value of 9)."

So how about this?

	if (ecc->codeword_size >= 9)

> +		chip->ecc_strength_ds = ecc->ecc_bits;
> +		chip->ecc_step_ds = 1 << ecc->codeword_size;
> +	} else {
> +		pr_debug("Invalid codeword size\n");
> +		return 0;

Do we really want to fail detection if we can't get the ECC parameters?
That's not what we do for ONFI, and we certainly don't do that for
extended ID decoding. Right now, ECC specifications are an optional
feature for autodetection, so I think this should be at most a
pr_warn(), but return successfully still. See the ONFI detection routine
for reference.

> +	}
> +
> +	return 1;
> +}
> +
> +/*
>   * nand_id_has_period - Check if an ID string has a given wraparound period
>   * @id_data: the ID string
>   * @arrlen: the length of the @id_data array
> @@ -3527,6 +3609,10 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>  		/* Check is chip is ONFI compliant */
>  		if (nand_flash_detect_onfi(mtd, chip, &busw))
>  			goto ident_done;
> +
> +		/* Check if the chip is JEDEC compliant */
> +		if (nand_flash_detect_jedec(mtd, chip, &busw))
> +			goto ident_done;
>  	}
>  
>  	if (!type->name)

Brian

Re: [PATCH v2 4/5] mtd: nand: parse out the JEDEC compliant nand

[Huang Shijie]

On Tue, Feb 11, 2014 at 12:32:24PM -0800, Brian Norris wrote:
> Hi Huang,
> 
> Looks good, but I have a few comments, now that I've given the spec a
> closer read.
> 
> On Sat, Feb 08, 2014 at 02:04:00PM +0800, Huang Shijie wrote:
> > This patch adds the parsing code for the JEDEC compliant nand.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  drivers/mtd/nand/nand_base.c |   86 ++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 86 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 47fdf17..eb9fb49 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3163,6 +3163,88 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  }
> >  
> >  /*
> > + * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
> > + */
> > +static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> > +					int *busw)
> > +{
> > +	struct nand_jedec_params *p = &chip->jedec_params;
> > +	struct jedec_ecc_info *ecc;
> > +	int val;
> > +	int i, j;
> > +
> > +	/* Try JEDEC for unknown chip or LP */
> > +	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
> 
> Where did you get this READID column address? I may be misreading

I get the column from the Toshiba and Micron datasheet.
the JEDEC really does not mention it.

> something, but I don't even find this in the JESD230A spec.
> 
> > +	if (chip->read_byte(mtd) != 'J' || chip->read_byte(mtd) != 'E' ||
> > +		chip->read_byte(mtd) != 'D' || chip->read_byte(mtd) != 'E' ||
> > +		chip->read_byte(mtd) != 'C')
> > +		return 0;
> > +
> > +	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40, -1);
> 
> Same here. I don't actually find this column address in the spec. I
ditto.

> really hope this is specified somewhere.
> 
> Also, now that we may have non-zero address to NAND_CMD_PARAM, do we
> need to make this adjustment so it can be used on x16 buswidth? Perhaps this diff?

sorry, I really do not know the x16 issue :(

the gpmi is 8bit.

> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 84cca611c2fd..1b1ad3d25336 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -921,7 +921,7 @@ static inline bool nand_is_slc(struct nand_chip *chip)
>   */
>  static inline int nand_opcode_8bits(unsigned int command)
>  {
> -	return command == NAND_CMD_READID;
> +	return command == NAND_CMD_READID || command == NAND_CMD_PARAM;
>  }
>  
>  /* return the supported JEDEC features. */
> 
> > +	for (i = 0; i < 3; i++) {
> > +		for (j = 0; j < sizeof(*p); j++)
> > +			((uint8_t *)p)[j] = chip->read_byte(mtd);
> > +
> > +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> > +				le16_to_cpu(p->crc))
> > +			break;
> > +	}
> > +
> > +	if (i == 3) {
> > +		pr_err("Could not find valid JEDEC parameter page; aborting\n");
> > +		return 0;
> > +	}
> > +
> > +	/* Check version */
> > +	val = le16_to_cpu(p->revision);
> > +	if (val & (1 << 2))
> > +		chip->jedec_version = 10;
> > +	else if (val & (1 << 1))
> > +		chip->jedec_version = 1; /* vendor specific version */
> 
> How did you determine that bit[1] means version 1 (or v0.1)? It's

The bit[1] is the vendor specific version. 
Some Micron chips uses the bit[1], such as MT29F64G08CBAB.

If we do not use the 1 for the it, do you have any suggestion
for the vendor specific version?


> unclear to me how to interpret the revision bitfield here, but it
> appears that bit[1] is not really useful to us yet, unless we're using
> vendor specific parameter page.

> 
> What do we do if we see bit[1] (vendor specific parameter page) but not
> bit[2] (parameter page revision 1.0 / standard revision 1.0)? I'm
> thinking that we should just ignore bit[1] entirely for now, and if the
> flash is missing bit[2], then we can't support it.

some Micron chips do use the bit[1]. i think that's why the JEDEC say:
"supports vendor specific parameter page" for the bit[1].

We'd better keep the support for the bit[1].
> 
> > +
> > +	if (!chip->jedec_version) {
> > +		pr_info("unsupported JEDEC version: %d\n", val);
> > +		return 0;
> > +	}
> > +
> > +	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
> > +	sanitize_string(p->model, sizeof(p->model));
> > +	if (!mtd->name)
> > +		mtd->name = p->model;
> > +
> > +	mtd->writesize = le32_to_cpu(p->byte_per_page);
> > +
> > +	/* Please reference to the comment for nand_flash_detect_onfi. */
> > +	mtd->erasesize = 1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
> > +	mtd->erasesize *= mtd->writesize;
> > +
> > +	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> > +
> > +	/* Please reference to the comment for nand_flash_detect_onfi. */
> > +	chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
> > +	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
> > +	chip->bits_per_cell = p->bits_per_cell;
> > +
> > +	if (jedec_feature(chip) & JEDEC_FEATURE_16_BIT_BUS)
> > +		*busw = NAND_BUSWIDTH_16;
> > +	else
> > +		*busw = 0;
> > +
> > +	/* ECC info */
> > +	ecc = p->ecc_info;
> 
> ecc_info is an array, and we're only looking at the first entry. This
> might be more clear:
> 
> 	ecc = &p->ecc_info[0];
okay.

> 
> > +
> > +	if (ecc->codeword_size) {
> 
> "The minimum value that shall be reported is 512 bytes (a value of 9)."
> 
> So how about this?
> 
> 	if (ecc->codeword_size >= 9)
okay.
> 
> > +		chip->ecc_strength_ds = ecc->ecc_bits;
> > +		chip->ecc_step_ds = 1 << ecc->codeword_size;
> > +	} else {
> > +		pr_debug("Invalid codeword size\n");
> > +		return 0;
> 
> Do we really want to fail detection if we can't get the ECC parameters?
> That's not what we do for ONFI, and we certainly don't do that for
> extended ID decoding. Right now, ECC specifications are an optional
> feature for autodetection, so I think this should be at most a
> pr_warn(), but return successfully still. See the ONFI detection routine
> for reference.
okay.

thanks
Huang Shijie

Re: [PATCH v2 4/5] mtd: nand: parse out the JEDEC compliant nand

[Brian Norris]

Hi,

On Fri, Feb 14, 2014 at 03:08:54PM +0800, Huang Shijie wrote:
> On Tue, Feb 11, 2014 at 12:32:24PM -0800, Brian Norris wrote:
> > On Sat, Feb 08, 2014 at 02:04:00PM +0800, Huang Shijie wrote:
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3163,6 +3163,88 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > >  }
> > >  
> > >  /*
> > > + * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
> > > + */
> > > +static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> > > +					int *busw)
> > > +{
> > > +	struct nand_jedec_params *p = &chip->jedec_params;
> > > +	struct jedec_ecc_info *ecc;
> > > +	int val;
> > > +	int i, j;
> > > +
> > > +	/* Try JEDEC for unknown chip or LP */
> > > +	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
> > 
> > Where did you get this READID column address? I may be misreading
> 
> I get the column from the Toshiba and Micron datasheet.
> the JEDEC really does not mention it.

OK, that's fine. It's still a bit strange the JEDEC doesn't mention it.

> > something, but I don't even find this in the JESD230A spec.
> > 
> > > +	if (chip->read_byte(mtd) != 'J' || chip->read_byte(mtd) != 'E' ||
> > > +		chip->read_byte(mtd) != 'D' || chip->read_byte(mtd) != 'E' ||
> > > +		chip->read_byte(mtd) != 'C')
> > > +		return 0;
> > > +
> > > +	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40, -1);
> > 
> > Same here. I don't actually find this column address in the spec. I
> ditto.
> 
> > really hope this is specified somewhere.
> > 
> > Also, now that we may have non-zero address to NAND_CMD_PARAM, do we
> > need to make this adjustment so it can be used on x16 buswidth? Perhaps this diff?
> 
> sorry, I really do not know the x16 issue :(
> 
> the gpmi is 8bit.

That's fine, I expected that. But I think that in the absence of
evidence otherwise, we should always send the address for NAND_CMD_PARAM
as if it's only on the lower 8 bits, not a x16-width column address. Can
you squash in my diff below, or else I can send it as a separate patch?

> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 84cca611c2fd..1b1ad3d25336 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -921,7 +921,7 @@ static inline bool nand_is_slc(struct nand_chip *chip)
> >   */
> >  static inline int nand_opcode_8bits(unsigned int command)
> >  {
> > -	return command == NAND_CMD_READID;
> > +	return command == NAND_CMD_READID || command == NAND_CMD_PARAM;
> >  }
> >  
> >  /* return the supported JEDEC features. */
> > 
> > > +	for (i = 0; i < 3; i++) {
> > > +		for (j = 0; j < sizeof(*p); j++)
> > > +			((uint8_t *)p)[j] = chip->read_byte(mtd);
> > > +
> > > +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> > > +				le16_to_cpu(p->crc))
> > > +			break;
> > > +	}
> > > +
> > > +	if (i == 3) {
> > > +		pr_err("Could not find valid JEDEC parameter page; aborting\n");
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Check version */
> > > +	val = le16_to_cpu(p->revision);
> > > +	if (val & (1 << 2))
> > > +		chip->jedec_version = 10;
> > > +	else if (val & (1 << 1))
> > > +		chip->jedec_version = 1; /* vendor specific version */
> > 
> > How did you determine that bit[1] means version 1 (or v0.1)? It's
> 
> The bit[1] is the vendor specific version. 
> Some Micron chips uses the bit[1], such as MT29F64G08CBAB.

I noticed a similar thing with my Micron datasheet.

> If we do not use the 1 for the it, do you have any suggestion
> for the vendor specific version?
> 
> 
> > unclear to me how to interpret the revision bitfield here, but it
> > appears that bit[1] is not really useful to us yet, unless we're using
> > vendor specific parameter page.
> 
> > 
> > What do we do if we see bit[1] (vendor specific parameter page) but not
> > bit[2] (parameter page revision 1.0 / standard revision 1.0)? I'm
> > thinking that we should just ignore bit[1] entirely for now, and if the
> > flash is missing bit[2], then we can't support it.
> 
> some Micron chips do use the bit[1]. i think that's why the JEDEC say:
> "supports vendor specific parameter page" for the bit[1].
> 
> We'd better keep the support for the bit[1].

OK. But we need to be careful that whatever this "vendor specific
parameter page" is, it's always compatible with the spec we're referring
to (v1.0). Otherwise, we can't rely on bit[1] to tell us anything
specific. This really seems like some sloppiness on either Micron or
JEDEC.

[snip]

Thanks,
Brian

[PATCH v2 5/5] mtd: nand: print out the right information for JEDEC compliant nand

[Huang Shijie]

Check the chip->jedec_version, and print out the right information
for JEDEC compliant nand.

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index eb9fb49..57443db 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3692,8 +3692,17 @@ ident_done:
 
 	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
 		*maf_id, *dev_id);
-	pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
-		chip->onfi_version ? chip->onfi_params.model : type->name);
+
+	if (chip->onfi_version)
+		pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
+				chip->onfi_params.model);
+	else if (chip->jedec_version)
+		pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
+				chip->jedec_params.model);
+	else
+		pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
+				type->name);
+
 	pr_info("%dMiB, %s, page size: %d, OOB size: %d\n",
 		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
 		mtd->writesize, mtd->oobsize);
-- 
1.7.2.rc3

Re: [PATCH v2 0/5] mtd: nand: support the JEDEC compliant nand.

[Brian Norris]

Hi Huang,

On Sat, Feb 08, 2014 at 02:03:56PM +0800, Huang Shijie wrote:
> Some vendors support the JEDEC standard only, such as Toshiba.
> We can get all the information for the JEDEC parameter page, just like
> we did with the ONFI parameter page.

It is heartening that they are finally wisening up for their MLC. I
don't believe Toshiba has plans to support this on their SLC, but we
can't have everything! SLC is more stable anyway, I think.

> This patch set adds the support for the JEDEC compliant nand chips.
> 
> You can download the JEDEC standard about the NAND in the:
>     www.jedec.org
> 
> My code references to the JESD230A, August 2013, revision 1.
> 
> Tested with Toshiba TH58TEG7DDKTA20(16K + 1280).
> (Unfortuately, this ECC info of its JEDEC parameter page is zero,
>  TOSHIBA FAE confirmed that they will fix it in future.)
> 
> v1 --> v2:
> 	[1] use the read_byte to replace the read_buf.
> 
> Huang Shijie (5):
>   mtd: nand: add the data structures for JEDEC parameter page
>   mtd: nand: add fields for JEDEC in nand_chip
>   mtd: nand: add a helper to get the supported features for JEDEC
>   mtd: nand: parse out the JEDEC compliant nand
>   mtd: nand: print out the right information for JEDEC compliant nand

Other than my comments on patches 2 and 4, ack'd by me. Thanks for the
work.

BTW, it looks like a few Micron parts I have actually support both ONFI
and JEDEC parameter pages. I'll see if I can test this out on them.

Brian

Re: [PATCH v2 0/5] mtd: nand: support the JEDEC compliant nand.

[Huang Shijie]

On Tue, Feb 11, 2014 at 12:43:25PM -0800, Brian Norris wrote:
> BTW, it looks like a few Micron parts I have actually support both ONFI
> and JEDEC parameter pages. I'll see if I can test this out on them.
yes. 

some Micron chips also supported the JEDEC.
I had already tested the Micron chips.

thanks
Huang Shijie