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

[Daniel Mack]

Hi,

On Fri, Dec 05, 2008 at 09:26:27PM +0100, Wolfgang Denk wrote:
> >  net/net.c |   15 ++++++++++-----
> >  1 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index 77e83b5..1c48236 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
> >  ulong		NetArpWaitTimerStart;
> >  int		NetArpWaitTry;
> >  
> > +static long get_timer_ms(long base)
> > +{
> > +	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
> > +}
> > +
> 
> 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.

That's what I've experienced, and as I didn't find a proper place to fix
it at a sane level, I fixed the problem locally. I agree that this might
not be the best place, so I'll happily accept better proposals.

> So  in  a  correct  configuration  get_timer_ms()  is  the  same   as
> get_timer(),  and  if  CONFIG_SYS_HZ is (incorrectly) not set to 1000

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?

> while  get_timer()  is  implemented  correctly,  then  get_timer_ms()
> willnot do what it claims to do.

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

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

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.

Best regards,
Daniel