[U-Boot] [PATCH v3] Freescale NFC NAND driver

[Stefan Roese <sr@denx.de>]

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 at denx.de
=====================================================================