[U-Boot] [PATCH v3 1/7] nand: Add zynq nand controller driver support

[Scott Wood]

On Mon, 2014-02-17 at 17:56 +0530, Siva Durga Prasad Paladugu wrote:
> +/* Generic flash bbt decriptors */
> +static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
> +static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
> +		NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> +	.offs = 4,
> +	.len = 4,
> +	.veroffs = 20,
> +	.maxblocks = 4,
> +	.pattern = bbt_pattern
> +};
> +
> +static struct nand_bbt_descr bbt_mirror_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
> +		NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> +	.offs = 4,
> +	.len = 4,
> +	.veroffs = 20,
> +	.maxblocks = 4,
> +	.pattern = mirror_pattern
> +};

If this BBT description is only for chips with on-die ECC, chips with
64-byte OOB, etc., say so in a comment -- and don't label them "Generic
flash bbt descriptors". :-)

> +/*
> + * 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 one bit is set.
> + *
> + * FIXME: Try to move this in common.h
> + */
> +static bool onehot(unsigned short value)
> +{
> +	bool onehot;
> +
> +	onehot = value && !(value & (value - 1));
> +	return onehot;
> +}
> +
> +/*
> + * 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 */
> +
> +	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;
> +	}
> +
> +	if (onehot(ecc_odd | ecc_even) == 1)

Why are you comparing a bool to an integer?

> +	} else if (page_addr != -1) /* Erase */
> +		cmd_data = page_addr;
> +	else if (column != -1) { /* Change read/write column, read id etc */
> +		/* Adjust columns for 16 bit bus width */

Please use braces here.

> +		if ((chip->options & NAND_BUSWIDTH_16) &&
> +		    ((command == NAND_CMD_READ0) ||
> +		     (command == NAND_CMD_SEQIN) ||
> +		     (command == NAND_CMD_RNDOUT) ||
> +		     (command == NAND_CMD_RNDIN)))
> +			column >>= 1;
> +		cmd_data = column;
> +	}
> +
> +	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))
> +		/* wait until command is processed */
> +		nand_wait_ready(mtd);
> +
> +}

Don't put newlines before an ending brace.

> +
> +/*
> + * 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;
> +
> +	/* Make sure that buf is 32 bit aligned */
> +	if (((int)buf & 0x3) != 0) {
> +		if (((int)buf & 0x1) != 0) {
> +			if (len) {
> +				*buf = readb(chip->IO_ADDR_R);
> +				buf += 1;
> +				len--;
> +			}
> +		}
> +
> +		if (((int)buf & 0x3) != 0) {
> +			if (len >= 2) {
> +				*(u16 *)buf = readw(chip->IO_ADDR_R);
> +				buf += 2;
> +				len -= 2;
> +			}
> +		}
> +	}

Cast pointers to "unsigned long" or "uintptr_t", not "int".

Doesn't the casting of buf violate C99 aliasing rules?  You can access
arbitrary pointers as char pointers, but not the other way around.

> +	xnand = calloc(1, sizeof(struct zynq_nand_info));
> +	if (!xnand) {
> +		printf("%s: failed to allocate\n", __func__);
> +		goto free;
> +	}
> +
> +	xnand->nand_base = (void *)ZYNQ_NAND_BASEADDR;

This is a bit confusing -- ZYNQ_NAND_BASEADDR looks like an I/O
address, but you don't have __iomem and aren't using I/O accessors, and
the contents appear to be normal software data.

> +	if ((maf_id == 0x2c) && ((dev_id == 0xf1) ||
> +				 (dev_id == 0xa1) || (dev_id == 0xb1) ||
> +				 (dev_id == 0xaa) || (dev_id == 0xba) ||
> +				 (dev_id == 0xda) || (dev_id == 0xca) ||
> +				 (dev_id == 0xac) || (dev_id == 0xbc) ||
> +				 (dev_id == 0xdc) || (dev_id == 0xcc) ||
> +				 (dev_id == 0xa3) || (dev_id == 0xb3) ||
> +				 (dev_id == 0xd3) || (dev_id == 0xc3))) {
> +		nand_chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES,
> +						ONDIE_ECC_FEATURE_ADDR, -1);
> +		for (i = 0; i < 4; i++)
> +			writeb(set_feature[i], nand_chip->IO_ADDR_W);

I wonder how much of this on-die ECC stuff belongs in the common code rathen than a controller driver.

> +		/* Wait for 1us after writing data with SET_FEATURES command */
> +		ndelay(1000);
> +
> +		nand_chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES,
> +						ONDIE_ECC_FEATURE_ADDR, -1);
> +		nand_chip->read_buf(mtd, get_feature, 4);
> +
> +		if (get_feature[0] & ONDIE_ECC_FEATURE_ENABLE) {
> +			debug("%s: OnDie ECC flash\n", __func__);
> +			ondie_ecc_enabled = 1;
> +		} else {
> +			printf("%s: Unable to detect OnDie ECC\n", __func__);

Why does the lack of on-die ECC warrant a message?

> +	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);

What is 0xc?  Don't use magic numbers.

> +		/* 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;
> +		}

On-die ECC implies 64 bytes of OOB?

> +	} 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_SOFT;
> +			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.write_page = zynq_nand_write_page_swecc;
> +			nand_chip->ecc.size = 256;

What's going on here?  Are you using soft ECC or not?

> +			break;
> +		}
> +
> +		if (mtd->oobsize == 16)
> +			nand_chip->ecc.layout = &nand_oob_16;
> +		else if (mtd->oobsize == 64)
> +			nand_chip->ecc.layout = &nand_oob_64;
> +	}

What if oobsize > 64?

> +
> +	/* Second phase scan */
> +	if (nand_scan_tail(mtd)) {
> +		printf("%s: nand_scan_tailfailed\n", __func__);

s/tailfailed/tail failed/

-Scott