From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Tue, 7 Aug 2018 15:28:02 +0200 Subject: [U-Boot] [PATCH 1/4 v2] spi: spi-mem: Use 2 SPI messages instead of a single full-duplex one In-Reply-To: <20180807121655.22346-1-sr@denx.de> References: <20180807121655.22346-1-sr@denx.de> Message-ID: <20180807152802.0a3d1332@bbrezillon> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese 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 > Suggested-by: Boris Brezillon > Cc: Miquel Raynal > Cc: Boris Brezillon > Cc: Jagan Teki Looks good overall, just a few comments (that you might chose to ignore if you disagree). Reviewed-by: Boris Brezillon > --- > 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 > #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]); > 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;