Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41965 )
Change subject: soc/mediatek/mt8183: Enable CA perbit mechanism ......................................................................
Patch Set 6:
(30 comments)
https://review.coreboot.org/c/coreboot/+/41965/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41965/6//COMMIT_MSG@10 PS6, Line 10: , need . Need to
https://review.coreboot.org/c/coreboot/+/41965/6//COMMIT_MSG@10 PS6, Line 10: too is too
https://review.coreboot.org/c/coreboot/+/41965/6//COMMIT_MSG@11 PS6, Line 11: risk risks
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 14: cke_fix_onoff This function seems to be a duplicate of dramc_cke_fix_onoff().
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 241: u8 bool?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 243: int 'u8' for consistency with other code
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 271: SET32_BITFIELDS(&ch[0].phy.pll2, : PLL2_RG_RCLRPLL_EN, 1); Fit into one line?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 278: SET32_BITFIELDS(&ch[0].phy.pll1, : PLL1_RG_RPHYPLL_EN, 1); Fit into one line?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 295: DRAMC_DPY_CLK_SW_CON_SC_DMDRAMCSHU_ACK Please align this with '&mtk_spm'
while ((READ32_BITFIELD(&mtk_spm->dramc_dpy_clk_sw_con, DRAMC_DPY_CLK_SW_CON_SC_DMDRAMCSHU_ACK) & shu_ack) != shu_ack) {
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 300: dramc_dbg("SHUFFLE End\n"); Do we really need this? This seems to be duplicate of
dramc_dbg("Shuffle flow complete\n");
below
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 320: cbt_switch_freq If this is only used in dramc_pi_calibration_api.c, why not declare and implement there as a static function?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 265: (u8 *) No need to cast.
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 265: u8 const u8
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 295: dramc_mode_reg_write_by_rank(chn, rank, 13, MR13Value); I think the MR13 values should have been correctly initialized in dramc_mode_reg_init() in dramc_init_setting.c, so that there's no need to do it again here. Could you confirm?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 296: MR13Value Same. This should have been initialized in dramc_mode_reg_init().
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 317: u8 fix_dqien = 0; : : fix_dqien = (cbt_on == 1) ? 3 : 0; u8 fix_dqien = (cbt_on == 1) ? 3 : 0;
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 360: u8 bool
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 362: is_final == 0 !is_final
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 386: size_t u8
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 393: perbit_dly[0] = params->cbt_ca_perbit_delay[chn][rk][0]; : perbit_dly[1] = params->cbt_ca_perbit_delay[chn][rk][1]; : perbit_dly[2] = params->cbt_ca_perbit_delay[chn][rk][2]; : perbit_dly[3] = params->cbt_ca_perbit_delay[chn][rk][3]; : perbit_dly[4] = params->cbt_ca_perbit_delay[chn][rk][4]; : perbit_dly[5] = params->cbt_ca_perbit_delay[chn][rk][5]; Use a for loop?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 411: u1VrefLevel Lower case
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 433: ca_center_diff This is all zeros. Why do we need this? In what situation will this contain non-zeros?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 447: size_t int
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 471: size_t u8
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 471: rank+1 rank + 1
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 496: size_t int
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/dra... PS6, Line 2974: u8 Please align with 'const struct sdram_params *pams,'.
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/inc... PS6, Line 30: CATRAINING CA_TRAINING?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/inc... PS6, Line 122: set_MRR_pinmux_mapping Take the chance to rename it to set_mrr_pinmux_mapping?
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_register.h:
https://review.coreboot.org/c/coreboot/+/41965/6/src/soc/mediatek/mt8183/inc... PS6, Line 955: SPM_POWER_ON_VAL0_SC_PHYPLL_SHU_EN_PCM SPM_POWER_ON_VAL0_SC_PHYPLL1_SHU_EN_PCM to be consistent with 'ch[chn].phy.pll1' naming?