[v6,2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

From: Liang Yang <liang.yang@amlogic.com>

Add initial support for the Amlogic NAND flash controller which found
in the Meson-GXBB/GXL/AXG SoCs.

Signed-off-by: Liang Yang <liang.yang@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
---
 drivers/mtd/nand/raw/Kconfig      |   10 +
 drivers/mtd/nand/raw/Makefile     |    1 +
 drivers/mtd/nand/raw/meson_nand.c | 1360 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1371 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/meson_nand.c

On Fri, 2 Nov 2018 00:42:21 +0800
Jianxin Pan <jianxin.pan@amlogic.com> wrote:

> +#define NFC_REG_CMD		0x00
> +#define NFC_CMD_DRD		(0x8 << 14)
> +#define NFC_CMD_IDLE		(0xc << 14)
> +#define NFC_CMD_DWR		(0x4 << 14)
> +#define NFC_CMD_CLE		(0x5 << 14)
> +#define NFC_CMD_ALE		(0x6 << 14)
> +#define NFC_CMD_ADL		((0 << 16) | (3 << 20))
> +#define NFC_CMD_ADH		((1 << 16) | (3 << 20))
> +#define NFC_CMD_AIL		((2 << 16) | (3 << 20))
> +#define NFC_CMD_AIH		((3 << 16) | (3 << 20))
> +#define NFC_CMD_SEED		((8 << 16) | (3 << 20))
> +#define NFC_CMD_M2N		((0 << 17) | (2 << 20))
> +#define NFC_CMD_N2M		((1 << 17) | (2 << 20))
> +#define NFC_CMD_RB		BIT(20)
> +#define NFC_CMD_IO6		((0xb << 10) | (1 << 18))
> +#define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
> +#define NFC_CMD_RB_INT		BIT(14)
> +
> +#define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
> +#define NFC_CMD_RB_TIMEOUT	0x18

Where does this value come from? Is it the typical timeout value,
and if it is, what's the value in milli/microseconds?

> +
> +#define NFC_REG_CFG		0x04
> +#define NFC_REG_DADR		0x08
> +#define NFC_REG_IADR		0x0c
> +#define NFC_REG_BUF		0x10
> +#define NFC_REG_INFO		0x14
> +#define NFC_REG_DC		0x18
> +#define NFC_REG_ADR		0x1c
> +#define NFC_REG_DL		0x20
> +#define NFC_REG_DH		0x24
> +#define NFC_REG_CADR		0x28
> +#define NFC_REG_SADR		0x2c
> +#define NFC_REG_PINS		0x30
> +#define NFC_REG_VER		0x38
> +
> +#define NFC_RB_IRQ_EN		BIT(21)
> +#define NFC_INT_MASK		(3 << 20)
> +
> +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)	\
> +	(								\
> +		(cmd_dir)			|			\
> +		((ran) << 19)			|			\
> +		((bch) << 14)			|			\
> +		((short_mode) << 13)		|			\
> +		(((page_size) & 0x7f) << 6)	|			\
> +		((pages) & 0x3f)					\
> +	)
> +
> +#define GENCMDDADDRL(adl, addr)		((adl) | ((addr) & 0xffff))
> +#define GENCMDDADDRH(adh, addr)		((adh) | (((addr) >> 16) & 0xffff))
> +#define GENCMDIADDRL(ail, addr)		((ail) | ((addr) & 0xffff))
> +#define GENCMDIADDRH(aih, addr)		((aih) | (((addr) >> 16) & 0xffff))
> +
> +#define RB_STA(x)		(1 << (26 + (x)))
> +#define DMA_DIR(dir)		((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
> +
> +#define ECC_CHECK_RETURN_FF	(-1)
> +
> +#define NAND_CE0		(0xe << 10)
> +#define NAND_CE1		(0xd << 10)
> +
> +#define DMA_BUSY_TIMEOUT	0x100000
> +#define CMD_FIFO_EMPTY_TIMEOUT	1000
> +
> +#define MAX_CE_NUM		2
> +
> +/* eMMC clock register, misc control */
> +#define SD_EMMC_CLOCK		0x00
> +#define CLK_ALWAYS_ON		BIT(28)
> +#define CLK_SELECT_NAND		BIT(31)
> +#define CLK_DIV_MASK		GENMASK(5, 0)
> +
> +#define NFC_CLK_CYCLE		6
> +
> +/* nand flash controller delay 3 ns */
> +#define NFC_DEFAULT_DELAY	3000
> +
> +#define MAX_ECC_INDEX		10
> +
> +#define MUX_CLK_NUM_PARENTS	2
> +
> +#define ROW_ADDER(page, index)	(((page) >> (8 * (index))) & 0xff)
> +#define MAX_CYCLE_ROW_ADDRS	3
> +#define MAX_CYCLE_COLUMN_ADDRS	2
> +#define DIRREAD			1
> +#define DIRWRITE		0
> +
> +#define ECC_PARITY_BCH8_512B	14
> +
> +#define PER_INFO_BYTE		8
> +#define ECC_GET_PROTECTED_OOB_BYTE(x, y)			\
> +		((x) >> (8 * (y)) & GENMASK(7, 0))

		(le64_to_cpu(x) >> (8 * (y)) & GENMASK(7, 0))

> +
> +#define ECC_SET_PROTECTED_OOB_BYTE(x, y, z)			\
> +	{							\
> +		(x) &= (~((__le64)(0xff) << (8 * (y))));	\

It's probably better to memset(0) the info_buf so that you can drop
this masking step.

> +		(x) |= ((__le64)(z) << (8 * (y)));		\

		    |= cpu_to_le64((u64)z << (8 * (y)));

> +	}
> +
> +#define ECC_COMPLETE            BIT(31)
> +#define ECC_ERR_CNT(x)		(((x) >> 24) & GENMASK(5, 0))
> +#define ECC_ZERO_CNT(x)		(((x) >> 16) & GENMASK(5, 0))
> +
> +struct meson_nfc_nand_chip {
> +	struct list_head node;
> +	struct nand_chip nand;
> +	int clk_rate;
> +	int level1_divider;
> +	int bus_timing;
> +	int twb;
> +	int tadl;
> +
> +	int bch_mode;
> +	u8 *data_buf;
> +	__le64 *info_buf;
> +	int nsels;
> +	u8 sels[0];
> +};

Please use unsigned integers where having a negative value is not
possible. I'm pretty sure this is the case of all int fields in this
struct. The same applies to the other structs.

[...]

> +
> +static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc,
> +				     unsigned int timeout_ms)
> +{
> +	u32 cmd_size = 0;
> +	int ret;
> +
> +	/* wait cmd fifo is empty */
> +	ret = readl_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size,
> +				 !NFC_CMD_GET_SIZE(cmd_size),
> +				 10, timeout_ms * 1000);

I guess you could use the relaxed version here.

> +	if (ret)
> +		dev_err(nfc->dev, "wait for empty cmd FIFO time out\n");
> +
> +	return ret;
> +}
> +
> +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc)
> +{
> +	meson_nfc_drain_cmd(nfc);
> +
> +	return meson_nfc_wait_cmd_finish(nfc, DMA_BUSY_TIMEOUT);
> +}
> +
> +static inline __le64 nfc_info_ptr(struct nand_chip *nand, int index)

Drop inline specifiers for things that are defined in .c files. The
compiler should be smart enough to determine when inlining is
appropriate.

> +{
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +
> +	return le64_to_cpu(meson_chip->info_buf[index]);

According to the function prototype, you should return
meson_chip->info_buf[index] directly, but I'm not even sure why you
need this helper. Just access meson_chip->info_buf[] directly.

> +}
> +

[...]

> +
> +static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
> +{
> +	u32 cmd, cfg;
> +	int ret = 0;
> +
> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> +	meson_nfc_drain_cmd(nfc);
> +	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> +
> +	cfg = readl(nfc->reg_base + NFC_REG_CFG);
> +	cfg |= (1 << 21);
> +	writel(cfg, nfc->reg_base + NFC_REG_CFG);
> +
> +	init_completion(&nfc->completion);
> +
> +	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> +		| nfc->param.chip_select | NFC_CMD_RB_TIMEOUT;
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	ret = wait_for_completion_timeout(&nfc->completion,
> +					  msecs_to_jiffies(timeout_ms));
> +	if (ret == 0) {
> +		dev_err(nfc->dev, "wait nand irq timeout\n");
> +		ret = -1;
> +	}
> +	return ret;
> +}
> +
> +static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)

	       ^ meson_nfc_set_protected_oob_bytes()

> +{
> +	__le64 info;
> +	int i, count;
> +
> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> +		info = nfc_info_ptr(nand, i);
> +		ECC_SET_PROTECTED_OOB_BYTE(info, 0, oob_buf[count]);
> +		ECC_SET_PROTECTED_OOB_BYTE(info, 1, oob_buf[count + 1]);

This is a NOOP: info is a local variable, so, anything you put in there
is just lost. It could work if nfc_info_ptr() was returning a pointer
and the local info var was also a pointer, but that's not the case
here.

BTW, maybe you don't need the ECC_SET/GET_PROTECTED_OOB_BYTE() macros.
Just do the conversion in the meson_nfc_get/set_protected_oob_bytes()
functions.

> +	}
> +}
> +
> +static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)

	       ^ meson_nfc_get_protected_oob_bytes()

> +{
> +	__le64 info;
> +	int i, count;
> +
> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> +		info = nfc_info_ptr(nand, i);
> +		oob_buf[count] = ECC_GET_PROTECTED_OOB_BYTE(info, 0);
> +		oob_buf[count + 1] = ECC_GET_PROTECTED_OOB_BYTE(info, 1);
> +	}
> +}
> +
> +static int meson_nfc_ecc_correct(struct nand_chip *nand)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	__le64 info;
> +	u32 bitflips = 0, i;
> +	int scramble;
> +	u8 zero_cnt;
> +
> +	scramble = (nand->options & NAND_NEED_SCRAMBLING) ? 1 : 0;
> +
> +	for (i = 0; i < nand->ecc.steps; i++) {
> +		info = nfc_info_ptr(nand, i);
> +		if (ECC_ERR_CNT(info) == 0x3f) {
> +			zero_cnt = ECC_ZERO_CNT(info);
> +			if (scramble && zero_cnt < nand->ecc.strength)
> +				return ECC_CHECK_RETURN_FF;
> +			mtd->ecc_stats.failed++;
> +			continue;
> +		}
> +		mtd->ecc_stats.corrected += ECC_ERR_CNT(info);
> +		bitflips = max_t(u32, bitflips, ECC_ERR_CNT(info));
> +	}
> +
> +	return bitflips;
> +}
> +
> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	u32 cmd;
> +
> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	meson_nfc_drain_cmd(nfc);

You probably don't want to drain the FIFO every time you read a byte on
the bus, and I guess the INPUT FIFO is at least as big as the CMD
FIFO, right? If that's the case, you should queue as much DRD cmd as
possible and only sync when the user explicitly requests it or when
the INPUT/READ FIFO is full.

> +
> +	meson_nfc_wait_cmd_finish(nfc, 1000);
> +
> +	return readb(nfc->reg_base + NFC_REG_BUF);
> +}
> +
> +static void meson_nfc_read_buf(struct mtd_info *mtd, u8 *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		buf[i] = meson_nfc_read_byte(mtd);
> +}
> +
> +static void meson_nfc_write_byte(struct mtd_info *mtd, u8 byte)
> +{
> +	struct meson_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
> +	u32 cmd;
> +
> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> +
> +	cmd = nfc->param.chip_select | NFC_CMD_DWR | (byte & 0xff);
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);

Nope, tWB is not needed between 2 data-write cycles. It's only needed
when a wait-RB is requested after a write.

> +	meson_nfc_cmd_idle(nfc, 0);

Why do you need that one?

> +
> +	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);

As for the read_byte() implementation, I don't think you should force a
sync here.

> +}
> +
> +static void meson_nfc_write_buf(struct mtd_info *mtd,
> +				const u8 *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		meson_nfc_write_byte(mtd, buf[i]);
> +}
> +
> +static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> +						int page, bool in)
> +{
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	const struct nand_sdr_timings *sdr =
> +		nand_get_sdr_timings(&nand->data_interface);
> +	int cs = nfc->param.chip_select;
> +	int i, cmd0, cmd_num;
> +	int ret = 0;
> +
> +	cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
> +	cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
> +	if (!in)
> +		cmd_num--;
> +
> +	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
> +	for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
> +		nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
> +
> +	for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
> +		nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
> +
> +	nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;

Having a fixed size array for the column and row address cycles does
not sound like a good idea to me (depending on the NAND chip you
connect, the number of cycles for the row and column differ), which
makes me realize the nand_rw_cmd struct is not such a good thing...

> +
> +	for (i = 0; i < cmd_num; i++)
> +		writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);

... why not write directly to the CMD reg?

> +
> +	if (in)
> +		meson_nfc_queue_rb(nfc, sdr->tR_max);
> +	else
> +		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
> +
> +	return ret;
> +}
> +
> +static int meson_nfc_write_page_sub(struct nand_chip *nand, const u8 *buf,
> +				    int page, int raw)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	const struct nand_sdr_timings *sdr =
> +		nand_get_sdr_timings(&nand->data_interface);
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	dma_addr_t daddr, iaddr;
> +	u32 cmd;
> +	int ret;
> +
> +	daddr = dma_map_single(nfc->dev, (void *)meson_chip->data_buf,
> +			       mtd->writesize + mtd->oobsize,
> +			       DMA_TO_DEVICE);
> +	ret = dma_mapping_error(nfc->dev, daddr);
> +	if (ret) {
> +		dev_err(nfc->dev, "dma mapping error\n");
> +		goto err;
> +	}
> +
> +	iaddr = dma_map_single(nfc->dev, (void *)meson_chip->info_buf,
> +			       nand->ecc.steps * PER_INFO_BYTE,
> +			       DMA_TO_DEVICE);
> +	ret = dma_mapping_error(nfc->dev, iaddr);
> +	if (ret) {
> +		dev_err(nfc->dev, "dma mapping error\n");
> +		goto err_map_daddr;
> +	}
> +
> +	ret = meson_nfc_rw_cmd_prepare_and_execute(nand, page, DIRWRITE);
> +	if (ret)
> +		goto err_map_iaddr;
> +
> +	cmd = GENCMDDADDRL(NFC_CMD_ADL, daddr);
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	cmd = GENCMDDADDRH(NFC_CMD_ADH, daddr);
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	cmd = GENCMDIADDRL(NFC_CMD_AIL, iaddr);
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	cmd = GENCMDIADDRH(NFC_CMD_AIH, iaddr);
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +
> +	meson_nfc_cmd_seed(nfc, page);
> +
> +	meson_nfc_cmd_access(nand, raw, DIRWRITE);
> +
> +	ret = meson_nfc_wait_dma_finish(nfc);
> +	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +	meson_nfc_queue_rb(nfc, sdr->tPROG_max);

Don't you have a way to queue the PAGEPROG and WAIT_RB instructions
before the DMA finishes?

> +
> +err_map_iaddr:
> +	dma_unmap_single(nfc->dev, iaddr,
> +			 nand->ecc.steps * PER_INFO_BYTE, DMA_TO_DEVICE);
> +err_map_daddr:
> +	dma_unmap_single(nfc->dev, daddr,
> +			 mtd->writesize + mtd->oobsize, DMA_TO_DEVICE);
> +err:
> +	return ret;
> +}
> +

[...]

> +static int meson_nfc_exec_op(struct nand_chip *nand,
> +			     const struct nand_operation *op, bool check_only)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	const struct nand_op_instr *instr = NULL;
> +	int ret = 0, cmd;
> +	unsigned int op_id;
> +	int i, delay_idle;
> +
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +		delay_idle = DIV_ROUND_UP(PSEC_TO_NSEC(instr->delay_ns),
> +					  meson_chip->level1_divider
> +					  * NFC_CLK_CYCLE);

On multi-line operations, put the operator on the previous line:

					  meson_chip->level1_divider *
					  NFC_CLK_CYCLE);


> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
> +			cmd |= instr->ctx.cmd.opcode & 0xff;
> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +			meson_nfc_cmd_idle(nfc, delay_idle);
> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +			}
> +			meson_nfc_cmd_idle(nfc, delay_idle);
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
> +					   instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
> +					    instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
> +			meson_nfc_cmd_idle(nfc, delay_idle);
> +			break;
> +		}

I think you could move the meson_nfc_cmd_idle() call here, and only add
it when instr->delay_ns != 0:

		if (instr->delay_ns)
			meson_nfc_cmd_idle(nfc, delay_idle);

> +	}

Don't you need a call to meson_nfc_wait_cmd_finish() here?

> +	return ret;
> +}

On 2018/11/5 23:53, Boris Brezillon wrote:
> On Fri, 2 Nov 2018 00:42:21 +0800
> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> 
>> +#define NFC_REG_CMD		0x00
>> +#define NFC_CMD_DRD		(0x8 << 14)
>> +#define NFC_CMD_IDLE		(0xc << 14)
>> +#define NFC_CMD_DWR		(0x4 << 14)
>> +#define NFC_CMD_CLE		(0x5 << 14)
>> +#define NFC_CMD_ALE		(0x6 << 14)
>> +#define NFC_CMD_ADL		((0 << 16) | (3 << 20))
>> +#define NFC_CMD_ADH		((1 << 16) | (3 << 20))
>> +#define NFC_CMD_AIL		((2 << 16) | (3 << 20))
>> +#define NFC_CMD_AIH		((3 << 16) | (3 << 20))
>> +#define NFC_CMD_SEED		((8 << 16) | (3 << 20))
>> +#define NFC_CMD_M2N		((0 << 17) | (2 << 20))
>> +#define NFC_CMD_N2M		((1 << 17) | (2 << 20))
>> +#define NFC_CMD_RB		BIT(20)
>> +#define NFC_CMD_IO6		((0xb << 10) | (1 << 18))
>> +#define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
>> +#define NFC_CMD_RB_INT		BIT(14)
>> +
>> +#define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
>> +#define NFC_CMD_RB_TIMEOUT	0x18
> 
> Where does this value come from? Is it the typical timeout value,
> and if it is, what's the value in milli/microseconds?
> 
it is about 5.2ms when one nand cycle is 20ns and calculate it with the 
max erase time of toshiba slc flash.i think it should be taken from the 
sdr_timings.
>> +
>> +#define NFC_REG_CFG		0x04
>> +#define NFC_REG_DADR		0x08
>> +#define NFC_REG_IADR		0x0c
>> +#define NFC_REG_BUF		0x10
>> +#define NFC_REG_INFO		0x14
>> +#define NFC_REG_DC		0x18
>> +#define NFC_REG_ADR		0x1c
>> +#define NFC_REG_DL		0x20
>> +#define NFC_REG_DH		0x24
>> +#define NFC_REG_CADR		0x28
>> +#define NFC_REG_SADR		0x2c
>> +#define NFC_REG_PINS		0x30
>> +#define NFC_REG_VER		0x38
>> +
>> +#define NFC_RB_IRQ_EN		BIT(21)
>> +#define NFC_INT_MASK		(3 << 20)
>> +
>> +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)	\
>> +	(								\
>> +		(cmd_dir)			|			\
>> +		((ran) << 19)			|			\
>> +		((bch) << 14)			|			\
>> +		((short_mode) << 13)		|			\
>> +		(((page_size) & 0x7f) << 6)	|			\
>> +		((pages) & 0x3f)					\
>> +	)
>> +
>> +#define GENCMDDADDRL(adl, addr)		((adl) | ((addr) & 0xffff))
>> +#define GENCMDDADDRH(adh, addr)		((adh) | (((addr) >> 16) & 0xffff))
>> +#define GENCMDIADDRL(ail, addr)		((ail) | ((addr) & 0xffff))
>> +#define GENCMDIADDRH(aih, addr)		((aih) | (((addr) >> 16) & 0xffff))
>> +
>> +#define RB_STA(x)		(1 << (26 + (x)))
>> +#define DMA_DIR(dir)		((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
>> +
>> +#define ECC_CHECK_RETURN_FF	(-1)
>> +
>> +#define NAND_CE0		(0xe << 10)
>> +#define NAND_CE1		(0xd << 10)
>> +
>> +#define DMA_BUSY_TIMEOUT	0x100000
>> +#define CMD_FIFO_EMPTY_TIMEOUT	1000
>> +
>> +#define MAX_CE_NUM		2
>> +
>> +/* eMMC clock register, misc control */
>> +#define SD_EMMC_CLOCK		0x00
>> +#define CLK_ALWAYS_ON		BIT(28)
>> +#define CLK_SELECT_NAND		BIT(31)
>> +#define CLK_DIV_MASK		GENMASK(5, 0)
>> +
>> +#define NFC_CLK_CYCLE		6
>> +
>> +/* nand flash controller delay 3 ns */
>> +#define NFC_DEFAULT_DELAY	3000
>> +
>> +#define MAX_ECC_INDEX		10
>> +
>> +#define MUX_CLK_NUM_PARENTS	2
>> +
>> +#define ROW_ADDER(page, index)	(((page) >> (8 * (index))) & 0xff)
>> +#define MAX_CYCLE_ROW_ADDRS	3
>> +#define MAX_CYCLE_COLUMN_ADDRS	2
>> +#define DIRREAD			1
>> +#define DIRWRITE		0
>> +
>> +#define ECC_PARITY_BCH8_512B	14
>> +
>> +#define PER_INFO_BYTE		8
>> +#define ECC_GET_PROTECTED_OOB_BYTE(x, y)			\
>> +		((x) >> (8 * (y)) & GENMASK(7, 0))
> 
> 		(le64_to_cpu(x) >> (8 * (y)) & GENMASK(7, 0))
> 
>> +
>> +#define ECC_SET_PROTECTED_OOB_BYTE(x, y, z)			\
>> +	{							\
>> +		(x) &= (~((__le64)(0xff) << (8 * (y))));	\
> 
> It's probably better to memset(0) the info_buf so that you can drop
> this masking step.
> 
ok.

>> +		(x) |= ((__le64)(z) << (8 * (y)));		\
> 
> 		    |= cpu_to_le64((u64)z << (8 * (y)));
> 
>> +	}
>> +
>> +#define ECC_COMPLETE            BIT(31)
>> +#define ECC_ERR_CNT(x)		(((x) >> 24) & GENMASK(5, 0))
>> +#define ECC_ZERO_CNT(x)		(((x) >> 16) & GENMASK(5, 0))
>> +
>> +struct meson_nfc_nand_chip {
>> +	struct list_head node;
>> +	struct nand_chip nand;
>> +	int clk_rate;
>> +	int level1_divider;
>> +	int bus_timing;
>> +	int twb;
>> +	int tadl;
>> +
>> +	int bch_mode;
>> +	u8 *data_buf;
>> +	__le64 *info_buf;
>> +	int nsels;
>> +	u8 sels[0];
>> +};
> 
> Please use unsigned integers where having a negative value is not
> possible. I'm pretty sure this is the case of all int fields in this
> struct. The same applies to the other structs.
> 
ok.

> [...]
> 
>> +
>> +static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc,
>> +				     unsigned int timeout_ms)
>> +{
>> +	u32 cmd_size = 0;
>> +	int ret;
>> +
>> +	/* wait cmd fifo is empty */
>> +	ret = readl_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size,
>> +				 !NFC_CMD_GET_SIZE(cmd_size),
>> +				 10, timeout_ms * 1000);
> 
> I guess you could use the relaxed version here.
> 
ok.

>> +	if (ret)
>> +		dev_err(nfc->dev, "wait for empty cmd FIFO time out\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc)
>> +{
>> +	meson_nfc_drain_cmd(nfc);
>> +
>> +	return meson_nfc_wait_cmd_finish(nfc, DMA_BUSY_TIMEOUT);
>> +}
>> +
>> +static inline __le64 nfc_info_ptr(struct nand_chip *nand, int index)
> 
> Drop inline specifiers for things that are defined in .c files. The
> compiler should be smart enough to determine when inlining is
> appropriate.
> 
ok.

>> +{
>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>> +
>> +	return le64_to_cpu(meson_chip->info_buf[index]);
> 
> According to the function prototype, you should return
> meson_chip->info_buf[index] directly, but I'm not even sure why you
> need this helper. Just access meson_chip->info_buf[] directly.
> 
ok. i will drop this helper.

>> +}
>> +
> 
> [...]
> 
>> +
>> +static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
>> +{
>> +	u32 cmd, cfg;
>> +	int ret = 0;
>> +
>> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
>> +	meson_nfc_drain_cmd(nfc);
>> +	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>> +
>> +	cfg = readl(nfc->reg_base + NFC_REG_CFG);
>> +	cfg |= (1 << 21);
>> +	writel(cfg, nfc->reg_base + NFC_REG_CFG);
>> +
>> +	init_completion(&nfc->completion);
>> +
>> +	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
>> +		| nfc->param.chip_select | NFC_CMD_RB_TIMEOUT;
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	ret = wait_for_completion_timeout(&nfc->completion,
>> +					  msecs_to_jiffies(timeout_ms));
>> +	if (ret == 0) {
>> +		dev_err(nfc->dev, "wait nand irq timeout\n");
>> +		ret = -1;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> 
> 	       ^ meson_nfc_set_protected_oob_bytes()
> 
>> +{
>> +	__le64 info;
>> +	int i, count;
>> +
>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>> +		info = nfc_info_ptr(nand, i);
>> +		ECC_SET_PROTECTED_OOB_BYTE(info, 0, oob_buf[count]);
>> +		ECC_SET_PROTECTED_OOB_BYTE(info, 1, oob_buf[count + 1]);
> 
> This is a NOOP: info is a local variable, so, anything you put in there
> is just lost. It could work if nfc_info_ptr() was returning a pointer
> and the local info var was also a pointer, but that's not the case
> here.
> 
wow! i made a mistake. previously i used __le64 *info here, but i don't 
know when i changed it carelessly.
thank you for pointing out it, i will fix it.

> BTW, maybe you don't need the ECC_SET/GET_PROTECTED_OOB_BYTE() macros.
> Just do the conversion in the meson_nfc_get/set_protected_oob_bytes()
> functions.
> 
ok.


>> +	}
>> +}
>> +
>> +static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
> 
> 	       ^ meson_nfc_get_protected_oob_bytes()
> 
>> +{
>> +	__le64 info;
>> +	int i, count;
>> +
>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>> +		info = nfc_info_ptr(nand, i);
>> +		oob_buf[count] = ECC_GET_PROTECTED_OOB_BYTE(info, 0);
>> +		oob_buf[count + 1] = ECC_GET_PROTECTED_OOB_BYTE(info, 1);
>> +	}
>> +}
>> +
>> +static int meson_nfc_ecc_correct(struct nand_chip *nand)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>> +	__le64 info;
>> +	u32 bitflips = 0, i;
>> +	int scramble;
>> +	u8 zero_cnt;
>> +
>> +	scramble = (nand->options & NAND_NEED_SCRAMBLING) ? 1 : 0;
>> +
>> +	for (i = 0; i < nand->ecc.steps; i++) {
>> +		info = nfc_info_ptr(nand, i);
>> +		if (ECC_ERR_CNT(info) == 0x3f) {
>> +			zero_cnt = ECC_ZERO_CNT(info);
>> +			if (scramble && zero_cnt < nand->ecc.strength)
>> +				return ECC_CHECK_RETURN_FF;
>> +			mtd->ecc_stats.failed++;
>> +			continue;
>> +		}
>> +		mtd->ecc_stats.corrected += ECC_ERR_CNT(info);
>> +		bitflips = max_t(u32, bitflips, ECC_ERR_CNT(info));
>> +	}
>> +
>> +	return bitflips;
>> +}
>> +
>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>> +	u32 cmd;
>> +
>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	meson_nfc_drain_cmd(nfc);
> 
> You probably don't want to drain the FIFO every time you read a byte on
> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> FIFO, right? If that's the case, you should queue as much DRD cmd as
> possible and only sync when the user explicitly requests it or when
> the INPUT/READ FIFO is full.
> 
Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one 
nand cycle to read one byte and covers the 1st byte every time reading. 
i think nfc controller is faster than nand cycle, but really it is not 
high efficiency when reading so many bytes once.
Or use dma command here like read_page and read_page_raw.

>> +
>> +	meson_nfc_wait_cmd_finish(nfc, 1000);
>> +
>> +	return readb(nfc->reg_base + NFC_REG_BUF);
>> +}
>> +
>> +static void meson_nfc_read_buf(struct mtd_info *mtd, u8 *buf, int len)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +		buf[i] = meson_nfc_read_byte(mtd);
>> +}
>> +
>> +static void meson_nfc_write_byte(struct mtd_info *mtd, u8 byte)
>> +{
>> +	struct meson_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
>> +	u32 cmd;
>> +
>> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
>> +
>> +	cmd = nfc->param.chip_select | NFC_CMD_DWR | (byte & 0xff);
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> 
> Nope, tWB is not needed between 2 data-write cycles. It's only needed
> when a wait-RB is requested after a write.
> 
ok, i will remove it.

>> +	meson_nfc_cmd_idle(nfc, 0);
> 
> Why do you need that one?
> 
em, it doesn't need here.

>> +
>> +	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> 
> As for the read_byte() implementation, I don't think you should force a
> sync here.
> 
ok, it can send 30 bytes (command fifo size subtract 2 idle command ) 
once with a sync. Or use dma command.

>> +}
>> +
>> +static void meson_nfc_write_buf(struct mtd_info *mtd,
>> +				const u8 *buf, int len)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +		meson_nfc_write_byte(mtd, buf[i]);
>> +}
>> +
>> +static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>> +						int page, bool in)
>> +{
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>> +	const struct nand_sdr_timings *sdr =
>> +		nand_get_sdr_timings(&nand->data_interface);
>> +	int cs = nfc->param.chip_select;
>> +	int i, cmd0, cmd_num;
>> +	int ret = 0;
>> +
>> +	cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
>> +	cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
>> +	if (!in)
>> +		cmd_num--;
>> +
>> +	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
>> +	for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
>> +		nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
>> +
>> +	for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
>> +		nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
>> +
>> +	nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
> 
> Having a fixed size array for the column and row address cycles does
> not sound like a good idea to me (depending on the NAND chip you
> connect, the number of cycles for the row and column differ), which
> makes me realize the nand_rw_cmd struct is not such a good thing...
> 
em , i will fix it by adding the size of the column and row address.
is that ok?

>> +
>> +	for (i = 0; i < cmd_num; i++)
>> +		writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);
> 
> ... why not write directly to the CMD reg?
> 

it seems that too many writel(xxx, nfc->reg_base + NFC_REG_CMD) in one 
function; so I want to cache all the commands and write it in a loop.

>> +
>> +	if (in)
>> +		meson_nfc_queue_rb(nfc, sdr->tR_max);
>> +	else
>> +		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
>> +
>> +	return ret;
>> +}
>> +
>> +static int meson_nfc_write_page_sub(struct nand_chip *nand, const u8 *buf,
>> +				    int page, int raw)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>> +	const struct nand_sdr_timings *sdr =
>> +		nand_get_sdr_timings(&nand->data_interface);
>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>> +	dma_addr_t daddr, iaddr;
>> +	u32 cmd;
>> +	int ret;
>> +
>> +	daddr = dma_map_single(nfc->dev, (void *)meson_chip->data_buf,
>> +			       mtd->writesize + mtd->oobsize,
>> +			       DMA_TO_DEVICE);
>> +	ret = dma_mapping_error(nfc->dev, daddr);
>> +	if (ret) {
>> +		dev_err(nfc->dev, "dma mapping error\n");
>> +		goto err;
>> +	}
>> +
>> +	iaddr = dma_map_single(nfc->dev, (void *)meson_chip->info_buf,
>> +			       nand->ecc.steps * PER_INFO_BYTE,
>> +			       DMA_TO_DEVICE);
>> +	ret = dma_mapping_error(nfc->dev, iaddr);
>> +	if (ret) {
>> +		dev_err(nfc->dev, "dma mapping error\n");
>> +		goto err_map_daddr;
>> +	}
>> +
>> +	ret = meson_nfc_rw_cmd_prepare_and_execute(nand, page, DIRWRITE);
>> +	if (ret)
>> +		goto err_map_iaddr;
>> +
>> +	cmd = GENCMDDADDRL(NFC_CMD_ADL, daddr);
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	cmd = GENCMDDADDRH(NFC_CMD_ADH, daddr);
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	cmd = GENCMDIADDRL(NFC_CMD_AIL, iaddr);
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	cmd = GENCMDIADDRH(NFC_CMD_AIH, iaddr);
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +
>> +	meson_nfc_cmd_seed(nfc, page);
>> +
>> +	meson_nfc_cmd_access(nand, raw, DIRWRITE);
>> +
>> +	ret = meson_nfc_wait_dma_finish(nfc);
>> +	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +	meson_nfc_queue_rb(nfc, sdr->tPROG_max);
> 
> Don't you have a way to queue the PAGEPROG and WAIT_RB instructions
> before the DMA finishes?
> 

it runs:
1) dma transfer ddr data to nand page cache.
2) wait dma finish
3) send the PAGEPROG command
4) wait rb finish

meson_nfc_wait_dma_finish(nfc) waits command fifo empty. Maybe it is 
difficult or uncessary to queue the PAGEPROG command and WAIT_RB between 
1) and 2).

is that right?


>> +
>> +err_map_iaddr:
>> +	dma_unmap_single(nfc->dev, iaddr,
>> +			 nand->ecc.steps * PER_INFO_BYTE, DMA_TO_DEVICE);
>> +err_map_daddr:
>> +	dma_unmap_single(nfc->dev, daddr,
>> +			 mtd->writesize + mtd->oobsize, DMA_TO_DEVICE);
>> +err:
>> +	return ret;
>> +}
>> +
> 
> [...]
> 
>> +static int meson_nfc_exec_op(struct nand_chip *nand,
>> +			     const struct nand_operation *op, bool check_only)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>> +	const struct nand_op_instr *instr = NULL;
>> +	int ret = 0, cmd;
>> +	unsigned int op_id;
>> +	int i, delay_idle;
>> +
>> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
>> +		instr = &op->instrs[op_id];
>> +		delay_idle = DIV_ROUND_UP(PSEC_TO_NSEC(instr->delay_ns),
>> +					  meson_chip->level1_divider
>> +					  * NFC_CLK_CYCLE);
> 
> On multi-line operations, put the operator on the previous line:
> 
ok, i will fix it.

> 					  meson_chip->level1_divider *
> 					  NFC_CLK_CYCLE);
> 
> 
>> +		switch (instr->type) {
>> +		case NAND_OP_CMD_INSTR:
>> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
>> +			cmd |= instr->ctx.cmd.opcode & 0xff;
>> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +			meson_nfc_cmd_idle(nfc, delay_idle);
>> +			break;
>> +
>> +		case NAND_OP_ADDR_INSTR:
>> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
>> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
>> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
>> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +			}
>> +			meson_nfc_cmd_idle(nfc, delay_idle);
>> +			break;
>> +
>> +		case NAND_OP_DATA_IN_INSTR:
>> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
>> +					   instr->ctx.data.len);
>> +			break;
>> +
>> +		case NAND_OP_DATA_OUT_INSTR:
>> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
>> +					    instr->ctx.data.len);
>> +			break;
>> +
>> +		case NAND_OP_WAITRDY_INSTR:
>> +			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
>> +			meson_nfc_cmd_idle(nfc, delay_idle);
>> +			break;
>> +		}
> 
> I think you could move the meson_nfc_cmd_idle() call here, and only add
> it when instr->delay_ns != 0:
> > 		if (instr->delay_ns)
> 			meson_nfc_cmd_idle(nfc, delay_idle);
> 
ok, thank you.

>> +	}
> 
> Don't you need a call to meson_nfc_wait_cmd_finish() here?
> 
it will be called in queue_rb, read_buf and write_buf. but i have no 
idea whether it still needs to be called corresponding to 
CMD_INSTR/ADDR_INSTR.To be strict, it should be called.

>> +	return ret;
>> +}
> 
> .
>

On Tue, 6 Nov 2018 17:08:00 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> On 2018/11/5 23:53, Boris Brezillon wrote:
> > On Fri, 2 Nov 2018 00:42:21 +0800
> > Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> >   
> >> +#define NFC_REG_CMD		0x00
> >> +#define NFC_CMD_DRD		(0x8 << 14)
> >> +#define NFC_CMD_IDLE		(0xc << 14)
> >> +#define NFC_CMD_DWR		(0x4 << 14)
> >> +#define NFC_CMD_CLE		(0x5 << 14)
> >> +#define NFC_CMD_ALE		(0x6 << 14)
> >> +#define NFC_CMD_ADL		((0 << 16) | (3 << 20))
> >> +#define NFC_CMD_ADH		((1 << 16) | (3 << 20))
> >> +#define NFC_CMD_AIL		((2 << 16) | (3 << 20))
> >> +#define NFC_CMD_AIH		((3 << 16) | (3 << 20))
> >> +#define NFC_CMD_SEED		((8 << 16) | (3 << 20))
> >> +#define NFC_CMD_M2N		((0 << 17) | (2 << 20))
> >> +#define NFC_CMD_N2M		((1 << 17) | (2 << 20))
> >> +#define NFC_CMD_RB		BIT(20)
> >> +#define NFC_CMD_IO6		((0xb << 10) | (1 << 18))
> >> +#define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
> >> +#define NFC_CMD_RB_INT		BIT(14)
> >> +
> >> +#define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
> >> +#define NFC_CMD_RB_TIMEOUT	0x18  
> > 
> > Where does this value come from? Is it the typical timeout value,
> > and if it is, what's the value in milli/microseconds?
> >   
> it is about 5.2ms when one nand cycle is 20ns and calculate it with the 
> max erase time of toshiba slc flash.i think it should be taken from the 
> sdr_timings.

Yes, it should. Or just pick the maximum value, since it's just a
timeout anyway and shouldn't expire if everything works as expected.

> >> +
> >> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >> +{
> >> +	struct nand_chip *nand = mtd_to_nand(mtd);
> >> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> +	u32 cmd;
> >> +
> >> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> +	meson_nfc_drain_cmd(nfc);  
> > 
> > You probably don't want to drain the FIFO every time you read a byte on
> > the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > FIFO, right? If that's the case, you should queue as much DRD cmd as
> > possible and only sync when the user explicitly requests it or when
> > the INPUT/READ FIFO is full.
> >   
> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one 
> nand cycle to read one byte and covers the 1st byte every time reading. 
> i think nfc controller is faster than nand cycle, but really it is not 
> high efficiency when reading so many bytes once.
> Or use dma command here like read_page and read_page_raw.

Yep, that's also an alternative, though you'll have to make sure the
buffer passed through the nand_op_inst is DMA-safe, and use a bounce
buffer when that's not the case.

> >> +
> >> +	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);  
> > 
> > As for the read_byte() implementation, I don't think you should force a
> > sync here.
> >   
> ok, it can send 30 bytes (command fifo size subtract 2 idle command ) 
> once with a sync.

Okay, still better than syncing after each transmitted byte.

> Or use dma command.

I guess that's the best option.

> 
> >> +}
> >> +
> >> +static void meson_nfc_write_buf(struct mtd_info *mtd,
> >> +				const u8 *buf, int len)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < len; i++)
> >> +		meson_nfc_write_byte(mtd, buf[i]);
> >> +}
> >> +
> >> +static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> >> +						int page, bool in)
> >> +{
> >> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> +	const struct nand_sdr_timings *sdr =
> >> +		nand_get_sdr_timings(&nand->data_interface);
> >> +	int cs = nfc->param.chip_select;
> >> +	int i, cmd0, cmd_num;
> >> +	int ret = 0;
> >> +
> >> +	cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
> >> +	cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
> >> +	if (!in)
> >> +		cmd_num--;
> >> +
> >> +	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
> >> +	for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
> >> +		nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
> >> +
> >> +	for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
> >> +		nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
> >> +
> >> +	nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;  
> > 
> > Having a fixed size array for the column and row address cycles does
> > not sound like a good idea to me (depending on the NAND chip you
> > connect, the number of cycles for the row and column differ), which
> > makes me realize the nand_rw_cmd struct is not such a good thing...
> >   
> em , i will fix it by adding the size of the column and row address.
> is that ok?
> 
> >> +
> >> +	for (i = 0; i < cmd_num; i++)
> >> +		writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);  
> > 
> > ... why not write directly to the CMD reg?
> >   
> 
> it seems that too many writel(xxx, nfc->reg_base + NFC_REG_CMD) in one 
> function; so I want to cache all the commands and write it in a loop.

Not sure why it makes a difference since you'll end up writing to
NFC_REG_CMD anyway.

BTW, you can probably use the writel_relaxed() instead of writel() when
writing to the CMD FIFO.

> 
> >> +
> >> +	if (in)
> >> +		meson_nfc_queue_rb(nfc, sdr->tR_max);
> >> +	else
> >> +		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int meson_nfc_write_page_sub(struct nand_chip *nand, const u8 *buf,
> >> +				    int page, int raw)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +	const struct nand_sdr_timings *sdr =
> >> +		nand_get_sdr_timings(&nand->data_interface);
> >> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >> +	dma_addr_t daddr, iaddr;
> >> +	u32 cmd;
> >> +	int ret;
> >> +
> >> +	daddr = dma_map_single(nfc->dev, (void *)meson_chip->data_buf,
> >> +			       mtd->writesize + mtd->oobsize,
> >> +			       DMA_TO_DEVICE);
> >> +	ret = dma_mapping_error(nfc->dev, daddr);
> >> +	if (ret) {
> >> +		dev_err(nfc->dev, "dma mapping error\n");
> >> +		goto err;
> >> +	}
> >> +
> >> +	iaddr = dma_map_single(nfc->dev, (void *)meson_chip->info_buf,
> >> +			       nand->ecc.steps * PER_INFO_BYTE,
> >> +			       DMA_TO_DEVICE);
> >> +	ret = dma_mapping_error(nfc->dev, iaddr);
> >> +	if (ret) {
> >> +		dev_err(nfc->dev, "dma mapping error\n");
> >> +		goto err_map_daddr;
> >> +	}
> >> +
> >> +	ret = meson_nfc_rw_cmd_prepare_and_execute(nand, page, DIRWRITE);
> >> +	if (ret)
> >> +		goto err_map_iaddr;
> >> +
> >> +	cmd = GENCMDDADDRL(NFC_CMD_ADL, daddr);
> >> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> +	cmd = GENCMDDADDRH(NFC_CMD_ADH, daddr);
> >> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> +	cmd = GENCMDIADDRL(NFC_CMD_AIL, iaddr);
> >> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> +	cmd = GENCMDIADDRH(NFC_CMD_AIH, iaddr);
> >> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +
> >> +	meson_nfc_cmd_seed(nfc, page);
> >> +
> >> +	meson_nfc_cmd_access(nand, raw, DIRWRITE);
> >> +
> >> +	ret = meson_nfc_wait_dma_finish(nfc);
> >> +	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
> >> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +	meson_nfc_queue_rb(nfc, sdr->tPROG_max);  
> > 
> > Don't you have a way to queue the PAGEPROG and WAIT_RB instructions
> > before the DMA finishes?
> >   
> 
> it runs:
> 1) dma transfer ddr data to nand page cache.
> 2) wait dma finish
> 3) send the PAGEPROG command
> 4) wait rb finish
> 
> meson_nfc_wait_dma_finish(nfc) waits command fifo empty. Maybe it is 
> difficult or uncessary to queue the PAGEPROG command and WAIT_RB between 
> 1) and 2).
> 
> is that right?
> 

Isn't the controller engine able to wait on the data transfer to be
complete before sending the next instruction in the CMD FIFO pipe?
I'm pretty sure it's able to do that, which would make
meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
for the CMD FIFO to be empty (assuming it guarantees the last command
has been executed).

> 
> >> +	}  
> > 
> > Don't you need a call to meson_nfc_wait_cmd_finish() here?
> >   
> it will be called in queue_rb, read_buf and write_buf. but i have no 
> idea whether it still needs to be called corresponding to 
> CMD_INSTR/ADDR_INSTR.To be strict, it should be called.

No, the synchronization is only needed before returning from
->exec_op(). Everything before can be queued without being
executed. Of course, if you run out of entries in the CMD/INPUT
FIFOs you'll have to do some sort of synchronization, but that should
be taken care of in your helpers.

On 2018/11/6 17:28, Boris Brezillon wrote:
> On Tue, 6 Nov 2018 17:08:00 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> On 2018/11/5 23:53, Boris Brezillon wrote:
>>> On Fri, 2 Nov 2018 00:42:21 +0800
>>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>>>    
>>>> +
>>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
>>>> +{
>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>> +	u32 cmd;
>>>> +
>>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +
>>>> +	meson_nfc_drain_cmd(nfc);
>>>
>>> You probably don't want to drain the FIFO every time you read a byte on
>>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
>>> FIFO, right? If that's the case, you should queue as much DRD cmd as
>>> possible and only sync when the user explicitly requests it or when
>>> the INPUT/READ FIFO is full.
>>>    
>> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
>> nand cycle to read one byte and covers the 1st byte every time reading.
>> i think nfc controller is faster than nand cycle, but really it is not
>> high efficiency when reading so many bytes once.
>> Or use dma command here like read_page and read_page_raw.
> 
> Yep, that's also an alternative, though you'll have to make sure the
> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> buffer when that's not the case.
> 
ok, i will try dma here.

>>>> +
>>>> +	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>>>
>>> As for the read_byte() implementation, I don't think you should force a
>>> sync here.
>>>    
>> ok, it can send 30 bytes (command fifo size subtract 2 idle command )
>> once with a sync.
> 
> Okay, still better than syncing after each transmitted byte.
> 
>> Or use dma command.
> 
> I guess that's the best option.
> 
ok, i will try dma here.
>>
>>>> +}
>>>> +
>>>> +static void meson_nfc_write_buf(struct mtd_info *mtd,
>>>> +				const u8 *buf, int len)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < len; i++)
>>>> +		meson_nfc_write_byte(mtd, buf[i]);
>>>> +}
>>>> +
>>>> +static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>>>> +						int page, bool in)
>>>> +{
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>> +	const struct nand_sdr_timings *sdr =
>>>> +		nand_get_sdr_timings(&nand->data_interface);
>>>> +	int cs = nfc->param.chip_select;
>>>> +	int i, cmd0, cmd_num;
>>>> +	int ret = 0;
>>>> +
>>>> +	cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
>>>> +	cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
>>>> +	if (!in)
>>>> +		cmd_num--;
>>>> +
>>>> +	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
>>>> +	for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
>>>> +		nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
>>>> +
>>>> +	for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
>>>> +		nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
>>>> +
>>>> +	nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
>>>
>>> Having a fixed size array for the column and row address cycles does
>>> not sound like a good idea to me (depending on the NAND chip you
>>> connect, the number of cycles for the row and column differ), which
>>> makes me realize the nand_rw_cmd struct is not such a good thing...
>>>    
>> em , i will fix it by adding the size of the column and row address.
>> is that ok?
>>
>>>> +
>>>> +	for (i = 0; i < cmd_num; i++)
>>>> +		writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);
>>>
>>> ... why not write directly to the CMD reg?
>>>    
>>
>> it seems that too many writel(xxx, nfc->reg_base + NFC_REG_CMD) in one
>> function; so I want to cache all the commands and write it in a loop.
> 
> Not sure why it makes a difference since you'll end up writing to
> NFC_REG_CMD anyway.
> 
> BTW, you can probably use the writel_relaxed() instead of writel() when
> writing to the CMD FIFO.
> 
ok.
>>
>>>> +
>>>> +	if (in)
>>>> +		meson_nfc_queue_rb(nfc, sdr->tR_max);
>>>> +	else
>>>> +		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int meson_nfc_write_page_sub(struct nand_chip *nand, const u8 *buf,
>>>> +				    int page, int raw)
>>>> +{
>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>> +	const struct nand_sdr_timings *sdr =
>>>> +		nand_get_sdr_timings(&nand->data_interface);
>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>> +	dma_addr_t daddr, iaddr;
>>>> +	u32 cmd;
>>>> +	int ret;
>>>> +
>>>> +	daddr = dma_map_single(nfc->dev, (void *)meson_chip->data_buf,
>>>> +			       mtd->writesize + mtd->oobsize,
>>>> +			       DMA_TO_DEVICE);
>>>> +	ret = dma_mapping_error(nfc->dev, daddr);
>>>> +	if (ret) {
>>>> +		dev_err(nfc->dev, "dma mapping error\n");
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	iaddr = dma_map_single(nfc->dev, (void *)meson_chip->info_buf,
>>>> +			       nand->ecc.steps * PER_INFO_BYTE,
>>>> +			       DMA_TO_DEVICE);
>>>> +	ret = dma_mapping_error(nfc->dev, iaddr);
>>>> +	if (ret) {
>>>> +		dev_err(nfc->dev, "dma mapping error\n");
>>>> +		goto err_map_daddr;
>>>> +	}
>>>> +
>>>> +	ret = meson_nfc_rw_cmd_prepare_and_execute(nand, page, DIRWRITE);
>>>> +	if (ret)
>>>> +		goto err_map_iaddr;
>>>> +
>>>> +	cmd = GENCMDDADDRL(NFC_CMD_ADL, daddr);
>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +
>>>> +	cmd = GENCMDDADDRH(NFC_CMD_ADH, daddr);
>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +
>>>> +	cmd = GENCMDIADDRL(NFC_CMD_AIL, iaddr);
>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +
>>>> +	cmd = GENCMDIADDRH(NFC_CMD_AIH, iaddr);
>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +
>>>> +	meson_nfc_cmd_seed(nfc, page);
>>>> +
>>>> +	meson_nfc_cmd_access(nand, raw, DIRWRITE);
>>>> +
>>>> +	ret = meson_nfc_wait_dma_finish(nfc);
>>>> +	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +	meson_nfc_queue_rb(nfc, sdr->tPROG_max);
>>>
>>> Don't you have a way to queue the PAGEPROG and WAIT_RB instructions
>>> before the DMA finishes?
>>>    
>>
>> it runs:
>> 1) dma transfer ddr data to nand page cache.
>> 2) wait dma finish
>> 3) send the PAGEPROG command
>> 4) wait rb finish
>>
>> meson_nfc_wait_dma_finish(nfc) waits command fifo empty. Maybe it is
>> difficult or uncessary to queue the PAGEPROG command and WAIT_RB between
>> 1) and 2).
>>
>> is that right?
>>
> 
> Isn't the controller engine able to wait on the data transfer to be
> complete before sending the next instruction in the CMD FIFO pipe?
> I'm pretty sure it's able to do that, which would make
> meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
> for the CMD FIFO to be empty (assuming it guarantees the last command
> has been executed).
> Maybe the nfc design is difference. dedicated nfc dma engine is 
concatenated with the command fifo, there is no other status to check 
whether dma is done.

>>
>>>> +	}
>>>
>>> Don't you need a call to meson_nfc_wait_cmd_finish() here?
>>>    
>> it will be called in queue_rb, read_buf and write_buf. but i have no
>> idea whether it still needs to be called corresponding to
>> CMD_INSTR/ADDR_INSTR.To be strict, it should be called.
> 
> No, the synchronization is only needed before returning from
> ->exec_op(). Everything before can be queued without being
> executed. Of course, if you run out of entries in the CMD/INPUT
> FIFOs you'll have to do some sort of synchronization, but that should
> be taken care of in your helpers.
> 
ok, i will add it.
> .
>

On Tue, 6 Nov 2018 18:00:37 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> On 2018/11/6 17:28, Boris Brezillon wrote:
> > On Tue, 6 Nov 2018 17:08:00 +0800
> > Liang Yang <liang.yang@amlogic.com> wrote:
> >   
> >> On 2018/11/5 23:53, Boris Brezillon wrote:  
> >>> On Fri, 2 Nov 2018 00:42:21 +0800
> >>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> >>>      
> >>>> +
> >>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >>>> +{
> >>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
> >>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >>>> +	u32 cmd;
> >>>> +
> >>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>>> +
> >>>> +	meson_nfc_drain_cmd(nfc);  
> >>>
> >>> You probably don't want to drain the FIFO every time you read a byte on
> >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> >>> possible and only sync when the user explicitly requests it or when
> >>> the INPUT/READ FIFO is full.
> >>>      
> >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> >> nand cycle to read one byte and covers the 1st byte every time reading.
> >> i think nfc controller is faster than nand cycle, but really it is not
> >> high efficiency when reading so many bytes once.
> >> Or use dma command here like read_page and read_page_raw.  
> > 
> > Yep, that's also an alternative, though you'll have to make sure the
> > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > buffer when that's not the case.
> >   
> ok, i will try dma here.

We should probably expose the bounce buf handling as generic helpers at
the rawnand level:

void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
{
	void *buf;

	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
		return NULL;

	if (virt_addr_valid(instr->data.in) &&
	    !object_is_on_stack(instr->data.buf.in))
		return instr->data.buf.in;

	return kzalloc(instr->data.len, GFP_KERNEL);
}

void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
				    void *buf)
{
	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
	    WARN_ON(!buf))
		return;

	if (buf == instr->data.buf.in)
		return;

	memcpy(instr->data.buf.in, buf, instr->data.len);
	kfree(buf);
}

const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
{
	void *buf;

	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
		return NULL;

	if (virt_addr_valid(instr->data.out) &&
	    !object_is_on_stack(instr->data.buf.out))
		return instr->data.buf.out;

	return kmemdup(instr->data.buf.out, GFP_KERNEL);
}

void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
				    void *buf)
{
	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
	    WARN_ON(!buf))
		return;

	if (buf != instr->data.buf.out)
		kfree(buf);
}

> > 
> > Isn't the controller engine able to wait on the data transfer to be
> > complete before sending the next instruction in the CMD FIFO pipe?
> > I'm pretty sure it's able to do that, which would make
> > meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
> > for the CMD FIFO to be empty (assuming it guarantees the last command
> > has been executed).
> > Maybe the nfc design is difference. dedicated nfc dma engine is   
> concatenated with the command fifo, there is no other status to check 
> whether dma is done.

No, I mean that internally a "DMA-transfer" instruction probably
forces the NAND controller to wait for the DMA transfer to be finished
before dequeuing/executing the next instruction. So, it should be safe
to queue the PROG and WAIT_RB instructions and just rely on the "FIFO
empty" event to consider the operation as finished. Then, all you'll
have to do is check the status reg to determine whether the
write operation succeeded or not.

On 2018/11/6 18:22, Boris Brezillon wrote:
> On Tue, 6 Nov 2018 18:00:37 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> On 2018/11/6 17:28, Boris Brezillon wrote:
>>> On Tue, 6 Nov 2018 17:08:00 +0800
>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>    
>>>> On 2018/11/5 23:53, Boris Brezillon wrote:
>>>>> On Fri, 2 Nov 2018 00:42:21 +0800
>>>>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>>>>>       
>>>>>> +
>>>>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
>>>>>> +{
>>>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>>>> +	u32 cmd;
>>>>>> +
>>>>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
>>>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>> +
>>>>>> +	meson_nfc_drain_cmd(nfc);
>>>>>
>>>>> You probably don't want to drain the FIFO every time you read a byte on
>>>>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
>>>>> FIFO, right? If that's the case, you should queue as much DRD cmd as
>>>>> possible and only sync when the user explicitly requests it or when
>>>>> the INPUT/READ FIFO is full.
>>>>>       
>>>> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
>>>> nand cycle to read one byte and covers the 1st byte every time reading.
>>>> i think nfc controller is faster than nand cycle, but really it is not
>>>> high efficiency when reading so many bytes once.
>>>> Or use dma command here like read_page and read_page_raw.
>>>
>>> Yep, that's also an alternative, though you'll have to make sure the
>>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
>>> buffer when that's not the case.
>>>    
>> ok, i will try dma here.
> 
> We should probably expose the bounce buf handling as generic helpers at
> the rawnand level:
> 
> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> {
> 	void *buf;
> 
> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> 		return NULL;
> 
> 	if (virt_addr_valid(instr->data.in) &&
> 	    !object_is_on_stack(instr->data.buf.in))
> 		return instr->data.buf.in;
> 
> 	return kzalloc(instr->data.len, GFP_KERNEL);
> }
> 
> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> 				    void *buf)
> {
> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> 	    WARN_ON(!buf))
> 		return;
> 
> 	if (buf == instr->data.buf.in)
> 		return;
> 
> 	memcpy(instr->data.buf.in, buf, instr->data.len);
> 	kfree(buf);
> }
> 
> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> {
> 	void *buf;
> 
> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> 		return NULL;
> 
> 	if (virt_addr_valid(instr->data.out) &&
> 	    !object_is_on_stack(instr->data.buf.out))
> 		return instr->data.buf.out;
> 
> 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
> }
> 
> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> 				    void *buf)
> {
> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> 	    WARN_ON(!buf))
> 		return;
> 
> 	if (buf != instr->data.buf.out)
> 		kfree(buf);
> }
>

that is more convenient.
i will use meson_chip->databuf as the bounce mid-buffer now.

>>>
>>> Isn't the controller engine able to wait on the data transfer to be
>>> complete before sending the next instruction in the CMD FIFO pipe?
>>> I'm pretty sure it's able to do that, which would make
>>> meson_nfc_wait_dma_finish() useless, and all you'd have to do is wait
>>> for the CMD FIFO to be empty (assuming it guarantees the last command
>>> has been executed).
>>> Maybe the nfc design is difference. dedicated nfc dma engine is
>> concatenated with the command fifo, there is no other status to check
>> whether dma is done.
> 
> No, I mean that internally a "DMA-transfer" instruction probably
> forces the NAND controller to wait for the DMA transfer to be finished
> before dequeuing/executing the next instruction. So, it should be safe
> to queue the PROG and WAIT_RB instructions and just rely on the "FIFO
> empty" event to consider the operation as finished. Then, all you'll
> have to do is check the status reg to determine whether the
> write operation succeeded or not.
> 
em, i got the point. indeed, until dma is checked done, nfc will execute 
next command in the command queue. so i will consider it.

> .
>

On Tue, 6 Nov 2018 19:08:27 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> On 2018/11/6 18:22, Boris Brezillon wrote:
> > On Tue, 6 Nov 2018 18:00:37 +0800
> > Liang Yang <liang.yang@amlogic.com> wrote:
> >   
> >> On 2018/11/6 17:28, Boris Brezillon wrote:  
> >>> On Tue, 6 Nov 2018 17:08:00 +0800
> >>> Liang Yang <liang.yang@amlogic.com> wrote:
> >>>      
> >>>> On 2018/11/5 23:53, Boris Brezillon wrote:  
> >>>>> On Fri, 2 Nov 2018 00:42:21 +0800
> >>>>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> >>>>>         
> >>>>>> +
> >>>>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> >>>>>> +{
> >>>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
> >>>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >>>>>> +	u32 cmd;
> >>>>>> +
> >>>>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> >>>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>>>>> +
> >>>>>> +	meson_nfc_drain_cmd(nfc);  
> >>>>>
> >>>>> You probably don't want to drain the FIFO every time you read a byte on
> >>>>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> >>>>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> >>>>> possible and only sync when the user explicitly requests it or when
> >>>>> the INPUT/READ FIFO is full.
> >>>>>         
> >>>> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> >>>> nand cycle to read one byte and covers the 1st byte every time reading.
> >>>> i think nfc controller is faster than nand cycle, but really it is not
> >>>> high efficiency when reading so many bytes once.
> >>>> Or use dma command here like read_page and read_page_raw.  
> >>>
> >>> Yep, that's also an alternative, though you'll have to make sure the
> >>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> >>> buffer when that's not the case.
> >>>      
> >> ok, i will try dma here.  
> > 
> > We should probably expose the bounce buf handling as generic helpers at
> > the rawnand level:
> > 
> > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > {
> > 	void *buf;
> > 
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > 		return NULL;
> > 
> > 	if (virt_addr_valid(instr->data.in) &&
> > 	    !object_is_on_stack(instr->data.buf.in))
> > 		return instr->data.buf.in;
> > 
> > 	return kzalloc(instr->data.len, GFP_KERNEL);
> > }
> > 
> > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > 				    void *buf)
> > {
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > 	    WARN_ON(!buf))
> > 		return;
> > 
> > 	if (buf == instr->data.buf.in)
> > 		return;
> > 
> > 	memcpy(instr->data.buf.in, buf, instr->data.len);
> > 	kfree(buf);
> > }
> > 
> > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > {
> > 	void *buf;
> > 
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > 		return NULL;
> > 
> > 	if (virt_addr_valid(instr->data.out) &&
> > 	    !object_is_on_stack(instr->data.buf.out))
> > 		return instr->data.buf.out;
> > 
> > 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > }
> > 
> > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > 				    void *buf)
> > {
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > 	    WARN_ON(!buf))
> > 		return;
> > 
> > 	if (buf != instr->data.buf.out)
> > 		kfree(buf);
> > }
> >  
> 
> that is more convenient.
> i will use meson_chip->databuf as the bounce mid-buffer now.

It won't work: the bounce buffer is allocated after the detection, and
the detection code is calling ->exec_op().

Just add a new patch to you series adding these helpers to nand_base.c.

On 2018/11/7 0:16, Boris Brezillon wrote:
> On Tue, 6 Nov 2018 19:08:27 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> On 2018/11/6 18:22, Boris Brezillon wrote:
>>> On Tue, 6 Nov 2018 18:00:37 +0800
>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>    
>>>> On 2018/11/6 17:28, Boris Brezillon wrote:
>>>>> On Tue, 6 Nov 2018 17:08:00 +0800
>>>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>>>       
>>>>>> On 2018/11/5 23:53, Boris Brezillon wrote:
>>>>>>> On Fri, 2 Nov 2018 00:42:21 +0800
>>>>>>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>>>>>>>          
>>>>>>>> +
>>>>>>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
>>>>>>>> +{
>>>>>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>>>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>>>>>> +	u32 cmd;
>>>>>>>> +
>>>>>>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
>>>>>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>>> +
>>>>>>>> +	meson_nfc_drain_cmd(nfc);
>>>>>>>
>>>>>>> You probably don't want to drain the FIFO every time you read a byte on
>>>>>>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
>>>>>>> FIFO, right? If that's the case, you should queue as much DRD cmd as
>>>>>>> possible and only sync when the user explicitly requests it or when
>>>>>>> the INPUT/READ FIFO is full.
>>>>>>>          
>>>>>> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
>>>>>> nand cycle to read one byte and covers the 1st byte every time reading.
>>>>>> i think nfc controller is faster than nand cycle, but really it is not
>>>>>> high efficiency when reading so many bytes once.
>>>>>> Or use dma command here like read_page and read_page_raw.
>>>>>
>>>>> Yep, that's also an alternative, though you'll have to make sure the
>>>>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
>>>>> buffer when that's not the case.
>>>>>       
>>>> ok, i will try dma here.
>>>
>>> We should probably expose the bounce buf handling as generic helpers at
>>> the rawnand level:
>>>
>>> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
>>> {
>>> 	void *buf;
>>>
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
>>> 		return NULL;
>>>
>>> 	if (virt_addr_valid(instr->data.in) &&
>>> 	    !object_is_on_stack(instr->data.buf.in))
>>> 		return instr->data.buf.in;
>>>
>>> 	return kzalloc(instr->data.len, GFP_KERNEL);
>>> }
>>>
>>> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
>>> 				    void *buf)
>>> {
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
>>> 	    WARN_ON(!buf))
>>> 		return;
>>>
>>> 	if (buf == instr->data.buf.in)
>>> 		return;
>>>
>>> 	memcpy(instr->data.buf.in, buf, instr->data.len);
>>> 	kfree(buf);
>>> }
>>>
>>> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
>>> {
>>> 	void *buf;
>>>
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
>>> 		return NULL;
>>>
>>> 	if (virt_addr_valid(instr->data.out) &&
>>> 	    !object_is_on_stack(instr->data.buf.out))
>>> 		return instr->data.buf.out;
>>>
>>> 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
>>> }
>>>
>>> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
>>> 				    void *buf)
>>> {
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
>>> 	    WARN_ON(!buf))
>>> 		return;
>>>
>>> 	if (buf != instr->data.buf.out)
>>> 		kfree(buf);
>>> }
>>>   
>>
>> that is more convenient.
>> i will use meson_chip->databuf as the bounce mid-buffer now.
> 
> It won't work: the bounce buffer is allocated after the detection, and
> the detection code is calling ->exec_op().
> 
> Just add a new patch to you series adding these helpers to nand_base.c.
>

ok

> .
>

On 2018/11/7 0:16, Boris Brezillon wrote:
> On Tue, 6 Nov 2018 19:08:27 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> On 2018/11/6 18:22, Boris Brezillon wrote:
>>> On Tue, 6 Nov 2018 18:00:37 +0800
>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>    
>>>> On 2018/11/6 17:28, Boris Brezillon wrote:
>>>>> On Tue, 6 Nov 2018 17:08:00 +0800
>>>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>>>       
>>>>>> On 2018/11/5 23:53, Boris Brezillon wrote:
>>>>>>> On Fri, 2 Nov 2018 00:42:21 +0800
>>>>>>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>>>>>>>          
>>>>>>>> +
>>>>>>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
>>>>>>>> +{
>>>>>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>>>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>>>>>> +	u32 cmd;
>>>>>>>> +
>>>>>>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
>>>>>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>>> +
>>>>>>>> +	meson_nfc_drain_cmd(nfc);
>>>>>>>
>>>>>>> You probably don't want to drain the FIFO every time you read a byte on
>>>>>>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
>>>>>>> FIFO, right? If that's the case, you should queue as much DRD cmd as
>>>>>>> possible and only sync when the user explicitly requests it or when
>>>>>>> the INPUT/READ FIFO is full.
>>>>>>>          
>>>>>> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
>>>>>> nand cycle to read one byte and covers the 1st byte every time reading.
>>>>>> i think nfc controller is faster than nand cycle, but really it is not
>>>>>> high efficiency when reading so many bytes once.
>>>>>> Or use dma command here like read_page and read_page_raw.
>>>>>
>>>>> Yep, that's also an alternative, though you'll have to make sure the
>>>>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
>>>>> buffer when that's not the case.
>>>>>       
>>>> ok, i will try dma here.
>>>
>>> We should probably expose the bounce buf handling as generic helpers at
>>> the rawnand level:
>>>
>>> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
>>> {
>>> 	void *buf;
>>>
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
>>> 		return NULL;
>>>
>>> 	if (virt_addr_valid(instr->data.in) &&
>>> 	    !object_is_on_stack(instr->data.buf.in))
>>> 		return instr->data.buf.in;
>>>
>>> 	return kzalloc(instr->data.len, GFP_KERNEL);
>>> }
>>>
>>> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
>>> 				    void *buf)
>>> {
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
>>> 	    WARN_ON(!buf))
>>> 		return;
>>>
>>> 	if (buf == instr->data.buf.in)
>>> 		return;
>>>
>>> 	memcpy(instr->data.buf.in, buf, instr->data.len);
>>> 	kfree(buf);
>>> }
>>>
>>> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
>>> {
>>> 	void *buf;
>>>
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
>>> 		return NULL;
>>>
>>> 	if (virt_addr_valid(instr->data.out) &&
>>> 	    !object_is_on_stack(instr->data.buf.out))
>>> 		return instr->data.buf.out;
>>>
>>> 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
>>> }
>>>
>>> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
>>> 				    void *buf)
>>> {
>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
>>> 	    WARN_ON(!buf))
>>> 		return;
>>>
>>> 	if (buf != instr->data.buf.out)
>>> 		kfree(buf);
>>> }
>>>   
>>
>> that is more convenient.
>> i will use meson_chip->databuf as the bounce mid-buffer now.
> 
> It won't work: the bounce buffer is allocated after the detection, and
> the detection code is calling ->exec_op().
> 
> Just add a new patch to you series adding these helpers to nand_base.c.
> 

when using these helpers, i finally find that i still need a *info 
buffer* which is used to get status and ecc counter. even i don't need
to check *info buffer*, it is still needed. i just malloc *info_buffer* 
in driver now. and then i think some dedicated dma may need a minimum 
size(such as 4 bytes). it is luckly that meson nfc is not limited about 
the minimum size and i tested it.
so what is your suggestion about it?

> .
>

Hello,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 6 Nov 2018
11:22:06 +0100:

> On Tue, 6 Nov 2018 18:00:37 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
> > On 2018/11/6 17:28, Boris Brezillon wrote:  
> > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > Liang Yang <liang.yang@amlogic.com> wrote:
> > >     
> > >> On 2018/11/5 23:53, Boris Brezillon wrote:    
> > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > >>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> > >>>        
> > >>>> +
> > >>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > >>>> +{
> > >>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
> > >>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> > >>>> +	u32 cmd;
> > >>>> +
> > >>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > >>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > >>>> +
> > >>>> +	meson_nfc_drain_cmd(nfc);    
> > >>>
> > >>> You probably don't want to drain the FIFO every time you read a byte on
> > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > >>> possible and only sync when the user explicitly requests it or when
> > >>> the INPUT/READ FIFO is full.
> > >>>        
> > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > >> i think nfc controller is faster than nand cycle, but really it is not
> > >> high efficiency when reading so many bytes once.
> > >> Or use dma command here like read_page and read_page_raw.    
> > > 
> > > Yep, that's also an alternative, though you'll have to make sure the
> > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > buffer when that's not the case.
> > >     
> > ok, i will try dma here.  
> 
> We should probably expose the bounce buf handling as generic helpers at
> the rawnand level:
> 
> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> {
> 	void *buf;
> 
> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> 		return NULL;
> 
> 	if (virt_addr_valid(instr->data.in) &&
> 	    !object_is_on_stack(instr->data.buf.in))
> 		return instr->data.buf.in;
> 
> 	return kzalloc(instr->data.len, GFP_KERNEL);
> }
> 
> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> 				    void *buf)
> {
> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> 	    WARN_ON(!buf))
> 		return;
> 
> 	if (buf == instr->data.buf.in)
> 		return;
> 
> 	memcpy(instr->data.buf.in, buf, instr->data.len);
> 	kfree(buf);
> }
> 
> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> {
> 	void *buf;
> 
> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> 		return NULL;
> 
> 	if (virt_addr_valid(instr->data.out) &&
> 	    !object_is_on_stack(instr->data.buf.out))
> 		return instr->data.buf.out;
> 
> 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
> }
> 
> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> 				    void *buf)
> {
> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> 	    WARN_ON(!buf))
> 		return;
> 
> 	if (buf != instr->data.buf.out)
> 		kfree(buf);
> }

Not that I am against such function, but maybe they should come with
comments stating that there is no reliable way to find if a buffer is
DMA-able at runtime and these are just sanity checks (ie. required, but
probably not enough). This is my understanding of Wolfram's recent talk
at ELCE [1]. I suppose using the CONFIG_DMA_API_DEBUG option could help
more reliably to find such issues.

[1] https://www.youtube.com/watch?v=JDwaMClvV-s

Thanks,
Miquèl

+Wolfram to give some inputs on the DMA issue.

On Mon, 12 Nov 2018 17:13:51 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello,
> 
> Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 6 Nov 2018
> 11:22:06 +0100:
> 
> > On Tue, 6 Nov 2018 18:00:37 +0800
> > Liang Yang <liang.yang@amlogic.com> wrote:
> >   
> > > On 2018/11/6 17:28, Boris Brezillon wrote:    
> > > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > > Liang Yang <liang.yang@amlogic.com> wrote:
> > > >       
> > > >> On 2018/11/5 23:53, Boris Brezillon wrote:      
> > > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > > >>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> > > >>>          
> > > >>>> +
> > > >>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > > >>>> +{
> > > >>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
> > > >>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> > > >>>> +	u32 cmd;
> > > >>>> +
> > > >>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > > >>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > > >>>> +
> > > >>>> +	meson_nfc_drain_cmd(nfc);      
> > > >>>
> > > >>> You probably don't want to drain the FIFO every time you read a byte on
> > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > > >>> possible and only sync when the user explicitly requests it or when
> > > >>> the INPUT/READ FIFO is full.
> > > >>>          
> > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > > >> i think nfc controller is faster than nand cycle, but really it is not
> > > >> high efficiency when reading so many bytes once.
> > > >> Or use dma command here like read_page and read_page_raw.      
> > > > 
> > > > Yep, that's also an alternative, though you'll have to make sure the
> > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > > buffer when that's not the case.
> > > >       
> > > ok, i will try dma here.    
> > 
> > We should probably expose the bounce buf handling as generic helpers at
> > the rawnand level:
> > 
> > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > {
> > 	void *buf;
> > 
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > 		return NULL;
> > 
> > 	if (virt_addr_valid(instr->data.in) &&
> > 	    !object_is_on_stack(instr->data.buf.in))
> > 		return instr->data.buf.in;
> > 
> > 	return kzalloc(instr->data.len, GFP_KERNEL);
> > }
> > 
> > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > 				    void *buf)
> > {
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > 	    WARN_ON(!buf))
> > 		return;
> > 
> > 	if (buf == instr->data.buf.in)
> > 		return;
> > 
> > 	memcpy(instr->data.buf.in, buf, instr->data.len);
> > 	kfree(buf);
> > }
> > 
> > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > {
> > 	void *buf;
> > 
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > 		return NULL;
> > 
> > 	if (virt_addr_valid(instr->data.out) &&
> > 	    !object_is_on_stack(instr->data.buf.out))
> > 		return instr->data.buf.out;
> > 
> > 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > }
> > 
> > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > 				    void *buf)
> > {
> > 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > 	    WARN_ON(!buf))
> > 		return;
> > 
> > 	if (buf != instr->data.buf.out)
> > 		kfree(buf);
> > }  
> 
> Not that I am against such function, but maybe they should come with
> comments stating that there is no reliable way to find if a buffer is
> DMA-able at runtime and these are just sanity checks (ie. required, but
> probably not enough).

It's not 100% reliable, but it should cover most cases. Note that the
NAND framework already uses virt_addr_valid() to decide when to use its
internal bounce buffer, so this should be fixed too if we want a fully
reliable solution.

> This is my understanding of Wolfram's recent talk
> at ELCE [1].

Yes, you're right, but the NAND framework does not provide any guarantee
on the buf passed to ->exec_op() simply because the MTD layer does not
provide such a guarantee. Reworking that to match how the i2c framework
handles it is possible (with a flag set when the buffer is known to be
DMA-safe), but it requires rewriting all MTD users if we want to keep
decent perfs (the amount of data transfered to a flash is an order of
magnitude bigger than what you usually receive/send from/to an I2C
device). Also, I'm not even sure the DMA_SAFE flag covers all weird
cases like the "DMA engine embedded in the controller is not able to
access the whole physical memory range" one. So ideally we should have
something that checks if a pointer is DMA-safe at the device level and
then at the arch level.

A temporary solution would be to add a hook at the nand_controller
level:

	bool (*buf_is_dma_safe)(struct nand_chip *chip, void *buf,
				size_t len);

And then fallback to the default implementation when it's not
implemented:

static bool nand_buf_is_dma_safe(struct nand_chip *chip, void *buf,
				 size_t len)
{
	if (chip->controller->ops && chip->controller->ops->is_dma_safe)
		return chip->controller->ops->is_dma_safe(chip, buf,
							  len);

	return virt_addr_valid(buf) && !object_is_on_stack(buf);
}

> I suppose using the CONFIG_DMA_API_DEBUG option could help
> more reliably to find such issues.

Actually, the problem is not only about detecting offenders but being
able to detect when a buffer is not DMA-safe at runtime in order to
allocate/use a bounce buffer.

On Mon, 12 Nov 2018 17:54:16 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> +Wolfram to give some inputs on the DMA issue.
> 
> On Mon, 12 Nov 2018 17:13:51 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hello,
> > 
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 6 Nov 2018
> > 11:22:06 +0100:
> >   
> > > On Tue, 6 Nov 2018 18:00:37 +0800
> > > Liang Yang <liang.yang@amlogic.com> wrote:
> > >     
> > > > On 2018/11/6 17:28, Boris Brezillon wrote:      
> > > > > On Tue, 6 Nov 2018 17:08:00 +0800
> > > > > Liang Yang <liang.yang@amlogic.com> wrote:
> > > > >         
> > > > >> On 2018/11/5 23:53, Boris Brezillon wrote:        
> > > > >>> On Fri, 2 Nov 2018 00:42:21 +0800
> > > > >>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> > > > >>>            
> > > > >>>> +
> > > > >>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
> > > > >>>> +{
> > > > >>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
> > > > >>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> > > > >>>> +	u32 cmd;
> > > > >>>> +
> > > > >>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
> > > > >>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> > > > >>>> +
> > > > >>>> +	meson_nfc_drain_cmd(nfc);        
> > > > >>>
> > > > >>> You probably don't want to drain the FIFO every time you read a byte on
> > > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
> > > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as
> > > > >>> possible and only sync when the user explicitly requests it or when
> > > > >>> the INPUT/READ FIFO is full.
> > > > >>>            
> > > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
> > > > >> nand cycle to read one byte and covers the 1st byte every time reading.
> > > > >> i think nfc controller is faster than nand cycle, but really it is not
> > > > >> high efficiency when reading so many bytes once.
> > > > >> Or use dma command here like read_page and read_page_raw.        
> > > > > 
> > > > > Yep, that's also an alternative, though you'll have to make sure the
> > > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce
> > > > > buffer when that's not the case.
> > > > >         
> > > > ok, i will try dma here.      
> > > 
> > > We should probably expose the bounce buf handling as generic helpers at
> > > the rawnand level:
> > > 
> > > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
> > > {
> > > 	void *buf;
> > > 
> > > 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
> > > 		return NULL;
> > > 
> > > 	if (virt_addr_valid(instr->data.in) &&
> > > 	    !object_is_on_stack(instr->data.buf.in))
> > > 		return instr->data.buf.in;
> > > 
> > > 	return kzalloc(instr->data.len, GFP_KERNEL);
> > > }
> > > 
> > > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
> > > 				    void *buf)
> > > {
> > > 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
> > > 	    WARN_ON(!buf))
> > > 		return;
> > > 
> > > 	if (buf == instr->data.buf.in)
> > > 		return;
> > > 
> > > 	memcpy(instr->data.buf.in, buf, instr->data.len);
> > > 	kfree(buf);
> > > }
> > > 
> > > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
> > > {
> > > 	void *buf;
> > > 
> > > 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
> > > 		return NULL;
> > > 
> > > 	if (virt_addr_valid(instr->data.out) &&
> > > 	    !object_is_on_stack(instr->data.buf.out))
> > > 		return instr->data.buf.out;
> > > 
> > > 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
> > > }
> > > 
> > > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
> > > 				    void *buf)
> > > {
> > > 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
> > > 	    WARN_ON(!buf))
> > > 		return;
> > > 
> > > 	if (buf != instr->data.buf.out)
> > > 		kfree(buf);
> > > }    
> > 
> > Not that I am against such function, but maybe they should come with
> > comments stating that there is no reliable way to find if a buffer is
> > DMA-able at runtime and these are just sanity checks (ie. required, but
> > probably not enough).  
> 
> It's not 100% reliable, but it should cover most cases. Note that the
> NAND framework already uses virt_addr_valid() to decide when to use its
> internal bounce buffer, so this should be fixed too if we want a fully
> reliable solution.
> 
> > This is my understanding of Wolfram's recent talk
> > at ELCE [1].  
> 
> Yes, you're right, but the NAND framework does not provide any guarantee
> on the buf passed to ->exec_op() simply because the MTD layer does not
> provide such a guarantee. Reworking that to match how the i2c framework
> handles it is possible (with a flag set when the buffer is known to be
> DMA-safe), but it requires rewriting all MTD users if we want to keep
> decent perfs (the amount of data transfered to a flash is an order of
> magnitude bigger than what you usually receive/send from/to an I2C
> device). Also, I'm not even sure the DMA_SAFE flag covers all weird
> cases like the "DMA engine embedded in the controller is not able to
> access the whole physical memory range" one.

I forgot that this problem was handled at dma_map time (a bounce
buffer is allocated if needed, and this decision is based on
dev->dma_mask).

> So ideally we should have
> something that checks if a pointer is DMA-safe at the device level and
> then at the arch level.
> 
> A temporary solution would be to add a hook at the nand_controller
> level:
> 
> 	bool (*buf_is_dma_safe)(struct nand_chip *chip, void *buf,
> 				size_t len);
> 
> And then fallback to the default implementation when it's not
> implemented:
> 
> static bool nand_buf_is_dma_safe(struct nand_chip *chip, void *buf,
> 				 size_t len)
> {
> 	if (chip->controller->ops && chip->controller->ops->is_dma_safe)
> 		return chip->controller->ops->is_dma_safe(chip, buf,
> 							  len);
> 
> 	return virt_addr_valid(buf) && !object_is_on_stack(buf);
> }
> 
> > I suppose using the CONFIG_DMA_API_DEBUG option could help
> > more reliably to find such issues.  
> 
> Actually, the problem is not only about detecting offenders but being
> able to detect when a buffer is not DMA-safe at runtime in order to
> allocate/use a bounce buffer.

Hi Boris,

I have implemented dma access base on these helpers you provided below.
we prepare to send v7 version now, so when will these helpers be pushed?

On 2018/11/13 1:45, Boris Brezillon wrote:
> On Mon, 12 Nov 2018 17:54:16 +0100
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>> +Wolfram to give some inputs on the DMA issue.
>>
>> On Mon, 12 Nov 2018 17:13:51 +0100
>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>>> Hello,
>>>
>>> Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 6 Nov 2018
>>> 11:22:06 +0100:
>>>    
>>>> On Tue, 6 Nov 2018 18:00:37 +0800
>>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>>      
>>>>> On 2018/11/6 17:28, Boris Brezillon wrote:
>>>>>> On Tue, 6 Nov 2018 17:08:00 +0800
>>>>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>>>>          
>>>>>>> On 2018/11/5 23:53, Boris Brezillon wrote:
>>>>>>>> On Fri, 2 Nov 2018 00:42:21 +0800
>>>>>>>> Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>>>>>>>>             
>>>>>>>>> +
>>>>>>>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
>>>>>>>>> +{
>>>>>>>>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>>>>>>>>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>>>>>>> +	u32 cmd;
>>>>>>>>> +
>>>>>>>>> +	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
>>>>>>>>> +	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>>>> +
>>>>>>>>> +	meson_nfc_drain_cmd(nfc);
>>>>>>>>
>>>>>>>> You probably don't want to drain the FIFO every time you read a byte on
>>>>>>>> the bus, and I guess the INPUT FIFO is at least as big as the CMD
>>>>>>>> FIFO, right? If that's the case, you should queue as much DRD cmd as
>>>>>>>> possible and only sync when the user explicitly requests it or when
>>>>>>>> the INPUT/READ FIFO is full.
>>>>>>>>             
>>>>>>> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one
>>>>>>> nand cycle to read one byte and covers the 1st byte every time reading.
>>>>>>> i think nfc controller is faster than nand cycle, but really it is not
>>>>>>> high efficiency when reading so many bytes once.
>>>>>>> Or use dma command here like read_page and read_page_raw.
>>>>>>
>>>>>> Yep, that's also an alternative, though you'll have to make sure the
>>>>>> buffer passed through the nand_op_inst is DMA-safe, and use a bounce
>>>>>> buffer when that's not the case.
>>>>>>          
>>>>> ok, i will try dma here.
>>>>
>>>> We should probably expose the bounce buf handling as generic helpers at
>>>> the rawnand level:
>>>>
>>>> void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr)
>>>> {
>>>> 	void *buf;
>>>>
>>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR))
>>>> 		return NULL;
>>>>
>>>> 	if (virt_addr_valid(instr->data.in) &&
>>>> 	    !object_is_on_stack(instr->data.buf.in))
>>>> 		return instr->data.buf.in;
>>>>
>>>> 	return kzalloc(instr->data.len, GFP_KERNEL);
>>>> }
>>>>
>>>> void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr,
>>>> 				    void *buf)
>>>> {
>>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) ||
>>>> 	    WARN_ON(!buf))
>>>> 		return;
>>>>
>>>> 	if (buf == instr->data.buf.in)
>>>> 		return;
>>>>
>>>> 	memcpy(instr->data.buf.in, buf, instr->data.len);
>>>> 	kfree(buf);
>>>> }
>>>>
>>>> const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr)
>>>> {
>>>> 	void *buf;
>>>>
>>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR))
>>>> 		return NULL;
>>>>
>>>> 	if (virt_addr_valid(instr->data.out) &&
>>>> 	    !object_is_on_stack(instr->data.buf.out))
>>>> 		return instr->data.buf.out;
>>>>
>>>> 	return kmemdup(instr->data.buf.out, GFP_KERNEL);
>>>> }
>>>>
>>>> void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr,
>>>> 				    void *buf)
>>>> {
>>>> 	if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) ||
>>>> 	    WARN_ON(!buf))
>>>> 		return;
>>>>
>>>> 	if (buf != instr->data.buf.out)
>>>> 		kfree(buf);
>>>> }
>>>
>>> Not that I am against such function, but maybe they should come with
>>> comments stating that there is no reliable way to find if a buffer is
>>> DMA-able at runtime and these are just sanity checks (ie. required, but
>>> probably not enough).
>>
>> It's not 100% reliable, but it should cover most cases. Note that the
>> NAND framework already uses virt_addr_valid() to decide when to use its
>> internal bounce buffer, so this should be fixed too if we want a fully
>> reliable solution.
>>
>>> This is my understanding of Wolfram's recent talk
>>> at ELCE [1].
>>
>> Yes, you're right, but the NAND framework does not provide any guarantee
>> on the buf passed to ->exec_op() simply because the MTD layer does not
>> provide such a guarantee. Reworking that to match how the i2c framework
>> handles it is possible (with a flag set when the buffer is known to be
>> DMA-safe), but it requires rewriting all MTD users if we want to keep
>> decent perfs (the amount of data transfered to a flash is an order of
>> magnitude bigger than what you usually receive/send from/to an I2C
>> device). Also, I'm not even sure the DMA_SAFE flag covers all weird
>> cases like the "DMA engine embedded in the controller is not able to
>> access the whole physical memory range" one.
> 
> I forgot that this problem was handled at dma_map time (a bounce
> buffer is allocated if needed, and this decision is based on
> dev->dma_mask).
> 
>> So ideally we should have
>> something that checks if a pointer is DMA-safe at the device level and
>> then at the arch level.
>>
>> A temporary solution would be to add a hook at the nand_controller
>> level:
>>
>> 	bool (*buf_is_dma_safe)(struct nand_chip *chip, void *buf,
>> 				size_t len);
>>
>> And then fallback to the default implementation when it's not
>> implemented:
>>
>> static bool nand_buf_is_dma_safe(struct nand_chip *chip, void *buf,
>> 				 size_t len)
>> {
>> 	if (chip->controller->ops && chip->controller->ops->is_dma_safe)
>> 		return chip->controller->ops->is_dma_safe(chip, buf,
>> 							  len);
>>
>> 	return virt_addr_valid(buf) && !object_is_on_stack(buf);
>> }
>>
>>> I suppose using the CONFIG_DMA_API_DEBUG option could help
>>> more reliably to find such issues.
>>
>> Actually, the problem is not only about detecting offenders but being
>> able to detect when a buffer is not DMA-safe at runtime in order to
>> allocate/use a bounce buffer.
> 
> .
>

Hi Liang,

Liang Yang <liang.yang@amlogic.com> wrote on Thu, 15 Nov 2018 19:25:07
+0800:

> Hi Boris,
> 
> I have implemented dma access base on these helpers you provided below.
> we prepare to send v7 version now, so when will these helpers be pushed?

Thanks for your work. You can send the v7 so we will have a look at the
overall driver; but since we raised the DMA buffers issue we had a
discussion with Boris about how to handle them and I think we are going
to adopt the same solution as Wolfram in the I2C subsystem: manual
flagging. Sadly, this is probably the best we can do to ensure proper
DMA support.

There is nothing set is stone yet but I started a small rework to
handle MTD operations differently (and add a DMA_SAFE flag), you can
have a look there [1]. Don't base your work on it for now as it is just
a preliminary version, subject to big changes.

[1] https://github.com/miquelraynal/linux/commits/dma-safe-buffers

Thanks,
Miquèl

On Thu, 15 Nov 2018 14:04:00 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Liang,
> 
> Liang Yang <liang.yang@amlogic.com> wrote on Thu, 15 Nov 2018 19:25:07
> +0800:
> 
> > Hi Boris,
> > 
> > I have implemented dma access base on these helpers you provided below.
> > we prepare to send v7 version now, so when will these helpers be pushed?  
> 
> Thanks for your work. You can send the v7 so we will have a look at the
> overall driver; but since we raised the DMA buffers issue we had a
> discussion with Boris about how to handle them and I think we are going
> to adopt the same solution as Wolfram in the I2C subsystem: manual
> flagging. Sadly, this is probably the best we can do to ensure proper
> DMA support.
> 
> There is nothing set is stone yet but I started a small rework to
> handle MTD operations differently (and add a DMA_SAFE flag), you can
> have a look there [1]. Don't base your work on it for now as it is just
> a preliminary version, subject to big changes.

In order to not block the driver, I'd suggest that you move the helper
I proposed directly into your driver and prefix them with 'meson_'.

Hi Boris and Miquel,

understand. i will move helpers into nfc driver to avoid some errors 
when sending the patch.

On 2018/11/15 21:09, Boris Brezillon wrote:
> On Thu, 15 Nov 2018 14:04:00 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
>> Hi Liang,
>>
>> Liang Yang <liang.yang@amlogic.com> wrote on Thu, 15 Nov 2018 19:25:07
>> +0800:
>>
>>> Hi Boris,
>>>
>>> I have implemented dma access base on these helpers you provided below.
>>> we prepare to send v7 version now, so when will these helpers be pushed?
>>
>> Thanks for your work. You can send the v7 so we will have a look at the
>> overall driver; but since we raised the DMA buffers issue we had a
>> discussion with Boris about how to handle them and I think we are going
>> to adopt the same solution as Wolfram in the I2C subsystem: manual
>> flagging. Sadly, this is probably the best we can do to ensure proper
>> DMA support.
>>
>> There is nothing set is stone yet but I started a small rework to
>> handle MTD operations differently (and add a DMA_SAFE flag), you can
>> have a look there [1]. Don't base your work on it for now as it is just
>> a preliminary version, subject to big changes.
> 
> In order to not block the driver, I'd suggest that you move the helper
> I proposed directly into your driver and prefix them with 'meson_'.
> 
> .
>