Change in coreboot[master]: src/mainboard/supermicro/x11-lga1151v2-series: Add Support for X11SCH-F
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37441 )
Change subject: src/mainboard/supermicro/x11-lga1151v2-series: Add Support for X11SCH-F ......................................................................
Patch Set 51:
(19 comments)
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/Kconfig:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 6: # select HAVE_ACPI_RESUME
Hmm?
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 22: Supermicro_X11_LGA1151V2_SERIES
This doesn't need underscores
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 39: : config MAINBOARD_VENDOR : string : default "Supermicro"
Not needed (if it is, then check src/mainboard/supermicro/Kconfig)
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 56: config CONSOLE_POST : bool : default n
Isn't this default already?
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 18: const struct pad_config * early_gpio_table = get_early_gpio_table(&num);
"foo * bar" should be "foo *bar"
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 28: garbeled
garbled
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/devicetree.cb:
PS49:
No license header
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 10: device pci 02.0 on end # Integrated Graphics Device
Is it really on?
Yup.
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 11: device pci 04.0 on end # SA Thermal device : device pci 04.0 on end # Intel Xeon E3
Um, this is duplicated
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 62:
empty line
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 13: // Some generic macros
Please drop this comment
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 19: Scope (_SB) { : Device (PCI0)
Device (_SB. […]
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 27: // Chipset specific sleep states
Not chipset specific, this should also be dropped
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/memory.c:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 29: /* : * Baseboard Rcomp target values. : */
This fits in a single line
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 44: const struct cnl_mb_cfg * variant_memcfg_config(void)
Should be fixed
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 10: size_t num = 0;
Please fix the indentation
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/variants/x11sch-f/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 3: PCH_GPIO_H
Um, not PCH?
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 15:
Do we need so many spaces?
Done
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/variants/x11sch-f/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 68: register "usb2_ports[0]" = "USB2_PORT_MID(OC0)"
I think we can drop one tab here
Done
-
Christian Walter (Code Review)