Attention is currently required from: Maciej Pijanowski, Michał Kopeć.
Nicholas Sudsgaard has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80609?usp=email )
Change subject: mb/lenovo/m920q: add board ......................................................................
Patch Set 3:
(16 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80609/comment/cb48c306_8ac5a8fb : PS3, Line 7: mb/lenovo/m920q: add board nit: mb/lenovo: Add ThinkCentre M920q
https://review.coreboot.org/c/coreboot/+/80609/comment/8d17986f_6bae7e6c : PS3, Line 9: Add Lenovo Thinkcentre Tiny mainboard. This seems redundant.
Patchset:
PS3: Nice seeing another ThinkCentre! 😊
File src/mainboard/lenovo/m920q/Kconfig:
https://review.coreboot.org/c/coreboot/+/80609/comment/aeb290cd_d9efb929 : PS3, Line 1: if BOARD_LENOVO_M920Q Add SPDX license header.
https://review.coreboot.org/c/coreboot/+/80609/comment/15a77201_0ca3aaac : PS3, Line 4: def_bool y It would be better sorted.
``` select BOARD_ROMSIZE_KB_24576 select DRIVERS_UART_8250IO select HAVE_ACPI_RESUME select HAVE_ACPI_TABLES select HAVE_CMOS_DEFAULT select HAVE_OPTION_TABLE select INTEL_GMA_HAVE_VBT select MAINBOARD_HAS_LIBGFXINIT select MAINBOARD_HAS_TPM2 select MAINBOARD_USES_IFD_GBE_REGION select MEMORY_MAPPED_TPM select SOC_INTEL_CANNONLAKE_PCH_H select SOC_INTEL_COFFEELAKE select SUPERIO_NUVOTON_NCT6687D ```
File src/mainboard/lenovo/m920q/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/80609/comment/0974c1f8_13405570 : PS3, Line 1: config BOARD_LENOVO_M920Q Add SPDX license header.
File src/mainboard/lenovo/m920q/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/80609/comment/ec977b34_2d0f6b20 : PS3, Line 3: subdirs-y += spd This seems unnecessary.
File src/mainboard/lenovo/m920q/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/80609/comment/01e77aea_7c19d0f6 : PS3, Line 13: # FSP Configuration This comment seems redundant.
https://review.coreboot.org/c/coreboot/+/80609/comment/8f82bc99_ced95bc0 : PS3, Line 14: register "usb2_ports[0]" = "USB2_PORT_MID(OC0)" # Internal USB header I would suggest this format for bulk definitions. ``` register "usb2_ports" = "{ [0] = ..., // comment ... }" ```
https://review.coreboot.org/c/coreboot/+/80609/comment/acebef20_e2a8a874 : PS3, Line 23: register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC1)" # Front port (charger) Here too.
https://review.coreboot.org/c/coreboot/+/80609/comment/5f229a93_dcae91cf : PS3, Line 32: register "SataPortsEnable[0]" = "1" Here too.
https://review.coreboot.org/c/coreboot/+/80609/comment/cbb6171d_b2a5ac5a : PS3, Line 42: register "PcieRpEnable[0]" = "0" Here too.
https://review.coreboot.org/c/coreboot/+/80609/comment/4e0f8438_c2246d57 : PS3, Line 67: register "PcieClkSrcUsage[0]" = "0x40" # PCIEx8 slot Here too.
https://review.coreboot.org/c/coreboot/+/80609/comment/4b3adafe_9e07c5d0 : PS3, Line 84: register "PcieClkSrcClkReq[0]" = "0" Here too.
https://review.coreboot.org/c/coreboot/+/80609/comment/ed59cf9c_3fb01ec9 : PS3, Line 106: device domain 0 on I would suggest using `ref x` instead of `pci xx.x`. This should also eliminate the need for most of the comments. You can find the aliases in `soc/intel/cannonlake/chipset.cb`
File src/mainboard/lenovo/m920q/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/80609/comment/0719799e_86b84e77 : 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.