Attention is currently required from: Paul Menzel, Angel Pons, CK HU, Yu-Ping Wu, Yidi Lin. Xi Chen 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 56:
(42 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/44716/comment/9da689ba_f63cd5d9 PS46, Line 8:
- Please describe the implementation, and mention the datasheet name and revision. […]
1 Add CBT description. need more information? 2 CBT is tested by HW simulation and waveform measurement.
File src/soc/mediatek/mt8192/dramc_dvfs.c:
https://review.coreboot.org/c/coreboot/+/44716/comment/fdaa31c5_c51e78aa PS42, Line 196: switched
is switched
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/27c040bf_a501bb03 PS42, Line 196: Double confirm
"Reconfirm" or just "Confirm"
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/f124925e_83c52900 PS42, Line 199: u8 mr13 = mr_value->mr13[rk]; : mr13 |= (BIT(6) | BIT(7)); : mr_value->mr13[rk] = mr13;
Just […]
Ack
File src/soc/mediatek/mt8192/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/44716/comment/ee21b841_0b57921a PS42, Line 122: return ((range & 0x1) << 6) | (vref_lev & 0x3f);
Move the if statement below and return vref_org.
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/cf37373b_2ff1cc2d PS42, Line 130: === vref_new: 0x%x --> 0x%x\
vref: %#x --> %#x
Ack
File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44716/comment/6865e329_7d3aa271 PS42, Line 16: }
comma
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/43bd2bab_b9dd8597 PS42, Line 595: u8
const u8
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/5217efa8_146adaeb PS42, Line 595: u8
Align with "const struct ddr_cali"
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/e8e2df5b_d2f3e7ac PS42, Line 602: d
u
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/c9ed8b32_c1348dcd PS42, Line 602: Per
Is it Per or Pre?
per bit, which means CA per bit delay line
https://review.coreboot.org/c/coreboot/+/44716/comment/739365cf_d471b88e PS42, Line 603: ca_mapping
Align
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/7fb052d9_ebc88107 PS42, Line 624: put
How about "set"?
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/eb53b070_6d8b9150 PS42, Line 634: 0xF
SHU_SELPH_CA5_DLY_CKE has only 2 bits. Should we use 0x3 here? Same below.
only 3 bits, use 0x7.
https://review.coreboot.org/c/coreboot/+/44716/comment/bb5bed74_57c4bddd PS42, Line 638: put
How about "set"?
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/a5e3b2c6_3f73ce6d PS42, Line 666: capi
What does "capi" mean here? Is it the same as "ca_pi" in get_ca_pi_per_ui?
ca pi per clock: 64(freq > 400), 32(freq == 400) per clock == 2UI
change to: get_ca_pi_per_clock
https://review.coreboot.org/c/coreboot/+/44716/comment/bc379c68_8fcbcad9 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? […]
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/57fb095b_3ab0175d PS42, Line 684: ratio
Is the value guaranteed to fit in u8? What are all possible values?
The possible values of ratio is 0 or 1, changed to u8.
https://review.coreboot.org/c/coreboot/+/44716/comment/24fe0c8f_c0e97611 PS42, Line 687: cbtui
cbt_ui?
yes, cbt_ui, change to get_cbt_ui_limit
https://review.coreboot.org/c/coreboot/+/44716/comment/5c5dc4b3_cd433fb6 PS42, Line 709: get_ca_mck
Move this function before put_ca_mck(). Let's order these functions like this: […]
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/ae71b33e_613f8bbe PS42, Line 716: u32
Align
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/07b519f5_6e882375 PS42, Line 726: i++
How about "i = 0; i < 28; i += 4" and then we don't have to write "i * 4" below?
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/0c4e7158_4976273a PS42, Line 738: if (ui_new)
No need to do null check. Just initialize *ui_new to 0 and add values to it. […]
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/284efe01_e8ed2345 PS42, Line 748: p2u
Why isn't this u8?
remove it, use macro.
https://review.coreboot.org/c/coreboot/+/44716/comment/332e914e_cdfc3ee6 PS42, Line 748: 0, pi = 0
No need to initialize.
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/9237dad1_8a086d69 PS42, Line 749: tmp
I'd call it "new".
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/c7ffda00_ca797958 PS42, Line 754: d
u
Done, also change s16 pi_dly to u8 pi_dly.
https://review.coreboot.org/c/coreboot/+/44716/comment/3c9dec75_f832ef5b PS42, Line 754: :
One space after ":"
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/6674deb9_43390903 PS42, Line 791: u32 ui_new = 0, mck_new = 0;
No need to initialize.
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/994a7c99_e9aca491 PS42, Line 805: :
Space after ":"
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/a5f6cac9_12e0304e PS42, Line 827: final_ca_clk
Use cmd_dly here.
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/aa986b52_3ea7cf96 PS42, Line 827: CA Dly
CA Dly (original)
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/3760d5bb_0654d506 PS42, Line 829: final_ca_clk
cmd_dly
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/8466e8e8_c551f533 PS42, Line 830: after adjust, CA Dly
CA Dly (adjusted)
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/b3faa0fb_d98010cc PS42, Line 840: delay
If "dly" means delay, use "dly" for consistency. […]
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/d7c4d138_b6405b95 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 […]
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/42da1cde_46c662aa PS42, Line 864: d
u
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/d73f2aee_d5e20bf4 PS42, Line 875: (
No need for parentheses.
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/c1497b95_920bbf1f PS42, Line 883:
Extra space
Ack
https://review.coreboot.org/c/coreboot/+/44716/comment/c7a009ad_62645c0d PS42, Line 900:
const
Ack
File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44716/comment/248e218d_03e445d8 PS44, Line 597: prebit
`perbit`?
yes, has fixed to perbit.
File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44716/comment/5beb9c3e_d328e260 PS46, Line 1278:
Please remove the blank line.
Ack