Kyösti Mälkki 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:
(7 comments)
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)
It's not an insurmountable problem, but I would prefer initializing timestamps in picasso before cbm […]
Ack
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@94 PS3, Line 94: ts_cache = (void *)_timestamp;
Can't use car_get_var_ptr() after _timestamp is moved outside _car_relocatable. Currently FSP1. […]
Done
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?
Done
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 ch […]
Done
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?
Patchset #3 comments. I don't know whether amd/picasso will use .bss or REGION(timestamp) for romstage.
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. […]
Done
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?
Done