Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35163 ) Change subject: mb/supermicro: Add X10SLM+-LN4F as X10SLM variant ...................................................................... Patch Set 2: Code-Review+1 (12 comments) Thank you for doing this. It looks pretty good, there are just a few comments I'd like to see addressed. https://review.coreboot.org/c/coreboot/+/35163/2//COMMIT_MSG Commit Message: PS2: Any information about how this was tested would be nice. https://review.coreboot.org/c/coreboot/+/35163/2//COMMIT_MSG@9 PS2, Line 9: X10SLM-F X10SLM+-F https://review.coreboot.org/c/coreboot/+/35163/2//COMMIT_MSG@10 PS2, Line 10: X10SLM-LN4F X10SLM+-LN4F https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm/Kconfig: https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 20: config BOARD_SPECIFIC_OPTIONS This options here are sorted alphabetically, and it would be nice to keep it that way, so long as it doesn't interfere with any other more informative orderings. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 34: select SUPERIO_ASPEED_AST2400 # The board's BMC As far as I can tell, all this does is link in `superio/aspeed/common/ early_serial.c`. Yet the AST2400 isn't used for serial on the X10SLM+-F, the NCT6776 is, and I would expect the same for the the -LN4F. As per my comments on CB:34905, I don't think the AST2400 is wired up to act as a super I/O at all on these boards, so selecting it doesn't seem to make sense for future effects of the symbol either. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 35: select MAINBOARD_HAS_LPC_TPM : select MAINBOARD_HAS_TPM2 Since this TPM config affects the code flow, and it's untested at least on the X10SLM+-F, it should only be added in a separate patch. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm/Kconfig.name: https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 3: config BOARD_SUPERMICRO_X10SLM_PLUS_LN4F Small nit: it would be nice to have a blank line between the two configs. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm/board_info.txt: https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 2: Board URLs: : X10SLM-F: https://www.supermicro.com/products/motherboard/xeon/c220/x10slm_-f.cfm : X10SLM-LN4F: https://www.supermicro.com/en/products/motherboard/X10SLM+-LN4F As far as I know, only a single URL can be supported here if we want the hyperlinks on the board status HTML page [1] to keep working. [1]: https://coreboot.org/status/board-status.html https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm/gpio.c: PS2: Please put solely formatting changes in a separate patch. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm/mainboard.c: https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 56: CHIP_NAME("X10SLM+-F").enable_dev = mainboard_enable, Spurious. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm/variants/x10slm-ln4f/devicetree.cb: PS2: I think using an overridetree, like the x220 and others do, would be optimal. https://review.coreboot.org/c/coreboot/+/35163/2/src/mainboard/supermicro/x1... PS2, Line 38: device pci 01.1 on end # PEG 11 I think -LM4F only has PEG10. -- To view, visit https://review.coreboot.org/c/coreboot/+/35163 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I686d8d4e2ec5b4eb2db214b6e0827ac9c33829d1 Gerrit-Change-Number: 35163 Gerrit-PatchSet: 2 Gerrit-Owner: Christoph Pomaska <github@aufmachen.jetzt> Gerrit-Reviewer: Christoph Pomaska <github@aufmachen.jetzt> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Tristan Corrick <tristan@corrick.kiwi> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Sun, 01 Sep 2019 18:45:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment