[v4,2/2] mtd: spi-nor: use spi-mem dirmap API
diff mbox series

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
Related show

Commit Message

Sergei Shtylyov Jan. 27, 2020, 8:29 p.m. UTC
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(-)

Comments

Ambarus Tudor Feb. 16, 2020, 11:03 p.m. UTC | #1
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
Sergei Shtylyov Feb. 17, 2020, 6:43 p.m. UTC | #2
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

Patch
diff mbox series

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