Attention is currently required from: Paul Menzel, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56339 )
Change subject: asrock/e3c246d4i: Add Intel Coffee Lake board ......................................................................
Patch Set 9:
(15 comments)
File src/mainboard/asrock/e3c246d4i/Kconfig:
https://review.coreboot.org/c/coreboot/+/56339/comment/c5988c22_f5c43a7a PS9, Line 5: select BOARD_ROMSIZE_KB_32768 : select HAVE_ACPI_TABLES : select SUPERIO_ASPEED_AST2400 : select DRIVERS_ASPEED_AST_COMMON : select DRIVERS_ASPEED_AST2050 : select SOC_INTEL_CANNONLAKE_PCH_H : select SOC_INTEL_COFFEELAKE : select ONBOARD_VGA_IS_PRIMARY : select SOC_INTEL_COMMON_BLOCK_LPC_COMB_ENABLE Use alphabetical order?
https://review.coreboot.org/c/coreboot/+/56339/comment/6cf36100_aa248b84 PS9, Line 35: # 0 - 0x3f8: physical COM1 port : # 1 - 0x2f8: SOL console on BMC : config UART_FOR_CONSOLE : int : default 1 Any reason to default to 1? I'd usually expect the physical serial port to be the default.
https://review.coreboot.org/c/coreboot/+/56339/comment/9f5ed624_68bbc879 PS9, Line 15: config MAINBOARD_DIR : string : default "asrock/e3c246d4i" : : config MAINBOARD_PART_NUMBER : string : default "E3C246D4I" : : config CBFS_SIZE : hex : default 0x800000 : : config CONSOLE_POST : bool : default y : : config ONBOARD_VGA_IS_PRIMARY : bool : default y : : # 0 - 0x3f8: physical COM1 port : # 1 - 0x2f8: SOL console on BMC : config UART_FOR_CONSOLE : int : default 1 : : config MAX_CPUS : int : default 16 : : config DIMM_MAX : int : default 4 Please remove all the type declarations, they're redundant.
File src/mainboard/asrock/e3c246d4i/bootblock.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/b568c812_e643e999 PS9, Line 26: SuperIO chip off BMC I'm not sure if I follow. Does it refer to the SuperIO functionality of the BMC?
File src/mainboard/asrock/e3c246d4i/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56339/comment/1973a419_19b771b4 PS9, Line 3: .chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT, This was removed in CB:56967
https://review.coreboot.org/c/coreboot/+/56339/comment/99b26f00_992e954b PS9, Line 40: end nit: align the `end` keywords?
device pci 00.0 on end # Host Bridge device pci 01.0 on end # dGPU Port device pci 02.0 off end # Integrated Graphics Device device pci 04.0 off end # SA Thermal device
https://review.coreboot.org/c/coreboot/+/56339/comment/22e2847e_80671a79 PS9, Line 41: device pci 01.0 on end # dGPU Port Add a `smbios_slot_desc`?
https://review.coreboot.org/c/coreboot/+/56339/comment/fd94652a_3c682349 PS9, Line 148: register "gen1_dec" = "0x00fc0201" : register "gen2_dec" = "0x000C0291" : register "gen3_dec" = "0x000C0c1a" What's with the mixed casing in the hex numbers?
https://review.coreboot.org/c/coreboot/+/56339/comment/cc68c00a_ab24da20 PS9, Line 154: device pci 1f.1 off end # P2SB : device pci 1f.2 off end # Power Management Controller Needs to be changed as done in CB:57574
File src/mainboard/asrock/e3c246d4i/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/56339/comment/441c2cdc_f04b0b14 PS9, Line 21: #include <soc/intel/common/block/acpi/acpi/northbridge.asl> : #include <soc/intel/cannonlake/acpi/southbridge.asl> Missing one tab
File src/mainboard/asrock/e3c246d4i/gpio.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/30549464_cbde900d PS4, Line 16: /* DW0: PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | (1 << 1) - IGNORED */
See purism/librem_cnl
Basically, the only thing I'd keep is the first comment for each pad. Looks like it's just a matter of deleting all lines that contain `DW0` (or `/* DW0: ` to be safe):
/* GPP_A0 - RCIN# */ PAD_CFG_NF(GPP_A0, NONE, DEEP, NF1),
Another option:
PAD_CFG_NF(GPP_A0, NONE, DEEP, NF1), /* GPP_A0 - RCIN# */
File src/mainboard/asrock/e3c246d4i/gpio.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/42f768c3_b3d1e562 PS9, Line 505: /* ------- GPIO Group AZA ------- */ : : /* ------- GPIO Group VGPIO_0 ------- */ : : /* ------- GPIO Group VGPIO_1 ------- */ Drop these comments
https://review.coreboot.org/c/coreboot/+/56339/comment/06cdf4dd_17266111 PS9, Line 983: /* ------- GPIO Group SPI ------- */ Drop this comment
https://review.coreboot.org/c/coreboot/+/56339/comment/dd218538_6b15f6ad PS9, Line 987: /* ------- GPIO Group CPU ------- */ : : /* ------- GPIO Group JTAG ------- */ Drop these comments
File src/mainboard/asrock/e3c246d4i/romstage.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/699f6db7_5a6e0d3c PS9, Line 23: /* TODO: No idea, taken from coffeelake rvp */ : .rcomp_resistor = {121, 81, 100}, : /* TODO: No idea, taken from coffeelake rvp*/ : .rcomp_targets = {100, 40, 20, 20, 26}, These values are for ULT... Please use the following values instead:
.rcomp_resistor = {121, 75, 100}, .rcomp_targets = {60, 26, 20, 20, 26},
-
Angel Pons (Code Review)