Yu-Ping Wu 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 43:
(21 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 declare this as a local variable inside this function:
u8 dqs_level = 0xff;
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 254: u8 early_break_cnt = 5; const
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.
u16 err_code; const u16 err_code_min = 0x7fff;
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.
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.
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.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 290: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 313: = One space after "="
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);
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 345: ( No need for parentheses.
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
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 349: ) Add {}
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 357: u8 dqsien_pi = 0; No need to initialize it.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 358: ph_dly = 0, ph_start = 0, ph_end = 0 Also no need to initialize these.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 358: = 0xff Since ph_dly_final is an output argument of dramc_8_phase_cal_find_best_dly(), there's no need for initialization.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 359: = 0 Same. Since this is an output argument, no need to initialize it.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 379: dramc_dbg("phase_sm err!\n"); Since this indicates a serious bug in this code block, I'd prefer
die("Invalid phase_sm: %u\n", phase_sm); return;
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 383: d u. Same for the others.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 391: dqs_level Should we declare this *inside* the for loop of ph_dly or *outside*?
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 397: ph_dly_loop_break Since it's used as a boolean, declare it as 'int' or 'bool'. Then, we may write
if (ph_dly_loop_break)
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 403: d u