Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44716 )
Change subject: soc/mediatek/mt8192: Do dramc command bus training ......................................................................
Patch Set 44:
(53 comments)
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_dvfs.c:
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 196: switched is switched
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 196: Double confirm "Reconfirm" or just "Confirm"
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 199: u8 mr13 = mr_value->mr13[rk]; : mr13 |= (BIT(6) | BIT(7)); : mr_value->mr13[rk] = mr13; Just
mr_value->mr13[rk] |= BIT(6) | BIT(7);
And remove {}.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 122: return ((range & 0x1) << 6) | (vref_lev & 0x3f); Move the if statement below and return vref_org.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 126: 8 Why is this 8?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 130: === vref_new: 0x%x --> 0x%x\ vref: %#x --> %#x
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 16: } comma
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 595: u8 const u8
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 595: u8 Align with "const struct ddr_cali"
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 602: Per Is it Per or Pre?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 602: d u
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 603: ca_mapping Align
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 624: put How about "set"?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 629: 0xF0000000U Please define constants for these 2 numbers: 0xF0000000U and 0x0FFFFFFFU.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 634: 0xF SHU_SELPH_CA5_DLY_CKE has only 2 bits. Should we use 0x3 here? Same below.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 638: put How about "set"?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 666: capi What does "capi" mean here? Is it the same as "ca_pi" in get_ca_pi_per_ui?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 674: static u8 get_ca_pi_per_ui(void) It's kind of weird for a function to return a constant. Why no use a macro?
#define CA_PI_PER_UI 32
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 684: ratio Is the value guaranteed to fit in u8? What are all possible values?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 687: cbtui cbt_ui?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 709: get_ca_mck Move this function before put_ca_mck(). Let's order these functions like this:
get_x(); set_x(); get_y(); set_y();
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 716: u32 Align
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 726: i++ How about "i = 0; i < 28; i += 4" and then we don't have to write "i * 4" below?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 738: if (ui_new) No need to do null check. Just initialize *ui_new to 0 and add values to it.
Same for mck_new.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 748: 0, pi = 0 No need to initialize.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 748: p2u Why isn't this u8?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 749: tmp I'd call it "new".
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 754: : One space after ":"
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 754: d u
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 791: u32 ui_new = 0, mck_new = 0; No need to initialize.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 805: : Space after ":"
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 827: CA Dly CA Dly (original)
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 827: final_ca_clk Use cmd_dly here.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 829: final_ca_clk cmd_dly
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 830: after adjust, CA Dly CA Dly (adjusted)
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 840: delay If "dly" means delay, use "dly" for consistency. Maybe final_cs_dly?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 847: cs_dly[RANK_0] = params->cbt_cs_dly[chn][RANK_0]; : cs_dly[RANK_1] = params->cbt_cs_dly[chn][RANK_1]; Simply
const u8 *cs_dly = cali->params->cbt_cs_dly[chn];
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 864: d u
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 875: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 883: Extra space
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 889: mr13 |= BIT(6); Do we need BIT(7) if (fsp != FSP_1)?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 900: const
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 944: if (state_do_term[chn] == do_term) So nothing will be done if do_term is 0?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 958: ( No parantheses
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 959: mr13 |= BIT(6); Why clear BIT(6) and then set BIT(6)?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1018: ( No parentheses.
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1095: range = mr12 >> 6; If we define
range = mr12 & BIT(6);
then we can simplify "(range & 0x1) << 6" with "range" both below and in get_cbt_vref_pinmux_value().
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1112: (0x1 << 6) BIT(6)?
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1117: d u
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1133: Remove blank line
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1212: u8 ca_final_center[CA_NUM_LP4] = {0}; const
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1237: size_t int
https://review.coreboot.org/c/coreboot/+/44716/42/src/soc/mediatek/mt8192/dr... PS42, Line 1276: size_t int