Change in ...coreboot[master]: arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31202

Change subject: arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE
......................................................................

arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE

timestamp_tick_freq_mhz() is used to populate the coreboot tables
which cbmem can use to determine timestamps without needing userspace
tools to get the TSC frequency rate. CONFIG_TSC_CONSTANT_RATE depends
on CONFIG_UDELAY_TSC which is not the case on all targets that could
still make use of this functionality.

Change-Id: I8c34eb811e27d2a91ccf1ca6f48a638da5f6cfd9
Signed-off-by: Arthur Heymans 
---
M src/arch/x86/timestamp.c
1 file changed, 13 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/31202/1

diff --git a/src/arch/x86/timestamp.c b/src/arch/x86/timestamp.c
index b5257c4..f7b5541 100644
--- a/src/arch/x86/timestamp.c
+++ b/src/arch/x86/timestamp.c
@@ -21,14 +21,20 @@
    return rdtscll();
 }

+/* We want build to fail on targets with CONFIG_TSC_CONSTANT_RATE
+   if an implementation is lacking since it used to compute udelays.
+   FIXME: provide an implementation on every target to get rid of this.*/
+#if !IS_ENABLED(CONFIG_TSC_CONSTANT_RATE)
+unsigned long __weak tsc_freq_mhz(void)
+{
+	/* Default to not knowing TSC frequency. cbmem will have to fallback
+	 * on trying to determine it in userspace. */
+	return 0;
+}
+#endif
+
 int timestamp_tick_freq_mhz(void)
 {
    /* Chipsets that have a constant TSC provide this value correctly. */
-	if (IS_ENABLED(CONFIG_TSC_CONSTANT_RATE))
-		return tsc_freq_mhz();
-
-	/* Filling tick_freq_mhz = 0 in timestamps-table will trigger
-	 * userspace utility to try deduce it from the running system.
-	 */
-	return 0;
+	return tsc_freq_mhz();
 }

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

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

Change subject: arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE
......................................................................

Patch Set 2:

I see you already visited https://review.coreboot.org/c/coreboot/+/30762

I wuold rather fail build-time here. In case TSC_MONOTONIC_TIMER=y you get div-by-zero in delay_tsc.c with the weak implementation, but seems like it is behind three different Kconfig options if that happens.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31202 )

Change subject: arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE
......................................................................

Patch Set 2: Code-Review-1

I don't understand this at all. Can we please discuss if
TSC_CONSTANT_RATE should have a dependency at all?

Change in coreboot[master]: arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31202 )

Change subject: arch/x86/timestamp.c: Don't depend on CONFIG_TSC_CONSTANT_RATE
......................................................................

Abandoned