Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/21774 )
Change subject: mb/dell: Add Dell Optiplex 790 ......................................................................
Patch Set 59:
(21 comments)
I've cleaned up the unresolved comments on older patchsets, so that it's easier to keep track of actually unresolved stuff.
https://review.coreboot.org/c/coreboot/+/21774/47//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/21774/47//COMMIT_MSG@30 PS47, Line 30: - SuperI/O
What SuperIO chip does this board have? Internet tells me it's a SMSC SCH5544, could you please conf […]
Ack
https://review.coreboot.org/c/coreboot/+/21774/54//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/21774/54//COMMIT_MSG@11 PS54, Line 11: There are (at least) three different mainboards:
The idea is to support the three board models. AFAIK, Christoph has DT/MT and USFF, I have SFF.
Will tidy up the commit message
https://review.coreboot.org/c/coreboot/+/21774/54//COMMIT_MSG@21 PS54, Line 21: This port has been tested with: USFF
I recall checking, DT and MT use the same mainboard. They only differ in thickness.
Ack
https://review.coreboot.org/c/coreboot/+/21774/57//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/21774/57//COMMIT_MSG@12 PS57, Line 12: : - DT: 4 RAM slots, PEG, PCIe x1, PCI, PCIe x16 : - MT: 4 RAM slots, PEG, PCIe x1, PCI, PCIe x16
Both of these use the exact same PCB, but the MT version has an extra SATA connector installed on th […]
Unburied into current patch set
https://review.coreboot.org/c/coreboot/+/21774/59//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/21774/59//COMMIT_MSG@14 PS59, Line 14: - MT: 4 RAM slots, PEG, PCIe x1, PCI, PCIe x16
The PCBs are identical, but MT has an unpopulated SATA port. I shall reword this a bit. […]
I recovered a comment I had on an earlier patchset:
Both of these use the exact same PCB, but the MT version has an extra SATA connector installed on the mainboard. DT has the footprint for the connector, but it is not installed.
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Kconfig:
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 30: : config HAVE_IFD_BIN : bool : default n : : config HAVE_ME_BIN : bool : default n
No longer needed, please remove.
Done
https://review.coreboot.org/c/coreboot/+/21774/54/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Kconfig:
https://review.coreboot.org/c/coreboot/+/21774/54/src/mainboard/dell/optiple... PS54, Line 22: optiplex_790_
I'd just leave the form factor, e.g. […]
Unburied into current patch set
https://review.coreboot.org/c/coreboot/+/21774/54/src/mainboard/dell/optiple... PS54, Line 30: config DEVICETREE
There should be no need to override the variant mechanism for this. […]
Unburied into current patch set
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Kconfig:
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 22: optiplex_790_usff Rename the variants so that they are just the form factor. If we are in `src/mb/dell/optiplex_790/`, it is pretty clear that `usff` is referring to the Optiplex 790 USFF.
https://review.coreboot.org/c/coreboot/+/21774/54/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/21774/54/src/mainboard/dell/optiple... PS54, Line 3: config BOARD_DELL_OPTIPLEX_790_USFF
Please add an empty line between these
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Makefile.inc:
PS59: Fix this for C_ENV_BB (s/romstage.c/early_init.c and include in bootblock as well)
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/board_info.txt:
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 3: (With service mode jumper set)
I believe this is better fit in documentation.
meh
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 1: # : #
Double empty line
Ack
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 17: check gfx.ndid and gfx.did
If gfx.ndid = 3, gfx.did should have the same length.
Ack
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 45: docking_supported
A dock for a desktop?
Ack
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 51: 1
I don't think any port is hotpluggable on a desktop board.
Ack
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 39: #include <northbridge/intel/sandybridge/acpi/sandybridge.asl> : #include <drivers/intel/gma/acpi/default_brightness_levels.asl> : #include <southbridge/intel/bd82x6x/acpi/pch.asl>
Please correct indentation.
Done
https://review.coreboot.org/c/coreboot/+/21774/50/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/21774/50/src/mainboard/dell/optiple... PS50, Line 23: : -- For a three-pipe setup, bandwidth is shared between the 2nd and : -- the 3rd pipe. Thus, probe ports that likely have a high-resolution : -- display attached first.
The mainboard only has two video outputs (DP and VGA), this does not make any sense.
Done
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/mainboard.c:
https://review.coreboot.org/c/coreboot/+/21774/47/src/mainboard/dell/optiple... PS47, Line 25: 0
GMA_INT15_ACTIVE_LFP_NONE
Done
https://review.coreboot.org/c/coreboot/+/21774/50/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/mainboard.c:
https://review.coreboot.org/c/coreboot/+/21774/50/src/mainboard/dell/optiple... PS50, Line 14:
Double empty line
Unburied into current patchset
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/mainboard.c:
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 13: Double empty line. Decide if this file is worth keeping.