Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47707 )
Change subject: mb/intel/ehlcrb: Add initial mainboard code ......................................................................
Patch Set 4: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... File src/mainboard/intel/elkhartlake_crb/Kconfig:
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... PS4, Line 43: default 8 You have this define in Kconfig of soc already:
config MAX_CPUS int default 4
The Kconfig for the soc should have the higher number as it is meant for every possible SKU. Then, if needed for this particular CRB, one could adjust MAX_CPUS here with a lower value.
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... File src/mainboard/intel/elkhartlake_crb/board_info.txt:
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... PS4, Line 2: crb Maybe "CRB" would be better here?
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... PS4, Line 6: Flashrom support: y Does flashrom already have support for EHL?
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... File src/mainboard/intel/elkhartlake_crb/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... PS4, Line 1: FLASH@0xff000000 0x1000000 { There is a feature in fmaptool where the definition of the offset for the regions is not needed anymore (see https://review.coreboot.org/c/coreboot/+/33276). This makes the fmd-file better readable and maintainable. I understand that this is just a copy from jasperlake but maybe you can keep that in mind when you start adjusting for Elkhartlake.
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... File src/mainboard/intel/elkhartlake_crb/mainboard.c:
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... PS4, Line 33: sku2147483647 What does this string mean? Looks strange to me but I may not have the right meaning.
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... File src/mainboard/intel/elkhartlake_crb/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/47707/4/src/mainboard/intel/elkhart... PS4, Line 17: const struct mb_cfg *variant_memcfg_config(uint8_t board_id); The upper comment does not apply to this function. Maybe rearrange the order so that the comment can be exclusive for the upper three functions?