Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/27904 )
Change subject: mb/intel/coffeelake_rvp: Add support for new board coffeelake RVP ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/K... PS1, Line 14: select UART_DEBUG please add select INTEL_GMA_HAVE_VBT and add the respective data.vbt files in the variant dirs.
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/K... PS1, Line 66: config ME_BIN_PATH : string : depends on HAVE_ME_BIN : default "3rdparty/blobs/mainboard/$(CONFIG_MAINBOARD_DIR)/me.bin" : : config EC_BIN_PATH : string : depends on HAVE_EC_BIN : default "3rdparty/blobs/mainboard/$(CONFIG_MAINBOARD_DIR)/ec.bin" This should be put somewhere else.
Could you reuse southbridge/intel/common/firmware/Kconfig for this?
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/d... File src/mainboard/intel/coffeelake_rvp/dsdt.asl:
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/d... PS1, Line 41: shift this and the #endif left
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/s... File src/mainboard/intel/coffeelake_rvp/smihandler.c:
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/s... PS1, Line 27: case 0x99: : printk(BIOS_DEBUG, "Sample\n"); : smm_get_gnvs()->smif = 0; : break; Remove?
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/v... File src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb:
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/v... PS1, Line 98: #if IS_ENABLED(CONFIG_INCLUDE_SND_MAX98373_NHLT) Would be cleaner to shift this left IMO.
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/v... PS1, Line 171: device pci 1f.3 on I think you'll also want this in the devicetree on an #else path for the preprocessor condition.
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/v... File src/mainboard/intel/coffeelake_rvp/variants/cfl_h/include/variant/gpio.h:
https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/v... PS1, Line 1: /* all these gpio.h files seem quite superfluous. Are they really needed?