[PATCH] ppc: fix ciface_milliseconds using incorrect frequency for delay
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; static void ciface_milliseconds( unsigned long args[], unsigned long ret[] ) @@ -146,7 +147,7 @@ ciface_milliseconds( unsigned long args[], unsigned long ret[] ) : "cc"); ticks = (((unsigned long long)tbu) << 32) | (unsigned long long)tbl; - msecs = (1000 * ticks) / TIMER_FREQUENCY; + msecs = (1000 * ticks) / timer_freq; PUSH( msecs ); } -- 2.30.2
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. Regards, BALATON Zoltan
static void ciface_milliseconds( unsigned long args[], unsigned long ret[] ) @@ -146,7 +147,7 @@ ciface_milliseconds( unsigned long args[], unsigned long ret[] ) : "cc");
ticks = (((unsigned long long)tbu) << 32) | (unsigned long long)tbl; - msecs = (1000 * ticks) / TIMER_FREQUENCY; + msecs = (1000 * ticks) / timer_freq; PUSH( msecs ); }
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). Glenn
Regards, BALATON Zoltan
static void ciface_milliseconds( unsigned long args[], unsigned long ret[] ) @@ -146,7 +147,7 @@ ciface_milliseconds( unsigned long args[], unsigned long ret[] ) : "cc");
ticks = (((unsigned long long)tbu) << 32) | (unsigned long long)tbl; - msecs = (1000 * ticks) / TIMER_FREQUENCY; + msecs = (1000 * ticks) / timer_freq; PUSH( msecs ); }
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. Regards, BALATON Zoltan
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
On 01/02/2022 11:44, BALATON Zoltan wrote:
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.
To the best of my knowledge the briq and pearpc variants aren't being used anywhere, or at least no-one has asked about them on the mailing list. I'd suspect that if someone were interested enough to resurrect them then that there would be a number of things that would require fixing up.
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.
The long term aim is to add a small interrupt routine to increment a Forth variable as is already done for SPARC64, so then the delay logic can be implemented in common Forth code rather than re-implement get_timer_freq(). So for now I think the patch is the best solution. ATB, Mark.
On Mon, 31 Jan 2022 11:15:04 -0600 Glenn Washburn <development@efficientek.com> 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.
Meant to add a "Suggested-By:" tag here for you Mark. So please add if you desire. Glenn
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;
static void ciface_milliseconds( unsigned long args[], unsigned long ret[] ) @@ -146,7 +147,7 @@ ciface_milliseconds( unsigned long args[], unsigned long ret[] ) : "cc");
ticks = (((unsigned long long)tbu) << 32) | (unsigned long long)tbl; - msecs = (1000 * ticks) / TIMER_FREQUENCY; + msecs = (1000 * ticks) / timer_freq; PUSH( msecs ); }
On 31/01/2022 17:15, 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
I think just a small change to the first sentence: "Instead of using a constant frequency for all CPUs, use the processor timebase frequency provided by QEMU". Other than that it looks good to me, so I'm happy to apply it to master if no-one else has any further comments.
--- 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;
static void ciface_milliseconds( unsigned long args[], unsigned long ret[] ) @@ -146,7 +147,7 @@ ciface_milliseconds( unsigned long args[], unsigned long ret[] ) : "cc");
ticks = (((unsigned long long)tbu) << 32) | (unsigned long long)tbl; - msecs = (1000 * ticks) / TIMER_FREQUENCY; + msecs = (1000 * ticks) / timer_freq; PUSH( msecs ); }
ATB, Mark.
On 05/02/2022 10:41, Mark Cave-Ayland wrote:
On 31/01/2022 17:15, 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
I think just a small change to the first sentence: "Instead of using a constant frequency for all CPUs, use the processor timebase frequency provided by QEMU".
Other than that it looks good to me, so I'm happy to apply it to master if no-one else has any further comments.
--- 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; static void ciface_milliseconds( unsigned long args[], unsigned long ret[] ) @@ -146,7 +147,7 @@ ciface_milliseconds( unsigned long args[], unsigned long ret[] ) : "cc"); ticks = (((unsigned long long)tbu) << 32) | (unsigned long long)tbl; - msecs = (1000 * ticks) / TIMER_FREQUENCY; + msecs = (1000 * ticks) / timer_freq; PUSH( msecs ); }
Thanks - no further comments, so I've pushed this to git master with the updated commit message. ATB, Mark.
participants (3)
-
BALATON Zoltan -
Glenn Washburn -
Mark Cave-Ayland