Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34332 )
Change subject: mediatek/mt8183: Support more DRAM frequency bootup ......................................................................
Patch Set 36:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 108: ((cke_on ? 1 : 0) << 6) | ((cke_off ? 1 : 0) << 7)); @hungte Could we write "cke_on << 6" directly? What's the convention in coreboot?
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 149: (~(1 << DQ_DIV_SHIFT))
~(1 << DQ_DIV_SHIRT) […]
Done
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 176: (byte * 4)
byte * 4
Done
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 223: (rank == RANK_0)
rank == RANK_0
Done
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 875: if (freq_group == LP4X_DDR1600) : coarse_start = 18; : else if (freq_group == LP4X_DDR2400) : coarse_start = 25; : else if (freq_group == LP4X_DDR3200) : coarse_start = 25; : else if (freq_group == LP4X_DDR3600) : coarse_start = 21; : else : coarse_start = 26; Change to switch statement.
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 1254: else : *begin = -64; I don't see how this could possibly happen because the above conditions seem to include all the cases.
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 1545: clock_rate = 800; @hungte There are lots of switch statements for freq_group like this. Should we change this to die()? Or it is expected to have more frequency groups in the future?