Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1378: * dramc init using dramc_setting_DDRxxx() to overwrite the default settings.
Done
Second part of the string is not done:
For other frequencies dramc init uses dramc_setting_DDRxxx() to override the default settings.
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : }
MR13Value is a global value need dynamic update between FSP0 and FSP1, can't pre-init at dramc_mode_ […]
The update of MR13 only depends on freq_group, which is always the same in each call to dramc_cmd_bus_training() from dramc_calibrate_all_channels(). Also, in dramc_calibrate_all_channels() MR13 is never referenced before calling dramc_cmd_bus_training(), so we can move the MR13 update to dramc_calibrate_all_channels() before the for loop without changing the update logic.
We can further move the MR13 update to run_calib(), or equivalently, dramc_init(). Since shared->mr is never referenced before calling dramc_mode_reg_init() in dramc_init(), I still think there's no problem moving the MR13 update to dramc_mode_reg_init(), right before the following line:
mr->MR13Value = MR13Value;