[U-Boot] [PATCH 1/4] spi: spi-mem: Add optional half-duplex SPI transfer mode

[U-Boot] [PATCH 1/4] spi: spi-mem: Add optional half-duplex SPI transfer mode

[Stefan Roese]

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 <sr@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
---
 drivers/spi/Kconfig   |  8 ++++++++
 drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9fbd26740d..5bd8289284 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -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
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 07ce799170..617fbbfe09 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -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;
+		} 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);
+		}
+	} 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++)
@@ -387,7 +412,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		return ret;
 
 	if (rx_data)
-		memcpy(op->data.buf.in, rx_buf + pos, op->data.nbytes);
+		memcpy(op->data.buf.in, rx_buf + pos2, op->data.nbytes);
 
 	kfree(tx_buf);
 	kfree(rx_buf);
-- 
2.18.0

[U-Boot] [PATCH 2/4] mtd: nand: 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>
---
 drivers/mtd/nand/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
index 0b793695cc..888f765b90 100644
--- a/drivers/mtd/nand/core.c
+++ b/drivers/mtd/nand/core.c
@@ -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;
-- 
2.18.0

[U-Boot] [PATCH 3/4] 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>
---
 cmd/mtd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 221b12500f..38a89736cf 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 EINVAL;
 	}
 
 	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 EINVAL;
 		}
 
 		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 EINVAL;
 		}
 
 		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 ENOMEM;
 		}
 
 		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 -ret;
 		}
 
 		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 EINVAL;
 		}
 
 		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 EINVAL;
 		}
 
 		erase_op.mtd = mtd;
-- 
2.18.0

[U-Boot] [PATCH 4/4] 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 (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 <sr@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Jagan Teki <jagan@openedev.com>
---
 cmd/mtd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 38a89736cf..6d27698d1e 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 EINVAL;
 		}
 
+		printf("Erasing 0x%08llx ... 0x%08llx (%d page(s))\n",
+		       off, off + len - 1, mtd_len_to_pages(mtd, len));
+
 		erase_op.mtd = mtd;
 		erase_op.addr = off;
 		erase_op.len = len;
-- 
2.18.0

[U-Boot] [PATCH 1/4] spi: spi-mem: Add optional half-duplex SPI transfer mode

[Boris Brezillon]

Hi Stefan,

On Mon,  6 Aug 2018 17:12:50 +0200
Stefan Roese <sr@denx.de> 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 <sr@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> Cc: Jagan Teki <jagan@openedev.com>
> ---
>  drivers/spi/Kconfig   |  8 ++++++++
>  drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 9fbd26740d..5bd8289284 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -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
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 07ce799170..617fbbfe09 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -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;
> +		} 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.

> +	} 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++)
> @@ -387,7 +412,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>  		return ret;
>  
>  	if (rx_data)
> -		memcpy(op->data.buf.in, rx_buf + pos, op->data.nbytes);
> +		memcpy(op->data.buf.in, rx_buf + pos2, op->data.nbytes);
>  
>  	kfree(tx_buf);
>  	kfree(rx_buf);

Regards,

Boris

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

[Boris Brezillon]

Hi Stefan,

On Mon,  6 Aug 2018 17:12:51 +0200
Stefan Roese <sr@denx.de> wrote:

> 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).

Regards,

Boris

[1]https://elixir.bootlin.com/u-boot/v2018.09-rc1/source/drivers/mtd/nand/nand_base.c#L2911

> 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>
> ---
>  drivers/mtd/nand/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> index 0b793695cc..888f765b90 100644
> --- a/drivers/mtd/nand/core.c
> +++ b/drivers/mtd/nand/core.c
> @@ -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;

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

[Boris Brezillon]

On Mon,  6 Aug 2018 17:12:52 +0200
Stefan Roese <sr@denx.de> 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 <sr@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> Cc: Jagan Teki <jagan@openedev.com>
> ---
>  cmd/mtd.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index 221b12500f..38a89736cf 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 EINVAL;

How about using CMD_RET_FAILURE for all errors?

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

[Boris Brezillon]

On Mon,  6 Aug 2018 17:12:53 +0200
Stefan Roese <sr@denx.de> 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 <sr@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> Cc: Jagan Teki <jagan@openedev.com>
> ---
>  cmd/mtd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index 38a89736cf..6d27698d1e 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 EINVAL;
>  		}
>  
> +		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).

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

[U-Boot] [PATCH 1/4] spi: spi-mem: Add optional half-duplex SPI transfer mode

[Stefan Roese]

Hi Boris,

On 06.08.2018 21:53, Boris Brezillon wrote:
> Hi Stefan,
> 
> On Mon,  6 Aug 2018 17:12:50 +0200
> Stefan Roese <sr@denx.de> 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 <sr@denx.de>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>> Cc: Jagan Teki <jagan@openedev.com>
>> ---
>>   drivers/spi/Kconfig   |  8 ++++++++
>>   drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
>>   2 files changed, 36 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 9fbd26740d..5bd8289284 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -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
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>> index 07ce799170..617fbbfe09 100644
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -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;
>> +		} 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.

I also thought about this but was not "brave" enough to drop the
current single xfer. If you and Miquel think this is good idea, then
I will gladly work on such an implementation. Getting rid of this
compile time option would be good.

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

I'm pretty sure that we can assume this is the case. Otherwise such a
driver should be easy to fix to support this type of xfer.

Thanks,
Stefan

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

[Stefan Roese]

Hi Boris,

On 06.08.2018 22:38, Boris Brezillon wrote:
> On Mon,  6 Aug 2018 17:12:52 +0200
> Stefan Roese <sr@denx.de> 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 <sr@denx.de>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>> Cc: Jagan Teki <jagan@openedev.com>
>> ---
>>   cmd/mtd.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/mtd.c b/cmd/mtd.c
>> index 221b12500f..38a89736cf 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 EINVAL;
> 
> How about using CMD_RET_FAILURE for all errors?

Good idea, thanks. Will change in v2.

Thanks,
Stefan

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

[Stefan Roese]

Hi Boris,

On 06.08.2018 22:41, Boris Brezillon wrote:
> On Mon,  6 Aug 2018 17:12:53 +0200
> Stefan Roese <sr@denx.de> 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 <sr@denx.de>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>> Cc: Jagan Teki <jagan@openedev.com>
>> ---
>>   cmd/mtd.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/cmd/mtd.c b/cmd/mtd.c
>> index 38a89736cf..6d27698d1e 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 EINVAL;
>>   		}
>>   
>> +		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).

Will do for v2.

Thanks,
Stefan

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

[Stefan Roese]

Hi Boris,

On 06.08.2018 22:06, Boris Brezillon wrote:
> Hi Stefan,
> 
> On Mon,  6 Aug 2018 17:12:51 +0200
> Stefan Roese <sr@denx.de> wrote:
> 
>> 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 see. AFAIR, the default behavior in U-Boot when erasing NAND chips
is that bad blocks are skipped though. Thats why I came up with this
idea.

> 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).

Thanks for the suggestions.

I'll double-check all the new "mtd" commands in respect to handling bad
blocks again later this week and will try to come up with different
approach supporting erase, read and write in the same way.

Thanks,
Stefan

[U-Boot] [PATCH 1/4] spi: spi-mem: Add optional half-duplex SPI transfer mode

[Boris Brezillon]

On Tue, 7 Aug 2018 07:06:58 +0200
Stefan Roese <sr@denx.de> wrote:

> Hi Boris,
> 
> On 06.08.2018 21:53, Boris Brezillon wrote:
> > Hi Stefan,
> > 
> > On Mon,  6 Aug 2018 17:12:50 +0200
> > Stefan Roese <sr@denx.de> 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 <sr@denx.de>
> >> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> >> Cc: Jagan Teki <jagan@openedev.com>
> >> ---
> >>   drivers/spi/Kconfig   |  8 ++++++++
> >>   drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++---
> >>   2 files changed, 36 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> >> index 9fbd26740d..5bd8289284 100644
> >> --- a/drivers/spi/Kconfig
> >> +++ b/drivers/spi/Kconfig
> >> @@ -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
> >> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> >> index 07ce799170..617fbbfe09 100644
> >> --- a/drivers/spi/spi-mem.c
> >> +++ b/drivers/spi/spi-mem.c
> >> @@ -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;
> >> +		} 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)

Actually it's:

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

but I guess you figured this out already.