[U-Boot-Users] [PATCH] Add 'imload' command
'imload' provides a more direct means to load from an image file.
Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload().
Signed-off-by: Kumar Gala galak@kernel.crashing.org ---
Note, this is against the u-boot-testing new-image branch.
common/cmd_bootm.c | 180 ++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 133 insertions(+), 47 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 2ddb191..0211b86 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -112,6 +112,58 @@ static boot_os_fn do_bootm_artos;
ulong load_addr = CFG_LOAD_ADDR; /* Default Load Address */
+int load_image(image_header_t *hdr, ulong img_data, ulong img_len, ulong dst) +{ + const char *type_name; + uint unc_len = CFG_BOOTM_LEN; + ulong end = 0; + + type_name = image_get_type_name (image_get_type (hdr)); + + switch (image_get_comp (hdr)) { + case IH_COMP_NONE: + if (dst == (ulong)hdr) { + printf (" XIP %s ... ", type_name); + } else { + printf (" Loading %s ... ", type_name); + + memmove_wd ((void *)dst, (void *)img_data, + img_len, CHUNKSZ); + + end = dst + img_len; + } + break; + case IH_COMP_GZIP: + printf (" Uncompressing %s ... ", type_name); + if (gunzip ((void *)dst, unc_len, + (uchar *)img_data, &img_len) != 0) + return -1; + + end = dst + img_len; + break; +#ifdef CONFIG_BZIP2 + case IH_COMP_BZIP2: + printf (" Uncompressing %s ... ", type_name); + /* + * If we've got less than 4 MB of malloc() space, + * use slower decompression algorithm which requires + * at most 2300 KB of memory. + */ + if (BZ2_bzBuffToBuffDecompress ((char*)dst, + &unc_len, (char *)img_data, img_len, + CFG_MALLOC_LEN < (4096 * 1024), 0) != BZ_OK) + return -1; + + end = dst + unc_len; + break; +#endif /* CONFIG_BZIP2 */ + default: + return -2; + } + + puts ("OK\n"); + return end; +}
/*******************************************************************/ /* bootm - boot application image from image in memory */ @@ -120,7 +172,6 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { ulong iflag; const char *type_name; - uint unc_len = CFG_BOOTM_LEN; int verify = getenv_verify();
image_header_t *hdr; @@ -160,61 +211,23 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) image_start = (ulong)hdr; image_end = image_get_image_end (hdr); load_start = image_get_load (hdr); - load_end = 0;
- switch (image_get_comp (hdr)) { - case IH_COMP_NONE: - if (image_get_load (hdr) == (ulong)hdr) { - printf (" XIP %s ... ", type_name); - } else { - printf (" Loading %s ... ", type_name); - - memmove_wd ((void *)image_get_load (hdr), - (void *)os_data, os_len, CHUNKSZ); - - load_end = load_start + os_len; - puts("OK\n"); - } - break; - case IH_COMP_GZIP: - printf (" Uncompressing %s ... ", type_name); - if (gunzip ((void *)image_get_load (hdr), unc_len, - (uchar *)os_data, &os_len) != 0) { - puts ("GUNZIP ERROR - must RESET board to recover\n"); - show_boot_progress (-6); - do_reset (cmdtp, flag, argc, argv); - } + load_end = load_image(hdr, os_data, os_len, load_start);
- load_end = load_start + os_len; - break; -#ifdef CONFIG_BZIP2 - case IH_COMP_BZIP2: - printf (" Uncompressing %s ... ", type_name); - /* - * If we've got less than 4 MB of malloc() space, - * use slower decompression algorithm which requires - * at most 2300 KB of memory. - */ - int i = BZ2_bzBuffToBuffDecompress ((char*)image_get_load (hdr), - &unc_len, (char *)os_data, os_len, - CFG_MALLOC_LEN < (4096 * 1024), 0); - if (i != BZ_OK) { - printf ("BUNZIP2 ERROR %d - must RESET board to recover\n", i); - show_boot_progress (-6); - do_reset (cmdtp, flag, argc, argv); - } + if (load_end == -1) { + printf("%s ERROR - must RESET board to recover\n", type_name); + show_boot_progress (-6); + do_reset (cmdtp, flag, argc, argv); + }
- load_end = load_start + unc_len; - break; -#endif /* CONFIG_BZIP2 */ - default: + if (load_end == -2) { if (iflag) enable_interrupts(); printf ("Unimplemented compression type %d\n", image_get_comp (hdr)); show_boot_progress (-7); return 1; } - puts ("OK\n"); + debug (" kernel loaded at 0x%08lx, end = 0x%08lx\n", load_start, load_end); show_boot_progress (7);
@@ -482,6 +495,79 @@ U_BOOT_CMD( ); #endif
+/*******************************************************************/ +/* imload - load image file */ +/*******************************************************************/ +#if defined(CONFIG_CMD_IMLOAD) +int do_imload (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) +{ + ulong addr = load_addr; + int end; + int verify = getenv_verify(); + char buf[12]; + image_header_t *h; + + if (argc > 1) + addr = simple_strtoul(argv[1], NULL, 16); + + h = (image_header_t *)addr; + + if (!image_check_magic (h)) { + puts (" Bad Magic Number\n"); + return 1; + } + + if (!image_check_hcrc (h)) { + puts (" Bad Header Checksum\n"); + return 1; + } + + print_image_hdr(h); + + if (verify) { + puts (" Verifying Checksum ... "); + if (!image_check_dcrc (h)) { + printf ("Bad Data CRC\n"); + return 1; + } + puts ("OK\n"); + } + + if (argc == 3) + addr = simple_strtoul(argv[2], NULL, 16); + else + addr = image_get_load(h); + + printf("\n Loading image at %08x\n", addr); + + end = load_image(h, image_get_data(h), image_get_data_size(h), addr); + + if (end > 0) { + sprintf(buf, "%lX", addr); + setenv("fileaddr", buf); + sprintf(buf, "%lX", end - addr); + setenv("filesize", buf); + return 0; + } + + if (end == -1) + puts("\n Decompression Error\n"); + + return 1; +} + +U_BOOT_CMD( + imload, 3, 1, do_imload, + "imload - load image from application image\n", + "[addr] [load addr]\n" + " - load image from application image located at address 'addr'\n" + " or $loadaddr if 'addr' is not provided to either the load\n" + " address specified in the application image header or the\n" + " 'load addr' argument; this includes verification of the\n" + " image contents (magic number, header and payload checksums)\n" +); +#endif +
/*******************************************************************/ /* imls - list all images found in flash */
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file.
Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload().
Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments: - The load_image routine (and consequently imload commad) will not work when the image is stored in Data Flash. - The code as-is will clearly not work with the new image format -- how about we wait a few days for the new format patchset I've mentioned in my previous email today? Note that the patchset will have a routine that you could use to deal with the Data Flash scenario.
Regards, Bartlomiej
On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file. Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload(). Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments:
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
- The code as-is will clearly not work with the new image format --
how about we wait a few days for the new format patchset I've mentioned in my previous email today? Note that the patchset will have a routine that you could use to deal with the Data Flash scenario.
I'm concerned about how long it will be before people adopt the new image format. Also, do you at least have a spec for the new image format?
- k
Kumar Gala wrote:
On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file. Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload(). Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments:
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
- The code as-is will clearly not work with the new image format --
how about we wait a few days for the new format patchset I've mentioned in my previous email today? Note that the patchset will have a routine that you could use to deal with the Data Flash scenario.
I'm concerned about how long it will be before people adopt the new image format. Also, do you at least have a spec for the new image format?
There has been quite a bit information on the new image format posted to the list, please refer to the following threads: http://www.nabble.com/RFC%3A-New-U-boot-image-format-to14277371.html#a144179... http://www.nabble.com/RFC%3A-New-uImage-format-bindings-to14417699.html#a144... http://www.nabble.com/RFC%3A-new-bootm-syntax-to14594482.html#a14594482
Please let me know if you have any comments or questions.
Regards, Bartlomiej
On Feb 13, 2008, at 1:55 PM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file. Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload(). Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments:
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
Just to be clear, at a quick glance, I assume load_image will work ok from do_bootm() but not from do_imload(). (since do_bootm is calling get_kernel).
- The code as-is will clearly not work with the new image format
-- how about we wait a few days for the new format patchset I've mentioned in my previous email today? Note that the patchset will have a routine that you could use to deal with the Data Flash scenario.
I'm concerned about how long it will be before people adopt the new image format. Also, do you at least have a spec for the new image format?
There has been quite a bit information on the new image format posted to the list, please refer to the following threads: http://www.nabble.com/RFC%3A-New-U-boot-image-format-to14277371.html#a144179...
http://www.nabble.com/RFC%3A-New-uImage-format-bindings-to14417699.html#a144...
this one seem to have what I'm looking for, for some reason I didn't see it.
http://www.nabble.com/RFC%3A-new-bootm-syntax- to14594482.html#a14594482
Please let me know if you have any comments or questions.
- k
Kumar Gala wrote: [...]
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
Just to be clear, at a quick glance, I assume load_image will work ok from do_bootm() but not from do_imload(). (since do_bootm is calling get_kernel).
Correct.
Regards, Bartlomiej
On Feb 13, 2008, at 2:22 PM, Bartlomiej Sieka wrote:
Kumar Gala wrote: [...]
- The load_image routine (and consequently imload commad) will
not work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
Just to be clear, at a quick glance, I assume load_image will work ok from do_bootm() but not from do_imload(). (since do_bootm is calling get_kernel).
Correct.
Ok, I don't know much about the DATAFLASH device, but is it any different than reading from something like NAND or a HD? It seems like we shouldn't burden the code w/providing special handling for accessing it if its not directly memory accessible like NOR flash is.
- k
On Feb 13, 2008 12:55 PM, Bartlomiej Sieka tur@semihalf.com wrote:
Kumar Gala wrote:
On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file. Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload(). Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments:
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
Ugh, please don't continue down that path. Dataflash is a serial flash technology, but the driver pretends that it is memory mapped. It is not a good abstraction that I really think needs to be removed. I don't think it is a good idea to add that mis-feature into new commands.
Cheers, g.
On Feb 13, 2008, at 7:13 PM, Grant Likely wrote:
On Feb 13, 2008 12:55 PM, Bartlomiej Sieka tur@semihalf.com wrote:
Kumar Gala wrote:
On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file. Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload(). Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments:
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
Ugh, please don't continue down that path. Dataflash is a serial flash technology, but the driver pretends that it is memory mapped. It is not a good abstraction that I really think needs to be removed. I don't think it is a good idea to add that mis-feature into new commands.
agreed. I don't plan on supporting it.
- k
Grant Likely wrote:
On Feb 13, 2008 12:55 PM, Bartlomiej Sieka tur@semihalf.com wrote:
Kumar Gala wrote:
On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
Kumar Gala wrote:
'imload' provides a more direct means to load from an image file. Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload(). Signed-off-by: Kumar Gala galak@kernel.crashing.org
Note, this is against the u-boot-testing new-image branch.
Thanks.
Two comments:
- The load_image routine (and consequently imload commad) will not
work when the image is stored in Data Flash.
what's the issue here?
Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel() (formerly in do_bootm()), especially the read_dataflash() function. The issue is that you have to copy data from Data Flash in a specific way in order to have random access to it. So for example this line in your code: type_name = image_get_type_name (image_get_type (hdr)); will effectively try to access hdr->ih_type, which will not work when hdr is an address in Data Flash.
Ugh, please don't continue down that path. Dataflash is a serial flash technology, but the driver pretends that it is memory mapped. It is not a good abstraction that I really think needs to be removed.
Hi Grant,
Not sure if your comment was directed to Kumar or me, but I'm replying just in case.
I'm all for removing the code under CONFIG_HAS_DATAFLASH from at least boot-related functions (haven't looked at other affected places in U-Boot). We've not removed it in the new image format work, because by default we try to preserve the status quo. When the new format handling is introduced, we don't want to break too many things that people are used to.
Regards, Bartlomiej
Hi Grant,
Ugh, please don't continue down that path. Dataflash is a serial flash technology, but the driver pretends that it is memory mapped. It is not a good abstraction that I really think needs to be removed. I don't think it is a good idea to add that mis-feature into new commands.
Seconded. That abstraction always irritated me a lot and made the code only harder to read.
Cheers Detlev
In message Pine.LNX.4.64.0802122307330.22593@blarg.am.freescale.net you wrote:
'imload' provides a more direct means to load from an image file.
What exactly is an "image file" here (which sorts of images are allowed?) and what does "load" mean (which actions are performed on the file)?
Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload().
Hm... bootm is restrictred to a certain sub-set of image types. imload on the other hand sounds pretty generic to me and should not have such restrictions.
- switch (image_get_comp (hdr)) {
- case IH_COMP_NONE:
if (dst == (ulong)hdr) {printf (" XIP %s ... ", type_name);
This is an ugly hack in the old code that should be removed by the new implementation.
- "imload - load image from application image\n",
- "[addr] [load addr]\n"
What exactly is an "application image" ?
- " - load image from application image located at address 'addr'\n"
- " or $loadaddr if 'addr' is not provided to either the load\n"
- " address specified in the application image header or the\n"
- " 'load addr' argument; this includes verification of the\n"
- " image contents (magic number, header and payload checksums)\n"
Please be terse. We don't want to spend the memory footprint on prosa.
Best regards,
Wolfgang Denk
On Feb 13, 2008, at 4:31 PM, Wolfgang Denk wrote:
In message <Pine.LNX. 4.64.0802122307330.22593@blarg.am.freescale.net> you wrote:
'imload' provides a more direct means to load from an image file.
What exactly is an "image file" here (which sorts of images are allowed?) and what does "load" mean (which actions are performed on the file)?
its suppose to be a 'uImage' not sure what the base way to refer to that is.
Also created a load_image routine out of the code in do_bootm() that is shared between do_bootm() and do_imload().
Hm... bootm is restrictred to a certain sub-set of image types. imload on the other hand sounds pretty generic to me and should not have such restrictions.
what other image formats should we be supporting?
- switch (image_get_comp (hdr)) {
- case IH_COMP_NONE:
if (dst == (ulong)hdr) {printf (" XIP %s ... ", type_name);This is an ugly hack in the old code that should be removed by the new implementation.
- "imload - load image from application image\n",
- "[addr] [load addr]\n"
What exactly is an "application image" ?
I stole this from iminfo help.
- " - load image from application image located at address
'addr'\n"
- " or $loadaddr if 'addr' is not provided to either the load\n"
- " address specified in the application image header or the\n"
- " 'load addr' argument; this includes verification of the\n"
- " image contents (magic number, header and payload checksums)
\n"
Please be terse. We don't want to spend the memory footprint on prosa.
no problem :)
- k
In message 7DB626E7-34A4-4EAA-B470-E2CC49F97CA3@kernel.crashing.org you wrote:
What exactly is an "image file" here (which sorts of images are allowed?) and what does "load" mean (which actions are performed on the file)?
its suppose to be a 'uImage' not sure what the base way to refer to that is.
Why just uImage?
what other image formats should we be supporting?
All?
What exactly is an "application image" ?
I stole this from iminfo help.
Argh... where is the brown paper bag?
Best regards,
Wolfgang Denk
On Feb 13, 2008, at 5:01 PM, Wolfgang Denk wrote:
In message <7DB626E7-34A4-4EAA-B470- E2CC49F97CA3@kernel.crashing.org> you wrote:
What exactly is an "image file" here (which sorts of images are allowed?) and what does "load" mean (which actions are performed on the file)?
its suppose to be a 'uImage' not sure what the base way to refer to that is.
Why just uImage?
what other image formats do we support? I assume we'll extend it for the new format style once that exists.
what other image formats should we be supporting?
All?
I feel like I'm missing something here. I agree that IH_TYPE_MULTI needs to be supported, but I don't see why the code doesn't work for the bulk of the other IH_TYPE_* variants. It appears the only compression types we support today ore NONE, GZIP, and BZIP2.
So what am I missing?
What exactly is an "application image" ?
I stole this from iminfo help.
Argh... where is the brown paper bag?
:)
- k
In message 3192541E-1FBD-4638-A317-3A25F316CD8C@kernel.crashing.org you wrote:
Why just uImage?
what other image formats do we support? I assume we'll extend it for the new format style once that exists.
uImage means "OS Kernel Image" (IH_TYPE_KERNEL in include/image.h).
I feel like I'm missing something here. I agree that IH_TYPE_MULTI needs to be supported, but I don't see why the code doesn't work for the bulk of the other IH_TYPE_* variants. It appears the only compression types we support today ore NONE, GZIP, and BZIP2.
So what am I missing?
I think it makes sense to allow support for Standalone Program, RAMDisk, Multi-File, Firmware, Script, Filesystem, and FDT images as well.
Best regards,
Wolfgang Denk
On Feb 13, 2008, at 6:37 PM, Wolfgang Denk wrote:
In message <3192541E-1FBD-4638- A317-3A25F316CD8C@kernel.crashing.org> you wrote:
Why just uImage?
what other image formats do we support? I assume we'll extend it for the new format style once that exists.
uImage means "OS Kernel Image" (IH_TYPE_KERNEL in include/image.h).
What's the term to specify an image wrapped via mkimage?
I feel like I'm missing something here. I agree that IH_TYPE_MULTI needs to be supported, but I don't see why the code doesn't work for the bulk of the other IH_TYPE_* variants. It appears the only compression types we support today ore NONE, GZIP, and BZIP2.
So what am I missing?
I think it makes sense to allow support for Standalone Program, RAMDisk, Multi-File, Firmware, Script, Filesystem, and FDT images as well.
agreed, I think the only one I don't support right now is multi- image. (however, I'm not sure what an image of type Filesystem actually means).
- k
- switch (image_get_comp (hdr)) {
- case IH_COMP_NONE:
if (dst == (ulong)hdr) {printf (" XIP %s ... ", type_name);This is an ugly hack in the old code that should be removed by the new implementation.
can you explain this further, I'm happy to git rid of it, just need to understand the hack.
- k
participants (5)
-
Bartlomiej Sieka -
Detlev Zundel -
Grant Likely -
Kumar Gala -
Wolfgang Denk