Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38860 )
Change subject: mb/google/hatch: Create palkia variant ......................................................................
Patch Set 20: Code-Review+1
(15 comments)
Patch Set 20: Code-Review+1
Could anyone please +2? or any other suggestion?
Remember that, once the suggestions in the comments have been done, they need to be marked as resolved. I have done that for you 😊
Other than one last comment, I would say this looks good.
https://review.coreboot.org/c/coreboot/+/38860/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/palkia/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38860/6/src/mainboard/google/hatch/... PS6, Line 22: ramstage-y += ramstage.c
This should be removed too if the ramstage.c is removed. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/palkia/gpio.c:
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 26: /* A11 : GSPI1_CS1# ==> NC */ : PAD_NC(GPP_A11, NONE), : /* A12 : ISH_GP6 ==> NC */ : PAD_NC(GPP_A12, NONE),
Remove this. This should be inherited from baseboard gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 39: PAD_NC(GPP_A23, NONE), : /* B19 : GSPI1_CS0# ==> NC */
Add a newline when changing the GPIO group. ie: […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 52: /* C6 : GPP_C6 ==> NC */ : PAD_NC(GPP_C6, NONE),
Remove this. This should be inherited from baseboard gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 58: /* D5 : ISH_I2C0_SDA ==> NC */ : PAD_NC(GPP_D5, NONE), : /* D6 : ISH_I2C0_SCL ==> NC */ : PAD_NC(GPP_D6, NONE), : /* D7 : ISH_I2C1_SDA ==> NC */ : PAD_NC(GPP_D7, NONE), : /* D8 : ISH_I2C1_SCL ==> NC */ : PAD_NC(GPP_D8, NONE), : /* D10 : ISH_SPI_CLK ==> NC */ : PAD_NC(GPP_D10, NONE),
Remove this. This should be inherited from baseboard gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 70: /* D21 : SPI1_IO2 ==> NC */ : PAD_NC(GPP_D21, NONE),
Remove this. This should be inherited from baseboard gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 72: /* F0 : GPP_F0 ==> NC */
Remove this.
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 73: /* E12 : USB_A_OC_OD USB_OC3 */ : PAD_CFG_NF(GPP_E12, NONE, DEEP, NF1),
Remove this. This should be inherited from baseboard gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 163: /* : * Default GPIO settings before entering non-S5 sleep states. : * Configure A12: FPMCU_RST_ODL as GPO before entering sleep. : * This guarantees that A12's native3 function is disabled. : * See https://review.coreboot.org/c/coreboot/+/32111 . : */ : static const struct pad_config default_sleep_gpio_table[] = { : PAD_CFG_GPO(GPP_A12, 1, DEEP), /* FPMCU_RST_ODL */ : }; :
Remove this, we don't use GPP_A12.
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 173: /* : * GPIO settings before entering S5, which are same as : * default_sleep_gpio_table but also, turn off FPMCU. : */ : static const struct pad_config s5_sleep_gpio_table[] = { : PAD_CFG_GPO(GPP_A12, 0, DEEP), /* FPMCU_RST_ODL */ : PAD_CFG_GPO(GPP_C11, 0, DEEP), /* PCH_FP_PWR_EN */ : }; :
Remove this. We don't use GPP_A12 and GPP_C11. […]
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 182: const struct pad_config *variant_sleep_gpio_table(u8 slp_typ, size_t *num) : { : if (slp_typ == ACPI_S5) { : *num = ARRAY_SIZE(s5_sleep_gpio_table); : return s5_sleep_gpio_table; : } : *num = ARRAY_SIZE(default_sleep_gpio_table); : return default_sleep_gpio_table; : }
Remove this if the sleep_gpio_table is removed.
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/palkia/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 103: TSR0
Could you please double check this value?
Done
https://review.coreboot.org/c/coreboot/+/38860/20/src/mainboard/google/hatch... File src/mainboard/google/hatch/variants/palkia/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/38860/20/src/mainboard/google/hatch... PS20, Line 95: TSR0 TSR3
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/palkia/ramstage.c:
PS2:
Remove this file, i don't think we need this.
Done
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 29: gpio_output(GPP_C11, 1); : mdelay(1); : gpio_output(GPP_A12, 1);
Remove this. We don't need this.
Done