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

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

2014-02-20 Thread Wolfgang Denk

Dear Charles Manning,

In message <1392942516-3488-1-git-send-email-cdhmann...@gmail.com> you wrote:
> This one passes checkpatch too :-)
> 
> Signed-off-by: Charles Manning 
> ---
>  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: w...@denx.de
Put your Nose to the Grindstone!
 -- Amalgamated Plastic Surgeons and Toolmakers, Ltd.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-02-20 Thread Charles Manning

This one passes checkpatch too :-)

Signed-off-by: Charles Manning 
---
 common/image.c   |1 +
 include/image.h  |1 +
 spl/Makefile |8 ++
 tools/Makefile   |2 +
 tools/mkimage.c  |2 +
 tools/mkimage.h  |1 +
 tools/socfpgaimage.c |  348 ++
 7 files changed, 363 insertions(+)
 create mode 100644 tools/socfpgaimage.c

diff --git a/common/image.c b/common/image.c
index 95498e6..1a2e107 100644
--- a/common/image.c
+++ b/common/image.c
@@ -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", },
diff --git a/include/image.h b/include/image.h
index f5adc50..95a0bf7 100644
--- a/include/image.h
+++ b/include/image.h
@@ -166,6 +166,7 @@
 #define IH_TYPE_AISIMAGE   13  /* TI Davinci AIS Image */
 #define IH_TYPE_KERNEL_NOLOAD  14  /* OS Kernel Image, can run from any 
load address */
 #define IH_TYPE_PBLIMAGE   15  /* Freescale PBL Boot Image */
+#define IH_TYPE_SOCFPGAIMAGE   16  /* Altera SOCFPGA Preloader Image   
*/
 
 /*
  * Compression Types
diff --git a/spl/Makefile b/spl/Makefile
index c0abe3d..8f061b0 100644
--- 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 $< $@
 
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)u-boot-spl.bin
+   $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+
 ifneq ($(CONFIG_IMX_CONFIG),)
 $(OBJTREE)/SPL:$(obj)u-boot-spl.bin
$(OBJTREE)/tools/mkimage -n  $(SRCTREE)/$(CONFIG_IMX_CONFIG) -T 
imximage \
@@ -154,6 +158,10 @@ endif
 
 ALL-y  += $(obj)u-boot-spl.bin
 
+ifdef CONFIG_SOCFPGA
+ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin
+endif
+
 ifdef CONFIG_SAMSUNG
 ALL-y  += $(obj)$(BOARD)-spl.bin
 endif
diff --git a/tools/Makefile b/tools/Makefile
index 686840a..52f4bfc 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -95,6 +95,7 @@ NOPED_OBJ_FILES-y += kwbimage.o
 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
 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)
diff --git a/tools/mkimage.c b/tools/mkimage.c
index e43b09f..87bacc0 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -167,6 +167,8 @@ main (int argc, char **argv)
init_ubl_image_type();
/* Init Davinci AIS support */
init_ais_image_type();
+   /* Init Altera SOCFPGA image generation/list support */
+   init_socfpga_image_type();
 
params.cmdname = *argv;
params.addr = params.ep = 0;
diff --git a/tools/mkimage.h b/tools/mkimage.h
index ea45f5c..eda4832 100644
--- a/tools/mkimage.h
+++ b/tools/mkimage.h
@@ -157,5 +157,6 @@ void init_default_image_type (void);
 void init_fit_image_type (void);
 void init_ubl_image_type(void);
 void init_omap_image_type(void);
+void init_socfpga_image_type(void);
 
 #endif /* _MKIIMAGE_H_ */
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c
new file mode 100644
index 000..9944c78
--- /dev/null
+++ b/tools/socfpgaimage.c
@@ -0,0 +1,348 @@
+/*
+ * Copyright (C) 2014 Charles Manning 
+ *
+ * Use as you see fit.
+ *
+ * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf
+ * Note this doc is not entirely accurate.
+ *
+ * "Header" is a structure of the following format.
+ * this is positioned at 0x40.
+ *
+ * Endian is LSB.
+ *
+ * Offset   Length   Usage
+ * ---
+ *  04   Validation word 0x31305341
+ *  41   Version (whatever, zero is fine)
+ *  51   Flags   (unused, zero is fine)
+ *  62   Length  (in units of u32, including the end checksum).
+ *  82   Zero
+ *   0x0A2   Checksum over the heder. NB Not CRC32
+ *
+ * At the end of the code we have a 32-bit CRC checksum over whole binary
+ * excluding the CRC.
+ *
+ *

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

2014-02-20 Thread Charles Manning

This tool signs a preloader (built from SPL) when CONFIG_SOCFPGA is set.

Signed-off-by: Charles Manning 
---
 common/image.c   |1 +
 include/image.h  |1 +
 spl/Makefile |8 ++
 tools/Makefile   |2 +
 tools/mkimage.c  |2 +
 tools/mkimage.h  |1 +
 tools/socfpgaimage.c |  343 ++
 7 files changed, 358 insertions(+)
 create mode 100644 tools/socfpgaimage.c

diff --git a/common/image.c b/common/image.c
index 95498e6..1a2e107 100644
--- a/common/image.c
+++ b/common/image.c
@@ -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", },
diff --git a/include/image.h b/include/image.h
index f5adc50..95a0bf7 100644
--- a/include/image.h
+++ b/include/image.h
@@ -166,6 +166,7 @@
 #define IH_TYPE_AISIMAGE   13  /* TI Davinci AIS Image */
 #define IH_TYPE_KERNEL_NOLOAD  14  /* OS Kernel Image, can run from any 
load address */
 #define IH_TYPE_PBLIMAGE   15  /* Freescale PBL Boot Image */
+#define IH_TYPE_SOCFPGAIMAGE   16  /* Altera SOCFPGA Preloader Image   
*/
 
 /*
  * Compression Types
diff --git a/spl/Makefile b/spl/Makefile
index c0abe3d..8f061b0 100644
--- 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 $< $@
 
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)u-boot-spl.bin
+   $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+
 ifneq ($(CONFIG_IMX_CONFIG),)
 $(OBJTREE)/SPL:$(obj)u-boot-spl.bin
$(OBJTREE)/tools/mkimage -n  $(SRCTREE)/$(CONFIG_IMX_CONFIG) -T 
imximage \
@@ -154,6 +158,10 @@ endif
 
 ALL-y  += $(obj)u-boot-spl.bin
 
+ifdef CONFIG_SOCFPGA
+ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin
+endif
+
 ifdef CONFIG_SAMSUNG
 ALL-y  += $(obj)$(BOARD)-spl.bin
 endif
diff --git a/tools/Makefile b/tools/Makefile
index 686840a..52f4bfc 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -95,6 +95,7 @@ NOPED_OBJ_FILES-y += kwbimage.o
 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
 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)
diff --git a/tools/mkimage.c b/tools/mkimage.c
index e43b09f..87bacc0 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -167,6 +167,8 @@ main (int argc, char **argv)
init_ubl_image_type();
/* Init Davinci AIS support */
init_ais_image_type();
+   /* Init Altera SOCFPGA image generation/list support */
+   init_socfpga_image_type();
 
params.cmdname = *argv;
params.addr = params.ep = 0;
diff --git a/tools/mkimage.h b/tools/mkimage.h
index ea45f5c..eda4832 100644
--- a/tools/mkimage.h
+++ b/tools/mkimage.h
@@ -157,5 +157,6 @@ void init_default_image_type (void);
 void init_fit_image_type (void);
 void init_ubl_image_type(void);
 void init_omap_image_type(void);
+void init_socfpga_image_type(void);
 
 #endif /* _MKIIMAGE_H_ */
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c
new file mode 100644
index 000..92abd93
--- /dev/null
+++ b/tools/socfpgaimage.c
@@ -0,0 +1,343 @@
+/*
+ * Copyright (C) 2014 Charles Manning 
+ *
+ * Use as you see fit.
+ *
+ * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf
+ * Note this doc is not entirely accurate.
+ *
+ * "Header" is a structure of the following format.
+ * this is positioned at 0x40.
+ *
+ * Endian is LSB.
+ *
+ * Offset   Length   Usage
+ * ---
+ *  04   Validation word 0x31305341
+ *  41   Version (whatever, zero is fine)
+ *  51   Flags   (unused, zero is fine)
+ *  62   Length  (in units of u32, including the end checksum).
+ *  82   Zero
+ *   0x0A2   Checksum over the heder. NB Not CRC32
+ *
+ * At the end of the code we have a 32-bit CRC checksum over whole

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information