John Rigby wrote:
ADS5121 rev4 / MPC5121e rev2 only
What is unique about that revision that it cannot share a driver? It certainly shouldn't be board-specific...
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 jrigby@freescale.com
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?
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?
+/*
- 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.
Leave out oobavail, it will be calculated by generic code.
+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.
+static struct nand_ecclayout nand_hw_eccoob_4k = {
- .eccbytes = 64, /* actually 72 but only room for 64 */
Let's fix that, then.
+/*
- 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?
- 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?
+/*!
- 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.
+/*
- 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.
+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.
+/*!
- 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 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. :-)
- 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.
+/*
- 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.
+static int mpc5121_nand_wait(struct mtd_info *mtd, struct nand_chip *chip) +{
- return (int)(get_dev_status());
+}
Unnecessary cast.
+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?
+static struct nand_bbt_descr smallpage_memorybased = {
- .options = NAND_BBT_SCAN2NDPAGE,
- .offs = 5,
- .len = 1,
- .pattern = scan_ff_pattern
+};
This is the default.
+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?
+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;elsethis->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.
- /* 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().
-Scott