Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/8] Introduce generic ONFI support
@ 2019-07-22  5:56 shiva.linuxworks
  2019-07-22  5:56 ` [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h shiva.linuxworks
                   ` (7 more replies)
  0 siblings, 8 replies; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Current support to ONFI parameter page is only for raw NAND, this patch
series turn ONFI support into generic. So that, other NAND devices like
SPI NAND can use this.

The series has five parts.
1. Prepare for tunrning ONFI into generic
2. Turn ONFI into generic
3. Turn SPI NAND core to use parameter page
4. Redesign Micron SPI NAND driver implementation
5. Support for new Micron SPI NAND devices

Changes in V4
-------------
* Turn ONFI support into generic.
  Re-designed as per the Boris's comment.
  Common functions are moved to nand/onfi.c.
  Function to prase ONFI table is defined in nand/onfi.c
* Enable parameter page support in SPI NAND core.
  Re-designed as per changes in ONFI generic support.
  Function defined to read parameter page.
  Function defined to detect parameter page.

Changes in V3
-------------

* Split the patches as per suggestion
* Addressed the comments
* Some fixes which I missed in last version

Shivamurthy Shastri (8):
  mtd: nand: move ONFI related functions to onfi.h
  mtd: nand: move support functions for ONFI to nand/onfi.c
  mtd: nand: create ONFI table parsing instance
  mtd: spinand: enabled parameter page support
  mtd: spinand: micron: prepare for generalizing driver
  mtd: spinand: micron: Turn driver implementation generic
  mtd: spinand: micron: Fix read failure in Micron M70A flashes
  mtd: spinand: micron: Enable micron flashes with multi-die

 drivers/mtd/nand/Makefile        |   2 +-
 drivers/mtd/nand/onfi.c          | 121 +++++++++++++++++++++++++++
 drivers/mtd/nand/raw/internals.h |   1 -
 drivers/mtd/nand/raw/nand_base.c |  18 ----
 drivers/mtd/nand/raw/nand_onfi.c |  65 +--------------
 drivers/mtd/nand/spi/core.c      | 136 ++++++++++++++++++++++++++++++-
 drivers/mtd/nand/spi/micron.c    | 107 +++++++++++++++++-------
 include/linux/mtd/onfi.h         |  11 +++
 include/linux/mtd/spinand.h      |   7 ++
 9 files changed, 356 insertions(+), 112 deletions(-)
 create mode 100644 drivers/mtd/nand/onfi.c

-- 
2.17.1


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

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

* [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07  8:34   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c shiva.linuxworks
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

These functions will be used by both raw NAND and SPI NAND, which
supports ONFI like standards.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/raw/internals.h | 1 -
 include/linux/mtd/onfi.h         | 9 +++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
index cba6fe7dd8c4..ed323087d884 100644
--- a/drivers/mtd/nand/raw/internals.h
+++ b/drivers/mtd/nand/raw/internals.h
@@ -140,7 +140,6 @@ void nand_legacy_adjust_cmdfunc(struct nand_chip *chip);
 int nand_legacy_check_hooks(struct nand_chip *chip);
 
 /* ONFI functions */
-u16 onfi_crc16(u16 crc, u8 const *p, size_t len);
 int nand_onfi_detect(struct nand_chip *chip);
 
 /* JEDEC functions */
diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
index 339ac798568e..2c8a05a02bb0 100644
--- a/include/linux/mtd/onfi.h
+++ b/include/linux/mtd/onfi.h
@@ -10,6 +10,7 @@
 #ifndef __LINUX_MTD_ONFI_H
 #define __LINUX_MTD_ONFI_H
 
+#include <linux/mtd/nand.h>
 #include <linux/types.h>
 
 /* ONFI version bits */
@@ -175,4 +176,12 @@ struct onfi_params {
 	u8 vendor[88];
 };
 
+/* ONFI functions */
+u16 onfi_crc16(u16 crc, u8 const *p, size_t len);
+void nand_bit_wise_majority(const void **srcbufs,
+			    unsigned int nsrcbufs,
+			    void *dstbuf,
+			    unsigned int bufsize);
+void sanitize_string(u8 *s, size_t len);
+
 #endif /* __LINUX_MTD_ONFI_H */
-- 
2.17.1


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

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

* [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
  2019-07-22  5:56 ` [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07  9:03   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 3/8] mtd: nand: create ONFI table parsing instance shiva.linuxworks
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

These functions are support functions for enabling ONFI standard and
common between raw NAND and SPI NAND.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/Makefile        |  2 +-
 drivers/mtd/nand/onfi.c          | 89 ++++++++++++++++++++++++++++++++
 drivers/mtd/nand/raw/nand_base.c | 18 -------
 drivers/mtd/nand/raw/nand_onfi.c | 43 ---------------
 4 files changed, 90 insertions(+), 62 deletions(-)
 create mode 100644 drivers/mtd/nand/onfi.c

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 7ecd80c0a66e..221945c223c3 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-nandcore-objs := core.o bbt.o
+nandcore-objs := core.o bbt.o onfi.o
 obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
 
 obj-y	+= onenand/
diff --git a/drivers/mtd/nand/onfi.c b/drivers/mtd/nand/onfi.c
new file mode 100644
index 000000000000..7aaf36dfc5e0
--- /dev/null
+++ b/drivers/mtd/nand/onfi.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt)     "nand-onfi: " fmt
+
+#include <linux/mtd/onfi.h>
+#include <linux/mtd/nand.h>
+
+/**
+ * onfi_crc16() - Check CRC of ONFI table
+ * @crc: base CRC
+ * @p: buffer pointing to ONFI table
+ * @len: length of ONFI table
+ *
+ * Return: CRC of the ONFI table
+ */
+u16 onfi_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;
+}
+EXPORT_SYMBOL_GPL(onfi_crc16);
+
+/**
+ * nand_bit_wise_majority() - Recover data with bit-wise majority
+ * @srcbufs: buffer pointing to ONFI table
+ * @nsrcbufs: length of ONFI table
+ * @dstbuf: valid ONFI table to be returned
+ * @bufsize: length og valid ONFI table
+ *
+ */
+void nand_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;
+	}
+}
+EXPORT_SYMBOL_GPL(nand_bit_wise_majority);
+
+/**
+ * sanitize_string() - Sanitize ONFI strings so we can safely print them
+ * @s: string to be sanitized
+ * @len: length of the string
+ *
+ */
+void sanitize_string(u8 *s, size_t len)
+{
+	ssize_t i;
+
+	/* Null terminate */
+	s[len - 1] = 0;
+
+	/* Remove non printable chars */
+	for (i = 0; i < len - 1; i++) {
+		if (s[i] < ' ' || s[i] > 127)
+			s[i] = '?';
+	}
+
+	/* Remove trailing spaces */
+	strim(s);
+}
+EXPORT_SYMBOL_GPL(sanitize_string);
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 6ecd1c496ce3..c198829bcd79 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4375,24 +4375,6 @@ static void nand_set_defaults(struct nand_chip *chip)
 		chip->buf_align = 1;
 }
 
-/* Sanitize ONFI strings so we can safely print them */
-void sanitize_string(uint8_t *s, size_t len)
-{
-	ssize_t i;
-
-	/* Null terminate */
-	s[len - 1] = 0;
-
-	/* Remove non printable chars */
-	for (i = 0; i < len - 1; i++) {
-		if (s[i] < ' ' || s[i] > 127)
-			s[i] = '?';
-	}
-
-	/* Remove trailing spaces */
-	strim(s);
-}
-
 /*
  * nand_id_has_period - Check if an ID string has a given wraparound period
  * @id_data: the ID string
diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 0b879bd0a68c..2e8edfa636ef 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -16,18 +16,6 @@
 
 #include "internals.h"
 
-u16 onfi_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;
-}
-
 /* Parse the Extended Parameter Page. */
 static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
 					    struct nand_onfi_params *p)
@@ -103,37 +91,6 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
 	return ret;
 }
 
-/*
- * Recover data with bit-wise majority
- */
-static void nand_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;
-	}
-}
-
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
-- 
2.17.1


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

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

* [PATCH 3/8] mtd: nand: create ONFI table parsing instance
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
  2019-07-22  5:56 ` [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h shiva.linuxworks
  2019-07-22  5:56 ` [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07  9:09   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 4/8] mtd: spinand: enabled parameter page support shiva.linuxworks
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

ONFI table parsing is common, as most of the variables are common
between raw and SPI NAND. The parsing function is instantiated in
onfi.c, which fills ONFI parameters into nand_memory_organization.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/onfi.c          | 32 ++++++++++++++++++++++++++++++++
 drivers/mtd/nand/raw/nand_onfi.c | 22 ++--------------------
 include/linux/mtd/onfi.h         |  2 ++
 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/nand/onfi.c b/drivers/mtd/nand/onfi.c
index 7aaf36dfc5e0..e78700894aea 100644
--- a/drivers/mtd/nand/onfi.c
+++ b/drivers/mtd/nand/onfi.c
@@ -87,3 +87,35 @@ void sanitize_string(u8 *s, size_t len)
 	strim(s);
 }
 EXPORT_SYMBOL_GPL(sanitize_string);
+
+/**
+ * fill_nand_memorg() - Parse ONFI table and fill memorg
+ * @memorg: NAND memorg to be filled
+ * @p: ONFI table to be parsed
+ *
+ */
+void parse_onfi_params(struct nand_memory_organization *memorg,
+		       struct nand_onfi_params *p)
+{
+	memorg->pagesize = le32_to_cpu(p->byte_per_page);
+
+	/*
+	 * pages_per_block and blocks_per_lun may not be a power-of-2 size
+	 * (don't ask me who thought of this...). MTD assumes that these
+	 * dimensions will be power-of-2, so just truncate the remaining area.
+	 */
+	memorg->pages_per_eraseblock =
+			1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
+
+	memorg->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+
+	memorg->luns_per_target = p->lun_count;
+	memorg->planes_per_lun = 1 << p->interleaved_bits;
+
+	/* See erasesize comment */
+	memorg->eraseblocks_per_lun =
+		1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
+	memorg->max_bad_eraseblocks_per_lun = le32_to_cpu(p->blocks_per_lun);
+	memorg->bits_per_cell = p->bits_per_cell;
+}
+EXPORT_SYMBOL_GPL(parse_onfi_params);
diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 2e8edfa636ef..263796d3298c 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -181,30 +181,12 @@ int nand_onfi_detect(struct nand_chip *chip)
 		goto free_onfi_param_page;
 	}
 
-	memorg->pagesize = le32_to_cpu(p->byte_per_page);
-	mtd->writesize = memorg->pagesize;
+	parse_onfi_params(memorg, p);
 
-	/*
-	 * pages_per_block and blocks_per_lun may not be a power-of-2 size
-	 * (don't ask me who thought of this...). MTD assumes that these
-	 * dimensions will be power-of-2, so just truncate the remaining area.
-	 */
-	memorg->pages_per_eraseblock =
-			1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
+	mtd->writesize = memorg->pagesize;
 	mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
-
-	memorg->oobsize = le16_to_cpu(p->spare_bytes_per_page);
 	mtd->oobsize = memorg->oobsize;
 
-	memorg->luns_per_target = p->lun_count;
-	memorg->planes_per_lun = 1 << p->interleaved_bits;
-
-	/* See erasesize comment */
-	memorg->eraseblocks_per_lun =
-		1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
-	memorg->max_bad_eraseblocks_per_lun = le32_to_cpu(p->blocks_per_lun);
-	memorg->bits_per_cell = p->bits_per_cell;
-
 	if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
 		chip->options |= NAND_BUSWIDTH_16;
 
diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
index 2c8a05a02bb0..4cacf4e9db6d 100644
--- a/include/linux/mtd/onfi.h
+++ b/include/linux/mtd/onfi.h
@@ -183,5 +183,7 @@ void nand_bit_wise_majority(const void **srcbufs,
 			    void *dstbuf,
 			    unsigned int bufsize);
 void sanitize_string(u8 *s, size_t len);
+void parse_onfi_params(struct nand_memory_organization *memorg,
+		       struct nand_onfi_params *p);
 
 #endif /* __LINUX_MTD_ONFI_H */
-- 
2.17.1


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

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

* [PATCH 4/8] mtd: spinand: enabled parameter page support
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
                   ` (2 preceding siblings ...)
  2019-07-22  5:56 ` [PATCH 3/8] mtd: nand: create ONFI table parsing instance shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07  9:48   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver shiva.linuxworks
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Some of the SPI NAND devices has parameter page, which is similar to
ONFI table.

But, it may not be self sufficient to propagate all the required
parameters. Fixup function has been added in struct manufacturer to
accommodate this.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/spi/core.c | 134 ++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h |   3 +
 2 files changed, 137 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 89f6beefb01c..7ae76dab9141 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -400,6 +400,131 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
 	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
 }
 
+/**
+ * spinand_read_param_page_op - Read parameter page operation
+ * @spinand: the spinand
+ * @page: page number where parameter page tables can be found
+ * @buf: buffer used to store the parameter page
+ * @len: length of the buffer
+ *
+ * Read parameter page
+ *
+ * Returns 0 on success, a negative error code otherwise.
+ */
+static int spinand_parameter_page_read(struct spinand_device *spinand,
+				       u8 page, void *buf, unsigned int len)
+{
+	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
+	struct spi_mem_op pread_cache_op =
+				SPINAND_PAGE_READ_FROM_CACHE_OP(false,
+								0,
+								1,
+								buf,
+								len);
+	u8 feature;
+	u8 status;
+	int ret;
+
+	if (len && !buf)
+		return -EINVAL;
+
+	ret = spinand_read_reg_op(spinand, REG_CFG,
+				  &feature);
+	if (ret)
+		return ret;
+
+	/* CFG_OTP_ENABLE is used to enable parameter page access */
+	feature |= CFG_OTP_ENABLE;
+
+	spinand_write_reg_op(spinand, REG_CFG, feature);
+
+	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
+	if (ret)
+		return ret;
+
+	ret = spinand_wait(spinand, &status);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
+	if (ret)
+		return ret;
+
+	ret = spinand_read_reg_op(spinand, REG_CFG,
+				  &feature);
+	if (ret)
+		return ret;
+
+	feature &= ~CFG_OTP_ENABLE;
+
+	spinand_write_reg_op(spinand, REG_CFG, feature);
+
+	return 0;
+}
+
+static int spinand_param_page_detect(struct spinand_device *spinand)
+{
+	struct mtd_info *mtd = spinand_to_mtd(spinand);
+	struct nand_memory_organization *memorg;
+	struct nand_onfi_params *p;
+	struct nand_device *base = spinand_to_nand(spinand);
+	int i, ret;
+
+	memorg = nanddev_get_memorg(base);
+
+	/* Allocate buffer to hold parameter page */
+	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * 3);
+	if (ret) {
+		ret = 0;
+		goto free_param_page;
+	}
+
+	for (i = 0; i < 3; i++) {
+		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
+				le16_to_cpu(p->crc)) {
+			if (i)
+				memcpy(p, &p[i], sizeof(*p));
+			break;
+		}
+	}
+
+	if (i == 3) {
+		const void *srcbufs[3] = {p, p + 1, p + 2};
+
+		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
+		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
+				       sizeof(*p));
+
+		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
+				le16_to_cpu(p->crc)) {
+			pr_err("ONFI parameter recovery failed, aborting\n");
+			goto free_param_page;
+		}
+	}
+
+	parse_onfi_params(memorg, p);
+
+	mtd->writesize = memorg->pagesize;
+	mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
+	mtd->oobsize = memorg->oobsize;
+
+	/* Manufacturers may interpret the parameter page differently */
+	if (spinand->manufacturer->ops->fixup_param_page)
+		spinand->manufacturer->ops->fixup_param_page(spinand, p);
+
+	/* Identification done, free the full parameter page and exit */
+	ret = 1;
+
+free_param_page:
+	kfree(p);
+
+	return ret;
+}
+
 static int spinand_check_ecc_status(struct spinand_device *spinand, u8 status)
 {
 	struct nand_device *nand = spinand_to_nand(spinand);
@@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device *spinand)
 		return ret;
 	}
 
+	if (!spinand->base.memorg.pagesize) {
+		ret = spinand_param_page_detect(spinand);
+		if (ret <= 0) {
+			dev_err(dev, "no parameter page for %*phN\n",
+				SPINAND_MAX_ID_LEN, spinand->id.data);
+			return -ENODEV;
+		}
+	}
+
 	if (nand->memorg.ntargets > 1 && !spinand->select_target) {
 		dev_err(dev,
 			"SPI NANDs with more than one die must implement ->select_target()\n");
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 4ea558bd3c46..fea820a20bc9 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -15,6 +15,7 @@
 #include <linux/mtd/nand.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi-mem.h>
+#include <linux/mtd/onfi.h>
 
 /**
  * Standard SPI NAND flash operations
@@ -209,6 +210,8 @@ struct spinand_manufacturer_ops {
 	int (*detect)(struct spinand_device *spinand);
 	int (*init)(struct spinand_device *spinand);
 	void (*cleanup)(struct spinand_device *spinand);
+	void (*fixup_param_page)(struct spinand_device *spinand,
+				 struct nand_onfi_params *p);
 };
 
 /**
-- 
2.17.1


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

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

* [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
                   ` (3 preceding siblings ...)
  2019-07-22  5:56 ` [PATCH 4/8] mtd: spinand: enabled parameter page support shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07  9:51   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic shiva.linuxworks
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Generalize 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..95bc5264ebc1 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_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_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_ooblayout_ops = {
+	.ecc = micron_ooblayout_ecc,
+	.free = micron_ooblayout_free,
 };
 
-static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
-					 u8 status)
+static int micron_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_ooblayout_ops,
+				     micron_ecc_get_status)),
 };
 
 static int micron_spinand_detect(struct spinand_device *spinand)
-- 
2.17.1


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

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

* [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
                   ` (4 preceding siblings ...)
  2019-07-22  5:56 ` [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07 10:04   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes shiva.linuxworks
  2019-07-22  5:56 ` [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die shiva.linuxworks
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Driver is redesigned using parameter page to support Micron SPI NAND
flashes.
The reason why spinand_select_op_variant globalized is that the Micron
driver no longer calling spinand_match_and_init.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/mtd/nand/spi/core.c   |  2 +-
 drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++----------
 include/linux/mtd/spinand.h   |  4 +++
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 7ae76dab9141..aae715d388b7 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct spinand_device *spinand)
 		return spinand->manufacturer->ops->cleanup(spinand);
 }
 
-static const struct spi_mem_op *
+const struct spi_mem_op *
 spinand_select_op_variant(struct spinand_device *spinand,
 			  const struct spinand_op_variants *variants)
 {
diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 95bc5264ebc1..6fde93ec23a1 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -90,22 +90,10 @@ static int micron_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, 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_ooblayout_ops,
-				     micron_ecc_get_status)),
-};
-
 static int micron_spinand_detect(struct spinand_device *spinand)
 {
+	const struct spi_mem_op *op;
 	u8 *id = spinand->id.data;
-	int ret;
 
 	/*
 	 * Micron SPI NAND read ID need a dummy byte,
@@ -114,16 +102,55 @@ 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]);
-	if (ret)
-		return ret;
+	spinand->flags = 0;
+	spinand->eccinfo.get_status = micron_ecc_get_status;
+	spinand->eccinfo.ooblayout = &micron_ooblayout_ops;
+
+	op = spinand_select_op_variant(spinand,
+				       &read_cache_variants);
+	if (!op)
+		return -ENOTSUPP;
+
+	spinand->op_templates.read_cache = op;
+
+	op = spinand_select_op_variant(spinand,
+				       &write_cache_variants);
+	if (!op)
+		return -ENOTSUPP;
+
+	spinand->op_templates.write_cache = op;
+
+	op = spinand_select_op_variant(spinand,
+				       &update_cache_variants);
+	spinand->op_templates.update_cache = op;
 
 	return 1;
 }
 
+static void micron_fixup_param_page(struct spinand_device *spinand,
+				    struct nand_onfi_params *p)
+{
+	/*
+	 * As per Micron datasheets vendor[83] is defined as
+	 * die_select_feature
+	 */
+	if (p->vendor[83] && !p->interleaved_bits)
+		spinand->base.memorg.planes_per_lun = 1 << p->vendor[0];
+
+	spinand->base.memorg.ntargets = p->lun_count;
+	spinand->base.memorg.luns_per_target = 1;
+
+	/*
+	 * As per Micron datasheets,
+	 * vendor[82] is ECC maximum correctability
+	 */
+	spinand->base.eccreq.strength = p->vendor[82];
+	spinand->base.eccreq.step_size = 512;
+}
+
 static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
 	.detect = micron_spinand_detect,
+	.fixup_param_page = micron_fixup_param_page,
 };
 
 const struct spinand_manufacturer micron_spinand_manufacturer = {
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index fea820a20bc9..ddb2194273a8 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -461,4 +461,8 @@ int spinand_match_and_init(struct spinand_device *dev,
 int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
 int spinand_select_target(struct spinand_device *spinand, unsigned int target);
 
+const struct spi_mem_op *
+spinand_select_op_variant(struct spinand_device *spinand,
+			  const struct spinand_op_variants *variants);
+
 #endif /* __LINUX_MTD_SPINAND_H */
-- 
2.17.1


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

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

* [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
                   ` (5 preceding siblings ...)
  2019-07-22  5:56 ` [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07 10:05   ` Miquel Raynal
  2019-07-22  5:56 ` [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die shiva.linuxworks
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

M70A series flashes by default enable continuous read feature (BIT0 in
configuration register). This feature will not expose the ECC to host
and causing read failure.

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

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 6fde93ec23a1..1e28ea3d1362 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -127,6 +127,15 @@ static int micron_spinand_detect(struct spinand_device *spinand)
 	return 1;
 }
 
+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 void micron_fixup_param_page(struct spinand_device *spinand,
 				    struct nand_onfi_params *p)
 {
@@ -150,6 +159,7 @@ static void micron_fixup_param_page(struct spinand_device *spinand,
 
 static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
 	.detect = micron_spinand_detect,
+	.init = micron_spinand_init,
 	.fixup_param_page = micron_fixup_param_page,
 };
 
-- 
2.17.1


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

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

* [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die
  2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
                   ` (6 preceding siblings ...)
  2019-07-22  5:56 ` [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes shiva.linuxworks
@ 2019-07-22  5:56 ` shiva.linuxworks
  2019-08-07 10:08   ` Miquel Raynal
  7 siblings, 1 reply; 28+ messages in thread
From: shiva.linuxworks @ 2019-07-22  5:56 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, David Woodhouse, Brian Norris,
	Marek Vasut, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Frieder Schrempf, Shivamurthy Shastri,
	linux-mtd, linux-kernel, Jeff Kletsky, Chuanhong Guo,
	liaoweixiong

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Some of the Micron flashes has multi-die, and need to select the die
each time while accessing it.

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

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 1e28ea3d1362..fa2b43caf39d 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -90,6 +90,19 @@ static int micron_ecc_get_status(struct spinand_device *spinand,
 	return -EINVAL;
 }
 
+static int micron_select_target(struct spinand_device *spinand,
+				unsigned int target)
+{
+	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
+						      spinand->scratchbuf);
+
+	if (target == 1)
+		target = 0x40;
+
+	*spinand->scratchbuf = target;
+	return spi_mem_exec_op(spinand->spimem, &op);
+}
+
 static int micron_spinand_detect(struct spinand_device *spinand)
 {
 	const struct spi_mem_op *op;
@@ -105,6 +118,7 @@ static int micron_spinand_detect(struct spinand_device *spinand)
 	spinand->flags = 0;
 	spinand->eccinfo.get_status = micron_ecc_get_status;
 	spinand->eccinfo.ooblayout = &micron_ooblayout_ops;
+	spinand->select_target = micron_select_target;
 
 	op = spinand_select_op_variant(spinand,
 				       &read_cache_variants);
-- 
2.17.1


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

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

* Re: [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h
  2019-07-22  5:56 ` [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h shiva.linuxworks
@ 2019-08-07  8:34   ` Miquel Raynal
  2019-08-19  8:35     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07  8:34 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:14 +0200:

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> These functions will be used by both raw NAND and SPI NAND, which
> supports ONFI like standards.

This is not exactly what you do. Why not:

mtd: nand: export ONFI related functions to onfi.h

These functions can be used by all flavors of NAND chips (raw, SPI)
which may all follow ONFI standards. Export the related functions in
the onfi.h generic file.

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/raw/internals.h | 1 -
>  include/linux/mtd/onfi.h         | 9 +++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index cba6fe7dd8c4..ed323087d884 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -140,7 +140,6 @@ void nand_legacy_adjust_cmdfunc(struct nand_chip *chip);
>  int nand_legacy_check_hooks(struct nand_chip *chip);
>  
>  /* ONFI functions */
> -u16 onfi_crc16(u16 crc, u8 const *p, size_t len);
>  int nand_onfi_detect(struct nand_chip *chip);
>  
>  /* JEDEC functions */
> diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
> index 339ac798568e..2c8a05a02bb0 100644
> --- a/include/linux/mtd/onfi.h
> +++ b/include/linux/mtd/onfi.h
> @@ -10,6 +10,7 @@
>  #ifndef __LINUX_MTD_ONFI_H
>  #define __LINUX_MTD_ONFI_H
>  
> +#include <linux/mtd/nand.h>

This should be removed, or at least not added at this moment.

>  #include <linux/types.h>
>  
>  /* ONFI version bits */
> @@ -175,4 +176,12 @@ struct onfi_params {
>  	u8 vendor[88];
>  };
>  
> +/* ONFI functions */
> +u16 onfi_crc16(u16 crc, u8 const *p, size_t len);
> +void nand_bit_wise_majority(const void **srcbufs,
> +			    unsigned int nsrcbufs,
> +			    void *dstbuf,
> +			    unsigned int bufsize);

Don't export this function while you don't use it from elsewhere.

> +void sanitize_string(u8 *s, size_t len);

This one is used by jedec code and has no onfi-related logic, so you
may want to export it (only when you will use it) in another header
like linux/mtd/nand.h

> +
>  #endif /* __LINUX_MTD_ONFI_H */

Thanks,
Miquèl

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

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

* Re: [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c
  2019-07-22  5:56 ` [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c shiva.linuxworks
@ 2019-08-07  9:03   ` Miquel Raynal
  2019-08-19  8:36     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07  9:03 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi shiva.linuxworks@gmail.com,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:15 +0200:

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

"mtd: nand: move ONFI specific helpers to nand/onfi.c"?

> 
> These functions are support functions for enabling ONFI standard and
> common between raw NAND and SPI NAND.

"
These are ONFI specific helpers that might be shared between raw and
SPI NAND logics, move them to a generic place.

While at it, add kernel doc on the function parameters.
"

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/Makefile        |  2 +-
>  drivers/mtd/nand/onfi.c          | 89 ++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/raw/nand_base.c | 18 -------
>  drivers/mtd/nand/raw/nand_onfi.c | 43 ---------------
>  4 files changed, 90 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/mtd/nand/onfi.c
> 
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 7ecd80c0a66e..221945c223c3 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -nandcore-objs := core.o bbt.o
> +nandcore-objs := core.o bbt.o onfi.o
>  obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
>  
>  obj-y	+= onenand/
> diff --git a/drivers/mtd/nand/onfi.c b/drivers/mtd/nand/onfi.c
> new file mode 100644
> index 000000000000..7aaf36dfc5e0
> --- /dev/null
> +++ b/drivers/mtd/nand/onfi.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define pr_fmt(fmt)     "nand-onfi: " fmt
> +
> +#include <linux/mtd/onfi.h>
> +#include <linux/mtd/nand.h>
> +
> +/**
> + * onfi_crc16() - Check CRC of ONFI table

There is no check in this function.

                     Derive the CRC of an ONFI table

> + * @crc: base CRC
> + * @p: buffer pointing to ONFI table
                            ^ the
> + * @len: length of ONFI table
                     ^the
> + *
> + * Return: CRC of the ONFI table
      @return: the CRC of the given ONFI table

> + */
> +u16 onfi_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;
> +}
> +EXPORT_SYMBOL_GPL(onfi_crc16);
> +
> +/**
> + * nand_bit_wise_majority() - Recover data with bit-wise majority
> + * @srcbufs: buffer pointing to ONFI table
> + * @nsrcbufs: length of ONFI table
                         ^the
> + * @dstbuf: valid ONFI table to be returned
> + * @bufsize: length og valid ONFI table
                       of the valid...
> + *

Extra line

> + */
> +void nand_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;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(nand_bit_wise_majority);
> +
> +/**
> + * sanitize_string() - Sanitize ONFI strings so we can safely print them

It is used by JEDEC logic so this should be moved elsewhere and not
refer to any ONFI naming.

> + * @s: string to be sanitized
> + * @len: length of the string
> + *
> + */
> +void sanitize_string(u8 *s, size_t len)
> +{
> +	ssize_t i;
> +
> +	/* Null terminate */
> +	s[len - 1] = 0;
> +
> +	/* Remove non printable chars */
> +	for (i = 0; i < len - 1; i++) {
> +		if (s[i] < ' ' || s[i] > 127)
> +			s[i] = '?';
> +	}
> +
> +	/* Remove trailing spaces */
> +	strim(s);
> +}
> +EXPORT_SYMBOL_GPL(sanitize_string);
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 6ecd1c496ce3..c198829bcd79 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4375,24 +4375,6 @@ static void nand_set_defaults(struct nand_chip *chip)
>  		chip->buf_align = 1;
>  }
>  
> -/* Sanitize ONFI strings so we can safely print them */
> -void sanitize_string(uint8_t *s, size_t len)
> -{
> -	ssize_t i;
> -
> -	/* Null terminate */
> -	s[len - 1] = 0;
> -
> -	/* Remove non printable chars */
> -	for (i = 0; i < len - 1; i++) {
> -		if (s[i] < ' ' || s[i] > 127)
> -			s[i] = '?';
> -	}
> -
> -	/* Remove trailing spaces */
> -	strim(s);
> -}
> -
>  /*
>   * nand_id_has_period - Check if an ID string has a given wraparound period
>   * @id_data: the ID string
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 0b879bd0a68c..2e8edfa636ef 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -16,18 +16,6 @@
>  
>  #include "internals.h"
>  
> -u16 onfi_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;
> -}
> -
>  /* Parse the Extended Parameter Page. */
>  static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
>  					    struct nand_onfi_params *p)
> @@ -103,37 +91,6 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
>  	return ret;
>  }
>  
> -/*
> - * Recover data with bit-wise majority
> - */
> -static void nand_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;
> -	}
> -}
> -
>  /*
>   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
>   */

Thanks,
Miquèl

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

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

* Re: [PATCH 3/8] mtd: nand: create ONFI table parsing instance
  2019-07-22  5:56 ` [PATCH 3/8] mtd: nand: create ONFI table parsing instance shiva.linuxworks
@ 2019-08-07  9:09   ` Miquel Raynal
  0 siblings, 0 replies; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07  9:09 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:16 +0200:

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

"Create one generic ONFI table parsing instance"

> 
> ONFI table parsing is common, as most of the variables are common
> between raw and SPI NAND. The parsing function is instantiated in
> onfi.c, which fills ONFI parameters into nand_memory_organization.

... into nand_memory_organization just as before.

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/onfi.c          | 32 ++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/raw/nand_onfi.c | 22 ++--------------------
>  include/linux/mtd/onfi.h         |  2 ++
>  3 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/nand/onfi.c b/drivers/mtd/nand/onfi.c
> index 7aaf36dfc5e0..e78700894aea 100644
> --- a/drivers/mtd/nand/onfi.c
> +++ b/drivers/mtd/nand/onfi.c
> @@ -87,3 +87,35 @@ void sanitize_string(u8 *s, size_t len)
>  	strim(s);
>  }
>  EXPORT_SYMBOL_GPL(sanitize_string);
> +
> +/**
> + * fill_nand_memorg() - Parse ONFI table and fill memorg

      ^ parse_onfi_params() - Parse an ONFI table and fill a memory
      organization structure

> + * @memorg: NAND memorg to be filled

                    memory organization core structure to be filled

> + * @p: ONFI table to be parsed
> + *
> + */
> +void parse_onfi_params(struct nand_memory_organization *memorg,
> +		       struct nand_onfi_params *p)
> +{
> +	memorg->pagesize = le32_to_cpu(p->byte_per_page);
> +
> +	/*
> +	 * pages_per_block and blocks_per_lun may not be a power-of-2 size
> +	 * (don't ask me who thought of this...). MTD assumes that these
> +	 * dimensions will be power-of-2, so just truncate the remaining area.
> +	 */
> +	memorg->pages_per_eraseblock =
> +			1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
> +
> +	memorg->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> +
> +	memorg->luns_per_target = p->lun_count;
> +	memorg->planes_per_lun = 1 << p->interleaved_bits;
> +
> +	/* See erasesize comment */
> +	memorg->eraseblocks_per_lun =
> +		1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
> +	memorg->max_bad_eraseblocks_per_lun = le32_to_cpu(p->blocks_per_lun);
> +	memorg->bits_per_cell = p->bits_per_cell;
> +}
> +EXPORT_SYMBOL_GPL(parse_onfi_params);
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 2e8edfa636ef..263796d3298c 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -181,30 +181,12 @@ int nand_onfi_detect(struct nand_chip *chip)
>  		goto free_onfi_param_page;
>  	}
>  
> -	memorg->pagesize = le32_to_cpu(p->byte_per_page);
> -	mtd->writesize = memorg->pagesize;
> +	parse_onfi_params(memorg, p);
>  
> -	/*
> -	 * pages_per_block and blocks_per_lun may not be a power-of-2 size
> -	 * (don't ask me who thought of this...). MTD assumes that these
> -	 * dimensions will be power-of-2, so just truncate the remaining area.
> -	 */
> -	memorg->pages_per_eraseblock =
> -			1 << (fls(le32_to_cpu(p->pages_per_block)) - 1);
> +	mtd->writesize = memorg->pagesize;
>  	mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
> -
> -	memorg->oobsize = le16_to_cpu(p->spare_bytes_per_page);
>  	mtd->oobsize = memorg->oobsize;
>  
> -	memorg->luns_per_target = p->lun_count;
> -	memorg->planes_per_lun = 1 << p->interleaved_bits;
> -
> -	/* See erasesize comment */
> -	memorg->eraseblocks_per_lun =
> -		1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
> -	memorg->max_bad_eraseblocks_per_lun = le32_to_cpu(p->blocks_per_lun);
> -	memorg->bits_per_cell = p->bits_per_cell;
> -
>  	if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
> diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
> index 2c8a05a02bb0..4cacf4e9db6d 100644
> --- a/include/linux/mtd/onfi.h
> +++ b/include/linux/mtd/onfi.h
> @@ -183,5 +183,7 @@ void nand_bit_wise_majority(const void **srcbufs,
>  			    void *dstbuf,
>  			    unsigned int bufsize);
>  void sanitize_string(u8 *s, size_t len);
> +void parse_onfi_params(struct nand_memory_organization *memorg,
> +		       struct nand_onfi_params *p);
>  
>  #endif /* __LINUX_MTD_ONFI_H */




Thanks,
Miquèl

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

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

* Re: [PATCH 4/8] mtd: spinand: enabled parameter page support
  2019-07-22  5:56 ` [PATCH 4/8] mtd: spinand: enabled parameter page support shiva.linuxworks
@ 2019-08-07  9:48   ` Miquel Raynal
  2019-08-19  8:51     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07  9:48 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200:

"mtd: spinand: enable parameter page support"

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> Some of the SPI NAND devices has parameter page, which is similar to
                 -             have a
> ONFI table.
  regular raw NAND ONFI tables.

> 
> But, it may not be self sufficient to propagate all the required
  As it may not be
> parameters. Fixup function has been added in struct manufacturer to
            , a fixup        is being added in the manufacturer structure
> accommodate this.

The fixup function sentence should be dropped from the commit message,
see below.

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/spi/core.c | 134 ++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h |   3 +
>  2 files changed, 137 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 89f6beefb01c..7ae76dab9141 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -400,6 +400,131 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
>  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
>  }
>  
> +/**
> + * spinand_read_param_page_op - Read parameter page operation

Again, the name in the doc does not fit the function you describe

> + * @spinand: the spinand
                    SPI-NAND chip

Shiva, there are way too much typos and shortcuts in your series.
Please be more careful otherwise we can't focus on the technical
aspects. I am not a native English speaker at all but please, plain
English is not C code. We talk SPI-NAND and not spinand, we say
structure and not struct, acronyms are uppercase, etc.

> + * @page: page number where parameter page tables can be found
                              ^ the
> + * @buf: buffer used to store the parameter page
> + * @len: length of the buffer
> + *
> + * Read parameter page
          the
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + */
> +static int spinand_parameter_page_read(struct spinand_device *spinand,
> +				       u8 page, void *buf, unsigned int len)
> +{
> +	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> +	struct spi_mem_op pread_cache_op =
> +				SPINAND_PAGE_READ_FROM_CACHE_OP(false,
> +								0,
> +								1,
> +								buf,
> +								len);

That's ok if you cross the 80 characters boundary here. You may put "0,
1," on the first line and "buf, len);" on the second.

> +	u8 feature;
> +	u8 status;
> +	int ret;
> +
> +	if (len && !buf)
> +		return -EINVAL;
> +
> +	ret = spinand_read_reg_op(spinand, REG_CFG,
> +				  &feature);
> +	if (ret)
> +		return ret;
> +
> +	/* CFG_OTP_ENABLE is used to enable parameter page access */
> +	feature |= CFG_OTP_ENABLE;
> +
> +	spinand_write_reg_op(spinand, REG_CFG, feature);
> +
> +	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> +	if (ret)
> +		return ret;
> +
> +	ret = spinand_wait(spinand, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> +	if (ret)
> +		return ret;
> +
> +	ret = spinand_read_reg_op(spinand, REG_CFG,
> +				  &feature);
> +	if (ret)
> +		return ret;
> +
> +	feature &= ~CFG_OTP_ENABLE;
> +
> +	spinand_write_reg_op(spinand, REG_CFG, feature);
> +
> +	return 0;
> +}
> +
Add the kernel doc please

Change the below function so that it returns 1 if the page was
detected, 0 if it did not, an negative error code otherwise.

> +static int spinand_param_page_detect(struct spinand_device *spinand)
> +{
> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> +	struct nand_memory_organization *memorg;
> +	struct nand_onfi_params *p;
> +	struct nand_device *base = spinand_to_nand(spinand);
> +	int i, ret;
> +
> +	memorg = nanddev_get_memorg(base);
> +
> +	/* Allocate buffer to hold parameter page */
> +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * 3);
> +	if (ret) {
> +		ret = 0;

No, you should return the error in case of error. You will later handle
the fact that there is no parameter page.

> +		goto free_param_page;
> +	}
> +
> +	for (i = 0; i < 3; i++) {
> +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
                                                           ^
If you force the parameter page to be 254 bytes long it means you limit
yourself to ONFI standard. That's not a problem, but then you should
mention it in the function name.

> +				le16_to_cpu(p->crc)) {
> +			if (i)
> +				memcpy(p, &p[i], sizeof(*p));
> +			break;
> +		}
> +	}
> +
> +	if (i == 3) {
> +		const void *srcbufs[3] = {p, p + 1, p + 2};
> +
> +		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
> +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> +				       sizeof(*p));
> +
> +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> +				le16_to_cpu(p->crc)) {
> +			pr_err("ONFI parameter recovery failed, aborting\n");
> +			goto free_param_page;
> +		}
> +	}

The whole for-loop and the if (i==3) condition is exactly the same as
for raw NANDs and must be extracted in a generic function:
1/ extract the function from nand/raw/nand_onfi.c and put it in
nand/onfi.c.
2/ then use it in this patch.

> +
> +	parse_onfi_params(memorg, p);
> +
> +	mtd->writesize = memorg->pagesize;
> +	mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
> +	mtd->oobsize = memorg->oobsize;

This will be handled by nanddev_init, should be removed.

> +
> +	/* Manufacturers may interpret the parameter page differently */
> +	if (spinand->manufacturer->ops->fixup_param_page)
> +		spinand->manufacturer->ops->fixup_param_page(spinand, p);

The whole "manufacturer fixup" should be done separately.

> +
> +	/* Identification done, free the full parameter page and exit */
> +	ret = 1;
> +
> +free_param_page:
> +	kfree(p);
> +
> +	return ret;
> +}
> +
>  static int spinand_check_ecc_status(struct spinand_device *spinand, u8 status)
>  {
>  	struct nand_device *nand = spinand_to_nand(spinand);
> @@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device *spinand)
>  		return ret;
>  	}
>  
> +	if (!spinand->base.memorg.pagesize) {
> +		ret = spinand_param_page_detect(spinand);
> +		if (ret <= 0) {
> +			dev_err(dev, "no parameter page for %*phN\n",

Not sure at this stage dev will give something meaningful. Anyway I
don't think we should scream at the user if his NAND is not an ONFI one
so please return an error only if ret < 0. If ret == 0 or ret == 1,
don't warn the user.

> +				SPINAND_MAX_ID_LEN, spinand->id.data);
> +			return -ENODEV;
> +		}
> +	}
> +
>  	if (nand->memorg.ntargets > 1 && !spinand->select_target) {
>  		dev_err(dev,
>  			"SPI NANDs with more than one die must implement ->select_target()\n");
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 4ea558bd3c46..fea820a20bc9 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -15,6 +15,7 @@
>  #include <linux/mtd/nand.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi-mem.h>
> +#include <linux/mtd/onfi.h>
>  
>  /**
>   * Standard SPI NAND flash operations
> @@ -209,6 +210,8 @@ struct spinand_manufacturer_ops {
>  	int (*detect)(struct spinand_device *spinand);
>  	int (*init)(struct spinand_device *spinand);
>  	void (*cleanup)(struct spinand_device *spinand);
> +	void (*fixup_param_page)(struct spinand_device *spinand,
> +				 struct nand_onfi_params *p);

Please do this in a separate patch.

>  };
>  
>  /**

Thanks,
Miquèl

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

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

* Re: [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver
  2019-07-22  5:56 ` [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver shiva.linuxworks
@ 2019-08-07  9:51   ` Miquel Raynal
  0 siblings, 0 replies; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07  9:51 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:18 +0200:

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

"mtd: spinand: micron: Rename helpers and structures to be more generic"

> Generalize OOB layout structure and function names.

Change the prefix of Micron driver's functions and structures before
supporting more chips: s/mt29f2g01abagd/micron/

> 
> 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..95bc5264ebc1 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_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_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_ooblayout_ops = {
> +	.ecc = micron_ooblayout_ecc,
> +	.free = micron_ooblayout_free,
>  };
>  
> -static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
> -					 u8 status)
> +static int micron_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_ooblayout_ops,
> +				     micron_ecc_get_status)),
>  };
>  
>  static int micron_spinand_detect(struct spinand_device *spinand)


Thanks,
Miquèl

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

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

* Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-07-22  5:56 ` [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic shiva.linuxworks
@ 2019-08-07 10:04   ` Miquel Raynal
  2019-08-19  9:03     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 1 reply; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07 10:04 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200:

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

I am not sure the "turn implemenatation generic" title describes what
you do.
 
> Driver is redesigned using parameter page to support Micron SPI NAND
> flashes.

Redesigned is perhaps a bit too much.

"
> The reason why spinand_select_op_variant globalized is that the Micron
> driver no longer calling spinand_match_and_init.
> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/spi/core.c   |  2 +-
>  drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++----------
>  include/linux/mtd/spinand.h   |  4 +++
>  3 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 7ae76dab9141..aae715d388b7 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct spinand_device *spinand)
>  		return spinand->manufacturer->ops->cleanup(spinand);
>  }
>  
> -static const struct spi_mem_op *
> +const struct spi_mem_op *
>  spinand_select_op_variant(struct spinand_device *spinand,
>  			  const struct spinand_op_variants *variants)
>  {
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 95bc5264ebc1..6fde93ec23a1 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -90,22 +90,10 @@ static int micron_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, 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_ooblayout_ops,
> -				     micron_ecc_get_status)),
> -};
> -

I don't know if it is wise to drop this structure.

>  static int micron_spinand_detect(struct spinand_device *spinand)
>  {
> +	const struct spi_mem_op *op;
>  	u8 *id = spinand->id.data;
> -	int ret;
>  
>  	/*
>  	 * Micron SPI NAND read ID need a dummy byte,
> @@ -114,16 +102,55 @@ 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]);

I am not sure this is the right solution. I would keep this call and
overwrite what you need to overwrite with the fixup hook.

> -	if (ret)
> -		return ret;
> +	spinand->flags = 0;
> +	spinand->eccinfo.get_status = micron_ecc_get_status;
> +	spinand->eccinfo.ooblayout = &micron_ooblayout_ops;
> +
> +	op = spinand_select_op_variant(spinand,
> +				       &read_cache_variants);
> +	if (!op)
> +		return -ENOTSUPP;
> +
> +	spinand->op_templates.read_cache = op;
> +
> +	op = spinand_select_op_variant(spinand,
> +				       &write_cache_variants);
> +	if (!op)
> +		return -ENOTSUPP;
> +
> +	spinand->op_templates.write_cache = op;
> +
> +	op = spinand_select_op_variant(spinand,
> +				       &update_cache_variants);
> +	spinand->op_templates.update_cache = op;
>  
>  	return 1;
>  }
>  
> +static void micron_fixup_param_page(struct spinand_device *spinand,
> +				    struct nand_onfi_params *p)
> +{
> +	/*
> +	 * As per Micron datasheets vendor[83] is defined as
> +	 * die_select_feature
> +	 */
> +	if (p->vendor[83] && !p->interleaved_bits)
> +		spinand->base.memorg.planes_per_lun = 1 << p->vendor[0];
> +
> +	spinand->base.memorg.ntargets = p->lun_count;
> +	spinand->base.memorg.luns_per_target = 1;
> +
> +	/*
> +	 * As per Micron datasheets,
> +	 * vendor[82] is ECC maximum correctability
> +	 */
> +	spinand->base.eccreq.strength = p->vendor[82];
> +	spinand->base.eccreq.step_size = 512;
> +}
> +
>  static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
>  	.detect = micron_spinand_detect,
> +	.fixup_param_page = micron_fixup_param_page,
>  };
>  
>  const struct spinand_manufacturer micron_spinand_manufacturer = {
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index fea820a20bc9..ddb2194273a8 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -461,4 +461,8 @@ int spinand_match_and_init(struct spinand_device *dev,
>  int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
>  int spinand_select_target(struct spinand_device *spinand, unsigned int target);
>  
> +const struct spi_mem_op *
> +spinand_select_op_variant(struct spinand_device *spinand,
> +			  const struct spinand_op_variants *variants);
> +
>  #endif /* __LINUX_MTD_SPINAND_H */




Thanks,
Miquèl

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

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

* Re: [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes
  2019-07-22  5:56 ` [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes shiva.linuxworks
@ 2019-08-07 10:05   ` Miquel Raynal
  0 siblings, 0 replies; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07 10:05 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:20 +0200:

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> M70A series flashes by default enable continuous read feature (BIT0 in
> configuration register). This feature will not expose the ECC to host
> and causing read failure.

This is not readable enough, can you rephrase? Besides that you can add
my 

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/spi/micron.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 6fde93ec23a1..1e28ea3d1362 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -127,6 +127,15 @@ static int micron_spinand_detect(struct spinand_device *spinand)
>  	return 1;
>  }
>  
> +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 void micron_fixup_param_page(struct spinand_device *spinand,
>  				    struct nand_onfi_params *p)
>  {
> @@ -150,6 +159,7 @@ static void micron_fixup_param_page(struct spinand_device *spinand,
>  
>  static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
>  	.detect = micron_spinand_detect,
> +	.init = micron_spinand_init,
>  	.fixup_param_page = micron_fixup_param_page,
>  };
>  


Thanks,
Miquèl

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

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

* Re: [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die
  2019-07-22  5:56 ` [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die shiva.linuxworks
@ 2019-08-07 10:08   ` Miquel Raynal
  0 siblings, 0 replies; 28+ messages in thread
From: Miquel Raynal @ 2019-08-07 10:08 UTC (permalink / raw)
  To: shiva.linuxworks
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse, Shivamurthy Shastri

Hi Shiva,

shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:21 +0200:

"with multiple dies" in the title

> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> Some of the Micron flashes has multi-die, and need to select the die
                             have multiple dies and
> each time while accessing it.
> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>  drivers/mtd/nand/spi/micron.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 1e28ea3d1362..fa2b43caf39d 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -90,6 +90,19 @@ static int micron_ecc_get_status(struct spinand_device *spinand,
>  	return -EINVAL;
>  }
>  

Please explain in a comment what you do here, like in the commit title.

> +static int micron_select_target(struct spinand_device *spinand,
> +				unsigned int target)
> +{
> +	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> +						      spinand->scratchbuf);
> +
> +	if (target == 1)
> +		target = 0x40;
> +
> +	*spinand->scratchbuf = target;

if (target == 1)
        *spinand->scratchbuf = 0x40

space

> +	return spi_mem_exec_op(spinand->spimem, &op);
> +}
> +
>  static int micron_spinand_detect(struct spinand_device *spinand)
>  {
>  	const struct spi_mem_op *op;
> @@ -105,6 +118,7 @@ static int micron_spinand_detect(struct spinand_device *spinand)
>  	spinand->flags = 0;
>  	spinand->eccinfo.get_status = micron_ecc_get_status;
>  	spinand->eccinfo.ooblayout = &micron_ooblayout_ops;
> +	spinand->select_target = micron_select_target;
>  
>  	op = spinand_select_op_variant(spinand,
>  				       &read_cache_variants);




Thanks,
Miquèl

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

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

* RE: [EXT] Re: [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h
  2019-08-07  8:34   ` Miquel Raynal
@ 2019-08-19  8:35     ` " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 0 replies; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-08-19  8:35 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hi Miquel,

Thanks for reviewing.

> 
> Hi Shiva,
> 
> shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:14 +0200:
> 
> > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> >
> > These functions will be used by both raw NAND and SPI NAND, which
> > supports ONFI like standards.
> 
> This is not exactly what you do. Why not:
> 
> mtd: nand: export ONFI related functions to onfi.h
> 
> These functions can be used by all flavors of NAND chips (raw, SPI)
> which may all follow ONFI standards. Export the related functions in
> the onfi.h generic file.
> 

Looks good. I will use this.

> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > ---
> >  drivers/mtd/nand/raw/internals.h | 1 -
> >  include/linux/mtd/onfi.h         | 9 +++++++++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/nand/raw/internals.h
> b/drivers/mtd/nand/raw/internals.h
> > index cba6fe7dd8c4..ed323087d884 100644
> > --- a/drivers/mtd/nand/raw/internals.h
> > +++ b/drivers/mtd/nand/raw/internals.h
> > @@ -140,7 +140,6 @@ void nand_legacy_adjust_cmdfunc(struct
> nand_chip *chip);
> >  int nand_legacy_check_hooks(struct nand_chip *chip);
> >
> >  /* ONFI functions */
> > -u16 onfi_crc16(u16 crc, u8 const *p, size_t len);
> >  int nand_onfi_detect(struct nand_chip *chip);
> >
> >  /* JEDEC functions */
> > diff --git a/include/linux/mtd/onfi.h b/include/linux/mtd/onfi.h
> > index 339ac798568e..2c8a05a02bb0 100644
> > --- a/include/linux/mtd/onfi.h
> > +++ b/include/linux/mtd/onfi.h
> > @@ -10,6 +10,7 @@
> >  #ifndef __LINUX_MTD_ONFI_H
> >  #define __LINUX_MTD_ONFI_H
> >
> > +#include <linux/mtd/nand.h>
> 
> This should be removed, or at least not added at this moment.
> 

okay.

> >  #include <linux/types.h>
> >
> >  /* ONFI version bits */
> > @@ -175,4 +176,12 @@ struct onfi_params {
> >  	u8 vendor[88];
> >  };
> >
> > +/* ONFI functions */
> > +u16 onfi_crc16(u16 crc, u8 const *p, size_t len);
> > +void nand_bit_wise_majority(const void **srcbufs,
> > +			    unsigned int nsrcbufs,
> > +			    void *dstbuf,
> > +			    unsigned int bufsize);
> 
> Don't export this function while you don't use it from elsewhere.
> 

The function will be moved to nand/onfi.c in next patch and will be
used by both raw and SPI NAND.

> > +void sanitize_string(u8 *s, size_t len);
> 
> This one is used by jedec code and has no onfi-related logic, so you
> may want to export it (only when you will use it) in another header
> like linux/mtd/nand.h

okay, I will keep this as it is.

> 
> > +
> >  #endif /* __LINUX_MTD_ONFI_H */
> 
> Thanks,
> Miquèl

Thanks,
Shiva

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

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

* RE: [EXT] Re: [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c
  2019-08-07  9:03   ` Miquel Raynal
@ 2019-08-19  8:36     ` " Shivamurthy Shastri (sshivamurthy)
  0 siblings, 0 replies; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-08-19  8:36 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hi Miquel,

> 
> Hi shiva.linuxworks@gmail.com,
> 
> shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:15 +0200:
> 
> > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> "mtd: nand: move ONFI specific helpers to nand/onfi.c"?

I will use this.

> 
> >
> > These functions are support functions for enabling ONFI standard and
> > common between raw NAND and SPI NAND.
> 
> "
> These are ONFI specific helpers that might be shared between raw and
> SPI NAND logics, move them to a generic place.
> 
> While at it, add kernel doc on the function parameters.
> "
> 

I will use this.

> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > ---
> >  drivers/mtd/nand/Makefile        |  2 +-
> >  drivers/mtd/nand/onfi.c          | 89
> ++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/raw/nand_base.c | 18 -------
> >  drivers/mtd/nand/raw/nand_onfi.c | 43 ---------------
> >  4 files changed, 90 insertions(+), 62 deletions(-)
> >  create mode 100644 drivers/mtd/nand/onfi.c
> >
> > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> > index 7ecd80c0a66e..221945c223c3 100644
> > --- a/drivers/mtd/nand/Makefile
> > +++ b/drivers/mtd/nand/Makefile
> > @@ -1,6 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >
> > -nandcore-objs := core.o bbt.o
> > +nandcore-objs := core.o bbt.o onfi.o
> >  obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
> >
> >  obj-y	+= onenand/
> > diff --git a/drivers/mtd/nand/onfi.c b/drivers/mtd/nand/onfi.c
> > new file mode 100644
> > index 000000000000..7aaf36dfc5e0
> > --- /dev/null
> > +++ b/drivers/mtd/nand/onfi.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define pr_fmt(fmt)     "nand-onfi: " fmt
> > +
> > +#include <linux/mtd/onfi.h>
> > +#include <linux/mtd/nand.h>
> > +
> > +/**
> > + * onfi_crc16() - Check CRC of ONFI table
> 
> There is no check in this function.
> 
>                      Derive the CRC of an ONFI table

okay.

> 
> > + * @crc: base CRC
> > + * @p: buffer pointing to ONFI table
>                             ^ the
> > + * @len: length of ONFI table
>                      ^the
> > + *
> > + * Return: CRC of the ONFI table
>       @return: the CRC of the given ONFI table
> 
> > + */
> > +u16 onfi_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;
> > +}
> > +EXPORT_SYMBOL_GPL(onfi_crc16);
> > +
> > +/**
> > + * nand_bit_wise_majority() - Recover data with bit-wise majority
> > + * @srcbufs: buffer pointing to ONFI table
> > + * @nsrcbufs: length of ONFI table
>                          ^the
> > + * @dstbuf: valid ONFI table to be returned
> > + * @bufsize: length og valid ONFI table
>                        of the valid...
> > + *
> 
> Extra line
> 
> > + */
> > +void nand_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;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(nand_bit_wise_majority);
> > +
> > +/**
> > + * sanitize_string() - Sanitize ONFI strings so we can safely print them
> 
> It is used by JEDEC logic so this should be moved elsewhere and not
> refer to any ONFI naming.

okay, I will move this.

> 
> > + * @s: string to be sanitized
> > + * @len: length of the string
> > + *
> > + */
> > +void sanitize_string(u8 *s, size_t len)
> > +{
> > +	ssize_t i;
> > +
> > +	/* Null terminate */
> > +	s[len - 1] = 0;
> > +
> > +	/* Remove non printable chars */
> > +	for (i = 0; i < len - 1; i++) {
> > +		if (s[i] < ' ' || s[i] > 127)
> > +			s[i] = '?';
> > +	}
> > +
> > +	/* Remove trailing spaces */
> > +	strim(s);
> > +}
> > +EXPORT_SYMBOL_GPL(sanitize_string);
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> > index 6ecd1c496ce3..c198829bcd79 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -4375,24 +4375,6 @@ static void nand_set_defaults(struct nand_chip
> *chip)
> >  		chip->buf_align = 1;
> >  }
> >
> > -/* Sanitize ONFI strings so we can safely print them */
> > -void sanitize_string(uint8_t *s, size_t len)
> > -{
> > -	ssize_t i;
> > -
> > -	/* Null terminate */
> > -	s[len - 1] = 0;
> > -
> > -	/* Remove non printable chars */
> > -	for (i = 0; i < len - 1; i++) {
> > -		if (s[i] < ' ' || s[i] > 127)
> > -			s[i] = '?';
> > -	}
> > -
> > -	/* Remove trailing spaces */
> > -	strim(s);
> > -}
> > -
> >  /*
> >   * nand_id_has_period - Check if an ID string has a given wraparound
> period
> >   * @id_data: the ID string
> > diff --git a/drivers/mtd/nand/raw/nand_onfi.c
> b/drivers/mtd/nand/raw/nand_onfi.c
> > index 0b879bd0a68c..2e8edfa636ef 100644
> > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > @@ -16,18 +16,6 @@
> >
> >  #include "internals.h"
> >
> > -u16 onfi_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;
> > -}
> > -
> >  /* Parse the Extended Parameter Page. */
> >  static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
> >  					    struct nand_onfi_params *p)
> > @@ -103,37 +91,6 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> >  	return ret;
> >  }
> >
> > -/*
> > - * Recover data with bit-wise majority
> > - */
> > -static void nand_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;
> > -	}
> > -}
> > -
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> >   */
> 
> Thanks,
> Miquèl

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

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

* RE: [EXT] Re: [PATCH 4/8] mtd: spinand: enabled parameter page support
  2019-08-07  9:48   ` Miquel Raynal
@ 2019-08-19  8:51     ` " Shivamurthy Shastri (sshivamurthy)
  2019-08-19  9:21       ` Miquel Raynal
  0 siblings, 1 reply; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-08-19  8:51 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hi Miquel,

> 
> Hi Shiva,
> 
> shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200:
> 
> "mtd: spinand: enable parameter page support"
> 
> > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> >
> > Some of the SPI NAND devices has parameter page, which is similar to
>                  -             have a
> > ONFI table.
>   regular raw NAND ONFI tables.
> 
> >
> > But, it may not be self sufficient to propagate all the required
>   As it may not be
> > parameters. Fixup function has been added in struct manufacturer to
>             , a fixup        is being added in the manufacturer structure
> > accommodate this.
> 
> The fixup function sentence should be dropped from the commit message,
> see below.

Okay, I will create separate patch for fixup function.

> 
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > ---
> >  drivers/mtd/nand/spi/core.c | 134
> ++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/spinand.h |   3 +
> >  2 files changed, 137 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 89f6beefb01c..7ae76dab9141 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct
> spinand_device *spinand, u8 lock)
> >  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> >  }
> >
> > +/**
> > + * spinand_read_param_page_op - Read parameter page operation
> 
> Again, the name in the doc does not fit the function you describe
> 
> > + * @spinand: the spinand
>                     SPI-NAND chip
> 
> Shiva, there are way too much typos and shortcuts in your series.
> Please be more careful otherwise we can't focus on the technical
> aspects. I am not a native English speaker at all but please, plain
> English is not C code. We talk SPI-NAND and not spinand, we say
> structure and not struct, acronyms are uppercase, etc.
> 

Sorry for the inconvenience caused, I will take care from next time.

> > + * @page: page number where parameter page tables can be found
>                               ^ the
> > + * @buf: buffer used to store the parameter page
> > + * @len: length of the buffer
> > + *
> > + * Read parameter page
>           the
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +static int spinand_parameter_page_read(struct spinand_device *spinand,
> > +				       u8 page, void *buf, unsigned int len)
> > +{
> > +	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> > +	struct spi_mem_op pread_cache_op =
> > +
> 	SPINAND_PAGE_READ_FROM_CACHE_OP(false,
> > +								0,
> > +								1,
> > +								buf,
> > +								len);
> 
> That's ok if you cross the 80 characters boundary here. You may put "0,
> 1," on the first line and "buf, len);" on the second.
> 
> > +	u8 feature;
> > +	u8 status;
> > +	int ret;
> > +
> > +	if (len && !buf)
> > +		return -EINVAL;
> > +
> > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > +				  &feature);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* CFG_OTP_ENABLE is used to enable parameter page access */
> > +	feature |= CFG_OTP_ENABLE;
> > +
> > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > +
> > +	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = spinand_wait(spinand, &status);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > +				  &feature);
> > +	if (ret)
> > +		return ret;
> > +
> > +	feature &= ~CFG_OTP_ENABLE;
> > +
> > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > +
> > +	return 0;
> > +}
> > +
> Add the kernel doc please
> 
> Change the below function so that it returns 1 if the page was
> detected, 0 if it did not, an negative error code otherwise.
> 
> > +static int spinand_param_page_detect(struct spinand_device *spinand)
> > +{
> > +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> > +	struct nand_memory_organization *memorg;
> > +	struct nand_onfi_params *p;
> > +	struct nand_device *base = spinand_to_nand(spinand);
> > +	int i, ret;
> > +
> > +	memorg = nanddev_get_memorg(base);
> > +
> > +	/* Allocate buffer to hold parameter page */
> > +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> > +	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) *
> 3);
> > +	if (ret) {
> > +		ret = 0;
> 
> No, you should return the error in case of error. You will later handle
> the fact that there is no parameter page.

okay.

> 
> > +		goto free_param_page;
> > +	}
> > +
> > +	for (i = 0; i < 3; i++) {
> > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
>                                                            ^
> If you force the parameter page to be 254 bytes long it means you limit
> yourself to ONFI standard. That's not a problem, but then you should
> mention it in the function name.

okay, I will mention in kernel doc.

> 
> > +				le16_to_cpu(p->crc)) {
> > +			if (i)
> > +				memcpy(p, &p[i], sizeof(*p));
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (i == 3) {
> > +		const void *srcbufs[3] = {p, p + 1, p + 2};
> > +
> > +		pr_warn("Could not find a valid ONFI parameter page, trying
> bit-wise majority to recover it\n");
> > +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > +				       sizeof(*p));
> > +
> > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > +				le16_to_cpu(p->crc)) {
> > +			pr_err("ONFI parameter recovery failed,
> aborting\n");
> > +			goto free_param_page;
> > +		}
> > +	}
> 
> The whole for-loop and the if (i==3) condition is exactly the same as
> for raw NANDs and must be extracted in a generic function:
> 1/ extract the function from nand/raw/nand_onfi.c and put it in
> nand/onfi.c.
> 2/ then use it in this patch.

I have done this intentionally, because in raw NAND case there is function
"nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) 
it is necessary to create new function.

Let me know if you have different opinion.

> 
> > +
> > +	parse_onfi_params(memorg, p);
> > +
> > +	mtd->writesize = memorg->pagesize;
> > +	mtd->erasesize = memorg->pages_per_eraseblock * memorg-
> >pagesize;
> > +	mtd->oobsize = memorg->oobsize;
> 
> This will be handled by nanddev_init, should be removed.
> 
> > +
> > +	/* Manufacturers may interpret the parameter page differently */
> > +	if (spinand->manufacturer->ops->fixup_param_page)
> > +		spinand->manufacturer->ops->fixup_param_page(spinand,
> p);
> 
> The whole "manufacturer fixup" should be done separately.
> 
> > +
> > +	/* Identification done, free the full parameter page and exit */
> > +	ret = 1;
> > +
> > +free_param_page:
> > +	kfree(p);
> > +
> > +	return ret;
> > +}
> > +
> >  static int spinand_check_ecc_status(struct spinand_device *spinand, u8
> status)
> >  {
> >  	struct nand_device *nand = spinand_to_nand(spinand);
> > @@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device
> *spinand)
> >  		return ret;
> >  	}
> >
> > +	if (!spinand->base.memorg.pagesize) {
> > +		ret = spinand_param_page_detect(spinand);
> > +		if (ret <= 0) {
> > +			dev_err(dev, "no parameter page for %*phN\n",
> 
> Not sure at this stage dev will give something meaningful. Anyway I
> don't think we should scream at the user if his NAND is not an ONFI one
> so please return an error only if ret < 0. If ret == 0 or ret == 1,
> don't warn the user.

I will do it as per your suggestion.

> 
> > +				SPINAND_MAX_ID_LEN, spinand->id.data);
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> >  	if (nand->memorg.ntargets > 1 && !spinand->select_target) {
> >  		dev_err(dev,
> >  			"SPI NANDs with more than one die must implement
> ->select_target()\n");
> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index 4ea558bd3c46..fea820a20bc9 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -15,6 +15,7 @@
> >  #include <linux/mtd/nand.h>
> >  #include <linux/spi/spi.h>
> >  #include <linux/spi/spi-mem.h>
> > +#include <linux/mtd/onfi.h>
> >
> >  /**
> >   * Standard SPI NAND flash operations
> > @@ -209,6 +210,8 @@ struct spinand_manufacturer_ops {
> >  	int (*detect)(struct spinand_device *spinand);
> >  	int (*init)(struct spinand_device *spinand);
> >  	void (*cleanup)(struct spinand_device *spinand);
> > +	void (*fixup_param_page)(struct spinand_device *spinand,
> > +				 struct nand_onfi_params *p);
> 
> Please do this in a separate patch.
> 
> >  };
> >
> >  /**
> 
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* RE: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-08-07 10:04   ` Miquel Raynal
@ 2019-08-19  9:03     ` " Shivamurthy Shastri (sshivamurthy)
  2019-08-19  9:19       ` Miquel Raynal
  2019-10-07  8:13       ` Boris Brezillon
  0 siblings, 2 replies; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-08-19  9:03 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hi Miquel,

> 
> Hi Shiva,
> 
> shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200:
> 
> > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> >
> 
> I am not sure the "turn implemenatation generic" title describes what
> you do.
> 
> > Driver is redesigned using parameter page to support Micron SPI NAND
> > flashes.
> 
> Redesigned is perhaps a bit too much.
> 
> "
> > The reason why spinand_select_op_variant globalized is that the Micron
> > driver no longer calling spinand_match_and_init.
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > ---
> >  drivers/mtd/nand/spi/core.c   |  2 +-
> >  drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++-------
> ---
> >  include/linux/mtd/spinand.h   |  4 +++
> >  3 files changed, 49 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 7ae76dab9141..aae715d388b7 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct
> spinand_device *spinand)
> >  		return spinand->manufacturer->ops->cleanup(spinand);
> >  }
> >
> > -static const struct spi_mem_op *
> > +const struct spi_mem_op *
> >  spinand_select_op_variant(struct spinand_device *spinand,
> >  			  const struct spinand_op_variants *variants)
> >  {
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 95bc5264ebc1..6fde93ec23a1 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -90,22 +90,10 @@ static int micron_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, 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_ooblayout_ops,
> > -				     micron_ecc_get_status)),
> > -};
> > -
> 
> I don't know if it is wise to drop this structure.
> 
> >  static int micron_spinand_detect(struct spinand_device *spinand)
> >  {
> > +	const struct spi_mem_op *op;
> >  	u8 *id = spinand->id.data;
> > -	int ret;
> >
> >  	/*
> >  	 * Micron SPI NAND read ID need a dummy byte,
> > @@ -114,16 +102,55 @@ 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]);
> 
> I am not sure this is the right solution. I would keep this call and
> overwrite what you need to overwrite with the fixup hook.
> 

Then, I will have dummy structure like below.

static const struct spinand_info micron_spinand_table[] = {                      
        SPINAND_INFO(NULL, 0,                                                                    
                     NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),           
                     NAND_ECCREQ(0, 0),                                                                       
                     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,              
                                              &write_cache_variants,             
                                              &update_cache_variants),           
                     0,                                                                                         
                     SPINAND_ECCINFO(&micron_ooblayout_ops,                      
                                     micron_ecc_get_status)),                    
};                                    

Let me know if you are thinking for different approach.

> > -	if (ret)
> > -		return ret;
> > +	spinand->flags = 0;
> > +	spinand->eccinfo.get_status = micron_ecc_get_status;
> > +	spinand->eccinfo.ooblayout = &micron_ooblayout_ops;
> > +
> > +	op = spinand_select_op_variant(spinand,
> > +				       &read_cache_variants);
> > +	if (!op)
> > +		return -ENOTSUPP;
> > +
> > +	spinand->op_templates.read_cache = op;
> > +
> > +	op = spinand_select_op_variant(spinand,
> > +				       &write_cache_variants);
> > +	if (!op)
> > +		return -ENOTSUPP;
> > +
> > +	spinand->op_templates.write_cache = op;
> > +
> > +	op = spinand_select_op_variant(spinand,
> > +				       &update_cache_variants);
> > +	spinand->op_templates.update_cache = op;
> >
> >  	return 1;
> >  }
> >
> > +static void micron_fixup_param_page(struct spinand_device *spinand,
> > +				    struct nand_onfi_params *p)
> > +{
> > +	/*
> > +	 * As per Micron datasheets vendor[83] is defined as
> > +	 * die_select_feature
> > +	 */
> > +	if (p->vendor[83] && !p->interleaved_bits)
> > +		spinand->base.memorg.planes_per_lun = 1 << p-
> >vendor[0];
> > +
> > +	spinand->base.memorg.ntargets = p->lun_count;
> > +	spinand->base.memorg.luns_per_target = 1;
> > +
> > +	/*
> > +	 * As per Micron datasheets,
> > +	 * vendor[82] is ECC maximum correctability
> > +	 */
> > +	spinand->base.eccreq.strength = p->vendor[82];
> > +	spinand->base.eccreq.step_size = 512;
> > +}
> > +
> >  static const struct spinand_manufacturer_ops
> micron_spinand_manuf_ops = {
> >  	.detect = micron_spinand_detect,
> > +	.fixup_param_page = micron_fixup_param_page,
> >  };
> >
> >  const struct spinand_manufacturer micron_spinand_manufacturer = {
> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index fea820a20bc9..ddb2194273a8 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -461,4 +461,8 @@ int spinand_match_and_init(struct spinand_device
> *dev,
> >  int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
> >  int spinand_select_target(struct spinand_device *spinand, unsigned int
> target);
> >
> > +const struct spi_mem_op *
> > +spinand_select_op_variant(struct spinand_device *spinand,
> > +			  const struct spinand_op_variants *variants);
> > +
> >  #endif /* __LINUX_MTD_SPINAND_H */
> 
> 
> 
> 
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* Re: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-08-19  9:03     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
@ 2019-08-19  9:19       ` Miquel Raynal
  2019-09-16 10:41         ` Shivamurthy Shastri (sshivamurthy)
  2019-10-07  8:13       ` Boris Brezillon
  1 sibling, 1 reply; 28+ messages in thread
From: Miquel Raynal @ 2019-08-19  9:19 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hi Boris,

I need your opinion on the question below.

"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
Mon, 19 Aug 2019 09:03:38 +0000:

> Hi Miquel,
> 
> > 
> > Hi Shiva,
> > 
> > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200:
> >   
> > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > >  
> > 
> > I am not sure the "turn implemenatation generic" title describes what
> > you do.
> >   
> > > Driver is redesigned using parameter page to support Micron SPI NAND
> > > flashes.  
> > 
> > Redesigned is perhaps a bit too much.
> > 
> > "  
> > > The reason why spinand_select_op_variant globalized is that the Micron
> > > driver no longer calling spinand_match_and_init.
> > >
> > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > ---
> > >  drivers/mtd/nand/spi/core.c   |  2 +-
> > >  drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++-------  
> > ---  
> > >  include/linux/mtd/spinand.h   |  4 +++
> > >  3 files changed, 49 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > index 7ae76dab9141..aae715d388b7 100644
> > > --- a/drivers/mtd/nand/spi/core.c
> > > +++ b/drivers/mtd/nand/spi/core.c
> > > @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct  
> > spinand_device *spinand)  
> > >  		return spinand->manufacturer->ops->cleanup(spinand);
> > >  }
> > >
> > > -static const struct spi_mem_op *
> > > +const struct spi_mem_op *
> > >  spinand_select_op_variant(struct spinand_device *spinand,
> > >  			  const struct spinand_op_variants *variants)
> > >  {
> > > diff --git a/drivers/mtd/nand/spi/micron.c  
> > b/drivers/mtd/nand/spi/micron.c  
> > > index 95bc5264ebc1..6fde93ec23a1 100644
> > > --- a/drivers/mtd/nand/spi/micron.c
> > > +++ b/drivers/mtd/nand/spi/micron.c
> > > @@ -90,22 +90,10 @@ static int micron_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, 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_ooblayout_ops,
> > > -				     micron_ecc_get_status)),
> > > -};
> > > -  
> > 
> > I don't know if it is wise to drop this structure.
> >   
> > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > >  {
> > > +	const struct spi_mem_op *op;
> > >  	u8 *id = spinand->id.data;
> > > -	int ret;
> > >
> > >  	/*
> > >  	 * Micron SPI NAND read ID need a dummy byte,
> > > @@ -114,16 +102,55 @@ 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]);
> > 
> > I am not sure this is the right solution. I would keep this call and
> > overwrite what you need to overwrite with the fixup hook.
> >   
> 
> Then, I will have dummy structure like below.
> 
> static const struct spinand_info micron_spinand_table[] = {                      
>         SPINAND_INFO(NULL, 0,                                                                    
>                      NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),           
>                      NAND_ECCREQ(0, 0),                                                                       
>                      SPINAND_INFO_OP_VARIANTS(&read_cache_variants,              
>                                               &write_cache_variants,             
>                                               &update_cache_variants),           
>                      0,                                                                                         
>                      SPINAND_ECCINFO(&micron_ooblayout_ops,                      
>                                      micron_ecc_get_status)),                    
> };                                    
> 
> Let me know if you are thinking for different approach.
> 

Thanks,
Miquèl

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

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

* Re: [EXT] Re: [PATCH 4/8] mtd: spinand: enabled parameter page support
  2019-08-19  8:51     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
@ 2019-08-19  9:21       ` Miquel Raynal
  2019-09-30 13:23         ` Shivamurthy Shastri (sshivamurthy)
  2019-10-07  7:52         ` Boris Brezillon
  0 siblings, 2 replies; 28+ messages in thread
From: Miquel Raynal @ 2019-08-19  9:21 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hi Boris,

Another question for you :)

"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
Mon, 19 Aug 2019 08:51:52 +0000:

> Hi Miquel,
> 
> > 
> > Hi Shiva,
> > 
> > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200:
> > 
> > "mtd: spinand: enable parameter page support"
> >   
> > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > >
> > > Some of the SPI NAND devices has parameter page, which is similar to  
> >                  -             have a  
> > > ONFI table.  
> >   regular raw NAND ONFI tables.
> >   
> > >
> > > But, it may not be self sufficient to propagate all the required  
> >   As it may not be  
> > > parameters. Fixup function has been added in struct manufacturer to  
> >             , a fixup        is being added in the manufacturer structure  
> > > accommodate this.  
> > 
> > The fixup function sentence should be dropped from the commit message,
> > see below.  
> 
> Okay, I will create separate patch for fixup function.
> 
> >   
> > >
> > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > ---
> > >  drivers/mtd/nand/spi/core.c | 134  
> > ++++++++++++++++++++++++++++++++++++  
> > >  include/linux/mtd/spinand.h |   3 +
> > >  2 files changed, 137 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > index 89f6beefb01c..7ae76dab9141 100644
> > > --- a/drivers/mtd/nand/spi/core.c
> > > +++ b/drivers/mtd/nand/spi/core.c
> > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct  
> > spinand_device *spinand, u8 lock)  
> > >  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> > >  }
> > >
> > > +/**
> > > + * spinand_read_param_page_op - Read parameter page operation  
> > 
> > Again, the name in the doc does not fit the function you describe
> >   
> > > + * @spinand: the spinand  
> >                     SPI-NAND chip
> > 
> > Shiva, there are way too much typos and shortcuts in your series.
> > Please be more careful otherwise we can't focus on the technical
> > aspects. I am not a native English speaker at all but please, plain
> > English is not C code. We talk SPI-NAND and not spinand, we say
> > structure and not struct, acronyms are uppercase, etc.
> >   
> 
> Sorry for the inconvenience caused, I will take care from next time.
> 
> > > + * @page: page number where parameter page tables can be found  
> >                               ^ the  
> > > + * @buf: buffer used to store the parameter page
> > > + * @len: length of the buffer
> > > + *
> > > + * Read parameter page  
> >           the  
> > > + *
> > > + * Returns 0 on success, a negative error code otherwise.
> > > + */
> > > +static int spinand_parameter_page_read(struct spinand_device *spinand,
> > > +				       u8 page, void *buf, unsigned int len)
> > > +{
> > > +	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> > > +	struct spi_mem_op pread_cache_op =
> > > +  
> > 	SPINAND_PAGE_READ_FROM_CACHE_OP(false,  
> > > +								0,
> > > +								1,
> > > +								buf,
> > > +								len);  
> > 
> > That's ok if you cross the 80 characters boundary here. You may put "0,
> > 1," on the first line and "buf, len);" on the second.
> >   
> > > +	u8 feature;
> > > +	u8 status;
> > > +	int ret;
> > > +
> > > +	if (len && !buf)
> > > +		return -EINVAL;
> > > +
> > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > +				  &feature);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* CFG_OTP_ENABLE is used to enable parameter page access */
> > > +	feature |= CFG_OTP_ENABLE;
> > > +
> > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > +
> > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = spinand_wait(spinand, &status);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > +				  &feature);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	feature &= ~CFG_OTP_ENABLE;
> > > +
> > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > +
> > > +	return 0;
> > > +}
> > > +  
> > Add the kernel doc please
> > 
> > Change the below function so that it returns 1 if the page was
> > detected, 0 if it did not, an negative error code otherwise.
> >   
> > > +static int spinand_param_page_detect(struct spinand_device *spinand)
> > > +{
> > > +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> > > +	struct nand_memory_organization *memorg;
> > > +	struct nand_onfi_params *p;
> > > +	struct nand_device *base = spinand_to_nand(spinand);
> > > +	int i, ret;
> > > +
> > > +	memorg = nanddev_get_memorg(base);
> > > +
> > > +	/* Allocate buffer to hold parameter page */
> > > +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > > +	if (!p)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) *  
> > 3);  
> > > +	if (ret) {
> > > +		ret = 0;  
> > 
> > No, you should return the error in case of error. You will later handle
> > the fact that there is no parameter page.  
> 
> okay.
> 
> >   
> > > +		goto free_param_page;
> > > +	}
> > > +
> > > +	for (i = 0; i < 3; i++) {
> > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==  
> >                                                            ^
> > If you force the parameter page to be 254 bytes long it means you limit
> > yourself to ONFI standard. That's not a problem, but then you should
> > mention it in the function name.  
> 
> okay, I will mention in kernel doc.
> 
> >   
> > > +				le16_to_cpu(p->crc)) {
> > > +			if (i)
> > > +				memcpy(p, &p[i], sizeof(*p));
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (i == 3) {
> > > +		const void *srcbufs[3] = {p, p + 1, p + 2};
> > > +
> > > +		pr_warn("Could not find a valid ONFI parameter page, trying  
> > bit-wise majority to recover it\n");  
> > > +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > > +				       sizeof(*p));
> > > +
> > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > > +				le16_to_cpu(p->crc)) {
> > > +			pr_err("ONFI parameter recovery failed,  
> > aborting\n");  
> > > +			goto free_param_page;
> > > +		}
> > > +	}  
> > 
> > The whole for-loop and the if (i==3) condition is exactly the same as
> > for raw NANDs and must be extracted in a generic function:
> > 1/ extract the function from nand/raw/nand_onfi.c and put it in
> > nand/onfi.c.
> > 2/ then use it in this patch.  
> 
> I have done this intentionally, because in raw NAND case there is function
> "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) 
> it is necessary to create new function.
> 
> Let me know if you have different opinion.

I don't have a strong opinion on that. Boris what do you think? Shall
we duplicate the code? It's not just about the if condition, it's the
whole for loop which is very similar.


Thanks,
Miquèl

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

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

* RE: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-08-19  9:19       ` Miquel Raynal
@ 2019-09-16 10:41         ` Shivamurthy Shastri (sshivamurthy)
  0 siblings, 0 replies; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-09-16 10:41 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hello Miquel & Boris,

Just a gentle reminder that I'd like some feedback.

Thanks,
Shiva

> 
> Hi Boris,
> 
> I need your opinion on the question below.
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote
> on
> Mon, 19 Aug 2019 09:03:38 +0000:
> 
> > Hi Miquel,
> >
> > >
> > > Hi Shiva,
> > >
> > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200:
> > >
> > > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > >
> > >
> > > I am not sure the "turn implemenatation generic" title describes what
> > > you do.
> > >
> > > > Driver is redesigned using parameter page to support Micron SPI NAND
> > > > flashes.
> > >
> > > Redesigned is perhaps a bit too much.
> > >
> > > "
> > > > The reason why spinand_select_op_variant globalized is that the
> Micron
> > > > driver no longer calling spinand_match_and_init.
> > > >
> > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > > ---
> > > >  drivers/mtd/nand/spi/core.c   |  2 +-
> > > >  drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++--
> -----
> > > ---
> > > >  include/linux/mtd/spinand.h   |  4 +++
> > > >  3 files changed, 49 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > > index 7ae76dab9141..aae715d388b7 100644
> > > > --- a/drivers/mtd/nand/spi/core.c
> > > > +++ b/drivers/mtd/nand/spi/core.c
> > > > @@ -920,7 +920,7 @@ static void
> spinand_manufacturer_cleanup(struct
> > > spinand_device *spinand)
> > > >  		return spinand->manufacturer->ops->cleanup(spinand);
> > > >  }
> > > >
> > > > -static const struct spi_mem_op *
> > > > +const struct spi_mem_op *
> > > >  spinand_select_op_variant(struct spinand_device *spinand,
> > > >  			  const struct spinand_op_variants *variants)
> > > >  {
> > > > diff --git a/drivers/mtd/nand/spi/micron.c
> > > b/drivers/mtd/nand/spi/micron.c
> > > > index 95bc5264ebc1..6fde93ec23a1 100644
> > > > --- a/drivers/mtd/nand/spi/micron.c
> > > > +++ b/drivers/mtd/nand/spi/micron.c
> > > > @@ -90,22 +90,10 @@ static int micron_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, 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_ooblayout_ops,
> > > > -				     micron_ecc_get_status)),
> > > > -};
> > > > -
> > >
> > > I don't know if it is wise to drop this structure.
> > >
> > > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > > >  {
> > > > +	const struct spi_mem_op *op;
> > > >  	u8 *id = spinand->id.data;
> > > > -	int ret;
> > > >
> > > >  	/*
> > > >  	 * Micron SPI NAND read ID need a dummy byte,
> > > > @@ -114,16 +102,55 @@ 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]);
> > >
> > > I am not sure this is the right solution. I would keep this call and
> > > overwrite what you need to overwrite with the fixup hook.
> > >
> >
> > Then, I will have dummy structure like below.
> >
> > static const struct spinand_info micron_spinand_table[] = {
> >         SPINAND_INFO(NULL, 0,
> >                      NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),
> >                      NAND_ECCREQ(0, 0),
> >                      SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> >                                               &write_cache_variants,
> >                                               &update_cache_variants),
> >                      0,
> >                      SPINAND_ECCINFO(&micron_ooblayout_ops,
> >                                      micron_ecc_get_status)),
> > };
> >
> > Let me know if you are thinking for different approach.
> >
> 
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* RE: [EXT] Re: [PATCH 4/8] mtd: spinand: enabled parameter page support
  2019-08-19  9:21       ` Miquel Raynal
@ 2019-09-30 13:23         ` Shivamurthy Shastri (sshivamurthy)
  2019-10-07  7:52         ` Boris Brezillon
  1 sibling, 0 replies; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-09-30 13:23 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Chuanhong Guo, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Brian Norris, David Woodhouse

Hello Miquel & Boris,

Just a gentle reminder that I'd like some feedback.

Thanks,
Shiva

> 
> Hi Boris,
> 
> Another question for you :)
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote
> on
> Mon, 19 Aug 2019 08:51:52 +0000:
> 
> > Hi Miquel,
> >
> > >
> > > Hi Shiva,
> > >
> > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200:
> > >
> > > "mtd: spinand: enable parameter page support"
> > >
> > > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > >
> > > > Some of the SPI NAND devices has parameter page, which is similar to
> > >                  -             have a
> > > > ONFI table.
> > >   regular raw NAND ONFI tables.
> > >
> > > >
> > > > But, it may not be self sufficient to propagate all the required
> > >   As it may not be
> > > > parameters. Fixup function has been added in struct manufacturer to
> > >             , a fixup        is being added in the manufacturer structure
> > > > accommodate this.
> > >
> > > The fixup function sentence should be dropped from the commit
> message,
> > > see below.
> >
> > Okay, I will create separate patch for fixup function.
> >
> > >
> > > >
> > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > > ---
> > > >  drivers/mtd/nand/spi/core.c | 134
> > > ++++++++++++++++++++++++++++++++++++
> > > >  include/linux/mtd/spinand.h |   3 +
> > > >  2 files changed, 137 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > > index 89f6beefb01c..7ae76dab9141 100644
> > > > --- a/drivers/mtd/nand/spi/core.c
> > > > +++ b/drivers/mtd/nand/spi/core.c
> > > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct
> > > spinand_device *spinand, u8 lock)
> > > >  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> > > >  }
> > > >
> > > > +/**
> > > > + * spinand_read_param_page_op - Read parameter page operation
> > >
> > > Again, the name in the doc does not fit the function you describe
> > >
> > > > + * @spinand: the spinand
> > >                     SPI-NAND chip
> > >
> > > Shiva, there are way too much typos and shortcuts in your series.
> > > Please be more careful otherwise we can't focus on the technical
> > > aspects. I am not a native English speaker at all but please, plain
> > > English is not C code. We talk SPI-NAND and not spinand, we say
> > > structure and not struct, acronyms are uppercase, etc.
> > >
> >
> > Sorry for the inconvenience caused, I will take care from next time.
> >
> > > > + * @page: page number where parameter page tables can be found
> > >                               ^ the
> > > > + * @buf: buffer used to store the parameter page
> > > > + * @len: length of the buffer
> > > > + *
> > > > + * Read parameter page
> > >           the
> > > > + *
> > > > + * Returns 0 on success, a negative error code otherwise.
> > > > + */
> > > > +static int spinand_parameter_page_read(struct spinand_device
> *spinand,
> > > > +				       u8 page, void *buf, unsigned int len)
> > > > +{
> > > > +	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> > > > +	struct spi_mem_op pread_cache_op =
> > > > +
> > > 	SPINAND_PAGE_READ_FROM_CACHE_OP(false,
> > > > +								0,
> > > > +								1,
> > > > +								buf,
> > > > +								len);
> > >
> > > That's ok if you cross the 80 characters boundary here. You may put "0,
> > > 1," on the first line and "buf, len);" on the second.
> > >
> > > > +	u8 feature;
> > > > +	u8 status;
> > > > +	int ret;
> > > > +
> > > > +	if (len && !buf)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > > +				  &feature);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* CFG_OTP_ENABLE is used to enable parameter page access */
> > > > +	feature |= CFG_OTP_ENABLE;
> > > > +
> > > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > > +
> > > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = spinand_wait(spinand, &status);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > > +				  &feature);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	feature &= ~CFG_OTP_ENABLE;
> > > > +
> > > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > Add the kernel doc please
> > >
> > > Change the below function so that it returns 1 if the page was
> > > detected, 0 if it did not, an negative error code otherwise.
> > >
> > > > +static int spinand_param_page_detect(struct spinand_device
> *spinand)
> > > > +{
> > > > +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> > > > +	struct nand_memory_organization *memorg;
> > > > +	struct nand_onfi_params *p;
> > > > +	struct nand_device *base = spinand_to_nand(spinand);
> > > > +	int i, ret;
> > > > +
> > > > +	memorg = nanddev_get_memorg(base);
> > > > +
> > > > +	/* Allocate buffer to hold parameter page */
> > > > +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > > > +	if (!p)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) *
> > > 3);
> > > > +	if (ret) {
> > > > +		ret = 0;
> > >
> > > No, you should return the error in case of error. You will later handle
> > > the fact that there is no parameter page.
> >
> > okay.
> >
> > >
> > > > +		goto free_param_page;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < 3; i++) {
> > > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> > >                                                            ^
> > > If you force the parameter page to be 254 bytes long it means you limit
> > > yourself to ONFI standard. That's not a problem, but then you should
> > > mention it in the function name.
> >
> > okay, I will mention in kernel doc.
> >
> > >
> > > > +				le16_to_cpu(p->crc)) {
> > > > +			if (i)
> > > > +				memcpy(p, &p[i], sizeof(*p));
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (i == 3) {
> > > > +		const void *srcbufs[3] = {p, p + 1, p + 2};
> > > > +
> > > > +		pr_warn("Could not find a valid ONFI parameter page, trying
> > > bit-wise majority to recover it\n");
> > > > +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > > > +				       sizeof(*p));
> > > > +
> > > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > > > +				le16_to_cpu(p->crc)) {
> > > > +			pr_err("ONFI parameter recovery failed,
> > > aborting\n");
> > > > +			goto free_param_page;
> > > > +		}
> > > > +	}
> > >
> > > The whole for-loop and the if (i==3) condition is exactly the same as
> > > for raw NANDs and must be extracted in a generic function:
> > > 1/ extract the function from nand/raw/nand_onfi.c and put it in
> > > nand/onfi.c.
> > > 2/ then use it in this patch.
> >
> > I have done this intentionally, because in raw NAND case there is function
> > "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3)
> > it is necessary to create new function.
> >
> > Let me know if you have different opinion.
> 
> I don't have a strong opinion on that. Boris what do you think? Shall
> we duplicate the code? It's not just about the if condition, it's the
> whole for loop which is very similar.
> 
> 
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* Re: [EXT] Re: [PATCH 4/8] mtd: spinand: enabled parameter page support
  2019-08-19  9:21       ` Miquel Raynal
  2019-09-30 13:23         ` Shivamurthy Shastri (sshivamurthy)
@ 2019-10-07  7:52         ` Boris Brezillon
  1 sibling, 0 replies; 28+ messages in thread
From: Boris Brezillon @ 2019-10-07  7:52 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Brian Norris, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Chuanhong Guo, David Woodhouse,
	Shivamurthy Shastri \(sshivamurthy\)

On Mon, 19 Aug 2019 11:21:18 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Another question for you :)
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on
> Mon, 19 Aug 2019 08:51:52 +0000:
> 
> > Hi Miquel,
> >   
> > > 
> > > Hi Shiva,
> > > 
> > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200:
> > > 
> > > "mtd: spinand: enable parameter page support"
> > >     
> > > > From: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > >
> > > > Some of the SPI NAND devices has parameter page, which is similar to    
> > >                  -             have a    
> > > > ONFI table.    
> > >   regular raw NAND ONFI tables.
> > >     
> > > >
> > > > But, it may not be self sufficient to propagate all the required    
> > >   As it may not be    
> > > > parameters. Fixup function has been added in struct manufacturer to    
> > >             , a fixup        is being added in the manufacturer structure    
> > > > accommodate this.    
> > > 
> > > The fixup function sentence should be dropped from the commit message,
> > > see below.    
> > 
> > Okay, I will create separate patch for fixup function.
> >   
> > >     
> > > >
> > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > > > ---
> > > >  drivers/mtd/nand/spi/core.c | 134    
> > > ++++++++++++++++++++++++++++++++++++    
> > > >  include/linux/mtd/spinand.h |   3 +
> > > >  2 files changed, 137 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > > index 89f6beefb01c..7ae76dab9141 100644
> > > > --- a/drivers/mtd/nand/spi/core.c
> > > > +++ b/drivers/mtd/nand/spi/core.c
> > > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct    
> > > spinand_device *spinand, u8 lock)    
> > > >  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> > > >  }
> > > >
> > > > +/**
> > > > + * spinand_read_param_page_op - Read parameter page operation    
> > > 
> > > Again, the name in the doc does not fit the function you describe
> > >     
> > > > + * @spinand: the spinand    
> > >                     SPI-NAND chip
> > > 
> > > Shiva, there are way too much typos and shortcuts in your series.
> > > Please be more careful otherwise we can't focus on the technical
> > > aspects. I am not a native English speaker at all but please, plain
> > > English is not C code. We talk SPI-NAND and not spinand, we say
> > > structure and not struct, acronyms are uppercase, etc.
> > >     
> > 
> > Sorry for the inconvenience caused, I will take care from next time.
> >   
> > > > + * @page: page number where parameter page tables can be found    
> > >                               ^ the    
> > > > + * @buf: buffer used to store the parameter page
> > > > + * @len: length of the buffer
> > > > + *
> > > > + * Read parameter page    
> > >           the    
> > > > + *
> > > > + * Returns 0 on success, a negative error code otherwise.
> > > > + */
> > > > +static int spinand_parameter_page_read(struct spinand_device *spinand,
> > > > +				       u8 page, void *buf, unsigned int len)
> > > > +{
> > > > +	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> > > > +	struct spi_mem_op pread_cache_op =
> > > > +    
> > > 	SPINAND_PAGE_READ_FROM_CACHE_OP(false,    
> > > > +								0,
> > > > +								1,
> > > > +								buf,
> > > > +								len);    
> > > 
> > > That's ok if you cross the 80 characters boundary here. You may put "0,
> > > 1," on the first line and "buf, len);" on the second.
> > >     
> > > > +	u8 feature;
> > > > +	u8 status;
> > > > +	int ret;
> > > > +
> > > > +	if (len && !buf)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > > +				  &feature);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* CFG_OTP_ENABLE is used to enable parameter page access */
> > > > +	feature |= CFG_OTP_ENABLE;
> > > > +
> > > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > > +
> > > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = spinand_wait(spinand, &status);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > > +				  &feature);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	feature &= ~CFG_OTP_ENABLE;
> > > > +
> > > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +    
> > > Add the kernel doc please
> > > 
> > > Change the below function so that it returns 1 if the page was
> > > detected, 0 if it did not, an negative error code otherwise.
> > >     
> > > > +static int spinand_param_page_detect(struct spinand_device *spinand)
> > > > +{
> > > > +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> > > > +	struct nand_memory_organization *memorg;
> > > > +	struct nand_onfi_params *p;
> > > > +	struct nand_device *base = spinand_to_nand(spinand);
> > > > +	int i, ret;
> > > > +
> > > > +	memorg = nanddev_get_memorg(base);
> > > > +
> > > > +	/* Allocate buffer to hold parameter page */
> > > > +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > > > +	if (!p)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) *    
> > > 3);    
> > > > +	if (ret) {
> > > > +		ret = 0;    
> > > 
> > > No, you should return the error in case of error. You will later handle
> > > the fact that there is no parameter page.    
> > 
> > okay.
> >   
> > >     
> > > > +		goto free_param_page;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < 3; i++) {
> > > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==    
> > >                                                            ^
> > > If you force the parameter page to be 254 bytes long it means you limit
> > > yourself to ONFI standard. That's not a problem, but then you should
> > > mention it in the function name.    
> > 
> > okay, I will mention in kernel doc.
> >   
> > >     
> > > > +				le16_to_cpu(p->crc)) {
> > > > +			if (i)
> > > > +				memcpy(p, &p[i], sizeof(*p));
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (i == 3) {
> > > > +		const void *srcbufs[3] = {p, p + 1, p + 2};
> > > > +
> > > > +		pr_warn("Could not find a valid ONFI parameter page, trying    
> > > bit-wise majority to recover it\n");    
> > > > +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > > > +				       sizeof(*p));
> > > > +
> > > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > > > +				le16_to_cpu(p->crc)) {
> > > > +			pr_err("ONFI parameter recovery failed,    
> > > aborting\n");    
> > > > +			goto free_param_page;
> > > > +		}
> > > > +	}    
> > > 
> > > The whole for-loop and the if (i==3) condition is exactly the same as
> > > for raw NANDs and must be extracted in a generic function:
> > > 1/ extract the function from nand/raw/nand_onfi.c and put it in
> > > nand/onfi.c.
> > > 2/ then use it in this patch.

Sounds like a good idea indeed.
  
> > 
> > I have done this intentionally, because in raw NAND case there is function
> > "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) 
> > it is necessary to create new function.

Well, reading more than just a single param page in a single step
shouldn't hurt. You can let the caller choose the number of page it
wants to read (can be more than 3) and pass this information to the
helper function. You should probably call nand_bit_wise_majority() from
this helper too.

> > 
> > Let me know if you have different opinion.  
> 
> I don't have a strong opinion on that. Boris what do you think? Shall
> we duplicate the code? It's not just about the if condition, it's the
> whole for loop which is very similar.

I think Miquel is right, we shouldn't duplicate this logic.

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

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

* Re: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-08-19  9:03     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
  2019-08-19  9:19       ` Miquel Raynal
@ 2019-10-07  8:13       ` Boris Brezillon
  2019-10-14 12:49         ` Shivamurthy Shastri (sshivamurthy)
  1 sibling, 1 reply; 28+ messages in thread
From: Boris Brezillon @ 2019-10-07  8:13 UTC (permalink / raw)
  To: Shivamurthy Shastri (sshivamurthy)
  Cc: Brian Norris, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Miquel Raynal, Jeff Kletsky, Chuanhong Guo, David Woodhouse

On Mon, 19 Aug 2019 09:03:38 +0000
"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote:

> >   
> > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > >  {
> > > +	const struct spi_mem_op *op;
> > >  	u8 *id = spinand->id.data;
> > > -	int ret;
> > >
> > >  	/*
> > >  	 * Micron SPI NAND read ID need a dummy byte,
> > > @@ -114,16 +102,55 @@ 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]);
> > 
> > I am not sure this is the right solution. I would keep this call and
> > overwrite what you need to overwrite with the fixup hook.
> >   

I'm definitely not comfortable with this whole "rely on ONFi
param-page" thing. Vendors have proven to get it wrong from time to
time, so before we do that, I'd like to make sure all currently
supported Micron NANDs (looks like we only support MT29F2G01ABAGD, so
that shouldn't be hard) expose the right thing there. For instance, are
we sure the ECC layout is always the same, and if not, do we have a
reliable way to extract that?

> 
> Then, I will have dummy structure like below.
> 
> static const struct spinand_info micron_spinand_table[] = {                      
>         SPINAND_INFO(NULL, 0,                                                                    
>                      NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),           
>                      NAND_ECCREQ(0, 0),                                                                       
>                      SPINAND_INFO_OP_VARIANTS(&read_cache_variants,              
>                                               &write_cache_variants,             
>                                               &update_cache_variants),           
>                      0,                                                                                         
>                      SPINAND_ECCINFO(&micron_ooblayout_ops,                      
>                                      micron_ecc_get_status)),                    
> };   

> 
> Let me know if you are thinking for different approach.

Exposing dummy entries is useless. If you're entirely sure all Micron
SPI NANDs have a valid ONFi param page, then no need to use the
ID-based detection. But as I said above, I feel param-page-based
detection is going to be as messy as SFDP-based detection is for SPI
NORs. Vendors tend to make mistakes which we have to fix to make
things work. ID-based detection is much more reliable in this regard,
as long as we don't have ID collisions :P.
Plus, it looks like only a few manufacturers decided to use ONFi param
pages to expose SPI NAND info (AFAICT, only Micron and Macronix do
that), which is not surprising since the ONFi param page has been
created to describe parallel NANDs not SPI NANDs (if you look closely
enough, you'll notice that some fields are meaningless for SPI NANDs).

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

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

* RE: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic
  2019-10-07  8:13       ` Boris Brezillon
@ 2019-10-14 12:49         ` Shivamurthy Shastri (sshivamurthy)
  0 siblings, 0 replies; 28+ messages in thread
From: Shivamurthy Shastri (sshivamurthy) @ 2019-10-14 12:49 UTC (permalink / raw)
  To: Boris Brezillon, Miquel Raynal
  Cc: Brian Norris, Vignesh Raghavendra, Boris Brezillon,
	Marcel Ziswiler, Richard Weinberger, linux-kernel,
	Frieder Schrempf, liaoweixiong, Marek Vasut, linux-mtd,
	Jeff Kletsky, Chuanhong Guo, David Woodhouse

Hi Boris,

Thank you for the review.

> 
> On Mon, 19 Aug 2019 09:03:38 +0000
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote:
> 
> > >
> > > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > > >  {
> > > > +	const struct spi_mem_op *op;
> > > >  	u8 *id = spinand->id.data;
> > > > -	int ret;
> > > >
> > > >  	/*
> > > >  	 * Micron SPI NAND read ID need a dummy byte,
> > > > @@ -114,16 +102,55 @@ 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]);
> > >
> > > I am not sure this is the right solution. I would keep this call and
> > > overwrite what you need to overwrite with the fixup hook.
> > >
> 
> I'm definitely not comfortable with this whole "rely on ONFi
> param-page" thing. Vendors have proven to get it wrong from time to
> time, so before we do that, I'd like to make sure all currently
> supported Micron NANDs (looks like we only support MT29F2G01ABAGD, so
> that shouldn't be hard) expose the right thing there. For instance, are
> we sure the ECC layout is always the same, and if not, do we have a
> reliable way to extract that?
> 
> >
> > Then, I will have dummy structure like below.
> >
> > static const struct spinand_info micron_spinand_table[] = {
> >         SPINAND_INFO(NULL, 0,
> >                      NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),
> >                      NAND_ECCREQ(0, 0),
> >                      SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> >                                               &write_cache_variants,
> >                                               &update_cache_variants),
> >                      0,
> >                      SPINAND_ECCINFO(&micron_ooblayout_ops,
> >                                      micron_ecc_get_status)),
> > };
> 
> >
> > Let me know if you are thinking for different approach.
> 
> Exposing dummy entries is useless. If you're entirely sure all Micron
> SPI NANDs have a valid ONFi param page, then no need to use the
> ID-based detection. But as I said above, I feel param-page-based
> detection is going to be as messy as SFDP-based detection is for SPI
> NORs. Vendors tend to make mistakes which we have to fix to make
> things work. ID-based detection is much more reliable in this regard,
> as long as we don't have ID collisions :P.
> Plus, it looks like only a few manufacturers decided to use ONFi param
> pages to expose SPI NAND info (AFAICT, only Micron and Macronix do
> that), which is not surprising since the ONFi param page has been
> created to describe parallel NANDs not SPI NANDs (if you look closely
> enough, you'll notice that some fields are meaningless for SPI NANDs).

Okay, I will send new patches with ID-based detection for the new devices.


Thanks,
Shiva

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

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

end of thread, back to index

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-22  5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
2019-07-22  5:56 ` [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h shiva.linuxworks
2019-08-07  8:34   ` Miquel Raynal
2019-08-19  8:35     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-07-22  5:56 ` [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c shiva.linuxworks
2019-08-07  9:03   ` Miquel Raynal
2019-08-19  8:36     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-07-22  5:56 ` [PATCH 3/8] mtd: nand: create ONFI table parsing instance shiva.linuxworks
2019-08-07  9:09   ` Miquel Raynal
2019-07-22  5:56 ` [PATCH 4/8] mtd: spinand: enabled parameter page support shiva.linuxworks
2019-08-07  9:48   ` Miquel Raynal
2019-08-19  8:51     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-08-19  9:21       ` Miquel Raynal
2019-09-30 13:23         ` Shivamurthy Shastri (sshivamurthy)
2019-10-07  7:52         ` Boris Brezillon
2019-07-22  5:56 ` [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver shiva.linuxworks
2019-08-07  9:51   ` Miquel Raynal
2019-07-22  5:56 ` [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic shiva.linuxworks
2019-08-07 10:04   ` Miquel Raynal
2019-08-19  9:03     ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-08-19  9:19       ` Miquel Raynal
2019-09-16 10:41         ` Shivamurthy Shastri (sshivamurthy)
2019-10-07  8:13       ` Boris Brezillon
2019-10-14 12:49         ` Shivamurthy Shastri (sshivamurthy)
2019-07-22  5:56 ` [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes shiva.linuxworks
2019-08-07 10:05   ` Miquel Raynal
2019-07-22  5:56 ` [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die shiva.linuxworks
2019-08-07 10:08   ` Miquel Raynal

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git