[U-Boot] [PATCH v6 4/7] jz4740 nand driver

[Scott Wood]

On Mon, 28 Feb 2011 14:57:11 +0800
Xiangfu Liu <xiangfu at openmobilefree.net> wrote:

> +static int jz_nand_rs_correct_data(struct mtd_info *mtd, u_char *dat,
> +				   u_char *read_ecc, u_char *calc_ecc)
> +{
> +	int k;
> +	uint32_t errcnt, index, mask, status;
> +	volatile u8 *paraddr = (volatile u8 *) &emc->nfpar[0];
> +
> +	/* Set PAR values */
> +	static uint8_t all_ff_ecc[] = 
> +		{0xcd, 0x9d, 0x90, 0x58, 0xf4, 0x8b, 0xff, 0xb7, 0x6f};

const?

> +
> +	if (read_ecc[0] == 0xff && read_ecc[1] == 0xff &&
> +	    read_ecc[2] == 0xff && read_ecc[3] == 0xff &&
> +	    read_ecc[4] == 0xff && read_ecc[5] == 0xff &&
> +	    read_ecc[6] == 0xff && read_ecc[7] == 0xff &&
> +	    read_ecc[8] == 0xff) {
> +		for (k = 0; k < 9; k++)
> +			writeb(all_ff_ecc[k], (paraddr + k));
> +	} else {
> +		for (k = 0; k < 9; k++)
> +			writeb(read_ecc[k], (paraddr + k));
> +	}

Wouldn't it be simpler  to do:

writeb(..., &emc->nfpar[k]);

And don't use volatile.

> +	/* Set PRDY */
> +	writel(readl(&emc->nfecr) | EMC_NFECR_PRDY, &emc->nfecr);
> +
> +	/* Wait for completion */
> +	do {
> +		status = readl(&emc->nfints);
> +	} while (!(status & EMC_NFINTS_DECF));
> +
> +	/* disable ecc */
> +	writel(readl(&emc->nfecr) & ~EMC_NFECR_ECCE, &emc->nfecr);
> +
> +	/* Check decoding */
> +	if (!(status & EMC_NFINTS_ERR))
> +		return 0;
> +
> +	if (status & EMC_NFINTS_UNCOR) {
> +		printf("uncorrectable ecc\n");
> +		return -1;
> +	}
> +
> +	errcnt = (status & EMC_NFINTS_ERRCNT_MASK) >> EMC_NFINTS_ERRCNT_BIT;
> +

> +#ifdef CONFIG_NAND_SPL
> +       return 0;
> +#endif

Spaces should be a tab.

> +	switch (errcnt) {
> +	case 4:
> +		index = (readl(&emc->nferr[3]) & EMC_NFERR_INDEX_MASK) >>
> +			EMC_NFERR_INDEX_BIT;
> +		mask = (readl(&emc->nferr[3]) & EMC_NFERR_MASK_MASK) >> 
> +			EMC_NFERR_MASK_BIT;
> +		jz_rs_correct(dat, index, mask);
> +	case 3:
> +		index = (readl(&emc->nferr[2]) & EMC_NFERR_INDEX_MASK) >>
> +			EMC_NFERR_INDEX_BIT;
> +		mask = (readl(&emc->nferr[2]) & EMC_NFERR_MASK_MASK) >> 
> +			EMC_NFERR_MASK_BIT;
> +		jz_rs_correct(dat, index, mask);
> +	case 2:
> +		index = (readl(&emc->nferr[1]) & EMC_NFERR_INDEX_MASK) >> 
> +			EMC_NFERR_INDEX_BIT;
> +		mask = (readl(&emc->nferr[1]) & EMC_NFERR_MASK_MASK) >> 
> +			EMC_NFERR_MASK_BIT;
> +		jz_rs_correct(dat, index, mask);
> +	case 1:
> +		index = (readl(&emc->nferr[0]) & EMC_NFERR_INDEX_MASK) >>
> +			EMC_NFERR_INDEX_BIT;
> +		mask = (readl(&emc->nferr[0]) & EMC_NFERR_MASK_MASK) >>
> +			EMC_NFERR_MASK_BIT;
> +		jz_rs_correct(dat, index, mask);
> +	default:
> +		break;
> +	}
> +
> +	return errcnt;

You don't do ECC correction in SPL?  Can it not fit?

> +/*
> + * Main initialization routine
> + */
> +int board_nand_init(struct nand_chip *nand)
> +{
> +#ifdef CONFIG_NAND_SPL
> +extern void pll_init(void);
> +extern void sdram_init(void);
> +extern int serial_init(void);

Externs should go in header files.

> +	__gpio_as_sdram_16bit_4720();
> +	__gpio_as_uart0();
> +
> +	pll_init();
> +	serial_init();
> +	sdram_init();

> +#if defined(CONFIG_QI_LB60)
> +#define KEY_U_OUT       (32 * 2 + 16)
> +#define KEY_U_IN        (32 * 3 + 19)
> +	__gpio_as_input(KEY_U_IN);
> +	__gpio_enable_pull(KEY_U_IN);
> +	__gpio_as_output(KEY_U_OUT);
> +	__gpio_clear_pin(KEY_U_OUT);
> +
> +	if (__gpio_get_pin(KEY_U_IN) == 0)
> +		usb_boot();
> +#endif
> +#endif

This stuff does not belong in the NAND driver; it belongs under your board
or cpu directory.

> +	uint32_t reg;
> +
> +	reg = readl(&emc->nfcsr); 
> +	reg |= EMC_NFCSR_NFE1;	/* EMC setup, Set NFE bit */
> +	writel(reg, &emc->nfcsr); 
> +
> +	writel(EMC_SMCR1_OPT_NAND, &emc->smcr[1]);
> +
> +	nand->IO_ADDR_R		= JZ_NAND_DATA_ADDR;
> +	nand->IO_ADDR_W		= JZ_NAND_DATA_ADDR;
> +	nand->cmd_ctrl		= jz_nand_cmd_ctrl;
> +	nand->dev_ready		= jz_nand_device_ready;
> +#ifdef CONFIG_NAND_SPL
> +	nand->read_byte		= nand_read_byte;
> +	nand->read_buf		= nand_read_buf;
> +#endif
> +	nand->ecc.hwctl		= jz_nand_hwctl;
> +	nand->ecc.correct	= jz_nand_rs_correct_data;
> +	nand->ecc.calculate	= jz_nand_rs_calculate_ecc;
> +	nand->ecc.mode		= NAND_ECC_HW_OOB_FIRST;
> +	nand->ecc.size		= CONFIG_SYS_NAND_ECCSIZE;
> +	nand->ecc.bytes		= CONFIG_SYS_NAND_ECCBYTES;
> +	nand->ecc.layout	= &qi_lb60_ecclayout_2gb;
> +	nand->chip_delay	= 50;

You don't set nand->options...

Don't you want a bad block table?

-Scott