[U-Boot] [PATCH 1/4 v3] spi: spi-mem: Use 2 SPI messages instead of a single full-duplex one

[U-Boot] [PATCH 1/4 v3] spi: spi-mem: Use 2 SPI messages instead of a single full-duplex one

[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 be transmitted.

With this change, only a small buffer for the command, address and
dummy bytes needs to be allocated. We use the TX and RX buffers that are
passed to spi_mem_exec_op() directly.

Signed-off-by: Stefan Roese <sr@denx.de>
Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
v3:
- Allocate buffer for cmd, addr and dummy bytes
- Return code for spi_xfer is checked
- Drop rx_data and tx_data and use rx_buf / tx_buf instead
  as suggested by Boris
- Minor coding style changes

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 | 85 ++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 42 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 07ce799170..af9aef009a 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -196,12 +196,14 @@ EXPORT_SYMBOL_GPL(spi_mem_supports_op);
  */
 int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 {
-	bool rx_data = op->data.nbytes && (op->data.dir == SPI_MEM_DATA_IN);
-	bool tx_data = op->data.nbytes && (op->data.dir == SPI_MEM_DATA_OUT);
 	struct udevice *bus = slave->dev->parent;
 	struct dm_spi_ops *ops = spi_get_ops(bus);
-	unsigned int xfer_len, pos = 0;
-	u8 *tx_buf, *rx_buf = NULL;
+	unsigned int pos = 0;
+	const u8 *tx_buf = NULL;
+	u8 *rx_buf = NULL;
+	u8 *op_buf;
+	int op_len;
+	u32 flag;
 	int ret;
 	int i;
 
@@ -330,67 +332,66 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		return -ENOTSUPP;
 	}
 
-	xfer_len = sizeof(op->cmd.opcode) + op->addr.nbytes +
-		   op->dummy.nbytes + op->data.nbytes;
-
-	/*
-	 * 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;
-
-	if (rx_data) {
-		rx_buf = kzalloc(xfer_len, GFP_KERNEL);
-		if (!rx_buf)
-			return -ENOMEM;
+	if (op->data.nbytes) {
+		if (op->data.dir == SPI_MEM_DATA_IN)
+			rx_buf = op->data.buf.in;
+		else
+			tx_buf = op->data.buf.out;
 	}
 
+	op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
+	op_buf = calloc(1, 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) {
-		memset(tx_buf + pos, 0xff, op->dummy.nbytes);
-		pos += op->dummy.nbytes;
-	}
+	if (op->dummy.nbytes)
+		memset(op_buf + pos, 0xff, op->dummy.nbytes);
+
+	/* 1st transfer: opcode + address + dummy cycles */
+	flag = SPI_XFER_BEGIN;
+	/* Make sure to set END bit if no tx or rx data messages follow */
+	if (!tx_buf && !rx_buf)
+		flag |= SPI_XFER_END;
+
+	ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
+	if (ret)
+		return ret;
 
-	if (tx_data)
-		memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);
+	/* 2nd transfer: rx or tx data path */
+	if (tx_buf || rx_buf) {
+		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
+			       rx_buf, SPI_XFER_END);
+		if (ret)
+			return ret;
+	}
 
-	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++)
-		debug("%02x ", tx_data ? tx_buf[i] : rx_buf[i]);
+	      tx_buf || rx_buf ? op->data.nbytes : 0,
+	      tx_buf || rx_buf ? (tx_buf ? "out" : "in") : "-");
+	for (i = 0; i < op->data.nbytes; i++)
+		debug("%02x ", tx_buf ? tx_buf[i] : rx_buf[i]);
 	debug("[ret %d]\n", ret);
 
+	free(op_buf);
+
 	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;
-- 
2.18.0

[U-Boot] [PATCH 2/4 v3] cmd: mtd: Don't abort erase operation when a bad block is detected

[Stefan Roese]

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 <sr@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
---
v3:
- Handle bad-block skipping completely in cmd/mtd. as suggested by
  Boris
- Add message upon bad-block detection

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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 221b12500f..9f552e7c4a 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -360,7 +360,21 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		erase_op.len = len;
 		erase_op.scrub = scrub;
 
-		ret = mtd_erase(mtd, &erase_op);
+		while (erase_op.len) {
+			ret = mtd_erase(mtd, &erase_op);
+
+			/* Abort if its not an bad block error */
+			if (ret != -EIO)
+				break;
+
+			printf("Skipping bad block at 0x%08llx\n",
+			       erase_op.fail_addr);
+
+			/* Skip bad block and continue behind it */
+			erase_op.len -= erase_op.fail_addr - erase_op.addr;
+			erase_op.len -= mtd->erasesize;
+			erase_op.addr = erase_op.fail_addr + mtd->erasesize;
+		}
 	} else {
 		return CMD_RET_USAGE;
 	}
-- 
2.18.0

[U-Boot] [PATCH 3/4 v3] cmd: mtd: Don't use with negative return codes for shell commands

[Stefan Roese]

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 <sr@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
v3:
- No changes

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

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

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 9f552e7c4a..b0ad0616cc 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -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;
@@ -379,7 +379,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[] =
-- 
2.18.0

[U-Boot] [PATCH 4/4 v3] cmd: mtd: Add info text to mtd erase subcommand

[Stefan Roese]

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 <sr@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
---
v3:
- No changes

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

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

diff --git a/cmd/mtd.c b/cmd/mtd.c
index b0ad0616cc..03a48e7e22 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -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;
-- 
2.18.0