[U-Boot] [PATCH v3] Nand driver for Nomadik SoC

[Scott Wood]

On Sun, Feb 08, 2009 at 12:19:56AM +0100, Alessandro Rubini wrote:
> +static inline int parity(int b) /* b is really a byte; returns 0 or ~0 */

If it's really a byte, then why not tell the compiler this with uint8_t?

> +{
> +	__asm__ __volatile__(
> +		"eor   %0, %0, %0, lsr #4\n\t"
> +		"eor   %0, %0, %0, lsr #2\n\t"
> +		"eor   %0, %0, %0, lsr #1\n\t"
> +		"ands  %0, %0, #1\n\t"
> +		"subne %0, %0, #2\t"
> +		: "=r" (b) : "0" (b));
> +	return b;
> +}

Why is this volatile?
The underscores are unnecessary, BTW.

Have you verified that this is noticeably better than C code?

> +/*
> + * This is the ECC routine used in hardware, according to the manual.
> + * HW claims to make the calculation but not the correction; so we must
> + * recalculate the bytes for a comparison.
> + */

Why must you recalculate?  What does the hardware do with the ECC it
calculates?

> +	diff = (r ^ c) & ((1<<24)-1); /* use 24 bits only */

Put spaces around binary operators.

> +/* This is the layout used by older installations, we keep compatible */
> +struct nand_ecclayout nomadik_ecc_layout = {
> +	.eccbytes = 3 * 4,
> +	.eccpos = { /* each subpage has 16 bytes: pos 2,3,4 hosts ECC */
> +		0x02, 0x03, 0x04,
> +		0x12, 0x13, 0x14,
> +		0x22, 0x23, 0x24,
> +		0x32, 0x33, 0x34},
> +	.oobfree = { {0x08, 0x08}, {0x18, 0x08}, {0x28, 0x08}, {0x38, 0x08} },
> +};

Any particular reason why bytes 0x05-0x07, 0x10-0x11, 0x15-0x17, etc. aren't marked
free?

> +/* Copy a buffer 32bits at a time: faster than defualt method which is 8bit */
> +static void nomadik_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	int i;
> +	struct nand_chip *chip = mtd->priv;
> +	u32 *p = (u32 *) buf;
> +
> +	len >>= 2;
> +	writel(0, REG_FSMC_ECCR0);
> +	for (i = 0; i < len; i++)
> +		p[i] = readl(chip->IO_ADDR_R);
> +}

What if "len" isn't a multiple of 4?

> +int board_nand_init(struct nand_chip *chip)
> +{
> +	/* Set up the FSMC_PCR0 for nand access*/
> +	writel(0x0000004a, REG_FSMC_PCR0);
> +	/* Set up FSMC_PMEM0, FSMC_PATT0 with timing data for access */
> +	writel(0x00020401, REG_FSMC_PMEM0);
> +	writel(0x00020404, REG_FSMC_PATT0);
> +
> +	chip->options = NAND_COPYBACK |	NAND_CACHEPRG | NAND_NO_PADDING;
> +	chip->cmd_ctrl = nomadik_nand_hwcontrol;
> +	chip->dev_ready = nomadik_nand_ready;
> +	/* The chip allows 32bit reads, so avoid the default 8bit copy */
> +	chip->read_buf = nomadik_nand_read_buf;
> +
> +	/* ECC: follow the hardware-defined rulse, but do it in sw */

"rules"

> -#define CONFIG_SYS_NAND_BASE		0x40000000
> +#define CONFIG_SYS_NAND_BASE		0x40000000 /* SMPS0n */

What is "SMPS0n"?

-Scott