Paul Menzel 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)
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 never be stored in flash?
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.
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.
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. `result` or something similar would be less confusing.
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 21: i == 0 Please use CB_SUCCESS and friends.
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.
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.
https://review.coreboot.org/c/coreboot/+/44569/13/src/soc/mediatek/mt8192/me... PS13, Line 87: DRAM-K What does the *K* mean?