Huayang Duan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35164 )
Change subject: mediatek/mt8183: Use calibration result do fast calibration ......................................................................
Patch Set 12:
(12 comments)
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@7 PS12, Line 7: mediatek/mt8183: Use calibration result do fast calibration
*to* looks wrong: […]
Done
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@9 PS12, Line 9: , if
. If
Done
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@10 PS12, Line 10: do
for
Done
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@10 PS12, Line 10: use this calibration result do fast calibration to reduce the bootup time.
Some words fit on the line above (75 characters).
Done
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@11 PS12, Line 11:
Please explain the implementation. What is the shuffling about? […]
Done
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 616: dramc_dbg("bypass duty calibration\n");
… and use cached calibration data
no need add here
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 618: u8
Why u8? Why not `int` or `unsigned int`?
why int better than u8?
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 620: dramc_duty_set_dqs_delay(chn, params->duty_dqs_delay[chn]);
Please measure the times by using the stopwatch framework. (Maybe a separate commit. […]
Done
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 658: die("Invalid DDR frequency group %u\n", freq_group);
Wrong indentation
Done
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 911: dramc_dbg("[bypass Gating]\n");
Why format it like that?
what is the meaning?
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 1759: dramc_dbg("bypass RX vref:%d\n", vref_begin);
Please add a space after the colon.
Done
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 1763: dramc_dbg("bypass TX vref:%d\n", vref_begin);
Ditto.
Done