Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47707 )
Change subject: mb/intel/ehlcrb: Add initial mainboard code ......................................................................
Patch Set 5:
(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: […]
Done
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?
Done
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?
We have not tested it, but it should be supported as the SPI chip we used in CRB is in the supported hardware list (W25Q256) https://www.flashrom.org/Supported_hardware
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 anym […]
This is good input. I spent 30mins just to calculate the offset 😂 Will remove it. Thanks!
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.
Yeah this is more JSL & chrome specific, plan to remove it later in EHL updates.
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. […]
Good catch. Done