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

Stefan Agner stefan at agner.ch
Sun Feb 25 23:48:36 PST 2018


On 22.02.2018 23:06, Boris Brezillon wrote:
> On Thu, 22 Feb 2018 21:29:17 +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>
>> ---
>>  drivers/mtd/nand/raw/vf610_nfc.c | 439 +++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 425 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
>> index 5d7a1f8f580f..9baf80766307 100644
>> --- a/drivers/mtd/nand/raw/vf610_nfc.c
>> +++ b/drivers/mtd/nand/raw/vf610_nfc.c
>> @@ -74,6 +74,22 @@
>>  #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_NADDR_BYTES(x)		GENMASK(13, 13 - (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
>>  #define ECC_45_BYTE			6
>> @@ -97,10 +113,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
>> @@ -165,6 +185,7 @@ struct vf610_nfc {
>>  	enum vf610_nfc_variant variant;
>>  	struct clk *clk;
>>  	bool use_hw_ecc;
>> +	bool page_access;
> 
> This field deserves a comment IMO, even if you describe in details what
> it does in the rd/wr_from/to_sram() funcs.
> 
>>  	u32 ecc_mode;
>>  };
>>
>> @@ -173,6 +194,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);
>> @@ -214,6 +240,86 @@ static inline void vf610_nfc_memcpy(void *dst, const void __iomem *src,
>>  	memcpy(dst, src, n);
>>  }
>>
>> +static inline bool vf610_nfc_is_little_endian(void)
> 
> It's not really the NFC that is LE, is it? I mean, you could have the
> kernel configured in BIG ENDIAN for this platform, and then you wouldn't
> have to do the byte-swapping.
> 
>> +{
>> +#ifdef __LITTLE_ENDIAN
>> +	return true;
>> +#else
>> +	return false;
>> +#endif
>> +}
>> +
>> +/**
>> + * Read accessor for internal SRAM buffer
>> + * @dst: destination address in regular memory
>> + * @src: source address in SRAM buffer
>> + * @len: bytes to copy
>> + * @fix_endian: Fix endianness if required
>> + *
>> + * Use this accessor for the internal SRAM buffers. On the ARM
>> + * Freescale Vybrid SoC it's known that the driver can treat
>> + * the SRAM buffer as if it's memory. Other platform might need
>> + * to treat the buffers differently.
>> + *
>> + * The controller stores bytes from the NAND chip internally in big
>> + * endianness. On little endian platforms such as Vybrid this leads
>> + * to reversed byte order.
>> + * For performance reason (and earlier probably due to unanawareness)
> 
> 							  ^ unawareness
> 
>> + * the driver avoids correcting endianness where it has control over
>> + * write and read side (e.g. page wise data access).
>> + * In case endianness matters len should be a multiple of 4.
>> + */
>> +static inline void vf610_nfc_rd_from_sram(void *dst, const void __iomem *src,
>> +					  size_t len, bool fix_endian)
>> +{
>> +	if (vf610_nfc_is_little_endian() && fix_endian) {
>> +		unsigned int i;
>> +
>> +		for (i = 0; i < len; i += 4) {
>> +			u32 val = be32_to_cpu(__raw_readl(src + i));
>> +			memcpy(dst + i, &val, min(sizeof(val), len - i));
>> +		}
>> +	} else {
>> +		memcpy_fromio(dst, src, len);
>> +	}
>> +}
>> +
>> +/**
>> + * Write accessor for internal SRAM buffer
>> + * @dst: destination address in SRAM buffer
>> + * @src: source address in regular memory
>> + * @len: bytes to copy
>> + * @fix_endian: Fix endianness if required
>> + *
>> + * Use this accessor for the internal SRAM buffers. On the ARM
>> + * Freescale Vybrid SoC it's known that the driver can treat
>> + * the SRAM buffer as if it's memory. Other platform might need
>> + * to treat the buffers differently.
>> + *
>> + * The controller stores bytes from the NAND chip internally in big
>> + * endianness. On little endian platforms such as Vybrid this leads
>> + * to reversed byte order.
>> + * For performance reason (and earlier probably due to unanawareness)
> 
> 							  ^ unawareness
> 
>> + * the driver avoids correcting endianness where it has control over
>> + * write and read side (e.g. page wise data access).
>> + * In case endianness matters len should be a multiple of 4.
> 
> That's the opposite actually. If fix_endian is set, we don't have any
> requirement on len alignment, because the cpu_to_be32(val) will save
> us. But we have a problem when fix_endian is false an len is not
> aligned on 4, because, AFAIR memcpy_toio() does 32-bit accesses when
> things are properly aligned on 32 bits, and when that's not the case it
> falls back to 16 or 8 bit accesses, which in our case may lead to data
> loss on LE platforms.
> 
>> + */
>> +static inline void vf610_nfc_wr_to_sram(void __iomem *dst, const void *src,
>> +					size_t len, bool fix_endian)
>> +{
>> +	if (vf610_nfc_is_little_endian() && fix_endian) {
>> +		unsigned int i;
>> +
>> +		for (i = 0; i < len; i += 4) {
>> +			u32 val;
>> +			memcpy(&val, src + i, min(sizeof(val), len - i));
>> +			__raw_writel(cpu_to_be32(val), dst + i);
>> +		}
>> +	} else {
>> +		memcpy_toio(dst, src, len);
>> +	}
>> +}
>> +
>>  /* Clear flags for upcoming command */
>>  static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc)
>>  {
>> @@ -489,6 +595,170 @@ 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);
>> +	int op_id = -1, trfr_sz = 0, offset;
>> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> +	bool force8bit = false;
>> +
>> +	/*
>> +	 * Some ops are optional, but the hardware requires the operations
>> +	 * to be in this exact order.
>> +	 * The op parser enforces the order and makes sure that there isn't
>> +	 * a read and write element in a single operation.
>> +	 */
>> +	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) {
>> +		int naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
>> +		int i = nand_subop_get_addr_start_off(subop, op_id);
>> +
>> +		for (; i < naddrs; i++) {
>> +			u8 val = instr->ctx.addr.addrs[i];
>> +			if (i < 2)
>> +				col |= COL_ADDR(i, val);
>> +			else
>> +				row |= ROW_ADDR(i - 2, val);
>> +		}
>> +		code |= COMMAND_NADDR_BYTES(naddrs);
>> +
>> +		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) {
>> +		trfr_sz = nand_subop_get_data_len(subop, op_id);
>> +		offset = nand_subop_get_data_start_off(subop, op_id);
>> +		force8bit = instr->ctx.data.force_8bit;
>> +
>> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n",
>> +			trfr_sz, offset);
>> +
>> +		/* We don't care about endianness when writing a NAND page */
>> +		vf610_nfc_wr_to_sram(nfc->regs + NFC_MAIN_AREA(0) + offset,
>> +				     instr->ctx.data.buf.in + offset,
>> +				     trfr_sz, !nfc->page_access);
>> +		code |= COMMAND_WRITE_DATA;
>> +
>> +		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);
> 
> You seem to have debug traces almost everywhere except here and...
> 
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_WAITRDY_INSTR) {
>> +		code |= COMMAND_RB_HANDSHAKE;
>> +
>> +		instr = vf610_get_next_instr(subop, &op_id);
> 
> ... here. Can we please make that consistent. Either drop all of them
> or add the missing ones.
> 
> IMHO it's more interesting to have a dev_dbg() giving the col, row,
> cmd1, cmd2 and trfr_sz values in vf610_nfc_run() rather than those you
> add here, because they kind of duplicate the information given in
> nand_op_parser_trace().
> 
>> +	}
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
>> +		trfr_sz = nand_subop_get_data_len(subop, op_id);
>> +		offset = nand_subop_get_data_start_off(subop, op_id);
>> +		force8bit = instr->ctx.data.force_8bit;
>> +
>> +		dev_dbg(nfc->dev, "OP_DATA_IN: len %d, offset %d\n",
>> +			trfr_sz, offset);
>> +
>> +		code |= COMMAND_READ_DATA;
>> +	}
> 
> Still not a big fan of the
> 
> 	if (instr && instr->type == NAND_OP_XXX_INSTR) {
> 		...
> 		instr = vf610_get_next_instr(subop, &op_id);
> 	}
> 
> approach, but I'm ready to make a concession on that :-).
> 
>> +
>> +	if (force8bit && (chip->options & NAND_BUSWIDTH_16))
>> +		vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
>> +
>> +	cmd2 |= code << CMD_CODE_SHIFT;
>> +
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> +
>> +	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
>> +		/* We don't care about endianness when reading a NAND page */
>> +		vf610_nfc_rd_from_sram(instr->ctx.data.buf.in + offset,
>> +				       nfc->regs + NFC_MAIN_AREA(0) + offset,
>> +				       trfr_sz, !nfc->page_access);
>> +	}
>> +
>> +	if (force8bit && (chip->options & NAND_BUSWIDTH_16))
>> +		vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
>> +
>> +	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_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)),
>> +	);
>> +
>> +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);
> 
> Bad idea: according to the pattern list you described above the first
> instruction is not necessarily a CMD instruction. I think you should
> drop this dev_dbg().
> 
>> +
>> +	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)
>>   */
>> @@ -526,9 +796,14 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>>  	if (!(ecc_status & ECC_STATUS_MASK))
>>  		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);
>> +	/*
>> +	 * Read OOB without ECC unit enabled. We temporarily set ->page_access
>> +	 * to true to make sure vf610_nfc_cmd() does not swap bytes when
>> +	 * reading data from the internal SRAM.
>> +	 */
>> +	nfc->page_access = true;
>> +	nand_read_oob_op(&nfc->chip, page, 0, oob, mtd->oobsize);
>> +	nfc->page_access = false;
>>
>>  	/*
>>  	 * On an erased page, bit count (including OOB) should be zero or
>> @@ -542,12 +817,46 @@ 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_ecc_mode(nfc, ECC_BYPASS);
>> +
>> +	/* We don't care about endianness when reading a NAND page */
>> +	vf610_nfc_rd_from_sram(buf, nfc->regs + NFC_MAIN_AREA(0),
>> +			       mtd->writesize, false);
>>  	if (oob_required)
>> -		vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +		vf610_nfc_rd_from_sram(chip->oob_poi,
>> +				       nfc->regs + NFC_MAIN_AREA(0) +
>> +						   mtd->writesize,
>> +				       mtd->oobsize, false);
>>
>>  	stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
>>
>> @@ -564,16 +873,113 @@ 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;
>> +
>> +	/* We don't care about endianness when writing a NAND page */
>> +	vf610_nfc_wr_to_sram(nfc->regs + NFC_MAIN_AREA(0), buf,
>> +			     mtd->writesize, false);
>>
>> -	/* Always write whole page including OOB due to HW ECC */
>> -	nfc->use_hw_ecc = true;
>> -	nfc->write_sz = mtd->writesize + mtd->oobsize;
>> +	code |= COMMAND_RB_HANDSHAKE;
>> +	cmd2 |= code << CMD_CODE_SHIFT;
>>
>> -	return nand_prog_page_end_op(chip);
>> +	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> +	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vf610_nfc_read_page_raw(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 ret;
>> +
>> +	/*
>> +	 * We temporarily set ->page_access to true to make sure
>> +	 * vf610_nfc_cmd() does not swap bytes when reading data
>> +	 * from the internal SRAM.
>> +	 */
>> +	nfc->page_access = true;
>> +	ret = nand_read_page_raw(mtd, chip, buf, oob_required, page);
>> +	nfc->page_access = false;
>> +
>> +	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)
>> +{
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int ret;
>> +
>> +	/*
>> +	 * We temporarily set ->page_access to true to make sure
>> +	 * vf610_nfc_cmd() does not swap bytes when reading data
> 
> 						    ^ writing data to
> 
>> +	 * from the internal SRAM.
>> +	 */
>> +	nfc->page_access = true;
>> +	ret = nand_write_page_raw(mtd, chip, buf, oob_required, page);
>> +	nfc->page_access = false;
>> +
>> +	return ret;
> 
> As you discovered while debugging the new implementation, this doesn't
> work, because then the STATUS byte is read without the proper
> byte-swapping enabled. So the solution would be to replace the above
> code by:
> 
> 	nfc->page_access = true;
> 	ret = nand_prog_page_begin_op(chip, page, 0, buf,
> 				      mtd->writesize);
> 	if (!ret && oob_required)
> 		ret = nand_write_data_op(chip, chip->oob_poi,
> 					 mtd->oobsize, false);
> 	nfc->page_access = false;
> 
> 	if (ret)
> 		return ret;
> 
> 	return nand_prog_page_end_op(chip);
> 
>> +}
>> +
>> +static int vf610_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>> +			int page)
>> +{
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int ret;
>> +
>> +	/*
>> +	 * We temporarily set ->page_access to true to make sure
>> +	 * vf610_nfc_cmd() does not swap bytes when reading data
>> +	 * from the internal SRAM.
>> +	 */
>> +	nfc->page_access = true;
>> +	ret = nand_read_oob_std(mtd, chip, page);
>> +	nfc->page_access = false;
>> +
>> +	return ret;
>> +}
>> +static int vf610_nfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
>> +			int page)
>> +{
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	int ret;
>> +
>> +	/*
>> +	 * We temporarily set ->page_access to true to make sure
>> +	 * vf610_nfc_cmd() does not swap bytes when reading data
>> +	 * from the internal SRAM.
>> +	 */
>> +	nfc->page_access = true;
>> +	ret = nand_write_oob_std(mtd, chip, page);
>> +	nfc->page_access = false;
>> +
>> +	return ret;
> 
> Same problem here. The above logic should be replaced by:
> 
> 	nfc->page_access = true;
> 	ret = nand_prog_page_begin_op(chip, page, mtd->writesize,
> 				      chip->oob_poi, mtd->oobsize);
> 
> 	nfc->page_access = false;
> 
> 	if (ret)
> 		return ret;
> 
> 	return nand_prog_page_end_op(chip);
> 

Read/write seems to work, but there seems to be an issue with data:

root at colibri-vf:~# insmod mtd_oobtest.ko dev=3
[   69.609120]
[   69.610664] =================================================
[   69.616734] mtd_oobtest: MTD device: 3
[   69.631659] mtd_oobtest: MTD device size 16777216, eraseblock size
131072, page size 2048, count of eraseblocks 128, pages per eraseblock
64, OOB size 64
[   69.647814] mtd_test: scanning for bad eraseblocks
[   69.654166] mtd_test: scanned 128 eraseblocks, 0 are bad
[   69.659507] mtd_oobtest: test 1 of 5
[   69.736812] mtd_oobtest: writing OOBs of whole device
[   69.755753] mtd_oobtest: written up to eraseblock 0
[   71.500646] mtd_oobtest: written 128 eraseblocks
[   71.505397] mtd_oobtest: verifying all eraseblocks
[   71.510336] mtd_oobtest: error @addr[0x0:0x0] 0xff -> 0xe diff 0xf1
[   71.516727] mtd_oobtest: error @addr[0x0:0x1] 0xff -> 0x10 diff 0xef
[   71.523164] mtd_oobtest: error: verify failed at 0x0
[   71.530372] mtd_oobtest: error @addr[0x800:0x0] 0xff -> 0x82 diff
0x7d
[   71.537083] mtd_oobtest: error @addr[0x800:0x1] 0xff -> 0x10 diff
0xef
[   71.543709] mtd_oobtest: error: verify failed at 0x800

Need to check what is exactly going wrong.

--
Stefan

>>  }
>>
>>  static const struct of_device_id vf610_nfc_dt_ids[] = {
>> @@ -590,6 +996,7 @@ static void vf610_nfc_preinit_controller(struct vf610_nfc *nfc)
>>  	vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_BOOT_MODE_BIT);
>>  	vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT);
>>  	vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT);
>> +	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
>>
>>  	/* Disable virtual pages, only one elementary transfer unit */
>>  	vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
>> @@ -686,6 +1093,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;
>> @@ -755,7 +1163,10 @@ 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.read_page_raw = vf610_nfc_read_page_raw;
>> +		chip->ecc.write_page_raw = vf610_nfc_write_page_raw;
>> +		chip->ecc.read_oob = vf610_nfc_read_oob;
>> +		chip->ecc.write_oob = vf610_nfc_write_oob;
>>  		chip->ecc.size = PAGE_2K;
>>  	}
>>



More information about the linux-mtd mailing list