| Message ID | 20210713130538.646-4-a-nandan@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: spinand: Add Octal DTR SPI (8D-8D-8D) mode support | expand |
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
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
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
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.
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 --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;
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(+)