Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33824 )
Change subject: mb/google/hatch: Create Akemi variant ......................................................................
Patch Set 23:
(21 comments)
Patch Set 19:
Patch Set 19:
Patch Set 19:
Sorry Peichao, because of the rebase, there are a few changes that need to be made:
override_early_gpio_table() is now variant_early_gpio_table() AND
it needs to contain ALL of the early GPIOs needed by the board, because of a quirk in the way the override- code works. I haven't taken a look at the Akemi schematic yet, but depending on how closely you followed the reference schematic, these are probably the items you need to add to the early_gpio_table:
/* A12 : FPMCU_RST_ODL */ PAD_CFG_GPO(GPP_A12, 0, DEEP), /* B15 : H1_SLAVE_SPI_CS_L */ PAD_CFG_NF(GPP_B15, NONE, DEEP, NF1), /* B16 : H1_SLAVE_SPI_CLK */ PAD_CFG_NF(GPP_B16, NONE, DEEP, NF1), /* B17 : H1_SLAVE_SPI_MISO_R */ PAD_CFG_NF(GPP_B17, NONE, DEEP, NF1), /* B18 : H1_SLAVE_SPI_MOSI_R */ PAD_CFG_NF(GPP_B18, NONE, DEEP, NF1), /* C14 : BT_DISABLE_L */ PAD_CFG_GPO(GPP_C14, 0, DEEP), /* PCH_WP_OD */ PAD_CFG_GPI(GPP_C20, NONE, DEEP), /* C21 : H1_PCH_INT_ODL */ PAD_CFG_GPI_APIC(GPP_C21, NONE, PLTRST, LEVEL, INVERT), /* C23 : WLAN_PE_RST# */ PAD_CFG_GPO(GPP_C23, 1, DEEP), /* E1 : M2_SSD_PEDET */ PAD_CFG_NF(GPP_E1, NONE, DEEP, NF1), /* E5 : SATA_DEVSLP1 */ PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* F2 : MEM_CH_SEL */ PAD_CFG_GPI(GPP_F2, NONE, PLTRST), /* F11 : PCH_MEM_STRAP2 */ PAD_CFG_GPI(GPP_F11, NONE, PLTRST), /* F20 : PCH_MEM_STRAP0 */ PAD_CFG_GPI(GPP_F20, NONE, PLTRST), /* F21 : PCH_MEM_STRAP1 */ PAD_CFG_GPI(GPP_F21, NONE, PLTRST), /* F22 : PCH_MEM_STRAP3 */ PAD_CFG_GPI(GPP_F22, NONE, PLTRST),
Copy that, let me do it
Oops, I mean minus the PCH_MEM_STRAP pins; they don't need to be included in the early_gpio_table.
https://review.coreboot.org/c/coreboot/+/33824/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33824/8//COMMIT_MSG@7 PS8, Line 7: mainboard/google/hatch: create akemi variant.
Please remove the dot/period at the end of the commit message summary.
Done
https://review.coreboot.org/c/coreboot/+/33824/8//COMMIT_MSG@17 PS8, Line 17: Signed-off-by: Peichao Wangpeichao.wang@bitland.corp-partner.google.com
Please add a space before the < to separate the name and the address.
Done
https://review.coreboot.org/c/coreboot/+/33824/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33824/10//COMMIT_MSG@11 PS10, Line 11: BUG=none
Please file an issue in Google issue tracker to track the status of creating new Akemi firmware incl […]
Done
https://review.coreboot.org/c/coreboot/+/33824/10//COMMIT_MSG@14 PS10, Line 14: /build/hatch/firmware/coreboot-private/3rdparty/blobs/baseboard/hatch
Yes, they should be in /build/hatch/firmware/
Done
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... PS6, Line 70: default "AKEMI TEST 4326" if BOARD_GOOGLE_AKEMI
Can you please put this in alphabetical order here and for other configs below?
Done
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... PS7, Line 39: config CHROMEOS_WIFI_SAR : bool "Enable SAR options for Chrome OS build" : depends on CHROMEOS : select DSAR_ENABLE : select GEO_SAR_ENABLE : select SAR_ENABLE : select USE_SAR : select WIFI_SAR_CBFS :
This already merged here: https://review.coreboot.org/c/coreboot/+/34580. […]
Done
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... PS6, Line 9: config BOARD_GOOGLE_HATCH_WHL : bool "-> Hatch_whl" : select BOARD_GOOGLE_BASEBOARD_HATCH : select BOARD_ROMSIZE_KB_32768 : select SOC_INTEL_WHISKEYLAKE
I believe you need to rebase on ToT
Done
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... PS6, Line 33: BOARD_GOOGLE_AKEMI
same here
Done
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... PS7, Line 9: : : : : :
This was already removed here: https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... File src/mainboard/google/hatch/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... PS10, Line 3:
redundant line
Done
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... PS10, Line 5:
nit: most use two spaces here
Done
https://review.coreboot.org/c/coreboot/+/33824/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/33824/4/src/mainboard/google/hatch/... PS4, Line 27: /* F11 : EMMC_CMD ==> EMMC_CMD */ : PAD_CFG_NF(GPP_F11, UP_20K, DEEP, NF1), : /* F12 : EMMC_DATA0 ==> EMMC_DAT0 */ : PAD_CFG_NF(GPP_F12, UP_20K, DEEP, NF1), : /* F13 : EMMC_DATA1 ==> EMMC_DAT1 */ : PAD_CFG_NF(GPP_F13, UP_20K, DEEP, NF1), : /* F14 : EMMC_DATA2 ==> EMMC_DAT2 */ : PAD_CFG_NF(GPP_F14, UP_20K, DEEP, NF1), : /* F15 : EMMC_DATA3 ==> EMMC_DAT3 */ : PAD_CFG_NF(GPP_F15, UP_20K, DEEP, NF1), : /* F16 : EMMC_DATA4 ==> EMMC_DAT4 */ : PAD_CFG_NF(GPP_F16, UP_20K, DEEP, NF1), : /* F17 : EMMC_DATA5 ==> EMMC_DAT5 */ : PAD_CFG_NF(GPP_F17, UP_20K, DEEP, NF1), : /* F18 : EMMC_DATA6 ==> EMMC_DAT6 */ : PAD_CFG_NF(GPP_F18, UP_20K, DEEP, NF1), : /* F19 : EMMC_DATA7 ==> EMMC_DAT7 */ : PAD_CFG_NF(GPP_F19, UP_20K, DEEP, NF1), : /* F20 : EMMC_RCLK ==> EMMC_RCLK */ : PAD_CFG_NF(GPP_F20, DN_20K, DEEP, NF1), : /* F21 : EMMC_CLK ==> EMMC_CLK */ : PAD_CFG_NF(GPP_F21, DN_20K, DEEP, NF1),
Need configure PAD as well as meet Intel PDG eMMC requirement. Chapter 7.6. […]
Done
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... PS7, Line 22: gpio_table
The latest Akemi schematic and layout file has been uploaded to share folder, the link as below. […]
Done
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... PS10, Line 33: /* A0 : NC */ : PAD_NC(GPP_A0, NONE), : /* A6 : NC */ : PAD_NC(GPP_A6, NONE), : /* A8 : NC */ : PAD_NC(GPP_A8, NONE), : /* A10 : NC */ : PAD_NC(GPP_A10, NONE), : /* A11 : NC */ : PAD_NC(GPP_A11, NONE), : /* A12 : NC */ : PAD_NC(GPP_A12, NONE), : /* A18 : NC */ : PAD_NC(GPP_A18, NONE), : /* A19 : NC */ : PAD_NC(GPP_A19, NONE), : /* A22 : NC */ : PAD_NC(GPP_A22, NONE), : /* A23 : NC */ : PAD_NC(GPP_A23, NONE), : /* B20 : NC */ : PAD_NC(GPP_B20, NONE), : /* B21 : NC */ : PAD_NC(GPP_B21, NONE), : /* B22 : NC */ : PAD_NC(GPP_B22, NONE), : /* C11 : NC */ : PAD_NC(GPP_C11, NONE), : /* F1 : NC */ : PAD_NC(GPP_F1, NONE),
configure GPIO according to real schematic.
Done
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... File src/mainboard/google/hatch/variants/akemi/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/33824/10/src/mainboard/google/hatch... PS10, Line 4: 2018
2019
Done
https://review.coreboot.org/c/coreboot/+/33824/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33824/5/src/mainboard/google/hatch/... PS5, Line 73: chip drivers/i2c/hid : register "generic.hid" = ""PNP0C50"" : register "generic.desc" = ""Synaptics Touchpad"" : register "generic.irq" = "ACPI_IRQ_EDGE_LOW(GPP_A21_IRQ)" : register "generic.wake" = "GPE0_DW0_21" : register "generic.probed" = "1" : register "hid_desc_reg_offset" = "0x20" : device i2c 0x2c on end : end
Add Synaptics touchpad configuration.
Done
https://review.coreboot.org/c/coreboot/+/33824/5/src/mainboard/google/hatch/... PS5, Line 95: device i2c 10 on end
slave address is 0x10 and follow our panel setting.
Done
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... PS6, Line 38: .rise_time_ns = 50, : .fall_time_ns = 15,
Can you please remove these if they are just being copied from hatch?
Done
https://review.coreboot.org/c/coreboot/+/33824/6/src/mainboard/google/hatch/... PS6, Line 69: register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_D21_IRQ)"
You need to set the probed property since you have multiple touchpad devices defined here.
Done
https://review.coreboot.org/c/coreboot/+/33824/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/overridetree.cb:
PS7:
Is this file a copy of hatch? If yes, the devices added to i2c and spi buses should probably be remo […]
Done
https://review.coreboot.org/c/coreboot/+/33824/17/src/mainboard/google/hatch... File src/mainboard/google/hatch/variants/akemi/overridetree.cb:
PS17:
I'd rather for the initial commit if you just put in a mostly empty devicetree; otherwise stuff may […]
Done