Angel Pons 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 41:
(13 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 7: *some* Why only some? Why the stressing? Is there a programmer that does not work?
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 18: # Does s3resume work?
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 25: detection Only detection? What about the rest, like, ECC getting enabled? It either works, it doesn't, or your CPU can't make use of ECC memory, so untested.
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?) It's acpi, but don't do a copypasta from a copypasta please.
If vendor firmware makes LNXTHERM appear, then look at the vendor DSDT and check if there's anything that exists in coreboot already
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?) Coalescing, very likely. But who knows what this means if the affected PCI device is not mentioned?
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 And why is this an issue, exactly?
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 46: ## ToDo : : - Fix issues above : - Fix TODOs mentioned in code That's rather obvious, I'd say.
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 50: - Validate slp registers : - Validate usb settings in devicetree Validate how?
https://review.coreboot.org/c/coreboot/+/35427/41/Documentation/mainboard/su... PS41, Line 78: : +------------------+--------------------------------------------------+ : | Other slots | 1x RS232 (ext) | : | | 1x RS232 header | : | | 1x TPM header | : | | 1x Power SMB header | : | | 5x PWM Fan connector | : | | 2x I-SGPIO | : | | 2x S-ATA DOM Power connector | : | | 1x XDP Port | : | | 1x External BMC I2C Header (for IPMI card) | : | | 1x Chassis Intrusion Header | : +------------------+--------------------------------------------------+ If you link the vendor's device page, I don't think you need to copypasta that information here.
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?
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
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?
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.
I think FSP has UPDs for GPIO configs, but who knows if they're even being used?