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.