Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44715 )
Change subject: soc/mediatek/mt8192: Implement dram all channel calibration ......................................................................
Patch Set 41:
(21 comments)
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4034: reg_transfer Align with 'const struct'
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4043: ( No parentheses
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4045: ( No parentheses
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4065: {&ch[chn].ao.shu_rk[rk].shurk_selph_dq3, 0} Starts with a new line.
reg_transfer ui_regs[] = { {...}, {...}, };
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4484: &ch[chn].ao Define a local pointer for this.
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4495: dqs_oen_final = (dqs_oen_delay[0] > dqs_oen_delay[1]) ? : dqs_oen_delay[1] : dqs_oen_delay[0]; : dqs_oen_final += 1; dqs_oen_final = MIN(dqs_oen_delay[0], dqs_oen_delay[1]) + 1;
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4524: dq_oen_final = (dq_oen_delay[0] > dq_oen_delay[1]) ? : dq_oen_delay[1] : dq_oen_delay[0]; : dq_oen_final += 1; Same
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4546: dram_freq_grp const
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4571: 12 Why 12? Isn't jump_ratio_index guaranteed to be <= DRAM_DFS_SHU_MAX?
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4577: src_freq != 400 Can we write (freq_group != DDRFREQ_400)?
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4583: Jump_RATIO "jump_ratio" or "Jump ratio"
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4583: \t Avoid using tabs in logs.
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4583: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4583: d u
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4603: 9 How many of them are expected here? 10 or 12?
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 4709: DramC Write-DBI %s dramc_write_dbi: %s
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 11: u8 div_shift = 0; : s8 ui_move = 0; No need for initialization.
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 15: (s8) Any reason for the cast?
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 261: dbi_mode const
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 279: DBI Lowercase
https://review.coreboot.org/c/coreboot/+/44715/41/src/soc/mediatek/mt8192/dr... PS41, Line 378: frequency %d calibration finish Calibration of frequency %u finished