Dear Wolfgang Denk, > Dear Marek Vasut, > > In message <[email protected]> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
