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

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