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

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