Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
it also get the vendor_id for get the current vendor and print it out after calibration PASS .
It's confusing for a function that returns one information and have another information only in its side effect (print).
Please change these to two functions, say
dramc_get_calibrated_max_density() dramc_get_calibrated_vendor_id()
and you can do this in caller side:
dramc_show("Vendor id is %#x\n", dramc_get_calibrated_vendor_id()); density = dramc_get_calibrated_max_density(); ...
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... PS4, Line 151: const struct sdram_params *sdram_param = get_sdram_config();
the point of sdram_param using at line-217 for fast K
Sorry, what do you mean?
Fast-k returns early at line 175, and it does not need get_sdram_config().
Full-k starts at 191 and it needs get_sdram_config() (for geometry).
Partial-k starts at 217 and also needs whole sdram_param.
So I think we can move this to before line 192, so that fast-k won't need to waste time reading sdram_config.