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 60:
(22 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
I recovered a comment I had on an earlier patchset: […]
See latest patchset
https://review.coreboot.org/c/coreboot/+/21774/60//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/21774/60//COMMIT_MSG@11 PS60, Line 11: There are (at least) three different mainboards: The Optiplex 790 comes in four different sizes: MT, DT, SFF and USFF. The mainboard PCBs for MT and DT are identical, the only difference is that MT has an extra SATA port.
https://review.coreboot.org/c/coreboot/+/21774/60//COMMIT_MSG@13 PS60, Line 13: - DT: 4 RAM slots, PEG, PCIe x1, PCI, PCIe x16 Add the SATA port info here:
MT: 4 SATA DT: 3 SATA SFF: 3 SATA USFF: you tell me
https://review.coreboot.org/c/coreboot/+/21774/60//COMMIT_MSG@18 PS60, Line 18: The variants have different PCI/PCIe configurations and are not The three mainboard types have different PCI/PCIe configurations, so they need to be different variants.
https://review.coreboot.org/c/coreboot/+/21774/60//COMMIT_MSG@21 PS60, Line 21: This port has been tested with: SFF, USFF need to test DT/MT, I think? I should retest SFF as well
https://review.coreboot.org/c/coreboot/+/21774/60/Documentation/mainboard/de... File Documentation/mainboard/dell/optiplex790.md:
https://review.coreboot.org/c/coreboot/+/21774/60/Documentation/mainboard/de... PS60, Line 44: adapter "adapter"? programmer, maybe?
https://review.coreboot.org/c/coreboot/+/21774/60/Documentation/mainboard/de... PS60, Line 45: Though Note that
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 1: romstage-y += variants/$(VARIANT_DIR)/gpio.c Missing some things, for example:
bootblock-y += variants/$(VARIANT_DIR)/gpio.c
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 31: 0x0 0
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 37: 0x0 0
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 24: 0x0000000b should be decimal
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 25: /* NID 0x01: Subsystem ID. */ All the "NID" comments are of no use
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 26: 0x0 decimal too
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 57: 0x10ec0887, /* Codec Vendor / Device ID: Realtek */ Pack the AZALIA_PIN_CFG macros together, and leave some space between codecs
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 60: 0x0000000f decimal
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 62: 0x2 decimal
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 108: 0x00000004 decimal
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 110: 0x3 decimal
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... File src/mainboard/dell/optiplex_790/variants/optiplex_790_usff/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 14: : register "gfx.did" = "{ 0x80000100, 0x80000240, 0x80000410 }" : register "gfx.link_frequency_270_mhz" = "0" : register "gfx.ndid" = "3" This can be dropped
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 21: 0x0 0
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 29: 0x0 0
https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optiple... PS60, Line 35: 0x0 0