Change in ...coreboot[master]: arch/x86/postcar: Add separate timestamp for postcar stage
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31762
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
arch/x86/postcar: Add separate timestamp for postcar stage
This patch adds dedicated timestamp value for postcar stage.
TEST=Able to see "start of postcar" and "end of postcar" timestamp while executing cbmem -t after booting to chrome console.
cbmem -t
951:returning from FspMemoryInit 20,404,341 (20,022,190) 4:end of romstage 20,445,466 (41,124) 1103:start of postcar 20,478,259 (32,793) 1104:end of postcar 20,489,064 (10,805)
Change-Id: I084f66949667ad598f811d4233b4e639bc4c113e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar.c M src/arch/x86/postcar_loader.c M src/commonlib/include/commonlib/timestamp_serialized.h 3 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31762/1
diff --git a/src/arch/x86/postcar.c b/src/arch/x86/postcar.c index ea05824..c2d91be 100644 --- a/src/arch/x86/postcar.c +++ b/src/arch/x86/postcar.c @@ -19,6 +19,7 @@ #include <cpu/x86/mtrr.h> #include <main_decl.h> #include <program_loading.h> +#include <timestamp.h>
/* * Systems without a native coreboot cache-as-ram teardown may implement @@ -37,6 +38,8 @@
display_mtrrs();
+ timestamp_add_now(TS_END_POSTCAR); + /* Load and run ramstage. */ run_ramstage(); } diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index b08b5ea..2d99eef 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -175,5 +175,7 @@ } else load_postcar_cbfs(&prog, pcf);
+ timestamp_add_now(TS_START_POSTCAR); + prog_run(&prog); } diff --git a/src/commonlib/include/commonlib/timestamp_serialized.h b/src/commonlib/include/commonlib/timestamp_serialized.h index cbf07b5..cc088f5 100644 --- a/src/commonlib/include/commonlib/timestamp_serialized.h +++ b/src/commonlib/include/commonlib/timestamp_serialized.h @@ -143,6 +143,8 @@
TS_START_KERNEL = 1101, TS_KERNEL_DECOMPRESSION = 1102, + TS_START_POSTCAR = 1103, + TS_END_POSTCAR = 1104, };
static const struct timestamp_id_to_name { @@ -257,6 +259,8 @@ { TS_FSP_BEFORE_END_OF_FIRMWARE, "calling FspNotify(EndOfFirmware)" }, { TS_FSP_AFTER_END_OF_FIRMWARE, "returning from FspNotify(EndOfFirmware)" }, + { TS_START_POSTCAR, "start of postcar" }, + { TS_END_POSTCAR, "end of postcar" }, };
#endif
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)
I noticed the previous CL addresses some of my comments but not all.
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(); There is the following in run_ramstage():
timestamp_add_now(TS_END_ROMSTAGE);
That addition should be conditionalized on ENV_POSTCAR and ENV_ROMSTAGE.
Similarly, TS_END_ROMSTAGE should reside in run_postcar_phase() where you put TS_START_POSTCAR.
That would be more consistent and not get a weirder sequence of timestamps.
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); It seems to me for romstage and ramstage the start timestamp is in the stage itself. Why is postcar's start timestamp not in the beginning of the stage?
Subrata Banik 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();
There is the following in 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);
Similarly, TS_END_ROMSTAGE should reside in run_postcar_phase() where you put TS_START_POSTCAR.
yes, i will take care of that. Saw your review comment
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);
It seems to me for romstage and ramstage the start timestamp is in the stage itself. […]
I was trying to keep timestamp_add_now(TS_START_POSTCAR) inside postcar.c file main() function. But looks like timestamp has some prerequisites which is getting satisfied after cbmem_init() and by that time majority of postcar stage is over. Just to catch entire postcar stage activity, i just thought to keep this timestamp before start executing postcar stage.
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.
Subrata Banik 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:
(1 comment)
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);
But that makes things inconsistent. […]
you should comment/document such notions.
I will add some comment to note that point.
new timestamp that is essentially "about to start" vs "actually started", but we can read between the lines as to what is happening there.
I will name may be "Starting of postcar" and "end of postcar" to make thinks consistent across ?
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:
(1 comment)
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);
you should comment/document such notions.
I will add some comment to note that point.
new timestamp that is essentially "about to start" vs "actually started", but we can read between the lines as to what is happening there.
I will name may be "Starting of postcar" and "end of postcar" to make thinks consistent across ?
I was commenting about how the current usage is actually done for romstage and ramstage. I would expect to see a similar pattern for postcar, but this CL doesn't do that.
Hello Aaron Durbin, ron minnich, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31762
to look at the new patch set (#2).
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
arch/x86/postcar: Add separate timestamp for postcar stage
This patch adds dedicated timestamp value for postcar stage.
TEST=Able to see "start of postcar" and "end of postcar" timestamp while executing cbmem -t after booting to chrome console.
cbmem -t
951:returning from FspMemoryInit 20,485,324 (20,103,067) 4:end of romstage 20,559,235 (73,910) 1103:start of postcar 20,560,266 (1,031) 1104:end of postcar 20,570,038 (9,772)
Change-Id: I084f66949667ad598f811d4233b4e639bc4c113e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar.c M src/commonlib/include/commonlib/timestamp_serialized.h 2 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31762/2
Subrata Banik 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:
(1 comment)
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. […]
Done
Patrick Rudolph 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 2:
(1 comment)
https://review.coreboot.org/#/c/31762/2/src/commonlib/include/commonlib/time... File src/commonlib/include/commonlib/timestamp_serialized.h:
https://review.coreboot.org/#/c/31762/2/src/commonlib/include/commonlib/time... PS2, Line 146: TS_START_POSTCAR = 1103, use 100+ as those aren't related to depthcharge
Hello Aaron Durbin, ron minnich, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31762
to look at the new patch set (#3).
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
arch/x86/postcar: Add separate timestamp for postcar stage
This patch adds dedicated timestamp value for postcar stage.
TEST=Able to see "start of postcar" and "end of postcar" timestamp while executing cbmem -t after booting to chrome console.
cbmem -t
951:returning from FspMemoryInit 20,485,324 (20,103,067) 4:end of romstage 20,559,235 (73,910) 100:start of postcar 20,560,266 (1,031) 101:end of postcar 20,570,038 (9,772)
Change-Id: I084f66949667ad598f811d4233b4e639bc4c113e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar.c M src/commonlib/include/commonlib/timestamp_serialized.h 2 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31762/3
Subrata Banik 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 2:
(1 comment)
https://review.coreboot.org/#/c/31762/2/src/commonlib/include/commonlib/time... File src/commonlib/include/commonlib/timestamp_serialized.h:
https://review.coreboot.org/#/c/31762/2/src/commonlib/include/commonlib/time... PS2, Line 146: TS_START_POSTCAR = 1103,
use 100+ as those aren't related to depthcharge
Done
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 3:
(1 comment)
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c File src/arch/x86/postcar.c:
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c@43 PS3, Line 43: timestamp_add_now(TS_END_POSTCAR); I think you should put this in in run_ramstage() were the END_ROMSTAGE bit is:
if (ENV_POSTCAR) timestamp_add_now(TS_END_POSTCAR); if (ENV_ROMSTAGE) timestamp_add_now(TS_END_ROMSTAGE);
Kyösti Mälkki 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 3:
(1 comment)
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c File src/arch/x86/postcar.c:
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c@39 PS3, Line 39: console_init(); Not sure why these are after cbmem_initialize(), specially moving console_init().
Subrata Banik 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 3:
(1 comment)
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c File src/arch/x86/postcar.c:
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c@39 PS3, Line 39: console_init();
Not sure why these are after cbmem_initialize(), specially moving console_init().
cbmem init is must before timestamp_add_now() hence cbmem_init call is before timestamp. we can move console_init() at its original position.
Subrata Banik 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 3:
(1 comment)
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c File src/arch/x86/postcar.c:
https://review.coreboot.org/#/c/31762/3/src/arch/x86/postcar.c@43 PS3, Line 43: timestamp_add_now(TS_END_POSTCAR);
I think you should put this in in run_ramstage() were the END_ROMSTAGE bit is: […]
Done
Hello Aaron Durbin, ron minnich, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31762
to look at the new patch set (#4).
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
arch/x86/postcar: Add separate timestamp for postcar stage
This patch adds dedicated timestamp value for postcar stage.
TEST=Able to see "start of postcar" and "end of postcar" timestamp while executing cbmem -t after booting to chrome console.
cbmem -t
951:returning from FspMemoryInit 20,485,324 (20,103,067) 4:end of romstage 20,559,235 (73,910) 100:start of postcar 20,560,266 (1,031) 101:end of postcar 20,570,038 (9,772)
Change-Id: I084f66949667ad598f811d4233b4e639bc4c113e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar.c M src/commonlib/include/commonlib/timestamp_serialized.h M src/lib/prog_loaders.c 3 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31762/4
build bot (Jenkins) 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 4:
(1 comment)
https://review.coreboot.org/#/c/31762/4/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/31762/4/src/lib/prog_loaders.c@131 PS4, Line 131: if(ENV_POSTCAR) space required before the open parenthesis '('
Hello Aaron Durbin, ron minnich, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31762
to look at the new patch set (#5).
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
arch/x86/postcar: Add separate timestamp for postcar stage
This patch adds dedicated timestamp value for postcar stage.
TEST=Able to see "start of postcar" and "end of postcar" timestamp while executing cbmem -t after booting to chrome console.
cbmem -t
951:returning from FspMemoryInit 20,485,324 (20,103,067) 4:end of romstage 20,559,235 (73,910) 100:start of postcar 20,560,266 (1,031) 101:end of postcar 20,570,038 (9,772)
Change-Id: I084f66949667ad598f811d4233b4e639bc4c113e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar.c M src/commonlib/include/commonlib/timestamp_serialized.h M src/lib/prog_loaders.c 3 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31762/5
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 5: Code-Review+2
(1 comment)
this and the following patch could really be collapsed into one.
https://review.coreboot.org/#/c/31762/5/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/31762/5/src/lib/prog_loaders.c@134 PS5, Line 134: timestamp_add_now(TS_END_ROMSTAGE); Why not just fix this up in this patch instead of knowingly having bad timestamps here for romstage?
Subrata Banik 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 5:
(1 comment)
this and the following patch could really be collapsed into one.
I thought it will be easy for review and more purposeful as in first patch we want to add new timestamp for postcar and in next we want to fix the brokenness of romstage timestamp.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31762 )
Change subject: arch/x86/postcar: Add separate timestamp for postcar stage ......................................................................
arch/x86/postcar: Add separate timestamp for postcar stage
This patch adds dedicated timestamp value for postcar stage.
TEST=Able to see "start of postcar" and "end of postcar" timestamp while executing cbmem -t after booting to chrome console.
cbmem -t
951:returning from FspMemoryInit 20,485,324 (20,103,067) 4:end of romstage 20,559,235 (73,910) 100:start of postcar 20,560,266 (1,031) 101:end of postcar 20,570,038 (9,772)
Change-Id: I084f66949667ad598f811d4233b4e639bc4c113e Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31762 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/arch/x86/postcar.c M src/commonlib/include/commonlib/timestamp_serialized.h M src/lib/prog_loaders.c 3 files changed, 10 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/x86/postcar.c b/src/arch/x86/postcar.c index ea05824..b4efc94 100644 --- a/src/arch/x86/postcar.c +++ b/src/arch/x86/postcar.c @@ -19,6 +19,7 @@ #include <cpu/x86/mtrr.h> #include <main_decl.h> #include <program_loading.h> +#include <timestamp.h>
/* * Systems without a native coreboot cache-as-ram teardown may implement @@ -35,6 +36,8 @@ /* Recover cbmem so infrastruture using it is functional. */ cbmem_initialize();
+ timestamp_add_now(TS_START_POSTCAR); + display_mtrrs();
/* Load and run ramstage. */ diff --git a/src/commonlib/include/commonlib/timestamp_serialized.h b/src/commonlib/include/commonlib/timestamp_serialized.h index cbf07b5..bb0dcfc 100644 --- a/src/commonlib/include/commonlib/timestamp_serialized.h +++ b/src/commonlib/include/commonlib/timestamp_serialized.h @@ -63,6 +63,8 @@ TS_LOAD_PAYLOAD = 90, TS_ACPI_WAKE_JUMP = 98, TS_SELFBOOT_JUMP = 99, + TS_START_POSTCAR = 100, + TS_END_POSTCAR = 101,
/* 500+ reserved for vendorcode extensions (500-600: google/chromeos) */ TS_START_COPYVER = 501, @@ -257,6 +259,8 @@ { TS_FSP_BEFORE_END_OF_FIRMWARE, "calling FspNotify(EndOfFirmware)" }, { TS_FSP_AFTER_END_OF_FIRMWARE, "returning from FspNotify(EndOfFirmware)" }, + { TS_START_POSTCAR, "start of postcar" }, + { TS_END_POSTCAR, "end of postcar" }, };
#endif diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 70ea7ef..da021f1 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -128,6 +128,9 @@ struct prog ramstage = PROG_INIT(PROG_RAMSTAGE, CONFIG_CBFS_PREFIX "/ramstage");
+ if (ENV_POSTCAR) + timestamp_add_now(TS_END_POSTCAR); + timestamp_add_now(TS_END_ROMSTAGE);
/*
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Subrata Banik (Code Review)