[U-Boot] CONFIG_SYS_HZ: Please revert breaking net commit

[U-Boot] CONFIG_SYS_HZ: Please revert breaking net commit

[Detlef Vollmann]

Hello,

 From CHANGELOG:
<quote>
commit 49f3bdbba8071f56d950a9498b6cdb998b35340a
Author: Bartlomiej Sieka <tur@semihalf.com>
Date:   Wed Oct 1 15:26:28 2008 +0200

     net: express the first argument to NetSetTimeout() in milliseconds

     Enforce millisecond semantics of the first argument to 
NetSetTimeout() --
     the change is transparent for well-behaving boards (CFG_HZ == 1000 and
     get_timer() countiing in milliseconds).

     Rationale for this patch is to enable millisecond granularity for
     network-related timeouts, which is needed for the upcoming automatic
     software update feature.

     Summary of changes:
     - do not scale the first argument to NetSetTimeout() by CFG_HZ
     - change timeout values used in the networking code to milliseconds

     Signed-off-by: Rafal Czubak <rcz@semihalf.com>
     Signed-off-by: Bartlomiej Sieka <tur@semihalf.com>
     Signed-off-by: Ben Warren <biggerbadderben@gmail.com>
</quote>

This commit breaks *silently* all boards that don't return
milliseconds from get_timer, i.e. have a different value for
CONFIG_SYS_HZ than 1000.
I'm open to any discussion about removing CONFIG_SYS_HZ, but
if it is removed, it should be removed explicitely, and not
just silently ignored.

So I see following reasons to revert the above commit:
  - silently breaks existing boards
  - silently ignores a still valid CONFIG setting
  - bad programming style anyway: even if CONFIG_SYS_HZ would
    go away, there should be some internal define that should
    be used in code
  - the intended benefit 'millisecond granularity for network-related
    timeouts' can be achieved while still keeping CONFIG_SYS_HZ

Best Regards,
   Detlef

-- 
Detlef Vollmann   vollmann engineering gmbh
Linux and C++ for Embedded Systems    http://www.vollmann.ch/

[U-Boot] CONFIG_SYS_HZ: Please revert breaking net commit

[Wolfgang Denk]

Dear Detlef Vollmann,

In message <49A44806.9050902@vollmann.ch> you wrote:
> 
> commit 49f3bdbba8071f56d950a9498b6cdb998b35340a
...
> This commit breaks *silently* all boards that don't return
> milliseconds from get_timer, i.e. have a different value for
> CONFIG_SYS_HZ than 1000.

No, this is not true. All these boards ARE already broken because they
use anincorrect timer setup. The patch just makes the problem visible.

> I'm open to any discussion about removing CONFIG_SYS_HZ, but
> if it is removed, it should be removed explicitely, and not
> just silently ignored.

There is no need in the code to use CONFIG_SYS_HZ, because the code
just used get_timer() as specified, i. e. it expects correct
behaviour.  If your implementation is broken, you should fix your
implementation.

BTW: how comes it takes you nearly half a year to detect this?

> So I see following reasons to revert the above commit:
>   - silently breaks existing boards

These boards were already broken before.

>   - silently ignores a still valid CONFIG setting

It does not ignore it. It does not need it, because it is specified
that get_timer() uses milisecond resolution.

>   - bad programming style anyway: even if CONFIG_SYS_HZ would
>     go away, there should be some internal define that should
>     be used in code

I don't understand what you mean.

>   - the intended benefit 'millisecond granularity for network-related
>     timeouts' can be achieved while still keeping CONFIG_SYS_HZ

Instead of complaining about a patch that was discussed and cehcekd
in many months ago you could rather spend your time on fixing your
broken boards.

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 at denx.de
This is now.  Later is later.