Attention is currently required from: Nico Huber, Peter Lemenkov, Alexander Couzens, Patrick Rudolph, amersel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52659 )
Change subject: mb/lenovo/w541: Add Thinkpad W541 ......................................................................
Patch Set 14:
(19 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52659/comment/729a0817_68292e04 PS14, Line 28: when IFD is unlocked Internal flashing can also be done with a locked IFD, but not all regions are readable/writable.
https://review.coreboot.org/c/coreboot/+/52659/comment/224cb5f3_bf070109 PS14, Line 39: - Each boot takes 20s Sounds like you're using four dual-rank DIMMs (20 seconds), and MRC cache is not working (each boot). I imagine S3 suspend/resume doesn't work either.
File src/mainboard/lenovo/w541/Kconfig:
https://review.coreboot.org/c/coreboot/+/52659/comment/dbf6f4a8_48673a58 PS14, Line 34: config VGA_BIOS_FILE : string : default "pci8086,0416.rom" Please remove. Setting a default for this only makes sense if the file exists
https://review.coreboot.org/c/coreboot/+/52659/comment/9a37b10f_551da4a7 PS14, Line 38: config VGA_BIOS_ID : string : default "8086,0416" This ID depends on the installed CPU. I would remove it.
File src/mainboard/lenovo/w541/board_info.txt:
https://review.coreboot.org/c/coreboot/+/52659/comment/e95d87ac_49f87553 PS14, Line 6: n y?
File src/mainboard/lenovo/w541/cmos.default:
https://review.coreboot.org/c/coreboot/+/52659/comment/6583bcc0_1b4b0fdc PS14, Line 6: first_battery=Primary I only see support for one battery
File src/mainboard/lenovo/w541/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/52659/comment/1e5eb2cd_d2acce2c PS14, Line 25: Host bridge Host bridge One `Host bridge` is enough
https://review.coreboot.org/c/coreboot/+/52659/comment/f21e4528_9fceffbe PS14, Line 26: PCIe Bridge for discrete graphics Unsupported PCI device 8086:0c01 Just `PCIe graphics` would suffice
https://review.coreboot.org/c/coreboot/+/52659/comment/ce5224ca_2d8889ff PS14, Line 27: Internal graphics VGA controller How about `iGPU`?
https://review.coreboot.org/c/coreboot/+/52659/comment/3f0f4f91_ada9a4db PS14, Line 30: Mini-HD audio Audio controller `audio` appears twice, I'd just use `Mini-HD`
https://review.coreboot.org/c/coreboot/+/52659/comment/fc204833_bab08b7b PS14, Line 39: # 0(HDD), 4(M.2), 5(ODD) : register "sata_port_map" = "0x31" Two things:
1. Please indent with tabs 2. Schematics say that SATA port 1 is routed to another M.2/NGFF connector
https://review.coreboot.org/c/coreboot/+/52659/comment/24ead449_47ecffd8 PS14, Line 41: nit: I like to keep device lines in devicetrees aligned as follows:
device pci 14.0 on end # xHCI Controller device pci 16.0 on end # Management Engine Interface 1 device pci 16.1 off end # Management Engine Interface 2 device pci 16.2 off end # Management Engine IDE-R device pci 16.3 on end # Management Engine KT device pci 19.0 on # Intel Gigabit Ethernet
https://review.coreboot.org/c/coreboot/+/52659/comment/b4c83ea4_4dd0a08c PS14, Line 46: Unsupported PCI device 8086:153a Please drop this part
https://review.coreboot.org/c/coreboot/+/52659/comment/7f35199a_006801e0 PS14, Line 57: LPC bridge PCI-LPC bridge Just `LPC bridge` is enough
File src/mainboard/lenovo/w541/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/5b974270_7afad145 PS14, Line 6: Realtek Realtek ALC292
File src/mainboard/lenovo/w541/mainboard.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/db388b57_fa787081 PS14, Line 5: #include <ec/lenovo/h8/h8.h> Not used?
https://review.coreboot.org/c/coreboot/+/52659/comment/ee9936c8_205dcfaa PS14, Line 9: * FIXME: fix those values*/ Seem right to me?
File src/mainboard/lenovo/w541/romstage.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/7df537e3_9b61e4f4 PS14, Line 34: u32 reg32 = pci_read_config32(PCI_DEV(0, 0, 0), DEVEN); : reg32 &= ~DEVEN_D1F0EN; : : pci_write_config32(PCI_DEV(0, 0, 0), DEVEN, reg32); Much simpler:
pci_and_config32(HOST_BRIDGE, DEVEN, ~DEVEN_D1F0EN);
File src/mainboard/lenovo/w541/smihandler.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/8d6fca09_820dcc4e PS14, Line 13: #define GPE_EC_WAKE 13 Seems to be correct.