Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34988 )
Change subject: mediatek/mt8183: Implement the dramc init setting ......................................................................
Patch Set 10:
(14 comments)
Gave this patch a quick read, got some suggestions to improve it a bit
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
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)
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
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:
for (u8 dqs = 0; dqs < DQS_NUMBER; dqs++) {
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
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?
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. Is the DDR3200 case intentionally missing?
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. Would it be possible to use an 'struct ACTime' instead?
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:
root = (freq_group == LP4X_DDR3600) ? 1 : 0;
Or even just a direct assignment, as boolean values are either 0 or 1:
root = (freq_group == LP4X_DDR3600);
If that doesn't look clear enough, two negations can be added to let the reader know this will be either 0 or 1, and that is intentional. Should not be needed though.
root = !!(freq_group == LP4X_DDR3600);
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 119: 0x2
Aren't they register values, just like how you constructed SELPH_DQS0 and SELPH_DQS1 ?
IMHO, they do look like 4-bit register values. Otherwise, I guess one would be writing individual bits.
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 121: 0x1 DQS_OEN_DELAY_0P5T ?
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 129: 0x3 DQS_DELAY_2T ?
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 139: 0x4 Shouldn't this be 'DQS_DELAY_0P5T' ?
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 141: 0x3 And this, 'DQS_OEN_DELAY_2T' ?