Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 149: if (info->timestamp_offset != 0) { If this condition fails bootblock_main_* will never be called and we'll return to assembly.
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 174: base_timestamp += psp_ts_table->base_time - psp_last_ts; I'm having a hard time following the logic.
A: time when psp timer started ticking (unknown in absolute terms) B: psp_ts_table->base_time is the first timestamp taken on psp. C: psp_last_ts is when x86 was released
A through C are in psp time domain of 1us tick.
D: first timestamp x86 reads -> base_timestamp in this code and relative to C
So the base_time in the new timestamp tables needs to have a negative offset of psp_last_time (C) for the x86 derived timestamps for the math to work out.
Why wouldn't we just subtract psp_last_ts from base_timestamp such that it gets added in timestamp_add() for every subsequent entry? Is psp_last_ts absolute or relative to psp_ts_table->base_time?
The math may work out, but the current code doesn't read as straight forward. Regardless of relative vs absolute things would read clearer by subtracting psp_last_ts which is calculating C - A as an offset. i.e. either:
relative: base_timestamp -= psp_last_ts + psp_ts_table->base_time; absolute: base_timestamp -= psp_last_ts;
I think the same is true for the psp entries. They should be adjusted according to the calculated offset once we know the new base_time being passed into bootblock_main_with_timestamp(). i.e. Those should be adjusted by the same magnitude but in the opposite direction.
Once we calculated new base_timestamp that same value should be used to manipulate the psp entries:
tse->entry_stamp -= base_timestamp;
Again, I think the math may work out, but it's not super clear. And we're duplicating the math when it should be calculated once and utilized.
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 175: bootblock_main_with_timestamp(base_timestamp, FWIW, this manipulation of timestamp table base time moves the basis around that's impossible to maintain/retrieve time to first timestamp in psp. We lose all that visibility.