[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>

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).
+