Tommie Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43442 )
Change subject: mb/google/volteer: Add new variant Lingcod ......................................................................
Patch Set 11:
(22 comments)
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/lingcod/gpio.c:
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 8: /* Leave eSPI pins untouched from default settings */
remove this.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 9: gpio_table
rename to override_gpio_table to align with the other variant.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 10: /* A0 thru A6 come configured out of reset, do not touch */ : /* A0 : ESPI_IO0 ==> ESPI_IO_0 */ : /* A1 : ESPI_IO1 ==> ESPI_IO_1 */ : /* A2 : ESPI_IO2 ==> ESPI_IO_2 */ : /* A3 : ESPI_IO3 ==> ESPI_IO_3 */ : /* A4 : ESPI_CS# ==> ESPI_CS_L */ : /* A5 : ESPI_CLK ==> ESPI_CLK */ : /* A6 : ESPI_RESET# ==> NC(TP764) */
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 28: /* A18 : DDSP_HPDB ==> NOT USED */ : PAD_NC(GPP_A18, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 34: /* A21 : DDPC_CTRCLK ==> NOT USED */ : PAD_NC(GPP_A21, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 49: /* B19 : GSPI1_CS0# ==> NOT USED */ : PAD_NC(GPP_B19, NONE), : /* B20 : GSPI1_CLK ==> NOT USED */ : PAD_NC(GPP_B20, NONE), : /* B21 : GSPI1_MISO ==> NOT USED */ : PAD_NC(GPP_B21, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 60: /* C7 : SML1DATA ==> NOT USED */ : PAD_NC(GPP_C7, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 64: /* C11 : UART0_CTS# ==> NOT USED */ : PAD_NC(GPP_C11, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 74: /* C20 : UART2_RXD ==> NOT USED */ : PAD_NC(GPP_C20, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 81: /* D5 : SRCCLKREQ0$ ==> SSD_CLKREQ_ODL */ : PAD_CFG_NF(GPP_D5, NONE, DEEP, NF1),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 109: /* F6 : CNV_PA_BLANKING ==> NC */ : PAD_NC(GPP_F6, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 120: /* H8 : I2C4_SDA ==> NOT USED */ : PAD_NC(GPP_H8, NONE), : /* H9 : I2C4_SCL ==> NOT USED */ : PAD_NC(GPP_H9, NONE),
remove this. This is same as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 198: gpio_table
rename to override_gpio_table to align with the other variant.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 199: gpio_table
rename to override_gpio_table to align with the other variant.
Done
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/lingcod/gpio.c:
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 51: /* C22 : UART2_RTS# ==> PCH_FPMCU_BOOT0 */ : PAD_CFG_GPO(GPP_C22, 0, DEEP), : /* C23 : UART2_CTS# ==> FPMCU_RST_ODL */ : PAD_CFG_GPO(GPP_C23, 1, DEEP),
Could you please double check if you need to support the fingerprint? I don' find the fingerprint fr […]
We do not support fingerprint and PEN.
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 62: /* D17 : ISH_GP4 ==> EN_CVF_PWR */ : PAD_CFG_GPO(GPP_D17, 1, DEEP),
Do you need this? From the schematics, the PP3300_FCAM doesn't connect to anything. […]
Configure EN_FCAM_PWR to output HINGE_COMBO_P22 through U85.
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 65: /* E1 : SPI1_IO2 ==> PEN_DET_ODL */ : PAD_CFG_GPI_SCI_LOW(GPP_E1, NONE, DEEP, EDGE_SINGLE),
Will the project support the pen ejection? I don't see the implementation from the schematics.
Done
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 77: /* E12 : SPI1_MISO_IO1 ==> PEN_ALERT_ODL */ : PAD_CFG_GPI(GPP_E12, NONE, DEEP),
Please check if this is needed. I don't see the pen charger.
Done
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 97: /* H19 : TIME_SYNC0 ==> USER_PRES_FP_ODL */ : PAD_CFG_GPI(GPP_H19, NONE, DEEP),
Could you please double check if you need to support the fingerprint?
Done
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 153: /* C22 : UART2_RTS# ==> PCH_FPMCU_BOOT0 */ : PAD_CFG_GPO(GPP_C22, 0, DEEP), :
Could you please double check if you need to support the fingerprint?
Done
https://review.coreboot.org/c/coreboot/+/43442/10/src/mainboard/google/volte... PS10, Line 156: /* E12 : SPI1_MISO_IO1 ==> PEN_ALERT_ODL */ : PAD_CFG_GPO(GPP_E12, 1, DEEP),
Please check if this is needed. I don't see the pen charger.
Done
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/lingcod/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43442/9/src/mainboard/google/voltee... PS9, Line 23: register "SerialIoI2cMode" = "{
Nit: I think this has to define all SerialIO devices. […]
All 6 elements of the SerialIoI2cMode array are defined.