Christoph Pomaska has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/21774 )
Change subject: mb/dell: Add Dell Optiplex 790 ......................................................................
Patch Set 61:
(14 comments)
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. […]
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 34: config VGA_BIOS_FILE : string : default "pci8086,0102.rom" : : config VGA_BIOS_ID : string : default "8086,0102"
it's pointless, the board has a socket.
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 42: config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID : hex : default 0x4ad : : config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID : hex : default 0x1028
delet
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. […]
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 22: /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0;
Drop
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 16: #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
Is unused
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 23: // DSDT revision: ACPI 2.0 and up
Use C comments for consistency.
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 36: #include <southbridge/intel/bd82x6x/acpi/sleepstates.asl>
Some of these headers need to be corrected!
acpi/sleepstates.asl is absent in kontron/ktqm77/dsdt.asl, so I'd simply remove it.
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.
The variables initialised here only seem to affect laptops.
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 17: register "gfx.did" = "{ 0x80000100, 0x80000240, 0x80000410 }" : register "gfx.link_frequency_270_mhz" = "0" : register "gfx.ndid" = "3"
take out the trash
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 23: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 50: # FIXME: What can be hotplugged? : register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" : register "pcie_port_coalesce" = "0"
Remove
What does removing these entries do? It looks like it's not correct anyway since the PCIe X1 port is confirmed hotplug-able.
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 62: on #
align
Done
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 78: d
MT/DT probably want this on
Done
-
Christoph Pomaska (Code Review)