Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40351 )
Change subject: mb/dell/optiplex_9010: Add Dell OptiPlex 9010 SFF support ......................................................................
Patch Set 3:
(31 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
Done
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... PS2, Line 37: Optional
Code seems to always want it
Added a Kconfig option to include the firmware on demand.
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?
No. Added to flashing guide
https://review.coreboot.org/c/coreboot/+/40351/2/Documentation/mainboard/del... PS2, Line 89: exploit
"use" would sound better
Done
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. […]
Done
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
Done
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
Made it optional with Kconfig setting
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?
No logic in ACPI is present for those GNVS fields. Removed.
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
Removed since unused.
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
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 16: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 25: 0x0
0
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 31: 0x0
0
Done
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! 😄
That's not me, autoport did it :)
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 53: subsystemid 0x1028 0x052c
This can be factored out
Done
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
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 104: PCI-LPC bridge
Drop this part
Done
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?
Done
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
Done
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
Done
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)
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 12: 0x0
Just 0
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 14: /* NID 0x12. */
You can remove these comments. […]
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 15: 0x0
Also just 0
Done
https://review.coreboot.org/c/coreboot/+/40351/2/src/mainboard/dell/optiplex... PS2, Line 45:
Same as above
Done
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?
Done
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
Done