Attention is currently required from: Bora Guvendik, Furquan Shaikh, Selma Bensaid, 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/296b3e35_706cd631 PS3, Line 7: Add helper fucntions
Subrata, the problem is that you're flipping the sign on the negative timestamps in https://review.coreboot.org/c/coreboot/+/59555/2/util/cbmem/cbmem.c#618,
I agree with this feedback.
and you shouldn't. If you don't then the sorting will happen exactly as Angel said, 944 first, then in order up to 947, then timestamp 0, then the positive ones. That is what we want.
Ahh, got it. yes, this is the order what we want.
Please see my comment on CB:59555, it should really just take a couple of lines to do this, not a second sort and all the extra stuff you're doing. The final output should be
944:CSE sent 'Boot Stall Done' to PMC 0 945:CSE started to handle ICC configuration 766,000 (766,000) 946:CSE sent 'Host BIOS Prep Done' to PMC 771,000 (5,000) 947:CSE received 'CPU Reset Done Ack sent' from PMC 778,000 (7,000) 0:1st timestamp 1,074,000 (296,000)
(We could also choose not to set the first timestamp to 0 and print the numbers negative as they are if you prefer that... but I think that would be more confusing than helpful.)
true, adding negative would have been nice but might create confusion. Additionally, we want to accommodate the pre-cpu reset time as well in total FW booting time till kernel. For sure we don't want to eliminate those while calculating the final time.
-
Subrata Banik (Code Review)