Re: [RFC PATCH 1/6] spi: Extend the core to ease integration of SPI memory controllers

[Maxime Chevallier <maxime.chevallier@xxxxxxxx>]

Hi Boris,

> From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> 
> Some controllers are exposing high-level interfaces to access various
> kind of SPI memories. Unfortunately they do not fit in the current
> spi_controller model and usually have drivers placed in
> drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI
> memories in general.
> 
> This is an attempt at defining a SPI memory interface which works for
> all kinds of SPI memories (NORs, NANDs, SRAMs).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/spi/spi.c       | 423
> +++++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/spi/spi.h | 226 ++++++++++++++++++++++++++ 2 files
> changed, 646 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b33a727a0158..57bc540a0521 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2057,6 +2057,24 @@ static int of_spi_register_master(struct
> spi_controller *ctlr) }
>  #endif
>  

[...]

> +int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0;
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	struct spi_transfer xfers[4] = { };
> +	struct spi_message msg;
> +	u8 *tmpbuf;
> +	int ret;
> +
> +	if (!spi_mem_supports_op(mem, op))
> +		return -ENOTSUPP;
> +
> +	if (ctlr->mem_ops) {
> +		if (ctlr->auto_runtime_pm) {
> +			ret = pm_runtime_get_sync(ctlr->dev.parent);
> +			if (ret < 0) {
> +				dev_err(&ctlr->dev,
> +					"Failed to power device:
> %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
> +
> +		mutex_lock(&ctlr->bus_lock_mutex);
> +		mutex_lock(&ctlr->io_mutex);
> +		ret = ctlr->mem_ops->exec_op(mem, op);

As a user, what prevented me from using spi_flash_read is that it
bypasses the message queue. I have a setup that uses spi_async and I
have to make sure everything goes in the right order, so I ended up
using spi_write_then_read instead.

Is there a way to make so that the message that uses exec_op are issued
in the correct order regarding messages that are already queued ?

Maybe we could extend spi_message or spi_transfer to store all
this opcode/dummy/addr information, so that we would use the standard
interfaces spi_sync / spi_async, and make this mechanism of exec_op
transparent from the user ?


> +		mutex_unlock(&ctlr->io_mutex);
> +		mutex_unlock(&ctlr->bus_lock_mutex);
> +
> +		if (ctlr->auto_runtime_pm)
> +			pm_runtime_put(ctlr->dev.parent);
> +
> +		/*
> +		 * Some controllers only optimize specific paths
> (typically the
> +		 * read path) and expect the core to use the regular
> SPI
> +		 * interface in these cases.
> +		 */
> +		if (!ret || ret != -ENOTSUPP)
> +			return ret;
> +	}
> +
> +	tmpbufsize = sizeof(op->cmd.opcode) + op->addr.nbytes +
> +		     op->dummy.nbytes;
> +
> +	/*
> +	 * 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.
> +	 */
> +	tmpbuf = kzalloc(tmpbufsize, GFP_KERNEL | GFP_DMA);
> +	if (!tmpbuf)
> +		return -ENOMEM;
> +
> +	spi_message_init(&msg);
> +
> +	tmpbuf[0] = op->cmd.opcode;
> +	xfers[xferpos].tx_buf = tmpbuf;
> +	xfers[xferpos].len = sizeof(op->cmd.opcode);
> +	xfers[xferpos].tx_nbits = op->cmd.buswidth;
> +	spi_message_add_tail(&xfers[xferpos], &msg);
> +	xferpos++;
> +	totalxferlen++;
> +
> +	if (op->addr.nbytes) {
> +		memcpy(tmpbuf + 1, op->addr.buf, op->addr.nbytes);
> +		xfers[xferpos].tx_buf = tmpbuf + 1;
> +		xfers[xferpos].len = op->addr.nbytes;
> +		xfers[xferpos].tx_nbits = op->addr.buswidth;
> +		spi_message_add_tail(&xfers[xferpos], &msg);
> +		xferpos++;
> +		totalxferlen += op->addr.nbytes;
> +	}
> +
> +	if (op->dummy.nbytes) {
> +		memset(tmpbuf + op->addr.nbytes + 1, 0xff,
> op->dummy.nbytes);
> +		xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
> +		xfers[xferpos].len = op->dummy.nbytes;
> +		xfers[xferpos].tx_nbits = op->dummy.buswidth;
> +		spi_message_add_tail(&xfers[xferpos], &msg);
> +		xferpos++;
> +		totalxferlen += op->dummy.nbytes;
> +	}

Can't we use just one xfer for all the opcode, addr and dummy bytes ?

> +	if (op->data.nbytes) {
> +		if (op->data.dir == SPI_MEM_DATA_IN) {
> +			xfers[xferpos].rx_buf = op->data.buf.in;
> +			xfers[xferpos].rx_nbits = op->data.buswidth;
> +		} else {
> +			xfers[xferpos].tx_buf = op->data.buf.out;
> +			xfers[xferpos].tx_nbits = op->data.buswidth;
> +		}
> +
> +		xfers[xferpos].len = op->data.nbytes;
> +		spi_message_add_tail(&xfers[xferpos], &msg);
> +		xferpos++;
> +		totalxferlen += op->data.nbytes;
> +	}
> +
> +	ret = spi_sync(mem->spi, &msg);
> +
> +	kfree(tmpbuf);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (msg.actual_length != totalxferlen)
> +		return -EIO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_mem_exec_op);

[...]

Thanks,

Maxime

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html