Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37441 )
Change subject: mb/supermicro/x11-lga1151v2-series: Add support for X11SCH-F ......................................................................
Patch Set 60: Code-Review+1
(8 comments)
https://review.coreboot.org/c/coreboot/+/37441/60/MAINTAINERS File MAINTAINERS:
https://review.coreboot.org/c/coreboot/+/37441/60/MAINTAINERS@384 PS60, Line 384: The maintainers entry should go over here, with the other supermicro boards
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 18: Scope (_SB) { : Device (PCI0) { Device (_SB.PCI0)
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 26: Drop this blank line?
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/memory.c:
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 27: .rcomp_resistor = {121, 81, 100}, Are you sure? These values are for CFL-U
I'd say these should be { 121, 75, 100 }
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 30: .rcomp_targets = {100, 40, 20, 20, 26}, Same here, these look like CFL-U values.
I'd say these should be { 60, 26, 20, 20, 26 }
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 17: /* This must be one, otherwise FSP crashes ... */ Drop this comment
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/variants/x11sch-f/gpio.c:
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 417: PAD_CFG_GPO(GPP_B0, 1, DEEP), What does this do?
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151v2-series/variants/x11sch-f/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37441/60/src/mainboard/supermicro/x... PS60, Line 108: # Internal GFX : register "InternalGfx" = "0" Hrm, but device 2 is enabled on the devicetree?