coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: util/spd_tools: Add LP5 support for ADL
by Reka Norman (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Tim Wawrzynczak, Reka Norman, Sridhar Siricilla, Balaji Manigandan, Krishna P Bhat D. Hello build bot (Jenkins), Tim Wawrzynczak, Sridhar Siricilla, Nick Vaccaro, Balaji Manigandan, Krishna P Bhat D, Karthik Ramasubramanian, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/58679 to look at the new patch set (#9). Change subject: util/spd_tools: Add LP5 support for ADL ...................................................................... util/spd_tools: Add LP5 support for ADL Add LP5 support to spd_tools. Currently, only Intel Alder Lake (ADL) is supported. The SPDs are generated based on a combination of: - The LPDDR5 spec JESD209-5B. - The SPD spec SPD4.1.2.M-2 (the LPDDR3/4 spec is used since JEDEC has not released an SPD spec for LPDDR5). - Intel recommendations in advisory #616599. BUG=b:201234943, b:198704251 TEST=Generate the SPD and manifests for a test part, and check that the SPD matches Intel's expectation. More details in CB:58680. Change-Id: Ic1e68d44f7c0ad64aa9904b7e1297d24bd5db56e Signed-off-by: Reka Norman <rekanorman(a)google.com> --- M util/spd_tools/README.md M util/spd_tools/src/part_id_gen/part_id_gen.go A util/spd_tools/src/spd_gen/lp5.go M util/spd_tools/src/spd_gen/spd_gen.go 4 files changed, 726 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/58679/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/58679 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic1e68d44f7c0ad64aa9904b7e1297d24bd5db56e Gerrit-Change-Number: 58679 Gerrit-PatchSet: 9 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: commonlib/mem_pool: Allow configuring the alignment
by Karthik Ramasubramanian (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Raul Rangel, Furquan Shaikh, Julius Werner, Angel Pons. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56580 ) Change subject: commonlib/mem_pool: Allow configuring the alignment ...................................................................... Patch Set 11: (2 comments) File src/commonlib/include/commonlib/mem_pool.h: https://review.coreboot.org/c/coreboot/+/56580/comment/346ccaf2_aa3de021 PS11, Line 58: alignment > This will raise a divide by 0 error. […] Works for me. File src/commonlib/mem_pool.c: https://review.coreboot.org/c/coreboot/+/56580/comment/493ddbe7_59cc1e17 PS11, Line 11: mp->alignment > ``` […] Returning NULL should be fine. -- To view, visit https://review.coreboot.org/c/coreboot/+/56580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8d77ffe4411f86c54450305320c9f52ab41a3075 Gerrit-Change-Number: 56580 Gerrit-PatchSet: 11 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 21:26:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/lib: Support fallback option for fw_config
by Wonkyu Kim (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Furquan Shaikh, Jamie Ryu, Ravishankar Sarawadi, Tim Wawrzynczak, Subrata Banik, Julius Werner, Nick Vaccaro, Raj Astekar, Srinidhi N Kaushik. Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58833 ) Change subject: src/lib: Support fallback option for fw_config ...................................................................... Patch Set 5: (2 comments) File src/lib/fw_config.c: https://review.coreboot.org/c/coreboot/+/58833/comment/ee9fc4b6_c4b62377 PS5, Line 31: fw_config_value = UNDEFINED_FW_CONFIG; > this line is now redundant Can be it safer put the value other than dependent on api implmenetation(google_chromeec_cbi_get_fw_config)? https://review.coreboot.org/c/coreboot/+/58833/comment/7fb4e41f_afb11870 PS5, Line 43: fw_config_value = UNDEFINED_FW_CONFIG; > also redundant Can be it safer put the value other than dependent on api implmenetation(cbfs_load)? -- To view, visit https://review.coreboot.org/c/coreboot/+/58833 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I215c13a4fcb9dc3b94f73c770e704d4e353e9cff Gerrit-Change-Number: 58833 Gerrit-PatchSet: 5 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 21:18:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/spd_tools: Add LP5 support for ADL
by Karthik Ramasubramanian (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Tim Wawrzynczak, Reka Norman, Sridhar Siricilla, Balaji Manigandan, Krishna P Bhat D. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58679 ) Change subject: util/spd_tools: Add LP5 support for ADL ...................................................................... Patch Set 8: Code-Review+2 (2 comments) Patchset: PS8: > The lp4x SPD tools support Cezanne (AMD) as well, and the ddr4 tools support Picasso (AMD) too, so t […] Reka, +1 to your assessment. Until we know the MRC requirements for other SoC vendors, this is as general as it can get. I will work with you when LP5 is adopted by AMD mainboards. Nick, As Tim mentioned, both Zork and Guybrush has been using the SPD tools. File util/spd_tools/src/spd_gen/lp5.go: https://review.coreboot.org/c/coreboot/+/58679/comment/b907fb44_dbd860e1 PS5, Line 18: DensityPerDieGb int > Yes, LP5 only supports one channel per die. […] Not for this CL: Does that mean we can remove the RAM_CHANNEL_SELECT GPIO strap? Something to check out in the schematics. -- To view, visit https://review.coreboot.org/c/coreboot/+/58679 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic1e68d44f7c0ad64aa9904b7e1297d24bd5db56e Gerrit-Change-Number: 58679 Gerrit-PatchSet: 8 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 20:24:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Reka Norman <rekanorman(a)chromium.org> Comment-In-Reply-To: Nick Vaccaro <nvaccaro(a)google.com> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: commonlib/mem_pool: Allow configuring the alignment
by Raul Rangel (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Furquan Shaikh, Julius Werner, Angel Pons, Karthik Ramasubramanian. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56580 ) Change subject: commonlib/mem_pool: Allow configuring the alignment ...................................................................... Patch Set 11: (2 comments) File src/commonlib/include/commonlib/mem_pool.h: https://review.coreboot.org/c/coreboot/+/56580/comment/b71decfb_646353ec PS11, Line 58: alignment > What is buf % 0? This will raise a divide by 0 error. Should I add an assert? File src/commonlib/mem_pool.c: https://review.coreboot.org/c/coreboot/+/56580/comment/fb531628_6c6af05a PS11, Line 11: mp->alignment > What is the behavior of ALIGN_UP(sz, 0)? Is it defined or undefined? ``` ALIGN_UP(64, 0) => 0x0 ``` Should I add an assert, or just return NULL if alignment is 0? -- To view, visit https://review.coreboot.org/c/coreboot/+/56580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8d77ffe4411f86c54450305320c9f52ab41a3075 Gerrit-Change-Number: 56580 Gerrit-PatchSet: 11 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 20:21:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: commonlib/mem_pool: Allow configuring the alignment
by Karthik Ramasubramanian (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Raul Rangel, Furquan Shaikh, Julius Werner, Angel Pons. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56580 ) Change subject: commonlib/mem_pool: Allow configuring the alignment ...................................................................... Patch Set 11: (3 comments) File src/commonlib/include/commonlib/mem_pool.h: https://review.coreboot.org/c/coreboot/+/56580/comment/cf8e647f_f8ed7280 PS11, Line 20: default 8 byte aligned Nit: Now there is no default alignment. Alignment has to be explicitly specified. https://review.coreboot.org/c/coreboot/+/56580/comment/c9c3f034_edee9121 PS11, Line 58: alignment What is buf % 0? File src/commonlib/mem_pool.c: https://review.coreboot.org/c/coreboot/+/56580/comment/0b7e8f2a_10bf9ea7 PS11, Line 11: mp->alignment What is the behavior of ALIGN_UP(sz, 0)? Is it defined or undefined? -- To view, visit https://review.coreboot.org/c/coreboot/+/56580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8d77ffe4411f86c54450305320c9f52ab41a3075 Gerrit-Change-Number: 56580 Gerrit-PatchSet: 11 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 20:14:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: add generic gpio lock mechanism
by Nick Vaccaro (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Patrick Rudolph, Karthik Ramasubramanian. Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58351 ) Change subject: soc/intel/common: add generic gpio lock mechanism ...................................................................... Patch Set 4: (1 comment) File src/soc/intel/common/block/smm/smihandler.c: https://review.coreboot.org/c/coreboot/+/58351/comment/2b2bba3e_e9f4b90f PS1, Line 348: gpio_lock_pad > I think you may also need `HAVE_DEBUG_GPIO` ? I implemented some timestamping to measure the lock timing. It shows that patchset 2's approach of calling gpio_lock_pad() for each pad is faster than combining those into a single call and saving the multiple p2sb hides. Looking at the code, I'm not sure how to explain that. I ran the tests multiple ways. Hisenberg is definitely at play here as I had to enable a few things and include timestamp code in places it wasn't before to get timestamps to work from within SMM. Both versions implement the timestamp the same way (by calling timestamp_get() at the start and at the finish and subtracting start time from finish time), so theoretically the heisenberg factor should affect them equally / have same affect on both. That being said, I'm not seeing something that would explain why multi is taking longer than single. If folks have any thoughts on that, please let me know. I may need to add further timestamping inside the routine to see what's really going on. Here's a snapshot of the timings for the two patsets (single is patchset 2, multi is patchset 4) : From single locking code (patchset 2 plus logging code) : soc_lock_gpios: ENTER at 19696381159 gpio_lock_pad: Locking pad 58 configuration gpio_lock_pad: Locking pad 3 configuration gpio_lock_pad: Locking pad 4 configuration gpio_lock_pad: Locking pad 5 configuration gpio_lock_pad: Locking pad 6 configuration gpio_lock_pad: Locking pad 7 configuration gpio_lock_pad: Locking pad 8 configuration gpio_lock_pad: Locking pad 14 configuration gpio_lock_pad: Locking pad 15 configuration gpio_lock_pad: Locking pad 16 configuration gpio_lock_pad: Locking pad 17 configuration gpio_lock_pad: Locking pad 99 configuration gpio_lock_pad: Locking pad 100 configuration gpio_lock_pad: Locking pad 101 configuration gpio_lock_pad: Locking pad 102 configuration gpio_lock_pad: Locking pad 108 configuration gpio_lock_pad: Locking pad 109 configuration gpio_lock_pad: Locking pad 110 configuration gpio_lock_pad: Locking pad 111 configuration gpio_lock_pad: Locking pad 112 configuration gpio_lock_pad: Locking pad 113 configuration gpio_lock_pad: Locking pad 114 configuration gpio_lock_pad: Locking pad 115 configuration gpio_lock_pad: Locking pad 116 configuration gpio_lock_pad: Locking pad 117 configuration gpio_lock_pad: Locking pad 359 configuration gpio_lock_pad: Locking pad 87 configuration gpio_lock_pad: Locking pad 88 configuration gpio_lock_pad: Locking pad 351 configuration gpio_lock_pad: Locking pad 352 configuration gpio_lock_pad: Locking pad 356 configuration gpio_lock_pad: Locking pad 360 configuration gpio_lock_pad: Locking pad 361 configuration gpio_lock_pad: Locking pad 362 configuration gpio_lock_pad: Locking pad 363 configuration gpio_lock_pad: Locking pad 367 configuration gpio_lock_pad: Locking pad 330 configuration gpio_lock_pad: Locking pad 331 configuration gpio_lock_pad: Locking pad 332 configuration gpio_lock_pad: Locking pad 333 configuration gpio_lock_pad: Locking pad 334 configuration gpio_lock_pad: Locking pad 335 configuration gpio_lock_pad: Locking pad 336 configuration gpio_lock_pad: Locking pad 337 configuration soc_lock_gpios: EXIT at 19897787169 soc_lock_gpios: Locking gpios via gpio_lock_pad() took 201406010 ======================================================================== From multi locking code (patchset 4 + logging code) soc_lock_gpios: ENTER at 19727883141 gpio_lock_multiple_pads: Locking pad 58 configuration gpio_lock_multiple_pads: Locking pad 3 configuration gpio_lock_multiple_pads: Locking pad 4 configuration gpio_lock_multiple_pads: Locking pad 5 configuration gpio_lock_multiple_pads: Locking pad 6 configuration gpio_lock_multiple_pads: Locking pad 7 configuration gpio_lock_multiple_pads: Locking pad 8 configuration gpio_lock_multiple_pads: Locking pad 14 configuration gpio_lock_multiple_pads: Locking pad 15 configuration gpio_lock_multiple_pads: Locking pad 16 configuration gpio_lock_multiple_pads: Locking pad 17 configuration gpio_lock_multiple_pads: Locking pad 99 configuration gpio_lock_multiple_pads: Locking pad 100 configuration gpio_lock_multiple_pads: Locking pad 101 configuration gpio_lock_multiple_pads: Locking pad 102 configuration gpio_lock_multiple_pads: Locking pad 108 configuration gpio_lock_multiple_pads: Locking pad 109 configuration gpio_lock_multiple_pads: Locking pad 110 configuration gpio_lock_multiple_pads: Locking pad 111 configuration gpio_lock_multiple_pads: Locking pad 112 configuration gpio_lock_multiple_pads: Locking pad 113 configuration gpio_lock_multiple_pads: Locking pad 114 configuration gpio_lock_multiple_pads: Locking pad 115 configuration gpio_lock_multiple_pads: Locking pad 116 configuration gpio_lock_multiple_pads: Locking pad 117 configuration gpio_lock_multiple_pads: Locking pad 359 configuration gpio_lock_multiple_pads: Locking pad 87 configuration gpio_lock_multiple_pads: Locking pad 88 configuration gpio_lock_multiple_pads: Locking pad 351 configuration gpio_lock_multiple_pads: Locking pad 352 configuration gpio_lock_multiple_pads: Locking pad 356 configuration gpio_lock_multiple_pads: Locking pad 360 configuration gpio_lock_multiple_pads: Locking pad 361 configuration gpio_lock_multiple_pads: Locking pad 362 configuration gpio_lock_multiple_pads: Locking pad 363 configuration gpio_lock_multiple_pads: Locking pad 367 configuration gpio_lock_multiple_pads: Locking pad 330 configuration gpio_lock_multiple_pads: Locking pad 331 configuration gpio_lock_multiple_pads: Locking pad 332 configuration gpio_lock_multiple_pads: Locking pad 333 configuration gpio_lock_multiple_pads: Locking pad 334 configuration gpio_lock_multiple_pads: Locking pad 335 configuration gpio_lock_multiple_pads: Locking pad 336 configuration gpio_lock_multiple_pads: Locking pad 337 configuration soc_lock_gpios: EXIT at 19971509855 soc_lock_gpios: Locking took 243626714 =========================================================================== Below are the diffs for the timing logging code for each patchset. Multi-lock timing code diff : diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index e91ddac82d..b6c7ad9086 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -297,4 +297,5 @@ smm-y += memmove.c smm-y += memset.c smm-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c +smm-y += timestamp.c smm-srcs += $(wildcard src/mainboard/$(MAINBOARDDIR)/smihandler.c) diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 8da875e255..ed25b80c7a 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -118,6 +118,7 @@ ramstage-$(CONFIG_ARCH_RAMSTAGE_X86_32) += gcc.c smm-y += gcc.c endif +smm-y += timestamp.c romstage-$(CONFIG_GENERIC_UDELAY) += timer.c ramstage-$(CONFIG_VENDOR_EMULATION) += ramdetect.c diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 7347d07b16..4df6bca636 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -68,8 +68,8 @@ static int timestamp_should_run(void) * Only check boot_cpu() in other stages than * ENV_PAYLOAD_LOADER on x86. */ - if ((!ENV_PAYLOAD_LOADER && ENV_X86) && !boot_cpu()) - return 0; + //if ((!ENV_PAYLOAD_LOADER && ENV_X86) && !boot_cpu()) + // return 0; return 1; } diff --git a/src/mainboard/google/brya/Kconfig b/src/mainboard/google/brya/Kconfig index e6e44e2f56..a0278389fa 100644 --- a/src/mainboard/google/brya/Kconfig +++ b/src/mainboard/google/brya/Kconfig @@ -45,6 +45,10 @@ config BOARD_GOOGLE_BRYA_COMMON select SOC_INTEL_COMMON_BLOCK_PCIE_RTD3 select SOC_INTEL_CSE_LITE_SKU select INTEL_CAR_NEM #TODO - Enable INTEL_CAR_NEM_ENHANCED + select DEBUG_SMI + select DEBUG_GPIO + select HAVE_DEBUG_GPIO + select COLLECT_TIMESTAMPS_TSC config BASEBOARD_DIR string diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index 1198541f64..9769632cea 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -29,6 +29,9 @@ #include <spi-generic.h> #include <stdint.h> +#include <timestamp.h> +#include <cpu/x86/tsc.h> + /* SoC overrides. */ __weak const struct smm_save_state_ops *get_smm_save_state_ops(void) @@ -323,6 +326,8 @@ __weak const struct gpio_lock_config *mb_gpio_lock_config(size_t *num) return NULL; } +#define DO_TIMESTAMP + static void soc_lock_gpios(void) { const struct gpio_lock_config *soc_gpios; @@ -330,6 +335,13 @@ static void soc_lock_gpios(void) size_t soc_gpio_num; size_t mb_gpio_num; +#ifdef DO_TIMESTAMP + uint64_t tstamp2; + uint64_t tstamp = timestamp_get(); + + printk(BIOS_INFO, "%s: ENTER at %lld\n",__func__, tstamp); +#endif + /* get list of gpios from SoC, exit if SoC does not export a list */ soc_gpios = soc_gpio_lock_config(&soc_gpio_num); @@ -338,6 +350,13 @@ static void soc_lock_gpios(void) /* Lock all mainboard and soc requested gpios */ gpio_lock_multiple_pads(soc_gpios, soc_gpio_num, mb_gpios, mb_gpio_num); + +#ifdef DO_TIMESTAMP + tstamp2 = timestamp_get(); + printk(BIOS_INFO, "%s: EXIT at %lld\n",__func__, tstamp2); + printk(BIOS_INFO, "%s: Locking took %lld\n",__func__, tstamp2 - tstamp); +#endif + } =========================================================================== Here's the logging code for patchset 2 that calls gpio_lock_pad() individually for each gpio to lock : diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index e91ddac82d..c3283aa323 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -295,6 +295,8 @@ smm-$(CONFIG_IDT_IN_EVERY_STAGE) += idt.S smm-y += memcpy.c smm-y += memmove.c smm-y += memset.c +smm-y += timestamp.c smm-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c +smm-y += timestamp.c smm-srcs += $(wildcard src/mainboard/$(MAINBOARDDIR)/smihandler.c) diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 8da875e255..ed25b80c7a 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -118,6 +118,7 @@ ramstage-$(CONFIG_ARCH_RAMSTAGE_X86_32) += gcc.c smm-y += gcc.c endif +smm-y += timestamp.c romstage-$(CONFIG_GENERIC_UDELAY) += timer.c ramstage-$(CONFIG_VENDOR_EMULATION) += ramdetect.c diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 7347d07b16..4df6bca636 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -68,8 +68,8 @@ static int timestamp_should_run(void) * Only check boot_cpu() in other stages than * ENV_PAYLOAD_LOADER on x86. */ - if ((!ENV_PAYLOAD_LOADER && ENV_X86) && !boot_cpu()) - return 0; + //if ((!ENV_PAYLOAD_LOADER && ENV_X86) && !boot_cpu()) + // return 0; return 1; } diff --git a/src/mainboard/google/brya/Kconfig b/src/mainboard/google/brya/Kconfig index e6e44e2f56..a0278389fa 100644 --- a/src/mainboard/google/brya/Kconfig +++ b/src/mainboard/google/brya/Kconfig @@ -45,6 +45,10 @@ config BOARD_GOOGLE_BRYA_COMMON select SOC_INTEL_COMMON_BLOCK_PCIE_RTD3 select SOC_INTEL_CSE_LITE_SKU select INTEL_CAR_NEM #TODO - Enable INTEL_CAR_NEM_ENHANCED + select DEBUG_SMI + select DEBUG_GPIO + select HAVE_DEBUG_GPIO + select COLLECT_TIMESTAMPS_TSC config BASEBOARD_DIR string diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index c1f278dfea..7d00ebb4be 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -29,6 +29,8 @@ #include <spi-generic.h> #include <stdint.h> +#include <timestamp.h> + /* SoC overrides. */ __weak const struct smm_save_state_ops *get_smm_save_state_ops(void) @@ -325,6 +327,10 @@ static void soc_lock_gpios(void) size_t mb_gpio_num; int i, j; + uint64_t ts, ts2; + ts = timestamp_get(); + printk(BIOS_ERR, "%s: ENTER at %lld\n", __func__, ts); + /* get list of gpios from SoC, exit if SoC does not export a list */ soc_gpios = soc_gpio_lock_config(&soc_gpio_num); @@ -345,6 +351,9 @@ static void soc_lock_gpios(void) if (i >= mb_gpio_num) gpio_lock_pad(soc_gpios[j].gpio, soc_gpios[j].action); } + ts2 = timestamp_get(); + printk(BIOS_ERR, "%s: EXIT at %lld\n", __func__, ts2); + printk(BIOS_ERR, "%s: Locking gpios via gpio_lock_pad() took %lld\n", __func__, ts2 - ts); } static void finalize(void) -- To view, visit https://review.coreboot.org/c/coreboot/+/58351 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I42979fb89567d8bcd9392da4fb8c4113ef427b14 Gerrit-Change-Number: 58351 Gerrit-PatchSet: 4 Gerrit-Owner: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 20:04:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Nick Vaccaro <nvaccaro(a)google.com> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/amd/common/fsp: check FSP binary version.
by Name of user not set (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Andrey Petrov, Patrick Rudolph, Julian Schroeder, Felix Held. Name of user not set #1003801 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58869 ) Change subject: src/soc/amd/common/fsp: check FSP binary version. ...................................................................... Patch Set 1: (1 comment) File src/drivers/intel/fsp2_0/util.c: https://review.coreboot.org/c/coreboot/+/58869/comment/d9e74e18_592d8e41 PS1, Line 80: struct fsp_info_header *info = (struct fsp_info_header *) (fsp_file + FSP_INFO_HEADER_OFF); > ok, an also hdr->revision. […] at offset 0x94 we have a fsp_info_header,not a fsp_header. the two structures are different. After all the AMD image revision value shows up at the correct place in the fsp_info_header. fsp_revision holds information pertaining to the FSP standard and does not encode AMD binary version information. -- To view, visit https://review.coreboot.org/c/coreboot/+/58869 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibf50f16b5e9793d946a95970fcdabc4c07289646 Gerrit-Change-Number: 58869 Gerrit-PatchSet: 1 Gerrit-Owner: Name of user not set #1003801 Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 03 Nov 2021 20:04:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Name of user not set #1003801 Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/amd/common/fsp: check FSP binary version.
by Felix Held (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Name of user not set #1003801, Andrey Petrov, Patrick Rudolph, Julian Schroeder. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58869 ) Change subject: src/soc/amd/common/fsp: check FSP binary version. ...................................................................... Patch Set 1: (1 comment) File src/drivers/intel/fsp2_0/util.c: https://review.coreboot.org/c/coreboot/+/58869/comment/5c89c18e_5639cb0e PS1, Line 80: struct fsp_info_header *info = (struct fsp_info_header *) (fsp_file + FSP_INFO_HEADER_OFF); > hdr->fsp_revision should have all needed information ok, an also hdr->revision. but it's all included in hdr which gets passed as an argument to soc_validate_fspm_header -- To view, visit https://review.coreboot.org/c/coreboot/+/58869 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibf50f16b5e9793d946a95970fcdabc4c07289646 Gerrit-Change-Number: 58869 Gerrit-PatchSet: 1 Gerrit-Owner: Name of user not set #1003801 Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Name of user not set #1003801 Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 19:50:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Name of user not set #1003801 Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/amd/common/fsp: check FSP binary version.
by Felix Held (Code Review) 03 Nov '21

03 Nov '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Name of user not set #1003801, Andrey Petrov, Patrick Rudolph, Julian Schroeder. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58869 ) Change subject: src/soc/amd/common/fsp: check FSP binary version. ...................................................................... Patch Set 1: (3 comments) File src/drivers/intel/fsp2_0/include/fsp/info_header.h: https://review.coreboot.org/c/coreboot/+/58869/comment/a974167d_6254280b PS1, Line 34: #define FSP_INFO_HEADER_OFF 0x94 > same offset. different header. same offset, but the fsp_header isn't what's in the binary. see fsp_identify https://review.coreboot.org/c/coreboot/+/58869/comment/8322e04e_a6eaf44f PS1, Line 35: fsp_info_header > this is an fsp_info_header. Quite different from a fsp_header. ok, this isn't a redefinition. fsp_identify looks at the raw header and fills in the fsp_header struct fields. so i'd just use the fsp_header struct like it's done right now and don't use an additional struct for the raw fsp_info_header File src/drivers/intel/fsp2_0/util.c: https://review.coreboot.org/c/coreboot/+/58869/comment/ddc0c988_9cf54693 PS1, Line 80: struct fsp_info_header *info = (struct fsp_info_header *) (fsp_file + FSP_INFO_HEADER_OFF); > where would I get the fsp binary version from? I need the fsp_info_header to get the AMD image revis […] hdr->fsp_revision should have all needed information -- To view, visit https://review.coreboot.org/c/coreboot/+/58869 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibf50f16b5e9793d946a95970fcdabc4c07289646 Gerrit-Change-Number: 58869 Gerrit-PatchSet: 1 Gerrit-Owner: Name of user not set #1003801 Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Name of user not set #1003801 Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Nov 2021 19:46:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Name of user not set #1003801 Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021