[U-Boot] [PATCH v2 3/6] i.MX31: Add i.MX31 NAND Flash Controller driver.

Scott Wood scottwood at freescale.com
Mon Aug 18 23:02:58 CEST 2008


On Mon, Aug 18, 2008 at 11:30:44AM +0200, Magnus Lilja wrote:
> +/* The bool type is used locally in this file, added for U-boot. */
> +typedef enum {false = 0, true = 1 } bool;

Please remove this.

> +struct nand_info {
> +	bool bSpareOnly;
> +	bool bStatusRequest;

No Hungarian notation.  noJavaCaps.

> +static struct nand_ecclayout nand_hw_eccoob_2k = {
> +	.eccbytes = 20,
> +	.eccpos = {6, 7, 8, 9, 10, 22, 23, 24, 25, 26,
> +		   38, 39, 40, 41, 42, 54, 55, 56, 57, 58},
> +	.oobfree = {
> +		{.offset = 0,
> +		 .length = 5},

Bytes 0 and 1 are not free (they're the bad block marker).  Byte 5 *is*
free.

> +		{.offset = 11,
> +		 .length = 10},

Length should be 11.

> +/* Define some generic bad / good block scan pattern which are used
> + * while scanning a device for factory marked good / bad blocks. */
> +static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
> +
> +static struct nand_bbt_descr smallpage_memorybased = {
> +	.options = NAND_BBT_SCAN2NDPAGE,
> +	.offs = 5,
> +	.len = 1,
> +	.pattern = scan_ff_pattern
> +};
> +
> +static struct nand_bbt_descr largepage_memorybased = {
> +	.options = 0,
> +	.offs = 0,
> +	.len = 2,
> +	.pattern = scan_ff_pattern
> +};
> +
> +/* Generic flash bbt decriptors */
> +static uint8_t bbt_pattern[] = { 'B', 'b', 't', '0' };
> +static uint8_t 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 = 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
> +};

Is there any reason the default layout can't be used?

> +/**
> + * This function will maintains state of single bit Error
> + * in Main & spare  area
> + *
> + * @param buf_id	Specify Internal RAM Buffer number (0-3)
> + * @param spare  	set to true if only spare area needs correction
> + */
> +static void mxc_nd_correct_ecc(u8 buf_id, bool spare)
> +{
> +#ifdef CONFIG_MTD_NAND_MXC_ECC_CORRECTION_OPTION2
> +	/* To maintain single bit error in previous page */
> +	static int lastErrMain, lastErrSpare;
> +#endif
> +	u16 value, ecc_status;
> +
> +	/* Read the ECC result */
> +	ecc_status = NFC_ECC_STATUS_RESULT;
> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
> +		 "mxc_nd_correct_ecc (Ecc status=%x)\n", ecc_status);
> +
> +#ifdef CONFIG_MTD_NAND_MXC_ECC_CORRECTION_OPTION2

What is this ifdef for?  Please document.

> +/**
> + * This function requests the NANDFC to perform a read of the
> + * NAND device status and returns the current status.
> + *
> + * @return  device status
> + */
> +static u16 get_dev_status(void)
> +{
> +	volatile u16 *mainBuf = MAIN_AREA1;
> +	u32 store;
> +	u16 ret;
> +	/* Issue status request to NAND device */
> +
> +	/* store the main area1 first word, later do recovery */
> +	store = *((u32 *) mainBuf);
> +	/*
> +	 * NANDFC buffer 1 is used for device status to prevent
> +	 * corruption of read/write buffer on status requests.
> +	 */
> +	NFC_BUF_ADDR = 1;
> +
> +	/* Read status into main buffer */
> +	NFC_CONFIG1 &= ~NFC_SP_EN;
> +	NFC_CONFIG2 = NFC_STATUS;
> +
> +	/* Wait for operation to complete */
> +	wait_op_done(TROP_US_DELAY, 0, true);
> +
> +	/* Status is placed in first word of main buffer */
> +	/* get status, then recovery area 1 data */
> +	ret = mainBuf[0];
> +	*((u32 *) mainBuf) = store;

This cast violates C strict aliasing rules.  Use a union if you
absolutely must use a 32-bit access here.

> +static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> +				  u_char *ecc_code)
> +{
> +	/*
> +	 * Just return success.  HW ECC does not read/write the NFC spare
> +	 * buffer.  Only the FLASH spare area contains the calcuated ECC.
> +	 */
> +	return 0;
> +}

Hmm, maybe you should implement write_page() instead.

You may want to do a read-back after write to fill in oob_poi.

> +/**
> + * This function reads byte from the NAND Flash
> + *
> + * @param       mtd     MTD structure for the NAND Flash
> + *
> + * @return    data read from the NAND Flash
> + */
> +static u_char mxc_nand_read_byte(struct mtd_info *mtd)
> +{
> +	u_char retVal = 0;
> +	u16 col, rdWord;
> +	volatile u16 *mainBuf = MAIN_AREA0;
> +	volatile u16 *spareBuf = SPARE_AREA0;
> +
> +	/* Check for status request */
> +	if (g_nandfc_info.bStatusRequest)
> +		return get_dev_status() & 0xFF;
> +
> +	/* Get column for 16-bit access */
> +	col = g_nandfc_info.colAddr >> 1;
> +
> +	/* If we are accessing the spare region */
> +	if (g_nandfc_info.bSpareOnly)
> +		rdWord = spareBuf[col];
> +	else
> +		rdWord = mainBuf[col];

I thought you could only do 32-bit accesses?

> +/**
> + * This function writes data of length \b len to buffer \b buf. The data
> + * to be written on NAND Flash is first copied to RAMbuffer. After the
> + * Data Input Operation by the NFC, the data is written to NAND Flash.
> + *
> + * @param       mtd     MTD structure for the NAND Flash
> + * @param       buf     data to be written to NAND Flash
> + * @param       len     number of bytes to be written
> + */
> +static void mxc_nand_write_buf(struct mtd_info *mtd,
> +			       const u_char *buf, int len)
> +{
> +	int n;
> +	int col;
> +	int i = 0;
> +
> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
> +		 "mxc_nand_write_buf(col = %d, len = %d)\n",
> +		 g_nandfc_info.colAddr, len);
> +
> +	col = g_nandfc_info.colAddr;
> +
> +	/* Adjust saved column address */
> +	if (col < mtd->writesize && g_nandfc_info.bSpareOnly)
> +		col += mtd->writesize;
> +
> +	n = mtd->writesize + mtd->oobsize - col;
> +	n = min(len, n);

If len exceeds mtd->writesize + mtd->oobsize - col, then print an error,
don't silently clip it.

> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
> +		 "%s:%d: col = %d, n = %d\n", __FUNCTION__, __LINE__, col, n);
> +
> +	while (n) {
> +		volatile u32 *p;
> +		if (col < mtd->writesize)
> +			p = (volatile u32 *)((ulong) (MAIN_AREA0) + (col & ~3));
> +		else
> +			p = (volatile u32 *)((ulong) (SPARE_AREA0) -
> +					     mtd->writesize + (col & ~3));
> +
> +		MTDDEBUG(MTD_DEBUG_LEVEL3, "%s:%d: p = %p\n",
> +			 __FUNCTION__, __LINE__, p);
> +
> +		if (((col | (int)&buf[i]) & 3) || n < 16) {

Don't cast pointers to "int"; use uintptr_t if you must cast to an
integer type.

> +			u32 data = 0;
> +
> +			if (col & 3 || n < 4)
> +				data = *p;
> +
> +			switch (col & 3) {
> +			case 0:
> +				if (n) {
> +					data = (data & 0xffffff00) |
> +					    (buf[i++] << 0);
> +					n--;
> +					col++;
> +				}

If this controller really insists on 32-bit accesses to the buffer
(yuck), and the requested access isn't aligned, then memcpy_32 the hw
buffer to a sw buffer, and do an ordinary memcpy from that to the
requested location.

For full page accesses, the buffer should be aligned, so the only likely
double-copy is for small OOB accesses, where the double copy doesn't
matter much.

Likewise in read_buf().

> +/**
> + * This function is used by the upper layer to verify the data in NAND Flash
> + * with the data in the \b buf.
> + *
> + * @param       mtd     MTD structure for the NAND Flash
> + * @param       buf     data to be verified
> + * @param       len     length of the data to be verified
> + *
> + * @return      -EFAULT if error else 0
> + */
> +static int
> +mxc_nand_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
> +	return -1; /* Was -EFAULT */
> +}

Is there any particular reason you don't implement this?

> +/**
> + * This function is used by upper layer for select and deselect of the NAND
> + * chip.
> + *
> + * @param       mtd     MTD structure for the NAND Flash
> + * @param       chip    val indicating select or deselect
> + */
> +static void mxc_nand_select_chip(struct mtd_info *mtd, int chip)
> +{
> +#ifdef CONFIG_MTD_NAND_MXC_FORCE_CE

What does this option do, and why is it an option?

> +/**
> + * This function is used by the upper layer to write command to NAND Flash
> + * for different operations to be carried out on NAND Flash
> + *
> + * @param       mtd             MTD structure for the NAND Flash
> + * @param       command         command for NAND Flash
> + * @param       column          column offset for the page read
> + * @param       page_addr       page to be read from NAND Flash
> + */
> +static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
> +			     int column, int page_addr)
> +{
> +	bool useirq = false;

This is never set to anything but false.

> +	case NAND_CMD_SEQIN:
> +		if (column >= mtd->writesize) {
> +			if (is2k_Pagesize) {
> +				/*
> +				 * FIXME: before send SEQIN command for
> +				 * write OOB, we must read one page out.
> +				 * For K9F1GXX has no READ1 command to set
> +				 * current HW pointer to spare area, we must
> +				 * write the whole page including OOB
> +				 * together.

NACK.  Large page devices have a 2-byte column address; use that with
READ0 to select the OOB.

> +	/*
> +	 * Write out column address, if necessary
> +	 */
> +	if (column != -1) {
> +		/*
> +		 * MXC NANDFC can only perform full page+spare or
> +		 * spare-only read/write.  When the upper layers
> +		 * layers perform a read/write buf operation,
> +		 * we will used the saved column adress to index into
> +		 * the full page.
> +		 */
> +		send_addr(0, page_addr == -1);
> +		if (is2k_Pagesize)
> +			/* another col addr cycle for 2k page */
> +			send_addr(0, false);

In the spare-only case, the second address byte should be
"mtd->writesize >> 8" (or ">> 9" for 16-bit devices).

> +#ifdef CONFIG_MXC_NAND_LOW_LEVEL_ERASE
> +static void mxc_low_erase(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd->priv;
> +	unsigned int page_addr, addr;
> +	u_char status;
> +
> +	MTDDEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : mxc_low_erase:Erasing NAND\n");
> +	for (addr = 0; addr < this->chipsize; addr += mtd->erasesize) {
> +		page_addr = addr / mtd->writesize;
> +		mxc_nand_command(mtd, NAND_CMD_ERASE1, -1, page_addr);
> +		mxc_nand_command(mtd, NAND_CMD_ERASE2, -1, -1);
> +		mxc_nand_command(mtd, NAND_CMD_STATUS, -1, -1);
> +		status = mxc_nand_read_byte(mtd);
> +		if (status & NAND_STATUS_FAIL) {
> +			printk(KERN_ERR
> +			       "ERASE FAILED(block = %d,status = 0x%x)\n",
> +			       addr / mtd->erasesize, status);
> +		}
> +	}
> +
> +}
> +#endif

What is this for?  Where is it called?

> +	memset((char *)&g_nandfc_info, 0, sizeof(g_nandfc_info));

Unnecessary cast -- and unnecessary memset.

> +	this = nand;

Why not just refer to it as "nand", or rename the parameter "this"?

> +	mtd = &mxc_nand_data->mtd;
> +	mtd->priv = this;
> +	this->priv = mxc_nand_data;
> +
> +	/* 50 us command delay time */
> +	this->chip_delay = 5;

I don't think you need chip_delay if you implement cmdfunc and dev_ready.

-Scott



More information about the U-Boot mailing list