On Fri, 6 Jun 2014, Mark Cave-Ayland wrote:
SPARC64 and PPC. I guess something in arch/ppc must try and redefine the milliseconds word somewhere - if you could locate the duplicate definition (and even better submit a patch to remove it), that would be a great help. As you can see, I'm a bit short on time at the moment...
I've found these:
arch/ppc/briq/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/briq/methods.c: { "milliseconds", ciface_milliseconds }, arch/ppc/mol/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/mol/methods.c: { "milliseconds", ciface_milliseconds }, arch/ppc/pearpc/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/pearpc/methods.c: { "milliseconds", ciface_milliseconds }, arch/ppc/qemu/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/qemu/methods.c: { "milliseconds", ciface_milliseconds },
Should these all be removed? They all seem to be different. Is the default (dummy) implementation now better instead of these?
Regards, BALATON Zoltan
On 06/06/14 11:15, BALATON Zoltan wrote:
On Fri, 6 Jun 2014, Mark Cave-Ayland wrote:
SPARC64 and PPC. I guess something in arch/ppc must try and redefine the milliseconds word somewhere - if you could locate the duplicate definition (and even better submit a patch to remove it), that would be a great help. As you can see, I'm a bit short on time at the moment...
I've found these:
arch/ppc/briq/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/briq/methods.c: { "milliseconds", ciface_milliseconds }, arch/ppc/mol/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/mol/methods.c: { "milliseconds", ciface_milliseconds }, arch/ppc/pearpc/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/pearpc/methods.c: { "milliseconds", ciface_milliseconds }, arch/ppc/qemu/methods.c:ciface_milliseconds( unsigned long args[], unsigned long ret[] ) arch/ppc/qemu/methods.c: { "milliseconds", ciface_milliseconds },
Should these all be removed? They all seem to be different. Is the default (dummy) implementation now better instead of these?
Heh well that's the first time I've noticed that code. It's completely wrong - for starters, it uses a fixed TB instead of using the one from QEMU so it's not going to measure time accurately anyhow.
The correct thing to do is similar to SPARC in that you create a 100Hz interrupt (possibly using the PPC decrementer?) that should increment a variable representing the number of ms elapsed. All you need to do is then point the new code towards that variable and it will handle get-msecs and milliseconds automatically.
The interrupt routine is probably do-able in about 20 lines or less of PPC asm but it's beyond my current PPC-fu. It would be a good project for you though, and would probably fix the incorrect timing CIF milliseconds with -M mac99 :)
ATB,
Mark.
The correct thing to do is similar to SPARC in that you create a 100Hz interrupt (possibly using the PPC decrementer?) that should increment a variable representing the number of ms elapsed. All you need to do is then point the new code towards that variable and it will handle get-msecs and milliseconds automatically.
The interrupt routine is probably do-able in about 20 lines or less of PPC asm but it's beyond my current PPC-fu. It would be a good project for you though, and would probably fix the incorrect timing CIF milliseconds with -M mac99 :)
There is no reason at all to use the decrementer and interrupts, with all the problems inherent in that; just use the timebase, it's what it's for. The decrementer and timebase tick at the same frequency.
Segher
On Sun, 8 Jun 2014, Segher Boessenkool wrote:
The correct thing to do is similar to SPARC in that you create a 100Hz interrupt (possibly using the PPC decrementer?) that should increment a variable representing the number of ms elapsed. All you need to do is then point the new code towards that variable and it will handle get-msecs and milliseconds automatically.
The interrupt routine is probably do-able in about 20 lines or less of PPC asm but it's beyond my current PPC-fu. It would be a good project for you though, and would probably fix the incorrect timing CIF milliseconds with -M mac99 :)
There is no reason at all to use the decrementer and interrupts, with all the problems inherent in that; just use the timebase, it's what it's for. The decrementer and timebase tick at the same frequency.
That's what I thought and the current code already does that only using the wrong TBFREQ value. This could be easily fixed the same way as is done in arch/ppc/qemu/init.c:223:
/* From drivers/timer.c */ extern unsigned long timer_freq;
but probably it would be better to clean this up at the same time. The definition in drivers/timer.c:79 says:
/* * TODO: pass via lb table */ unsigned long timer_freq = 10000000 / 4;
What is the lb table and where is it?
Regards, BALATON Zoltan
On 08/06/14 13:02, BALATON Zoltan wrote:
The interrupt routine is probably do-able in about 20 lines or less of PPC asm but it's beyond my current PPC-fu. It would be a good project for you though, and would probably fix the incorrect timing CIF milliseconds with -M mac99 :)
There is no reason at all to use the decrementer and interrupts, with all the problems inherent in that; just use the timebase, it's what it's for. The decrementer and timebase tick at the same frequency.
That's what I thought and the current code already does that only using the wrong TBFREQ value. This could be easily fixed the same way as is done in arch/ppc/qemu/init.c:223:
Part of the motivation for the previous timer changes was to ensure that we only drop into C for the minimum time required for the architecture-specific code.
Currently that code was based on accessing a counter updated via interrupt, and given that this is the way it works on all other architectures then it makes more sense to do things this way. I appreciate that the accessing the timebase register directly is the more PPC way to do this, however there are other features such as alarms that aren't yet implemented in OpenBIOS which would require Forth routines to be invoked under timer interrupt control, and so if we're changing this I'd like to do it in a way that doesn't rule out this option later.
/* From drivers/timer.c */ extern unsigned long timer_freq;
but probably it would be better to clean this up at the same time. The definition in drivers/timer.c:79 says:
/*
- TODO: pass via lb table
*/ unsigned long timer_freq = 10000000 / 4;
What is the lb table and where is it?
I would hazard a guess that "lb table" means "LinuxBIOS table" which I would imagine is similar to the QEMU fw_cfg interface?
ATB,
Mark.
On Sun, 8 Jun 2014, Mark Cave-Ayland wrote:
On 08/06/14 13:02, BALATON Zoltan wrote:
The interrupt routine is probably do-able in about 20 lines or less of PPC asm but it's beyond my current PPC-fu. It would be a good project for you though, and would probably fix the incorrect timing CIF milliseconds with -M mac99 :)
There is no reason at all to use the decrementer and interrupts, with all the problems inherent in that; just use the timebase, it's what it's for. The decrementer and timebase tick at the same frequency.
That's what I thought and the current code already does that only using the wrong TBFREQ value. This could be easily fixed the same way as is done in arch/ppc/qemu/init.c:223:
Part of the motivation for the previous timer changes was to ensure that we only drop into C for the minimum time required for the architecture-specific code.
Currently that code was based on accessing a counter updated via interrupt, and given that this is the way it works on all other architectures then it makes more sense to do things this way. I appreciate that the accessing the timebase register directly is the more PPC way to do this, however there are other features such as alarms that aren't yet implemented in OpenBIOS which would require Forth routines to be invoked under timer interrupt control, and so if we're changing this I'd like to do it in a way that doesn't rule out this option later.
I leave this to someone else. Apart from the message I see no negative effects so far so it's not a priority for me. (Nothing seems to use the milliseconds callback.)
Regards, BALATON Zoltan
-
BALATON Zoltan -
Mark Cave-Ayland -
Segher Boessenkool