Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34873 )
Change subject: mb/supermicro: Add Supermicro X9SCL+-F ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/34873/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34873/1//COMMIT_MSG@9 PS1, Line 9: coreboot gets stuck : in or before postCAR. Sounds like bad RAM config...
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl-f/Kconfig:
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 51: : config VGA_BIOS_FILE : string : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/vgabios.bin" I don't think the VBIOS can be committed to the blobs repo. I'd drop this path
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 59: 10sb Doesn't look like proper hex
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl-f/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 24: /* 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; I don't think there's code handling these values
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 32: // the lid is open by default. : gnvs->lids = 1; I don't think so
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl-f/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 4: : register "gfx.use_spread_spectrum_clock" = "0" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "0" : register "gpu_dp_c_hotplug" = "0" : register "gpu_dp_d_hotplug" = "0" : 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" = "0" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000" These values that are zero can be dropped
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 26: end Could move the lone "end" to the previous line (applies to all similar cases)
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 31: d Right after the "device domain 0x0 on" line, add this line:
subsystemid 0x15d9 0x0624 inherit
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 34: r Can be dropped
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 39: r Can be dropped
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 56: subsystemid 0x15d9 0x0624 Can drop these when inheriting the subsystemid
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 102: off Kind of curious that PEG is disabled. I think there's a shortage of PCIe lanes to cover all the PCIe slots.
The manual says that the top two PCIe x8 slots can be either 3.0 or 2.0, which means they are likely to be a split PEG x16 port. (usually, SNB can only do PCIe 2.0, and IVB can do PCIe 3.0)
Not sure if additional config is required for lane splitting, though. I believe that lspci on vendor will probably show two functions for this device. Could you please check?
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl-f/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... PS1, Line 16: : #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB I think these can be dropped
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl-f/mainboard.c:
PS1: If the iGPU is disabled, then this whole file becomes irrelevant.
https://review.coreboot.org/c/coreboot/+/34873/1/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl-f/romstage.c:
PS1: Not sure if you have to handle the other nuvoton chipie here, the WPCM450