Scott, thanks for your feedback. I can easily fix most of the issues. The one question I have is if this can go in only supporting 5121 rev2. If I need to add rev1 support it will take more time than I have right now.
Thanks John > Scott Wood wrote: > John Rigby wrote: > >> ADS5121 rev4 / MPC5121e rev2 only >> > > What is unique about that revision that it cannot share a driver? They could but it would be ugly. > It > certainly shouldn't be board-specific... > I agree, the config belongs in the board config file and the chip select belongs in a board file. > >> This controller treats 2K pages as 4 512 byte pages >> and the hw ecc is over the combined 512 byte main >> area and the first 7 bytes of the spare area. >> >> The hw ecc is stored in the last 9 bytes of the >> spare area. >> >> This all means the the spare area can not be written >> separately from the main. This means unmodified JFFS2 >> will not work. >> > > Sigh... Smack the hardware people for me. > > >> Signed-off-by: John Rigby <[EMAIL PROTECTED]> >> --- >> board/ads5121/ads5121.c | 1 + >> drivers/mtd/nand/Makefile | 1 + >> drivers/mtd/nand/mpc5121rev2_nand.c | 1122 >> +++++++++++++++++++++++++++++++++++ >> > > This filename is very specific, especially since the i.MX NAND controller > looks very similar. How about fsl_nfc_nand.c? > Ok. > >> diff --git a/drivers/mtd/nand/mpc5121rev2_nand.c >> b/drivers/mtd/nand/mpc5121rev2_nand.c >> new file mode 100644 >> index 0000000..88555ab >> --- /dev/null >> +++ b/drivers/mtd/nand/mpc5121rev2_nand.c >> @@ -0,0 +1,1122 @@ >> +/* >> + * Copyright 2004-2008 Freescale Semiconductor, Inc. All Rights Reserved. >> + * >> + * Based on drivers/mtd/nand/mpc5121_nand.c >> + * which was forked from drivers/mtd/nand/mxc_nd.c >> > > Forking's bad, mmkay? > Yes, I'm guilty. The original was pretty ugly. > >> +/* >> + * OOB placement block for use with hardware ecc generation >> + */ >> +static struct nand_ecclayout nand_hw_eccoob_512 = { >> + .eccbytes = 9, >> + .eccpos = { >> + 7, 8, 9, 10, 11, 12, 13, 14, 15, >> + }, >> + .oobavail = 5, >> + .oobfree = { >> + {0, 5} >> + }, >> +}; >> > > Looks like you also have a free byte at offset 6. > Good catch. > Leave out oobavail, it will be calculated by generic code. > Ok. > >> +static struct nand_ecclayout nand_hw_eccoob_2k = { >> + .eccbytes = 36, >> + .eccpos = { >> + /* 9 bytes of ecc for each 512 bytes of data */ >> + 7, 8, 9, 10, 11, 12, 13, 14, 15, >> + 23, 24, 25, 26, 27, 28, 29, 30, 31, >> + 39, 40, 41, 42, 43, 44, 45, 46, 47, >> + 55, 56, 57, 58, 59, 60, 61, 62, 63, >> + }, >> + .oobavail = 26, >> + .oobfree = { >> + {0, 5}, >> + {16, 7}, >> + {32, 7}, >> + {48, 7}, >> + }, >> +}; >> > > Byte zero is not free (it's used for bad-block markers), and byte one > should be reserved for this as well. Bytes 5 and 6 are free. > Ok. > >> +static struct nand_ecclayout nand_hw_eccoob_4k = { >> + .eccbytes = 64, /* actually 72 but only room for 64 */ >> > > Let's fix that, then. > This is defined in generic mtd code that has exposure to userland. include/mtd/mtd-abi.h: /* * ECC layout control structure. Exported to userspace for * diagnosis and to allow creation of raw images */ struct nand_ecclayout { uint32_t eccbytes; uint32_t eccpos[64]; uint32_t oobavail; struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES]; }; > >> +/* >> + * Functions to transfer data to/from spare erea. >> + */ >> +static void copy_from_spare(struct mtd_info *mtd, void *pbuf, int len) >> +{ >> + u16 ooblen = mtd->oobsize; >> + u8 i, count, size; >> + >> + count = mtd->writesize >> 9; >> + size = (ooblen / count >> 1) << 1; >> > > If you want to round down to the nearest even number, use & ~1 (why would > it ever be odd?). If not, what's going on? > I'll clean this up. This is left over from the i.MX version where io access's have to be word at a time. > >> + for (i = 0; i < count - 1; i++) { >> + memcpy_fromio(pbuf, SPARE_AREA(i), size); >> + pbuf += size; >> + len -= size; >> + } >> > > Shouldn't you check to make sure that len doesn't go negative > yes > >> +/*! >> + * This function polls the NFC to wait for the basic operation to complete >> by >> + * checking the INT bit of config2 register. >> + * >> + * @maxRetries number of retry attempts (separated by 1 us) >> + */ >> > > noJavaCase, please. > Oops, missed one. > >> +/* >> + * Function to correct the detected errors. This NFC corrects all the errors >> + * detected. So this function is not required. >> + */ >> +static int mpc5121_nand_correct_data(struct mtd_info *mtd, u_char *dat, >> + u_char *read_ecc, u_char *calc_ecc) >> +{ >> + panic("Shouldn't be called here: %d\n", __LINE__); >> + return 0; /* FIXME */ >> +} >> + >> +/* >> + * Function to calculate the ECC for the data to be stored in the Nand >> device. >> + * This NFC has a hardware RS(511,503) ECC engine together with the RS ECC >> + * CONTROL blocks are responsible for detection and correction of up to >> + * 4 symbols of 9 bits each in 528 byte page. >> + * So this function is not required. >> + */ >> + >> +static int mpc5121_nand_calculate_ecc(struct mtd_info *mtd, const u_char >> *dat, >> + u_char *ecc_code) >> +{ >> + panic("Shouldn't be called here %d \n", __LINE__); >> + return 0; /* FIXME */ >> +} >> > > Just leave these function pointers NULL, since you replace read_page > and write_page. > ok > +static u_char mpc5121_nand_read_byte(struct mtd_info *mtd) > +{ > + void *area_buf; > + u_char rv; > + > + /* Check for status request */ > + if (priv->status_req) { > + rv = get_dev_status() & 0xff; > + return rv; > + } > + > + if (priv->spare_only) > + area_buf = SPARE_AREA(0); > + else > + area_buf = MAIN_AREA(0); > + > + rv = in_8(area_buf + priv->col_addr); > + priv->col_addr++; > + return rv; > +} > > You appear to support using read_byte on the spare area with READOOB, but > not if the buffer had merely advanced past the main area. Not sure that > it matters here, though. > ok > >> +/*! >> + * This function reads byte from the NAND Flash >> + * >> + * @mtd MTD structure for the NAND Flash >> + * >> + * @return data read from the NAND Flash >> + */ >> +static u_char mpc5121_nand_read_byte16(struct mtd_info *mtd) >> +{ >> + /* Check for status request */ >> + if (priv->status_req) >> + return (get_dev_status() & 0xff); >> + >> + return mpc5121_nand_read_word(mtd) & 0xff; >> +} >> > > read_byte should never be called with 16-bit NAND. > This is the replacement for nand_read_byte16 in nand_base.c. /** * nand_read_byte16 - [DEFAULT] read one byte endianess aware from the chip * @mtd: MTD device structure * * Default read function for 16bit buswith with * endianess conversion */ static uint8_t nand_read_byte16(struct mtd_info *mtd) { struct nand_chip *chip = mtd->priv; return (uint8_t) cpu_to_le16(readw(chip->IO_ADDR_R)); } > >> +/*! >> + * This function writes data of length \b len from buffer \b buf to the NAND >> + * internal RAM buffer's MAIN area 0. >> + * >> + * @mtd MTD structure for the NAND Flash >> + * @buf data to be written to NAND Flash >> + * @len number of bytes to be written >> + */ >> +static void mpc5121_nand_write_buf(struct mtd_info *mtd, >> + const u_char *buf, int len) >> +{ >> + printf("re-work may be needed?\n"); >> > > Answer this before we apply the patch. :-) > Perhaps I can just get rid of this whole routine? > >> + if (page_addr != -1) >> + do { >> + send_addr((page_addr & 0xff)); >> + page_mask >>= 8; >> + page_addr >>= 8; >> + } while (page_mask != 0); >> +} >> > > Braces around multi-line if-bodies. > ok > >> + >> +/* >> + * Function to read a page from nand device. >> + */ >> +static void read_full_page(struct mtd_info *mtd, int page_addr) >> +{ >> + send_cmd(NAND_CMD_READ0); >> + >> + mpc5121_nand_do_addr_cycle(mtd, 0, page_addr); >> + >> + if (IS_LARGE_PAGE_NAND) { >> + send_cmd(NAND_CMD_READSTART); >> + send_read_page(0); >> + } else >> + send_read_page(0); >> > > If you put braces around one half of the if, put it around the other. > ok > >> +static int mpc5121_nand_wait(struct mtd_info *mtd, struct nand_chip *chip) >> +{ >> + return (int)(get_dev_status()); >> +} >> > > Unnecessary cast. > ok > >> +static int mpc5121_nand_read_oob(struct mtd_info *mtd, struct nand_chip >> *chip, >> + int page, int sndcmd) >> +{ >> + if (sndcmd) { >> + read_full_page(mtd, page); >> + sndcmd = 0; >> + } >> + >> + copy_from_spare(mtd, chip->oob_poi, mtd->oobsize); >> + return sndcmd; >> +} >> + >> +static int mpc5121_nand_write_oob(struct mtd_info *mtd, struct nand_chip >> *chip, >> + int page) >> +{ >> + int status = 0; >> + int read_oob_col = 0; >> + >> + send_cmd(NAND_CMD_READ0); >> + send_cmd(NAND_CMD_SEQIN); >> + mpc5121_nand_do_addr_cycle(mtd, read_oob_col, page); >> + >> + /* copy the oob data */ >> + copy_to_spare(mtd, chip->oob_poi, mtd->oobsize); >> + >> + send_prog_page(0); >> + >> + send_cmd(NAND_CMD_PAGEPROG); >> + >> + status = mpc5121_nand_wait(mtd, chip); >> + if (status & NAND_STATUS_FAIL) >> + return -1; >> + return 0; >> +} >> > > Why override these rather than let them go through the command function? > Not sure, I think it is so we can call the copy to/from spare routine. > >> +static struct nand_bbt_descr smallpage_memorybased = { >> + .options = NAND_BBT_SCAN2NDPAGE, >> + .offs = 5, >> + .len = 1, >> + .pattern = scan_ff_pattern >> +}; >> > > This is the default. > ok > >> +static struct nand_bbt_descr largepage_memorybased = { >> + .options = 0, >> + .offs = 5, >> + .len = 2, >> + .pattern = scan_ff_pattern >> +}; >> > > This isn't normal -- why are you starting it at offset 5? > will fix > >> +static int mpc5121_nand_scan_bbt(struct mtd_info *mtd) >> +{ >> + struct nand_chip *this = mtd->priv; >> + >> + if (IS_2K_PAGE_NAND) >> + this->ecc.layout = &nand_hw_eccoob_2k; >> + else if (IS_4K_PAGE_NAND) >> + if (priv->sparesize == 128) >> + this->ecc.layout = &nand_hw_eccoob_4k; >> + else >> + this->ecc.layout = &nand_hw_eccoob_4k_218_spare; >> + else >> + this->ecc.layout = &nand_hw_eccoob_512; >> + >> + /* propagate ecc.layout to mtd_info */ >> + mtd->ecclayout = this->ecc.layout; >> + >> +#if 0 >> + /* jffs2 should not write oob */ >> + mtd->flags &= ~MTD_OOB_WRITEABLE; >> +#endif >> > > Should this be set, or not? No dead code. > This is left over from the linux driver which has a patch that makes JFFS2 not write to the OOB area. > >> + /* use flash based bbt */ >> + this->bbt_td = &bbt_main_descr; >> + this->bbt_md = &bbt_mirror_descr; >> + >> + /* update flash based bbt */ >> + this->options |= NAND_USE_FLASH_BBT; >> + >> + if (!this->badblock_pattern) >> + this->badblock_pattern = (mtd->writesize > 512) ? >> + &largepage_memorybased : &smallpage_memorybased; >> + >> + /* Build bad block table */ >> + return nand_scan_bbt(mtd, this->badblock_pattern); >> +} >> > > I'd rather scan_bbt not be abused for this -- we need to fix the NAND > probe code so that drivers can do things between nand_scan() and > nand_scan_tail(). > And until then? > -Scott > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot