| Message ID | 20180205232120.5851-5-boris.brezillon@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: Extend the framework to generically support memory devices | expand |
Hi Boris, On Tue, 6 Feb 2018 00:21:18 +0100, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > 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@free-electrons.com> > --- > 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;
On Sun, 11 Feb 2018 16:17:06 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > On Tue, 6 Feb 2018 00:21:18 +0100, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > 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@free-electrons.com> > > --- > > 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; > > >
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
On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > 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@free-electrons.com> > --- > 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; >
On Mon, 12 Feb 2018 17:13:55 +0530 Vignesh R <vigneshr@ti.com> wrote: > On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: > > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > 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@free-electrons.com> > > --- > > 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; > > >
On 12-Feb-18 6:01 PM, Boris Brezillon wrote: > On Mon, 12 Feb 2018 17:13:55 +0530 > Vignesh R <vigneshr@ti.com> wrote: > >> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: >> > From: Boris Brezillon <boris.brezillon@free-electrons.com> >> > >> > 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@free-electrons.com> >> > --- >> > 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
On Mon, 12 Feb 2018 21:30:09 +0530 Vignesh R <vigneshr@ti.com> wrote: > On 12-Feb-18 6:01 PM, Boris Brezillon wrote: > > On Mon, 12 Feb 2018 17:13:55 +0530 > > Vignesh R <vigneshr@ti.com> wrote: > > > >> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: > >> > From: Boris Brezillon <boris.brezillon@free-electrons.com> > >> > > >> > 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@free-electrons.com> > >> > --- > >> > 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
On 12-Feb-18 9:38 PM, Boris Brezillon wrote: > On Mon, 12 Feb 2018 21:30:09 +0530 > Vignesh R <vigneshr@ti.com> wrote: > >> On 12-Feb-18 6:01 PM, Boris Brezillon wrote: >>> On Mon, 12 Feb 2018 17:13:55 +0530 >>> Vignesh R <vigneshr@ti.com> wrote: >>> >>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: >>>>> From: Boris Brezillon <boris.brezillon@free-electrons.com> >>>>> >>>>> 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@free-electrons.com> >>>>> --- >>>>> 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.
On Wed, 14 Feb 2018 21:55:10 +0530 Vignesh R <vigneshr@ti.com> wrote: > On 12-Feb-18 9:38 PM, Boris Brezillon wrote: > > On Mon, 12 Feb 2018 21:30:09 +0530 > > Vignesh R <vigneshr@ti.com> wrote: > > > >> On 12-Feb-18 6:01 PM, Boris Brezillon wrote: > >>> On Mon, 12 Feb 2018 17:13:55 +0530 > >>> Vignesh R <vigneshr@ti.com> wrote: > >>> > >>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: > >>>>> From: Boris Brezillon <boris.brezillon@free-electrons.com> > >>>>> > >>>>> 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@free-electrons.com> > >>>>> --- > >>>>> 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.
Hi Boris, hi Vignesh, On 14.02.2018 20:09, Boris Brezillon wrote: > On Wed, 14 Feb 2018 21:55:10 +0530 > Vignesh R <vigneshr@ti.com> wrote: > >> On 12-Feb-18 9:38 PM, Boris Brezillon wrote: >>> On Mon, 12 Feb 2018 21:30:09 +0530 >>> Vignesh R <vigneshr@ti.com> wrote: >>> >>>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote: >>>>> On Mon, 12 Feb 2018 17:13:55 +0530 >>>>> Vignesh R <vigneshr@ti.com> wrote: >>>>> >>>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: >>>>>>> From: Boris Brezillon <boris.brezillon@free-electrons.com> >>>>>>> >>>>>>> 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@free-electrons.com> >>>>>>> --- >>>>>>> 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
On Wed, 14 Feb 2018 20:44:20 +0000 Schrempf Frieder <frieder.schrempf@exceet.de> 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 <vigneshr@ti.com> wrote: > > > >> On 12-Feb-18 9:38 PM, Boris Brezillon wrote: > >>> On Mon, 12 Feb 2018 21:30:09 +0530 > >>> Vignesh R <vigneshr@ti.com> wrote: > >>> > >>>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote: > >>>>> On Mon, 12 Feb 2018 17:13:55 +0530 > >>>>> Vignesh R <vigneshr@ti.com> wrote: > >>>>> > >>>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: > >>>>>>> From: Boris Brezillon <boris.brezillon@free-electrons.com> > >>>>>>> > >>>>>>> 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@free-electrons.com> > >>>>>>> --- > >>>>>>> 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
Hi, On 14.02.2018 22:00, Boris Brezillon wrote: > On Wed, 14 Feb 2018 20:44:20 +0000 > Schrempf Frieder <frieder.schrempf@exceet.de> 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 <vigneshr@ti.com> wrote: >>> >>>> On 12-Feb-18 9:38 PM, Boris Brezillon wrote: >>>>> On Mon, 12 Feb 2018 21:30:09 +0530 >>>>> Vignesh R <vigneshr@ti.com> wrote: >>>>> >>>>>> On 12-Feb-18 6:01 PM, Boris Brezillon wrote: >>>>>>> On Mon, 12 Feb 2018 17:13:55 +0530 >>>>>>> Vignesh R <vigneshr@ti.com> wrote: >>>>>>> >>>>>>>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: >>>>>>>>> From: Boris Brezillon <boris.brezillon@free-electrons.com> >>>>>>>>> >>>>>>>>> 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@free-electrons.com> >>>>>>>>> --- >>>>>>>>> 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
On Tue, 6 Feb 2018 00:21:18 +0100 Boris Brezillon <boris.brezillon@bootlin.com> 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;
[...] >>>>>> >>>>>>> +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 :)
On Sat, 17 Feb 2018 16:31:48 +0530 Vignesh R <vigneshr@ti.com> 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
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;