Dear Murali Karicheri,
In message <1390255810-19486-2-git-send-email-m-kariche...@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: w...@denx.de
"The X11 source code style is ATROCIOUS and should not be used as a
model." - Doug Gwyn
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
