Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35017 )
Change subject: mediatek/mt8183: Set DRAM voltage for each DRAM frequency ......................................................................
Patch Set 11:
(4 comments)
Note that you're not supposed to give yourself Code-Review+2 (and it doesn't work, anyway).
https://review.coreboot.org/c/coreboot/+/35017/11/src/soc/mediatek/mt8183/em... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/35017/11/src/soc/mediatek/mt8183/em... PS11, Line 51: enum { Rather than this and a raw uint array, please define a struct with three members.
https://review.coreboot.org/c/coreboot/+/35017/11/src/soc/mediatek/mt8183/em... PS11, Line 59: 1125000, 600000 If the latter two values are always constant, do they really need to be in here?
https://review.coreboot.org/c/coreboot/+/35017/11/src/soc/mediatek/mt8183/em... PS11, Line 400: #if DUAL_FREQ_K This seems to be like a very complicated way to write
{ if (DUAL_FREQ_K) { set_dram_voltabe_by_freq(LP4X_LOW); ...
set_dram_voltage_by_freq(LP4X_MIDDLE); ... }
set_dram_voltage_by_freq(LP4X_HIGH); ... }
https://review.coreboot.org/c/coreboot/+/35017/11/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/35017/11/src/soc/mediatek/mt8183/in... PS11, Line 41: #define DUAL_FREQ_K 0 What is this? If you want configurable code paths, use Kconfig.
-
Julius Werner (Code Review)