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(®->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(®->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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
