linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes
@ 2019-02-04 11:17 Shivamurthy Shastri (sshivamurthy)
  2019-02-04 12:32 ` Emil Lenngren
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-02-04 11:17 UTC (permalink / raw)
  To: Boris Brezillon, Miquel Raynal, linux-mtd, linux-kernel
  Cc: Chuanhong Guo, Richard Weinberger, Schrempf Frieder, Marek Vasut,
	Frieder Schrempf, Brian Norris, David Woodhouse,
	Bean Huo (beanhuo)

Driver is redesigned using parameter page to support all the Micron
SPI NAND flashes.

Parameter page of Micron flashes is similar to ONFI parameter table and
functionality is same, so copied some of the common functions like crc16
and bit_wise_majority from nand_onfi.c.

This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
MT29F1G01ABXFD.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
---
 drivers/mtd/nand/spi/micron.c | 172 +++++++++++++++++++++++++++-------
 drivers/mtd/nand/spi/micron.h |  83 ++++++++++++++++
 2 files changed, 221 insertions(+), 34 deletions(-)
 create mode 100644 drivers/mtd/nand/spi/micron.h

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 9c4381d6847b..c9c53fd0aa01 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -10,13 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/mtd/spinand.h>
 
-#define SPINAND_MFR_MICRON		0x2c
-
-#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
-#define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
-#define MICRON_STATUS_ECC_1TO3_BITFLIPS	(1 << 4)
-#define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
-#define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
+#include "micron.h"
 
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
@@ -34,38 +28,38 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
 		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
 		SPINAND_PROG_LOAD(false, 0, NULL, 0));
 
-static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int section,
-					struct mtd_oob_region *region)
+static int ooblayout_ecc(struct mtd_info *mtd, int section,
+			 struct mtd_oob_region *region)
 {
 	if (section)
 		return -ERANGE;
 
-	region->offset = 64;
-	region->length = 64;
+	region->offset = mtd->oobsize / 2;
+	region->length = mtd->oobsize / 2;
 
 	return 0;
 }
 
-static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int section,
-					 struct mtd_oob_region *region)
+static int ooblayout_free(struct mtd_info *mtd, int section,
+			  struct mtd_oob_region *region)
 {
 	if (section)
 		return -ERANGE;
 
 	/* Reserve 2 bytes for the BBM. */
 	region->offset = 2;
-	region->length = 62;
+	region->length = (mtd->oobsize / 2) - 2;
 
 	return 0;
 }
 
-static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
-	.ecc = mt29f2g01abagd_ooblayout_ecc,
-	.free = mt29f2g01abagd_ooblayout_free,
+static const struct mtd_ooblayout_ops ooblayout = {
+	.ecc = ooblayout_ecc,
+	.free = ooblayout_free,
 };
 
-static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
-					 u8 status)
+static int ecc_get_status(struct spinand_device *spinand,
+			  u8 status)
 {
 	switch (status & MICRON_STATUS_ECC_MASK) {
 	case STATUS_ECC_NO_BITFLIPS:
@@ -90,22 +84,53 @@ static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
 	return -EINVAL;
 }
 
-static const struct spinand_info micron_spinand_table[] = {
-	SPINAND_INFO("MT29F2G01ABAGD", 0x24,
-		     NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
-		     NAND_ECCREQ(8, 512),
-		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
-					      &write_cache_variants,
-					      &update_cache_variants),
-		     0,
-		     SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
-				     mt29f2g01abagd_ecc_get_status)),
-};
+static u16 spinand_crc16(u16 crc, u8 const *p, size_t len)
+{
+	int i;
+
+	while (len--) {
+		crc ^= *p++ << 8;
+		for (i = 0; i < 8; i++)
+			crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
+	}
+
+	return crc;
+}
+
+static void bit_wise_majority(const void **srcbufs,
+			      unsigned int nsrcbufs,
+				   void *dstbuf,
+				   unsigned int bufsize)
+{
+	int i, j, k;
+
+	for (i = 0; i < bufsize; i++) {
+		u8 val = 0;
+
+		for (j = 0; j < 8; j++) {
+			unsigned int cnt = 0;
+
+			for (k = 0; k < nsrcbufs; k++) {
+				const u8 *srcbuf = srcbufs[k];
+
+				if (srcbuf[i] & BIT(j))
+					cnt++;
+			}
+
+			if (cnt > nsrcbufs / 2)
+				val |= BIT(j);
+		}
+
+		((u8 *)dstbuf)[i] = val;
+	}
+}
 
 static int micron_spinand_detect(struct spinand_device *spinand)
 {
+	struct spinand_info deviceinfo;
+	struct micron_spinand_params *params;
 	u8 *id = spinand->id.data;
-	int ret;
+	int ret, i;
 
 	/*
 	 * Micron SPI NAND read ID need a dummy byte,
@@ -114,16 +139,95 @@ static int micron_spinand_detect(struct spinand_device *spinand)
 	if (id[1] != SPINAND_MFR_MICRON)
 		return 0;
 
-	ret = spinand_match_and_init(spinand, micron_spinand_table,
-				     ARRAY_SIZE(micron_spinand_table), id[2]);
+	params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
+	if (!params)
+		return -ENOMEM;
+
+	ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE, params,
+					  sizeof(*params) * 3);
 	if (ret)
-		return ret;
+		goto free_params;
+
+	for (i = 0; i < 3; i++) {
+		if (spinand_crc16(0x4F4E, (u8 *)&params[i], 254) ==
+				le16_to_cpu(params->crc)) {
+			if (i)
+				memcpy(params, &params[i], sizeof(*params));
+			break;
+		}
+	}
+
+	if (i == 3) {
+		const void *srcbufs[3] = {params, params + 1, params + 2};
+
+		pr_warn("No valid parameter page, trying bit-wise majority to recover it\n");
+		bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
+				  sizeof(*params));
+
+		if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
+				le16_to_cpu(params->crc)) {
+			pr_err("Parameter page recovery failed, aborting\n");
+			goto free_params;
+		}
+	}
+
+	params->model[sizeof(params->model) - 1] = 0;
+	strim(params->model);
+
+	deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
+	if (!deviceinfo.model) {
+		ret = -ENOMEM;
+		goto free_params;
+	}
+
+	deviceinfo.devid = id[2];
+	deviceinfo.flags = 0;
+	deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
+	deviceinfo.memorg.pagesize = params->byte_per_page;
+	deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
+	deviceinfo.memorg.pages_per_eraseblock = params->pages_per_block;
+	deviceinfo.memorg.eraseblocks_per_lun =
+		params->blocks_per_lun * params->lun_count;
+	deviceinfo.memorg.planes_per_lun = params->lun_count;
+	deviceinfo.memorg.luns_per_target = 1;
+	deviceinfo.memorg.ntargets = 1;
+	deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
+	deviceinfo.eccreq.step_size = 512;
+	deviceinfo.eccinfo.get_status = ecc_get_status;
+	deviceinfo.eccinfo.ooblayout = &ooblayout;
+	deviceinfo.op_variants.read_cache = &read_cache_variants;
+	deviceinfo.op_variants.write_cache = &write_cache_variants;
+	deviceinfo.op_variants.update_cache = &update_cache_variants;
+
+	ret = spinand_match_and_init(spinand, &deviceinfo,
+				     1, id[2]);
+	if (ret)
+		goto free_model;
+
+	kfree(params);
 
 	return 1;
+
+free_model:
+	kfree(deviceinfo.model);
+free_params:
+	kfree(params);
+
+	return ret;
+}
+
+static int micron_spinand_init(struct spinand_device *spinand)
+{
+	/*
+	 * Some of the Micron flashes enable this BIT by default,
+	 * and there is a chance of read failure due to this.
+	 */
+	return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
 }
 
 static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
 	.detect = micron_spinand_detect,
+	.init = micron_spinand_init,
 };
 
 const struct spinand_manufacturer micron_spinand_manufacturer = {
diff --git a/drivers/mtd/nand/spi/micron.h b/drivers/mtd/nand/spi/micron.h
new file mode 100644
index 000000000000..c2cf3bee6f7e
--- /dev/null
+++ b/drivers/mtd/nand/spi/micron.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 Micron Technology, Inc.
+ *
+ * Authors:
+ *	Shivamurthy Shastri <sshivamurthy@micron.com>
+ */
+
+#ifndef __MICRON_H
+#define __MICRON_H
+
+#define SPINAND_MFR_MICRON		0x2c
+
+#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
+#define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
+#define MICRON_STATUS_ECC_1TO3_BITFLIPS	BIT(4)
+#define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
+#define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
+
+#define UNIQUE_ID_PAGE	0x00
+#define PARAMETER_PAGE	0x01
+
+/*
+ * Micron SPI NAND has parameter table similar to ONFI
+ */
+struct micron_spinand_params {
+	/* rev info and features block */
+	u8 sig[4];
+	__le16 revision;
+	__le16 features;
+	__le16 opt_cmd;
+	u8 reserved0[22];
+
+	/* manufacturer information block */
+	char manufacturer[12];
+	char model[20];
+	u8 manufact_id;
+	__le16 date_code;
+	u8 reserved1[13];
+
+	/* memory organization block */
+	__le32 byte_per_page;
+	__le16 spare_bytes_per_page;
+	__le32 data_bytes_per_ppage;
+	__le16 spare_bytes_per_ppage;
+	__le32 pages_per_block;
+	__le32 blocks_per_lun;
+	u8 lun_count;
+	u8 addr_cycles;
+	u8 bits_per_cell;
+	__le16 bb_per_lun;
+	__le16 block_endurance;
+	u8 guaranteed_good_blocks;
+	__le16 guaranteed_block_endurance;
+	u8 programs_per_page;
+	u8 ppage_attr;
+	u8 ecc_bits;
+	u8 interleaved_bits;
+	u8 interleaved_ops;
+	u8 reserved2[13];
+
+	/* electrical parameter block */
+	u8 io_pin_capacitance_max;
+	__le16 async_timing_mode;
+	__le16 program_cache_timing_mode;
+	__le16 t_prog;
+	__le16 t_bers;
+	__le16 t_r;
+	__le16 t_ccs;
+	u8 reserved3[23];
+
+	/* vendor */
+	__le16 vendor_revision;
+	u8 vendor_specific[14];
+	u8 reserved4[68];
+	u8 ecc_max_correct_ability;
+	u8 die_select_feature;
+	u8 reserved5[4];
+
+	__le16 crc;
+} __packed;
+
+#endif /* __MICRON_H */
-- 
2.17.1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes
  2019-02-04 11:17 [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes Shivamurthy Shastri (sshivamurthy)
@ 2019-02-04 12:32 ` Emil Lenngren
  2019-02-04 18:01 ` Boris Brezillon
  2019-02-04 23:05 ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: Emil Lenngren @ 2019-02-04 12:32 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Boris Brezillon, Richard Weinberger, linux-kernel,
	Schrempf Frieder, Marek Vasut, Frieder Schrempf, linux-mtd,
	Miquel Raynal, Brian Norris, Chuanhong Guo, David Woodhouse,
	Bean Huo (beanhuo)

Hi,

Den mån 4 feb. 2019 kl 12:18 skrev Shivamurthy Shastri (sshivamurthy)
<sshivamurthy@micron.com>:
>
> Driver is redesigned using parameter page to support all the Micron
> SPI NAND flashes.
>
> Parameter page of Micron flashes is similar to ONFI parameter table and
> functionality is same, so copied some of the common functions like crc16
> and bit_wise_majority from nand_onfi.c.
>
> This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
> MT29F1G01ABXFD.
>

> -static const struct spinand_info micron_spinand_table[] = {
> -       SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> -                    NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),

> +       deviceinfo.memorg.eraseblocks_per_lun =
> +               params->blocks_per_lun * params->lun_count;
> +       deviceinfo.memorg.planes_per_lun = params->lun_count;
> +       deviceinfo.memorg.luns_per_target = 1;
> +       deviceinfo.memorg.ntargets = 1;

> +       __le32 blocks_per_lun;
> +       u8 lun_count;
> +       u8 addr_cycles;
> +       u8 bits_per_cell;
> +       __le16 bb_per_lun;

I have a question about the lun_count. As it is now, the
planes_per_lun parameter is initialized to 2 in NAND_MEMORG. In your
patch, it is instead initialized from the "lun_count" property from
the parameter table. But I looked at a datasheet I found by a simple
Google search (https://www.google.se/search?q=micron+nand+spi+datasheet),
the first hit is to the 1 Gb flash MT29F1G01AAADD. That device clearly
has two planes per lun (you need the "plane select" bit in the
requests), but still, according to the parameter page data structure,
byte 100, Number of logical units is set to 01h. Also, the "blocks per
lun" count, which is called "blocks per unit" is 1024, which should be
512 if this parameter really meant "blocks per plane" and the
calculation in the patch was correct.

As a reference, the 2 Gb version of the Macronix flash
(http://www.macronix.com/Lists/Datasheet/Attachments/6866/MX35LF2GE4AB,%203V,%202Gb,%20v1.5.pdf),
also has two planes per lun. It also sets byte 100, Number of logical
units to 01h.

So what I'm wondering is of course if this parameter is the correct
one to use for planes_per_lun. I tried to locate the correct "planes
per lun" parameter in the table, but didn't find anyone. Maybe it's
the unfortunate fact that "planes per lun" isn't exposed in the
parameter table?

/Emil

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes
  2019-02-04 11:17 [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes Shivamurthy Shastri (sshivamurthy)
  2019-02-04 12:32 ` Emil Lenngren
@ 2019-02-04 18:01 ` Boris Brezillon
  2019-03-04 13:29   ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  2019-02-04 23:05 ` kbuild test robot
  2 siblings, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2019-02-04 18:01 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Richard Weinberger, linux-kernel, Schrempf Frieder, Marek Vasut,
	Frieder Schrempf, linux-mtd, Miquel Raynal, Brian Norris,
	Chuanhong Guo, David Woodhouse, Bean Huo (beanhuo)

Hi Shivamurthy,

On Mon, 4 Feb 2019 11:17:51 +0000
"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote:

> Driver is redesigned using parameter page to support all the Micron
> SPI NAND flashes.

Do all Micron SPI NANDs really expose a valid ONFI param page? If
that's not the case, then relying on ONFi parsing only sounds like a
bad idea.

> 
> Parameter page of Micron flashes is similar to ONFI parameter table and
> functionality is same, so copied some of the common functions like crc16
> and bit_wise_majority from nand_onfi.c.

Most of the code is generic and does not depend on the spinand layer,
plus, we already have ONFI param page parsing code in
drivers/mtd/nand/raw/ which you're intentionally duplicating in a
version that will not be re-usable by the raw NAND layer even after
converting it to use the generic NAND layer.

Please move ONFi parsing code to drivers/mtd/nand/onfi.c and make it
generic.

> 
> This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
> MT29F1G01ABXFD.
> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>

I wish this code review had happened publicly.

> ---
>  drivers/mtd/nand/spi/micron.c | 172 +++++++++++++++++++++++++++-------
>  drivers/mtd/nand/spi/micron.h |  83 ++++++++++++++++
>  2 files changed, 221 insertions(+), 34 deletions(-)
>  create mode 100644 drivers/mtd/nand/spi/micron.h
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 9c4381d6847b..c9c53fd0aa01 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -10,13 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mtd/spinand.h>
>  
> -#define SPINAND_MFR_MICRON		0x2c
> -
> -#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
> -#define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
> -#define MICRON_STATUS_ECC_1TO3_BITFLIPS	(1 << 4)
> -#define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> -#define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
> +#include "micron.h"
>  
>  static SPINAND_OP_VARIANTS(read_cache_variants,
>  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> @@ -34,38 +28,38 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
>  		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
>  		SPINAND_PROG_LOAD(false, 0, NULL, 0));
>  
> -static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int section,
> -					struct mtd_oob_region *region)
> +static int ooblayout_ecc(struct mtd_info *mtd, int section,
> +			 struct mtd_oob_region *region)
>  {
>  	if (section)
>  		return -ERANGE;
>  
> -	region->offset = 64;
> -	region->length = 64;
> +	region->offset = mtd->oobsize / 2;
> +	region->length = mtd->oobsize / 2;
>  
>  	return 0;
>  }
>  
> -static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int section,
> -					 struct mtd_oob_region *region)
> +static int ooblayout_free(struct mtd_info *mtd, int section,
> +			  struct mtd_oob_region *region)
>  {
>  	if (section)
>  		return -ERANGE;
>  
>  	/* Reserve 2 bytes for the BBM. */
>  	region->offset = 2;
> -	region->length = 62;
> +	region->length = (mtd->oobsize / 2) - 2;
>  
>  	return 0;
>  }
>  
> -static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
> -	.ecc = mt29f2g01abagd_ooblayout_ecc,
> -	.free = mt29f2g01abagd_ooblayout_free,
> +static const struct mtd_ooblayout_ops ooblayout = {
> +	.ecc = ooblayout_ecc,
> +	.free = ooblayout_free,
>  };
>  
> -static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
> -					 u8 status)
> +static int ecc_get_status(struct spinand_device *spinand,
> +			  u8 status)
>  {
>  	switch (status & MICRON_STATUS_ECC_MASK) {
>  	case STATUS_ECC_NO_BITFLIPS:
> @@ -90,22 +84,53 @@ static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
>  	return -EINVAL;
>  }
>  
> -static const struct spinand_info micron_spinand_table[] = {
> -	SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> -		     NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
> -		     NAND_ECCREQ(8, 512),
> -		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> -					      &write_cache_variants,
> -					      &update_cache_variants),
> -		     0,
> -		     SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
> -				     mt29f2g01abagd_ecc_get_status)),
> -};
> +static u16 spinand_crc16(u16 crc, u8 const *p, size_t len)
> +{
> +	int i;
> +
> +	while (len--) {
> +		crc ^= *p++ << 8;
> +		for (i = 0; i < 8; i++)
> +			crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
> +	}
> +
> +	return crc;
> +}
> +
> +static void bit_wise_majority(const void **srcbufs,
> +			      unsigned int nsrcbufs,
> +				   void *dstbuf,
> +				   unsigned int bufsize)
> +{
> +	int i, j, k;
> +
> +	for (i = 0; i < bufsize; i++) {
> +		u8 val = 0;
> +
> +		for (j = 0; j < 8; j++) {
> +			unsigned int cnt = 0;
> +
> +			for (k = 0; k < nsrcbufs; k++) {
> +				const u8 *srcbuf = srcbufs[k];
> +
> +				if (srcbuf[i] & BIT(j))
> +					cnt++;
> +			}
> +
> +			if (cnt > nsrcbufs / 2)
> +				val |= BIT(j);
> +		}
> +
> +		((u8 *)dstbuf)[i] = val;
> +	}
> +}
>  
>  static int micron_spinand_detect(struct spinand_device *spinand)
>  {
> +	struct spinand_info deviceinfo;
> +	struct micron_spinand_params *params;
>  	u8 *id = spinand->id.data;
> -	int ret;
> +	int ret, i;
>  
>  	/*
>  	 * Micron SPI NAND read ID need a dummy byte,
> @@ -114,16 +139,95 @@ static int micron_spinand_detect(struct spinand_device *spinand)
>  	if (id[1] != SPINAND_MFR_MICRON)
>  		return 0;
>  
> -	ret = spinand_match_and_init(spinand, micron_spinand_table,
> -				     ARRAY_SIZE(micron_spinand_table), id[2]);
> +	params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
> +	if (!params)
> +		return -ENOMEM;
> +
> +	ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE, params,
> +					  sizeof(*params) * 3);
>  	if (ret)
> -		return ret;
> +		goto free_params;
> +
> +	for (i = 0; i < 3; i++) {
> +		if (spinand_crc16(0x4F4E, (u8 *)&params[i], 254) ==
> +				le16_to_cpu(params->crc)) {
> +			if (i)
> +				memcpy(params, &params[i], sizeof(*params));
> +			break;
> +		}
> +	}
> +
> +	if (i == 3) {
> +		const void *srcbufs[3] = {params, params + 1, params + 2};
> +
> +		pr_warn("No valid parameter page, trying bit-wise majority to recover it\n");
> +		bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
> +				  sizeof(*params));
> +
> +		if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
> +				le16_to_cpu(params->crc)) {
> +			pr_err("Parameter page recovery failed, aborting\n");
> +			goto free_params;
> +		}
> +	}
> +
> +	params->model[sizeof(params->model) - 1] = 0;
> +	strim(params->model);
> +
> +	deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
> +	if (!deviceinfo.model) {
> +		ret = -ENOMEM;
> +		goto free_params;
> +	}
> +
> +	deviceinfo.devid = id[2];
> +	deviceinfo.flags = 0;
> +	deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
> +	deviceinfo.memorg.pagesize = params->byte_per_page;
> +	deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
> +	deviceinfo.memorg.pages_per_eraseblock = params->pages_per_block;
> +	deviceinfo.memorg.eraseblocks_per_lun =
> +		params->blocks_per_lun * params->lun_count;
> +	deviceinfo.memorg.planes_per_lun = params->lun_count;

As pointed by Emil, this is wrong, params->lun_count should be used to
fill luns_per_target. ->planes_per_lun should be extracted from
->interleaved_bits.

> +	deviceinfo.memorg.luns_per_target = 1;
> +	deviceinfo.memorg.ntargets = 1;
> +	deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
> +	deviceinfo.eccreq.step_size = 512;
> +	deviceinfo.eccinfo.get_status = ecc_get_status;
> +	deviceinfo.eccinfo.ooblayout = &ooblayout;

Are all devices really using the same get_status method and the same
layout. Sounds risky to me to assume this is the case.

> +	deviceinfo.op_variants.read_cache = &read_cache_variants;
> +	deviceinfo.op_variants.write_cache = &write_cache_variants;
> +	deviceinfo.op_variants.update_cache = &update_cache_variants;
> +
> +	ret = spinand_match_and_init(spinand, &deviceinfo,
> +				     1, id[2]);

Please don't abuse the spinand_match_and_init() function. Fill the
nand_device object directly instead of creating a temporary spinand_info
instance with the expected id.

> +	if (ret)
> +		goto free_model;
> +
> +	kfree(params);
>  
>  	return 1;
> +
> +free_model:
> +	kfree(deviceinfo.model);
> +free_params:
> +	kfree(params);
> +
> +	return ret;
> +}
> +
> +static int micron_spinand_init(struct spinand_device *spinand)
> +{
> +	/*
> +	 * Some of the Micron flashes enable this BIT by default,
> +	 * and there is a chance of read failure due to this.
> +	 */
> +	return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
>  }
>  
>  static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
>  	.detect = micron_spinand_detect,
> +	.init = micron_spinand_init,
>  };
>  
>  const struct spinand_manufacturer micron_spinand_manufacturer = {
> diff --git a/drivers/mtd/nand/spi/micron.h b/drivers/mtd/nand/spi/micron.h
> new file mode 100644
> index 000000000000..c2cf3bee6f7e
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/micron.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 Micron Technology, Inc.
> + *
> + * Authors:
> + *	Shivamurthy Shastri <sshivamurthy@micron.com>
> + */
> +
> +#ifndef __MICRON_H
> +#define __MICRON_H
> +
> +#define SPINAND_MFR_MICRON		0x2c
> +
> +#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
> +#define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
> +#define MICRON_STATUS_ECC_1TO3_BITFLIPS	BIT(4)
> +#define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> +#define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
> +
> +#define UNIQUE_ID_PAGE	0x00
> +#define PARAMETER_PAGE	0x01
> +
> +/*
> + * Micron SPI NAND has parameter table similar to ONFI
> + */
> +struct micron_spinand_params {
> +	/* rev info and features block */
> +	u8 sig[4];
> +	__le16 revision;
> +	__le16 features;
> +	__le16 opt_cmd;
> +	u8 reserved0[22];
> +
> +	/* manufacturer information block */
> +	char manufacturer[12];
> +	char model[20];
> +	u8 manufact_id;
> +	__le16 date_code;
> +	u8 reserved1[13];
> +
> +	/* memory organization block */
> +	__le32 byte_per_page;
> +	__le16 spare_bytes_per_page;
> +	__le32 data_bytes_per_ppage;
> +	__le16 spare_bytes_per_ppage;
> +	__le32 pages_per_block;
> +	__le32 blocks_per_lun;
> +	u8 lun_count;
> +	u8 addr_cycles;
> +	u8 bits_per_cell;
> +	__le16 bb_per_lun;
> +	__le16 block_endurance;
> +	u8 guaranteed_good_blocks;
> +	__le16 guaranteed_block_endurance;
> +	u8 programs_per_page;
> +	u8 ppage_attr;
> +	u8 ecc_bits;
> +	u8 interleaved_bits;
> +	u8 interleaved_ops;
> +	u8 reserved2[13];
> +
> +	/* electrical parameter block */
> +	u8 io_pin_capacitance_max;
> +	__le16 async_timing_mode;
> +	__le16 program_cache_timing_mode;
> +	__le16 t_prog;
> +	__le16 t_bers;
> +	__le16 t_r;
> +	__le16 t_ccs;
> +	u8 reserved3[23];
> +
> +	/* vendor */
> +	__le16 vendor_revision;
> +	u8 vendor_specific[14];
> +	u8 reserved4[68];
> +	u8 ecc_max_correct_ability;
> +	u8 die_select_feature;
> +	u8 reserved5[4];
> +
> +	__le16 crc;
> +} __packed;
> +

Please use the nand_onfi_params definition (include/linux/mtd/onfi.h).

> +#endif /* __MICRON_H */


Regards,

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes
  2019-02-04 11:17 [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes Shivamurthy Shastri (sshivamurthy)
  2019-02-04 12:32 ` Emil Lenngren
  2019-02-04 18:01 ` Boris Brezillon
@ 2019-02-04 23:05 ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2019-02-04 23:05 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Chuanhong Guo, Boris Brezillon, Richard Weinberger, linux-kernel,
	Schrempf Frieder, Marek Vasut, Frieder Schrempf, linux-mtd,
	kbuild-all, Miquel Raynal, Brian Norris, David Woodhouse,
	Bean Huo (beanhuo)

[-- Attachment #1: Type: text/plain, Size: 6025 bytes --]

Hi Shivamurthy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mtd/nand/next]
[also build test WARNING on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Shivamurthy-Shastri-sshivamurthy/Support-parameter-page-and-Redesign-Micron-SPI-NAND/20190205-012740
base:   git://git.infradead.org/linux-mtd.git nand/next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

All warnings (new ones prefixed by >>):

>> drivers/mtd/nand/spi/micron.c:186:36: sparse: warning: incorrect type in assignment (different base types)
   drivers/mtd/nand/spi/micron.c:186:36: sparse:    expected unsigned int [assigned] pagesize
   drivers/mtd/nand/spi/micron.c:186:36: sparse:    got restricted __le32 [usertype] byte_per_page
   drivers/mtd/nand/spi/micron.c:187:35: sparse: warning: incorrect type in assignment (different base types)
   drivers/mtd/nand/spi/micron.c:187:35: sparse:    expected unsigned int [assigned] oobsize
   drivers/mtd/nand/spi/micron.c:187:35: sparse:    got restricted __le16 [usertype] spare_bytes_per_page
   drivers/mtd/nand/spi/micron.c:188:48: sparse: warning: incorrect type in assignment (different base types)
   drivers/mtd/nand/spi/micron.c:188:48: sparse:    expected unsigned int [assigned] pages_per_eraseblock
   drivers/mtd/nand/spi/micron.c:188:48: sparse:    got restricted __le32 [usertype] pages_per_block
>> drivers/mtd/nand/spi/micron.c:190:23: sparse: warning: restricted __le32 degrades to integer

sparse warnings: (new ones prefixed by >>)

   drivers/mtd/nand/spi/micron.c:186:36: sparse: warning: incorrect type in assignment (different base types)
>> drivers/mtd/nand/spi/micron.c:186:36: sparse:    expected unsigned int [assigned] pagesize
>> drivers/mtd/nand/spi/micron.c:186:36: sparse:    got restricted __le32 [usertype] byte_per_page
   drivers/mtd/nand/spi/micron.c:187:35: sparse: warning: incorrect type in assignment (different base types)
>> drivers/mtd/nand/spi/micron.c:187:35: sparse:    expected unsigned int [assigned] oobsize
>> drivers/mtd/nand/spi/micron.c:187:35: sparse:    got restricted __le16 [usertype] spare_bytes_per_page
   drivers/mtd/nand/spi/micron.c:188:48: sparse: warning: incorrect type in assignment (different base types)
>> drivers/mtd/nand/spi/micron.c:188:48: sparse:    expected unsigned int [assigned] pages_per_eraseblock
>> drivers/mtd/nand/spi/micron.c:188:48: sparse:    got restricted __le32 [usertype] pages_per_block
   drivers/mtd/nand/spi/micron.c:190:23: sparse: warning: restricted __le32 degrades to integer

vim +186 drivers/mtd/nand/spi/micron.c

   127	
   128	static int micron_spinand_detect(struct spinand_device *spinand)
   129	{
   130		struct spinand_info deviceinfo;
   131		struct micron_spinand_params *params;
   132		u8 *id = spinand->id.data;
   133		int ret, i;
   134	
   135		/*
   136		 * Micron SPI NAND read ID need a dummy byte,
   137		 * so the first byte in raw_id is dummy.
   138		 */
   139		if (id[1] != SPINAND_MFR_MICRON)
   140			return 0;
   141	
   142		params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
   143		if (!params)
   144			return -ENOMEM;
   145	
   146		ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE, params,
   147						  sizeof(*params) * 3);
   148		if (ret)
   149			goto free_params;
   150	
   151		for (i = 0; i < 3; i++) {
   152			if (spinand_crc16(0x4F4E, (u8 *)&params[i], 254) ==
   153					le16_to_cpu(params->crc)) {
   154				if (i)
   155					memcpy(params, &params[i], sizeof(*params));
   156				break;
   157			}
   158		}
   159	
   160		if (i == 3) {
   161			const void *srcbufs[3] = {params, params + 1, params + 2};
   162	
   163			pr_warn("No valid parameter page, trying bit-wise majority to recover it\n");
   164			bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
   165					  sizeof(*params));
   166	
   167			if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
   168					le16_to_cpu(params->crc)) {
   169				pr_err("Parameter page recovery failed, aborting\n");
   170				goto free_params;
   171			}
   172		}
   173	
   174		params->model[sizeof(params->model) - 1] = 0;
   175		strim(params->model);
   176	
   177		deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
   178		if (!deviceinfo.model) {
   179			ret = -ENOMEM;
   180			goto free_params;
   181		}
   182	
   183		deviceinfo.devid = id[2];
   184		deviceinfo.flags = 0;
   185		deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
 > 186		deviceinfo.memorg.pagesize = params->byte_per_page;
 > 187		deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
 > 188		deviceinfo.memorg.pages_per_eraseblock = params->pages_per_block;
   189		deviceinfo.memorg.eraseblocks_per_lun =
 > 190			params->blocks_per_lun * params->lun_count;
   191		deviceinfo.memorg.planes_per_lun = params->lun_count;
   192		deviceinfo.memorg.luns_per_target = 1;
   193		deviceinfo.memorg.ntargets = 1;
   194		deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
   195		deviceinfo.eccreq.step_size = 512;
   196		deviceinfo.eccinfo.get_status = ecc_get_status;
   197		deviceinfo.eccinfo.ooblayout = &ooblayout;
   198		deviceinfo.op_variants.read_cache = &read_cache_variants;
   199		deviceinfo.op_variants.write_cache = &write_cache_variants;
   200		deviceinfo.op_variants.update_cache = &update_cache_variants;
   201	
   202		ret = spinand_match_and_init(spinand, &deviceinfo,
   203					     1, id[2]);
   204		if (ret)
   205			goto free_model;
   206	
   207		kfree(params);
   208	
   209		return 1;
   210	
   211	free_model:
   212		kfree(deviceinfo.model);
   213	free_params:
   214		kfree(params);
   215	
   216		return ret;
   217	}
   218	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67299 bytes --]

[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes
  2019-02-04 18:01 ` Boris Brezillon
@ 2019-03-04 13:29   ` Shivamurthy Shastri (sshivamurthy)
  2019-03-04 18:13     ` Miquel Raynal
  0 siblings, 1 reply; 6+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-03-04 13:29 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Richard Weinberger, linux-kernel, Schrempf Frieder, Marek Vasut,
	Frieder Schrempf, linux-mtd, Miquel Raynal, Brian Norris,
	Chuanhong Guo, David Woodhouse, Bean Huo (beanhuo)

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon <bbrezillon@kernel.org>
> Sent: Monday, February 4, 2019 7:02 PM
> To: Shivamurthy Shastri (sshivamurthy) <sshivamurthy@micron.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Chuanhong Guo
> <gch981213@gmail.com>; Richard Weinberger <richard@nod.at>; Schrempf
> Frieder <frieder.schrempf@kontron.De>; Marek Vasut
> <marek.vasut@gmail.com>; Frieder Schrempf
> <frieder.schrempf@exceet.de>; Brian Norris
> <computersforpeace@gmail.com>; David Woodhouse
> <dwmw2@infradead.org>; Bean Huo (beanhuo) <beanhuo@micron.com>
> Subject: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron
> SPI NAND flashes
> 
> Hi Shivamurthy,
> 
> On Mon, 4 Feb 2019 11:17:51 +0000
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote:
> 
> > Driver is redesigned using parameter page to support all the Micron
> > SPI NAND flashes.
> 
> Do all Micron SPI NANDs really expose a valid ONFI param page? If
> that's not the case, then relying on ONFi parsing only sounds like a
> bad idea.

Micron SPI NAND datasheet does not confirm to be as ONFI standard.
However, they all expose parameter page, which I used for development.

> 
> >
> > Parameter page of Micron flashes is similar to ONFI parameter table and
> > functionality is same, so copied some of the common functions like crc16
> > and bit_wise_majority from nand_onfi.c.
> 
> Most of the code is generic and does not depend on the spinand layer,
> plus, we already have ONFI param page parsing code in
> drivers/mtd/nand/raw/ which you're intentionally duplicating in a
> version that will not be re-usable by the raw NAND layer even after
> converting it to use the generic NAND layer.
> 
> Please move ONFi parsing code to drivers/mtd/nand/onfi.c and make it
> generic.

As I said before, it is not compliant to ONFI standard, I think it is better not 
to make it generic.

> 
> >
> > This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD,
> MT29F8G01ADXFD,
> > MT29F1G01ABXFD.
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > Reviewed-by: Bean Huo <beanhuo@micron.com>
> 
> I wish this code review had happened publicly.
> 
> > ---
> >  drivers/mtd/nand/spi/micron.c | 172 +++++++++++++++++++++++++++--
> -----
> >  drivers/mtd/nand/spi/micron.h |  83 ++++++++++++++++
> >  2 files changed, 221 insertions(+), 34 deletions(-)
> >  create mode 100644 drivers/mtd/nand/spi/micron.h
> >
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 9c4381d6847b..c9c53fd0aa01 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -10,13 +10,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/mtd/spinand.h>
> >
> > -#define SPINAND_MFR_MICRON		0x2c
> > -
> > -#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
> > -#define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
> > -#define MICRON_STATUS_ECC_1TO3_BITFLIPS	(1 << 4)
> > -#define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> > -#define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
> > +#include "micron.h"
> >
> >  static SPINAND_OP_VARIANTS(read_cache_variants,
> >  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2,
> NULL, 0),
> > @@ -34,38 +28,38 @@ static
> SPINAND_OP_VARIANTS(update_cache_variants,
> >  		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> >  		SPINAND_PROG_LOAD(false, 0, NULL, 0));
> >
> > -static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int
> section,
> > -					struct mtd_oob_region *region)
> > +static int ooblayout_ecc(struct mtd_info *mtd, int section,
> > +			 struct mtd_oob_region *region)
> >  {
> >  	if (section)
> >  		return -ERANGE;
> >
> > -	region->offset = 64;
> > -	region->length = 64;
> > +	region->offset = mtd->oobsize / 2;
> > +	region->length = mtd->oobsize / 2;
> >
> >  	return 0;
> >  }
> >
> > -static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int
> section,
> > -					 struct mtd_oob_region *region)
> > +static int ooblayout_free(struct mtd_info *mtd, int section,
> > +			  struct mtd_oob_region *region)
> >  {
> >  	if (section)
> >  		return -ERANGE;
> >
> >  	/* Reserve 2 bytes for the BBM. */
> >  	region->offset = 2;
> > -	region->length = 62;
> > +	region->length = (mtd->oobsize / 2) - 2;
> >
> >  	return 0;
> >  }
> >
> > -static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
> > -	.ecc = mt29f2g01abagd_ooblayout_ecc,
> > -	.free = mt29f2g01abagd_ooblayout_free,
> > +static const struct mtd_ooblayout_ops ooblayout = {
> > +	.ecc = ooblayout_ecc,
> > +	.free = ooblayout_free,
> >  };
> >
> > -static int mt29f2g01abagd_ecc_get_status(struct spinand_device
> *spinand,
> > -					 u8 status)
> > +static int ecc_get_status(struct spinand_device *spinand,
> > +			  u8 status)
> >  {
> >  	switch (status & MICRON_STATUS_ECC_MASK) {
> >  	case STATUS_ECC_NO_BITFLIPS:
> > @@ -90,22 +84,53 @@ static int mt29f2g01abagd_ecc_get_status(struct
> spinand_device *spinand,
> >  	return -EINVAL;
> >  }
> >
> > -static const struct spinand_info micron_spinand_table[] = {
> > -	SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> > -		     NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
> > -		     NAND_ECCREQ(8, 512),
> > -		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > -					      &write_cache_variants,
> > -					      &update_cache_variants),
> > -		     0,
> > -		     SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
> > -				     mt29f2g01abagd_ecc_get_status)),
> > -};
> > +static u16 spinand_crc16(u16 crc, u8 const *p, size_t len)
> > +{
> > +	int i;
> > +
> > +	while (len--) {
> > +		crc ^= *p++ << 8;
> > +		for (i = 0; i < 8; i++)
> > +			crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
> > +	}
> > +
> > +	return crc;
> > +}
> > +
> > +static void bit_wise_majority(const void **srcbufs,
> > +			      unsigned int nsrcbufs,
> > +				   void *dstbuf,
> > +				   unsigned int bufsize)
> > +{
> > +	int i, j, k;
> > +
> > +	for (i = 0; i < bufsize; i++) {
> > +		u8 val = 0;
> > +
> > +		for (j = 0; j < 8; j++) {
> > +			unsigned int cnt = 0;
> > +
> > +			for (k = 0; k < nsrcbufs; k++) {
> > +				const u8 *srcbuf = srcbufs[k];
> > +
> > +				if (srcbuf[i] & BIT(j))
> > +					cnt++;
> > +			}
> > +
> > +			if (cnt > nsrcbufs / 2)
> > +				val |= BIT(j);
> > +		}
> > +
> > +		((u8 *)dstbuf)[i] = val;
> > +	}
> > +}
> >
> >  static int micron_spinand_detect(struct spinand_device *spinand)
> >  {
> > +	struct spinand_info deviceinfo;
> > +	struct micron_spinand_params *params;
> >  	u8 *id = spinand->id.data;
> > -	int ret;
> > +	int ret, i;
> >
> >  	/*
> >  	 * Micron SPI NAND read ID need a dummy byte,
> > @@ -114,16 +139,95 @@ static int micron_spinand_detect(struct
> spinand_device *spinand)
> >  	if (id[1] != SPINAND_MFR_MICRON)
> >  		return 0;
> >
> > -	ret = spinand_match_and_init(spinand, micron_spinand_table,
> > -				     ARRAY_SIZE(micron_spinand_table),
> id[2]);
> > +	params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
> > +	if (!params)
> > +		return -ENOMEM;
> > +
> > +	ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE,
> params,
> > +					  sizeof(*params) * 3);
> >  	if (ret)
> > -		return ret;
> > +		goto free_params;
> > +
> > +	for (i = 0; i < 3; i++) {
> > +		if (spinand_crc16(0x4F4E, (u8 *)&params[i], 254) ==
> > +				le16_to_cpu(params->crc)) {
> > +			if (i)
> > +				memcpy(params, &params[i],
> sizeof(*params));
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (i == 3) {
> > +		const void *srcbufs[3] = {params, params + 1, params + 2};
> > +
> > +		pr_warn("No valid parameter page, trying bit-wise majority
> to recover it\n");
> > +		bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
> > +				  sizeof(*params));
> > +
> > +		if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
> > +				le16_to_cpu(params->crc)) {
> > +			pr_err("Parameter page recovery failed,
> aborting\n");
> > +			goto free_params;
> > +		}
> > +	}
> > +
> > +	params->model[sizeof(params->model) - 1] = 0;
> > +	strim(params->model);
> > +
> > +	deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
> > +	if (!deviceinfo.model) {
> > +		ret = -ENOMEM;
> > +		goto free_params;
> > +	}
> > +
> > +	deviceinfo.devid = id[2];
> > +	deviceinfo.flags = 0;
> > +	deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
> > +	deviceinfo.memorg.pagesize = params->byte_per_page;
> > +	deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
> > +	deviceinfo.memorg.pages_per_eraseblock = params-
> >pages_per_block;
> > +	deviceinfo.memorg.eraseblocks_per_lun =
> > +		params->blocks_per_lun * params->lun_count;
> > +	deviceinfo.memorg.planes_per_lun = params->lun_count;
> 
> As pointed by Emil, this is wrong, params->lun_count should be used to
> fill luns_per_target. ->planes_per_lun should be extracted from
> ->interleaved_bits.

It is my bad, I will correct it.
However, Micron SPI NAND's use vendor specific area ->vendor_specific[0] .

> 
> > +	deviceinfo.memorg.luns_per_target = 1;
> > +	deviceinfo.memorg.ntargets = 1;
> > +	deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
> > +	deviceinfo.eccreq.step_size = 512;
> > +	deviceinfo.eccinfo.get_status = ecc_get_status;
> > +	deviceinfo.eccinfo.ooblayout = &ooblayout;
> 
> Are all devices really using the same get_status method and the same
> layout. Sounds risky to me to assume this is the case.
> 
> > +	deviceinfo.op_variants.read_cache = &read_cache_variants;
> > +	deviceinfo.op_variants.write_cache = &write_cache_variants;
> > +	deviceinfo.op_variants.update_cache = &update_cache_variants;
> > +
> > +	ret = spinand_match_and_init(spinand, &deviceinfo,
> > +				     1, id[2]);
> 
> Please don't abuse the spinand_match_and_init() function. Fill the
> nand_device object directly instead of creating a temporary spinand_info
> instance with the expected id.
> 
> > +	if (ret)
> > +		goto free_model;
> > +
> > +	kfree(params);
> >
> >  	return 1;
> > +
> > +free_model:
> > +	kfree(deviceinfo.model);
> > +free_params:
> > +	kfree(params);
> > +
> > +	return ret;
> > +}
> > +
> > +static int micron_spinand_init(struct spinand_device *spinand)
> > +{
> > +	/*
> > +	 * Some of the Micron flashes enable this BIT by default,
> > +	 * and there is a chance of read failure due to this.
> > +	 */
> > +	return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> >  }
> >
> >  static const struct spinand_manufacturer_ops
> micron_spinand_manuf_ops = {
> >  	.detect = micron_spinand_detect,
> > +	.init = micron_spinand_init,
> >  };
> >
> >  const struct spinand_manufacturer micron_spinand_manufacturer = {
> > diff --git a/drivers/mtd/nand/spi/micron.h
> b/drivers/mtd/nand/spi/micron.h
> > new file mode 100644
> > index 000000000000..c2cf3bee6f7e
> > --- /dev/null
> > +++ b/drivers/mtd/nand/spi/micron.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 Micron Technology, Inc.
> > + *
> > + * Authors:
> > + *	Shivamurthy Shastri <sshivamurthy@micron.com>
> > + */
> > +
> > +#ifndef __MICRON_H
> > +#define __MICRON_H
> > +
> > +#define SPINAND_MFR_MICRON		0x2c
> > +
> > +#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
> > +#define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
> > +#define MICRON_STATUS_ECC_1TO3_BITFLIPS	BIT(4)
> > +#define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> > +#define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
> > +
> > +#define UNIQUE_ID_PAGE	0x00
> > +#define PARAMETER_PAGE	0x01
> > +
> > +/*
> > + * Micron SPI NAND has parameter table similar to ONFI
> > + */
> > +struct micron_spinand_params {
> > +	/* rev info and features block */
> > +	u8 sig[4];
> > +	__le16 revision;
> > +	__le16 features;
> > +	__le16 opt_cmd;
> > +	u8 reserved0[22];
> > +
> > +	/* manufacturer information block */
> > +	char manufacturer[12];
> > +	char model[20];
> > +	u8 manufact_id;
> > +	__le16 date_code;
> > +	u8 reserved1[13];
> > +
> > +	/* memory organization block */
> > +	__le32 byte_per_page;
> > +	__le16 spare_bytes_per_page;
> > +	__le32 data_bytes_per_ppage;
> > +	__le16 spare_bytes_per_ppage;
> > +	__le32 pages_per_block;
> > +	__le32 blocks_per_lun;
> > +	u8 lun_count;
> > +	u8 addr_cycles;
> > +	u8 bits_per_cell;
> > +	__le16 bb_per_lun;
> > +	__le16 block_endurance;
> > +	u8 guaranteed_good_blocks;
> > +	__le16 guaranteed_block_endurance;
> > +	u8 programs_per_page;
> > +	u8 ppage_attr;
> > +	u8 ecc_bits;
> > +	u8 interleaved_bits;
> > +	u8 interleaved_ops;
> > +	u8 reserved2[13];
> > +
> > +	/* electrical parameter block */
> > +	u8 io_pin_capacitance_max;
> > +	__le16 async_timing_mode;
> > +	__le16 program_cache_timing_mode;
> > +	__le16 t_prog;
> > +	__le16 t_bers;
> > +	__le16 t_r;
> > +	__le16 t_ccs;
> > +	u8 reserved3[23];
> > +
> > +	/* vendor */
> > +	__le16 vendor_revision;
> > +	u8 vendor_specific[14];
> > +	u8 reserved4[68];
> > +	u8 ecc_max_correct_ability;
> > +	u8 die_select_feature;
> > +	u8 reserved5[4];
> > +
> > +	__le16 crc;
> > +} __packed;
> > +
> 
> Please use the nand_onfi_params definition (include/linux/mtd/onfi.h).
> 
> > +#endif /* __MICRON_H */
> 
> 
> Regards,
> 
> Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes
  2019-03-04 13:29   ` [EXT] " Shivamurthy Shastri (sshivamurthy)
@ 2019-03-04 18:13     ` Miquel Raynal
  0 siblings, 0 replies; 6+ messages in thread
From: Miquel Raynal @ 2019-03-04 18:13 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Boris Brezillon, Richard Weinberger, linux-kernel,
	Schrempf Frieder, Marek Vasut, Frieder Schrempf, linux-mtd,
	Brian Norris, Chuanhong Guo, David Woodhouse, Bean Huo (beanhuo)

Hello,

"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
Mon, 4 Mar 2019 13:29:21 +0000:

> Hi Boris,
> 
> > -----Original Message-----
> > From: Boris Brezillon <bbrezillon@kernel.org>
> > Sent: Monday, February 4, 2019 7:02 PM
> > To: Shivamurthy Shastri (sshivamurthy) <sshivamurthy@micron.com>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; linux-
> > mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Chuanhong Guo
> > <gch981213@gmail.com>; Richard Weinberger <richard@nod.at>; Schrempf
> > Frieder <frieder.schrempf@kontron.De>; Marek Vasut
> > <marek.vasut@gmail.com>; Frieder Schrempf
> > <frieder.schrempf@exceet.de>; Brian Norris
> > <computersforpeace@gmail.com>; David Woodhouse
> > <dwmw2@infradead.org>; Bean Huo (beanhuo) <beanhuo@micron.com>
> > Subject: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron
> > SPI NAND flashes
> > 
> > Hi Shivamurthy,
> > 
> > On Mon, 4 Feb 2019 11:17:51 +0000
> > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote:
> >   
> > > Driver is redesigned using parameter page to support all the Micron
> > > SPI NAND flashes.  
> > 
> > Do all Micron SPI NANDs really expose a valid ONFI param page? If
> > that's not the case, then relying on ONFi parsing only sounds like a
> > bad idea.  
> 
> Micron SPI NAND datasheet does not confirm to be as ONFI standard.
> However, they all expose parameter page, which I used for development.
> 
> >   
> > >
> > > Parameter page of Micron flashes is similar to ONFI parameter table and
> > > functionality is same, so copied some of the common functions like crc16
> > > and bit_wise_majority from nand_onfi.c.  
> > 
> > Most of the code is generic and does not depend on the spinand layer,
> > plus, we already have ONFI param page parsing code in
> > drivers/mtd/nand/raw/ which you're intentionally duplicating in a
> > version that will not be re-usable by the raw NAND layer even after
> > converting it to use the generic NAND layer.
> > 
> > Please move ONFi parsing code to drivers/mtd/nand/onfi.c and make it
> > generic.  
> 
> As I said before, it is not compliant to ONFI standard, I think it is better not 
> to make it generic.

For what I see it is too similar to copy all that code. I agree with
Boris. If there are some specificities that are not in the ONFI
standard you can do some late changes in the parameter page from
Micron's driver I guess?


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-03-04 18:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 11:17 [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes Shivamurthy Shastri (sshivamurthy)
2019-02-04 12:32 ` Emil Lenngren
2019-02-04 18:01 ` Boris Brezillon
2019-03-04 13:29   ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-03-04 18:13     ` Miquel Raynal
2019-02-04 23:05 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).