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

[kevin.morfitt at fearnside-systems.co.uk]

On 22/06/2009 20:26, Scott Wood wrote:
> 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.
> 

These type names (and the 'const') are in the existing s3c24x0 code so I 
just made my new code follow the same style and Lindent and checkpatch 
didn't complain. The u-boot coding style guidelines say we should use the 
Linux coding style and this says that 'mixed case names are frowned upon' 
and 'It's a _mistake_ to use typedef for structures' so it doesn't meet 
the coding style, at least for the use of typedef if not for the upper 
case names. I'll change it throughout the s3c24x0 code.

>> +#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?
> 

I ported this from the Linux s3c2410 NAND driver (which covers s3c2440
as well as s3c2410). It worked when I tested it (after I enabled hardware
ECC and fixed the problem below), but I don't know enough about how mtd
hardware ecc works to understand why it was done this way in the Linux 
kernel. A comment in the kernel code says that nand_ecclayout is 
'Exported to userspace for diagnosis and to allow creation of raw 
images' so it's likely I haven't tested this bit as all I did was check 
that NAND read/write worked. I'll have a look at it in more detail.

>> +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?
> 

Thanks, I should obviously be more careful with cut and paste! I also 
forgot to test the patch with hardware ECC enabled.

>> +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?
> 

I'll have a look at doing that. I obviously need to look at it in more
detail.

>> +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?
> 

Again, this comes from the existing s3c24x0 code so I'll change it
throughout.

> Should the values come from the board config file?
> 

Yes. I'll add these to the board config file.

Thanks
Kevin

> -Scott
> 



__________ Information from ESET NOD32 Antivirus, version of virus signature database 4179 (20090622) __________

The message was checked by ESET NOD32 Antivirus.

http://www.eset.com