Stefan,
My only concern is that the u-boot and linux nand drivers need to have the same approach regarding ecc. The linux driver recently submitted only supports sw ecc because using hw ecc means the spare area is not writeable. The u-boot driver that I submitted supported hw_ecc only and was compatible with the linux driver in ltib. Unusable spare is an issue for JFFS2 but not UBIFS. If I were to make the decision I would just say not to JFFS2 on NAND. UBIFS is a far better solution for NAND anyway.
I have seen at least one other controller where the controller included the spare in the ECC so I don't know if this is a trend or not.
John
On Thu, Jun 4, 2009 at 7:18 AM, Stefan Roese sr@denx.de wrote:
Hi Scott,
I'll try to continue with this patch so that we can integrate it hopefully soon. I already addressed some of your comments (the easy ones ;)). Please find some further questions below (I'm still new to the FSL NFC):
On Thursday 06 November 2008 00:06:48 Scott Wood wrote:
+static struct fsl_nfc_private {
- struct mtd_info mtd;
- char spare_only;
- char status_req;
- u16 col_addr;
- int writesize;
- int sparesize;
- int width;
- int chipsel;
+} *priv;
Is it plausable that there will ever be a chip with more than one of these controllers?
I have no idea. What do you suggest I should change here?
+/*
- 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,- },
- .oobfree = {
{0, 5} /* byte 5 is factory bad block marker */- },
+};
Is byte 6 free?
Looks this way. I'll add it to the free area. John, please shout if you think this is not correct.
+static struct nand_ecclayout nand_hw_eccoob_4k_218_spare = {
- .eccbytes = 64, /* actually 144 but only room for 64 */
- .eccpos = {
/* 18 bytes of ecc for each 512 bytes of data */7, 8, 9, 10, 11, 12, 13, 14, 15,16, 17, 18, 19, 20, 21, 22, 23, 24,33, 34, 35, 36, 37, 38, 39, 40, 41,42, 43, 44, 45, 46, 47, 48, 49, 50,59, 60, 61, 62, 63, 64, 65, 66, 67,68, 69, 70, 71, 72, 73, 74, 75, 76,85, 86, 87, 88, 89, 90, 91, 92, 93,94, /* 95, 96, 97, 98, 99, 100, 101, 102,111, 112, 113, 114, 115, 116, 117, 118, 119,120, 121, 122, 123, 124, 125, 126, 127, 128,137, 138, 139, 140, 141, 142, 143, 144, 145,146, 147, 148, 149, 150, 151, 152, 153, 154,163, 164, 165, 166, 167, 168, 169, 170, 171,172, 173, 174, 175, 176, 177, 178, 179, 180,189, 190, 191, 192, 193, 194, 195, 196, 197,198, 199, 200, 201, 202, 203, 204, 205, 206, */- },
- .oobfree = {
{2, 5}, /* bytes 0 and 1 are factory bad block markers */{25, 8},{51, 8},{77, 8},{103, 8},{129, 8},{155, 8},{181, 8},- },
+};
Need to change NAND_MAX_OOBSIZE.
I'll send a patch for this shortly.
+#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC +static void fsl_nfc_select_chip(u8 cs) +{
- u32 val = NFC_READW(NFC_BUF_ADDR);
- val &= ~0x60;
- val |= cs << 5;
- NFC_WRITEW(NFC_BUF_ADDR, val);
+} +#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip +#endif
+/*!
- This function is used by upper layer for select and deselect of the
NAND + * chip
- @mtd MTD structure for the NAND Flash
- @chip val indicating select or deselect
- */
+static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)
This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is not defined.
Works fine here on a board without CONFIG_FSL_NFC_BOARD_CS_FUNC defined. So I'll leave it as is.
- case NAND_CMD_READOOB:
priv->col_addr = column;priv->spare_only = 1;command = NAND_CMD_READ0; /* only READ0 is valid */break;What about small-page flash that takes an actual READOOB command?
I don't have access to a board with small-page NAND. So I can't test anything here.
+static int fsl_nfc_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 fsl_nfc_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);
- fsl_nfc_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 = fsl_nfc_wait(mtd, chip);
- if (status & NAND_STATUS_FAIL)
return -1;- return 0;
+}
Again, I'm fairly sure you don't need these if the other functions are defined properly.
OK, I removed these functions and tested a bit on my MPC5121 board. Everything seems to be working fine without it.
Again, John please let me know if you think these functions are really necessary.
+/*
- These are identical to the generic versions except
- for the offsets.
- */
+static struct nand_bbt_descr bbt_main_descr = {
- .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,- .offs = 0,
- .len = 4,
- .veroffs = 4,
- .maxblocks = 4,
- .pattern = bbt_pattern
+};
+static struct nand_bbt_descr bbt_mirror_descr = {
- .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,- .offs = 0,
- .len = 4,
- .veroffs = 4,
- .maxblocks = 4,
- .pattern = mirror_pattern
+};
This will overlap the bad block marker on large-page flash.
Good catch. Do you have any idea how can this be solved?
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================