Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44700 )
Change subject: soc/mediatek/mt8192: Do dramc init settings ......................................................................
Patch Set 28:
(11 comments)
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_utility.c:
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 17: /* frequency freq_group div_mode shuffle_saved vref_cali vcore*/ Given that the struct definition is few lines above, I don't think we need this comment.
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 111: : One space after ":".
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 121: : One space after ":".
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 125: get_highest_freq_group Doesn't this always return DDRFREQ_2133?
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 133: : Same.
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 141: freq_shuffle_table[k_shu] Declare a local variable for this?
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 153: : One space after ":"
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 153: ( One space before "("
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 153: dramc_dbg("cali data(size:%ld) use fsp:%d, freq_group:%d, div_mode:%d, shu:%d, vref_cali:%d, odt_onoff:%d, vcore:%d\n", Line too long (> 96 chars)
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 162: { No need for parentheses.
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 164: MISC_STATUSA_REFRESH_QUEUE_CNT Please align with "READ32_BITFIELD".