Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34988 )
Change subject: mediatek/mt8183: Implement the dramc init setting ......................................................................
Patch Set 12:
(11 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 50: if (freq_group == LP4X_DDR1600) { : mid_cap_sel = 0x0; : cap_sel = 0x3; : } else if (freq_group == LP4X_DDR2400) { : mid_cap_sel = 0x3; : cap_sel = 0x0; : } else if (freq_group == LP4X_DDR3200) { : mid_cap_sel = 0x2; : cap_sel = 0x0; : } else if (freq_group == LP4X_DDR3600) { : mid_cap_sel = 0x1; : cap_sel = 0x0; : }
I would suggest using a 'switch' statement here
Done
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 150: if (freq_group == LP4X_DDR3200 || freq_group == LP4X_DDR1600) : sdm_pcw = 0x7b00; : else if (freq_group == LP4X_DDR2400) : sdm_pcw = 0x5c00; : else if (freq_group == LP4X_DDR3600) : sdm_pcw = 0x8a00;
This could be done at the beginning of the function (where my previous comment is)
Done
https://review.coreboot.org/c/coreboot/+/34988/9/src/soc/mediatek/mt8183/dra... PS9, Line 262: : if (freq_group == LP4X_DDR3600) : delta = 0xD96; : else if (freq_group == LP4X_DDR3200) : delta = 0xC03; : else : delta = 0;
Same as before
Done
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 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
https://review.coreboot.org/c/coreboot/+/34988/11/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34988/11/src/soc/mediatek/mt8183/dr... PS11, Line 54: break;
Wrong indentation.
Done
https://review.coreboot.org/c/coreboot/+/34988/11/src/soc/mediatek/mt8183/dr... PS11, Line 615: break;
Wrong indentation.
Done
https://review.coreboot.org/c/coreboot/+/34988/11/src/soc/mediatek/mt8183/dr... PS11, Line 1387: ACTime Should it be lower case?
https://review.coreboot.org/c/coreboot/+/34988/11/src/soc/mediatek/mt8183/dr... PS11, Line 1643: root = (freq_group == LP4X_DDR3600) ? 1: 0;
spaces required around that ':' (ctx:VxW)
Done