Karthik Ramasubramanian 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 16:
(3 comments)
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?
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.h, it can take 0-23: PCH rootport, 0x70: LAN, 0x80: unspecified but in use, 0xFF: not used.
Why can't we configure 0xFF explicitly?
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