Xi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44700 )
Change subject: soc/mediatek/mt8192: Do dramc init settings ......................................................................
Patch Set 37:
(14 comments)
https://review.coreboot.org/c/coreboot/+/44700/10/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44700/10/src/soc/mediatek/mt8192/dr... PS10, Line 5: #include <soc/mt6359p.h>
@Xi, as discussed, please use regulator API
Done
https://review.coreboot.org/c/coreboot/+/44700/10/src/soc/mediatek/mt8192/dr... PS10, Line 12: mt6359p_buck_set_voltage
ditto
Done
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.
Ack
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 111: :
One space after ":".
Ack
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 121: :
One space after ":".
Ack
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?
8192 may be configured as max freq DDRFREQ_1600, this function returns the max freq that is configured not the HW max spec.
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 133: :
Same.
Ack
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?
Ack
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 153: :
One space after ":"
Ack
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)
Ack
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 153: (
One space before "("
Ack
https://review.coreboot.org/c/coreboot/+/44700/28/src/soc/mediatek/mt8192/dr... PS28, Line 162: {
No need for parentheses.
Ack
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".
Ack
https://review.coreboot.org/c/coreboot/+/44700/34/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_utility.c:
https://review.coreboot.org/c/coreboot/+/44700/34/src/soc/mediatek/mt8192/dr... PS34, Line 122: 800
Write […]
Ack