Furquan Shaikh 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:
(11 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. Is that a miss or intentional?
https://review.coreboot.org/c/coreboot/+/41797/7/src/mainboard/google/voltee... PS7, Line 128: CVF_LPSS_INT_L What does this line do?
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.c
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.
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.c
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.
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.c
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. Are the COEX pins used?
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?
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.
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.