Attention is currently required from: Nico Huber, Peter Lemenkov, Angel Pons, Alexander Couzens, Patrick Rudolph. amersel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52659 )
Change subject: mb/lenovo/w541: Add Thinkpad W541 ......................................................................
Patch Set 15:
(17 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52659/comment/7f308840_e8418b2a 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) […]
Oh. Each boot should be corrected to first boot. When I reboot, it takes booting takes no time at all. S3 suspend/resume appears to be working.
I believe I'm currently using two dual rank DIMMs. It takes even longer when I have all four slots filled. I've never actually timed it though, 20s is a personal estimate.
File src/mainboard/lenovo/w541/Kconfig:
https://review.coreboot.org/c/coreboot/+/52659/comment/f8165476_32554b38 PS14, Line 34: config VGA_BIOS_FILE : string : default "pci8086,0416.rom"
Please remove. […]
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/6952c021_a1a147d5 PS14, Line 38: config VGA_BIOS_ID : string : default "8086,0416"
This ID depends on the installed CPU. I would remove it.
Done
File src/mainboard/lenovo/w541/board_info.txt:
https://review.coreboot.org/c/coreboot/+/52659/comment/10ae3e3c_581f999e PS14, Line 6: n
y?
Done
File src/mainboard/lenovo/w541/cmos.default:
https://review.coreboot.org/c/coreboot/+/52659/comment/2f1e6fa8_9c41a583 PS14, Line 6: first_battery=Primary
I only see support for one battery
Done
File src/mainboard/lenovo/w541/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/52659/comment/5f8347f8_0add4be7 PS14, Line 25: Host bridge Host bridge
One `Host bridge` is enough
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/ae49b269_1c9ee331 PS14, Line 26: PCIe Bridge for discrete graphics Unsupported PCI device 8086:0c01
Just `PCIe graphics` would suffice
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/540eebee_0402ad29 PS14, Line 27: Internal graphics VGA controller
How about `iGPU`?
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/0fbc5887_d9fd4b43 PS14, Line 30: Mini-HD audio Audio controller
`audio` appears twice, I'd just use `Mini-HD`
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/f2a0a445_2b48090f PS14, Line 39: # 0(HDD), 4(M.2), 5(ODD) : register "sata_port_map" = "0x31"
Two things: […]
Interesting that the schematics say there is a M.2/NGFF on the IO Subcard since there appears to be no such thing on the actual subcard.
Regardless, I've added it here in case the port is actually there or unique to other subcards.
https://review.coreboot.org/c/coreboot/+/52659/comment/3cbcc952_fbe03e5d PS14, Line 41:
nit: I like to keep device lines in devicetrees aligned as follows: […]
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/737fea6f_434e8ac8 PS14, Line 46: Unsupported PCI device 8086:153a
Please drop this part
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/cfc0f227_d8ff2e69 PS14, Line 57: LPC bridge PCI-LPC bridge
Just `LPC bridge` is enough
Done
File src/mainboard/lenovo/w541/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/46153be0_d9439e65 PS14, Line 6: Realtek
Realtek ALC292
Done
File src/mainboard/lenovo/w541/mainboard.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/a0cf74fd_63b72339 PS14, Line 5: #include <ec/lenovo/h8/h8.h>
Not used?
Done
https://review.coreboot.org/c/coreboot/+/52659/comment/af3e13cd_72595dd0 PS14, Line 9: * FIXME: fix those values*/
Seem right to me?
Done
File src/mainboard/lenovo/w541/romstage.c:
https://review.coreboot.org/c/coreboot/+/52659/comment/9e962d3b_964fdf27 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: […]
Done
-
amersel (Code Review)