[U-Boot] [PATCH 1/1] Fix issues and add new commands for onenand.

[Scott Wood]

On Wed, Jan 07, 2009 at 08:56:25AM +0530, Manikandan Pillai wrote:
> This patch has been generated against the tip of u-boot-arm git - origin/omap3 
> branch.

Please make sure that any patch applies on top of the "next" branch of
u-boot-nand-flash (or rebase during the merge window).

> This patch provides for a few more commands for onenand in tune with what
> is avaiable for NAND chips and also debugging functions like scrub and 
> markbad. The bad block checking has been fixed for errors since the new
> framework was failing in reading OOB data.

Another way to have the same functionality available on NAND and OneNAND
is to merge the code. :-)

Could you separate the bugfixes, or at least indicate specifically what
the bug is?

> +	case 3:
> +		if ((strncmp(argv[1], "markbad", 7) == 0) && (argc == 3)) {

You can only get here if argc == 3, so the extra check is redundant.

> +			int ret ;
> +			ofs = simple_strtoul(argv[2], NULL, 16);
> +			if (ofs >= onenand_mtd.size) {
> +				printf("Error : offset exceeds size\n");
> +				return 0;
> +			} else
> +				ret = onenand_block_markbad(&onenand_mtd, ofs);
> +

Unnecessary else.

> +			if (ret)
> +				printf("Error marking bad-block\n");
> +			else
> +				printf("Done\n");
> +			return 0;
> +		}
> +		printf("OneNAND : wrong number of arguments\n");
> +		onenand_print_device_info(onenand_chip.device_id);
> +		printf("Usage:\n%s\n", cmdtp->usage);
> +		return 0;
> +
>  	default:
>  		/* At least 4 args */
> -		if (strncmp(argv[1], "erase", 5) == 0) {
> +		if (((strncmp(argv[1], "erase", 5) == 0) ||
> +			(strncmp(argv[1], "scrub", 5) == 0)) &&
> +			(argc == 4)) {

Please align continuation lines with the beginning of the condition; just
hitting tab once makes it hard to distinguish them from the if-body.

>  			struct erase_info instr = {
>  				.callback	= NULL,
>  			};
> -			ulong start, end;
> -			ulong block;
> +			ulong start = 0, end = 0;
> +			ulong block = 0;
>  			char *endtail;
>  
>  			if (strncmp(argv[2], "block", 5) == 0) {
>  				start = simple_strtoul(argv[3], NULL, 10);
>  				endtail = strchr(argv[3], '-');
>  				end = simple_strtoul(endtail + 1, NULL, 10);
> +				if (end < start) {
> +					printf("Error : erase failed - ");
> +					printf("end block incorrect\n");
> +					break;

Just use one printf() with a continuation line -- or better yet, factor
the code out into its own function so it's not so heavily indented.

> -		if (strncmp(argv[1], "write", 5) == 0) {
> +		} else if ((strncmp(argv[1], "write", 5) == 0) &&
> +			(argc == 5)) {
>  			ulong addr = simple_strtoul(argv[2], NULL, 16);
>  			ulong ofs = simple_strtoul(argv[3], NULL, 16);
>  			size_t len = simple_strtoul(argv[4], NULL, 16);
>  			size_t retlen = 0;
>  
> -			onenand_write(&onenand_mtd, ofs, len, &retlen,
> +			ret = onenand_write(&onenand_mtd, ofs, len, &retlen,
>  				      (u_char *) addr);
> -			printf("Done\n");
> +			if (ret)
> +				printf("Error writing to device\n");
> +			else
> +				printf("Done\n");
>  
>  			return 0;
> -		}
> -
> -		if (strncmp(argv[1], "block", 5) == 0) {
> +		} else if (strncmp(argv[1], "block", 5) == 0) {

The else is not necessary.

> @@ -790,13 +868,11 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
>  	}
>  
>  	ops->oobretlen = read;
> -
>  	if (ret)
>  		return ret;
>  
>  	if (mtd->ecc_stats.failed - stats.failed)
>  		return -EBADMSG;
> -
>  	return 0;
>  }
>  
> @@ -1250,7 +1326,6 @@ static int onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to,
>  
>  	/* Initialize retlen, in case of early exit */
>  	ops->oobretlen = 0;
> -
>  	if (mode == MTD_OOB_AUTO)
>  		oobsize = this->ecclayout->oobavail;
>  	else

Please don't make unrelated (and IMHO undesireable) whitespace changes.

> +				printf("onenand_erase: not erasing\
> +				factory bad blk @0x%x\n", (int)addr);

That's going to insert a bunch of tabs into the output.

Do this instead:

printf("a really long line"
       "and the continuation");

> +			if (onenand_block_isbad(mtd, addr) == 0) {
> +				/* block is not a known bad block. Erase it */
> +				this->command(mtd, ONENAND_CMD_ERASE,\
> +						addr, block_size);

Unnecessary backslash.

-Scott