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

[Lukasz Majewski]

Hi Heiko,

> 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 ... 

Applied this patch (with no default CRC32 calculation - v3) to
u-boot-dfu tree.

> but we should add
> to DFU such a check ... or?
> 
> bye,
> Heiko

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group