Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31762 )
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31762/1/src/arch/x86/postcar.c File src/arch/x86/postcar.c:
https://review.coreboot.org/#/c/31762/1/src/arch/x86/postcar.c@44 PS1, Line 44: run_ramstage();
That addition should be conditionalized on ENV_POSTCAR and ENV_ROMSTAGE.
Are you referring this? https://review.coreboot.org/#/c/coreboot/+/31608/3/src/lib/prog_loaders.c if (!ENV_POSTCAR) timestamp_add_now(TS_END_ROMSTAGE);
Yes, but it should really be:
if (ENV_POST_CAR) timestamp_add_now(TS_END_POSTCAR); if (ENV_ROMSTAGE) timetsamp_add_now(TS_END_ROMSTAGE);
https://review.coreboot.org/#/c/31762/1/src/arch/x86/postcar_loader.c File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/#/c/31762/1/src/arch/x86/postcar_loader.c@178 PS1, Line 178: timestamp_add_now(TS_START_POSTCAR);
I was trying to keep timestamp_add_now(TS_START_POSTCAR) inside postcar.c file main() function. […]
But that makes things inconsistent. If you are making assumptions about why you are putting things in various places you should comment/document such notions.
Putting TS_START_POSTCAR in the stage itself makes things consistent. There's inherently missing coverage w/o adding a new timestamp that is essentially "about to start" vs "actually started", but we can read between the lines as to what is happening there.