Thank you for doing this. It looks pretty good, there are just a few
comments I'd like to see addressed.
Patch set 2:Code-Review +1
12 comments:
Any information about how this was tested would be nice.
Patch Set #2, Line 9: X10SLM-F
X10SLM+-F
Patch Set #2, Line 10: X10SLM-LN4F
X10SLM+-LN4F
File src/mainboard/supermicro/x10slm/Kconfig:
Patch Set #2, 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.
Patch Set #2, 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.
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.
File src/mainboard/supermicro/x10slm/Kconfig.name:
Patch Set #2, Line 3: config BOARD_SUPERMICRO_X10SLM_PLUS_LN4F
Small nit: it would be nice to have a blank line between the two configs.
File src/mainboard/supermicro/x10slm/board_info.txt:
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
File src/mainboard/supermicro/x10slm/gpio.c:
Please put solely formatting changes in a separate patch.
File src/mainboard/supermicro/x10slm/mainboard.c:
Patch Set #2, Line 56: CHIP_NAME("X10SLM+-F").enable_dev = mainboard_enable,
Spurious.
File src/mainboard/supermicro/x10slm/variants/x10slm-ln4f/devicetree.cb:
I think using an overridetree, like the x220 and others do, would be optimal.
Patch Set #2, Line 38: device pci 01.1 on end # PEG 11
I think -LM4F only has PEG10.
To view, visit change 35163. To unsubscribe, or for help writing mail filters, visit settings.