Furquan Shaikh 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 10:
(12 comments)
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 50: SerialIoI2cMode Just curious: Can we not use memcpy for the params here?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 70: config = config_of_soc(); This can potentially go onto the previous line?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 131: pcidev_on_root(PCH_DEV_SLOT_XHCI, 5); Why not use pcidev_path_on_root(PCH_DEVFN_...);
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 138: params->SdCardGpioCmdPadTermination = 0x1F; What does this mean? Shouldn't this be mainboard controlled just like the active high config above?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 143: pcidev_on_root(PCH_DEV_SLOT_STORAGE, 0); same comment as above.
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 152: pcidev_on_root(PCH_DEV_SLOT_XHCI, 1); same comment as above.
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... PS10, Line 125: #define PCH_DEV_SLOT_STORAGE 0x1a : #define PCH_DEVFN_EMMC _PCH_DEVFN(STORAGE, 0) : #define PCH_DEV_EMMC _PCH_DEV(STORAGE, 0) Is this true for both TGL and JSL?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 80: temporary why? and for how long?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 83: 0x02 fsp_params_tgl.c uses some enums here. Can we do the same for JSL?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 96: for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSspEnable); i++) { : m_cfg->PchHdaAudioLinkSspEnable[i] = : config->PchHdaAudioLinkSspEnable[i]; : } Would memcpy work?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 101: for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable); i++) { : m_cfg->PchHdaAudioLinkSndwEnable[i] = : config->PchHdaAudioLinkSndwEnable[i]; : } Would memcpy work?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 113: /* Enable SMBus controller based on config */ : m_cfg->SmbusEnable = config->SmbusEnable; : : /* Set debug probe type */ : m_cfg->PlatformDebugConsent = config->DebugConsent; : : /* Vt-D config */ : m_cfg->VtdDisable = 0; Why are these not done as part of soc_memory_init_params?