[U-Boot] [PATCH V4] ARM: mxs: tools: Add mkimage support for MXS bootstream

[Marek Vasut]

Dear Stefano Babic,

> dcd_Hi Marek,
> 
> On 20/08/2013 01:45, Marek Vasut wrote:
> > Add mkimage support for generating and verifying MXS bootstream.
> > The implementation here is mostly a glue code between MXSSB v0.4
> > and mkimage, but the long-term goal is to rectify this and merge
> > MXSSB with mkimage more tightly. Once this code is properly in
> > U-Boot, MXSSB shall be deprecated in favor of mkimage-mxsimage
> > support.
> > 
> > Note that the mxsimage generator needs libcrypto from OpenSSL, I
> > therefore enabled the libcrypto/libssl unconditionally.
> > 
> > MXSSB: http://git.denx.de/?p=mxssb.git;a=summary
> > 
> > The code is based on research presented at:
> > http://www.rockbox.org/wiki/SbFileFormat
> > 
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Cc: Tom Rini <trini at ti.com>
> > Cc: Fabio Estevam <fabio.estevam at freescale.com>
> > Cc: Stefano Babic <sbabic at denx.de>
> > Cc: Otavio Salvador <otavio at ossystems.com.br>
> > ---
> > 
> >  arch/arm/cpu/arm926ejs/mxs/mxsimage.mx23.cfg |    6 +
> >  arch/arm/cpu/arm926ejs/mxs/mxsimage.mx28.cfg |    8 +
> >  common/image.c                               |    1 +
> >  config.mk                                    |    9 +
> >  doc/README.mxsimage                          |  165 ++
> >  include/image.h                              |    1 +
> >  tools/Makefile                               |    2 +
> >  tools/mkimage.c                              |    2 +
> >  tools/mkimage.h                              |    1 +
> >  tools/mxsimage.c                             | 2347
> >  ++++++++++++++++++++++++++ tools/mxsimage.h                            
> >  |  230 +++
> >  11 files changed, 2772 insertions(+)
> >  create mode 100644 arch/arm/cpu/arm926ejs/mxs/mxsimage.mx23.cfg
> >  create mode 100644 arch/arm/cpu/arm926ejs/mxs/mxsimage.mx28.cfg
> >  create mode 100644 doc/README.mxsimage
> >  create mode 100644 tools/mxsimage.c
> >  create mode 100644 tools/mxsimage.h
> > 
> > V2: Remove the time hack fixing timestamp at certain time
> > 
> >     Enable -lssl and -lcrypto only if CONFIG_MX23/CONFIG_MX28 is set
> > 
> > V3: Checkpatch fixes for all but openssl
> > V4: Conditionally compile mxsimage support into mkimage by defining
> > -DCONFIG_MXS
> > 
> >     in HOSTCFLAGS and then in turn checking for this in the mxsimage.c
> > 
> > diff --git a/arch/arm/cpu/arm926ejs/mxs/mxsimage.mx23.cfg
> > b/arch/arm/cpu/arm926ejs/mxs/mxsimage.mx23.cfg new file mode 100644
> > index 0000000..8118767
> 
> I get a compiler warning testing your patch:
> 
> mxsimage.c: In function ‘mxsimage_generate’:
> mxsimage.c:1535:28: warning: ‘ilen’ may be used uninitialized in this
> function [-Wuninitialized]
> mxsimage.c:1496:12: note: ‘ilen’ was declared here
> 
> 
> Can you fix it ?

OK

> > --- /dev/null
> > +++ b/arch/arm/cpu/arm926ejs/mxs/mxsimage.mx23.cfg
> > @@ -0,0 +1,6 @@
> > +SECTION 0x0 BOOTABLE
> > + TAG LAST
> > + LOAD     0x0        spl/u-boot-spl.bin
> > + CALL     0x14       0x0
> > + LOAD     0x40000100 u-boot.bin
> > + CALL     0x40000100 0x0
> > diff --git a/arch/arm/cpu/arm926ejs/mxs/mxsimage.mx28.cfg
> > b/arch/arm/cpu/arm926ejs/mxs/mxsimage.mx28.cfg new file mode 100644
> > index 0000000..ea772f0
> > --- /dev/null
> > +++ b/arch/arm/cpu/arm926ejs/mxs/mxsimage.mx28.cfg
> > @@ -0,0 +1,8 @@
> > +SECTION 0x0 BOOTABLE
> > + TAG LAST
> > + LOAD     0x0        spl/u-boot-spl.bin
> > + LOAD IVT 0x8000     0x14
> > + CALL HAB 0x8000     0x0
> > + LOAD     0x40000100 u-boot.bin
> > + LOAD IVT 0x8000     0x40000100
> > + CALL HAB 0x8000     0x0
> 
> You are talking about examples - to better understand, do we need some
> board specific files for them ? Or do these two files cover 99% of the
> cases ?

Covers the regular usage.

[...]

> > +      MODE string_mode
> > +      - Restart the CPU and start booting from device specified by the
> > +	"string_mode" argument. The "string_mode" differs for each CPU
> > +	and can be:
> > +         i.MX23, string_mode = USB/I2C/SPI1_FLASH/SPI2_FLASH/NAND_BCH
> > +                               JTAG/SPI3_EEPROM/SD_SSP0/SD_SSP1
> > +         i.MX28, string_mode = USB/I2C/SPI2_FLASH/SPI3_FLASH/NAND_BCH
> > +                               JTAG/SPI2_EEPROM/SD_SSP0/SD_SSP1
> > +
> 
> Is this equivalent to the "bmode" command on i.MX5/6 ?

BMODE command? How does that one work?

> >   * Compression Types
> > 
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 33fad6b..bbae5a2 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -83,6 +83,7 @@ NOPED_OBJ_FILES-y += aisimage.o
> > 
> >  NOPED_OBJ_FILES-y += kwbimage.o
> >  NOPED_OBJ_FILES-y += pblimage.o
> >  NOPED_OBJ_FILES-y += imximage.o
> > 
> > +NOPED_OBJ_FILES-y += mxsimage.o
> 
> Can you reorder the list ?

How? It's out of order already.

> >  NOPED_OBJ_FILES-y += image-host.o
> >  NOPED_OBJ_FILES-y += omapimage.o
> >  NOPED_OBJ_FILES-y += mkenvimage.o
> > 
> > @@ -209,6 +210,7 @@ $(obj)mkimage$(SFX):	$(obj)aisimage.o \
> > 
> >  			$(obj)image-host.o \
> >  			$(FIT_SIG_OBJS) \
> >  			$(obj)imximage.o \
> > 
> > +			$(obj)mxsimage.o \
> 
> Ditto

Ditto, it's already out of order.

[...]

> > +
> > +/*
> > + * CRC32
> > + */
> > +static uint32_t crc32(uint8_t *data, uint32_t len)
> > +{
> > +	const uint32_t poly = 0x04c11db7;
> 
> Comparing this polinomial with the one in lib/crc32.c, they are
> identical. The crc32 function you define here should give the same
> result as our old crc32 (global) in lib/crc32.c. Then, can you drop this
> one and use the already implemented function ?

As I do not want to diverge more than necessary, this can be done in subsequent 
patch.

[...]

> > +	/*
> > +	 * Append the command to the last section.
> > +	 */
> > +	if (!sctx->cmd_head) {
> > +		sctx->cmd_head = cctx;
> > +		sctx->cmd_tail = cctx;
> > +	} else {
> > +		sctx->cmd_tail->cmd = cctx;
> > +		sctx->cmd_tail = cctx;
> > +	}
> > +
> 
> This append stuff can be factorized at least with a macro - it is
> repeated again and again in all commands.

This can (read : shall) be even replaced by include/linked_lists.h  , but not in 
this patch.

[...]