[U-Boot] [PATCH 1/5] tools: mkimage: add support for gpimage format

[Wolfgang Denk]

Dear Murali Karicheri,

In message <1390255810-19486-2-git-send-email-m-karicheri2 at ti.com> you wrote:
> This patch add support for gpimage format as a preparatory
> patch for porting u-boot for keystone2 devices and is
> based on omapimage format. It re-uses gph header to store the
> size and loadaddr as done in omapimage.c
...
> --- a/common/image.c
> +++ b/common/image.c
> @@ -137,6 +137,7 @@ static const table_entry_t uimage_type[] = {
>  	{	IH_TYPE_STANDALONE, "standalone", "Standalone Program", },
>  	{	IH_TYPE_UBLIMAGE,   "ublimage",   "Davinci UBL image",},
>  	{	IH_TYPE_MXSIMAGE,   "mxsimage",   "Freescale MXS Boot Image",},
> +	{	IH_TYPE_GPIMAGE,    "gpimage",    "TI KeyStone SPL Image",},
>  	{	-1,		    "",		  "",			},

Please keep the list sorted.  While you are at it, please also fi the
incorrect placement of the IH_TYPE_MXSIMAGE entry.

> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -84,7 +84,9 @@ NOPED_OBJ_FILES-y += imagetool.o
>  NOPED_OBJ_FILES-y += mkenvimage.o
>  NOPED_OBJ_FILES-y += mkimage.o
>  NOPED_OBJ_FILES-y += mxsimage.o
> +NOPED_OBJ_FILES-y += gpimage-common.o
>  NOPED_OBJ_FILES-y += omapimage.o
> +NOPED_OBJ_FILES-y += gpimage.o
>  NOPED_OBJ_FILES-y += os_support.o

Please keep the list sorted.

> @@ -219,7 +221,9 @@ $(obj)dumpimage$(SFX):	$(obj)aisimage.o \
>  			$(obj)dumpimage.o \
>  			$(obj)md5.o \
>  			$(obj)mxsimage.o \
> +			$(obj)gpimage-common.o \
>  			$(obj)omapimage.o \
> +			$(obj)gpimage.o \

Ditto.  Please fix this issue globally.


> +uint32_t gpimage_swap32(uint32_t data)
> +{
> +	return cpu_to_be32(data);
> +}

The function name is misleading.  Also it is not clear why you need
this function at all.  You can use cpu_to_be32() directly - that will
make the code much easier to read.

> +/* TODO: do we need the below 2 functions?? */
> +int valid_gph_size(uint32_t size)
> +{
> +	return size;
> +}
> +
> +int valid_gph_load_addr(uint32_t load_addr)
> +{
> +	return load_addr;
> +}

These functions are obviously bogus.  Please dump them.

> +int gph_verify_header(struct gp_header *gph, int do_swap32)
> +{
> +	uint32_t gph_size, gph_load_addr;
> +
> +	if (do_swap32) {
> +		gph_size = gpimage_swap32(gph->size);
> +		gph_load_addr = gpimage_swap32(gph->load_addr);
> +	} else {
> +		gph_size = gph->size;
> +		gph_load_addr = gph->load_addr;
> +	}

I think it should be possible top write this code in such a way that
you can avoid both the if-else and passing the  do_swap32  parameter.
It is my impression that the whole endianess handling needs some
refinemant.

Actually I cannot see a place where do_swap32=0 is used..

> +void gph_print_header(const struct gp_header *gph, int do_swap32)
> +{
> +	uint32_t gph_size, gph_load_addr;
> +
> +	if (do_swap32) {
> +		gph_size = gpimage_swap32(gph->size);
> +		gph_load_addr = gpimage_swap32(gph->load_addr);
> +	} else {
> +		gph_size = gph->size;
> +		gph_load_addr = gph->load_addr;
> +	}

This is repeasted code.  Get rid of it.

> +void gph_set_header(struct gp_header *gph, uint32_t size, uint32_t load_addr,
> +	int do_swap32)
> +{
> +	if (do_swap32) {
> +		gph->size = gpimage_swap32(size);
> +		gph->load_addr = gpimage_swap32(load_addr);
> +	} else {
> +		gph->size = size;
> +		gph->load_addr = load_addr;
> +	}

And again.



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
"The X11 source code style is ATROCIOUS and should not be used  as  a
model."                                                   - Doug Gwyn