Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41797 )
Change subject: mb/google/volteer: move volteer-specific GPIOs to variant gpio.c ......................................................................
Patch Set 7:
(16 comments)
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 46: USB_C0_DP_HPD
Just curious: I see that USB_C0_OC_ODL is retained in baseboard, but HPD is being moved to variant. […]
It was a miss, thanks for the catch.
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 128: CVF_LPSS_INT_L
What does this line do?
It is connected via jumper to CVF_LPSS_INT_R_L (1.8V, I). PU, 10K on CVF module. Ask me offline if you want more detail.
I'm going to change this to a NC, since it's not currently used on volteer and we removed all the other support for the module that this was added for.
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/malefor/gpio.c:
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 85: /* D9 : ISH_SPI_CS# ==> NOT USED */ : PAD_NC(GPP_D9, NONE), : /* D10 : ISH_SPI_CLK ==> PCH_GSPI2_CVF_CLK_STRAP */ : PAD_NC(GPP_D10, NONE), : /* D11 : ISH_SPI_MISO ==> NOT USED */ : PAD_NC(GPP_D11, NONE), : /* D12 : ISH_SPI_MOSI ==> PCH_GSPI2_CVF_MISO_STRAP */ : PAD_NC(GPP_D12, NONE),
All these can be dropped. They now match baseboard/gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 93: /* D13 : ISH_UART0_RXD ==> UART_ISH_RX_DEBUG_TX */ : PAD_CFG_NF(GPP_D13, NONE, DEEP, NF1), : /* D14 : ISH_UART0_TXD ==> UART_ISH_TX_DEBUG_RX */ : PAD_CFG_NF(GPP_D14, NONE, DEEP, NF1),
I don't think we have ISH being used on malefor.
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 110: /* E6 : THC0_SPI1_RST# ==> GPPE6_STRAP */ : PAD_NC(GPP_E6, NONE),
Can be dropped. Same as baseboard/gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 114: /* E10 : SPI1_CS# ==> USB_C0_AUXP_DC */ : PAD_CFG_GPO(GPP_E10, 0, DEEP),
This pin should removed since MB side need AUX function which should keep same as baseboard settings […]
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 120: /* E13 : SPI1_MOSI_IO0 ==> USB_C0_AUXN_DC */ : PAD_CFG_GPO(GPP_E13, 0, DEEP),
This pin should removed since MB side need AUX function which should keep same as baseboard settings […]
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 125: /* F6 : CNV_PA_BLANKING ==> WWAN_WLAN_COEX3 */ : PAD_CFG_NF(GPP_F6, NONE, DEEP, NF1),
Same question about COEX as below.
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 129: /* F10 : GPPF10_STRAP */ : PAD_NC(GPP_F10, DN_20K),
This can be dropped. It is the same as in baseboard/gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 138: /* H8 : I2C4_SDA ==> WWAN_WLAN_COEX1 */ : PAD_CFG_NF(GPP_H8, NONE, DEEP, NF2), : /* H9 : I2C4_SCL ==> WWAN_WLAN_COEX2 */ : PAD_CFG_NF(GPP_H9, NONE, DEEP, NF2),
Looks like malefor doesn't have WWAN. […]
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 150: /* H20 : IMGCLKOUT1 ==> EN_MIPI_RCAM_PWR */ : PAD_CFG_GPO(GPP_H20, 1, PLTRST),
malefor doesn't use MIPI camera, so this pin should same as baseboard with NC settings.
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 168: /* S0 : SNDW0_CLK ==> SNDW0_HP_CLK */ : PAD_CFG_NF(GPP_S0, NONE, DEEP, NF1), : /* S1 : SNDW0_DATA ==> SNDW0_HP_DATA */ : PAD_CFG_NF(GPP_S1, NONE, DEEP, NF1), : /* S2 : SNDW1_CLK ==> SNDW1_SPKR_CLK */ : PAD_CFG_NF(GPP_S2, NONE, DEEP, NF1), : /* S3 : SNDW1_DATA ==> SNDW1_SPKR_DATA */ : PAD_CFG_NF(GPP_S3, NONE, DEEP, NF1),
Is this correct? It looks like malefor isn't using SNDW based on the other pad configuration?
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 181: /* GPD9: SLP_WLAN# ==> SLP_WLAN_L */ : PAD_CFG_NF(GPD9, NONE, DEEP, NF1),
Does malefor really use this? I think this goes NC on most boards.
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 183: /* GPD10: SLP_S5# ==> SLP_S5_L */ : PAD_CFG_NF(GPD10, NONE, DEEP, NF1),
This is the same as baseboard/gpio.c. No need to add to variant here.
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 222: /* F11 : THC1_SPI2_CLK ==> EN_PP3300_WWAN */ : PAD_CFG_GPO(GPP_F11, 1, DEEP), : : /* F12 : GSXDOUT ==> WWAN_RST_ODL : To meet timing constrains - drive reset low. : Deasserted in ramstage. */ : PAD_CFG_GPO(GPP_F12, 0, DEEP),
malefor doesn't need these WWAN related GPIO settings, should keep NC.
Done
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/gpio.c:
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 163: /* H19 : TIME_SYNC0 ==> USER_PRES_FP_ODL */
'PRES' may be misspelled - perhaps 'PRESS'?
Ack