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

[Scott Wood]

On 12/13/2012 12:10:58 AM, Vipin Kumar wrote:
>> 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

Still, it might be better to fix that than to build upon a  
no-longer-accurate assumption.

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

Do you really need 512 bytes for fdt_check_header() to work?

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

If you want to do that as a separate patch, that's fine -- but the code  
is sufficiently different that I don't think there's a strong  
consistency argument to be made.

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

Error strings are an exception for the sake of greppability.  From  
Linux's Documentation/CodingStyle:

   Statements longer than 80 columns will be broken into sensible  
chunks, unless
   exceeding 80 columns significantly increases readability and does  
not hide
   information. Descendants are always substantially shorter than the  
parent and
   are placed substantially to the right. The same applies to function  
headers
   with a long argument list. However, never break user-visible strings  
such as
   printk messages, because that breaks the ability to grep for them.

>> 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");

It's a little more verbose than I'd have done, but OK.  Can you put a  
space between "memory" and "(cannot", though?

-Scott