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

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

On 11/7/2012 5:00 AM, Scott Wood wrote:
> 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.
>

OK. I would correct this in v2

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

You mean I can separate the NOR list images code in one routine and NAND 
in another?

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

Accepted

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

Yes. I was a bit reluctant because I don't know about that format.
OK, I would try to add it in v2

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

OK, I would give it a try. Please wait for v2

> -Scott
>

btw, thanks for the review

How about other patches, Albert, Wolfgang ?

Regards
Vipin