Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40351 )
Change subject: mb/dell/optiplex_9010: Add board support ......................................................................
Patch Set 2:
(32 comments)
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... File Documentation/mainboard/dell/optiplex_9010.md:
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... PS2, Line 25: | Firmware | Dell original firmware or coreboot | Uh, this row seems unnecessary
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... PS2, Line 37: Optional Code seems to always want it
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... PS2, Line 87: There are no restrictions as to the programmer device. It is only recommended Are there diodes on the flash chips?
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... PS2, Line 89: exploit "use" would sound better
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/Kconfig:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 18: INTEL_GMA_HAVE_VBT The default name for the VBT is data.vbt, but yours is vbt.data
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 39: config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID I think these symbols were dropped
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 17: cbfs-files-y += sch5545_ecfw.bin This will always be needed
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 8: /* Enable USB ports in S3 by default */ : gnvs->s3u0 = 1; : gnvs->s3u1 = 1; Does this work?
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 12: // the lid is open by default. Comment style and capitalization
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/cmos.layout:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 28: #120 264 r 0 unused Please remove all commented-out "unused" lines
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 2: register "gfx.did" = "{ 0x80000100, 0x80000240, 0x80000410}" : register "gfx.ndid" = "3" : register "gfx.use_spread_spectrum_clock" = "0" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "4" : register "gpu_dp_d_hotplug" = "4" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "4" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000" Please drop all of this. It shouldn't be necessary.
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 16: 0x0 0
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 25: 0x0 0
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 26: end Please pick the "end" words up, and put them onto the previous line?
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 31: 0x0 0
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 32: 6 Cougar Liar! That's actually Series 7 Panther Point! 😄
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 38: register "docking_supported" = "0" Zero is already the default value, can drop this
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 43: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Zero is already the default value, can drop this
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 53: subsystemid 0x1028 0x052c This can be factored out
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 71: Audio Audio Stereo audio! I think we only need one "audio" though
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 104: PCI-LPC bridge Drop this part
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 146: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x1028 0x052c : end : device pci 01.0 on # PEG1 (blue slot1) : subsystemid 0x1028 0x052c : smbios_slot_desc "0xB6" "4" "SLOT1" "0x0D" : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x1028 0x052c : end : device pci 06.0 off # PEG2 : end Move these above the "chip southbridge" entry?
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 4: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB : #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0 These aren't needed
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 28: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> We don't need to include this anymore
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 1: -- SPDX-License-Identifier: GPL-2.0-only nit: most of the gma-mainboard.ads are GPL-2.0-or-later, could this one be as well for consistency?
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 10: 0x0000000b Just 11 (and please keep the comments aligned)
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 12: 0x0 Just 0
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 14: /* NID 0x12. */ You can remove these comments. They don't really say anything important
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 15: 0x0 Also just 0
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 45: Same as above
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/sch5545_ec.h:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 11: #define EC_GPIO_PP (0 << 0) Are my eyes bad, or is the alignment off?
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/sch5545_ec.c:
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 44: GPIIO GPIO