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 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?
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;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.
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