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 4:
(18 comments)
Patchset:
PS4: https://i.imgur.com/HZS8mwc.png
File src/mainboard/asrock/e3c246d4i/Kconfig:
https://review.coreboot.org/c/coreboot/+/56339/comment/36b7bec4_3e695800 PS4, Line 22: e3c246d4i Shouldn't this be uppercase? E3C246D4I
https://review.coreboot.org/c/coreboot/+/56339/comment/3ce6b337_1bb745bf PS4, Line 36: Hardware COM1 port Everything is hardware. Do you mean the *physical* RS232 connector labelled COM1?
File src/mainboard/asrock/e3c246d4i/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/56339/comment/a5386d73_54fa06b0 PS4, Line 2: e3c246d4i Same, E3C246D4I
File src/mainboard/asrock/e3c246d4i/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56339/comment/ed73ded0_d5fcb26e PS4, Line 1: CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/include Copy-pasta?
File src/mainboard/asrock/e3c246d4i/bootblock.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/cab63d9e_fc0f6ee1 PS4, Line 19: 4 3?
https://review.coreboot.org/c/coreboot/+/56339/comment/bfa5524f_be0107c8 PS4, Line 22: return AST2400_SUART1; This path should never be reached, as it means something is very wrong with the Kconfig setting. How about:
return dead_code_t(uint8_t);
If this always causes build errors (even for valid values), try dropping the `com` parameter and using `CONFIG_UART_FOR_CONSOLE` directly in the switch statement.
File src/mainboard/asrock/e3c246d4i/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56339/comment/5a754b5d_1eff1f5f PS2, Line 47: device pci 12.0 on end # Thermal Subsystem
Should the comments be aligned?
I always align the comments with tabs.
File src/mainboard/asrock/e3c246d4i/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56339/comment/1dd109f5_01f06be7 PS4, Line 7: # Power limit Dead comment
https://review.coreboot.org/c/coreboot/+/56339/comment/ac7bf8af_0bc24b1b PS4, Line 14: # Serial IRQ Continuous Redundant comment, drop?
https://review.coreboot.org/c/coreboot/+/56339/comment/b5ba461b_bc9301a6 PS4, Line 93: device pci 17.0 on # SATA Enable SATA power optimizer too?
register "satapwroptimize" = "1"
https://review.coreboot.org/c/coreboot/+/56339/comment/488ffd28_775dff75 PS4, Line 94: register "SataPortsEnable[0]" = "1" : register "SataPortsEnable[1]" = "1" : register "SataPortsEnable[2]" = "1" : register "SataPortsEnable[3]" = "1" : register "SataPortsEnable[4]" = "1" : register "SataPortsEnable[5]" = "1" : register "SataPortsEnable[6]" = "1" : register "SataPortsEnable[7]" = "1" Use an array initialiser for brevity's sake?
register "SataPortsEnable" = "{ [0] = 1, [1] = 1, [2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, [7] = 1, }"
https://review.coreboot.org/c/coreboot/+/56339/comment/5b82fcb1_3da403d3 PS4, Line 107: device pci 1b.0 on # PCI Express Port 17 What is this used for? Avoiding coalescing?
There's two OcuLink connectors and a M.2 slot.
File src/mainboard/asrock/e3c246d4i/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/56339/comment/d3e2900b_4ef8812e PS4, Line 10: // OEM revision Is it, though?
File src/mainboard/asrock/e3c246d4i/gpio.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/984449d3_66011ece PS4, Line 16: /* DW0: PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | (1 << 1) - IGNORED */ Comments should be trimmed
File src/mainboard/asrock/e3c246d4i/romstage.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/776efcd3_b16e32af PS4, Line 7: esthetic *a*esthetic
Personally, I'd rather put this information in a documentation page.
https://review.coreboot.org/c/coreboot/+/56339/comment/a33a69e0_3a9e5b04 PS4, Line 19: 0xa8 Is this really 0xa8? I'd expect it to be 0xa6 (or even better, `0x53 << 1`)
https://review.coreboot.org/c/coreboot/+/56339/comment/74f2a540_ac015fc6 PS4, Line 26: 50 Out of curiosity, where does this 50 come from?
-
Angel Pons (Code Review)