Re: [RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs

[Jan Glauber <jan.glauber@caviumnetworks.com>]

On Wed, Mar 29, 2017 at 11:29:32AM +0200, Boris Brezillon wrote:
> Hi Jan,
> 
> This is a preliminary review, I might come up with additional comments
> later on.

Hi Boris,

thanks for reviewing the driver. I'm exactly looking for comments about
the design and fundamental things I need to change. I didn't even bother
to run checkpatch on the RFC :).

> On Mon, 27 Mar 2017 18:05:24 +0200
> Jan Glauber <jglauber@cavium.com> wrote:
> 
> > Add a driver for the nand flash controller as found on Cavium's
> > ARM64 SOCs.
> > 
> > The nand flash controller can support up to 8 chips and is presented
> > as a PCI device. It uses a DMA engine to transfer data between the nand
> > and L2/DRAM and a programmable command queue to issue multiple
> > nand flash commands together.
> > 
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> >  drivers/mtd/nand/Kconfig       |    6 +
> >  drivers/mtd/nand/Makefile      |    1 +
> >  drivers/mtd/nand/cavium_nand.c | 1160 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/cavium_nand.h |  231 ++++++++
> >  4 files changed, 1398 insertions(+)
> >  create mode 100644 drivers/mtd/nand/cavium_nand.c
> >  create mode 100644 drivers/mtd/nand/cavium_nand.h
> > 
> 
> [...]
> 
> > +
> > +/* default parameters used for probing chips */
> > +static int default_onfi_timing = 0;
> > +static int default_width = 1; /* 8 bit */
> > +static int default_page_size = 2048;
> > +static struct ndf_set_tm_par_cmd default_timing_parms;
> 
> Hm, I don't think this is a good idea to have default parameters. The
> NAND core should provide anything you need in term of NAND chip
> detection, so selection the timings, bus width and page size should not
> be a problem.

If the ONFI information can be parsed without these default I'm fine
with removing them.

> > +
> > +/*
> > + * Get the number of bits required to encode the column bits. This
> > + * does not include bits required for the OOB area.
> > + */
> > +static int ndf_get_column_bits(struct nand_chip *nand)
> > +{
> > +	int page_size;
> > +
> > +	if (!nand)
> > +		page_size = default_page_size;
> > +	else
> > +		page_size = nand->onfi_params.byte_per_page;
> > +	return get_bitmask_order(page_size - 1);
> > +}
> 
> Please drop this helper, the page size is already exposed in
> mtd->writesize.

OK.

> > +
> > +irqreturn_t cvm_nfc_isr(int irq, void *dev_id)
> > +{
> > +	struct cvm_nfc *tn = dev_id;
> > +
> > +	wake_up(&tn->controller.wq);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Read a single byte from the temporary buffer. Used after READID
> > + * to get the NAND information and for STATUS.
> > + */
> > +static u8 cvm_nand_read_byte(struct mtd_info *mtd)
> > +{
> > +	struct nand_chip *nand = mtd_to_nand(mtd);
> > +	struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +
> > +	if (tn->use_status)
> > +		return *tn->stat;
> > +
> > +	if (tn->buf.data_index < tn->buf.data_len)
> > +		return tn->buf.dmabuf[tn->buf.data_index++];
> > +	else
> > +		dev_err(tn->dev, "No data to read\n");
> > +
> > +	return 0xff;
> > +}
> > +
> > +/*
> > + * Read a number of pending bytes from the temporary buffer. Used
> > + * to get page and OOB data.
> > + */
> > +static void cvm_nand_read_buf(struct mtd_info *mtd, u8 *buf, int len)
> > +{
> > +	struct nand_chip *nand = mtd_to_nand(mtd);
> > +	struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +
> > +	if (len > tn->buf.data_len - tn->buf.data_index) {
> > +		dev_err(tn->dev, "Not enough data for read of %d bytes\n", len);
> > +		return;
> > +	}
> > +
> > +	memcpy(buf, tn->buf.dmabuf + tn->buf.data_index, len);
> > +	tn->buf.data_index += len;
> > +}
> > +
> > +static void cvm_nand_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
> > +{
> > +	struct nand_chip *nand = mtd_to_nand(mtd);
> > +	struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +
> > +	memcpy(tn->buf.dmabuf + tn->buf.data_len, buf, len);
> > +	tn->buf.data_len += len;
> > +}
> 
> It seems that cvm_nand_read/write_byte/buf() are returning data that
> have already been retrieved (problably during the ->cmdfunc() phase).

Yes.

> That's not how it's supposed to work. The core is expecting the data
> transfer to be done when ->read/write_buf() is called. Doing that in
> ->cmdfunc() is risky, because when you're there you have no clue about
> how much bytes the core expect.

It seems to work fine, I've never seen the core trying to do more bytes in
the read/write_buf() then requested in ->cmdfunc().

> [...]
> 
> > +static int cvm_nand_set_features(struct mtd_info *mtd,
> > +				      struct nand_chip *chip, int feature_addr,
> > +				      u8 *subfeature_para)
> 
> Do you really need you own implementation of ->set_feature()?
> 
> > +{
> > +	struct nand_chip *nand = mtd_to_nand(mtd);
> > +	struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +	int rc;
> > +
> > +	rc = ndf_build_pre_cmd(tn, NAND_CMD_SET_FEATURES, 1, feature_addr, 0);
> > +	if (rc)
> > +		return rc;
> > +
> > +	memcpy(tn->buf.dmabuf, subfeature_para, 4);
> > +	memset(tn->buf.dmabuf + 4, 0, 4);
> > +
> > +	rc = ndf_queue_cmd_write(tn, 8);
> > +	if (rc)
> > +		return rc;
> > +
> > +	ndf_setup_dma(tn, 0, tn->buf.dmaaddr, 8);
> > +
> > +	rc = ndf_wait_for_busy_done(tn);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = ndf_build_post_cmd(tn);
> > +	if (rc)
> > +		return rc;
> > +	return 0;
> > +}
> > +
> 
> [...]
> 
> > +
> > +/*
> > + * Read a page from NAND. If the buffer has room, the out of band
> > + * data will be included.
> > + */
> > +int ndf_page_read(struct cvm_nfc *tn, u64 addr, int len)
> > +{
> > +	int rc;
> > +
> > +	memset(tn->buf.dmabuf, 0xff, len);
> > +	rc = ndf_read(tn, NAND_CMD_READ0, 4, addr, NAND_CMD_READSTART, len);
> > +	if (rc)
> > +		return rc;
> > +
> > +	return rc;
> > +}
> > +
> > +/* Erase a NAND block */
> > +static int ndf_block_erase(struct cvm_nfc *tn, u64 addr)
> 
> Ditto.
> 
> > +{
> > +	struct nand_chip *nand = tn->controller.active;
> > +	int row, rc;
> > +
> > +	row = addr >> ndf_get_column_bits(nand);
> > +	rc = ndf_build_pre_cmd(tn, NAND_CMD_ERASE1, 2, row, NAND_CMD_ERASE2);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* Wait for R_B to signal erase is complete  */
> > +        rc = ndf_wait_for_busy_done(tn);
> > +        if (rc)
> > +                return rc;
> > +
> > +        rc = ndf_build_post_cmd(tn);
> > +        if (rc)
> > +                return rc;
> 
> Use tabs and not spaces for indentation (you can run checkpatch to
> identify these coding style issues).

I'll fix these in the next version.

> > +
> > +        /* Wait until the command queue is idle */
> > +	return ndf_wait_idle(tn);
> > +}
> > +
> 
> [...]
> 
> 
> > +
> > +static void cvm_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
> > +				  int column, int page_addr)
> 
> Did you try implementing ->cmd_ctrl() instead of ->cmdfunc(). Your
> controller seems to be highly configurable and, at first glance, I think
> you can simplify the driver by implementing ->cmd_ctrl() and relying on
> the default ->cmdfunc() implementation.
> 

I've looked at the sunxi_nand driver but ->cmd_ctrl() is very different
from ->cmdfunc() and the later looks like a better match for our controller.

The Cavium controller needs to write the commands (NAND_CMD_READ0, etc.)
into its pseudo instructions (see ndf_queue_cmd_cle()).
So how can I do this low-level stuff with ->cmd_ctrl()?

For instance for reading data I have ndf_page_read() that is used for
both NAND_CMD_READ0 and NAND_CMD_READOOB. Without hacking into ->cmdfunc(),
how would I differentiate between the two commands in read_buf()?

> > +{
> > +	struct nand_chip *nand = mtd_to_nand(mtd);
> > +	struct cvm_nand_chip *cvm_nand = to_cvm_nand(nand);
> > +	struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +	int rc;
> > +
> > +	tn->selected_chip = cvm_nand->cs;
> > +	if (tn->selected_chip < 0 || tn->selected_chip >= NAND_MAX_CHIPS) {
> > +		dev_err(tn->dev, "invalid chip select\n");
> > +		return;
> > +	}
> > +
> > +	tn->use_status = false;
> > +	cvm_nand->oob_access = false;
> > +
> > +	switch (command) {
> > +	case NAND_CMD_READID:
> > +		tn->buf.data_index = 0;
> > +		memset(tn->buf.dmabuf, 0xff, 8);
> > +		rc = ndf_read(tn, command, 1, column, 0, 8);
> > +		if (rc < 0)
> > +			dev_err(tn->dev, "READID failed with %d\n", rc);
> > +		else
> > +			tn->buf.data_len = rc;
> > +		break;
> > +
> > +	case NAND_CMD_READOOB:
> > +		cvm_nand->oob_access = true;
> > +		tn->buf.data_index = 0;
> > +		tn->buf.data_len = ndf_page_read(tn,
> > +				(page_addr << nand->page_shift) + 0x800,
> > +				mtd->oobsize);
> > +
> > +		if (tn->buf.data_len < mtd->oobsize) {
> > +			dev_err(tn->dev, "READOOB failed with %d\n",
> > +				tn->buf.data_len);
> > +			tn->buf.data_len = 0;
> > +		}
> > +		break;
> > +
> > +	case NAND_CMD_READ0:
> > +		tn->buf.data_index = 0;
> > +		tn->buf.data_len = ndf_page_read(tn,
> > +				column + (page_addr << nand->page_shift),
> > +				(1 << nand->page_shift) + mtd->oobsize);
> > +		if (tn->buf.data_len < (1 << nand->page_shift) + mtd->oobsize) {
> > +			dev_err(tn->dev, "READ0 failed with %d\n",
> > +				tn->buf.data_len);
> > +			tn->buf.data_len = 0;
> > +		}
> > +		break;
> > +
> > +	case NAND_CMD_STATUS:
> > +		tn->use_status = true;
> > +		memset(tn->stat, 0xff, 8);
> > +		rc = ndf_read(tn, command, 0, 0, 0, 8);
> > +		if (rc < 0)
> > +			dev_err(tn->dev, "STATUS failed with %d\n", rc);
> > +		break;
> > +
> > +	case NAND_CMD_RESET:
> > +		tn->buf.data_index = 0;
> > +		tn->buf.data_len = 0;
> > +		memset(tn->buf.dmabuf, 0xff, tn->buf.dmabuflen);
> > +		rc = cvm_nand_reset(tn);
> > +		if (rc < 0)
> > +			dev_err(tn->dev, "RESET failed with %d\n", rc);
> > +                break;
> > +
> > +	case NAND_CMD_PARAM:
> > +		tn->buf.data_index = column;
> > +		memset(tn->buf.dmabuf, 0xff, tn->buf.dmabuflen);
> > +		rc = ndf_read(tn, command, 1, 0, 0, 2048);
> > +		if (rc < 0)
> > +			dev_err(tn->dev, "PARAM failed with %d\n", rc);
> > +		else
> > +			tn->buf.data_len = rc;
> > +                break;
> > +
> > +	case NAND_CMD_RNDOUT:
> > +		tn->buf.data_index = column;
> > +		break;
> > +
> > +	case NAND_CMD_ERASE1:
> > +		if (ndf_block_erase(tn, page_addr << nand->page_shift))
> > +			dev_err(tn->dev, "ERASE1 failed\n");
> > +		break;
> > +
> > +	case NAND_CMD_ERASE2:
> > +		/* We do all erase processing in the first command, so ignore
> > +		 * this one.
> > +		 */
> > +		break;
> > +
> > +	case NAND_CMD_SEQIN:
> > +		if (column == mtd->writesize)
> > +			cvm_nand->oob_access = true;
> > +		tn->buf.data_index = column;
> > +		tn->buf.data_len = column;
> > +		cvm_nand->selected_page = page_addr;
> > +		break;
> > +
> > +	case NAND_CMD_PAGEPROG:
> > +		rc = ndf_page_write(tn,
> > +			cvm_nand->selected_page << nand->page_shift);
> > +		if (rc)
> > +			dev_err(tn->dev, "PAGEPROG failed with %d\n", rc);
> > +		break;
> > +
> > +	default:
> > +		WARN_ON_ONCE(1);
> > +		dev_err(tn->dev, "unhandled nand cmd: %x\n", command);
> > +	}
> > +}
> > +
> > +static int cvm_nfc_chip_init_timings(struct cvm_nand_chip *chip,
> > +					   struct device_node *np)
> > +{
> > +	const struct nand_sdr_timings *timings;
> > +	int ret, mode;
> > +
> > +	mode = onfi_get_async_timing_mode(&chip->nand);
> > +	if (mode == ONFI_TIMING_MODE_UNKNOWN) {
> > +		mode = chip->nand.onfi_timing_mode_default;
> > +	} else {
> > +		u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
> > +
> > +		mode = fls(mode) - 1;
> > +		if (mode < 0)
> > +			mode = 0;
> > +
> > +		feature[0] = mode;
> > +		ret = chip->nand.onfi_set_features(&chip->nand.mtd, &chip->nand,
> > +						ONFI_FEATURE_ADDR_TIMING_MODE,
> > +						feature);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	timings = onfi_async_timing_mode_to_sdr_timings(mode);
> > +	if (IS_ERR(timings))
> > +		return PTR_ERR(timings);
> > +
> > +	return cvm_nfc_chip_set_timings(chip, timings);
> > +}
> 
> 
> Please have a look at the ->setup_data_interface() hook [1] instead of
> initializing the timings at probe/init time.

OK.

> [...]
> 
> > diff --git a/drivers/mtd/nand/cavium_nand.h b/drivers/mtd/nand/cavium_nand.h
> > new file mode 100644
> > index 0000000..7030c57
> > --- /dev/null
> > +++ b/drivers/mtd/nand/cavium_nand.h
> > @@ -0,0 +1,231 @@
> > +#ifndef _CAVIUM_NAND_H
> > +#define _CAVIUM_NAND_H
> > +
> > +#include <linux/bitops.h>
> > +
> > +/*
> > + * The NDF_CMD queue takes commands between 16 - 128 bit.
> > + * All commands must be 16 bit aligned and are little endian.
> > + * WAIT_STATUS commands must be 64 bit aligned.
> > + * Commands are selected by the 4 bit opcode.
> > + *
> > + * Available Commands:
> > + *
> > + * 16 Bit:
> > + *   NOP
> > + *   WAIT
> > + *   BUS_ACQ, BUS_REL
> > + *   CHIP_EN, CHIP_DIS
> > + *
> > + * 32 Bit:
> > + *   CLE_CMD
> > + *   RD_CMD, RD_EDO_CMD
> > + *   WR_CMD
> > + *
> > + * 64 Bit:
> > + *   SET_TM_PAR
> > + *
> > + * 96 Bit:
> > + *   ALE_CMD
> > + *
> > + * 128 Bit:
> > + *   WAIT_STATUS, WAIT_STATUS_ALE
> > + */
> > +
> > +/* NDF Register offsets */
> > +#define NDF_CMD			0x0
> > +#define NDF_MISC		0x8
> > +#define NDF_ECC_CNT		0x10
> > +#define NDF_DRBELL		0x30
> > +#define NDF_ST_REG		0x38	/* status */
> > +#define NDF_INT			0x40
> > +#define NDF_INT_W1S		0x48
> > +#define NDF_DMA_CFG		0x50
> > +#define NDF_DMA_ADR		0x58
> > +#define NDF_INT_ENA_W1C		0x60
> > +#define NDF_INT_ENA_W1S		0x68
> > +
> > +/* NDF command opcodes */
> > +#define NDF_OP_NOP		0x0
> > +#define NDF_OP_SET_TM_PAR	0x1
> > +#define NDF_OP_WAIT		0x2
> > +#define NDF_OP_CHIP_EN_DIS	0x3
> > +#define NDF_OP_CLE_CMD		0x4
> > +#define NDF_OP_ALE_CMD		0x5
> > +#define NDF_OP_WR_CMD		0x8
> > +#define NDF_OP_RD_CMD		0x9
> > +#define NDF_OP_RD_EDO_CMD	0xa
> > +#define NDF_OP_WAIT_STATUS	0xb	/* same opcode for WAIT_STATUS_ALE */
> > +#define NDF_OP_BUS_ACQ_REL	0xf
> > +
> > +#define NDF_BUS_ACQUIRE		1
> > +#define NDF_BUS_RELEASE		0
> > +
> > +struct ndf_nop_cmd {
> > +	u16 opcode	: 4;
> > +	u16 nop		: 12;
> > +};
> > +
> > +struct ndf_wait_cmd {
> > +	u16 opcode	: 4;
> > +	u16 r_b		: 1;	/* wait for one cycle or PBUS_WAIT deassert */
> > +	u16		: 3;
> > +	u16 wlen	: 3;	/* timing parameter select */
> > +	u16		: 5;
> > +};
> > +
> > +struct ndf_bus_cmd {
> > +	u16 opcode	: 4;
> > +	u16 direction	: 4;	/* 1 = acquire, 0 = release */
> > +	u16		: 8;
> > +};
> > +
> > +struct ndf_chip_cmd {
> > +	u16 opcode	: 4;
> > +	u16 chip	: 3;	/* select chip, 0 = disable */
> > +	u16 enable	: 1;	/* 1 = enable, 0 = disable */
> > +	u16 bus_width	: 2;	/* 10 = 16 bit, 01 = 8 bit */
> > +	u16		: 6;
> > +};
> > +
> > +struct ndf_cle_cmd {
> > +	u32 opcode	: 4;
> > +	u32		: 4;
> > +	u32 cmd_data	: 8;	/* command sent to the PBUS AD pins */
> > +	u32 clen1	: 3;	/* time between PBUS CLE and WE asserts */
> > +	u32 clen2	: 3;	/* time WE remains asserted */
> > +	u32 clen3	: 3;	/* time between WE deassert and CLE */
> > +	u32		: 7;
> > +};
> > +
> > +/* RD_EDO_CMD uses the same layout as RD_CMD */
> > +struct ndf_rd_cmd {
> > +	u32 opcode	: 4;
> > +	u32 data	: 16;	/* data bytes */
> > +	u32 rlen1	: 3;
> > +	u32 rlen2	: 3;
> > +	u32 rlen3	: 3;
> > +	u32 rlen4	: 3;
> > +};
> > +
> > +struct ndf_wr_cmd {
> > +	u32 opcode	: 4;
> > +	u32 data	: 16;	/* data bytes */
> > +	u32		: 4;
> > +	u32 wlen1	: 3;
> > +	u32 wlen2	: 3;
> > +	u32		: 3;
> > +};
> > +
> > +struct ndf_set_tm_par_cmd {
> > +	u64 opcode	: 4;
> > +	u64 tim_mult	: 4;	/* multiplier for the seven paramters */
> > +	u64 tm_par1	: 8;	/* --> Following are the 7 timing parameters that */
> > +	u64 tm_par2	: 8;	/*     specify the number of coprocessor cycles.  */
> > +	u64 tm_par3	: 8;	/*     A value of zero means one cycle.		  */
> > +	u64 tm_par4	: 8;	/*     All values are scaled by tim_mult	  */
> > +	u64 tm_par5	: 8;	/*     using tim_par * (2 ^ tim_mult).		  */
> > +	u64 tm_par6	: 8;
> > +	u64 tm_par7	: 8;
> > +};
> > +
> > +struct ndf_ale_cmd {
> > +	u32 opcode	: 4;
> > +	u32		: 4;
> > +	u32 adr_byte_num: 4;	/* number of address bytes to be sent */
> > +	u32		: 4;
> > +	u32 alen1	: 3;
> > +	u32 alen2	: 3;
> > +	u32 alen3	: 3;
> > +	u32 alen4	: 3;
> > +	u32		: 4;
> > +	u8 adr_byt1;
> > +	u8 adr_byt2;
> > +	u8 adr_byt3;
> > +	u8 adr_byt4;
> > +	u8 adr_byt5;
> > +	u8 adr_byt6;
> > +	u8 adr_byt7;
> > +	u8 adr_byt8;
> > +};
> > +
> > +struct ndf_wait_status_cmd {
> > +	u32 opcode	: 4;
> > +	u32		: 4;
> > +	u32 data	: 8;	/* data */
> > +	u32 clen1	: 3;
> > +	u32 clen2	: 3;
> > +	u32 clen3	: 3;
> > +	u32		: 8;
> > +	u32 ale_ind	: 8;	/* set to 5 to select WAIT_STATUS_ALE command */
> > +	u32 adr_byte_num: 4;	/* ALE only: number of address bytes to be sent */
> > +	u32		: 4;
> > +	u32 alen1	: 3;	/* ALE only */
> > +	u32 alen2	: 3;	/* ALE only */
> > +	u32 alen3	: 3;	/* ALE only */
> > +	u32 alen4	: 3;	/* ALE only */
> > +	u32		: 4;
> > +	u8 adr_byt[4];		/* ALE only */
> > +	u32 nine	: 4;	/* set to 9 */
> > +	u32 and_mask	: 8;
> > +	u32 comp_byte	: 8;
> > +	u32 rlen1	: 3;
> > +	u32 rlen2	: 3;
> > +	u32 rlen3	: 3;
> > +	u32 rlen4	: 3;
> > +};
> > +
> > +union ndf_cmd {
> > +	u64 val[2];
> > +	union {
> > +		struct ndf_nop_cmd		nop;
> > +		struct ndf_wait_cmd		wait;
> > +		struct ndf_bus_cmd		bus_acq_rel;
> > +		struct ndf_chip_cmd		chip_en_dis;
> > +		struct ndf_cle_cmd		cle_cmd;
> > +		struct ndf_rd_cmd		rd_cmd;
> > +		struct ndf_wr_cmd		wr_cmd;
> > +		struct ndf_set_tm_par_cmd	set_tm_par;
> > +		struct ndf_ale_cmd		ale_cmd;
> > +		struct ndf_wait_status_cmd	wait_status;
> > +	} u;
> > +};
> 
> I need some time to process all these information, but your controller
> seems to be a complex/highly-configurable beast. That's really
> interesting :-).
> I'll come up with more comments/question after reviewing more carefully
> the command creation logic.

Great. I'm afraid out controller is quite different from existing
hardware, at least I didn't find a driver that does things simalar (like
the command building and queueing).

I'm happy to help with any more information you need about our hardware.

Thanks!
Jan

> Thanks,
> 
> Boris
> 
> [1]http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L854