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)
-
Nick Vaccaro (Code Review)