Attention is currently required from: Rizwan Qureshi, Tim Wawrzynczak, Subrata Banik, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55225 )
Change subject: soc/intel/alderlake/romstage: Refactor soc_memory_init_params function ......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/55225/comment/d0b204bb_63624548 PS1, Line 12: This would help to increase the code readability and in future : meaningful addition of FSP-M UPDs is possible rather adding UPDs randomly I like this change! :).
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55225/comment/ccbbea9d_da538af4 PS1, Line 63: memory_init nit: Since all these functions live within romstage/fsp_params.c, do we need memory_init in the name here? Just `fill_xyz_params()` or `fill_fspm_xyz_params()`?
https://review.coreboot.org/c/coreboot/+/55225/comment/adee9629_cc4f95c7 PS1, Line 72: Probably as part of follow-up change, this can be updated to set UPDs differently based on InternalGfx:
``` if (m_cfg->InternalGfx) { m_cfg->IgdDvmt50PreAlloc = IGD_SM_60MB; m_cfg->DdiPortAConfig = config->DdiPortAConfig; m_cfg->DdiPortBConfig = config->DdiPortBConfig; ... } else { m_cfg->IgdDvmt50PreAlloc = 0; m_cfg->DdiPortAConfig = 0; m_cfg->DdiPortBConfig = 0; ... } ```
https://review.coreboot.org/c/coreboot/+/55225/comment/c29e81f2_bc78aae1 PS1, Line 101: m_cfg->ChHashMask = 0x30CC; Not for this change, but this setting doesn't even take effect. ChHashOverride is set to 0. I think this should be dropped in a follow-up CL.
https://review.coreboot.org/c/coreboot/+/55225/comment/bcf5a5a3_06a0ae45 PS1, Line 187: disable This function is not just disabling clksrc.
https://review.coreboot.org/c/coreboot/+/55225/comment/216c2458_c386e190 PS1, Line 320: Fill IGD related FSP-M UPDs nit: Not sure if the comments are really required since they mostly repeat the function names. If you think it is good to have them, feel free to leave as is.