Paul Menzel 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:
(13 comments)
This is a huge diffstat. Thank you. Please document the implementation in comments and/or the commit message.
To reduce the indentation level, several code blocks should be factored out into functions.
Lastly, measurements should be done using the stopwatch framework.
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:
… for faster calibration
or
Use cached calibration for faster boot
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@9 PS12, Line 9: , if . If
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).
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@10 PS12, Line 10: do for
https://review.coreboot.org/c/coreboot/+/35164/12//COMMIT_MSG@11 PS12, Line 11: Please explain the implementation. What is the shuffling about?
Please give specific numbers how much the time is reduced.
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
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`?
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.)
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?
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/dr... PS12, Line 1617: for (u8 byte = 0; byte < DQS_NUMBER; byte++) { Can’t you factor this out into a dedicated function?
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.
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.
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/em... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/35164/12/src/soc/mediatek/mt8183/em... PS12, Line 71: return false; Use ternary operator? Or even better, directly return:
return (params->calibration_result_start == PARAM_START_PATTERN && params->calibration_result_end == PARAM_END_PATTERN)