[U-Boot] [PATCH] socfpga: Add a signer that is integrated into mkimage

[Wolfgang Denk]

Dear Charles Manning,

In message <1392942516-3488-1-git-send-email-cdhmanning at gmail.com> you wrote:
> This one passes checkpatch too :-)
> 
> Signed-off-by: Charles Manning <cdhmanning at gmail.com>
> ---
>  common/image.c       |    1 +
...

Would you please read [1] and especially [2], the section about
posting modified versions of patches ?

You are posting multiple patches with the same subject but different
content.  Do you think we have time to figure out what might be the
difference?  Please make sure to include a version tagin the Subject:
line, and to add a history oof changes in the comment section.

And - do you think that "This one passes checkpatch too :-)" is a
helpful and descriptive commit message?  [No, it is not.]

[1] http://www.denx.de/wiki/U-Boot/Patches
[2] http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions

> @@ -144,6 +144,7 @@ static const table_entry_t uimage_type[] = {
>  	{	IH_TYPE_INVALID,    NULL,	  "Invalid Image",	},
>  	{	IH_TYPE_MULTI,	    "multi",	  "Multi-File Image",	},
>  	{	IH_TYPE_OMAPIMAGE,  "omapimage",  "TI OMAP SPL With GP CH",},
> +	{	IH_TYPE_SOCFPGAIMAGE,  "socfpgaimage",  "Altera SOCFPGA preloader",},
>  	{	IH_TYPE_PBLIMAGE,   "pblimage",   "Freescale PBL Boot Image",},
>  	{	IH_TYPE_RAMDISK,    "ramdisk",	  "RAMDisk Image",	},
>  	{	IH_TYPE_SCRIPT,     "script",	  "Script",		},

Please always keep such lists sorted.

> --- a/spl/Makefile
> +++ b/spl/Makefile
> @@ -144,8 +144,12 @@ $(OBJTREE)/MLO:	$(obj)u-boot-spl.bin
>  
>  $(OBJTREE)/MLO.byteswap: $(obj)u-boot-spl.bin
>  	$(OBJTREE)/tools/mkimage -T omapimage -n byteswap \
> +
>  		-a $(CONFIG_SPL_TEXT_BASE) -d $< $@

Did you actually test this?  IMO this will break the build.

> +$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)u-boot-spl.bin
> +	$(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@

"socfpga-signed-preloader.bin" is a terribly long name.  Can we find a
better one?

> +ifdef CONFIG_SOCFPGA
> +ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin
> +endif

Can we not use "ALL-$(CONFIG_SOCFPGA)" and avoid the ifdef ?

>  NOPED_OBJ_FILES-y += pblimage.o
>  NOPED_OBJ_FILES-y += imximage.o
>  NOPED_OBJ_FILES-y += omapimage.o
> +NOPED_OBJ_FILES-y += socfpgaimage.o
>  NOPED_OBJ_FILES-y += mkenvimage.o
>  NOPED_OBJ_FILES-y += mkimage.o

Keep sorted.

>  OBJ_FILES-$(CONFIG_SMDK5250) += mkexynosspl.o
> @@ -214,6 +215,7 @@ $(obj)mkimage$(SFX):	$(obj)aisimage.o \
>  			$(obj)mkimage.o \
>  			$(obj)os_support.o \
>  			$(obj)omapimage.o \
> +			$(obj)socfpgaimage.o \
>  			$(obj)sha1.o \
>  			$(obj)ublimage.o \
>  			$(LIBFDT_OBJS)

Keep sorted.

>  	/* Init Davinci AIS support */
>  	init_ais_image_type();
> +	/* Init Altera SOCFPGA image generation/list support */
> +	init_socfpga_image_type();

	/* Init Altera SOCFPGA support */

should be sufficient.

> + * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf
> + * Note this doc is not entirely accurate.

Would you care to explain the errors in the document that might cause
problems to the reader?

> + * This uses the CRC32 calc out of the well known Apple
> + * crc32.c code. Copyright for the CRC code:

1) Why exactly do we need another implementation of CRC32.  We already
have some - why cannot we use these?

2) Please provide _exact_ reference. See [3] 

   [3] http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign

> + * COPYRIGHT (C) 1986 Gary S. Brown.  You may use this program, or
> + *  code or tables extracted from it, as desired without restriction.
> + *
> + */

3) You better move this into a separate file, and assign a separate
   license ID tag for it.

4) I cannot find a license ID tag in your new code. Please add.

> +/* To work in with the mkimage framework, we do some ugly stuff...
> + *
> + * First we prepend a fake header big enough to make the file 64k.
> + * When set_header is called, we fix this up by moving the image
> + * around in the buffer.
> + */

Incorrect multiline comment style. Please fix globally.

Thanks.

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
Put your Nose to the Grindstone!
                 -- Amalgamated Plastic Surgeons and Toolmakers, Ltd.