[U-Boot] [PATCH v3] dfu: Introduction of the "dfu_hash_algo" env variable for checksum method setting

[Heiko Schocher]

Hello Lukasz,

Am 16.05.2014 10:58, schrieb Lukasz Majewski:
> Hi Wolfgang, Tom,
>
>> Hi Wolfgang,
>>
>>> Dear Lukasz,
>>>
>>> In message<20140515090904.32f1d13d at amdc2363>  you wrote:
>>>>
>>>>>> What I complained about is the change in behaviour.  I asked
>>>>>> to make the existing behaviour the default, so unaware users
>>>>>> will not be affected. Only if you intentionally want some
>>>>>> other behaviour you can then enable this by setting the env
>>>>>> variable.
>>>>>
>>>>> Well, looking at mainline usage of DFU, Lukasz is speaking for
>>>>> about half of the users / implementors.  Since Denx is working
>>>>> with the other half, can you shed some light on actual use vs
>>>>> theoretical possibilities?
>>>>
>>>> I don't want to urge anybody on making any conclusion here :-),
>>>> but I would be very grateful if we could come up with an
>>>> agreement.
>>>>
>>>> As I've stated previously, my opinion is similar to the one
>>>> presented by Tom in this message.
>>>>
>>>> For me it would be best to not calculate any checksum on default
>>>> and only enable it when needed.
>>>
>>> I asked Heiko to run some actual tests on the boards where he has to
>>> maintain DFU for.  For a 288 MiB image he did not measure any
>>> difference - with your patch applied, both with and without CRC
>>> enabled, we would get the same (slow) 1:54 minutes download time.
>>
>> As I've spoken with Heiko, am33xx uses NAND memory. I deal with eMMC.
>> Moreover, the size of "chunks" are different - 1 MiB and 32 MiB.
>>
>> I must double check for the rationale for chunk size of 32 MiB at
>> Trats/Trats2 boards. I suspect, that eMMC write performance depend
>> on that.
>>
>> I will perform some performance tests with 1 MiB chunk size and share
>> the result.
>
> Unfortunately the 32 MiB is fixed for our platform. since lthor uses it
> by default.
>
>>
>>>
>>> This reinforces my speculation that you are actually addressing the
>>> wrong problem.  Instead of adding new code and environment variables
>>> and making the system even more complex, we should just leave
>>> everything as is,
>>
>> During working on this patch I've replaced the crc32() method with the
>> call to hash_method(), which IMHO is welcome.
>>
>> I also don't personally like the crc32, hence I like the choice which
>> this patch gives me to use other algorithm (for which I've got HW
>> support on my platform - e.g. MD5).
>>
>>> and you should try to find out why the CRC
>>> calculation is so low for you.  Checking if caches are enabled is
>>> probably among the things that should be done first.
>>
>> L1 is enabled. L2 has been disabled on purpose (power consumption
>> reduction).
>
> Regarding L2 - our platform requires SMC calls to enable and manage L2
> cache. In my opinion support for this in u-boot is an overkill.
>
>
> Can we conclude this whole discussion? The main point was if we should
> keep calculating and displaying crc32 as default for DFU transfers.
>
> I'm for the option to NOT display and calculate it by default (PATCH
> v3).

I talked with the siemens board customer, they also do not check/use
the displayed value from U-Boot ...

So, for me it is OK to not display this value ... but we should add
to DFU such a check ... or?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany