[U-Boot] [PATCH 22/34] nand: Add zynq nand controller driver support

[Scott Wood]

On Tue, 2013-11-05 at 23:16 +0530, Jagannadha Sutradharudu Teki wrote:
> +#define zynq_nand_smc_base	((struct zynq_nand_smc_regs *)ZYNQ_SMC_BASEADDR)

__iomem

Please consider running sparse on new code.

> +/*
> + * struct zynq_nand_command_format - Defines NAND flash command format
> + * @start_cmd:		First cycle command (Start command)
> + * @end_cmd:		Second cycle command (Last command)
> + * @addr_cycles:	Number of address cycles required to send the address
> + * @end_cmd_valid:	The second cycle command is valid for cmd or data phase
> + */
> +struct zynq_nand_command_format {
> +	int start_cmd;
> +	int end_cmd;
> +	u8 addr_cycles;
> +	u8 end_cmd_valid;
> +};

Why are the first two "int" and the second two "u8"?

> +
> +/*
> + * struct zynq_nand_info - Defines the NAND flash driver instance
> + * @parts:		Pointer to the mtd_partition structure
> + * @nand_base:		Virtual address of the NAND flash device
> + * @end_cmd_pending:	End command is pending
> + * @end_cmd:		End command
> + */
> +struct zynq_nand_info {
> +#ifdef CONFIG_MTD_PARTITIONS
> +	struct mtd_partition	*parts;
> +#endif
> +	void __iomem		*nand_base;
> +	unsigned long		end_cmd_pending;
> +	unsigned long		end_cmd;
> +};

Why long?

> +
> +#define ONDIE_ECC_FEATURE_ADDR	0x90
> +
> +/*  The NAND flash operations command format */
> +static const struct zynq_nand_command_format zynq_nand_commands[] = {
> +	{NAND_CMD_READ0, NAND_CMD_READSTART, 5, ZYNQ_NAND_CMD_PHASE},
> +	{NAND_CMD_RNDOUT, NAND_CMD_RNDOUTSTART, 2, ZYNQ_NAND_CMD_PHASE},
> +	{NAND_CMD_READID, NAND_CMD_NONE, 1, NAND_CMD_NONE},
> +	{NAND_CMD_STATUS, NAND_CMD_NONE, 0, NAND_CMD_NONE},
> +	{NAND_CMD_SEQIN, NAND_CMD_PAGEPROG, 5, ZYNQ_NAND_DATA_PHASE},
> +	{NAND_CMD_RNDIN, NAND_CMD_NONE, 2, NAND_CMD_NONE},
> +	{NAND_CMD_ERASE1, NAND_CMD_ERASE2, 3, ZYNQ_NAND_CMD_PHASE},
> +	{NAND_CMD_RESET, NAND_CMD_NONE, 0, NAND_CMD_NONE},
> +	{NAND_CMD_PARAM, NAND_CMD_NONE, 1, NAND_CMD_NONE},
> +	{NAND_CMD_GET_FEATURES, NAND_CMD_NONE, 1, NAND_CMD_NONE},
> +	{NAND_CMD_SET_FEATURES, NAND_CMD_NONE, 1, NAND_CMD_NONE},
> +	{NAND_CMD_NONE, NAND_CMD_NONE, 0, 0},

I'm not sure that NAND_CMD_NONE is appropriate for end_cmd_valid -- it's
not a field that otherwise takes NAND_CMD_xxx values.  Just use zero.

> +	/* Add all the flash commands supported by the flash device and Linux
> +	 * The cache program command is not supported by driver because driver
> +	 * cant differentiate between page program and cached page program from
> +	 * start command, these commands can be differentiated through end
> +	 * command, which doesn't fit in to the driver design. The cache program
> +	 * command is not supported by NAND subsystem also, look at 1612 line
> +	 * number (in nand_write_page function) of nand_base.c file.
> +	 * {NAND_CMD_SEQIN, NAND_CMD_CACHEDPROG, 5, ZYNQ_NAND_YES}
> +	 */

What is ZYNQ_NAND_YES?

> +};
> +
> +/* Define default oob placement schemes for large and small page devices */
> +static struct nand_ecclayout nand_oob_16 = {
> +	.eccbytes = 3,
> +	.eccpos = {13, 14, 15},
> +	.oobfree = {
> +		{ .offset = 0, .length = 12 }
> +	}
> +};

Isn't your oobfree overlapping the bad block marker?

Normally (with small-page NAND) the marker is at offset 5 for 8-bit and
offset 0 for 16-bit, not at offset 12.

> +	/* Wait max 10ms */
> +	timeout = 10;
> +	status = readl(&zynq_nand_smc_base->esr);
> +	while (status & ZYNQ_NAND_ECC_BUSY) {
> +		status = readl(&zynq_nand_smc_base->esr);
> +		if (timeout == 0)
> +			return -1;
> +		timeout--;
> +		udelay(1);
> +	}

Comment says 10ms, code says 10us.

> +
> +	return status;
> +}
> +
> +/*
> + * zynq_nand_init_nand_flash - Initialize NAND controller
> + * @option:	Device property flags
> + *
> + * This function initializes the NAND flash interface on the NAND controller.
> + *
> + * returns:	0 on success or error value on failure
> + */
> +static int zynq_nand_init_nand_flash(int option)
> +{
> +	u32 status;
> +
> +	/* disable interrupts */
> +	writel(ZYNQ_NAND_CLR_CONFIG, &zynq_nand_smc_base->cfr);
> +	/* Initialize the NAND interface by setting cycles and operation mode */
> +	writel(ZYNQ_NAND_SET_CYCLES, &zynq_nand_smc_base->scr);
> +	if (option & NAND_BUSWIDTH_16)
> +		writel((ZYNQ_NAND_SET_OPMODE | 0x1), &zynq_nand_smc_base->sor);
> +	else
> +		writel(ZYNQ_NAND_SET_OPMODE, &zynq_nand_smc_base->sor);

That 0x1 should be defined symbolically.

> +		/* ECC value valid */
> +		if (ecc_status & 0x40) {

0x40 should be defined symbolically.

> +/*
> + * onehot - onehot function
> + * @value:	value to check for onehot
> + *
> + * This function checks whether a value is onehot or not.
> + * onehot is if and only if onebit is set.
> + *
> + */
> +static int onehot(unsigned short value)
> +{
> +	return ((value & (value-1)) == 0);
> +}

Too many parentheses, and need spaces around '-'.
Why "unsigned short"?
Why use an obscure (at least to software people) name for this, rather
than some variant of "power of 2", or even "one_bit_set"?
s/onebit/one bit/

Note that the above function will also return true if no bits are set,
which contradicts the definition in the comment (yes, I know you don't
call it under such circumstances).

Function should return bool.

> +/*
> + * zynq_nand_correct_data - ECC correction function
> + * @mtd:	Pointer to the mtd_info structure
> + * @buf:	Pointer to the page data
> + * @read_ecc:	Pointer to the ECC value read from spare data area
> + * @calc_ecc:	Pointer to the calculated ECC value
> + *
> + * This function corrects the ECC single bit errors & detects 2-bit errors.
> + *
> + * returns:	0 if no ECC errors found
> + *		1 if single bit error found and corrected.
> + *		-1 if multiple ECC errors found.
> + */
> +static int zynq_nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
> +			unsigned char *read_ecc, unsigned char *calc_ecc)
> +{
> +	unsigned char bit_addr;
> +	unsigned int byte_addr;
> +	unsigned short ecc_odd, ecc_even;
> +	unsigned short read_ecc_lower, read_ecc_upper;
> +	unsigned short calc_ecc_lower, calc_ecc_upper;
> +
> +	read_ecc_lower = (read_ecc[0] | (read_ecc[1] << 8)) & 0xfff;
> +	read_ecc_upper = ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) & 0xfff;
> +
> +	calc_ecc_lower = (calc_ecc[0] | (calc_ecc[1] << 8)) & 0xfff;
> +	calc_ecc_upper = ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) & 0xfff;
> +
> +	ecc_odd = read_ecc_lower ^ calc_ecc_lower;
> +	ecc_even = read_ecc_upper ^ calc_ecc_upper;
> +
> +	if ((ecc_odd == 0) && (ecc_even == 0))
> +		return 0;       /* no error */
> +	else if (ecc_odd == (~ecc_even & 0xfff)) {
> +		/* bits [11:3] of error code is byte offset */
> +		byte_addr = (ecc_odd >> 3) & 0x1ff;
> +		/* bits [2:0] of error code is bit offset */
> +		bit_addr = ecc_odd & 0x7;
> +		/* Toggling error bit */
> +		buf[byte_addr] ^= (1 << bit_addr);
> +		return 1;
> +	} else if (onehot(ecc_odd | ecc_even) == 1) {
> +		return 1; /* one error in parity */
> +	} else {
> +		return -1; /* Uncorrectable error */
> +	}
> +}

You don't need "else" after "return"...

> +
> +/*
> + * zynq_nand_read_oob - [REPLACABLE] the most common OOB data read function

The version in nand_base.c is replaceable and (perhaps) the most common
implementation, but this one isn't.

> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @page:	page number to read
> + * @sndcmd:	flag whether to issue read command or not
> + */
> +static int zynq_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> +			int page)
> +{
> +	unsigned long data_width = 4;

Why long?

> +	unsigned long data_phase_addr = 0;
> +	u8 *p;
> +
> +	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> +
> +	p = chip->oob_poi;
> +	chip->read_buf(mtd, p, (mtd->oobsize - data_width));
> +	p += (mtd->oobsize - data_width);

Unnecessary parentheses.  Likewise elsewhere in the patch.

> +static int zynq_nand_write_page_hwecc(struct mtd_info *mtd,
> +	struct nand_chip *chip, const u8 *buf, int oob_required)
> +{
> +	int i, eccsize = chip->ecc.size;
> +	int eccsteps = chip->ecc.steps;
> +	u8 *ecc_calc = chip->buffers->ecccalc;
> +	const u8 *p = buf;
> +	u32 *eccpos = chip->ecc.layout->eccpos;
> +	unsigned long data_phase_addr = 0;
> +	unsigned long data_width = 4;
> +	u8 *oob_ptr;
> +
> +	for (; (eccsteps - 1); eccsteps--) {
> +		chip->write_buf(mtd, p, eccsize);
> +		p += eccsize;
> +	}

Why is eccsteps initialized at the declaration rather than as part of
the loop?

> +/*
> + * zynq_nand_write_page_swecc - [REPLACABLE] software ecc based page
> + * write function

Is there a reason you need both hwecc and swecc support?

More importantly, is there a reason why you need to duplicate these
functions for swecc?  I don't see any difference versus the original in
nand_base.c.

> +/*
> + * zynq_nand_select_chip - Select the flash device
> + * @mtd:	Pointer to the mtd_info structure
> + * @chip:	Chip number to be selected
> + *
> + * This function is empty as the NAND controller handles chip select line
> + * internally based on the chip address passed in command and data phase.
> + */
> +static void zynq_nand_select_chip(struct mtd_info *mtd, int chip)
> +{
> +	return;
> +}

"return;" is unnecessary

> +/*
> + * zynq_nand_cmd_function - Send command to NAND device
> + * @mtd:	Pointer to the mtd_info structure
> + * @command:	The command to be sent to the flash device
> + * @column:	The column address for this command, -1 if none
> + * @page_addr:	The page address for this command, -1 if none
> + */
> +static void zynq_nand_cmd_function(struct mtd_info *mtd, unsigned int command,
> +				 int column, int page_addr)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	const struct zynq_nand_command_format *curr_cmd = NULL;
> +	struct zynq_nand_info *xnand;
> +	void *cmd_addr;
> +	unsigned long cmd_data = 0;
> +	unsigned long cmd_phase_addr = 0;
> +	unsigned long data_phase_addr = 0;
> +	unsigned long end_cmd = 0;
> +	unsigned long end_cmd_valid = 0;
> +	unsigned long i;

Other than the addresses, why long?

> +
> +	xnand = (struct zynq_nand_info *)chip->priv;
> +	if (xnand->end_cmd_pending) {
> +		/* Check for end command if this command request is same as the
> +		 * pending command then return
> +		 */
> +		if (xnand->end_cmd == command) {
> +			xnand->end_cmd = 0;
> +			xnand->end_cmd_pending = 0;
> +			return;
> +		}
> +	}
> +
> +	/* Emulate NAND_CMD_READOOB for large page device */
> +	if ((mtd->writesize > ZYNQ_NAND_ECC_SIZE) &&
> +	    (command == NAND_CMD_READOOB)) {
> +		column += mtd->writesize;
> +		command = NAND_CMD_READ0;
> +	}
> +
> +	/* Get the command format */
> +	for (i = 0; (zynq_nand_commands[i].start_cmd != NAND_CMD_NONE ||
> +		zynq_nand_commands[i].end_cmd != NAND_CMD_NONE); i++) {
> +		if (command == zynq_nand_commands[i].start_cmd)

Please do not line up the continuation line with the loop body.

Why not use "i < ARRAY_SIZE(zynq_nand_commands)" instead, and get rid of
the sentinel?

> +			curr_cmd = &zynq_nand_commands[i];
> +	}
> +	if (curr_cmd == NULL)
> +		return;
> +
> +	/* Clear interrupt */
> +	writel((1 << 4), &zynq_nand_smc_base->cfr);

1 << 4 should be defined symbolically.

> +	} else
> +		;

Get rid of that else.

> +	writel(cmd_data, cmd_addr);
> +
> +	if (curr_cmd->end_cmd_valid) {
> +		xnand->end_cmd = curr_cmd->end_cmd;
> +		xnand->end_cmd_pending = 1;
> +	}
> +
> +	ndelay(100);
> +
> +	if ((command == NAND_CMD_READ0) ||
> +	    (command == NAND_CMD_RESET) ||
> +	    (command == NAND_CMD_PARAM) ||
> +	    (command == NAND_CMD_GET_FEATURES)) {
> +		while (!chip->dev_ready(mtd))
> +				;

Timeout?

> +		return;
> +	}
> +}

That "return;" does nothing.

> +/*
> + * zynq_nand_read_buf - read chip data into buffer
> + * @mtd:        MTD device structure
> + * @buf:        buffer to store date
> + * @len:        number of bytes to read
> + */
> +static void zynq_nand_read_buf(struct mtd_info *mtd, u8 *buf, int len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	const u32 *nand = chip->IO_ADDR_R;
> +
> +	/* Make sure that buf is 32 bit aligned */
> +	if (((int)buf & 0x3) != 0) {
> +		if (((int)buf & 0x1) != 0) {
> +			if (len) {
> +				*buf = readb(nand);
> +				buf += 1;
> +				len--;
> +			}
> +		}

Don't cast pointers to "int".  Use (unsigned) long or (u)intptr_t.

> +		if (((int)buf & 0x3) != 0) {
> +			if (len >= 2) {
> +				*(u16 *)buf = readw(nand);

I believe this violates C99 aliasing rules.

> +				buf += 2;
> +				len -= 2;
> +			}
> +		}
> +	}
> +
> +	/* copy aligned data */
> +	while (len >= 4) {
> +		*(u32 *)buf = readl(nand);

Likewise.

For why GCC doesn't complain (or at least a characterization of when it
doesn't complain), see this:
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html

That said, I'm not sure how one would do this without violating strict
aliasing (other than with inline asm), and as long as this function
never gets inlined, GCC should produce code that's OK.

> +static int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
> +{
> +	struct zynq_nand_info *xnand;
> +	struct mtd_info *mtd;
> +	unsigned long ecc_page_size;
> +	int err = -1;
> +	u8 maf_id, dev_id, i;
> +	u8 get_feature[4];
> +	u8 set_feature[4] = {0x08, 0x00, 0x00, 0x00};
> +	unsigned long ecc_cfg;
> +	int ondie_ecc_enabled = 0;

Does that 0x08 have a symbolic name?

> 	if (ondie_ecc_enabled) {
> +		/* bypass the controller ECC block */
> +		ecc_cfg = readl(&zynq_nand_smc_base->emcr);
> +		ecc_cfg &= ~0xc;
> +		writel(ecc_cfg, &zynq_nand_smc_base->emcr);
> +
> +		/* The software ECC routines won't work
> +		 * with the SMC controller
> +		 */
> +		nand_chip->ecc.mode = NAND_ECC_HW;
> +		nand_chip->ecc.strength = 1;
> +		nand_chip->ecc.read_page = zynq_nand_read_page_raw_nooob;
> +		nand_chip->ecc.read_subpage = zynq_nand_read_subpage_raw;
> +		nand_chip->ecc.write_page = zynq_nand_write_page_raw;
> +		nand_chip->ecc.read_page_raw = zynq_nand_read_page_raw;
> +		nand_chip->ecc.write_page_raw = zynq_nand_write_page_raw;
> +		nand_chip->ecc.read_oob = zynq_nand_read_oob;
> +		nand_chip->ecc.write_oob = zynq_nand_write_oob;
> +		nand_chip->ecc.size = mtd->writesize;
> +		nand_chip->ecc.bytes = 0;
> +
> +		/* NAND with on-die ECC supports subpage reads */
> +		nand_chip->options |= NAND_SUBPAGE_READ;
> +
> +		/* On-Die ECC spare bytes offset 8 is used for ECC codes */
> +		if (ondie_ecc_enabled) {
> +			nand_chip->ecc.layout = &ondie_nand_oob_64;
> +			/* Use the BBT pattern descriptors */
> +			nand_chip->bbt_td = &bbt_main_descr;
> +			nand_chip->bbt_md = &bbt_mirror_descr;
> +		}
> +	} else {
> +		/* Hardware ECC generates 3 bytes ECC code for each 512 bytes */
> +		nand_chip->ecc.mode = NAND_ECC_HW;
> +		nand_chip->ecc.strength = 1;
> +		nand_chip->ecc.size = ZYNQ_NAND_ECC_SIZE;
> +		nand_chip->ecc.bytes = 3;
> +		nand_chip->ecc.calculate = zynq_nand_calculate_hwecc;
> +		nand_chip->ecc.correct = zynq_nand_correct_data;
> +		nand_chip->ecc.hwctl = NULL;
> +		nand_chip->ecc.read_page = zynq_nand_read_page_hwecc;
> +		nand_chip->ecc.write_page = zynq_nand_write_page_hwecc;
> +		nand_chip->ecc.read_page_raw = zynq_nand_read_page_raw;
> +		nand_chip->ecc.write_page_raw = zynq_nand_write_page_raw;
> +		nand_chip->ecc.read_oob = zynq_nand_read_oob;
> +		nand_chip->ecc.write_oob = zynq_nand_write_oob;
> +
> +		switch (mtd->writesize) {
> +		case 512:
> +			ecc_page_size = 0x1;
> +			/* Set the ECC memory config register */
> +			writel((ZYNQ_NAND_ECC_CONFIG | ecc_page_size),
> +			       &zynq_nand_smc_base->emcr);
> +			break;
> +		case 1024:
> +			ecc_page_size = 0x2;
> +			/* Set the ECC memory config register */
> +			writel((ZYNQ_NAND_ECC_CONFIG | ecc_page_size),
> +			       &zynq_nand_smc_base->emcr);
> +			break;
> +		case 2048:
> +			ecc_page_size = 0x3;
> +			/* Set the ECC memory config register */
> +			writel((ZYNQ_NAND_ECC_CONFIG | ecc_page_size),
> +			       &zynq_nand_smc_base->emcr);
> +			break;
> +		default:
> +			/* The software ECC routines won't work with
> +			 * the SMC controller
> +			 */
> +			nand_chip->ecc.mode = NAND_ECC_HW;
> +			nand_chip->ecc.calculate = nand_calculate_ecc;
> +			nand_chip->ecc.correct = nand_correct_data;
> +			nand_chip->ecc.read_page = zynq_nand_read_page_swecc;
> +			/* nand_chip->ecc.read_subpage = nand_read_subpage; */
> +			nand_chip->ecc.write_page = zynq_nand_write_page_swecc;

Why do you have NAND_ECC_HW together with zynq_nand_*_page_swecc?

> +			nand_chip->ecc.read_page_raw = zynq_nand_read_page_raw;
> +			nand_chip->ecc.write_page_raw =
> +						zynq_nand_write_page_raw;
> +			nand_chip->ecc.read_oob = zynq_nand_read_oob;
> +			nand_chip->ecc.write_oob = zynq_nand_write_oob;
> +			nand_chip->ecc.size = 256;
> +			nand_chip->ecc.bytes = 3;
> +			break;
> +		}
> +
> +		if (mtd->oobsize == 16)
> +			nand_chip->ecc.layout = &nand_oob_16;
> +		else if (mtd->oobsize == 64)
> +			nand_chip->ecc.layout = &nand_oob_64;
> +		else
> +			;
> +	}

There seems to be some duplicated things above that do the same thing on
every possible code path (or even multiple times on the same codepath,
such as setting nand_chip->ecc.mode to NAND_ECC_HW).

And again, get rid of that do-nothing "else".

-Scott