spi: spi-mem: Add optional half-duplex SPI transfer mode

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]>

---
 drivers/mtd/nand/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 0b793695cc..888f765b90 100644
@@ -162,7 +162,7 @@ int nanddev_mtd_erase(struct mtd_info *mtd, struct erase_info *einfo)
 	nanddev_offs_to_pos(nand, einfo->addr + einfo->len - 1, &last);
 	while (nanddev_pos_cmp(&pos, &last) <= 0) {
 		ret = nanddev_erase(nand, &pos);
-		if (ret) {
+		if (ret && ret != -EIO) {
 			einfo->fail_addr = nanddev_pos_to_offs(nand, &pos);
 
 			return ret;

Hi Stefan,

On Mon, 6 Aug 2018 17:12:51 +0200
Stefan Roese <[email protected]> wrote:

Quoted 4 lines

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.

That's not what the raw NAND framework does [1], and I'm almost sure MTD users expect erase ops to stop when a bad block is found and "skip bad block" was not explicitly requested.

I'd suggest moving to an approach where cmd/mtd.c erases blocks one by one and checks the status of each block (with mtd_block_isbad()) before calling mtd_erase(). Alternatively, we could add a 'bool skipbad' field to struct erase_info, but that means getting away from Linux implementation (which is already the case since uboot has an extra "int scrub" field).

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]>

---
 cmd/mtd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

cmd/mtd.c +0 -0

index 221b12500f..38a89736cf 100644

Quoted 22 lines · On Mon, 6 Aug 2018 17:12:52 +0200 Stefan Roese <[email protected]> wrote:

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]>

---
  cmd/mtd.c | 14 +++++++-------
  1 file changed, 7 insertions(+), 7 deletions(-)

cmd/mtd.c +0 -0

index 221b12500f..38a89736cf 100644
@@ -188,7 +188,7 @@ static int do_mtd_list(void)
 

cmd/mtd.c static int do_mtd_list(void) · line 191

Quoted 4 lines

cmd/mtd.c +1 -1

  	if (!dev_nb) {
  		printf("No MTD device found\n");
-		return -EINVAL;
+		return EINVAL;

How about using CMD_RET_FAILURE for all errors?

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 (65536 page(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]>

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

cmd/mtd.c +0 -0

index 38a89736cf..6d27698d1e 100644

cmd/mtd.c +1 -0

+
 		erase_op.mtd = mtd;
 		erase_op.addr = off;
 		erase_op.len = len;

Quoted 25 lines · On Mon, 6 Aug 2018 17:12:53 +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 (65536 page(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]>

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

cmd/mtd.c +0 -0

index 38a89736cf..6d27698d1e 100644
@@ -355,6 +355,9 @@ static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  			return EINVAL;
  		}
 

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

Quoted 2 lines

cmd/mtd.c +2 -0

+		printf("Erasing 0x%08llx ... 0x%08llx (%d page(s))\n",
+		       off, off + len - 1, mtd_len_to_pages(mtd, len));

Just a detail, but we usually count things in eraseblocks (not pages) when erasing an MTD device (you can use mtd_div_by_eb(len, mtd) to do that).

Some SPI controller might now support full-duplex SPI transfers. This option can be enabled to use half-duplex operation mode for such SPI controllers.

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

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/spi/Kconfig   |  8 ++++++++
 drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

drivers/spi/Kconfig +8 -0

index 9fbd26740d..5bd8289284 100644
@@ -25,6 +25,14 @@ config SPI_MEM
 	  This extension is meant to simplify interaction with SPI memories
 	  by providing an high-level interface to send memory-like commands.
 
+config SPI_MEM_HALF_DUPLEX
+	bool "Use half-duplex SPI transfer"
+	depends on SPI_MEM
+	help
+	  Some SPI controller might not support full-duplex SPI transfers.
+	  This option can be enabled to use half-duplex operation mode for
+	  such SPI controllers.
+
 config ALTERA_SPI
 	bool "Altera SPI driver"
 	help

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

index 07ce799170..617fbbfe09 100644
@@ -202,6 +202,10 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	struct dm_spi_ops *ops = spi_get_ops(bus);
 	unsigned int xfer_len, pos = 0;
 	u8 *tx_buf, *rx_buf = NULL;
+	unsigned int pos2;
+	int tx_len;
+	int rx_len;
+	u32 flag;
 	int ret;
 	int i;
 

@@ -370,8 +374,29 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	if (tx_data)
 		memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);
 
-	ret = spi_xfer(slave, xfer_len * 8, tx_buf, rx_buf,
-		       SPI_XFER_BEGIN | SPI_XFER_END);
+	pos2 = pos;
+	if (CONFIG_IS_ENABLED(SPI_MEM_HALF_DUPLEX)) {
+		if (rx_data) {
+			tx_len = (sizeof(op->cmd.opcode) +
+				  op->addr.nbytes + op->dummy.nbytes);
+			rx_len = op->data.nbytes;
+			flag = SPI_XFER_BEGIN;
+			pos2 = 0;

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

+	} else {
+		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++)

Hi Stefan,

Quoted 61 lines · On Mon, 6 Aug 2018 17:12:50 +0200 Stefan Roese <[email protected]> wrote:

Some SPI controller might now support full-duplex SPI transfers. This option can be enabled to use half-duplex operation mode for such SPI controllers.

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

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
  drivers/spi/Kconfig   |  8 ++++++++
  drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
  2 files changed, 36 insertions(+), 3 deletions(-)

drivers/spi/Kconfig +8 -0

index 9fbd26740d..5bd8289284 100644
@@ -25,6 +25,14 @@ config SPI_MEM
  	  This extension is meant to simplify interaction with SPI memories
  	  by providing an high-level interface to send memory-like commands.
 
+config SPI_MEM_HALF_DUPLEX
+	bool "Use half-duplex SPI transfer"
+	depends on SPI_MEM
+	help
+	  Some SPI controller might not support full-duplex SPI transfers.
+	  This option can be enabled to use half-duplex operation mode for
+	  such SPI controllers.
+
  config ALTERA_SPI
  	bool "Altera SPI driver"
  	help

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

index 07ce799170..617fbbfe09 100644
@@ -202,6 +202,10 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
  	struct dm_spi_ops *ops = spi_get_ops(bus);
  	unsigned int xfer_len, pos = 0;
  	u8 *tx_buf, *rx_buf = NULL;
+	unsigned int pos2;
+	int tx_len;
+	int rx_len;
+	u32 flag;
  	int ret;
  	int i;
 

@@ -370,8 +374,29 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
  	if (tx_data)
  		memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);
 
-	ret = spi_xfer(slave, xfer_len * 8, tx_buf, rx_buf,
-		       SPI_XFER_BEGIN | SPI_XFER_END);
+	pos2 = pos;
+	if (CONFIG_IS_ENABLED(SPI_MEM_HALF_DUPLEX)) {
+		if (rx_data) {
+			tx_len = (sizeof(op->cmd.opcode) +
+				  op->addr.nbytes + op->dummy.nbytes);
+			rx_len = op->data.nbytes;
+			flag = SPI_XFER_BEGIN;
+			pos2 = 0;

drivers/spi/spi-mem.c int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) · line 394

Quoted 10 lines

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

+		} else {
+			tx_len = xfer_len;
+			rx_len = 0;
+			flag = SPI_XFER_BEGIN | SPI_XFER_END;
+		}
+		ret = spi_xfer(slave, tx_len * 8, tx_buf, NULL, flag);
+		if (rx_data) {
+			ret = spi_xfer(slave, rx_len * 8, NULL,
+				       rx_buf, SPI_XFER_END);
+		}

Why not doing that all the time and split things in 3 transfers instead of 2:

1/ the opcode + address + dummy cycles
2/ the tx data
3/ the rx data (optional)

This way you should have to allocate the rx/tx buf and you'd get rid of the memcpy.

Of course, that only works if all SPI controllers are capable of dealing with transfers that have only SPI_XFER_BEGIN or SPI_XFER_END set.

Hi Boris,

Quoted 83 lines · Boris Brezillon

Hi Stefan,

Quoted 61 lines · On Mon, 6 Aug 2018 17:12:50 +0200 Stefan Roese <[email protected]> wrote:

Some SPI controller might now support full-duplex SPI transfers. This option can be enabled to use half-duplex operation mode for such SPI controllers.

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

Cc Miquel Raynal <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Jagan Teki <[email protected]>

---
  drivers/spi/Kconfig   |  8 ++++++++
  drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
  2 files changed, 36 insertions(+), 3 deletions(-)

drivers/spi/Kconfig +8 -0

index 9fbd26740d..5bd8289284 100644
@@ -25,6 +25,14 @@ config SPI_MEM
  	  This extension is meant to simplify interaction with SPI memories
  	  by providing an high-level interface to send memory-like commands.
 
+config SPI_MEM_HALF_DUPLEX
+	bool "Use half-duplex SPI transfer"
+	depends on SPI_MEM
+	help
+	  Some SPI controller might not support full-duplex SPI transfers.
+	  This option can be enabled to use half-duplex operation mode for
+	  such SPI controllers.
+
  config ALTERA_SPI
  	bool "Altera SPI driver"
  	help

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

index 07ce799170..617fbbfe09 100644
@@ -202,6 +202,10 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
  	struct dm_spi_ops *ops = spi_get_ops(bus);
  	unsigned int xfer_len, pos = 0;
  	u8 *tx_buf, *rx_buf = NULL;
+	unsigned int pos2;
+	int tx_len;
+	int rx_len;
+	u32 flag;
  	int ret;
  	int i;
 

@@ -370,8 +374,29 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
  	if (tx_data)
  		memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);
 
-	ret = spi_xfer(slave, xfer_len * 8, tx_buf, rx_buf,
-		       SPI_XFER_BEGIN | SPI_XFER_END);
+	pos2 = pos;
+	if (CONFIG_IS_ENABLED(SPI_MEM_HALF_DUPLEX)) {
+		if (rx_data) {
+			tx_len = (sizeof(op->cmd.opcode) +
+				  op->addr.nbytes + op->dummy.nbytes);
+			rx_len = op->data.nbytes;
+			flag = SPI_XFER_BEGIN;
+			pos2 = 0;

drivers/spi/spi-mem.c int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) · line 394

Quoted 10 lines

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

+		} else {
+			tx_len = xfer_len;
+			rx_len = 0;
+			flag = SPI_XFER_BEGIN | SPI_XFER_END;
+		}
+		ret = spi_xfer(slave, tx_len * 8, tx_buf, NULL, flag);
+		if (rx_data) {
+			ret = spi_xfer(slave, rx_len * 8, NULL,
+				       rx_buf, SPI_XFER_END);
+		}  

Why not doing that all the time and split things in 3 transfers instead of 2:

1/ the opcode + address + dummy cycles
2/ the tx data
3/ the rx data (optional)