[U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

[Scott Wood]

On 04/17/2012 01:50 PM, Simon Glass wrote:
> +#define DEBUG

Did you mean to leave this in?

> +/**
> + * [DEFAULT] Read one byte from the chip
> + *
> + * @param mtd	MTD device structure
> + * @return	data byte
> + *
> + * Default read function for 8bit bus-width
> + */

This isn't the default read function, it's the tegra read function.

> +static uint8_t read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	u32 dword_read;
> +	struct nand_drv *info;
> +
> +	info = (struct nand_drv *)chip->priv;
> +	if (info->pio_byte_index > 3)
> +		return 0;
> +
> +	/* We can read this data multiple times and get the same word */
> +	dword_read = readl(&info->reg->resp);
> +	dword_read = dword_read >> (8 * info->pio_byte_index);
> +	info->pio_byte_index++;
> +	return (uint8_t)dword_read;
> +}

So you only read up to 4 bytes via this method?  If this is really all
that's ever needed, please add a comment to that effect.

> +/**
> + * [DEFAULT] Write buffer to chip
> + *
> + * @param mtd	MTD device structure
> + * @param buf	data buffer
> + * @param len	number of bytes to write
> + *
> + * Default write function for 8bit bus-width
> + */
> +static void write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> +{
> +	int i, j, l;
> +	struct nand_chip *chip = mtd->priv;
> +	struct nand_drv *info;
> +
> +	info = (struct nand_drv *)chip->priv;
> +
> +	for (i = 0; i < len / 4; i++) {
> +		l = ((int *)buf)[i];

If you're assuming the buffer is 32-bit aligned, comment it.  Ideally
these assumptions should be stated in the interface itself...

Should also comment that there's an endian dependency here.

> +		writel(l, &info->reg->resp);
> +		writel(CMD_GO | CMD_PIO | CMD_TX |
> +			((4 - 1) << CMD_TRANS_SIZE_SHIFT)
> +			| CMD_A_VALID | CMD_CE0,
> +			&info->reg->command);
> +
> +		if (!nand_waitfor_cmd_completion(info->reg))
> +			printf("Command timeout during write_buf\n");

You need to wait for completion every 4 bytes?  Where is the DMA?

> +	case NAND_CMD_RNDOUT:
> +		printf("%s: Unsupported RNDOUT command\n", __func__);
> +		return;
[snip]
> +	default:
> +		printf("%s: Unsupported command %d\n", __func__, command);
> +		return;
> +	}

Doesn't the print in the default case already handle RNDOUT?

> +	if ((reg_val & DEC_STATUS_A_ECC_FAIL) && databuf) {
> +		reg_val = readl(&reg->bch_dec_status_buf);
> +		/*
> +		 * If uncorrectable error occurs on data area, then see whether
> +		 * they are all FF. If all are FF, it's a blank page.
> +		 * Not error.
> +		 */
> +		if ((reg_val & BCH_DEC_STATUS_FAIL_SEC_FLAG_MASK) &&
> +			!blank_check(databuf, a_len))
> +			return_val |= ECC_DATA_ERROR;
> +	}
> +
> +	if ((reg_val & DEC_STATUS_B_ECC_FAIL) && oobbuf) {
> +		reg_val = readl(&reg->bch_dec_status_buf);
> +		/*
> +		 * If uncorrectable error occurs on tag area, then see whether
> +		 * they are all FF. If all are FF, it's a blank page.
> +		 * Not error.
> +		 */
> +		if ((reg_val & BCH_DEC_STATUS_FAIL_TAG_MASK) &&
> +			!blank_check(oobbuf, b_len))
> +			return_val |= ECC_TAG_ERROR;

Please don't line up the continuation line with the if-body.

What is the difference between an "A" fail and a "B" fail?  Do you
really want to do the blank_check twice?

> +	/* Need to be 4-byte aligned */
> +	tag_ptr = (char *)&tag_buf;
>
> +
> +	stop_command(info->reg);
> +
> +	writel((1 << chip->page_shift) - 1, &info->reg->dma_cfg_a);
> +	writel((u32)buf, &info->reg->data_block_ptr);
> +
> +	if (with_ecc) {
> +		writel((u32)tag_ptr, &info->reg->tag_ptr);
> +		if (is_writing)
> +			memcpy(tag_ptr, chip->oob_poi + free->offset,
> +				config->tag_bytes +
> +				config->tag_ecc_bytes);
> +	} else
> +		writel((u32)chip->oob_poi, &info->reg->tag_ptr);

Should use virt_to_phys(), even if it currently makes no difference on
this platform.

> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)

Make this static.

> +/**
> + * Board-specific NAND initialization
> + *
> + * @param nand	nand chip info structure
> + * @return 0, after initialized, -1 on error
> + */
> +int board_nand_init(struct nand_chip *nand)

Please consider using CONFIG_SYS_NAND_SELF_INIT.

> diff --git a/drivers/mtd/nand/tegra2_nand.h b/drivers/mtd/nand/tegra2_nand.h
> new file mode 100644
> index 0000000..7e74be7
> --- /dev/null
> +++ b/drivers/mtd/nand/tegra2_nand.h
> @@ -0,0 +1,257 @@
> +/*
> + * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +/* register offset */
> +#define COMMAND_0		0x00
> +#define CMD_GO			(1 << 31)
> +#define CMD_CLE			(1 << 30)
> +#define CMD_ALE			(1 << 29)
> +#define CMD_PIO			(1 << 28)
> +#define CMD_TX			(1 << 27)
> +#define CMD_RX			(1 << 26)
> +#define CMD_SEC_CMD		(1 << 25)
> +#define CMD_AFT_DAT_MASK	(1 << 24)
> +#define CMD_AFT_DAT_DISABLE	0
> +#define CMD_AFT_DAT_ENABLE	(1 << 24)
> +#define CMD_TRANS_SIZE_SHIFT	20
> +#define CMD_TRANS_SIZE_PAGE	8

Please use proper namespacing on symbols defined in headers.

-Scott