Xi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44570 )
Change subject: soc/mediatek/mt8192: Do dram full calibration ......................................................................
Patch Set 15:
(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 al […]
Ack.
measure/timings already exists, in CBFS: CBFS log: locating 'fallbak/dram', Found @ offset 2af40 size 1c50d. read ... 12802us
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.
The name "dram" won't modify, i think it can also stay here?
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).
Ack
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 83: return -1;
Please print an error message.
Ack
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 86: return -2;
Please print an error message.
Ack
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.
Ack
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 149: Full Calibration
Starting full calilbration …
Ack
https://review.coreboot.org/c/coreboot/+/44570/11/src/soc/mediatek/mt8192/me... PS11, Line 149: DRAM-K
What does DRAM-K stand for?
stands for: dram calibration.
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.
Ack
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.
Ack