[RFC PATCH v2] mtd: nand: vf610_nfc: make use of ->exec_op()

Boris Brezillon boris.brezillon at free-electrons.com
Mon Jan 15 14:07:16 PST 2018


On Sun, 14 Jan 2018 23:00:12 +0100
Stefan Agner <stefan at 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 at agner.ch>
> ---
> Hi,
> 
> In this second version I was able to remove all special command
> handling. As Boris suggested, the special READID/STATUS handling
> is really not needed and all return values can be read from the
> regular data buffer. This simplified code even more and allowed
> to get rid of the hack in the MTD core. The driver is now able
> to handle all commands in a single function. Therefor I also got
> rid of the command parser, since everything is handled in a single
> function anyway.
> 
> The only thing which is somewhat unfortunate is the byte ordering
> which needs to be reversed for commands which read flash data (id,
> status and parameter page). All those request 8-bit handling, so
> I used this as an indicator to fix byte order.
> 
> I did meassured a slight performance drop (before vs. now):
> - write speed is 4036 KiB/s vs 3495 KiB/s
> - read speed is 13741 KiB/s vs 13490 KiB/s
> 
> --
> Stefan
> 
>  drivers/mtd/nand/vf610_nfc.c | 444 +++++++++++++++++++------------------------
>  1 file changed, 194 insertions(+), 250 deletions(-)
> 
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> index 80d31a58e558..fb428a94bbd4 100644
> --- a/drivers/mtd/nand/vf610_nfc.c
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -59,20 +59,20 @@
>  #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)
> +#define COMMAND_CAR_BYTE2		BIT(12)
> +#define COMMAND_RAR_BYTE1		BIT(11)
> +#define COMMAND_RAR_BYTE2		BIT(10)
> +#define COMMAND_RAR_BYTE3		BIT(9)

ADDRESS cycles will always be specified in little-endian (column
cycles first and then row cycles), so maybe you can do:

#define COMMAND_NADDR_CYCLES(x)		GENMASK(14, 14 - (x) - 1)

> +#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
> @@ -97,10 +97,21 @@
>  /* NFC_COL_ADDR Field */
>  #define COL_ADDR_MASK				0x0000FFFF
>  #define COL_ADDR_SHIFT				0
> +#define COL_ADDR_BYTE1_SHIFT			0
> +#define COL_ADDR_BYTE2_SHIFT			8
> +#define COL_ADDR_BYTE1(x)			((x & 0xFF) << COL_ADDR_BYTE1_SHIFT)
> +#define COL_ADDR_BYTE2(x)			((x & 0XFF) << COL_ADDR_BYTE2_SHIFT)

Or just

#define COL_ADDR(pos, val)		((val) << (8 * (pos))) 

> +
>  
>  /* NFC_ROW_ADDR Field */
>  #define ROW_ADDR_MASK				0x00FFFFFF
>  #define ROW_ADDR_SHIFT				0
> +#define ROW_ADDR_BYTE1_SHIFT			0
> +#define ROW_ADDR_BYTE2_SHIFT			8
> +#define ROW_ADDR_BYTE3_SHIFT			16
> +#define ROW_ADDR_BYTE1(x)			((x & 0xFF) << ROW_ADDR_BYTE1_SHIFT)
> +#define ROW_ADDR_BYTE2(x)			((x & 0XFF) << ROW_ADDR_BYTE2_SHIFT)
> +#define ROW_ADDR_BYTE3(x)			((x & 0XFF) << ROW_ADDR_BYTE3_SHIFT)

Same here:

#define ROW_ADDR(pos, val)		((val) << (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
> @@ -142,13 +153,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,
>  };
> @@ -158,10 +162,7 @@ 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;
> @@ -173,6 +174,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);
> +}

Not related to this patch, but the chip and nfc objects should be
separated, and the NFC object should inherit from nand_hw_control.

> +
>  static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
>  {
>  	return readl(nfc->regs + reg);
> @@ -214,7 +220,6 @@ static inline void vf610_nfc_memcpy(void *dst, const void __iomem *src,
>  	memcpy(dst, src, n);
>  }
>  
> -/* Clear flags for upcoming command */
>  static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc)
>  {
>  	u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS);
> @@ -243,51 +248,22 @@ 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)
> +static void vf610_nfc_send_code(struct vf610_nfc *nfc, u32 cmd_code)
>  {
> -	return vf610_nfc_read(nfc, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK;
> +	vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_CODE_MASK, CMD_CODE_SHIFT,
> +			    cmd_code);
>  }
>  
> -static void vf610_nfc_send_command(struct vf610_nfc *nfc, u32 cmd_byte1,
> -				   u32 cmd_code)
> +static void vf610_nfc_set_cmd1(struct vf610_nfc *nfc, u32 cmd_byte1)
>  {
> -	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);
> +	vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_BYTE1_MASK,
> +			    CMD_BYTE1_SHIFT, cmd_byte1);
>  }
>  
> -static void vf610_nfc_send_commands(struct vf610_nfc *nfc, u32 cmd_byte1,
> -				    u32 cmd_byte2, u32 cmd_code)
> +static void vf610_nfc_set_cmd2(struct vf610_nfc *nfc, u32 cmd_byte2)
>  {
> -	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);
> +	vf610_nfc_set_field(nfc, NFC_FLASH_CMD1, CMD_BYTE2_MASK,
> +			    CMD_BYTE2_SHIFT, cmd_byte2);

Hm, you should prepare all reg values in an intermediate object, and then
push everything to the regs once the whole operation is ready to be
submitted. This set_field() might be one the reason you get better perfs
before this rework.

>  }
>  
>  static irqreturn_t vf610_nfc_irq(int irq, void *data)
> @@ -301,19 +277,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,
> @@ -326,167 +289,158 @@ 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)
> +static inline const struct nand_op_instr *vf610_get_next_instr(
> +	const struct nand_operation *op, int *op_id)
>  {
> -	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> -	int trfr_sz = nfc->chip.options & NAND_BUSWIDTH_16 ? 1 : 0;
> +	if (*op_id + 1 >= op->ninstrs)
> +		return NULL;
>  
> -	nfc->buf_offset = max(column, 0);
> -	nfc->alt_buf = ALT_BUF_DATA;
> +	return &op->instrs[++(*op_id)];
> +}
>  
> -	switch (command) {
> -	case NAND_CMD_SEQIN:
> -		/* Use valid column/page from preread... */
> -		vf610_nfc_addr_cycle(nfc, column, page);
> -		nfc->buf_offset = 0;
> +static int vf610_nfc_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op,
> +			     bool check_only)
> +{
> +	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, trfr_sz = 0, code = 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;
> +
> +	/* Some ops are optional, but they need to be in order */
> +	instr = vf610_get_next_instr(op, &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);
> +		vf610_nfc_set_cmd1(nfc, instr->ctx.cmd.opcode);
> +		code |= COMMAND_CMD_BYTE1;
> +
> +		instr = vf610_get_next_instr(op, &op_id);
>  	}
>  
> -	vf610_nfc_done(nfc);
> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> +		int addr = 0;
> +		u32 col = 0, row;
>  
> -	nfc->use_hw_ecc = false;
> -	nfc->write_sz = 0;
> -}
> +		if (instr->ctx.addr.naddrs > 1) {
> +			col = COL_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_CAR_BYTE1;
>  
> -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;
> +			if (mtd->writesize > 512) {
> +				col |= COL_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]);
> +				code |= COMMAND_CAR_BYTE2;
> +			}
> +		}
>  
> -	/* Alternate buffers are only supported through read_byte */
> -	WARN_ON(nfc->alt_buf);
> +		row = ROW_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]);
> +		code |= COMMAND_RAR_BYTE1;
> +		if (addr < instr->ctx.addr.naddrs) {
> +			row |= ROW_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_RAR_BYTE2;
> +		}
> +		if (addr < instr->ctx.addr.naddrs) {
> +			row |= ROW_ADDR_BYTE3(instr->ctx.addr.addrs[addr++]);
> +			code |= COMMAND_RAR_BYTE3;
> +		}
>  
> -	vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, len);
> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> +		vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> +			    COL_ADDR_SHIFT, col);
>  
> -	nfc->buf_offset += len;
> -}
> +		vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> +			    ROW_ADDR_SHIFT, row);
>  
> -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;
> +		instr = vf610_get_next_instr(op, &op_id);
> +	}
>  
> -	l = min_t(uint, len, mtd->writesize + mtd->oobsize - c);
> -	vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> +		int len = instr->ctx.data.len;
> +		int offset = 0;
>  
> -	nfc->write_sz += l;
> -	nfc->buf_offset += l;
> -}
> +		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(op, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> +		vf610_nfc_set_cmd2(nfc, instr->ctx.cmd.opcode);
> +		code |= COMMAND_CMD_BYTE2;
> +
> +		instr = vf610_get_next_instr(op, &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(op, &op_id);
> +	}
> +
> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> +		int len = instr->ctx.data.len;
> +		code |= COMMAND_READ_DATA;
> +		trfr_sz += len;
> +	}
> +
> +	if (nfc->use_hw_ecc) {
> +		vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> +
> +		/* Always transfer complete page with OOB when using HW ECC */
> +		trfr_sz = mtd->writesize + mtd->oobsize;
> +	} else {
> +		vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> +	}
> +	vf610_nfc_transfer_size(nfc, trfr_sz);
> +	vf610_nfc_send_code(nfc, code);
> +
> +	dev_dbg(nfc->dev, "Start: code: 0x%04x, hw ecc: %d, trfr_sz %d\n",
> +		code, nfc->use_hw_ecc, trfr_sz);
> +	vf610_nfc_done(nfc);
> +
> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
> +		int len = instr->ctx.data.len;
> +		int offset = 0;
> +		bool fix_byte_order = false;
>  
> -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 */
> +		fix_byte_order = true;
>  #endif
> -	default:
> -		tmp = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
> -		break;
> -	}
> -	nfc->buf_offset++;
> -	return tmp;
> -}
> +		dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n",
> +			instr->ctx.data.force_8bit , len, offset);
>  
> -static u16 vf610_nfc_read_word(struct mtd_info *mtd)
> -{
> -	u16 tmp;
> +		/*
> +		 * 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;
>  
> -	vf610_nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp));
> -	return tmp;
> -}
> +			while (len--) {
> +				int c = offset ^ 0x3;
>  
> -/* 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;
> +				*buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
> +				//dev_info(nfc->dev, "[%d]: %02x\n", offset, *buf);
> +				buf++; offset++;
> +			}
> +		} else {
> +			vf610_nfc_memcpy(instr->ctx.data.buf.in + offset,
> +					 nfc->regs + NFC_MAIN_AREA(0) + offset,
> +					 len);
> +		}
> +	}
> +
> +	return 0;
>  }

Hm, it's really hard to review things with these interleaved
deletions/additions. For the next version I recommend that you split things
in 2 steps:
1/ add ->exec_op()
2/ remove old hooks

Anyway, I think you should use the generic parser in order to simplify the
vf610_nfc_exec_op() logic and make it more robust/future-proof.

Note that you can describe several patterns that all points to the same
subop handler. This way the generic parser can isolate the sub-patterns
and pass them to your subop handler which no longer has to validate
instruction order.

>  
>  /*
> @@ -511,21 +465,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)
>  {
> @@ -542,8 +481,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_page_op(&nfc->chip, page, mtd->writesize, oob, mtd->oobsize);
>  
>  	/*
>  	 * On an erased page, bit count (including OOB) should be zero or
> @@ -557,12 +495,17 @@ 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)
>  {
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>  	int eccsize = chip->ecc.size;
>  	int stat;
>  
> +	nfc->use_hw_ecc = true;

Why not writing to NFC_CFG here instead of doing it your ->exec_op()
implem? Remember that ->exec_op() should be as dumb as possible.

>  	nand_read_page_op(chip, page, 0, buf, eccsize);
>  	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);

Hm, it should be done with the generic nand_read_data_op() helper. If
you want to handle page read differently in order to optimize this path,
you shouldn't be using nand_read_page_op() in the first place.

Anyway, when you call nand_read_page_op(), all that should go on the
bus are the CMD+ADDR cycles + eccsize bytes of DATA_IN. Given that you
copy things from the internal SRAM to the ->oob_poi buffer, i suspect
you're breaking the first and most important rule behind ->exec_op() =>
"don't try to be smart, just do what you were asked to do, and if you
can't, return an error".

> +	nfc->use_hw_ecc = false;
>  
>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
>  
> @@ -579,16 +522,20 @@ 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 ret;
>  
> -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
> -	if (oob_required)
> -		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
> -
> -	/* Always write whole page including OOB due to HW ECC */
>  	nfc->use_hw_ecc = true;
> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
> +	ret = nand_prog_page_op(chip, page, 0, buf, mtd->writesize);
> +	nfc->use_hw_ecc = false;

Same as above, looks like your ->exec_op() is too smart: it decides to
transfer OOB data even if it's not asked to.

I really think you want custom handling for page accessors, which is
perfectly fine, but then it shouldn't use the generic xxxx_op() helpers.

>  
> -	return nand_prog_page_end_op(chip);
> +	return ret;
> +}
> +
> +static int vf610_nfc_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> +				const uint8_t *buf, int oob_required, int page)
> +{
> +	return nand_prog_page_op(chip, page, 0, buf,
> +			mtd->writesize + oob_required ? mtd->oobsize : 0);
>  }
>  
>  static const struct of_device_id vf610_nfc_dt_ids[] = {
> @@ -606,6 +553,9 @@ static void vf610_nfc_preinit_controller(struct vf610_nfc *nfc)
>  	vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT);
>  	vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT);
>  
> +	/* Make sure flags are cleared on startup */
> +	vf610_nfc_clear_status(nfc);
> +
>  	/* Disable virtual pages, only one elementary transfer unit */
>  	vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
>  			    CONFIG_PAGE_CNT_SHIFT, 1);
> @@ -695,15 +645,8 @@ 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;
> -	chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>  
>  	chip->options |= NAND_NO_SUBPAGE_WRITE;
>  
> @@ -770,6 +713,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
>  
>  		chip->ecc.read_page = vf610_nfc_read_page;
>  		chip->ecc.write_page = vf610_nfc_write_page;
> +		chip->ecc.write_page_raw = vf610_nfc_write_page_raw;
>  
>  		chip->ecc.size = PAGE_2K;
>  	}




More information about the linux-mtd mailing list