Subrata Banik 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 16:
(18 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.
Done
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.
Done
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?
yes, it is between LP and H PCH
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?
yes, it is between LP and H PCH
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?
yes, it is between LP and H PCH
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 95: dev_find_slot
pcidev_on_root
Done
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 109: dev_find_slot
pcidev_on_root
Done
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 166: dev_find_slot
pcidev_on_root
Done
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 182: dev_find_slot
pcidev_on_root
Done
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/fsp_params.c@... PS13, Line 206: dev_find_slot
pcidev_on_root
Done
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.
I don't think, i can remove it now till all mb initial patches got merged
here is the error when i was trying to delete this function, but i agree to remove this once we have entire ICL tree merged.
i386-elf-ld.bfd: /cb-build/coreboot-gerrit.0/INTEL_ICELAKE_RVPU/generated/ramstage.o: in function `platform_fsp_silicon_init_params_cb': /home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/intel/icelake/fsp_params.c:85: undefined reference to `mainboard_silicon_init_params' make[1]: *** [src/arch/x86/Makefile.inc:388: /cb-build/coreboot-gerrit.0/INTEL_ICELAKE_RVPU/cbfs/fallback/ramstage.debug] Error 1 make[1]: Leaving directory '/home/coreboot/slave-root/workspace/coreboot-gerrit'
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()
Done
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/coreboot/+/31954/13/src/vendorcode/intel/fsp/f... + 1802
/** Offset 0x025D - Internal Graphics Pre-allocated Memory Size of memory preallocated for internal graphics. 0x00:0MB, 0x01:32MB, 0x02:64MB, 0x03:96MB, 0x04:128MB, 0x05:160MB, 0xF0:4MB, 0xF1:8MB, 0xF2:12MB, 0xF3:16MB, 0xF4:20MB, 0xF5:24MB, 0xF6:28MB, 0xF7:32MB, 0xF8:36MB, 0xF9:40MB, 0xFA:44MB, 0xFB:48MB, 0xFC:52MB, 0xFD:56MB, 0xFE:60MB **/ UINT8 IgdDvmt50PreAlloc;
this is what we are feeding into coreboot from FSP
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/coreboot/+/31954/13/src/vendorcode/intel/fsp/f... + 1768
/** Offset 0x0234 - Board Type MrcBoardType, Options are 0=Mobile/Mobile Halo, 1=Desktop/DT Halo, 5=ULT/ULX/Mobile Halo, 7=UP Server 0:Mobile/Mobile Halo, 1:Desktop/DT Halo, 5:ULT/ULX/Mobile Halo, 7:UP Server **/ UINT8 UserBd;
ICL with coreboot is not targeted for 0:Mobile/Mobile Halo, 1:Desktop/DT Halo, or 7:UP Server
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 64: m_cfg->VmxEnable = config->VmxEnable;
Now, we are using Kconfig for Vmx […]
Done
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 69: dev_find_slot
pcidev_on_root
Done
https://review.coreboot.org/#/c/31955/13/src/soc/intel/icelake/romstage/fsp_... PS13, Line 71: config_t
Don't use typedefs for this.
Done
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.
same as other file