[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Wolfgang Denk]

Dear Daniel Mack,

In message <20081205204357.GD3940 at buzzloop.caiaq.de> you wrote:
> 
> > This is by definition a NO-OP  at  best,  and  misleading  and  wrong
> > otherwise.  get_timer()  is defined to return millisecond resolution,
> > and CONFIG_SYS_HZ is supposed to be 1000.
> 
> The timer implementation (at least the one for PXA processors) assumes
> that the OSCR register increments 1000 times a second. Which it doesn't
> for PXA3xx variants. Hence, all functions from cpu/pxa/interrupts.c will
> behave entirely differently on a PXA270 compared to a PXA3xx, and so all
> code using this functions will break.

So this is a bug, and needs to be fixed.

> Why is a CONFIG_SYS_HZ != 1000 considered incorrect? Or let me spin it
> that way: if that's incorrect, what does this variable exist for at all?

It exists only for historic reasons, I used it  to  optimize  between
timer  accuracy  and  overhead on slow (33 MHz) MP8xx systems some 8+
years ago. It has been considered a constant of 1000 ever since. Only
some broken ports used it differently. Unfortunately this went
unnoticed way too long.

> What is get_timer() supposed to return, anyway? I didn't find any
> documentation about it and assumed that it straightly returns the
> primary system timer of the CPU (which it perfectly does for PXAs).

get_timer(base) returns the number of milliseconds since "base".

> > Not to mention what happens if someone has CONFIG_SYS_HZ defined as
> > 999, for example.
> 
> Not sure whether I got your point here. If the system timer increments
> 999 times per second and CONFIG_SYS_HZ is set accordingly, my function
> does the right thing, doesn't it? I'm not up to any flamewar, I just
> want to understand where you see the problem.

It doesn't:

	return get_timer(base) / (CONFIG_SYS_HZ / 1000);

gives

	return get_timer(base) / (999 / 1000);

which gives

	return get_timer(base) / 0;

which gives a problem.

> As fas as I understand the big picture, a function like mine should
> exist somewhere in the code. Probably not in net/net.c, though.

No, it should not exist at all. It is  not  needed  for  all  correct
implementations where get_timer() is already returning milliseconds.

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
Do not simplify the design of a program if a way can be found to make
it complex and wonderful.