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

[Boris Brezillon <boris.brezillon@bootlin.com>]

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