Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44569 )
Change subject: soc/mediatek/mt8192: Do dram fast calibration ......................................................................
Patch Set 13:
(8 comments)
A new patch for fix issues: https://review.coreboot.org/c/coreboot/+/46585
https://review.coreboot.org/c/coreboot/+/44569/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44569/13//COMMIT_MSG@10 PS13, Line 10:
So this currently does not work yet, as there is no full RAM init support, so calibration data can n […]
Dram full calibration: https://review.coreboot.org/c/coreboot/+/44570/11
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/Kc... File src/soc/mediatek/mt8192/Kconfig:
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/Kc... PS13, Line 22: This option enables additional DRAM related debug messages.
The commit message misses an explanation, why this defaults to yes.
The option contains many useful dram calibration log which helps developers analyse dram status. So, default to yes.
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/Kc... PS13, Line 40: default y
The commit message misses an explanation, why this defaults to yes.
Ack
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 18: int i
That’s confusing, as i is normal the loop variable. […]
Ack
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 21: i == 0
Please use CB_SUCCESS and friends.
Ack
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 24: printk(BIOS_ERR, "DRAM memory test failed\n");
Please use a common prefix for DRAM related messages. [MEM] would work.
Ack
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 23: if (i != 0) { : printk(BIOS_ERR, "DRAM memory test failed\n"); : return -1; : }
We already print fail above. Maybe print the address in the error message too.
will output the error code.
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 87: DRAM-K
What does the *K* mean?
K means calibration.