Attention is currently required from: Alexander Couzens, Maciej Pijanowski, Michał Kopeć, Nicholas Sudsgaard, Paul Menzel.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80609?usp=email )
Change subject: mb/lenovo: Add ThinkCentre M920q (Cannon Lake) ......................................................................
Patch Set 4:
(22 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80609/comment/7e2c5df9_3857c5ba : PS3, Line 7: mb/lenovo/m920q: add board
Maybe also put the PCH/socket in it too: […]
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/97c9ec44_63d17c3a : PS3, Line 9: Add Lenovo Thinkcentre Tiny mainboard.
This seems redundant.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/11cc5d2e_e1e63ab7 : PS3, Line 30: PCIex8
Maybe: PCIe x8
Done
File src/mainboard/lenovo/m920q/Kconfig:
https://review.coreboot.org/c/coreboot/+/80609/comment/3eda47ec_869d962a : PS3, Line 1: if BOARD_LENOVO_M920Q
Add SPDX license header.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/21095cab_9917b05d : PS3, Line 4: def_bool y
It would be better sorted. […]
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/c7c242c2_e8b35cda : PS3, Line 5: select SOC_INTEL_CANNONLAKE_PCH_H
You probably also need `SOC_INTEL_COMMON_BLOCK_HDA_VERB` here so that HDA verbs are applied.
Done
File src/mainboard/lenovo/m920q/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/80609/comment/67592bc4_6317c844 : PS3, Line 1: config BOARD_LENOVO_M920Q
Add SPDX license header.
Done
File src/mainboard/lenovo/m920q/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/80609/comment/081274a6_1b107ecf : PS3, Line 3: subdirs-y += spd
This seems unnecessary.
Done
File src/mainboard/lenovo/m920q/devicetree.cb:
PS3:
Remove options which are disabled
Done
PS3:
Move the options into the scope of the related controllers
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/d849cf1c_39a8f5d8 : PS3, Line 13: # FSP Configuration
This comment seems redundant.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/9e0a73cd_11f2c491 : PS3, Line 14: register "usb2_ports[0]" = "USB2_PORT_MID(OC0)" # Internal USB header
I would suggest this format for bulk definitions. […]
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/2fafc39d_32b8757f : PS3, Line 23: register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC1)" # Front port (charger)
Here too.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/ea4b3f1e_f31c447b : PS3, Line 32: register "SataPortsEnable[0]" = "1"
Here too.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/d2d1614b_d060c050 : PS3, Line 42: register "PcieRpEnable[0]" = "0"
Here too.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/c357e034_e8b117e7 : PS3, Line 67: register "PcieClkSrcUsage[0]" = "0x40" # PCIEx8 slot
Here too.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/f937c713_9a81fb58 : PS3, Line 84: register "PcieClkSrcClkReq[0]" = "0"
Here too.
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/5f9bd063_a95ae230 : PS3, Line 106: device domain 0 on
I would suggest using `ref x` instead of `pci xx.x`. […]
Done
File src/mainboard/lenovo/m920q/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/80609/comment/940e1fbf_43f1f3e3 : PS3, Line 16: // global NVS and variables
Remove superfluous comment
Done
https://review.coreboot.org/c/coreboot/+/80609/comment/1eaab739_031fc0d2 : PS3, Line 19: Scope (_SB) { : Device (PCI0)
Done
File src/mainboard/lenovo/m920q/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/80609/comment/f755804c_02dec1e6 : PS3, Line 20: 0x8086280b, /* Codec Vendor / Device ID: Intel Kabylake HDMI */
I think you can add `SOC_INTEL_COMMON_BLOCK_HDA_VERB` in the Kconfig for this.
Done
File src/mainboard/lenovo/m920q/mainboard.c:
https://review.coreboot.org/c/coreboot/+/80609/comment/180b24b2_35183848 : PS3, Line 9: static void mainboard_enable(struct device *dev) : { : // nothing? : } : : struct chip_operations mainboard_ops = { : .enable_dev = mainboard_enable, : }; :
Move this into ramstage.c and do the GPIO configuration in mainboard_enable(). […]
Done