[U-Boot] [PATCH 4/7] mxc_nand: add nand driver for MX2/MX3

[Ilya Yanok]

Hi Scott,

please review the updated patch (will be posted as a follow-up).

Scott Wood wrote:
> Please look at drivers/mtd/nand/mpc5121_nfc.c, which AFAICT is very
> similar hardware, and see if anything can be factored out into common
> code, and try to keep the rest looking the same except where the hardware
> actually differs.
>   

Hmm... For now we just can't spend enough effort for this...

>> +static struct nand_ecclayout nand_hw_eccoob_16 = {
>> +	.eccbytes = 5,
>> +	.eccpos = {6, 7, 8, 9, 10},
>> +	.oobfree = {{0, 6}, {12, 4}, }
>> +};
>>     
>
> This implies the bad block marker is one byte at offset 11 (it's all
> that's left), but I don't see any override of the bad block pattern.
>   

This is surely a bug. Fixed.

>> +static void *mxc_nand_memcpy32(void *dest, void *source, size_t size)
>> +{
>> +	uint32_t *s = source, *d = dest;
>> +
>> +	size >>= 2;
>> +	while (size--)
>> +		*d++ = *s++;
>> +	return dest;
>> +}
>>     
>
> This should probably be using I/O accessors (possibly raw) -- and should
> take uint32_t *, not void *.
>   

Fixed.

>> +/*
>> + * This function requests the NANDFC to perform a read of the
>> + * NAND device status and returns the current status.
>> + */
>> +static uint16_t get_dev_status(struct mxc_nand_host *host)
>> +{
>> +	void __iomem *main_buf = host->regs->main_area1;
>> +	uint32_t store;
>> +	uint16_t ret, tmp;
>> +	/* Issue status request to NAND device */
>> +
>> +	/* store the main area1 first word, later do recovery */
>> +	store = readl(main_buf);
>> +	/*
>> +	 * NANDFC buffer 1 is used for device status to prevent
>> +	 * corruption of read/write buffer on status requests.
>> +	 */
>> +	writew(1, &host->regs->nfc_buf_addr);
>>     
>
> But it looks like buffer 1 is used for data with large page flash.
>   

Well, we save first word of the buffer and then recover it.

> Other drivers don't seem to have any problem with status reads clobbering
> the buffer...
>
>   
>> +/* This functions is used by upper layer to checks if device is ready */
>> +static int mxc_nand_dev_ready(struct mtd_info *mtd)
>>     
>
> "This functions is"?  I'd say this comment is pretty redundant with respect
> to the function name anyway...
>   

Fixed.

>> +static u_char mxc_nand_read_byte(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct mxc_nand_host *host = nand_chip->priv;
>> +	uint8_t ret = 0;
>> +	uint16_t col, rd_word;
>> +	uint16_t __iomem *main_buf =
>> +		(uint16_t __iomem *)host->regs->main_area0;
>> +	uint16_t __iomem *spare_buf =
>> +		(uint16_t __iomem *)host->regs->spare_area0;
>>     
>
> According to Magnus Lilja, "the nand flash controller can only handle 32
> bit read/write operations, any other size will cause an abort (or
> something like that)".  But now we're accessing it as 16-bit?
>   

16-bit accesses work quite well. Problem was with 8-bit accesses.

>> +static uint16_t mxc_nand_read_word(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct mxc_nand_host *host = nand_chip->priv;
>> +	uint16_t col, rd_word, ret;
>> +	uint16_t __iomem *p;
>> +
>> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
>> +	      "mxc_nand_read_word(col = %d)\n", host->col_addr);
>> +
>> +	col = host->col_addr;
>> +	/* Adjust saved column address */
>> +	if (col < mtd->writesize && host->spare_only)
>> +		col += mtd->writesize;
>> +
>> +	if (col < mtd->writesize) {
>> +		p = (uint16_t __iomem *)(host->regs->main_area0 + (col >> 1));
>> +	} else {
>> +		p = (uint16_t __iomem *)(host->regs->spare_area0 +
>> +				((col - mtd->writesize) >> 1));
>> +	}
>> +
>> +	if (col & 1) {
>> +		rd_word = readw(p);
>> +		ret = (rd_word >> 8) & 0xff;
>> +		rd_word = readw(&p[1]);
>> +		ret |= (rd_word << 8) & 0xff00;
>> +
>>     
>
> col should never be odd if you're reading words.
>   

It can be odd if previously we've read a byte.

>> +/*
>> + * Write data of length len to buffer 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
>> + */
>> +static void mxc_nand_write_buf(struct mtd_info *mtd,
>> +				const u_char *buf, int len)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct mxc_nand_host *host = nand_chip->priv;
>> +	int n, col, i = 0;
>> +
>> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
>> +	      "mxc_nand_write_buf(col = %d, len = %d)\n", host->col_addr,
>> +	      len);
>> +
>> +	col = host->col_addr;
>> +
>> +	/* Adjust saved column address */
>> +	if (col < mtd->writesize && host->spare_only)
>> +		col += mtd->writesize;
>> +
>> +	n = mtd->writesize + mtd->oobsize - col;
>> +	n = min(len, n);
>> +
>> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
>> +	      "%s:%d: col = %d, n = %d\n", __func__, __LINE__, col, n);
>> +
>> +	while (n) {
>>     
>
> Safer to do while (n > 0), especially when the code is this complex.
>   

Fixed.

>> +		void __iomem *p;
>> +
>> +		if (col < mtd->writesize) {
>> +			p = host->regs->main_area0 + (col & ~3);
>> +		} else {
>> +			p = host->regs->spare_area0 -
>> +						mtd->writesize + (col & ~3);
>> +		}
>> +
>> +		MTDDEBUG(MTD_DEBUG_LEVEL3, "%s:%d: p = %p\n", __func__,
>> +		      __LINE__, p);
>> +
>> +		if (((col | (int)&buf[i]) & 3) || n < 16) {
>>     
>
> Do not cast pointers to "int".  Use "uintptr_t" or "unsigned long" if you
> must.
>
> Why < 16 and not < 4?
>   

Fixed.

>> +			uint32_t data = 0;
>> +
>> +			if (col & 3 || n < 4)
>> +				data = readl(p);
>>     
>
> If that condition doesn't hold, the data we use is zero?
>   

If that condition doesn't hold we are going to rewrite a whole 32-bit
word so there is no need to read it's old content. But I've changed that
piece of code as you suggested anyway.

>> +
>> +			switch (col & 3) {
>> +			case 0:
>> +				if (n) {
>> +					data = (data & 0xffffff00) |
>> +					    (buf[i++] << 0);
>> +					n--;
>> +					col++;
>> +				}
>> +			case 1:
>> +				if (n) {
>> +					data = (data & 0xffff00ff) |
>> +					    (buf[i++] << 8);
>> +					n--;
>> +					col++;
>> +				}
>> +			case 2:
>> +				if (n) {
>> +					data = (data & 0xff00ffff) |
>> +					    (buf[i++] << 16);
>> +					n--;
>> +					col++;
>> +				}
>> +			case 3:
>> +				if (n) {
>> +					data = (data & 0x00ffffff) |
>> +					    (buf[i++] << 24);
>> +					n--;
>> +					col++;
>> +				}
>> +			}
>> +			writel(data, p);
>>     
>
> Might I suggest this?
>
> union {
> 	uint32_t word;
> 	uint8_t bytes[4];
> } nfc_word;
>
> nfc_word.word = readl(p);
> nfc_word.bytes[col & 3] = buf[i++];
> n--;
> col++;
>
> writel(nfc_word.word, p);
>
> As a side benefit, you lose the endian dependency.
>   

Thanks! I've used your code.

>> +			mxc_nand_memcpy32(p, (void *)&buf[i], m);
>>     
>
> Unnecessary cast.
>   

Fixed.

>> +	/* Update saved column address */
>> +	host->col_addr = col;
>> +
>> +}
>>     
>
> No blank lines before the brace at the end of a block.
>   

Fixed.

>> +
>> +/*
>> + * Used by the upper layer to verify the data in NAND Flash
>> + * with the data in the buf.
>> + */
>> +static int mxc_nand_verify_buf(struct mtd_info *mtd,
>> +				const u_char *buf, int len)
>> +{
>> +	return -EFAULT;
>> +}
>>     
>
> Umm...
>   

I've added verify_buf function.

>> +	case NAND_CMD_SEQIN:
>> +		if (column >= mtd->writesize) {
>> +			/*
>> +			 * 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.
>> +			 */
>> +			if (host->pagesize_2k) {
>> +				/* call ourself to read a page */
>> +				mxc_nand_command(mtd, NAND_CMD_READ0, 0,
>> +						page_addr);
>> +			}
>>     
>
> Should this be #ifdef HWECC?  And update the comment to indicate the
> actual problem, which is the unusual hardware ECC implementation.  I
> don't see what the lack of a "READ1" command has to do with it.
>   

I've updated the comment.

> And is this actually a FIXME if it's already being done?
>
>   
>> +#ifdef CONFIG_MXC_NAND_HWECC
>> +	this->ecc.calculate = mxc_nand_calculate_ecc;
>> +	this->ecc.hwctl = mxc_nand_enable_hwecc;
>> +	this->ecc.correct = mxc_nand_correct_data;
>> +	this->ecc.mode = NAND_ECC_HW;
>> +	this->ecc.size = 512;
>> +	this->ecc.bytes = 3;
>> +	this->ecc.layout = &nand_hw_eccoob_8;
>> +	tmp = readw(&host->regs->nfc_config1);
>> +	tmp |= NFC_ECC_EN;
>> +	writew(tmp, &host->regs->nfc_config1);
>> +#else
>> +	this->ecc.size = 512;
>> +	this->ecc.bytes = 3;
>> +	this->ecc.layout = &nand_hw_eccoob_8;
>> +	this->ecc.mode = NAND_ECC_SOFT;
>> +	tmp = readw(&host->regs->nfc_config1);
>> +	tmp &= ~NFC_ECC_EN;
>> +	writew(tmp, &host->regs->nfc_config1);
>> +#endif
>>     
>
> Soft ECC only supports 256-byte ECC blocks (anything you set here to the
> contrary will just be overwritten), and you'll need a layout that
> accommodates that with enough ECC bytes.
>
> Alternatively, you can implement 512-byte soft ECC if you don't want to
> waste those 3 extra OOB bytes. :-)
>
>   
>> +	host->pagesize_2k = 0;
>>     
>
> So large page is currently unsupported?
>   

Linux driver was fixed recently and now it claims to support 2K page
size... I've added all needed fixes but I can understand how this driver
should detect the pagesize... Linux driver calls nand_scan_ident()
itself for this... Do you think I can calls nand_scan_ident() from my
board_nand_init() function?

Regards, Ilya.