Huayang Duan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34988 )
Change subject: mediatek/mt8183: Implement the dramc init setting ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 592: }
This brace should go one tab to the left, since it is opened on line 582: […]
Done
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 602: if (freq_group == LP4X_DDR2400) { : clkDelay[CHANNEL_A] = clkDelay[CHANNEL_B] = 0; : dqsDelay[CHANNEL_A][0] = 0; : dqsDelay[CHANNEL_A][1] = -2; : dqsDelay[CHANNEL_B][0] = 0; : dqsDelay[CHANNEL_B][1] = -2; : } else if (freq_group == LP4X_DDR3200) { : clkDelay[CHANNEL_A] = clkDelay[CHANNEL_B] = 1; : dqsDelay[CHANNEL_A][0] = 1; : dqsDelay[CHANNEL_A][1] = -2; : dqsDelay[CHANNEL_B][0] = 1; : dqsDelay[CHANNEL_B][1] = -2; : } else if (freq_group == LP4X_DDR3600) { : clkDelay[CHANNEL_A] = 2; : clkDelay[CHANNEL_B] = 1; : dqsDelay[CHANNEL_A][0] = 0; : dqsDelay[CHANNEL_A][1] = 0; : dqsDelay[CHANNEL_B][0] = -1; : dqsDelay[CHANNEL_B][1] = 0; : } else { : clkDelay[CHANNEL_A] = 2; : clkDelay[CHANNEL_B] = 1; : dqsDelay[CHANNEL_A][0] = 0; : dqsDelay[CHANNEL_A][1] = 0; : dqsDelay[CHANNEL_B][0] = -1; : dqsDelay[CHANNEL_B][1] = 0; : }
This couls also be a 'switch' statement
Done
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 679: u8 MR01Value[FSP_MAX] = {0x26, 0x56}; : u8 MR13Value = (1 << 4) | (1 << 3);
Is this defined outside a function for a particular reason?
MR01Value and MR13Value will used at other function for dynamic adjust some settings. MR01Value used at rx dqs gating. MR13Value used at set mr13 vrcg to normal function.
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 1343: : if (freq_group == LP4X_DDR1600) : dramc_setting_DDR1600(); : else if (freq_group == LP4X_DDR2400) : dramc_setting_DDR2400(); : else if (freq_group == LP4X_DDR3600) : dramc_setting_DDR3600();
This can also be a 'switch' statement. […]
the default setting is designed for 3200 case, for other frequency need more settings to overwrite default settings, so no have 3200 case.
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 1576: u8 dqsinctl, datlat, new_datlat, trcd, trrd, twr, twtr, trc, tras; : u8 trp, trpab, tfaw, trtw_ODT_on, trtp, txp, refcnt; : u8 trfc, trfcpb, tzqcs, refcnt_fr_clk, txrefcnt, tmrr2w_ODT_on; : u8 twtpd, trtpd, xrtw2w, xrtw2r, xrtr2w, xrtr2r, twtr_05T; : u8 trtw_ODT_on_05T, twtpd_05T, trtpd_05T, tfaw_05T, trrd_05T; : u8 twr_05T, tras_05T, trpab_05T, trp_05T, trcd_05T, trtp_05T; : u8 txp_05T, trfc_05T, trfcpb_05T, trc_05T, r_dmcatrain_intv; : u8 r_dmmrw_intv, r_dmfspchg_prdcnt, ckeprd, ckelckcnt, zqlat2;
That's a lot of variables. […]
Done
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 1639: if (freq_group == LP4X_DDR3600) : root = 1; : else : root = 0;
Maybe use a ternary (Elvis) operator: […]
Done