Dear Daniel Mack,
In message 20081205204357.GD3940@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