[U-Boot] [PATCH 3/3] dfu: Introduction of the "dfu_checksum_method" env variable for checksum method setting

[Pantelis Antoniou]

Hi Lukasz,

On Mar 31, 2014, at 3:04 PM, Lukasz Majewski wrote:

> Hi Pantelis,
> 
>> Hi Lukasz,
>> 
>> On Mar 31, 2014, at 11:48 AM, Lukasz Majewski wrote:
>> 
>>> Up till now the CRC32 of received data was calculated
>>> unconditionally. The standard crc32 implementation causes long
>>> delays when large images were uploaded.
>>> 
>>> The "dfu_checksum_method" environment variable gives the
>>> opportunity to enable on demand (when e.g. debugging) the crc32
>>> calculation. It can be done without need to recompile the u-boot
>>> binary.
>>> 
>>> By default the crc32 is not calculated.
>>> 
>>> Tests results:
>>> 400 MiB ums.img file
>>> With 		crc32 calculation: 65 sec [avg 6.29 MB/s]
>>> Without 		crc32 calculation: 25 sec [avg 16.17 MB/s]
>>> 
>> 
>> That's interesting; I'm surprised that there's so much difference.
>> Can we get some info about the environment? I.e. board, whether cache
>> is enabled etc.
> 
> 
> Board Exynos4412 - Trats2. Cache L1 enabled, cache L2 disabled.
> 
> Crc32 is calculated for 32 MiB chunks of data in the received buffer.
> I'm using the standard software crc32 u-boot's implementation. It is
> the same as the one for perl-crc32 debian package.
> 

Thanks for the report. Would it be too much to ask for the time it
takes to do a crc32 of the same image on user-space after boot?

I'm interested in the effect the disabling of L2 has.

> 
>> 
>> The crc table is per byte and I guess lookups maybe expensive.
> 
> It seems so. Moreover the Exynos4412 has HW crypto engine which
> supports SHA1, MD5 and other algorithms. Unfortunately it doesn't
> provide speedup for crc32.
> 
> 

You could do a basic tradeoff of speed vs memory by creating larger tables
but it might not work if we're cache trashing.

Or even try using CRC with smaller tables (i.e. 4 bits) which would not affect
the cache much.

Regards

-- Pantelis

>> 
>> Regards
>> 
>> -- Pantelis
>> 
>> 
>>> Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
>>> ---
>>> drivers/dfu/dfu.c |   34 ++++++++++++++++++++++++++++++----
>>> include/dfu.h     |    5 +++++
>>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>>> index e15f959..5d50b47 100644
>>> --- a/drivers/dfu/dfu.c
>>> +++ b/drivers/dfu/dfu.c
>>> @@ -20,6 +20,7 @@ static bool dfu_reset_request;
>>> static LIST_HEAD(dfu_list);
>>> static int dfu_alt_num;
>>> static int alt_num_count;
>>> +static int dfu_checksum_method;
>>> 
>>> bool dfu_reset(void)
>>> {
>>> @@ -99,6 +100,23 @@ unsigned char *dfu_get_buf(void)
>>> 	return dfu_buf;
>>> }
>>> 
>>> +static int dfu_get_checksum_method(void)
>>> +{
>>> +	char *s;
>>> +
>>> +	s = getenv("dfu_checksum_method");
>>> +	if (!s)
>>> +		return DFU_NO_CHECKSUM;
>>> +
>>> +	if (!strcmp(s, "crc32")) {
>>> +		debug("%s: DFU checksum method: %s\n", __func__,
>>> s);
>>> +		return DFU_CRC32;
>>> +	} else {
>>> +		error("DFU checksum method: %s not supported!\n",
>>> s);
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> static int dfu_write_buffer_drain(struct dfu_entity *dfu)
>>> {
>>> 	long w_size;
>>> @@ -109,8 +127,8 @@ static int dfu_write_buffer_drain(struct
>>> dfu_entity *dfu) if (w_size == 0)
>>> 		return 0;
>>> 
>>> -	/* update CRC32 */
>>> -	dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
>>> +	if (dfu_checksum_method == DFU_CRC32)
>>> +		dfu->crc = crc32(dfu->crc, dfu->i_buf_start,
>>> w_size);
>>> 
>>> 	ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start,
>>> &w_size); if (ret)
>>> @@ -234,7 +252,8 @@ static int dfu_read_buffer_fill(struct
>>> dfu_entity *dfu, void *buf, int size) /* consume */
>>> 		if (chunk > 0) {
>>> 			memcpy(buf, dfu->i_buf, chunk);
>>> -			dfu->crc = crc32(dfu->crc, buf, chunk);
>>> +			if (dfu_checksum_method == DFU_CRC32)
>>> +				dfu->crc = crc32(dfu->crc, buf,
>>> chunk); dfu->i_buf += chunk;
>>> 			dfu->b_left -= chunk;
>>> 			dfu->r_left -= chunk;
>>> @@ -318,7 +337,9 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
>>> int size, int blk_seq_num) }
>>> 
>>> 	if (ret < size) {
>>> -		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name,
>>> dfu->crc);
>>> +		if (dfu_checksum_method == DFU_CRC32)
>>> +			debug("%s: %s CRC32: 0x%x\n", __func__,
>>> dfu->name,
>>> +			      dfu->crc);
>>> 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
>>> 
>>> 		dfu_free_buf();
>>> @@ -393,6 +414,11 @@ int dfu_config_entities(char *env, char
>>> *interface, int num) dfu_alt_num = dfu_find_alt_num(env);
>>> 	debug("%s: dfu_alt_num=%d\n", __func__, dfu_alt_num);
>>> 
>>> +	ret = dfu_get_checksum_method();
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	dfu_checksum_method = ret;
>>> +
>>> 	dfu = calloc(sizeof(*dfu), dfu_alt_num);
>>> 	if (!dfu)
>>> 		return -1;
>>> diff --git a/include/dfu.h b/include/dfu.h
>>> index 751f0fd..855d6dc 100644
>>> --- a/include/dfu.h
>>> +++ b/include/dfu.h
>>> @@ -37,6 +37,11 @@ enum dfu_op {
>>> 	DFU_OP_WRITE,
>>> };
>>> 
>>> +enum dfu_checksum {
>>> +	DFU_NO_CHECKSUM = 0,
>>> +	DFU_CRC32,
>>> +};
>>> +
>>> #define DFU_NOT_SUPPORTED -1
>>> 
>>> struct mmc_internal_data {
>>> -- 
>>> 1.7.10.4
>>> 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group