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 8: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@124 PS7, Line 124: return car_get_ptr(glob_ts_table);
Why would he have cbmem_find() here now, see patchset #3 comment? I thought you analyzed for ramstag […]
Right, I guess we don't need cbmem_find() because CBMEM recovery always sets the pointer already.
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@74 PS8, Line 74: static struct timestamp_cache *timestamp_cache_get(void) nit: Now that there are no other relevant members in struct timestamp_cache, you could change this to return &ts_cache->table...
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@131 PS8, Line 131: ts_cache = timestamp_cache_get(); ...and then simplify this to just
ts_table = timestamp_cache_get();
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@203 PS8, Line 203: if (!ENV_ROMSTAGE_OR_BEFORE) nit: Should this maybe just be an assert()? Because doing this is just incorrect now, right? We can fix the code that does this instead.