[U-Boot] [PATCH 1/4 v2] spi: spi-mem: Use 2 SPI messages instead of a single full-duplex one
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 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 --- 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(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 07ce799170..84e33aa979 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -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 @@ -200,8 +202,12 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) 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; + u8 *rx_buf; + u8 op_buf[OP_BUFFER_SIZE_MAX]; + int op_len; + u32 flag; int ret; int i;
@@ -330,67 +336,65 @@ 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; + tx_buf = op->data.buf.out; + rx_buf = op->data.buf.in;
- /* - * 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; + op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes; + if (op_len > OP_BUFFER_SIZE_MAX) { + printf("Length for cmd+addr+dummy too big (%d)\n", op_len); + return -EIO; } + 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) { - 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_data && !rx_data) + flag |= SPI_XFER_END; + ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag); + + if (tx_data) { + /* 2nd transfer a: tx data path */ + ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf, NULL, + SPI_XFER_END); }
- if (tx_data) - memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes); + if (rx_data) { + /* 2nd transfer b: rx data path */ + ret = spi_xfer(slave, op->data.nbytes * 8, NULL, rx_buf, + SPI_XFER_END); + }
- 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;
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 --- 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(+)
diff --git a/cmd/mtd.c b/cmd/mtd.c index 221b12500f..999d686e66 100644 --- a/cmd/mtd.c +++ b/cmd/mtd.c @@ -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 { diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c index 0b793695cc..c9d7a646f6 100644 --- a/drivers/mtd/nand/core.c +++ b/drivers/mtd/nand/core.c @@ -163,6 +163,12 @@ int nanddev_mtd_erase(struct mtd_info *mtd, struct erase_info *einfo) while (nanddev_pos_cmp(&pos, &last) <= 0) { ret = nanddev_erase(nand, &pos); if (ret) { + /* U-Boot special: Allow the users to skip bad blocks */ + if ((ret == -EIO) && einfo->skipbad) { + nanddev_pos_next_eraseblock(nand, &pos); + continue; + } + einfo->fail_addr = nanddev_pos_to_offs(nand, &pos);
return ret; diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 4ca5d764d7..3a09dbab95 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -53,6 +53,7 @@ struct erase_info { u_char state; struct erase_info *next; int scrub; + bool skipbad; };
struct mtd_erase_region_info {
On Tue, 7 Aug 2018 14:16:53 +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.
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
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(+)
diff --git a/cmd/mtd.c b/cmd/mtd.c index 221b12500f..999d686e66 100644 --- a/cmd/mtd.c +++ b/cmd/mtd.c @@ -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 {
diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c index 0b793695cc..c9d7a646f6 100644 --- a/drivers/mtd/nand/core.c +++ b/drivers/mtd/nand/core.c @@ -163,6 +163,12 @@ int nanddev_mtd_erase(struct mtd_info *mtd, struct erase_info *einfo) while (nanddev_pos_cmp(&pos, &last) <= 0) { ret = nanddev_erase(nand, &pos); if (ret) {
/* U-Boot special: Allow the users to skip bad blocks */if ((ret == -EIO) && einfo->skipbad) {nanddev_pos_next_eraseblock(nand, &pos);continue;}
If you choose this approach you'll have to patch rawnand and onenand code to do the same (maybe others too), because we want mtd erase to work the same way on all MTD devices.
To be honest, I'd prefer to see that handled in cmd/mtd.c. If you don't like the idea of erasing one block at a time, you can always check the ->fail_addr info when mtd_erase() returns an error, and start over from ->fail_addr + mtd->erasesize.
einfo->fail_addr = nanddev_pos_to_offs(nand, &pos); return ret;diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 4ca5d764d7..3a09dbab95 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -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 sr@denx.de Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Boris Brezillon boris.brezillon@bootlin.com Cc: Jagan Teki jagan@openedev.com --- 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 999d686e66..b29aec18ca 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; @@ -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[] =
On Tue, 7 Aug 2018 14:16:54 +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
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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 999d686e66..b29aec18ca 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;
@@ -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 sr@denx.de Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Boris Brezillon boris.brezillon@bootlin.com Cc: Jagan Teki jagan@openedev.com --- 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 b29aec18ca..7dc77edf28 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;
Hi Stefan,
On Tue, 7 Aug 2018 14:16:55 +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 (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
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 b29aec18ca..7dc77edf28 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;
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.
Regards,
Boris
Hi Boris,
On 07.08.2018 15:32, Boris Brezillon wrote:
Hi Stefan,
On Tue, 7 Aug 2018 14:16:55 +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 (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
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 b29aec18ca..7dc77edf28 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;
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.
Squashing them would be just fine with me. I won't find much time today to rework the remaining patches but will do so most likely tomorrow. Please stay tuned and many thanks for the really helpful reviews.
Thanks, Stefan
On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de 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.
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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 07ce799170..84e33aa979 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -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
@@ -200,8 +202,12 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) 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;
- u8 *rx_buf;
- u8 op_buf[OP_BUFFER_SIZE_MAX];
u8 op_buf[OP_BUFFER_SIZE_MAX] = { };
and you can get rid of the memset(0) in the code.
- int op_len;
- u32 flag; int ret; int i;
@@ -330,67 +336,65 @@ 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;
- tx_buf = op->data.buf.out;
- rx_buf = op->data.buf.in;
I think you can get rid of rx/tx_data and just keep rx/tx_buf. Initialize them to NULL at declaration time and then do:
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; }
- /*
* 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;
- op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
- if (op_len > OP_BUFFER_SIZE_MAX) {
printf("Length for cmd+addr+dummy too big (%d)\n", op_len);return -EIO;
While I agree we shouldn't exceed the 16 bytes for cmd, addr and dummy cycles, I'm not a big fan of those hardcoded limitations that needs to be adjusted every time we realize the initial choice was too restrictive.
Do you have a good reason for avoiding kzalloc() here (perfs, dynamic allocation not available in some cases?)?
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) {
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_data && !rx_data)
flag |= SPI_XFER_END;
I'd add a blank line here.
- ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
You should check ret here.
- if (tx_data) {
/* 2nd transfer a: tx data path */ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf, NULL,SPI_XFER_END);
and here
}
- if (tx_data)
memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);
- if (rx_data) {
/* 2nd transfer b: rx data path */ret = spi_xfer(slave, op->data.nbytes * 8, NULL, rx_buf,SPI_XFER_END);
and here
- }
If you've initialized rx/tx_buf as suggested above, you can do:
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]);
- 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;
Hi Stefan, Jagan,
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de 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.
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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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?
Thanks, Miquèl
Hi Miquel,
On 08.08.2018 10:56, Miquel Raynal wrote:
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de 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.
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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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?
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?
Thanks, Stefan
Hi Stefan,
Stefan Roese sr@denx.de wrote on Thu, 9 Aug 2018 07:24:14 +0200:
Hi Miquel,
On 08.08.2018 10:56, Miquel Raynal wrote:
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200
Stefan Roese sr@denx.de 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.
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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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?
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?
Thanks, Miquèl
Hi Miquel,
On 09.08.2018 10:13, Miquel Raynal wrote:
Stefan Roese sr@denx.de wrote on Thu, 9 Aug 2018 07:24:14 +0200:
Hi Miquel,
On 08.08.2018 10:56, Miquel Raynal wrote:
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200
Stefan Roese sr@denx.de 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.
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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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?
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?
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.
Thanks, Stefan
Hi Stefan,
Stefan Roese sr@denx.de wrote on Thu, 9 Aug 2018 11:10:15 +0200:
Hi Miquel,
On 09.08.2018 10:13, Miquel Raynal wrote:
Stefan Roese sr@denx.de wrote on Thu, 9 Aug 2018 07:24:14 +0200:
Hi Miquel,
On 08.08.2018 10:56, Miquel Raynal wrote:
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200
Stefan Roese sr@denx.de 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.
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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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?
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?
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.
Thanks for your help, Miquèl
Hi Miquel,
On 09.08.2018 16:50, Miquel Raynal wrote:
Stefan Roese sr@denx.de wrote on Thu, 9 Aug 2018 11:10:15 +0200:
Hi Miquel,
On 09.08.2018 10:13, Miquel Raynal wrote:
Stefan Roese sr@denx.de wrote on Thu, 9 Aug 2018 07:24:14 +0200:
Hi Miquel,
On 08.08.2018 10:56, Miquel Raynal wrote:
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
> On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de 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. > > 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 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
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
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?
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?
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.
I see. I was not aware, that the fixes from Boris have not been squashed yet. Then re-sending this whole series is probably the best solution.
As we missed the merge window, maybe I'll also take the time to rework mtdparts. I plan to do that next week.
That would be great.
Thanks, Stefan
Hi Stefan,
Minor typo if you decide to send a v3.
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de 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 ?
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 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
Thanks, Miquèl
Hi Miquel,
On 08.08.2018 11:58, Miquel Raynal wrote:
Hi Stefan,
Minor typo if you decide to send a v3.
Boris Brezillon boris.brezillon@bootlin.com wrote on Tue, 7 Aug 2018 15:28:02 +0200:
On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de 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 ?
Thanks. I'll be working an v3 today.
Thanks, Stefan
participants (3)
-
Boris Brezillon -
Miquel Raynal -
Stefan Roese