diff mbox

[U-Boot,16/20] Roll crc32 into hash infrastructure

Message ID 1356548233-5570-17-git-send-email-sjg@chromium.org
State Superseded, archived
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass Dec. 26, 2012, 6:57 p.m. UTC
Add the CRC32 algorithm to the list of available hashes, and make
the crc32 command use hash_command(). Add a new crc32_wd_buf() to
make this possible, which puts its result in a buffer rather than
returning it as a 32-bit value.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_mem.c     |   73 ++++---------------------------------------------
 common/hash.c        |    6 ++++
 include/hash.h       |    2 +-
 include/u-boot/crc.h |   11 +++++++
 lib/crc32.c          |    9 ++++++
 5 files changed, 33 insertions(+), 68 deletions(-)

Comments

Simon Glass Feb. 15, 2013, 11:57 p.m. UTC | #1
On Wed, Dec 26, 2012 at 10:57 AM, Simon Glass <sjg@chromium.org> wrote:
> Add the CRC32 algorithm to the list of available hashes, and make
> the crc32 command use hash_command(). Add a new crc32_wd_buf() to
> make this possible, which puts its result in a buffer rather than
> returning it as a 32-bit value.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to x86/master.

> ---
>  common/cmd_mem.c     |   73 ++++---------------------------------------------
>  common/hash.c        |    6 ++++
>  include/hash.h       |    2 +-
>  include/u-boot/crc.h |   11 +++++++
>  lib/crc32.c          |    9 ++++++
>  5 files changed, 33 insertions(+), 68 deletions(-)
>
Wolfgang Denk Feb. 17, 2013, 8:53 p.m. UTC | #2
Dear Simon Glass,

In message <CAPnjgZ0hKP-Lr9TiUXCQEa-JhOGEB_6JXuBsCJq-FtAYhDJnww@mail.gmail.com> you wrote:
> On Wed, Dec 26, 2012 at 10:57 AM, Simon Glass <sjg@chromium.org> wrote:
> > Add the CRC32 algorithm to the list of available hashes, and make
> > the crc32 command use hash_command(). Add a new crc32_wd_buf() to
> > make this possible, which puts its result in a buffer rather than
> > returning it as a 32-bit value.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> 
> Applied to x86/master.
> 
> > ---
> >  common/cmd_mem.c     |   73 ++++---------------------------------------------
> >  common/hash.c        |    6 ++++
> >  include/hash.h       |    2 +-
> >  include/u-boot/crc.h |   11 +++++++
> >  lib/crc32.c          |    9 ++++++
> >  5 files changed, 33 insertions(+), 68 deletions(-)

Umm... this (and a lot of the other things) is all common stuff.  This
is not supposed to go through the x86 repository.

The x86 tree should ONLY be used for x86 spoecific stuff.

Please do no use this for pull requests.  Tom has to pick up these
directly.

Best regards,

Wolfgang Denk
Simon Glass Feb. 17, 2013, 9:34 p.m. UTC | #3
Hi Wolfgang,

On Sun, Feb 17, 2013 at 12:53 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ0hKP-Lr9TiUXCQEa-JhOGEB_6JXuBsCJq-FtAYhDJnww@mail.gmail.com> you wrote:
>> On Wed, Dec 26, 2012 at 10:57 AM, Simon Glass <sjg@chromium.org> wrote:
>> > Add the CRC32 algorithm to the list of available hashes, and make
>> > the crc32 command use hash_command(). Add a new crc32_wd_buf() to
>> > make this possible, which puts its result in a buffer rather than
>> > returning it as a 32-bit value.
>> >
>> > Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> Applied to x86/master.
>>
>> > ---
>> >  common/cmd_mem.c     |   73 ++++---------------------------------------------
>> >  common/hash.c        |    6 ++++
>> >  include/hash.h       |    2 +-
>> >  include/u-boot/crc.h |   11 +++++++
>> >  lib/crc32.c          |    9 ++++++
>> >  5 files changed, 33 insertions(+), 68 deletions(-)
>
> Umm... this (and a lot of the other things) is all common stuff.  This
> is not supposed to go through the x86 repository.
>
> The x86 tree should ONLY be used for x86 spoecific stuff.
>
> Please do no use this for pull requests.  Tom has to pick up these
> directly.

OK, understood, have replied on your other thread and will await
further instructions.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Use C++ to confuse your enemies; use C to produce stable code.
Tom Rini Feb. 17, 2013, 11:26 p.m. UTC | #4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/17/2013 03:53 PM, Wolfgang Denk wrote:
> Dear Simon Glass,
> 
> In message
> <CAPnjgZ0hKP-Lr9TiUXCQEa-JhOGEB_6JXuBsCJq-FtAYhDJnww@mail.gmail.com>
> you wrote:
>> On Wed, Dec 26, 2012 at 10:57 AM, Simon Glass <sjg@chromium.org>
>> wrote:
>>> Add the CRC32 algorithm to the list of available hashes, and
>>> make the crc32 command use hash_command(). Add a new
>>> crc32_wd_buf() to make this possible, which puts its result in
>>> a buffer rather than returning it as a 32-bit value.
>>> 
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> 
>> Applied to x86/master.
>> 
>>> --- common/cmd_mem.c     |   73
>>> ++++--------------------------------------------- common/hash.c
>>> |    6 ++++ include/hash.h       |    2 +- include/u-boot/crc.h
>>> |   11 +++++++ lib/crc32.c          |    9 ++++++ 5 files
>>> changed, 33 insertions(+), 68 deletions(-)
> 
> Umm... this (and a lot of the other things) is all common stuff.
> This is not supposed to go through the x86 repository.
> 
> The x86 tree should ONLY be used for x86 spoecific stuff.
> 
> Please do no use this for pull requests.  Tom has to pick up these 
> directly.

There's another thread I don't have yet (and I don't have this one in
gmail yet even).  But, I am OK with custodians using their repos, but
not the master branch, for unrelated but otherwise good patches. I'm
also fine with patchwork bundles.  I suppose we could use the staging
repository for these changes instead.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRIWchAAoJENk4IS6UOR1WE+MQAKYZeaLDzgA7rLu6vJl2h4QU
Ip3CI724hAjE2tr8WzIit3T9owLc1kI0xNnWtT8wi9YylP/EqIwgFs01ZfUUtm9Z
TEaSTE+vnDMegTByD4Z3w7AoiF2Ebw9ZL0bTqeG8AZ8Nz4x1iwSWvUK2htYvd47F
QoffFAI0UmHCdfYD9Bq+XcQ9CS7zjpS6uXNqroqSfiEzjFRtLMn8n0pUtqiPKgFi
cX9pLi+ZwmNY7N+xNa7UzUbYsTml4V+J7HxhRyjzHFHeCnFmE3WYP1+lmifxRZ+m
+mZCEXSusUgiGGiLwh5vvfLNVGyYLxTAoA4cPFvqWWdwU51QMoz0GfoqiR4GS5Fn
Z3Mrvkm380v4mHvB0m8PjtSfvm5/PUtTz7ZCXpUudCaB2Z8zKsZJy56hWaYQDr9I
ZyHbR2bBOiGvdD9oOfvVj8CtXvC3VtJZ2GJpXDSlbsH6Ew9ONvjpfVqQEU7zT9Sz
rBchE/iXxjgqi3iwXdll89McA4NJIO31uKa92ILDPXjE6joOHc5uyv1tlu1n+E5X
m4+CwfHnqUgeaRBPGm4M8O5vVJeD5LLw7ulcKU91rXiE5DG/5dWzFatTffOn5e7A
neuQoCZQd04Vi9FQp4eLhVatUzHZfkf6qV1pTwMQFR2pdlRigW3dYJH1gvyw3EQY
DFGnIt0HBgonpJZBnzBh
=m6Rk
-----END PGP SIGNATURE-----
Wolfgang Denk Feb. 18, 2013, 11:35 a.m. UTC | #5
Dear Tom,

In message <51216721.1010603@ti.com> you wrote:
>
> There's another thread I don't have yet (and I don't have this one in
> gmail yet even).  But, I am OK with custodians using their repos, but
> not the master branch, for unrelated but otherwise good patches. I'm
> also fine with patchwork bundles.  I suppose we could use the staging
> repository for these changes instead.

What I mostly object about there is that these patches would go into
mainline basicly unreviewed, as patch submission and pull request is
all done from a single person, with no other feedback on the patches
at all.  And this affects a lot of common code...

Actually, I see this change when pulling u-boot-x86.git/master:

-> bloat-o-meter u-boot-before u-boot
add/remove: 9/0 grow/shrink: 3/14 up/down: 1006/-560 (446)
function                                     old     new   delta
hash_command                                   -     424    +424
strncasecmp                                    -     156    +156
simple_itoa                                    -     104    +104
crc32_wd_buf                                   -      76     +76
setenv_hex                                     -      68     +68
setenv_ulong                                   -      52     +52
strcasecmp                                     -      36     +36
do_mem_loopw                                 304     328     +24
static.local                                   -      22     +22
do_mem_loop                                  268     288     +20
hash_algo                                      -      16     +16
do_mem_cmp                                   332     340      +8
do_mem_mw                                    224     220      -4
set_working_fdt_addr                          72      52     -20
load_serial_ymodem                           300     280     -20
load_serial                                  512     492     -20
index_partitions                             200     180     -20
do_load_serial_bin                          1844    1824     -20
do_load                                      468     448     -20
do_jffs2_fsload                              320     300     -20
do_imgextract                                636     592     -44
NetLoop                                      832     788     -44
do_mem_cp                                    312     252     -60
do_bootm                                    1244    1180     -64
do_mem_crc                                   188      88    -100
do_mem_mtest                                1436    1332    -104


So there are changes all over the place, including a growth of the
memory footprint.  I think this needs at least minimal review.

Best regards,

Wolfgang Denk
Simon Glass Feb. 18, 2013, 4:36 p.m. UTC | #6
Hi Wolfgang,

On Mon, Feb 18, 2013 at 3:35 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Tom,
>
> In message <51216721.1010603@ti.com> you wrote:
>>
>> There's another thread I don't have yet (and I don't have this one in
>> gmail yet even).  But, I am OK with custodians using their repos, but
>> not the master branch, for unrelated but otherwise good patches. I'm
>> also fine with patchwork bundles.  I suppose we could use the staging
>> repository for these changes instead.
>
> What I mostly object about there is that these patches would go into
> mainline basicly unreviewed, as patch submission and pull request is
> all done from a single person, with no other feedback on the patches
> at all.  And this affects a lot of common code...

Fair enough. I suspect a number of people scan the code, but few feel
invested enough to formally Ack it. Also, providing a full review of
such a series can take quite a bit of time. Against that, I think it
is better to get code in and tested than have it sit around until just
before the next release.

>
> Actually, I see this change when pulling u-boot-x86.git/master:

Thanks for looking at it. Some of it is just the code moving around,
but I will take a look at why hash_command grows so much, and why the
overall size has grown. Clearly I didn't do enough here when I checked
the series. One change was trying to unify the verify feature, so
perhaps something has gone wrong there.

>
> -> bloat-o-meter u-boot-before u-boot
> add/remove: 9/0 grow/shrink: 3/14 up/down: 1006/-560 (446)
> function                                     old     new   delta
> hash_command                                   -     424    +424
> strncasecmp                                    -     156    +156
> simple_itoa                                    -     104    +104
> crc32_wd_buf                                   -      76     +76
> setenv_hex                                     -      68     +68
> setenv_ulong                                   -      52     +52
> strcasecmp                                     -      36     +36
> do_mem_loopw                                 304     328     +24
> static.local                                   -      22     +22
> do_mem_loop                                  268     288     +20
> hash_algo                                      -      16     +16
> do_mem_cmp                                   332     340      +8
> do_mem_mw                                    224     220      -4
> set_working_fdt_addr                          72      52     -20
> load_serial_ymodem                           300     280     -20
> load_serial                                  512     492     -20
> index_partitions                             200     180     -20
> do_load_serial_bin                          1844    1824     -20
> do_load                                      468     448     -20
> do_jffs2_fsload                              320     300     -20
> do_imgextract                                636     592     -44
> NetLoop                                      832     788     -44
> do_mem_cp                                    312     252     -60
> do_bootm                                    1244    1180     -64
> do_mem_crc                                   188      88    -100
> do_mem_mtest                                1436    1332    -104
>
>
> So there are changes all over the place, including a growth of the
> memory footprint.  I think this needs at least minimal review.

We need more reviewers I think.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Time is an illusion perpetrated by the manufacturers of space.
Tom Rini Feb. 18, 2013, 5:05 p.m. UTC | #7
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/18/2013 11:36 AM, Simon Glass wrote:
> Hi Wolfgang,
> 
> On Mon, Feb 18, 2013 at 3:35 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Tom,
>> 
>> In message <51216721.1010603@ti.com> you wrote:
>>> 
>>> There's another thread I don't have yet (and I don't have this 
>>> one in gmail yet even).  But, I am OK with custodians using 
>>> their repos, but not the master branch, for unrelated but 
>>> otherwise good patches. I'm also fine with patchwork bundles. I
>>> suppose we could use the staging repository for these changes 
>>> instead.
>> 
>> What I mostly object about there is that these patches would go 
>> into mainline basicly unreviewed, as patch submission and pull 
>> request is all done from a single person, with no other feedback 
>> on the patches at all.  And this affects a lot of common code...
> 
> Fair enough. I suspect a number of people scan the code, but few 
> feel invested enough to formally Ack it. Also, providing a full 
> review of such a series can take quite a bit of time. Against
> that, I think it is better to get code in and tested than have it
> sit around until just before the next release.
[snip]
>> So there are changes all over the place, including a growth of 
>> the memory footprint.  I think this needs at least minimal 
>> review.
> 
> We need more reviewers I think.

This is where I'm trying to find a good balance right now.  If we just
wait for reviewed-by lines to fly by, things will sit forever.  Partly
because enough folks don't feel like they "own" things enough to risk
saying they reviewed something that turns out later to have a bug.
And partly there's just not enough folks reading patches.  I do try
and give things some sort of read-over when it's the person posting
who is doing the merging, especially for common rather than "their
area" code.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRIl9FAAoJENk4IS6UOR1WJqMP/ie04KKI6NpMPDei9QSJ9+qg
peFMXyVXoNWVZ0OuVSgVYOyBNtTmZeUmNsyamtE/1QifAWX6F2lwXHl2teYcebMk
7Fn/N3uzoVisfcFhY3Ec0dgBigYbRm5hiSHF7qzBkS78FfI5XSFaR/XjkshCgLlC
i5Vf8Gh6ilb67fLumzxnXU2LLpbfcoOoT7iMLX0C5SFkx9sjo4k/5/WC64jsx8IS
NQiaoFX3Ow7uU63G7jEJ4hiqXsp2ulWZTBA4ynN7ydGYiPo+sRXoLRaBYB9yAkRQ
3Uj1a101VX+9LWdNoMRpqv6W3gq75+8nSggovK0DmxtF4X5PaF17Xmpc8dBTT9rE
cCIkePKDNgg6QjTAtCxk7+nw997JSjrj1nV0R2+Jm225tby4hIjmnIfnCNgeYbbI
II7+ecaUl4w+GxB1SgjFLmEyW0unDsYZauT4sXxSdBp/UOrs4I3XYW4gFofifMcB
peJELQYVUHnXblZ+xR+8zY3URsn2vNRxNq5fUDWMUADgvwecfvWFeKaVGDA+aWHs
vNFKWayZbt5MpqG4aQJ4mzIhf9avNytf8BSSQ+LFp53xOl5f08OsioN9+4H3rOND
LRuiMZ5wtrlJea3Eoi3PFXeO/l4N4eKvDNbwNSwDbS4EZj+4mZbGrNxGglcQK3C+
1EM1fpRioWEXpGaMpmKV
=W6Pn
-----END PGP SIGNATURE-----
Simon Glass Feb. 18, 2013, 5:06 p.m. UTC | #8
Hi Wolfgang,

On Mon, Feb 18, 2013 at 3:35 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Tom,
>
> In message <51216721.1010603@ti.com> you wrote:
>>
>> There's another thread I don't have yet (and I don't have this one in
>> gmail yet even).  But, I am OK with custodians using their repos, but
>> not the master branch, for unrelated but otherwise good patches. I'm
>> also fine with patchwork bundles.  I suppose we could use the staging
>> repository for these changes instead.
>
> What I mostly object about there is that these patches would go into
> mainline basicly unreviewed, as patch submission and pull request is
> all done from a single person, with no other feedback on the patches
> at all.  And this affects a lot of common code...
>
> Actually, I see this change when pulling u-boot-x86.git/master:
>
> -> bloat-o-meter u-boot-before u-boot

What board is this please?

Some specific notes here - I think it boils down to moving crc32 into
the hash framework. This adds some overhead, but has a few benefits.

> add/remove: 9/0 grow/shrink: 3/14 up/down: 1006/-560 (446)
> function                                     old     new   delta
> hash_command                                   -     424    +424

This is the generic hashing command. What is happening here is that
the crc32 command is getting a few more features, more like sha1sum.
However, this might not be desriable - and in fact this patch changes
the behaviour of the CRC storage and verify to support using an
environment variable, and requiring * before the argument when an
address is required! That needs to be fixed, at least.

The intent is to try to unify the hashing/crc features into a single
framework. If you enable only crc32 and nothing else then this has
quite a cost (0.5KB at present). I think I can reduce this code by
making the full features of hash.c only available when something more
than just crc32 is enabled. However, it might involve some #ifdefs...

> strncasecmp                                    -     156    +156
> simple_itoa                                    -     104    +104
> crc32_wd_buf                                   -      76     +76
> setenv_hex                                     -      68     +68
> setenv_ulong                                   -      52     +52
> strcasecmp                                     -      36     +36
> do_mem_loopw                                 304     328     +24
> static.local                                   -      22     +22
> do_mem_loop                                  268     288     +20
> hash_algo                                      -      16     +16
> do_mem_cmp                                   332     340      +8
> do_mem_mw                                    224     220      -4
> set_working_fdt_addr                          72      52     -20
> load_serial_ymodem                           300     280     -20
> load_serial                                  512     492     -20
> index_partitions                             200     180     -20
> do_load_serial_bin                          1844    1824     -20
> do_load                                      468     448     -20
> do_jffs2_fsload                              320     300     -20
> do_imgextract                                636     592     -44
> NetLoop                                      832     788     -44
> do_mem_cp                                    312     252     -60
> do_bootm                                    1244    1180     -64
> do_mem_crc                                   188      88    -100
> do_mem_mtest                                1436    1332    -104
>
>
> So there are changes all over the place, including a growth of the
> memory footprint.  I think this needs at least minimal review.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Time is an illusion perpetrated by the manufacturers of space.
Tom Rini Feb. 18, 2013, 10:45 p.m. UTC | #9
On Mon, Feb 18, 2013 at 09:06:01AM -0800, Simon Glass wrote:
> Hi Wolfgang,
> 
> On Mon, Feb 18, 2013 at 3:35 AM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Tom,
> >
> > In message <51216721.1010603@ti.com> you wrote:
> >>
> >> There's another thread I don't have yet (and I don't have this one in
> >> gmail yet even).  But, I am OK with custodians using their repos, but
> >> not the master branch, for unrelated but otherwise good patches. I'm
> >> also fine with patchwork bundles.  I suppose we could use the staging
> >> repository for these changes instead.
> >
> > What I mostly object about there is that these patches would go into
> > mainline basicly unreviewed, as patch submission and pull request is
> > all done from a single person, with no other feedback on the patches
> > at all.  And this affects a lot of common code...
> >
> > Actually, I see this change when pulling u-boot-x86.git/master:
> >
> > -> bloat-o-meter u-boot-before u-boot
> 
> What board is this please?
> 
> Some specific notes here - I think it boils down to moving crc32 into
> the hash framework. This adds some overhead, but has a few benefits.

So you're going to v2 this part?
Wolfgang Denk Feb. 18, 2013, 11:14 p.m. UTC | #10
Dear Simon,

In message <CAPnjgZ09okKf0qTnifKOrvg37nEnSNLFzyLBLp+4jt6U_L3RCg@mail.gmail.com> you wrote:
> 
> > -> bloat-o-meter u-boot-before u-boot
> 
> What board is this please?

That was TQM5200S

> This is the generic hashing command. What is happening here is that
> the crc32 command is getting a few more features, more like sha1sum.
> However, this might not be desriable - and in fact this patch changes
> the behaviour of the CRC storage and verify to support using an
> environment variable, and requiring * before the argument when an
> address is required! That needs to be fixed, at least.

Indeed - such a change of user interface must not be done here (though
it does make a lot of sense to use common code for this stuff).

> The intent is to try to unify the hashing/crc features into a single

Understood and appreciayed.

> framework. If you enable only crc32 and nothing else then this has
> quite a cost (0.5KB at present). I think I can reduce this code by
> making the full features of hash.c only available when something more
> than just crc32 is enabled. However, it might involve some #ifdefs...

Actually 0.5 k is quite heavy impact, so I guess the #ifdef's win...


Best regards,

Wolfgang Denk
Simon Glass Feb. 19, 2013, 5:24 a.m. UTC | #11
Hi Tom,

On Mon, Feb 18, 2013 at 2:45 PM, Tom Rini <trini@ti.com> wrote:
> On Mon, Feb 18, 2013 at 09:06:01AM -0800, Simon Glass wrote:
>> Hi Wolfgang,
>>
>> On Mon, Feb 18, 2013 at 3:35 AM, Wolfgang Denk <wd@denx.de> wrote:
>> > Dear Tom,
>> >
>> > In message <51216721.1010603@ti.com> you wrote:
>> >>
>> >> There's another thread I don't have yet (and I don't have this one in
>> >> gmail yet even).  But, I am OK with custodians using their repos, but
>> >> not the master branch, for unrelated but otherwise good patches. I'm
>> >> also fine with patchwork bundles.  I suppose we could use the staging
>> >> repository for these changes instead.
>> >
>> > What I mostly object about there is that these patches would go into
>> > mainline basicly unreviewed, as patch submission and pull request is
>> > all done from a single person, with no other feedback on the patches
>> > at all.  And this affects a lot of common code...
>> >
>> > Actually, I see this change when pulling u-boot-x86.git/master:
>> >
>> > -> bloat-o-meter u-boot-before u-boot
>>
>> What board is this please?
>>
>> Some specific notes here - I think it boils down to moving crc32 into
>> the hash framework. This adds some overhead, but has a few benefits.
>
> So you're going to v2 this part?

Yes, sorry for the hold-up, but I think this patch needs a few tweaks.
I may even need to create a separate patch depending on how much I
need to change common/hash.c. Hopefully it is just a case of adding
some #ifdefs (!) and another parameter to select the behaviour
required. I will make time for this tomorrow because then I am away
until next week.

>
> --
> Tom
Simon Glass Feb. 20, 2013, 5:04 p.m. UTC | #12
Hi Tom,

On Mon, Feb 18, 2013 at 2:45 PM, Tom Rini <trini@ti.com> wrote:
> On Mon, Feb 18, 2013 at 09:06:01AM -0800, Simon Glass wrote:
>> Hi Wolfgang,
>>
>> On Mon, Feb 18, 2013 at 3:35 AM, Wolfgang Denk <wd@denx.de> wrote:
>> > Dear Tom,
>> >
>> > In message <51216721.1010603@ti.com> you wrote:
>> >>
>> >> There's another thread I don't have yet (and I don't have this one in
>> >> gmail yet even).  But, I am OK with custodians using their repos, but
>> >> not the master branch, for unrelated but otherwise good patches. I'm
>> >> also fine with patchwork bundles.  I suppose we could use the staging
>> >> repository for these changes instead.
>> >
>> > What I mostly object about there is that these patches would go into
>> > mainline basicly unreviewed, as patch submission and pull request is
>> > all done from a single person, with no other feedback on the patches
>> > at all.  And this affects a lot of common code...
>> >
>> > Actually, I see this change when pulling u-boot-x86.git/master:
>> >
>> > -> bloat-o-meter u-boot-before u-boot
>>
>> What board is this please?
>>
>> Some specific notes here - I think it boils down to moving crc32 into
>> the hash framework. This adds some overhead, but has a few benefits.
>
> So you're going to v2 this part?

I have just sent 3 v2 patches to the mailing list. There is a new
patch 16 to be inserted before this patch, an updated version of this
patch, and an updated version of the final patch in the series (which
was number 20 and is now number 21).

I have ended up just using #ifdef to remove most of the hash code if
only crc32 is in use. It's not particularly elegant but I was not able
to significantly reduce the code size impact any other way. Please
take a look at it and see what you think.

Just in case it is useful, I have pushed a new 'us-mem3' branch to
x86, but this is not for pulling (it has not been through patchwork),
just for testing / comparison.

I will be away until Monday so probably no further response from me until then.

Regards,
Simon

>
> --
> Tom
diff mbox

Patch

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index c45a31e..701157d 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -32,6 +32,7 @@ 
 #ifdef CONFIG_HAS_DATAFLASH
 #include <dataflash.h>
 #endif
+#include <hash.h>
 #include <watchdog.h>
 #include <asm/io.h>
 
@@ -1093,89 +1094,27 @@  mod_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char * const argv[])
 
 #ifdef CONFIG_CMD_CRC32
 
-#ifndef CONFIG_CRC32_VERIFY
-
 static int do_mem_crc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	ulong addr, length;
-	ulong crc;
-	ulong *ptr;
-
-	if (argc < 3)
-		return CMD_RET_USAGE;
-
-	addr = simple_strtoul (argv[1], NULL, 16);
-	addr += base_address;
-
-	length = simple_strtoul (argv[2], NULL, 16);
-
-	crc = crc32_wd (0, (const uchar *) addr, length, CHUNKSZ_CRC32);
-
-	printf ("CRC32 for %08lx ... %08lx ==> %08lx\n",
-			addr, addr + length - 1, crc);
-
-	if (argc > 3) {
-		ptr = (ulong *) simple_strtoul (argv[3], NULL, 16);
-		*ptr = crc;
-	}
-
-	return 0;
-}
-
-#else	/* CONFIG_CRC32_VERIFY */
-
-int do_mem_crc (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	ulong addr, length;
-	ulong crc;
-	ulong *ptr;
-	ulong vcrc;
-	int verify;
+	int verify = 0;
 	int ac;
 	char * const *av;
 
-	if (argc < 3) {
-usage:
+	if (argc < 3)
 		return CMD_RET_USAGE;
-	}
 
 	av = argv + 1;
 	ac = argc - 1;
+#ifdef CONFIG_HASH_VERIFY
 	if (strcmp(*av, "-v") == 0) {
 		verify = 1;
 		av++;
 		ac--;
-		if (ac < 3)
-			goto usage;
-	} else
-		verify = 0;
-
-	addr = simple_strtoul(*av++, NULL, 16);
-	addr += base_address;
-	length = simple_strtoul(*av++, NULL, 16);
-
-	crc = crc32_wd (0, (const uchar *) addr, length, CHUNKSZ_CRC32);
-
-	if (!verify) {
-		printf ("CRC32 for %08lx ... %08lx ==> %08lx\n",
-				addr, addr + length - 1, crc);
-		if (ac > 2) {
-			ptr = (ulong *) simple_strtoul (*av++, NULL, 16);
-			*ptr = crc;
-		}
-	} else {
-		vcrc = simple_strtoul(*av++, NULL, 16);
-		if (vcrc != crc) {
-			printf ("CRC32 for %08lx ... %08lx ==> %08lx != %08lx ** ERROR **\n",
-					addr, addr + length - 1, crc, vcrc);
-			return 1;
-		}
 	}
+#endif
 
-	return 0;
-
+	return hash_command("crc32", verify, cmdtp, flag, ac, av);
 }
-#endif	/* CONFIG_CRC32_VERIFY */
 
 #endif
 
diff --git a/common/hash.c b/common/hash.c
index e3a6e43..e92b4b1 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -50,6 +50,12 @@  static struct hash_algo hash_algo[] = {
 		CHUNKSZ_SHA256,
 	},
 #endif
+	{
+		"CRC32",
+		4,
+		crc32_wd_buf,
+		CHUNKSZ_CRC32,
+	},
 };
 
 /**
diff --git a/include/hash.h b/include/hash.h
index 34ba558..ba2ba65 100644
--- a/include/hash.h
+++ b/include/hash.h
@@ -22,7 +22,7 @@ 
 #ifndef _HASH_H
 #define _HASH_H
 
-#ifdef CONFIG_SHA1SUM_VERIFY
+#if defined(CONFIG_SHA1SUM_VERIFY) || defined(CONFIG_CRC32_VERIFY)
 #define CONFIG_HASH_VERIFY
 #endif
 
diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h
index 07badbf..08e509e 100644
--- a/include/u-boot/crc.h
+++ b/include/u-boot/crc.h
@@ -30,4 +30,15 @@  uint32_t crc32 (uint32_t, const unsigned char *, uint);
 uint32_t crc32_wd (uint32_t, const unsigned char *, uint, uint);
 uint32_t crc32_no_comp (uint32_t, const unsigned char *, uint);
 
+/**
+ * crc32_wd_buf - Perform CRC32 on a buffer and return result in buffer
+ *
+ * @input:	Input buffer
+ * @ilen:	Input buffer length
+ * @output:	Place to put checksum result (4 bytes)
+ * @chunk_sz:	Trigger watchdog after processing this many bytes
+ */
+void crc32_wd_buf(const unsigned char *input, uint ilen,
+		    unsigned char *output, uint chunk_sz);
+
 #endif /* _UBOOT_CRC_H */
diff --git a/lib/crc32.c b/lib/crc32.c
index 27335a3..76205da 100644
--- a/lib/crc32.c
+++ b/lib/crc32.c
@@ -249,3 +249,12 @@  uint32_t ZEXPORT crc32_wd (uint32_t crc,
 
 	return crc;
 }
+
+void crc32_wd_buf(const unsigned char *input, unsigned int ilen,
+		unsigned char *output, unsigned int chunk_sz)
+{
+	uint32_t crc;
+
+	crc = crc32_wd(0, input, ilen, chunk_sz);
+	memcpy(output, &crc, sizeof(crc));
+}