Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44718 )
Change subject: soc/mediatek/mt8192: Do rx dqs gating training ......................................................................
Patch Set 44:
(37 comments)
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1369: struct const
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1368: static u8 rxdqs_gating_get_tx_dly_min(dram_freq_grp freq_group, : struct rxdqs_gating_best_win *rxdqs_best_win) One argument per line:
static u8 rxdqs_gating_get_tx_dly_min( dram_freq_grp freq_group, struct rxdqs_gating_best_win *rxdqs_best_win)
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1371: = 0 No initialization
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1371: 0xff UINT8_MAX
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1374: d u
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1380: Just one tab
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1387: if (tx_dly_dqs_gated < tx_dly_min) : tx_dly_min = tx_dly_dqs_gated; tx_dly_min = MIN(tx_dly_min, tx_dly_dqs_gated);
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1393: rxdqs_gating_get_tx_dly_max This is almost identical to rxdqs_gating_get_tx_dly_min(). Please merge these 2 functions:
static rxdqs_gating_get_tx_dly(freq_group, *rxdqs_best_win, *tx_dly_min, *tx_dly_max)
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1419: u8 Align with 'const struct'
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1424: = {0} No need to initialize.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1438: size_t int
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1441: pi_per_ui = 32; : ui_per_mck = 16; Add 'const' for these.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1449: MCK Please change to lowercase. Same below.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1461: d u
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1489: size_t int
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1501: P p
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1507: best_coarse_mck[rk][0] = Can we use a for loop?
for (u8 dqs = 0; dqs < DQS_NUMBER; dqs++) { best_coarse_ui[rk][dqs] = ... }
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1522: SHU_RK_B0_DQSIEN_MCK_UI_DLY_DQSIEN_MCK_P0_B0 Shouldn't this be SHU_RK_B1_DQSIEN_MCK_UI_DLY_DQSIEN_UI_P0_B1 (B0 vs B1)?
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1537: P p
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1546: ( No parentheses.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1549: ( No parentheses.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1551: 2T, 0.5T What do 2T and 0.5T mean? Can we use
dly(MCK, UI)
just like in rxdqs_gating_get_tx_dly_min()?
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1555: for Please merge it into the previous for loop. I don't think the order of debugging logs is that important.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1556: P1 Out of curiosity, what does "P" mean here?
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1584: if (best_coarse_mck[RANK_0][dqs] > best_coarse_mck[RANK_1][dqs]) { : rank_sel_mck_p0[dqs] = (best_coarse_mck[RANK_0][dqs] > 0) ? : (best_coarse_mck[RANK_0][dqs] - 1) : 0; : rank_sel_mck_p1[dqs] = (best_coarse_mck_P1[RANK_0][dqs] > 0) ? : (best_coarse_mck_P1[RANK_0][dqs] - 1) : 0; : } else { : rank_sel_mck_p0[dqs] = (best_coarse_mck[RANK_1][dqs] > 0) ? : (best_coarse_mck[RANK_1][dqs] - 1) : 0; : rank_sel_mck_p1[dqs] = (best_coarse_mck_P1[RANK_1][dqs] > 0) ? : (best_coarse_mck_P1[RANK_1][dqs] - 1) : 0; : } Simplify by
if (best_coarse_mck[RANK_0][dqs] > best_coarse_mck[RANK_1][dqs]) best_rk = RANK_0; else best_rk = RANK_1;
then do the calculation with best_k.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1606: tx_dly_max This is only used for printing a debug message?
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1606: u8 Align
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1613: = 0 No need to initialize.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1614: = 0 No need to initialize.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1633: d u
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1634: Just one tab
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1641: MISC_SHU_RK_DQSCTL_DQSINCTL Align with &ch[chn]
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1642: change_dqs_inctl Is it guaranteed that (change_dqs_inctl <= read_dqs_inctl)?
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1650: if (read_dqs_inctl >= 2) : rank_inctl_root = read_dqs_inctl - 2; : else : rank_inctl_root = 0; Write "rank_inctl_root = (...) ? ... : ..." for consistency.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1669: T Can we have a consistent prefix "[RxdqsGatingPostProcess]"? Maybe
"[RxdqsGatingPostProcess] min: %u, max: %u, change_dqs_inctl: %d\n"
Same below.
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1671: RANKINCTL rank_inctl_root
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 276: should enable the auto refresh before do RX and TX calibration Enable auto refresh before RX and TX calibration