Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38303 )
Change subject: mainboard/prodrive/hermes: Add new mainboard port ......................................................................
Patch Set 43:
(17 comments)
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG@7 PS43, Line 7: mainboard
mb
Ack
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG@9 PS43, Line 9: mainboard
It can also function as desktop or workstation
Ack
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG@11 PS43, Line 11: FSP
Maybe mention which FSP? I guess CoffeeLakeFspBinPkg
Ack
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG@18 PS43, Line 18: * PCIe Link Width x8 on Slot6 by changing PCIe mux
Also i5 9600K and pentium gold g5400
Ack
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG@19 PS43, Line 19: DDR Dual Channel with 1x1, 2x2 : * DDR Single Channel with two DIMMs on one channel : * DDR Single Channel with one DIMM on one channel
Maybe group this together? […]
Ack
https://review.coreboot.org/c/coreboot/+/38303/43//COMMIT_MSG@35 PS43, Line 35: except the one under ETH1
It is working, was a hw issue I believe.
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 34: 2400
2500?
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 14: // Some generic macros
Please remove this comment, it was dropped from the tree
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 20: Scope (_SB) { : Device (PCI0)
To avoid the inconsistent brace placement, how about using a single scope? […]
If gives me: dsdt.asl 108: Scope (_SB.PCI0) { Error 6148 - ^ Illegal open scope on external object from within DSDT
dsdt.asl 108: Scope (_SB.PCI0) { Error 6117 - ^ Existing object has invalid type for Scope operator (_SB.PCI0 [Untyped])
dsdt.asl 423: Scope (_SB.PCI0) { Error 6148 - ^ Illegal open scope on external object from within DSDT
dsdt.asl 1252: Scope (_SB.PCI0) { Error 6148 - ^ Illegal open scope on external object from within DSDT
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 28: // Chipset specific sleep states
Same for this comment
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/eeprom.c:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 12: /* Check
Code style suggests that these comments start like this: […]
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 12: EEPROM
Which EEPROM is this code for?
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/memory.c:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 31: * Baseboard Rcomp target values.
nit: this coment fits in a single line like the rest
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 24: *
This asterisk should not be there, as per code style
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/variants/baseboard/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 13:
Double empty line
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... File src/mainboard/prodrive/hermes/variants/baseboard/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 62: Enumartion
Enumeration
Ack
https://review.coreboot.org/c/coreboot/+/38303/43/src/mainboard/prodrive/her... PS43, Line 64: //
Maybe use a colon? […]
Ack