Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44302 )
Change subject: mb/lenovo/thinkcentre_m91p add ThinkCentre M91p ......................................................................
Patch Set 1:
(12 comments)
Welcome!
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 9: /* The lid is open by default. */ : gnvs->lids = 1; Which lid? This isn't a laptop, so this can be dropped.
You can then drop the FIXME above as well.
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 1: # FIXME: GPU registers may not always apply. : register "gfx" = "GMA_STATIC_DISPLAYS(0)" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "0" : register "gpu_dp_c_hotplug" = "0" : register "gpu_dp_d_hotplug" = "0" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "0" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000" This can be dropped completely (starting from the FIXME up until `gpu_pch_backlight` (inclusive).
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 23: end nit: I usually `pick up` these lone `end` keywords and put them on the same line if there's nothing inside the corresponding `device`:
device lapic 0x0 on end device lapic 0xacac off end
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 31: register "docking_supported" = "0" Any devicetree field that is zero and does not provide any useful info can be removed. For example, this.
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 33: register "gen2_dec" = "0x00000000" : register "gen3_dec" = "0x00000000" : register "gen4_dec" = "0x00000000" : register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Also zero, can be dropped
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 60: device pci 1c.0 off # PCIe Port #1 : end : device pci 1c.1 off # PCIe Port #2 : end : device pci 1c.2 off # PCIe Port #3 : end : device pci 1c.3 off # PCIe Port #4 : end : device pci 1c.4 off # PCIe Port #5 : end : device pci 1c.5 off # PCIe Port #6 : end : device pci 1c.6 off # PCIe Port #7 : end : device pci 1c.7 off # PCIe Port #8 : end this is weird.
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 96: Host bridge Host bridge One `Host bridge` is enough, isn't it? 😄
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 96: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x17aa 0x3070 : end : device pci 01.0 off # PEG : end : device pci 02.0 on # iGPU : subsystemid 0x17aa 0x3070 : end These devices correspond to northbridge functions. I prefer to have them above the southbridge (that is, just before `chip southbridge/intel/bd82x6x # Intel Series 6 Cougar Point PCH`)
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 1: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB These definitions are unused and can be removed.
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 25: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> No backlight on this board, so this isn't necessary.
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/early_init.c:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 3: nit: double blank line
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 33: /* FIXME: Put proper SPD map here. */ Easy way to test: check if the board is able to boot with a single DIMM, for every slot. Especially interesting are the two middle slots.