Attention is currently required from: Alexander Couzens, Nicholas Chin, Nico Huber, Paul Menzel.
Nicholas Sudsgaard 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 6:
(9 comments)
File src/mainboard/lenovo/thinkcentre_m710s/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/80343/comment/c43080ef_8af0c00a : PS5, Line 6: ramstage-y += early_init.c
This is not needed in ramstage.
Done
File src/mainboard/lenovo/thinkcentre_m710s/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80343/comment/a9928e55_3294707b : PS5, Line 5: # FSP Configuration
This is a remnant from times where we configured a lot of FSP directly at […]
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/b42126bf_6b1f70e2 : PS5, Line 14: register "PcieRpClkSrcNumber[0]" = "0"
Did you test the x16 slot? Please mention that one explicitly in the […]
Since I don't have a x16 device, I tested it with a x1 device and it worked. I'm not sure about the `PcieRP*` settings I just saw something similar in `asrock/h110m`.
https://review.coreboot.org/c/coreboot/+/80343/comment/869eb793_55543ac8 : PS5, Line 51: register "PcieRpClkReqSupport[4]" = "false"
In the schematics it seems connected as CLKREQ_LAN#. If it's not working […]
If I remember correctly you suggested to disable it on the IRC when I was having issues with PCI errors when booting Ubuntu's LiveCD (doesn't seem to be an issue on "actual" systems).
Anyway, (I'm not sure how to test it but) it's not showing any issues in `dmesg` and seems to work fine with it enabled.
https://review.coreboot.org/c/coreboot/+/80343/comment/a7cb81e3_ae4b0054 : PS5, Line 54: end
I see a PCIe-PCI bridge in the schematics on `pcie_rp6`. […]
There's a spot allocated for that on the board but nothing is mounted. You can see that on the image of the board in the documentation: https://review.coreboot.org/c/coreboot/+/80411/4/Documentation/mainboard/len....
I added it to make it easier for someone to port to another IB250MH board in the future.
File src/mainboard/lenovo/thinkcentre_m710s/early_init.c:
PS5:
Please join this with `bootblock.c`. We usually have either one of them.
Done
File src/mainboard/lenovo/thinkcentre_m710s/gpio.h:
https://review.coreboot.org/c/coreboot/+/80343/comment/94439c46_70799efe : 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 […]
I updated it to the values `inteltool -iiii` generates. I also added the names of the GPIOs from the schematics.
File src/mainboard/lenovo/thinkcentre_m710s/romstage.c:
https://review.coreboot.org/c/coreboot/+/80343/comment/ba8b7cc3_8f10fad9 : PS5, Line 11: 50
First should be 60, for the 4 DIMM configuration.
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/b84fe6ab_2d717f5b : PS5, Line 32:
No space after the casts, please.
Done