Attention is currently required from: Bora Guvendik, Furquan Shaikh, Tim Wawrzynczak, Julius Werner, Angel Pons, Aaron Durbin. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51445 )
Change subject: timestamp: Add new helper functions ......................................................................
Patch Set 8:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51445/comment/4b2e9120_197c82cf PS3, Line 7: Add helper fucntions
Hi Bora,
Not sure if you've heard yet, but Furquan isn't working on coreboot anymore. I believe +Tim is taking over some of his former work, although I'm not sure if he has context on this effort. I can also try to help although I'm not familiar with your platform in particular.
You are right, of course, that the formula should be `base_delta * ts->tick_freq_mhz / base_freq_mhz`, not `base_delta * base_freq_mhz / ts->tick_freq_mhz`.
However, looking at this again with a few months distance, I'm actually not so sure anymore why we even need all this rewind() stuff. Fundamentally, your reason for doing that was just that some of the new timestamps you were trying to add are chronologically *before* the base_time, right?
I guess we have other options as well to create 2 cbmem tables 1. with Pre-CPU reset timestamp (that we are implementing newly from ADL onwards) 2. with Post-CPU reset timestamp (regular cbmem -t on IA platform)
Now we would like to create things organically where #2 above can rebase based on #1. Additionally, unlike some other SoC platform, where #1 can be collected as early after CPU reset, is not likely the case with IA platform. Rather when we are able to collect #1 (typically, at early romstage state or late might be), we already have #2 available.
It makes things more complicated as we would like to make the timestamp appear as if we have collected #1 prior to #2, which is not the case unfortunately. It poses the requirement of rewinding the #2 table based on #1 timestamp.
I had thought of simple merging #1 and #2 together without rebasing but Furquan has suggested it might not portrait the exact situation nicely.
But why is that such a problem? Alternatively, we could just redefine (struct timestamp_entry).entry_stamp from uint64_t to int64_t, and declare that it should be allowed to be negative. Then I don't think you need any API changes here and just need to modify utils/cbmem to be aware of that case, and "rebase" all of those timestamps to the lowest one in the list when displaying them.
I kind of get what your are suggesting but I would let Bora to share a sample timestamp for #1 and #2 above to illustrate the scenario. Want to make sure we have exact timestamp value appearing at base rather delta which may not be the exact while doing (Tn - T(n-1)) = negative number.
I think that would require the minimum amount of extra logic in firmware code because you can basically just use timestamp_add() and pass a (potentially) negative number. (You may still need to do frequency conversions, that's where my other suggestion would come in of just rewrite x86's timestamp_get() to immediately convert to microseconds -- all other architecture ports already do it that way anyway, and then we'll never have any of this frequency confusion anymore.)
This is good suggestion, if all other arch does the same, its easy to implement and land the CL in that way. Thanks for suggestion.
What do you think?
-
Subrata Banik (Code Review)