Change in ...coreboot[master]: timestamp: Move timestamp_should_run() call

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

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31524

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

timestamp: Move timestamp_should_run() call

Old location caused spurious error messages when
called from APs, where timestamp_add_now() should
do nothing.

Moving the test also makes usecs_from_boot() usable
from APs (assuming cache coherency).

Change-Id: Ice9ece11b15bbe1a58a038cda3d299862e6f822b
Signed-off-by: Kyösti Mälkki 
---
M src/lib/timestamp.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31524/1

diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c
index 6885e7b..f84b9d5 100644
--- a/src/lib/timestamp.c
+++ b/src/lib/timestamp.c
@@ -125,9 +125,6 @@
    MAYBE_STATIC struct timestamp_table *ts_table = NULL;
    struct timestamp_cache *ts_cache;

-	if (!timestamp_should_run())
-		return NULL;
-
    if (ts_table != NULL)
    	return ts_table;

@@ -188,6 +185,9 @@
 {
    struct timestamp_table *ts_table;

+	if (!timestamp_should_run())
+		return;
+
    ts_table = timestamp_table_get();

if (!ts_table) {

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

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

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 1:

That implementation of get_us_since_boot() should not depend on COLLECT_TIMESTAMPS. Maybe it could be somehere in TSC/APIC/MONOTONIC_TIMER instead?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31524 )

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 1: Code-Review+1

LGTM but I'm not really familiar with the need for timestamp_should_run().
> That implementation of get_us_since_boot() should not depend on COLLECT_TIMESTAMPS. Maybe it could be somehere in TSC/APIC/MONOTONIC_TIMER instead?

That seems somewhat difficult... I don't think you can provide that (at least implemented the way it is now) without pulling in most of the timestamp code anyway.

[Julius Werner (Code Review)]

Julius Werner has removed Julius Werner from this change.  ( https://review.coreboot.org/c/coreboot/+/31524 )

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Removed reviewer Julius Werner.

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

Hello Werner Zeh, Julius Werner, build bot (Jenkins),

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

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

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

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

timestamp: Move timestamp_should_run() call

Old location caused spurious error messages when
called from APs, where timestamp_add_now() should
do nothing.

Moving the test also makes get_us_from_boot() usable
from APs (assuming cache coherency).

Change-Id: Ice9ece11b15bbe1a58a038cda3d299862e6f822b
Signed-off-by: Kyösti Mälkki 
---
M src/lib/timestamp.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31524/2

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

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

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 2:

(1 comment)
> Patch Set 1: Code-Review+1
>
> LGTM but I'm not really familiar with the need for timestamp_should_run().
> > That implementation of get_us_since_boot() should not depend on COLLECT_TIMESTAMPS. Maybe it could be somehere in TSC/APIC/MONOTONIC_TIMER instead?
>
> That seems somewhat difficult... I don't think you can provide that (at least implemented the way it is now) without pulling in most of the timestamp code anyway.

I just feel it's mixing "an optional debug feature" with the need for actual time reference monotonic timers can provide without the collection. But it's not the scope here anyways.

https://review.coreboot.org/#/c/31524/2/src/lib/timestamp.c 
File src/lib/timestamp.c:

https://review.coreboot.org/#/c/31524/2/src/lib/timestamp.c@113 
PS2, Line 113:  * based x86 platforms. */
Well the usage is commented here, but the reference to AMD (and ARCH_X86) is somewhat vague. IMHO the entire collection becomes unusable or hard to parse if we let AP CPUs add entries.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31524 )

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31524 )

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 2: Code-Review+2

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31524 )

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 2: Code-Review+1
> Patch Set 2:
>
> (1 comment)
> > Patch Set 1: Code-Review+1
> >
> > LGTM but I'm not really familiar with the need for timestamp_should_run().
> > > That implementation of get_us_since_boot() should not depend on COLLECT_TIMESTAMPS. Maybe it could be somehere in TSC/APIC/MONOTONIC_TIMER instead?
> >
> > That seems somewhat difficult... I don't think you can provide that (at least implemented the way it is now) without pulling in most of the timestamp code anyway.
>
> I just feel it's mixing "an optional debug feature" with the need for actual time reference monotonic timers can provide without the collection. But it's not the scope here anyways.

This function was implemented here because all the information needed are available here.
In general it is not important to me haveing it here, I just need the functionality it provides. Adding it here was the proper way for me.

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

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

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
> > Patch Set 2:
> >
> > (1 comment)
> > > Patch Set 1: Code-Review+1
> > >
> > > LGTM but I'm not really familiar with the need for timestamp_should_run().
> > > > That implementation of get_us_since_boot() should not depend on COLLECT_TIMESTAMPS. Maybe it could be somehere in TSC/APIC/MONOTONIC_TIMER instead?
> > >
> > > That seems somewhat difficult... I don't think you can provide that (at least implemented the way it is now) without pulling in most of the timestamp code anyway.
> >
> > I just feel it's mixing "an optional debug feature" with the need for actual time reference monotonic timers can provide without the collection. But it's not the scope here anyways.
>
> This function was implemented here because all the information needed are available here.
> In general it is not important to me haveing it here, I just need the functionality it provides. Adding it here was the proper way for me.

Yes. It's just that it was never well defined when basetime is stamped in relation to power-on and what happens with early soft resets. I think AMD's mostly make the first stamp in romstage. The boards you use may vary between bootblock and romstage, and if there are ME / FSP resets the relation to realtime is vague.

Timer code could use some TLC anyways, there is a bit of mess on udelay() and TSC at least.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31524 )

Change subject: timestamp: Move timestamp_should_run() call
......................................................................

timestamp: Move timestamp_should_run() call

Old location caused spurious error messages when
called from APs, where timestamp_add_now() should
do nothing.

Moving the test also makes get_us_from_boot() usable
from APs (assuming cache coherency).

Change-Id: Ice9ece11b15bbe1a58a038cda3d299862e6f822b
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/31524
Reviewed-by: Aaron Durbin 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Werner Zeh 
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/lib/timestamp.c
1 file changed, 3 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Werner Zeh: Looks good to me, but someone else must approve
  Furquan Shaikh: Looks good to me, approved
  Julius Werner: Looks good to me, but someone else must approve

diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c
index 6885e7b..f84b9d5 100644
--- a/src/lib/timestamp.c
+++ b/src/lib/timestamp.c
@@ -125,9 +125,6 @@
    MAYBE_STATIC struct timestamp_table *ts_table = NULL;
    struct timestamp_cache *ts_cache;

-	if (!timestamp_should_run())
-		return NULL;
-
    if (ts_table != NULL)
    	return ts_table;

@@ -188,6 +185,9 @@
 {
    struct timestamp_table *ts_table;

+	if (!timestamp_should_run())
+		return;
+
    ts_table = timestamp_table_get();

if (!ts_table) {