From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 1 Oct 2014 13:51:32 +0200
Subject: [U-Boot] [PATCH 07/51] tools: socfpga: Add socfpga preloader
	signing to mkimage
In-Reply-To: <1412158247.11125.18.camel@clsee-VirtualBox.altera.com>
References: <1411304339-11348-1-git-send-email-marex@denx.de>
	<1411304339-11348-8-git-send-email-marex@denx.de>
	<1412158247.11125.18.camel@clsee-VirtualBox.altera.com>
Message-ID: <201410011351.32660.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wednesday, October 01, 2014 at 12:10:47 PM, Chin Liang See wrote:

[...]

> > + * The image is padded out to 64k, because that is what is
> > + * typically used to write the image to the boot medium.
> > + */
> > +
> > +#include "pbl_crc32.h"
> 
> Seems I cannot find this file

See tools/pbl_crc32.h

[...]

> > +static void build_header(uint8_t *buf,
> > +			  uint8_t version,
> > +			  uint8_t flags,
> > +			  uint16_t length_bytes)
> > +{
> > +	header.validation = htole32(VALIDATION_WORD);
> > +	header.version = version;
> > +	header.flags = flags;
> > +	header.length_u32 = htole16(length_bytes/4);
> > +	header.zero = 0;
> > +	header.checksum = htole16(hdr_checksum((const uint8_t *)&header, 10));
> 
> Seems 10 is a magic number here.
> Suggest to use sizeof(header) - sizeof(header.checksum)

Even better, I can pass the whole *header into hdr_checksum and contain all that 
size computation in there. Your formula included.

[...]

> > +/* Sign the buffer and return the signed buffer size */
> > +static int sign_buffer(uint8_t *buf,
> > +			uint8_t version, uint8_t flags,
> > +			int len, int pad_64k)
> > +{
> > +	uint32_t calc_crc;
> > +
> > +	/* Align the length up */
> > +	len = (len + 3) & (~3);
> > +
> > +	/* Build header, adding 4 bytes to length to hold the CRC32. */
> > +	build_header(buf + HEADER_OFFSET,  version, flags, len + 4);
> > +
> > +	/* Calculate and apply the CRC */
> > +	calc_crc = ~pbl_crc32(0, (char *)buf, len);
> > +
> 
> For this, we can reuse the lib/bzlib_private.h
> 
> 	/* Calculate and apply the CRC */
> 	BZ_INITIALISE_CRC(calc_crc);
> 	while (len--) {
> 		BZ_UPDATE_CRC(calc_crc, *buf);
> 		buf++;
> 	}
> 	calc_crc ^= ~0;

Do you mean I should replace the one function call with this whole block ?

[...]

The rest was addressed.