[coreboot-gerrit] Change in coreboot[master]: mb/intel/coffeelake_rvp: Add support for new board coffeelake RVP

[Arthur Heymans (Code Review)]

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/Kconfig
File src/mainboard/intel/coffeelake_rvp/Kconfig:

https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/Kconfig@14
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/Kconfig@66
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/dsdt.asl
File src/mainboard/intel/coffeelake_rvp/dsdt.asl:

https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/dsdt.asl@41
PS1, Line 41: 	
shift this and the #endif left


https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/smihandler.c
File src/mainboard/intel/coffeelake_rvp/smihandler.c:

https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/smihandler.c@27
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/variants/cfl_h/devicetree.cb
File src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb:

https://review.coreboot.org/#/c/27904/1/src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb@98
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/variants/cfl_h/devicetree.cb@171
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/variants/cfl_h/include/variant/gpio.h
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/variants/cfl_h/include/variant/gpio.h@1
PS1, Line 1: /*
all these gpio.h files seem quite superfluous. Are they really needed?



-- 
To view, visit https://review.coreboot.org/27904
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id37bfeb0ae51fd630fec96273216dbb2900782c7
Gerrit-Change-Number: 27904
Gerrit-PatchSet: 1
Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela at intel.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki at intel.com>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Comment-Date: Tue, 07 Aug 2018 12:24:25 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180807/240aedfe/attachment.html>