Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35032/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35032/5/src/arch/x86/car.ld@69 PS5, Line 69: _car_ehci_dbg_info_end = .; We're also moving the location of ehci info in this patch to be outside of relocatable window. Do it in another patch?
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@55 PS5, Line 55: static struct timestamp_cache timestamp_cache; Given that TIMESTAMP_CACHE_IN_BSS is 0 why isn't that as well as this object removed?
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@168 PS5, Line 168: timestamp_add_table_entry(timestamp_table_get(), id, ts_time); Below you check the return value of timestamp_table_get() for NULL but you don't here. Why?
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@246 PS5, Line 246: static void timestamp_sync_cache_to_cbmem(int is_recovery) Why did this function move? Can we put it back to its original location for easier reviewing?
https://review.coreboot.org/c/coreboot/+/35032/5/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/soc/intel/fsp_broadwell... PS5, Line 108: timestamp_init(get_initial_timestamp()); Put these changes in another patch? Also add some comments as to why this should be placed here.