[U-Boot] [PATCH v2] ADS5121 NAND driver
John Rigby
jrigby at freescale.com
Thu Oct 30 00:37:52 CET 2008
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 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?
>
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
>
>
More information about the U-Boot
mailing list