Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47501 )
Change subject: mb/google/volteer/v/volteer2: Config for passive USB-C DB on C1 ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47501/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/47501/3/src/mainboard/google/voltee... PS3, Line 129: /* Set up port C1 for USB3 passive DB */ : if (fw_config_probe(FW_CONFIG(DB_USB, USB3_PASSIVE))) { : /* : * TGL UP3/UP4 Processor EDS vol. 2a rev. 1.2 : * section 3.6.10 : * set IOM_TYPEC_SW_CONFIGURATION_4.PORT2_HSL_ORIENTATION_OVRRD_EN : */ : cfg->TcssAuxOri |= BIT(2); : cfg->IomTypeCPortPadCfg[2] = GPIO_ID_TCP1_AUXP_DC; : cfg->IomTypeCPortPadCfg[3] = GPIO_ID_TCP1_AUXN_DC; : }
hmmm... can we update struct soc_intel_tigerlake_config from there? […]
We can (using `config_of_soc()`), and if we trust the `GpioOverride` UPD in FSP-S, then we could move the boot state we call `fw_config_handle` from. Right now it's BS_DEV_ENABLE, but it could be earlier (BS_PRE_DEVICE) so that it definitely runs before FSP-S. I'll take a look at doing that in a followup.
https://review.coreboot.org/c/coreboot/+/47501/7/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/47501/7/src/mainboard/google/voltee... PS7, Line 129: #if CONFIG(VARIANT_HAS_PASSIVE_USB_DB) : /* Set up port C1 for USB3 passive DB */ : if (fw_config_probe(FW_CONFIG(DB_USB, USB3_PASSIVE))) { : /* : * TGL UP3/UP4 Processor EDS vol. 2a rev. 1.2 : * section 3.6.10 : * set IOM_TYPEC_SW_CONFIGURATION_4.PORT2_HSL_ORIENTATION_OVRRD_EN : */ : cfg->TcssAuxOri |= IOM_TCSS_PORT_CTRL(1, : IOM_TCSS_ORI_NORMAL, IOM_TCSS_MUX_INTERNAL); : cfg->IomTypeCPortPadCfg[2] = GPIO_ID_TCP1_AUXP_DC; : cfg->IomTypeCPortPadCfg[3] = GPIO_ID_TCP1_AUXN_DC; : } : #endif /* VARIANT_HAS_PASSIVE_USB_DB */ suggestion: add a new variant-specific function to handle this, e.g., `void variant_configure_usb_db(void)`
with a __weak implementation here that does nothing.
The #if goes away here, e.g. just call `variant_configure_usb_db()`
then in variants/volteer2/variant.c (new file), implement it just like it is here, e.g.
``` void variant_configure_usb_db(void) { if (fw_config_probe(FW_CONFIG(DB_USB, USB3_PASSIVE))) { /* * TGL UP3/UP4 Processor EDS vol. 2a rev. 1.2 * section 3.6.10 * set IOM_TYPEC_SW_CONFIGURATION_4.PORT2_HSL_ORIENTATION_OVRRD_EN */ cfg->TcssAuxOri |= IOM_TCSS_PORT_CTRL(1, IOM_TCSS_ORI_NORMAL, IOM_TCSS_MUX_INTERNAL); cfg->IomTypeCPortPadCfg[2] = GPIO_ID_TCP1_AUXP_DC; cfg->IomTypeCPortPadCfg[3] = GPIO_ID_TCP1_AUXN_DC; } } ```
Then the whole VARIANT_HAS_PASSIVE_DB goes away