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

[Vipin Kumar <vipin.kumar@st.com>]

On 12/14/2012 3:22 AM, Scott Wood wrote:
> 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.
>

OK, I would try that in v4

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

I have reduced it already to 64 bytes in v3

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

Check if it is OK in v3

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

Yes, thanks for reminding. The error strings are more readable already 
in v3. Please take a look

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

Sure. I will do that in v4

-Vipin