[U-Boot] [PATCH] BOOT: Add RAW ramdisk support to bootz

[Marek Vasut]

Dear Wolfgang Denk,

> Dear Marek Vasut,
> 
> In message <1331857196-29512-1-git-send-email-marex at 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