Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31955 )
Change subject: soc/intel/icelake: Pass FSP-M/S UPD as per ICL requirement ......................................................................
Patch Set 13:
(17 comments)
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 32: SA_DEV_ROOT Using a macro to hide a call to a function is not a good idea.
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 38: config_t don't use typedefs for structs.
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 40: CONFIG_SOC_INTEL_I2C_DEV_MAX Why is there a Kconfig option for this? Does it change depending on soc variants?
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 44: CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX Why is there a Kconfig option for this? Does it change depending on soc variants?
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 50: CONFIG_SOC_INTEL_UART_DEV_MAX Why is there a Kconfig option for this? Does it change depending on soc variants?
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 95: dev_find_slot pcidev_on_root
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 109: dev_find_slot pcidev_on_root
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 166: dev_find_slot pcidev_on_root
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 182: dev_find_slot pcidev_on_root
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 206: dev_find_slot pcidev_on_root
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 220: __weak void mainboard_silicon_init_params(FSP_S_CONFIG *params) : { : printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); : } Please remove this. You don't want it to build without an implementation.
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 27: dev_find_slot use pcidev_on_root()
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 31: 0xFE can you make this number sensible instead of needing a comment?
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 37: BOARD_TYPE_ULT_ULX Should this be configurable?
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 69: dev_find_slot pcidev_on_root
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 71: config_t Don't use typedefs for this.
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 84: __weak void mainboard_memory_init_params(FSPM_UPD *mupd) : { : printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); : } Please remove this. You generally don't want it to compile without an implementation of this.