On Mon, 31 Jan 2022, BALATON Zoltan wrote:
On Mon, 31 Jan 2022, Glenn Washburn wrote:
On Mon, 31 Jan 2022 18:40:05 +0100 (CET) BALATON Zoltan balaton@eik.bme.hu wrote:
On Mon, 31 Jan 2022, Glenn Washburn wrote:
Instead of using a constant frequency for all CPUs, which have varying frequencies, use the time frequency as returned by the processor. This fixes issues where delays from ciface_milliseconds were not corresponding to elapsed time as given by the real time clock on certain platforms, eg. QEMU's mac99 machine.
Signed-off-by: Glenn Washburn development@efficientek.com
Mark, if I missed something in the commit message, feel free to make edits as you see fit.
Glenn
arch/ppc/qemu/methods.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/ppc/qemu/methods.c b/arch/ppc/qemu/methods.c index 930b47c..a423b9f 100644 --- a/arch/ppc/qemu/methods.c +++ b/arch/ppc/qemu/methods.c @@ -126,7 +126,8 @@ ciface_quiesce( unsigned long args[], unsigned long ret[] ) }
/* ( -- ms ) */ -#define TIMER_FREQUENCY 16600000ULL +/* From drivers/timer.c */ +extern unsigned long timer_freq;
Hmm, drivers/timer.h has a function get_timer_freq(void) declared which is used by similar methods in arch/ppc/briq and arch/ppc/pearpc (although not including the header but declaring the function locally) but the actual definition of that function seems to be missing. Maybe it would be better to find out where did it go and do it similarly in qemu as in other variants (which are apparently not working now but I haven't tried this, just had a brief look trying to review this patch but quickly gave up after finding this so only have this comment to add). If we don't care about those other versions this patch may work too.
I tried using get_timer_freq() and noticed the same thing. I ended up going with what Mark suggested and is already done in arch/ppc/qemu/init.c (hence the exact same comment). While I like in general your suggestion, I'm not interested in doing that work. I don't use those versions (nor do I use PPC in general, I'm solely interesteed to the extent that I can make improvements to GRUB's testing).
I understand that and did not mean to say you should do it, just broght up what I've found looking at the patch. It's up to Mark to decide if he'll take this patch as is (as it would fix the problem and help you at least) or goes after the missing function to find out what happened to this in the past. Maybe the git log could have some history but I wasn't interested enough to find that out.
Looks like it was in commit f1fb52d6884180e2c32631507cad57fdc203b3e6 which replaced the previous function returning a fixed value with a global variable. Maybe the function should be kept while introducing the global so it does not break existing code. I've also noticed that the QEMU version has more complex code for getting timebase value from CPU than other similar functions that just do an mftb. This was fixed in e85a0d9bdebc77e40629428d8d8f9081373c00c4 but only for qemu. I wonder if this function should be somewhere in arch/ppc instead common for all variants replacing local variants? We also have timebase.S there which already has a similar get_ticks function defined.
Regards, BALATON Zoltan