Xi Chen 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 48:
(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 […]
Add const modifier, and i'd prefer to keep it for shorter name, ok?
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4048: (
No need for the paratheses.
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4051: match
Found matched
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4051: d
u
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4056: dbg
error
Ack
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
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4059: memcpy
Can we use a pointer instead? […]
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4072: rodt_tracking_mck
This is always 0. […]
remove it now.
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].
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4080: ac_tim.datlat
What if this is 0?
update codes: if ac_tim.datlat == 1, datlat_dsel = 0.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4085: info
err or warn?
Ack
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.
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 15: t
Remove "t"
jesd209-4 spec term tRFCAB, so change to TRFCAB_?
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 19: M
Add a trailing ","
Ack
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?
Actually, we may use actiming 1800 mapping to 1600 freq, so define the enum.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 29: M
Add a trailing ","
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 39: u8
const
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 40: dram_freq_grp
const
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 42: ptRFCab_Opt
Lowercase
Follow jesd spec, we'd prefer to use this style for clear name. tRFCab
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 43: tRFCab_Opt
Lowercase
Follow jesd spec, we'd prefer to use this style for clear name.
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 171: size_t
u8
Ack
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);
Please fix this
Ack
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);
Please fix this
Done
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 177: d
u
Ack
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
"T" is an standard clock interval, we'd prefer to keep the upper case for more clear. ok?
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/in... PS41, Line 970: };
One blank line before endif
Ack