Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33325 )
Change subject: mainboard: Add Clevo W650SZ ......................................................................
Patch Set 5: Code-Review+1
(15 comments)
Looks good!
https://review.coreboot.org/#/c/33325/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33325/5//COMMIT_MSG@10 PS5, Line 10: 30890 Doesn't seem to be this one
https://review.coreboot.org/#/c/33325/5//COMMIT_MSG@12 PS5, Line 12: This laptop has two flash chips with 2MB(ME)+4MB(BIOS), so it depends : on 30980. : Was merged
https://review.coreboot.org/#/c/33325/5//COMMIT_MSG@16 PS5, Line 16: : The EHCI debug port is at the right hand side of the laptop, but the : debug message stops to display after mrc.bin initializes USB. You probably want to re-init the console after mrc.bin has run
https://review.coreboot.org/#/c/33325/5/Documentation/mainboard/clevo/w650sz... File Documentation/mainboard/clevo/w650sz.md:
https://review.coreboot.org/#/c/33325/5/Documentation/mainboard/clevo/w650sz... PS5, Line 52: --layout layout.txt see --ifd
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/Kconfig File src/mainboard/clevo/w650sz/Kconfig:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/Kconfig@1 PS5, Line 1: NOTEBOOK CLEVO
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/Kconfig@3... PS5, Line 33: : config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID : hex : default 0x655 : : config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID : hex : default 0x1558 The Haswell code doesn't use these
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/Kconfig.n... File src/mainboard/clevo/w650sz/Kconfig.name:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/Kconfig.n... PS5, Line 1: NOTEBOOK CLEVO
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/board_inf... File src/mainboard/clevo/w650sz/board_info.txt:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/board_inf... PS5, Line 5: n n?
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... File src/mainboard/clevo/w650sz/devicetree.cb:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 2: gfx.did Should be as long as "gfx.ndid"
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 55: end Please move these to the previous line
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 65: Audio Audio Please drop one "Audio"
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 101: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x1558 0x0655 : end : device pci 01.0 on # PCIe Bridge for discrete graphics : subsystemid 0x1558 0x0655 : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x1558 0x0655 : end : device pci 03.0 on # Mini-HD audio Audio controller : subsystemid 0x1558 0x0655 : end Since these appear at the top of "lspci", it makes more sense to move them above the "chip" entry.
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/dsdt.asl File src/mainboard/clevo/w650sz/dsdt.asl:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/dsdt.asl@... PS5, Line 30: : /* Some generic macros */ Please remove this obnoxious comment.
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/hda_verb.... File src/mainboard/clevo/w650sz/hda_verb.c:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/hda_verb.... PS5, Line 27: : /* NID 0x24. */ These comments can be removed
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/mainboard... File src/mainboard/clevo/w650sz/mainboard.c:
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/mainboard... PS5, Line 33: GMA_INT15_PANEL_FIT_DEFAULT, : GMA_INT15_BOOT_DISPLAY_DEFAULT, 0); Please align these with the previous line