[coreboot-gerrit] [L] Change in coreboot[main]: mb/google/fatcat: Add GPIO settings

[Subrata Banik (Code Review)]

Attention is currently required from: Jérémy Compostella, Pranava Y N.

Subrata Banik has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/84405?usp=email )

Change subject: mb/google/fatcat: Add GPIO settings
......................................................................

Patch Set 1:

(8 comments)

File src/mainboard/google/fatcat/variants/fatcat/gpio.c:

https://review.coreboot.org/c/coreboot/+/84405/comment/f12bee74_22e1867c?usp... :
PS1, Line 10: 	/* GPP_A */
remove such comments from this file.

```/* GPP_A00 */``` itself if enough to know that it belongs to GPP_A community

https://review.coreboot.org/c/coreboot/+/84405/comment/766442fc_9a8f57d4?usp... :
PS1, Line 416: early_gpio_table
ideally boot block code should program minimal GPIOs like UART, TPM

https://review.coreboot.org/c/coreboot/+/84405/comment/4f4dec2b_4552d09d?usp... :
PS1, Line 422: 	/* GPP_H10:     UART0_BUF_RTS */
             : 	PAD_CFG_NF(GPP_H10, NONE, DEEP, NF1),
             : 	/* GPP_H11:     UART0_BUF_CTS */
             : 	PAD_CFG_NF(GPP_H11, NONE, DEEP, NF1),
I don't think u need these

https://review.coreboot.org/c/coreboot/+/84405/comment/1b4ec3a8_d6717153?usp... :
PS1, Line 427: 	/* GPP_A08:     X1_PCIE_SLOT_PWR_EN */
             : 	PAD_CFG_GPO(GPP_A08, 0, PLTRST),
             : 
why we are enabling x1 power port here w/o probing the FW config ?

https://review.coreboot.org/c/coreboot/+/84405/comment/c63eee51_42cea985?usp... :
PS1, Line 437: 	/* GPP_B16:     GEN5_SSD_PWREN */
             : 	PAD_CFG_GPO(GPP_B16, 0, PLTRST),
this should be done using a FW config

https://review.coreboot.org/c/coreboot/+/84405/comment/0498eb22_7b238272?usp... :
PS1, Line 440: 	/* GPP_C05:     CRD1_PWREN */
I don't think we are using CRD1, please don't keep everyone power on by default which will have an impact on idle power

https://review.coreboot.org/c/coreboot/+/84405/comment/6732bc91_ab7031e2?usp... :
PS1, Line 452: 	/* M.2 WWAN */
same, this should be inside FW config.

https://review.coreboot.org/c/coreboot/+/84405/comment/08fc9ed5_13523019?usp... :
PS1, Line 459: 
             : 	/* GPP_D03:     M.2_WWAN_PERST_GPIO_N */
same