Attention is currently required from: Paul Menzel, Angel Pons. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56339 )
Change subject: asrock/e3c246d4i: Add Intel Coffee Lake board ......................................................................
Patch Set 6:
(17 comments)
File src/mainboard/asrock/e3c246d4i/Kconfig:
https://review.coreboot.org/c/coreboot/+/56339/comment/ee5491a9_2b820ecc PS4, Line 22: e3c246d4i
Shouldn't this be uppercase? E3C246D4I
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/2bf24c51_a7f87bbf PS4, Line 36: Hardware COM1 port
Everything is hardware. […]
Done
File src/mainboard/asrock/e3c246d4i/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/56339/comment/141d11f9_3b6355ba PS4, Line 2: e3c246d4i
Same, E3C246D4I
Done
File src/mainboard/asrock/e3c246d4i/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56339/comment/d66b73bf_3fa82d75 PS4, Line 1: CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/include
Copy-pasta?
Done
File src/mainboard/asrock/e3c246d4i/bootblock.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/8714da19_3d8b387e PS4, Line 19: 4
3?
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/6e010b60_9b090721 PS4, Line 22: return AST2400_SUART1;
This path should never be reached, as it means something is very wrong with the Kconfig setting. […]
Done
File src/mainboard/asrock/e3c246d4i/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56339/comment/fd97b2cb_3ff773c7 PS2, Line 47: device pci 12.0 on end # Thermal Subsystem
I always align the comments with tabs.
Done
File src/mainboard/asrock/e3c246d4i/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56339/comment/eb1627c0_d0b3c592 PS4, Line 7: # Power limit
Dead comment
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/b7903bdd_4158ba61 PS4, Line 14: # Serial IRQ Continuous
Redundant comment, drop?
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/b8507e5d_2095c2b2 PS4, Line 93: device pci 17.0 on # SATA
Enable SATA power optimizer too? […]
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/80ab6a1a_3d257676 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? […]
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/99753030_945c351b 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.
That's what the vendor does. It looks like FSP automatically disables it and does coalescing regardless of it being on here...
File src/mainboard/asrock/e3c246d4i/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/56339/comment/44aa6706_f6b844de PS4, Line 10: // OEM revision
Is it, though?
Done
File src/mainboard/asrock/e3c246d4i/gpio.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/64cea08e_b8b717d5 PS4, Line 16: /* DW0: PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | (1 << 1) - IGNORED */
Comments should be trimmed
'/* Pad configuration was generated automatically using intelp2m utility */'. What should be trimmed from the comments?
File src/mainboard/asrock/e3c246d4i/romstage.c:
https://review.coreboot.org/c/coreboot/+/56339/comment/92dcf0ef_14616444 PS4, Line 7: esthetic
*a*esthetic […]
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/3394d2c5_b31eca6b PS4, Line 19: 0xa8
Is this really 0xa8? I'd expect it to be 0xa6 (or even better, `0x53 << 1`)
Done
https://review.coreboot.org/c/coreboot/+/56339/comment/5b989b8a_1f7fea7a PS4, Line 26: 50
Out of curiosity, where does this 50 come from?
system76/oryp5. I have no idea what it should be... Any suggestions?
-
Arthur Heymans (Code Review)