Zhuohao Lee has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38860 )
Change subject: mb/google/palkia: Create palkia variant ......................................................................
Patch Set 2:
(25 comments)
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.c
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: PAD_NC(GPP_A23, NONE),
/* B19 : GSPI1_CS0# ==> NC */
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.c
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.c
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.c
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 72: /* F0 : GPP_F0 ==> NC */ Remove this.
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.c
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.
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. Besides, please add the below to gpio_table[]:
PAD_NC(GPP_C11, NONE),
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.
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 80: 90, 70, 50, 50 Do we need to add this?
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 103: TSR0 Could you please double check this value?
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/palkia/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 20: register "usb2_ports[3]" = "USB2_PORT_LONG(OC3)" # Type A : register "usb3_ports[3]" = "USB3_PORT_DEFAULT(OC3)" # Type A : From the schematics, you don't need to set port3. Instead, you should change the port2 setting to internal USB for SD card, like this:
register "usb2_ports[2]" = "USB2_PORT_LONG(OC_SKIP)" # SD CARD register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC_SKIP)" # SD CARD
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 30: #| GSPI1 | FP MCU | Remove this.
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 63: .gspi[0] = { : .speed_mhz = 1, : .early_init = 1, : }, : }" Remove this.
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 72: device usb 0.0 on No right type-c port. Add this: chip drivers/usb/acpi # No Right Type-C port device usb 2.1 off end end
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 73: chip drivers/usb/acpi : # No Type-A port : device usb 2.2 off end : end From Schematics, this should be like this:
chip drivers/usb/acpi register "desc" = ""Micro SD Card"" register "type" = "UPC_TYPE_INTERNAL" device usb 2.2 on end end
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 77: chip drivers/usb/acpi : # No Type-A Port : device usb 2.3 off end : end From Schematics, this should be like this: chip drivers/usb/acpi register "desc" = ""Left Type-A Port"" register "type" = "UPC_TYPE_A" register "group" = "ACPI_PLD_GROUP(1, 2)" device usb 2.3 on end end
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 85: chip drivers/usb/acpi : register "desc" = ""Left Type-A Port"" : register "type" = "UPC_TYPE_A" : register "group" = "ACPI_PLD_GROUP(1, 2)" : device usb 2.4 on end : end : chip drivers/usb/acpi : register "desc" = ""Left Type-A Port"" : register "type" = "UPC_TYPE_A" : register "group" = "ACPI_PLD_GROUP(1, 2)" : device usb 3.4 on end : end Remove this. I don't see this port from the schematics.
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 97: end Add USB 3 setting as below: chip drivers/usb/acpi # No Right Type-C port device usb 3.1 off end end chip drivers/usb/acpi register "desc" = ""Micro SD card"" register "type" = "UPC_TYPE_INTERNAL" device usb 3.2 on end end chip drivers/usb/acpi register "desc" = ""Left Type-A Port"" register "type" = "UPC_TYPE_A" register "group" = "ACPI_PLD_GROUP(1, 2)" device usb 3.3 on end end
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 121: register "generic.reset_gpio" = : "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" : register "generic.reset_delay_ms" = "120" : register "generic.reset_off_delay_ms" = "1" Remove this. From schematics, it looks like we don't use this pin.
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 137: chip drivers/generic/gpio_keys : register "name" = ""PENH"" : register "gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_A8)" : register "key.wake" = "GPE0_DW0_08" : register "key.wakeup_event_action" = "EV_ACT_ASSERTED" : register "key.dev_name" = ""EJCT"" : register "key.linux_code" = "SW_PEN_INSERTED" : register "key.linux_input_type" = "EV_SW" : register "key.label" = ""pen_eject"" : device generic 0 on end : end Remove this.
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 157: register "generic.reset_gpio" = : "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" : register "generic.reset_delay_ms" = "120" : register "generic.reset_off_delay_ms" = "1" Remove this. From schematics, it looks like we don't use this pin.
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... PS2, Line 206: device pci 1e.3 on : chip drivers/spi/acpi : register "name" = ""CRFP"" : register "hid" = "ACPI_DT_NAMESPACE_HID" : register "uid" = "1" : register "compat_string" = ""google,cros-ec-spi"" : register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A23_IRQ)" : register "wake" = "GPE0_DW0_23" : device spi 1 on end : end # FPMCU : end # GSPI #1 We don't use GSPI1, remove it. ie:
# GSPI #1 unused device pci 1e.3 off end
https://review.coreboot.org/c/coreboot/+/38860/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/palkia/ramstage.c:
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.