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.