[U-Boot] [PATCH v3] Freescale NFC NAND driver

[Scott Wood]

On Tue, Nov 04, 2008 at 07:02:41PM -0700, John Rigby wrote:
> +#define MIN(x, y)		((x < y) ? x : y)

Please use the min() macro defined in include/common.h.

> +static struct fsl_nfc_private {
> +	struct mtd_info mtd;
> +	char spare_only;
> +	char status_req;
> +	u16 col_addr;
> +	int writesize;
> +	int sparesize;
> +	int width;
> +	int chipsel;
> +} *priv;

Is it plausable that there will ever be a chip with more than one of
these controllers?

> +/*
> + * OOB placement block for use with hardware ecc generation
> + */
> +static struct nand_ecclayout nand_hw_eccoob_512 = {
> +	.eccbytes = 9,
> +	.eccpos = {
> +		7, 8, 9, 10, 11, 12, 13, 14, 15,
> +	},
> +	.oobfree = {
> +		{0, 5} /* byte 5 is factory bad block marker */
> +	},
> +};

Is byte 6 free?

> +static struct nand_ecclayout nand_hw_eccoob_4k_218_spare = {
> +	.eccbytes = 64,	/* actually 144 but only room for 64 */
> +	.eccpos = {
> +		/* 18 bytes of ecc for each 512 bytes of data */
> +		7, 8, 9, 10, 11, 12, 13, 14, 15,
> +		    16, 17, 18, 19, 20, 21, 22, 23, 24,
> +		33, 34, 35, 36, 37, 38, 39, 40, 41,
> +		    42, 43, 44, 45, 46, 47, 48, 49, 50,
> +		59, 60, 61, 62, 63, 64, 65, 66, 67,
> +		    68, 69, 70, 71, 72, 73, 74, 75, 76,
> +		85, 86, 87, 88, 89, 90, 91, 92, 93,
> +		    94, /* 95, 96, 97, 98, 99, 100, 101, 102,
> +		111, 112, 113, 114, 115, 116, 117, 118, 119,
> +		    120, 121, 122, 123, 124, 125, 126, 127, 128,
> +		137, 138, 139, 140, 141, 142, 143, 144, 145,
> +		    146, 147, 148, 149, 150, 151, 152, 153, 154,
> +		163, 164, 165, 166, 167, 168, 169, 170, 171,
> +		    172, 173, 174, 175, 176, 177, 178, 179, 180,
> +		189, 190, 191, 192, 193, 194, 195, 196, 197,
> +		    198, 199, 200, 201, 202, 203, 204, 205, 206, */
> +	},
> +	.oobfree = {
> +		{2, 5}, /* bytes 0 and 1 are factory bad block markers */
> +		{25, 8},
> +		{51, 8},
> +		{77, 8},
> +		{103, 8},
> +		{129, 8},
> +		{155, 8},
> +		{181, 8},
> +	},
> +};

Need to change NAND_MAX_OOBSIZE.

> +/*!
> + * This function polls the NFC to wait for the basic operation to complete by
> + * checking the INT bit of config2 register.
> + *
> + * @max_retries	number of retry attempts (separated by 1 us)
> + */
> +static void wait_op_done(int max_retries)
> +{
> +

No blank line here.

> +	}
> +	if (max_retries <= 0)

Blank line between the last two.

> +static void fsl_nfc_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +	NFC_WRITEW(NFC_CONFIG1, (NFC_READW(NFC_CONFIG1) | NFC_ECC_EN));
> +	return;
> +}

Unnecessary "return" keyword.

> +/*!
> + * This function writes data of length \b len from buffer \b buf to the NAND
> + * internal RAM buffer's MAIN area 0.
> + *
> + * @mtd		MTD structure for the NAND Flash
> + * @buf		data to be written to NAND Flash
> + * @len		number of bytes to be written
> + */
> +static void fsl_nfc_write_buf(struct mtd_info *mtd,
> +					const u_char *buf, int len)
> +{
> +	if (priv->col_addr >= mtd->writesize || priv->spare_only) {
> +		copy_to_spare(mtd, (char *)buf, len);
> +		return;
> +	} else {
> +		priv->col_addr += len;
> +		memcpy_toio(MAIN_AREA(0), (void *)buf, len);
> +	}
> +}

What if both main and spare areas are written at once?

Also, rewrite

if (...) {
	foo;
	return;
} else {
	bar;
}

as:

if (...) {
	foo;
	return;
}

bar;

or

if (...)
	foo;
else
	bar;


> +#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC
> +static void fsl_nfc_select_chip(u8 cs)
> +{
> +	u32 val = NFC_READW(NFC_BUF_ADDR);
> +
> +	val &= ~0x60;
> +	val |= cs << 5;
> +	NFC_WRITEW(NFC_BUF_ADDR, val);
> +}
> +#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip
> +#endif
> +
> +
> +/*!
> + * This function is used by upper layer for select and deselect of the NAND
> + * chip
> + *
> + * @mtd		MTD structure for the NAND Flash
> + * @chip	val indicating select or deselect
> + */
> +static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)

This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is
not defined.

> +/*
> + * Function to read a page from nand device.
> + */
> +static void read_full_page(struct mtd_info *mtd, int page_addr)
> +{
> +	send_cmd(NAND_CMD_READ0);
> +
> +	fsl_nfc_do_addr_cycle(mtd, 0, page_addr);
> +
> +	if (IS_LARGE_PAGE_NAND) {
> +		send_cmd(NAND_CMD_READSTART);
> +		send_read_page(0);
> +	} else {
> +		send_read_page(0);
> +	}
> +}

Factor out the send_read_page(0);

> +	case NAND_CMD_READOOB:
> +		priv->col_addr = column;
> +		priv->spare_only = 1;
> +		command = NAND_CMD_READ0;	/* only READ0 is valid */
> +		break;

What about small-page flash that takes an actual READOOB command?

> +static int fsl_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> +				int page, int sndcmd)
> +{
> +	if (sndcmd) {
> +		read_full_page(mtd, page);
> +		sndcmd = 0;
> +	}
> +
> +	copy_from_spare(mtd, chip->oob_poi, mtd->oobsize);
> +	return sndcmd;
> +}
> +
> +static int fsl_nfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> +					int page)
> +{
> +	int status = 0;
> +	int read_oob_col = 0;
> +
> +	send_cmd(NAND_CMD_READ0);
> +	send_cmd(NAND_CMD_SEQIN);
> +	fsl_nfc_do_addr_cycle(mtd, read_oob_col, page);
> +
> +	/* copy the oob data */
> +	copy_to_spare(mtd, chip->oob_poi, mtd->oobsize);
> +
> +	send_prog_page(0);
> +
> +	send_cmd(NAND_CMD_PAGEPROG);
> +
> +	status = fsl_nfc_wait(mtd, chip);
> +	if (status & NAND_STATUS_FAIL)
> +		return -1;
> +	return 0;
> +}

Again, I'm fairly sure you don't need these if the other functions are
defined properly.

> +/*
> + * These are identical to the generic versions except
> + * for the offsets.
> + */
> +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 = 0,
> +	.len = 4,
> +	.veroffs = 4,
> +	.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 = 0,
> +	.len = 4,
> +	.veroffs = 4,
> +	.maxblocks = 4,
> +	.pattern = mirror_pattern
> +};

This will overlap the bad block marker on large-page flash.

-Scott