On Fri, Jul 17, 2009 at 02:53:55PM +0400, Ilya Yanok 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?
> +/*
> + * 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.
> + /*
> + * 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...
> +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.
> + 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.
> +#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)...
> + 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?
> + 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?
> + /* 50 us command delay time */
> + this->chip_delay = 5;
5 or 50?
> + 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.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
