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

[Scott Wood]

On 01/13/2012 05:10 PM, Simon Glass wrote:
> +/* Information about an attached NAND chip */
> +struct fdt_nand {
> +	struct nand_ctlr *reg;
> +	int enabled;		/* 1 to enable, 0 to disable */
> +	struct fdt_gpio_state wp_gpio;	/* write-protect GPIO */
> +	int width;		/* bit width, normally 8 */
> +	int tag_ecc_bytes;	/* ECC bytes to be generated for tag bytes */
> +	int tag_bytes;		/* Tag bytes in spare area */
> +	int data_ecc_bytes;	/* ECC bytes for data area */
> +	int skipped_spare_bytes;
> +	/*
> +	 * How many bytes in spare area:
> +	 * spare area = skipped bytes + ECC bytes of data area
> +	 * + tag bytes + ECC bytes of tag bytes
> +	 */
> +	int page_spare_bytes;
> +	int page_data_bytes;	/* Bytes in data area */
> +	unsigned timing[FDT_NAND_TIMING_COUNT];

s/unsigned/u32/g

Likewise, any of these other fields that correspond to device tree
fields should be u32 or s32.

...and even when you do mean "unsigned int", please spell it out.

> +struct nand_info {

Please do not define "struct nand_info" when there is already a
different "nand_info_t".

> +struct nand_info nand_ctrl;

static (or better, dynamic).

> +/**
> + * Wait for command completion
> + *
> + * @param reg	nand_ctlr structure
> + * @return
> + *	1 - Command completed
> + *	0 - Timeout
> + */
> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
> +{
> +	int i;
> +	u32 reg_val;
> +
> +	for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
> +		if ((readl(&reg->command) & CMD_GO) ||
> +			!(readl(&reg->status) &
> +				STATUS_RBSY0) ||
> +			!(readl(&reg->isr) &
> +				ISR_IS_CMD_DONE)) {
> +			udelay(1);
> +			continue;
> +		}
> +		reg_val = readl(&reg->dma_mst_ctrl);
> +		/*
> +		 * If DMA_MST_CTRL_EN_A_ENABLE or
> +		 * DMA_MST_CTRL_EN_B_ENABLE is set,
> +		 * that means DMA engine is running, then we
> +		 * have to wait until
> +		 * DMA_MST_CTRL_IS_DMA_DONE
> +		 * is cleared for DMA transfer completion.
> +		 */
> +		if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
> +			DMA_MST_CTRL_EN_B_ENABLE)) {
> +			if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
> +				return 1;

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

E.g. either line up DMA_MST_CTRL_EN_B_ENABLE with
DMA_MST_CTRL_EN_A_ENABLE (my preference), or just add another tab (if
you want to be a tabs-only purist).

> +		} else
> +			return 1;

If braces are used on one side of the if/else, use on both sides.

> +		udelay(1);
> +	}
> +	return 0;
> +}
> +
> +/**
> + * [DEFAULT] Read one byte from the chip
> + *
> + * @param mtd	MTD device structure
> + * @return	data byte
> + *
> + * Default read function for 8bit bus-width
> + */
> +static uint8_t read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	int dword_read;

s/int/u32/

> +	struct nand_info *info;
> +
> +	info = (struct nand_info *) chip->priv;
> +
> +	dword_read = readl(&info->reg->resp);
> +	dword_read = dword_read >> (8 * info->pio_byte_index);
> +	info->pio_byte_index++;
> +	return (uint8_t) dword_read;

No space after casts.

What happens when pio_byte_index > 3?  Please don't assume that upper
bits will be ignored, even if that happens to be true on this chip.

How does info->reg->resp work?  You don't seem to be choosing the dword
to read based on pio_byte_index, which suggests that the act of reading
resp changes what will be read the next time.  But, you read it on each
byte, which would advance resp four times too fast if it's simply
returning a new dword each time.

If the hardware is really auto-advancing resp only on every fourth
access, that needs a comment.

> +/* Hardware specific access to control-lines */
> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
> +	unsigned int ctrl)
> +{
> +}

Don't implement this at all if it doesn't make sense for this hardware.

> +
> +/**
> + * Clear all interrupt status bits
> + *
> + * @param reg	nand_ctlr structure
> + */
> +static void nand_clear_interrupt_status(struct nand_ctlr *reg)
> +{
> +	u32 reg_val;
> +
> +	/* Clear interrupt status */
> +	reg_val = readl(&reg->isr);
> +	writel(reg_val, &reg->isr);
> +}
> +
> +/**
> + * [DEFAULT] Send command to NAND device
> + *
> + * @param mtd		MTD device structure
> + * @param command	the command to be sent
> + * @param column	the column address for this command, -1 if none
> + * @param page_addr	the page address for this command, -1 if none
> + */
> +static void nand_command(struct mtd_info *mtd, unsigned int command,
> +	int column, int page_addr)
> +{
> +	register struct nand_chip *chip = mtd->priv;

Are you really seeing any difference by using the register keyword?

> +	struct nand_info *info;
> +
> +	info = (struct nand_info *) chip->priv;
> +
> +	/*
> +	 * Write out the command to the device.
> +	 */
> +	if (mtd->writesize < 2048) {
> +		/*
> +		 * Only command NAND_CMD_RESET or NAND_CMD_READID will come
> +		 * here before mtd->writesize is initialized, we don't have
> +		 * any action here because page size of NAND HY27UF084G2B
> +		 * is 2048 bytes and mtd->writesize will be 2048 after
> +		 * initialized.
> +		 */

Why are you assuming a particular NAND chip here?

If you want to not support certain page sizes in this driver, fine, but
document that somewhere more prominent, and I don't see why this test is
needed.

> +	} else {
> +		/* Emulate NAND_CMD_READOOB */
> +		if (command == NAND_CMD_READOOB) {
> +			column += mtd->writesize;
> +			command = NAND_CMD_READ0;
> +		}
> +
> +		/* Adjust columns for 16 bit bus-width */
> +		if (column != -1 && (chip->options & NAND_BUSWIDTH_16))
> +			column >>= 1;
> +	}

Why does the treatment of column for NAND_CMD_READID depend on whether
you've yet filled in mtd->writesize?

> +	nand_clear_interrupt_status(info->reg);
> +
> +	/* Stop DMA engine, clear DMA completion status */
> +	writel(DMA_MST_CTRL_EN_A_DISABLE
> +		| DMA_MST_CTRL_EN_B_DISABLE
> +		| DMA_MST_CTRL_IS_DMA_DONE,
> +		&info->reg->dma_mst_ctrl);
> +
> +	/*
> +	 * Program and erase have their own busy handlers
> +	 * status and sequential in needs no delay
> +	 */
> +	switch (command) {
> +	case NAND_CMD_READID:
> +		writel(NAND_CMD_READID, &info->reg->cmd_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_ALE | CMD_PIO
> +			| CMD_RX |
> +			(CMD_TRANS_SIZE_BYTES4 << CMD_TRANS_SIZE_SHIFT)
> +			| CMD_CE0,
> +			&info->reg->command);
> +		info->pio_byte_index = 0;
> +		break;

Looks like you don't use column at all for READID -- no ONFI support, I
guess.

> +	case NAND_CMD_READ0:
> +		writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
> +		writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
> +		writel((page_addr << 16) | (column & 0xFFFF),
> +			&info->reg->addr_reg1);
> +		writel(page_addr >> 16, &info->reg->addr_reg2);
> +		return;
> +	case NAND_CMD_SEQIN:
> +		writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
> +		writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
> +		writel((page_addr << 16) | (column & 0xFFFF),
> +			&info->reg->addr_reg1);
> +		writel(page_addr >> 16,
> +			&info->reg->addr_reg2);
> +		return;
> +	case NAND_CMD_PAGEPROG:
> +		return;
> +	case NAND_CMD_ERASE1:
> +		writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
> +		writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
> +		writel(page_addr, &info->reg->addr_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_ALE |
> +			CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
> +			&info->reg->command);
> +		break;
> +	case NAND_CMD_RNDOUT:
> +		return;

Don't silently ignore RNDOUT -- if you don't support it and it happens,
complain loudly.

> +	case NAND_CMD_ERASE2:
> +		return;
> +	case NAND_CMD_STATUS:
> +		writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
> +			| (CMD_TRANS_SIZE_BYTES1 <<
> +				CMD_TRANS_SIZE_SHIFT)
> +			| CMD_CE0,
> +			&info->reg->command);
> +		info->pio_byte_index = 0;
> +		break;
> +	case NAND_CMD_RESET:
> +		writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_CE0,
> +			&info->reg->command);
> +		break;
> +	default:
> +		return;

Likewise, complain if you get an unrecognized command.

> +/**
> + * Set up NAND bus width and page size
> + *
> + * @param info		nand_info structure
> + * @param *reg_val	address of reg_val
> + * @return none - value is set in reg_val
> + */
> +static void set_bus_width_page_size(struct fdt_nand *config,
> +	u32 *reg_val)
> +{
> +	if (config->width == 8)
> +		*reg_val = CFG_BUS_WIDTH_8BIT;
> +	else
> +		*reg_val = CFG_BUS_WIDTH_16BIT;
> +
> +	if (config->page_data_bytes == 256)
> +		*reg_val |= CFG_PAGE_SIZE_256;
> +	else if (config->page_data_bytes == 512)
> +		*reg_val |= CFG_PAGE_SIZE_512;
> +	else if (config->page_data_bytes == 1024)
> +		*reg_val |= CFG_PAGE_SIZE_1024;
> +	else if (config->page_data_bytes == 2048)
> +		*reg_val |= CFG_PAGE_SIZE_2048;

Really, page sizes of 256 and 1024 bytes?