Angel Pons 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 49: Code-Review+1
(22 comments)
https://review.coreboot.org/c/coreboot/+/37441/49/Documentation/mainboard/su... File Documentation/mainboard/supermicro/x11-lga1151v2-series/x11-lga1151v2-series.md:
https://review.coreboot.org/c/coreboot/+/37441/49/Documentation/mainboard/su... PS49, Line 19: - Intel ME can be cleaned using me_cleaner (~4.5 MB more free space) Any consequences? Crippling the ME firmware usually breaks things.
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/Kconfig:
PS49: No license header
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 6: # select HAVE_ACPI_RESUME Hmm?
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 22: Supermicro_X11_LGA1151V2_SERIES This doesn't need underscores
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)
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?
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/acpi_tables.c:
PS49: I think this file is no longer required
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 28: garbeled garbled
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
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?
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
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 62: empty line
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
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 19: Scope (_SB) { : Device (PCI0) Device (_SB.PCI0)
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
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
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 44: const struct cnl_mb_cfg * variant_memcfg_config(void)
"foo * bar" should be "foo *bar"
Should be fixed
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, no spaces at the start of a line
Please fix the indentation
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 18: params->PchHdaVcType = 0x1; Make this common then?
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?
https://review.coreboot.org/c/coreboot/+/37441/49/src/mainboard/supermicro/x... PS49, Line 15: Do we need so many spaces?
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