[U-Boot] [PATCH] image: Implement IH_TYPE_KERNEL_REL

[Wolfgang Denk]

Dear Stephen Warren,

In message <1317940584-19528-1-git-send-email-swarren at nvidia.com> you wrote:
> uImage files contain absolute "load" and "entry" addresses. Such a concept
> is incompatible with using the same kernel image on multiple SoCs, each with
> a potentially different SDRAM base. To support that, create a new image type
> IH_TYPE_KERNEL_REL, which is handled identically to IH_TYPE_KERNEL, except
> that the "load" and "entry" properties are an offset from the base of SDRAM,
> rather than an absolute address.
> 
> "An offset from the base of SDRAM" is specified as:
> a) CONFIG_SYS_SDRAM_BASE, if set.
> b) Otherwise, for ARM, the start address of the first bank of SDRAM known
>    to U-Boot.
> c) Otherwise, 0.

I agree with Kumar: it should be sufficient to have this omment in
the code.  But please define "first bank" - does it mena the firs one
initialized, or the lowest start address, or the lowest chip select,
or ... ?


Also, I think you are right, and we should add IH_TYPE_FLATDT_REL as
well.

...
>  	/* Remaining, type dependent properties */
>  	if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>  	    (type == IH_TYPE_RAMDISK) || (type == IH_TYPE_FIRMWARE) ||
> -	    (type == IH_TYPE_FLATDT)) {
> +	    (type == IH_TYPE_FLATDT) || (type == IH_TYPE_KERNEL_REL)) {

Can we please re-arrange this a bit, like that:

	if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_KERNEL_REL) ||
	    (type == IH_TYPE_FLATDT) || (type == IH_TYPE_FLATDT_REL) ||
	    (type == IH_TYPE_RAMDISK) || (type == IH_TYPE_FIRMWARE)  ||
	    (type == IH_TYPE_STANDALONE)) {
...

> +		(type == IH_TYPE_FIRMWARE) || (type == IH_TYPE_KERNEL_REL)) {
> +		ret = fit_image_get_load_raw (fit, image_noffset, &raw);
> +		ret |= fit_image_get_load_abs (fit, image_noffset, &abs);

This looks strange to me.  Please add at least a comment what's going
on here.

>  		if (ret)
>  			printf ("unavailable\n");
> -		else
> -			printf ("0x%08lx\n", load);
> +		else {

Need braces in both branches.

>  		if (ret)
>  			printf ("unavailable\n");
> -		else
> -			printf ("0x%08lx\n", entry);
> +		else {

Ditto.

> - * fit_image_get_load - get load address property for a given component image node
> + * fit_image_get_load_raw - get raw load address property for a given component image node

Line too long. [Did you run checkpatch?]

> +/**
> + * fit_image_get_entry_raw - get raw entry point address property for a given component image node

Incorrect multiline comment style.


Hm... this appears to add some additional code.  How much does the
size grow?

Can we make support for relative images optional?

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
Only a fool fights in a burning house.
	-- Kank the Klingon, "Day of the Dove", stardate unknown