Attention is currently required from: Alexander Couzens, Felix Singer, 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 9:
(14 comments)
File src/mainboard/lenovo/thinkcentre_m710s/Kconfig:
https://review.coreboot.org/c/coreboot/+/80343/comment/e689998f_61563745 : PS8, Line 7: select BOARD_ROMSIZE_KB_8192 : select SOC_INTEL_KABYLAKE : select SKYLAKE_SOC_PCH_H : select HAVE_ACPI_TABLES : select HAVE_ACPI_RESUME : select INTEL_GMA_HAVE_VBT : select MAINBOARD_HAS_LIBGFXINIT : select SOC_INTEL_COMMON_BLOCK_HDA_VERB : select SUPERIO_ITE_IT8629E : select MAINBOARD_HAS_TPM2 : s
Sort alphabetically: […]
Done
File src/mainboard/lenovo/thinkcentre_m710s/bootblock.c:
https://review.coreboot.org/c/coreboot/+/80343/comment/bb7c9efb_408abc04 : PS8, Line 21: gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table));
FSP might reconfigure some GPIOs. […]
Done
File src/mainboard/lenovo/thinkcentre_m710s/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80343/comment/099947ca_5583cb94 : PS8, Line 2: # Model: IB250MH
What does that mean?
My understanding is that mainboards are reused and I wanted to make it easier for anyone in the future to locate the same mainboard model by a simple grep.
Maybe `board_info.txt` would be a more appropriate location?
https://review.coreboot.org/c/coreboot/+/80343/comment/27d5c7ac_8c4275f6 : PS8, Line 19: register "usb2_ports[0]" = "USB2_PORT_MID(OC1)" # USB30A : register "usb2_ports[1]" = "USB2_PORT_MID(OC1)" # USB30B : register "usb2_ports[2]" = "USB2_PORT_MID(OC2)" # F_USB30_1A : register "usb2_ports[3]" = "USB2_PORT_MID(OC2)" # F_USB30_1B : register "usb2_ports[4]" = "USB2_PORT_MID(OC3)" # F_USB30_2A : register "usb2_ports[5]" = "USB2_PORT_MID(OC3)" # F_USB30_2B : register "usb2_ports[6]" = "USB2_PORT_MID(OC_SKIP)" # M.2 Bluetooth : register "usb2_ports[7]" = "USB2_PORT_MID(OC5)" # USB_LANA : register "usb2_ports[8]" = "USB2_PORT_MID(OC5)" # USB_LANB : register "usb2_ports[9]" = "USB2_PORT_MID(OC_SKIP)" # F_USB1 (Pins 5, 7) : register "usb2_ports[10]" = "USB2_PORT_MID(OC_SKIP)" # F_USB1 (Pins 6, 8) : # Used by the SD/MMC reader. :
Please use: […]
That is what I originally tried to do. However, I couldn't add comments like that. I found a "workaround" though. 👍
https://review.coreboot.org/c/coreboot/+/80343/comment/432bd76e_491e2fb3 : PS8, Line 33: register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC1)" # USB30A
Same here
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/8b38d462_aea9fff9 : PS8, Line 45: register "SataPortsEnable[0]" = "true" # SATA1
Same here
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/b7f8cc60_17209880 : PS8, Line 48: register "SataPortsEnable[3]" = "false" # SATA4 (not mounted)
Did you test if it works? If not, please remove.
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/c30b3969_5cc91122 : PS8, Line 58: device ref pcie_rp6 off end # PCI (not mounted)
Did you test if it works? If not, please remove.
Done
File src/mainboard/lenovo/thinkcentre_m710s/gpio.h:
https://review.coreboot.org/c/coreboot/+/80343/comment/fb9d7032_1c38aaee : PS8, Line 27: PAD_CFG_NF(GPP_A14, NONE, DEEP, NF1),
Nit: Missing description
Everything with a missing description is not connected to anything on the schematics (these were generated by `intelp2m`). I changed all of these to `PAD_NC(*, NONE)`, I assume this is safe.
https://review.coreboot.org/c/coreboot/+/80343/comment/9bc01312_b317845f : PS8, Line 50: PAD_CFG_GPO(GPP_B11, 0, DEEP),
Nit: Missing description
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/27103e45_0dce4a56 : PS8, Line 88: PAD_NC(GPP_C21, NONE), // UART2_TXD
Does it have pads or headers for UART? Is RXD accessible as well?
UART2_RXD is not connected to anything on the schematics.
https://pasteboard.co/6UQbHUs9w8KS.png
https://review.coreboot.org/c/coreboot/+/80343/comment/7d6189bf_bd53e594 : PS8, Line 189: PAD_CFG_NF(GPP_H3, NONE, PLTRST, NF1),
Nit: Missing description.
Done
https://review.coreboot.org/c/coreboot/+/80343/comment/1213e9b9_f1e9fa51 : PS8, Line 208: PAD_CFG_GPI_TRIG_OWN(GPP_H22, NONE, PLTRST, OFF, ACPI),
Nit: Missing description
Done
File src/mainboard/lenovo/thinkcentre_m710s/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/80343/comment/8fb9cb8c_d7eaa821 : PS8, Line 20: AZALIA_PIN_CFG(0, 0x1e, 0x411111f0),
Sorry if my question here is a bit misleading. […]
There was Intel HDMI/DP here (https://review.coreboot.org/c/coreboot/+/80343/4..5/src/mainboard/lenovo/thi...) but I got rid of it and used `SOC_INTEL_COMMON_BLOCK_HDA_VERB` instead. Looking at the pins now, they all seem to be `0x58560010`. I feel like I may have lost multichannel support should I revert this?