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-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..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
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
