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

Commit Message

Boris Brezillon Feb. 5, 2018, 11:21 p.m. UTC

From: Boris Brezillon <[e-mail]>

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 <[e-mail]>
---
 drivers/spi/spi-ti-qspi.c | 85 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 72 insertions(+), 13 deletions(-)

Comments

Miquel Raynal Feb. 11, 2018, 3:17 p.m. UTC | #1

Hi Boris,

On Tue,  6 Feb 2018 00:21:18 +0100, Boris Brezillon
<[e-mail]> wrote:

> From: Boris Brezillon <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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? Moreover if the
purpose of adding this function is to remove spi_flash_read().

> +	if (from > 0x4000000)
> +		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 Feb. 11, 2018, 5:18 p.m. UTC | #2

On Sun, 11 Feb 2018 16:17:06 +0100
Miquel Raynal <[e-mail]> wrote:

> Hi Boris,
> 
> On Tue,  6 Feb 2018 00:21:18 +0100, Boris Brezillon
> <[e-mail]> wrote:
> 
> > From: Boris Brezillon <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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;  
> 
> 
>

Miquel Raynal Feb. 12, 2018, 7:54 a.m. UTC | #3

Hi Boris,


> > > @@ -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.

Ok, I mixed the functions in my head and this answers the below
question as well.

> 
> > 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).
> 

Thanks,
Miquèl

Vignesh Raghavendra Feb. 12, 2018, 11:43 a.m. UTC | #4

On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
> From: Boris Brezillon <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

[...]

> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> +	.exec_op = ti_qspi_exec_mem_op,

       .supports_op = ti_qspi_supports_mem_op,

Its required as per spi_controller_check_ops() in Patch 1/6

> +};
> +
>  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 Feb. 12, 2018, 12:31 p.m. UTC | #5

On Mon, 12 Feb 2018 17:13:55 +0530
Vignesh R <[e-mail]> wrote:

> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
> > From: Boris Brezillon <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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  
> 
> [...]
> 
> > +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> > +	.exec_op = ti_qspi_exec_mem_op,  
> 
>        .supports_op = ti_qspi_supports_mem_op,
> 
> Its required as per spi_controller_check_ops() in Patch 1/6

->supports_op() is optional, and if it's missing, the core will do the
regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
implementation). This being said, if you think a custom ->supports_op()
implementation is needed for this controller I can add one.

> 
> > +};
> > +
> >  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;
> >   
>

Vignesh Raghavendra Feb. 12, 2018, 4 p.m. UTC | #6

On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
> On Mon, 12 Feb 2018 17:13:55 +0530
> Vignesh R <[e-mail]> wrote:
> 
>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
>> > From: Boris Brezillon <[e-mail]>
>> > 
>> > 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 <[e-mail]>
>> > ---
>> >  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  
>> 
>> [...]
>> 
>> > +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>> > +   .exec_op = ti_qspi_exec_mem_op,  
>> 
>>        .supports_op = ti_qspi_supports_mem_op,
>> 
>> Its required as per spi_controller_check_ops() in Patch 1/6
> 
> ->supports_op() is optional, and if it's missing, the core will do the
> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> implementation). 

You might have overlooked spi_controller_check_ops() from Patch 1/6:
+static int spi_controller_check_ops(struct spi_controller *ctlr)
+{
+	/*
+	 * The controller can implement only the high-level SPI-memory
+	 * operations if it does not support regular SPI transfers.
+	 */
+	if (ctlr->mem_ops) {
+		if (!ctlr->mem_ops->supports_op ||
+		    !ctlr->mem_ops->exec_op)
+			return -EINVAL;
+	} else if (!ctlr->transfer && !ctlr->transfer_one &&
+		   !ctlr->transfer_one_message) {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+

So if ->supports_op() is not populated by SPI controller driver, then
driver probe fails with -EINVAL. This is what I observed on my TI
hardware when testing this patch series.

> This being said, if you think a custom ->supports_op()
> implementation is needed for this controller I can add one.
> 

spi_mem_supports_op() should suffice for now if above issue is fixed.

Regards
Vignesh

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

Boris Brezillon Feb. 12, 2018, 4:08 p.m. UTC | #7

On Mon, 12 Feb 2018 21:30:09 +0530
Vignesh R <[e-mail]> wrote:

> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
> > On Mon, 12 Feb 2018 17:13:55 +0530
> > Vignesh R <[e-mail]> wrote:
> >   
> >> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:  
> >> > From: Boris Brezillon <[e-mail]>
> >> > 
> >> > 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 <[e-mail]>
> >> > ---
> >> >  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    
> >> 
> >> [...]
> >>   
> >> > +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >> > +   .exec_op = ti_qspi_exec_mem_op,    
> >> 
> >>        .supports_op = ti_qspi_supports_mem_op,
> >> 
> >> Its required as per spi_controller_check_ops() in Patch 1/6  
> >   
> > ->supports_op() is optional, and if it's missing, the core will do the  
> > regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> > implementation).   
> 
> You might have overlooked spi_controller_check_ops() from Patch 1/6:
> +static int spi_controller_check_ops(struct spi_controller *ctlr)
> +{
> +	/*
> +	 * The controller can implement only the high-level SPI-memory
> +	 * operations if it does not support regular SPI transfers.
> +	 */
> +	if (ctlr->mem_ops) {
> +		if (!ctlr->mem_ops->supports_op ||
> +		    !ctlr->mem_ops->exec_op)
> +			return -EINVAL;
> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
> +		   !ctlr->transfer_one_message) {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> 
> So if ->supports_op() is not populated by SPI controller driver, then
> driver probe fails with -EINVAL. This is what I observed on my TI
> hardware when testing this patch series.

Correct. Then I should fix spi_controller_check_ops() to allow empty
ctlr->mem_ops->supports_op.

> 
> > This being said, if you think a custom ->supports_op()
> > implementation is needed for this controller I can add one.
> >   
> 
> spi_mem_supports_op() should suffice for now if above issue is fixed.

Cool. IIUC, you tested the series on a TI SoC. Does it work as
expected? Do you see any perf regressions?

Regards,

Boris

Vignesh Raghavendra Feb. 14, 2018, 4:25 p.m. UTC | #8

On 12-Feb-18 9:38 PM, Boris Brezillon wrote:
> On Mon, 12 Feb 2018 21:30:09 +0530
> Vignesh R <[e-mail]> wrote:
> 
>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
>>> On Mon, 12 Feb 2018 17:13:55 +0530
>>> Vignesh R <[e-mail]> wrote:
>>>   
>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:  
>>>>> From: Boris Brezillon <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> ---
>>>>>   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    
>>>>
>>>> [...]
>>>>   
>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>>>>> +   .exec_op = ti_qspi_exec_mem_op,    
>>>>
>>>>         .supports_op = ti_qspi_supports_mem_op,
>>>>
>>>> Its required as per spi_controller_check_ops() in Patch 1/6  
>>>   
>>> ->supports_op() is optional, and if it's missing, the core will do the  
>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
>>> implementation).   
>>
>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
>> +{
>> +	/*
>> +	 * The controller can implement only the high-level SPI-memory
>> +	 * operations if it does not support regular SPI transfers.
>> +	 */
>> +	if (ctlr->mem_ops) {
>> +		if (!ctlr->mem_ops->supports_op ||
>> +		    !ctlr->mem_ops->exec_op)
>> +			return -EINVAL;
>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
>> +		   !ctlr->transfer_one_message) {
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>
>> So if ->supports_op() is not populated by SPI controller driver, then
>> driver probe fails with -EINVAL. This is what I observed on my TI
>> hardware when testing this patch series.
> 
> Correct. Then I should fix spi_controller_check_ops() to allow empty
> ctlr->mem_ops->supports_op.
> 
>>
>>> This being said, if you think a custom ->supports_op()
>>> implementation is needed for this controller I can add one.
>>>   
>>
>> spi_mem_supports_op() should suffice for now if above issue is fixed.
> 
> Cool. IIUC, you tested the series on a TI SoC. Does it work as
> expected? Do you see any perf regressions?
> 

I am planning to collect throughput numbers with this series for TI
QSPI. I don't think there would be noticeable degradation.
But, it would be interesting to test for a driver thats now under
drivers/mtd/spi-nor moved to drivers/spi and see if added overhead of
m25p80 layer + spi core has any impact.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Boris Brezillon Feb. 14, 2018, 7:09 p.m. UTC | #9

On Wed, 14 Feb 2018 21:55:10 +0530
Vignesh R <[e-mail]> wrote:

> On 12-Feb-18 9:38 PM, Boris Brezillon wrote:
> > On Mon, 12 Feb 2018 21:30:09 +0530
> > Vignesh R <[e-mail]> wrote:
> >   
> >> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:  
> >>> On Mon, 12 Feb 2018 17:13:55 +0530
> >>> Vignesh R <[e-mail]> wrote:
> >>>     
> >>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:    
> >>>>> From: Boris Brezillon <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>> ---
> >>>>>   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      
> >>>>
> >>>> [...]
> >>>>     
> >>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >>>>> +   .exec_op = ti_qspi_exec_mem_op,      
> >>>>
> >>>>         .supports_op = ti_qspi_supports_mem_op,
> >>>>
> >>>> Its required as per spi_controller_check_ops() in Patch 1/6    
> >>>     
> >>> ->supports_op() is optional, and if it's missing, the core will do the    
> >>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> >>> implementation).     
> >>
> >> You might have overlooked spi_controller_check_ops() from Patch 1/6:
> >> +static int spi_controller_check_ops(struct spi_controller *ctlr)
> >> +{
> >> +	/*
> >> +	 * The controller can implement only the high-level SPI-memory
> >> +	 * operations if it does not support regular SPI transfers.
> >> +	 */
> >> +	if (ctlr->mem_ops) {
> >> +		if (!ctlr->mem_ops->supports_op ||
> >> +		    !ctlr->mem_ops->exec_op)
> >> +			return -EINVAL;
> >> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
> >> +		   !ctlr->transfer_one_message) {
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>
> >> So if ->supports_op() is not populated by SPI controller driver, then
> >> driver probe fails with -EINVAL. This is what I observed on my TI
> >> hardware when testing this patch series.  
> > 
> > Correct. Then I should fix spi_controller_check_ops() to allow empty
> > ctlr->mem_ops->supports_op.
> >   
> >>  
> >>> This being said, if you think a custom ->supports_op()
> >>> implementation is needed for this controller I can add one.
> >>>     
> >>
> >> spi_mem_supports_op() should suffice for now if above issue is fixed.  
> > 
> > Cool. IIUC, you tested the series on a TI SoC. Does it work as
> > expected? Do you see any perf regressions?
> >   
> 
> I am planning to collect throughput numbers with this series for TI
> QSPI. I don't think there would be noticeable degradation.

Ok.

> But, it would be interesting to test for a driver thats now under
> drivers/mtd/spi-nor moved to drivers/spi and see if added overhead of
> m25p80 layer + spi core has any impact.

I'm working with Frieder on the fsl-qspi rework, so we should have
numbers soon.

Frieder Schrempf Feb. 14, 2018, 8:44 p.m. UTC | #10

Hi Boris, hi Vignesh,

On 14.02.2018 20:09, Boris Brezillon wrote:
> On Wed, 14 Feb 2018 21:55:10 +0530
> Vignesh R <[e-mail]> wrote:
> 
>> On 12-Feb-18 9:38 PM, Boris Brezillon wrote:
>>> On Mon, 12 Feb 2018 21:30:09 +0530
>>> Vignesh R <[e-mail]> wrote:
>>>    
>>>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
>>>>> On Mon, 12 Feb 2018 17:13:55 +0530
>>>>> Vignesh R <[e-mail]> wrote:
>>>>>      
>>>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
>>>>>>> From: Boris Brezillon <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>> ---
>>>>>>>    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
>>>>>>
>>>>>> [...]
>>>>>>      
>>>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>>>>>>> +   .exec_op = ti_qspi_exec_mem_op,
>>>>>>
>>>>>>          .supports_op = ti_qspi_supports_mem_op,
>>>>>>
>>>>>> Its required as per spi_controller_check_ops() in Patch 1/6
>>>>>      
>>>>> ->supports_op() is optional, and if it's missing, the core will do the
>>>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
>>>>> implementation).
>>>>
>>>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
>>>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
>>>> +{
>>>> +	/*
>>>> +	 * The controller can implement only the high-level SPI-memory
>>>> +	 * operations if it does not support regular SPI transfers.
>>>> +	 */
>>>> +	if (ctlr->mem_ops) {
>>>> +		if (!ctlr->mem_ops->supports_op ||
>>>> +		    !ctlr->mem_ops->exec_op)
>>>> +			return -EINVAL;
>>>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
>>>> +		   !ctlr->transfer_one_message) {
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>
>>>> So if ->supports_op() is not populated by SPI controller driver, then
>>>> driver probe fails with -EINVAL. This is what I observed on my TI
>>>> hardware when testing this patch series.
>>>
>>> Correct. Then I should fix spi_controller_check_ops() to allow empty
>>> ctlr->mem_ops->supports_op.
>>>    
>>>>   
>>>>> This being said, if you think a custom ->supports_op()
>>>>> implementation is needed for this controller I can add one.
>>>>>      
>>>>
>>>> spi_mem_supports_op() should suffice for now if above issue is fixed.
>>>
>>> Cool. IIUC, you tested the series on a TI SoC. Does it work as
>>> expected? Do you see any perf regressions?
>>>    
>>
>> I am planning to collect throughput numbers with this series for TI
>> QSPI. I don't think there would be noticeable degradation.
> 
> Ok.
> 
>> But, it would be interesting to test for a driver thats now under
>> drivers/mtd/spi-nor moved to drivers/spi and see if added overhead of
>> m25p80 layer + spi core has any impact.
> 
> I'm working with Frieder on the fsl-qspi rework, so we should have
> numbers soon.

I made a speed comparison between fsl-quadspi.c and Boris' 
spi-fsl-qspi.c using a Micron MT25QL512 64MB NOR: [1]

It seems like the spi-mem-based driver is slower up to about 40%.

I had to remove USE_FSR, as FSR read/write doesn't work with both drivers:

-       { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | 
USE_FSR | SPI_NOR_QUAD_READ) },
+       { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | 
SPI_NOR_QUAD_READ) },

For the spi-mem driver I set spi-tx-bus-width = <1> to match with 
fsl-quadspi.c (does not use quad write).
My dts looks like this for both cases:

&qspi {
     pinctrl-names = "default";
     pinctrl-0 = <&pinctrl_qspi>;
     status = "okay";

     flash0: n25q512ax3@0 {
         #address-cells = <1>;
         #size-cells = <1>;
         compatible = "micron,n25q512ax3", "jedec,spi-nor";
         spi-max-frequency = <108000000>;
         spi-rx-bus-width = <4>;
         spi-tx-bus-width = <1>;
         reg = <0>;
     };
};

Regards,

Frieder

[1]: https://paste.ee/p/dc9KM
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Boris Brezillon Feb. 14, 2018, 9 p.m. UTC | #11

On Wed, 14 Feb 2018 20:44:20 +0000
Schrempf Frieder <[e-mail]> wrote:

> Hi Boris, hi Vignesh,
> 
> On 14.02.2018 20:09, Boris Brezillon wrote:
> > On Wed, 14 Feb 2018 21:55:10 +0530
> > Vignesh R <[e-mail]> wrote:
> >   
> >> On 12-Feb-18 9:38 PM, Boris Brezillon wrote:  
> >>> On Mon, 12 Feb 2018 21:30:09 +0530
> >>> Vignesh R <[e-mail]> wrote:
> >>>      
> >>>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:  
> >>>>> On Mon, 12 Feb 2018 17:13:55 +0530
> >>>>> Vignesh R <[e-mail]> wrote:
> >>>>>        
> >>>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:  
> >>>>>>> From: Boris Brezillon <[e-mail]>
> >>>>>>>
> >>>>>>> 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 <[e-mail]>
> >>>>>>> ---
> >>>>>>>    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  
> >>>>>>
> >>>>>> [...]
> >>>>>>        
> >>>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >>>>>>> +   .exec_op = ti_qspi_exec_mem_op,  
> >>>>>>
> >>>>>>          .supports_op = ti_qspi_supports_mem_op,
> >>>>>>
> >>>>>> Its required as per spi_controller_check_ops() in Patch 1/6  
> >>>>>        
> >>>>> ->supports_op() is optional, and if it's missing, the core will do the  
> >>>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> >>>>> implementation).  
> >>>>
> >>>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
> >>>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
> >>>> +{
> >>>> +	/*
> >>>> +	 * The controller can implement only the high-level SPI-memory
> >>>> +	 * operations if it does not support regular SPI transfers.
> >>>> +	 */
> >>>> +	if (ctlr->mem_ops) {
> >>>> +		if (!ctlr->mem_ops->supports_op ||
> >>>> +		    !ctlr->mem_ops->exec_op)
> >>>> +			return -EINVAL;
> >>>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
> >>>> +		   !ctlr->transfer_one_message) {
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>
> >>>> So if ->supports_op() is not populated by SPI controller driver, then
> >>>> driver probe fails with -EINVAL. This is what I observed on my TI
> >>>> hardware when testing this patch series.  
> >>>
> >>> Correct. Then I should fix spi_controller_check_ops() to allow empty
> >>> ctlr->mem_ops->supports_op.
> >>>      
> >>>>     
> >>>>> This being said, if you think a custom ->supports_op()
> >>>>> implementation is needed for this controller I can add one.
> >>>>>        
> >>>>
> >>>> spi_mem_supports_op() should suffice for now if above issue is fixed.  
> >>>
> >>> Cool. IIUC, you tested the series on a TI SoC. Does it work as
> >>> expected? Do you see any perf regressions?
> >>>      
> >>
> >> I am planning to collect throughput numbers with this series for TI
> >> QSPI. I don't think there would be noticeable degradation.  
> > 
> > Ok.
> >   
> >> But, it would be interesting to test for a driver thats now under
> >> drivers/mtd/spi-nor moved to drivers/spi and see if added overhead of
> >> m25p80 layer + spi core has any impact.  
> > 
> > I'm working with Frieder on the fsl-qspi rework, so we should have
> > numbers soon.  
> 
> I made a speed comparison between fsl-quadspi.c and Boris' 
> spi-fsl-qspi.c using a Micron MT25QL512 64MB NOR: [1]
> 
> It seems like the spi-mem-based driver is slower up to about 40%.

Ouch, not good! Are you sure the clk is running at the same freq (you
can check /sys/kernel/debug/clk/clk_summary)?

I guess the read path is optimized by some kind of pre-fetching (that's
particularly true for the 'read eraseblock' test where we see a 50%
gain with the old driver) which can't be done with the new driver (at
least in its current state). What I'm more surprised about is the
difference in the write speed. 

> 
> I had to remove USE_FSR, as FSR read/write doesn't work with both drivers:
> 
> -       { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | 
> USE_FSR | SPI_NOR_QUAD_READ) },
> +       { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | 
> SPI_NOR_QUAD_READ) },
> 
> For the spi-mem driver I set spi-tx-bus-width = <1> to match with 
> fsl-quadspi.c (does not use quad write).
> My dts looks like this for both cases:
> 
> &qspi {
>      pinctrl-names = "default";
>      pinctrl-0 = <&pinctrl_qspi>;
>      status = "okay";
> 
>      flash0: n25q512ax3@0 {
>          #address-cells = <1>;
>          #size-cells = <1>;
>          compatible = "micron,n25q512ax3", "jedec,spi-nor";
>          spi-max-frequency = <108000000>;
>          spi-rx-bus-width = <4>;
>          spi-tx-bus-width = <1>;
>          reg = <0>;
>      };
> };
> 
> Regards,
> 
> Frieder
> 
> [1]: https://paste.ee/p/dc9KM

Frieder Schrempf Feb. 15, 2018, 4:38 p.m. UTC | #12

Hi,

On 14.02.2018 22:00, Boris Brezillon wrote:
> On Wed, 14 Feb 2018 20:44:20 +0000
> Schrempf Frieder <[e-mail]> wrote:
> 
>> Hi Boris, hi Vignesh,
>>
>> On 14.02.2018 20:09, Boris Brezillon wrote:
>>> On Wed, 14 Feb 2018 21:55:10 +0530
>>> Vignesh R <[e-mail]> wrote:
>>>    
>>>> On 12-Feb-18 9:38 PM, Boris Brezillon wrote:
>>>>> On Mon, 12 Feb 2018 21:30:09 +0530
>>>>> Vignesh R <[e-mail]> wrote:
>>>>>       
>>>>>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
>>>>>>> On Mon, 12 Feb 2018 17:13:55 +0530
>>>>>>> Vignesh R <[e-mail]> wrote:
>>>>>>>         
>>>>>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
>>>>>>>>> From: Boris Brezillon <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>     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
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>         
>>>>>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>>>>>>>>> +   .exec_op = ti_qspi_exec_mem_op,
>>>>>>>>
>>>>>>>>           .supports_op = ti_qspi_supports_mem_op,
>>>>>>>>
>>>>>>>> Its required as per spi_controller_check_ops() in Patch 1/6
>>>>>>>         
>>>>>>> ->supports_op() is optional, and if it's missing, the core will do the
>>>>>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
>>>>>>> implementation).
>>>>>>
>>>>>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
>>>>>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
>>>>>> +{
>>>>>> +	/*
>>>>>> +	 * The controller can implement only the high-level SPI-memory
>>>>>> +	 * operations if it does not support regular SPI transfers.
>>>>>> +	 */
>>>>>> +	if (ctlr->mem_ops) {
>>>>>> +		if (!ctlr->mem_ops->supports_op ||
>>>>>> +		    !ctlr->mem_ops->exec_op)
>>>>>> +			return -EINVAL;
>>>>>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
>>>>>> +		   !ctlr->transfer_one_message) {
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>>
>>>>>> So if ->supports_op() is not populated by SPI controller driver, then
>>>>>> driver probe fails with -EINVAL. This is what I observed on my TI
>>>>>> hardware when testing this patch series.
>>>>>
>>>>> Correct. Then I should fix spi_controller_check_ops() to allow empty
>>>>> ctlr->mem_ops->supports_op.
>>>>>       
>>>>>>      
>>>>>>> This being said, if you think a custom ->supports_op()
>>>>>>> implementation is needed for this controller I can add one.
>>>>>>>         
>>>>>>
>>>>>> spi_mem_supports_op() should suffice for now if above issue is fixed.
>>>>>
>>>>> Cool. IIUC, you tested the series on a TI SoC. Does it work as
>>>>> expected? Do you see any perf regressions?
>>>>>       
>>>>
>>>> I am planning to collect throughput numbers with this series for TI
>>>> QSPI. I don't think there would be noticeable degradation.
>>>
>>> Ok.
>>>    
>>>> But, it would be interesting to test for a driver thats now under
>>>> drivers/mtd/spi-nor moved to drivers/spi and see if added overhead of
>>>> m25p80 layer + spi core has any impact.
>>>
>>> I'm working with Frieder on the fsl-qspi rework, so we should have
>>> numbers soon.
>>
>> I made a speed comparison between fsl-quadspi.c and Boris'
>> spi-fsl-qspi.c using a Micron MT25QL512 64MB NOR: [1]
>>
>> It seems like the spi-mem-based driver is slower up to about 40%.
> 
> Ouch, not good! Are you sure the clk is running at the same freq (you
> can check /sys/kernel/debug/clk/clk_summary)?
> 
> I guess the read path is optimized by some kind of pre-fetching (that's
> particularly true for the 'read eraseblock' test where we see a 50%
> gain with the old driver) which can't be done with the new driver (at
> least in its current state). What I'm more surprised about is the
> difference in the write speed.

So as Boris and I found out, moving the clk_prep_enable/disable() calls 
out of exec_op() improves performance a lot.

The write performance for single eraseblocks is still slower (~12%).
But apart from that the other values are almost equal or even faster.

As Boris tried out with his SPI NAND implementation, an other measure to 
improve performance is to use polling while waiting for the completion 
of a flash operation, as the current implementation with interrupt 
causes "long" scheduling latency delays.

> 
>>
>> I had to remove USE_FSR, as FSR read/write doesn't work with both drivers:
>>
>> -       { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ) },
>> +       { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
>> SPI_NOR_QUAD_READ) },
>>
>> For the spi-mem driver I set spi-tx-bus-width = <1> to match with
>> fsl-quadspi.c (does not use quad write).
>> My dts looks like this for both cases:
>>
>> &qspi {
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&pinctrl_qspi>;
>>       status = "okay";
>>
>>       flash0: n25q512ax3@0 {
>>           #address-cells = <1>;
>>           #size-cells = <1>;
>>           compatible = "micron,n25q512ax3", "jedec,spi-nor";
>>           spi-max-frequency = <108000000>;
>>           spi-rx-bus-width = <4>;
>>           spi-tx-bus-width = <1>;
>>           reg = <0>;
>>       };
>> };
>>
>> Regards,
>>
>> Frieder
>>
>> [1]: https://paste.ee/p/dc9KM

Regards,

Frieder

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

Boris Brezillon Feb. 16, 2018, 10:25 a.m. UTC | #13

On Tue,  6 Feb 2018 00:21:18 +0100
Boris Brezillon <[e-mail]> wrote:


> @@ -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. */
> +	if (from > 0x4000000)
> +		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)) {

As Vignesh just reported, it should be virt_addr_valid(op->data.buf.in)
and not !virt_addr_valid(op->data.buf.in).

> +			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;

Vignesh Raghavendra Feb. 17, 2018, 11:01 a.m. UTC | #14

[...]
>>>>>>     
>>>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>>>>>>> +   .exec_op = ti_qspi_exec_mem_op,      
>>>>>>
>>>>>>         .supports_op = ti_qspi_supports_mem_op,
>>>>>>
>>>>>> Its required as per spi_controller_check_ops() in Patch 1/6    
>>>>>     
>>>>> ->supports_op() is optional, and if it's missing, the core will do the    
>>>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
>>>>> implementation).     
>>>>
>>>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
>>>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
>>>> +{
>>>> +	/*
>>>> +	 * The controller can implement only the high-level SPI-memory
>>>> +	 * operations if it does not support regular SPI transfers.
>>>> +	 */
>>>> +	if (ctlr->mem_ops) {
>>>> +		if (!ctlr->mem_ops->supports_op ||
>>>> +		    !ctlr->mem_ops->exec_op)
>>>> +			return -EINVAL;
>>>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
>>>> +		   !ctlr->transfer_one_message) {
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>
>>>> So if ->supports_op() is not populated by SPI controller driver, then
>>>> driver probe fails with -EINVAL. This is what I observed on my TI
>>>> hardware when testing this patch series.  
>>>
>>> Correct. Then I should fix spi_controller_check_ops() to allow empty
>>> ctlr->mem_ops->supports_op.
>>>   
>>>>  
>>>>> This being said, if you think a custom ->supports_op()
>>>>> implementation is needed for this controller I can add one.
>>>>>     
>>>>
>>>> spi_mem_supports_op() should suffice for now if above issue is fixed.  
>>>
>>> Cool. IIUC, you tested the series on a TI SoC. Does it work as
>>> expected? Do you see any perf regressions?
>>>   

No other functional failures or performance issues so far wrt TI QSPI.
Things look good :)

Boris Brezillon Feb. 17, 2018, 9:52 p.m. UTC | #15

On Sat, 17 Feb 2018 16:31:48 +0530
Vignesh R <[e-mail]> wrote:

> [...]
> >>>>>>       
> >>>>>>> +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >>>>>>> +   .exec_op = ti_qspi_exec_mem_op,        
> >>>>>>
> >>>>>>         .supports_op = ti_qspi_supports_mem_op,
> >>>>>>
> >>>>>> Its required as per spi_controller_check_ops() in Patch 1/6      
> >>>>>       
> >>>>> ->supports_op() is optional, and if it's missing, the core will do the      
> >>>>> regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> >>>>> implementation).       
> >>>>
> >>>> You might have overlooked spi_controller_check_ops() from Patch 1/6:
> >>>> +static int spi_controller_check_ops(struct spi_controller *ctlr)
> >>>> +{
> >>>> +	/*
> >>>> +	 * The controller can implement only the high-level SPI-memory
> >>>> +	 * operations if it does not support regular SPI transfers.
> >>>> +	 */
> >>>> +	if (ctlr->mem_ops) {
> >>>> +		if (!ctlr->mem_ops->supports_op ||
> >>>> +		    !ctlr->mem_ops->exec_op)
> >>>> +			return -EINVAL;
> >>>> +	} else if (!ctlr->transfer && !ctlr->transfer_one &&
> >>>> +		   !ctlr->transfer_one_message) {
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>
> >>>> So if ->supports_op() is not populated by SPI controller driver, then
> >>>> driver probe fails with -EINVAL. This is what I observed on my TI
> >>>> hardware when testing this patch series.    
> >>>
> >>> Correct. Then I should fix spi_controller_check_ops() to allow empty
> >>> ctlr->mem_ops->supports_op.
> >>>     
> >>>>    
> >>>>> This being said, if you think a custom ->supports_op()
> >>>>> implementation is needed for this controller I can add one.
> >>>>>       
> >>>>
> >>>> spi_mem_supports_op() should suffice for now if above issue is fixed.    
> >>>
> >>> Cool. IIUC, you tested the series on a TI SoC. Does it work as
> >>> expected? Do you see any perf regressions?
> >>>     
> 
> No other functional failures or performance issues so far wrt TI QSPI.
> Things look good :)

That's good news. I'll send a v2 addressing the problems you and others
reported soon, but I'd like to have Cyrille's and Mark's feedback first.

BTW, don't hesitate to comment on the interface itself. Do you think
it's missing important features? Do you need more information to better
optimize SPI memory accesses? ...
This truly was an RFC: I'm new to these QSPI/SPI-NOR/SPI-NAND stuff, and
I know you and others already faced various problems and thought about
different solutions to address them. Now is a good time to re-think the
whole thing and design the spi_mem interface in a way that allows us to
better support all these QSPI-memory-oriented controllers.

Regards,

Boris

Patch

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. */
+	if (from > 0x4000000)
+		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;