Attention is currently required from: CK HU, Yu-Ping Wu, Yidi Lin. Xi Chen 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 55:
(22 comments)
File src/soc/mediatek/mt8192/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/44715/comment/27f1d944_1533eef6 PS41, Line 4034: reg_transfer
Align with 'const struct'
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/3c70c2cb_199bedda PS41, Line 4043: (
No parentheses
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/1e570f25_a21913e0 PS41, Line 4045: (
No parentheses
(~(1 << div_shift)) --> ~(1 << div_shift)
https://review.coreboot.org/c/coreboot/+/44715/comment/2df81929_fbd6c9a5 PS41, Line 4065: {&ch[chn].ao.shu_rk[rk].shurk_selph_dq3, 0}
Starts with a new line. […]
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/29b4e3fc_ed06f4d5 PS41, Line 4076: u8
int
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/c5c373cf_b62a637f PS41, Line 4484: &ch[chn].ao
Define a local pointer for this.
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/a4c0d149_b829ba6b 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;
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/4fa29191_78d0a3b8 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
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/4e9d6e6b_e8d53827 PS41, Line 4546: dram_freq_grp
const
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/e3a10b26_0e268edc PS41, Line 4571: 12
Why 12? Isn't jump_ratio_index guaranteed to be <= DRAM_DFS_SHU_MAX?
Yes, right. Confirmed with design, 12 is too big, can set to DRAM_DFS_SHU_MAX now.
https://review.coreboot.org/c/coreboot/+/44715/comment/52be6319_0ece4ce3 PS41, Line 4577: src_freq != 400
Can we write (freq_group != DDRFREQ_400)?
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/4ed12666_8850b61f PS41, Line 4583: 0x%x
%#x
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/ccb82521_4fe61a52 PS41, Line 4583: \t
Avoid using tabs in logs.
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/13ce4aaa_2c86e243 PS41, Line 4583: Jump_RATIO
"jump_ratio" or "Jump ratio"
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/b081d111_b7d3f9d9 PS41, Line 4583: d
u
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/2cd17ec5_42477420 PS41, Line 4603: 9
How many of them are expected here? 10 or 12?
DRAM_DFS_SHU_MAX. will remove the un-used jump ratio.
https://review.coreboot.org/c/coreboot/+/44715/comment/7a6b3c7e_d0407236 PS41, Line 4709: DramC Write-DBI %s
dramc_write_dbi: %s
Write-DBI is one feature, we'd prefer to use "Dramc Write-DBI: %s" ?
File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44715/comment/b0da9093_5582a9f5 PS41, Line 11: u8 div_shift = 0; : s8 ui_move = 0;
No need for initialization.
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/9926438b_51ce5d8d PS41, Line 15: (s8)
Any reason for the cast?
no need, can remove it.
https://review.coreboot.org/c/coreboot/+/44715/comment/26019790_7b03f577 PS41, Line 261: dbi_mode
const
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/ac683cf9_eb988c57 PS41, Line 279: DBI
Lowercase
Ack
https://review.coreboot.org/c/coreboot/+/44715/comment/d890f24d_62a4e0ac PS41, Line 378: frequency %d calibration finish
Calibration of frequency %u finished
Calibration of data rate %u finished get_frequency(&cali) * 2