Documentation/RFC/intel-gpio-cleanup.md

   1 # Background
   2
   3 CB:31250 ("soc/intel/cannonlake: Configure GPIOs again after FSP-S is
   4 done") introduced a workaround in coreboot for `soc/intel/cannonlake`
   5 platforms to save and restore GPIO configuration performed by
   6 mainboard across call to FSP Silicon Init (FSP-S). This workaround was
   7 required because FSP-S was configuring GPIOs differently than
   8 mainboard resulting in boot and runtime issues because of
   9 misconfigured GPIOs. This issue was observed on `google/hatch`
  10 mainboard and was raised with Intel to get the FSP behavior
  11 fixed. Until the fix in FSP was available, this workaround was used to
  12 ensure that the mainboards can operate correctly and were not impacted
  13 by the GPIO misconfiguration in FSP-S.
  14
  15 The issues observed on `google/hatch` mainboard were fixed by adding
  16 (if required) and initializing appropriate FSP UPDs. This UPD
  17 initialization ensured that FSP did not configure any GPIOs
  18 differently than the mainboard configuration. Fixes included:
  19  * CB:31375 ("soc/intel/cannonlake: Configure serial debug uart")
  20  * CB:31520 ("soc/intel/cannonlake: Assign FSP UPDs for HPD and Data/CLK of DDI ports")
  21  * CB:32176 ("mb/google/hatch: Update GPIO settings for SD card and SPI1 Chip select")
  22  * CB:34900 ("soc/intel/cnl: Add provision to configure SD controller write protect pin")
  23
  24 With the above changes merged, it was verified on `google/hatch`
  25 mainboard that the workaround for GPIO reconfiguration was not
  26 needed. However, at the time, we missed dropping the workaround in
  27 'soc/intel/cannonlake`. Currently, this workaround is used by the
  28 following mainboards:
  29  * `google/drallion`
  30  * `google/sarien`
  31  * `purism/librem_cnl`
  32  * `system76/lemp9`
  33
  34 As verified on `google/hatch`, FSP v1263 included all UPD additions
  35 that were required for addressing this issue.
  36
  37 # Proposal
  38
  39 * The workaround can be safely dropped from `soc/intel/cannonlake`
  40   only after the above mainboards have verified that FSP-S does not
  41   configure any pads differently than the mainboard in coreboot. Since
  42   the fix included initialization of FSP UPDs correctly, the above
  43   mainboards can use the following diff to check what pads change
  44   after FSP-S has run:
  45
  46 ```
  47 diff --git a/src/soc/intel/common/block/gpio/gpio.c b/src/soc/intel/common/block/gpio/gpio.c
  48 index 28e78fb366..0cce41b316 100644
  49 --- a/src/soc/intel/common/block/gpio/gpio.c
  50 +++ b/src/soc/intel/common/block/gpio/gpio.c
  51 @@ -303,10 +303,10 @@ static void gpio_configure_pad(const struct pad_config *cfg)
  52                 /* Patch GPIO settings for SoC specifically */
  53                 soc_pad_conf = soc_gpio_pad_config_fixup(cfg, i, soc_pad_conf);
  54
  55 -               if (CONFIG(DEBUG_GPIO))
  56 +               if (soc_pad_conf != pad_conf)
  57                         printk(BIOS_DEBUG,
  58 -                       "gpio_padcfg [0x%02x, %02zd] DW%d [0x%08x : 0x%08x"
  59 -                       " : 0x%08x]\n",
  60 +                       "%d: gpio_padcfg [0x%02x, %02zd] DW%d [0x%08x : 0x%08x"
  61 +                       " : 0x%08x]\n", cfg->pad,
  62                         comm->port, relative_pad_in_comm(comm, cfg->pad), i,
  63                         pad_conf,/* old value */
  64                         cfg->pad_config[i],/* value passed from gpio table */
  65 ```
  66
  67 Depending upon the pads that are misconfigured by FSP-S, these
  68 mainboards will have to set UPDs appropriately. Once this is verified
  69 by the above mainboards, the workaround implemented in CB:31250 can be
  70 dropped.
  71
  72 * The fix implemented in FSP/coreboot for `soc/intel/cannonlake`
  73   platforms is not really the right long term solution for the
  74   problem. Ideally, FSP should not be touching any GPIO configuration
  75   and letting coreboot configure the pads as per mainboard
  76   design. This recommendation was accepted and implemented by Intel
  77   starting with Jasper Lake and Tiger Lake platforms using a single
  78   UPD `GpioOverride` that coreboot can set so that FSP does not change
  79   any GPIO configuration. However, this implementation is not
  80   backported to any older platforms. Given the issues that we have
  81   observed across different platforms, the second proposal is to:
  82
  83   - Add a Kconfig `CHECK_GPIO_CONFIG_CHANGES` that enables checks
  84     in coreboot to stash GPIO pad configuration before various calls
  85     to FSP and compares the configuration on return from FSP.
  86   - This will have to be implemented as part of
  87     drivers/intel/fsp/fsp2_0/ to check for the above config selection
  88     and make callbacks `gpio_snapshot()` and `gpio_verify_snapshot()`
  89     to identify and print information about pads that have changed
  90     configuration after calls to FSP.
  91   - This config can be kept disabled by default and mainboard
  92     developers can enable them as and when required for debug.
  93   - This will be helpful not just for the `soc/intel/cannonlake`
  94     platforms that want to get rid of the above workaround, but also
  95     for all future platforms using FSP to identify and catch any GPIO
  96     misconfigurations that might slip in to any platforms (in case the
  97     `GpioOverride` UPD is not honored by any code path within FSP).
  98