Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45192 )
Change subject: soc/intel/alderlake/romstage: Do initial SoC commit till romstage ......................................................................
Patch Set 8: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... PS7, Line 109: uint8_t dimm_cfg = DISABLE_DIMM1; /* Use only DIMM0 */
If you see this two line, it will be much clear […]
I checked memory support for ADL and I see the following:
+-------------+--------+---------+ | Memory Type | Max Ch | Max DPC | +-------------+--------+---------+ | DDR4 | 1 | 2 | | LPDDR4(X) | 4 | 1 | | LPDDR5 | 4 | 1 | | DDR5 | 2 | 1 | +-------------+--------+---------+
These numbers are for a single memory controller. Since ADL has two memory controllers, there's twice as many channels in total. DPC stays the same.
So, 2DPC is only possible with DDR4, and we would handle that differently anyway. Therefore, this `DISABLE_DIMM1` looks correct. What now looks odd is `ENABLE_BOTH_DIMMS` below.
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... PS7, Line 136: ENABLE_BOTH_DIMMS Since the second SPD pointer is zero, shouldn't this be `DISABLE_DIMM1` as well?
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/mem... PS7, Line 183: mem_cfg->ECT = board_cfg->ect;
https://github. […]
Thank you. BTW, looks like the comments on those files are wrong. They say *Disable* ECT 😄
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/res... File src/soc/intel/alderlake/reset.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/res... PS7, Line 31: die("unknown reset type");
good suggestion Angel. can i do that as incremental after this CL. […]
Sounds good to me. Thanks!
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45192/7/src/soc/intel/alderlake/rom... PS7, Line 91: 1
good idea, i will do that after this CL?
Sure. If other platforms have an UPD to control HyperThreading, it would be nice to hook it up as well.
https://review.coreboot.org/c/coreboot/+/45192/8/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45192/8/src/soc/intel/alderlake/rom... PS8, Line 69: /* DP port config */ : m_cfg->DdiPortAConfig = config->DdiPortAConfig; : m_cfg->DdiPortBConfig = config->DdiPortBConfig; : m_cfg->DdiPortAHpd = config->DdiPortAHpd; : m_cfg->DdiPortBHpd = config->DdiPortBHpd; : m_cfg->DdiPortCHpd = config->DdiPortCHpd; : m_cfg->DdiPort1Hpd = config->DdiPort1Hpd; : m_cfg->DdiPort2Hpd = config->DdiPort2Hpd; : m_cfg->DdiPort3Hpd = config->DdiPort3Hpd; : m_cfg->DdiPort4Hpd = config->DdiPort4Hpd; : m_cfg->DdiPortADdc = config->DdiPortADdc; : m_cfg->DdiPortBDdc = config->DdiPortBDdc; : m_cfg->DdiPortCDdc = config->DdiPortCDdc; : m_cfg->DdiPort1Ddc = config->DdiPort1Ddc; : m_cfg->DdiPort2Ddc = config->DdiPort2Ddc; : m_cfg->DdiPort3Ddc = config->DdiPort3Ddc; : m_cfg->DdiPort4Ddc = config->DdiPort4Ddc; I think these settings only configure some GPIOs. If we configure all GPIOs in coreboot, these should be bypassed so that FSP doesn't override what has been set in coreboot.