Duan Huayang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Current the DRAM control only support 4GB DDR, add more large size DDR support to meet the market requirement.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/mainboard/google/kukui/sdram_configs.c M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h 7 files changed, 175 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/1
diff --git a/src/mainboard/google/kukui/sdram_configs.c b/src/mainboard/google/kukui/sdram_configs.c index 73bd2f8..649ab10 100644 --- a/src/mainboard/google/kukui/sdram_configs.c +++ b/src/mainboard/google/kukui/sdram_configs.c @@ -30,3 +30,8 @@
return ¶ms; } + +u32 get_rom_code(void) +{ + return ram_code(); +} diff --git a/src/soc/mediatek/mt8183/dramc_param.c b/src/soc/mediatek/mt8183/dramc_param.c index 54d2209..546ad3a 100644 --- a/src/soc/mediatek/mt8183/dramc_param.c +++ b/src/soc/mediatek/mt8183/dramc_param.c @@ -31,12 +31,37 @@ return validate_dramc_param(blob) == DRAMC_SUCCESS; }
+static void set_ddr_size_type(struct dramc_param *param) +{ + u32 ramcode = get_rom_code(); + u16 ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; + + switch (ramcode) { + case 1: + case 2: + case 3: + case 4: + case 5: + case 6: + case 7: + case 8: + ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; + break; + default: + break; + } + + param->header.ddr_size_type = ddr_size_type; +} + int initialize_dramc_param(void *blob, u16 config) { struct dramc_param *param = blob; struct dramc_param_header *hdr = ¶m->header;
memset(blob, 0, sizeof(*param)); + + set_ddr_size_type(param); hdr->magic = DRAMC_PARAM_HEADER_MAGIC; hdr->size = sizeof(*param); hdr->version = DRAMC_PARAM_HEADER_VERSION; diff --git a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c index b557550..6d5db3a 100644 --- a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c +++ b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c @@ -131,6 +131,19 @@ dramc_dbg("Write MR%d =0x%x\n", mr_idx, value); }
+static u16 dramc_mode_reg_read_by_rank(u8 chn, u8 rank, u8 mr_idx) +{ + u8 value; + u32 rk_bak = READ32_BITFIELD(&ch[chn].ao.mrs, MRS_MRRRK); + + SET32_BITFIELDS(&ch[chn].ao.mrs, MRS_MRRRK, rank); + value = dramc_mode_reg_read(chn, mr_idx); + SET32_BITFIELDS(&ch[chn].ao.mrs, MRS_MRRRK, rk_bak); + + dramc_dbg("Mode reg read rank%d MR%d = %#x\n", rank, mr_idx, value); + return value; +} + static void dramc_mode_reg_write_by_rank(u8 chn, u8 rank, u8 mr_idx, u8 value) { @@ -2678,6 +2691,54 @@ } }
+u8 get_dram_infor_after_cal(void) +{ + u8 vendor_id, density, max_density = 0; + u64 dram_size, max_size = 0; + + vendor_id = dramc_mode_reg_read_by_rank(CHANNEL_A, RANK_0, 5) & 0xff; + dramc_show("Vendor id is %#x\n", vendor_id); + + for (u8 rk = RANK_0; rk < RANK_MAX; rk++) { + density = dramc_mode_reg_read_by_rank(CHANNEL_A, rk, 8) & 0xff; + dramc_dbg("MR8 %#x\n", density); + density = (density >> 2) & 0xf; + + switch (density) { + case 0x0: + dram_size = 0x20000000; + break; + case 0x1: + dram_size = 0x30000000; + break; + case 0x2: + dram_size = 0x40000000; + break; + case 0x3: + dram_size = 0x60000000; + break; + case 0x4: + dram_size = 0x80000000; + break; + case 0x5: + dram_size = 0xc0000000; + break; + case 0x6: + dram_size = 0x100000000L; + break; + default: + dram_size = 0; + } + if (dram_size > max_size) { + max_size = dram_size; + max_density = density; + } + dramc_show("RK%d size %dGb, density:%d\n", rk, (u32)(dram_size >> 27), max_density); + } + + return max_density; +} + int dramc_calibrate_all_channels(const struct sdram_params *pams, u8 freq_group, const struct mr_value *mr) { diff --git a/src/soc/mediatek/mt8183/emi.c b/src/soc/mediatek/mt8183/emi.c index c03f945..6a465e3 100644 --- a/src/soc/mediatek/mt8183/emi.c +++ b/src/soc/mediatek/mt8183/emi.c @@ -50,10 +50,10 @@ };
struct optimize_ac_time { - u8 rfc; - u8 rfc_05t; - u8 rfc_pb; - u8 rfrc_pb05t; + u8 trfc; + u8 trfrc_05t; + u8 trfc_pb; + u8 trfrc_pb_05t; u16 tx_ref_cnt; };
@@ -320,30 +320,79 @@ setbits32(&ch[0].phy.misc_ctrl1, 0x1 << 31); }
-static void dramc_ac_timing_optimize(u8 freq_group) +static void dramc_ac_timing_optimize(u8 freq_group, u8 density) { - struct optimize_ac_time rf_cab_opt[LP4X_DDRFREQ_MAX] = { - [LP4X_DDR1600] = {.rfc = 44, .rfc_05t = 0, .rfc_pb = 16, - .rfrc_pb05t = 0, .tx_ref_cnt = 62}, - [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .rfc_pb = 30, - .rfrc_pb05t = 0, .tx_ref_cnt = 91}, - [LP4X_DDR3200] = {.rfc = 100, .rfc_05t = 0, .rfc_pb = 44, - .rfrc_pb05t = 0, .tx_ref_cnt = 119}, - [LP4X_DDR3600] = {.rfc = 118, .rfc_05t = 1, .rfc_pb = 53, - .rfrc_pb05t = 1, .tx_ref_cnt = 138}, + u8 rfcab_grp = 0; + u8 trfc = 101, trfrc_05t = 0, trfc_pb = 44, trfrc_pb_05t = 0, tx_ref_cnt = 118; + enum tRFCAB{tRFCAB_130 = 0, tRFCAB_180, tRFCAB_280, tRFCAB_380, tRFCAB_NUM}; + + struct optimize_ac_time rf_cab_opt[LP4X_DDRFREQ_MAX][tRFCAB_NUM] = { + /* LP4X_DDR1600 */ + {{.trfc = 14, .trfrc_05t = 0, .trfc_pb = 0, .trfrc_pb_05t = 0, .tx_ref_cnt = 32}, + {.trfc = 24, .trfrc_05t = 0, .trfc_pb = 6, .trfrc_pb_05t = 0, .tx_ref_cnt = 42}, + {.trfc = 44, .trfrc_05t = 0, .trfc_pb = 16, .trfrc_pb_05t = 0, .tx_ref_cnt = 62}, + {.trfc = 64, .trfrc_05t = 0, .trfc_pb = 26, .trfrc_pb_05t = 0, .tx_ref_cnt = 82}}, + /* LP4X_DDR2400 */ + {{.trfc = 27, .trfrc_05t = 0, .trfc_pb = 6, .trfrc_pb_05t = 0, .tx_ref_cnt = 46}, + {.trfc = 42, .trfrc_05t = 0, .trfc_pb = 15, .trfrc_pb_05t = 0, .tx_ref_cnt = 61}, + {.trfc = 72, .trfrc_05t = 0, .trfc_pb = 30, .trfrc_pb_05t = 0, .tx_ref_cnt = 91}, + {.trfc = 102, .trfrc_05t = 0, .trfc_pb = 45, .trfrc_pb_05t = 0, .tx_ref_cnt = 121}}, + /* LP4X_DDR3200 */ + {{.trfc = 40, .trfrc_05t = 0, .trfc_pb = 12, .trfrc_pb_05t = 0, .tx_ref_cnt = 59}, + {.trfc = 60, .trfrc_05t = 0, .trfc_pb = 24, .trfrc_pb_05t = 0, .tx_ref_cnt = 79}, + {.trfc = 100, .trfrc_05t = 0, .trfc_pb = 44, .trfrc_pb_05t = 0, .tx_ref_cnt = 119}, + {.trfc = 140, .trfrc_05t = 0, .trfc_pb = 64, .trfrc_pb_05t = 0, .tx_ref_cnt = 159}}, + /* LP4X_DDR3600 */ + {{.trfc = 48, .trfrc_05t = 1, .trfc_pb = 16, .trfrc_pb_05t = 0, .tx_ref_cnt = 68}, + {.trfc = 72, .trfrc_05t = 0, .trfc_pb = 30, .trfrc_pb_05t = 0, .tx_ref_cnt = 92}, + {.trfc = 118, .trfrc_05t = 1, .trfc_pb = 53, .trfrc_pb_05t = 1, .tx_ref_cnt = 138}, + {.trfc = 165, .trfrc_05t = 0, .trfc_pb = 76, .trfrc_pb_05t = 1, .tx_ref_cnt = 185}}, };
+ switch (density) { + /* 4Gb per die (2Gb per channel) */ + case 0x0: + rfcab_grp = tRFCAB_130; + break; + /* 6Gb per die (3Gb per channel) or 8Gb per die (4Gb per channel) */ + case 0x1: + case 0x2: + rfcab_grp = tRFCAB_180; + break; + /* 12Gb per die (6Gb per channel) or 16Gb per die (8Gb per channel) */ + case 0x3: + case 0x4: + rfcab_grp = tRFCAB_280; + break; + /* 24Gb per die (12Gb per channel) or 32Gb per die (16Gb per channel) */ + case 0x5: + case 0x6: + rfcab_grp = tRFCAB_380; + break; + default: + dramc_dbg("density err!\n"); + break; + } + + trfc = rf_cab_opt[freq_group][rfcab_grp].trfc; + trfrc_05t = rf_cab_opt[freq_group][rfcab_grp].trfrc_05t; + trfc_pb = rf_cab_opt[freq_group][rfcab_grp].trfc_pb; + trfrc_pb_05t = rf_cab_opt[freq_group][rfcab_grp].trfrc_pb_05t; + tx_ref_cnt = rf_cab_opt[freq_group][rfcab_grp].tx_ref_cnt; + dramc_dbg("Density %d, trfc %u, trfrc_05t %d, tx_ref_cnt %d, trfc_pb %d, trfrc_pb_05t %d\n", + density, trfc, trfrc_05t, tx_ref_cnt, trfc_pb, trfrc_pb_05t); + for (size_t chn = 0; chn < CHANNEL_MAX; chn++) { clrsetbits32(&ch[chn].ao.shu[0].actim[3], - 0xff << 16, rf_cab_opt[freq_group].rfc << 16); + 0xff << 16, trfc << 16); clrsetbits32(&ch[chn].ao.shu[0].ac_time_05t, - 0x1 << 2, rf_cab_opt[freq_group].rfc_05t << 2); + 0x1 << 2, trfrc_05t << 2); clrsetbits32(&ch[chn].ao.shu[0].actim[4], - 0x3ff << 0, rf_cab_opt[freq_group].tx_ref_cnt << 0); + 0x3ff << 0, tx_ref_cnt << 0); clrsetbits32(&ch[chn].ao.shu[0].actim[3], - 0xff << 0, rf_cab_opt[freq_group].rfc_pb << 0); + 0xff << 0, trfc_pb << 0); clrsetbits32(&ch[chn].ao.shu[0].ac_time_05t, - 0x1 << 1, rf_cab_opt[freq_group].rfrc_pb05t << 1); + 0x1 << 1, trfrc_pb_05t << 1); } }
@@ -495,6 +544,7 @@ struct dram_shared_data *shared, const int shuffle, bool *first_run) { + u8 density = 0; const u8 *freq_tbl;
if (CONFIG(MT8183_DRAM_EMCP)) @@ -519,7 +569,9 @@ dramc_dbg("Start K (current clock: %u\n", params->frequency); if (dramc_calibrate_all_channels(params, freq_group, &shared->mr) != 0) return -1; - dramc_ac_timing_optimize(freq_group); + + density = get_dram_infor_after_cal(); + dramc_ac_timing_optimize(freq_group, density); dramc_dbg("K finished (current clock: %u\n", params->frequency);
dramc_save_result_to_shuffle(DRAM_DFS_SHUFFLE_1, shuffle); diff --git a/src/soc/mediatek/mt8183/include/soc/dramc_param.h b/src/soc/mediatek/mt8183/include/soc/dramc_param.h index 7b3ba7e..8cadb28 100644 --- a/src/soc/mediatek/mt8183/include/soc/dramc_param.h +++ b/src/soc/mediatek/mt8183/include/soc/dramc_param.h @@ -11,7 +11,7 @@
enum { DRAMC_PARAM_HEADER_MAGIC = 0x44524d4b, - DRAMC_PARAM_HEADER_VERSION = 2, + DRAMC_PARAM_HEADER_VERSION = 3, };
enum DRAMC_PARAM_STATUS_CODES { @@ -38,11 +38,18 @@ DRAMC_FLAG_HAS_SAVED_DATA = 0x0001, };
+enum DRAMC_PARAM_DDR_SIZE_TYPE { + DDR_TYPE_2CH_2RK_4GB_2_2, + DDR_TYPE_2CH_2RK_6GB_3_3, + DDR_TYPE_2CH_2RK_8GB_4_4, +}; + struct dramc_param_header { u32 status; /* DRAMC_PARAM_STATUS_CODES */ u32 magic; - u32 version; - u32 size; /* size of whole dramc_param */ + u32 version; /* DRAMC_PARAM_HEADER_VERSION */ + u32 size; /* size of whole dramc_param */ + u16 ddr_size_type; /* DRAMC_PARAM_DDR_SIZE_TYPE */ u16 config; /* DRAMC_PARAM_CONFIG */ u16 flags; /* DRAMC_PARAM_FLAGS */ u32 checksum; 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 0de86f6..6aa8f0b 100644 --- a/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h +++ b/src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h @@ -107,5 +107,6 @@ u32 get_shu_freq(u8 shu); void dramc_hw_dqsosc(u8 chn); void dramc_dqs_precalculation_preset(void); +u8 get_dram_infor_after_cal(void);
#endif /* _DRAMC_PI_API_MT8183_H */ diff --git a/src/soc/mediatek/mt8183/include/soc/emi.h b/src/soc/mediatek/mt8183/include/soc/emi.h index 3862d54..46921fe 100644 --- a/src/soc/mediatek/mt8183/include/soc/emi.h +++ b/src/soc/mediatek/mt8183/include/soc/emi.h @@ -86,6 +86,7 @@
extern const u8 phy_mapping[CHANNEL_MAX][16];
+u32 get_rom_code(void); int complex_mem_test(u8 *start, unsigned int len); size_t sdram_size(void); const struct sdram_params *get_sdram_config(void);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 1:
(22 comments)
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/dra... PS1, Line 2736: dramc_show("RK%d size %dGb, density:%d\n", rk, (u32)(dram_size >> 27), max_density); line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 327: enum tRFCAB{tRFCAB_130 = 0, tRFCAB_180, tRFCAB_280, tRFCAB_380, tRFCAB_NUM}; missing space after enum definition
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 331: {{.trfc = 14, .trfrc_05t = 0, .trfc_pb = 0, .trfrc_pb_05t = 0, .tx_ref_cnt = 32}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 332: {.trfc = 24, .trfrc_05t = 0, .trfc_pb = 6, .trfrc_pb_05t = 0, .tx_ref_cnt = 42}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 333: {.trfc = 44, .trfrc_05t = 0, .trfc_pb = 16, .trfrc_pb_05t = 0, .tx_ref_cnt = 62}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 334: {.trfc = 64, .trfrc_05t = 0, .trfc_pb = 26, .trfrc_pb_05t = 0, .tx_ref_cnt = 82}}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 334: {.trfc = 64, .trfrc_05t = 0, .trfc_pb = 26, .trfrc_pb_05t = 0, .tx_ref_cnt = 82}}, space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 336: {{.trfc = 27, .trfrc_05t = 0, .trfc_pb = 6, .trfrc_pb_05t = 0, .tx_ref_cnt = 46}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 337: {.trfc = 42, .trfrc_05t = 0, .trfc_pb = 15, .trfrc_pb_05t = 0, .tx_ref_cnt = 61}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 338: {.trfc = 72, .trfrc_05t = 0, .trfc_pb = 30, .trfrc_pb_05t = 0, .tx_ref_cnt = 91}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 339: {.trfc = 102, .trfrc_05t = 0, .trfc_pb = 45, .trfrc_pb_05t = 0, .tx_ref_cnt = 121}}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 339: {.trfc = 102, .trfrc_05t = 0, .trfc_pb = 45, .trfrc_pb_05t = 0, .tx_ref_cnt = 121}}, space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 341: {{.trfc = 40, .trfrc_05t = 0, .trfc_pb = 12, .trfrc_pb_05t = 0, .tx_ref_cnt = 59}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 342: {.trfc = 60, .trfrc_05t = 0, .trfc_pb = 24, .trfrc_pb_05t = 0, .tx_ref_cnt = 79}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 343: {.trfc = 100, .trfrc_05t = 0, .trfc_pb = 44, .trfrc_pb_05t = 0, .tx_ref_cnt = 119}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 344: {.trfc = 140, .trfrc_05t = 0, .trfc_pb = 64, .trfrc_pb_05t = 0, .tx_ref_cnt = 159}}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 344: {.trfc = 140, .trfrc_05t = 0, .trfc_pb = 64, .trfrc_pb_05t = 0, .tx_ref_cnt = 159}}, space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 346: {{.trfc = 48, .trfrc_05t = 1, .trfc_pb = 16, .trfrc_pb_05t = 0, .tx_ref_cnt = 68}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 347: {.trfc = 72, .trfrc_05t = 0, .trfc_pb = 30, .trfrc_pb_05t = 0, .tx_ref_cnt = 92}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 348: {.trfc = 118, .trfrc_05t = 1, .trfc_pb = 53, .trfrc_pb_05t = 1, .tx_ref_cnt = 138}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 349: {.trfc = 165, .trfrc_05t = 0, .trfc_pb = 76, .trfrc_pb_05t = 1, .tx_ref_cnt = 185}}, line over 96 characters
https://review.coreboot.org/c/coreboot/+/41949/1/src/soc/mediatek/mt8183/emi... PS1, Line 349: {.trfc = 165, .trfrc_05t = 0, .trfc_pb = 76, .trfrc_pb_05t = 1, .tx_ref_cnt = 185}}, space required after that close brace '}'
Hello build bot (Jenkins), Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#2).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Current the DRAM control only support 4GB DDR, add more large size DDR support to meet the market requirement.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/mainboard/google/kukui/sdram_configs.c M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h 7 files changed, 192 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 2:
(26 comments)
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 334: .tx_ref_cnt = 42}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 334: .tx_ref_cnt = 42}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 336: .tx_ref_cnt = 62}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 336: .tx_ref_cnt = 62}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 338: .tx_ref_cnt = 82} }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 338: .tx_ref_cnt = 82} }, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 343: .tx_ref_cnt = 61}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 343: .tx_ref_cnt = 61}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 345: .tx_ref_cnt = 91}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 345: .tx_ref_cnt = 91}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 347: .tx_ref_cnt = 121} }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 347: .tx_ref_cnt = 121} }, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 352: .tx_ref_cnt = 79}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 352: .tx_ref_cnt = 79}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 354: .tx_ref_cnt = 119}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 354: .tx_ref_cnt = 119}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 356: .tx_ref_cnt = 159} }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 356: .tx_ref_cnt = 159} }, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 359: .tx_ref_cnt = 68}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 359: .tx_ref_cnt = 68}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 361: .tx_ref_cnt = 92}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 361: .tx_ref_cnt = 92}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 363: .tx_ref_cnt = 138}, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 363: .tx_ref_cnt = 138}, please, no space before tabs
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 365: .tx_ref_cnt = 185} }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/41949/2/src/soc/mediatek/mt8183/emi... PS2, Line 365: .tx_ref_cnt = 185} }, please, no space before tabs
Hello build bot (Jenkins), Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#3).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Current the DRAM control only support 4GB DDR, add more large size DDR support to meet the market requirement.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/mainboard/google/kukui/sdram_configs.c M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h 7 files changed, 192 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/3
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 3:
(3 comments)
I think we want to separate this into two patches.
1. Add DDR size in dramc_config (and sdram_config) 2. Support 4/6/8 GB in MT8183 EMI
In the meantime, I think the right execution path should be:
1. ddr_size_type (or just ddr_size, as 4/6/8 ? will you need channel info or just total size would be good enough?) should be one information in sdram_configs 2. we should load the right sdram_configs using ramcode. 3. kukui board romstage to pass the information when creating dramc_param 4. mt8183/emi takes the param and pass to dramk
BTW, @jwerner did you have DDR size info as input for Trogdor?
https://review.coreboot.org/c/coreboot/+/41949/3/src/mainboard/google/kukui/... File src/mainboard/google/kukui/sdram_configs.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/mainboard/google/kukui/... PS3, Line 34: u32 get_rom_code(void) : { : return ram_code(); : } I don't see the point of duplicating function here...?
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 34: static void set_ddr_size_type(struct dramc_param *param) : { : u32 ramcode = get_rom_code(); : u16 ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : : switch (ramcode) { : case 1: : case 2: : case 3: : case 4: : case 5: : case 6: : case 7: : case 8: : ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : break; : default: : break; : } : : param->header.ddr_size_type = ddr_size_type; : } no, we should not do this. The ddr_size_type should be a data directly from sdram_config.
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 64: set_ddr_size_type(param); Please move this to be a param of initialize_dramc_param.
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 34: static void set_ddr_size_type(struct dramc_param *param) : { : u32 ramcode = get_rom_code(); : u16 ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : : switch (ramcode) { : case 1: : case 2: : case 3: : case 4: : case 5: : case 6: : case 7: : case 8: : ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : break; : default: : break; : } : : param->header.ddr_size_type = ddr_size_type; : }
no, we should not do this. The ddr_size_type should be a data directly from sdram_config.
the sdram_config are uesless now, it only can be called at the partial k flow. current the full K and fast K do not use the sdram_config
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 64: set_ddr_size_type(param);
Please move this to be a param of initialize_dramc_param.
please more details.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 34: static void set_ddr_size_type(struct dramc_param *param) : { : u32 ramcode = get_rom_code(); : u16 ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : : switch (ramcode) { : case 1: : case 2: : case 3: : case 4: : case 5: : case 6: : case 7: : case 8: : ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : break; : default: : break; : } : : param->header.ddr_size_type = ddr_size_type; : }
the sdram_config are uesless now,
But your change just makes it important again.
My point is, RAMCODE -> DRAM CONFIG mapping should be a board-specific thing, and definitely not for code inside SOC.
Meanwhile, we're planning to change the mapping table to be model-specific, e.g., the same RAM code may be 4GB for model A and 6 GB for model B in future; so this information must come from some where, and sdram_config should be the right place.
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 64: set_ddr_size_type(param);
please more details.
I'd prefer this being change to
int initialize_dramc_param(void *blob, u32 ddr_geometry, u16 config) { ... hdr->ddr_geometry = ddr_geometry; }
Like said, all the ram code to geometry(config) translation should happen in board specific functions, not in the SOC side functions.
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41949/3/src/mainboard/google/kukui/... File src/mainboard/google/kukui/sdram_configs.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/mainboard/google/kukui/... PS3, Line 34: u32 get_rom_code(void) : { : return ram_code(); : }
I don't see the point of duplicating function here... […]
Done
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 34: static void set_ddr_size_type(struct dramc_param *param) : { : u32 ramcode = get_rom_code(); : u16 ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : : switch (ramcode) { : case 1: : case 2: : case 3: : case 4: : case 5: : case 6: : case 7: : case 8: : ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : break; : default: : break; : } : : param->header.ddr_size_type = ddr_size_type; : }
the sdram_config are uesless now, […]
Done
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 64: set_ddr_size_type(param);
I'd prefer this being change to […]
Done
Hello build bot (Jenkins), Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#4).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Current the DRAM control only support 4GB DDR, add more large size DDR support to meet the market requirement.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h M src/soc/mediatek/mt8183/memory.c 7 files changed, 225 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/4
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 41: param->header. hdr->
And please move this to the same order (e.g., after version, before config) as data in struct
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal What about changing this name to be dramc_get_calibrated_max_density() ?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... PS4, Line 151: const struct sdram_params *sdram_param = get_sdram_config(); this is not needed for fask-k. Please move this to before running full calibration.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41949/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/4//COMMIT_MSG@9 PS4, Line 9: Current the DRAM control only support 4GB DDR, add more large size DDR support : to meet the market requirement. : Currently the DRAM initialization code can only work on 4GB and we want to support larger memory in future by adding geometry information to the DRAM calibration parameters.
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41949/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/4//COMMIT_MSG@9 PS4, Line 9: Current the DRAM control only support 4GB DDR, add more large size DDR support : to meet the market requirement. :
Currently the DRAM initialization code can only work on 4GB and […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 41: param->header.
hdr-> […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
What about changing this name to be dramc_get_calibrated_max_density() ?
it also get the vendor_id for get the current vendor and print it out after calibration PASS .
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... PS4, Line 151: const struct sdram_params *sdram_param = get_sdram_config();
this is not needed for fask-k. Please move this to before running full calibration.
the point of sdram_param using at line-217 for fast K
Hello build bot (Jenkins), Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#5).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and we want to support large memory in future by adding geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h M src/soc/mediatek/mt8183/memory.c 7 files changed, 225 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/5
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
it also get the vendor_id for get the current vendor and print it out after calibration PASS .
It's confusing for a function that returns one information and have another information only in its side effect (print).
Please change these to two functions, say
dramc_get_calibrated_max_density() dramc_get_calibrated_vendor_id()
and you can do this in caller side:
dramc_show("Vendor id is %#x\n", dramc_get_calibrated_vendor_id()); density = dramc_get_calibrated_max_density(); ...
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... PS4, Line 151: const struct sdram_params *sdram_param = get_sdram_config();
the point of sdram_param using at line-217 for fast K
Sorry, what do you mean?
Fast-k returns early at line 175, and it does not need get_sdram_config().
Full-k starts at 191 and it needs get_sdram_config() (for geometry).
Partial-k starts at 217 and also needs whole sdram_param.
So I think we can move this to before line 192, so that fast-k won't need to waste time reading sdram_config.
Hello build bot (Jenkins), Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#6).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and we want to support large memory in future by adding geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h M src/soc/mediatek/mt8183/memory.c 7 files changed, 225 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... PS6, Line 2694: void get_dram_infor_after_cal(u8* density_result) "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... PS6, Line 110: void get_dram_infor_after_cal(u8* density_result); "foo* bar" should be "foo *bar"
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
It's confusing for a function that returns one information and have another information only in its […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/mem... PS4, Line 151: const struct sdram_params *sdram_param = get_sdram_config();
Sorry, what do you mean? […]
Done
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 6:
(13 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 134: u16 u8? The type of the argument 'value' of dramc_mode_reg_write_by_rank() is u8.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2699: & 0xff If we return u8 from dramc_mode_reg_write_by_rank(), then we don't have to "& 0xff" here.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2730: dram_size = 0; break
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27 Why is this 27?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: [LP4X_DDR1600] Could we preserve this?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: 101 This value won't be used anywhere.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 329: struct const struct
Or even make this static to simplify the initialization.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 331: { Could we write
[tRFCAB_130] = {...}
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg dramc_err
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break Do we want to return here, or assume rfcab_grp = 0 for the remaining of this function?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 393: rf_cab_opt[freq_group][rfcab_grp] Could we declare a pointer for this?
const struct optimize_ac_time *opt; // Or whatever name you prefer opt = &rf_cab_opt[freq_group][rfcab_grp]; trfc = opt->trfc; ...
Maybe we don't even need the "trfc"... local variables.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 57: u32 size; /* size of whole dramc_param */ Please either
1. Do not modify this line, or 2. Align all comments in struct dramc_param_header
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 68: DQS_NUMBER This file is starting to diverge from the one in vendor/mtk-dramk (https://chrome-internal-review.googlesource.com/c/chromeos/vendor/mtk-dramk/...). Would you consider renaming this to DQS_NUMBER_LP4, or doing the opposite?
Hello build bot (Jenkins), Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#7).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and we want to support large memory in future by adding geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h M src/soc/mediatek/mt8183/memory.c 7 files changed, 225 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/7
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... PS6, Line 2694: get_dram_infor_after_cal ok... you changed that to a pointer. not the type I preferred but fine if you still want to do that.
Anyway, can you rename this to
void dramc_get_calibrated_info(u8 *density_result)
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/emi... PS6, Line 591: K finished (current clock: %u K finished (current clock: %u, density: %u)\n"
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 67: ddr_geometry oops, I just realized we're duplicating ddr geometry here.
@huayang, it's really sorry but I wonder if we can revert the changes to a easier way, e.g.:
1. Don't put ddr_geometry in header. Keep dramc_param.h unchanged. 2. Add ddr_geometry in sdram_param (and leave it in emi.h)
So we have minimal changes in Coreboot side.
On the contrary, you can have some logic in dramk-k blob to read and trust the ddr geometry first from sdram_config.
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/emi.h:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... PS6, Line 11: struct dramc_param; : struct dramc_param_ops; No longer needed given you've included dramc_param.h
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 7:
(11 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 134: u16
u8? The type of the argument 'value' of dramc_mode_reg_write_by_rank() is u8.
the MR value read by in value = READ32_BITFIELD(&ch[chn].nao.mrr_status, MRR_STATUS_MRR_REG); in dramc_mode_reg_read(), the read length is 16, so return value is 16 bits too. but the driver use less 8bit only.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2699: & 0xff
If we return u8 from dramc_mode_reg_write_by_rank(), then we don't have to "& 0xff" here.
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2730: dram_size = 0;
break
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27
Why is this 27?
from the LPDDR4 standard SPEC of ‘JESD209-4B.pdf’, the MR8[5:2] is the density size. 0000B: 4Gb dual channel die 0001B: 6Gb dual channel die 0010B: 8Gb dual channel die 0011B: 12Gb dual channel die 0100B: 16Gb dual channel die 0101B: 24Gb dual channel die 0110B: 32Gb dual channel die
this is just print for human readable size
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 329: struct
const struct […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 331: {
Could we write […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg
dramc_err
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break
Do we want to return here, or assume rfcab_grp = 0 for the remaining of this function?
from the LPDDR4 standard SPEC of ‘JESD209-4B.pdf’, the MR8[5:2] is the density size, it only return 0-6 0000B: 4Gb dual channel die 0001B: 6Gb dual channel die 0010B: 8Gb dual channel die 0011B: 12Gb dual channel die 0100B: 16Gb dual channel die 0101B: 24Gb dual channel die 0110B: 32Gb dual channel die
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 393: rf_cab_opt[freq_group][rfcab_grp]
Could we declare a pointer for this? […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 57: u32 size; /* size of whole dramc_param */
Please either […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 68: DQS_NUMBER
This file is starting to diverge from the one in vendor/mtk-dramk (https://chrome-internal-review. […]
the MT8183 SOC also support LPDDR3 DDR, DQS_NUMBER of LP3 is 4 and DQS_NUMBER of the LP4X is 2. but chromepad only have LP4x DDR, so need rename all QS_NUMBER to DQS_NUMBER_LP4 in coreboot?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 134: u16
the MR value read by in value = READ32_BITFIELD(&ch[chn].nao. […]
Shouldn't the local variable 'value' be defined to be 'u16' instead of 'u8'? Then we can change the return type of this function to 'u8' and return 'value & 0xff' in this function.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
Done
I agree. Please split this function into two, and call them with
dramc_show("Vendor id is %#x\n", dramc_get_calibrated_vendor_id()); density = dramc_get_calibrated_max_density();
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27
from the LPDDR4 standard SPEC of ‘JESD209-4B.pdf’, the MR8[5:2] is the density size. […]
What's the unit of 'dram_size'? If it's in bytes, this should be 30 instead of 27.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg
Done
Forgot to change this?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break
from the LPDDR4 standard SPEC of ‘JESD209-4B. […]
If it's impossible to reach the 'default' case, let's change this to 'return'.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 393: rf_cab_opt[freq_group][rfcab_grp]
Done
Forgot to change this?
Hello build bot (Jenkins), Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#8).
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and we want to support large memory in future by adding geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/dramc_param.c M src/soc/mediatek/mt8183/dramc_pi_calibration_api.c M src/soc/mediatek/mt8183/emi.c M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h M src/soc/mediatek/mt8183/include/soc/emi.h M src/soc/mediatek/mt8183/memory.c 7 files changed, 230 insertions(+), 81 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/8
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@10 PS8, Line 10: in future by adding geometry information
… in future, so add geometry information …
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@10 PS8, Line 10: we Nit: Fits on line above.
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@12 PS8, Line 12: Mention that the header version is increased to 3, and that stuff from `emi.h` is moved.
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... PS8, Line 2694: infor Just *info*?
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... PS8, Line 2735: max_density = density; Please print a warning in this case.
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 56: u8 trfrc_pb_05t; Renaming the variables, makes this hard to review in Gerrit.
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 327: enum tRFCAB {tRFCAB_130 = 0, tRFCAB_180, tRFCAB_280, tRFCAB_380, tRFCAB_NUM}; Please put each assignment on a separate line.
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 393: dramc_err("density err!\n"); Please make the message more elaborate. Maybe also output the unsupported value.
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 568: u8 density = 0; The initialization is not needed?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 134: u16
Shouldn't the local variable 'value' be defined to be 'u16' instead of 'u8'? Then we can change the […]
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... PS8, Line 2735: max_density = density;
Please print a warning in this case.
It's just updating max_density, not an unexpected situation.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: 101
This value won't be used anywhere.
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg
Forgot to change this?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 393: rf_cab_opt[freq_group][rfcab_grp]
Forgot to change this?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 68: DQS_NUMBER
the MT8183 SOC also support LPDDR3 DDR, DQS_NUMBER of LP3 is 4 and DQS_NUMBER of the LP4X is 2. […]
If we follow Hung-Te's suggestion above and keep dramc_param.h unchanged, then we won't have to do it.
Hello build bot (Jenkins), Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#9).
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support large memory in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/include/soc/dramc_common_mt8183.h M src/soc/mediatek/mt8183/include/soc/dramc_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 3 files changed, 65 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/9
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 9:
(19 comments)
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@10 PS8, Line 10: we
Nit: Fits on line above.
Done
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@10 PS8, Line 10: in future by adding geometry information
… in future, so add geometry information …
Done
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@12 PS8, Line 12:
Mention that the header version is increased to 3, and that stuff from `emi.h` is moved.
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
I agree. Please split this function into two, and call them with […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27
What's the unit of 'dram_size'? If it's in bytes, this should be 30 instead of 27.
Done
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... PS6, Line 2694: get_dram_infor_after_cal
ok... you changed that to a pointer. not the type I preferred but fine if you still want to do that. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... PS8, Line 2694: infor
Just *info*?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: [LP4X_DDR1600]
Could we preserve this?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: [LP4X_DDR1600]
Could we preserve this?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break
If it's impossible to reach the 'default' case, let's change this to 'return'.
Done
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/emi... PS6, Line 591: K finished (current clock: %u
K finished (current clock: %u, density: %u)\n"
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 56: u8 trfrc_pb_05t;
Renaming the variables, makes this hard to review in Gerrit.
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 327: enum tRFCAB {tRFCAB_130 = 0, tRFCAB_180, tRFCAB_280, tRFCAB_380, tRFCAB_NUM};
Please put each assignment on a separate line.
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 393: dramc_err("density err!\n");
Please make the message more elaborate. Maybe also output the unsupported value.
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 568: u8 density = 0;
The initialization is not needed?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 67: ddr_geometry
oops, I just realized we're duplicating ddr geometry here. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 67: ddr_geometry
oops, I just realized we're duplicating ddr geometry here. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 68: DQS_NUMBER
If we follow Hung-Te's suggestion above and keep dramc_param. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/emi.h:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... PS6, Line 11: struct dramc_param; : struct dramc_param_ops;
No longer needed given you've included dramc_param. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 10: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41949/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/10//COMMIT_MSG@10 PS10, Line 10: large memory larger memory sizes
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 393: dramc_err("density err!\n");
Done
Forgot to change this?
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
PS10: Do we really need to modify this file? Could we move enum DRAMC_PARAM_GEOMETRY_TYPE to emi.h?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_common_mt8183.h:
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... PS10, Line 37: #define DQS_NUMBER_LP4 DQS_NUMBER : #define DQ_DATA_WIDTH_LP4 DQ_DATA_WIDTH Could we rename DQ_DATA_WIDTH and DQ_DATA_WIDTH directly? So that we don't have to define "aliases" for them.
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... PS10, Line 67: Please add this blank line back.
PS10:
Do we really need to modify this file? Could we move enum DRAMC_PARAM_GEOMETRY_TYPE to emi. […]
After discussing with Hung-Te. Now I understand what you're trying to do.
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... PS10, Line 78: cbt_ca_perbit_delay Why add this? Is this used anywhere?
Hello build bot (Jenkins), Paul Menzel, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#11).
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support larger memory sizes in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa 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_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 4 files changed, 19 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/11
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 12:
(5 comments)
https://review.coreboot.org/c/coreboot/+/41949/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/10//COMMIT_MSG@10 PS10, Line 10: large memory
larger memory sizes
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 393: dramc_err("density err!\n");
Forgot to change this?
Done
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_common_mt8183.h:
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... PS10, Line 37: #define DQS_NUMBER_LP4 DQS_NUMBER : #define DQ_DATA_WIDTH_LP4 DQ_DATA_WIDTH
Could we rename DQ_DATA_WIDTH and DQ_DATA_WIDTH directly? So that we don't have to define "aliases" […]
Done
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... PS10, Line 67:
Please add this blank line back.
Done
https://review.coreboot.org/c/coreboot/+/41949/10/src/soc/mediatek/mt8183/in... PS10, Line 78: cbt_ca_perbit_delay
Why add this? Is this used anywhere?
Done
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/12//COMMIT_MSG@10 PS12, Line 10: information Line too long. Please put this in the next line.
https://review.coreboot.org/c/coreboot/+/41949/12/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/emi.h:
https://review.coreboot.org/c/coreboot/+/41949/12/src/soc/mediatek/mt8183/in... PS12, Line 9: struct sdram_params { May I ask why you decided to NOT move this to dramc_param.h?
Hello build bot (Jenkins), Paul Menzel, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#13).
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support larger memory sizes in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa 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_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 4 files changed, 19 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/13
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41949/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/12//COMMIT_MSG@10 PS12, Line 10: information
Line too long. Please put this in the next line.
Done
https://review.coreboot.org/c/coreboot/+/41949/12//COMMIT_MSG@10 PS12, Line 10: information
Line too long. Please put this in the next line.
Done
https://review.coreboot.org/c/coreboot/+/41949/12/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/emi.h:
https://review.coreboot.org/c/coreboot/+/41949/12/src/soc/mediatek/mt8183/in... PS12, Line 9: struct sdram_params {
May I ask why you decided to NOT move this to dramc_param. […]
to less changes for MT8183 soc.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41949/14/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/14/src/soc/mediatek/mt8183/in... PS14, Line 6: #include <stdint.h> : #include <sys/types.h> : #include <soc/emi.h> : #include <soc/dramc_common_mt8183.h> Could we sort these lexicographically?
https://review.coreboot.org/c/coreboot/+/41949/14/src/soc/mediatek/mt8183/in... PS14, Line 11: enum DRAMC_PARAM_SOURCE { : DRAMC_PARAM_SOURCE_SDRAM_INVALID = 0, : DRAMC_PARAM_SOURCE_SDRAM_CONFIG, : DRAMC_PARAM_SOURCE_FLASH, : }; Why move this here? Since this file already includes emi.h, could we keep this in emi.h?
https://review.coreboot.org/c/coreboot/+/41949/12/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/emi.h:
https://review.coreboot.org/c/coreboot/+/41949/12/src/soc/mediatek/mt8183/in... PS12, Line 9: struct sdram_params {
to less changes for MT8183 soc.
Ack
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/14/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/14/src/soc/mediatek/mt8183/in... PS14, Line 6: #include <stdint.h> : #include <sys/types.h> : #include <soc/emi.h> : #include <soc/dramc_common_mt8183.h>
Could we sort these lexicographically?
Done
https://review.coreboot.org/c/coreboot/+/41949/14/src/soc/mediatek/mt8183/in... PS14, Line 11: enum DRAMC_PARAM_SOURCE { : DRAMC_PARAM_SOURCE_SDRAM_INVALID = 0, : DRAMC_PARAM_SOURCE_SDRAM_CONFIG, : DRAMC_PARAM_SOURCE_FLASH, : };
Why move this here? Since this file already includes emi.h, could we keep this in emi. […]
Done
Hello build bot (Jenkins), Paul Menzel, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#16).
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support larger memory sizes in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa 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_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 4 files changed, 14 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/16
Hello build bot (Jenkins), Paul Menzel, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#17).
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support larger memory sizes in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa 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_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 4 files changed, 14 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/17
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 17: Code-Review+1
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 17: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 17: Code-Review+2
Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Angel Pons, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41949
to look at the new patch set (#18).
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support larger memory sizes in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa 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_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 4 files changed, 14 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41949/18
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup
Currently the DRAM initialization code can only work on 4GB size and want to support larger memory sizes in future, so add geometry information to the DRAM calibration parameters.
BUG=none BRANCH=kukui TEST=Boots correctly on Kukui
Change-Id: I1fdf50b75c6a552c0a889f21e1a81ab4b9a305fa Signed-off-by: Huayang Duan huayang.duan@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41949 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Yu-Ping Wu yupingso@google.com Reviewed-by: Hung-Te Lin hungte@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.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_param.h M src/soc/mediatek/mt8183/include/soc/emi.h 4 files changed, 14 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Hung-Te Lin: Looks good to me, approved Yu-Ping Wu: Looks good to me, but someone else must approve
diff --git a/src/soc/mediatek/mt8183/dramc_init_setting.c b/src/soc/mediatek/mt8183/dramc_init_setting.c index 9ae0aae..794fad1 100644 --- a/src/soc/mediatek/mt8183/dramc_init_setting.c +++ b/src/soc/mediatek/mt8183/dramc_init_setting.c @@ -5,6 +5,7 @@ #include <delay.h> #include <soc/emi.h> #include <soc/dramc_pi_api.h> +#include <soc/dramc_param.h> #include <soc/dramc_register.h> #include <soc/infracfg.h> #include <string.h> diff --git a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c index 50d847f..fce7c9f 100644 --- a/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c +++ b/src/soc/mediatek/mt8183/dramc_pi_calibration_api.c @@ -6,6 +6,7 @@ #include <device/mmio.h> #include <soc/emi.h> #include <soc/dramc_register.h> +#include <soc/dramc_param.h> #include <soc/dramc_pi_api.h> #include <timer.h>
diff --git a/src/soc/mediatek/mt8183/include/soc/dramc_param.h b/src/soc/mediatek/mt8183/include/soc/dramc_param.h index a883fe7..e35e4f5 100644 --- a/src/soc/mediatek/mt8183/include/soc/dramc_param.h +++ b/src/soc/mediatek/mt8183/include/soc/dramc_param.h @@ -3,14 +3,14 @@ #ifndef SOC_MEDIATEK_MT8183_DRAMC_PARAM_H #define SOC_MEDIATEK_MT8183_DRAMC_PARAM_H
+#include <soc/dramc_common_mt8183.h> +#include <soc/emi.h> #include <stdint.h> #include <sys/types.h>
-#include "emi.h" - enum { DRAMC_PARAM_HEADER_MAGIC = 0x44524d4b, - DRAMC_PARAM_HEADER_VERSION = 2, + DRAMC_PARAM_HEADER_VERSION = 3, };
enum DRAMC_PARAM_STATUS_CODES { @@ -37,10 +37,16 @@ DRAMC_FLAG_HAS_SAVED_DATA = 0x0001, };
+enum DRAMC_PARAM_GEOMETRY_TYPE { + DDR_TYPE_2CH_2RK_4GB_2_2, + DDR_TYPE_2CH_2RK_6GB_3_3, + DDR_TYPE_2CH_2RK_8GB_4_4, +}; + struct dramc_param_header { u32 status; /* DRAMC_PARAM_STATUS_CODES */ u32 magic; - u32 version; + u32 version; /* DRAMC_PARAM_HEADER_VERSION */ u32 size; /* size of whole dramc_param */ u16 config; /* DRAMC_PARAM_CONFIG */ u16 flags; /* DRAMC_PARAM_FLAGS */ diff --git a/src/soc/mediatek/mt8183/include/soc/emi.h b/src/soc/mediatek/mt8183/include/soc/emi.h index b2d78b9..cf794de 100644 --- a/src/soc/mediatek/mt8183/include/soc/emi.h +++ b/src/soc/mediatek/mt8183/include/soc/emi.h @@ -13,8 +13,9 @@ };
struct sdram_params { - u16 source; /* DRAMC_PARAM_SOURCE */ + u16 source; /* DRAMC_PARAM_SOURCE */ u16 frequency; + u32 ddr_geometry; /* DRAMC_PARAM_GEOMETRY_TYPE */ u8 wr_level[CHANNEL_MAX][RANK_MAX][DQS_NUMBER];
/* DUTY */
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Duan Huayang (Code Review)
-
Duan huayang (Code Review)
-
Hung-Te Lin (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)