Attention is currently required from: Julius Werner, Angel Pons, Subrata Banik, Aaron Durbin. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51445 )
Change subject: timestamp: Add helper fucntions ......................................................................
Patch Set 3:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51445/comment/43695f91_1c1bf362 PS3, Line 7: Add helper fucntions
I think the time_from_base in your naming is confusing, because all timestamp_add()s are relative to the base.
`timestamp_add()` actually takes an absolute time and then converts it into a value relative to the base(https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...): ``` ts_time -= ts_table->base_time; ```
I mentioned it as `time_from_base` because it is the delta from the base rather than an absolute time. See https://imgur.com/7wWcBbv (I have never really shared snapshots here before, so if there is another preferred way, please let me know). Yes, it is possible to do the math at the caller site to perform tX + (b' - t4) and then use timestamp_add to subtract (b' - t4), but I think that is unnecessary. With that we can avoid such calculations and comments to convert entries back and forth between relative and absolute times: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src....
The real difference in the one you propose is just that you're using a different scale (microseconds instead of TSC ticks).
I did sneak in the scale parameter in the same function to allow the caller to provide the conversion factor. In this case, one time is in mseconds and other in TSC. But even if we move everything to microseconds, I think we are still going to require the same functions i.e. rewind_from_base() and timestamp_add_time_from_base() as commented above. So, there is more difference that just a different scale.
I still think this might all be easier if we first change the timestamp library to do everything in microseconds.
I agree that changing to microseconds can make things slightly simpler, but it isn't going to eliminate the functions mentioned above.
File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/51445/comment/17791448_37a4842f PS3, Line 26: * Add a new raw timestamp in microseconds into timestamp_table at index 'n'.
Yeah I know, but that's one division instruction, it shouldn't really be noticeable. […]
Agree that having everything in microseconds can keep things a little simpler. We would still need the new APIs as explained on the other comment. One thing that I am unsure about is some platforms that say "unknown TSC frequency". I will need to dig a little more to understand the history behind it. But, I think we should do that as a separate effort for move to microseconds since these new APIs are anyways required.
-
Furquan Shaikh (Code Review)