Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 19: #include <fsp/ppi/mp_service_ppi.h>
Move it one line above so that it is consistent with the alphabetical order of include.
Done
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 47: config = config_of_soc();
Nit: declare and initialize in the previous line itself.
Done
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 65: sizeof(config->SerialIoUartMode));
Just curious: FSP UPD seems to support 7 I2C/GSPI/UART ports whereas in JSL there are 6,3 & 3 ports […]
They are more as reserved fields, the controller number supported is retrieved in FSP and is specific to platform, for JSL it returns 6,3 & 3 for i2C, UART and GSPI respectively.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 48: /* If Audio Codec is enabled, enable FSP UPD */ : dev = pcidev_path_on_root(PCH_DEVFN_HDA); : if (!dev) : m_cfg->PchHdaEnable = 0; : else : m_cfg->PchHdaEnable = dev->enabled;
Can this be moved with the other Audio related configuration below for better grouping?
Done
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 67: config->PcieClkSrcUsage[i] == 0
I am not sure if it cannot take the value of 0. As per the comments in tigerlake's chip. […]
if config does not get set explicity through the devicetree(remains 0), this would ensure the Clkrsrc ic configuerd as not used.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 103: m_cfg->PchHdaAudioLinkDmicEnable[0] = config->PchHdaAudioLinkDmicEnable[0]; : m_cfg->PchHdaAudioLinkDmicEnable[1] = config->PchHdaAudioLinkDmicEnable[1];
Use memcpy
Done