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

[Ilya Yanok]

Hi Scott,

thanks for the review.

Scott Wood wrote:
>> +/* OOB placement block for use with hardware ecc generation */
>> +static struct nand_ecclayout nand_hw_eccoob = {
>> +	.eccbytes = 5,
>> +	.eccpos = {6, 7, 8, 9, 10},
>> +	.oobfree = {{0, 5}, {11, 5}, }
>> +};
>> +
>> +#ifndef CONFIG_MXC_NAND_HWECC
>> +static struct nand_ecclayout nand_hw_eccoob_soft = {
>> +	.eccbytes = 6,
>> +	.eccpos = {6, 7, 8, 9, 10, 11},
>> +	.oobfree = {{0, 5}, {12, 4}, }
>> +};
>> +#endif
>>     
>
> Why is one struct #ifndef, but the other isn't #ifdef?
>   

Fixed.

>> +/*
>> + * This function requests the NANDFC to initate the transfer
>> + * of data currently in the NANDFC RAM buffer to the NAND device.
>> + */
>> +static void send_prog_page(struct mxc_nand_host *host, uint8_t buf_id,
>> +			int spare_only)
>> +{
>> +	MTDDEBUG(MTD_DEBUG_LEVEL3, "send_prog_page (%d)\n", spare_only);
>> +
>> +	/* NANDFC buffer 0 is used for page read/write */
>> +	writew(buf_id, &host->regs->nfc_buf_addr);
>>     
>
> Comment does not match code.
>   

Comment removed.

>> +	/*
>> +	 * 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);
>>     
>
> From discussion on the previous patch:
>   
>>> 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.
>>     
>
> So then there's no longer any special reason to use buffer 1 for status,
> and that comment is misleading...
>   

Misleading comment removed.

>> +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;
>> +
>> +	/* Check for status request */
>> +	if (host->status_request)
>> +		return get_dev_status(host) & 0xFF;
>> +
>> +	/* Get column for 16-bit access */
>> +	col = host->col_addr >> 1;
>> +
>> +	/* If we are accessing the spare region */
>> +	if (host->spare_only)
>> +		rd_word = readw(&spare_buf[col]);
>> +	else
>> +		rd_word = readw(&main_buf[col]);
>> +
>> +	/* Pick upper/lower byte of word from RAM buffer */
>> +	if (host->col_addr & 0x1)
>> +		ret = (rd_word >> 8) & 0xFF;
>> +	else
>> +		ret = rd_word & 0xFF;
>>     
>
> Use a union, as in read_buf().  Otherwise, this breaks on big-endian --
> you may not care, but it's better to not have such dependencies lurking
> in the code that could be reused who-knows-where.
>   

Ok. Fixed.

>> +	if (col & 1) {
>> +		rd_word = readw(p);
>> +		ret = (rd_word >> 8) & 0xff;
>> +		rd_word = readw(&p[1]);
>> +		ret |= (rd_word << 8) & 0xff00;
>>     
>
> Again, this is unnecessary, but if you insist, use a union.
>   

I understood that read_byte()/read_word() never mix up in existing code
but the API doesn't completely exclude such possibility so I'd like this
case to be treated correctly. I've updated the code to use union but if
you insist I'll remove this if clause completely.

>> +#ifdef CONFIG_MTD_NAND_MXC_FORCE_CE
>> +	if (chip > 0) {
>> +		MTDDEBUG(MTD_DEBUG_LEVEL0,
>> +		      "ERROR:  Illegal chip select (chip = %d)\n", chip);
>>     
>
> If it's an error, that's not exactly debug (especially since there's no
> way to propagate the error upwards)...
>   

Changed to printf().

>> +		return;
>> +	}
>> +
>> +	if (chip == -1) {
>> +		writew(readw(&host->regs->nfc_config1) & ~NFC_CE,
>> +				&host->regs->nfc_config1);
>> +		return;
>> +	}
>> +
>> +	writew(readw(&host->regs->nfc_config1) | NFC_CE,
>> +			&host->regs->nfc_config1);
>> +#endif
>>     
>
> #else?
>   

For me it just works (I've never defined CONFIG_MTD_NAND_MXC_FORCE_CE).

>> +		if (column >= mtd->writesize) {
>> +			/*
>> +			 * before sending SEQIN command for partial write,
>> +			 * we need read one page out. FSL NFC does not support
>> +			 * partial write. It alway send out 512+ecc+512+ecc ...
>> +			 * for large page nand flash. But for small page nand
>> +			 * flash, it does support SPARE ONLY operation.
>> +			 */
>> +			if (host->pagesize_2k) {
>> +				/* call ourself to read a page */
>> +				mxc_nand_command(mtd, NAND_CMD_READ0, 0,
>> +						page_addr);
>> +			}
>>     
>
> #ifdef CONFIG_MXC_NAND_HWECC?
>   

Uh... I have to admit I don't really have clear understanding of this
problem and I can't find it's description in i.MX27 Reference Manual...
Maybe you can point me in some direction? For now I'm not sure if
putting this under #ifdef won't break something...

>> +	/* 50 us command delay time */
>> +	this->chip_delay = 5;
>>     
>
> 5 or 50?
>   

Comment fixed.

>> +	host->pagesize_2k = 0;
>>     
>
> Make this board-configurable.  Has large page been tested?  If not,
> perhaps it should stay out for now, given how weird it is on this
> controller.
>   

I've never tested it with large page devices... So I'd vote to leave
large-page support disabled.

Regards, Ilya.