[U-Boot] [PATCH v2] ADS5121 NAND driver
Scott Wood
scottwood at freescale.com
Wed Oct 29 22:28:25 CET 2008
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 at 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;
> + 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.
> + /* 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
More information about the U-Boot
mailing list