linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Add new series Micron SPI NAND devices
@ 2020-01-19 14:54 shiva.linuxworks
  2020-01-19 14:54 ` [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names shiva.linuxworks
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: shiva.linuxworks @ 2020-01-19 14:54 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Boris Brezillon, Frieder Schrempf, linux-mtd, linux-kernel
  Cc: Shivamurthy Shastri

From: Shivamurthy Shastri <sshivamurthy@micron.com>

This patchset is for the new series of Micron SPI NAND devices, and the
following links are their datasheets.

M78A:
[1] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m78a_1gb_3v_nand_spi.pdf
[2] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m78a_1gb_1_8v_nand_spi.pdf

M79A:
[3] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m79a_2gb_1_8v_nand_spi.pdf
[4] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m79a_ddp_4gb_3v_nand_spi.pdf

M70A:
[5] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m70a_4gb_3v_nand_spi.pdf
[6] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m70a_4gb_1_8v_nand_spi.pdf
[7] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m70a_ddp_8gb_3v_nand_spi.pdf
[8] https://www.micron.com/~/media/documents/products/data-sheet/nand-flash/70-series/m70a_ddp_8gb_1_8v_nand_spi.pdf

Changes since v1:
-----------------

1. The patch split into multiple patches.
2. Added comments for selecting the die.

Shivamurthy Shastri (4):
  mtd: spinand: Generalize the OOB layout structure and function names
  mtd: spinand: Add new Micron SPI NAND devices
  mtd: spinand: Add M70A series Micron SPI NAND devices
  mtd: spinand: Add new Micron SPI NAND devices with multiple dies

 drivers/mtd/nand/spi/micron.c | 140 ++++++++++++++++++++++++++++++----
 1 file changed, 126 insertions(+), 14 deletions(-)

-- 
2.17.1


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

* [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names
  2020-01-19 14:54 [PATCH 0/4] Add new series Micron SPI NAND devices shiva.linuxworks
@ 2020-01-19 14:54 ` shiva.linuxworks
  2020-01-30  8:30   ` Miquel Raynal
  2020-01-19 14:54 ` [PATCH 2/4] mtd: spinand: Add new Micron SPI NAND devices shiva.linuxworks
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: shiva.linuxworks @ 2020-01-19 14:54 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Boris Brezillon, Frieder Schrempf, linux-mtd, linux-kernel
  Cc: Shivamurthy Shastri

From: Shivamurthy Shastri <sshivamurthy@micron.com>

In order to add new Micron SPI NAND devices, we generalized the OOB
layout structure and function names.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/spi/micron.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 7d7b1f7fcf71..c028d0d7e236 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -34,38 +34,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 micron_8_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 micron_8_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 micron_8_ooblayout = {
+	.ecc = micron_8_ooblayout_ecc,
+	.free = micron_8_ooblayout_free,
 };
 
-static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
-					 u8 status)
+static int micron_8_ecc_get_status(struct spinand_device *spinand,
+				   u8 status)
 {
 	switch (status & MICRON_STATUS_ECC_MASK) {
 	case STATUS_ECC_NO_BITFLIPS:
@@ -98,8 +98,8 @@ static const struct spinand_info micron_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
-				     mt29f2g01abagd_ecc_get_status)),
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status)),
 };
 
 static int micron_spinand_detect(struct spinand_device *spinand)
-- 
2.17.1


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

* [PATCH 2/4] mtd: spinand: Add new Micron SPI NAND devices
  2020-01-19 14:54 [PATCH 0/4] Add new series Micron SPI NAND devices shiva.linuxworks
  2020-01-19 14:54 ` [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names shiva.linuxworks
@ 2020-01-19 14:54 ` shiva.linuxworks
  2020-01-19 14:54 ` [PATCH 3/4] mtd: spinand: Add M70A series " shiva.linuxworks
  2020-01-19 14:54 ` [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies shiva.linuxworks
  3 siblings, 0 replies; 15+ messages in thread
From: shiva.linuxworks @ 2020-01-19 14:54 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Boris Brezillon, Frieder Schrempf, linux-mtd, linux-kernel
  Cc: Shivamurthy Shastri

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Add device table for M79A and M78A series Micron SPI NAND devices.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/spi/micron.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index c028d0d7e236..5fd1f921ef12 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -91,6 +91,7 @@ static int micron_8_ecc_get_status(struct spinand_device *spinand,
 }
 
 static const struct spinand_info micron_spinand_table[] = {
+	/* M79A 2Gb 3.3V */
 	SPINAND_INFO("MT29F2G01ABAGD", 0x24,
 		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
 		     NAND_ECCREQ(8, 512),
@@ -100,6 +101,36 @@ static const struct spinand_info micron_spinand_table[] = {
 		     0,
 		     SPINAND_ECCINFO(&micron_8_ooblayout,
 				     micron_8_ecc_get_status)),
+	/* M79A 2Gb 1.8V */
+	SPINAND_INFO("MT29F2G01ABBGD", 0x25,
+		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status)),
+	/* M78A 1Gb 3.3V */
+	SPINAND_INFO("MT29F1G01ABAFD", 0x14,
+		     NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status)),
+	/* M78A 1Gb 1.8V */
+	SPINAND_INFO("MT29F1G01ABAFD", 0x15,
+		     NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status)),
 };
 
 static int micron_spinand_detect(struct spinand_device *spinand)
-- 
2.17.1


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

* [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-19 14:54 [PATCH 0/4] Add new series Micron SPI NAND devices shiva.linuxworks
  2020-01-19 14:54 ` [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names shiva.linuxworks
  2020-01-19 14:54 ` [PATCH 2/4] mtd: spinand: Add new Micron SPI NAND devices shiva.linuxworks
@ 2020-01-19 14:54 ` shiva.linuxworks
  2020-01-20 10:16   ` Miquel Raynal
  2020-01-19 14:54 ` [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies shiva.linuxworks
  3 siblings, 1 reply; 15+ messages in thread
From: shiva.linuxworks @ 2020-01-19 14:54 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Boris Brezillon, Frieder Schrempf, linux-mtd, linux-kernel
  Cc: Shivamurthy Shastri

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Add device table for M70A series Micron SPI NAND devices.

While at it, disable the Continuous Read feature which is enabled by
default.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/spi/micron.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 5fd1f921ef12..45fc37c58f8a 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -131,6 +131,26 @@ static const struct spinand_info micron_spinand_table[] = {
 		     0,
 		     SPINAND_ECCINFO(&micron_8_ooblayout,
 				     micron_8_ecc_get_status)),
+	/* M70A 4Gb 3.3V */
+	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
+		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status)),
+	/* M70A 4Gb 1.8V */
+	SPINAND_INFO("MT29F4G01ABBFD", 0x35,
+		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status)),
 };
 
 static int micron_spinand_detect(struct spinand_device *spinand)
@@ -153,8 +173,19 @@ static int micron_spinand_detect(struct spinand_device *spinand)
 	return 1;
 }
 
+static int micron_spinand_init(struct spinand_device *spinand)
+{
+	/*
+	 * M70A device series enable Continuous Read feature at Power-up,
+	 * which is not supported. Disable this bit to avoid any possible
+	 * failure.
+	 */
+	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 = {
-- 
2.17.1


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

* [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies
  2020-01-19 14:54 [PATCH 0/4] Add new series Micron SPI NAND devices shiva.linuxworks
                   ` (2 preceding siblings ...)
  2020-01-19 14:54 ` [PATCH 3/4] mtd: spinand: Add M70A series " shiva.linuxworks
@ 2020-01-19 14:54 ` shiva.linuxworks
  2020-01-20 10:22   ` Miquel Raynal
  3 siblings, 1 reply; 15+ messages in thread
From: shiva.linuxworks @ 2020-01-19 14:54 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Boris Brezillon, Frieder Schrempf, linux-mtd, linux-kernel
  Cc: Shivamurthy Shastri

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Add device table for new Micron SPI NAND devices, which have multiple
dies. While at it, add support to select the die.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/spi/micron.c | 50 +++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 45fc37c58f8a..03b486843210 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -18,6 +18,8 @@
 #define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
 #define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
 
+#define MICRON_DIE_SELECTION_BIT	6
+
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
@@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops micron_8_ooblayout = {
 	.free = micron_8_ooblayout_free,
 };
 
+static int micron_select_target(struct spinand_device *spinand,
+				unsigned int target)
+{
+	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
+						      spinand->scratchbuf);
+
+	/*
+	 * As per datasheet, die selection is done by the 6th bit of Die
+	 * Select Register (Address 0xD0).
+	 */
+	*spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;
+
+	return spi_mem_exec_op(spinand->spimem, &op);
+}
+
 static int micron_8_ecc_get_status(struct spinand_device *spinand,
 				   u8 status)
 {
@@ -131,6 +148,17 @@ static const struct spinand_info micron_spinand_table[] = {
 		     0,
 		     SPINAND_ECCINFO(&micron_8_ooblayout,
 				     micron_8_ecc_get_status)),
+	/* M79A 4Gb 3.3V */
+	SPINAND_INFO("MT29F4G01ADAGD", 0x36,
+		     NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status),
+		     SPINAND_SELECT_TARGET(micron_select_target)),
 	/* M70A 4Gb 3.3V */
 	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
 		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
@@ -151,6 +179,28 @@ static const struct spinand_info micron_spinand_table[] = {
 		     0,
 		     SPINAND_ECCINFO(&micron_8_ooblayout,
 				     micron_8_ecc_get_status)),
+	/* M70A 8Gb 3.3V */
+	SPINAND_INFO("MT29F8G01ADAFD", 0x46,
+		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status),
+		     SPINAND_SELECT_TARGET(micron_select_target)),
+	/* M70A 8Gb 1.8V */
+	SPINAND_INFO("MT29F8G01ADBFD", 0x47,
+		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
+		     NAND_ECCREQ(8, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&micron_8_ooblayout,
+				     micron_8_ecc_get_status),
+		     SPINAND_SELECT_TARGET(micron_select_target)),
 };
 
 static int micron_spinand_detect(struct spinand_device *spinand)
-- 
2.17.1


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

* Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-19 14:54 ` [PATCH 3/4] mtd: spinand: Add M70A series " shiva.linuxworks
@ 2020-01-20 10:16   ` Miquel Raynal
  2020-01-21 12:23     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 15+ messages in thread
From: Miquel Raynal @ 2020-01-20 10:16 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Shiva,

This is remark common to the four patches: you miss the 'v2' prefix in
the object.

shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:31 +0100:

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> Add device table for M70A series Micron SPI NAND devices.
> 
> While at it, disable the Continuous Read feature which is enabled by
> default.

Can you please give us more detail on why this is an issue?

Shall we backport it to stable?

As a rule of thumb, when you start a sentence by "while at it" in a
commit message and this is not a trivial change : split the patch,
please. Unless this is really related and in this case explain how and
why in the commit message.

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/spi/micron.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 5fd1f921ef12..45fc37c58f8a 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -131,6 +131,26 @@ static const struct spinand_info micron_spinand_table[] = {
>  		     0,
>  		     SPINAND_ECCINFO(&micron_8_ooblayout,
>  				     micron_8_ecc_get_status)),
> +	/* M70A 4Gb 3.3V */
> +	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> +		     NAND_ECCREQ(8, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     0,
> +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> +				     micron_8_ecc_get_status)),
> +	/* M70A 4Gb 1.8V */
> +	SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> +		     NAND_ECCREQ(8, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     0,
> +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> +				     micron_8_ecc_get_status)),
>  };
>  
>  static int micron_spinand_detect(struct spinand_device *spinand)
> @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct spinand_device *spinand)
>  	return 1;
>  }
>  
> +static int micron_spinand_init(struct spinand_device *spinand)
> +{
> +	/*
> +	 * M70A device series enable Continuous Read feature at Power-up,
> +	 * which is not supported. Disable this bit to avoid any possible
> +	 * failure.
> +	 */
> +	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 = {

Thanks,
Miquèl

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

* Re: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies
  2020-01-19 14:54 ` [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies shiva.linuxworks
@ 2020-01-20 10:22   ` Miquel Raynal
  2020-01-21 12:23     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 15+ messages in thread
From: Miquel Raynal @ 2020-01-20 10:22 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:32 +0100:

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> Add device table for new Micron SPI NAND devices, which have multiple
> dies. While at it, add support to select the die.

Same comment as in 3/4.

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/spi/micron.c | 50 +++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 45fc37c58f8a..03b486843210 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -18,6 +18,8 @@
>  #define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
>  #define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
>  
> +#define MICRON_DIE_SELECTION_BIT	6
> +
>  static SPINAND_OP_VARIANTS(read_cache_variants,
>  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> @@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops micron_8_ooblayout = {
>  	.free = micron_8_ooblayout_free,
>  };
>  
> +static int micron_select_target(struct spinand_device *spinand,
> +				unsigned int target)
> +{
> +	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> +						      spinand->scratchbuf);
> +
> +	/*
> +	 * As per datasheet, die selection is done by the 6th bit of Die
> +	 * Select Register (Address 0xD0).
> +	 */

I would put this comment close to the macro definition.

> +	*spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;

Either target is or or 1 and you can use the BIT macro, or you suppose
it can go higher and the _BIT suffix does not fit. _SHIFT would work
and creating a macro directly would be even better.

> +
> +	return spi_mem_exec_op(spinand->spimem, &op);
> +}
> +

Where is this function used?

>  static int micron_8_ecc_get_status(struct spinand_device *spinand,
>  				   u8 status)
>  {
> @@ -131,6 +148,17 @@ static const struct spinand_info micron_spinand_table[] = {
>  		     0,
>  		     SPINAND_ECCINFO(&micron_8_ooblayout,
>  				     micron_8_ecc_get_status)),
> +	/* M79A 4Gb 3.3V */
> +	SPINAND_INFO("MT29F4G01ADAGD", 0x36,
> +		     NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> +		     NAND_ECCREQ(8, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     0,
> +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> +				     micron_8_ecc_get_status),
> +		     SPINAND_SELECT_TARGET(micron_select_target)),
>  	/* M70A 4Gb 3.3V */
>  	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
>  		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> @@ -151,6 +179,28 @@ static const struct spinand_info micron_spinand_table[] = {
>  		     0,
>  		     SPINAND_ECCINFO(&micron_8_ooblayout,
>  				     micron_8_ecc_get_status)),
> +	/* M70A 8Gb 3.3V */
> +	SPINAND_INFO("MT29F8G01ADAFD", 0x46,
> +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> +		     NAND_ECCREQ(8, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     0,
> +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> +				     micron_8_ecc_get_status),
> +		     SPINAND_SELECT_TARGET(micron_select_target)),
> +	/* M70A 8Gb 1.8V */
> +	SPINAND_INFO("MT29F8G01ADBFD", 0x47,
> +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> +		     NAND_ECCREQ(8, 512),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     0,
> +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> +				     micron_8_ecc_get_status),
> +		     SPINAND_SELECT_TARGET(micron_select_target)),
>  };
>  
>  static int micron_spinand_detect(struct spinand_device *spinand)




Thanks,
Miquèl

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

* RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-20 10:16   ` Miquel Raynal
@ 2020-01-21 12:23     ` Shivamurthy Shastri (sshivamurthy)
  2020-01-21 13:40       ` Miquel Raynal
  0 siblings, 1 reply; 15+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2020-01-21 12:23 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Miquel,

> 
> Hi Shiva,
> 
> This is remark common to the four patches: you miss the 'v2' prefix in
> the object.
> 

Sorry for this mistake.
I recognized this after sending out the patches.

> shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> 
> > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> >
> > Add device table for M70A series Micron SPI NAND devices.
> >
> > While at it, disable the Continuous Read feature which is enabled by
> > default.
> 
> Can you please give us more detail on why this is an issue?

"Continuous Read" is the new feature added by the Micron for 
M70A series devices. If this feature is enabled, the READ command 
doesn't output the OOB area. The following short description
describes this feature.

Description:
If the Continuous Read feature is enabled, the device provides 
the capability to read the whole block with a single command.
However, the read command doesn't output the OOB area.

Read command behavior (if Continuous Read enabled):
The READ CACHE command doesn't require the starting column address.
The device always output the data starting from the first column of the
cache register, and once the end of the cache register reached, the data
output continues through the next page. With the continuous read mode,
it is possible to read out the entire block using a single READ command, and
once the end of the block reached, the output pins become High-Z state.

> 
> Shall we backport it to stable?

This is not a bug fix and applicable only to M70A series devices, there is no
need to backport.
(FYI, the previously enabled device was M79A series)

> 
> As a rule of thumb, when you start a sentence by "while at it" in a
> commit message and this is not a trivial change : split the patch,
> please. Unless this is really related and in this case explain how and
> why in the commit message.

Okay, I will explain in my next version.

> 
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > ---
> >  drivers/mtd/nand/spi/micron.c | 31
> +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 5fd1f921ef12..45fc37c58f8a 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -131,6 +131,26 @@ static const struct spinand_info
> micron_spinand_table[] = {
> >  		     0,
> >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> >  				     micron_8_ecc_get_status)),
> > +	/* M70A 4Gb 3.3V */
> > +	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > +		     NAND_ECCREQ(8, 512),
> > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > +					      &write_cache_variants,
> > +					      &update_cache_variants),
> > +		     0,
> > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > +				     micron_8_ecc_get_status)),
> > +	/* M70A 4Gb 1.8V */
> > +	SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > +		     NAND_ECCREQ(8, 512),
> > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > +					      &write_cache_variants,
> > +					      &update_cache_variants),
> > +		     0,
> > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > +				     micron_8_ecc_get_status)),
> >  };
> >
> >  static int micron_spinand_detect(struct spinand_device *spinand)
> > @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct
> spinand_device *spinand)
> >  	return 1;
> >  }
> >
> > +static int micron_spinand_init(struct spinand_device *spinand)
> > +{
> > +	/*
> > +	 * M70A device series enable Continuous Read feature at Power-up,
> > +	 * which is not supported. Disable this bit to avoid any possible
> > +	 * failure.
> > +	 */
> > +	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 = {
> 
> Thanks,
> Miquèl

Thanks,
Shiva

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

* RE: [EXT] Re: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies
  2020-01-20 10:22   ` Miquel Raynal
@ 2020-01-21 12:23     ` Shivamurthy Shastri (sshivamurthy)
  2020-01-21 13:44       ` Miquel Raynal
  0 siblings, 1 reply; 15+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2020-01-21 12:23 UTC (permalink / raw)
  To: 'Miquel Raynal'
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, shiva.linuxworks

Hi Miquel,

> 
> Hi Shiva,
> 
> shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:32 +0100:
> 
> > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> >
> > Add device table for new Micron SPI NAND devices, which have multiple
> > dies. While at it, add support to select the die.
> 
> Same comment as in 3/4.

I will correct the comment.

> 
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > ---
> >  drivers/mtd/nand/spi/micron.c | 50
> +++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 45fc37c58f8a..03b486843210 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -18,6 +18,8 @@
> >  #define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> >  #define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
> >
> > +#define MICRON_DIE_SELECTION_BIT	6
> > +
> >  static SPINAND_OP_VARIANTS(read_cache_variants,
> >  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2,
> NULL, 0),
> >  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> > @@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops
> micron_8_ooblayout = {
> >  	.free = micron_8_ooblayout_free,
> >  };
> >
> > +static int micron_select_target(struct spinand_device *spinand,
> > +				unsigned int target)
> > +{
> > +	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> > +						      spinand->scratchbuf);
> > +
> > +	/*
> > +	 * As per datasheet, die selection is done by the 6th bit of Die
> > +	 * Select Register (Address 0xD0).
> > +	 */
> 
> I would put this comment close to the macro definition.

Sure, I will do it.

> 
> > +	*spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;
> 
> Either target is or or 1 and you can use the BIT macro, or you suppose
> it can go higher and the _BIT suffix does not fit. _SHIFT would work
> and creating a macro directly would be even better.
> 

I will create macro directly and send the code in next version.

> > +
> > +	return spi_mem_exec_op(spinand->spimem, &op);
> > +}
> > +
> 
> Where is this function used?

IIUC your question, the function is used below in device table.
The line is something like, 

SPINAND_SELECT_TARGET(micron_select_target))

for all the devices with multiple dies.

> 
> >  static int micron_8_ecc_get_status(struct spinand_device *spinand,
> >  				   u8 status)
> >  {
> > @@ -131,6 +148,17 @@ static const struct spinand_info
> micron_spinand_table[] = {
> >  		     0,
> >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> >  				     micron_8_ecc_get_status)),
> > +	/* M79A 4Gb 3.3V */
> > +	SPINAND_INFO("MT29F4G01ADAGD", 0x36,
> > +		     NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> > +		     NAND_ECCREQ(8, 512),
> > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > +					      &write_cache_variants,
> > +					      &update_cache_variants),
> > +		     0,
> > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > +				     micron_8_ecc_get_status),
> > +		     SPINAND_SELECT_TARGET(micron_select_target)),
> >  	/* M70A 4Gb 3.3V */
> >  	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> >  		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > @@ -151,6 +179,28 @@ static const struct spinand_info
> micron_spinand_table[] = {
> >  		     0,
> >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> >  				     micron_8_ecc_get_status)),
> > +	/* M70A 8Gb 3.3V */
> > +	SPINAND_INFO("MT29F8G01ADAFD", 0x46,
> > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > +		     NAND_ECCREQ(8, 512),
> > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > +					      &write_cache_variants,
> > +					      &update_cache_variants),
> > +		     0,
> > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > +				     micron_8_ecc_get_status),
> > +		     SPINAND_SELECT_TARGET(micron_select_target)),
> > +	/* M70A 8Gb 1.8V */
> > +	SPINAND_INFO("MT29F8G01ADBFD", 0x47,
> > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > +		     NAND_ECCREQ(8, 512),
> > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > +					      &write_cache_variants,
> > +					      &update_cache_variants),
> > +		     0,
> > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > +				     micron_8_ecc_get_status),
> > +		     SPINAND_SELECT_TARGET(micron_select_target)),
> >  };
> >
> >  static int micron_spinand_detect(struct spinand_device *spinand)
> 
> 
> 
> 
> Thanks,
> Miquèl

Thanks,
Shiva

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

* Re: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-21 12:23     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
@ 2020-01-21 13:40       ` Miquel Raynal
  2020-01-21 14:59         ` Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 15+ messages in thread
From: Miquel Raynal @ 2020-01-21 13:40 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Shivamurthy,

"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
Tue, 21 Jan 2020 12:23:20 +0000:

> Hi Miquel,
> 
> > 
> > Hi Shiva,
> > 
> > This is remark common to the four patches: you miss the 'v2' prefix in
> > the object.
> >   
> 
> Sorry for this mistake.
> I recognized this after sending out the patches.
> 
> > shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> >   
> > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > >
> > > Add device table for M70A series Micron SPI NAND devices.
> > >
> > > While at it, disable the Continuous Read feature which is enabled by
> > > default.  
> > 
> > Can you please give us more detail on why this is an issue?  
> 
> "Continuous Read" is the new feature added by the Micron for 
> M70A series devices. If this feature is enabled, the READ command 
> doesn't output the OOB area. The following short description
> describes this feature.
> 
> Description:
> If the Continuous Read feature is enabled, the device provides 
> the capability to read the whole block with a single command.
> However, the read command doesn't output the OOB area.
> 
> Read command behavior (if Continuous Read enabled):
> The READ CACHE command doesn't require the starting column address.
> The device always output the data starting from the first column of the
> cache register, and once the end of the cache register reached, the data
> output continues through the next page. With the continuous read mode,
> it is possible to read out the entire block using a single READ command, and
> once the end of the block reached, the output pins become High-Z state.

Ok I understand better. In this case there is no need to split this
commit, instead just reword the commit log to something like:

--->8---
Add device table for M70A series Micron SPI-NAND devices.

As opposed to the M60A series already supported, M70A parts have the
"Continuous Read" feature enabled by default which does not fit the
subsystem needs.

<here explain the feature>.

Hence, we disable the feature at probe time.
---8<---

However, below, you disable this bit for all the parts. Is this really
ok? Souldn't we make it more specific to this series?

> 
> > 
> > Shall we backport it to stable?  
> 
> This is not a bug fix and applicable only to M70A series devices, there is no
> need to backport.
> (FYI, the previously enabled device was M79A series)
> 
> > 
> > As a rule of thumb, when you start a sentence by "while at it" in a
> > commit message and this is not a trivial change : split the patch,
> > please. Unless this is really related and in this case explain how and
> > why in the commit message.  
> 
> Okay, I will explain in my next version.
> 
> >   
> > >
> > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > ---
> > >  drivers/mtd/nand/spi/micron.c | 31  
> > +++++++++++++++++++++++++++++++  
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/spi/micron.c  
> > b/drivers/mtd/nand/spi/micron.c  
> > > index 5fd1f921ef12..45fc37c58f8a 100644
> > > --- a/drivers/mtd/nand/spi/micron.c
> > > +++ b/drivers/mtd/nand/spi/micron.c
> > > @@ -131,6 +131,26 @@ static const struct spinand_info  
> > micron_spinand_table[] = {  
> > >  		     0,
> > >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > >  				     micron_8_ecc_get_status)),
> > > +	/* M70A 4Gb 3.3V */
> > > +	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > +		     NAND_ECCREQ(8, 512),
> > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > +					      &write_cache_variants,
> > > +					      &update_cache_variants),
> > > +		     0,
> > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > +				     micron_8_ecc_get_status)),
> > > +	/* M70A 4Gb 1.8V */
> > > +	SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> > > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > +		     NAND_ECCREQ(8, 512),
> > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > +					      &write_cache_variants,
> > > +					      &update_cache_variants),
> > > +		     0,
> > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > +				     micron_8_ecc_get_status)),
> > >  };
> > >
> > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > > @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct  
> > spinand_device *spinand)  
> > >  	return 1;
> > >  }
> > >
> > > +static int micron_spinand_init(struct spinand_device *spinand)
> > > +{
> > > +	/*
> > > +	 * M70A device series enable Continuous Read feature at Power-up,
> > > +	 * which is not supported. Disable this bit to avoid any possible
> > > +	 * failure.
> > > +	 */
> > > +	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 = {  
> > 
> > Thanks,
> > Miquèl  
> 
> Thanks,
> Shiva




Thanks,
Miquèl

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

* Re: [EXT] Re: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies
  2020-01-21 12:23     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
@ 2020-01-21 13:44       ` Miquel Raynal
  0 siblings, 0 replies; 15+ messages in thread
From: Miquel Raynal @ 2020-01-21 13:44 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, shiva.linuxworks

Hi Shivamurthy,

"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
Tue, 21 Jan 2020 12:23:36 +0000:

> Hi Miquel,
> 
> > 
> > Hi Shiva,
> > 
> > shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:32 +0100:
> >   
> > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > >
> > > Add device table for new Micron SPI NAND devices, which have multiple
> > > dies. While at it, add support to select the die.  
> > 
> > Same comment as in 3/4.  
> 
> I will correct the comment.

Actually now with more explanation I understand better. Please
keep in mind that anybody not knowing what you do on a daily basis
should understand what this commit does and why.

So like before, you actually don't need to split this patch, but
instead rework the commit message.

> 
> >   
> > >
> > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > ---
> > >  drivers/mtd/nand/spi/micron.c | 50  
> > +++++++++++++++++++++++++++++++++++  
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/spi/micron.c  
> > b/drivers/mtd/nand/spi/micron.c  
> > > index 45fc37c58f8a..03b486843210 100644
> > > --- a/drivers/mtd/nand/spi/micron.c
> > > +++ b/drivers/mtd/nand/spi/micron.c
> > > @@ -18,6 +18,8 @@
> > >  #define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> > >  #define MICRON_STATUS_ECC_7TO8_BITFLIPS	(5 << 4)
> > >
> > > +#define MICRON_DIE_SELECTION_BIT	6
> > > +
> > >  static SPINAND_OP_VARIANTS(read_cache_variants,
> > >  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2,  
> > NULL, 0),  
> > >  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> > > @@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops  
> > micron_8_ooblayout = {  
> > >  	.free = micron_8_ooblayout_free,
> > >  };
> > >
> > > +static int micron_select_target(struct spinand_device *spinand,
> > > +				unsigned int target)
> > > +{
> > > +	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> > > +						      spinand->scratchbuf);
> > > +
> > > +	/*
> > > +	 * As per datasheet, die selection is done by the 6th bit of Die
> > > +	 * Select Register (Address 0xD0).
> > > +	 */  
> > 
> > I would put this comment close to the macro definition.  
> 
> Sure, I will do it.
> 
> >   
> > > +	*spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;  
> > 
> > Either target is or or 1 and you can use the BIT macro, or you suppose
> > it can go higher and the _BIT suffix does not fit. _SHIFT would work
> > and creating a macro directly would be even better.
> >   
> 
> I will create macro directly and send the code in next version.
> 
> > > +
> > > +	return spi_mem_exec_op(spinand->spimem, &op);
> > > +}
> > > +  
> > 
> > Where is this function used?  
> 
> IIUC your question, the function is used below in device table.
> The line is something like, 
> 
> SPINAND_SELECT_TARGET(micron_select_target))

I just missed it :)

> 
> for all the devices with multiple dies.
> 
> >   
> > >  static int micron_8_ecc_get_status(struct spinand_device *spinand,
> > >  				   u8 status)
> > >  {
> > > @@ -131,6 +148,17 @@ static const struct spinand_info  
> > micron_spinand_table[] = {  
> > >  		     0,
> > >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > >  				     micron_8_ecc_get_status)),
> > > +	/* M79A 4Gb 3.3V */
> > > +	SPINAND_INFO("MT29F4G01ADAGD", 0x36,
> > > +		     NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> > > +		     NAND_ECCREQ(8, 512),
> > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > +					      &write_cache_variants,
> > > +					      &update_cache_variants),
> > > +		     0,
> > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > +				     micron_8_ecc_get_status),
> > > +		     SPINAND_SELECT_TARGET(micron_select_target)),
> > >  	/* M70A 4Gb 3.3V */
> > >  	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > >  		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > @@ -151,6 +179,28 @@ static const struct spinand_info  
> > micron_spinand_table[] = {  
> > >  		     0,
> > >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > >  				     micron_8_ecc_get_status)),
> > > +	/* M70A 8Gb 3.3V */
> > > +	SPINAND_INFO("MT29F8G01ADAFD", 0x46,
> > > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > > +		     NAND_ECCREQ(8, 512),
> > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > +					      &write_cache_variants,
> > > +					      &update_cache_variants),
> > > +		     0,
> > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > +				     micron_8_ecc_get_status),
> > > +		     SPINAND_SELECT_TARGET(micron_select_target)),
> > > +	/* M70A 8Gb 1.8V */
> > > +	SPINAND_INFO("MT29F8G01ADBFD", 0x47,
> > > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > > +		     NAND_ECCREQ(8, 512),
> > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > +					      &write_cache_variants,
> > > +					      &update_cache_variants),
> > > +		     0,
> > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > +				     micron_8_ecc_get_status),
> > > +		     SPINAND_SELECT_TARGET(micron_select_target)),
> > >  };
> > >
> > >  static int micron_spinand_detect(struct spinand_device *spinand)  
> > 
> > 
> > 
> > 
> > Thanks,
> > Miquèl  
> 
> Thanks,
> Shiva




Thanks,
Miquèl

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

* RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-21 13:40       ` Miquel Raynal
@ 2020-01-21 14:59         ` Shivamurthy Shastri (sshivamurthy)
  2020-01-21 15:27           ` Miquel Raynal
  0 siblings, 1 reply; 15+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2020-01-21 14:59 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Miquel,

> 
> Hi Shivamurthy,
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote
> on
> Tue, 21 Jan 2020 12:23:20 +0000:
> 
> > Hi Miquel,
> >
> > >
> > > Hi Shiva,
> > >
> > > This is remark common to the four patches: you miss the 'v2' prefix in
> > > the object.
> > >
> >
> > Sorry for this mistake.
> > I recognized this after sending out the patches.
> >
> > > shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> > >
> > > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > >
> > > > Add device table for M70A series Micron SPI NAND devices.
> > > >
> > > > While at it, disable the Continuous Read feature which is enabled by
> > > > default.
> > >
> > > Can you please give us more detail on why this is an issue?
> >
> > "Continuous Read" is the new feature added by the Micron for
> > M70A series devices. If this feature is enabled, the READ command
> > doesn't output the OOB area. The following short description
> > describes this feature.
> >
> > Description:
> > If the Continuous Read feature is enabled, the device provides
> > the capability to read the whole block with a single command.
> > However, the read command doesn't output the OOB area.
> >
> > Read command behavior (if Continuous Read enabled):
> > The READ CACHE command doesn't require the starting column address.
> > The device always output the data starting from the first column of the
> > cache register, and once the end of the cache register reached, the data
> > output continues through the next page. With the continuous read mode,
> > it is possible to read out the entire block using a single READ command, and
> > once the end of the block reached, the output pins become High-Z state.
> 
> Ok I understand better. In this case there is no need to split this
> commit, instead just reword the commit log to something like:
> 
> --->8---
> Add device table for M70A series Micron SPI-NAND devices.
> 
> As opposed to the M60A series already supported, M70A parts have the
> "Continuous Read" feature enabled by default which does not fit the
> subsystem needs.
> 
> <here explain the feature>.
> 
> Hence, we disable the feature at probe time.
> ---8<---
> 

Sure, I will change as per your suggestion.

> However, below, you disable this bit for all the parts. Is this really
> ok? Souldn't we make it more specific to this series?

It is ok because this bit is unused in other series.

> 
> >
> > >
> > > Shall we backport it to stable?
> >
> > This is not a bug fix and applicable only to M70A series devices, there is no
> > need to backport.
> > (FYI, the previously enabled device was M79A series)
> >
> > >
> > > As a rule of thumb, when you start a sentence by "while at it" in a
> > > commit message and this is not a trivial change : split the patch,
> > > please. Unless this is really related and in this case explain how and
> > > why in the commit message.
> >
> > Okay, I will explain in my next version.
> >
> > >
> > > >
> > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > > ---
> > > >  drivers/mtd/nand/spi/micron.c | 31
> > > +++++++++++++++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/micron.c
> > > b/drivers/mtd/nand/spi/micron.c
> > > > index 5fd1f921ef12..45fc37c58f8a 100644
> > > > --- a/drivers/mtd/nand/spi/micron.c
> > > > +++ b/drivers/mtd/nand/spi/micron.c
> > > > @@ -131,6 +131,26 @@ static const struct spinand_info
> > > micron_spinand_table[] = {
> > > >  		     0,
> > > >  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > >  				     micron_8_ecc_get_status)),
> > > > +	/* M70A 4Gb 3.3V */
> > > > +	SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > > > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > > +		     NAND_ECCREQ(8, 512),
> > > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > > +					      &write_cache_variants,
> > > > +					      &update_cache_variants),
> > > > +		     0,
> > > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > > +				     micron_8_ecc_get_status)),
> > > > +	/* M70A 4Gb 1.8V */
> > > > +	SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> > > > +		     NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > > +		     NAND_ECCREQ(8, 512),
> > > > +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > > +					      &write_cache_variants,
> > > > +					      &update_cache_variants),
> > > > +		     0,
> > > > +		     SPINAND_ECCINFO(&micron_8_ooblayout,
> > > > +				     micron_8_ecc_get_status)),
> > > >  };
> > > >
> > > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > > > @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct
> > > spinand_device *spinand)
> > > >  	return 1;
> > > >  }
> > > >
> > > > +static int micron_spinand_init(struct spinand_device *spinand)
> > > > +{
> > > > +	/*
> > > > +	 * M70A device series enable Continuous Read feature at Power-up,
> > > > +	 * which is not supported. Disable this bit to avoid any possible
> > > > +	 * failure.
> > > > +	 */
> > > > +	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 = {
> > >

Thanks,
Shiva

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

* Re: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-21 14:59         ` Shivamurthy Shastri (sshivamurthy)
@ 2020-01-21 15:27           ` Miquel Raynal
  2020-01-23  9:01             ` Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 15+ messages in thread
From: Miquel Raynal @ 2020-01-21 15:27 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Shivamurthy,

"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
Tue, 21 Jan 2020 14:59:11 +0000:

> Hi Miquel,
> 
> > 
> > Hi Shivamurthy,
> > 
> > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote
> > on
> > Tue, 21 Jan 2020 12:23:20 +0000:
> >   
> > > Hi Miquel,
> > >  
> > > >
> > > > Hi Shiva,
> > > >
> > > > This is remark common to the four patches: you miss the 'v2' prefix in
> > > > the object.
> > > >  
> > >
> > > Sorry for this mistake.
> > > I recognized this after sending out the patches.
> > >  
> > > > shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> > > >  
> > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > > >
> > > > > Add device table for M70A series Micron SPI NAND devices.
> > > > >
> > > > > While at it, disable the Continuous Read feature which is enabled by
> > > > > default.  
> > > >
> > > > Can you please give us more detail on why this is an issue?  
> > >
> > > "Continuous Read" is the new feature added by the Micron for
> > > M70A series devices. If this feature is enabled, the READ command
> > > doesn't output the OOB area. The following short description
> > > describes this feature.
> > >
> > > Description:
> > > If the Continuous Read feature is enabled, the device provides
> > > the capability to read the whole block with a single command.
> > > However, the read command doesn't output the OOB area.
> > >
> > > Read command behavior (if Continuous Read enabled):
> > > The READ CACHE command doesn't require the starting column address.
> > > The device always output the data starting from the first column of the
> > > cache register, and once the end of the cache register reached, the data
> > > output continues through the next page. With the continuous read mode,
> > > it is possible to read out the entire block using a single READ command, and
> > > once the end of the block reached, the output pins become High-Z state.  
> > 
> > Ok I understand better. In this case there is no need to split this
> > commit, instead just reword the commit log to something like:
> >   
> > --->8---  
> > Add device table for M70A series Micron SPI-NAND devices.
> > 
> > As opposed to the M60A series already supported, M70A parts have the
> > "Continuous Read" feature enabled by default which does not fit the
> > subsystem needs.
> > 
> > <here explain the feature>.
> > 
> > Hence, we disable the feature at probe time.
> > ---8<---
> >   
> 
> Sure, I will change as per your suggestion.
> 
> > However, below, you disable this bit for all the parts. Is this really
> > ok? Souldn't we make it more specific to this series?  
> 
> It is ok because this bit is unused in other series.

I would rather prefer to avoid this kind of arrangement. Please change
this bit only for the series which has it.


Thanks,
Miquèl

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

* RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices
  2020-01-21 15:27           ` Miquel Raynal
@ 2020-01-23  9:01             ` Shivamurthy Shastri (sshivamurthy)
  0 siblings, 0 replies; 15+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2020-01-23  9:01 UTC (permalink / raw)
  To: 'Miquel Raynal'
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Miquel,

> 
> Hi Shivamurthy,
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote
> on
> Tue, 21 Jan 2020 14:59:11 +0000:
> 
> > Hi Miquel,
> >
> > >
> > > Hi Shivamurthy,
> > >
> > > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com>
> wrote
> > > on
> > > Tue, 21 Jan 2020 12:23:20 +0000:
> > >
> > > > Hi Miquel,
> > > >
> > > > >
> > > > > Hi Shiva,
> > > > >
> > > > > This is remark common to the four patches: you miss the 'v2' prefix in
> > > > > the object.
> > > > >
> > > >
> > > > Sorry for this mistake.
> > > > I recognized this after sending out the patches.
> > > >
> > > > > shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:31
> +0100:
> > > > >
> > > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > > > >
> > > > > > Add device table for M70A series Micron SPI NAND devices.
> > > > > >
> > > > > > While at it, disable the Continuous Read feature which is enabled by
> > > > > > default.
> > > > >
> > > > > Can you please give us more detail on why this is an issue?
> > > >
> > > > "Continuous Read" is the new feature added by the Micron for
> > > > M70A series devices. If this feature is enabled, the READ command
> > > > doesn't output the OOB area. The following short description
> > > > describes this feature.
> > > >
> > > > Description:
> > > > If the Continuous Read feature is enabled, the device provides
> > > > the capability to read the whole block with a single command.
> > > > However, the read command doesn't output the OOB area.
> > > >
> > > > Read command behavior (if Continuous Read enabled):
> > > > The READ CACHE command doesn't require the starting column
> address.
> > > > The device always output the data starting from the first column of the
> > > > cache register, and once the end of the cache register reached, the
> data
> > > > output continues through the next page. With the continuous read
> mode,
> > > > it is possible to read out the entire block using a single READ command,
> and
> > > > once the end of the block reached, the output pins become High-Z
> state.
> > >
> > > Ok I understand better. In this case there is no need to split this
> > > commit, instead just reword the commit log to something like:
> > >
> > > --->8---
> > > Add device table for M70A series Micron SPI-NAND devices.
> > >
> > > As opposed to the M60A series already supported, M70A parts have the
> > > "Continuous Read" feature enabled by default which does not fit the
> > > subsystem needs.
> > >
> > > <here explain the feature>.
> > >
> > > Hence, we disable the feature at probe time.
> > > ---8<---
> > >
> >
> > Sure, I will change as per your suggestion.
> >
> > > However, below, you disable this bit for all the parts. Is this really
> > > ok? Souldn't we make it more specific to this series?
> >
> > It is ok because this bit is unused in other series.
> 
> I would rather prefer to avoid this kind of arrangement. Please change
> this bit only for the series which has it.

There is no other way to identify the series of the device except by using the
"flags" from the structure spinand_info. So I will proceed with this approach.

1. In spinand.h --> I will create macro

#define SPINAND_HAS_CR_FEAT_BIT         BIT(1)

2. Use this flag inside micron_spinand_init function to identify the device.


> 
> 
> Thanks,
> Miquèl

Thanks,
Shiva

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

* Re: [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names
  2020-01-19 14:54 ` [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names shiva.linuxworks
@ 2020-01-30  8:30   ` Miquel Raynal
  0 siblings, 0 replies; 15+ messages in thread
From: Miquel Raynal @ 2020-01-30  8:30 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Frieder Schrempf, linux-mtd, linux-kernel, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Sun, 19 Jan 2020 15:54:29 +0100:

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 

I would also prefix the commit with "mtd: spinand: micron:".

> In order to add new Micron SPI NAND devices, we generalized the OOB
> layout structure and function names.
> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>

Thanks,
Miquèl

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

end of thread, other threads:[~2020-01-30  8:30 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-19 14:54 [PATCH 0/4] Add new series Micron SPI NAND devices shiva.linuxworks
2020-01-19 14:54 ` [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names shiva.linuxworks
2020-01-30  8:30   ` Miquel Raynal
2020-01-19 14:54 ` [PATCH 2/4] mtd: spinand: Add new Micron SPI NAND devices shiva.linuxworks
2020-01-19 14:54 ` [PATCH 3/4] mtd: spinand: Add M70A series " shiva.linuxworks
2020-01-20 10:16   ` Miquel Raynal
2020-01-21 12:23     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2020-01-21 13:40       ` Miquel Raynal
2020-01-21 14:59         ` Shivamurthy Shastri (sshivamurthy)
2020-01-21 15:27           ` Miquel Raynal
2020-01-23  9:01             ` Shivamurthy Shastri (sshivamurthy)
2020-01-19 14:54 ` [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies shiva.linuxworks
2020-01-20 10:22   ` Miquel Raynal
2020-01-21 12:23     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2020-01-21 13:44       ` Miquel Raynal

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