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 46:
(14 comments)
https://review.coreboot.org/c/coreboot/+/44709/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44709/6//COMMIT_MSG@8 PS6, Line 8:
Please elaborate.
Moved to PS46.
https://review.coreboot.org/c/coreboot/+/44709/46//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44709/46//COMMIT_MSG@8 PS46, Line 8: From Paul's comment: Please elaborate.
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
Yes, you are very right! Use local variable instead of a pointer.
Done
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 259: ones_cnt[DQS_NUMBER]
Right! use int instead of int array.
Done
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 358: = 0xff
If ph_dly_final isn't initialized, the compile will tip may-be-uninitialized if not call "dramc_8_ph […]
Similar to ph_dly_loop_break, I'd prefer doing initialization within dramc_8_phase_cal_find_best_dly(). I think the warning should disappear.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 359: = 0
Yes, it's an output argument, but dramc_8_phase_cal_find_best_dly(... […]
Yes, that's what I mean.
https://review.coreboot.org/c/coreboot/+/44709/43/src/soc/mediatek/mt8192/dr... PS43, Line 391: dqs_level
move inside the function dramc_8_phase_cal_find_best_dly.
Ack
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 257: static static const
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 261: 0xff Since it's used multiple times, please define a macro for this. Maybe
#define DQS_LEVEL_UNKNOWN 0xff
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 263: d u
Same for *all* below.
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 328: else if (err_code >= err_code_min) Just "else"
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 331: 1 Does this mean success or failure?
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 341: dramc_dbg("phase_sm err!\n"); Since this is theoretically impossible to happen, how about
die("Invalid phase_sm: %u!\n", phase_sm);
https://review.coreboot.org/c/coreboot/+/44709/46/src/soc/mediatek/mt8192/dr... PS46, Line 402:
trailing whitespace
Please fix this.