spi: spi-mem: Use 2 SPI messages instead of a single full-duplex one

It was noticed, that the erase command (mtd erase spi-nand0) aborts upon the first bad block. With this change, bad blocks are now skipped and the erase operation will continue.

Signed-off-by Stefan Roese <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Use an U-Boot "mtd" command specific option to skip the bad block upon erase so that other MTD users are not affected by this change

 cmd/mtd.c               | 1 +
 drivers/mtd/nand/core.c | 6 ++++++
 include/linux/mtd/mtd.h | 1 +
 3 files changed, 8 insertions(+)

cmd/mtd.c +1 -0

index 221b12500f..999d686e66 100644
@@ -359,6 +359,7 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		erase_op.addr = off;
 		erase_op.len = len;
 		erase_op.scrub = scrub;
+		erase_op.skipbad = true;
 
 		ret = mtd_erase(mtd, &erase_op);
 	} else {

drivers/mtd/nand/core.c +0 -0

index 0b793695cc..c9d7a646f6 100644

drivers/mtd/nand/core.c +0 -0

 			einfo->fail_addr = nanddev_pos_to_offs(nand, &pos);
 
 			return ret;

include/linux/mtd/mtd.h +1 -0

index 4ca5d764d7..3a09dbab95 100644
@@ -53,6 +53,7 @@ struct erase_info {
 	u_char state;
 	struct erase_info *next;
 	int scrub;
+	bool skipbad;
 };
 
 struct mtd_erase_region_info {

When negative return codes are used in commands (do_foo()), the shell prints these messages:

exit not allowed from main input shell.

Change the return codes in the new mtd commands to use only positive values and these annoying warnings are gone.

Signed-off-by Stefan Roese <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Use CMD_RET_FAILURE as return value as suggested by Boris

 cmd/mtd.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

cmd/mtd.c +10 -8

index 999d686e66..b29aec18ca 100644
@@ -188,7 +188,7 @@ static int do_mtd_list(void)
 
 	if (!dev_nb) {
 		printf("No MTD device found\n");
-		return -EINVAL;
+		return CMD_RET_FAILURE;
 	}
 
 	return 0;

@@ -269,13 +269,13 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (mtd_is_aligned_with_min_io_size(mtd, off)) {
 			printf("Offset not aligned with a page (0x%x)\n",
 			       mtd->writesize);
-			return -EINVAL;
+			return CMD_RET_FAILURE;
 		}
 
 		if (mtd_is_aligned_with_min_io_size(mtd, len)) {
 			printf("Size not a multiple of a page (0x%x)\n",
 			       mtd->writesize);
-			return -EINVAL;
+			return CMD_RET_FAILURE;
 		}
 
 		if (dump)

@@ -285,7 +285,7 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 		if (!buf) {
 			printf("Could not map/allocate the user buffer\n");
-			return -ENOMEM;
+			return CMD_RET_FAILURE;
 		}
 
 		printf("%s %lldB (%d page(s)) at offset 0x%08llx%s%s\n",

@@ -306,7 +306,7 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (ret) {
 			printf("%s on %s failed with error %d\n",
 			       read ? "Read" : "Write", mtd->name, ret);
-			return ret;
+			return CMD_RET_FAILURE;
 		}
 
 		if (dump) {

@@ -346,13 +346,13 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (mtd_is_aligned_with_block_size(mtd, off)) {
 			printf("Offset not aligned with a block (0x%x)\n",
 			       mtd->erasesize);
-			return -EINVAL;
+			return CMD_RET_FAILURE;
 		}
 
 		if (mtd_is_aligned_with_block_size(mtd, len)) {
 			printf("Size not a multiple of a block (0x%x)\n",
 			       mtd->erasesize);
-			return -EINVAL;
+			return CMD_RET_FAILURE;
 		}
 
 		erase_op.mtd = mtd;

@@ -366,7 +366,9 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return CMD_RET_USAGE;
 	}
 
-	return ret;
+	if (ret)
+		return CMD_RET_FAILURE;
+	return 0;
 }
 
 static char mtd_help_text[] =

Adding this info helps seeing, what really is being erased - especially if no arguments are passed for offset and size. Now this is the output:

=> mtd erase spi-nand0
Erasing 0x00000000 ... 0x07ffffff (1024 eraseblock(s))
nand: attempt to erase a bad/reserved block @6000000
nand: attempt to erase a bad/reserved block @7fe0000

Signed-off-by Stefan Roese <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Print number of eraseblocks instead of pages as suggested by Boris

 cmd/mtd.c | 3 +++
 1 file changed, 3 insertions(+)

cmd/mtd.c +0 -0

index b29aec18ca..7dc77edf28 100644

Hi Stefan,

Quoted 25 lines · On Tue, 7 Aug 2018 14:16:55 +0200 Stefan Roese <[email protected]> wrote:

Adding this info helps seeing, what really is being erased - especially if no arguments are passed for offset and size. Now this is the output:

=> mtd erase spi-nand0
Erasing 0x00000000 ... 0x07ffffff (1024 eraseblock(s))
nand: attempt to erase a bad/reserved block @6000000
nand: attempt to erase a bad/reserved block @7fe0000

Signed-off-by Stefan Roese <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Print number of eraseblocks instead of pages as suggested by Boris

  cmd/mtd.c | 3 +++
  1 file changed, 3 insertions(+)

cmd/mtd.c +0 -0

index b29aec18ca..7dc77edf28 100644

cmd/mtd.c static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 363

Quoted 9 lines

cmd/mtd.c +3 -0

@@ -355,6 +355,9 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  			return CMD_RET_FAILURE;
  		}
 
+		printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
+		       off, off + len - 1, mtd_div_by_eb(len, mtd));
+
  		erase_op.mtd = mtd;
  		erase_op.addr = off;
  		erase_op.len = len;

Hm, you might want to tell the user where the request failed (when it failed).

Also, maybe we should squash your changes in Miquel's commits since the series has not been merged yet. Unless you'd like to keep them separate.

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
v2:
- Replaces patch "spi: spi-mem: Add optional half-duplex SPI transfer mode" from first patchset version
- No compile-time option but the default to use 2 separate SPI messages to transfer the command and data

 drivers/spi/spi-mem.c | 74 +++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 35 deletions(-)

drivers/spi/spi-mem.c +2 -0

index 07ce799170..84e33aa979 100644
@@ -15,6 +15,8 @@
 #include <spi-mem.h>
 #endif
 
+#define OP_BUFFER_SIZE_MAX	16	/* Max size for cmd + addr + dummy */
+
 #ifndef __UBOOT__
 /**
  * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a

drivers/spi/spi-mem.c +0 -8

-	/*
-	 * Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so
-	 * we're guaranteed that this buffer is DMA-able, as required by the
-	 * SPI layer.
-	 */
-	tx_buf = kzalloc(xfer_len, GFP_KERNEL);
-	if (!tx_buf)
-		return -ENOMEM;

drivers/spi/spi-mem.c +4 -4

+	memset(op_buf, 0x00, op_len);
 
 	ret = spi_claim_bus(slave);
 	if (ret < 0)
 		return ret;
 
-	tx_buf[pos++] = op->cmd.opcode;
+	op_buf[pos++] = op->cmd.opcode;
 
 	if (op->addr.nbytes) {
 		for (i = 0; i < op->addr.nbytes; i++)
-			tx_buf[pos + i] = op->addr.val >>
-					 (8 * (op->addr.nbytes - i - 1));
+			op_buf[pos + i] = op->addr.val >>
+				(8 * (op->addr.nbytes - i - 1));
 
 		pos += op->addr.nbytes;
 	}
 
-	if (op->dummy.nbytes) {

drivers/spi/spi-mem.c +2 -10

-	ret = spi_xfer(slave, xfer_len * 8, tx_buf, rx_buf,
-		       SPI_XFER_BEGIN | SPI_XFER_END);
 	spi_release_bus(slave);
 
 	for (i = 0; i < pos; i++)
-		debug("%02x ", tx_buf[i]);
+		debug("%02x ", op_buf[i]);
 	debug("| [%dB %s] ",
 	      tx_data || rx_data ? op->data.nbytes : 0,
 	      tx_data || rx_data ? (tx_data ? "out" : "in") : "-");
-	for (; i < xfer_len; i++)
+	for (i = 0; i < op->data.nbytes; i++)
 		debug("%02x ", tx_data ? tx_buf[i] : rx_buf[i]);
 	debug("[ret %d]\n", ret);
 
 	if (ret < 0)
 		return ret;
-
-	if (rx_data)
-		memcpy(op->data.buf.in, rx_buf + pos, op->data.nbytes);
-
-	kfree(tx_buf);
-	kfree(rx_buf);
 #endif /* __UBOOT__ */
 
 	return 0;

Quoted 4 lines · On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese <[email protected]> wrote:

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 8 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Looks good overall, just a few comments (that you might chose to ignore if you disagree).

Reviewed-by Boris Brezillon <[email protected]>

Boris Brezillon <[email protected]> wrote on Tue, 7 Aug 2018 15:28:02 +0200:

Quoted 16 lines

Quoted 4 lines · On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese <[email protected]> wrote:

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 8 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Quoted 5 lines

Looks good overall, just a few comments (that you might chose to ignore if you disagree).

Reviewed-by Boris Brezillon <[email protected]>

Sorry for being a bit late on the discussion, but while I do agree with the change, I'm not sure about its implementation : I think SPI controllers are supposed to be abstracted by the SPI layer. Addressing the controller's limitations in the SPI-mem layer would not be appropriate.

Would it be possible to adapt spi_xfer() to handle such case?

Hi Miquel,

Quoted 28 lines · Miquel Raynal

Boris Brezillon <[email protected]> wrote on Tue, 7 Aug 2018 15:28:02 +0200:

Quoted 16 lines

Quoted 1 line

On Tue, 7 Aug 2018 14:16:52 +0200

Quoted 4 lines · Stefan Roese

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 8 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Quoted 6 lines

Looks good overall, just a few comments (that you might chose to ignore if you disagree).

Reviewed-by Boris Brezillon <[email protected]>

Sorry for being a bit late on the discussion, but while I do agree with

the change, I'm not sure about its implementation : I think SPI controllers are supposed to be abstracted by the SPI layer. Addressing the controller's limitations in the SPI-mem layer would not be appropriate.

Quoted 1 line

Quoted 1 line

Would it be possible to adapt spi_xfer() to handle such case?

No, I don't think so. Its impossible to guess in the SPI driver layer, which parts of the message are TX data and which are RX data. So this message can't be split up into a half-duplex one here. This can only be done in the driver which constructs the SPI messages.

Or did I miss something here?

Stefan Roese <[email protected]> wrote on Thu, 9 Aug 2018 07:24:14 +0200:

Quoted 31 lines

Hi Miquel,

Quoted 28 lines · Miquel Raynal

Boris Brezillon <[email protected]> wrote on Tue, 7 Aug 2018 15:28:02 +0200:

Quoted 16 lines

Quoted 1 line

On Tue, 7 Aug 2018 14:16:52 +0200

Quoted 4 lines · Stefan Roese

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 8 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Quoted 6 lines

Looks good overall, just a few comments (that you might chose to ignore if you disagree).

Reviewed-by Boris Brezillon <[email protected]>

Sorry for being a bit late on the discussion, but while I do agree with

the change, I'm not sure about its implementation : I think SPI controllers are supposed to be abstracted by the SPI layer. Addressing the controller's limitations in the SPI-mem layer would not be appropriate.

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Would it be possible to adapt spi_xfer() to handle such case?

No, I don't think so. Its impossible to guess in the SPI driver layer, which parts of the message are TX data and which are RX data. So this message can't be split up into a half-duplex one here. This can only be done in the driver which constructs the SPI messages.

Or did I miss something here?

Actually I'm fine with it as SPI-mem only uses half-duplex. It's not necessary to limit its use to SPI controllers (drivers) supporting full-duplex.

Stefan, shall I fold your changes in my series and resend? Or do I resend only my original patches and Jagan/Tom will apply yours on top of it?

Hi Miquel,

Quoted 42 lines · Miquel Raynal

Stefan Roese <[email protected]> wrote on Thu, 9 Aug 2018 07:24:14 +0200:

Quoted 32 lines

Quoted 1 line

Hi Miquel,

Quoted 20 lines · Miquel Raynal

Boris Brezillon <[email protected]> wrote on Tue, 7 Aug 2018 15:28:02 +0200:

Quoted 18 lines

Quoted 1 line

On Tue, 7 Aug 2018 14:16:52 +0200

Quoted 4 lines · Stefan Roese

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Some SPI controller do not support full-duplex SPI transfers. This patch

changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 8 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Looks good overall, just a few comments (that you might chose to ignore if you disagree).

Quoted 8 lines

Quoted 3 lines

Reviewed-by Boris Brezillon <[email protected]>

Sorry for being a bit late on the discussion, but while I do agree with

the change, I'm not sure about its implementation : I think SPI controllers are supposed to be abstracted by the SPI layer. Addressing the controller's limitations in the SPI-mem layer would not be appropriate.

Quoted 1 line

Would it be possible to adapt spi_xfer() to handle such case?

Quoted 7 lines

No, I don't think so. Its impossible to guess in the SPI driver layer, which parts of the message are TX data and which are RX data. So this message can't be split up into a half-duplex one here. This can only be done in the driver which constructs the SPI messages.

Or did I miss something here?
Actually I'm fine with it as SPI-mem only uses half-duplex. It's not

necessary to limit its use to SPI controllers (drivers) supporting full-duplex.

Quoted 3 lines

Quoted 1 line

Stefan, shall I fold your changes in my series and resend? Or do I

resend only my original patches and Jagan/Tom will apply yours on top of it?

Do you need to re-send your original patches? If yes, then please do with my patches folded in. If a re-send is not necessary, then Jagan or Tom should be able to apply your latest patchset and mine on top of it.

Stefan Roese <[email protected]> wrote on Thu, 9 Aug 2018 11:10:15 +0200:

Quoted 49 lines

Hi Miquel,

Quoted 45 lines · Miquel Raynal

Stefan Roese <[email protected]> wrote on Thu, 9 Aug 2018 07:24:14 +0200:

Quoted 32 lines

Quoted 1 line

Hi Miquel,

Quoted 20 lines · Miquel Raynal

Boris Brezillon <[email protected]> wrote on Tue, 7 Aug 2018 15:28:02 +0200:

Quoted 18 lines

Quoted 1 line

On Tue, 7 Aug 2018 14:16:52 +0200

Quoted 4 lines · Stefan Roese

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Some SPI controller do not support full-duplex SPI transfers. This patch

changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 8 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Looks good overall, just a few comments (that you might chose to ignore if you disagree).

Quoted 8 lines

Quoted 3 lines

Reviewed-by Boris Brezillon <[email protected]>

Sorry for being a bit late on the discussion, but while I do agree with

the change, I'm not sure about its implementation : I think SPI controllers are supposed to be abstracted by the SPI layer. Addressing the controller's limitations in the SPI-mem layer would not be appropriate.

Quoted 1 line

Would it be possible to adapt spi_xfer() to handle such case?

Quoted 7 lines

No, I don't think so. Its impossible to guess in the SPI driver layer, which parts of the message are TX data and which are RX data. So this message can't be split up into a half-duplex one here. This can only be done in the driver which constructs the SPI messages.

Or did I miss something here?
Actually I'm fine with it as SPI-mem only uses half-duplex. It's not

necessary to limit its use to SPI controllers (drivers) supporting full-duplex.

Quoted 1 line

Stefan, shall I fold your changes in my series and resend? Or do I

resend only my original patches and Jagan/Tom will apply yours on top of it?

Quoted 4 lines

Do you need to re-send your original patches? If yes, then please do with my patches folded in. If a re-send is not necessary, then Jagan or Tom should be able to apply your latest patchset and mine on top of it.

I suppose Jagan prefers formal patches than a Github branch so basically I have to pull Boris fixes, squash them, do the same with yours and resend.

As we missed the merge window, maybe I'll also take the time to rework mtdparts. I plan to do that next week.

Quoted 3 lines · On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese <[email protected]> wrote:

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

Quoted 9 lines

With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.

Signed-off-by Stefan Roese <[email protected]>

Suggested-by Boris Brezillon <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

Hi Stefan,

Minor typo if you decide to send a v3.

Boris Brezillon <[email protected]> wrote on Tue, 7 Aug 2018 15:28:02 +0200:

Quoted 6 lines

Quoted 3 lines · On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese <[email protected]> wrote:

Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.

       ^ be ?