Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... File src/soc/mediatek/mt8192/Kconfig:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 36: low, : medium and high I thought we have 7 different frequencies in mt8192.
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 43: will do enables
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 23: result != 0
result != CB_SUCCESS
Actually, the return type of complex_mem_test() is int (not enum cb_err), and I only see one instance of CB_SUCCESS in src/mainboard/google. I wonder if we really need to use CB_SUCCESS here, given that returning 0 is already the convention on success.
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 24: printk(BIOS_ERR, "[MEM] DRAM memory test failed: %d\n", result); How about we print only one message regardless of the test result?
int result = complex_mem_test(...); if (result != 0) printk(BIOS_ERR, "[MEM] Complex R/W mem test failed: %d", result); else printk(BIOS_INFO, "[MEM] Complex R/W mem test passed");