From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Tue, 6 Nov 2012 17:30:26 -0600
Subject: [U-Boot] [Drivers PATCH 17/19] imls: Add support to list images
 in NAND device
In-Reply-To: <09794284905d5ef15a117c85b9db44092402317a.1351877124.git.vipin.kumar@st.com>
	(from vipin.kumar@st.com on Fri Nov  2 12:40:02 2012)
Message-ID: <1352244626.21833.12@snotra>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 11/02/2012 12:40:02 PM, 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>
> ---
>  common/cmd_bootm.c | 98  
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 83fa5d7..ca3c430 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -62,6 +62,11 @@
>  #include <linux/lzo.h>
>  #endif /* CONFIG_LZO */
> 
> +#if defined(CONFIG_CMD_NAND)
> +#include <linux/err.h>
> +#include <nand.h>
> +#endif

You shouldn't need to ifdef-protect header files.

>  DECLARE_GLOBAL_DATA_PTR;
> 
>  #ifndef CONFIG_SYS_BOOTM_LEN
> @@ -1221,6 +1226,99 @@ next_sector:		;
>  next_bank:	;
>  	}
> 
> +#if defined(CONFIG_CMD_NAND)
> +	printf("\n");
> +	nand_info_t *nand;
> +	image_header_t image_header;
> +	image_header_t *header = &image_header;
> +	int nand_dev = nand_curr_device;
> +	unsigned long img_size;
> +	size_t hdr_size, read_len;
> +	loff_t off;
> +	unsigned int crc;
> +	u_char *data;
> +
> +	/* 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 0;
> +	}

Please move the NAND and NOR code into their own functions.

> +
> +	for (nand_dev = 0; nand_dev < CONFIG_SYS_MAX_NAND_DEVICE;  
> nand_dev++) {
> +
> +		nand = &nand_info[nand_dev];
> +		if ((!nand->name) || (!nand->size))
> +			continue;

Redundant parentheses.

> +		data = malloc(nand->writesize);
> +		if (!data) {
> +			puts("No memory available to list NAND  
> images\n");
> +			return 0;
> +		}
> +
> +		for (off = 0; off < nand->size; off += nand->erasesize)  
> {
> +			int ret;
> +
> +			if (nand_block_isbad(nand, off))
> +				continue;
> +
> +			hdr_size = sizeof(image_header_t);
> +			ret = nand_read(nand, off, &hdr_size, (u_char  
> *)header);
> +			if (ret < 0 && ret != -EUCLEAN)
> +				continue;

I guess you don't use nand_read_skip_bad() because you don't want to  
allocate a buffer for the whole image...  How about moving this code to  
nand_util.c?  That would at least allow some refactoring rather than  
duplication.

> +			if (!image_check_hcrc(header))
> +				continue;
> +
> +			printf("Legacy Image at NAND device %d offset  
> %08lX:\n",
> +					nand_dev, (ulong)off);
> +			image_print_contents(header);

Shouldn't you check for FIT images as well?

> +			puts("   Verifying Checksum ... ");
> +			crc = 0;
> +			img_size = ntohl(header->ih_size);
> +			img_size += hdr_size;
> +
> +			while (img_size > 0) {
> +				int blockoff = 0;
> +
> +				while (nand_block_isbad(nand, off)) {
> +					off += nand->erasesize;
> +					if (off >= nand->size)
> +						goto out;
> +				}
> +
> +				do {
> +					read_len = min(img_size,
> +							 
> nand->writesize);
> +					ret = nand_read(nand, off +  
> blockoff,
> +							&read_len,  
> data);
> +					if (ret < 0 && ret != -EUCLEAN)
> +						break;
> +
> +					crc = crc32(crc, data +  
> hdr_size,
> +							read_len -  
> hdr_size);
> +					img_size -= read_len;
> +					blockoff += read_len;
> +					hdr_size = 0;
> +				} while (img_size &&
> +						(blockoff <  
> nand->erasesize));
> +
> +				off += nand->erasesize;
> +				if (off >= nand->size)
> +					goto out;
> +			}
> +			off -= nand->erasesize;
> +out:
> +			if (crc != ntohl(header->ih_dcrc))
> +				puts("   Bad Data CRC\n");
> +			else
> +				puts("OK\n");
> +		}

Please refactor this into separate functions to improve readability.   
Maybe put a nand_crc_skip_bad() function into nand_util.c?

-Scott