[U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command

[Ivan Khoronzhuk]

On 06/20/2014 04:00 AM, Scott Wood wrote:
> On Fri, May 16, 2014 at 09:26:36PM +0300, Khoronzhuk, Ivan wrote:
>> From: WingMan Kwok <w-kwok2 at ti.com>
>>
>> This commit adds a nand ecclayout command that allows the ecclayout of
>> the current nand device to be changed during run time. This feature is
>> useful when using u-boot to write something to nand flash that will be
>> read by other applications, such as ROM bootloader, that expects a
>> different ECC layout. In that case, change the current nand device
>> ecclayout using the "nand ecclayout set" command before writing the
>> data to nand flash.
>>
>> Signed-off-by: WingMan Kwok <w-kwok2 at ti.com>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk at ti.com>
>>
>> ---
>> common/cmd_nand.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   include/nand.h    |  10 +++++
>>   2 files changed, 124 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
>> index 04ab0f1..7cbe6fc 100644
>> --- a/common/cmd_nand.c
>> +++ b/common/cmd_nand.c
>> @@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev)
>>   	}
>>   }
>>   
>> +#ifdef CONFIG_CMD_NAND_ECCLAYOUT
>> +static void nand_print_ecclayout_info(struct nand_ecclayout *p)
>> +{
>> +	int i;
>> +	struct nand_oobfree *oobfree;
>> +
>> +	if (!p)
>> +		return;
>> +
>> +	printf("  num ecc bytes: %d\n", p->eccbytes);
>> +	puts("  ecc pos:\n    ");
>> +
>> +	for (i = 0; i < p->eccbytes; i++) {
>> +		if (i && !(i % 9))
>> +			printf("\n    ");
>> +
>> +		printf("%2d ", p->eccpos[i]);
>> +	}
> Why 9?

It's to print a new line on every 9th character position.
I'll add a comment.


>
>> +	puts("\n  oobfree:\n");
>> +	puts("    offset length\n");
>> +
>> +	oobfree = p->oobfree;
>> +	for (i = 0; oobfree->length && i < MTD_MAX_OOBFREE_ENTRIES_LARGE; i++) {
>> +		printf("     %2d    %2d\n", oobfree->offset, oobfree->length);
>> +		oobfree++;
>> +	}
>> +}
>> +
>> +static void nand_print_device_ecclayout(int dev)
>> +{
>> +	int idx;
>> +	nand_info_t *nand = &nand_info[dev];
>> +	struct nand_chip *chip = nand->priv;
>> +
>> +	idx = board_nand_ecclayout_get_idx(chip, chip->ecc.layout);
>> +
>> +	if (idx < 0) {
>> +		puts("no ecc layout\n");
>> +		return;
>> +	}
>> +
>> +	printf("\necc layout %d:\n", idx);
>> +	nand_print_ecclayout_info(chip->ecc.layout);
>> +}
>> +#endif
>> +
>>   static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   {
>>   	int i, ret = 0;
>> @@ -506,8 +553,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   			putc('\n');
>>   			if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE)
>>   				puts("no devices available\n");
>> -			else
>> +			else {
>>   				nand_print_and_set_info(dev);
>> +#ifdef CONFIG_CMD_NAND_ECCLAYOUT
>> +				nand_print_device_ecclayout(dev);
>> +#endif
>> +			}
>>   			return 0;
>>   		}
> Braces are needed on both sides of if/else if used on one side.

Yes.

>
> It would be better to provide an inline no-op stub when
> CONFIG_CMD_NAND_ECCLAYOUT, than ifdeffing at the call site.

Yes. It's better.

>
> Does it really make sense to dump this information here, when the user
> could use the ecclayout command instead?

Yes. Seems it's redundant. I'll better delete it.

>
>> @@ -836,6 +887,60 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   	}
>>   #endif
>>   
>> +#ifdef CONFIG_CMD_NAND_ECCLAYOUT
>> +	if (strcmp(cmd, "ecclayout") == 0) {
>> +		int idx;
>> +		struct nand_ecclayout *p;
>> +		nand_info_t *nand = &nand_info[dev];
>> +		struct nand_chip *chip = nand->priv;
>> +
>> +		if (argc < 3) {
>> +			puts("Current device ecclayout:\n");
>> +			nand_print_device_ecclayout(dev);
>> +			return 0;
>> +		}
>> +
>> +		if (!strcmp(argv[2], "set")) {
>> +			if (argc < 4)
>> +				return 1;
>> +
>> +			idx = (int)simple_strtoul(argv[3], NULL, 10);
>> +			if (!board_nand_ecclayout_set(chip, idx)) {
>> +				p = chip->ecc.layout;
>> +				p->oobavail = 0;
>> +				for (i = 0; p->oobfree[i].length &&
>> +						i < ARRAY_SIZE(p->oobfree); i++)
>> +					p->oobavail += p->oobfree[i].length;
>> +
>> +				nand->oobavail = p->oobavail;
> Shouldn't board_nand_ecclayout_set() take care of this?  Possibly via
> common helper functions, but not here.

Yes, It should be moved to board_nand_ecclayout_set()

>> +			} else {
>> +				printf("Setting current device"
>> +				       " to ecc layout %d FAILED!\n", idx);
> Don't wrap user-visible strings (checkpatch should have warned you about
> this).

For user's eyes it's not wrapped.

>> +			}
>> +
>> +			return 0;
>> +		}
>> +
>> +		if (strcmp(argv[2], "all") != 0)
>> +			return 1;
>> +
>> +		/* show all available ecc layouts */
>> +		puts("Available ecc layouts:\n");
>> +
>> +		idx = 0;
>> +		while (1) {
>> +			p = board_nand_ecclayout_get_layout(chip, idx);
>> +			if (!p)
>> +				break;
>> +
>> +			printf("\n ecc layout %d:\n", idx++);
>> +			nand_print_ecclayout_info(p);
>> +		}
> Please use a for loop rather than hiding the increment in the loop body.

Ok.

>
>> +
>> +		return 0;
>> +	}
>> +#endif
>> +
>>   usage:
>>   	return CMD_RET_USAGE;
>>   }
>> @@ -890,6 +995,14 @@ static char nand_help_text[] =
>>   	"nand env.oob set off|partition - set enviromnent offset\n"
>>   	"nand env.oob get - get environment offset"
>>   #endif
>> +#ifdef CONFIG_CMD_NAND_ECCLAYOUT
>> +	"\n"
>> +	"nand ecclayout <set <idx>|all> - show or set ecclayout"
>> +	" of current device\n"
>> +	"    'all' shows all available ecc layouts for setting"
>> +	" to current device\n"
>> +	"    'set' sets current device ecc layout to layout indexed by idx\n"
>> +#endif
> This help text doesn't include the plain "nand ecclayout" to show the
> current layout.

Yes.

>> diff --git a/include/nand.h b/include/nand.h
>> index fc735d1..0e8a093 100644
>> --- a/include/nand.h
>> +++ b/include/nand.h
>> @@ -155,6 +155,16 @@ void nand_deselect(void);
>>   void board_nand_select_device(struct nand_chip *nand, int chip);
>>   #endif
>>   
>> +#ifdef CONFIG_CMD_NAND_ECCLAYOUT
>> +int board_nand_ecclayout_get_idx(struct nand_chip *nand,
>> +				 struct nand_ecclayout *p);
>> +
>> +struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
>> +						       int idx);
>> +
>> +int board_nand_ecclayout_set(struct nand_chip *nand, int idx);
>> +#endif
> Don't ifdef prototypes.

Ok. Thanks! I will repost soon.

>
> -Scott

-- 
Regards,
Ivan Khoronzhuk