Attention is currently required from: Alexander Couzens, Cleyton Silva, Keith Hui.
Angel Pons has posted comments on this change by Cleyton Silva. ( https://review.coreboot.org/c/coreboot/+/83670?usp=email )
Change subject: mb/lenovo: Add IH61M mainboard ......................................................................
Patch Set 1: Code-Review+1
(24 comments)
Patchset:
PS1: Welcome to coreboot!
Commit Message:
https://review.coreboot.org/c/coreboot/+/83670/comment/d1282377_c83b0844?usp... : PS1, Line 13: 2MB+4MB Hmmm, usually the larger chip is the first one. Could you please check? I believe inteltool's dump option would answer this question, if you have a full firmware dump.
https://review.coreboot.org/c/coreboot/+/83670/comment/6fb84b74_e905652f?usp... : PS1, Line 20: - All USB ports : - USB 2.0 headers "All USB ports" would already include "USB 2.0 headers", I believe. Was any USB port/header/connector not tested?
https://review.coreboot.org/c/coreboot/+/83670/comment/59760835_3dd1e926?usp... : PS1, Line 26: - Audio If the `F_AUDIO` header is untested, I imagine this refers to the rear audio jacks. Would be nice to specify
https://review.coreboot.org/c/coreboot/+/83670/comment/73a013b4_12a0af4f?usp... : PS1, Line 43: - Internal flashing : - External flashing (in-circuit) Flashing internally should be possible when running coreboot. Could you please describe the errors you get in each case?
Also, how was the board flashed?
File src/mainboard/lenovo/ih61m/Kconfig:
https://review.coreboot.org/c/coreboot/+/83670/comment/5a95f317_387a5295?usp... : PS1, Line 27: config DRAM_RESET_GATE_GPIO : int : default 60 Default, can be dropped.
https://review.coreboot.org/c/coreboot/+/83670/comment/e784873a_52fe2775?usp... : PS1, Line 31: config USBDEBUG_HCD_INDEX : int : default 2 #top usb port located next to vga connector : #default 1 #yellow upper header F_USB1 The `int` type is not needed, and I would refactor the comments a bit:
```suggestion # HCD 1: yellow upper header F_USB1 # HCD 2: top USB port located next to VGA connector config USBDEBUG_HCD_INDEX default 2 ```
Defaulting to the connector on the back panel is very reasonable (it doesn't require extra hardware to be used)
File src/mainboard/lenovo/ih61m/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/83670/comment/34efbda6_3f9a743d?usp... : PS1, Line 1: config BOARD_LENOVO_IH61MV1 : bool "IH61M Ver:1.0" : : config BOARD_LENOVO_IH61MV4 : bool "IH61M Ver:4.2" nit-pick: Add an extra underscore to the Kconfig symbols to separate the model and the version?
```suggestion config BOARD_LENOVO_IH61M_V1 bool "IH61M Ver:1.0"
config BOARD_LENOVO_IH61M_V4 bool "IH61M Ver:4.2" ```
File src/mainboard/lenovo/ih61m/acpi_tables.c:
PS1: You can delete this file. autoport generates it by default but it only makes a difference if other code uses these GNVS values.
File src/mainboard/lenovo/ih61m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/83670/comment/de293a66_c5f03988?usp... : PS1, Line 1: # SPDX-License-Identifier: GPL-2.0-or-later : chip northbridge/intel/sandybridge nit-pick: Add a blank line after the license header?
```suggestion # SPDX-License-Identifier: GPL-2.0-or-later
chip northbridge/intel/sandybridge ```
https://review.coreboot.org/c/coreboot/+/83670/comment/5e953c89_d2379311?usp... : PS1, Line 6: device ref host_bridge on end # Host bridge Devices that are already on/off in the chipset devicetree [1] can be omitted. Here, the host bridge is already enabled.
[1]: https://github.com/coreboot/coreboot/blob/main/src/northbridge/intel/sandybr...
https://review.coreboot.org/c/coreboot/+/83670/comment/e3d07e84_ea6fd2c1?usp... : PS1, Line 10: register "docking_supported" = "0" Generally, a devicetree "register" that is set to a zero value can be omitted (it is already zero by default). This line can be omitted
https://review.coreboot.org/c/coreboot/+/83670/comment/9839224f_046973bd?usp... : PS1, Line 14: register "gen4_dec" = "0x00000000" : register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Zero by default, can also be omitted
https://review.coreboot.org/c/coreboot/+/83670/comment/5e62c1aa_cf8517e8?usp... : PS1, Line 37: device ref mei1 on end : device ref me_ide_r off end : device ref me_kt off end Same state as chipset devicetree, can be omitted.
https://review.coreboot.org/c/coreboot/+/83670/comment/008da120_e377072c?usp... : PS1, Line 41: device ref gbe on end This device is only used with a special kind of Intel GbE controllers. Looks like this board uses a Realtek GbE controller instead, so this line can be removed.
https://review.coreboot.org/c/coreboot/+/83670/comment/72bcbc7c_2cd5f947?usp... : PS1, Line 43: device ref pcie_rp1 on end : device ref pcie_rp4 on end # PCIEX1_1 : device ref pcie_rp5 on end # PCIEX1_2 : device ref pcie_rp6 on end Do you know what the rest of PCIe ports are used for? From the V1.0 schematics, looks like the mapping is as follows: ```suggestion device ref pcie_rp4 on end # PCIEX1_1 slot device ref pcie_rp5 on end # PCIEX1_2 slot device ref pcie_rp6 on end # Realtek RTL8111E GbE ```
https://review.coreboot.org/c/coreboot/+/83670/comment/cd8031a2_6b8d4c4c?usp... : PS1, Line 67: io 0x60 = 0x02f8 nit-pick: Use consistent style across the file, e.g.
```suggestion io 0x60 = 0x2f8 ```
https://review.coreboot.org/c/coreboot/+/83670/comment/31a5bd41_d60f0ec1?usp... : PS1, Line 107: device ref sata2 off end # SATA (Legacy) Already disabled in chipset devicetree, can remove from here
https://review.coreboot.org/c/coreboot/+/83670/comment/f788367e_e89ecc24?usp... : PS1, Line 108: device ref smbus on end # SMBus Already enabled in chipset devicetree, can remove from here
File src/mainboard/lenovo/ih61m/early_init.c:
https://review.coreboot.org/c/coreboot/+/83670/comment/89f7ae5e_3961f6ff?usp... : PS1, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ : #include <bootblock_common.h> Nit-pick: Blank line after license header:
```suggestion /* SPDX-License-Identifier: GPL-2.0-only */
#include <bootblock_common.h> ```
File src/mainboard/lenovo/ih61m/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/83670/comment/16f03323_65b25afe?usp... : PS1, Line 11: ports : constant Port_List := : (HDMI1, : HDMI2, : HDMI3, : Analog, : others => Disabled); V1.0 schematics say DVI-D is on digital output D (HDMI3):
```suggestion ports : constant Port_List := (HDMI3, -- DVI-D Analog, -- VGA others => Disabled); ```
File src/mainboard/lenovo/ih61m/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/83670/comment/025f808f_ac7aaa75?usp... : PS1, Line 21: 0x80862805, /* Codec Vendor / Device ID: Intel */ : 0x80860101, /* Subsystem ID */ : 4, /* Number of 4 dword sets */ : AZALIA_SUBVENDOR(3, 0x80860101), : AZALIA_PIN_CFG(3, 0x05, 0x18560010), : AZALIA_PIN_CFG(3, 0x06, 0x18560020), : AZALIA_PIN_CFG(3, 0x07, 0x18560030), This is for the iGPU's audio outputs. If this board doesn't have HDMI or DP outputs, this is irrelevant (neither DVI nor VGA support audio)
https://review.coreboot.org/c/coreboot/+/83670/comment/0b4b7186_e71b0828?usp... : PS1, Line 28: nit-pick: drop blank line
File src/mainboard/lenovo/ih61m/mainboard.c:
https://review.coreboot.org/c/coreboot/+/83670/comment/340c5338_4f5ca01c?usp... : PS1, Line 10: beep(2500, 100); This is not mainboard-specific. Instead, consider making a separate change that adds a Kconfig option to enable beeping on boot.