diff mbox series

[03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

Message ID 20210713130538.646-4-a-nandan@ti.com (mailing list archive)
State New, archived
Headers show
Series mtd: spinand: Add Octal DTR SPI (8D-8D-8D) mode support | expand

Commit Message

Apurva Nandan July 13, 2021, 1:05 p.m. UTC
Currently, the op macros in spinand.h don't give the option to setup
any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
Having a function that setups the op based on reg_proto would be
better than trying to write all the setup logic in op macros.

Create a spimem_setup_op() that would setup cmd, addr, dummy and data
phase of the spi_mem op, for the given spinand->reg_proto. And hence,
call the spimem_setup_op() before executing any spi_mem op.

Note: In this commit, spimem_setup_op() isn't called in the
read_reg_op(), write_reg_op() and wait() functions, as they need
modifications in address value and data nbytes when in Octal DTR mode.
This will be fixed in a later commit.

Signed-off-by: Apurva Nandan <a-nandan@ti.com>
---
 drivers/mtd/nand/spi/core.c | 51 +++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

Comments

Miquel Raynal Aug. 6, 2021, 6:30 p.m. UTC | #1
Hi Apurva,

Apurva Nandan <a-nandan@ti.com> wrote on Tue, 13 Jul 2021 13:05:28
+0000:

> Currently, the op macros in spinand.h don't give the option to setup
> any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
> Having a function that setups the op based on reg_proto would be
> better than trying to write all the setup logic in op macros.
> 
> Create a spimem_setup_op() that would setup cmd, addr, dummy and data
> phase of the spi_mem op, for the given spinand->reg_proto. And hence,
> call the spimem_setup_op() before executing any spi_mem op.
> 
> Note: In this commit, spimem_setup_op() isn't called in the
> read_reg_op(), write_reg_op() and wait() functions, as they need
> modifications in address value and data nbytes when in Octal DTR mode.
> This will be fixed in a later commit.

Thanks for this series!

So far I am fine with your changes, but I don't like the setup_op()
naming much. I don't yet have a better idea, could you propose
something more meaningful?

> Signed-off-by: Apurva Nandan <a-nandan@ti.com>

Thanks,
Miquèl
Apurva Nandan Aug. 20, 2021, 9:52 a.m. UTC | #2
Hi Miquèl,

On 07/08/21 12:00 am, Miquel Raynal wrote:
> Hi Apurva,
> 
> Apurva Nandan <a-nandan@ti.com> wrote on Tue, 13 Jul 2021 13:05:28
> +0000:
> 
>> Currently, the op macros in spinand.h don't give the option to setup
>> any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
>> Having a function that setups the op based on reg_proto would be
>> better than trying to write all the setup logic in op macros.
>>
>> Create a spimem_setup_op() that would setup cmd, addr, dummy and data
>> phase of the spi_mem op, for the given spinand->reg_proto. And hence,
>> call the spimem_setup_op() before executing any spi_mem op.
>>
>> Note: In this commit, spimem_setup_op() isn't called in the
>> read_reg_op(), write_reg_op() and wait() functions, as they need
>> modifications in address value and data nbytes when in Octal DTR mode.
>> This will be fixed in a later commit.
> 
> Thanks for this series!
> 
> So far I am fine with your changes, but I don't like the setup_op()
> naming much. I don't yet have a better idea, could you propose
> something more meaningful?
> 

I made this similar to the spi_nor_spimem_setup_op(), which essentially 
does the same task as this in the spi-nor core.

Other names that I can think of are:

- config_op(), adjust_op(), amend_op(), patch_op()

or

- handle_op_variations(), apply_op_variations()

What do you suggest?

>> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> 
> Thanks,
> Miquèl
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
Thanks,
Apurva Nandan
Miquel Raynal Aug. 20, 2021, 12:08 p.m. UTC | #3
Hi Apurva,

Boris, you might have a good idea for the naming discussed below?

Apurva Nandan <a-nandan@ti.com> wrote on Fri, 20 Aug 2021 15:22:54
+0530:

> Hi Miquèl,
> 
> On 07/08/21 12:00 am, Miquel Raynal wrote:
> > Hi Apurva,
> > 
> > Apurva Nandan <a-nandan@ti.com> wrote on Tue, 13 Jul 2021 13:05:28
> > +0000:
> >   
> >> Currently, the op macros in spinand.h don't give the option to setup
> >> any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
> >> Having a function that setups the op based on reg_proto would be
> >> better than trying to write all the setup logic in op macros.
> >>
> >> Create a spimem_setup_op() that would setup cmd, addr, dummy and data
> >> phase of the spi_mem op, for the given spinand->reg_proto. And hence,
> >> call the spimem_setup_op() before executing any spi_mem op.
> >>
> >> Note: In this commit, spimem_setup_op() isn't called in the
> >> read_reg_op(), write_reg_op() and wait() functions, as they need
> >> modifications in address value and data nbytes when in Octal DTR mode.
> >> This will be fixed in a later commit.  
> > 
> > Thanks for this series!
> > 
> > So far I am fine with your changes, but I don't like the setup_op()
> > naming much. I don't yet have a better idea, could you propose
> > something more meaningful?
> >   
> 
> I made this similar to the spi_nor_spimem_setup_op(), which essentially does the same task as this in the spi-nor core.
> 
> Other names that I can think of are:
> 
> - config_op(), adjust_op(), amend_op(), patch_op()
> 
> or
> 
> - handle_op_variations(), apply_op_variations()
> 
> What do you suggest?
> 
> >> Signed-off-by: Apurva Nandan <a-nandan@ti.com>  
> > 
> > Thanks,
> > Miquèl
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >   
> Thanks,
> Apurva Nandan

Thanks,
Miquèl
Boris Brezillon Aug. 23, 2021, 7:11 a.m. UTC | #4
On Fri, 20 Aug 2021 14:08:40 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Apurva,
> 
> Boris, you might have a good idea for the naming discussed below?

{patch,adjust,tweak}_op() all sound good to me. This being said, I'm
a bit concerned about doing this op tweaking in the hot-path.
Looks like something that should be done at probe or when switching to
8D mode, and kept around. The alternative would be to have per-mode op
tables, which I think would be clearer.
Miquel Raynal Aug. 23, 2021, 7:24 a.m. UTC | #5
Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 23 Aug
2021 09:11:45 +0200:

> On Fri, 20 Aug 2021 14:08:40 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Apurva,
> > 
> > Boris, you might have a good idea for the naming discussed below?  
> 
> {patch,adjust,tweak}_op() all sound good to me. This being said, I'm
> a bit concerned about doing this op tweaking in the hot-path.
> Looks like something that should be done at probe or when switching to
> 8D mode, and kept around. The alternative would be to have per-mode op
> tables, which I think would be clearer.

True! Thanks for the idea!

Cheers,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index a4f25649e293..2e59faecc8f5 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -20,6 +20,51 @@ 
 #include <linux/spi/spi.h>
 #include <linux/spi/spi-mem.h>
 
+/**
+ * spinand_setup_op() - Helper function to setup the spi_mem op based on the
+ *			spinand->reg_proto
+ * @spinand: the spinand device
+ * @op: the spi_mem op to setup
+ *
+ * Set up buswidth and dtr fields for cmd, addr, dummy and data phase. Also
+ * adjust cmd opcode and dummy nbytes. This function doesn't make any changes
+ * to addr val or data buf.
+ */
+static void spinand_setup_op(const struct spinand_device *spinand,
+			     struct spi_mem_op *op)
+{
+	u8 op_buswidth = SPINAND_PROTO_BUSWIDTH(spinand->reg_proto);
+	u8 op_is_dtr = SPINAND_PROTO_IS_DTR(spinand->reg_proto);
+
+	if (spinand->reg_proto == SPINAND_SINGLE_STR)
+		return;
+
+	op->cmd.buswidth = op_buswidth;
+	op->cmd.dtr = op_is_dtr;
+	if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
+		op->cmd.opcode = (op->cmd.opcode << 8) | op->cmd.opcode;
+		op->cmd.nbytes = 2;
+	}
+
+	if (op->addr.nbytes) {
+		op->addr.buswidth = op_buswidth;
+		op->addr.dtr = op_is_dtr;
+	}
+
+	if (op->dummy.nbytes) {
+		op->dummy.buswidth = op_buswidth;
+		if (op_is_dtr) {
+			op->dummy.nbytes *= 2;
+			op->dummy.dtr = true;
+		}
+	}
+
+	if (op->data.nbytes) {
+		op->data.buswidth = op_buswidth;
+		op->data.dtr = op_is_dtr;
+	}
+}
+
 static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
 {
 	struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg,
@@ -341,6 +386,7 @@  static int spinand_write_enable_op(struct spinand_device *spinand)
 {
 	struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);
 
+	spinand_setup_op(spinand, &op);
 	return spi_mem_exec_op(spinand->spimem, &op);
 }
 
@@ -351,6 +397,7 @@  static int spinand_load_page_op(struct spinand_device *spinand,
 	unsigned int row = nanddev_pos_to_row(nand, &req->pos);
 	struct spi_mem_op op = SPINAND_PAGE_READ_OP(row);
 
+	spinand_setup_op(spinand, &op);
 	return spi_mem_exec_op(spinand->spimem, &op);
 }
 
@@ -475,6 +522,7 @@  static int spinand_program_op(struct spinand_device *spinand,
 	unsigned int row = nanddev_pos_to_row(nand, &req->pos);
 	struct spi_mem_op op = SPINAND_PROG_EXEC_OP(row);
 
+	spinand_setup_op(spinand, &op);
 	return spi_mem_exec_op(spinand->spimem, &op);
 }
 
@@ -485,6 +533,7 @@  static int spinand_erase_op(struct spinand_device *spinand,
 	unsigned int row = nanddev_pos_to_row(nand, pos);
 	struct spi_mem_op op = SPINAND_BLK_ERASE_OP(row);
 
+	spinand_setup_op(spinand, &op);
 	return spi_mem_exec_op(spinand->spimem, &op);
 }
 
@@ -531,6 +580,7 @@  static int spinand_read_id_op(struct spinand_device *spinand, u8 naddr,
 		naddr, ndummy, spinand->scratchbuf, SPINAND_MAX_ID_LEN);
 	int ret;
 
+	spinand_setup_op(spinand, &op);
 	ret = spi_mem_exec_op(spinand->spimem, &op);
 	if (!ret)
 		memcpy(buf, spinand->scratchbuf, SPINAND_MAX_ID_LEN);
@@ -543,6 +593,7 @@  static int spinand_reset_op(struct spinand_device *spinand)
 	struct spi_mem_op op = SPINAND_RESET_OP;
 	int ret;
 
+	spinand_setup_op(spinand, &op);
 	ret = spi_mem_exec_op(spinand->spimem, &op);
 	if (ret)
 		return ret;