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