[U-Boot] [PATCH v2] imls: Add support to list images in NAND device

[Vipin Kumar]

[...]

>>
>>   README             |   3 +-
>>   common/cmd_bootm.c | 133
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 134 insertions(+), 2 deletions(-)
>>
>> diff --git a/README b/README
>> index 2077c3b..ec5c31e 100644
>> --- a/README
>> +++ b/README
>> @@ -831,7 +831,8 @@ The following options need to be configured:
>>   		CONFIG_CMD_I2C		* I2C serial bus support
>>   		CONFIG_CMD_IDE		* IDE harddisk support
>>   		CONFIG_CMD_IMI		  iminfo
>> -		CONFIG_CMD_IMLS		  List all found images
>> +		CONFIG_CMD_IMLS		  List all images found in flash
>> +		CONFIG_CMD_IMLS_NAND	  List all images found in NAND
>> device
>
> s/in flash/in NOR flash/
> s/in NAND device/in NAND flash/
>

OK

> Or better, just have one CONFIG_CMD_IMLS and have it operate on
> whatever flash types are configured into U-Boot.
>

I didn't do it because until now the CONFIG_CMD_IMLS config is tightly 
bound with flash only eg config_cmd_default.h enables CONFIG_CMD_IMLS 
only when CONFIG_SYS_NO_FLASH is not defined. I thought there might be 
other places as well

>>   		CONFIG_CMD_IMMAP	* IMMR dump support
>>   		CONFIG_CMD_IMPORTENV	* import an environment
>>   		CONFIG_CMD_INI		* import data from an ini file
>> into the env
>> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
>> index d256ddf..8ee562a 100644
>> --- a/common/cmd_bootm.c
>> +++ b/common/cmd_bootm.c
>> @@ -83,6 +83,9 @@ extern flash_info_t flash_info[]; /* info for FLASH
>> chips */
>>   static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char *
>> const argv[]);
>>   #endif
>>
>> +#include<linux/err.h>
>> +#include<nand.h>
>> +
>>   #ifdef CONFIG_SILENT_CONSOLE
>>   static void fixup_silent_linux(void);
>>   #endif
>> @@ -1175,7 +1178,7 @@ U_BOOT_CMD(
>>   /* imls - list all images found in flash */
>>   /*******************************************************************/
>>   #if defined(CONFIG_CMD_IMLS)
>> -static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char *
>> const argv[])
>> +static void do_imls_flash(void)
>
> s/flash/nor/
>

OK

>>   {
>>   	flash_info_t *info;
>>   	int i, j;
>> @@ -1224,6 +1227,134 @@ next_sector:		;
>>   		}
>>   next_bank:	;
>>   	}
>> +}
>> +#endif
>> +
>> +#if defined(CONFIG_CMD_IMLS_NAND)
>> +static void do_imls_nand(void)
>> +{
>> +	nand_info_t *nand;
>> +	int nand_dev = nand_curr_device;
>> +	size_t read_size;
>> +	loff_t off;
>> +	u8 buffer[512];
>
> Why 512?
>

Basically there are 2 image types supported as of today.
  * Legacy: 64 byte header
  * FIT: < 512 byte header

After reading the first 512 bytes from each block of NAND, we try to 
validate the header and only if the header validation is successful, we 
malloc the space for the whole image and read the image into it

>> +	const image_header_t *header = (const image_header_t *)buffer;
>> +
>> +	/* the following commands operate on the current device */
>> +	if (nand_dev<  0 || nand_dev>= CONFIG_SYS_MAX_NAND_DEVICE) {
>> +		puts("\nNo NAND devices available\n");
>> +		return;
>> +	}
>
> "following commands", plural?
>

I think its a copy paste error

> And this command seems to operate on all devices, not just the current
> one.
>

Yes, that's why a copy paste

>> +
>> +	printf("\n");
>> +
>> +	for (nand_dev = 0; nand_dev<  CONFIG_SYS_MAX_NAND_DEVICE;
>> nand_dev++) {
>> +
>
> Don't put a blank line inside braces at the beginning/end of blocks.
>

OK

>> +		nand =&nand_info[nand_dev];
>> +		if (!nand->name || !nand->size)
>> +			continue;
>> +
>> +		for (off = 0; off<  nand->size; off += nand->erasesize)
>> {
>> +			int ret;
>> +			void *imgdata;
>> +
>> +			if (nand_block_isbad(nand, off))
>> +				goto next_block;
>> +
>> +			read_size = sizeof(buffer);
>> +
>> +			ret = nand_read(nand, off,&read_size, buffer);
>> +			if (ret<  0&&  ret != -EUCLEAN)
>> +				goto next_block;
>
> s/goto next_block/continue/
>

hmmm, OK.
I copied the original code ie for listing images from nor flash.
Should I also correct it !!

>> +			header = (const image_header_t *)buffer;
>> +
>> +			switch (genimg_get_format(buffer)) {
>> +			case IMAGE_FORMAT_LEGACY:
>> +				if (!image_check_hcrc(header))
>> +					goto next_block;
>> +
>> +				read_size =
>> image_get_image_size(header);
>> +
>> +				imgdata = malloc(read_size);
>> +				if (!imgdata) {
>> +					printf("Not able to list all
>> images " \
>> +						"(Low memory)\n");
>
> Don't line-wrap error strings.
>

80 column ?

>> +					goto next_block;
>> +				}
>> +
>> +				ret = nand_read_skip_bad(nand, off,
>> &read_size,
>> +						imgdata);
>> +				if (ret<  0&&  ret != -EUCLEAN) {
>> +					free(imgdata);
>> +					goto next_block;
>> +				}
>
> s/goto next_block/break/g
>
> ...or better, factor the switch out to its own function.
>

Didn't get that.

>> +
>> +				printf("Legacy Image at NAND device %d
>> " \
>> +					"offset %08llX:\n", nand_dev,
>> off);
>> +				image_print_contents(imgdata);
>> +
>> +				puts("   Verifying Checksum ... ");
>> +				if (!image_check_dcrc(imgdata))
>> +					puts("Bad Data CRC\n");
>> +				else
>> +					puts("OK\n");
>> +
>> +				free(imgdata);
>> +				break;
>> +#if defined(CONFIG_FIT)
>> +			case IMAGE_FORMAT_FIT:
>> +				read_size = fit_get_size(buffer);
>> +
>> +				imgdata = malloc(read_size);
>> +				if (!imgdata) {
>> +					printf("May be a FIT Image at
>> NAND " \
>> +						"device %d offset
>> %08llX:\n",
>> +							nand_dev, off);
>> +					printf("   Low memory(cannot
>> allocate" \
>> +							" memory for
>> image)\n");
>> +					goto next_block;
>
> Why is the no-memory error message different for FIT versus legacy
> images?  I realize that at this point you don't know if it's a FIT
> versus some other dtb, but why do you print the device and offset here
> but not in the legacy case?  Why "Low memory(cannot allocate memory for
> image)" versus just " (Low memory)"?
>

Typo :(
I would give the following print for both
	printf("May be a FIT Image at NAND " \
		"device %d offset %08llX:\n",
			nand_dev, off);
	printf("   Low memory(cannot allocate" \
			" memory for image)\n");

>> +				}
>> +
>> +				ret = nand_read_skip_bad(nand, off,
>> &read_size,
>> +						imgdata);
>> +				if (ret<  0&&  ret != -EUCLEAN) {
>> +					free(imgdata);
>> +					goto next_block;
>> +				}
>> +
>> +				if (!fit_check_format(imgdata)) {
>> +					free(imgdata);
>> +					goto next_block;
>> +				}
>> +
>> +				printf("FIT Image at NAND device %d " \
>> +					"offset %08llX:\n", nand_dev,
>> off);
>> +
>> +				fit_print_contents(imgdata);
>> +				free(imgdata);
>> +				break;
>> +#endif
>> +			default:
>> +				goto next_block;
>> +			}
>
> The default: doesn't do anything -- just leave it out.
>

OK

>> +
>> +next_block:		;
>> +		}
>
> Instead of using goto please factor out the switch to its own function
> and use return.
>

Ah, now I get your point
Thanks

I would send a v3 soon

> -Scott
> .
>