Re: [RFC PATCH 4/6] spi: ti-qspi: Implement the spi_mem interface

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Sun, 11 Feb 2018 16:17:06 +0100
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hi Boris,
> 
> On Tue,  6 Feb 2018 00:21:18 +0100, Boris Brezillon
> <boris.brezillon@xxxxxxxxxxx> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > 
> > The spi_mem interface is meant to replace the spi_flash_read() one.
> > Implement the ->exec_op() method so that we can smoothly get rid of the
> > old interface.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/spi/spi-ti-qspi.c | 85 +++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 72 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> > index c24d9b45a27c..40cac3ef6cc9 100644
> > --- a/drivers/spi/spi-ti-qspi.c
> > +++ b/drivers/spi/spi-ti-qspi.c
> > @@ -434,12 +434,10 @@ static int ti_qspi_dma_xfer(struct ti_qspi *qspi, dma_addr_t dma_dst,
> >  	return 0;
> >  }
> >  
> > -static int ti_qspi_dma_bounce_buffer(struct ti_qspi *qspi,
> > -				     struct spi_flash_read_message *msg)
> > +static int ti_qspi_dma_bounce_buffer(struct ti_qspi *qspi, loff_t offs,
> > +				     void *to, size_t readsize)
> >  {
> > -	size_t readsize = msg->len;
> > -	void *to = msg->buf;
> > -	dma_addr_t dma_src = qspi->mmap_phys_base + msg->from;
> > +	dma_addr_t dma_src = qspi->mmap_phys_base + offs;
> >  	int ret = 0;
> >  
> >  	/*
> > @@ -507,13 +505,14 @@ static void ti_qspi_disable_memory_map(struct spi_device *spi)
> >  	qspi->mmap_enabled = false;
> >  }
> >  
> > -static void ti_qspi_setup_mmap_read(struct spi_device *spi,
> > -				    struct spi_flash_read_message *msg)
> > +static void ti_qspi_setup_mmap_read(struct spi_device *spi, u8 opcode,
> > +				    u8 data_nbits, u8 addr_width,
> > +				    u8 dummy_bytes)
> >  {
> >  	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
> > -	u32 memval = msg->read_opcode;
> > +	u32 memval = opcode;
> >  
> > -	switch (msg->data_nbits) {
> > +	switch (data_nbits) {
> >  	case SPI_NBITS_QUAD:
> >  		memval |= QSPI_SETUP_RD_QUAD;
> >  		break;
> > @@ -524,8 +523,8 @@ static void ti_qspi_setup_mmap_read(struct spi_device *spi,
> >  		memval |= QSPI_SETUP_RD_NORMAL;
> >  		break;
> >  	}
> > -	memval |= ((msg->addr_width - 1) << QSPI_SETUP_ADDR_SHIFT |
> > -		   msg->dummy_bytes << QSPI_SETUP_DUMMY_SHIFT);
> > +	memval |= ((addr_width - 1) << QSPI_SETUP_ADDR_SHIFT |
> > +		   dummy_bytes << QSPI_SETUP_DUMMY_SHIFT);
> >  	ti_qspi_write(qspi, memval,
> >  		      QSPI_SPI_SETUP_REG(spi->chip_select));
> >  }
> > @@ -546,13 +545,15 @@ static int ti_qspi_spi_flash_read(struct spi_device *spi,
> >  
> >  	if (!qspi->mmap_enabled)
> >  		ti_qspi_enable_memory_map(spi);
> > -	ti_qspi_setup_mmap_read(spi, msg);
> > +	ti_qspi_setup_mmap_read(spi, msg->read_opcode, msg->data_nbits,
> > +				msg->addr_width, msg->dummy_bytes);
> >  
> >  	if (qspi->rx_chan) {
> >  		if (msg->cur_msg_mapped)
> >  			ret = ti_qspi_dma_xfer_sg(qspi, msg->rx_sg, msg->from);
> >  		else
> > -			ret = ti_qspi_dma_bounce_buffer(qspi, msg);
> > +			ret = ti_qspi_dma_bounce_buffer(qspi, msg->from,
> > +							msg->buf, msg->len);
> >  		if (ret)
> >  			goto err_unlock;
> >  	} else {
> > @@ -566,6 +567,62 @@ static int ti_qspi_spi_flash_read(struct spi_device *spi,
> >  	return ret;
> >  }
> >  
> > +static int ti_qspi_exec_mem_op(struct spi_mem *mem,
> > +			       const struct spi_mem_op *op)
> > +{
> > +	struct ti_qspi *qspi = spi_master_get_devdata(mem->spi->master);
> > +	int i, ret = 0;
> > +	u32 from = 0;
> > +
> > +	/* Only optimize read path. */
> > +	if (!op->data.nbytes || op->data.dir != SPI_MEM_DATA_IN ||
> > +	    !op->addr.nbytes || op->addr.nbytes > 4)
> > +		return -ENOTSUPP;
> > +
> > +	for (i = 0; i < op->addr.nbytes; i++) {
> > +		from <<= 8;
> > +		from |= op->addr.buf[i];
> > +	}
> > +
> > +	/* Address exceeds MMIO window size, fall back to regular mode. */  
> 
> I don't understand how do you fall back to regular mode?

If you look at spi_mem_exec_op() you'll see that if the controller
->exec_op() returns -ENOTSUPP, the function will try to execute the
operation using the regular spi_sync() API. I'll try to make it clearer
in my next iteration.

> Moreover if the
> purpose of adding this function is to remove spi_flash_read().

Sorry, I don't get that one. Yes, the spi_mem_ops interface is here to
replace the ->spi_flash_xx() one, and that's exactly what I'm doing
here: porting the existing implementation to the new interface, keeping
the exact same limitations (only read path is optimized, and the request
has to fall in the iomem range mapped by the driver).

> 
> > +	if (from > 0x4000000)

Oops! Looks like I forgot to update the code to store the qspi_mmap
resource size in ti_qspi struct and use it here to detect whether
mmap access is allowed or not. Will fix that in v2.

> > +		return -ENOTSUPP;
> > +
> > +	mutex_lock(&qspi->list_lock);
> > +
> > +	if (!qspi->mmap_enabled)
> > +		ti_qspi_enable_memory_map(mem->spi);
> > +	ti_qspi_setup_mmap_read(mem->spi, op->cmd.opcode, op->data.buswidth,
> > +				op->addr.nbytes, op->dummy.nbytes);
> > +
> > +	if (qspi->rx_chan) {
> > +		struct sg_table sgt;
> > +
> > +		if (!virt_addr_valid(op->data.buf.in) &&
> > +		    !spi_controller_dma_map_mem_op_data(mem->spi->master, op,
> > +							&sgt)) {
> > +			ret = ti_qspi_dma_xfer_sg(qspi, sgt, from);
> > +			spi_controller_dma_unmap_mem_op_data(mem->spi->master,
> > +							     op, &sgt);
> > +		} else {
> > +			ret = ti_qspi_dma_bounce_buffer(qspi, from,
> > +							op->data.buf.in,
> > +							op->data.nbytes);
> > +		}
> > +	} else {
> > +		memcpy_fromio(op->data.buf.in, qspi->mmap_base + from,
> > +			      op->data.nbytes);
> > +	}
> > +
> > +	mutex_unlock(&qspi->list_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> > +	.exec_op = ti_qspi_exec_mem_op,
> > +};
> > +
> >  static int ti_qspi_start_transfer_one(struct spi_master *master,
> >  		struct spi_message *m)
> >  {
> > @@ -673,6 +730,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
> >  	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
> >  				     SPI_BPW_MASK(8);
> >  	master->spi_flash_read = ti_qspi_spi_flash_read;
> > +	master->mem_ops = &ti_qspi_mem_ops;
> >  
> >  	if (!of_property_read_u32(np, "num-cs", &num_cs))
> >  		master->num_chipselect = num_cs;
> > @@ -785,6 +843,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
> >  				 PTR_ERR(qspi->mmap_base));
> >  			qspi->mmap_base = NULL;
> >  			master->spi_flash_read = NULL;
> > +			master->mem_ops = NULL;
> >  		}
> >  	}
> >  	qspi->mmap_enabled = false;  
> 
> 
> 



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
--
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