Re: [U-Boot] [Drivers PATCH 17/19] imls: Add support to list images in NAND device

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