| Message ID | 1331857196-29512-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
Dear Marek Vasut, In message <1331857196-29512-1-git-send-email-marex@denx.de> you wrote: > This patch allows loading RAW ramdisk via bootz command. The raw ramdisk is > loaded only in case it's size is specified: > > bootz <kernel addr> <ramdisk addr>:<ramdisk size> <fdt addr> > > For example: > > bootz 0x42000000 0x43000000:0x12345 0x44000000 ... > --- a/common/image.c > +++ b/common/image.c > @@ -797,6 +797,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, > ulong rd_addr, rd_load; > ulong rd_data, rd_len; > const image_header_t *rd_hdr; > + char *end; > #if defined(CONFIG_FIT) > void *fit_hdr; > const char *fit_uname_config = NULL; > @@ -845,10 +846,18 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, > } else > #endif > { > - rd_addr = simple_strtoul(argv[2], NULL, 16); > + rd_addr = simple_strtoul(argv[2], &end, 16); > debug("* ramdisk: cmdline image address = " > "0x%08lx\n", > rd_addr); > + > + if (end[0] == ':') { > + rd_len = simple_strtoul(++end, > + NULL, 16); > + debug("* ramdisk: cmdline image " > + "length = 0x%08lx\n", > + rd_len); > + } This is common code, which gets used not only for bootz. I see a number of problems: - You add code size even for systems which don't enable "bootz" support. - You change the syntax of all boot related commands where a ramdisk address may be passed (i. e. "bootm") without documenting it. - Using this syntax with a mkimage wrapped ramdisk image will cause bad things to happen, even if you specific the correct size. The API should be more robust. > + /* > + * Check if rd_len was manually overridden, if it was, > + * we're loading RAW ramdisk. > + */ This is undocumented policy, and I dislike such a side-effect based design (if we have a size parameter, it must be a raw image). Best regards, Wolfgang Denk
Dear Wolfgang Denk, > Dear Marek Vasut, > > In message <1331857196-29512-1-git-send-email-marex@denx.de> you wrote: > > This patch allows loading RAW ramdisk via bootz command. The raw ramdisk > > is > > > > loaded only in case it's size is specified: > > bootz <kernel addr> <ramdisk addr>:<ramdisk size> <fdt addr> > > > > For example: > > bootz 0x42000000 0x43000000:0x12345 0x44000000 > > ... > > > --- a/common/image.c > > +++ b/common/image.c > > @@ -797,6 +797,7 @@ int boot_get_ramdisk(int argc, char * const argv[], > > bootm_headers_t *images, > > > > ulong rd_addr, rd_load; > > ulong rd_data, rd_len; > > const image_header_t *rd_hdr; > > > > + char *end; > > > > #if defined(CONFIG_FIT) > > > > void *fit_hdr; > > const char *fit_uname_config = NULL; > > > > @@ -845,10 +846,18 @@ int boot_get_ramdisk(int argc, char * const argv[], > > bootm_headers_t *images, > > > > } else > > > > #endif > > > > { > > > > - rd_addr = simple_strtoul(argv[2], NULL, 16); > > + rd_addr = simple_strtoul(argv[2], &end, 16); > > > > debug("* ramdisk: cmdline image address = " > > > > "0x%08lx\n", > > rd_addr); > > > > + > > + if (end[0] == ':') { > > + rd_len = simple_strtoul(++end, > > + NULL, 16); > > + debug("* ramdisk: cmdline image " > > + "length = 0x%08lx\n", > > + rd_len); > > + } > > This is common code, which gets used not only for bootz. > > I see a number of problems: > > - You add code size even for systems which don't enable "bootz" > support. This is a good point, do you agree to introduce CONFIG_SUPPORT_RAW_RAMDISK to enable this feature? > > - You change the syntax of all boot related commands where a ramdisk > address may be passed (i. e. "bootm") without documenting it. Right, this should be documented in the top-level README file? Or is there some other place? > > - Using this syntax with a mkimage wrapped ramdisk image will cause > bad things to happen, even if you specific the correct size. The > API should be more robust. It actually won't. If you use mkimage warped ramdisk, it'll recognise it's magic numbers and simply ignore the stuff behind ":". Though I hope noone in sane mind would do that. > > > + /* > > + * Check if rd_len was manually overridden, if it was, > > + * we're loading RAW ramdisk. > > + */ > > This is undocumented policy, and I dislike such a side-effect based > design (if we have a size parameter, it must be a raw image). It must not, but if it is, it'll be used as raw. > > Best regards, > > Wolfgang Denk Thanks for good ideas! Best regards, Marek Vasut
Dear Marek Vasut, In message <201203160945.40467.marex@denx.de> you wrote: > > > - You add code size even for systems which don't enable "bootz" > > support. > > This is a good point, do you agree to introduce CONFIG_SUPPORT_RAW_RAMDISK to > enable this feature? Fine with me - please also document the new option. > > - You change the syntax of all boot related commands where a ramdisk > > address may be passed (i. e. "bootm") without documenting it. > > Right, this should be documented in the top-level README file? Or is there some > other place? Hm... I have no strong opinion here. > > - Using this syntax with a mkimage wrapped ramdisk image will cause > > bad things to happen, even if you specific the correct size. The > > API should be more robust. > > It actually won't. If you use mkimage warped ramdisk, it'll recognise it's magic > numbers and simply ignore the stuff behind ":". Though I hope noone in sane mind > would do that. Argh... Passing arguments that silently get ignored under certain conditions is not something I like to see. And are you sure hat it will work like that? [I mean, has this been tested?] > > > + /* > > > + * Check if rd_len was manually overridden, if it was, > > > + * we're loading RAW ramdisk. > > > + */ > > > > This is undocumented policy, and I dislike such a side-effect based > > design (if we have a size parameter, it must be a raw image). > > It must not, but if it is, it'll be used as raw. Sorry, I cannot parse this. Best regards, Wolfgang Denk
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 9efac8b..872a49c 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1628,9 +1628,11 @@ static int do_bootz(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) U_BOOT_CMD( bootz, CONFIG_SYS_MAXARGS, 1, do_bootz, "boot Linux zImage image from memory", - "[addr [initrd] [fdt]]\n - boot Linux zImage stored in memory\n" + "[addr [initrd[:size]] [fdt]]\n" + " - boot Linux zImage stored in memory\n" "\tThe argument 'initrd' is optional and specifies the address\n" - "\tof the initrd in memory.\n" + "\tof the initrd in memory. The optional argument ':size' allows\n" + "\tspecifying the size of RAW initrd.\n" #if defined(CONFIG_OF_LIBFDT) "\tWhen booting a Linux kernel which requires a flat device-tree\n" "\ta third argument is required which is the address of the\n" diff --git a/common/image.c b/common/image.c index 95c7a15..e9d87d8 100644 --- a/common/image.c +++ b/common/image.c @@ -797,6 +797,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, ulong rd_addr, rd_load; ulong rd_data, rd_len; const image_header_t *rd_hdr; + char *end; #if defined(CONFIG_FIT) void *fit_hdr; const char *fit_uname_config = NULL; @@ -845,10 +846,18 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, } else #endif { - rd_addr = simple_strtoul(argv[2], NULL, 16); + rd_addr = simple_strtoul(argv[2], &end, 16); debug("* ramdisk: cmdline image address = " "0x%08lx\n", rd_addr); + + if (end[0] == ':') { + rd_len = simple_strtoul(++end, + NULL, 16); + debug("* ramdisk: cmdline image " + "length = 0x%08lx\n", + rd_len); + } } #if defined(CONFIG_FIT) } else { @@ -990,9 +999,17 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, break; #endif default: - puts("Wrong Ramdisk Image Format\n"); - rd_data = rd_len = rd_load = 0; - return 1; + /* + * Check if rd_len was manually overridden, if it was, + * we're loading RAW ramdisk. + */ + if (rd_len != 0) { + rd_data = rd_addr; + } else { + puts("Wrong Ramdisk Image Format\n"); + rd_data = rd_len = rd_load = 0; + return 1; + } } } else if (images->legacy_hdr_valid && image_check_type(&images->legacy_hdr_os_copy,
This patch allows loading RAW ramdisk via bootz command. The raw ramdisk is loaded only in case it's size is specified: bootz <kernel addr> <ramdisk addr>:<ramdisk size> <fdt addr> For example: bootz 0x42000000 0x43000000:0x12345 0x44000000 Signed-off-by: Marek Vasut <marex@denx.de> Cc: Tom Warren <TWarren@nvidia.com> Cc: albert.u.boot@aribaud.net Cc: afleming@gmail.com, Cc: Simon Glass <sjg@chromium.org>, Cc: Stephen Warren <swarren@nvidia.com> Cc: Nicolas Pitre <nico@fluxnic.net> Cc: Wolfgang Denk <wd@denx.de> Cc: Detlev Zundel <dzu@denx.de> --- common/cmd_bootm.c | 6 ++++-- common/image.c | 25 +++++++++++++++++++++---- 2 files changed, 25 insertions(+), 6 deletions(-) NOTE: This patch depends on previous zImage/bootz patch. Testers are very welcome!