Documentation/RFC/intel-gpio-cleanup.md

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