Change in ...coreboot[master]: arch/riscv: Fix arch timer timebase

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33797

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

arch/riscv: Fix arch timer timebase

The arch timer uses a platform specific clock source.
Add a Kconfig that holds this clock frequency and use it to calculate
the time passed by.

Tested on qemu-system-riscv:
Fixes delays being ten times shorter as expected.

Change-Id: I2588149e2ee32130a2c41695c4c723b57d4fa827
Signed-off-by: Patrick Rudolph 
---
M src/arch/riscv/Kconfig
M src/arch/riscv/arch_timer.c
M src/soc/sifive/fu540/Kconfig
M src/soc/ucb/riscv/Kconfig
4 files changed, 22 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/33797/1

diff --git a/src/arch/riscv/Kconfig b/src/arch/riscv/Kconfig
index 25a3980..6b12ff0 100644
--- a/src/arch/riscv/Kconfig
+++ b/src/arch/riscv/Kconfig
@@ -72,5 +72,9 @@
    bool
    default n

+config RISCV_ARCH_TIMER_FREQ
+	int
+	depends on RISCV_USE_ARCH_TIMER
+
 config RISCV_WORKING_HARTID
    int
diff --git a/src/arch/riscv/arch_timer.c b/src/arch/riscv/arch_timer.c
index 55b1f72..71b9771 100644
--- a/src/arch/riscv/arch_timer.c
+++ b/src/arch/riscv/arch_timer.c
@@ -20,9 +20,18 @@
 #include <timer.h>
 #include <mcall.h>

+/* mtime is clocked by platform specific clock source */
+static const uint32_t clocks_per_usec = CONFIG_RISCV_ARCH_TIMER_FREQ/1000000;
+
 void timer_monotonic_get(struct mono_time *mt)
 {
+	uint64_t raw;
+
    if (HLS()->time == NULL)
    	die("time not set in HLS");
-	mono_time_set_usecs(mt, (long)read64((void *)(HLS()->time)));
+	if (!clocks_per_usec)
+		die("Unsupported mtime clock frequency");
+
+	raw = read64((void *)(HLS()->time));
+	mono_time_set_usecs(mt, (long)(raw / clocks_per_usec));
 }
diff --git a/src/soc/sifive/fu540/Kconfig b/src/soc/sifive/fu540/Kconfig
index 6ebde33..a10a146 100644
--- a/src/soc/sifive/fu540/Kconfig
+++ b/src/soc/sifive/fu540/Kconfig
@@ -49,4 +49,8 @@
    int
    default 0

+config RISCV_ARCH_TIMER_FREQ
+	int
+	default 1000000
+
 endif
diff --git a/src/soc/ucb/riscv/Kconfig b/src/soc/ucb/riscv/Kconfig
index ad48c1c..49887a4 100644
--- a/src/soc/ucb/riscv/Kconfig
+++ b/src/soc/ucb/riscv/Kconfig
@@ -14,6 +14,10 @@

if SOC_UCB_RISCV

+config RISCV_ARCH_TIMER_FREQ
+	int
+	default 10000000
+
 if ARCH_RISCV_RV64

config RISCV_ARCH

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 1: Code-Review+1

I guess this is fine as a quick fix. Ideally, the timebase should be runtime detected, think MSRs of the x86 world, but simpler.

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c 
File src/arch/riscv/arch_timer.c:

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@33 
PS1, Line 33: 		die("Unsupported mtime clock frequency");
Could you use static assert instead?
Better to check at build time

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+1
>
> I guess this is fine as a quick fix. Ideally, the timebase should be runtime detected, think MSRs of the x86 world, but simpler.

I couldn't find a CSR that reflects the clock frequency. It looks like that on fu540 it's an external clock driving mtime counter that board manufacturer should connect to 1MHz.
Do you have additional documentation how to do runtime detection?

[ron minnich (Code Review)]

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c 
File src/arch/riscv/arch_timer.c:

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@31 
PS1, Line 31: 		die("time not set in HLS");
does this HAVE to be a die? Can we somehow stumble along with bad values?

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@33 
PS1, Line 33: 		die("Unsupported mtime clock frequency");
> Could you use static assert instead? […]

Like Philip said. It's depressing to configure a value wrong and end up with a basic brick :-)

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 1:

It looks like the property 'timebase-frequency' in FDT provided by the BOOTROM holds the frequency we are looking for. Reading it would require to write a devicetree parser that works in _PRE_RAM_ environments (which might also be useful on other platforms to read the amount of DRAM installed).

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c 
File src/arch/riscv/arch_timer.c:

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@31 
PS1, Line 31: 		die("time not set in HLS");
> does this HAVE to be a die? Can we somehow stumble along with bad values?

Well 'die()' is just another 'printk()' called before 'console_init()' here that would not show up, even on UART TX line.

With 'printk()' called after 'console_init()', and if UART code hits 'udelay()', we have infinite recursion here, possibly a silent reset loop is what you will see.

It's possibly me who put 'udelay()' there in the UART_MEM code, ideas are welcome on how to get it removed while still having some controlled timeouts there. Timeouts don't have to be accurate but reasonable.

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@36 
PS1, Line 36: 	mono_time_set_usecs(mt, (long)(raw / clocks_per_usec));
unsigned long ?

There is a fair amount of loss in accuracy should you want to support timebases that are not multiples of 1 MHz. HW timer running at 1.999 MHz would be evil. Consider leaving a comment about that or improve the math.

[Patrick Rudolph (Code Review)]

Hello Kyösti Mälkki, ron minnich, Jonathan Neuschäfer, Philipp Deppenwiese, build bot (Jenkins), Philipp Hug, Xiang Wang,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33797

to look at the new patch set (#2).

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

arch/riscv: Fix arch timer timebase

The arch timer uses a platform specific clock source.
Add a Kconfig that holds this clock frequency and use it to calculate
the time passed by.

Update Kconfig and Documentation with a TODO:
Implement bootblock FDT parser and read 'timebase-frequency' instead
of using a Kconfig.

Tested on qemu-system-riscv:
Fixes delays being ten times shorter as expected.

Change-Id: I2588149e2ee32130a2c41695c4c723b57d4fa827
Signed-off-by: Patrick Rudolph 
---
M Documentation/arch/riscv/index.md
M src/arch/riscv/Kconfig
M src/arch/riscv/arch_timer.c
M src/soc/sifive/fu540/Kconfig
M src/soc/ucb/riscv/Kconfig
5 files changed, 34 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/33797/2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c 
File src/arch/riscv/arch_timer.c:

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@31 
PS1, Line 31: 		die("time not set in HLS");
> Well 'die()' is just another 'printk()' called before 'console_init()' here that would not show up,  […]

It might end in an recursion, but I don't see an easy fix for now.
We could just increment a static software counter each time this function is called.

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@33 
PS1, Line 33: 		die("Unsupported mtime clock frequency");
> Like Philip said. […]

Done.

https://review.coreboot.org/#/c/33797/1/src/arch/riscv/arch_timer.c@36 
PS1, Line 36: 	mono_time_set_usecs(mt, (long)(raw / clocks_per_usec));
> unsigned long ? […]

mono_time_set_usecs takes a long as argument.
That's the way it's implemented all over in coreboot. Updated comments.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: arch/riscv: Fix arch timer timebase

[Xiang Wang (Code Review)]

Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: arch/riscv: Fix arch timer timebase

[Christian Walter (Code Review)]

Christian Walter has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

arch/riscv: Fix arch timer timebase

The arch timer uses a platform specific clock source.
Add a Kconfig that holds this clock frequency and use it to calculate
the time passed by.

Update Kconfig and Documentation with a TODO:
Implement bootblock FDT parser and read 'timebase-frequency' instead
of using a Kconfig.

Tested on qemu-system-riscv:
Fixes delays being ten times shorter as expected.

Change-Id: I2588149e2ee32130a2c41695c4c723b57d4fa827
Signed-off-by: Patrick Rudolph 
---
M Documentation/arch/riscv/index.md
M src/arch/riscv/Kconfig
M src/arch/riscv/arch_timer.c
M src/soc/sifive/fu540/Kconfig
M src/soc/ucb/riscv/Kconfig
5 files changed, 35 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/33797/3

Change in coreboot[master]: arch/riscv: Fix arch timer timebase

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33797 )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Patch Set 3:

Also have a look at: https://review.coreboot.org/c/coreboot/+/37549
I'm slightly in favour of 37549 as it doesn't add another KConfig which we will have to get rid of again.

[S] Change in coreboot[master]: arch/riscv: Fix arch timer timebase

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33797?usp=email )

Change subject: arch/riscv: Fix arch timer timebase
......................................................................

Abandoned