Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28640 )
Change subject: mb/clevo/kbl-u: Add mainboard and variants N130WU/N131WU ......................................................................
Patch Set 114:
(14 comments)
https://review.coreboot.org/c/coreboot/+/28640/114/Documentation/mainboard/c... File Documentation/mainboard/clevo/index.md:
PS114: Please don't. Just add the boards directly to `Documentation/mainboard/index.md`, without the indirection of this file.
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/Kconf... File src/mainboard/clevo/Kconfig:
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/Kconf... PS114, Line 2: # drop the #
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... File src/mainboard/clevo/kbl-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 3: x we usually spell Kconfig symbols in uppercase
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 80: config MAINBOARD_SUPPORTS_SKYLAKE_CPU : default n if BOARD_CLEVO_N13xWU : : config MAINBOARD_SUPPORTS_KABYLAKE_DUAL : default n if BOARD_CLEVO_N13xWU Is this needed?
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... File src/mainboard/clevo/kbl-u/variants/n13xwu/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 5: register "deep_s3_enable_ac" = "0" : register "deep_s3_enable_dc" = "0" : register "deep_s5_enable_ac" = "0" : register "deep_s5_enable_dc" = "0" : register "deep_sx_config" = "DSX_EN_LAN_WAKE_PIN" If Deep Sx is not enabled, drop this.
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 15: 3 There's an enum for this.
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 19: register "pirqa_routing" = "PCH_IRQ11" : register "pirqb_routing" = "PCH_IRQ10" : register "pirqc_routing" = "PCH_IRQ11" : register "pirqd_routing" = "PCH_IRQ11" : register "pirqe_routing" = "PCH_IRQ11" : register "pirqf_routing" = "PCH_IRQ11" : register "pirqg_routing" = "PCH_IRQ11" : register "pirqh_routing" = "PCH_IRQ11" Drop this
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 58: register "domain_vr_config[VR_SYSTEM_AGENT]" = "{ Is this needed?
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 63: .psi3enable = 0, : .psi4enable = 0, I thought this was enabled by default now?
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 160: 0 There's an enum for this
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 162: register "SataPortsEnable[1]" = "0" This should be enabled
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 164: register "SataPortsDevSlp[0]" = "0" : register "SataPortsDevSlp[2]" = "0" Drop this
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 166: register "SataSpeedLimit" = "2" Why? Problems at higher speeds?
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 170: device pci 00.0 on end # x4 TBT Are these device nodes necessary?