Julius Werner 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 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35032/3/src/include/timestamp.h File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/35032/3/src/include/timestamp.h@28 PS3, Line 28: * it's up to the chipset/arch to call timestamp_init() in the former stages. This doesn't really make it clear anymore that it should only be called in *one* of those stages (like the old text).
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@51 PS3, Line 51: #define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR) So... dumb question: does this even still do anything? I think this was only used for LATE_CBMEM_INIT or something? Because the first timestamp I see being added in ramstage is *after* cbmem_initialize(), so it should already go to CBMEM. But even if you did try to log one earlier, timestamp_init() is also called after cbmem_init(), and before that the BSS cache is uninitialized... so there's no window where you could possible log into the cache there either. (For postcar I think the same is true, and I don't see it calling timestamp_init() at all.)
If that's true, we should just get rid of anything implying there was a BSS cache at all (which simplifies a bunch of more stuff here).
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@146 PS3, Line 146: if (ts_table == NULL && HAS_CBMEM) I don't think this would ever be hit?
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@224 PS3, Line 224: ts_cache->cache_state != TIMESTAMP_CACHE_UNINITIALIZED) nit: Somewhat tangential, but I think we could get rid of the whole cache_state thing if you just check the glob_ts_table here (and also initialize it in timestamp_cache_init())? All we need is an indication that any sort of timestamp stuff has been set up already, and checking that for NULL should be good enough.