Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44713 )
Change subject: soc/mediatek/mt8192: Add dramc ac timing setting ......................................................................
Patch Set 41:
(27 comments)
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4037: dram_freq_grp const
Actually this is only used twice. Maybe we don't need it.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4039: u8 rank_inctl = 0, tx_dly = 0, datlat_dsel = 0; : u8 rodt_tracking_mck = 0, root = 0, tx_rank_inctl = 0; : u8 tref_bw = 0, tfaw_05t = 0, trrd_05t = 0; : u16 xrtwtw = 0, xtrtrt = 0, xrtw2r = 0, xrtr2w = 0, tfaw = 0; : u16 trtw = 0, trtw_05t = 0, tmrr2w = 0, trrd = 0; : u16 phs_inctl = 0; : u32 rank_inctl_root; Most of them don't need an initialized value. For others such as 'root', please declare with a 'const' modifier.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4048: ( No need for the paratheses.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4051: d u
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4051: match Found matched
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4056: dbg error
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4056: , no match AC timing table : no matched AC timing table found
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4059: memcpy Can we use a pointer instead?
const struct ac_timing *ac_tim; ... ac_tim = &ac_timing_tbl[table_idx];
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4072: rodt_tracking_mck This is always 0. Why do we need this?
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4077: SHU_MISC_RX_PIPE_CTRL_RX_PIPE_BYPASS_EN Please align with &ch[0].
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4080: ac_tim.datlat What if this is 0?
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4085: info err or warn?
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 11: u8 rf_group = 0, cab_id = 0; No need for initialization.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 15: t Remove "t"
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 19: M Add a trailing ","
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 21: enum Since it's a one-to-one mapping, can we use dram_freq_grp directly?
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 29: M Add a trailing ","
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 39: u8 const
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 40: dram_freq_grp const
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 42: ptRFCab_Opt Lowercase
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 43: tRFCab_Opt Lowercase
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 171: size_t u8
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 173: SET32_BITFIELDS(&ch[chn].ao.shu_ac_time_05t, SHU_AC_TIME_05T_TRFC_05T, trfc_05t);
line over 96 characters
Please fix this
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 176: SET32_BITFIELDS(&ch[chn].ao.shu_ac_time_05t, SHU_AC_TIME_05T_TRFCPB_05T, trfrc_pb05t);
line over 96 characters
Please fix this
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 177: d u
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/dramc_ac_timing.h:
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/in... PS41, Line 76: T t
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/in... PS41, Line 970: }; One blank line before endif