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

[Scott Wood <scottwood@freescale.com>]

On 12/12/2012 03:20:24 AM, Vipin Kumar wrote:
> imls does not list the images in NAND devices. This patch implements  
> this
> support for legacy type images.
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
> Hello Scott,
> 
> There has been sometime since you reviewed the first version of this  
> patch.
> http://lists.denx.de/pipermail/u-boot/2012-November/139631.html
> 
> I am sorry for a late response on this. I was waiting for other  
> comments if any
> on the whole patch-set
> 
> Regards
> Vipin
> 
>  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/

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

>  		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/

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

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

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

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

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

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

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

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

> +				}
> +
> +				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.

> +
> +next_block:		;
> +		}

Instead of using goto please factor out the switch to its own function  
and use return.

-Scott