Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33766 )
Change subject: soc/amd/picasso: Update monotonic timer ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/33766/4/src/soc/amd/picasso/monoton... File src/soc/amd/picasso/monotonic_timer.c:
https://review.coreboot.org/c/coreboot/+/33766/4/src/soc/amd/picasso/monoton... PS4, Line 25: Did this come from stoneyridge? Implementation of monotonic timer should not depend on timestamp function declarations in any way, so maybe do a bit of renaming there.
https://review.coreboot.org/c/coreboot/+/33766/4/src/soc/amd/picasso/monoton... PS4, Line 18: #include <timestamp.h> Not needed anymore, although I don't see replacement timestamp_get() anywhere yet.
https://review.coreboot.org/c/coreboot/+/33766/4/src/soc/amd/picasso/monoton... PS4, Line 26: } mono_counter; I kind of feel we could have one common definition that is arch-agnostic for this struct, but I did not dig deeper into this.
https://review.coreboot.org/c/coreboot/+/33766/4/src/soc/amd/picasso/monoton... PS4, Line 32: counter_msr = rdmsr(TSC_MSR); Is this different from rdtscll()? From what I recall, on Intel that is a shortcut instruction to access TSC MSR requiring less privileges.
Do you need MFENCE/LFENCE before accessing the counter MSR?
https://review.coreboot.org/c/coreboot/+/33766/4/src/soc/amd/picasso/monoton... PS4, Line 44: void timer_monotonic_get(struct mono_time *mt) How is this different from the generic TSC_MONOTONIC_TIMER ?