Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34332 )
Change subject: mediatek/mt8183: Support DDR frequency 3600Mbps ......................................................................
Patch Set 12:
(9 comments)
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 82: if (u1CA_DLL_Mode[chn] == DLL_MASTER) { : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[0], : (0x1 << 31) | (0x1 << 30) | (0xf << 20) | (0xf << 16) | : (0xf << 12) | (0x1 << 10) | (0x1 << 9) | (0x1 << 4), : (0x0 << 31) | (0x0 << 30) | (0x6 << 20) | (0x9 << 16) | : (0x8 << 12) | (0x1 << 10) | (0x1 << 9) | (0x1 << 4)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[1], : (0x1 << 2) | (0x1 << 0), (0x1 << 2) | (0x0 << 0)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_cmd[6], 0x1 << 7, 0x1 << 7); : } else { : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[0], : (0x1 << 31) | (0x1 << 30) | (0xf << 20) | (0xf << 16) | : (0xf << 12) | (0x1 << 10) | (0x1 << 9) | (0x1 << 4), : (0x1 << 31) | (0x1 << 30) | (0x7 << 20) | (0x7 << 16) | : (0x8 << 12) | (0x1 << 10) | (0x1 << 9) | (0x0 << 4)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[1], : (0x1 << 2) | (0x1 << 0), (0x0 << 2) | (0x1 << 0)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_cmd[6], 0x1 << 7, 0x0 << 7); : } it's really not easy to figure out the difference in such long list. I'd recommend setting them to different vars, for example
int is_dll_master = (u1CA_DLL_Mode[chn] == DLL_MASTER);
clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[0], 01x << 31 | 0x1 << 30 | 0xf << 20 | 0xf << 16 | 0xf << 12 | 0x1 << 10 | 0x1 << 9 | 0x1 << 4, !is_master << 31 | !is_master << 30 | (is_master ? 7 : 6) << 20 | (is_master ? 9 : 7) << 16, ....
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 248: (0x1 << 4 no need to quote as a simple arg, i.e.
setbits_le32(..., 0x1 << 4);
Same to all similar calls.
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 265: delat do you want to say delta?
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 268: else so for other groups, delat = 0?
I think you can add en else to make it clear.
else delat = 0;
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 271: ((freq_group == LP4X_DDR3200) || (freq_group == LP4X_DDR3600)) No need to quote.
if (a == b || c == d)
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 309: ((freq == LP4X_DDR1600) || (freq == LP4X_DDR2400)) ? FSP_0 : FSP_1; if (freq == ... || freq == ...) return FSP_0; else return FSP_1;
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 651: #define TIME_OUT_CNT 100 //100us since only one function use it, put this into dramc_zq_calibration as
const u32 TIMEOUT_COUNT = 100;
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 672: do { : resp = (read32(&ch[chn].nao.spcmdresp) >> 4) & 0x1; : u4TimeCnt--; : udelay(1); : } while ((resp == 0) && (u4TimeCnt > 0)); : : if (u4TimeCnt == 0) { : dramc_dbg("ZQCAL Start fail (time out)\n"); : return 1; : } use wait_us macro?
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 688: do { : resp = (read32(&ch[chn].nao.spcmdresp) >> 6) & 0x1; : u4TimeCnt--; : udelay(1); : } while ((resp == 0) && (u4TimeCnt > 0)); : : if (u4TimeCnt == 0) { : dramc_dbg("ZQCAL Latch fail (time out)\n"); : return 1; : } use wait_us macro?