[U-Boot] [PATCH V3] BOOT: Add "bootz" command to boot Linux zImage

[Wolfgang Denk]

Dear Marek Vasut,

In message <1331588061-21546-1-git-send-email-marex at denx.de> you wrote:
> 
> This command boots Linux zImage from where the zImage is loaded to. Passing
> initrd and fdt is supported.

I have but a few formal questions / issues.

...
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -160,35 +160,8 @@ static boot_os_fn *boot_os[] = {
>  
>  bootm_headers_t images;		/* pointers to os/initrd/fdt images */
>  
> -/* Allow for arch specific config before we boot */
> -void __arch_preboot_os(void)
> -{
> -	/* please define platform specific arch_preboot_os() */
> -}
> -void arch_preboot_os(void) __attribute__((weak, alias("__arch_preboot_os")));
> -
>  #define IH_INITRD_ARCH IH_ARCH_DEFAULT
>  
> -static void bootm_start_lmb(void)
> -{
...

Note: this is bootm (or boot*) related code.


> --- a/common/image.c
> +++ b/common/image.c
> @@ -3190,3 +3190,28 @@ static int fit_check_ramdisk(const void *fit, int rd_noffset, uint8_t arch,

This is a file that deals with U-Boot images etc.  It is in no way
related to any of the "boot*" commands.

> +#ifdef	CONFIG_LMB
> +void boot_start_lmb(bootm_headers_t *images)
> +{
> +	ulong		mem_start;
> +	phys_size_t	mem_size;
> +
> +	lmb_init(&images->lmb);
> +
> +	mem_start = getenv_bootm_low();
> +	mem_size = getenv_bootm_size();
> +
> +	lmb_add(&images->lmb, (phys_addr_t)mem_start, mem_size);
> +
> +	arch_lmb_reserve(&images->lmb);
> +	board_lmb_reserve(&images->lmb);
> +}
> +#endif
> +
> +/* Allow for arch specific config before we boot */
> +void __arch_preboot_os(void)
> +{
> +	/* please define platform specific arch_preboot_os() */
> +}
> +void arch_preboot_os(void) __attribute__((weak, alias("__arch_preboot_os")));

Putting this code here makes no sense.  You could chose any othewr
random file as well.

> diff --git a/include/image.h b/include/image.h
> index bbf80f0..26b8a20 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -486,6 +486,14 @@ void image_multi_getimg(const image_header_t *hdr, ulong idx,
>  
>  void image_print_contents(const void *hdr);
>  
> +#ifdef CONFIG_LMB
> +void boot_start_lmb(bootm_headers_t *images);
> +#else
> +static inline void boot_start_lmb(bootm_headers_t *image) { }
> +#endif
> +
> +extern void arch_preboot_os(void);

Sorry, no.  image.[ch] is definitely not the right place for this
stuff.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many QA engineers does it take to screw in a lightbulb? 3:  1  to
screw it in and 2 to say "I told you so" when it doesn't work.