Iru Cai (vimacs) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33325 )
Change subject: mainboard: Add Clevo W650SZ ......................................................................
Patch Set 6:
(14 comments)
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
Done
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
Done
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
Done
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
Done
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?
I don't know what it should be. It cannot be internally flashed when running OEM firmware, but can when running coreboot.
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. […]
Done
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 34: register "gen3_dec" = "0x00000000"
I think you can remove this line. The default is 0.
Done
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 35: register "gen4_dec" = "0x00000000"
I think you can remove this line. The default is 0.
Done
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 55: end
Please move these to the previous line
Done
https://review.coreboot.org/#/c/33325/5/src/mainboard/clevo/w650sz/devicetre... PS5, Line 65: Audio Audio
Please drop one "Audio"
Done
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.
Done
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.
Done
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
Done
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
Done