Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44570 )
Change subject: soc/mediatek/mt8192: Do dram full calibration ......................................................................
Patch Set 11:
(10 comments)
https://review.coreboot.org/c/coreboot/+/44570/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44570/11//COMMIT_MSG@10 PS11, Line 10: Please elaborate, what blob version you used for testing, and paste the newly added messages, and also measure and add timings.
Please also mention and elaborate on the newly addad blob `dram.elf`. How big is it? What does it do?
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/Ma... File src/soc/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/Ma... PS11, Line 41: dram I think, I’d be good to make that name configurable in Kconfig.
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 75: static int dram_run_full_calibration(struct dramc_param *dparam) Please measure the time of the blob execution (add CBMEM timestamps).
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 83: return -1; Please print an error message.
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 86: return -2; Please print an error message.
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 135: printk(BIOS_ERR, "Failed to run fast calibration\n"); This with the new messages below is confusing. The return value should be printed.
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 149: DRAM-K What does DRAM-K stand for?
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 149: Full Calibration Starting full calilbration …
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 153: printk(BIOS_INFO, "Full calibration passed\n"); It should be prefixed with some RAM related term or be more elaborate.
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 160: } If the the different error codes are not considered (-1…-4), they shouldn’t be used.