[PATCH 0/3] mtd: rawnand: brcmnand: Convert to exec_op()

[PATCH 0/3] mtd: rawnand: brcmnand: Convert to exec_op()

[Boris Brezillon]

Hello,

A bit of context to explain the motivation behind those conversions
I've been sending for the last couple of weeks. The raw NAND subsystem
carries a lot of history which makes any rework not only painful, but
also subject to regressions which we only detect when someone dares to
update its kernel on one of those ancient HW (I'm not saying brcmnand
is one of those ancient hardware BTW, but others in the NAND directory
are pretty old). While carrying drivers for old HW is not a problem per
se, carrying ancient and unmaintained drivers that are not converted to
new APIs is a maintenance burden (again, that does not really apply to
brcmnand, as Kamal as always prompt to reply to patches targeting this
driver), hence this massive conversion attempt I'm conducting here.

So here is a series converting the brcmnand NAND controller driver to
exec_op(). It's worth noting that I took the simplest path for this
implementation, using low-level operations for everything that's passed
to exec_op(). There are 2 reasons to that, the first one is that I
don't have the hardware to test and also don't know how this series
will be received so, I decided to take the quickest approach. But even
if we put that aside, I'm not sure the extra complexity implied by
the specialized operation handlers would be worth it, given that the
read/write page paths (those where performance really matters) are
already optimized (see the {read,write}_page[_raw]() implementations).
That leaves us with things that are only executed at boot time (ID,
PARAM_PAGE reads), or things that are simple enough (STATUS read,
ERASE) to not generate to much overhead if we don't use the dedicated
hardware functions. I'm of course open to reworking that part if someone
can validate my changes and come up with numbers showing that the
dedicated functions approach improves perfs.

Regards,

Boris

Boris Brezillon (3):
  mtd: rawnand: Add the concept of destructive operation
  mtd: rawnand: bcrmnand: Add exec_op() support
  mtd: rawnand: brcmnand: Get rid of the legacy interface implementation

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 248 +++++------------------
 include/linux/mtd/rawnand.h              |  11 +
 2 files changed, 66 insertions(+), 193 deletions(-)

-- 
2.25.3


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/3] mtd: rawnand: Add the concept of destructive operation

[Boris Brezillon]

Erase and program operations need the WP (Write Protect) pin to be
de-asserted to take effect. Let's add the concept of destructive
operation and pass the information to exec_op() so controllers know
when they should de-assert this pin without having to guess it from
the command opcode.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 include/linux/mtd/rawnand.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index c47cbcb86b71..6014e7389507 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -854,6 +854,8 @@ struct nand_op_parser {
 /**
  * struct nand_operation - NAND operation descriptor
  * @cs: the CS line to select for this NAND operation
+ * @deassert_wp: set to true when the operation requires the WP pin to be
+ *		 de-asserted (ERASE, PROG, ...)
  * @instrs: array of instructions to execute
  * @ninstrs: length of the @instrs array
  *
@@ -861,6 +863,7 @@ struct nand_op_parser {
  */
 struct nand_operation {
 	unsigned int cs;
+	bool deassert_wp;
 	const struct nand_op_instr *instrs;
 	unsigned int ninstrs;
 };
@@ -872,6 +875,14 @@ struct nand_operation {
 		.ninstrs = ARRAY_SIZE(_instrs),			\
 	}
 
+#define NAND_DESTRUCTIVE_OPERATION(_cs, _instrs)		\
+	{							\
+		.cs = _cs,					\
+		.deassert_wp = true,				\
+		.instrs = _instrs,				\
+		.ninstrs = ARRAY_SIZE(_instrs),			\
+	}
+
 int nand_op_parser_exec_op(struct nand_chip *chip,
 			   const struct nand_op_parser *parser,
 			   const struct nand_operation *op, bool check_only);
-- 
2.25.3


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/3] mtd: rawnand: bcrmnand: Add exec_op() support

[Boris Brezillon]

This implementation of exec_op() relies on low-level operations only. We
could add support for high-level operations too through an op parser,
but the gain is likely to be negligible since read/write page operations
already have a fast path ({readwrite}_page[raw]() implementations).

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 72 ++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..e70117146755 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1599,6 +1599,77 @@ static int brcmnand_low_level_op(struct brcmnand_host *host,
 	return brcmnand_waitfunc(chip);
 }
 
+static void brcmnand_exec_instr(struct brcmnand_host *host,
+				const struct nand_op_instr *instr,
+				bool last_op)
+{
+	unsigned int i;
+	const u8 *out;
+	u8 *in;
+
+	switch (instr->type) {
+	case NAND_OP_CMD_INSTR:
+		brcmnand_low_level_op(host, LL_OP_CMD,
+				      instr->ctx.cmd.opcode, last_op);
+		break;
+
+	case NAND_OP_ADDR_INSTR:
+		for (i = 0; i < instr->ctx.addr.naddrs; i++)
+			brcmnand_low_level_op(host, LL_OP_ADDR,
+					      instr->ctx.addr.addrs[i],
+					      last_op);
+		break;
+
+	case NAND_OP_DATA_IN_INSTR:
+		in = instr->ctx.data.buf.in;
+		for (i = 0; i < instr->ctx.data.len; i++) {
+			brcmnand_low_level_op(host, LL_OP_RD, 0, last_op);
+			in[i] = brcmnand_read_reg(host->ctrl,
+						  BRCMNAND_LL_RDATA);
+		}
+		break;
+
+	case NAND_OP_DATA_OUT_INSTR:
+		out = instr->ctx.data.buf.out;
+		for (i = 0; i < instr->ctx.data.len; i++)
+			brcmnand_low_level_op(host, LL_OP_WR, out[i], last_op);
+		break;
+
+	case NAND_OP_WAITRDY_INSTR:
+		/*
+		 * Nothing to do here, brcmnand_low_level_op() already waits on
+		 * FLASH_READY every time it's called.
+		 */
+		break;
+
+	default:
+		break;
+	}
+}
+
+static int brcmnand_exec_op(struct nand_chip *chip,
+			    const struct nand_operation *op,
+			    bool check_only)
+{
+	struct brcmnand_host *host = nand_get_controller_data(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned int i;
+
+	if (check_only)
+		return 0;
+
+	if (op->deassert_wp)
+		brcmnand_wp(mtd, 0);
+
+	for (i = 0; i < op->ninstrs; i++)
+		brcmnand_exec_instr(host, &op->instrs[i], i == op->ninstrs - 1);
+
+	if (op->deassert_wp)
+		brcmnand_wp(mtd, 1);
+
+	return 0;
+}
+
 static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
 			     int column, int page_addr)
 {
@@ -2597,6 +2668,7 @@ static int brcmnand_attach_chip(struct nand_chip *chip)
 
 static const struct nand_controller_ops brcmnand_controller_ops = {
 	.attach_chip = brcmnand_attach_chip,
+	.exec_op = brcmnand_exec_op,
 };
 
 static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
-- 
2.25.3


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/3] mtd: rawnand: brcmnand: Get rid of the legacy interface implementation

[Boris Brezillon]

Now that exec_op() is implemented, we can get rid of the legacy
interface implementation.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 210 -----------------------
 1 file changed, 210 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e70117146755..f7f75e98ed21 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -919,19 +919,6 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl)
 		return -1;
 }
 
-static int brcmnand_get_sector_size_1k(struct brcmnand_host *host)
-{
-	struct brcmnand_controller *ctrl = host->ctrl;
-	int shift = brcmnand_sector_1k_shift(ctrl);
-	u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
-						  BRCMNAND_CS_ACC_CONTROL);
-
-	if (shift < 0)
-		return 0;
-
-	return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1;
-}
-
 static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val)
 {
 	struct brcmnand_controller *ctrl = host->ctrl;
@@ -1495,12 +1482,6 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
  * NAND MTD API: read/program/erase
  ***********************************************************************/
 
-static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat,
-			      unsigned int ctrl)
-{
-	/* intentionally left blank */
-}
-
 static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
 {
 	struct brcmnand_host *host = nand_get_controller_data(chip);
@@ -1670,190 +1651,6 @@ static int brcmnand_exec_op(struct nand_chip *chip,
 	return 0;
 }
 
-static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
-			     int column, int page_addr)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct brcmnand_host *host = nand_get_controller_data(chip);
-	struct brcmnand_controller *ctrl = host->ctrl;
-	u64 addr = (u64)page_addr << chip->page_shift;
-	int native_cmd = 0;
-
-	if (command == NAND_CMD_READID || command == NAND_CMD_PARAM ||
-			command == NAND_CMD_RNDOUT)
-		addr = (u64)column;
-	/* Avoid propagating a negative, don't-care address */
-	else if (page_addr < 0)
-		addr = 0;
-
-	dev_dbg(ctrl->dev, "cmd 0x%x addr 0x%llx\n", command,
-		(unsigned long long)addr);
-
-	host->last_cmd = command;
-	host->last_byte = 0;
-	host->last_addr = addr;
-
-	switch (command) {
-	case NAND_CMD_RESET:
-		native_cmd = CMD_FLASH_RESET;
-		break;
-	case NAND_CMD_STATUS:
-		native_cmd = CMD_STATUS_READ;
-		break;
-	case NAND_CMD_READID:
-		native_cmd = CMD_DEVICE_ID_READ;
-		break;
-	case NAND_CMD_READOOB:
-		native_cmd = CMD_SPARE_AREA_READ;
-		break;
-	case NAND_CMD_ERASE1:
-		native_cmd = CMD_BLOCK_ERASE;
-		brcmnand_wp(mtd, 0);
-		break;
-	case NAND_CMD_PARAM:
-		native_cmd = CMD_PARAMETER_READ;
-		break;
-	case NAND_CMD_SET_FEATURES:
-	case NAND_CMD_GET_FEATURES:
-		brcmnand_low_level_op(host, LL_OP_CMD, command, false);
-		brcmnand_low_level_op(host, LL_OP_ADDR, column, false);
-		break;
-	case NAND_CMD_RNDOUT:
-		native_cmd = CMD_PARAMETER_CHANGE_COL;
-		addr &= ~((u64)(FC_BYTES - 1));
-		/*
-		 * HW quirk: PARAMETER_CHANGE_COL requires SECTOR_SIZE_1K=0
-		 * NB: hwcfg.sector_size_1k may not be initialized yet
-		 */
-		if (brcmnand_get_sector_size_1k(host)) {
-			host->hwcfg.sector_size_1k =
-				brcmnand_get_sector_size_1k(host);
-			brcmnand_set_sector_size_1k(host, 0);
-		}
-		break;
-	}
-
-	if (!native_cmd)
-		return;
-
-	brcmnand_set_cmd_addr(mtd, addr);
-	brcmnand_send_cmd(host, native_cmd);
-	brcmnand_waitfunc(chip);
-
-	if (native_cmd == CMD_PARAMETER_READ ||
-			native_cmd == CMD_PARAMETER_CHANGE_COL) {
-		/* Copy flash cache word-wise */
-		u32 *flash_cache = (u32 *)ctrl->flash_cache;
-		int i;
-
-		brcmnand_soc_data_bus_prepare(ctrl->soc, true);
-
-		/*
-		 * Must cache the FLASH_CACHE now, since changes in
-		 * SECTOR_SIZE_1K may invalidate it
-		 */
-		for (i = 0; i < FC_WORDS; i++)
-			/*
-			 * Flash cache is big endian for parameter pages, at
-			 * least on STB SoCs
-			 */
-			flash_cache[i] = be32_to_cpu(brcmnand_read_fc(ctrl, i));
-
-		brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
-
-		/* Cleanup from HW quirk: restore SECTOR_SIZE_1K */
-		if (host->hwcfg.sector_size_1k)
-			brcmnand_set_sector_size_1k(host,
-						    host->hwcfg.sector_size_1k);
-	}
-
-	/* Re-enable protection is necessary only after erase */
-	if (command == NAND_CMD_ERASE1)
-		brcmnand_wp(mtd, 1);
-}
-
-static uint8_t brcmnand_read_byte(struct nand_chip *chip)
-{
-	struct brcmnand_host *host = nand_get_controller_data(chip);
-	struct brcmnand_controller *ctrl = host->ctrl;
-	uint8_t ret = 0;
-	int addr, offs;
-
-	switch (host->last_cmd) {
-	case NAND_CMD_READID:
-		if (host->last_byte < 4)
-			ret = brcmnand_read_reg(ctrl, BRCMNAND_ID) >>
-				(24 - (host->last_byte << 3));
-		else if (host->last_byte < 8)
-			ret = brcmnand_read_reg(ctrl, BRCMNAND_ID_EXT) >>
-				(56 - (host->last_byte << 3));
-		break;
-
-	case NAND_CMD_READOOB:
-		ret = oob_reg_read(ctrl, host->last_byte);
-		break;
-
-	case NAND_CMD_STATUS:
-		ret = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) &
-					INTFC_FLASH_STATUS;
-		if (wp_on) /* hide WP status */
-			ret |= NAND_STATUS_WP;
-		break;
-
-	case NAND_CMD_PARAM:
-	case NAND_CMD_RNDOUT:
-		addr = host->last_addr + host->last_byte;
-		offs = addr & (FC_BYTES - 1);
-
-		/* At FC_BYTES boundary, switch to next column */
-		if (host->last_byte > 0 && offs == 0)
-			nand_change_read_column_op(chip, addr, NULL, 0, false);
-
-		ret = ctrl->flash_cache[offs];
-		break;
-	case NAND_CMD_GET_FEATURES:
-		if (host->last_byte >= ONFI_SUBFEATURE_PARAM_LEN) {
-			ret = 0;
-		} else {
-			bool last = host->last_byte ==
-				ONFI_SUBFEATURE_PARAM_LEN - 1;
-			brcmnand_low_level_op(host, LL_OP_RD, 0, last);
-			ret = brcmnand_read_reg(ctrl, BRCMNAND_LL_RDATA) & 0xff;
-		}
-	}
-
-	dev_dbg(ctrl->dev, "read byte = 0x%02x\n", ret);
-	host->last_byte++;
-
-	return ret;
-}
-
-static void brcmnand_read_buf(struct nand_chip *chip, uint8_t *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++, buf++)
-		*buf = brcmnand_read_byte(chip);
-}
-
-static void brcmnand_write_buf(struct nand_chip *chip, const uint8_t *buf,
-			       int len)
-{
-	int i;
-	struct brcmnand_host *host = nand_get_controller_data(chip);
-
-	switch (host->last_cmd) {
-	case NAND_CMD_SET_FEATURES:
-		for (i = 0; i < len; i++)
-			brcmnand_low_level_op(host, LL_OP_WR, buf[i],
-						  (i + 1) == len);
-		break;
-	default:
-		BUG();
-		break;
-	}
-}
-
 /**
  *  Kick EDU engine
  */
@@ -2699,13 +2496,6 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)
 	mtd->owner = THIS_MODULE;
 	mtd->dev.parent = &pdev->dev;
 
-	chip->legacy.cmd_ctrl = brcmnand_cmd_ctrl;
-	chip->legacy.cmdfunc = brcmnand_cmdfunc;
-	chip->legacy.waitfunc = brcmnand_waitfunc;
-	chip->legacy.read_byte = brcmnand_read_byte;
-	chip->legacy.read_buf = brcmnand_read_buf;
-	chip->legacy.write_buf = brcmnand_write_buf;
-
 	chip->ecc.mode = NAND_ECC_HW;
 	chip->ecc.read_page = brcmnand_read_page;
 	chip->ecc.write_page = brcmnand_write_page;
-- 
2.25.3


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: rawnand: Add the concept of destructive operation

[Boris Brezillon]

On Sat,  2 May 2020 18:34:30 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Erase and program operations need the WP (Write Protect) pin to be
> de-asserted to take effect. Let's add the concept of destructive
> operation and pass the information to exec_op() so controllers know
> when they should de-assert this pin without having to guess it from
> the command opcode.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  include/linux/mtd/rawnand.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index c47cbcb86b71..6014e7389507 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -854,6 +854,8 @@ struct nand_op_parser {
>  /**
>   * struct nand_operation - NAND operation descriptor
>   * @cs: the CS line to select for this NAND operation
> + * @deassert_wp: set to true when the operation requires the WP pin to be
> + *		 de-asserted (ERASE, PROG, ...)
>   * @instrs: array of instructions to execute
>   * @ninstrs: length of the @instrs array
>   *
> @@ -861,6 +863,7 @@ struct nand_op_parser {
>   */
>  struct nand_operation {
>  	unsigned int cs;
> +	bool deassert_wp;
>  	const struct nand_op_instr *instrs;
>  	unsigned int ninstrs;
>  };
> @@ -872,6 +875,14 @@ struct nand_operation {
>  		.ninstrs = ARRAY_SIZE(_instrs),			\
>  	}
>  
> +#define NAND_DESTRUCTIVE_OPERATION(_cs, _instrs)		\
> +	{							\
> +		.cs = _cs,					\
> +		.deassert_wp = true,				\
> +		.instrs = _instrs,				\
> +		.ninstrs = ARRAY_SIZE(_instrs),			\
> +	}
> +
>  int nand_op_parser_exec_op(struct nand_chip *chip,
>  			   const struct nand_op_parser *parser,
>  			   const struct nand_operation *op, bool check_only);

The following diff should be part of this patch, otherwise none of the
operations are flagged as destructive. I'll fix that in v2, but I'd still
like to get feedback before sending a new version.

--->8---
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index fb8addf7637e..4111e7ac0834 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1308,7 +1308,8 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page,
                NAND_OP_CMD(NAND_CMD_PAGEPROG, PSEC_TO_NSEC(sdr->tWB_max)),
                NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tPROG_max), 0),
        };
-       struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
+       struct nand_operation op = NAND_DESTRUCTIVE_OPERATION(chip->cur_cs,
+                                                             instrs);
        int naddrs = nand_fill_column_cycles(chip, addrs, offset_in_page);
        int ret;
        u8 status;
@@ -1695,7 +1696,8 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
                                    PSEC_TO_MSEC(sdr->tWB_max)),
                        NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tBERS_max), 0),
                };
-               struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
+               struct nand_operation op = NAND_DESTRUCTIVE_OPERATION(chip->cur_cs,
+                                                                     instrs);
 
                if (chip->options & NAND_ROW_ADDR_3)
                        instrs[1].ctx.addr.naddrs++;

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: rawnand: Add the concept of destructive operation

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 3 May
2020 10:00:18 +0200:

> On Sat,  2 May 2020 18:34:30 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > Erase and program operations need the WP (Write Protect) pin to be
> > de-asserted to take effect. Let's add the concept of destructive
> > operation and pass the information to exec_op() so controllers know
> > when they should de-assert this pin without having to guess it from
> > the command opcode.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  include/linux/mtd/rawnand.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index c47cbcb86b71..6014e7389507 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -854,6 +854,8 @@ struct nand_op_parser {
> >  /**
> >   * struct nand_operation - NAND operation descriptor
> >   * @cs: the CS line to select for this NAND operation
> > + * @deassert_wp: set to true when the operation requires the WP pin to be
> > + *		 de-asserted (ERASE, PROG, ...)
> >   * @instrs: array of instructions to execute
> >   * @ninstrs: length of the @instrs array
> >   *
> > @@ -861,6 +863,7 @@ struct nand_op_parser {
> >   */
> >  struct nand_operation {
> >  	unsigned int cs;
> > +	bool deassert_wp;
> >  	const struct nand_op_instr *instrs;
> >  	unsigned int ninstrs;
> >  };
> > @@ -872,6 +875,14 @@ struct nand_operation {
> >  		.ninstrs = ARRAY_SIZE(_instrs),			\
> >  	}
> >  
> > +#define NAND_DESTRUCTIVE_OPERATION(_cs, _instrs)		\
> > +	{							\
> > +		.cs = _cs,					\
> > +		.deassert_wp = true,				\
> > +		.instrs = _instrs,				\
> > +		.ninstrs = ARRAY_SIZE(_instrs),			\
> > +	}
> > +
> >  int nand_op_parser_exec_op(struct nand_chip *chip,
> >  			   const struct nand_op_parser *parser,
> >  			   const struct nand_operation *op, bool check_only);  
> 
> The following diff should be part of this patch, otherwise none of the
> operations are flagged as destructive. I'll fix that in v2, but I'd still
> like to get feedback before sending a new version.
> 
> --->8---  
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index fb8addf7637e..4111e7ac0834 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1308,7 +1308,8 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page,
>                 NAND_OP_CMD(NAND_CMD_PAGEPROG, PSEC_TO_NSEC(sdr->tWB_max)),
>                 NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tPROG_max), 0),
>         };
> -       struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
> +       struct nand_operation op = NAND_DESTRUCTIVE_OPERATION(chip->cur_cs,
> +                                                             instrs);
>         int naddrs = nand_fill_column_cycles(chip, addrs, offset_in_page);
>         int ret;
>         u8 status;
> @@ -1695,7 +1696,8 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
>                                     PSEC_TO_MSEC(sdr->tWB_max)),
>                         NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tBERS_max), 0),
>                 };
> -               struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
> +               struct nand_operation op = NAND_DESTRUCTIVE_OPERATION(chip->cur_cs,
> +                                                                     instrs);
>  
>                 if (chip->options & NAND_ROW_ADDR_3)
>                         instrs[1].ctx.addr.naddrs++;

What about nand_prog_page_end_op() or even
nand_write_change_column_op()?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: rawnand: bcrmnand: Add exec_op() support

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat,  2 May
2020 18:34:31 +0200:

> This implementation of exec_op() relies on low-level operations only. We
> could add support for high-level operations too through an op parser,
> but the gain is likely to be negligible since read/write page operations
> already have a fast path ({readwrite}_page[raw]() implementations).

Agreed.

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 72 ++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index e4e3ceeac38f..e70117146755 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1599,6 +1599,77 @@ static int brcmnand_low_level_op(struct brcmnand_host *host,
>  	return brcmnand_waitfunc(chip);
>  }
>  
> +static void brcmnand_exec_instr(struct brcmnand_host *host,
> +				const struct nand_op_instr *instr,
> +				bool last_op)
> +{
> +	unsigned int i;
> +	const u8 *out;
> +	u8 *in;
> +
> +	switch (instr->type) {
> +	case NAND_OP_CMD_INSTR:
> +		brcmnand_low_level_op(host, LL_OP_CMD,
> +				      instr->ctx.cmd.opcode, last_op);
> +		break;
> +
> +	case NAND_OP_ADDR_INSTR:
> +		for (i = 0; i < instr->ctx.addr.naddrs; i++)
> +			brcmnand_low_level_op(host, LL_OP_ADDR,
> +					      instr->ctx.addr.addrs[i],
> +					      last_op);
> +		break;
> +
> +	case NAND_OP_DATA_IN_INSTR:
> +		in = instr->ctx.data.buf.in;
> +		for (i = 0; i < instr->ctx.data.len; i++) {
> +			brcmnand_low_level_op(host, LL_OP_RD, 0, last_op);
> +			in[i] = brcmnand_read_reg(host->ctrl,
> +						  BRCMNAND_LL_RDATA);
> +		}
> +		break;
> +
> +	case NAND_OP_DATA_OUT_INSTR:
> +		out = instr->ctx.data.buf.out;
> +		for (i = 0; i < instr->ctx.data.len; i++)
> +			brcmnand_low_level_op(host, LL_OP_WR, out[i], last_op);
> +		break;
> +
> +	case NAND_OP_WAITRDY_INSTR:
> +		/*
> +		 * Nothing to do here, brcmnand_low_level_op() already waits on
> +		 * FLASH_READY every time it's called.
> +		 */
> +		break;
> +
> +	default:
> +		break;
> +	}
> +}
> +
> +static int brcmnand_exec_op(struct nand_chip *chip,
> +			    const struct nand_operation *op,
> +			    bool check_only)
> +{
> +	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	unsigned int i;
> +
> +	if (check_only)
> +		return 0;
> +
> +	if (op->deassert_wp)
> +		brcmnand_wp(mtd, 0);
> +
> +	for (i = 0; i < op->ninstrs; i++)
> +		brcmnand_exec_instr(host, &op->instrs[i], i == op->ninstrs - 1);

Maybe                                                          (              ) 

to improve readability?

Or even maybe using an intermediate boolean?

> +
> +	if (op->deassert_wp)
> +		brcmnand_wp(mtd, 1);
> +
> +	return 0;
> +}
> +
>  static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
>  			     int column, int page_addr)
>  {
> @@ -2597,6 +2668,7 @@ static int brcmnand_attach_chip(struct nand_chip *chip)
>  
>  static const struct nand_controller_ops brcmnand_controller_ops = {
>  	.attach_chip = brcmnand_attach_chip,
> +	.exec_op = brcmnand_exec_op,
>  };
>  
>  static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn)

With this tiny change (don't resend if unneeded):

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/