Attention is currently required from: Alexander Couzens, Nicholas Chin, Nicholas Sudsgaard, Paul Menzel.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80343?usp=email )
Change subject: mainboard/lenovo: Add ThinkCentre M710s (Skylake) ......................................................................
Patch Set 5: Code-Review+1
(13 comments)
Patchset:
PS5: Beside a small nits and places where I wished for a comment, this looks very good :)
Feel free to ignore any comment that I already marked resolved and take the comments mentioning "schematics" with a grain of salt as we might have different versions of them.
File src/mainboard/lenovo/thinkcentre_m710s/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/80343/comment/58113de1_6070710f : PS5, Line 6: ramstage-y += early_init.c This is not needed in ramstage.
File src/mainboard/lenovo/thinkcentre_m710s/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80343/comment/e9071032_7c555607 : PS5, Line 5: # FSP Configuration This is a remnant from times where we configured a lot of FSP directly at the toplevel. The `eist_enable` in particular is a coreboot setting. So please drop the comment.
https://review.coreboot.org/c/coreboot/+/80343/comment/025ebb94_f26545e1 : PS5, Line 10: device ref system_agent on end This one is essential and hence already enabled in the `chipset.cb`. You can drop this at your convenience.
https://review.coreboot.org/c/coreboot/+/80343/comment/15f498c6_8f17db57 : PS5, Line 14: register "PcieRpClkSrcNumber[0]" = "0" Did you test the x16 slot? Please mention that one explicitly in the commit message.
The `PcieRp*` settings are actually for the PCH root ports. Though, it seems Intel forgot to add ClkReq settings for the PEG. If this works, it deserves a comment.
https://review.coreboot.org/c/coreboot/+/80343/comment/8441b25d_810ceae9 : PS5, Line 51: register "PcieRpClkReqSupport[4]" = "false" In the schematics it seems connected as CLKREQ_LAN#. If it's not working please add a comment that it's unknown why.
https://review.coreboot.org/c/coreboot/+/80343/comment/370b9beb_7d40ad9b : PS5, Line 54: end I see a PCIe-PCI bridge in the schematics on `pcie_rp6`. Is it not populated?
https://review.coreboot.org/c/coreboot/+/80343/comment/247dcf44_aed16f6c : PS5, Line 112: # Vendor values dumped using util/superiotool. Did you push a superiotool patch yet? I'm still a little concerned about the shift in LDNs we were seeing. But if the current port works, e.g. COM1 in the OS, then the numbers here must be correct.
File src/mainboard/lenovo/thinkcentre_m710s/early_init.c:
PS5: Please join this with `bootblock.c`. We usually have either one of them.
File src/mainboard/lenovo/thinkcentre_m710s/gpio.h:
https://review.coreboot.org/c/coreboot/+/80343/comment/698bf55c_ed7230f3 : PS5, Line 14: _PAD_CFG_STRUCT(GPP_A0, PAD_FUNC(NF1) | PAD_RESET(PLTRST) | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0), Technically the _-prefixed macros are reserved for internal use in soc/intel/common/. If there is no compatible high-level macro, that's something to investigate.
I don't mind merging it like this as there are other cases in the tree. But I'll skip the GPIOs in this review.
File src/mainboard/lenovo/thinkcentre_m710s/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/80343/comment/879e3dcc_05a06d4d : PS5, Line 20: AZALIA_PIN_CFG(0, 0x1e, 0x411111f0), We could decode these (there are plenty of definitions and macros in device/azalia*.h). Though, having this done by hand once, I know it's not fun. We should write a tool for the decoding, I guess. Or live with the encoded numbers (nobody seems to care anyway).
Anyway, this is good enough. Just wanted to have it mentioned that it's not all magic ;)
File src/mainboard/lenovo/thinkcentre_m710s/romstage.c:
https://review.coreboot.org/c/coreboot/+/80343/comment/727a7b72_2f9fbc0c : PS5, Line 11: 50 First should be 60, for the 4 DIMM configuration.
https://review.coreboot.org/c/coreboot/+/80343/comment/31217355_bbee994e : PS5, Line 32: No space after the casts, please.