| Message ID | 13ff717c-a85c-8532-b86e-1dc04af0c9dd@cogentembedded.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: use spi-mem dirmap API | expand |
Hi, Sergei, Looks good. Few nits below that I can fix when applying. Let me know if you're ok with the changes. On Monday, January 27, 2020 10:29:06 PM EET Sergei Shtylyov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Make use of the spi-mem direct mapping API to let advanced controllers > optimize read/write operations when they support direct mapping. > > Based on the original patch by Boris Brezillon > <boris.brezillon@bootlin.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > --- > Changes in version 4: > - moved the spi_mem_dirmap_{read|write}() calls closer to the ending of > spi_nor_{read|write}(), adapting to the chnages caused by the new patch > splitting spi_nor_spimem_xfer_data()... > > Changes in version 3: > - simplified the way spi_mem_dirmap_{read|write}() are called; > - added Boris' tag. > > Changes in version 2: > - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() > to spi_nor_spimem_{read|write}_data(). > > drivers/mtd/spi-nor/spi-nor.c | 97 > ++++++++++++++++++++++++++++++++++++++---- include/linux/mtd/spi-nor.h | > 5 ++ > 2 files changed, 93 insertions(+), 9 deletions(-) > > Index: linux/drivers/mtd/spi-nor/spi-nor.c > =================================================================== > --- linux.orig/drivers/mtd/spi-nor/spi-nor.c > +++ linux/drivers/mtd/spi-nor/spi-nor.c > @@ -306,6 +306,7 @@ static ssize_t spi_nor_spimem_read_data( > SPI_MEM_OP_DUMMY(nor->read_dummy, 1), > SPI_MEM_OP_DATA_IN(len, buf, 1)); > bool usebouncebuf; > + ssize_t nbytes; > int error; > > /* get transfer protocols. */ > @@ -319,14 +320,23 @@ static ssize_t spi_nor_spimem_read_data( > > usebouncebuf = spi_nor_spimem_bounce(nor, &op); > > - error = spi_nor_spimem_exec_op(nor, &op); > - if (error) > - return error; > + if (nor->dirmap.rdesc) { > + nbytes = spi_mem_dirmap_read(nor->dirmap.rdesc, op.addr.val, op.data.nbytes = spi_mem_dirmap_read() ? This way we can get rid of the local variable nbytes. > + op.data.nbytes, > op.data.buf.in); + if (nbytes < 0) > + return nbytes; > + } else { > + error = spi_nor_spimem_exec_op(nor, &op); > + if (error) > + return error; > + > + nbytes = op.data.nbytes; > + } > > if (usebouncebuf) > - memcpy(buf, op.data.buf.in, op.data.nbytes); > + memcpy(buf, op.data.buf.in, nbytes); > > - return op.data.nbytes; > + return nbytes; > } > > /** > @@ -366,6 +376,7 @@ static ssize_t spi_nor_spimem_write_data > SPI_MEM_OP_NO_DUMMY, > SPI_MEM_OP_DATA_OUT(len, buf, 1)); > bool usebouncebuf; > + ssize_t nbytes; > int error; > > op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); > @@ -379,11 +390,19 @@ static ssize_t spi_nor_spimem_write_data > if (usebouncebuf) > memcpy(nor->bouncebuf, buf, op.data.nbytes); > > - error = spi_nor_spimem_exec_op(nor, &op); > - if (error) > - return error; > + if (nor->dirmap.wdesc) { > + nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, > op.addr.val, + op.data.nbytes, I'll align this to "(" op.data.nbytes = spi_mem_dirmap_write() ? This way we can get rid of the local variable nbytes. > op.data.buf.out); + if (nbytes < 0) > + return nbytes; you already return nbytes below, we can drop this check. > + } else { > + error = spi_nor_spimem_exec_op(nor, &op); > + if (error) > + return error; > + nbytes = op.data.nbytes; > + } > > - return op.data.nbytes; > + return nbytes; > } Cheers, ta
On 02/17/2020 02:03 AM, Tudor.Ambarus@microchip.com wrote: > Looks good. Few nits below that I can fix when applying. Let me know if > you're ok with the changes. > > On Monday, January 27, 2020 10:29:06 PM EET Sergei Shtylyov wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> >> Make use of the spi-mem direct mapping API to let advanced controllers >> optimize read/write operations when they support direct mapping. >> >> Based on the original patch by Boris Brezillon >> <boris.brezillon@bootlin.com>. >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >> >> --- >> Changes in version 4: >> - moved the spi_mem_dirmap_{read|write}() calls closer to the ending of >> spi_nor_{read|write}(), adapting to the chnages caused by the new patch >> splitting spi_nor_spimem_xfer_data()... >> >> Changes in version 3: >> - simplified the way spi_mem_dirmap_{read|write}() are called; >> - added Boris' tag. >> >> Changes in version 2: >> - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}() >> to spi_nor_spimem_{read|write}_data(). >> >> drivers/mtd/spi-nor/spi-nor.c | 97 >> ++++++++++++++++++++++++++++++++++++++---- include/linux/mtd/spi-nor.h | >> 5 ++ >> 2 files changed, 93 insertions(+), 9 deletions(-) >> >> Index: linux/drivers/mtd/spi-nor/spi-nor.c >> =================================================================== >> --- linux.orig/drivers/mtd/spi-nor/spi-nor.c >> +++ linux/drivers/mtd/spi-nor/spi-nor.c [...] >> @@ -319,14 +320,23 @@ static ssize_t spi_nor_spimem_read_data( >> >> usebouncebuf = spi_nor_spimem_bounce(nor, &op); >> >> - error = spi_nor_spimem_exec_op(nor, &op); >> - if (error) >> - return error; >> + if (nor->dirmap.rdesc) { >> + nbytes = spi_mem_dirmap_read(nor->dirmap.rdesc, op.addr.val, > > op.data.nbytes = spi_mem_dirmap_read() ? op.data.nbytes is *unsigned int*, spi_mem_dirmap_read() returns ssize_t. > This way we can get rid of the local variable nbytes. op.data.nbytes can't carry the error codes, not even supposed to be of a signed type... [...] >> @@ -379,11 +390,19 @@ static ssize_t spi_nor_spimem_write_data >> if (usebouncebuf) >> memcpy(nor->bouncebuf, buf, op.data.nbytes); >> >> - error = spi_nor_spimem_exec_op(nor, &op); >> - if (error) >> - return error; >> + if (nor->dirmap.wdesc) { >> + nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, >> op.addr.val, + op.data.nbytes, > I'll align this to "(" Sorry about missing that. Copy&paste from the read function played its role here... > op.data.nbytes = spi_mem_dirmap_write() ? Same comments as in the read function... > This way we can get rid of the local variable nbytes. > >> op.data.buf.out); + if (nbytes < 0) >> + return nbytes; > > you already return nbytes below, we can drop this check. Yeah, sorry about that! We've already copied from the bounce buffer >> + } else { >> + error = spi_nor_spimem_exec_op(nor, &op); >> + if (error) >> + return error; >> + nbytes = op.data.nbytes; >> + } >> >> - return op.data.nbytes; >> + return nbytes; >> } > > Cheers, > ta MBR, Sergei
Index: linux/drivers/mtd/spi-nor/spi-nor.c =================================================================== --- linux.orig/drivers/mtd/spi-nor/spi-nor.c +++ linux/drivers/mtd/spi-nor/spi-nor.c @@ -306,6 +306,7 @@ static ssize_t spi_nor_spimem_read_data( SPI_MEM_OP_DUMMY(nor->read_dummy, 1), SPI_MEM_OP_DATA_IN(len, buf, 1)); bool usebouncebuf; + ssize_t nbytes; int error; /* get transfer protocols. */ @@ -319,14 +320,23 @@ static ssize_t spi_nor_spimem_read_data( usebouncebuf = spi_nor_spimem_bounce(nor, &op); - error = spi_nor_spimem_exec_op(nor, &op); - if (error) - return error; + if (nor->dirmap.rdesc) { + nbytes = spi_mem_dirmap_read(nor->dirmap.rdesc, op.addr.val, + op.data.nbytes, op.data.buf.in); + if (nbytes < 0) + return nbytes; + } else { + error = spi_nor_spimem_exec_op(nor, &op); + if (error) + return error; + + nbytes = op.data.nbytes; + } if (usebouncebuf) - memcpy(buf, op.data.buf.in, op.data.nbytes); + memcpy(buf, op.data.buf.in, nbytes); - return op.data.nbytes; + return nbytes; } /** @@ -366,6 +376,7 @@ static ssize_t spi_nor_spimem_write_data SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_OUT(len, buf, 1)); bool usebouncebuf; + ssize_t nbytes; int error; op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); @@ -379,11 +390,19 @@ static ssize_t spi_nor_spimem_write_data if (usebouncebuf) memcpy(nor->bouncebuf, buf, op.data.nbytes); - error = spi_nor_spimem_exec_op(nor, &op); - if (error) - return error; + if (nor->dirmap.wdesc) { + nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val, + op.data.nbytes, op.data.buf.out); + if (nbytes < 0) + return nbytes; + } else { + error = spi_nor_spimem_exec_op(nor, &op); + if (error) + return error; + nbytes = op.data.nbytes; + } - return op.data.nbytes; + return nbytes; } /** @@ -5270,6 +5289,58 @@ int spi_nor_scan(struct spi_nor *nor, co } EXPORT_SYMBOL_GPL(spi_nor_scan); +static int spi_nor_create_read_dirmap(struct spi_nor *nor) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_DUMMY(nor->read_dummy, 1), + SPI_MEM_OP_DATA_IN(0, NULL, 1)), + .offset = 0, + .length = nor->mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + + /* convert the dummy cycles to the number of bytes */ + op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8; + + nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, + &info); + return PTR_ERR_OR_ZERO(nor->dirmap.rdesc); +} + +static int spi_nor_create_write_dirmap(struct spi_nor *nor) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1), + SPI_MEM_OP_ADDR(nor->addr_width, 0, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(0, NULL, 1)), + .offset = 0, + .length = nor->mtd.size, + }; + struct spi_mem_op *op = &info.op_tmpl; + + /* get transfer protocols. */ + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); + op->dummy.buswidth = op->addr.buswidth; + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); + + if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) + op->addr.nbytes = 0; + + nor->dirmap.wdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, + &info); + return PTR_ERR_OR_ZERO(nor->dirmap.wdesc); +} + static int spi_nor_probe(struct spi_mem *spimem) { struct spi_device *spi = spimem->spi; @@ -5331,6 +5402,14 @@ static int spi_nor_probe(struct spi_mem return -ENOMEM; } + ret = spi_nor_create_read_dirmap(nor); + if (ret) + return ret; + + ret = spi_nor_create_write_dirmap(nor); + if (ret) + return ret; + return mtd_device_register(&nor->mtd, data ? data->parts : NULL, data ? data->nr_parts : 0); } Index: linux/include/linux/mtd/spi-nor.h =================================================================== --- linux.orig/include/linux/mtd/spi-nor.h +++ linux/include/linux/mtd/spi-nor.h @@ -604,6 +604,11 @@ struct spi_nor { struct spi_nor_flash_parameter params; + struct { + struct spi_mem_dirmap_desc *rdesc; + struct spi_mem_dirmap_desc *wdesc; + } dirmap; + void *priv; };