[ARM] fix the calculation of the values in the 24 MHz clock

Information
Submitter:	Koan-Sin Tan
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

fix the calculation of the value in the system register for the 24 MHz clock

Let x be the value,
  currTick() / Frequency = x / (24 x 10^6) 
  x = curTick() * 24 / SimClock::Int::us

on realview platform, sched_clock() relies on the 24 MHz. So does the timestamp for printk(). The original calculation is problematic.


Similar calculation for the 100 Hz register. 
Let x be the value,
  currTick() / Frequency = x  / 100 
  x = curTick() * 100 / Frequency

Testing Done

Issue Summary

Description	From	Last Updated	Status
Why not just make it 24.0? Alternatively (curTick() / us) * 24?	Andreas Hansson	May 8, 2012, 8:31 p.m.	Open
What about 100 MHz clock?	Ali Saidi	May 9, 2012, 12:47 a.m.	Open
Very minor thing, but we might as well put this on one line: Tick clk = ...;	Andreas Hansson	May 9, 2012, 6:36 p.m.	Open
For symmetry, could this be SimClock::Int::s?	Andreas Hansson	May 9, 2012, 6:36 p.m.	Open

Summary:

-	fix the calculation of value in the 24 MHz clock
+	[ARM] fix the calculation of the value in the 24 MHz clock

Description:

~		fix the calculation of value in the system register for 24 MHz clock
	~	fix the calculation of the value in the system register for the 24 MHz clock

		Let x be the value,
		currTick() / Frequency = x / (24 x 10^6)
		x = curTick() * 24 / SimClock::Int::us

~		on realview platform, sched_clock() replies on the 24 MHz. So does the timestamp for printk(). The original calculation is problematic.
	~	on realview platform, sched_clock() relies on the 24 MHz. So does the timestamp for printk(). The original calculation is problematic.

src/dev/arm/rv_ctrl.cc (Diff revision 1)

Why not just make it 24.0? Alternatively (curTick() / us) * 24?

Show all issues

Koan-Sin Tan May 8, 2012, 9:30 p.m. (May 8, 2012, 9:30 p.m.)

it's not 24, by default, Frequency = 10^12, SimClock::Int::us = 10^ 12 / 10 ^6 = 10 ^ 6
For  (curTick() / us) * 24, I thought  (curTick()  * 24 / us) should be more precise.

src/dev/arm/rv_ctrl.cc (Diff revision 1)

What about 100 MHz clock?

Show all issues

Koan-Sin Tan May 9, 2012, 11:19 a.m. (May 9, 2012, 11:19 a.m.)

As far as I can tell, this should be a 100 Hz clock [1].

  curTick() / Frequency = x / 100 
  x = curTick * 100 / Frequency

So, it should be  clk100 = (Tick) (curTick() * 100 / SimClock::Frequency);

I don't know who uses this 100 Hz clock.
I happend to find the 24 MHz problem because when I noticed the kernel jiffies variable (which uses sched_clock())  is running too slow.

[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0440b/I1006122.html

Thanks for noticing this and providing a fix. I'll try to commit it in the next few days.

Change Summary:

Add fix for the 100 Hz clock. See previous discussion with Ali. I don't know if Linux kernel uses it though.

Summary:

-	[ARM] fix the calculation of the value in the 24 MHz clock
+	[ARM] fix the calculation of the values in the 24 MHz clock

Description:

		fix the calculation of the value in the system register for the 24 MHz clock

		Let x be the value,
		currTick() / Frequency = x / (24 x 10^6)
		x = curTick() * 24 / SimClock::Int::us

		on realview platform, sched_clock() relies on the 24 MHz. So does the timestamp for printk(). The original calculation is problematic.
	+
	+
	+
	+	Similar calculation for the 100 Hz register.
	+	Let x be the value,
	+	currTick() / Frequency = x / 100
	+	x = curTick() * 100 / Frequency

Diff:

Revision 2 (+4 -4)

Show changes

	src/dev/arm/rv_ctrl.hh
	src/dev/arm/rv_ctrl.cc

src/dev/arm/rv_ctrl.cc (Diff revision 2)

Very minor thing, but we might as well put this on one line:

Tick clk = ...;

Show all issues

Koan-Sin Tan May 10, 2012, 12:54 a.m. (May 10, 2012, 12:54 a.m.)

well, clang will complain, it gave me something like, 

   build/ARM/dev/arm/rv_ctrl.cc:70:14: note: jump bypasses variable initialization        Tick clk = (Tick)(curTick() * 24 / SimClock::Int::us);

And g++ 4.2 said:
  build/ARM/dev/arm/rv_ctrl.cc:73: error: jump to case label
  build/ARM/dev/arm/rv_ctrl.cc:70: error:   crosses initialization of ‘Tick clk’
   
And, yes, add {} will eliminate the error. But that's weird

Andreas Hansson May 10, 2012, 1:29 a.m. (May 10, 2012, 1:29 a.m.)

Very valid point indeed.

It's rather ironic that not initializing is fine. I suppose the other option is to not have the temporary variable and do the assignment directly in setData.

src/dev/arm/rv_ctrl.cc (Diff revision 2)

For symmetry, could this be SimClock::Int::s?

Show all issues

Koan-Sin Tan May 10, 2012, 12:54 a.m. (May 10, 2012, 12:54 a.m.)
```
yes, I can do that
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Summary:

Description:

Change Summary:

Summary:

Description:

Diff:

Status: Closed (submitted)