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

[Tom Rini <trini@ti.com>]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140120/474b3cc0/attachment.pgp>