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

Pantelis Antoniou
2014-03-31T12:16:41Z

Hi Lukasz,

Hmm, looking at the code the crc is updated when draining the buffer.
That means that in essence you're working with cache-cold data.

Can you try performing the crc32 right in the dfu_write() function
just after the memcpy?

Regards

-- Pantelis

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.
>
>
>>
>> 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.
>
>
>>
>> Regards
>>
>> -- Pantelis
>>
>>
>>> Signed-off-by: Lukasz Majewski <l.majewski@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