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:
(20 comments)
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
*three* is before the list, and the list has four items. […]
The PCBs are identical, but MT has an unpopulated SATA port. I shall reword this a bit.
This was a note for Sellerie and I to know what boards there are. Now that we know for sure, I think this would also go well into the documentation
https://review.coreboot.org/c/coreboot/+/21774/59//COMMIT_MSG@42 PS59, Line 42: - VBT is missing
Are VBT’s needed for desktop systems?
They are needed. What needs to be checked is if the three variants use the same VBT
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 14: select MAINBOARD_USES_IFD_GBE_REGION
Don’t you need […]
Then it won't build, because support for that SIO isn't merged in? It also needs code to initialize the SIO, which isn't there either...
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.
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
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Kconfig.name:
PS59: nit: split the three form factors out, to have prettier mainboard part numbers
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 4: * Copyright (C) 2012 The Chromium OS Authors. All rights reserved.
Can be removed. It’s not present in current autoport output.
You are assuming this file was generated with autoport. It is actually taken from another board: asus/p8h61-m_pro has the exact same file.
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 18: */
This is not added in the latest autoport version.
See above.
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
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
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.
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!
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:
PS59: use overridetrees
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
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 23: 0x0 0
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
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... PS59, Line 62: on # align
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/hda_verb.c:
PS59: Fix this mess, as done with other boards.
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_usff/devicetree.cb:
PS59: See the other devicetree
https://review.coreboot.org/c/coreboot/+/21774/59/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_usff/hda_verb.c:
PS59: See the other file. Also, compare for differences.
-
Angel Pons (Code Review)