summaryrefslogtreecommitdiff
path: root/Documentation
diff options
context:
space:
mode:
authorFurquan Shaikh <furquan@google.com>2021-02-16 23:27:52 -0800
committerMichael Niewöhner <foss@mniewoehner.de>2021-04-16 17:42:12 +0000
commit66ab5eb521fc7dd5995b7fae1a0beaf9dbef01d2 (patch)
treeef49b90b7b750cb76dde18aadd6efc1077209921 /Documentation
parentc7204b5a431f02d0b52e0beba54370ea4eacb216 (diff)
downloadcoreboot-66ab5eb521fc7dd5995b7fae1a0beaf9dbef01d2.tar.xz
[RFC] Address the leftover TODO in soc/intel/cannonlake
Change-Id: I4c989de4d2af3e810fb0e4803d0fa2396917d93e Signed-off-by: Furquan Shaikh <furquan@google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/50829 Reviewed-by: Michael Niewöhner <foss@mniewoehner.de> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Diffstat (limited to 'Documentation')
-rw-r--r--Documentation/RFC/intel-gpio-cleanup.md98
1 files changed, 98 insertions, 0 deletions
diff --git a/Documentation/RFC/intel-gpio-cleanup.md b/Documentation/RFC/intel-gpio-cleanup.md
new file mode 100644
index 0000000000..130132b56f
--- /dev/null
+++ b/Documentation/RFC/intel-gpio-cleanup.md
@@ -0,0 +1,98 @@
+# Background
+
+CB:31250 ("soc/intel/cannonlake: Configure GPIOs again after FSP-S is
+done") introduced a workaround in coreboot for `soc/intel/cannonlake`
+platforms to save and restore GPIO configuration performed by
+mainboard across call to FSP Silicon Init (FSP-S). This workaround was
+required because FSP-S was configuring GPIOs differently than
+mainboard resulting in boot and runtime issues because of
+misconfigured GPIOs. This issue was observed on `google/hatch`
+mainboard and was raised with Intel to get the FSP behavior
+fixed. Until the fix in FSP was available, this workaround was used to
+ensure that the mainboards can operate correctly and were not impacted
+by the GPIO misconfiguration in FSP-S.
+
+The issues observed on `google/hatch` mainboard were fixed by adding
+(if required) and initializing appropriate FSP UPDs. This UPD
+initialization ensured that FSP did not configure any GPIOs
+differently than the mainboard configuration. Fixes included:
+ * CB:31375 ("soc/intel/cannonlake: Configure serial debug uart")
+ * CB:31520 ("soc/intel/cannonlake: Assign FSP UPDs for HPD and Data/CLK of DDI ports")
+ * CB:32176 ("mb/google/hatch: Update GPIO settings for SD card and SPI1 Chip select")
+ * CB:34900 ("soc/intel/cnl: Add provision to configure SD controller write protect pin")
+
+With the above changes merged, it was verified on `google/hatch`
+mainboard that the workaround for GPIO reconfiguration was not
+needed. However, at the time, we missed dropping the workaround in
+'soc/intel/cannonlake`. Currently, this workaround is used by the
+following mainboards:
+ * `google/drallion`
+ * `google/sarien`
+ * `purism/librem_cnl`
+ * `system76/lemp9`
+
+As verified on `google/hatch`, FSP v1263 included all UPD additions
+that were required for addressing this issue.
+
+# Proposal
+
+* The workaround can be safely dropped from `soc/intel/cannonlake`
+ only after the above mainboards have verified that FSP-S does not
+ configure any pads differently than the mainboard in coreboot. Since
+ the fix included initialization of FSP UPDs correctly, the above
+ mainboards can use the following diff to check what pads change
+ after FSP-S has run:
+
+```
+diff --git a/src/soc/intel/common/block/gpio/gpio.c b/src/soc/intel/common/block/gpio/gpio.c
+index 28e78fb366..0cce41b316 100644
+--- a/src/soc/intel/common/block/gpio/gpio.c
++++ b/src/soc/intel/common/block/gpio/gpio.c
+@@ -303,10 +303,10 @@ static void gpio_configure_pad(const struct pad_config *cfg)
+ /* Patch GPIO settings for SoC specifically */
+ soc_pad_conf = soc_gpio_pad_config_fixup(cfg, i, soc_pad_conf);
+
+- if (CONFIG(DEBUG_GPIO))
++ if (soc_pad_conf != pad_conf)
+ printk(BIOS_DEBUG,
+- "gpio_padcfg [0x%02x, %02zd] DW%d [0x%08x : 0x%08x"
+- " : 0x%08x]\n",
++ "%d: gpio_padcfg [0x%02x, %02zd] DW%d [0x%08x : 0x%08x"
++ " : 0x%08x]\n", cfg->pad,
+ comm->port, relative_pad_in_comm(comm, cfg->pad), i,
+ pad_conf,/* old value */
+ cfg->pad_config[i],/* value passed from gpio table */
+```
+
+Depending upon the pads that are misconfigured by FSP-S, these
+mainboards will have to set UPDs appropriately. Once this is verified
+by the above mainboards, the workaround implemented in CB:31250 can be
+dropped.
+
+* The fix implemented in FSP/coreboot for `soc/intel/cannonlake`
+ platforms is not really the right long term solution for the
+ problem. Ideally, FSP should not be touching any GPIO configuration
+ and letting coreboot configure the pads as per mainboard
+ design. This recommendation was accepted and implemented by Intel
+ starting with Jasper Lake and Tiger Lake platforms using a single
+ UPD `GpioOverride` that coreboot can set so that FSP does not change
+ any GPIO configuration. However, this implementation is not
+ backported to any older platforms. Given the issues that we have
+ observed across different platforms, the second proposal is to:
+
+ - Add a Kconfig `CHECK_GPIO_CONFIG_CHANGES` that enables checks
+ in coreboot to stash GPIO pad configuration before various calls
+ to FSP and compares the configuration on return from FSP.
+ - This will have to be implemented as part of
+ drivers/intel/fsp/fsp2_0/ to check for the above config selection
+ and make callbacks `gpio_snapshot()` and `gpio_verify_snapshot()`
+ to identify and print information about pads that have changed
+ configuration after calls to FSP.
+ - This config can be kept disabled by default and mainboard
+ developers can enable them as and when required for debug.
+ - This will be helpful not just for the `soc/intel/cannonlake`
+ platforms that want to get rid of the above workaround, but also
+ for all future platforms using FSP to identify and catch any GPIO
+ misconfigurations that might slip in to any platforms (in case the
+ `GpioOverride` UPD is not honored by any code path within FSP).
+