Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35427 )
Change subject: mb/supermicro/x11-lga1151-series: add x11ssm-f board ......................................................................
Patch Set 42:
(8 comments)
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... File Documentation/mainboard/supermicro/x11-lga1151-series/x11ssm-f/x11ssm-tf.md:
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 38: (acpi, src/mainboard/gigabyte/ga-h61m-s2pv/acpi/thermal.asl maybe?)
This comment does not say anything about copypasta^^ This is just an example how this was done there […]
Done
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 39: PCI function 4 swapped to 0 (huh?) : - PCI function 2 swapped to 0 (huh?)
right, removed
Done
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 41: - register differences to vendor firmare: : - BIOS_PCI_EXP_EN 1->0 : - PCI_EXP_EN 1->0 : - GPIO_TIER2_SCI_EN 1->0
because I didn't check what these are until now
Done
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 46: ## ToDo : : - Fix issues above : - Fix TODOs mentioned in code
Ack
Done
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/include/mainboard.h:
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... PS41, Line 17: struct device;
What's this?
o.O I have no clue... removed
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... PS41, Line 22: void variant_mainboard_init(struct device *dev)
should be on the previous line
Done
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... PS41, Line 27: : /* do common init */ : // placeholder for common mainboard initialization
Why these comments?
IMHO a little bit of documentation is does not hurt, does it?
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35427/41/src/mainboard/supermicro/x... PS41, Line 24: Find out why the polarities from gpio.h gets overwritten by FSP.
Somewhere, it is stated that FSP overrides GPIO settings, and then coreboot has to restore them. […]
yeah, I don't even know if it's required to set them here :S