Xi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44709 )
Change subject: soc/mediatek/mt8192: Add dramc 8 phase calibration ......................................................................
Patch Set 44:
(12 comments)
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 252: u8 *dqs_level
Since the calculated value of dqs_level isn't used in dramc_8_phase_cal_set_best_dly(), we can decla […]
Yes, you are very right! Use local variable instead of a pointer.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 254: u8 early_break_cnt = 5;
const
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 257: static s16 err_code = 0x7fff, err_code_min = 0x7fff;
No need to make them static. […]
"err_code" remove static modifier, "err_code_min" is resued at line:323, so keep static modifier.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 258: u16 dqs_dly = 0, jm_dly_start = 0, jm_dly_end = 512, jm_dly_step = 1;
No need to initialize dqs_dly. Add 'const' modifier for the other 3 variables.
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 259: ones_cnt[DQS_NUMBER]
ones_cnt[1] is never used.
Right! use int instead of int array.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 286: MISC_DUTY_TOGGLE_CNT_TOGGLE_CNT
Please align with "&ch[0]". Same below.
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 290: (
No need for parentheses.
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 313: =
One space after "="
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 315: if (r_tmp > p_tmp) : err_code = r_tmp - p_tmp; : else : err_code = p_tmp - r_tmp;
err_code = ABS(p_tmp - r_tmp);
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 345: (
No need for parentheses.
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 347: is fail (to Default)!
failed; falling back to default
Ack
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 349: )
Add {}
Ack