[U-Boot] [PATCH-ARM 1/2] Add support for the Embest SBC2440-II Board

[Scott Wood]

Keven Morfitt wrote:
> diff --git a/drivers/mtd/nand/s3c2410_nand.c b/drivers/mtd/nand/s3c2410_nand.c
> index 60bfd10..b93787c 100644
> --- a/drivers/mtd/nand/s3c2410_nand.c
> +++ b/drivers/mtd/nand/s3c2410_nand.c
> @@ -36,7 +36,7 @@
>  static void s3c2410_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  {
>  	struct nand_chip *chip = mtd->priv;
> -	S3C24X0_NAND * const nand = S3C24X0_GetBase_NAND();
> +	S3C2410_NAND * const nand = S3C2410_GetBase_NAND();

This isn't new to this patch, but please don't make type names look like
preprocessor macros (i.e. not ALL CAPS).  Also, no space after * when
it means pointer and not multiply.

Is there any particular reason to declare the pointer itself as const? 
It's just a local variable.

> +#ifdef CONFIG_S3C2440_NAND_HWECC
> +/* new oob placement block for use with hardware ecc generation
> + */
> +static struct nand_ecclayout nand_hw_eccoob = {
> +	.eccbytes = 3,
> +	.eccpos = {0, 1, 2},
> +	.oobfree = { {8, 8} }
> +};

Any reason why bytes 3, 4, 6, and 7 aren't free?

> +static int s3c2440_dev_ready(struct mtd_info *mtd)
> +{
> +	S3C2440_NAND * const nand = S3C2440_GetBase_NAND();
> +
> +	debugX(1, "dev_ready\n");
> +	return readl(&nand->NFSTAT) & 0x01;
> +}
> +
> +#ifdef CONFIG_S3C2440_NAND_HWECC
> +void s3c2440_nand_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +	S3C2440_UART * const nand = S3C2440_GetBase_NAND();

UART?

> +static int s3c2440_nand_correct_data(struct mtd_info *mtd, u_char *dat,
> +				     u_char *read_ecc, u_char *calc_ecc)
> +{
> +	unsigned int diff0, diff1, diff2;
> +	unsigned int bit, byte;
> +
> +	debugX(2, "s3c2440_nand_correct_data:\n");
> +
> +	diff0 = read_ecc[0] ^ calc_ecc[0];
> +	diff1 = read_ecc[1] ^ calc_ecc[1];
> +	diff2 = read_ecc[2] ^ calc_ecc[2];
> +
> +	debugX(3, "rd %02x%02x%02x calc %02x%02x%02x diff %02x%02x%02x\n",
> +		read_ecc[0], read_ecc[1], read_ecc[2],
> +		calc_ecc[0], calc_ecc[1], calc_ecc[2],
> +		diff0, diff1, diff2);
> +
> +	if (diff0 == 0 && diff1 == 0 && diff2 == 0)
> +		return 0;	/* ECC is ok */
> +
> +	/* Can we correct this ECC (ie, one row and column change).
> +	 * Note, this is similar to the 256 error code on smartmedia */
> +
> +	if (((diff0 ^ (diff0 >> 1)) & 0x55) == 0x55 &&
> +	    ((diff1 ^ (diff1 >> 1)) & 0x55) == 0x55 &&
> +	    ((diff2 ^ (diff2 >> 1)) & 0x55) == 0x55) {
> +		/* calculate the bit position of the error */
> +		bit  = ((diff2 >> 3) & 1) |
> +		       ((diff2 >> 4) & 2) |
> +		       ((diff2 >> 5) & 4);
> +
> +		/* calculate the byte position of the error */
> +		byte = ((diff2 << 7) & 0x100) |
> +		       ((diff1 << 0) & 0x80)  |
> +		       ((diff1 << 1) & 0x40)  |
> +		       ((diff1 << 2) & 0x20)  |
> +		       ((diff1 << 3) & 0x10)  |
> +		       ((diff0 >> 4) & 0x08)  |
> +		       ((diff0 >> 3) & 0x04)  |
> +		       ((diff0 >> 2) & 0x02)  |
> +		       ((diff0 >> 1) & 0x01);
> +
> +		debugX(2, "correcting error bit %d, byte %d\n", bit, byte);
> +
> +		dat[byte] ^= (1 << bit);
> +		return 1;
> +	}
> +
> +	debugX(2, "Failed to correct ECC error\n");
> +	return -1;
> +}

Hmm, this looks very similar to the generic correct_data, except that it
uses a 512-byte block (and is missing the final countbits check -- not
sure what that's for, perhaps a single bit error in the ECC itself?). 

Perhaps it should be factored out into the generic code?

> +int board_nand_init(struct nand_chip *nand)
> +{
> +	u_int32_t cfg;
> +	u_int8_t tacls, twrph0, twrph1;
> +	S3C24X0_CLOCK_POWER * const clk_power = S3C24X0_GetBase_CLOCK_POWER();
> +	S3C2440_NAND * const nand_reg = S3C2440_GetBase_NAND();
> +
> +	debugX(1, "board_nand_init()\n");
> +
> +	writel(readl(&clk_power->CLKCON) | (1 << 4),
> +	       &clk_power->CLKCON);
> +
> +	/* initialize hardware */
> +	twrph0 = 3; twrph1 = 1; tacls = 0;

Put each statement on its own line.  Do these really need to be held in
local variables rather than just passing them directly to these macros:

> +	cfg = S3C2440_NFCONF_TACLS(tacls);
> +	cfg |= S3C2440_NFCONF_TWRPH0(twrph0);
> +	cfg |= S3C2440_NFCONF_TWRPH1(twrph1);

...which include the same name, so it doesn't have any descriptive value?

Should the values come from the board config file?

-Scott