Change in coreboot[master]: soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT...
Hello Duan huayang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to review the following change.
Change subject: soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT MERGE] ......................................................................
soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT MERGE]
Update setting of DRS config.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 23 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/1
diff --git a/src/soc/mediatek/mt8183/dramc_init_setting.c b/src/soc/mediatek/mt8183/dramc_init_setting.c index 9eebfe8..f452a44 100644 --- a/src/soc/mediatek/mt8183/dramc_init_setting.c +++ b/src/soc/mediatek/mt8183/dramc_init_setting.c @@ -1219,8 +1219,8 @@ (0x7 << 0) | (0x7 << 4) | (0x7 << 8) | (0x7 << 12) | (0x7 << 16) | (0x7 << 20) | (0x7 << 24) | (0x7 << 28)); clrbits32(&ch[0].ao.shu[0].selph_ca5, 0x7 << 8); - clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0); - clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1); + clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0_3200); + clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1_3200);
for (size_t rank = 0; rank < 2; rank++) { clrsetbits32(&ch[0].ao.shu[0].rk[rank].selph_dq[0], diff --git a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c index 024c039..ae14480 100644 --- a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c +++ b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c @@ -221,9 +221,7 @@ { dramc_auto_refresh_switch(chn, false);
- if (rank == RANK_0 && (freq_group == LP4X_DDR3600 || - freq_group == LP4X_DDR1600 || - freq_group == LP4X_DDR2400)) + if (rank == RANK_0) write_leveling_move_dqs_instead_of_clk(chn);
SET32_BITFIELDS(&ch[chn].phy.shu[0].rk[rank].ca_cmd[9], @@ -245,9 +243,11 @@ }
static void dramc_cmd_bus_training(u8 chn, u8 rank, u8 freq_group, - const struct sdram_params *params, const bool fast_calib) + const struct sdram_params *params, const bool fast_calib, + struct mr_value *mr) { u32 final_vref, clk_dly, cmd_dly, cs_dly; + u8 fsp = get_freq_fsq(freq_group);
clk_dly = params->cbt_clk_dly[chn][rank]; cmd_dly = params->cbt_cmd_dly[chn][rank]; @@ -267,7 +267,15 @@ SHU1_R0_CA_CMD9_RG_RK0_ARPI_CS, cs_dly);
/* CBT set vref */ - dramc_mode_reg_write_by_rank(chn, rank, 12, final_vref); + if (fsp == FSP_0) { + mr->MR13Value &= ~(1<<6); + mr->MR13Value &= 0x7f; + } else { + mr->MR13Value |= (1<<6); + mr->MR13Value |= 0x80; + } + dramc_mode_reg_write_by_rank(chn, rank, 13, mr->MR13Value); + dramc_mode_reg_write_by_rank(chn, rank, 12, final_vref | ( 0x1 << 6)); }
static void dramc_read_dbi_onoff(size_t chn, bool on) @@ -1823,7 +1831,7 @@ vref_end = vref_begin + 1; dramc_dbg("bypass RX vref: %d\n", vref_begin); } else if (type == TX_WIN_DQ_ONLY) { - vref_begin = params->tx_vref[chn][rank]; + vref_begin = params->tx_vref[chn][rank] | (vref_range << 6) ; vref_end = vref_begin + 1; dramc_dbg("bypass TX vref: %d\n", vref_begin); } @@ -2678,7 +2686,7 @@ }
int dramc_calibrate_all_channels(const struct sdram_params *pams, u8 freq_group, - const struct mr_value *mr) + struct mr_value *mr) { bool fast_calib; switch (pams->source) { @@ -2702,7 +2710,7 @@ dramc_dbg("Start K: freq=%d, ch=%d, rank=%d\n", freq_group, chn, rk); dramc_cmd_bus_training(chn, rk, freq_group, pams, - fast_calib); + fast_calib, mr); dramc_write_leveling(chn, rk, freq_group, pams->wr_level); dramc_auto_refresh_switch(chn, true);
diff --git a/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h b/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h index 19d92b5..e2ca81b 100644 --- a/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h +++ b/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h @@ -10,8 +10,8 @@
#define dramc_err(_x_...) printk(BIOS_ERR, _x_) #define dramc_show(_x_...) printk(BIOS_INFO, _x_) -#if CONFIG(DEBUG_DRAM) -#define dramc_dbg(_x_...) printk(BIOS_DEBUG, _x_) +#if 1//IS_ENABLED(CONFIG_DEBUG_DRAM) +#define dramc_dbg(_x_...) printk(BIOS_INFO, _x_) #else #define dramc_dbg(_x_...) #endif @@ -75,12 +75,12 @@ DQ_DIV_MASK = BIT(DQ_DIV_SHIFT) - 1, OEN_SHIFT = 16,
- SELPH_DQS0 = _SELPH_DQS_BITS(0x3, 0x3), - SELPH_DQS1 = _SELPH_DQS_BITS(0x4, 0x1), SELPH_DQS0_1600 = _SELPH_DQS_BITS(0x2, 0x1), SELPH_DQS1_1600 = _SELPH_DQS_BITS(0x1, 0x6), SELPH_DQS0_2400 = _SELPH_DQS_BITS(0x3, 0x2), SELPH_DQS1_2400 = _SELPH_DQS_BITS(0x1, 0x6), + SELPH_DQS0_3200 = _SELPH_DQS_BITS(0x3, 0x3), + SELPH_DQS1_3200 = _SELPH_DQS_BITS(0x5, 0x2), SELPH_DQS0_3600 = _SELPH_DQS_BITS(0x4, 0x3), SELPH_DQS1_3600 = _SELPH_DQS_BITS(0x1, 0x6), }; @@ -99,7 +99,7 @@ void dramc_apply_config_before_calibration(u8 freq_group); void dramc_apply_config_after_calibration(const struct mr_value *mr); int dramc_calibrate_all_channels(const struct sdram_params *pams, - u8 freq_group, const struct mr_value *mr); + u8 freq_group, struct mr_value *mr); void dramc_hw_gating_onoff(u8 chn, bool onoff); void dramc_enable_phy_dcm(u8 chn, bool bEn); void dramc_mode_reg_write(u8 chn, u8 mr_idx, u8 value);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT MERGE] ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40525/1/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/1/src/soc/mediatek/mt8183/dra... PS1, Line 278: dramc_mode_reg_write_by_rank(chn, rank, 12, final_vref | ( 0x1 << 6)); space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/40525/1/src/soc/mediatek/mt8183/dra... PS1, Line 1834: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6) ; space prohibited before semicolon
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#2).
Change subject: soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT MERGE] ......................................................................
soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT MERGE]
Update setting of DRS config.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 32 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency should use range 1 [DRAFT] [DONOT MERGE] ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... PS2, Line 227: dramc_dbg("write leveling[%d][%d][%d] = 0x%x\n", chn0, rk,dqs, wr_level[chn0][rk][dqs]); line over 96 characters
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... PS2, Line 227: dramc_dbg("write leveling[%d][%d][%d] = 0x%x\n", chn0, rk,dqs, wr_level[chn0][rk][dqs]); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... PS2, Line 1843: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6) ; space prohibited before semicolon
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#3).
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
soc/mediatek/mt8183: High frequency of DRAM should use term settings
For low frequency of DRAM, should use un-term setting to reduce the power consumption, but for high frequency that max than 2400Mbsp, should enale the term to improve the signal integrity.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 22 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/3
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... PS3, Line 1222: SELPH_DQS0_3200 this is only for 3200? what if we're running with 3600 or lower frequency? should we set the corredsponding values
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40525/1/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/1/src/soc/mediatek/mt8183/dra... PS1, Line 278: dramc_mode_reg_write_by_rank(chn, rank, 12, final_vref | ( 0x1 << 6));
space prohibited after that open parenthesis '('
Ack
https://review.coreboot.org/c/coreboot/+/40525/1/src/soc/mediatek/mt8183/dra... PS1, Line 1834: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6) ;
space prohibited before semicolon
Ack
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... PS2, Line 227: dramc_dbg("write leveling[%d][%d][%d] = 0x%x\n", chn0, rk,dqs, wr_level[chn0][rk][dqs]);
line over 96 characters
Ack
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... PS2, Line 227: dramc_dbg("write leveling[%d][%d][%d] = 0x%x\n", chn0, rk,dqs, wr_level[chn0][rk][dqs]);
space required after that ',' (ctx:VxV)
Ack
https://review.coreboot.org/c/coreboot/+/40525/2/src/soc/mediatek/mt8183/dra... PS2, Line 1843: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6) ;
space prohibited before semicolon
Ack
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#4).
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
soc/mediatek/mt8183: High frequency of DRAM should use term settings
For low frequency of DRAM, should use un-term setting to reduce the power consumption, but for high frequency that max than 2400Mbsp, should enable the term to improve the signal integrity.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 22 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/4
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/4//COMMIT_MSG@10 PS4, Line 10: max is larger
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#5).
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
soc/mediatek/mt8183: High frequency of DRAM should use term settings
For low frequency of DRAM, should use un-term setting to reduce the power consumption, but for high frequency that large than 2400Mbsp, should enable the term to improve the signal integrity.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 22 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/5
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 5:
(7 comments)
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 271: ~(1<<6) ~(1 << 6)
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 271: please use only one space (and lines below).
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 274: (1<<6) 1 << 6
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 278: final_vref | (0x1 << 6) why don't we do this directly so you don't need to do | 1 << 6 every time? e.g.,
final_vref |= 1 << 6; dramc_mode_reg...(..., final_vref); dramc_dbg(...., final_vref, mr->MR13Value);
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 278: (0x1 << 6) 1 << 6
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 279: (0x1 << 6) 1 << 6
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 1835: (vref_range << 6) vref_range << 6
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#6).
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
soc/mediatek/mt8183: High frequency of DRAM should use term settings
For low frequency of DRAM, should use un-term setting to reduce the power consumption, but for high frequency that large than 2400Mbsp, should enable the term to improve the signal integrity.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 23 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/6/src/soc/mediatek/mt8183/dra... PS6, Line 269: final_vref |= 1 <<6; need consistent spacing around '<<' (ctx:WxV)
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#7).
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
soc/mediatek/mt8183: High frequency of DRAM should use term settings
For low frequency of DRAM, should use un-term setting to reduce the power consumption, but for high frequency that large than 2400Mbsp, should enable the term to improve the signal integrity.
BUG=153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 23 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/7
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@10 PS7, Line 10: the Fit into previous line.
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : } Could we initialize the value in dramc_mode_reg_init()? And hence no need to remove the "const" modifier?
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 281: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 1837: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6); Line longer than 80 characters.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@7 PS7, Line 7: High frequency of DRAM should use term settings Use term settings for high DRAM frequency
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@9 PS7, Line 9: should This can be removed.
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should This can be removed also.
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@13 PS7, Line 13: 153614919 b:153614919
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: High frequency of DRAM should use term settings ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@10 PS7, Line 10: that large than larger than
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should enable the term to improve the signal integrity. Is that mentioned in some datasheet?
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 1837: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6);
Line longer than 80 characters.
The current line length limit is 96 characters.
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#8).
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
soc/mediatek/mt8183: Use term settings for high DRAM frequency
For low frequency of DRAM, use un-term setting to reduce power consumption, but for high frequency large than 2400Mbsp, enable the term to improve the signal integrity.
BUG=b:153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 23 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/8
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@7 PS7, Line 7: High frequency of DRAM should use term settings
Use term settings for high DRAM frequency
Done
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@9 PS7, Line 9: should
This can be removed.
Done
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@10 PS7, Line 10: that large than
larger than
Done
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@10 PS7, Line 10: the
Fit into previous line.
Done
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should enable the term to improve the signal integrity.
Is that mentioned in some datasheet?
4.29 Command Bus Training The LPDDR4-SDRAM command bus must be trained before enabling termination for high-frequency operation. LPDDR4 provides an internal VREF(CA) that defaults to a level suitable for un-terminated, low frequency operation, but the VREF(CA) must be trained to achieve suitable receiver voltage margin for terminated, high-frequency operation.
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should
This can be removed also.
Done
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@13 PS7, Line 13: 153614919
b:153614919
Done
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 281: 0x%x
%#x
Done
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 1837: vref_begin = params->tx_vref[chn][rank] | (vref_range << 6);
The current line length limit is 96 characters.
Ack
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... PS3, Line 1222: SELPH_DQS0_3200
this is only for 3200? […]
Hi huayang, I think this we still not answered yet?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(8 comments)
https://review.coreboot.org/c/coreboot/+/40525/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/4//COMMIT_MSG@10 PS4, Line 10: max
is larger
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 271:
please use only one space (and lines below).
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 271: ~(1<<6)
~(1 << 6)
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 274: (1<<6)
1 << 6
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 278: final_vref | (0x1 << 6)
why don't we do this directly so you don't need to do | 1 << 6 every time? e.g., […]
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 278: (0x1 << 6)
1 << 6
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 279: (0x1 << 6)
1 << 6
Ack
https://review.coreboot.org/c/coreboot/+/40525/4/src/soc/mediatek/mt8183/dra... PS4, Line 1835: (vref_range << 6)
vref_range << 6
Ack
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should enable the term to improve the signal integrity.
4.29 Command Bus Training […]
Can you put a reference for where these description come from? Something like
8183 Application Notes 4.29 command Bus Training
(I don't know if that's the right reference -simply giving an example)
And have the reference included in commit message.
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(1 comment)
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should enable the term to improve the signal integrity.
Can you put a reference for where these description come from? Something like […]
this is from JEDEC STANDARD <JESD209-4B.pdf>
4.29 Command Bus Training The LPDDR4-SDRAM command bus must be trained before enabling termination for high-frequency operation. LPDDR4 provides an internal VREF(CA) that defaults to a level suitable for un-terminated, low frequency operation, but the VREF(CA) must be trained to achieve suitable receiver voltage margin for terminated, high-frequency operation.
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... PS3, Line 1222: SELPH_DQS0_3200
Hi huayang, I think this we still not answered yet?
we just use SELPH_DQS0_3200 for default value in dramc_setting() but for other frequency, for example 3600Mbps, it will overwrite this value like this. this flow is same as the DRAM full k blob that be verified.
static void dramc_setting_DDR3600(void) { clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0_3600); clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1_3600); } static void dramc_setting_DDR2400(void) { clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0_2400); clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1_2400); }
dramc_setting() {
clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0_3200); clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1_3200);
switch (freq_group) { case LP4X_DDR1600: dramc_setting_DDR1600(); break; case LP4X_DDR2400: dramc_setting_DDR2400(); break; case LP4X_DDR3200: /* Do nothing */ break; case LP4X_DDR3600: dramc_setting_DDR3600(); break; default: die("Invalid DDR frequency group %u\n", freq_group); return; } }
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/7//COMMIT_MSG@11 PS7, Line 11: should enable the term to improve the signal integrity.
this is from JEDEC STANDARD <JESD209-4B.pdf> […]
Ack
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG@11 PS8, Line 11: . Reference: JEDEC STANDARD <JESD209-4B> 4.29 Command Bus Training
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/3/src/soc/mediatek/mt8183/dra... PS3, Line 1222: SELPH_DQS0_3200
we just use SELPH_DQS0_3200 for default value in dramc_setting() […]
Done
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... PS8, Line 1222: clrsetbits32 Can you add a comment before this line?
/* 3200 is the default value and may be changed in dramc_setting_DDRXXXX calls later */
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... PS8, Line 1222: clrsetbits32
Can you add a comment before this line? […]
the whole function of dramc_setting_DDR1600() and dramc_setting_DDR2400() and dramc_setting_DDR3600 are overwrite default 3200Mbps setting. that is default value set as 3200Mbsp, then if calibration other frequency, only change the difference setting between 3200. the 3600 diff with 3200 only selph_dqs0 and selph_dqs1, so at function of dramc_setting_DDR3600, only have those 2 settings.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... PS8, Line 1222: clrsetbits32
the whole function of […]
You can rephrase to whatever - just make the comment clear that 3200 is just a default and may be changed.
Hello build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#9).
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
soc/mediatek/mt8183: Use term settings for high DRAM frequency
For low frequency of DRAM, use un-term setting to reduce power consumption, but for high frequency large than 2400Mbsp, enable the term to improve the signal integrity.
BUG=b:153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 26 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/9
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/8/src/soc/mediatek/mt8183/dra... PS8, Line 1222: clrsetbits32
You can rephrase to whatever - just make the comment clear that 3200 is just a default and may be ch […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 9: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1378: * dramc init using dramc_setting_DDRxxx() to overwrite the default settings. Please improve the wording and spelling. Maybe:
The default dramc settings were tuned at a frequency of 3200Mbps. For other frequencies darmc init uses dramc_setting_DDRxxx() to override the default settings.
I am not sure about the first meaning though.
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1379: */ Please use the allowed comment styles.
https://doc.coreboot.org/coding_style.html#commenting
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 9: -Code-Review
Hello Hung-Te Lin, build bot (Jenkins), Julius Werner, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#10).
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
soc/mediatek/mt8183: Use term settings for high DRAM frequency
For low frequency of DRAM, use un-term setting to reduce power consumption, but for high frequency large than 2400Mbsp, enable the term to improve the signal integrity.
BUG=b:153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 26 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/10
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1378: * dramc init using dramc_setting_DDRxxx() to overwrite the default settings.
Please improve the wording and spelling. Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1379: */
Please use the allowed comment styles. […]
Done
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG@11 PS8, Line 11: .
Reference: JEDEC STANDARD <JESD209-4B> 4. […]
Done
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : }
Could we initialize the value in dramc_mode_reg_init()? And hence no need to remove the "const" modi […]
MR13Value is a global value need dynamic update between FSP0 and FSP1, can't pre-init at dramc_mode_reg_init();
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG@11 PS8, Line 11: .
Done
Hi huayang, this is not Done - please add that reference to your commit message. Something like:
For low frequency ... but for ... the signal integrity.
Reference: JEDEC STANDARD <JESD209-4B> 4.29 Command Bus Training
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : }
MR13Value is a global value need dynamic update between FSP0 and FSP1, can't pre-init at dramc_mode_ […]
Hi huayang, dramc_mode_reg_init already has a operate_fsp, isn't that the value you're looking for?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Use term settings for high DRAM frequency ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1378: * dramc init using dramc_setting_DDRxxx() to overwrite the default settings.
Done
Second part of the string is not done:
For other frequencies dramc init uses dramc_setting_DDRxxx() to override the default settings.
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : }
MR13Value is a global value need dynamic update between FSP0 and FSP1, can't pre-init at dramc_mode_ […]
The update of MR13 only depends on freq_group, which is always the same in each call to dramc_cmd_bus_training() from dramc_calibrate_all_channels(). Also, in dramc_calibrate_all_channels() MR13 is never referenced before calling dramc_cmd_bus_training(), so we can move the MR13 update to dramc_calibrate_all_channels() before the for loop without changing the update logic.
We can further move the MR13 update to run_calib(), or equivalently, dramc_init(). Since shared->mr is never referenced before calling dramc_mode_reg_init() in dramc_init(), I still think there's no problem moving the MR13 update to dramc_mode_reg_init(), right before the following line:
mr->MR13Value = MR13Value;
Hello Hung-Te Lin, build bot (Jenkins), Julius Werner, Yu-Ping Wu, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#11).
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
soc/mediatek/mt8183: Set CA and DQ vref range to correct value
The CA vref should alway select range[1]. But in fast calibration flow, we missing the range select and casuse the CA vref using the range0 value.
The DQ vref should select corrent range corresponds to current frequency, that is for 1600Mbps, 2400Mbps should select range[1], for 3200Mbps and 3600Mbps should select range[0].
Refer to the 'JESD209-4 - Low Power Double Data Rate 4X(LPDDR4X).pdf', used MR12 to set Vref(CA) levels, used MR14 to set VREF(DQ) levels. MR12 range[0] values from 15.0% to 44.9%, range[1] values from 32.9% to 62.9%, MR14 range[0] and range[1] value same as MR12.
BUG=b:153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 12 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/11
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/8//COMMIT_MSG@11 PS8, Line 11: .
Hi huayang, this is not Done - please add that reference to your commit message. Something like: […]
Done
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/40525/9/src/soc/mediatek/mt8183/dra... PS9, Line 1378: * dramc init using dramc_setting_DDRxxx() to overwrite the default settings.
Second part of the string is not done: […]
Done
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : }
The update of MR13 only depends on freq_group, which is always the same in each call to dramc_cmd_bu […]
yes, you are right, MR13 have been set to correct value at dramc_mode_reg_init(), so not update it again at command bus training.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 11: Code-Review+1
(8 comments)
just a few nits in message.
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: using to use
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: missing missed
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: select selection
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: casuse caused
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@12 PS11, Line 12: corrent correct or current?
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@12 PS11, Line 12: corresponds that corresponds
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@13 PS11, Line 13: should to
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@14 PS11, Line 14: should to
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/40525/7/src/soc/mediatek/mt8183/dra... PS7, Line 272: if (fsp == FSP_0) { : mr->MR13Value &= ~(1 << 6); : mr->MR13Value &= 0x7f; : } else { : mr->MR13Value |= (1 << 6); : mr->MR13Value |= 0x80; : }
yes, you are right, MR13 have been set to correct value at dramc_mode_reg_init(), so not update it a […]
Ack
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 11:
(8 comments)
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: select
selection
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: using
to use
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: casuse
caused
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@10 PS11, Line 10: missing
missed
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@12 PS11, Line 12: corrent
correct or current?
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@12 PS11, Line 12: corresponds
that corresponds
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@13 PS11, Line 13: should
to
Done
https://review.coreboot.org/c/coreboot/+/40525/11//COMMIT_MSG@14 PS11, Line 14: should
to
Done
Hello Hung-Te Lin, build bot (Jenkins), Julius Werner, Yu-Ping Wu, Duan huayang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40525
to look at the new patch set (#12).
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
soc/mediatek/mt8183: Set CA and DQ vref range to correct value
The CA vref should alway select range[1]. But in fast calibration flow, we missed the range selection and caused the CA vref to use the range[0] value.
The DQ vref should select correct range that corresponds to current frequency, that is for 1600Mbps, 2400Mbps to select range[1], for 3200Mbps and 3600Mbps to select range[0].
Refer to the 'JESD209-4 - Low Power Double Data Rate 4X(LPDDR4X).pdf', used MR12 to set Vref(CA) levels, used MR14 to set VREF(DQ) levels. MR12 range[0] values from 15.0% to 44.9%, range[1] values from 32.9% to 62.9%, MR14 range[0] and range[1] values same as MR12.
BUG=b:153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 12 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40525/12
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 12: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 12: Code-Review+1
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
soc/mediatek/mt8183: Set CA and DQ vref range to correct value
The CA vref should alway select range[1]. But in fast calibration flow, we missed the range selection and caused the CA vref to use the range[0] value.
The DQ vref should select correct range that corresponds to current frequency, that is for 1600Mbps, 2400Mbps to select range[1], for 3200Mbps and 3600Mbps to select range[0].
Refer to the 'JESD209-4 - Low Power Double Data Rate 4X(LPDDR4X).pdf', used MR12 to set Vref(CA) levels, used MR14 to set VREF(DQ) levels. MR12 range[0] values from 15.0% to 44.9%, range[1] values from 32.9% to 62.9%, MR14 range[0] and range[1] values same as MR12.
BUG=b:153614919 BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: Ie7680b1bf0c29c946d18e3b27626ce6f31c4216b Signed-off-by: Huayang Duan huayang.duan@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40525 Reviewed-by: Hung-Te Lin hungte@chromium.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/mediatek/mt8183/dramc_init_setting.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h 3 files changed, 12 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Hung-Te Lin: Looks good to me, approved
diff --git a/src/soc/mediatek/mt8183/dramc_init_setting.c b/src/soc/mediatek/mt8183/dramc_init_setting.c index 2440814f..6f7ae377 100644 --- a/src/soc/mediatek/mt8183/dramc_init_setting.c +++ b/src/soc/mediatek/mt8183/dramc_init_setting.c @@ -1218,8 +1218,8 @@ (0x7 << 0) | (0x7 << 4) | (0x7 << 8) | (0x7 << 12) | (0x7 << 16) | (0x7 << 20) | (0x7 << 24) | (0x7 << 28)); clrbits32(&ch[0].ao.shu[0].selph_ca5, 0x7 << 8); - clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0); - clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1); + clrsetbits32(&ch[0].ao.shu[0].selph_dqs0, 0x77777777, SELPH_DQS0_3200); + clrsetbits32(&ch[0].ao.shu[0].selph_dqs1, 0x77777777, SELPH_DQS1_3200);
for (size_t rank = 0; rank < 2; rank++) { clrsetbits32(&ch[0].ao.shu[0].rk[rank].selph_dq[0], @@ -1373,6 +1373,9 @@ setbits32(&ch[0].phy.shu[0].b[1].dq[7], (0x1 << 12) | (0x1 << 13)); clrbits32(&ch[0].ao.shu[0].dqs2dq_tx, 0x1f << 0);
+ /* The default dramc init settings were tuned at frequency of 3200Mbps. + For other frequencies uses dramc_setting_DDRxxx() to overwrite + the default settings. */ switch (freq_group) { case LP4X_DDR1600: dramc_setting_DDR1600(); diff --git a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c index 7242dd3..aac6d17 100644 --- a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c +++ b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c @@ -220,9 +220,7 @@ { dramc_auto_refresh_switch(chn, false);
- if (rank == RANK_0 && (freq_group == LP4X_DDR3600 || - freq_group == LP4X_DDR1600 || - freq_group == LP4X_DDR2400)) + if (rank == RANK_0) write_leveling_move_dqs_instead_of_clk(chn);
SET32_BITFIELDS(&ch[chn].phy.shu[0].rk[rank].ca_cmd[9], @@ -265,8 +263,11 @@ SET32_BITFIELDS(&ch[chn].phy.shu[0].rk[rank].ca_cmd[9], SHU1_R0_CA_CMD9_RG_RK0_ARPI_CS, cs_dly);
+ final_vref |= (1 << 6); + /* CBT set vref */ dramc_mode_reg_write_by_rank(chn, rank, 12, final_vref); + dramc_dbg("final_vref: %#x\n", final_vref); }
static void dramc_read_dbi_onoff(size_t chn, bool on) @@ -1822,7 +1823,7 @@ vref_end = vref_begin + 1; dramc_dbg("bypass RX vref: %d\n", vref_begin); } else if (type == TX_WIN_DQ_ONLY) { - vref_begin = params->tx_vref[chn][rank]; + vref_begin = params->tx_vref[chn][rank] | (vref_range << 6); vref_end = vref_begin + 1; dramc_dbg("bypass TX vref: %d\n", vref_begin); } diff --git a/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h b/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h index 07b50d6..6643360 100644 --- a/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h +++ b/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h @@ -74,12 +74,12 @@ DQ_DIV_MASK = BIT(DQ_DIV_SHIFT) - 1, OEN_SHIFT = 16,
- SELPH_DQS0 = _SELPH_DQS_BITS(0x3, 0x3), - SELPH_DQS1 = _SELPH_DQS_BITS(0x4, 0x1), SELPH_DQS0_1600 = _SELPH_DQS_BITS(0x2, 0x1), SELPH_DQS1_1600 = _SELPH_DQS_BITS(0x1, 0x6), SELPH_DQS0_2400 = _SELPH_DQS_BITS(0x3, 0x2), SELPH_DQS1_2400 = _SELPH_DQS_BITS(0x1, 0x6), + SELPH_DQS0_3200 = _SELPH_DQS_BITS(0x3, 0x3), + SELPH_DQS1_3200 = _SELPH_DQS_BITS(0x5, 0x2), SELPH_DQS0_3600 = _SELPH_DQS_BITS(0x4, 0x3), SELPH_DQS1_3600 = _SELPH_DQS_BITS(0x1, 0x6), };
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40525 )
Change subject: soc/mediatek/mt8183: Set CA and DQ vref range to correct value ......................................................................
Patch Set 13:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3682 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3681 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3680 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3679
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
build bot (Jenkins) (Code Review)
-
Duan huayang (Code Review)
-
huayang duan (Code Review)
-
Hung-Te Lin (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)