[PATCH v3 0/6] refactoring and fix for Meson NAND

[PATCH v3 0/6] refactoring and fix for Meson NAND

[Arseniy Krasnov]

Hello,

this patchset does several things:

1) It fixes unstable behaviour of Meson driver, for example random ECC
   errors during reads. It is done by changing 'meson_nfc_queue_rb()'
   implementation. Sequence of commands inside this function is updated.
   This patch is suggested by Liang Yang <liang.yang@amlogic.com>.

   Here is link to discussion:
   https://lore.kernel.org/linux-mtd/a9f8307a-77d7-a69f-ce11-2629909172d2@sberdevices.ru/T/#ma8097bad0228f81a3d11a14389cdec44f45b86c8

2) It moves OOB free bytes to non-protected by ECC area. Here are some
   details:

   Current OOB free bytes are 4 bytes (2 x 2 user bytes) under ECC engine.
   Here is how it looks like in the current implementation:

   [ 2B user bytes ][     14B ECC codes    ]
   [ 2B user bytes ][     14B ECC codes    ]
   [ 16B unused area, not protected by ECC ]
   [ 16B unused area, not protected by ECC ]

   All 4 user bytes are protected by ECC. This patch changes OOB free
   bytes in this way:

   [ 2B unused area ][     14B ECC codes     ]
   [ 2B unused area ][     14B ECC codes     ]
   [  16B user bytes, not protected by ECC   ]
   [  16B user bytes, not protected by ECC   ]

   Now OOB user bytes are 32 bytes instead of 4 bytes and not protected
   by ECC.

   Motivation of this layout comes from problem with JFFS2. It uses OOB
   free bytes for cleanmarkers. Each cleanmarker is 4 bytes and written
   by JFFS2 driver (small remark - cleanmarkers are always written in
   case of NAND storage for JFFS2).
   We have two ways to write this data to OOB (e.g. user bytes):

   1) ECC mode. In this case it will be ECC covered user bytes, e.g.
      writing this bytes will update ECC codes. Problem fires, when
      JFFS2 tries to write this page later - this write makes controller
      to update ECC codes again, but it is impossible to do it correctly,
      because we can't update bits from 0 to 1 (only from 1 to 0).

   2) Raw mode. In this case ECC codes won't be updated. But later, it
      will be impossible to read this page in ECC mode, because we have
      some user bytes, but ECC codes are missed.

   So let's move OOB free bytes out of ECC area. In this case we can
   read/write OOB separately in raw mode and at the same time work with
   data in ECC mode. JFFS2 is happy now. User bytes are untouched - all
   of them are ignored during non-OOB access.

   I've tested this with mount/unmount/read/write cases for JFFS2 and
   nanddump/nandwrite utlities on AXG family (A113X SoC).

   Here is link to discussion:
   https://lore.kernel.org/linux-mtd/a9f8307a-77d7-a69f-ce11-2629909172d2@sberdevices.ru/T/#m3087bd06386a7f430cd5e343e22b25d724d3e2d7

3) Changes size of OOB read access - now, in both ECC and raw modes whole
   OOB (e.g. 64 bytes) is returned to the caller.

4) Checks buffer length on accesses to NAND controller.

5) Removes useless bitwise OR with zeroes.

6) Renames device tree node's name for chip selection from "reg" to
   "cs". See commit message.

Link to v1:
https://lore.kernel.org/linux-mtd/20230412061700.1492474-1-AVKrasnov@sberdevices.ru/
Link to v2:
https://lore.kernel.org/linux-mtd/20230426073632.3905682-1-AVKrasnov@sberdevices.ru/

Arseniy Krasnov (6):
  mtd: rawnand: meson: fix command sequence for read/write
  mtd: rawnand: meson: move OOB to non-protected ECC area
  mtd: rawnand: meson: always read whole OOB bytes
  mtd: rawnand: meson: check buffer length
  mtd: rawnand: meson: remove unneeded bitwise OR with zeroes
  mtd: rawnand: meson: rename node for chip select

 drivers/mtd/nand/raw/meson_nand.c | 274 ++++++++++++++++++++++++------
 1 file changed, 222 insertions(+), 52 deletions(-)

-- 
2.35.0

[PATCH v3 1/6] mtd: rawnand: meson: fix command sequence for read/write

[Arseniy Krasnov]

This fixes read/write functionality by:
1) Changing NFC_CMD_RB_INT bit value.
2) Adding extra NAND_CMD_STATUS command on each r/w request.
3) Adding extra idle commands during r/w request.
4) Adding extra NAND_CMD_READ0 on each read request.

Without this patch driver works unstable, for example there are a lot
of ECC errors.

Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
Suggested-by: Liang Yang <liang.yang@amlogic.com>
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 074e14225c06..08b3cf17c113 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -37,7 +37,7 @@
 #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
 #define NFC_CMD_SCRAMBLER_DISABLE	0
 #define NFC_CMD_SHORTMODE_DISABLE	0
-#define NFC_CMD_RB_INT		BIT(14)
+#define NFC_CMD_RB_INT ((0xb << 10) | BIT(18) | BIT(16))
 
 #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
 
@@ -392,7 +392,7 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
 	}
 }
 
-static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
+static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms, int cmd_read0)
 {
 	u32 cmd, cfg;
 	int ret = 0;
@@ -407,17 +407,29 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
 
 	reinit_completion(&nfc->completion);
 
+	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_STATUS;
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+	meson_nfc_cmd_idle(nfc, 5);
+
 	/* use the max erase time as the maximum clock for waiting R/B */
-	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
-		| nfc->param.chip_select | nfc->timing.tbers_max;
+	cmd = NFC_CMD_RB | NFC_CMD_RB_INT | nfc->timing.tbers_max;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+	meson_nfc_cmd_idle(nfc, 2);
 
 	ret = wait_for_completion_timeout(&nfc->completion,
 					  msecs_to_jiffies(timeout_ms));
 	if (ret == 0)
-		ret = -1;
+		return -1;
 
-	return ret;
+	if (!cmd_read0)
+		return 0;
+
+	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_READ0;
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+	meson_nfc_drain_cmd(nfc);
+	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
+
+	return 0;
 }
 
 static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
@@ -623,7 +635,7 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
 	if (in) {
 		nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
 		writel(nfc->cmdfifo.rw.cmd1, nfc->reg_base + NFC_REG_CMD);
-		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max));
+		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max), 1);
 	} else {
 		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
 	}
@@ -669,7 +681,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
 
 	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
-	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
+	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max), 0);
 
 	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
 
@@ -952,7 +964,7 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
 			break;
 
 		case NAND_OP_WAITRDY_INSTR:
-			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
+			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms, 0);
 			if (instr->delay_ns)
 				meson_nfc_cmd_idle(nfc, delay_idle);
 			break;
-- 
2.35.0

[PATCH v3 2/6] mtd: rawnand: meson: move OOB to non-protected ECC area

[Arseniy Krasnov]

This moves free bytes of OOB to non-protected ECC area. It is needed to
make JFFS2 works correctly with this NAND controller. Problem fires when
JFFS2 driver writes cleanmarker to some page and later it tries to write
to this page - write will be done successfully, but after that such page
becomes unreadable due to invalid ECC codes. This happens because second
write needs to update ECC codes, but it is impossible to do it correctly
without block erase. So idea of this patch is to split accesses to OOB
free bytes and data on each page - now both of them does not depends on
each other.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 192 ++++++++++++++++++++++++------
 1 file changed, 155 insertions(+), 37 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 08b3cf17c113..d7c81408cfa2 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -108,6 +108,9 @@
 
 #define PER_INFO_BYTE		8
 
+#define NFC_USER_BYTES		2
+#define NFC_OOB_PER_ECC(nand)	((nand)->ecc.bytes + NFC_USER_BYTES)
+
 struct meson_nfc_nand_chip {
 	struct list_head node;
 	struct nand_chip nand;
@@ -122,6 +125,7 @@ struct meson_nfc_nand_chip {
 	u8 *data_buf;
 	__le64 *info_buf;
 	u32 nsels;
+	u8 *oob_buf;
 	u8 sels[];
 };
 
@@ -338,7 +342,7 @@ static u8 *meson_nfc_oob_ptr(struct nand_chip *nand, int i)
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	int len;
 
-	len = nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i;
+	len = nand->ecc.size * (i + 1) + NFC_OOB_PER_ECC(nand) * i;
 
 	return meson_chip->data_buf + len;
 }
@@ -349,7 +353,7 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
 	int len, temp;
 
 	temp = nand->ecc.size + nand->ecc.bytes;
-	len = (temp + 2) * i;
+	len = (temp + NFC_USER_BYTES) * i;
 
 	return meson_chip->data_buf + len;
 }
@@ -357,29 +361,47 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
 static void meson_nfc_get_data_oob(struct nand_chip *nand,
 				   u8 *buf, u8 *oobbuf)
 {
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct mtd_info *mtd = nand_to_mtd(nand);
 	int i, oob_len = 0;
 	u8 *dsrc, *osrc;
+	u8 *oobtail;
 
-	oob_len = nand->ecc.bytes + 2;
+	oob_len = NFC_OOB_PER_ECC(nand);
 	for (i = 0; i < nand->ecc.steps; i++) {
 		if (buf) {
 			dsrc = meson_nfc_data_ptr(nand, i);
 			memcpy(buf, dsrc, nand->ecc.size);
 			buf += nand->ecc.size;
 		}
-		osrc = meson_nfc_oob_ptr(nand, i);
-		memcpy(oobbuf, osrc, oob_len);
-		oobbuf += oob_len;
+
+		if (oobbuf) {
+			osrc = meson_nfc_oob_ptr(nand, i);
+			memcpy(oobbuf, osrc, oob_len);
+			oobbuf += oob_len;
+		}
 	}
+
+	if (!oobbuf)
+		return;
+
+	oobtail = meson_chip->data_buf + nand->ecc.steps *
+		  (nand->ecc.size + oob_len);
+
+	/* 'oobbuf' if already shifted to the start of unused area. */
+	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
 }
 
 static void meson_nfc_set_data_oob(struct nand_chip *nand,
 				   const u8 *buf, u8 *oobbuf)
 {
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct mtd_info *mtd = nand_to_mtd(nand);
 	int i, oob_len = 0;
 	u8 *dsrc, *osrc;
+	u8 *oobtail;
 
-	oob_len = nand->ecc.bytes + 2;
+	oob_len = NFC_OOB_PER_ECC(nand);
 	for (i = 0; i < nand->ecc.steps; i++) {
 		if (buf) {
 			dsrc = meson_nfc_data_ptr(nand, i);
@@ -390,6 +412,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
 		memcpy(osrc, oobbuf, oob_len);
 		oobbuf += oob_len;
 	}
+
+	oobtail = meson_chip->data_buf + nand->ecc.steps *
+		  (nand->ecc.size + oob_len);
+
+	/* 'oobbuf' if already shifted to the start of unused area. */
+	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);
 }
 
 static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms, int cmd_read0)
@@ -436,25 +464,12 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
 {
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	__le64 *info;
-	int i, count;
-
-	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
-		info = &meson_chip->info_buf[i];
-		*info |= oob_buf[count];
-		*info |= oob_buf[count + 1] << 8;
-	}
-}
-
-static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
-{
-	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
-	__le64 *info;
-	int i, count;
+	int i;
 
-	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
+	for (i = 0; i < nand->ecc.steps; i++) {
 		info = &meson_chip->info_buf[i];
-		oob_buf[count] = *info;
-		oob_buf[count + 1] = *info >> 8;
+		/* Always ignore user bytes programming. */
+		*info |= 0xffff;
 	}
 }
 
@@ -698,18 +713,92 @@ static int meson_nfc_write_page_raw(struct nand_chip *nand, const u8 *buf,
 	return meson_nfc_write_page_sub(nand, page, 1);
 }
 
+static u32 meson_nfc_get_oob_bytes(struct nand_chip *nand)
+{
+	struct mtd_info *mtd = nand_to_mtd(nand);
+
+	return mtd->oobsize - nand->ecc.steps * NFC_OOB_PER_ECC(nand);
+}
+
+static int __meson_nfc_write_oob(struct nand_chip *nand, int page, u8 *oob_buf)
+{
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct mtd_info *mtd = nand_to_mtd(nand);
+	u32 page_size = mtd->writesize + mtd->oobsize;
+	u32 oob_bytes = meson_nfc_get_oob_bytes(nand);
+	int ret;
+
+	if (!oob_bytes)
+		return 0;
+
+	ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
+	if (ret)
+		return ret;
+
+	memcpy(meson_chip->oob_buf, oob_buf + (mtd->oobsize - oob_bytes),
+	       oob_bytes);
+
+	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
+					  meson_chip->oob_buf,
+					  oob_bytes, false);
+	if (ret)
+		return ret;
+
+	return nand_prog_page_end_op(nand);
+}
+
+static int __meson_nfc_read_oob(struct nand_chip *nand, int page,
+				u8 *oob_buf)
+{
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct mtd_info *mtd = nand_to_mtd(nand);
+	u32 oob_bytes;
+	u32 page_size;
+	int ret;
+
+	oob_bytes = meson_nfc_get_oob_bytes(nand);
+
+	if (!oob_bytes)
+		return 0;
+
+	ret = nand_read_page_op(nand, page, 0, NULL, 0);
+	if (ret)
+		return ret;
+
+	page_size = mtd->writesize + mtd->oobsize;
+
+	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
+					 meson_chip->oob_buf,
+					 oob_bytes, false);
+
+	if (!ret)
+		memcpy(oob_buf + (mtd->oobsize - oob_bytes),
+		       meson_chip->oob_buf,
+		       oob_bytes);
+
+	return ret;
+}
+
 static int meson_nfc_write_page_hwecc(struct nand_chip *nand,
 				      const u8 *buf, int oob_required, int page)
 {
 	struct mtd_info *mtd = nand_to_mtd(nand);
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	u8 *oob_buf = nand->oob_poi;
+	int ret;
 
 	memcpy(meson_chip->data_buf, buf, mtd->writesize);
 	memset(meson_chip->info_buf, 0, nand->ecc.steps * PER_INFO_BYTE);
 	meson_nfc_set_user_byte(nand, oob_buf);
 
-	return meson_nfc_write_page_sub(nand, page, 0);
+	ret = meson_nfc_write_page_sub(nand, page, 0);
+	if (ret)
+		return ret;
+
+	if (oob_required)
+		ret = __meson_nfc_write_oob(nand, page, oob_buf);
+
+	return ret;
 }
 
 static void meson_nfc_check_ecc_pages_valid(struct meson_nfc *nfc,
@@ -783,7 +872,7 @@ static int meson_nfc_read_page_raw(struct nand_chip *nand, u8 *buf,
 	if (ret)
 		return ret;
 
-	meson_nfc_get_data_oob(nand, buf, oob_buf);
+	meson_nfc_get_data_oob(nand, buf, oob_required ? oob_buf : NULL);
 
 	return 0;
 }
@@ -803,12 +892,12 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
 	if (ret)
 		return ret;
 
-	meson_nfc_get_user_byte(nand, oob_buf);
 	ret = meson_nfc_ecc_correct(nand, &bitflips, &correct_bitmap);
 	if (ret == ECC_CHECK_RETURN_FF) {
 		if (buf)
 			memset(buf, 0xff, mtd->writesize);
 		memset(oob_buf, 0xff, mtd->oobsize);
+		return bitflips;
 	} else if (ret < 0) {
 		if ((nand->options & NAND_NEED_SCRAMBLING) || !buf) {
 			mtd->ecc_stats.failed++;
@@ -820,12 +909,14 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
 
 		for (i = 0; i < nand->ecc.steps ; i++) {
 			u8 *data = buf + i * ecc->size;
-			u8 *oob = nand->oob_poi + i * (ecc->bytes + 2);
+			u8 *oob = nand->oob_poi + i * NFC_OOB_PER_ECC(nand);
 
 			if (correct_bitmap & BIT_ULL(i))
 				continue;
+
 			ret = nand_check_erased_ecc_chunk(data,	ecc->size,
-							  oob, ecc->bytes + 2,
+							  oob,
+							  NFC_OOB_PER_ECC(nand),
 							  NULL, 0,
 							  ecc->strength);
 			if (ret < 0) {
@@ -839,17 +930,30 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
 		memcpy(buf, meson_chip->data_buf, mtd->writesize);
 	}
 
+	if (oob_required)
+		__meson_nfc_read_oob(nand, page, oob_buf);
+
 	return bitflips;
 }
 
 static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
 {
-	return meson_nfc_read_page_raw(nand, NULL, 1, page);
+	return __meson_nfc_read_oob(nand, page, nand->oob_poi);
 }
 
 static int meson_nfc_read_oob(struct nand_chip *nand, int page)
 {
-	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
+	return __meson_nfc_read_oob(nand, page, nand->oob_poi);
+}
+
+static int meson_nfc_write_oob_raw(struct nand_chip *nand, int page)
+{
+	return __meson_nfc_write_oob(nand, page, nand->oob_poi);
+}
+
+static int meson_nfc_write_oob(struct nand_chip *nand, int page)
+{
+	return __meson_nfc_write_oob(nand, page, nand->oob_poi);
 }
 
 static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
@@ -982,7 +1086,7 @@ static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
 	if (section >= nand->ecc.steps)
 		return -ERANGE;
 
-	oobregion->offset =  2 + (section * (2 + nand->ecc.bytes));
+	oobregion->offset = NFC_USER_BYTES + section * NFC_OOB_PER_ECC(nand);
 	oobregion->length = nand->ecc.bytes;
 
 	return 0;
@@ -992,12 +1096,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
 				struct mtd_oob_region *oobregion)
 {
 	struct nand_chip *nand = mtd_to_nand(mtd);
+	u32 oob_bytes = meson_nfc_get_oob_bytes(nand);
 
 	if (section >= nand->ecc.steps)
 		return -ERANGE;
 
-	oobregion->offset = section * (2 + nand->ecc.bytes);
-	oobregion->length = 2;
+	/* Split rest of OOB area (not covered by ECC engine) per each
+	 * ECC section. This will be OOB data available to user.
+	 */
+	oobregion->offset = (section + nand->ecc.steps) * NFC_OOB_PER_ECC(nand);
+	oobregion->length = oob_bytes / nand->ecc.steps;
 
 	return 0;
 }
@@ -1184,6 +1292,9 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
 
 static void meson_nand_detach_chip(struct nand_chip *nand)
 {
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+
+	kfree(meson_chip->oob_buf);
 	meson_nfc_free_buffer(nand);
 }
 
@@ -1225,9 +1336,9 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 	nand->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
 	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
 	nand->ecc.write_page = meson_nfc_write_page_hwecc;
-	nand->ecc.write_oob_raw = nand_write_oob_std;
-	nand->ecc.write_oob = nand_write_oob_std;
 
+	nand->ecc.write_oob_raw = meson_nfc_write_oob_raw;
+	nand->ecc.write_oob = meson_nfc_write_oob;
 	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
 	nand->ecc.read_page = meson_nfc_read_page_hwecc;
 	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
@@ -1237,9 +1348,16 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 		dev_err(nfc->dev, "16bits bus width not supported");
 		return -EINVAL;
 	}
+
+	meson_chip->oob_buf = kmalloc(nand->ecc.bytes, GFP_KERNEL);
+	if (!meson_chip->oob_buf)
+		return -ENOMEM;
+
 	ret = meson_chip_buffer_init(nand);
-	if (ret)
+	if (ret) {
+		kfree(meson_chip->oob_buf);
 		return -ENOMEM;
+	}
 
 	return ret;
 }
-- 
2.35.0

[PATCH v3 3/6] mtd: rawnand: meson: always read whole OOB bytes

[Arseniy Krasnov]

This changes size of read access to OOB area by reading all bytes of
OOB (free bytes + ECC engine bytes).

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index d7c81408cfa2..331377a2c5dc 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -755,6 +755,30 @@ static int __meson_nfc_read_oob(struct nand_chip *nand, int page,
 	u32 oob_bytes;
 	u32 page_size;
 	int ret;
+	int i;
+
+	/* Read ECC codes and user bytes. */
+	for (i = 0; i < nand->ecc.steps; i++) {
+		u32 ecc_offs = nand->ecc.size * (i + 1) +
+			       NFC_OOB_PER_ECC(nand) * i;
+
+		ret = nand_read_page_op(nand, page, 0, NULL, 0);
+		if (ret)
+			return ret;
+
+		/* Use temporary buffer, because 'nand_change_read_column_op()'
+		 * seems work with some alignment, so we can't read data to
+		 * 'oob_buf' directly.
+		 */
+		ret = nand_change_read_column_op(nand, ecc_offs, meson_chip->oob_buf,
+						 NFC_OOB_PER_ECC(nand), false);
+		if (ret)
+			return ret;
+
+		memcpy(oob_buf + i * NFC_OOB_PER_ECC(nand),
+		       meson_chip->oob_buf,
+		       NFC_OOB_PER_ECC(nand));
+	}
 
 	oob_bytes = meson_nfc_get_oob_bytes(nand);
 
-- 
2.35.0

[PATCH v3 4/6] mtd: rawnand: meson: check buffer length

[Arseniy Krasnov]

This NAND controller has limited buffer length, so check it before
command execution to avoid length trim. Also check MTD write size on
chip attach.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 331377a2c5dc..ed7ec1bfd07e 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -111,6 +111,8 @@
 #define NFC_USER_BYTES		2
 #define NFC_OOB_PER_ECC(nand)	((nand)->ecc.bytes + NFC_USER_BYTES)
 
+#define NFC_CMD_RAW_LEN		GENMASK(13, 0)
+
 struct meson_nfc_nand_chip {
 	struct list_head node;
 	struct nand_chip nand;
@@ -284,7 +286,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
 
 	if (raw) {
 		len = mtd->writesize + mtd->oobsize;
-		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);
+		cmd = len | scrambler | DMA_DIR(dir);
 		writel(cmd, nfc->reg_base + NFC_REG_CMD);
 		return;
 	}
@@ -562,6 +564,9 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
 	u32 cmd;
 	u8 *info;
 
+	if (len > NFC_CMD_RAW_LEN)
+		return -EINVAL;
+
 	info = kzalloc(PER_INFO_BYTE, GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
@@ -571,7 +576,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
 	if (ret)
 		goto out;
 
-	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
+	cmd = NFC_CMD_N2M | len;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
 	meson_nfc_drain_cmd(nfc);
@@ -590,12 +595,15 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
 	int ret = 0;
 	u32 cmd;
 
+	if (len > NFC_CMD_RAW_LEN)
+		return -EINVAL;
+
 	ret = meson_nfc_dma_buffer_setup(nand, buf, len, NULL,
 					 0, DMA_TO_DEVICE);
 	if (ret)
 		return ret;
 
-	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
+	cmd = NFC_CMD_M2N | len;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
 	meson_nfc_drain_cmd(nfc);
@@ -1328,6 +1336,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	struct mtd_info *mtd = nand_to_mtd(nand);
 	int nsectors = mtd->writesize / 1024;
+	int raw_writesize;
 	int ret;
 
 	if (!mtd->name) {
@@ -1339,6 +1348,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 			return -ENOMEM;
 	}
 
+	raw_writesize = mtd->writesize + mtd->oobsize;
+	if (raw_writesize > NFC_CMD_RAW_LEN) {
+		dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n",
+			raw_writesize, NFC_CMD_RAW_LEN);
+		return -EINVAL;
+	}
+
 	if (nand->bbt_options & NAND_BBT_USE_FLASH)
 		nand->bbt_options |= NAND_BBT_NO_OOB;
 
-- 
2.35.0

[PATCH v3 5/6] mtd: rawnand: meson: remove unneeded bitwise OR with zeroes

[Arseniy Krasnov]

Both operations have no effect.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index ed7ec1bfd07e..bc99a098f3ca 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -630,12 +630,12 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
 	cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
 	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
 
-	addrs[0] = cs | NFC_CMD_ALE | 0;
+	addrs[0] = cs | NFC_CMD_ALE;
 	if (mtd->writesize <= 512) {
 		cmd_num--;
 		row_start = 1;
 	} else {
-		addrs[1] = cs | NFC_CMD_ALE | 0;
+		addrs[1] = cs | NFC_CMD_ALE;
 		row_start = 2;
 	}
 
-- 
2.35.0

[PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Arseniy Krasnov]

This renames node with values for chip select from "reg" to "cs". It is
needed because when OTP access is enabled on the attached storage, MTD
subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
tries to use "reg" node in its own manner, supposes that it has another
layout. All of this leads to device initialization failure.

Example:

[...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
[...] mtd mtd0: Failed to register OTP NVMEM device
[...] meson-nand ffe07800.nfc: failed to register MTD device: -22
[...] meson-nand ffe07800.nfc: failed to init NAND chips
[...] meson-nand: probe of ffe07800.nfc failed with error -22

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index bc99a098f3ca..28371919a6c5 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -1419,7 +1419,7 @@ meson_nfc_nand_chip_init(struct device *dev,
 	int ret, i;
 	u32 tmp, nsels;
 
-	nsels = of_property_count_elems_of_size(np, "reg", sizeof(u32));
+	nsels = of_property_count_elems_of_size(np, "cs", sizeof(u32));
 	if (!nsels || nsels > MAX_CE_NUM) {
 		dev_err(dev, "invalid register property size\n");
 		return -EINVAL;
@@ -1433,7 +1433,7 @@ meson_nfc_nand_chip_init(struct device *dev,
 	meson_chip->nsels = nsels;
 
 	for (i = 0; i < nsels; i++) {
-		ret = of_property_read_u32_index(np, "reg", i, &tmp);
+		ret = of_property_read_u32_index(np, "cs", i, &tmp);
 		if (ret) {
 			dev_err(dev, "could not retrieve register property: %d\n",
 				ret);
-- 
2.35.0

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Martin Blumenstingl]

Hello Arseniy,

On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
<AVKrasnov@sberdevices.ru> wrote:
>
> This renames node with values for chip select from "reg" to "cs". It is
> needed because when OTP access is enabled on the attached storage, MTD
> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
> tries to use "reg" node in its own manner, supposes that it has another
> layout. All of this leads to device initialization failure.
In general: if we change the device-tree interface (in this case:
replacing a "reg" with a "cs" property) the dt-bindings have to be
updated as well.
Documentation/devicetree/bindings/mtd/nand-controller.yaml and
Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
that the chip select of a NAND chip is specified with a "reg"
property.
Also the code has to be backwards compatible with old .dtbs.

> Example:
>
> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
> [...] mtd mtd0: Failed to register OTP NVMEM device
> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
> [...] meson-nand ffe07800.nfc: failed to init NAND chips
> [...] meson-nand: probe of ffe07800.nfc failed with error -22
This is odd - can you please share your definition of the &nfc node?

&nfc {
      nand_chip0: nand@0 {
        reg = <0>;
      };
};

This should result in nand_set_flash_node() being called with
&nand_chip0 (if it's called with &nfc then something is buggy in our
driver).
If there's no child nodes within &nand_chip0 then why would the
MTD-to-NVMEM code think that it has to parse something?
If you do have child nodes and those are partitions, then make sure
that the structure is correct (see the extra "partitions" node inside
which all partitions are nested):
&nand_chip0 {
    partitions {
        compatible = "fixed-partitions";
        #address-cells = <1>;
        #size-cells = <1>;

        partition@0 {
            label = "u-boot";
            reg = <0x0000000 0x4000>;
            read-only;
        };
    };
};


Best regards,,
Martin

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Miquel Raynal]

Hi Martin & Arseniy,

martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
+0200:

> Hello Arseniy,
> 
> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
> <AVKrasnov@sberdevices.ru> wrote:
> >
> > This renames node with values for chip select from "reg" to "cs". It is
> > needed because when OTP access is enabled on the attached storage, MTD
> > subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
> > tries to use "reg" node in its own manner, supposes that it has another
> > layout. All of this leads to device initialization failure.  
> In general: if we change the device-tree interface (in this case:
> replacing a "reg" with a "cs" property) the dt-bindings have to be
> updated as well.

True, and I would add, bindings should not be broken.

> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
> that the chip select of a NAND chip is specified with a "reg"
> property.

All NAND controller binding expect the chip-select to be in the
'reg' property, very much like a spi device would use reg to store the
cs as well: the reg property tells you how you address the device.

I also fully agree with Martin's comments below. Changing reg is likely
a wrong approach :)

> Also the code has to be backwards compatible with old .dtbs.
> 
> > Example:
> >
> > [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
> > [...] mtd mtd0: Failed to register OTP NVMEM device
> > [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
> > [...] meson-nand ffe07800.nfc: failed to init NAND chips
> > [...] meson-nand: probe of ffe07800.nfc failed with error -22  
> This is odd - can you please share your definition of the &nfc node?
> 
> &nfc {
>       nand_chip0: nand@0 {
>         reg = <0>;
>       };
> };
> 
> This should result in nand_set_flash_node() being called with
> &nand_chip0 (if it's called with &nfc then something is buggy in our
> driver).
> If there's no child nodes within &nand_chip0 then why would the
> MTD-to-NVMEM code think that it has to parse something?
> If you do have child nodes and those are partitions, then make sure
> that the structure is correct (see the extra "partitions" node inside
> which all partitions are nested):
> &nand_chip0 {
>     partitions {
>         compatible = "fixed-partitions";
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         partition@0 {
>             label = "u-boot";
>             reg = <0x0000000 0x4000>;
>             read-only;
>         };
>     };
> };
> 
> 
> Best regards,,
> Martin


Thanks,
Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Arseniy Krasnov]

On 10.05.2023 23:53, Miquel Raynal wrote:

Hello Martin, Miquel

> Hi Martin & Arseniy,
> 
> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
> +0200:
> 
>> Hello Arseniy,
>>
>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
>> <AVKrasnov@sberdevices.ru> wrote:
>>>
>>> This renames node with values for chip select from "reg" to "cs". It is
>>> needed because when OTP access is enabled on the attached storage, MTD
>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
>>> tries to use "reg" node in its own manner, supposes that it has another
>>> layout. All of this leads to device initialization failure.  
>> In general: if we change the device-tree interface (in this case:
>> replacing a "reg" with a "cs" property) the dt-bindings have to be
>> updated as well.
> 
> True, and I would add, bindings should not be broken.

I see, that's true. That is bad way to change bindings.

> 
>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
>> that the chip select of a NAND chip is specified with a "reg"
>> property.
> 
> All NAND controller binding expect the chip-select to be in the
> 'reg' property, very much like a spi device would use reg to store the
> cs as well: the reg property tells you how you address the device.
> 
> I also fully agree with Martin's comments below. Changing reg is likely
> a wrong approach :)
> 
>> Also the code has to be backwards compatible with old .dtbs.
>>
>>> Example:
>>>
>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
>>> [...] mtd mtd0: Failed to register OTP NVMEM device
>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22  
>> This is odd - can you please share your definition of the &nfc node?

Sure, here it is:

mtd_nand: nfc@7800 {                            
	compatible = "amlogic,meson-axg-nfc";
	...
	nand@0 {                                
        	reg = <0>;
        };
}

I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?

>>
>> &nfc {
>>       nand_chip0: nand@0 {
>>         reg = <0>;
>>       };
>> };
>>
>> This should result in nand_set_flash_node() being called with
>> &nand_chip0 (if it's called with &nfc then something is buggy in our
>> driver).
>> If there's no child nodes within &nand_chip0 then why would the
>> MTD-to-NVMEM code think that it has to parse something?
>> If you do have child nodes and those are partitions, then make sure
>> that the structure is correct (see the extra "partitions" node inside
>> which all partitions are nested):
>> &nand_chip0 {
>>     partitions {
>>         compatible = "fixed-partitions";
>>         #address-cells = <1>;
>>         #size-cells = <1>;
>>
>>         partition@0 {
>>             label = "u-boot";
>>             reg = <0x0000000 0x4000>;
>>             read-only;
>>         };
>>     };
>> };

No, partition nodes are disabled in this case.

Thanks, Arseniy

>>
>>
>> Best regards,,
>> Martin
> 
> 
> Thanks,
> Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300:

> On 10.05.2023 23:53, Miquel Raynal wrote:
> 
> Hello Martin, Miquel
> 
> > Hi Martin & Arseniy,
> > 
> > martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
> > +0200:
> >   
> >> Hello Arseniy,
> >>
> >> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
> >> <AVKrasnov@sberdevices.ru> wrote:  
> >>>
> >>> This renames node with values for chip select from "reg" to "cs". It is
> >>> needed because when OTP access is enabled on the attached storage, MTD
> >>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
> >>> tries to use "reg" node in its own manner, supposes that it has another
> >>> layout. All of this leads to device initialization failure.    
> >> In general: if we change the device-tree interface (in this case:
> >> replacing a "reg" with a "cs" property) the dt-bindings have to be
> >> updated as well.  
> > 
> > True, and I would add, bindings should not be broken.  
> 
> I see, that's true. That is bad way to change bindings.
> 
> >   
> >> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
> >> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
> >> that the chip select of a NAND chip is specified with a "reg"
> >> property.  
> > 
> > All NAND controller binding expect the chip-select to be in the
> > 'reg' property, very much like a spi device would use reg to store the
> > cs as well: the reg property tells you how you address the device.
> > 
> > I also fully agree with Martin's comments below. Changing reg is likely
> > a wrong approach :)
> >   
> >> Also the code has to be backwards compatible with old .dtbs.
> >>  
> >>> Example:
> >>>
> >>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
> >>> [...] mtd mtd0: Failed to register OTP NVMEM device
> >>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
> >>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
> >>> [...] meson-nand: probe of ffe07800.nfc failed with error -22    
> >> This is odd - can you please share your definition of the &nfc node?  
> 
> Sure, here it is:
> 
> mtd_nand: nfc@7800 {                            
> 	compatible = "amlogic,meson-axg-nfc";
> 	...
> 	nand@0 {                                
>         	reg = <0>;
>         };
> }
> 
> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?

We recently had issues with nvmem parsing, but I believe a mainline
kernel should now be perfectly working on this regard. What version of
the Linux kernel are you using?

Thanks,
Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Arseniy Krasnov]

On 11.05.2023 12:12, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300:
> 
>> On 10.05.2023 23:53, Miquel Raynal wrote:
>>
>> Hello Martin, Miquel
>>
>>> Hi Martin & Arseniy,
>>>
>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
>>> +0200:
>>>   
>>>> Hello Arseniy,
>>>>
>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
>>>> <AVKrasnov@sberdevices.ru> wrote:  
>>>>>
>>>>> This renames node with values for chip select from "reg" to "cs". It is
>>>>> needed because when OTP access is enabled on the attached storage, MTD
>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
>>>>> tries to use "reg" node in its own manner, supposes that it has another
>>>>> layout. All of this leads to device initialization failure.    
>>>> In general: if we change the device-tree interface (in this case:
>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be
>>>> updated as well.  
>>>
>>> True, and I would add, bindings should not be broken.  
>>
>> I see, that's true. That is bad way to change bindings.
>>
>>>   
>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
>>>> that the chip select of a NAND chip is specified with a "reg"
>>>> property.  
>>>
>>> All NAND controller binding expect the chip-select to be in the
>>> 'reg' property, very much like a spi device would use reg to store the
>>> cs as well: the reg property tells you how you address the device.
>>>
>>> I also fully agree with Martin's comments below. Changing reg is likely
>>> a wrong approach :)
>>>   
>>>> Also the code has to be backwards compatible with old .dtbs.
>>>>  
>>>>> Example:
>>>>>
>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device
>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22    
>>>> This is odd - can you please share your definition of the &nfc node?  
>>
>> Sure, here it is:
>>
>> mtd_nand: nfc@7800 {                            
>> 	compatible = "amlogic,meson-axg-nfc";
>> 	...
>> 	nand@0 {                                
>>         	reg = <0>;
>>         };
>> }
>>
>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?
> 
> We recently had issues with nvmem parsing, but I believe a mainline
> kernel should now be perfectly working on this regard. What version of
> the Linux kernel are you using?

My current version is:

VERSION = 6                                                             
PATCHLEVEL = 2                                                          
SUBLEVEL = 0                                                            
EXTRAVERSION = -rc8 

Fix was in drivers/nvmem/* ?

Thanks, Arseniy

> 
> Thanks,
> Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Arseniy Krasnov]

On 11.05.2023 12:17, Arseniy Krasnov wrote:
> 
> 
> On 11.05.2023 12:12, Miquel Raynal wrote:
>> Hi Arseniy,
>>
>> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300:
>>
>>> On 10.05.2023 23:53, Miquel Raynal wrote:
>>>
>>> Hello Martin, Miquel
>>>
>>>> Hi Martin & Arseniy,
>>>>
>>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
>>>> +0200:
>>>>   
>>>>> Hello Arseniy,
>>>>>
>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
>>>>> <AVKrasnov@sberdevices.ru> wrote:  
>>>>>>
>>>>>> This renames node with values for chip select from "reg" to "cs". It is
>>>>>> needed because when OTP access is enabled on the attached storage, MTD
>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
>>>>>> tries to use "reg" node in its own manner, supposes that it has another
>>>>>> layout. All of this leads to device initialization failure.    
>>>>> In general: if we change the device-tree interface (in this case:
>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be
>>>>> updated as well.  
>>>>
>>>> True, and I would add, bindings should not be broken.  
>>>
>>> I see, that's true. That is bad way to change bindings.
>>>
>>>>   
>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
>>>>> that the chip select of a NAND chip is specified with a "reg"
>>>>> property.  
>>>>
>>>> All NAND controller binding expect the chip-select to be in the
>>>> 'reg' property, very much like a spi device would use reg to store the
>>>> cs as well: the reg property tells you how you address the device.
>>>>
>>>> I also fully agree with Martin's comments below. Changing reg is likely
>>>> a wrong approach :)
>>>>   
>>>>> Also the code has to be backwards compatible with old .dtbs.
>>>>>  
>>>>>> Example:
>>>>>>
>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device
>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22    
>>>>> This is odd - can you please share your definition of the &nfc node?  
>>>
>>> Sure, here it is:
>>>
>>> mtd_nand: nfc@7800 {                            
>>> 	compatible = "amlogic,meson-axg-nfc";
>>> 	...
>>> 	nand@0 {                                
>>>         	reg = <0>;
>>>         };
>>> }
>>>
>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?
>>
>> We recently had issues with nvmem parsing, but I believe a mainline
>> kernel should now be perfectly working on this regard. What version of
>> the Linux kernel are you using?
> 
> My current version is:
> 
> VERSION = 6                                                             
> PATCHLEVEL = 2                                                          
> SUBLEVEL = 0                                                            
> EXTRAVERSION = -rc8 
> 
> Fix was in drivers/nvmem/* ?
> 
> Thanks, Arseniy

Upd: I resolved problem in the following way:

nand@0 {                                
	reg = <0>;//chip select

	otp@0 {                         
		#address-cells = <2>;   
		#size-cells = <0>;      
		compatible = "user-otp";
		reg = <A B>;            
	};                              
	otp@1 {                         
		#address-cells = <2>;   
		#size-cells = <0>;      
		compatible = "factory-otp";
		reg = <C D>;            
	};                              
};

Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are
the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as
chip select as supposed.

I think, this patch should be abandoned in the next version.

Thanks, Arseniy

> 
>>
>> Thanks,
>> Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 13:16:59 +0300:

> On 11.05.2023 12:17, Arseniy Krasnov wrote:
> > 
> > 
> > On 11.05.2023 12:12, Miquel Raynal wrote:  
> >> Hi Arseniy,
> >>
> >> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300:
> >>  
> >>> On 10.05.2023 23:53, Miquel Raynal wrote:
> >>>
> >>> Hello Martin, Miquel
> >>>  
> >>>> Hi Martin & Arseniy,
> >>>>
> >>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
> >>>> +0200:
> >>>>     
> >>>>> Hello Arseniy,
> >>>>>
> >>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
> >>>>> <AVKrasnov@sberdevices.ru> wrote:    
> >>>>>>
> >>>>>> This renames node with values for chip select from "reg" to "cs". It is
> >>>>>> needed because when OTP access is enabled on the attached storage, MTD
> >>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
> >>>>>> tries to use "reg" node in its own manner, supposes that it has another
> >>>>>> layout. All of this leads to device initialization failure.      
> >>>>> In general: if we change the device-tree interface (in this case:
> >>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be
> >>>>> updated as well.    
> >>>>
> >>>> True, and I would add, bindings should not be broken.    
> >>>
> >>> I see, that's true. That is bad way to change bindings.
> >>>  
> >>>>     
> >>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
> >>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
> >>>>> that the chip select of a NAND chip is specified with a "reg"
> >>>>> property.    
> >>>>
> >>>> All NAND controller binding expect the chip-select to be in the
> >>>> 'reg' property, very much like a spi device would use reg to store the
> >>>> cs as well: the reg property tells you how you address the device.
> >>>>
> >>>> I also fully agree with Martin's comments below. Changing reg is likely
> >>>> a wrong approach :)
> >>>>     
> >>>>> Also the code has to be backwards compatible with old .dtbs.
> >>>>>    
> >>>>>> Example:
> >>>>>>
> >>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
> >>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device
> >>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
> >>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
> >>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22      
> >>>>> This is odd - can you please share your definition of the &nfc node?    
> >>>
> >>> Sure, here it is:
> >>>
> >>> mtd_nand: nfc@7800 {                            
> >>> 	compatible = "amlogic,meson-axg-nfc";
> >>> 	...
> >>> 	nand@0 {                                
> >>>         	reg = <0>;
> >>>         };
> >>> }
> >>>
> >>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
> >>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
> >>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
> >>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?  
> >>
> >> We recently had issues with nvmem parsing, but I believe a mainline
> >> kernel should now be perfectly working on this regard. What version of
> >> the Linux kernel are you using?  
> > 
> > My current version is:
> > 
> > VERSION = 6                                                             
> > PATCHLEVEL = 2                                                          
> > SUBLEVEL = 0                                                            
> > EXTRAVERSION = -rc8 
> > 
> > Fix was in drivers/nvmem/* ?
> > 
> > Thanks, Arseniy  
> 
> Upd: I resolved problem in the following way:
> 
> nand@0 {                                
> 	reg = <0>;//chip select
> 
	partitions {
		compatible = ...

> 	otp@0 {                         
> 		#address-cells = <2>;   
> 		#size-cells = <0>;      

#address/size-cells is not needed here

> 		compatible = "user-otp";
> 		reg = <A B>;            
> 	};                              
> 	otp@1 {                         
> 		#address-cells = <2>;   
> 		#size-cells = <0>;      

Ditto

> 		compatible = "factory-otp";
> 		reg = <C D>;            
> 	};                              

	};

> };
> 
> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are
> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as
> chip select as supposed.

I don't fully get it. The parsing on the nvmem side should not fail if
there is no subpartition/otp-region defined. Can you confirm an empty
NAND device node works? Because your last e-mail suggested the opposite.

> 
> I think, this patch should be abandoned in the next version.
> 
> Thanks, Arseniy
> 
> >   
> >>
> >> Thanks,
> >> Miquèl  


Thanks,
Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Arseniy Krasnov]

On 11.05.2023 15:11, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 13:16:59 +0300:
> 
>> On 11.05.2023 12:17, Arseniy Krasnov wrote:
>>>
>>>
>>> On 11.05.2023 12:12, Miquel Raynal wrote:  
>>>> Hi Arseniy,
>>>>
>>>> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300:
>>>>  
>>>>> On 10.05.2023 23:53, Miquel Raynal wrote:
>>>>>
>>>>> Hello Martin, Miquel
>>>>>  
>>>>>> Hi Martin & Arseniy,
>>>>>>
>>>>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37
>>>>>> +0200:
>>>>>>     
>>>>>>> Hello Arseniy,
>>>>>>>
>>>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
>>>>>>> <AVKrasnov@sberdevices.ru> wrote:    
>>>>>>>>
>>>>>>>> This renames node with values for chip select from "reg" to "cs". It is
>>>>>>>> needed because when OTP access is enabled on the attached storage, MTD
>>>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
>>>>>>>> tries to use "reg" node in its own manner, supposes that it has another
>>>>>>>> layout. All of this leads to device initialization failure.      
>>>>>>> In general: if we change the device-tree interface (in this case:
>>>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be
>>>>>>> updated as well.    
>>>>>>
>>>>>> True, and I would add, bindings should not be broken.    
>>>>>
>>>>> I see, that's true. That is bad way to change bindings.
>>>>>  
>>>>>>     
>>>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
>>>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
>>>>>>> that the chip select of a NAND chip is specified with a "reg"
>>>>>>> property.    
>>>>>>
>>>>>> All NAND controller binding expect the chip-select to be in the
>>>>>> 'reg' property, very much like a spi device would use reg to store the
>>>>>> cs as well: the reg property tells you how you address the device.
>>>>>>
>>>>>> I also fully agree with Martin's comments below. Changing reg is likely
>>>>>> a wrong approach :)
>>>>>>     
>>>>>>> Also the code has to be backwards compatible with old .dtbs.
>>>>>>>    
>>>>>>>> Example:
>>>>>>>>
>>>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
>>>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device
>>>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
>>>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
>>>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22      
>>>>>>> This is odd - can you please share your definition of the &nfc node?    
>>>>>
>>>>> Sure, here it is:
>>>>>
>>>>> mtd_nand: nfc@7800 {                            
>>>>> 	compatible = "amlogic,meson-axg-nfc";
>>>>> 	...
>>>>> 	nand@0 {                                
>>>>>         	reg = <0>;
>>>>>         };
>>>>> }
>>>>>
>>>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
>>>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
>>>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
>>>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?  
>>>>
>>>> We recently had issues with nvmem parsing, but I believe a mainline
>>>> kernel should now be perfectly working on this regard. What version of
>>>> the Linux kernel are you using?  
>>>
>>> My current version is:
>>>
>>> VERSION = 6                                                             
>>> PATCHLEVEL = 2                                                          
>>> SUBLEVEL = 0                                                            
>>> EXTRAVERSION = -rc8 
>>>
>>> Fix was in drivers/nvmem/* ?
>>>
>>> Thanks, Arseniy  
>>
>> Upd: I resolved problem in the following way:
>>
>> nand@0 {                                
>> 	reg = <0>;//chip select
>>
> 	partitions {
> 		compatible = ...
> 
>> 	otp@0 {                         
>> 		#address-cells = <2>;   
>> 		#size-cells = <0>;      
> 
> #address/size-cells is not needed here
> 
>> 		compatible = "user-otp";
>> 		reg = <A B>;            
>> 	};                              
>> 	otp@1 {                         
>> 		#address-cells = <2>;   
>> 		#size-cells = <0>;      
> 
> Ditto
> 
>> 		compatible = "factory-otp";
>> 		reg = <C D>;            
>> 	};                              
> 
> 	};
> 
>> };
>>
>> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are
>> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as
>> chip select as supposed.
> 
> I don't fully get it. The parsing on the nvmem side should not fail if
> there is no subpartition/otp-region defined. Can you confirm an empty
> NAND device node works? Because your last e-mail suggested the opposite.

Ok, so i'll describe what happens in my case. Let's NAND node be like this (IIUC this is
considered as empty NAND device):

mtd_nand: nfc@7800 {                            
	compatible = "amlogic,meson-axg-nfc";
	...
	nand@0 {                                
	       	reg = <0>;
	};
}

I see, that

1) 'mtd_otp_nvmem_add()' calls 'mtd_otp_nvmem_register()' twice for two types of
   OTP memory "user-otp" and "factory-otp". Let's take a look only on "user-otp".
2) 'mtd_otp_nvmem_register()' tries to lookup for node in 'nand@0' which is compatible with
   "user-otp" and then passes found (or not found, e.g. NULL) node to  'nvmem_register()'.
3) 'nvmem_register()' uses this node iterating over its childs and searching value "reg" in
   each child. If "user-otp" node is not found in 2), 'nvmem_register()' uses node 'nfc@7800'
   also looking for "reg" value in each of its child. In this case it found "reg" in 'nand@0'
   and fails.

Now, if i add "compatible = "user-otp";" to 'nand@0', in step 2) search will be successful,
and "reg" value will be used from this new node (or we remove "reg" from it - nothing happens
as You wrote). So, problem is that nvmem tries to parse node with invalid "reg" value.

Also I see, that 'nvmem_register()' is called earlier in 'mtd_nvmem_add()', but with no effect.
I think, that it is not related with enabled OTP feature.

Thanks, Arseniy

> 
>>
>> I think, this patch should be abandoned in the next version.
>>
>> Thanks, Arseniy
>>
>>>   
>>>>
>>>> Thanks,
>>>> Miquèl  
> 
> 
> Thanks,
> Miquèl

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Miquel Raynal]

Hi Arseniy,

I'm adding Rafał & Michael: any idea what could be wrong? The behavior
below does not look expected at all, but I thought we (= Rafał, mainly)
already sorted this out?

> >>>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
> >>>>>>> <AVKrasnov@sberdevices.ru> wrote:      
> >>>>>>>>
> >>>>>>>> This renames node with values for chip select from "reg" to "cs". It is
> >>>>>>>> needed because when OTP access is enabled on the attached storage, MTD
> >>>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
> >>>>>>>> tries to use "reg" node in its own manner, supposes that it has another
> >>>>>>>> layout. All of this leads to device initialization failure.        
> >>>>>>> In general: if we change the device-tree interface (in this case:
> >>>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be
> >>>>>>> updated as well.      
> >>>>>>
> >>>>>> True, and I would add, bindings should not be broken.      
> >>>>>
> >>>>> I see, that's true. That is bad way to change bindings.
> >>>>>    
> >>>>>>       
> >>>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
> >>>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
> >>>>>>> that the chip select of a NAND chip is specified with a "reg"
> >>>>>>> property.      
> >>>>>>
> >>>>>> All NAND controller binding expect the chip-select to be in the
> >>>>>> 'reg' property, very much like a spi device would use reg to store the
> >>>>>> cs as well: the reg property tells you how you address the device.
> >>>>>>
> >>>>>> I also fully agree with Martin's comments below. Changing reg is likely
> >>>>>> a wrong approach :)
> >>>>>>       
> >>>>>>> Also the code has to be backwards compatible with old .dtbs.
> >>>>>>>      
> >>>>>>>> Example:
> >>>>>>>>
> >>>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
> >>>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device
> >>>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
> >>>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
> >>>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22        
> >>>>>>> This is odd - can you please share your definition of the &nfc node?      
> >>>>>
> >>>>> Sure, here it is:
> >>>>>
> >>>>> mtd_nand: nfc@7800 {                            
> >>>>> 	compatible = "amlogic,meson-axg-nfc";
> >>>>> 	...
> >>>>> 	nand@0 {                                
> >>>>>         	reg = <0>;
> >>>>>         };
> >>>>> }
> >>>>>
> >>>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
> >>>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
> >>>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
> >>>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?    
> >>>>
> >>>> We recently had issues with nvmem parsing, but I believe a mainline
> >>>> kernel should now be perfectly working on this regard. What version of
> >>>> the Linux kernel are you using?    
> >>>
> >>> My current version is:
> >>>
> >>> VERSION = 6                                                             
> >>> PATCHLEVEL = 2                                                          
> >>> SUBLEVEL = 0                                                            
> >>> EXTRAVERSION = -rc8 
> >>>
> >>> Fix was in drivers/nvmem/* ?
> >>>
> >>> Thanks, Arseniy    
> >>
> >> Upd: I resolved problem in the following way:
> >>
> >> nand@0 {                                
> >> 	reg = <0>;//chip select
> >>  
> > 	partitions {
> > 		compatible = ...
> >   
> >> 	otp@0 {                         
> >> 		#address-cells = <2>;   
> >> 		#size-cells = <0>;        
> > 
> > #address/size-cells is not needed here
> >   
> >> 		compatible = "user-otp";
> >> 		reg = <A B>;            
> >> 	};                              
> >> 	otp@1 {                         
> >> 		#address-cells = <2>;   
> >> 		#size-cells = <0>;        
> > 
> > Ditto
> >   
> >> 		compatible = "factory-otp";
> >> 		reg = <C D>;            
> >> 	};                                
> > 
> > 	};
> >   
> >> };
> >>
> >> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are
> >> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as
> >> chip select as supposed.  
> > 
> > I don't fully get it. The parsing on the nvmem side should not fail if
> > there is no subpartition/otp-region defined. Can you confirm an empty
> > NAND device node works? Because your last e-mail suggested the opposite.  
> 
> Ok, so i'll describe what happens in my case. Let's NAND node be like this (IIUC this is
> considered as empty NAND device):
> 
> mtd_nand: nfc@7800 {                            
> 	compatible = "amlogic,meson-axg-nfc";
> 	...
> 	nand@0 {                                
> 	       	reg = <0>;
> 	};
> }
> 
> I see, that
> 
> 1) 'mtd_otp_nvmem_add()' calls 'mtd_otp_nvmem_register()' twice for two types of
>    OTP memory "user-otp" and "factory-otp". Let's take a look only on "user-otp".
> 2) 'mtd_otp_nvmem_register()' tries to lookup for node in 'nand@0' which is compatible with
>    "user-otp" and then passes found (or not found, e.g. NULL) node to  'nvmem_register()'.
> 3) 'nvmem_register()' uses this node iterating over its childs and searching value "reg" in
>    each child. If "user-otp" node is not found in 2), 'nvmem_register()' uses node 'nfc@7800'
>    also looking for "reg" value in each of its child. In this case it found "reg" in 'nand@0'
>    and fails.
> 
> Now, if i add "compatible = "user-otp";" to 'nand@0', in step 2) search will be successful,
> and "reg" value will be used from this new node (or we remove "reg" from it - nothing happens
> as You wrote). So, problem is that nvmem tries to parse node with invalid "reg" value.
> 
> Also I see, that 'nvmem_register()' is called earlier in 'mtd_nvmem_add()', but with no effect.
> I think, that it is not related with enabled OTP feature.
> 
> Thanks, Arseniy

Re: [PATCH v3 6/6] mtd: rawnand: meson: rename node for chip select

[Arseniy Krasnov]

On 12.05.2023 17:49, Miquel Raynal wrote:
> Hi Arseniy,
> 
> I'm adding Rafał & Michael: any idea what could be wrong? The behavior
> below does not look expected at all, but I thought we (= Rafał, mainly)
> already sorted this out?
> 

Hi Miquel, thanks for help!

just to clarify an expected behaviour: if i have the following layout in the device tree

mtd_nand: nfc@7800 {                            
 	compatible = "amlogic,meson-axg-nfc";
 	...
 	nand@0 {                                
         	reg = <0>;
         };
}

node used by 'nvmem_add_cells_from_of()' must be NULL? or 'nand@0'?

I guess, that in above dts I have node 'nfc@7800' in use, because 'mtd_otp_nvmem_register()' uses
the following device before passing 'config' to 'nvmem_register()':

        /* OTP nvmem will be registered on the physical device */       
        config.dev = mtd->dev.parent;

'mtd->dev.parent' is 'nfc@7800'.

May be 'mtd_otp_nvmem_register()' must initialize 'no_of_node' field of 'config' like in 'mtd_nvmem_add()' ?
This field is documented as:

* @no_of_node:	Device should not use the parent's of_node even if it's !NULL.

In this case node passed to 'nvmem_add_cells_from_of()' will be NULL.

Thanks, Arseniy

>>>>>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov
>>>>>>>>> <AVKrasnov@sberdevices.ru> wrote:      
>>>>>>>>>>
>>>>>>>>>> This renames node with values for chip select from "reg" to "cs". It is
>>>>>>>>>> needed because when OTP access is enabled on the attached storage, MTD
>>>>>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn
>>>>>>>>>> tries to use "reg" node in its own manner, supposes that it has another
>>>>>>>>>> layout. All of this leads to device initialization failure.        
>>>>>>>>> In general: if we change the device-tree interface (in this case:
>>>>>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be
>>>>>>>>> updated as well.      
>>>>>>>>
>>>>>>>> True, and I would add, bindings should not be broken.      
>>>>>>>
>>>>>>> I see, that's true. That is bad way to change bindings.
>>>>>>>    
>>>>>>>>       
>>>>>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and
>>>>>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show
>>>>>>>>> that the chip select of a NAND chip is specified with a "reg"
>>>>>>>>> property.      
>>>>>>>>
>>>>>>>> All NAND controller binding expect the chip-select to be in the
>>>>>>>> 'reg' property, very much like a spi device would use reg to store the
>>>>>>>> cs as well: the reg property tells you how you address the device.
>>>>>>>>
>>>>>>>> I also fully agree with Martin's comments below. Changing reg is likely
>>>>>>>> a wrong approach :)
>>>>>>>>       
>>>>>>>>> Also the code has to be backwards compatible with old .dtbs.
>>>>>>>>>      
>>>>>>>>>> Example:
>>>>>>>>>>
>>>>>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/...
>>>>>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device
>>>>>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22
>>>>>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips
>>>>>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22        
>>>>>>>>> This is odd - can you please share your definition of the &nfc node?      
>>>>>>>
>>>>>>> Sure, here it is:
>>>>>>>
>>>>>>> mtd_nand: nfc@7800 {                            
>>>>>>> 	compatible = "amlogic,meson-axg-nfc";
>>>>>>> 	...
>>>>>>> 	nand@0 {                                
>>>>>>>         	reg = <0>;
>>>>>>>         };
>>>>>>> }
>>>>>>>
>>>>>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose
>>>>>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called
>>>>>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such
>>>>>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ?    
>>>>>>
>>>>>> We recently had issues with nvmem parsing, but I believe a mainline
>>>>>> kernel should now be perfectly working on this regard. What version of
>>>>>> the Linux kernel are you using?    
>>>>>
>>>>> My current version is:
>>>>>
>>>>> VERSION = 6                                                             
>>>>> PATCHLEVEL = 2                                                          
>>>>> SUBLEVEL = 0                                                            
>>>>> EXTRAVERSION = -rc8 
>>>>>
>>>>> Fix was in drivers/nvmem/* ?
>>>>>
>>>>> Thanks, Arseniy    
>>>>
>>>> Upd: I resolved problem in the following way:
>>>>
>>>> nand@0 {                                
>>>> 	reg = <0>;//chip select
>>>>  
>>> 	partitions {
>>> 		compatible = ...
>>>   
>>>> 	otp@0 {                         
>>>> 		#address-cells = <2>;   
>>>> 		#size-cells = <0>;        
>>>
>>> #address/size-cells is not needed here
>>>   
>>>> 		compatible = "user-otp";
>>>> 		reg = <A B>;            
>>>> 	};                              
>>>> 	otp@1 {                         
>>>> 		#address-cells = <2>;   
>>>> 		#size-cells = <0>;        
>>>
>>> Ditto
>>>   
>>>> 		compatible = "factory-otp";
>>>> 		reg = <C D>;            
>>>> 	};                                
>>>
>>> 	};
>>>   
>>>> };
>>>>
>>>> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are
>>>> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as
>>>> chip select as supposed.  
>>>
>>> I don't fully get it. The parsing on the nvmem side should not fail if
>>> there is no subpartition/otp-region defined. Can you confirm an empty
>>> NAND device node works? Because your last e-mail suggested the opposite.  
>>
>> Ok, so i'll describe what happens in my case. Let's NAND node be like this (IIUC this is
>> considered as empty NAND device):
>>
>> mtd_nand: nfc@7800 {                            
>> 	compatible = "amlogic,meson-axg-nfc";
>> 	...
>> 	nand@0 {                                
>> 	       	reg = <0>;
>> 	};
>> }
>>
>> I see, that
>>
>> 1) 'mtd_otp_nvmem_add()' calls 'mtd_otp_nvmem_register()' twice for two types of
>>    OTP memory "user-otp" and "factory-otp". Let's take a look only on "user-otp".
>> 2) 'mtd_otp_nvmem_register()' tries to lookup for node in 'nand@0' which is compatible with
>>    "user-otp" and then passes found (or not found, e.g. NULL) node to  'nvmem_register()'.
>> 3) 'nvmem_register()' uses this node iterating over its childs and searching value "reg" in
>>    each child. If "user-otp" node is not found in 2), 'nvmem_register()' uses node 'nfc@7800'
>>    also looking for "reg" value in each of its child. In this case it found "reg" in 'nand@0'
>>    and fails.
>>
>> Now, if i add "compatible = "user-otp";" to 'nand@0', in step 2) search will be successful,
>> and "reg" value will be used from this new node (or we remove "reg" from it - nothing happens
>> as You wrote). So, problem is that nvmem tries to parse node with invalid "reg" value.
>>
>> Also I see, that 'nvmem_register()' is called earlier in 'mtd_nvmem_add()', but with no effect.
>> I think, that it is not related with enabled OTP feature.
>>
>> Thanks, Arseniy