[RFC PATCH v3 0/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[RFC PATCH v3 0/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

Third revision of the rework patchset to use exec_op for NXP
Vybrid (and others) NAND Flash Controller.

I now avoided calling back into the stack for the ECC read/
write pages. This increases speed 4469 KiB/s write speed and
13490 KiB/s read speed (v2 was 3495 KiB/s/13490 KiB/s).

IMHO it start to look better, probably still needs some fine
tuning. What I don't like too much is that the custom
read/write page accessors dupplicate some code, maybe this
could be done somewhat nicer.

--
Stefan

Changes in v3:
- Separate exec_op() callback addition and removal of old callbacks
- Push data into regs in one function
- Readd op parser
- Implement custom read/write page for hardware ECC
- Rely on generic ecc.write_page_raw
- Use nand_read_oob_op instead of nand_read_page_op

Stefan Agner (3):
  mtd: nand: vf610_nfc: remove unused function
  mtd: nand: vf610_nfc: make use of ->exec_op()
  mtd: nand: vf610_nfc: remove old hooks

 drivers/mtd/nand/vf610_nfc.c | 493 +++++++++++++++++++++----------------------
 1 file changed, 239 insertions(+), 254 deletions(-)

-- 
2.16.1

[RFC PATCH v3 1/3] mtd: nand: vf610_nfc: remove unused function

[Stefan Agner]

The function count_written_bits has been replaced by the generic
nand_check_erased_ecc_chunk() function with commit 48c25cf44118
("mtd: nand: vf610_nfc: use nand_check_erased_ecc_chunk() helper").
Remove the unused function.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/vf610_nfc.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index 80d31a58e558..2fa61cbdbaf7 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -511,21 +511,6 @@ static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
 	vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
 }
 
-/* Count the number of 0's in buff up to max_bits */
-static inline int count_written_bits(uint8_t *buff, int size, int max_bits)
-{
-	uint32_t *buff32 = (uint32_t *)buff;
-	int k, written_bits = 0;
-
-	for (k = 0; k < (size / 4); k++) {
-		written_bits += hweight32(~buff32[k]);
-		if (unlikely(written_bits > max_bits))
-			break;
-	}
-
-	return written_bits;
-}
-
 static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
 					 uint8_t *oob, int page)
 {
-- 
2.16.1

Re: [RFC PATCH v3 1/3] mtd: nand: vf610_nfc: remove unused function

[Boris Brezillon]

On Fri,  9 Feb 2018 00:59:19 +0100
Stefan Agner <stefan@agner.ch> wrote:

> The function count_written_bits has been replaced by the generic
> nand_check_erased_ecc_chunk() function with commit 48c25cf44118
> ("mtd: nand: vf610_nfc: use nand_check_erased_ecc_chunk() helper").
> Remove the unused function.
> 

Applied.

Thanks,

Boris

> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mtd/nand/vf610_nfc.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> index 80d31a58e558..2fa61cbdbaf7 100644
> --- a/drivers/mtd/nand/vf610_nfc.c
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -511,21 +511,6 @@ static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>  	vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
>  }
>  
> -/* Count the number of 0's in buff up to max_bits */
> -static inline int count_written_bits(uint8_t *buff, int size, int max_bits)
> -{
> -	uint32_t *buff32 = (uint32_t *)buff;
> -	int k, written_bits = 0;
> -
> -	for (k = 0; k < (size / 4); k++) {
> -		written_bits += hweight32(~buff32[k]);
> -		if (unlikely(written_bits > max_bits))
> -			break;
> -	}
> -
> -	return written_bits;
> -}
> -
>  static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  					 uint8_t *oob, int page)
>  {



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

This reworks the driver to make use of ->exec_op() callback. The
command sequencer of the VF610 NFC aligns well with the new ops
interface.

The ops are translated to a NFC command code while filling the
necessary registers. Instead of using the special status and
read id command codes (which require the status/id form special
registers) the driver now uses the main data buffer for all
commands. This simplifies the driver as no special casing is
needed.

For control data (status byte, id bytes and parameter page) the
driver needs to reverse byte order for little endian CPUs since
the controller seems to store the bytes in big endian order in
the data buffer.

The current state seems to pass MTD tests on a Colibri VF61.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 267 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index 2fa61cbdbaf7..eae95877422d 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -74,6 +74,21 @@
 #define RESET_CMD_CODE			0x4040
 #define STATUS_READ_CMD_CODE		0x4068
 
+/* NFC_CMD2[CODE] controller cycle bit masks */
+#define COMMAND_CMD_BYTE1		BIT(14)
+#define COMMAND_CAR_BYTE1		BIT(13)
+#define COMMAND_CAR_BYTE2		BIT(12)
+#define COMMAND_RAR_BYTE1		BIT(11)
+#define COMMAND_RAR_BYTE2		BIT(10)
+#define COMMAND_RAR_BYTE3		BIT(9)
+#define COMMAND_WRITE_DATA		BIT(8)
+#define COMMAND_CMD_BYTE2		BIT(7)
+#define COMMAND_RB_HANDSHAKE		BIT(6)
+#define COMMAND_READ_DATA		BIT(5)
+#define COMMAND_CMD_BYTE3		BIT(4)
+#define COMMAND_READ_STATUS		BIT(3)
+#define COMMAND_READ_ID			BIT(2)
+
 /* NFC ECC mode define */
 #define ECC_BYPASS			0
 #define ECC_45_BYTE			6
@@ -97,10 +112,14 @@
 /* NFC_COL_ADDR Field */
 #define COL_ADDR_MASK				0x0000FFFF
 #define COL_ADDR_SHIFT				0
+#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
+
 
 /* NFC_ROW_ADDR Field */
 #define ROW_ADDR_MASK				0x00FFFFFF
 #define ROW_ADDR_SHIFT				0
+#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
+
 #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
 #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
 #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
@@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
 	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
 }
 
+static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
+{
+	return container_of(chip, struct vf610_nfc, chip);
+}
+
 static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
 {
 	return readl(nfc->regs + reg);
@@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
 	return 1;
 }
 
+static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
+{
+	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
+			    COL_ADDR_SHIFT, col);
+
+	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
+			    ROW_ADDR_SHIFT, row);
+
+	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
+	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
+	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
+
+	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
+		col, row, cmd1, cmd2, trfr_sz);
+
+	vf610_nfc_done(nfc);
+}
+
+static inline const struct nand_op_instr *vf610_get_next_instr(
+	const struct nand_subop *subop, int *op_id)
+{
+	if (*op_id + 1 >= subop->ninstrs)
+		return NULL;
+
+	(*op_id)++;
+
+	return &subop->instrs[*op_id];
+}
+
+static int vf610_nfc_cmd(struct nand_chip *chip,
+				const struct nand_subop *subop)
+{
+	const struct nand_op_instr *instr;
+	struct vf610_nfc *nfc = chip_to_nfc(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	int op_id = -1, addr = 0, trfr_sz = 0;
+	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
+
+	/* Some ops are optional, but they need to be in order */
+	instr = vf610_get_next_instr(subop, &op_id);
+	if (!instr)
+		return -EINVAL;
+
+	if (instr && instr->type == NAND_OP_CMD_INSTR) {
+		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
+		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
+		code |= COMMAND_CMD_BYTE1;
+
+		instr = vf610_get_next_instr(subop, &op_id);
+	}
+
+	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
+
+		if (instr->ctx.addr.naddrs > 1) {
+			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
+			code |= COMMAND_CAR_BYTE1;
+
+			if (mtd->writesize > 512) {
+				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
+				code |= COMMAND_CAR_BYTE2;
+			}
+		}
+
+		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
+		code |= COMMAND_RAR_BYTE1;
+		if (addr < instr->ctx.addr.naddrs) {
+			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
+			code |= COMMAND_RAR_BYTE2;
+		}
+		if (addr < instr->ctx.addr.naddrs) {
+			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
+			code |= COMMAND_RAR_BYTE3;
+		}
+
+		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
+
+		instr = vf610_get_next_instr(subop, &op_id);
+	}
+
+	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
+		int len = nand_subop_get_data_len(subop, op_id);
+		int offset = nand_subop_get_data_start_off(subop, op_id);
+
+		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
+
+		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
+				 instr->ctx.data.buf.in + offset,
+				 len);
+		code |= COMMAND_WRITE_DATA;
+		trfr_sz += len;
+
+		instr = vf610_get_next_instr(subop, &op_id);
+	}
+
+	if (instr && instr->type == NAND_OP_CMD_INSTR) {
+		cmd1 |= instr->ctx.cmd.opcode << CMD_BYTE2_SHIFT;
+		code |= COMMAND_CMD_BYTE2;
+
+		instr = vf610_get_next_instr(subop, &op_id);
+	}
+
+	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
+		//dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n",
+		//		  instr->ctx.waitrdy.timeout_ms);
+		code |= COMMAND_RB_HANDSHAKE;
+
+		instr = vf610_get_next_instr(subop, &op_id);
+	}
+
+	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
+		int len = nand_subop_get_data_len(subop, op_id);
+		code |= COMMAND_READ_DATA;
+		trfr_sz += len;
+	}
+
+	cmd2 |= code << CMD_CODE_SHIFT;
+
+	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
+	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
+
+	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
+		int len = nand_subop_get_data_len(subop, op_id);
+		int offset = nand_subop_get_data_start_off(subop, op_id);
+		bool fix_byte_order = false;
+
+#ifdef __LITTLE_ENDIAN
+		fix_byte_order = true;
+#endif
+		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
+			instr->ctx.data.force_8bit , len, offset);
+
+		/*
+		 * HACK: force_8bit indicates reading of the parameter, status
+		 * or id data. The controllers seems to store data in big endian
+		 * byte order to the buffers. Reverse on little endian machines.
+		 */
+		if (instr->ctx.data.force_8bit && fix_byte_order) {
+			u8 *buf = instr->ctx.data.buf.in;
+
+			while (len--) {
+				int c = offset ^ 0x3;
+
+				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
+				buf++; offset++;
+			}
+		} else {
+			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
+					 nfc->regs + NFC_MAIN_AREA(0) + offset,
+					 len);
+		}
+	}
+
+	return 0;
+}
+
+static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
+	NAND_OP_PARSER_PATTERN(
+		vf610_nfc_cmd,
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
+		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
+		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
+	);
+
+static int vf610_nfc_exec_op(struct nand_chip *chip,
+			     const struct nand_operation *op,
+			     bool check_only)
+{
+	struct vf610_nfc *nfc = chip_to_nfc(chip);
+
+	dev_dbg(nfc->dev, "exec_op, opcode 0x%02x\n", op->instrs[0].ctx.cmd.opcode);
+
+	return nand_op_parser_exec_op(chip, &vf610_nfc_op_parser, op, check_only);
+}
+
+
 /*
  * This function supports Vybrid only (MPC5125 would have full RB and four CS)
  */
@@ -527,8 +729,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
 		return ecc_count;
 
 	/* Read OOB without ECC unit enabled */
-	vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page);
-	vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
+	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
 
 	/*
 	 * On an erased page, bit count (including OOB) should be zero or
@@ -542,12 +743,42 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
 static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 				uint8_t *buf, int oob_required, int page)
 {
-	int eccsize = chip->ecc.size;
+	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+	int trfr_sz = mtd->writesize + mtd->oobsize;
+	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
 	int stat;
 
-	nand_read_page_op(chip, page, 0, buf, eccsize);
+	cmd2 |= NAND_CMD_READ0 << CMD_BYTE1_SHIFT;
+	code |= COMMAND_CMD_BYTE1;
+
+	code |= COMMAND_CAR_BYTE1;
+	code |= COMMAND_CAR_BYTE2;
+
+	row = ROW_ADDR(0, page & 0xff);
+	code |= COMMAND_RAR_BYTE1;
+	row |= ROW_ADDR(1, page >> 8);
+	code |= COMMAND_RAR_BYTE2;
+
+	if (chip->options & NAND_ROW_ADDR_3) {
+		row |= ROW_ADDR(2, page >> 16);
+		code |= COMMAND_RAR_BYTE3;
+	}
+
+	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
+	code |= COMMAND_CMD_BYTE2;
+
+	code |= COMMAND_RB_HANDSHAKE;
+	code |= COMMAND_READ_DATA;
+	cmd2 |= code << CMD_CODE_SHIFT;
+
+	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
+	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
+
+	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
 	if (oob_required)
-		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
+		vf610_nfc_memcpy(chip->oob_poi,
+				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
+				 mtd->oobsize);
 
 	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
 
@@ -564,16 +795,39 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 				const uint8_t *buf, int oob_required, int page)
 {
 	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+	int trfr_sz = mtd->writesize + mtd->oobsize;
+	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
+	int ret = 0;
+
+	cmd2 |= NAND_CMD_SEQIN << CMD_BYTE1_SHIFT;
+	code |= COMMAND_CMD_BYTE1;
+
+	code |= COMMAND_CAR_BYTE1;
+	code |= COMMAND_CAR_BYTE2;
+
+	row = ROW_ADDR(0, page & 0xff);
+	code |= COMMAND_RAR_BYTE1;
+	row |= ROW_ADDR(1, page >> 8);
+	code |= COMMAND_RAR_BYTE2;
+	if (chip->options & NAND_ROW_ADDR_3) {
+		row |= ROW_ADDR(2, page >> 16);
+		code |= COMMAND_RAR_BYTE3;
+	}
 
-	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
-	if (oob_required)
-		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
+	cmd1 |= NAND_CMD_PAGEPROG << CMD_BYTE2_SHIFT;
+	code |= COMMAND_CMD_BYTE2;
+
+	code |= COMMAND_WRITE_DATA;
+
+	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0), buf, mtd->writesize);
+
+	code |= COMMAND_RB_HANDSHAKE;
+	cmd2 |= code << CMD_CODE_SHIFT;
 
-	/* Always write whole page including OOB due to HW ECC */
-	nfc->use_hw_ecc = true;
-	nfc->write_sz = mtd->writesize + mtd->oobsize;
+	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
+	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
 
-	return nand_prog_page_end_op(chip);
+	return ret;
 }
 
 static const struct of_device_id vf610_nfc_dt_ids[] = {
@@ -686,6 +940,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
 	chip->read_word = vf610_nfc_read_word;
 	chip->read_buf = vf610_nfc_read_buf;
 	chip->write_buf = vf610_nfc_write_buf;
+	chip->exec_op = vf610_nfc_exec_op;
 	chip->select_chip = vf610_nfc_select_chip;
 	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
 	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
-- 
2.16.1

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Miquel Raynal]

Hi Stefan,

Thanks for splitting the series, it is much easier to review.

On Fri,  9 Feb 2018 00:59:20 +0100, Stefan Agner <stefan@agner.ch>
wrote:

> This reworks the driver to make use of ->exec_op() callback. The
> command sequencer of the VF610 NFC aligns well with the new ops
> interface.
> 
> The ops are translated to a NFC command code while filling the
> necessary registers. Instead of using the special status and
> read id command codes (which require the status/id form special
> registers) the driver now uses the main data buffer for all
> commands. This simplifies the driver as no special casing is
> needed.
> 
> For control data (status byte, id bytes and parameter page) the
> driver needs to reverse byte order for little endian CPUs since
> the controller seems to store the bytes in big endian order in
> the data buffer.
> 
> The current state seems to pass MTD tests on a Colibri VF61.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 267 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> index 2fa61cbdbaf7..eae95877422d 100644
> --- a/drivers/mtd/nand/vf610_nfc.c
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -74,6 +74,21 @@
>  #define RESET_CMD_CODE			0x4040
>  #define STATUS_READ_CMD_CODE		0x4068
>  
> +/* NFC_CMD2[CODE] controller cycle bit masks */
> +#define COMMAND_CMD_BYTE1		BIT(14)
> +#define COMMAND_CAR_BYTE1		BIT(13)
> +#define COMMAND_CAR_BYTE2		BIT(12)
> +#define COMMAND_RAR_BYTE1		BIT(11)
> +#define COMMAND_RAR_BYTE2		BIT(10)
> +#define COMMAND_RAR_BYTE3		BIT(9)
> +#define COMMAND_WRITE_DATA		BIT(8)
> +#define COMMAND_CMD_BYTE2		BIT(7)
> +#define COMMAND_RB_HANDSHAKE		BIT(6)
> +#define COMMAND_READ_DATA		BIT(5)
> +#define COMMAND_CMD_BYTE3		BIT(4)
> +#define COMMAND_READ_STATUS		BIT(3)
> +#define COMMAND_READ_ID			BIT(2)
> +
>  /* NFC ECC mode define */
>  #define ECC_BYPASS			0
>  #define ECC_45_BYTE			6
> @@ -97,10 +112,14 @@
>  /* NFC_COL_ADDR Field */
>  #define COL_ADDR_MASK				0x0000FFFF
>  #define COL_ADDR_SHIFT				0
> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> +
>  
>  /* NFC_ROW_ADDR Field */
>  #define ROW_ADDR_MASK				0x00FFFFFF
>  #define ROW_ADDR_SHIFT				0
> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> +
>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
>  }
>  
> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
> +{
> +	return container_of(chip, struct vf610_nfc, chip);
> +}
> +
>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
>  {
>  	return readl(nfc->regs + reg);
> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
>  	return 1;
>  }
>  
> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
> +{
> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> +			    COL_ADDR_SHIFT, col);
> +
> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> +			    ROW_ADDR_SHIFT, row);
> +
> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
> +
> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
> +		col, row, cmd1, cmd2, trfr_sz);
> +
> +	vf610_nfc_done(nfc);
> +}
> +
> +static inline const struct nand_op_instr *vf610_get_next_instr(
> +	const struct nand_subop *subop, int *op_id)
> +{
> +	if (*op_id + 1 >= subop->ninstrs)
> +		return NULL;
> +
> +	(*op_id)++;
> +
> +	return &subop->instrs[*op_id];
> +}
> +
> +static int vf610_nfc_cmd(struct nand_chip *chip,
> +				const struct nand_subop *subop)
> +{
> +	const struct nand_op_instr *instr;
> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int op_id = -1, addr = 0, trfr_sz = 0;
> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> +
> +	/* Some ops are optional, but they need to be in order */
> +	instr = vf610_get_next_instr(subop, &op_id);
> +	if (!instr)
> +		return -EINVAL;

Maybe here you don't need to call get_next_instr but just derive the
pointer from the subop parameter?

> +
> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
> +		code |= COMMAND_CMD_BYTE1;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> +
> +		if (instr->ctx.addr.naddrs > 1) {
> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_CAR_BYTE1;
> +
> +			if (mtd->writesize > 512) {
> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
> +				code |= COMMAND_CAR_BYTE2;
> +			}
> +		}
> +
> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> +		code |= COMMAND_RAR_BYTE1;
> +		if (addr < instr->ctx.addr.naddrs) {
> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_RAR_BYTE2;
> +		}
> +		if (addr < instr->ctx.addr.naddrs) {
> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_RAR_BYTE3;
> +		}
> +
> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> +		int len = nand_subop_get_data_len(subop, op_id);
> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> +
> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> +
> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> +				 instr->ctx.data.buf.in + offset,
> +				 len);
> +		code |= COMMAND_WRITE_DATA;
> +		trfr_sz += len;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> +		cmd1 |= instr->ctx.cmd.opcode << CMD_BYTE2_SHIFT;
> +		code |= COMMAND_CMD_BYTE2;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
> +		//dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n",
> +		//		  instr->ctx.waitrdy.timeout_ms);

Debug comment   ^

> +		code |= COMMAND_RB_HANDSHAKE;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> +		int len = nand_subop_get_data_len(subop, op_id);
> +		code |= COMMAND_READ_DATA;
> +		trfr_sz += len;
> +	}
> +
> +	cmd2 |= code << CMD_CODE_SHIFT;
> +
> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> +
> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> +		int len = nand_subop_get_data_len(subop, op_id);
> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> +		bool fix_byte_order = false;
> +
> +#ifdef __LITTLE_ENDIAN
> +		fix_byte_order = true;
> +#endif
> +		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
> +			instr->ctx.data.force_8bit , len, offset);
> +
> +		/*
> +		 * HACK: force_8bit indicates reading of the parameter, status
> +		 * or id data. The controllers seems to store data in big endian
> +		 * byte order to the buffers. Reverse on little endian machines.
> +		 */
> +		if (instr->ctx.data.force_8bit && fix_byte_order) {
> +			u8 *buf = instr->ctx.data.buf.in;
> +
> +			while (len--) {
> +				int c = offset ^ 0x3;
> +
> +				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
> +				buf++; offset++;
> +			}
> +		} else {
> +			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
> +					 nfc->regs + NFC_MAIN_AREA(0) + offset,
> +					 len);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
> +	NAND_OP_PARSER_PATTERN(
> +		vf610_nfc_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),

Is this '5 address cycles' a real limit of the controller?

> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),

If PAGE_2K is 2048, you should use SZ_2K instead.

> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
> +	);

Actually, because this driver is very sequential in its way of handling
the instructions, you should split the *_exec_op() function into
smaller helpers, and declare one parser pattern per helper. I'm sure it
will be very easy to do.

> +
> +static int vf610_nfc_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op,
> +			     bool check_only)
> +{
> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> +
> +	dev_dbg(nfc->dev, "exec_op, opcode 0x%02x\n", op->instrs[0].ctx.cmd.opcode);
> +
> +	return nand_op_parser_exec_op(chip, &vf610_nfc_op_parser, op, check_only);
> +}
> +
> +
>  /*
>   * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>   */
> @@ -527,8 +729,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  		return ecc_count;
>  
>  	/* Read OOB without ECC unit enabled */
> -	vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page);
> -	vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> +	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
>  
>  	/*
>  	 * On an erased page, bit count (including OOB) should be zero or
> @@ -542,12 +743,42 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  				uint8_t *buf, int oob_required, int page)
>  {
> -	int eccsize = chip->ecc.size;
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	int trfr_sz = mtd->writesize + mtd->oobsize;
> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>  	int stat;
>  
> -	nand_read_page_op(chip, page, 0, buf, eccsize);
> +	cmd2 |= NAND_CMD_READ0 << CMD_BYTE1_SHIFT;
> +	code |= COMMAND_CMD_BYTE1;
> +
> +	code |= COMMAND_CAR_BYTE1;
> +	code |= COMMAND_CAR_BYTE2;
> +
> +	row = ROW_ADDR(0, page & 0xff);
> +	code |= COMMAND_RAR_BYTE1;
> +	row |= ROW_ADDR(1, page >> 8);
> +	code |= COMMAND_RAR_BYTE2;
> +
> +	if (chip->options & NAND_ROW_ADDR_3) {
> +		row |= ROW_ADDR(2, page >> 16);
> +		code |= COMMAND_RAR_BYTE3;
> +	}
> +
> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
> +	code |= COMMAND_CMD_BYTE2;
> +
> +	code |= COMMAND_RB_HANDSHAKE;
> +	code |= COMMAND_READ_DATA;
> +	cmd2 |= code << CMD_CODE_SHIFT;

I don't mind if you want to split all the '|=' flagging work in
multiple lines but perhaps some comments would be welcome too if there
is a particular logic.

> +
> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> +
> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
>  	if (oob_required)
> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +		vf610_nfc_memcpy(chip->oob_poi,
> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
> +				 mtd->oobsize);
>  
>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
>  
> @@ -564,16 +795,39 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  				const uint8_t *buf, int oob_required, int page)
>  {
>  	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	int trfr_sz = mtd->writesize + mtd->oobsize;
> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> +	int ret = 0;
> +
> +	cmd2 |= NAND_CMD_SEQIN << CMD_BYTE1_SHIFT;
> +	code |= COMMAND_CMD_BYTE1;
> +
> +	code |= COMMAND_CAR_BYTE1;
> +	code |= COMMAND_CAR_BYTE2;
> +
> +	row = ROW_ADDR(0, page & 0xff);
> +	code |= COMMAND_RAR_BYTE1;
> +	row |= ROW_ADDR(1, page >> 8);
> +	code |= COMMAND_RAR_BYTE2;
> +	if (chip->options & NAND_ROW_ADDR_3) {
> +		row |= ROW_ADDR(2, page >> 16);
> +		code |= COMMAND_RAR_BYTE3;
> +	}
>  
> -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
> -	if (oob_required)
> -		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +	cmd1 |= NAND_CMD_PAGEPROG << CMD_BYTE2_SHIFT;
> +	code |= COMMAND_CMD_BYTE2;
> +
> +	code |= COMMAND_WRITE_DATA;
> +
> +	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0), buf, mtd->writesize);
> +
> +	code |= COMMAND_RB_HANDSHAKE;
> +	cmd2 |= code << CMD_CODE_SHIFT;

Same here.

>  
> -	/* Always write whole page including OOB due to HW ECC */
> -	nfc->use_hw_ecc = true;
> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>  
> -	return nand_prog_page_end_op(chip);
> +	return ret;
>  }
>  
>  static const struct of_device_id vf610_nfc_dt_ids[] = {
> @@ -686,6 +940,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
>  	chip->read_word = vf610_nfc_read_word;
>  	chip->read_buf = vf610_nfc_read_buf;
>  	chip->write_buf = vf610_nfc_write_buf;
> +	chip->exec_op = vf610_nfc_exec_op;
>  	chip->select_chip = vf610_nfc_select_chip;
>  	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
>  	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;

This is in a good shape :)

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 09.02.2018 09:20, Miquel Raynal wrote:
> Hi Stefan,
> 
> Thanks for splitting the series, it is much easier to review.
> 
> On Fri,  9 Feb 2018 00:59:20 +0100, Stefan Agner <stefan@agner.ch>
> wrote:
> 
>> This reworks the driver to make use of ->exec_op() callback. The
>> command sequencer of the VF610 NFC aligns well with the new ops
>> interface.
>>
>> The ops are translated to a NFC command code while filling the
>> necessary registers. Instead of using the special status and
>> read id command codes (which require the status/id form special
>> registers) the driver now uses the main data buffer for all
>> commands. This simplifies the driver as no special casing is
>> needed.
>>
>> For control data (status byte, id bytes and parameter page) the
>> driver needs to reverse byte order for little endian CPUs since
>> the controller seems to store the bytes in big endian order in
>> the data buffer.
>>
>> The current state seems to pass MTD tests on a Colibri VF61.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 267 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> index 2fa61cbdbaf7..eae95877422d 100644
>> --- a/drivers/mtd/nand/vf610_nfc.c
>> +++ b/drivers/mtd/nand/vf610_nfc.c
>> @@ -74,6 +74,21 @@
>>  #define RESET_CMD_CODE			0x4040
>>  #define STATUS_READ_CMD_CODE		0x4068
>>
>> +/* NFC_CMD2[CODE] controller cycle bit masks */
>> +#define COMMAND_CMD_BYTE1		BIT(14)
>> +#define COMMAND_CAR_BYTE1		BIT(13)
>> +#define COMMAND_CAR_BYTE2		BIT(12)
>> +#define COMMAND_RAR_BYTE1		BIT(11)
>> +#define COMMAND_RAR_BYTE2		BIT(10)
>> +#define COMMAND_RAR_BYTE3		BIT(9)
>> +#define COMMAND_WRITE_DATA		BIT(8)
>> +#define COMMAND_CMD_BYTE2		BIT(7)
>> +#define COMMAND_RB_HANDSHAKE		BIT(6)
>> +#define COMMAND_READ_DATA		BIT(5)
>> +#define COMMAND_CMD_BYTE3		BIT(4)
>> +#define COMMAND_READ_STATUS		BIT(3)
>> +#define COMMAND_READ_ID			BIT(2)
>> +
>>  /* NFC ECC mode define */
>>  #define ECC_BYPASS			0
>>  #define ECC_45_BYTE			6
>> @@ -97,10 +112,14 @@
>>  /* NFC_COL_ADDR Field */
>>  #define COL_ADDR_MASK				0x0000FFFF
>>  #define COL_ADDR_SHIFT				0
>> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
>> +
>>
>>  /* NFC_ROW_ADDR Field */
>>  #define ROW_ADDR_MASK				0x00FFFFFF
>>  #define ROW_ADDR_SHIFT				0
>> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
>> +
>>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
>>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
>>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
>> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
>>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
>>  }
>>
>> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
>> +{
>> +	return container_of(chip, struct vf610_nfc, chip);
>> +}
>> +
>>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
>>  {
>>  	return readl(nfc->regs + reg);
>> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
>>  	return 1;
>>  }
>>
>> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
>> +{
>> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
>> +			    COL_ADDR_SHIFT, col);
>> +
>> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
>> +			    ROW_ADDR_SHIFT, row);
>> +
>> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
>> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
>> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
>> +
>> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
>> +		col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	vf610_nfc_done(nfc);
>> +}
>> +
>> +static inline const struct nand_op_instr *vf610_get_next_instr(
>> +	const struct nand_subop *subop, int *op_id)
>> +{
>> +	if (*op_id + 1 >= subop->ninstrs)
>> +		return NULL;
>> +
>> +	(*op_id)++;
>> +
>> +	return &subop->instrs[*op_id];
>> +}
>> +
>> +static int vf610_nfc_cmd(struct nand_chip *chip,
>> +				const struct nand_subop *subop)
>> +{
>> +	const struct nand_op_instr *instr;
>> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	int op_id = -1, addr = 0, trfr_sz = 0;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> +
>> +	/* Some ops are optional, but they need to be in order */
>> +	instr = vf610_get_next_instr(subop, &op_id);
>> +	if (!instr)
>> +		return -EINVAL;
> 
> Maybe here you don't need to call get_next_instr but just derive the
> pointer from the subop parameter?
> 

op_id needs to be incremented too. I know that helper isn't exactly
pretty, but I need a pointer to the instruction as well as its id within
instrs for helpers such as nand_subop_get_data_len...

>> +
>> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
>> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
>> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
>> +		code |= COMMAND_CMD_BYTE1;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
>> +
>> +		if (instr->ctx.addr.naddrs > 1) {
>> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> +			code |= COMMAND_CAR_BYTE1;
>> +
>> +			if (mtd->writesize > 512) {
>> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> +				code |= COMMAND_CAR_BYTE2;
>> +			}
>> +		}
>> +
>> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> +		code |= COMMAND_RAR_BYTE1;
>> +		if (addr < instr->ctx.addr.naddrs) {
>> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> +			code |= COMMAND_RAR_BYTE2;
>> +		}
>> +		if (addr < instr->ctx.addr.naddrs) {
>> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
>> +			code |= COMMAND_RAR_BYTE3;
>> +		}
>> +
>> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
>> +		int len = nand_subop_get_data_len(subop, op_id);
>> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> +
>> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
>> +
>> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
>> +				 instr->ctx.data.buf.in + offset,
>> +				 len);
>> +		code |= COMMAND_WRITE_DATA;
>> +		trfr_sz += len;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
>> +		cmd1 |= instr->ctx.cmd.opcode << CMD_BYTE2_SHIFT;
>> +		code |= COMMAND_CMD_BYTE2;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
>> +		//dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n",
>> +		//		  instr->ctx.waitrdy.timeout_ms);
> 
> Debug comment   ^
> 
>> +		code |= COMMAND_RB_HANDSHAKE;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
>> +		int len = nand_subop_get_data_len(subop, op_id);
>> +		code |= COMMAND_READ_DATA;
>> +		trfr_sz += len;
>> +	}
>> +
>> +	cmd2 |= code << CMD_CODE_SHIFT;
>> +
>> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
>> +		int len = nand_subop_get_data_len(subop, op_id);
>> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> +		bool fix_byte_order = false;
>> +
>> +#ifdef __LITTLE_ENDIAN
>> +		fix_byte_order = true;
>> +#endif
>> +		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
>> +			instr->ctx.data.force_8bit , len, offset);
>> +
>> +		/*
>> +		 * HACK: force_8bit indicates reading of the parameter, status
>> +		 * or id data. The controllers seems to store data in big endian
>> +		 * byte order to the buffers. Reverse on little endian machines.
>> +		 */
>> +		if (instr->ctx.data.force_8bit && fix_byte_order) {
>> +			u8 *buf = instr->ctx.data.buf.in;
>> +
>> +			while (len--) {
>> +				int c = offset ^ 0x3;
>> +
>> +				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
>> +				buf++; offset++;
>> +			}
>> +		} else {
>> +			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
>> +					 nfc->regs + NFC_MAIN_AREA(0) + offset,
>> +					 len);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
>> +	NAND_OP_PARSER_PATTERN(
>> +		vf610_nfc_cmd,
>> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> 
> Is this '5 address cycles' a real limit of the controller?
> 

Yes 2 bytes of column address and 3 bytes of row address. At least, that
is how they label it, not sure whether it is really relevant.

>> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
> 
> If PAGE_2K is 2048, you should use SZ_2K instead.
> 

PAGE_2K is used throughout the driver, maybe I should replace
everywhere?

Or #define PAGE_2K to be SZ_2K?

>> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
>> +	);
> 
> Actually, because this driver is very sequential in its way of handling
> the instructions, you should split the *_exec_op() function into
> smaller helpers, and declare one parser pattern per helper. I'm sure it
> will be very easy to do.
> 

Hm, if I would do that, then I would have to trigger a command in the
controller for each individual step... I guess this would slow down
things quite significantly.

Afaik, the controls strength is that it can handle a bunch of commands
in sequence on its own and we should make use of it as much as possible.

While every step in a command sequence can be optionally
enabled/disabled through the Command Code bitfield, the commands order
however is predefined. And that can be nicely reflected with this single
vf610_nfc_op_parser.

See also chapter 10.3.4.7 Flash Command Code Description of the
(publicly available) VFxxx Controller Reference Manual.


>> +
>> +static int vf610_nfc_exec_op(struct nand_chip *chip,
>> +			     const struct nand_operation *op,
>> +			     bool check_only)
>> +{
>> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
>> +
>> +	dev_dbg(nfc->dev, "exec_op, opcode 0x%02x\n", op->instrs[0].ctx.cmd.opcode);
>> +
>> +	return nand_op_parser_exec_op(chip, &vf610_nfc_op_parser, op, check_only);
>> +}
>> +
>> +
>>  /*
>>   * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>>   */
>> @@ -527,8 +729,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>>  		return ecc_count;
>>
>>  	/* Read OOB without ECC unit enabled */
>> -	vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page);
>> -	vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
>> +	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
>>
>>  	/*
>>  	 * On an erased page, bit count (including OOB) should be zero or
>> @@ -542,12 +743,42 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>>  static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>>  				uint8_t *buf, int oob_required, int page)
>>  {
>> -	int eccsize = chip->ecc.size;
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int trfr_sz = mtd->writesize + mtd->oobsize;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>>  	int stat;
>>
>> -	nand_read_page_op(chip, page, 0, buf, eccsize);
>> +	cmd2 |= NAND_CMD_READ0 << CMD_BYTE1_SHIFT;
>> +	code |= COMMAND_CMD_BYTE1;
>> +
>> +	code |= COMMAND_CAR_BYTE1;
>> +	code |= COMMAND_CAR_BYTE2;
>> +
>> +	row = ROW_ADDR(0, page & 0xff);
>> +	code |= COMMAND_RAR_BYTE1;
>> +	row |= ROW_ADDR(1, page >> 8);
>> +	code |= COMMAND_RAR_BYTE2;
>> +
>> +	if (chip->options & NAND_ROW_ADDR_3) {
>> +		row |= ROW_ADDR(2, page >> 16);
>> +		code |= COMMAND_RAR_BYTE3;
>> +	}
>> +
>> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
>> +	code |= COMMAND_CMD_BYTE2;
>> +
>> +	code |= COMMAND_RB_HANDSHAKE;
>> +	code |= COMMAND_READ_DATA;
>> +	cmd2 |= code << CMD_CODE_SHIFT;
> 
> I don't mind if you want to split all the '|=' flagging work in
> multiple lines but perhaps some comments would be welcome too if there
> is a particular logic.
> 

Sound reasonable. I was also thinking of merging some of that code in
function shared for read/write_page.

>> +
>> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
>>  	if (oob_required)
>> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +		vf610_nfc_memcpy(chip->oob_poi,
>> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
>> +				 mtd->oobsize);
>>
>>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
>>
>> @@ -564,16 +795,39 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>  				const uint8_t *buf, int oob_required, int page)
>>  {
>>  	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int trfr_sz = mtd->writesize + mtd->oobsize;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> +	int ret = 0;
>> +
>> +	cmd2 |= NAND_CMD_SEQIN << CMD_BYTE1_SHIFT;
>> +	code |= COMMAND_CMD_BYTE1;
>> +
>> +	code |= COMMAND_CAR_BYTE1;
>> +	code |= COMMAND_CAR_BYTE2;
>> +
>> +	row = ROW_ADDR(0, page & 0xff);
>> +	code |= COMMAND_RAR_BYTE1;
>> +	row |= ROW_ADDR(1, page >> 8);
>> +	code |= COMMAND_RAR_BYTE2;
>> +	if (chip->options & NAND_ROW_ADDR_3) {
>> +		row |= ROW_ADDR(2, page >> 16);
>> +		code |= COMMAND_RAR_BYTE3;
>> +	}
>>
>> -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
>> -	if (oob_required)
>> -		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +	cmd1 |= NAND_CMD_PAGEPROG << CMD_BYTE2_SHIFT;
>> +	code |= COMMAND_CMD_BYTE2;
>> +
>> +	code |= COMMAND_WRITE_DATA;
>> +
>> +	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0), buf, mtd->writesize);
>> +
>> +	code |= COMMAND_RB_HANDSHAKE;
>> +	cmd2 |= code << CMD_CODE_SHIFT;
> 
> Same here.
> 
>>
>> -	/* Always write whole page including OOB due to HW ECC */
>> -	nfc->use_hw_ecc = true;
>> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
>> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>>
>> -	return nand_prog_page_end_op(chip);
>> +	return ret;
>>  }
>>
>>  static const struct of_device_id vf610_nfc_dt_ids[] = {
>> @@ -686,6 +940,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
>>  	chip->read_word = vf610_nfc_read_word;
>>  	chip->read_buf = vf610_nfc_read_buf;
>>  	chip->write_buf = vf610_nfc_write_buf;
>> +	chip->exec_op = vf610_nfc_exec_op;
>>  	chip->select_chip = vf610_nfc_select_chip;
>>  	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
>>  	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
> 
> This is in a good shape :)
> 
> Thanks,
> Miquèl

Thanks!

--
Stefan

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Fri,  9 Feb 2018 00:59:20 +0100
Stefan Agner <stefan@agner.ch> wrote:

> This reworks the driver to make use of ->exec_op() callback. The
> command sequencer of the VF610 NFC aligns well with the new ops
> interface.
> 
> The ops are translated to a NFC command code while filling the
> necessary registers. Instead of using the special status and
> read id command codes (which require the status/id form special
> registers) the driver now uses the main data buffer for all
> commands. This simplifies the driver as no special casing is
> needed.
> 
> For control data (status byte, id bytes and parameter page) the
> driver needs to reverse byte order for little endian CPUs since
> the controller seems to store the bytes in big endian order in
> the data buffer.
> 
> The current state seems to pass MTD tests on a Colibri VF61.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 267 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> index 2fa61cbdbaf7..eae95877422d 100644
> --- a/drivers/mtd/nand/vf610_nfc.c
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -74,6 +74,21 @@
>  #define RESET_CMD_CODE			0x4040
>  #define STATUS_READ_CMD_CODE		0x4068
>  
> +/* NFC_CMD2[CODE] controller cycle bit masks */
> +#define COMMAND_CMD_BYTE1		BIT(14)
> +#define COMMAND_CAR_BYTE1		BIT(13)
> +#define COMMAND_CAR_BYTE2		BIT(12)
> +#define COMMAND_RAR_BYTE1		BIT(11)
> +#define COMMAND_RAR_BYTE2		BIT(10)
> +#define COMMAND_RAR_BYTE3		BIT(9)
> +#define COMMAND_WRITE_DATA		BIT(8)
> +#define COMMAND_CMD_BYTE2		BIT(7)
> +#define COMMAND_RB_HANDSHAKE		BIT(6)
> +#define COMMAND_READ_DATA		BIT(5)
> +#define COMMAND_CMD_BYTE3		BIT(4)
> +#define COMMAND_READ_STATUS		BIT(3)
> +#define COMMAND_READ_ID			BIT(2)
> +
>  /* NFC ECC mode define */
>  #define ECC_BYPASS			0
>  #define ECC_45_BYTE			6
> @@ -97,10 +112,14 @@
>  /* NFC_COL_ADDR Field */
>  #define COL_ADDR_MASK				0x0000FFFF
>  #define COL_ADDR_SHIFT				0
> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> +
>  
>  /* NFC_ROW_ADDR Field */
>  #define ROW_ADDR_MASK				0x00FFFFFF
>  #define ROW_ADDR_SHIFT				0
> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> +
>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
>  }
>  
> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
> +{
> +	return container_of(chip, struct vf610_nfc, chip);
> +}
> +
>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
>  {
>  	return readl(nfc->regs + reg);
> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
>  	return 1;
>  }
>  
> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)

Nitpick: please wrap the line to make checkpatch happy.

> +{
> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> +			    COL_ADDR_SHIFT, col);
> +
> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> +			    ROW_ADDR_SHIFT, row);
> +
> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
> +
> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
> +		col, row, cmd1, cmd2, trfr_sz);
> +
> +	vf610_nfc_done(nfc);
> +}
> +
> +static inline const struct nand_op_instr *vf610_get_next_instr(
> +	const struct nand_subop *subop, int *op_id)
> +{
> +	if (*op_id + 1 >= subop->ninstrs)
> +		return NULL;
> +
> +	(*op_id)++;
> +
> +	return &subop->instrs[*op_id];
> +}
> +
> +static int vf610_nfc_cmd(struct nand_chip *chip,
> +				const struct nand_subop *subop)
> +{
> +	const struct nand_op_instr *instr;
> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int op_id = -1, addr = 0, trfr_sz = 0;
> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> +
> +	/* Some ops are optional, but they need to be in order */
> +	instr = vf610_get_next_instr(subop, &op_id);
> +	if (!instr)
> +		return -EINVAL;
> +
> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
> +		code |= COMMAND_CMD_BYTE1;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> +

Hm, you're still checking the sequencing consistency here. This should
have been taken care by the core parser already, so really, this is not
needed.

> +		if (instr->ctx.addr.naddrs > 1) {
> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_CAR_BYTE1;
> +
> +			if (mtd->writesize > 512) {
> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
> +				code |= COMMAND_CAR_BYTE2;
> +			}

No, you shoudn't do that. Just put as many ADDR cycles as requested
without trying to figure out if they fit in the column or row field. We
really don't care here, as long as the ADDR cycles are issued on the
bus.

> +		}
> +
> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> +		code |= COMMAND_RAR_BYTE1;
> +		if (addr < instr->ctx.addr.naddrs) {
> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_RAR_BYTE2;
> +		}
> +		if (addr < instr->ctx.addr.naddrs) {
> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_RAR_BYTE3;
> +		}
> +
> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> +		int len = nand_subop_get_data_len(subop, op_id);
> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> +
> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> +
> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> +				 instr->ctx.data.buf.in + offset,
> +				 len);

I think you have the same endianness problem you have for the READ
path. For example, I doubt SET_FEATURES will work properly if you're
in LE. So I repeat my initial suggestion: always do the byte swapping
when you're transfering data to/from the SRAM from vf610_nfc_cmd()
and use vf610_nfc_memcpy() only in the ->read/write_page()
implementations. 

> +		code |= COMMAND_WRITE_DATA;
> +		trfr_sz += len;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> +		cmd1 |= instr->ctx.cmd.opcode << CMD_BYTE2_SHIFT;
> +		code |= COMMAND_CMD_BYTE2;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
> +		//dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n",
> +		//		  instr->ctx.waitrdy.timeout_ms);

I guess this should go away.

> +		code |= COMMAND_RB_HANDSHAKE;
> +
> +		instr = vf610_get_next_instr(subop, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> +		int len = nand_subop_get_data_len(subop, op_id);
> +		code |= COMMAND_READ_DATA;
> +		trfr_sz += len;
> +	}
> +
> +	cmd2 |= code << CMD_CODE_SHIFT;
> +
> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> +
> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> +		int len = nand_subop_get_data_len(subop, op_id);
> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> +		bool fix_byte_order = false;
> +
> +#ifdef __LITTLE_ENDIAN
> +		fix_byte_order = true;
> +#endif
> +		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
> +			instr->ctx.data.force_8bit , len, offset);
> +
> +		/*
> +		 * HACK: force_8bit indicates reading of the parameter, status
> +		 * or id data. The controllers seems to store data in big endian
> +		 * byte order to the buffers. Reverse on little endian machines.
> +		 */
> +		if (instr->ctx.data.force_8bit && fix_byte_order) {

I'm still convinced this is not the right solution to choose between
swap vs don't swap bytes. See my explanation above.

> +			u8 *buf = instr->ctx.data.buf.in;
> +
> +			while (len--) {
> +				int c = offset ^ 0x3;
> +
> +				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
> +				buf++; offset++;
> +			}
> +		} else {
> +			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
> +					 nfc->regs + NFC_MAIN_AREA(0) + offset,
> +					 len);
> +		}

I think the "copy to/from SRAM and swap bytes if needed" code should
should be move in dedicated functions.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
> +	NAND_OP_PARSER_PATTERN(
> +		vf610_nfc_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),

Are you sure you can do both a read and a write in a single pass? I
doubt this is the case, because the logic seems to share the same SRAM
for both operations, so I'd recommend splitting in 2 patterns:

	NAND_OP_PARSER_PATTERN(
                vf610_nfc_cmd,
                NAND_OP_PARSER_PAT_CMD_ELEM(true), 
                NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), 
                NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K +
	        OOB_MAX), NAND_OP_PARSER_PAT_CMD_ELEM(true),
                NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
        NAND_OP_PARSER_PATTERN(
                vf610_nfc_cmd,
                NAND_OP_PARSER_PAT_CMD_ELEM(true),
                NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
                NAND_OP_PARSER_PAT_CMD_ELEM(true),
                NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
                NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K +
	        OOB_MAX)),

> +	);

To sum-up, here is a diff [1] that applies on top of your patch and
illustrate what I have in mind.

> +
> +static int vf610_nfc_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op,
> +			     bool check_only)
> +{
> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> +
> +	dev_dbg(nfc->dev, "exec_op, opcode 0x%02x\n", op->instrs[0].ctx.cmd.opcode);
> +
> +	return nand_op_parser_exec_op(chip, &vf610_nfc_op_parser, op, check_only);
> +}
> +
> +
>  /*
>   * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>   */
> @@ -527,8 +729,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  		return ecc_count;
>  
>  	/* Read OOB without ECC unit enabled */
> -	vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page);
> -	vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> +	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
>  
>  	/*
>  	 * On an erased page, bit count (including OOB) should be zero or
> @@ -542,12 +743,42 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  				uint8_t *buf, int oob_required, int page)
>  {
> -	int eccsize = chip->ecc.size;
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	int trfr_sz = mtd->writesize + mtd->oobsize;
> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;

col is not needed here, just pass 0 to vf610_nfc_run() and you should
be good.

>  	int stat;
>  
> -	nand_read_page_op(chip, page, 0, buf, eccsize);
> +	cmd2 |= NAND_CMD_READ0 << CMD_BYTE1_SHIFT;
> +	code |= COMMAND_CMD_BYTE1;
> +
> +	code |= COMMAND_CAR_BYTE1;
> +	code |= COMMAND_CAR_BYTE2;
> +
> +	row = ROW_ADDR(0, page & 0xff);
> +	code |= COMMAND_RAR_BYTE1;
> +	row |= ROW_ADDR(1, page >> 8);
> +	code |= COMMAND_RAR_BYTE2;
> +
> +	if (chip->options & NAND_ROW_ADDR_3) {
> +		row |= ROW_ADDR(2, page >> 16);
> +		code |= COMMAND_RAR_BYTE3;
> +	}

The address setting could be simplified a bit:

	row = page;
	code |= COMMAND_CAR_BYTE1 | COMMAND_CAR_BYTE2 |
		COMMAND_RAR_BYTE1| COMMAND_RAR_BYTE2;
	if (chip->options & NAND_ROW_ADDR_3)
		code |= COMMAND_RAR_BYTE3;

> +
> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
> +	code |= COMMAND_CMD_BYTE2;
> +
> +	code |= COMMAND_RB_HANDSHAKE;
> +	code |= COMMAND_READ_DATA;
> +	cmd2 |= code << CMD_CODE_SHIFT;
> +
> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> +
> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
>  	if (oob_required)
> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +		vf610_nfc_memcpy(chip->oob_poi,
> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
> +				 mtd->oobsize);
>  
>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);

You should disable ECC here and not in vf610_nfc_cmd():

	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
	

>  
> @@ -564,16 +795,39 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  				const uint8_t *buf, int oob_required, int page)
>  {
>  	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	int trfr_sz = mtd->writesize + mtd->oobsize;
> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> +	int ret = 0;
> +
> +	cmd2 |= NAND_CMD_SEQIN << CMD_BYTE1_SHIFT;
> +	code |= COMMAND_CMD_BYTE1;
> +
> +	code |= COMMAND_CAR_BYTE1;
> +	code |= COMMAND_CAR_BYTE2;
> +
> +	row = ROW_ADDR(0, page & 0xff);
> +	code |= COMMAND_RAR_BYTE1;
> +	row |= ROW_ADDR(1, page >> 8);
> +	code |= COMMAND_RAR_BYTE2;
> +	if (chip->options & NAND_ROW_ADDR_3) {
> +		row |= ROW_ADDR(2, page >> 16);
> +		code |= COMMAND_RAR_BYTE3;
> +	}

See my comment in _read_page().

>  
> -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
> -	if (oob_required)
> -		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +	cmd1 |= NAND_CMD_PAGEPROG << CMD_BYTE2_SHIFT;
> +	code |= COMMAND_CMD_BYTE2;
> +
> +	code |= COMMAND_WRITE_DATA;
> +
> +	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0), buf, mtd->writesize);
> +
> +	code |= COMMAND_RB_HANDSHAKE;
> +	cmd2 |= code << CMD_CODE_SHIFT;
>  
> -	/* Always write whole page including OOB due to HW ECC */
> -	nfc->use_hw_ecc = true;
> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>  
> -	return nand_prog_page_end_op(chip);
> +	return ret;
>  }
>  
>  static const struct of_device_id vf610_nfc_dt_ids[] = {
> @@ -686,6 +940,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
>  	chip->read_word = vf610_nfc_read_word;
>  	chip->read_buf = vf610_nfc_read_buf;
>  	chip->write_buf = vf610_nfc_write_buf;
> +	chip->exec_op = vf610_nfc_exec_op;
>  	chip->select_chip = vf610_nfc_select_chip;
>  	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
>  	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;

That's all I have for now.

Regards,

Boris

[1]http://code.bulix.org/90iikz-278094

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 11.02.2018 11:54, Boris Brezillon wrote:
> On Fri,  9 Feb 2018 00:59:20 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
>> This reworks the driver to make use of ->exec_op() callback. The
>> command sequencer of the VF610 NFC aligns well with the new ops
>> interface.
>>
>> The ops are translated to a NFC command code while filling the
>> necessary registers. Instead of using the special status and
>> read id command codes (which require the status/id form special
>> registers) the driver now uses the main data buffer for all
>> commands. This simplifies the driver as no special casing is
>> needed.
>>
>> For control data (status byte, id bytes and parameter page) the
>> driver needs to reverse byte order for little endian CPUs since
>> the controller seems to store the bytes in big endian order in
>> the data buffer.
>>
>> The current state seems to pass MTD tests on a Colibri VF61.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 267 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> index 2fa61cbdbaf7..eae95877422d 100644
>> --- a/drivers/mtd/nand/vf610_nfc.c
>> +++ b/drivers/mtd/nand/vf610_nfc.c
>> @@ -74,6 +74,21 @@
>>  #define RESET_CMD_CODE			0x4040
>>  #define STATUS_READ_CMD_CODE		0x4068
>>
>> +/* NFC_CMD2[CODE] controller cycle bit masks */
>> +#define COMMAND_CMD_BYTE1		BIT(14)
>> +#define COMMAND_CAR_BYTE1		BIT(13)
>> +#define COMMAND_CAR_BYTE2		BIT(12)
>> +#define COMMAND_RAR_BYTE1		BIT(11)
>> +#define COMMAND_RAR_BYTE2		BIT(10)
>> +#define COMMAND_RAR_BYTE3		BIT(9)
>> +#define COMMAND_WRITE_DATA		BIT(8)
>> +#define COMMAND_CMD_BYTE2		BIT(7)
>> +#define COMMAND_RB_HANDSHAKE		BIT(6)
>> +#define COMMAND_READ_DATA		BIT(5)
>> +#define COMMAND_CMD_BYTE3		BIT(4)
>> +#define COMMAND_READ_STATUS		BIT(3)
>> +#define COMMAND_READ_ID			BIT(2)
>> +
>>  /* NFC ECC mode define */
>>  #define ECC_BYPASS			0
>>  #define ECC_45_BYTE			6
>> @@ -97,10 +112,14 @@
>>  /* NFC_COL_ADDR Field */
>>  #define COL_ADDR_MASK				0x0000FFFF
>>  #define COL_ADDR_SHIFT				0
>> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
>> +
>>
>>  /* NFC_ROW_ADDR Field */
>>  #define ROW_ADDR_MASK				0x00FFFFFF
>>  #define ROW_ADDR_SHIFT				0
>> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
>> +
>>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
>>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
>>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
>> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
>>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
>>  }
>>
>> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
>> +{
>> +	return container_of(chip, struct vf610_nfc, chip);
>> +}
>> +
>>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
>>  {
>>  	return readl(nfc->regs + reg);
>> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
>>  	return 1;
>>  }
>>
>> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
> 
> Nitpick: please wrap the line to make checkpatch happy.
> 

Ok.

>> +{
>> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
>> +			    COL_ADDR_SHIFT, col);
>> +
>> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
>> +			    ROW_ADDR_SHIFT, row);
>> +
>> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
>> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
>> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
>> +
>> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
>> +		col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	vf610_nfc_done(nfc);
>> +}
>> +
>> +static inline const struct nand_op_instr *vf610_get_next_instr(
>> +	const struct nand_subop *subop, int *op_id)
>> +{
>> +	if (*op_id + 1 >= subop->ninstrs)
>> +		return NULL;
>> +
>> +	(*op_id)++;
>> +
>> +	return &subop->instrs[*op_id];
>> +}
>> +
>> +static int vf610_nfc_cmd(struct nand_chip *chip,
>> +				const struct nand_subop *subop)
>> +{
>> +	const struct nand_op_instr *instr;
>> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	int op_id = -1, addr = 0, trfr_sz = 0;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> +
>> +	/* Some ops are optional, but they need to be in order */
>> +	instr = vf610_get_next_instr(subop, &op_id);
>> +	if (!instr)
>> +		return -EINVAL;
>> +
>> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
>> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
>> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
>> +		code |= COMMAND_CMD_BYTE1;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
>> +
> 
> Hm, you're still checking the sequencing consistency here. This should
> have been taken care by the core parser already, so really, this is not
> needed.
> 

Since those operations are optional, I have to check...

>> +		if (instr->ctx.addr.naddrs > 1) {
>> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> +			code |= COMMAND_CAR_BYTE1;
>> +
>> +			if (mtd->writesize > 512) {
>> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> +				code |= COMMAND_CAR_BYTE2;
>> +			}
> 
> No, you shoudn't do that. Just put as many ADDR cycles as requested
> without trying to figure out if they fit in the column or row field. We
> really don't care here, as long as the ADDR cycles are issued on the
> bus.
> 

Ok, so on a bus level column/row really does not matter? I wonder why
the controller makes a difference then? I remember that the controller
has auto increment feature, might that be the reason? Afaik we cannot
use such a feature currently...?

>> +		}
>> +
>> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> +		code |= COMMAND_RAR_BYTE1;
>> +		if (addr < instr->ctx.addr.naddrs) {
>> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> +			code |= COMMAND_RAR_BYTE2;
>> +		}
>> +		if (addr < instr->ctx.addr.naddrs) {
>> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
>> +			code |= COMMAND_RAR_BYTE3;
>> +		}
>> +
>> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
>> +		int len = nand_subop_get_data_len(subop, op_id);
>> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> +
>> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
>> +
>> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
>> +				 instr->ctx.data.buf.in + offset,
>> +				 len);
> 
> I think you have the same endianness problem you have for the READ
> path. For example, I doubt SET_FEATURES will work properly if you're
> in LE. So I repeat my initial suggestion: always do the byte swapping
> when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> and use vf610_nfc_memcpy() only in the ->read/write_page()
> implementations. 
> 

Hm, but doesn't that leads to wrong order of data when using e.g. raw
read/write page...?

>> +		code |= COMMAND_WRITE_DATA;
>> +		trfr_sz += len;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
>> +		cmd1 |= instr->ctx.cmd.opcode << CMD_BYTE2_SHIFT;
>> +		code |= COMMAND_CMD_BYTE2;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
>> +		//dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n",
>> +		//		  instr->ctx.waitrdy.timeout_ms);
> 
> I guess this should go away.
> 
>> +		code |= COMMAND_RB_HANDSHAKE;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
>> +		int len = nand_subop_get_data_len(subop, op_id);
>> +		code |= COMMAND_READ_DATA;
>> +		trfr_sz += len;
>> +	}
>> +
>> +	cmd2 |= code << CMD_CODE_SHIFT;
>> +
>> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
>> +		int len = nand_subop_get_data_len(subop, op_id);
>> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> +		bool fix_byte_order = false;
>> +
>> +#ifdef __LITTLE_ENDIAN
>> +		fix_byte_order = true;
>> +#endif
>> +		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
>> +			instr->ctx.data.force_8bit , len, offset);
>> +
>> +		/*
>> +		 * HACK: force_8bit indicates reading of the parameter, status
>> +		 * or id data. The controllers seems to store data in big endian
>> +		 * byte order to the buffers. Reverse on little endian machines.
>> +		 */
>> +		if (instr->ctx.data.force_8bit && fix_byte_order) {
> 
> I'm still convinced this is not the right solution to choose between
> swap vs don't swap bytes. See my explanation above.
> 
>> +			u8 *buf = instr->ctx.data.buf.in;
>> +
>> +			while (len--) {
>> +				int c = offset ^ 0x3;
>> +
>> +				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
>> +				buf++; offset++;
>> +			}
>> +		} else {
>> +			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
>> +					 nfc->regs + NFC_MAIN_AREA(0) + offset,
>> +					 len);
>> +		}
> 
> I think the "copy to/from SRAM and swap bytes if needed" code should
> should be move in dedicated functions.
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
>> +	NAND_OP_PARSER_PATTERN(
>> +		vf610_nfc_cmd,
>> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
>> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
>> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
> 
> Are you sure you can do both a read and a write in a single pass? I
> doubt this is the case, because the logic seems to share the same SRAM
> for both operations, so I'd recommend splitting in 2 patterns:
> 

Hm, yes I currently use the same buffer, so in the current state this
would not work. But the controller actually has four buffers, so I
*could* use different ones for reading and writing. But is reading and
writing in a single go something we need often that such an optimization
would make worthwhile?

> 	NAND_OP_PARSER_PATTERN(
>                 vf610_nfc_cmd,
>                 NAND_OP_PARSER_PAT_CMD_ELEM(true), 
>                 NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), 
>                 NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K +
> 	        OOB_MAX), NAND_OP_PARSER_PAT_CMD_ELEM(true),
>                 NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
>         NAND_OP_PARSER_PATTERN(
>                 vf610_nfc_cmd,
>                 NAND_OP_PARSER_PAT_CMD_ELEM(true),
>                 NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
>                 NAND_OP_PARSER_PAT_CMD_ELEM(true),
>                 NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>                 NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K +
> 	        OOB_MAX)),
> 
>> +	);
> 
> To sum-up, here is a diff [1] that applies on top of your patch and
> illustrate what I have in mind.
> 
>> +
>> +static int vf610_nfc_exec_op(struct nand_chip *chip,
>> +			     const struct nand_operation *op,
>> +			     bool check_only)
>> +{
>> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
>> +
>> +	dev_dbg(nfc->dev, "exec_op, opcode 0x%02x\n", op->instrs[0].ctx.cmd.opcode);
>> +
>> +	return nand_op_parser_exec_op(chip, &vf610_nfc_op_parser, op, check_only);
>> +}
>> +
>> +
>>  /*
>>   * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>>   */
>> @@ -527,8 +729,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>>  		return ecc_count;
>>
>>  	/* Read OOB without ECC unit enabled */
>> -	vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page);
>> -	vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
>> +	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
>>
>>  	/*
>>  	 * On an erased page, bit count (including OOB) should be zero or
>> @@ -542,12 +743,42 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>>  static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>>  				uint8_t *buf, int oob_required, int page)
>>  {
>> -	int eccsize = chip->ecc.size;
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int trfr_sz = mtd->writesize + mtd->oobsize;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> 
> col is not needed here, just pass 0 to vf610_nfc_run() and you should
> be good.
> 

Ok.

>>  	int stat;
>>
>> -	nand_read_page_op(chip, page, 0, buf, eccsize);
>> +	cmd2 |= NAND_CMD_READ0 << CMD_BYTE1_SHIFT;
>> +	code |= COMMAND_CMD_BYTE1;
>> +
>> +	code |= COMMAND_CAR_BYTE1;
>> +	code |= COMMAND_CAR_BYTE2;
>> +
>> +	row = ROW_ADDR(0, page & 0xff);
>> +	code |= COMMAND_RAR_BYTE1;
>> +	row |= ROW_ADDR(1, page >> 8);
>> +	code |= COMMAND_RAR_BYTE2;
>> +
>> +	if (chip->options & NAND_ROW_ADDR_3) {
>> +		row |= ROW_ADDR(2, page >> 16);
>> +		code |= COMMAND_RAR_BYTE3;
>> +	}
> 
> The address setting could be simplified a bit:
> 
> 	row = page;
> 	code |= COMMAND_CAR_BYTE1 | COMMAND_CAR_BYTE2 |
> 		COMMAND_RAR_BYTE1| COMMAND_RAR_BYTE2;
> 	if (chip->options & NAND_ROW_ADDR_3)
> 		code |= COMMAND_RAR_BYTE3;
> 

Agreed, and I will move that in a shared function since it is the same
for read/write.

>> +
>> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
>> +	code |= COMMAND_CMD_BYTE2;
>> +
>> +	code |= COMMAND_RB_HANDSHAKE;
>> +	code |= COMMAND_READ_DATA;
>> +	cmd2 |= code << CMD_CODE_SHIFT;
>> +
>> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
>>  	if (oob_required)
>> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +		vf610_nfc_memcpy(chip->oob_poi,
>> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
>> +				 mtd->oobsize);
>>
>>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
> 
> You should disable ECC here and not in vf610_nfc_cmd():
> 
> 	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> 
> 

Hm, but then we would have to also clear it at initialization time since
boot loader could have left it in any state. I somewhat prefer the
current state....

>>
>> @@ -564,16 +795,39 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>  				const uint8_t *buf, int oob_required, int page)
>>  {
>>  	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int trfr_sz = mtd->writesize + mtd->oobsize;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> +	int ret = 0;
>> +
>> +	cmd2 |= NAND_CMD_SEQIN << CMD_BYTE1_SHIFT;
>> +	code |= COMMAND_CMD_BYTE1;
>> +
>> +	code |= COMMAND_CAR_BYTE1;
>> +	code |= COMMAND_CAR_BYTE2;
>> +
>> +	row = ROW_ADDR(0, page & 0xff);
>> +	code |= COMMAND_RAR_BYTE1;
>> +	row |= ROW_ADDR(1, page >> 8);
>> +	code |= COMMAND_RAR_BYTE2;
>> +	if (chip->options & NAND_ROW_ADDR_3) {
>> +		row |= ROW_ADDR(2, page >> 16);
>> +		code |= COMMAND_RAR_BYTE3;
>> +	}
> 
> See my comment in _read_page().
> 
>>
>> -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
>> -	if (oob_required)
>> -		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +	cmd1 |= NAND_CMD_PAGEPROG << CMD_BYTE2_SHIFT;
>> +	code |= COMMAND_CMD_BYTE2;
>> +
>> +	code |= COMMAND_WRITE_DATA;
>> +
>> +	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0), buf, mtd->writesize);
>> +
>> +	code |= COMMAND_RB_HANDSHAKE;
>> +	cmd2 |= code << CMD_CODE_SHIFT;
>>
>> -	/* Always write whole page including OOB due to HW ECC */
>> -	nfc->use_hw_ecc = true;
>> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
>> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>>
>> -	return nand_prog_page_end_op(chip);
>> +	return ret;
>>  }
>>
>>  static const struct of_device_id vf610_nfc_dt_ids[] = {
>> @@ -686,6 +940,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
>>  	chip->read_word = vf610_nfc_read_word;
>>  	chip->read_buf = vf610_nfc_read_buf;
>>  	chip->write_buf = vf610_nfc_write_buf;
>> +	chip->exec_op = vf610_nfc_exec_op;
>>  	chip->select_chip = vf610_nfc_select_chip;
>>  	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
>>  	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
> 
> That's all I have for now.
> 
> Regards,
> 
> Boris
> 
> [1]http://code.bulix.org/90iikz-278094

Thanks, and thanks for reviewing!

--
Stefan

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Miquel Raynal]

Hi Stefan,

On Wed, 21 Feb 2018 00:15:18 +0100, Stefan Agner <stefan@agner.ch>
wrote:

> On 11.02.2018 11:54, Boris Brezillon wrote:
> > On Fri,  9 Feb 2018 00:59:20 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> >> This reworks the driver to make use of ->exec_op() callback. The
> >> command sequencer of the VF610 NFC aligns well with the new ops
> >> interface.
> >>
> >> The ops are translated to a NFC command code while filling the
> >> necessary registers. Instead of using the special status and
> >> read id command codes (which require the status/id form special
> >> registers) the driver now uses the main data buffer for all
> >> commands. This simplifies the driver as no special casing is
> >> needed.
> >>
> >> For control data (status byte, id bytes and parameter page) the
> >> driver needs to reverse byte order for little endian CPUs since
> >> the controller seems to store the bytes in big endian order in
> >> the data buffer.
> >>
> >> The current state seems to pass MTD tests on a Colibri VF61.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 267 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> >> index 2fa61cbdbaf7..eae95877422d 100644
> >> --- a/drivers/mtd/nand/vf610_nfc.c
> >> +++ b/drivers/mtd/nand/vf610_nfc.c
> >> @@ -74,6 +74,21 @@
> >>  #define RESET_CMD_CODE			0x4040
> >>  #define STATUS_READ_CMD_CODE		0x4068
> >>
> >> +/* NFC_CMD2[CODE] controller cycle bit masks */
> >> +#define COMMAND_CMD_BYTE1		BIT(14)
> >> +#define COMMAND_CAR_BYTE1		BIT(13)
> >> +#define COMMAND_CAR_BYTE2		BIT(12)
> >> +#define COMMAND_RAR_BYTE1		BIT(11)
> >> +#define COMMAND_RAR_BYTE2		BIT(10)
> >> +#define COMMAND_RAR_BYTE3		BIT(9)
> >> +#define COMMAND_WRITE_DATA		BIT(8)
> >> +#define COMMAND_CMD_BYTE2		BIT(7)
> >> +#define COMMAND_RB_HANDSHAKE		BIT(6)
> >> +#define COMMAND_READ_DATA		BIT(5)
> >> +#define COMMAND_CMD_BYTE3		BIT(4)
> >> +#define COMMAND_READ_STATUS		BIT(3)
> >> +#define COMMAND_READ_ID			BIT(2)
> >> +
> >>  /* NFC ECC mode define */
> >>  #define ECC_BYPASS			0
> >>  #define ECC_45_BYTE			6
> >> @@ -97,10 +112,14 @@
> >>  /* NFC_COL_ADDR Field */
> >>  #define COL_ADDR_MASK				0x0000FFFF
> >>  #define COL_ADDR_SHIFT				0
> >> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> >> +
> >>
> >>  /* NFC_ROW_ADDR Field */
> >>  #define ROW_ADDR_MASK				0x00FFFFFF
> >>  #define ROW_ADDR_SHIFT				0
> >> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> >> +
> >>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
> >>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
> >>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
> >> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
> >>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
> >>  }
> >>
> >> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
> >> +{
> >> +	return container_of(chip, struct vf610_nfc, chip);
> >> +}
> >> +
> >>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
> >>  {
> >>  	return readl(nfc->regs + reg);
> >> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
> >>  	return 1;
> >>  }
> >>
> >> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)  
> > 
> > Nitpick: please wrap the line to make checkpatch happy.
> >   
> 
> Ok.
> 
> >> +{
> >> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> >> +			    COL_ADDR_SHIFT, col);
> >> +
> >> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> >> +			    ROW_ADDR_SHIFT, row);
> >> +
> >> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
> >> +
> >> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
> >> +		col, row, cmd1, cmd2, trfr_sz);
> >> +
> >> +	vf610_nfc_done(nfc);
> >> +}
> >> +
> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
> >> +	const struct nand_subop *subop, int *op_id)
> >> +{
> >> +	if (*op_id + 1 >= subop->ninstrs)
> >> +		return NULL;
> >> +
> >> +	(*op_id)++;
> >> +
> >> +	return &subop->instrs[*op_id];
> >> +}
> >> +
> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
> >> +				const struct nand_subop *subop)
> >> +{
> >> +	const struct nand_op_instr *instr;
> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> +	int op_id = -1, addr = 0, trfr_sz = 0;
> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> >> +
> >> +	/* Some ops are optional, but they need to be in order */
> >> +	instr = vf610_get_next_instr(subop, &op_id);
> >> +	if (!instr)
> >> +		return -EINVAL;
> >> +
> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> >> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
> >> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
> >> +		code |= COMMAND_CMD_BYTE1;
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> >> +  
> > 
> > Hm, you're still checking the sequencing consistency here. This should
> > have been taken care by the core parser already, so really, this is not
> > needed.
> >   
> 
> Since those operations are optional, I have to check...
> 
> >> +		if (instr->ctx.addr.naddrs > 1) {
> >> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_CAR_BYTE1;
> >> +
> >> +			if (mtd->writesize > 512) {
> >> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> +				code |= COMMAND_CAR_BYTE2;
> >> +			}  
> > 
> > No, you shoudn't do that. Just put as many ADDR cycles as requested
> > without trying to figure out if they fit in the column or row field. We
> > really don't care here, as long as the ADDR cycles are issued on the
> > bus.
> >   
> 
> Ok, so on a bus level column/row really does not matter? I wonder why
> the controller makes a difference then? I remember that the controller
> has auto increment feature, might that be the reason? Afaik we cannot
> use such a feature currently...?

If I remember correctly, yes, this is the use case: auto-increment for
sequential cached accesses (not supported yet).

> 
> >> +		}
> >> +
> >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> +		code |= COMMAND_RAR_BYTE1;
> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_RAR_BYTE2;
> >> +		}
> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_RAR_BYTE3;
> >> +		}
> >> +
> >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> +
> >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> >> +
> >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> +				 instr->ctx.data.buf.in + offset,
> >> +				 len);  
> > 
> > I think you have the same endianness problem you have for the READ
> > path. For example, I doubt SET_FEATURES will work properly if you're
> > in LE. So I repeat my initial suggestion: always do the byte swapping
> > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> > and use vf610_nfc_memcpy() only in the ->read/write_page()
> > implementations. 
> >   
> 
> Hm, but doesn't that leads to wrong order of data when using e.g. raw
> read/write page...?

I am not comfortable with endianness, I'll let Boris answer this one.

> 
> >> +		code |= COMMAND_WRITE_DATA;
> >> +		trfr_sz += len;
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> >> +		cmd1 |= instr->ctx.cmd.opcode << CMD_BYTE2_SHIFT;
> >> +		code |= COMMAND_CMD_BYTE2;
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
> >> +		//dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n",
> >> +		//		  instr->ctx.waitrdy.timeout_ms);  
> > 
> > I guess this should go away.
> >   
> >> +		code |= COMMAND_RB_HANDSHAKE;
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> +		code |= COMMAND_READ_DATA;
> >> +		trfr_sz += len;
> >> +	}
> >> +
> >> +	cmd2 |= code << CMD_CODE_SHIFT;
> >> +
> >> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> >> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> >> +
> >> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> +		bool fix_byte_order = false;
> >> +
> >> +#ifdef __LITTLE_ENDIAN
> >> +		fix_byte_order = true;
> >> +#endif
> >> +		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
> >> +			instr->ctx.data.force_8bit , len, offset);
> >> +
> >> +		/*
> >> +		 * HACK: force_8bit indicates reading of the parameter, status
> >> +		 * or id data. The controllers seems to store data in big endian
> >> +		 * byte order to the buffers. Reverse on little endian machines.
> >> +		 */
> >> +		if (instr->ctx.data.force_8bit && fix_byte_order) {  
> > 
> > I'm still convinced this is not the right solution to choose between
> > swap vs don't swap bytes. See my explanation above.
> >   
> >> +			u8 *buf = instr->ctx.data.buf.in;
> >> +
> >> +			while (len--) {
> >> +				int c = offset ^ 0x3;
> >> +
> >> +				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
> >> +				buf++; offset++;
> >> +			}
> >> +		} else {
> >> +			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
> >> +					 nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> +					 len);
> >> +		}  
> > 
> > I think the "copy to/from SRAM and swap bytes if needed" code should
> > should be move in dedicated functions.
> >   
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
> >> +	NAND_OP_PARSER_PATTERN(
> >> +		vf610_nfc_cmd,
> >> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> >> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
> >> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> >> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),  
> > 
> > Are you sure you can do both a read and a write in a single pass? I
> > doubt this is the case, because the logic seems to share the same SRAM
> > for both operations, so I'd recommend splitting in 2 patterns:
> >   
> 
> Hm, yes I currently use the same buffer, so in the current state this
> would not work. But the controller actually has four buffers, so I
> *could* use different ones for reading and writing. But is reading and
> writing in a single go something we need often that such an optimization
> would make worthwhile?

I don't think this is ever used. You should probably go for the
simples approach.

> 
> > 	NAND_OP_PARSER_PATTERN(
> >                 vf610_nfc_cmd,
> >                 NAND_OP_PARSER_PAT_CMD_ELEM(true), 
> >                 NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5), 
> >                 NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K +
> > 	        OOB_MAX), NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >                 NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> >         NAND_OP_PARSER_PATTERN(
> >                 vf610_nfc_cmd,
> >                 NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >                 NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> >                 NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >                 NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> >                 NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K +
> > 	        OOB_MAX)),
> >   
> >> +	);  
> > 
> > To sum-up, here is a diff [1] that applies on top of your patch and
> > illustrate what I have in mind.
> >   
> >> +
> >> +static int vf610_nfc_exec_op(struct nand_chip *chip,
> >> +			     const struct nand_operation *op,
> >> +			     bool check_only)
> >> +{
> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> >> +
> >> +	dev_dbg(nfc->dev, "exec_op, opcode 0x%02x\n", op->instrs[0].ctx.cmd.opcode);
> >> +
> >> +	return nand_op_parser_exec_op(chip, &vf610_nfc_op_parser, op, check_only);
> >> +}
> >> +
> >> +
> >>  /*
> >>   * This function supports Vybrid only (MPC5125 would have full RB and four CS)
> >>   */
> >> @@ -527,8 +729,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> >>  		return ecc_count;
> >>
> >>  	/* Read OOB without ECC unit enabled */
> >> -	vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page);
> >> -	vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> >> +	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
> >>
> >>  	/*
> >>  	 * On an erased page, bit count (including OOB) should be zero or
> >> @@ -542,12 +743,42 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> >>  static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> >>  				uint8_t *buf, int oob_required, int page)
> >>  {
> >> -	int eccsize = chip->ecc.size;
> >> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> >> +	int trfr_sz = mtd->writesize + mtd->oobsize;
> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;  
> > 
> > col is not needed here, just pass 0 to vf610_nfc_run() and you should
> > be good.
> >   
> 
> Ok.
> 
> >>  	int stat;
> >>
> >> -	nand_read_page_op(chip, page, 0, buf, eccsize);
> >> +	cmd2 |= NAND_CMD_READ0 << CMD_BYTE1_SHIFT;
> >> +	code |= COMMAND_CMD_BYTE1;
> >> +
> >> +	code |= COMMAND_CAR_BYTE1;
> >> +	code |= COMMAND_CAR_BYTE2;
> >> +
> >> +	row = ROW_ADDR(0, page & 0xff);
> >> +	code |= COMMAND_RAR_BYTE1;
> >> +	row |= ROW_ADDR(1, page >> 8);
> >> +	code |= COMMAND_RAR_BYTE2;
> >> +
> >> +	if (chip->options & NAND_ROW_ADDR_3) {
> >> +		row |= ROW_ADDR(2, page >> 16);
> >> +		code |= COMMAND_RAR_BYTE3;
> >> +	}  
> > 
> > The address setting could be simplified a bit:
> > 
> > 	row = page;
> > 	code |= COMMAND_CAR_BYTE1 | COMMAND_CAR_BYTE2 |
> > 		COMMAND_RAR_BYTE1| COMMAND_RAR_BYTE2;
> > 	if (chip->options & NAND_ROW_ADDR_3)
> > 		code |= COMMAND_RAR_BYTE3;
> >   
> 
> Agreed, and I will move that in a shared function since it is the same
> for read/write.
> 
> >> +
> >> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
> >> +	code |= COMMAND_CMD_BYTE2;
> >> +
> >> +	code |= COMMAND_RB_HANDSHAKE;
> >> +	code |= COMMAND_READ_DATA;
> >> +	cmd2 |= code << CMD_CODE_SHIFT;
> >> +
> >> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> >> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> >> +
> >> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
> >>  	if (oob_required)
> >> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> >> +		vf610_nfc_memcpy(chip->oob_poi,
> >> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
> >> +				 mtd->oobsize);
> >>
> >>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);  
> > 
> > You should disable ECC here and not in vf610_nfc_cmd():
> > 
> > 	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> > 
> >   
> 
> Hm, but then we would have to also clear it at initialization time since
> boot loader could have left it in any state. I somewhat prefer the
> current state....

I completely agree with Boris on that, it is much clearer if you
activate/deactivate it by hand explicitly each time you need the ECC
engine enabled.

Of course you have to disable it at probe time.

> 
> >>
> >> @@ -564,16 +795,39 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> >>  				const uint8_t *buf, int oob_required, int page)
> >>  {
> >>  	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> >> +	int trfr_sz = mtd->writesize + mtd->oobsize;
> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> >> +	int ret = 0;
> >> +
> >> +	cmd2 |= NAND_CMD_SEQIN << CMD_BYTE1_SHIFT;
> >> +	code |= COMMAND_CMD_BYTE1;
> >> +
> >> +	code |= COMMAND_CAR_BYTE1;
> >> +	code |= COMMAND_CAR_BYTE2;
> >> +
> >> +	row = ROW_ADDR(0, page & 0xff);
> >> +	code |= COMMAND_RAR_BYTE1;
> >> +	row |= ROW_ADDR(1, page >> 8);
> >> +	code |= COMMAND_RAR_BYTE2;
> >> +	if (chip->options & NAND_ROW_ADDR_3) {
> >> +		row |= ROW_ADDR(2, page >> 16);
> >> +		code |= COMMAND_RAR_BYTE3;
> >> +	}  
> > 
> > See my comment in _read_page().
> >   
> >>
> >> -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
> >> -	if (oob_required)
> >> -		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
> >> +	cmd1 |= NAND_CMD_PAGEPROG << CMD_BYTE2_SHIFT;
> >> +	code |= COMMAND_CMD_BYTE2;
> >> +
> >> +	code |= COMMAND_WRITE_DATA;
> >> +
> >> +	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0), buf, mtd->writesize);
> >> +
> >> +	code |= COMMAND_RB_HANDSHAKE;
> >> +	cmd2 |= code << CMD_CODE_SHIFT;
> >>
> >> -	/* Always write whole page including OOB due to HW ECC */
> >> -	nfc->use_hw_ecc = true;
> >> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
> >> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> >> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> >>
> >> -	return nand_prog_page_end_op(chip);
> >> +	return ret;
> >>  }
> >>
> >>  static const struct of_device_id vf610_nfc_dt_ids[] = {
> >> @@ -686,6 +940,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
> >>  	chip->read_word = vf610_nfc_read_word;
> >>  	chip->read_buf = vf610_nfc_read_buf;
> >>  	chip->write_buf = vf610_nfc_write_buf;
> >> +	chip->exec_op = vf610_nfc_exec_op;
> >>  	chip->select_chip = vf610_nfc_select_chip;
> >>  	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> >>  	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;  
> > 
> > That's all I have for now.
> > 
> > Regards,
> > 
> > Boris
> > 
> > [1]http://code.bulix.org/90iikz-278094  
> 
> Thanks, and thanks for reviewing!
> 
> --
> Stefan


Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Wed, 21 Feb 2018 00:15:18 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 11.02.2018 11:54, Boris Brezillon wrote:
> > On Fri,  9 Feb 2018 00:59:20 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> >> This reworks the driver to make use of ->exec_op() callback. The
> >> command sequencer of the VF610 NFC aligns well with the new ops
> >> interface.
> >>
> >> The ops are translated to a NFC command code while filling the
> >> necessary registers. Instead of using the special status and
> >> read id command codes (which require the status/id form special
> >> registers) the driver now uses the main data buffer for all
> >> commands. This simplifies the driver as no special casing is
> >> needed.
> >>
> >> For control data (status byte, id bytes and parameter page) the
> >> driver needs to reverse byte order for little endian CPUs since
> >> the controller seems to store the bytes in big endian order in
> >> the data buffer.
> >>
> >> The current state seems to pass MTD tests on a Colibri VF61.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 267 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> >> index 2fa61cbdbaf7..eae95877422d 100644
> >> --- a/drivers/mtd/nand/vf610_nfc.c
> >> +++ b/drivers/mtd/nand/vf610_nfc.c
> >> @@ -74,6 +74,21 @@
> >>  #define RESET_CMD_CODE			0x4040
> >>  #define STATUS_READ_CMD_CODE		0x4068
> >>
> >> +/* NFC_CMD2[CODE] controller cycle bit masks */
> >> +#define COMMAND_CMD_BYTE1		BIT(14)
> >> +#define COMMAND_CAR_BYTE1		BIT(13)
> >> +#define COMMAND_CAR_BYTE2		BIT(12)
> >> +#define COMMAND_RAR_BYTE1		BIT(11)
> >> +#define COMMAND_RAR_BYTE2		BIT(10)
> >> +#define COMMAND_RAR_BYTE3		BIT(9)
> >> +#define COMMAND_WRITE_DATA		BIT(8)
> >> +#define COMMAND_CMD_BYTE2		BIT(7)
> >> +#define COMMAND_RB_HANDSHAKE		BIT(6)
> >> +#define COMMAND_READ_DATA		BIT(5)
> >> +#define COMMAND_CMD_BYTE3		BIT(4)
> >> +#define COMMAND_READ_STATUS		BIT(3)
> >> +#define COMMAND_READ_ID			BIT(2)
> >> +
> >>  /* NFC ECC mode define */
> >>  #define ECC_BYPASS			0
> >>  #define ECC_45_BYTE			6
> >> @@ -97,10 +112,14 @@
> >>  /* NFC_COL_ADDR Field */
> >>  #define COL_ADDR_MASK				0x0000FFFF
> >>  #define COL_ADDR_SHIFT				0
> >> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> >> +
> >>
> >>  /* NFC_ROW_ADDR Field */
> >>  #define ROW_ADDR_MASK				0x00FFFFFF
> >>  #define ROW_ADDR_SHIFT				0
> >> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
> >> +
> >>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
> >>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
> >>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
> >> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
> >>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
> >>  }
> >>
> >> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
> >> +{
> >> +	return container_of(chip, struct vf610_nfc, chip);
> >> +}
> >> +
> >>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
> >>  {
> >>  	return readl(nfc->regs + reg);
> >> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
> >>  	return 1;
> >>  }
> >>
> >> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)  
> > 
> > Nitpick: please wrap the line to make checkpatch happy.
> >   
> 
> Ok.
> 
> >> +{
> >> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> >> +			    COL_ADDR_SHIFT, col);
> >> +
> >> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> >> +			    ROW_ADDR_SHIFT, row);
> >> +
> >> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
> >> +
> >> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
> >> +		col, row, cmd1, cmd2, trfr_sz);
> >> +
> >> +	vf610_nfc_done(nfc);
> >> +}
> >> +
> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
> >> +	const struct nand_subop *subop, int *op_id)
> >> +{
> >> +	if (*op_id + 1 >= subop->ninstrs)
> >> +		return NULL;
> >> +
> >> +	(*op_id)++;
> >> +
> >> +	return &subop->instrs[*op_id];
> >> +}
> >> +
> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
> >> +				const struct nand_subop *subop)
> >> +{
> >> +	const struct nand_op_instr *instr;
> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> +	int op_id = -1, addr = 0, trfr_sz = 0;
> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> >> +
> >> +	/* Some ops are optional, but they need to be in order */
> >> +	instr = vf610_get_next_instr(subop, &op_id);
> >> +	if (!instr)
> >> +		return -EINVAL;
> >> +
> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> >> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
> >> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
> >> +		code |= COMMAND_CMD_BYTE1;
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> >> +  
> > 
> > Hm, you're still checking the sequencing consistency here. This should
> > have been taken care by the core parser already, so really, this is not
> > needed.
> >   
> 
> Since those operations are optional, I have to check...

Looks like my previous pastebin was not containing all the changes I
wanted to share with you, so here is the full version [1].

Hm, what do these checks add to the checks done in the parser? Even if
all instructions are optional in your pattern the core parser still
check that when they appear in the proper order. So, you can for
example never have CMD1+CMD2+ADDR*5 passed to vf610_nfc_cmd().

The only exception I can think of is when you have DATA_OUT+CMD. In
this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
directly, but that can easily be taken care of in the alternative
implementation I proposed:

		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
			/*
			 * If there was no CMD instruction before the
			 * DATA_OUT one, we must skip CMD_BYTE1 and use
			 * CMD_BYTE2 directly so that the CMD cycle is
			 * really placed after the DATA_OUT one.
			 */
			if (!ncmds)
				ncmds++;
			....
		}
 

> 
> >> +		if (instr->ctx.addr.naddrs > 1) {
> >> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_CAR_BYTE1;
> >> +
> >> +			if (mtd->writesize > 512) {
> >> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> +				code |= COMMAND_CAR_BYTE2;
> >> +			}  
> > 
> > No, you shoudn't do that. Just put as many ADDR cycles as requested
> > without trying to figure out if they fit in the column or row field. We
> > really don't care here, as long as the ADDR cycles are issued on the
> > bus.
> >   
> 
> Ok, so on a bus level column/row really does not matter?

Nope.

> I wonder why
> the controller makes a difference then? I remember that the controller
> has auto increment feature, might that be the reason?

Probably.

> Afaik we cannot
> use such a feature currently...?

No.


> >   
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
> >> +	NAND_OP_PARSER_PATTERN(
> >> +		vf610_nfc_cmd,
> >> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> >> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
> >> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> >> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> >> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),  
> > 
> > Are you sure you can do both a read and a write in a single pass? I
> > doubt this is the case, because the logic seems to share the same SRAM
> > for both operations, so I'd recommend splitting in 2 patterns:
> >   
> 
> Hm, yes I currently use the same buffer, so in the current state this
> would not work. But the controller actually has four buffers, so I
> *could* use different ones for reading and writing. But is reading and
> writing in a single go something we need often that such an optimization
> would make worthwhile?

No, and that's why I suggest to define 2 patterns instead of merging
everything in a single one.

> 
> >> +
> >> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
> >> +	code |= COMMAND_CMD_BYTE2;
> >> +
> >> +	code |= COMMAND_RB_HANDSHAKE;
> >> +	code |= COMMAND_READ_DATA;
> >> +	cmd2 |= code << CMD_CODE_SHIFT;
> >> +
> >> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> >> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
> >> +
> >> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
> >>  	if (oob_required)
> >> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> >> +		vf610_nfc_memcpy(chip->oob_poi,
> >> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
> >> +				 mtd->oobsize);
> >>
> >>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);  
> > 
> > You should disable ECC here and not in vf610_nfc_cmd():
> > 
> > 	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> > 
> >   
> 
> Hm, but then we would have to also clear it at initialization time since
> boot loader could have left it in any state.

Yes.

> I somewhat prefer the
> current state....

I don't :-). ->exec_op() is only supposed to handle NAND operations
not extra things like ECC. Since you already enable ECC in this
function it would be consistent to disable it before leaving the
function and leave the NFC in a known state so that future NAND
operations can be executed without taking extra precautions (like
disabling ECC).

Regards,

Boris

[1]http://code.bulix.org/njptts-286584
-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 21.02.2018 08:18, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 00:15:18 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
>> On 11.02.2018 11:54, Boris Brezillon wrote:
>> > On Fri,  9 Feb 2018 00:59:20 +0100
>> > Stefan Agner <stefan@agner.ch> wrote:
>> >
>> >> This reworks the driver to make use of ->exec_op() callback. The
>> >> command sequencer of the VF610 NFC aligns well with the new ops
>> >> interface.
>> >>
>> >> The ops are translated to a NFC command code while filling the
>> >> necessary registers. Instead of using the special status and
>> >> read id command codes (which require the status/id form special
>> >> registers) the driver now uses the main data buffer for all
>> >> commands. This simplifies the driver as no special casing is
>> >> needed.
>> >>
>> >> For control data (status byte, id bytes and parameter page) the
>> >> driver needs to reverse byte order for little endian CPUs since
>> >> the controller seems to store the bytes in big endian order in
>> >> the data buffer.
>> >>
>> >> The current state seems to pass MTD tests on a Colibri VF61.
>> >>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> ---
>> >>  drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
>> >>  1 file changed, 267 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> >> index 2fa61cbdbaf7..eae95877422d 100644
>> >> --- a/drivers/mtd/nand/vf610_nfc.c
>> >> +++ b/drivers/mtd/nand/vf610_nfc.c
>> >> @@ -74,6 +74,21 @@
>> >>  #define RESET_CMD_CODE			0x4040
>> >>  #define STATUS_READ_CMD_CODE		0x4068
>> >>
>> >> +/* NFC_CMD2[CODE] controller cycle bit masks */
>> >> +#define COMMAND_CMD_BYTE1		BIT(14)
>> >> +#define COMMAND_CAR_BYTE1		BIT(13)
>> >> +#define COMMAND_CAR_BYTE2		BIT(12)
>> >> +#define COMMAND_RAR_BYTE1		BIT(11)
>> >> +#define COMMAND_RAR_BYTE2		BIT(10)
>> >> +#define COMMAND_RAR_BYTE3		BIT(9)
>> >> +#define COMMAND_WRITE_DATA		BIT(8)
>> >> +#define COMMAND_CMD_BYTE2		BIT(7)
>> >> +#define COMMAND_RB_HANDSHAKE		BIT(6)
>> >> +#define COMMAND_READ_DATA		BIT(5)
>> >> +#define COMMAND_CMD_BYTE3		BIT(4)
>> >> +#define COMMAND_READ_STATUS		BIT(3)
>> >> +#define COMMAND_READ_ID			BIT(2)
>> >> +
>> >>  /* NFC ECC mode define */
>> >>  #define ECC_BYPASS			0
>> >>  #define ECC_45_BYTE			6
>> >> @@ -97,10 +112,14 @@
>> >>  /* NFC_COL_ADDR Field */
>> >>  #define COL_ADDR_MASK				0x0000FFFF
>> >>  #define COL_ADDR_SHIFT				0
>> >> +#define COL_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
>> >> +
>> >>
>> >>  /* NFC_ROW_ADDR Field */
>> >>  #define ROW_ADDR_MASK				0x00FFFFFF
>> >>  #define ROW_ADDR_SHIFT				0
>> >> +#define ROW_ADDR(pos, val)			((val & 0xFF) << (8 * (pos)))
>> >> +
>> >>  #define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
>> >>  #define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
>> >>  #define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
>> >> @@ -173,6 +192,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd)
>> >>  	return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip);
>> >>  }
>> >>
>> >> +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip)
>> >> +{
>> >> +	return container_of(chip, struct vf610_nfc, chip);
>> >> +}
>> >> +
>> >>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
>> >>  {
>> >>  	return readl(nfc->regs + reg);
>> >> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
>> >>  	return 1;
>> >>  }
>> >>
>> >> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
>> >
>> > Nitpick: please wrap the line to make checkpatch happy.
>> >
>>
>> Ok.
>>
>> >> +{
>> >> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
>> >> +			    COL_ADDR_SHIFT, col);
>> >> +
>> >> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
>> >> +			    ROW_ADDR_SHIFT, row);
>> >> +
>> >> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
>> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
>> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
>> >> +
>> >> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
>> >> +		col, row, cmd1, cmd2, trfr_sz);
>> >> +
>> >> +	vf610_nfc_done(nfc);
>> >> +}
>> >> +
>> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
>> >> +	const struct nand_subop *subop, int *op_id)
>> >> +{
>> >> +	if (*op_id + 1 >= subop->ninstrs)
>> >> +		return NULL;
>> >> +
>> >> +	(*op_id)++;
>> >> +
>> >> +	return &subop->instrs[*op_id];
>> >> +}
>> >> +
>> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
>> >> +				const struct nand_subop *subop)
>> >> +{
>> >> +	const struct nand_op_instr *instr;
>> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
>> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> >> +	int op_id = -1, addr = 0, trfr_sz = 0;
>> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> >> +
>> >> +	/* Some ops are optional, but they need to be in order */
>> >> +	instr = vf610_get_next_instr(subop, &op_id);
>> >> +	if (!instr)
>> >> +		return -EINVAL;
>> >> +
>> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
>> >> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
>> >> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
>> >> +		code |= COMMAND_CMD_BYTE1;
>> >> +
>> >> +		instr = vf610_get_next_instr(subop, &op_id);
>> >> +	}
>> >> +
>> >> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
>> >> +
>> >
>> > Hm, you're still checking the sequencing consistency here. This should
>> > have been taken care by the core parser already, so really, this is not
>> > needed.
>> >
>>
>> Since those operations are optional, I have to check...
> 
> Looks like my previous pastebin was not containing all the changes I
> wanted to share with you, so here is the full version [1].
> 
> Hm, what do these checks add to the checks done in the parser? Even if
> all instructions are optional in your pattern the core parser still
> check that when they appear in the proper order. So, you can for
> example never have CMD1+CMD2+ADDR*5 passed to vf610_nfc_cmd().

Sure...

> 
> The only exception I can think of is when you have DATA_OUT+CMD. In
> this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
> directly, but that can easily be taken care of in the alternative
> implementation I proposed:
> 
> 		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> 			/*
> 			 * If there was no CMD instruction before the
> 			 * DATA_OUT one, we must skip CMD_BYTE1 and use
> 			 * CMD_BYTE2 directly so that the CMD cycle is
> 			 * really placed after the DATA_OUT one.
> 			 */
> 			if (!ncmds)
> 				ncmds++;
> 			....
> 		}
>  
> 

I fully understand, I do not have to enforce order since I can rely on
the order passed by the stack.

It is also not what I am after. I just check which operation is going to
be next. Like your switch statement.

I just see don't see a real advantage in using a for loop. It makes it
harder to read, since the order of operations will no more be obvious.
It makes this ncmd work around necessary.

Using a for loop just because we can? I haven't seen a convincing
argument so far.


>>
>> >> +		if (instr->ctx.addr.naddrs > 1) {
>> >> +			col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> >> +			code |= COMMAND_CAR_BYTE1;
>> >> +
>> >> +			if (mtd->writesize > 512) {
>> >> +				col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> >> +				code |= COMMAND_CAR_BYTE2;
>> >> +			}
>> >
>> > No, you shoudn't do that. Just put as many ADDR cycles as requested
>> > without trying to figure out if they fit in the column or row field. We
>> > really don't care here, as long as the ADDR cycles are issued on the
>> > bus.
>> >
>>
>> Ok, so on a bus level column/row really does not matter?
> 
> Nope.
> 
>> I wonder why
>> the controller makes a difference then? I remember that the controller
>> has auto increment feature, might that be the reason?
> 
> Probably.
> 
>> Afaik we cannot
>> use such a feature currently...?
> 
> No.
> 
> 
>> >
>> >> +	}
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
>> >> +	NAND_OP_PARSER_PATTERN(
>> >> +		vf610_nfc_cmd,
>> >> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> >> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
>> >> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
>> >> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> >> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>> >> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
>> >
>> > Are you sure you can do both a read and a write in a single pass? I
>> > doubt this is the case, because the logic seems to share the same SRAM
>> > for both operations, so I'd recommend splitting in 2 patterns:
>> >
>>
>> Hm, yes I currently use the same buffer, so in the current state this
>> would not work. But the controller actually has four buffers, so I
>> *could* use different ones for reading and writing. But is reading and
>> writing in a single go something we need often that such an optimization
>> would make worthwhile?
> 
> No, and that's why I suggest to define 2 patterns instead of merging
> everything in a single one.
> 

Ok, will leave it using one buffer and use two patterns.

>>
>> >> +
>> >> +	cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
>> >> +	code |= COMMAND_CMD_BYTE2;
>> >> +
>> >> +	code |= COMMAND_RB_HANDSHAKE;
>> >> +	code |= COMMAND_READ_DATA;
>> >> +	cmd2 |= code << CMD_CODE_SHIFT;
>> >> +
>> >> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> >> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> >> +
>> >> +	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
>> >>  	if (oob_required)
>> >> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> >> +		vf610_nfc_memcpy(chip->oob_poi,
>> >> +				 nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
>> >> +				 mtd->oobsize);
>> >>
>> >>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
>> >
>> > You should disable ECC here and not in vf610_nfc_cmd():
>> >
>> > 	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
>> >
>> >
>>
>> Hm, but then we would have to also clear it at initialization time since
>> boot loader could have left it in any state.
> 
> Yes.
> 
>> I somewhat prefer the
>> current state....
> 
> I don't :-). ->exec_op() is only supposed to handle NAND operations
> not extra things like ECC. Since you already enable ECC in this
> function it would be consistent to disable it before leaving the
> function and leave the NFC in a known state so that future NAND
> operations can be executed without taking extra precautions (like
> disabling ECC).
> 

My initial thought was that configuring on every operation makes the
driver more stateless.... But give that also other fields are
initialized first, there is already considerable amount of state we rely
on. With that in mind, it makes sense to have disabled ECC as a default
state and have it only enabled in the ECC case. Will change that.


Any thoughts to the raw page read/write endianness issue?

>> I think you have the same endianness problem you have for the READ
>> path. For example, I doubt SET_FEATURES will work properly if you're
>> in LE. So I repeat my initial suggestion: always do the byte swapping
>> when you're transfering data to/from the SRAM from vf610_nfc_cmd()
>> and use vf610_nfc_memcpy() only in the ->read/write_page()
>> implementations. 
>>
> 
> Hm, but doesn't that leads to wrong order of data when using e.g. raw
> read/write page...?

With that last iteration I used the default implementation of the stack.

I guess I could just implement them too and use vf610_nfc_memcpy()?

This should be fine then for tdoay, but what if we have another data
related access in the future? It then also will make use of
vf610_nfc_cmd and change byte order...

--
Stefan

> Regards,
> 
> Boris
> 
> [1]http://code.bulix.org/njptts-286584

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Wed, 21 Feb 2018 09:30:32 +0100
Stefan Agner <stefan@agner.ch> wrote:

> 
> > 
> > The only exception I can think of is when you have DATA_OUT+CMD. In
> > this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
> > directly, but that can easily be taken care of in the alternative
> > implementation I proposed:
> > 
> > 		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> > 			/*
> > 			 * If there was no CMD instruction before the
> > 			 * DATA_OUT one, we must skip CMD_BYTE1 and use
> > 			 * CMD_BYTE2 directly so that the CMD cycle is
> > 			 * really placed after the DATA_OUT one.
> > 			 */
> > 			if (!ncmds)
> > 				ncmds++;
> > 			....
> > 		}
> >  
> >   
> 
> I fully understand, I do not have to enforce order since I can rely on
> the order passed by the stack.
> 
> It is also not what I am after. I just check which operation is going to
> be next. Like your switch statement.
> 
> I just see don't see a real advantage in using a for loop. It makes it
> harder to read, since the order of operations will no more be obvious.
> It makes this ncmd work around necessary.
> 
> Using a for loop just because we can? I haven't seen a convincing
> argument so far.
> 

I still prefer the for-loop+switch approach (I find it cleaner), but
that's probably a matter of taste.

> 
> >> I think you have the same endianness problem you have for the READ
> >> path. For example, I doubt SET_FEATURES will work properly if you're
> >> in LE. So I repeat my initial suggestion: always do the byte swapping
> >> when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> >> and use vf610_nfc_memcpy() only in the ->read/write_page()
> >> implementations. 
> >>  
> > 
> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
> > read/write page...?  
> 
> With that last iteration I used the default implementation of the stack.
> 
> I guess I could just implement them too and use vf610_nfc_memcpy()?

Yep (see my other email).

> 
> This should be fine then for tdoay, but what if we have another data
> related access in the future? It then also will make use of
> vf610_nfc_cmd and change byte order...

I'll reply with another question: what if you need to read data that
have been programmed by the flash vendor in some OTP sections (already
had to do that to support read-retry on some NANDs)? There's clearly
no ideal solution, we just have to chose the one which is less likely
to break things in the future. Today, we have a way to overload page
accessors, so let's use it.

BTW, it's not exactly about data related accesses, but accesses to data
for which you control the read and write path (that excludes
writes/reads to/from NAND registers like the SET/GET_FEATURES, because
then, data are used by the internal NAND logic to tweak its behavior).

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 21.02.2018 10:03, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 09:30:32 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
>>
>> >
>> > The only exception I can think of is when you have DATA_OUT+CMD. In
>> > this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
>> > directly, but that can easily be taken care of in the alternative
>> > implementation I proposed:
>> >
>> > 		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
>> > 			/*
>> > 			 * If there was no CMD instruction before the
>> > 			 * DATA_OUT one, we must skip CMD_BYTE1 and use
>> > 			 * CMD_BYTE2 directly so that the CMD cycle is
>> > 			 * really placed after the DATA_OUT one.
>> > 			 */
>> > 			if (!ncmds)
>> > 				ncmds++;
>> > 			....
>> > 		}
>> >
>> >
>>
>> I fully understand, I do not have to enforce order since I can rely on
>> the order passed by the stack.
>>
>> It is also not what I am after. I just check which operation is going to
>> be next. Like your switch statement.
>>
>> I just see don't see a real advantage in using a for loop. It makes it
>> harder to read, since the order of operations will no more be obvious.
>> It makes this ncmd work around necessary.
>>
>> Using a for loop just because we can? I haven't seen a convincing
>> argument so far.
>>
> 
> I still prefer the for-loop+switch approach (I find it cleaner), but
> that's probably a matter of taste.
> 

Let me make a cleaned up version of the if approach and then decide.

>>
>> >> I think you have the same endianness problem you have for the READ
>> >> path. For example, I doubt SET_FEATURES will work properly if you're
>> >> in LE. So I repeat my initial suggestion: always do the byte swapping
>> >> when you're transfering data to/from the SRAM from vf610_nfc_cmd()
>> >> and use vf610_nfc_memcpy() only in the ->read/write_page()
>> >> implementations.
>> >>
>> >
>> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
>> > read/write page...?
>>
>> With that last iteration I used the default implementation of the stack.
>>
>> I guess I could just implement them too and use vf610_nfc_memcpy()?
> 
> Yep (see my other email).
> 

Yes there was a race condition :-)

>>
>> This should be fine then for tdoay, but what if we have another data
>> related access in the future? It then also will make use of
>> vf610_nfc_cmd and change byte order...
> 
> I'll reply with another question: what if you need to read data that
> have been programmed by the flash vendor in some OTP sections (already
> had to do that to support read-retry on some NANDs)? There's clearly
> no ideal solution, we just have to chose the one which is less likely
> to break things in the future. Today, we have a way to overload page
> accessors, so let's use it.

Ok agreed. I guess we should clearly state somewhere what is going on.

> 
> BTW, it's not exactly about data related accesses, but accesses to data
> for which you control the read and write path (that excludes
> writes/reads to/from NAND registers like the SET/GET_FEATURES, because
> then, data are used by the internal NAND logic to tweak its behavior).

"Data related accesses which the drivers controls the read and write
path" seems like a nice summary in what case we want to avoid the byte
reordering.

--
Stefan

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Miquel Raynal]

Hi Stefan,


> >> >> +{
> >> >> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> >> >> +			    COL_ADDR_SHIFT, col);
> >> >> +
> >> >> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> >> >> +			    ROW_ADDR_SHIFT, row);
> >> >> +
> >> >> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
> >> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
> >> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
> >> >> +
> >> >> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
> >> >> +		col, row, cmd1, cmd2, trfr_sz);
> >> >> +
> >> >> +	vf610_nfc_done(nfc);
> >> >> +}
> >> >> +
> >> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
> >> >> +	const struct nand_subop *subop, int *op_id)
> >> >> +{
> >> >> +	if (*op_id + 1 >= subop->ninstrs)
> >> >> +		return NULL;
> >> >> +
> >> >> +	(*op_id)++;
> >> >> +
> >> >> +	return &subop->instrs[*op_id];
> >> >> +}
> >> >> +
> >> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
> >> >> +				const struct nand_subop *subop)
> >> >> +{
> >> >> +	const struct nand_op_instr *instr;
> >> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> >> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> >> +	int op_id = -1, addr = 0, trfr_sz = 0;
> >> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> >> >> +
> >> >> +	/* Some ops are optional, but they need to be in order */
> >> >> +	instr = vf610_get_next_instr(subop, &op_id);
> >> >> +	if (!instr)
> >> >> +		return -EINVAL;
> >> >> +
> >> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> >> >> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
> >> >> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
> >> >> +		code |= COMMAND_CMD_BYTE1;
> >> >> +
> >> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> >> +	}
> >> >> +
> >> >> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> >> >> +  
> >> >
> >> > Hm, you're still checking the sequencing consistency here. This should
> >> > have been taken care by the core parser already, so really, this is not
> >> > needed.
> >> >  
> >>
> >> Since those operations are optional, I have to check...  
> > 
> > Looks like my previous pastebin was not containing all the changes I
> > wanted to share with you, so here is the full version [1].
> > 
> > Hm, what do these checks add to the checks done in the parser? Even if
> > all instructions are optional in your pattern the core parser still
> > check that when they appear in the proper order. So, you can for
> > example never have CMD1+CMD2+ADDR*5 passed to vf610_nfc_cmd().  
> 
> Sure...
> 
> > 
> > The only exception I can think of is when you have DATA_OUT+CMD. In
> > this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
> > directly, but that can easily be taken care of in the alternative
> > implementation I proposed:
> > 
> > 		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> > 			/*
> > 			 * If there was no CMD instruction before the
> > 			 * DATA_OUT one, we must skip CMD_BYTE1 and use
> > 			 * CMD_BYTE2 directly so that the CMD cycle is
> > 			 * really placed after the DATA_OUT one.
> > 			 */
> > 			if (!ncmds)
> > 				ncmds++;
> > 			....
> > 		}
> >  
> >   
> 
> I fully understand, I do not have to enforce order since I can rely on
> the order passed by the stack.
> 
> It is also not what I am after. I just check which operation is going to
> be next. Like your switch statement.
> 
> I just see don't see a real advantage in using a for loop. It makes it
> harder to read, since the order of operations will no more be obvious.
> It makes this ncmd work around necessary.

Well, the aim of ->exec_op() was initially to get rid of the controller
specificities and be free to implement new NAND operations without
having to check every controller driver in the world, so doing it this
way freezes the possibilities to a reduced set of operations (ie. what
we do today). This is wrong from my point of view and I would really
welcome a for-loop approach instead.

Thanks,
Miquèl

> 
> Using a for loop just because we can? I haven't seen a convincing
> argument so far.
> 
> 

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 21.02.2018 11:09, Miquel Raynal wrote:
> Hi Stefan,
> 
> 
>> >> >> +{
>> >> >> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
>> >> >> +			    COL_ADDR_SHIFT, col);
>> >> >> +
>> >> >> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
>> >> >> +			    ROW_ADDR_SHIFT, row);
>> >> >> +
>> >> >> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
>> >> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
>> >> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
>> >> >> +
>> >> >> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
>> >> >> +		col, row, cmd1, cmd2, trfr_sz);
>> >> >> +
>> >> >> +	vf610_nfc_done(nfc);
>> >> >> +}
>> >> >> +
>> >> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
>> >> >> +	const struct nand_subop *subop, int *op_id)
>> >> >> +{
>> >> >> +	if (*op_id + 1 >= subop->ninstrs)
>> >> >> +		return NULL;
>> >> >> +
>> >> >> +	(*op_id)++;
>> >> >> +
>> >> >> +	return &subop->instrs[*op_id];
>> >> >> +}
>> >> >> +
>> >> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
>> >> >> +				const struct nand_subop *subop)
>> >> >> +{
>> >> >> +	const struct nand_op_instr *instr;
>> >> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
>> >> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> >> >> +	int op_id = -1, addr = 0, trfr_sz = 0;
>> >> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> >> >> +
>> >> >> +	/* Some ops are optional, but they need to be in order */
>> >> >> +	instr = vf610_get_next_instr(subop, &op_id);
>> >> >> +	if (!instr)
>> >> >> +		return -EINVAL;
>> >> >> +
>> >> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
>> >> >> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
>> >> >> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
>> >> >> +		code |= COMMAND_CMD_BYTE1;
>> >> >> +
>> >> >> +		instr = vf610_get_next_instr(subop, &op_id);
>> >> >> +	}
>> >> >> +
>> >> >> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
>> >> >> +
>> >> >
>> >> > Hm, you're still checking the sequencing consistency here. This should
>> >> > have been taken care by the core parser already, so really, this is not
>> >> > needed.
>> >> >
>> >>
>> >> Since those operations are optional, I have to check...
>> >
>> > Looks like my previous pastebin was not containing all the changes I
>> > wanted to share with you, so here is the full version [1].
>> >
>> > Hm, what do these checks add to the checks done in the parser? Even if
>> > all instructions are optional in your pattern the core parser still
>> > check that when they appear in the proper order. So, you can for
>> > example never have CMD1+CMD2+ADDR*5 passed to vf610_nfc_cmd().
>>
>> Sure...
>>
>> >
>> > The only exception I can think of is when you have DATA_OUT+CMD. In
>> > this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
>> > directly, but that can easily be taken care of in the alternative
>> > implementation I proposed:
>> >
>> > 		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
>> > 			/*
>> > 			 * If there was no CMD instruction before the
>> > 			 * DATA_OUT one, we must skip CMD_BYTE1 and use
>> > 			 * CMD_BYTE2 directly so that the CMD cycle is
>> > 			 * really placed after the DATA_OUT one.
>> > 			 */
>> > 			if (!ncmds)
>> > 				ncmds++;
>> > 			....
>> > 		}
>> >
>> >
>>
>> I fully understand, I do not have to enforce order since I can rely on
>> the order passed by the stack.
>>
>> It is also not what I am after. I just check which operation is going to
>> be next. Like your switch statement.
>>
>> I just see don't see a real advantage in using a for loop. It makes it
>> harder to read, since the order of operations will no more be obvious.
>> It makes this ncmd work around necessary.
> 
> Well, the aim of ->exec_op() was initially to get rid of the controller
> specificities and be free to implement new NAND operations without
> having to check every controller driver in the world, so doing it this
> way freezes the possibilities to a reduced set of operations (ie. what
> we do today). This is wrong from my point of view and I would really
> welcome a for-loop approach instead.

We can write a driver however we want, the hardware is still strictly
sequentially...

The current implementation should not restrict possible operations in
any way other than it is enforced by hardware anyway...

So the discussion is only about readability/style.

I will make a cleaned up v4 still sequentially and then we can compare
that to a for loop again to make a final decision.

--
Stefan


> 
> Thanks,
> Miquèl
> 
>>
>> Using a for loop just because we can? I haven't seen a convincing
>> argument so far.
>>
>>

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Wed, 21 Feb 2018 00:15:18 +0100
Stefan Agner <stefan@agner.ch> wrote:

> >> +		}
> >> +
> >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> +		code |= COMMAND_RAR_BYTE1;
> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_RAR_BYTE2;
> >> +		}
> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_RAR_BYTE3;
> >> +		}
> >> +
> >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> +
> >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> >> +
> >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> +				 instr->ctx.data.buf.in + offset,
> >> +				 len);  
> > 
> > I think you have the same endianness problem you have for the READ
> > path. For example, I doubt SET_FEATURES will work properly if you're
> > in LE. So I repeat my initial suggestion: always do the byte swapping
> > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> > and use vf610_nfc_memcpy() only in the ->read/write_page()
> > implementations. 
> >   
> 
> Hm, but doesn't that leads to wrong order of data when using e.g. raw
> read/write page...?

Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
prefer that to having an ->exec_op() implementation that tries to guess
what the core is trying to do.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Wed, 21 Feb 2018 09:28:21 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Wed, 21 Feb 2018 00:15:18 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
> > >> +		}
> > >> +
> > >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> > >> +		code |= COMMAND_RAR_BYTE1;
> > >> +		if (addr < instr->ctx.addr.naddrs) {
> > >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> > >> +			code |= COMMAND_RAR_BYTE2;
> > >> +		}
> > >> +		if (addr < instr->ctx.addr.naddrs) {
> > >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> > >> +			code |= COMMAND_RAR_BYTE3;
> > >> +		}
> > >> +
> > >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> > >> +
> > >> +		instr = vf610_get_next_instr(subop, &op_id);
> > >> +	}
> > >> +
> > >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> > >> +		int len = nand_subop_get_data_len(subop, op_id);
> > >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> > >> +
> > >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> > >> +
> > >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> > >> +				 instr->ctx.data.buf.in + offset,
> > >> +				 len);    
> > > 
> > > I think you have the same endianness problem you have for the READ
> > > path. For example, I doubt SET_FEATURES will work properly if you're
> > > in LE. So I repeat my initial suggestion: always do the byte swapping
> > > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> > > and use vf610_nfc_memcpy() only in the ->read/write_page()
> > > implementations. 
> > >     
> > 
> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
> > read/write page...?  
> 
> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
> prefer that to having an ->exec_op() implementation that tries to guess
> what the core is trying to do.
> 

One more thing, in its current state the driver may fail to detect
blocks marked bad by the manufacturer because of this endianness issue.
That should work most of the time because manufacturers tend to fill
the whole block with zeros when they want to mark it bad, but they're
not required to do that.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 21.02.2018 09:35, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 09:28:21 +0100
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>> On Wed, 21 Feb 2018 00:15:18 +0100
>> Stefan Agner <stefan@agner.ch> wrote:
>>
>> > >> +		}
>> > >> +
>> > >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> > >> +		code |= COMMAND_RAR_BYTE1;
>> > >> +		if (addr < instr->ctx.addr.naddrs) {
>> > >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> > >> +			code |= COMMAND_RAR_BYTE2;
>> > >> +		}
>> > >> +		if (addr < instr->ctx.addr.naddrs) {
>> > >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
>> > >> +			code |= COMMAND_RAR_BYTE3;
>> > >> +		}
>> > >> +
>> > >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
>> > >> +
>> > >> +		instr = vf610_get_next_instr(subop, &op_id);
>> > >> +	}
>> > >> +
>> > >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
>> > >> +		int len = nand_subop_get_data_len(subop, op_id);
>> > >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> > >> +
>> > >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
>> > >> +
>> > >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
>> > >> +				 instr->ctx.data.buf.in + offset,
>> > >> +				 len);
>> > >
>> > > I think you have the same endianness problem you have for the READ
>> > > path. For example, I doubt SET_FEATURES will work properly if you're
>> > > in LE. So I repeat my initial suggestion: always do the byte swapping
>> > > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
>> > > and use vf610_nfc_memcpy() only in the ->read/write_page()
>> > > implementations.
>> > >
>> >
>> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
>> > read/write page...?
>>
>> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
>> prefer that to having an ->exec_op() implementation that tries to guess
>> what the core is trying to do.
>>
> 
> One more thing, in its current state the driver may fail to detect
> blocks marked bad by the manufacturer because of this endianness issue.
> That should work most of the time because manufacturers tend to fill
> the whole block with zeros when they want to mark it bad, but they're
> not required to do that.

That sounds scary. I think bad block scan is done through oob reads
only. I guess if we only implement raw read/write and ecc read/write,
the stack will actually read OOB with proper ending when implemented as
you suggested.

--
Stefan

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Wed, 21 Feb 2018 13:24:14 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 21.02.2018 09:35, Boris Brezillon wrote:
> > On Wed, 21 Feb 2018 09:28:21 +0100
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >   
> >> On Wed, 21 Feb 2018 00:15:18 +0100
> >> Stefan Agner <stefan@agner.ch> wrote:
> >>  
> >> > >> +		}
> >> > >> +
> >> > >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> > >> +		code |= COMMAND_RAR_BYTE1;
> >> > >> +		if (addr < instr->ctx.addr.naddrs) {
> >> > >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> > >> +			code |= COMMAND_RAR_BYTE2;
> >> > >> +		}
> >> > >> +		if (addr < instr->ctx.addr.naddrs) {
> >> > >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> >> > >> +			code |= COMMAND_RAR_BYTE3;
> >> > >> +		}
> >> > >> +
> >> > >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> >> > >> +
> >> > >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> > >> +	}
> >> > >> +
> >> > >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> >> > >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> > >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> > >> +
> >> > >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> >> > >> +
> >> > >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> > >> +				 instr->ctx.data.buf.in + offset,
> >> > >> +				 len);  
> >> > >
> >> > > I think you have the same endianness problem you have for the READ
> >> > > path. For example, I doubt SET_FEATURES will work properly if you're
> >> > > in LE. So I repeat my initial suggestion: always do the byte swapping
> >> > > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> >> > > and use vf610_nfc_memcpy() only in the ->read/write_page()
> >> > > implementations.
> >> > >  
> >> >
> >> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
> >> > read/write page...?  
> >>
> >> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
> >> prefer that to having an ->exec_op() implementation that tries to guess
> >> what the core is trying to do.
> >>  
> > 
> > One more thing, in its current state the driver may fail to detect
> > blocks marked bad by the manufacturer because of this endianness issue.
> > That should work most of the time because manufacturers tend to fill
> > the whole block with zeros when they want to mark it bad, but they're
> > not required to do that.  
> 
> That sounds scary. I think bad block scan is done through oob reads
> only. I guess if we only implement raw read/write and ecc read/write,
> the stack will actually read OOB with proper ending when implemented as
> you suggested.

Nope. Because BBM can be written by the NAND manufacturer (during
production), and it's usually placed in the first byte (or first 2 bytes
for 16-bit NANDs) of the OOB region. So let's assume you read the OOB
region without taking care of endianness, on little-endian platforms
you'll check the value of oob[4] (and oob[3] in case it's a 16-bit NAND)
instead of oob[0] (and oob[1]).

Of course, this situation will not happen when Linux marks a block
bad, because then, it's in control of what is written, and since
endianness conversion is skipped in both read/write path we're good.

My intention was not to scare you, but this endianness issue is not
something trivial to solve, and it would have been wiser to force
all users of this controller (that includes bootrom, bootloaders, and
and Linux) to do the cpu_to_be32() conversion when writing to/reading
from the FIFO, 'cause know we're screwed.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 21.02.2018 13:46, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 13:24:14 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
>> On 21.02.2018 09:35, Boris Brezillon wrote:
>> > On Wed, 21 Feb 2018 09:28:21 +0100
>> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
>> >
>> >> On Wed, 21 Feb 2018 00:15:18 +0100
>> >> Stefan Agner <stefan@agner.ch> wrote:
>> >>
>> >> > >> +		}
>> >> > >> +
>> >> > >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> >> > >> +		code |= COMMAND_RAR_BYTE1;
>> >> > >> +		if (addr < instr->ctx.addr.naddrs) {
>> >> > >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> >> > >> +			code |= COMMAND_RAR_BYTE2;
>> >> > >> +		}
>> >> > >> +		if (addr < instr->ctx.addr.naddrs) {
>> >> > >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
>> >> > >> +			code |= COMMAND_RAR_BYTE3;
>> >> > >> +		}
>> >> > >> +
>> >> > >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
>> >> > >> +
>> >> > >> +		instr = vf610_get_next_instr(subop, &op_id);
>> >> > >> +	}
>> >> > >> +
>> >> > >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
>> >> > >> +		int len = nand_subop_get_data_len(subop, op_id);
>> >> > >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> >> > >> +
>> >> > >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
>> >> > >> +
>> >> > >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
>> >> > >> +				 instr->ctx.data.buf.in + offset,
>> >> > >> +				 len);
>> >> > >
>> >> > > I think you have the same endianness problem you have for the READ
>> >> > > path. For example, I doubt SET_FEATURES will work properly if you're
>> >> > > in LE. So I repeat my initial suggestion: always do the byte swapping
>> >> > > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
>> >> > > and use vf610_nfc_memcpy() only in the ->read/write_page()
>> >> > > implementations.
>> >> > >
>> >> >
>> >> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
>> >> > read/write page...?
>> >>
>> >> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
>> >> prefer that to having an ->exec_op() implementation that tries to guess
>> >> what the core is trying to do.
>> >>
>> >
>> > One more thing, in its current state the driver may fail to detect
>> > blocks marked bad by the manufacturer because of this endianness issue.
>> > That should work most of the time because manufacturers tend to fill
>> > the whole block with zeros when they want to mark it bad, but they're
>> > not required to do that.
>>
>> That sounds scary. I think bad block scan is done through oob reads
>> only. I guess if we only implement raw read/write and ecc read/write,
>> the stack will actually read OOB with proper ending when implemented as
>> you suggested.
> 
> Nope. Because BBM can be written by the NAND manufacturer (during
> production), and it's usually placed in the first byte (or first 2 bytes
> for 16-bit NANDs) of the OOB region. So let's assume you read the OOB
> region without taking care of endianness, on little-endian platforms
> you'll check the value of oob[4] (and oob[3] in case it's a 16-bit NAND)
> instead of oob[0] (and oob[1]).

Agreed.

But I think the stack normally uses scan_read_oob which subsequently
calls nand_read_oob_op. This will use regular exec_op and not the page
accessors, hence byte order should be fine...

> 
> Of course, this situation will not happen when Linux marks a block
> bad, because then, it's in control of what is written, and since
> endianness conversion is skipped in both read/write path we're good.
> 
> My intention was not to scare you, but this endianness issue is not
> something trivial to solve, and it would have been wiser to force
> all users of this controller (that includes bootrom, bootloaders, and
> and Linux) to do the cpu_to_be32() conversion when writing to/reading
> from the FIFO, 'cause know we're screwed.

Yes agreed... But too late for that now, at least not as a default
behavior.

--
Stefan

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Stefan Agner]

On 21.02.2018 09:28, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 00:15:18 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
>> >> +		}
>> >> +
>> >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> >> +		code |= COMMAND_RAR_BYTE1;
>> >> +		if (addr < instr->ctx.addr.naddrs) {
>> >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> >> +			code |= COMMAND_RAR_BYTE2;
>> >> +		}
>> >> +		if (addr < instr->ctx.addr.naddrs) {
>> >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
>> >> +			code |= COMMAND_RAR_BYTE3;
>> >> +		}
>> >> +
>> >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
>> >> +
>> >> +		instr = vf610_get_next_instr(subop, &op_id);
>> >> +	}
>> >> +
>> >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
>> >> +		int len = nand_subop_get_data_len(subop, op_id);
>> >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
>> >> +
>> >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
>> >> +
>> >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
>> >> +				 instr->ctx.data.buf.in + offset,
>> >> +				 len);
>> >
>> > I think you have the same endianness problem you have for the READ
>> > path. For example, I doubt SET_FEATURES will work properly if you're
>> > in LE. So I repeat my initial suggestion: always do the byte swapping
>> > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
>> > and use vf610_nfc_memcpy() only in the ->read/write_page()
>> > implementations.
>> >
>>
>> Hm, but doesn't that leads to wrong order of data when using e.g. raw
>> read/write page...?
> 
> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
> prefer that to having an ->exec_op() implementation that tries to guess
> what the core is trying to do.

Have implemented those callbacks and the driver mounts UBI just fine.
However, when trying to use the NAND tests this currently fails:

[   27.458447] =================================================
[   27.464427] mtd_oobtest: MTD device: 3
[   27.481006] mtd_oobtest: MTD device size 16777216, eraseblock size
131072, page size 2048, count of eraseblocks 128, pages per eraseblock
64, OOB size 64
[   27.495154] mtd_test: scanning for bad eraseblocks
[   27.500002] mtd_test: block 12 is bad
[   27.505482] mtd_test: scanned 128 eraseblocks, 1 are bad
[   27.510940] mtd_oobtest: test 1 of 5
[   27.587528] mtd_oobtest: writing OOBs of whole device
[   27.593113] mtd_oobtest: error: writeoob failed at 0x0
[   27.598274] mtd_oobtest: error: use_len 2, use_offset 0
[   27.605190] mtd_oobtest: error -5 occurred
[   27.609320] =================================================
insmod: ERROR: could not insert module mtd_oobtest.ko: Input/output
error
root@colibri-vf:~# insmod mtd_nandbiterrs.ko dev=3
[   30.075103]
[   30.076653] ==================================================
[   30.082770] mtd_nandbiterrs: MTD device: 3
[   30.100602] mtd_nandbiterrs: MTD device size 16777216,
eraseblock=131072, page=2048, oob=64
[   30.109019] mtd_nandbiterrs: Device uses 1 subpages of 2048 bytes
[   30.115260] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
[   30.123679] mtd_nandbiterrs: incremental biterrors test
[   30.129080] mtd_nandbiterrs: write_page
[   30.134474] mtd_nandbiterrs: rewrite page
[   30.138702] mtd_nandbiterrs: error: write_oob failed (-5)


I did overwrite ->{read,write}_{page,oob}[_raw]() and set ->page_access
in all of those, but still...

Do you have an idea what that could be? Otherwise will have to look
closer what might cause this.

--
Stefan

Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Thu, 22 Feb 2018 00:23:15 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 21.02.2018 09:28, Boris Brezillon wrote:
> > On Wed, 21 Feb 2018 00:15:18 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> >> >> +		}
> >> >> +
> >> >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> >> +		code |= COMMAND_RAR_BYTE1;
> >> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> >> +			code |= COMMAND_RAR_BYTE2;
> >> >> +		}
> >> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> >> >> +			code |= COMMAND_RAR_BYTE3;
> >> >> +		}
> >> >> +
> >> >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> >> >> +
> >> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> >> +	}
> >> >> +
> >> >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> >> >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> >> +
> >> >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> >> >> +
> >> >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> >> +				 instr->ctx.data.buf.in + offset,
> >> >> +				 len);  
> >> >
> >> > I think you have the same endianness problem you have for the READ
> >> > path. For example, I doubt SET_FEATURES will work properly if you're
> >> > in LE. So I repeat my initial suggestion: always do the byte swapping
> >> > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> >> > and use vf610_nfc_memcpy() only in the ->read/write_page()
> >> > implementations.
> >> >  
> >>
> >> Hm, but doesn't that leads to wrong order of data when using e.g. raw
> >> read/write page...?  
> > 
> > Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
> > prefer that to having an ->exec_op() implementation that tries to guess
> > what the core is trying to do.  
> 
> Have implemented those callbacks and the driver mounts UBI just fine.
> However, when trying to use the NAND tests this currently fails:
> 
> [   27.458447] =================================================
> [   27.464427] mtd_oobtest: MTD device: 3
> [   27.481006] mtd_oobtest: MTD device size 16777216, eraseblock size
> 131072, page size 2048, count of eraseblocks 128, pages per eraseblock
> 64, OOB size 64
> [   27.495154] mtd_test: scanning for bad eraseblocks
> [   27.500002] mtd_test: block 12 is bad
> [   27.505482] mtd_test: scanned 128 eraseblocks, 1 are bad
> [   27.510940] mtd_oobtest: test 1 of 5
> [   27.587528] mtd_oobtest: writing OOBs of whole device
> [   27.593113] mtd_oobtest: error: writeoob failed at 0x0
> [   27.598274] mtd_oobtest: error: use_len 2, use_offset 0
> [   27.605190] mtd_oobtest: error -5 occurred
> [   27.609320] =================================================
> insmod: ERROR: could not insert module mtd_oobtest.ko: Input/output
> error
> root@colibri-vf:~# insmod mtd_nandbiterrs.ko dev=3
> [   30.075103]
> [   30.076653] ==================================================
> [   30.082770] mtd_nandbiterrs: MTD device: 3
> [   30.100602] mtd_nandbiterrs: MTD device size 16777216,
> eraseblock=131072, page=2048, oob=64
> [   30.109019] mtd_nandbiterrs: Device uses 1 subpages of 2048 bytes
> [   30.115260] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
> [   30.123679] mtd_nandbiterrs: incremental biterrors test
> [   30.129080] mtd_nandbiterrs: write_page
> [   30.134474] mtd_nandbiterrs: rewrite page
> [   30.138702] mtd_nandbiterrs: error: write_oob failed (-5)
> 
> 
> I did overwrite ->{read,write}_{page,oob}[_raw]() and set ->page_access
> in all of those, but still...
> 
> Do you have an idea what that could be? Otherwise will have to look
> closer what might cause this.

Look like your ->write_oob() method is returning -EIO, but I can't help
you without the code. Can you push it on a public git repo?


-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[RFC PATCH v3 3/3] mtd: nand: vf610_nfc: remove old hooks

[Stefan Agner]

Now that the driver is using ->exec_op(), remove the old
hooks.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/vf610_nfc.c | 255 -------------------------------------------
 1 file changed, 255 deletions(-)

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index eae95877422d..ee80a3005adb 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -59,21 +59,6 @@
 #define OOB_64				0x0040
 #define OOB_MAX				0x0100
 
-/*
- * NFC_CMD2[CODE] values. See section:
- *  - 31.4.7 Flash Command Code Description, Vybrid manual
- *  - 23.8.6 Flash Command Sequencer, MPC5125 manual
- *
- * Briefly these are bitmasks of controller cycles.
- */
-#define READ_PAGE_CMD_CODE		0x7EE0
-#define READ_ONFI_PARAM_CMD_CODE	0x4860
-#define PROGRAM_PAGE_CMD_CODE		0x7FC0
-#define ERASE_CMD_CODE			0x4EC0
-#define READ_ID_CMD_CODE		0x4804
-#define RESET_CMD_CODE			0x4040
-#define STATUS_READ_CMD_CODE		0x4068
-
 /* NFC_CMD2[CODE] controller cycle bit masks */
 #define COMMAND_CMD_BYTE1		BIT(14)
 #define COMMAND_CAR_BYTE1		BIT(13)
@@ -161,13 +146,6 @@
 #define ECC_STATUS_MASK		0x80
 #define ECC_STATUS_ERR_COUNT	0x3F
 
-enum vf610_nfc_alt_buf {
-	ALT_BUF_DATA = 0,
-	ALT_BUF_ID = 1,
-	ALT_BUF_STAT = 2,
-	ALT_BUF_ONFI = 3,
-};
-
 enum vf610_nfc_variant {
 	NFC_VFC610 = 1,
 };
@@ -177,13 +155,9 @@ struct vf610_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct completion cmd_done;
-	uint buf_offset;
-	int write_sz;
 	/* Status and ID are in alternate locations. */
-	enum vf610_nfc_alt_buf alt_buf;
 	enum vf610_nfc_variant variant;
 	struct clk *clk;
-	bool use_hw_ecc;
 	u32 ecc_mode;
 };
 
@@ -267,53 +241,6 @@ static void vf610_nfc_done(struct vf610_nfc *nfc)
 	vf610_nfc_clear_status(nfc);
 }
 
-static u8 vf610_nfc_get_id(struct vf610_nfc *nfc, int col)
-{
-	u32 flash_id;
-
-	if (col < 4) {
-		flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS1);
-		flash_id >>= (3 - col) * 8;
-	} else {
-		flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS2);
-		flash_id >>= 24;
-	}
-
-	return flash_id & 0xff;
-}
-
-static u8 vf610_nfc_get_status(struct vf610_nfc *nfc)
-{
-	return vf610_nfc_read(nfc, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK;
-}
-
-static void vf610_nfc_send_command(struct vf610_nfc *nfc, u32 cmd_byte1,
-				   u32 cmd_code)
-{
-	u32 tmp;
-
-	vf610_nfc_clear_status(nfc);
-
-	tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD2);
-	tmp &= ~(CMD_BYTE1_MASK | CMD_CODE_MASK | BUFNO_MASK);
-	tmp |= cmd_byte1 << CMD_BYTE1_SHIFT;
-	tmp |= cmd_code << CMD_CODE_SHIFT;
-	vf610_nfc_write(nfc, NFC_FLASH_CMD2, tmp);
-}
-
-static void vf610_nfc_send_commands(struct vf610_nfc *nfc, u32 cmd_byte1,
-				    u32 cmd_byte2, u32 cmd_code)
-{
-	u32 tmp;
-
-	vf610_nfc_send_command(nfc, cmd_byte1, cmd_code);
-
-	tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD1);
-	tmp &= ~CMD_BYTE2_MASK;
-	tmp |= cmd_byte2 << CMD_BYTE2_SHIFT;
-	vf610_nfc_write(nfc, NFC_FLASH_CMD1, tmp);
-}
-
 static irqreturn_t vf610_nfc_irq(int irq, void *data)
 {
 	struct mtd_info *mtd = data;
@@ -325,19 +252,6 @@ static irqreturn_t vf610_nfc_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void vf610_nfc_addr_cycle(struct vf610_nfc *nfc, int column, int page)
-{
-	if (column != -1) {
-		if (nfc->chip.options & NAND_BUSWIDTH_16)
-			column = column / 2;
-		vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
-				    COL_ADDR_SHIFT, column);
-	}
-	if (page != -1)
-		vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
-				    ROW_ADDR_SHIFT, page);
-}
-
 static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode)
 {
 	vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG,
@@ -350,169 +264,6 @@ static inline void vf610_nfc_transfer_size(struct vf610_nfc *nfc, int size)
 	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, size);
 }
 
-static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
-			      int column, int page)
-{
-	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
-	int trfr_sz = nfc->chip.options & NAND_BUSWIDTH_16 ? 1 : 0;
-
-	nfc->buf_offset = max(column, 0);
-	nfc->alt_buf = ALT_BUF_DATA;
-
-	switch (command) {
-	case NAND_CMD_SEQIN:
-		/* Use valid column/page from preread... */
-		vf610_nfc_addr_cycle(nfc, column, page);
-		nfc->buf_offset = 0;
-
-		/*
-		 * SEQIN => data => PAGEPROG sequence is done by the controller
-		 * hence we do not need to issue the command here...
-		 */
-		return;
-	case NAND_CMD_PAGEPROG:
-		trfr_sz += nfc->write_sz;
-		vf610_nfc_transfer_size(nfc, trfr_sz);
-		vf610_nfc_send_commands(nfc, NAND_CMD_SEQIN,
-					command, PROGRAM_PAGE_CMD_CODE);
-		if (nfc->use_hw_ecc)
-			vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
-		else
-			vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
-		break;
-
-	case NAND_CMD_RESET:
-		vf610_nfc_transfer_size(nfc, 0);
-		vf610_nfc_send_command(nfc, command, RESET_CMD_CODE);
-		break;
-
-	case NAND_CMD_READOOB:
-		trfr_sz += mtd->oobsize;
-		column = mtd->writesize;
-		vf610_nfc_transfer_size(nfc, trfr_sz);
-		vf610_nfc_send_commands(nfc, NAND_CMD_READ0,
-					NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
-		vf610_nfc_addr_cycle(nfc, column, page);
-		vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
-		break;
-
-	case NAND_CMD_READ0:
-		trfr_sz += mtd->writesize + mtd->oobsize;
-		vf610_nfc_transfer_size(nfc, trfr_sz);
-		vf610_nfc_send_commands(nfc, NAND_CMD_READ0,
-					NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
-		vf610_nfc_addr_cycle(nfc, column, page);
-		vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
-		break;
-
-	case NAND_CMD_PARAM:
-		nfc->alt_buf = ALT_BUF_ONFI;
-		trfr_sz = 3 * sizeof(struct nand_onfi_params);
-		vf610_nfc_transfer_size(nfc, trfr_sz);
-		vf610_nfc_send_command(nfc, command, READ_ONFI_PARAM_CMD_CODE);
-		vf610_nfc_addr_cycle(nfc, -1, column);
-		vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
-		break;
-
-	case NAND_CMD_ERASE1:
-		vf610_nfc_transfer_size(nfc, 0);
-		vf610_nfc_send_commands(nfc, command,
-					NAND_CMD_ERASE2, ERASE_CMD_CODE);
-		vf610_nfc_addr_cycle(nfc, column, page);
-		break;
-
-	case NAND_CMD_READID:
-		nfc->alt_buf = ALT_BUF_ID;
-		nfc->buf_offset = 0;
-		vf610_nfc_transfer_size(nfc, 0);
-		vf610_nfc_send_command(nfc, command, READ_ID_CMD_CODE);
-		vf610_nfc_addr_cycle(nfc, -1, column);
-		break;
-
-	case NAND_CMD_STATUS:
-		nfc->alt_buf = ALT_BUF_STAT;
-		vf610_nfc_transfer_size(nfc, 0);
-		vf610_nfc_send_command(nfc, command, STATUS_READ_CMD_CODE);
-		break;
-	default:
-		return;
-	}
-
-	vf610_nfc_done(nfc);
-
-	nfc->use_hw_ecc = false;
-	nfc->write_sz = 0;
-}
-
-static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len)
-{
-	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
-	uint c = nfc->buf_offset;
-
-	/* Alternate buffers are only supported through read_byte */
-	WARN_ON(nfc->alt_buf);
-
-	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, len);
-
-	nfc->buf_offset += len;
-}
-
-static void vf610_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
-				int len)
-{
-	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
-	uint c = nfc->buf_offset;
-	uint l;
-
-	l = min_t(uint, len, mtd->writesize + mtd->oobsize - c);
-	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
-
-	nfc->write_sz += l;
-	nfc->buf_offset += l;
-}
-
-static uint8_t vf610_nfc_read_byte(struct mtd_info *mtd)
-{
-	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
-	u8 tmp;
-	uint c = nfc->buf_offset;
-
-	switch (nfc->alt_buf) {
-	case ALT_BUF_ID:
-		tmp = vf610_nfc_get_id(nfc, c);
-		break;
-	case ALT_BUF_STAT:
-		tmp = vf610_nfc_get_status(nfc);
-		break;
-#ifdef __LITTLE_ENDIAN
-	case ALT_BUF_ONFI:
-		/* Reverse byte since the controller uses big endianness */
-		c = nfc->buf_offset ^ 0x3;
-		/* fall-through */
-#endif
-	default:
-		tmp = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
-		break;
-	}
-	nfc->buf_offset++;
-	return tmp;
-}
-
-static u16 vf610_nfc_read_word(struct mtd_info *mtd)
-{
-	u16 tmp;
-
-	vf610_nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp));
-	return tmp;
-}
-
-/* If not provided, upper layers apply a fixed delay. */
-static int vf610_nfc_dev_ready(struct mtd_info *mtd)
-{
-	/* NFC handles R/B internally; always ready.  */
-	return 1;
-}
-
 static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
 {
 	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
@@ -934,12 +685,6 @@ static int vf610_nfc_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
-	chip->dev_ready = vf610_nfc_dev_ready;
-	chip->cmdfunc = vf610_nfc_command;
-	chip->read_byte = vf610_nfc_read_byte;
-	chip->read_word = vf610_nfc_read_word;
-	chip->read_buf = vf610_nfc_read_buf;
-	chip->write_buf = vf610_nfc_write_buf;
 	chip->exec_op = vf610_nfc_exec_op;
 	chip->select_chip = vf610_nfc_select_chip;
 	chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
-- 
2.16.1

Re: [RFC PATCH v3 0/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon]

On Fri,  9 Feb 2018 00:59:18 +0100
Stefan Agner <stefan@agner.ch> wrote:

> Third revision of the rework patchset to use exec_op for NXP
> Vybrid (and others) NAND Flash Controller.

Glad to see you didn't give up on that :-). It looks pretty good
already, just have some comments on patch 2, but I think you can drop
the RFC prefix on your next version.

> 
> I now avoided calling back into the stack for the ECC read/
> write pages. This increases speed 4469 KiB/s write speed and
> 13490 KiB/s read speed (v2 was 3495 KiB/s/13490 KiB/s).

Nice improvement on the write path.

> 
> IMHO it start to look better, probably still needs some fine
> tuning. What I don't like too much is that the custom
> read/write page accessors dupplicate some code, maybe this
> could be done somewhat nicer.

Hm, just had a look and I don't find there's that much code duplicated
here, but if you find a solution to share even more code I won't
complain ;-).

> 
> --
> Stefan
> 
> Changes in v3:
> - Separate exec_op() callback addition and removal of old callbacks
> - Push data into regs in one function
> - Readd op parser
> - Implement custom read/write page for hardware ECC
> - Rely on generic ecc.write_page_raw
> - Use nand_read_oob_op instead of nand_read_page_op
> 
> Stefan Agner (3):
>   mtd: nand: vf610_nfc: remove unused function
>   mtd: nand: vf610_nfc: make use of ->exec_op()
>   mtd: nand: vf610_nfc: remove old hooks
> 
>  drivers/mtd/nand/vf610_nfc.c | 493 +++++++++++++++++++++----------------------
>  1 file changed, 239 insertions(+), 254 deletions(-)
> 



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com