[coreboot-gerrit] Change in coreboot[master]: soc/intel/cannonlake: Configure GPIOs again after FSP-S is done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31250 )

Change subject: soc/intel/cannonlake: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 5:

(1 comment)

Patchset:

PS5:
> Furquan, do you remember what pads exactly were overwritten? I'm surprised there was no problem in s […]

I had used this diff:

diff --git a/src/soc/intel/common/block/gpio/gpio.c b/src/soc/intel/common/block/gpio/gpio.c
index 294218c42f..34d46bfe4d 100644
--- a/src/soc/intel/common/block/gpio/gpio.c
+++ b/src/soc/intel/common/block/gpio/gpio.c
@@ -265,10 +265,10 @@ static void gpio_configure_pad(const struct pad_config *cfg)
                soc_pad_conf &= mask[i];
                soc_pad_conf |= pad_conf & ~mask[i];

-               if (IS_ENABLED(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 */

And this was the output I got for hatch:

17: gpio_padcfg [0x6e, 17] DW0 [0x40000600 : 0x40000400 : 0x40000400]
31: gpio_padcfg [0x6e, 31] DW0 [0x40000700 : 0x40000400 : 0x40000400]
33: gpio_padcfg [0x6e, 33] DW0 [0x40000702 : 0x40000400 : 0x40000402]
201: gpio_padcfg [0x6a, 20] DW0 [0x40000500 : 0x40000100 : 0x40000100]
202: gpio_padcfg [0x6a, 21] DW0 [0x40100500 : 0x40900100 : 0x40900100]
206: gpio_padcfg [0x6a, 25] DW0 [0x40000402 : 0x40000800 : 0x40000802]
220: gpio_padcfg [0x6a, 39] DW0 [0x40000500 : 0x40000100 : 0x40000100]
221: gpio_padcfg [0x6a, 40] DW0 [0x40000500 : 0x40000100 : 0x40000100]
223: gpio_padcfg [0x6a, 42] DW0 [0x40000500 : 0x40000100 : 0x40000100]
225: gpio_padcfg [0x6a, 44] DW0 [0x40000500 : 0x40000100 : 0x40000100]
227: gpio_padcfg [0x6a, 46] DW0 [0x40000500 : 0x40000100 : 0x40000100]
51: gpio_padcfg [0x6e, 51] DW1 [0x00003c6c : 0x00000000 : 0x0000006c]
52: gpio_padcfg [0x6e, 52] DW1 [0x00003c6d : 0x00000000 : 0x0000006d]
53: gpio_padcfg [0x6e, 53] DW1 [0x00003c6e : 0x00000000 : 0x0000006e]
54: gpio_padcfg [0x6e, 54] DW1 [0x00003c6f : 0x00000000 : 0x0000006f]
55: gpio_padcfg [0x6e, 55] DW1 [0x00003c70 : 0x00000000 : 0x00000070]
58: gpio_padcfg [0x6e, 58] DW0 [0x40000500 : 0x40000100 : 0x40000100]

I have this in notes and internal bugs:

In some cases, FSP is just configuring a GPIO for native mode when the board requires it to be a GPI or GPO. In other cases, the termination is set differently and in some cases no-connect GPIOs are being configured as native functions.

Again from internal bug, I see that this was fixed in FSP or coreboot by adding and initializing required UPDs. Examples:
https://review.coreboot.org/c/coreboot/+/31520
https://review.coreboot.org/c/coreboot/+/34900

Eventually, for TGL/JSL, Intel added `GpioOverride` that skips all GPIO configuration in FSP if this UPD is set by coreboot.