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 11:
(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?
Done
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?
Done
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_... […]
Done
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?
Set the PU/PD configuration for SD card CMD GPIO PAD, agree needs to be moved to main board gpio configuration and not through FSP. Setting it to 0(dsc default) would allow FSP to skip it.
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.
Done
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.
Done
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?
No, guarded for JSL now.
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?
This was a wrong comment, we are not setting the override UPD(CpuRatioOverride) here, instead setting flex ratio for HFM
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. […]
Done
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?
Done
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?
Done
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?
Done