On Mon, Jan 20, 2014 at 11:46:49PM +0100, Wolfgang Denk wrote:
Dear Murali Karicheri,
In message 1390255810-19486-2-git-send-email-m-karicheri2@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..
This appears to be a blind re-use of the omap code where we do have a case where we have to do this kind of swap.