[U-Boot] [PATCH v4] socfpga: Add socfpga preloader signing to mkimage

[Wolfgang Denk]

Dear Charles,

In message <1393194939-29786-1-git-send-email-cdhmanning at gmail.com> you wrote:
> Like many platforms, the Altera socfpga platform requires that the
> preloader be "signed" in a certain way or the built-in boot ROM will
> not boot the code.
> 
> This change automatically creates an appropriately signed preloader
> from an SPL image.
> 
> Signed-off-by: Charles Manning <cdhmanning at gmail.com>
> ---
> 
> Changes for v3:
>  - Fix some coding style issues.
>  - Move from a standalone tool to the mkimgae framework.
> 
> Changes for v4:
>  - Fix more coding style issues.
>  - Fix typos in Makefile.
>  - Rebase on master (previous version was not on master, but on a 
>    working socfpga branch).

There may be perfectly valid reasons why you might decide to ingore a
review comments - sch comments may be wrong, too, after all.  But in
such a case it is always a good idea to provide feedback to the
reviewer why you decided not to follow his advice.  Otherwise he might
think you just missed or ignored the comment.

And this is what is happeneing (again) in your patch...

> diff --git a/spl/Makefile b/spl/Makefile
> index bf98024..90faaa6 100644
> --- a/spl/Makefile
> +++ b/spl/Makefile
> @@ -181,6 +181,14 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
>  
>  ALL-y	+= $(obj)/$(SPL_BIN).bin
>  
> +$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
> +	$(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
> +
> +

One blank line is sufficient.

I asked before:  "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

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

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

I aseked before: Would you care to explain the errors in the document
that might cause problems to the reader?

Noting that something contains errors without mentioning what these
are is really not helpful.

> + * This uses the CRC32 calc out of the well known Apple
> + * crc32.c code. CRC32 algorithms do not produce the same results.
> + * We need this one. Sorry about the coade bloat.

Both Gerhard and me asked before: Why exactly do we need another
implementation of CRC32.  We already have some - why cannot we use
these?

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

I asked before:  Please provide _exact_ reference. See 
http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign
for instructions how to do this.

I also commented before: If you really need this copied code (i. e.
you canot use one of the already existing implementations of CRC32 -
which I seriously doubt), then better move this into a separate file,
and assign a separate license ID tag for it.


I also asked this before: 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.
> + */

Also asked before: Incorrect multiline comment style. Please fix
globally.



It turns out that you basically ignored nearly ALL of trhe review
comments which I made before, twice.


It is an exhausting experience to deal with your patches :-(


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
Until you walk a mile in another man's moccasins, you  can't  imagine
the smell.