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

[Liang Yang]

On 2018/11/6 17:28, Boris Brezillon wrote:
> On Tue, 6 Nov 2018 17:08:00 +0800
> Liang Yang <liang.yang@xxxxxxxxxxx> wrote:
>
> > On 2018/11/5 23:53, Boris Brezillon wrote:
> > > On Fri, 2 Nov 2018 00:42:21 +0800
> > > Jianxin Pan <jianxin.pan@xxxxxxxxxxx> 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.
> .